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

[BUGFIX] New test and possible fix for issue #7265 (self-referent rel) #7266

Closed

Conversation

davelindquist-egistix
Copy link

This addresses issue #7265 (Self-Referent Relationship in Save Response Blows Up Save).

It includes a new test that replicates the problem - saving a record, and having the 'backend' return a relationship that is self-referent, thus blowing up Ember Data when it checks to see if the just-saved id has already been used. (Said id was added to the store as part of the relationships-handling.)

It also includes a patch to record-data.ts that solves the problem in the test, and does not cause any other tests to fail. HOWEVER, it does this by simply re-ordering the handling of relationships and the 'set the record id' call - I don't know if there is a reason that they were done in the opposite order originally, and thus don't know for certain if this will break something else. (I'm hoping the tests would have shown that, but perhaps wiser eyes can take a look to make certain.)

Thanks!

…ent relationship in save response blows up)
@davelindquist-egistix
Copy link
Author

Hmm, it appears that Travis runs additional tests beyond what happens with the tests locally, and these have failed. I will close this PR, and resubmit one with only the test in it.

@runspired
Copy link
Contributor

Did you ever resubmit? The fix here is simple enough I'd be ok with it since it slightly improves the situation (most of the time this sort of case doesn't have such an easy way to address it in the internals). I'd be happy to look at whatever failed and see if this could work.

@runspired runspired reopened this Apr 14, 2021
runspired added a commit that referenced this pull request May 4, 2021
@runspired
Copy link
Contributor

Confirmed this test passes against master so closing, but I like the ordering change and test since they make more sense anyway so am porting these to a new PR.

@runspired runspired closed this May 4, 2021
runspired added a commit that referenced this pull request May 5, 2021
runspired added a commit that referenced this pull request May 8, 2021
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.

2 participants