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

RESTSerializer.pushPayload does not allow sideloading of records #3090

Closed
amk221 opened this issue May 22, 2015 · 18 comments
Closed

RESTSerializer.pushPayload does not allow sideloading of records #3090

amk221 opened this issue May 22, 2015 · 18 comments

Comments

@amk221
Copy link
Contributor

amk221 commented May 22, 2015

Given a payload like:

{
     posts: [{}]
     _posts: [{}, {}]
}

I get this warning:
WARNING: Encountered "_posts" in payload, but no model was found for model name "-post" (resolved model name using blah@serializer:post:.typeForRoot("_posts"))

1.0.0-beta.18

@fivetanley
Copy link
Member

You should remove the underscore in the payload or override modelNameFromPayloadKey (we need to update the message it looks like).

Ember.String.pluralize('foo') == 'foos'.

@amk221
Copy link
Contributor Author

amk221 commented May 26, 2015

Quote from this page:
https://github.com/emberjs/data/blob/master/CHANGELOG.md#ember-data-100-beta3-september-28-2013

"Support primary and sideloaded data of the same type to be returned from array lookups (via posts and _posts)."

@fivetanley this looks like a regression?

@wecc
Copy link
Contributor

wecc commented May 26, 2015

@amk221 You are correct. Sideloading secondary records of the same type as primary is supported in RESTSerializers extractArray using _type as key.

However, I'm unable to reproduce the error, see http://emberjs.jsbin.com/riyevivoxi/1/edit

Would you be able to provide a JSBin demonstrating the error?

@wecc wecc reopened this May 26, 2015
@amk221
Copy link
Contributor Author

amk221 commented May 27, 2015

Ah, you're right

But, put that exact same code in a fresh ember-cli project (0.2.5) and it doesn't work

@amk221 amk221 closed this as completed Aug 3, 2015
@amk221 amk221 reopened this Sep 22, 2015
@amk221
Copy link
Contributor Author

amk221 commented Sep 22, 2015

I closed this a while ago, but realised it's still happening

In a jsbin/fiddle/twiddle:

store.modelFactoryFor('post') // function .Post
store.modelFactoryFor('_post') // function .Post

In an ember-cli app

store.modelFactoryFor('post') // function my-app@model:post:
store.modelFactoryFor('_post') // undefined

@wecc
Copy link
Contributor

wecc commented Sep 22, 2015

This seem unrelated since the RESTSerializer removes the prefixing _ when sideloading resources of the same type as the primary.

When does store.modelFactoryFor('_post') occur?

@amk221
Copy link
Contributor Author

amk221 commented Sep 22, 2015

for (var prop in payload) {  // prop = '_posts'
      var modelName = this.modelNameFromPayloadKey(prop); // modelName = '-post'
      if (!store.modelFactoryFor(modelName)) {  // modelFactoryFor('-post')
        Ember.warn(...
// ...

It's checking for the dash rather than an underscore?

@wecc
Copy link
Contributor

wecc commented Sep 22, 2015

can you link to that code?

@amk221
Copy link
Contributor Author

amk221 commented Sep 22, 2015

Ah, sure: rest-serializer.js#pushPayload

@wecc
Copy link
Contributor

wecc commented Sep 22, 2015

Ah, pushPayload()... hm, seems the behavior in pushPayload() and normalizeResponse() is a bit different. I can see why you'd expect them both to accept the same payloads even though it doesn't really matter to have records of the same type in a separate key for pushPayload() since it doesn't return anything.

@amk221
Copy link
Contributor Author

amk221 commented Sep 22, 2015

It was the same reason I mentioned in Slack earlier - if you manually pushPayload it warns that meta is present (tries to load a model named metum!). But it doesn't warn if you use findAll.

Essentially I was expecting pushPayload to be able to accept the exact same structure that would have been loaded had I used findAll.

@wecc
Copy link
Contributor

wecc commented Jan 6, 2016

We discussed this issue at today's meeting and I'll have a look at this and see if/how we can implement this in a backwards compatible way. Should probably not be that hard.

Essentially I was expecting pushPayload to be able to accept the exact same structure that would have been loaded had I used findAll.

👍

@wecc wecc removed the team-review label Jan 6, 2016
@wecc
Copy link
Contributor

wecc commented Oct 22, 2016

@pangratz Would this be covered by emberjs/rfcs#161?

@pangratz
Copy link
Member

That RFC doesn't explicitly handle the inconsistencies between pushPayload and normalizeResponse in the rest serializer and also doesn't explicitly state the behavioral differences between normalizeResponse and pushPayload.

So I tend to think that this is a bug in rest serializer, which could be fixed in the short term by adding logic for keys starting with _ in pushPayload. In the long term we should evaluate merging the logic within normalizeResponse and pushPayload of the rest serializer, like we already do in the json-api serializer.

@amk221
Copy link
Contributor Author

amk221 commented Jul 13, 2018

Any update on this, it's been a long while

@runspired
Copy link
Contributor

@amk221 I would recommend against using pushPayload in general. If you have a moment to work on an RFC: #8815

@runspired runspired changed the title Warning when sideloading models of the same type RESTSerializer.pushPayload does not allow sideloading of records Nov 16, 2018
@runspired
Copy link
Contributor

Closing, as making changes to pushPayload is not something we desire to do, and because pushPayload requires too much "special sauce" and "magical format knowledge" as it is.

@amk221
Copy link
Contributor Author

amk221 commented Nov 17, 2018 via email

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

No branches or pull requests

5 participants