[BUGFIX] extract polymorphic belongsTo in RESTSerializer #3835
Conversation
| // corresponding JSON-API representation. The former case is handled within | ||
| // the super class (JSONSerializer). | ||
| var isPolymorphic = relationshipMeta.options.polymorphic; | ||
| var typeProperty = `${relationshipKey}Type`; |
There was a problem hiding this comment.
I think this should probably go through keyForAttribute. I think we had a bug related to this previously. It would also make the ActiveModelSerializer work out of the box... I think.
There was a problem hiding this comment.
relationshipKey is the result of this.keyForRelationship(key, relationshipMeta.kind, 'deserialize'); from this line.
In serializePolymorphicType above in line 686 keyForAttribute is used, so using keyForAttribute here would make it consistent I guess. But judging by the name, wouldn't be keyForRelationship the correct one, for both serializePolymorphicType and the new serializePolymorphicType?
It looks like AMS uses `keyForRelationship ...
There was a problem hiding this comment.
Agree, keyForRelationship makes more sense.
There was a problem hiding this comment.
Just to make sure, my intention here was to do keyForAttributekeyForRelationship on the typeProperty string.
Even though relationshipKey is cased correctly that doesn't mean we can just append Type to the end of it.
Say you have a bestFriend: belongsTo() and you've overriden keyForRelationship to returned dasherized. bestFriend is now best-friend. Odds are your payload contains best-friend-type, not best-friendType.
|
@pangratz could you update the commit message to start with |
| }); | ||
|
|
||
| typeKey = key; | ||
| } |
There was a problem hiding this comment.
The current way to get the myKeyType string for the myKey relationship is using keyForAttribute which is not correct IMHO. This is changed in the course of this PR by adding a keyForPolymorphicType hook.
This could break current behavior, that's why a deprecation message is added. I will add a test for the deprecation if the deprecation is found to be necessary.
There was a problem hiding this comment.
Actually, I think this is save without the deprecation, since there wouldn't have been another way to change the way the polymorphic record is serialized except for overriding serializePolymorphicType as a whole...
There was a problem hiding this comment.
@pangratz it looks like a user can't use keyForPolymorphicType if it differs from ${key}Type; since key always wins. Is that correct? Is there anyway we could rewrite this logic so uses who define a custom keyForPolymorphicType get the new happy path without a deprecation warning?
There was a problem hiding this comment.
Uff, you're absolutely right. Nice catch! Lol, logic, how do you even work. I will change the code so the deprecation warning is only logged when keyForPolymorphicType is not overwritten...
This change adds the correct extraction of a polymorphic belongsTo
specified in the payload in the following form:
``` js
{
id: 123,
// ...
message: 1,
messageType: 'post'
}
```
where the model is specified as:
``` js
DS.Model.extend({
message: DS.belongsTo('message', { polymorphic: true })
})
```
---
This commit introduces a new hook `keyForPolymorphicType` which can be
overwritten to customize the key which holds the polymorphic type.
Currently `keyForAttribute` is used to get the key under which the type of a polymorphic record is serialized. This is not correct, as the new `keyForPolymorphicType` hook should be used. This commit uses the new hook and falls back to the previous vesion, if the key generated via the old method (keyForAttribute) is different to the new version and the `keyForPolymorphicType` has not been overwritten. A deprecation warning is logged if this is the case.
|
@bmac I've updated the logic and added a check which ignores the fallback when the |
[BUGFIX] extract polymorphic belongsTo in RESTSerializer
|
Thanks @pangratz. |
This change adds the correct extraction of a polymorphic belongsTo
specified in the payload in the following form:
where the model is specified as:
If this gets accepted, this should likely be ported back to
1.13as well.This closes #3806