default values are being overwritten in upsert #616
Conversation
| @@ -415,7 +415,9 @@ DataAccessObject.updateOrCreate = DataAccessObject.upsert = function upsert(data | |||
| var update = data; | |||
| var inst = data; | |||
| if(!(data instanceof Model)) { | |||
| console.log('not instance', data); | |||
| inst = new Model(data); | |||
There was a problem hiding this comment.
Maybe the constructor should have an option to skip default value generation in this case.
There was a problem hiding this comment.
Ok, I'll look into adding another option here: https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/model.js#L42
With that value turned on we skip the default value and defaultFn generation blocks.
The memory connector was returning incorrect values such that my tests were working with the proposed changes but failing because of the memory connector issue. In this test I'm trying to compare a Date object to what was being returned as a date represented by a string. Again, I can pull the memory connector issue out into a separate PR because it should be fixed and is separate from the issue related to this PR but was in the way of fixing it. |
@clarkbw What's your take on the suggestion above? |
Just ran it through the tests, its the right call. Likely fixes an issue in the |
|
Need to finish with more tests specifically for the |
|
Changed the option name to |
|
Did another pass over this change and I don't think there's a need for more individual tests on the @raymondfeng I think this is ready for your review |
| callback(err, data, { isNewInstance: !modelData }); | ||
| }); | ||
| callback(err, this.fromDb(model, data), { isNewInstance: !modelData }); | ||
| }.bind(this)); |
There was a problem hiding this comment.
Can we use self instead of bind? bind is expensive.
There was a problem hiding this comment.
Good point, sorry. Fixed.
Changes the after `save` callback in the memory connector to use the `fromDb` method to deserialize the data passed back to upsert and updateAttributes methods.
Creates a new applyDefaultValues option on the Model constructor
defaulting to true, the current behaviour.
Updates the dao module to pass `{ applyDefaultValues: false }` to the
Model constructor during the updateOrCreate method when we assume an
update is happening.
|
@raymondfeng I updated the memory connector as you requested to use |
default values are being overwritten in upsert
clarkbw/loopback-ds-timestamp-mixin#11 was filed and it appears to be an issue with the upsert coming from juggler.
This PR creates a test that fails and I believe I’ve tracked down the area of the code that causes the issue, wrapped in
console.logstatements currently.When you run the test it will print the following information:
As you can see in the debug output the
createdAtfield is being generated and then overwriting the existingcreatedAtfield. We need to prevent the default fields from being generated or passed to the update because default functions that are not idempotent will always be storing new values.I’m investigating a solution, any comments are appreciated.