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

attrs not working for json-api-serializer #3810

Closed
digabit opened this issue Oct 1, 2015 · 10 comments
Closed

attrs not working for json-api-serializer #3810

digabit opened this issue Oct 1, 2015 · 10 comments

Comments

@digabit
Copy link

digabit commented Oct 1, 2015

Defining an attrs map when using JSONAPISerializer does not seem to be working.

For example if I have the following model:

// app/models/product.js
export default DS.Model.extend({
    productNumber: DS.attr('string')
});

and the following serializer:

// app/serializers/product.js
export default DS.JSONAPISerializer.extend({
  attrs: {
    productNumber: 'partNumber'
  }
});

and JSON:API data like this coming over the wire:

{
    "data": [{
           "type": "products",
           "id": "15296",
           "attributes": {
               "partNumber": "3004-787"
           },
           "relationships": ...,
           "links": ...
        }]
}

The serializer does not assign '3004-787' to the productNumber property of the new product.

The root cause seems to be in json-api-serializer which uses normalizeUsingDeclaredMapping from json-serializer. That method looks for the keys on attrs at the root level of the hash. In JSON:API these properties are inside the attributes property. So it seems like we need to override normalizeUsingDeclaredMapping and do something like this:

// serializers/product.js
 normalizeUsingDeclaredMapping: function (typeClass, hash) {
    var get = Ember.get;
    var attrs = get(this, "attrs");
    var payloadKey, key;

    if (attrs) {
      for (key in attrs) {
        payloadKey = this._getMappedKey(key);
        if (!hash.attributes || !hash.attributes.hasOwnProperty(payloadKey)) {
          continue;
        }

        if (payloadKey !== key) {
          hash.attributes[key] = hash.attributes[payloadKey];
          delete hash.attributes[payloadKey];
        }
      }
    }
  }

This does in fact solve the problem in my environment. I didn't submit this as a pull request because I think there are probably other areas which I'm not familliar with but which need to be fixed as well. Like when serializing this property we need to do the reverse to construct the proper JSON from the model. Also does attrs apply to relationship names as well? Seems like it might have the same problem. Thanks

@HeroicEric
Copy link
Member

@digabit If your server is sending the key as partNumber, I think you'll want to use:

export default DS.JSONAPISerializer.extend({
  attrs: {
    productNumber: { key: 'partNumber' }
  }
});

@HeroicEric
Copy link
Member

Or maybe you need to do something like:

import DS from 'ember-data';

const { JSONAPISerializer } = DS;

export default JSONAPISerializer.extend({
  keyForAttribute(attr) {
    if (attr === 'partNumber') {
      attr = 'productNumber';
    }

    return this._super(...arguments);
  }
});

@digabit
Copy link
Author

digabit commented Oct 5, 2015

Thanks for the suggestions. Your first idea doesn't seem to work. Your second suggestion does give another way to do it although you need to:

return this._super(attr);

However what you are proposing are work-arounds. This is exactly what the attrs mapping is supposed to be for and it doesn't seem to be working.

@pangratz
Copy link
Member

pangratz commented Oct 5, 2015

@digabit I think you should open a PR so this is fixed in the JSONAPISerializer. The normalizeUsingDeclaredMapping normalizes the keys for attributes and relationships, so this needs to be considered in the fix. I would change this line with something like:

this.normalizeUsingDeclaredMapping(modelClass, resourceHash.attributes);
this.normalizeUsingDeclaredMapping(modelClass, resourceHash.relationships);

There should likely be checks added, since normalizeUsingDeclaredMapping expects a hash as second argument, but attributes or relationships might be undefined on the JSONAPI document.

@digabit
Copy link
Author

digabit commented Oct 5, 2015

@pangraz Ah yeah that is cleaner than overriding the whole method. Alright I'll put together a PR.

@HeroicEric
Copy link
Member

@digabit my bad. I misunderstood. I didn't realize normalizeUsingDeclaredMapping was a thing. I thought you added that.

@digabit
Copy link
Author

digabit commented Oct 5, 2015

@HeroicEric No problem!

@pangratz
Copy link
Member

Hey @digabit, kindly asking if you had any chance yet for the PR?

@weinberg
Copy link

Hi @pangratz - just filed the PR here: #3845 (using my personal account). Thanks

@bmac
Copy link
Member

bmac commented Oct 16, 2015

I believe this issue is solved by #3847.

@bmac bmac closed this as completed Oct 16, 2015
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

4 participants