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

Expose all information available in error responses #197

Merged
merged 5 commits into from
Sep 16, 2022

Conversation

jacegu
Copy link
Contributor

@jacegu jacegu commented Sep 14, 2022

Our error responses sometimes include an errors entry with details about errors on the attributes of the specific resource. We handle those errors via RequestError, but we are only considering the message.

This PR:

  • Adds an errors attribute to Dnsimple.ResponseErrorexception.
  • Makes sure that we correctly parse the error responses to provide the value for errors.
  • Adds missing specs around error response handling.
  • Fixes a bug where the client would crash trying to parse non json-encoded error responses.

As we are going to introduce more specs in this context, that won't rely
on the same fixture.
With JSON responses.
We are setting the ground to add the specific specs about the errors
payload in the response.
This was actually a bug in the client, where we were assuming all error
responses would have a JSON payload. I am replicating the behavior we
currently have in the Ruby client.
We are also considering the cases in which the payload doesn't include
this element and non-json-encoded 5xx responses.
@jacegu jacegu self-assigned this Sep 14, 2022
@jacegu jacegu requested review from a team and OleMchls and removed request for a team September 14, 2022 16:57
To RequestError#attribute_errors
@jacegu jacegu merged commit 0684f63 into main Sep 16, 2022
@jacegu jacegu deleted the change/expose-response-errors branch September 16, 2022 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants