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

Records not found in a coalescing request are created and left loading forever #4463

Closed
dgaus opened this issue Jul 6, 2016 · 20 comments
Closed
Assignees
Labels
🏷️ bug This PR primarily fixes a reported issue

Comments

@dgaus
Copy link

dgaus commented Jul 6, 2016

Hi, on ember-data 2.6.1 I'm issuing several .finds which get coalesced into one request. If any of them do not exist on the server I get a

WARNING: Ember Data expected to find records with the following ids in the adapter response but they were missing: [xx, ...].

However if later I try to create a record with any of those IDs, it fails because

The id xx has already been used with another record of type ....

Then obj = store.peekAll('model').findBy('id', 'xx').get('isLoading') === true, and I cannot do anything about it, including deleting it, because its state is 'loading'.

I am also using ember-django-adapter, so I am unsure if I should report it there, please let me know if you think it's their issue instead of ember-data's. Thanks

@runspired
Copy link
Contributor

We are rejecting the loading promise, but I'm not sure that rejection is being handled to transition the state away from loading anywhere.

cc @pangratz

@runspired
Copy link
Contributor

@dgaus can you check if this is still an issue on master? I cleaned up a lot of the data flow in that area and we may have nicely gotten a win on this because of it.

@dgaus
Copy link
Author

dgaus commented Dec 14, 2016

@runspired Thanks for the reply, however I installed emberjs/data#master (2.12.0-canary+a1bac10f3a) on Ember 2.9.1 and it is still an issue. I couldn't try ember#master since my app broke.

@runspired
Copy link
Contributor

@dgaus thanks! that lets me know it's still an issue, will add this to my TODO list :)

@broerse
Copy link

broerse commented Jan 3, 2017

@runspired I seem to have this issue. To see it in action

  • goto https://bloggr.exmer.com/ and wait for the records to load
  • goto authors
  • goto post again
  • use the filter and type something like 'test'
  • goto authors and see the extra records

Source: https://github.com/broerse/ember-cli-blog

@jlami
Copy link
Contributor

jlami commented Jan 4, 2017

I have a simple reproduction of this problem here: https://github.com/jlami/ember-bug-empty-data

The findRecord does have the unloadRecord in https://github.com/emberjs/data/blob/master/addon/-private/system/store/finders.js#L49
But the _findMany below that does not do anything like that. And the layer calling it doesn't clean up after itself either: https://github.com/emberjs/data/blob/master/addon/-private/system/store.js#L851
Maybe rejectInternalModels should do this?

@broerse
Copy link

broerse commented Jan 4, 2017

@runspired I use the _findMany but @jlami 's reproduction of this problem is a better testcase. @igorT do you have time to take a look?

@broerse
Copy link

broerse commented Jan 9, 2017

Just tested with ember-data 2.11.0 and the bug is not solved.

  • coalesceFindRequests: true, ==> Error
  • coalesceFindRequests: false, ==> No Error

@broerse
Copy link

broerse commented Jan 9, 2017

@runspired I will temporary set coalesceFindRequests: false to fix https://bloggr.exmer.com/ so you can't try the step above to see it in action anymore. The test by @jlami shows exactly what the problem is. Also when you set coalesceFindRequests: false in this example all is well. Tested with ember-data 2.11.0

@BrennanRoberts
Copy link

Another issue with (I think) the same root cause: if you try to do a findRecord on a record which was missing from a coalesced request, it will not attempt to do a network request, as the record is stuck in the isLoading state.

@runspired runspired added the Bug label Mar 17, 2018
@runspired runspired self-assigned this Mar 17, 2018
@carrotalan
Copy link

+1 experienced this issue today also

@scudmissile
Copy link

I've also encountered this issue because our back-end was simply omitting records that the current user in our application doesn't have permissions to see. While it looks like we'll be able to come up with a solution on our back-end, what I think would help is the ability to customize the handling of the rejectInternalModels function (perhaps on a per-adapter basis) so that we could decide how we want to handle the missing records in an appropriate way for our application. In our case, I think we'd want the data that was returned and to display an error for the individual model relationships that couldn't be found.

While turning off record coalescing is part of a work-around for now, it does significantly slow down the requests for us.

@runspired
Copy link
Contributor

we can likely clean up this behavior without introducing any new hooks, just by passing the correct Errors through and cleaning up the state of the records.

@mehulkar
Copy link

In the past didn't we just silently ignore missing records? I think the permissions use case that @scudmissile described is pretty valid. If I make a request for ids 1,2,3 and the server returns only 1 and 2, I would think that could be handled in application code, rather than framework code.

@thec0keman
Copy link

Is there a workaround for this ATM? Running into this on 3.12

@mehulkar
Copy link

mehulkar commented Feb 9, 2020

You can wrap all your findRecords into an RSVP.hashSettled, and then filter out the rejected records. It's not great and won't work if you don't know the IDs you want up front, but it works

@thec0keman
Copy link

Thanks! I don't think that'll work for our use case, since we're relying on async loading across a variety of components on the page.

At this point I think we may just inject the missing records in the adapter's findMany hook (and a missingRecord attribute)... not ideal, but seems like the only option with this use case and the current state of things.

@runspired
Copy link
Contributor

The move to REQUEST_MANAGER (active on master) means that this is far easier to resolve now from the error threading perspective. On the cleanup front, the move to identifiers means we'll be eliminating most of the behaviors around unload retaining state since we no longer need these objects as references.

Rejecting the appropriate requests from the findMany call would be a good starter PR for someone looking to contribute.

@runspired
Copy link
Contributor

Missing records are now properly rejected. #8113 cleaned up this rejection. Removal of InternalModel in #8084 and associated improvements to teardown, coupled with the removal of the state machine mean that this should not only no longer be an issue, but that also any rejections should be resolvable by application code. Happy to open a new issue for a specific case if one presents itself.

@runspired runspired moved this to RFC | Needs Implemented in EmberData Aug 12, 2022
@runspired runspired added 🏷️ bug This PR primarily fixes a reported issue and removed Bug labels Sep 11, 2023
@RobbieTheWagner
Copy link
Member

Is there any way to make this not an error? It definitely seems like we should be able to not throw when we get some things back and not others.

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
Archived in project
Development

No branches or pull requests