Do not apply default values on data from database [2.x]#1706
Merged
Conversation
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. Also note that default values are applied only on properties with `undefined` values. The value `null` does not trigger application of default values. This is important because SQL connectors return `null` for properties with no value set.
3 tasks
jannyHou
approved these changes
Apr 9, 2019
| // And findOrCreate an existing record | ||
| return Player.findOrCreate({id: created.id}, {name: 'updated'}); | ||
| }) | ||
| .then(function(result) { |
Contributor
There was a problem hiding this comment.
nitpick: I like the spread() function used in the original PR #1705, any reason switched back to then()?
Not a blocker for merging, just curious.
Member
Author
There was a problem hiding this comment.
In LB 3.x, we are using Bluebird as the promise library, therefore .spread() is available on all Promise instances.
In LB 2.x, we use the native Promise library if it's provided (Node.js 4.0 an higher), and require users to set global.Promise to any compatible Promise implementation on Node.js versions 0.10 & 0.12. The native Promise does not provide .spread() API.
Contributor
There was a problem hiding this comment.
oh I see. makes sense thanks!.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 likefindandfindOrCreatewill 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.
Also note that default values are applied only on properties with
undefinedvalues. The valuenulldoes not trigger application of default values. This is important because SQL connectors returnnullfor properties with no value set.Related issues
This is a back-port of #1705, which is a backport of #1704. Compared to #1704 (master/4.x), we are preserving backwards compatibility by introducing a new feature flag.
See also #1692.
/cc @sharathmuthu6 @snarjuna
Checklist
guide