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

Empty hasMany relationship not sent to server by model.save() #5539

Closed
les2 opened this issue Jul 24, 2018 · 17 comments
Closed

Empty hasMany relationship not sent to server by model.save() #5539

les2 opened this issue Jul 24, 2018 · 17 comments

Comments

@les2
Copy link

les2 commented Jul 24, 2018

While QAing our next release and we had to downgrade ember-data to 3.1.1 from 3.2.0 because of a weird bug.

When updating a hasMany relationship to an empty array and called .save() on the owning model, the empty array is not sent to the server.

example:

const Child = Model.extend({
   parent: belongsTo('parent', { async: false })
})

const Parent = Model.extend({
   items: hasMany('child', {async: false})
})

... later ...

const model = store.findRecord('parent', 99);
model.get('items').mapBy('id'); // ['1', '2', '3']

model.set('items', A());
model.save();

In 3.1.1, this worked --- an empty relationship would be sent to the server.

After upgrading to 3.2.0, it not longer sends the empty relationship.

Unfortunately, I haven't been able to put together a simple repo recipe on github - I only have the rather complicated example that is our application (which I can't share for obvious reasons).

I have to leave the office right now but will try to get some more details in this issue when I can (probably tomorrow).

That said, please be careful upgrading to 3.2.0 :)

@runspired
Copy link
Contributor

@ryanto did your work land prior to 3.2 or is this related to it?

@les2
Copy link
Author

les2 commented Jul 25, 2018

@runspired @ryanto I suspect that this line is to blame. you can see it in the diff from 3.1.1 to 3.2.0.

        // only serialize has many relationships that are not new
        let nonNewHasMany = hasMany.filter(item => item.record && !item.record.get('isNew'));

        if (nonNewHasMany.length > 0) { // <---- BUG HERE -- this line was added
          json.relationships = json.relationships || {};
             ...
            json.relationships[payloadKey] = { data }; // <---- when the hasMany relationship is empty, an item is never added here
          }
        }

To fix it, we could add logic for the case where the relationship is completely empty and then add an empty data: [] item to the relationships hash.

Aside

While looking at the code, I also observe that isNew items are not serialized. Seems like we should warn the user that the relationships aren't going to be persisted because they are isNew, or even throw a fatal error while in dev mode.

@les2
Copy link
Author

les2 commented Jul 25, 2018

@ryanto @runspired Let me know if you'd like me to get a PR up - I'd love to contribute.

@ryanto
Copy link
Contributor

ryanto commented Jul 25, 2018

Edit: Opps, I think I wasn't understanding this.

@les2 does this PR fix the issue? #5466

If not, do you mind adding a failing test? There's a few example has many tests you can copy from in the jsonapi serializer

@runspired I think my fix was post 3.2

@urbany
Copy link

urbany commented Jul 25, 2018

@ryanto @runspired I looked at the commits and could only find that fix in v3.4.0-beta.1
Might be a good idea to release a v3.3.1 with that fix, alongside #5541

@urbany
Copy link

urbany commented Jul 26, 2018

3.3.1 was just released but this is still not fixed, you have to use 3.4.0-beta.2

@runspired
Copy link
Contributor

runspired commented Jul 27, 2018

@urbany 3.3 was a re-release of 3.2. They are and will effectively remain the same release.

(That's not to say we might not backport this, just why this was done post 3.2 and yet not in 3.3)

@ryanto
Copy link
Contributor

ryanto commented Jul 27, 2018

Here are the PRs merging the fix back into...

3.2: #5546

3.3: #5547

@olivierchatry
Copy link

seems like 3.4 is also impacted. I have the same issue, and emberdata is clearly not serializing empty hasmany.

@ryanto
Copy link
Contributor

ryanto commented Sep 2, 2018

Hi @olivierchatry,

Have you enabled serializing the hasMany relationship in your serializer?

export default ApplicationSerializer.extend(, {
  attrs: {
    hasManyRelationshipNameHere: { serialize: true }
  }
});

@olivierchatry
Copy link

olivierchatry commented Sep 2, 2018

Hello @ryanto !

I think it does serialize it, but as soon as there is not more element in the relationships, it does not serialized the relationships anymore, even if there was one before. So the partial update, on the server side, does not remove the last element.

edit: removed last comment, was looking as the wrong pane of the network debugger :)

@olivierchatry
Copy link

Haaa wait, my fault, I override a function in my app that was producing the bug. Sorry !

@ryanto
Copy link
Contributor

ryanto commented Sep 14, 2018

Thanks @runspired !

Looks like the fix was released in 3.1.2 and 3.3.2. If you're running into this, give those versions a try.

@fran-worley
Copy link

fran-worley commented Sep 26, 2018

@ryanto 3.1.2 seems to work, but I still have the same issue in 3.3.2. @runspired can you confirm what the situation is with back-porting a fix to 3.3.x ? If it helps, I can confirm that the issue is fixed in v3.4.2

@jfschaff
Copy link

Seems like I am having this issue in v3.4.4 (but not systematically on all models)...

@acdn-tsmith
Copy link

Also having the issue on 3.25

@runspired
Copy link
Contributor

@acdn-tsmith its (more than) likely a different issue. A test PR or demo app/twiddle replicating the issue is welcome. If you can reproduce it please open a new issue for us!

Since serializers are predominantly an app-code (vs lib-code) concept, there's a relatively good chance you're seeing this due to code added or modified in your serializer. If you can't replicate without serializer modifications then that's something to look into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants