Skip to content

Commit

Permalink
Do not apply default values on data from database
Browse files Browse the repository at this point in the history
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 introducing a new model-level setting called
`applyDefaultsOnReads`, which is enabled by default for backwards
compatibility.

When this setting is set to `false`, operations like `find` and
`findOrCreate` will NOT apply default property values on data returned
by the database (connector).

Please note that most of the other CRUD methods did not apply default
values on database data as long as the connector provided native
implementation of the operation, that aspect is not changing.
  • Loading branch information
bajtos committed Apr 4, 2019
1 parent 2f093ce commit be1c365
Show file tree
Hide file tree
Showing 3 changed files with 143 additions and 2 deletions.
14 changes: 12 additions & 2 deletions lib/dao.js
Original file line number Diff line number Diff line change
Expand Up @@ -1109,8 +1109,15 @@ 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,
};
if (Model.settings.applyDefaultsOnReads === false) {
ctorOpts.applyDefaultValues = false;
}
obj = new Model(data, ctorOpts);
}

if (created) {
Expand Down Expand Up @@ -1632,6 +1639,9 @@ DataAccessObject.find = function find(query, options, cb) {
applySetters: false,
persisted: true,
};
if (Model.settings.applyDefaultsOnReads === false) {
ctorOpts.applyDefaultValues = false;
}
let obj;
try {
obj = new Model(data, ctorOpts);
Expand Down
65 changes: 65 additions & 0 deletions test/basic-querying.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -917,6 +917,71 @@ describe('basic-querying', function() {
});
});
});

it('applies default values by default', () => {
// Backwards compatibility, see
// 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});
let created;
return db.automigrate('Product')
.then(() => Product.create({name: 'Pen'}))
.then(result => {
created = result;

// Later on, we decide to introduce `active` property
Product.defineProperty('active', {
type: Boolean,
default: false,
});

// And query existing data
return Product.findOne();
})
.then(found => {
found.toObject().should.eql({
id: created.id,
name: 'Pen',
// Backwards-compatibility
// When Pen does not have "active" flag set, we change it to default
active: false,
});
});
});

it('preserves empty values from the database when "applyDefaultsOnReads" is false', () => {
// 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},
{applyDefaultsOnReads: false}
);
let created;
return db.automigrate('Product')
.then(() => Product.create({name: 'Pen'}))
.then(result => {
created = result;

// Later on, we decide to introduce `active` property
Product.defineProperty('active', {
type: Boolean,
default: false,
});

// And query existing data
return Product.findOne();
})
.then(found => {
found.toObject().should.eql({
id: created.id,
name: 'Pen',
active: undefined,
});
});
});
});

describe('count', function() {
Expand Down
66 changes: 66 additions & 0 deletions test/manipulation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1619,6 +1619,72 @@ describe('manipulation', function() {
})
.catch(done);
});

it('applies default values on returned data', () => {
// Backwards compatibility, see
// 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});
let created;
return db.automigrate('Product')
.then(() => Product.create({name: 'Pen'}))
.then(result => {
created = result;

// Later on, we decide to introduce `active` property
Product.defineProperty('active', {
type: Boolean,
default: false,
});

// and findOrCreate an existing record
return Product.findOrCreate({id: created.id}, {name: 'updated'});
})
.spread(found => {
found.toObject().should.eql({
id: created.id,
name: 'Pen',
// Backwards-compatibility
// When Pen does not have "active" flag set, we change it to default
active: false,
});
});
});

it('preserves empty values from the database when "applyDefaultsOnReads" is false', () => {
// 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},
{applyDefaultsOnReads: false}
);
let created;

return db.automigrate('Product')
.then(() => Product.create({name: 'Pen'}))
.then(result => {
created = result;

// Later on, we decide to introduce `active` property
Product.defineProperty('active', {
type: Boolean,
default: false,
});

// And findOrCreate an existing record
return Product.findOrCreate({id: created.id}, {name: 'updated'});
})
.spread(found => {
found.toObject().should.eql({
id: created.id,
name: 'Pen',
active: undefined,
});
});
});
});

describe('destroy', function() {
Expand Down

0 comments on commit be1c365

Please sign in to comment.