Skip to content

Raise client error for unhandled 4xx responses #311

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

Merged

Conversation

jamesstonehill
Copy link
Contributor

@jamesstonehill jamesstonehill commented Oct 9, 2018

Resolves #308

@gaorlov
Copy link
Collaborator

gaorlov commented Oct 9, 2018

This looks solid. Thank you so much for the contribution! Can you take a look and see if the test failures are due to your changes?

@jamesstonehill
Copy link
Contributor Author

Doesn't look like it. Looks like some kind of test file load order thing.

@gaorlov
Copy link
Collaborator

gaorlov commented Oct 9, 2018

@jamesstonehill those are the worst. Alright. Rerunning seems to have fixed it. I'll take a look later. Thanks!

@gaorlov
Copy link
Collaborator

gaorlov commented Oct 9, 2018

Oh. One last thing: can you add a description to the Changelog.md?

@gaorlov gaorlov merged commit 1f6879d into JsonApiClient:master Oct 10, 2018
@gaorlov
Copy link
Collaborator

gaorlov commented Oct 10, 2018

@jamesstonehill 1.6.2 is live

@marybethlee
Copy link
Contributor

marybethlee commented Oct 11, 2018

This is causing 422 errors to raise an error again, which was something that was reverted in this commit: 819531a#diff-a5876160692c35f965ad9d0dc8d46f69

422 errors are usually used for resource validation errors, should that particular status be excluded from the ClientError?

@gaorlov
Copy link
Collaborator

gaorlov commented Oct 11, 2018

That's not excellent. How would everyone feel about explicitly defining every 4xx error as its own error class?

@jamesstonehill
Copy link
Contributor Author

Why was that reverted? Raising an error may not be the best way to handle a 422 but it's far better than failing silently.

@jamesstonehill jamesstonehill deleted the raise-on-misc-client-error branch October 11, 2018 14:28
@marybethlee
Copy link
Contributor

I think most people would expect 422 to not raise an error, because the response is returning errors on the resource, and the gem is handling that appropriately -- the model.errors is filled with the data from the response

To not break compatibility for now, the easiest would be to not raise on 422 errors, and a more customized approach would be to allow users to configure how they want 4xx errors to behave

@jamesstonehill jamesstonehill restored the raise-on-misc-client-error branch October 11, 2018 14:38
@jamesstonehill jamesstonehill deleted the raise-on-misc-client-error branch October 11, 2018 14:38
@jamesstonehill
Copy link
Contributor Author

Yeah, that's a good point. Since these responses would populate the model errors raising probably isn't right here. Probably best to revert.

@jamesstonehill jamesstonehill restored the raise-on-misc-client-error branch October 11, 2018 14:38
@marybethlee
Copy link
Contributor

@jamesstonehill I think raising on the other 4xx errors is appropriate, it's just the 422 that should be ignored.

@jamesstonehill
Copy link
Contributor Author

Are there any other 4xx statuses that might return validation errors?

@jamesstonehill
Copy link
Contributor Author

https://github.com/JsonApiClient/json_api_client/pull/312/files

@marybethlee
Copy link
Contributor

I'm not sure if there are other statuses that might return validation errors, and the json api spec isn't specific about the 4xx errors, but 422 seems generally accepted.

@gaorlov
Copy link
Collaborator

gaorlov commented Oct 11, 2018

I think it's reasonable to allow them as they come up

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 this pull request may close these issues.

3 participants