getoutreach / epf

A framework for keeping your Ember.js apps in sync.
http://epf.io
MIT License
369 stars 33 forks source link

Some belongs_to relationship changes not persisted #52

Closed ginty closed 10 years ago

ginty commented 11 years ago

Given the following relationship:

App.User = Ep.Model.extend
  profile: Ep.belongsTo("profile")

App.Profile = Ep.Model.extend
  users: Ep.hasMany("user")

If a user already has a profile, then this works:

user.get("profile")    # -> profileA
user.set("profile", profileB)

session.flush()  # -> PUT(user) is made to server for the change

However when the profile is starts off from undefined, then no PUT occurs:

user.get("profile")    # -> undefined
user.set("profile", profileA)

session.flush()  # -> NO PUT(user) is made to server

Similarly changing the profile to null does not update either:

user.get("profile")    # -> profileA
user.set("profile", null)

session.flush()  # -> NO PUT(user) is made to server
jasonkriss commented 11 years ago

I'm unable to recreate this. Can you give some more context on where user, profileA, and profileB are coming from?

ginty commented 11 years ago

Thanks for looking into it.

I am running Ember rc6 and latest EPF.

There is nothing unusual about the relationship setup, it is using the built-in rest adapter settings and is declared in the models as shown above.

The context is not much more than is shown above, the profile ID comes from an edit user form and feeds into this logic on the controller where 'model' is the user instance:

if profile_id
  profile = App.Profile.find(profile_id)
  @get("model").set("profile", profile)
else
  @get("model").set("profile", null)

This all works fine except session.flush() doesn't do anything in the case where the user.profile was originally null or has become null.

I've been trying to work out what is going on but the EPF internals are a bit beyond me at present.

I can say for sure that in the failing case both the user and profile are marked as dirty. The session#flush method definitely has the two dirty models and the problem seems to be that the rest_adapter#flush method returns an empty array of models.

This method starts off with the two dirty models fetched from the session, but somehow in the course of performing the flush they get lost.

I'm not sure if this is significant but my user JSON already had an attribute called 'type', so I have changed this to 'userType' in the model and have this in my adapter:

App.Adapter.map "App.User",
  userType: {key: "type"}

This seems to work, apart from this issue I can CRUD user models quite happily.

jasonkriss commented 11 years ago

I'm not positive this is the issue, but App.Profile.find returns a promise, so that could be causing problems. Can you try this and see what happens:

var user = this.get('model');
if (profile_id)
  App.Profile.find(profile_id).then(function(profile) {
    user.set('profile', profile);
  });
else
  user.set('profile', null);
end
ghempton commented 11 years ago

I think @jasonkriss is correct. Let us know if that works.

ginty commented 11 years ago

I think correctly handling the promise helped a bit, the case where the user.profile starts off as null and is then assigned now works fine. However I still had the problem of the dirty state of the user not being tracked correctly when it started off with a profile which then became null.

I ultimately traced that to something on my side - before the profile was set to null on the user I had this code to remove the user from the profile:

App.Profile.find(profile_id).get("users").removeObject(user)

I didn't really need this in my code and it's a legacy of relationships not really working right in ED the last time I tried. However fair play to EPF, when I take this out it all works perfectly.

I do wonder if this is highlighting a corner case bug though.

The gist of what seems to be happening is that the original profile object goes into the dirty queue first, then the user. The profile goes through the operation#dirtyRelationships stuff first, something happens in there which seems to clear the profile relationship in the user shadow model. Then when it comes to diff'ing the user with the shadow no difference is found since they both have no profile any more.

Anyway its definitely not a problem that I can see a need to hit in reality, so close this as you see fit.

Thanks both for the help.

ghempton commented 10 years ago

I am going to close this issue. Let me know if this is still a problem. I think the starting point for this corner case would be a test.