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

Don't serialize new has many relationships #5324

Merged
merged 1 commit into from Apr 4, 2018

Conversation

ryanto
Copy link
Contributor

@ryanto ryanto commented Jan 12, 2018

Similar to #5317 but for hasMany.

Currently when saving a model with an unsaved hasMany a null id is used in the JSON:API document.

For example

let company = this.store.peekRecord('company', 1);

let user = this.store.createRecord('user');
company.get('users').addObject(user);

company.save();

Generates this post payload (note that we're using serializer option attrs.users.serialize = true)

{
  data: {
    type: 'company',
    id: '1',
    relationships: {
      users: [
        data: {
          type: 'users',
          id: null
        }
      ]
    }
  }
}

This is an invalid payload. To fix this, we'll filter out new records from using in the serialized belongsTo.

@ryanto ryanto force-pushed the dont_serialize_new_has_many branch from a2841d6 to 111c0d6 Compare January 12, 2018 20:39
Copy link
Member

@bmac bmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 9, 2018

Looks good if we don't want to automatically save the new record.
I just wonder in that scenario if we could add an option to send the newly created user together with the company. Not sure it that even makes sense.

@ryanto
Copy link
Contributor Author

ryanto commented Mar 9, 2018

@sly7-7 Yup! I think you're describing a common use case for most applications would want. I'm expecting JSONAPI operations to be the answer to that problem: json-api/json-api#1254

@runspired
Copy link
Contributor

@ryanto not super sure why this failed CI. Could you rebase and let it kick off again?

@ryanto ryanto force-pushed the dont_serialize_new_has_many branch from 111c0d6 to 484e3a6 Compare April 4, 2018 14:32
@ryanto
Copy link
Contributor Author

ryanto commented Apr 4, 2018

All set, I'll keep an eye on CI.

FYI when I pulled the latest and ran yarn install it looks like it added an entry to the lockfile.

@runspired
Copy link
Contributor

victim of a timeout, restarted tests again

@runspired runspired merged commit f3946fb into emberjs:master Apr 4, 2018
@runspired
Copy link
Contributor

Thanks @ryanto !

@juggy
Copy link
Contributor

juggy commented May 2, 2018

There is problem with this bug fix. An empty relationship is not serialized anymore, which is a different behavior than before. @runspired @ryanto

If you have a relation and remove it and save the record, to inform the backend of that relation change, you need to send that empty array back.

@ryanto
Copy link
Contributor Author

ryanto commented May 2, 2018

@juggy Yikes! Sorry about that.

Could you give me a failing test case, or even some example HTTP request/responses with what you expect vs what Ember Data is doing?

@juggy
Copy link
Contributor

juggy commented May 2, 2018

Well the format should be the one of 3.1...

...
"relationships": {
  "my_relations: {
    "data": []
  }
}

@ryanto
Copy link
Contributor Author

ryanto commented May 5, 2018

PR is here: #5466

@ryanto
Copy link
Contributor Author

ryanto commented May 8, 2018

@juggy A fix has been merged, let me know if you run into any other issues.

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

Successfully merging this pull request may close these issues.

None yet

5 participants