8000 Support ApiException.Content when actual type is ValidationApiException by DrakeLambert · Pull Request #953 · reactiveui/refit · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Support ApiException.Content when actual type is ValidationApiException #953

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Oct 1, 2020
Merged

Support ApiException.Content when actual type is ValidationApiException #953

merged 9 commits into from
Oct 1, 2020

Conversation

DrakeLambert
Copy link
Contributor

Fixes #636

Consider this usage of a Refit client:

try
{
    await refitClient.GetAsync(123);
}
catch (ApiException e)
{
    Console.WriteLine(e.Content);
}

If the response follows RFC 7808, then e.Content will unexpectedly be null.

This PR would change the behavior so that e.Content would still be set to the string content of the response message. If the exception is caught as ValidationApiException, the public ProblemDetails Content { get; } will still function as normal.

What kind of change does this PR introduce?

Enhancement: Support use of ApiException.Content when hidden by ValidationApiException.Content.

What is the current behavior?

ApiException.Content is left as default, null, when hidden by ValidationApiException.Content.

What is the new behavior?

ApiException.Content is set to the string content of the response message.

What might this PR break?

If consumers rely on ApiException.Content to be null when the actual type is ValidationApiException, then their code may break. As far as I can tell, the alternative is more of an issue than this fix.

Please check if the PR fulfills these r 8000 equirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What docs should be added for this? I neglected this step because I considered the original behavior to be unexpected.

@jamiehowarth0
Copy link
Member

Brilliant, thanks for this! I'll review shortly

@DrakeLambert
Copy link
Contributor Author

@jamiehowarth0 who else could give a review here? I'd like to see what y'all think of this change.

@jamiehowarth0
Copy link
Member

@jamiehowarth0 who else could give a review here? I'd like to see what y'all think of this change.

Sorry for the delay, I'll check this in the next 24hrs

@DrakeLambert
Copy link
Contributor Author

@jamiehowarth0 ping!

jamiehowarth0
jamiehowarth0 suggested changes Sep 30, 2020
View reviewed changes
jamiehowarth0
jamiehowarth0 approved these changes Oct 1, 2020
View reviewed changes
Copy link
Member
@jamiehowarth0 jamiehowarth0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks!

@jamiehowarth0 jamiehowarth0 merged commit 890ea5d into reactiveui:master Oct 1, 2020
@jamiehowarth0 jamiehowarth0 mentioned this pull request Oct 1, 2020
Closed
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jamiehowarth0 jamiehowarth0 jamiehowarth0 approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Content in ValidationApiException hides Content in derived ApiException
3 participants
@DrakeLambert @jamiehowarth0 @clairernovotny
0