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

Add StatusCode to HttpRequestException #32455

Merged
merged 12 commits into from
Feb 24, 2020

Conversation

yaakov-h
Copy link
Member

@yaakov-h yaakov-h commented Feb 17, 2020

Implements #911.

TODO:

Code:

  • Add property and constructor, as approved, to Exception object.
  • Add to reference assembly
  • Add tests that it's set when it should be and not when it's not.
  • Add tests that the property is persisted when the exception is serialized.
  • Update callsites to include this when constructing the exception
  • Add tests for callsites to ensure that they do

Docs:

  • Update docs for HttpRequestException to document the new property
  • Update docs for EnsureSuccessStatusCode to point developers at the new property?

The exception doesn't seem to be marked as Serializable in any way, so I'm not sure if/how I should deal with exception serialization.

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@stephentoub
Copy link
Member

The exception doesn't seem to be marked as Serializable in any way, so I'm not sure if/how I should deal with exception serialization.

Don't worry about it as part of this PR. If/when someone makes this exception serializable, it'll be up to them to do it in a way that is appropriately compatible with .NET Framework.
cc: @ViktorHofer

@davidsh davidsh added this to the 5.0 milestone Feb 18, 2020
@yaakov-h
Copy link
Member Author

ok, I've added some xmldoc comments by pretty much lifting text from similar members elsewhere and tweaking to suit.

@ViktorHofer
Copy link
Member

Don't worry about it as part of this PR. If/when someone makes this exception serializable, it'll be up to them to do it in a way that is appropriately compatible with .NET Framework.
cc: @ViktorHofer

The reason why I didn't make that exception serializable is because on .NET Framework it is being serialized with a different mechanism that we don't use on Core: http://index/?query=HttpRequestException (internal link only visible for MSFT employees). We could still make that Exception serializable with the standard mechanism but it wouldn't be exchangeable with .NET Framework.

Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

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

Looks good. Are there any other tests that could have StatusCode asserted in? Testing the convenience methods e.g. GetAsync would be good.

@yaakov-h
Copy link
Member Author

@scalablecory Testing the basic convenince methods could be useful. Would I also need to test the convenience methods in System.Net.Http.Formatting (or the upcoming JSON one), or only the base ones defined on HttpClient itself?

@davidsh
Copy link
Contributor

davidsh commented Feb 18, 2020

Would I also need to test the convenience methods in System.Net.Http.Formatting (or the upcoming JSON one), or only the base ones defined on HttpClient itself?

The unit tests for this PR should focus only on existing APIs in HttpClient.

@yaakov-h yaakov-h marked this pull request as ready for review February 19, 2020 07:59
@davidsh
Copy link
Contributor

davidsh commented Feb 24, 2020

@yaakov-h Can you clarify if you've addressed all PR feedback? There are still some unresolved comments in the PR. Thanks.

@scalablecory
Copy link
Contributor

/azp list

@scalablecory
Copy link
Contributor

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yaakov-h
Copy link
Member Author

@davidsh unless there are any further comments to be had on the xmldoc, I think that's everything.

@scalablecory
Copy link
Contributor

It looks good to me, my "accept" check still stands. Thanks for the PR.

@scalablecory scalablecory merged commit 3a95ea1 into dotnet:master Feb 24, 2020
@yaakov-h yaakov-h deleted the httprequestexception-statuscode branch February 24, 2020 22:36
alnikola added a commit to dotnet/dotnet-api-docs that referenced this pull request Jun 12, 2020
HttpRequestException.StatusCode property gets the HTTP status code associated with the exception if any.

It was introduced in dotnet/runtime#32455
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants