adopted-ember-addons / ember-changeset

Ember.js flavored changesets, inspired by Ecto
http://bit.ly/ember-changeset-demo
MIT License
431 stars 141 forks source link

Type-Error thrown when a JSON-API server responds with a 406 error and an error body #552

Open psbanka opened 4 years ago

psbanka commented 4 years ago

Version

3.9.2

Test Case

Sorry, don't have a test case for this

Steps to reproduce

When saving a changeset, have a server respond with a 406 error response and the following body:

{
  errors: [
    { 
      title: 'Well excuse me',
      detail: 'Error creating the thing you wanted.'
    }
  ]
}

Expected Behavior

I expect the Ember Data exception to be passed to my catch()rather than a type-error from ember-changeset

Actual Behavior

ember-changeset throws a TypeError:

TypeError: Cannot convert undefined or null to object, and it is impossible to get the original error back from Ember Data. Instead, I have to pull all the information out of the changeset.snapshot().changes object, save them to the model manually, and save the model in order to get the Ember Data exception.

snewcomer commented 4 years ago

@psbanka 👋 I merged a PR that included improvements to an existing test to more closely match your case. However, I'm still seeing the tests pass. Do you happen to know how your situation might not be reflected in the tests?

https://github.com/poteto/ember-changeset/blob/ae3beb656c7abb645df45552fd113f9ccff7670a/tests/unit/changeset-test.js#L1400

Feel free to reopen the issue if you feel the need to!

jherdman commented 3 years ago

I'm running into this as well. Is there a way to work around this?

jherdman commented 3 years ago

Some data I've gathered:

  1. Changeset appears to be valid on the client, and changeset.save() is called
  2. API responds with 422, looks something like this:
{"errors":{"user_email":["digit@example.com is already a client"]}}
  1. Deep in the stack of Ember Changeset, you see it failing here:

https://github.com/poteto/ember-changeset/blob/dbf405a9814afdc62b6b4619a36820c68908e30d/addon/utils/merge-deep.js#L61-L81

  1. Inspecting source reveals something like this:
{ userEmail: null, firstName: null, lastName: null, email: null }
  1. The below line is truthy

https://github.com/poteto/ember-changeset/blob/dbf405a9814afdc62b6b4619a36820c68908e30d/addon/utils/merge-deep.js#L75-L77

because typeof null === 'object'

  1. buildPathToValue is called with null as the source

  2. EXPLODE

jkeen commented 3 years ago

I'm running into this too and thank the gods I'm not alone in this, because I've been scratching my head for hours on this one and have not figured out a workaround yet.

@snewcomer Might want to reopen this one

snewcomer commented 3 years ago

@jherdman For 6), if possible === null, then the first part of that if expression would fail and we would move on.

Do you happen to know of a good test we could add?

jkeen commented 3 years ago

@jherdman For 6), if possible === null, then the first part of that if expression would fail and we would move on.

Merge-deep in ember-changeset and merge-deep in validated-changeset are slightly different, where that expression will fail appropriately in ember-changeset, but not in validated-changeset as typeof(null) == 'object'

https://github.com/poteto/ember-changeset/blob/master/addon/utils/merge-deep.js#L75

Yesterday when stepping through my code (which is using ember-changeset 3.10.1), I was hitting that spot in validated-changeset where it blows up like @jherdman described, when possible is null

https://github.com/validated-changeset/validated-changeset/blob/master/src/utils/merge-deep.ts#L71

Going up a level and starting at the mergeDeep call, it succeeds in the case of merging plain objects, but merging an object with null values { userEmail: null, firstName: null, lastName: null, email: null } and a changeset is where things get hinky.

snewcomer commented 3 years ago

Published 3.10.2. Hopefully this resolves the merge-deep issues!

jkeen commented 3 years ago

Works for me! Thanks @snewcomer