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

ActiveModelSerializer errors hash expecting camelcased properties instead of underscored properties in beta.17 #3067

Closed
jimmay5469 opened this issue May 13, 2015 · 14 comments · Fixed by #3076

Comments

@jimmay5469
Copy link
Contributor

When I upgrade to beta.17 my tests for server-side errors begin failing to show up, specifically errors for properties which are more than one word (first_name fails, email doesn't). I tried changing the response in the test to firstName and they began showing up again.

@jimmay5469
Copy link
Contributor Author

Just found that there is a test for this in the suite so I'm gonna make sure it isn't something on my end. Has anyone else seen this issue?

@jimmay5469
Copy link
Contributor Author

Does anyone know how to get the test here https://github.com/emberjs/data/blob/master/packages/activemodel-adapter/tests/integration/active-model-serializer-test.js#L332 to run? npm test does not seem to be running it for me...

@pangratz
Copy link
Member

Looks like this is related to changes I made in #3036. Can you post the body of your 422 response?

@pangratz
Copy link
Member

I think you're right, this seems buggy: http://emberjs.jsbin.com/kegiqusibi/1/edit?html,js,output. Clicking on save in this JSBin should print the errors under the firstName key and not first_name. I will investigate ...

@pangratz
Copy link
Member

So, the problem is that reason.errors is passed to the serializer here, where reason.errors is the errors object looks like { first_name: ["invalid firstName"] }, but the extractErrors in the serializer only normalizes the keys of the errors property of the passed object here...

@jimmay5469
Copy link
Contributor Author

I haven't dug in to the source enough to completely understand your conclusion, but the 422 response you used in your JSBin is almost identical to the response in my project's test.

What seems slightly more alarming to me at this point is that I don't think the test I mention above is running when executing npm test. I have made modifications to the test so that it should absolutely fail but the suite still completely passes when I run npm test, making me think that the test is not run at all. I checked the Travis build and the test output is the same as what I am seeing locally. Can you confirm that the test is actually running, and if so what are you running to get the test to execute?

@pangratz
Copy link
Member

I'm running the tests by starting ember server and navigating to http://localhost:4200. The test is running, as changing the assertion will show a failed test.

The behavior for the passed payload to InvalidError changed from v1.0.0-beta.16.1 to v1.0.0-beta.17 in d051616.

@jimmay5469
Copy link
Contributor Author

Ok, that is very re-assuring, I was just able to do the same. Thanks!

@pangratz
Copy link
Member

@jimmay5469 a quick patch before this is fixed upstream in ember-data would be:

DS.JSONSerializer.reopen({
  extractErrors: function(store, typeClass, payload, id) {
    if (payload && !payload.errors) {
      // JSONSerializer only normalizes the keys of
      // the `errors` property of the payload
      payload.errors = payload;
    }
    return this._super.apply(this, arguments);
  }
});

@jimmay5469
Copy link
Contributor Author

Much appreciated @pangratz!

@ghost
Copy link

ghost commented May 14, 2015

I'll be putting together a pull request to address this shortly.

@pangratz
Copy link
Member

I'll be putting together a pull request to address this shortly.

👍

@fivetanley
Copy link
Member

@pangratz @bdvholmes hate to bother y'all on the weekend but i'd love to release beta.18 with a fix on monday.

@ghost
Copy link

ghost commented May 17, 2015

I'm putting the fix together tomorrow, does that give you enough time?

Enviado desde mi iPhone

El may 16, 2015, a las 2:27 PM, Stanley Stuart [email protected] escribió:

@pangratz @bdvholmes hate to bother y'all on the weekend but i'd love to release beta.18 with a fix on monday.


Reply to this email directly or view it on GitHub.

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.

3 participants