Save with wait:true, patch: true and custom serialize() method
#64
Conversation
When calling the `save()` method with `wait: true` and `patch: true`, the current implmentation assumes a basic data structure and ignored the structure generated by an overridden `serialize` method. This fix invokes the `serialize()` method to ensure the user's data structure is respected. In order for this to work with `PATCH`, we have to inspect that structure to find where the attributes have been placed, and then limit the properties of those object to only the ones changed. This still makes some assumptions about the data structure (namely that all attributes appear within a single object within the serialized structure), but hopefully less restrictive ones than the current implementation.
| @@ -20,6 +24,30 @@ var wrapError = function (model, options) { | |||
| }; | |||
| }; | |||
|
|
|||
| // recursive function to find the attrs in the serialized object | |||
| function transformForPatch(obj, attrs) { | |||
There was a problem hiding this comment.
Can we change this to a var function like the others?
There was a problem hiding this comment.
It still needs to be named because it's recursive. Is
var transformForPatch = function transformForPatch(obj, attrs) {
okay? Some folks think it's weird having both.
There was a problem hiding this comment.
Yes this is what I meant, thanks. I should have been more explicit, sorry for the confusion.
| var clonedModel = new this.constructor(this.getAttributes({props: true})); | ||
| clonedModel.set(attrs); | ||
|
|
||
| options.attrs = assign(model.serialize(), attrs); |
There was a problem hiding this comment.
Missed commit here. This should be options.attrs = clonedModel.serialize();. Will add after fix above.
|
Thanks for the feedback @wraithgar. Additional commits to address those issues. |
| // if we're waiting we haven't actually set our attributes yet so | ||
| // we need to do make sure we send right data | ||
| if (options.wait && method !== 'patch') options.attrs = assign(model.serialize(), attrs); | ||
| if (options.wait && method !== 'patch') { | ||
| var clonedModel = new this.constructor(this.getAttributes({props: true})); |
There was a problem hiding this comment.
Was this the one that was causing errors when using this.serialize() then?
|
I think I've asked all the questions I had when looking at this PR. Would love more eyeballs on this. |
|
I've found some issues with this in production. I should be close to resolving, then will update this PR. |
fixes #52
When calling the
save()method withwait: trueandpatch: true, thecurrent implmentation assumes a basic data structure and ignored the
structure generated by an overridden
serializemethod.This fix invokes the
serialize()method to ensure the user's datastructure is respected. In order for this to work with
PATCH, we have toinspect that structure to find where the attributes have been placed, and
then limit the properties of those object to only the ones changed.
This still makes some assumptions about the data structure (namely that all
attributes appear within a single object within the serialized structure),
but hopefully less restrictive ones than the current implementation.