-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fixes hasMany records being duplicated after payload returns from server #1656
Conversation
|
||
return postWithoutComment.save(); | ||
})).then(async(function(post) { | ||
equal(post.get('comments.length'), 1, "post has 1 comment"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this fix, post.get('comments.length')
returns 2
here, which means you end up with two records with ID=2
. Pretty weird.
what triggers this edge case?... nm i read the test. |
if(record) { | ||
data.pushObjects(record.get(key).filterBy('isNew')); | ||
if(record && data.length) { | ||
var notDuplicated = function(records) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function should likely not be defined on the fly, as it doesn't close over anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what "doesn't close over anything" means? I believe this is very specific to this function, not a reason for adding more complexity to the store.js
scope. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what "doesn't close over anything" means? I believe this is very specific to this function, not a reason for adding more complexity to the store.js
scope. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that by "close over anything" he is referring to the fact that you do not need a closure (which would allow variables in your scope to be available inside this function). As it is now every time addUnsavedRecords
is called you would be creating a new function. I believe that this might be a performance issue. You could make notDuplicated
a function up out of addUnsavedRecords
with likely the same result as the current code, but without generating a new function very time it is called.
i left some high level comments. I wil try to give it more time soon. |
How do you run into this edge case? I agree that there is a problem here we should fix, but it seems to me that your test case has a lot of potential to leave you in a inconsistent state, where post has a foreign key which points to nothing if the comment doesn't end up being saved. |
My scenario: I have two stores, 1 for offline data (the default) and another for online data. Records have UUIDs, so records never collide (theoretically), so I grab a record from RESTAdapter and do an usual I wish @igorT I wrote the test above just to be able to reach into // cart_test.js
test("#save doesn't duplicate relationships", function() {
stop();
Em.run(function() {
cart.save().then(function(cart) {
equal(cart.get('cartItems.length'), 0, "cart has no items initialy");
cart.get('cartItems').pushObject(cartItem);
cart.set('customer', customer);
return Ember.RSVP.resolve(cart);
}).then(function(cart) {
equal(cart.get('cartItems.length'), 1, "cart has one item once saved");
cart.save().then(function(savedCart) {
equal(savedCart.get('cartItems.length'), 1, "cart continues having one item after being saved again");
start();
});
});
});
}); |
Changed the loop as per @stefanpenner's suggestion. |
You say that your problem appears when you push the record into the store, but in the test you calling createRecord instead of push? I think if you pushed it, you wouldn't have this problem?(I agree that this is a bug, but it seems like you shouldn't be doing createRecord for records you don't expect to have to POST to the server) |
@stefanpenner updated the code :) @igorT I agree with the part about |
Would there be anything I could do to get this merged? |
@igorT the client side UUID scenario is legit let me think through the problem + solution a bit though |
@stefanpenner thank you. My main interest is to be able to push an object without having it duplicated in the store. I believe there are architectural improvements to be made, but outside the scope of this PR. Given I've been working on a forked version of ED for 15 days, I'd love to have the solution for this particular problem merged and then, in a next PR, touch other related points. Please, let me know if that makes sense. |
Ping? This has been referenced multiple times. Maybe it's time for a merge? :) |
|
Having the same issue here +1 |
+1 |
1 similar comment
+1 |
I am about to have to stop using Ember if this isn't merged. No reason was given for not merging it. I understand we all lack time, but no one argumented reasonably against it. It's just making a part of the code more robust, not changing any behavior. Is there a reason for not clicking merge? Please, let us know. Thanks. |
@igorT I was coding in an entirely different problem and, coincidently, stumbled in this exact same duplication issue. Here's another failing test that makes no sense: test("#save doesn't lose relationships from the store", function() {
var phone, person;
stop();
Em.run(function() {
person = store.createRecord('person', { name: "Clint", cool: true });
phone = store.createRecord('phone', { number: 1234 });
console.log("phone", phone.id); // => 'abc'
console.log("person's total phones", person.get('phones.length')); // => 0
person.get('phones').pushObject(phone);
console.log("phone", phone.id); // => 'abc'
console.log("person's total phones", person.get('phones.length')); // => 1
person.save().then(function(person) {
console.log("total phones", person.get('phones.length')); // => 2
// WTF!
console.log("phone1's id", person.get('phones.firstObject.id')); // => 'abc'
console.log("phone2's id", person.get('phones.lastObject.id')); // => 'abc'
// WTF!
equal(person.get('phones.length'), 1, "person has 1 phone");
// test would continue...
start();
});
});
}); With all due respect, this is a serious bug. After 5 months waiting and patching my own Ember Data, I can't think of any reason why this wasn't merged yet. What deep analysis is there to be done? Seriously, I could be sending PRs all the time, helping Ember Data reach its 1.0 milestone, but I simply decided to stop my efforts for the last 5 months because I simply lost faith. Please, be kind and just click the green button. Or at least tell me what's wrong with this. All I want is to help. |
I think this makes sense. It will be fully fixed nicely by SSOT, because relationships are going to be a set, so they are not going to be able to have the same record twice. Part of the code conflicts with #1939 , I am happy to merge this once we figure out whether we want to use Ember.A or not |
There is an edge case where a hasMany record gets duplicated (same ID). Basically, if we generate a record locally using UUID or any custom ID format, then push it to production which will return the same ID previously defined, the store's array that holds records to be resolved is going to duplicate both items, even if they have the same ID. This commit fixes that by not allowing duplicated IDs in that array.
OMG OMG It's passing! |
I made couple of cleanups and resubmitted your PR as #1976 I have kept you as the commit author though :) WIll double check the correctness of Ember.A and merge tomorrow |
It's merged in master now! |
Awesomeness! :) |
Omg omg @igott, you're the best! |
Is anyone able to confirm whether or not this has been fixed as intended? I'm using the current master branch and find the same behaviour continues: I have a controller action that does: And in another controller action: My comments (hasMany) relationship then shows two records of duplicated data with the exception of the id. 'comments.lastObject' has an id of null (I expect this is the original record added with the pushObject) and the second containing the id of the comment record returned by the Rails app. I'm very new to Ember/Ember-data, so please excuse my ignorance if I've done something wrong, but I've spent the past 5 hours trying to figure this out. |
Same for me... I need to destroy and remove the duplicate object manually. |
@klopfer1 is your scenario the same as this issue, or is it different |
Not exactly, I have a parent, with many embedded childs. When I create a new child and save it, I have the new child twice. One with ID from the backend and one with ID: null. As far as I understood this PR should have solved this. |
@klopfer1 can you open a new issue then. One that tries to clearly outline the specific scenario. Also, if you have the time a PR with a failing test is often the easiest way to fix bugs. |
This PR solves two records with the same ID (e.g UUID). Basically, a record returned from backend with an ID of a record that already existed in the store would get duplicated. I don't think this covers null IDs. On Wed, Aug 20, 2014 at 10:52 AM, Gerrit S. notifications@github.com
|
you are right. I think its related to this #1656 (comment) |
It seems like this issue has regressed and is present in 1.13.14 |
Getting the same bug with null IDs: Ember : 2.3.0 |
FYI @igorT fixed this in another branch 1,5h years ago. Given this is a PR, I suggest opening a new issue and linking back to here. |
There is an edge case where a hasMany record gets duplicated (same ID).
Basically, if we generate a record locally using UUID or any custom ID
format, then push it to production which will return the same ID
previously defined, the store's array that holds records to be resolved
is going to duplicate both items, even if they have the same ID.
This commit fixes that by not allowing duplicated IDs in that array.
I need this badly for my app.