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

Error reason not propagating on some status codes #753

Merged
merged 9 commits into from
Sep 4, 2019

Conversation

ealsur
Copy link
Member

@ealsur ealsur commented Aug 30, 2019

Description

When executing a server requests, we have 2 possible errors:

  • Errors that generate exceptions
  • Errors that return the response directly (409, 412, 404)

For the first one, we convert the exception into a ResponseMessage with the ErrorMessage property coming from the exception.

The second kind gets treated as a normal response and passed forward, but the actual reason for the error is within the ResponseBody.

This PR fixes this scenario by making sure that, when EnsureSuccessStatusCode is called on a ResponseMessage, we generate the ErrorMessage based on the body if it wasn't already set. So for the cases where the errors are coming from exceptions, this is a no-op, but for the cases of errors coming as responses, we materialize the body and obtain the error.

Additionally, as part of the investigation, a bug was found in the GatewayStoreClient that was not propagating the body in the case of such status codes.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Closing issues

Closes #681

@ealsur ealsur added the bug Something isn't working label Aug 30, 2019
@ealsur ealsur requested a review from kirankumarkolli August 30, 2019 20:57
@ealsur ealsur requested a review from kirillg as a code owner August 30, 2019 20:57
@ealsur ealsur self-assigned this Aug 30, 2019
j82w
j82w previously approved these changes Sep 3, 2019
kirankumarkolli
kirankumarkolli previously approved these changes Sep 4, 2019
@kirankumarkolli kirankumarkolli dismissed stale reviews from j82w and themself via ec16bd7 September 4, 2019 17:54
@kirankumarkolli kirankumarkolli merged commit c5038a9 into master Sep 4, 2019
@kirankumarkolli kirankumarkolli deleted the users/ealsur/errormessage branch September 4, 2019 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conflict 409 errors not returning a reason when failing on trigger
3 participants