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

Loading a previously unloaded record doesn't update relationships #5136

Closed
workmanw opened this issue Aug 17, 2017 · 17 comments
Closed

Loading a previously unloaded record doesn't update relationships #5136

workmanw opened this issue Aug 17, 2017 · 17 comments
Labels
🏷️ bug This PR primarily fixes a reported issue

Comments

@workmanw
Copy link

workmanw commented Aug 17, 2017

Unfortunately I've hit another unloadRecord bug. When I unload a record that is on the belongsTo side of a one-to-many it is correctly removed from the hasMany array. However when I push that same record back into the store, it is not added back to the hasMany array.

Ember: 2.14.1
Ember-Data: 2.14.10

Reproduction repo: workmanw/unload-relationships-bug.
Code: app/controllers/index.js.
Live example: https://workmanw.github.io/unload-relationships-bug
Failing Test: #5137

To reproduce:

  1. Load Record
  2. Unload Record
  3. Load Record again.

You'll notice that the "All Comments" section shows the record was reloaded (re-pushed) into the store, however the relationship on the post is not updated to reflect this.

One note here, I'm using async: false on this relationship. If I use async: true it creates a crash on unload.

@workmanw
Copy link
Author

Just a little more poking around.

This functionality last worked correctly in ember-data-2.12.2. With ember-data-2.13.0 it was not possible to unload records and update relationships. That was fixed in ember-data-2.14.7 via #5097, however this issue was left lingering.

@workmanw workmanw changed the title Loading a record that was previously unloaded doesn't update relationships Loading a previously unloaded record doesn't update relationships Aug 17, 2017
workmanw pushed a commit to workmanw/data that referenced this issue Aug 17, 2017
@stefanpenner
Copy link
Member

@workmanw thanks for the failing test

@workmanw
Copy link
Author

You're welcome! Let me know if there is anything else I can do to help.

@stefanpenner
Copy link
Member

@workmanw mind adding another test for:

If I use async: true it creates a crash on unload.

That way we can fix both these issues and ensure they don't come back

@stefanpenner
Copy link
Member

stefanpenner commented Aug 24, 2017

Unfortunately I've hit another unloadRecord bug.

ooo my favorite ... :trollface:

@workmanw
Copy link
Author

@stefanpenner Sure thing, I'll add another test now.

@workmanw
Copy link
Author

@stefanpenner Actually, as I'm trying to write another test, I'm realizing that it may not be a valid use case. The reason is when you unload an async relationship, it still leaves the hasMany setup so that it can fetch it again if needed. Consider this (a little pseudo code):

let adam = store.peekRecord('person', 1);
adam.hasMany('boats').ids();   //  '[]'

store.push(/* a boat for adam */);
adam.hasMany('boats').ids();   // '[20]';

store.unloadRecord('boat', 20);
adam.hasMany('boats').ids();   // '[20]';

As you can see, just unloading the boat, no long tears down the relationship because it's async (which logically makes sense to me).


As for the error I mentioned, it turns out this was a different problem altogether. TL;DR; if I unload a model, then try to refetch it via it's async relationship and that fails, it throws a crash because this.record is now null. Here is a stack:

TypeError: Cannot read property 'eachAttribute' of null
    at new Snapshot (-private.js:5091)
    at InternalModel.createSnapshot (-private.js:6930)
    at _find (-private.js:8695)
    at Class._fetchRecord (-private.js:10670)
    at _fetchRecord (-private.js:10731)
    at Class._flushPendingFetchForType (-private.js:10830)
    at cb (ember.debug.js:27921)
    at OrderedSet.forEach (ember.debug.js:27728)
    at MapWithDefault.forEach (ember.debug.js:27929)
    at Class.flushAllPendingFetches (-private.js:10712)

I think this should be written up separately.

@stefanpenner
Copy link
Member

As you can see, just unloading the boat, no long tears down the relationship because it's async (which logically makes sense to me).

👍

I think this should be written up separately.

👍

stefanpenner pushed a commit that referenced this issue Aug 25, 2017
@stefanpenner
Copy link
Member

stefanpenner commented Aug 25, 2017

I've identified in broad strokes the issue, but haven't isolated the exact problem yet.

The issue as I see it so far, is if an internalModel is resurrected (unloaded, but still retained by an async relationship) \w updated relationship info, that relationship info isn't propagated to the inverse (in the case, person.boats is not updated).

I'll try to find more time later today to investigate further... I'll need to talk to @hjdivad, see what makes the most sense short-term, as we have a future path forward.


On a positive note, I found several other things while debugging this:

@workmanw
Copy link
Author

@stefanpenner Is there anything I can do to help here? I know you're working hard on it and don't want to rush you. If I can't come up with a workaround for this, I'm faced with downgrading ember-data to 2.12 again for our next release.

@lolmaus
Copy link
Contributor

lolmaus commented Sep 6, 2017

@workmanw Can you please update the demo to the latest Ember CLI, Ember & Ember Data? That would indicate that the issue is not covered by bugfixes from ED 2.15.

@workmanw
Copy link
Author

workmanw commented Sep 6, 2017

@lolmaus @stefanpenner -- Yes, this is still an issue with ember-data-2.15. I've updated my repo and my live example to use latest of Ember, Ember CLI and Ember Data.

Any help here would be greatly appreciated. Our application is stuck back on 2.12 because of all these unloadRecord issues.

@workmanw
Copy link
Author

@stefanpenner et al -- Is there anything that can be done to move this forward? I'm sorry to be a pest here, but this has locked us 3 releases behind (soon to be 4). We've explored every work around we can think of and can't seem to find a way to stay up to date. I hate feeling left behind, our ember-try tests have been failing since April, being in this state leaves us feeling disconnected from the whole community.

@vvainio
Copy link

vvainio commented Sep 25, 2017

Our fairly large Ember app is facing the same issue, and this has been a blocker for the past few releases as @workmanw described. I would appreciate any information whether this issue is being worked on, otherwise we'll need to try coming up with a workaround.

@eduardbaitinger
Copy link

Also facing the same problems with unloading records in our enterprise ember apps and therefore stuck on ember data 2.12 :(

workmanw pushed a commit to workmanw/data that referenced this issue Oct 17, 2017
hjdivad added a commit that referenced this issue Nov 2, 2017
Previously relationships were only cleared for new records.  Now they
are cleared for destroyed records as well, allowing destroyed records to
be pushed back into the store and properly update their inverse
relationships.

[Fix #5136]
hjdivad added a commit that referenced this issue Nov 2, 2017
Previously relationships were only cleared for new records.  Now they
are cleared for destroyed records as well, allowing destroyed records to
be pushed back into the store and properly update their inverse
relationships.

This fixes the public API for client-side delete via an adapter,
see #5136.
workmanw pushed a commit to workmanw/data that referenced this issue Dec 1, 2017
hjdivad pushed a commit that referenced this issue Dec 14, 2017
hjdivad pushed a commit that referenced this issue Jan 4, 2018
hjdivad pushed a commit that referenced this issue Jan 11, 2018
hjdivad pushed a commit that referenced this issue Jan 11, 2018
hjdivad added a commit that referenced this issue Jan 11, 2018
A number of use cases rely on unloadRecord's 2.12 behaviour of treating
unloadRecord as a client-side delete on the inverse side of a sync
relationship.

This pr restores that functionality, while retaining the
invalidate+refetch functionality for async relationships.

This behaviour is codified in a number of tests within
tests/integration/records/unload-test.js

Fix #5136, #5137
@lolmaus
Copy link
Contributor

lolmaus commented Jan 11, 2018

@igorT Woah, that's a nice surprise! 🙇‍♂️

I wonder if it fixes other issues as well. There are several closely related ones.

@hjdivad
Copy link
Member

hjdivad commented Jan 11, 2018

@lolmaus please report issues and tag me if you're still seeing unloadRecord related regressions.

There's an outstanding issue on that PR that still needs investigating, although it's related to filtering record arrays and not relationships.

bmac pushed a commit that referenced this issue Feb 13, 2018
(cherry picked from commit bbe8b41)
bmac pushed a commit that referenced this issue Feb 13, 2018
A number of use cases rely on unloadRecord's 2.12 behaviour of treating
unloadRecord as a client-side delete on the inverse side of a sync
relationship.

This pr restores that functionality, while retaining the
invalidate+refetch functionality for async relationships.

This behaviour is codified in a number of tests within
tests/integration/records/unload-test.js

Fix #5136, #5137

(cherry picked from commit 6e8a236)
mcfiredrill pushed a commit to mcfiredrill/data that referenced this issue Feb 15, 2018
(cherry picked from commit bbe8b41)
mcfiredrill pushed a commit to mcfiredrill/data that referenced this issue Feb 15, 2018
A number of use cases rely on unloadRecord's 2.12 behaviour of treating
unloadRecord as a client-side delete on the inverse side of a sync
relationship.

This pr restores that functionality, while retaining the
invalidate+refetch functionality for async relationships.

This behaviour is codified in a number of tests within
tests/integration/records/unload-test.js

Fix emberjs#5136, emberjs#5137

(cherry picked from commit 6e8a236)
@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

No branches or pull requests

7 participants