[BUGFIX release] Normalize attrs keys

[fsmanuel]

The JSONAPISerializer expects

attrs: {
  'first-name': 'firstname'
}

The PR fixes that and now both (dasherized and camelized) work

attrs: {
  'first-name': 'firstname',
  lastName: 'lastname'
}

It also fixes #3879 but it breaks a test. I wonder if the test is intentional because it declares a relationship mapping that uses keyForAttribute to normalize it. I added a test for an attribute but I'm worried to remove the (wrong) test.

I added two comments where I would add a deprecation if the attrs key includes a dash.

@wecc @bmac can you review?

[wecc]

Thanks for the PR @fsmanuel! Can you shed some light on why this would be needed?

The JSONAPISerializer expects

 attrs: {
   'first-name': 'firstname'
}

This feels wrong to me. The first string is the name of your model's attribute, the second string is the name of the payload key (equivalent to 'first-name: { key: 'firstname' }'). You'd probably never want to declare your attributes dasherized though.

By looking at the test an attribute named last-name and lastName would collide and be considered the same? That's probably not something we want to do.

Would love to know more about the use-case needing this :)

[fsmanuel]

@wecc you are right, it feels wrong. The dasherized version is the current behavior. I wasn't sure if we want to break that behavior (obviously it is wrong but has been the only way). I'm happy to remove it and add an error if we detect dasherized attrs keys.

[wecc]

@fsmanuel payload keys (attributes and relationships) with JSONAPISerializer defaults to dasherized but the serializer has no preference on what the model attributes looks like.

What's the problem this PR is trying to solve? Sorry if I'm missing something obvious here.

[fsmanuel]

As discussed with @wecc on Slack the intended behavior should be to check if the attrs key is present in the models attributes or relationships. We should log a warning if a key in attrs is not present in one of the Maps.

We can use something like:

_attributesOrRelationshipsHas(typeClass, key) {
  const has = get(typeClass, 'attributes').has(key) || get(typeClass, 'relationshipsByName').has(key);
  Ember.assert('There is no attribute or relationship with the name `' + key + '`. Check your serializers attrs hash.', has);

  return has;
}

If you are ok with it. I'll update the PR later.

[fsmanuel]

@wecc or @bmac does the snapshot has access to the typeClass? If so we could build that check into _getMappedKey. I think that would be the best place to validate the attrs key exists on the model.

[bmac]

@fsmanuel I think you can access the typeClass using snapshot.record.constructor.

[wecc]

Also snapshot.type

[fsmanuel]

Perfect. Will update the PR.

[fsmanuel]

@wecc updated. I also fixed my mistake that led to the RESTSerializer test failing.

[bmac]

I don't think its possible for this assertion to get triggered since it looks like _getMappedKey always gets called in a eachAttribute or eachRelationship loop.

[bmac]

Never mind I see it being called in normalizeUsingDeclaredMapping

[wecc]

Can we move the code from line 762 and 763 into the assert so it will be stripped correctly?

[fsmanuel]

@wecc good point.

[wecc]

Left a comment about the assert, other than that it looks good.

[wecc]

I think this comment should be removed as this is not really JSONAPISerializer specific. You would encounter this issue with any serializer where you customize keyForAttribute/keyForRelationship to not just pass through.

[fsmanuel]

true!

[fsmanuel]

updated.

[bmac]

Thanks @fsmanuel do you mind squashing this pr into 1 commit?

[fsmanuel]

@bmac i never did that before but it worked ;)

[bmac]

Thanks @fsmanuel

[fsmanuel]

@wecc @bmac thanks for the help and feedback.

[ghost]

This change has caused our app to have a huge number of warnings now for seemingly unrelated models, that didn't occur in the 2.2.0.beta.3:

WARNING: There is no attribute or relationship with the name `surveyType` on `user`. Check your serializers attrs hash.

But surveyType is not defined on user at all?

import DS from 'ember-data';

export default DS.Model.extend({
  email: DS.attr('string'),
  super: DS.attr('boolean', { defaultValue() { return false; }}),
  firstName: DS.attr('string'),
  lastName: DS.attr('string')
});

My feeling is this is to do with active-model-adapter?

[wecc]

@Soliah what does your serializer look like? I suspect you have surveyType in your attrs key in the serializer.

[ghost]

@wecc ah right, we do have that in there. Thanks.