Skip to content
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

Issue with updating keySource field #322

Closed
wants to merge 1 commit into from

Conversation

kastigar
Copy link
Contributor

@kastigar kastigar commented Apr 5, 2013

There is an issue with updating source field of relation. While initializing source field is removed and relation field appears. But updating will not remove source field and may cause future problems. I've created 2 test cases to show this issue. One is case when i get problem and second is case that displays a source of a problem.
I'm not so familiar with BR internals yet and I'm not sure with fix. But will propose: in updateRelations to check if keySource != key and typeof attributes[keySource] != 'undefined' then update model and delete attributes[keySource]. Smth like this.


test( "Catching case", function() {
var SubModel = Backbone.RelationalModel.extend({
idAttribute: 'id'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't necessary

@omriyariv
Copy link

+1 for this bug.

in updateRelations line ~ 1195:

this.attributes[ rel.keySource ] || this.attributes[ rel.key ]

This line is buggy in when you try to nullify a referenced model by setting keySource field to null.
I'm building an undo/redo system by keeping a stack of JSON representations of models. Simply trying to set the model to its previous JSON fail here...

@PaulUithol
Copy link
Owner

Hmm. I'm not totally sure what the desired behavior would be here - or which part of this is the bug exactly. It seems that the unset option triggers this in some way, but only if data is present on keySource. Should keySource be emptied after a set?

@PaulUithol
Copy link
Owner

Thanks a lot for the proper test, the commit above should fix this issue.

@omriyariv
Copy link

Hi, thanks for addressing this issue but the commit above didn't fix my bug. I made a JSFiddle that demonstrates it:
http://jsfiddle.net/pEMwn/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants