normalizeErrors with nested errors breaks for polymorphic associations

[jbourassa]

Calling normalizeRelationships from normalizeErrors breaks for polymorphic relationships in ActiveModelSerializer (introduced in #2392, see this diff). Here's a failing test.

This happens because ActiveModelSerializer's normalizeRelationships will inspect the payload for a type property. Since this is an error payload, the type property won't be set. Here's the (albeit not so pretty) workaround we have:

{
  // stuff...
  normalizeErrors: function(type, hash) {
    this.normalizeId(hash);
    this.normalizeAttributes(type, hash);

    if (this.keyForRelationship) {
      type.eachRelationship(function(key, relationship) {
        var payloadKey = this.keyForRelationship(key, relationship.kind);
        if (key !== payloadKey && hash.hasOwnProperty(payloadKey)) {
          hash[key] = hash[payloadKey];
          delete hash[payloadKey];
        }

        if(!hash || !hash[key]) return;
        // Array.isArray => basically checking for .kind === 'hasMany'
        if(Array.isArray(hash[key])) {
          hash[key].forEach(function(hash) {
            if(hash) this.normalizeErrors(relationship.type, hash);
          }.bind(this))
        }
        else {
          this.normalizeErrors(relationship.type, hash[key]);
        }
      }, this);
    }
  }
}

Would you like me to improve this code and submit a PR?

[bmac]

@jbourassa thanks for the workaround and test case. Would you like to open a pull request to fix this issue?

[jbourassa]

Would you like me to improve this code and submit a PR?

Would you like to open a pull request to fix this issue?

Absolutely! Will do in the next few days.

[jbourassa]

Hey @bmac,

I just updated the issue with a PR. Let me know what you think!

[bmac]

hash as an argument name is a little confusing because there already is a hash argument from the parent scope. Lets rename this to something else. Maybe error?

[jbourassa]

Agreed! I used errors since any key can hold multiple errors.

[bmac]

sounds good.

[bmac]

This test should assert that the record error object was extracted correctly. It might look something like:

var extractedError = serializer.extractErrors(env.store, MediocreVillain, payload, null); // id (last param) is not relevant here

deepEqual(extractedError, {
  evilMinions: [{ name: ['required'] }]
});

[jbourassa]

Also very correct, my bad. I didn't look very far and kept my original test that was flat out erroring. That is also fixed!

[jbourassa]

Actually, it's a bit more complicated than that. The serializer will not change the key from evil_minion to evilMinions because it is looking for evil_minion_ids.

So there are two things that are relevant in this test:

1. No matter what you pass in, extractErrors should crash (eg: not look for a .type property on an error payload)
2. If the serializer is configured to rename the evil_minions association to evilMinions, errors should also be renamed.

I updated the test again to cover (2), but I had to bring in EmbeddedRecord.

[bmac]

@igorT @tchak Does Ember Data's Error handling mechanism handle the case were the error object for a hasMany relationship be an array of error objects or do these errors get silently ignored? e.g.

{
    errors: {
      evil_minions: [
        { name: ['required'] },
        { name: ['required'] }
      ]
}

[jbourassa]

@bmac

Does Ember Data's Error handling mechanism handle the case were the error object for a hasMany relationship be an array of error objects or do these errors get silently ignored? [...]

I know you didn't ask me — I have no idea if support is intended or not — but it works and we do use it in our app, keeping it would be very nice 🙏

[bmac]

Sorry @jbourassa that question was meant to be directed at @igorT. I posted it to this issue to give him some context.

[jbourassa]

The tests were passing locally but not from a fresh install: I'll investigate.

[pangratz]

Since #3040 the normalizeUsingDeclaredMapping method is invoked inside normalizeErrors: this makes the above lines 293-297 obsolete as the hash already contains the normalized relationship keys. The call for this.normalizeRelationshipErrors inside normalizeErrors above needs to moved up though, so this works correctly.

[jbourassa]

I might be missing something but I don't think that's correct. normalizeUsingDeclaredMapping only normalize declared keys, not association defaults defined with keyForRelationship.

[pangratz]

Good catch! You're right, I haven't thought about keyForRelationship 👍

[jbourassa]

@pangratz Thanks for the feedback / sorry for not nailing it better. This is taking longer than I want but I'll update the PR at some point this week.

[pangratz]

sorry for not nailing it better

@jbourassa I hope I didn't leave the impression you need to apologize and if so I am very sorry! That was absolutely not my intention 😟 . I am very happy you took the time and effort to add this to the core of ember-data. I am using a quite similar implementation within an app but never had the time to submit a patch. Once this lands I can remove that bit 👯

Feel free to ping me if you want further feedback or need anything else. Cheers!

[tchak]

We are moving internal representation of errors to a son-api format. It means there is no such thing as nested errors or nested relationships on internal representation. We are going to need to normalize to something flat with the right pointer.

Something like ?

{
  "errors": [
    {
      "title": "required",
      "source": { "pointer": "included/3/attributes/name" }
    }
  ]
}

[jbourassa]

Got it! We're not transitioning our errors to json-api in in the next few weeks so I'll go ahead and close this PR.

[igorT]

@jbourassa why close if you are not transitioning, this is still a bug we should fix?

[jbourassa]

@igorT Well, would you rather me to reopen and revert the last commit that handles nested relationships? If I understand correctly, this code and the original bug will be obsolete when json-api errors are implemented, so I don't see why you'd want to merge this.

[igorT]

we still want the failing test though?

[jbourassa]

I might be missing something but if ember-data drops support for non-json-api error payload, the failing test will also be irrelevant, no?

[igorT]

We are switching our internal error format to json api, but active model serializer should still work.

[jbourassa]

Gotcha! I'll reopen and leave the failing test.

[jbourassa]

Done — I forced-push an updated test and rebased it on master.

[fivetanley]

@jbourassa Sorry for the late follow-up. Can you update the PR or close it if it's no longer needed?

[jbourassa]

This is still an issue with 1.13 (the version I'm on), but I think it's gone in 2.3 — I can't find the offending code in master. That said, I feel that this use case is pretty far from where ember data is going with JSON API, I don't think it's a bug at this point but just something that is not supported.

Let's close this.