Use modelNameFromPayloadKey when type is given #4194
Conversation
|
I think it would be good to add a test for the correct serializer too. This is an example of using a custom serializer in the tests. Also, I think this is a bugfix. Can you prefix the commit with |
| }; | ||
|
|
||
| var jsonHash = { | ||
| "home-planets": [{ id: "1", name: "Umber", type: "home-planets" }] |
There was a problem hiding this comment.
What do you think of changing type: "home-planets" to type: "my-custom-type" to make it more clear that modelNameFromPayloadKey is used to resolve the model and serializer?
backspace
force-pushed
the
modelNameFromPayloadKey-with-type
branch
from
aca7096 to
3d89ecd
Compare
|
Thanks for the pointer to an example serialiser override. I’ve overwritten the commit with a similar assertion for that and added the |
| serializer = store.serializerFor(hash.type); | ||
| modelClass = store.modelFor(hash.type); | ||
| serializer = store.serializerFor(this.modelNameFromPayloadKey(hash.type)); | ||
| modelClass = store.modelFor(this.modelNameFromPayloadKey(hash.type)); |
There was a problem hiding this comment.
I just realized this now, but at a first glance it looks weird the the modelName is normalized by a function called modelNameFromPayloadKey; since the passed argument is technically not a key from the payload.
That function originally has been added for the case where the payload key is different. So the blog/post entry in the following payload is used for the post model (see this documentation):
{
"blog/post": {
"id": 1
}
}modelNameFromPayloadKey is however already used within json serializer to normalize the modelName from hash.type (see here); so this would not be the first time here that this function is used differently than the name would imply.
To be clear, I think it is definitely a bugfix to use the normalized modelName within the if branch here, so this PR looks good to me. I am just not sure if there should be a different function (like modelNameFromPayloadType or something), which would be more appropriate...
There was a problem hiding this comment.
hmm yes, I see the confusion. The API that led me to find this bug is a weird pseudo-JSON API that was settled during the RC phase, it uses type. I was confused while debugging by the role that JSONSerializer inheritance played, but you probably know better than me whether it’s feasible to change the method name.
|
I see that this has conflicts now. Is it worth rebasing to keep updated? Might it eventually be merged? |
|
So this has been discussed in the team meeting. Eventually the inconsistency should be addressed via new additional @backspace can you rebase this PR and fix the merge conflicts? After that, this is good to go 🚀 |
The code checked for a model using the possibly-overridden modelNameFromPayloadKey but didn’t use the override when getting the serialiser and model.
backspace
force-pushed
the
modelNameFromPayloadKey-with-type
branch
from
3d89ecd to
0f46f66
Compare
|
Thanks for the update on the inconsistency. Rebased and ready! 😀 |
…type Use modelNameFromPayloadKey when type is given
|
🎉 👯 thanks @backspace! |
The code checked for a model using the possibly-overridden
modelNameFromPayloadKeybut didn’t use the override when getting the serialiser and model.The test is really only verifying the
store.modelFor(this.modelNameFromPayloadKey(hash.type));change, not the serialiser one, since I didn’t see any tests for custom serialisers in this file. Let me know if I should make a test for that regardless.