Dont serialize new belongsTo records

[ryanto]

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

For example

let user = this.store.createRecord('user');
let details = this.store.createRecord('user-details', { user });

user.save();

Generates this post payload:

{
  data: {
    type: 'users',
    id: '1',
    relationships: {
      user-details: {
        data: {
          type: 'user-details',
          id: null
        }
      }
    }
  }
}

According to the spec this is an invalid payload:

This PR addresses this by not including the belongsTo that is unsaved in the relationship key.

I took this existing PR and updated it to fit @wecc's comment: #4500. Big thanks to @pangratz

[wecc]

LGTM 👍

[bmac]

Thanks @ryanto.

[travisghansen]

How does this impact the embedded records mixin? Or those implementing their own rendition of it? While it does break spec, the idea seems to be the major issue being addressed by version 1.1 of the spec and seems to be the general consensus of how it would be solved (sending not only id/type but also attributes). The same principle applies to hasMany as well.

Having said that, my team have broke spec and rely on this behavior. While we're not running this version yet I suspect it will break us badly. If there's any way we could re-introduce the existing behavior via a flag or similar that would be extremely helpful. Thanks!

[ryanto]

@travisghansen Yikes! Sorry to hear this might break your app. I'm happy to help you work out a fix. Do you mind posting what payload your app is sending before this change, and what payload your app is sending with this change?

Version 1.1 of the JSON:API spec addresses multiple operation support and will be solved differently from the bug this PR originally addressed.

[travisghansen]

@ryanto yeah I'll have a look (currently on 2.11 so I won't be able to upgrade immediately, but I'll make the hack in 2.11 as a start). What I'm describing is more than multiple operation support (I believe). This is the unique situation where the it's a parent/child style relationship but also wanting to be atomic (at the API layer at least). It raises a difficult question where the parent doesn't actually exist so the children can't bind preemptively (ie: an invoice/PO/whatever with line items).

Regardless, thanks for the receptive response! As soon as I have a moment I'll apply the same logic to our existing code base and respond with examples.

[travisghansen]

@ryanto ok, I've done some testing and this doesn't appear to impact us (we're hijacking serializeBelongsTo). For some reason I was thinking this would impact the serialize method but clearly it doesn't. I think we're good for now. We're going to start to upgrade process in another week or so and I'll shout out with more info should it be necessary. Thanks again for the response and help!

[ghost]

This change broke functionality in our app too, probably because we have a custom solution for allowing multiple self-referencing records to be created in one atomic request to the backend. We make this work by using UUIDs generated on the frontend combined with async relationships, and sideload all records to be updated/persisted as part of the POST. Because of this we need to serialize the ID for the belongsTo of a new record, although given that it is async and has had a value set this seems like a reasonable thing for ED to be doing by default - because it has an ID to serialize.

Thoughts?

We'll revert to ED 3.0.1 for now, and maybe look at overriding serializeBelongsTo().

How far through is the JSON API 1.1 spec? Is it still at an RFC stage?

[runspired]

@lsg-richard you should absolutely override serializeBelongsTo. The default is just that, a default. It is not expected that it will work for everyone.

[fotinakis]

Just jumping in to say this also broke some serious things for us. We ended up fixing it by using store.push instead of createRecord so that the record we wanted in the store would not be marked as isNew anymore.

[runspired]

@fotinakis why wouldn’t you just implement a serializer hook that serializes them if your API supports it? Or were these actually existing records?

[fotinakis]

@runspired Was easier to fix the other way. They are simple records that we can assume exist on the backend by ID, so we happened to use createRecord to make them and associate them to the other object we were creating. They also don't have an API endpoint backing them.

[runspired]

If the data can be assumed to be canonical then using push is the correct thing to do. 😀

[ghost]

Just been looking at how to upgrade from [email protected] in light of this change, and wondering if changing addon/serializers/json-api.js:488 (as of [email protected]) from

let belongsToIsNotNew = belongsTo && belongsTo.record && !belongsTo.record.get('isNew')

to

let belongsToIsNotNew = belongsTo && belongsTo.record && !isBlank(record.get('id'))

would be considered?

The rationale is that if there's an ID set then we're in a position to return valid JSON API, even though yes technically the record may be in the isNew state.

[ryanto]

@lsg-richard good suggestion, I think that will work! I'm happy to make the change, @runspired thoughts?

@fotinakis Are you creating records with an id property as well?

[ghost]

This suggested change would also apply to serializeHasMany() which as of 3.2.0 is now doing an equivalent check on each item being serialized. i.e. the following line:

let nonNewHasMany = hasMany.filter(item => item.record && !item.record.get('isNew'));

would become

let nonNewHasMany = hasMany.filter(item => item.record && !isBlank(item.record.get('id'));

In this case I wonder if, for API consistency with shouldSerializeBelongsTo() and shouldSerializeHasMany(), there should also be a shouldSerializeHasManyItem() function, in which case the above would become:

let nonNewHasMany = hasMany.filter(item => this.shouldSerializeHasManyItem(snapshot, key, relationship, item));

...

shouldSerializeHasManyItem(snapshot, key, relationship, item) {
  return item.record && !isBlank(item.record.get('id'))
}

The latter may be a step too far for now, as it adds a new public API method, but it does feel consistent with the existing callbacks. The app I work on already adds its own system along these lines for doing custom filtering on hasMany serializations in order to optimize payload size in app-specific ways, which is partly what prompts me to make this suggestion.

[fsmanuel]

@lsg-richard @ryanto I would support the suggestion:

let belongsToIsNotNew = belongsTo && belongsTo.record && !isBlank(record.get('id'))

I'm having problems with my autoSave implementation for ember-local-storage. As it is a client side implementation I use generateIdForRecord. With the changes introduced in this PR that no longer works. The record has the ID but is still in isNew state. So a save will lose the relationship.

[phil294]

What is the roadmap here? Should an option (persistNewBelongsTo etc.) be added? Or should everyone needing this functionality override the serializeBelongsTo method?

We are struggling with this too, if I counted correctly as the fifth affected here.

[runspired]

@phil294 you should override serializeBelongsTo