fix non-object attributes being passed to model.parse()#2056
Merged
Conversation
Member
|
Won't this fix mean that we will still be left without the updated attributes? |
Contributor
Author
|
@ricardograca When the query returns the updated attributes in an object, yes it will add the updated attributes. Added a unit test for that behavior. Perhaps you know more about why this is, but I found that this code gets called twice on a model.save() - the first time Thank you for the review! |
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
2 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.
Introduction
When model.save() is executed with method: 'update', the
respargument is first the number of rows that were updated. This causesundefinedto be passed into this.parse on line 1183 in model.save(). Any custom model.parse functions on a model are expecting an object of attributes to be passed.Also, this branch can be reached if the save method is 'insert' and this.id is not null, which we have found to occur when query.options.returning was set to 'id' instead of the default '*' which bookshelf expects.
Motivation
Any custom model.parse functions are not expecting to be receiving any argument other than an object, potentially causing unexpected errors.
Proposed solution
Check that the response is an object before passing it to model.parse and merging with this.attributes.