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 status handler to recognize unknown status codes outside of 4xx/5… #31202

Conversation

duesenklipper
Copy link
Contributor

…xx and throw an UnknownHttpStatusCodeException

org.springframework.web.reactive.function.client.UnknownHttpStatusCodeException claims in its Javadoc that it will be thrown to signal that "an unknown (or custom) HTTP status code is received". Up until Spring 5/Spring Boot 2 this was indeed true. We recently upgraded to Spring Boot 3/Spring 6, and our code that expected to receive this exception no longer works.

It turns out that the DEFAULT_STATUS_HANDLER in DefaultWebClient tests HttpStatusCode.isError as a predicate before calling DefaultClientResponse#createException. createException is the only place UnknownHttpStatusException is instantiated.

This is a behavior change from Spring 5, since now only 4xx/5xx custom status codes can lead to a UnknownHttpStatusException, in spite of what the exception's javadoc says.

This PR splits DEFAULT_STATUS_HANDLER into one for errors, and one for unknown codes. Both call the same function, with different predicates - one calls isError(), the other calls a new function isWellKnownStatusCode(). I kept the logic for both WebClientResponseException and UnknownHttpStatusCodeException in createException(), since there still can be a custom 4xx/5xx code. I also provide a test that validates the behavior for custom codes outside of that range, in addition to the existing test which only verified for a custom 5xx code.

…xx and throw an UnknownHttpStatusCodeException
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 11, 2023
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

It looks this may have happened inadvertently as part of changes to introduce the HttpStatusCode interface. I think we should simply revert to how the Predicate was before, i.e. checking for codes greater than or equal to 400. I don't see any benefit from the additional default handler.

@poutsma what do you think?

@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: regression A bug that is also a regression and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Oct 30, 2023
@rstoyanchev rstoyanchev added this to the 6.1.0 milestone Oct 30, 2023
@rstoyanchev
Copy link
Contributor

Tentatively scheduled for 6.1 but still pending team review.

@rstoyanchev rstoyanchev self-assigned this Nov 1, 2023
rstoyanchev pushed a commit that referenced this pull request Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: regression A bug that is also a regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants