New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calling set with dot.separated key on a JSON/JSONB attribute will not flag the entire object as changed #4379

Merged
merged 1 commit into from Sep 18, 2015

Conversation

3 participants
@pixel2
Contributor

pixel2 commented Aug 24, 2015

I'm using a JSONB attribute but I'm not able to save the data using set() on my instance object. When reading the documentation is says that using a dot.separated key should change the instance and flag the entire object as changed. If I inspect the instance object I can see that the "_changed" property now contains my dot.separated keys not the entire object key as expected.

Looking at the source code for instance.js I found a problem in the way changed is called when using a det.separated key. I have attached a pull request that actually flag the entire object as changed and I also make sure only to flag if a property has changed.

Dottie.set(this.dataValues, key, value);
this.changed(key, true);
var previousDottieValue = Dottie.get(this.dataValues, key);
if (!_.isEqual(previousDottieValue, value)) {

This comment has been minimized.

@mickhansen

mickhansen Aug 24, 2015

Contributor

Why _.isEqual?
We simply use !== for the other logic.

This comment has been minimized.

@pixel2

pixel2 Aug 24, 2015

Contributor

I wanted to make sure to do a deep comparison if you pass a object as value, but maybe that is not a valid use case. I guess you could always use a multiple dot.separated key instead. If you prefer I can update the pull request to a !=== comparison?

This comment has been minimized.

@mickhansen

mickhansen Aug 24, 2015

Contributor

Hmm, i'm not sure, the overhead is likely a function call and two type calls (without looking at the lodash code), likely negligible, thoughts @janmeier?

This comment has been minimized.

@pixel2

pixel2 Aug 28, 2015

Contributor

Should I update the pull request with !== comparison? That way it is following the convention used in the other logic?

This comment has been minimized.

@pixel2

pixel2 Sep 17, 2015

Contributor

Any thoughts on this @mickhansen @janmeier ? I'm happy either way, so if you feel more comfortable with !== comparison I will change it.

This comment has been minimized.

@janmeier

janmeier Sep 17, 2015

Member

i vote for isEqual

This comment has been minimized.

@janmeier

janmeier Sep 17, 2015

Member

Could you add a test for setting with an object?

This comment has been minimized.

@pixel2

pixel2 Sep 17, 2015

Contributor

Unit tests added.

@@ -322,8 +322,11 @@ Instance.prototype.set = function(key, value, options) { // testhint options:non
// If attribute is not in model definition, return
if (!this._isAttribute(key)) {
if (key.indexOf('.') > -1 && this.Model._isJsonAttribute(key.split('.')[0])) {
Dottie.set(this.dataValues, key, value);
this.changed(key, true);

This comment has been minimized.

@mickhansen

mickhansen Aug 24, 2015

Contributor

Good catch, that was a dumb/un-tested piece of code by me it seems.

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 17, 2015

Good work - do you mind squashing to a single, descriptive commit, preferably prefixed with feat(model.set) , then I'll press the big green merge button

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 17, 2015

Oh, and a changelog entry as well would be awesome!

@pixel2 pixel2 force-pushed the pixel2:master branch from d4a1466 to 4585ba4 Sep 17, 2015

@pixel2

This comment has been minimized.

Contributor

pixel2 commented Sep 17, 2015

Done, waiting for that big green click :)

mickhansen added a commit that referenced this pull request Sep 18, 2015

Merge pull request #4379 from pixel2/master
Calling set with dot.separated key on a JSON/JSONB attribute will not flag the entire object as changed

@mickhansen mickhansen merged commit 1a272c8 into sequelize:master Sep 18, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 18, 2015

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment