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

adding and removing records from hasMany relationship broken. #2666

Closed
arenoir opened this issue Jan 6, 2015 · 30 comments · Fixed by #2722
Closed

adding and removing records from hasMany relationship broken. #2666

arenoir opened this issue Jan 6, 2015 · 30 comments · Fixed by #2722
Labels
🏷️ bug This PR primarily fixes a reported issue
Milestone

Comments

@arenoir
Copy link
Contributor

arenoir commented Jan 6, 2015

I am experiencing strange behavior in the latest version (1.0.0-beta.14.1) of ember data.

It appears that items removed from a hasMany collection reappear when adding new items.

Checkout the following jsbin http://jsbin.com/boluba/13

Remove an Item then add an item, the removed item will reappear. The strange thing is the item is not in the store when using the ember inspector.

@arenoir
Copy link
Contributor Author

arenoir commented Jan 6, 2015

It works as expected in 1.0.0-beta.12

http://jsbin.com/boluba/14/

@igorT
Copy link
Member

igorT commented Jan 6, 2015

Thanks for opening an issue, investigating.

@igorT
Copy link
Member

igorT commented Jan 6, 2015

Confirm this is a real bug, might take couple days to resolve, as it involves making smarter diffing of server/local state. Sorry about that :(

@nicolaschenet
Copy link

We have the same bug here. For now, we just implemented a very basic (and ugly) workaround, by filtering items by ('isDeleted', false). But that's not awesome at all :(

Hope to see some updates here soon ;)

@guillaumepotier
Copy link

👍

@arenoir
Copy link
Contributor Author

arenoir commented Jan 14, 2015

@igorT any word on what might be causing this? Perhaps you can point me in the right direction. I would like to take a stab at this.

@bmac bmac added the bug label Jan 15, 2015
dwickern added a commit to dwickern/ember-data that referenced this issue Jan 21, 2015
dwickern added a commit to dwickern/ember-data that referenced this issue Jan 21, 2015
@dwickern
Copy link
Contributor

This was slippery but I managed to create a test.

I expected that post.get('comments') would give me back the same RecordArray each time. The test passes if I just look at the original array (i.e. comment out lines 1273, 1279, 1287).

@arenoir
Copy link
Contributor Author

arenoir commented Jan 24, 2015

@dwickern thanks for the test I am flopping around on the floor trying to figure out what is going on.

It appears there are three arrays being managed for each hasMany relationship currentState, canonicalState and canonicalMembers. I think this is an attempt to do dirty tracking. IMHO I think this is technically not worth it. I prefer how rails handles this. Only belongs_to relationships are tracked. This makes sense because there is a primary_key that is directly effected. This management of has_many relationships is also responsible for one of my biggest issues; getting data out in the same form that it went in.

@igorT, @bmac what are your thoughts? Am I understanding this correctly?

@lightningdev45
Copy link

I am experiencing same bug. I don't know if it helps anyone, but the way in which the item is removed matters. If the record is removed using unloadRecord or destroyRecord I get the problem. If instead the record is removed using simply destroy(), the deleted record no longer appears the next time a record is added.

@bmac
Copy link
Member

bmac commented Jan 24, 2015

I finally had some time to sit down and look at this one. @dwickern's test case was extremely helpful in tracking down the issue. I have opened pr #2722 to hopefully fix the issue.

bmac added a commit to bmac/data that referenced this issue Jan 24, 2015
@eccegordo
Copy link
Contributor

Hi I have also come across this bug. Not sure how to create a failing test but have been able to create an example project that demonstrates it as well as JSBin.

From my investigation bug seems to have been introduced with this PR
#2576

Example repo
https://github.com/eccegordo/ed-dupe-bug

Example JSBin
http://jsbin.com/xunuwukexe/1

Create a comment that belongsTo a post
Delete comment and save
Add a new comment and save

The original comment comes back from dead, and the new comment is duplicated.

@dwickern
Copy link
Contributor

dwickern commented Feb 5, 2015

@eccegordo a workaround is to put something like this in your controller

comments: Ember.computed.filterBy('model.comments', 'isDeleted', false),

Here's your JSBin with the workaround applied:
http://jsbin.com/lilokocivi/1

@krzkrzkrz
Copy link

Seeing this too, on my end...

@dgaus
Copy link

dgaus commented Mar 12, 2015

+1

@bmac bmac added this to the 1.0.0-beta.16 milestone Mar 12, 2015
@davidlormor
Copy link

+1 here...if I destroy a record and then create a new one on the array, the destroyed record reappears. The Ember Inspector has the destroyed record in its canonicalState with the isDestroyed flag set to true.

edzen pushed a commit to OLDZenefits/emberjs-data that referenced this issue Mar 20, 2015
bmac pushed a commit to bmac/data that referenced this issue Mar 23, 2015
bmac added a commit to bmac/data that referenced this issue Mar 23, 2015
@dingchaoyan1983
Copy link

can anyone tell me this problem is resolved in which version of the ember-data

@bmac
Copy link
Member

bmac commented Oct 13, 2015

@dingchaoyan1983 I believe this issue was fixed in v1.0.0-beta.16.

@masciugo
Copy link

still experiencing this with ember data 2.10.. is it possible?

@gbattra
Copy link

gbattra commented Feb 1, 2017

i am also still experiencing this bug. i can imagine a lot of apps want to add and remove objects from their model's hasMany relations.

@runspired
Copy link
Contributor

@gregattra regression in 2.11, will be fixed shortly.

@masciugo you absolutely sure you see this on 2.10?

@masciugo
Copy link

masciugo commented Feb 3, 2017

@runspired No, not sure anymore cause I upgraded to 2.11. Anyway, I was actually experiencing this which is going to be resolved soon, I guess

@bkl1989
Copy link

bkl1989 commented Feb 3, 2017

I'm experiencing this. I was using 2.11, and I rolled back to 2.10. I'm still experiencing it there. I did a clean build to be sure. I'll continue to roll back and I'll comment with the most recent version that's working.

I am wondering something, and I don't mean it as a criticism - presumably, this a pretty common use case, so I'd figure you'd have some testing built around it. Given that this bug made it into a release or two, I am wondering whether I am doing something that is against convention. Like these examples, I'm just trying to remove a record from a hasMany array, subsequently saving the parent record. First, I was using destroyRecord, but I also tried removeObject and popObject.

@bkl1989
Copy link

bkl1989 commented Feb 3, 2017

I could reproduce the issue on 2.10.0, 2.9.0, and 2.8.0. The confines of my project don't permit rolling back further. In case you would like, I'll include a bit more detail:

  • I load a record that has a hasMany array (children) of length 1
  • I remove the record from the array using removeObject
  • I save the parent record
  • I get an error on save

I'm sorry I can't provide a code snippet or more detailed feedback. I am working at the moment, and can provide further feedback if it would help.

Thanks!

@bmac
Copy link
Member

bmac commented Feb 3, 2017

@bkl1989 what kind of error are you getting on save? A validation error triggering that results in DS.InvalidError or some other kind of error?

@bkl1989
Copy link

bkl1989 commented Feb 3, 2017

Here's the code (please excuse my nested promise...) I've commented to indicate where the error occurs. Some of this isn't relevant, but I have left it in for context and so I don't accidentally modify something important.

        //here, i have verified that moduleRecord.children is of length 1
        // (until popObject, reducing the length to 0)     
        let oldChild = moduleRecord.get('children').popObject();
        pageID = module.pageId || oldChild.get('fields.pageId');
        webLinkURL = module.url || oldChild.get('fields.url');

        if (module.linkType === 'None') {
          pageID = null;
          webLinkURL = null;
        }

        if (oldChild) {
          //the error is thrown at moduleRecord.save
          return moduleRecord.save().then(() => {
            //this line is never reached
            oldChild.deleteRecord();
            return oldChild.save();
          });
        }

And here is the error (caught in the .catch() of the enclosing promise chain)

   TypeError: Cannot read property '_relationships' of undefined
    at ManyRelationship.addCanonicalRecord (relationship.js:77)
    at ManyRelationship.addCanonicalRecord (has-many.js:45)
    at ManyRelationship.computeChanges (has-many.js:151)
    at ManyRelationship.updateRecordsFromAdapter (relationship.js:227)
    at store.js:2244
    at ext.js:500
    at cb (ember.debug.js:20938)
    at OrderedSet.forEach (ember.debug.js:20739)
    at Map.forEach (ember.debug.js:20942)
    at Function.eachRelationship (ext.js:499)

Also, in a debugger breakpoint inside the .catch, moduleRecord.get('children') indicates that, though the current state of the array is length 0, the canonical state is still length 1 (I am not sure if this is relevant / helpful)

Let me know if you'd like more detail. This is the best I can do for now without giving you access to the whole project :)

@runspired
Copy link
Contributor

@bkl1989 this seems unrelated, can you feed us more of the error caught?

@bkl1989
Copy link

bkl1989 commented Feb 3, 2017

@runspired Sorry, the first line of my error stack got pruned off in the block formatting. I updated the comment.

If it is unrelated, it's pretty similar. The removal of the child is the only modification to the module record at this point. The API call fires successfully and returns a payload which correctly represents the update. I'm not sure, after that, where the error occurs.

@runspired
Copy link
Contributor

The regression in 2.11 and 2.12 was fixed by #4791 and #4792 with the failing test #4790 added to cover this in the future.

@bkl1989 your issue is not similar at all. The issue here was not an error thrown, the only correlation is that both relate to deleting a record in a has many. I suggest you ask in the Ember Community Slack and try to replicate this in a twiddle.

@arian-kh
Copy link

arian-kh commented Jun 12, 2017

@runspired We're experiencing this issue in Ember data 2.13.1 Is there a version you recommend rolling back to?

@offirgolan
Copy link

@arian-kh I just ran into this issue with 2.13.2. Upgrading to 2.14.7 fixed it for me.

@runspired runspired added 🏷️ bug This PR primarily fixes a reported issue and removed Bug labels Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bug This PR primarily fixes a reported issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.