Complex model fields are returned by reference by `toJSON` #2315

Closed
jedrichards opened this Issue Feb 26, 2013 · 11 comments

Comments

Projects
None yet
7 participants
@jedrichards

If your model contains some complex data in some of its fields (say some nested simple objects) then the object returned by toJSON will refer to these by reference. So if you mess around with the values in your toJSON object then you are inadvertently silently poisoning your model data directly.

Here's an example:

// Init model with data object
var model = new Backbone.Model({
    simpleValue: "foo",
    objectValue: {
        subObject: {
            nestedValue: "bar"
        }
    }
});

// Create a toJSON representation
var modelJSON = model.toJSON();

// Change some values in your toJSON object
modelJSON.simpleValue = "baz";
modelJSON.objectValue.subObject.nestedValue = "baz";

console.log(model.get("simpleValue")); // returns "foo", as expected
console.log(model.get("objectValue").subObject.nestedValue); // returns "baz", lolwut?

So it seems like toJSON is returning references to complex objects in the model, not clones. Is that desired behaviour? It just took me ages to track down this issue. I was doing some post-processing and calculations on some to toJSON results before sending back to the server expecting it to be free of repercussions ...

@tgriesser

This comment has been minimized.

Show comment
Hide comment
@tgriesser

tgriesser Feb 26, 2013

Collaborator

Hi @jedrichards - thanks for the issue. This is in fact desired behavior, as get is one of the hottest code paths in Backbone, and doing a deep clone of the attributes object is a significant performance difference:
http://jsperf.com/tojson-stringify

The issues around deep-cloning have been brought several times before (both with toJSON and model.get) - #2065, #1601, #1602, #988, #1002 and it's likely not something that will change.

If you find it easier to work with deeply cloned attributes in your application, you can feel free to extend the Model's prototype with:

Backbone.Model.prototype.toJSON = function () {
  return JSON.parse(JSON.stringify(this.attributes));
};

Although, it might be worth putting a note below toJSON just as there is below Model#defaults.

Collaborator

tgriesser commented Feb 26, 2013

Hi @jedrichards - thanks for the issue. This is in fact desired behavior, as get is one of the hottest code paths in Backbone, and doing a deep clone of the attributes object is a significant performance difference:
http://jsperf.com/tojson-stringify

The issues around deep-cloning have been brought several times before (both with toJSON and model.get) - #2065, #1601, #1602, #988, #1002 and it's likely not something that will change.

If you find it easier to work with deeply cloned attributes in your application, you can feel free to extend the Model's prototype with:

Backbone.Model.prototype.toJSON = function () {
  return JSON.parse(JSON.stringify(this.attributes));
};

Although, it might be worth putting a note below toJSON just as there is below Model#defaults.

@jedrichards

This comment has been minimized.

Show comment
Hide comment
@jedrichards

jedrichards Feb 26, 2013

This isn't an issue around the behaviour of Model.get, this relates to the behaviour of Model.toJSON.

At the moment if you change a primitive/simple value in the Model.toJSON result then the original model remains unchanged (great, as expected), but if you change some field that resides in some structure of nested primitives you end up silently poisoning the model. Seems quite broken? Especially the different behaviour in the two scenarios.

I appreciate that you don't want to add a deep cloning expense on Model.get, but I'm not so sure why you're reluctant to add some simple cloning of JSON compatible primitives to toJSON?

People are used to JSON operations producing safe copies of constructs of simple nested primitives, the behaviour of toJSON breaks that contract at the moment.

This isn't an issue around the behaviour of Model.get, this relates to the behaviour of Model.toJSON.

At the moment if you change a primitive/simple value in the Model.toJSON result then the original model remains unchanged (great, as expected), but if you change some field that resides in some structure of nested primitives you end up silently poisoning the model. Seems quite broken? Especially the different behaviour in the two scenarios.

I appreciate that you don't want to add a deep cloning expense on Model.get, but I'm not so sure why you're reluctant to add some simple cloning of JSON compatible primitives to toJSON?

People are used to JSON operations producing safe copies of constructs of simple nested primitives, the behaviour of toJSON breaks that contract at the moment.

@tgriesser

This comment has been minimized.

Show comment
Hide comment
@tgriesser

tgriesser Feb 26, 2013

Collaborator

The issue really comes down to the fact that arrays and objects are passed by reference in javascript.

The issue people often run into with model.get is when you get an object or array which exists as an attribute value, modifying the value and setting it back on the model will not trigger a change (since you're modifying a reference to an attribute on the model).

var m = new Model({id:1, attr: {key: 'value'}});
var fetchedAttr = m.get('attr');
fetchedAttr.newKey = 'newValue';
m.set('attr', fetchedAttr) // Won't trigger a change, as the `get('attr')` returns the object by reference

I wouldn't say it's broken, it's more just how javascript works. If you prefer a different approach, it'd be best to override Backbone's default toJSON as mentioned above.

Edit: I think people are used to JSON operations such as JSON.parse or JSON.stringify producing safe copies, I'm not sure I'd say the same about toJSON - it only ensures a value which is formatted to be handled by JSON.stringify, which in the case of an object or array, wouldn't be ensuring a safe copy.

Collaborator

tgriesser commented Feb 26, 2013

The issue really comes down to the fact that arrays and objects are passed by reference in javascript.

The issue people often run into with model.get is when you get an object or array which exists as an attribute value, modifying the value and setting it back on the model will not trigger a change (since you're modifying a reference to an attribute on the model).

var m = new Model({id:1, attr: {key: 'value'}});
var fetchedAttr = m.get('attr');
fetchedAttr.newKey = 'newValue';
m.set('attr', fetchedAttr) // Won't trigger a change, as the `get('attr')` returns the object by reference

I wouldn't say it's broken, it's more just how javascript works. If you prefer a different approach, it'd be best to override Backbone's default toJSON as mentioned above.

Edit: I think people are used to JSON operations such as JSON.parse or JSON.stringify producing safe copies, I'm not sure I'd say the same about toJSON - it only ensures a value which is formatted to be handled by JSON.stringify, which in the case of an object or array, wouldn't be ensuring a safe copy.

@jedrichards

This comment has been minimized.

Show comment
Hide comment
@jedrichards

jedrichards Feb 27, 2013

I think toJSON implies that you are converting the model to something else, to a JSON-compatible object, ready for transport. I think its reasonable to expect that working with this converted object shouldn't have repercussions. The fact that it has repercussions for objects, and not for primitive values makes it even more troubling.

Anyway, just my 2p's worth :)

I think toJSON implies that you are converting the model to something else, to a JSON-compatible object, ready for transport. I think its reasonable to expect that working with this converted object shouldn't have repercussions. The fact that it has repercussions for objects, and not for primitive values makes it even more troubling.

Anyway, just my 2p's worth :)

@zackd

This comment has been minimized.

Show comment
Hide comment
@zackd

zackd Mar 8, 2013

+1 @jedrichards "I think its reasonable to expect that working with this converted object shouldn't have repercussions. The fact that it has repercussions for objects, and not for primitive values makes it even more troubling."

I've been struggling to debug a problem caused this exact behaviour! ..definitely wasnt expecting backbone to work in this way..

zackd commented Mar 8, 2013

+1 @jedrichards "I think its reasonable to expect that working with this converted object shouldn't have repercussions. The fact that it has repercussions for objects, and not for primitive values makes it even more troubling."

I've been struggling to debug a problem caused this exact behaviour! ..definitely wasnt expecting backbone to work in this way..

@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Mar 8, 2013

Collaborator

I think its reasonable to expect that working with this converted object shouldn't have repercussions. The fact that it has repercussions for objects, and not for primitive values makes it even more troubling.

Nah, @tgriesser has it right above. Backbone is just working with toJSON idiomatically. There's no need to clone the object unless you're going to change it, in which case you can copy it yourself explicitly, making it easy for others to follow.

Collaborator

braddunbar commented Mar 8, 2013

I think its reasonable to expect that working with this converted object shouldn't have repercussions. The fact that it has repercussions for objects, and not for primitive values makes it even more troubling.

Nah, @tgriesser has it right above. Backbone is just working with toJSON idiomatically. There's no need to clone the object unless you're going to change it, in which case you can copy it yourself explicitly, making it easy for others to follow.

@jedrichards

This comment has been minimized.

Show comment
Hide comment
@jedrichards

jedrichards Mar 8, 2013

If Backbone is using it purely idiomatically why not just pass a reference to the model's attributes object and be done with it? But as it is Backbone is doing some basic cloning because changing a primitive value in the toJSON result doesn't change the model. This is in contrast to objects which are added to the toJSON result by reference, as stated. I could buy all your arguments in support of passing references to toJSON if you were handling the cases of primitives and objects in the same way. It's not good design that they're handled differently, it should at least be consistent whichever way you go, either cloning or not.

If Backbone is using it purely idiomatically why not just pass a reference to the model's attributes object and be done with it? But as it is Backbone is doing some basic cloning because changing a primitive value in the toJSON result doesn't change the model. This is in contrast to objects which are added to the toJSON result by reference, as stated. I could buy all your arguments in support of passing references to toJSON if you were handling the cases of primitives and objects in the same way. It's not good design that they're handled differently, it should at least be consistent whichever way you go, either cloning or not.

@tgriesser

This comment has been minimized.

Show comment
Hide comment
@tgriesser

tgriesser Mar 8, 2013

Collaborator

@jedrichards - that's actually a fair point... @braddunbar any reason you know of for needing the shallow cloned attributes?

Collaborator

tgriesser commented Mar 8, 2013

@jedrichards - that's actually a fair point... @braddunbar any reason you know of for needing the shallow cloned attributes?

@mehcode

This comment has been minimized.

Show comment
Hide comment
@mehcode

mehcode Mar 8, 2013

To throw in my 2 cents.

At chaplin we developed a serialize method for use in views (https://github.com/chaplinjs/chaplin/blob/master/src/chaplin/models/model.coffee#L11-L54) that outperforms toJSON by a good margin as well as uses prototype delegation to protect changes to attributes. It's been working really well for us.

mehcode commented Mar 8, 2013

To throw in my 2 cents.

At chaplin we developed a serialize method for use in views (https://github.com/chaplinjs/chaplin/blob/master/src/chaplin/models/model.coffee#L11-L54) that outperforms toJSON by a good margin as well as uses prototype delegation to protect changes to attributes. It's been working really well for us.

@caseywebdev

This comment has been minimized.

Show comment
Hide comment
@caseywebdev

caseywebdev Mar 9, 2013

Collaborator

@tgriesser If people are using toJSON the right way, no reason for a shallow clone. If people are overriding toJSON they clone what they need to clone to leave attributes untouched.

Collaborator

caseywebdev commented Mar 9, 2013

@tgriesser If people are using toJSON the right way, no reason for a shallow clone. If people are overriding toJSON they clone what they need to clone to leave attributes untouched.

@tjgupta

This comment has been minimized.

Show comment
Hide comment
@tjgupta

tjgupta Mar 30, 2015

I just banged my head against this for quite a while too. It seems counter-intuitive that a method named toJSON() would output a shallow serialization of your model, but with complex attributes as pointers to the model.

Also, once I figured out why modifying data in my serialized toJSON object was affecting data in my model, I went to the documentation to understand if there was a deep copy option for model.toJSON(). It seems you can pass "options" in, but they are then not used. Perhaps this could be configurable? Or if not, then the documentation should be updated.
https://github.com/jashkenas/backbone/blob/master/backbone.js#L397

I'm happy to submit a PR to either update the docs, or add a "deep" option, depending on what direction you all prefer.

tjgupta commented Mar 30, 2015

I just banged my head against this for quite a while too. It seems counter-intuitive that a method named toJSON() would output a shallow serialization of your model, but with complex attributes as pointers to the model.

Also, once I figured out why modifying data in my serialized toJSON object was affecting data in my model, I went to the documentation to understand if there was a deep copy option for model.toJSON(). It seems you can pass "options" in, but they are then not used. Perhaps this could be configurable? Or if not, then the documentation should be updated.
https://github.com/jashkenas/backbone/blob/master/backbone.js#L397

I'm happy to submit a PR to either update the docs, or add a "deep" option, depending on what direction you all prefer.

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