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 default error handling from RequestHeadersSpec::retrieve for use with RequestHeadersSpec::exchange #22825

Closed
stkent opened this issue Apr 22, 2019 · 4 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@stkent
Copy link

stkent commented Apr 22, 2019

Affects: 5.1


This is an enhancement request around the RequestHeadersSpec::exchange and RequestHeadersSpec::retrieve methods.

Per the documentation for retrieve:

This method is a shortcut to using exchange() and decoding the response body through ClientResponse.

The decoding mentioned here also includes some default error handling that converts 4XX and 5XX status responses to WebClientResponseExceptions as follows:

private static Mono<WebClientResponseException> createResponseException(
		ClientResponse response, HttpRequest request) {

	return DataBufferUtils.join(response.body(BodyExtractors.toDataBuffers()))
			.map(dataBuffer -> {
				byte[] bytes = new byte[dataBuffer.readableByteCount()];
				dataBuffer.read(bytes);
				DataBufferUtils.release(dataBuffer);
				return bytes;
			})
			.defaultIfEmpty(new byte[0])
			.map(bodyBytes -> {
				Charset charset = response.headers().contentType()
						.map(MimeType::getCharset)
						.orElse(StandardCharsets.ISO_8859_1);
				if (HttpStatus.resolve(response.rawStatusCode()) != null) {
					return WebClientResponseException.create(
							response.statusCode().value(),
							response.statusCode().getReasonPhrase(),
							response.headers().asHttpHeaders(),
							bodyBytes,
							charset,
							request);
				}
				else {
					return new UnknownHttpStatusCodeException(
							response.rawStatusCode(),
							response.headers().asHttpHeaders(),
							bodyBytes,
							charset,
							request);
				}
			});
}

I would like to be able to apply this same error-handling to results of exchange before performing my own processing, so that error handling is unified across all requests (regardless of whether they are implemented using exchange + additional processing, or retrieve).

Would it be possible to expose this error-handling functionality publicly so that it can be composited with other response processing? I'm not sure what the appropriate mechanism would be if so, but it looks like some related discussion occurred in #22368.

If not, would the recommendation be to always prefer exchange over retrieve and to reimplement the logic above in a shared ExchangeFilterFunction?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 22, 2019
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Apr 30, 2019

Perhaps the createResponseException(ClientResponse, HttpRequest) method could move to ClientResponse (under some such name) with just a single argument HttpRequest. That way methods applications using exchange could create an exception more easily.

What do you think @poutsma? This would fit well next to #22368 as a related enhancement so I'll schedule it tentatively for 5.2.

@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 30, 2019
@rstoyanchev rstoyanchev added this to the 5.2 M2 milestone Apr 30, 2019
@rstoyanchev rstoyanchev modified the milestones: 5.2 M2, 5.2 M3 May 8, 2019
@poutsma poutsma self-assigned this Jun 11, 2019
@poutsma poutsma modified the milestones: 5.2 M3, 5.2 RC1 Jun 11, 2019
@rstoyanchev rstoyanchev added the type: enhancement A general enhancement label Jun 25, 2019
@poutsma
Copy link
Contributor

poutsma commented Jul 12, 2019

Fixed by introducing a createException() method on ClientResponse. I did not see a way for users to obtain a reference to a HttpRequest, so I dropped that argument and made sure the ClientResponse implementations obtained a request reference on construction.

@membersound
Copy link

membersound commented Jul 24, 2019

@poutsma Could you also add a createException(HttpStatus code)? Because eg I have an external webservice that I have no control of, and it usually returns 200 OK BUT an error response as body. So when I evaluate that body, I might want to return a clientResponse.createException(BAD_GATEWAY) based on the received web response. Which is not possible by simply calling clientResponse.createException(), which would create a json exception, BUT with status 200 OK proxied through!

@poutsma
Copy link
Contributor

poutsma commented Jul 29, 2019

@membersound If you create an issue for that enhancement, we will consider it. I cannot give any guarantees whether it will be implemented, since that does sound like a rather obscure use case.

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: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants