From e45292de081813282d5dee8c180d8713b474d25f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Mon, 1 Apr 2019 13:18:05 +0200 Subject: [PATCH] Do not apply default values on data from database Before this change, when a property was configured with a default value at LoopBack side and the database was returned a record with a missing value for such property, then we would supply use the configured default. This behavior is problematic for reasons explained in #1692. In this commit, we are fixing DAO operations like `find` and `findOrCreate` so that they do NOT apply default property values on data returned by the database (connector). Please note that most of the other CRUD methods were already not applying default values on database data as long as the connector provided native implementation of the operation. --- lib/dao.js | 12 +++++-- test/basic-querying.test.js | 24 +++++++++++++ test/defaults.test.js | 11 +++--- test/manipulation.test.js | 72 +++++++++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 8 deletions(-) diff --git a/lib/dao.js b/lib/dao.js index 9289d64c..24bd6682 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -1109,8 +1109,14 @@ DataAccessObject.findOrCreate = function findOrCreate(query, data, options, cb) let obj; if (data) { - obj = new Model(data, {fields: query.fields, applySetters: false, - persisted: true}); + const ctorOpts = { + fields: query.fields, + applySetters: false, + persisted: true, + // see https://github.com/strongloop/loopback-datasource-juggler/issues/1692 + applyDefaultValues: false, + }; + obj = new Model(data, ctorOpts); } if (created) { @@ -1631,6 +1637,8 @@ DataAccessObject.find = function find(query, options, cb) { fields: query.fields, applySetters: false, persisted: true, + // see https://github.com/strongloop/loopback-datasource-juggler/issues/1692 + applyDefaultValues: false, }; let obj; try { diff --git a/test/basic-querying.test.js b/test/basic-querying.test.js index 2610689c..90fd7735 100644 --- a/test/basic-querying.test.js +++ b/test/basic-querying.test.js @@ -922,6 +922,30 @@ describe('basic-querying', function() { }); }); }); + + it('preserves empty values from the database', async () => { + // https://github.com/strongloop/loopback-datasource-juggler/issues/1692 + + // Initially, all Products were always active, no property was needed + const Product = db.define('Product', {name: String}); + + await db.automigrate('Product'); + const created = await Product.create({name: 'Pen'}); + + // Later on, we decide to introduce `active` property + Product.defineProperty('active', { + type: Boolean, + default: false, + }); + + // And query existing data + const found = await Product.findOne(); + found.toObject().should.eql({ + id: created.id, + name: 'Pen', + active: undefined, + }); + }); }); describe('count', function() { diff --git a/test/defaults.test.js b/test/defaults.test.js index c0503c71..f31dcb69 100644 --- a/test/defaults.test.js +++ b/test/defaults.test.js @@ -34,13 +34,13 @@ describe('defaults', function() { }); }); - it('should apply defaults on read', function(done) { + it('should NOT apply defaults on read', function(done) { db.defineProperty('Server', 'host', { type: String, default: 'localhost', }); Server.all(function(err, servers) { - (new String('localhost')).should.equal(servers[0].host); + should(servers[0].host).be.undefined(); done(); }); }); @@ -49,10 +49,9 @@ describe('defaults', function() { Server.create({host: 'localhost', port: 8080}, function(err, s) { should.not.exist(err); s.port.should.equal(8080); - Server.find({fields: ['host']}, function(err, servers) { - servers[0].host.should.equal('localhost'); - servers[0].should.have.property('host'); - servers[0].should.have.property('port', undefined); + Server.findById(s.id, {fields: ['host']}, function(err, server) { + server.should.have.property('host', 'localhost'); + server.should.have.property('port', undefined); done(); }); }); diff --git a/test/manipulation.test.js b/test/manipulation.test.js index 9c939c0d..679e5be8 100644 --- a/test/manipulation.test.js +++ b/test/manipulation.test.js @@ -981,6 +981,30 @@ describe('manipulation', function() { } ); }); + + it('preserves empty values from the database', async () => { + // https://github.com/strongloop/loopback-datasource-juggler/issues/1692 + + // Initially, all Products were always active, no property was needed + const Product = db.define('Product', {name: String}); + + await db.automigrate('Product'); + const created = await Product.create({name: 'Pen'}); + + // Later on, we decide to introduce `active` property + Product.defineProperty('active', { + type: Boolean, + default: false, + }); + + // And updateOrCreate an existing record + const found = await Product.updateOrCreate({id: created.id, name: 'updated'}); + found.toObject().should.eql({ + id: created.id, + name: 'updated', + active: undefined, + }); + }); }); bdd.describeIf(connectorCapabilities.supportForceId !== false, @@ -1619,6 +1643,30 @@ describe('manipulation', function() { }) .catch(done); }); + + it('preserves empty values from the database', async () => { + // https://github.com/strongloop/loopback-datasource-juggler/issues/1692 + + // Initially, all Products were always active, no property was needed + const Product = db.define('Product', {name: String}); + + await db.automigrate('Product'); + const created = await Product.create({name: 'Pen'}); + + // Later on, we decide to introduce `active` property + Product.defineProperty('active', { + type: Boolean, + default: false, + }); + + // And findOrCreate an existing record + const [found] = await Product.findOrCreate({id: created.id}, {name: 'updated'}); + found.toObject().should.eql({ + id: created.id, + name: 'Pen', + active: undefined, + }); + }); }); describe('destroy', function() { @@ -2563,6 +2611,30 @@ describe('manipulation', function() { }); }); }); + + it('preserves empty values from the database', async () => { + // https://github.com/strongloop/loopback-datasource-juggler/issues/1692 + + // Initially, all Products were always active, no property was needed + const Product = db.define('Product', {name: String}); + + await db.automigrate('Product'); + const created = await Product.create({name: 'Pen'}); + + // Later on, we decide to introduce `active` property + Product.defineProperty('active', { + type: Boolean, + default: false, + }); + + // And upsertWithWhere an existing record + const found = await Product.upsertWithWhere({id: created.id}, {name: 'updated'}); + found.toObject().should.eql({ + id: created.id, + name: 'updated', + active: undefined, + }); + }); }); });