[BUGFIX] EmbeddedRecordMixin should include the type serializing hasMany as ids #3848
Conversation
|
@cibernox I don't think we can merge this as is without potentially breaking backwards compatibility. It would be better if there were a flag to opt into this behavior, possibly changing |
|
I completely forgot about this PR 😓 Then, would you the accept then a 3rd serializing strategy called |
|
I think we would accept a 3rd serializing strategy called |
|
Nahh, that name is as explicit at is can possibly be. I'll try to allocate some time over the weekend for this. |
|
@cibernox ping? |
cibernox
force-pushed
the
fix_bug_serializing_polymorphic_has_many_as_ids
branch
2 times, most recently
from
5c8abe8 to
90c0ae7
Compare
|
Santa Claus left this PR under the tree 👍 |
|
@igorT any comments on this PR? I will have some free time this weekend in case something needs to be changed |
|
@cibernox do you mind wrapping the changes in this pr in a feature flag? |
cibernox
force-pushed
the
fix_bug_serializing_polymorphic_has_many_as_ids
branch
from
90c0ae7 to
c191b7d
Compare
|
Flagged under |
| ``` | ||
|
|
||
| This is particularly useful for polymorphic relationships not backed by STI when just including the id | ||
| of the records is not enought. |
Maybe a solution would be to mention in the comment that the functionality is only available when the |
cibernox
force-pushed
the
fix_bug_serializing_polymorphic_has_many_as_ids
branch
from
c191b7d to
567ef9f
Compare
|
Applied feedback, and also tagged the commit as |
|
ping? |
cibernox
force-pushed
the
fix_bug_serializing_polymorphic_has_many_as_ids
branch
2 times, most recently
from
0f38e57 to
550ec55
Compare
|
Rebased and green. |
|
cc @igorT we should look at this tomorrow. |
…edding strategy for non STI polymorphic hasMany
Taking as example this scenario:
```js
User = DS.Model.extend({
name: DS.attr('string'),
pets: DS.hasMany('pet', { polymorphic: true })
});
Pet = DS.Model.extend({
name: DS.attr('string'),
});
Cat = Pet.extend({
// ...
});
Parrot = Pet.extend({
// ...
});
```
As of today, when using the `DS.EmbeddedRecordsMixin` in a serializer and configuring
the serialization stategy like this:
```
attrs: {
pets: { serialize: 'ids' }
}
```
The relationship is serialized:
```json
{
"user": {
"id": "1"
"name": "Bertin Osborne",
"pets": [1,2]
}
}
```
This works ok if the polymorphism is based on STI, but that is just one specific implementation
does not cover all use cases. Probably when a hasMany relationship is polymorphic the serialization
should generate an array of object containing `id` and `type` by default, but at this point this
can't be changed because it would break apps in the wild.
Because of that a new serialization strategy named `ids-and-types` has been created that covers this
use case.
Probably this should become the default behavior of the `ids` strategy in ember 3.0, but not for now.
For the same example above, this stragegy would generate the following payload:
```js
{
"user": {
"id": "1"
"name": "Bertin Osborne",
"pets": [
{ "id": "1", "type": "Cat" },
{ "id": "2", "type": "Parrot"}
]
}
}
```
Note that with this strategy if the differenty type of records don't whare the same ids space,
that is not a problem.
```js
{
"user": {
"id": "1"
"name": "Bertin Osborne",
"pets": [
{ "id": "1", "type": "Cat" },
{ "id": "1", "type": "Parrot"} // Same id, but different type
]
}
}
```
cibernox
force-pushed
the
fix_bug_serializing_polymorphic_has_many_as_ids
branch
from
550ec55 to
7989453
Compare
|
Rebased. |
…c_has_many_as_ids [BUGFIX] EmbeddedRecordMixin should include the type serializing hasMany as ids
|
👍 |
It is better explained with an example.
Lets say that that you have an error that contains many attachable items
(
attachables: DS.hasMany(‘attachable’, { polymorphic: true })) and the serializer looks like this:At the moment the serialized json contains
attachables: [1,2,3, ...]but that is useless for polymorphic relationships, the serialized array should contain{id, type}tuples (attachables: [{type: 'pdf', id: 1}, {type: 'xls', id: 1}])