Zombie record when id query mismatch #1831

bcardarella · 2014-03-23T23:05:33Z

If I query for a record of id 1 and the server returns a record of id
2 the store will have records of id 1 and id 2 as Ember Data
pre-emptively instantiates the record you are querying for.

A common use-case where this occurs is when querying for records with
a slug for an id. If I have a slug library server side for a Rails
application I do not want to just map my id column for the slug because
all of the relationships and foreign keys in my database are using the
auto-incrementing id column, not the slugs. The slugs are there for
presentation purposes only. However, on the client I will sometimes only
have a slug as the unique identifier to search for a given record on the
server.

This PR will do the following:

If you query the server for id 1 and it returns a record of id 2 it
will delete the previously instantiated record of id 1.

If you already have a pre-existing record of id 1 that is committed
and query the server for id 1 and the server returns a record of id
2 it will not delete the record of id 1.

If I query for a record of id `1` and the server returns a record of id `2` the store will have records of id `1` and id `2` as Ember Data pre-emptively instantiates the record you are querying for. A common use-case where this occurs is when querying for records of with a slug for an id. If I have a slug library server side for a Rails application I do not want to just map my id column for the slug because all of the relationships and foreign keys in my database are using the auto-incrementing id column, not the slugs. The slugs are there for presentation purposes only. However, on the client I will sometimes only ahve a slug as the unique identifier to search for a given record on the server. This PR will do the following: If you query the server for id `1` and it returns a record of id `2` it will delete the previously instantiated record of id `1`. If you already have a pre-existing record of id `1` that is committed and query the server for id `1` and the server returns a record of id `2` it will **not** delete the record of id `1`.

stefanpenner · 2014-03-23T23:31:50Z

seems ok as a short-term fix.

We really need to refactor the internals to just not create these zombie records...

bcardarella · 2014-03-24T01:57:31Z

jaaksarv · 2014-03-25T11:33:04Z

I have similar "zombie record" problem with belongsTo relationships. Will it fix it also?
#1810

igorT · 2014-05-17T22:54:53Z

packages/ember-data/lib/system/store.js

@@ -448,6 +448,13 @@ Store = Ember.Object.extend({
    Ember.assert("You tried to find a record but your adapter (for " + type + ") does not implement 'find'", adapter.find);

    var promise = _find(adapter, this, type, id);
+
+    promise.then(function(newRecord) {
+      if (!record.get('isNew') && newRecord.get('id') !== record.get('id')) {


It seems like this would work if you created it locally, but what happens if you loaded it from the server? It still gets deleted, but that actually seems like a feature, because it enables you to locally delete things that were deleted on the server, though in a crappy way.

igorT · 2014-05-17T22:56:34Z

👍 overall

bcardarella · 2014-05-18T08:31:51Z

@igorT I am OK making the changes if you'd like but it seemed from @stefanpenner's comments that the better solution is to actually refactor the internals to prevent zombie records from being created in the first place.

jaaksarv · 2014-05-18T13:25:26Z

I also vote for refactoring internals so zombie records are not created at all.

igorT · 2014-05-20T10:20:19Z

I think if you express to the store that you believe that there exists a record with a certain id, it is ok to create that record locally and put it in a loading state. Once SSOT works, that record will also have relationship data on it if there are any relationships pointing towards it. If we remove the unloaded/loading record state, we will have to figure out how to guard against multiple finds triggering multiple GET requests, how to deal with a reloading record, etc. I am not convinced that the benefit is worth it to deal with this edge case. @stefanpenner thoughts?

stefanpenner · 2014-05-20T12:22:46Z

one approach is to have an identity map the promise of find('user', 1) based on user + 1 + ('GETorfind`) and only create the record once we get the payload and build the record.

This both fixes the zombie record problem, and caches multiple requests for the same resource.

igorT · 2014-05-20T12:27:35Z

Having the unloaded record that can have partial data on it is something I view as a feature.

stefanpenner · 2014-05-20T12:28:27Z

@igorT which feature? If the record is not yet realized, it may have falsey data as the server will respond with new data. If you have already created relationships based on a faulty idea, changing the id (when it returns from the server) wont propagate correctly. What is the downside to not having a tangible record until it is actually realized?

igorT · 2014-05-20T12:29:57Z

The fact that you can have an unloaded record that has an id, but no payload yet. It allows you to manipulate it in and out of relationships. I don't see how we can support that if we lazily create the records only after find returns.

stefanpenner · 2014-05-20T12:35:23Z

unload would merely drop the promise from the identity map

Manipulating its existence within arrays before the server acknowledges its existence seems dubious at best. Even this exact issue would fail horribly if the ID changed after the data has arrived if relationships have been created based on the originating ID.

During a find, I don't believe we should treat an object as real until it has returned from the server.

igorT · 2014-05-20T12:36:41Z

It's a often requested and a strong use case I feel. The ability to do

parent2.set('child, parent1.get('child))

without having to fetch child.

igorT · 2014-05-20T12:39:37Z

I think the strongest use case is that you got the id as the foreign key, in which case we still need the concept of unloaded records. Creating two different code paths one for user initiated find, and the other one for calling find from a belongsTo seem very complex.

bcardarella · 2014-05-20T12:57:08Z

I would be reluctant to classify this as an edge case. Using slugs is a
fairly common thing in web apps.

On Tuesday, May 20, 2014, Igor Terzic [email protected] wrote:

I think the strongest use case is that you got the id as the foreign key,
in which case we still need the concept of unloaded records. Creating two
different code paths one for user initiated find, and the other one for
calling find from a belongsTo seem very complex.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1831#issuecomment-43619790
.

Brian Cardarella
CEO of DockYard
Visit us: http://dockyard.com
Call us: (855) DOCK-YRD
Follow me on Twitter: http://twitter.com/bcardarella
Follow us on Twitter: http://twitter.com/DockYard

igorT · 2014-05-20T13:00:09Z

If you are querying by something that you know is not an id, maybe you should use findQuery? Or some equivalent of that that allows for overwriting urls nicely.

igorT · 2014-05-20T13:05:14Z

I think this is a valuable use case to add to the url discussion: http://discuss.emberjs.com/t/nested-urls-for-ember-datas-restadapter-proposal/5290/ Your url is different than the canonical :type/:id and is basically :type/:slug Though I am not sure how to support records with null ids just yet.

igorT · 2014-05-20T13:06:56Z

Do your backends support both the /slug and the /id routes? As currently the record that you get with the actual id will go to the /id route once you try saving it/reloading it.

bcardarella · 2014-05-21T16:20:07Z

@igorT in Rails there are slug libraries that allow you to do:

Person.find(params[:id])

The find method is overridden to query by either the id or the slug so you can interchange both.

fivetanley · 2014-08-13T18:28:14Z

@bcardarella @igorT worth revisiting but not holding the v1.0.0-beta.9 release on this.

igorT · 2015-06-08T05:39:53Z

We now have a solution for this, @bmac has a PR, #3198 so closing this PR

igorT reviewed May 17, 2014
View reviewed changes

igorT mentioned this pull request May 28, 2014

Model ID may be modified? #1545

Closed

Dhaulagiri mentioned this pull request Sep 5, 2014

Phantom record Dhaulagiri/live-scoring#24

Closed

fivetanley force-pushed the master branch from b8af0ea to e588cf7 Compare September 9, 2014 21:42

lacek mentioned this pull request Sep 23, 2014

Unable to create record once find has been called #2150

Closed

igorT closed this Jun 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zombie record when id query mismatch #1831

Zombie record when id query mismatch #1831

bcardarella commented Mar 23, 2014

stefanpenner commented Mar 23, 2014

bcardarella commented Mar 24, 2014

jaaksarv commented Mar 25, 2014

igorT May 17, 2014

igorT commented May 17, 2014

bcardarella commented May 18, 2014

jaaksarv commented May 18, 2014

igorT commented May 20, 2014

stefanpenner commented May 20, 2014

igorT commented May 20, 2014

stefanpenner commented May 20, 2014

igorT commented May 20, 2014

stefanpenner commented May 20, 2014

igorT commented May 20, 2014

igorT commented May 20, 2014

bcardarella commented May 20, 2014

igorT commented May 20, 2014

igorT commented May 20, 2014

igorT commented May 20, 2014

bcardarella commented May 21, 2014

fivetanley commented Aug 13, 2014

igorT commented Jun 8, 2015

Zombie record when id query mismatch #1831

Zombie record when id query mismatch #1831

Conversation

bcardarella commented Mar 23, 2014

stefanpenner commented Mar 23, 2014

bcardarella commented Mar 24, 2014

jaaksarv commented Mar 25, 2014

igorT May 17, 2014

Choose a reason for hiding this comment

igorT commented May 17, 2014

bcardarella commented May 18, 2014

jaaksarv commented May 18, 2014

igorT commented May 20, 2014

stefanpenner commented May 20, 2014

igorT commented May 20, 2014

stefanpenner commented May 20, 2014

igorT commented May 20, 2014

stefanpenner commented May 20, 2014

igorT commented May 20, 2014

igorT commented May 20, 2014

bcardarella commented May 20, 2014

igorT commented May 20, 2014

igorT commented May 20, 2014

igorT commented May 20, 2014

bcardarella commented May 21, 2014

fivetanley commented Aug 13, 2014

igorT commented Jun 8, 2015