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.serializePolymorphicType broken for async relationships #2508

Closed
martinmaillard opened this issue Nov 25, 2014 · 10 comments · Fixed by #2623
Closed

RESTSerializer.serializePolymorphicType broken for async relationships #2508

martinmaillard opened this issue Nov 25, 2014 · 10 comments · Fixed by #2623

Comments

@martinmaillard
Copy link

When a relationship is defined as polymorphic and async, serializePolymorphicType raises key is undefined because the related object is wrapped in a promise, which breaks belongsTo.constructor.typeKey.

@martinmaillard
Copy link
Author

I just noticed that polymorphic was not listed as a valid option for belongsTo in the docs. Is it not supported anymore ?

@bmac
Copy link
Member

bmac commented Nov 25, 2014

I suspect the fact that polymorphic is missing in the belongsTo docs is a doc bug.

@bmac
Copy link
Member

bmac commented Nov 25, 2014

@martinmaillard it seems like you have a good understanding of the problem and this definitely seems like a bug. Would you be comfortable submitting a pr to fix this issue with a test case?

@martinmaillard
Copy link
Author

I'll see what I can do once I've checked how the new beta version behaves. I'm not sure I've got such a good understanding of the internals yet...

@martinmaillard
Copy link
Author

I started writing the tests for this issue and it made me realize that I don't know what the normal behavior should be. I see two cases for an async belongsTo:

  • the related record is already loaded
  • the related record is not loaded yet

In the first case, the record can be extracted from the promise and the polymorphicType property can be serialized as usual.

In the second case, I'm not sure. It does not seem like a good idea to load the record just to serialize its parent. Can we access the original payload of the parent record and extract the original polymorphicType property from it ?

@martinmaillard
Copy link
Author

Sooooo, it seems like this problem is not new at all (#1491), and a lot of work has already been done (#1535). I don't really understand why the PR is closed though...

@chadhietala
Copy link
Contributor

I believe this is also related #2342 (comment)

@sly7-7
Copy link
Contributor

sly7-7 commented Nov 28, 2014

@martinmaillard Hm, I didn't get the bug in #1535 was because of polymorphism, and I thought it was just a matter of async relation, which seem to work, considering this test passes.
I don't know if I will have time this week end, but I will try to at least write a failing test about your issue (async polymorphism relation), and will see if the suggestion in #1491 will fix this issue.

@chadhietala #2342 seems yet an other one, caused by polymorphism and MODEL_FACTORY_INJECTION, which I think basically broke polymorphism in ember-cli app.

@luketheobscure
Copy link
Contributor

Here's a hacky workaround in CoffeeScript... but it requires you to make sure that the async is fulfilled before you try to serialize it.

 serializePolymorphicType: (record, json, relationship) ->
    key = relationship.key
    belongsTo = record.get(key)
    # hack for async
    if belongsTo.constructor == DS.PromiseObject
      if belongsTo.get('isFulfilled')
        belongsTo = belongsTo.get('content')
      else
        throw Error("Unable to get belongsTo for relation that isn't fulfilled")
    key = if @keyForAttribute then this.keyForAttribute(key) else key
    if Ember.isNone(belongsTo)
      json[key + "_type"] = null
    else
      json[key + "_type"] = Ember.String.classify(belongsTo.constructor.typeKey)

@igorT
Copy link
Member

igorT commented Jan 6, 2015

Should be fixed by #2623

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

Successfully merging a pull request may close this issue.

6 participants