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

Still emit response if return type is Void #8367

Merged
merged 2 commits into from Nov 21, 2022
Merged

Conversation

yawkat
Copy link
Member

@yawkat yawkat commented Nov 18, 2022

Before this patch, HttpClient would remove the actual response from the response publisher if the expected body type is Void. I don't see a reason to do this, and it leads to HTTP filters not seeing the response as they should.

However there is an outstanding bug that this uncovered: If there is a declarative client method that is supposed to return Publisher, it instead always returns a Publisher<HttpResponse> (the type returned from HttpClient.exchange). The code here does not convert properly when the returnType is Publisher<Void> and the result is another publisher:

private Object convertPublisherResult(ReturnType<?> returnType, Object result) {
if (returnType.getType().isInstance(result)) {
return result;
}
return conversionService.convert(result, returnType.asArgument())
.orElseThrow(() -> new IllegalStateException("Cannot convert publisher result: " + result + " to '" + returnType.getType().getName() + "'"));
}
– this causes a class cast exception in HttpGetSpec.test simple get with Publisher<Void> return.

@dstepanov do you have any idea how to fix this?

Fixes #8366

Before this patch, HttpClient would remove the actual response from the response publisher if the expected body type is Void. I don't see a reason to do this, and it leads to HTTP filters not seeing the response as they should.
Fixes #8366
@yawkat yawkat requested review from timyates and graemerocher and removed request for timyates and graemerocher November 18, 2022 16:12
@timyates timyates added this to the 3.8.0 milestone Nov 21, 2022
@timyates timyates added the type: bug Something isn't working label Nov 21, 2022
Copy link
Member

@timyates timyates left a comment

Choose a reason for hiding this comment

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

Added it to the 3.8.0 project, and kicked off the Java CI again

The Graal CI is failing as there's no Graal for java 11?

@yawkat
Copy link
Member Author

yawkat commented Nov 21, 2022

not sure it'll be ready for 3.8.0

@dstepanov
Copy link
Contributor

What do you mean by removing of the response? Do you mean thr body of the response?

@timyates
Copy link
Member

not sure it'll be ready for 3.8.0

Cool, added it to 3.9.0 as well 😎

@yawkat
Copy link
Member Author

yawkat commented Nov 21, 2022

Nevermind, I've solved it differently.

Basically, HttpClientIntroductionAdvice has a special path for Void, Publisher<Void>, etc...: It calls .exchange (which returns a Publisher<HttpResponse<O>>) instead of .retrieve (which returns a Publisher<O>). Before this change, this worked fine, because for void bodyType, exchange would always return an empty publisher anyway, so the exact type of the publisher did not matter. Now that exchange correctly returns a publisher with a single HttpResponse<Void>, there was a cast exception because that HttpResponse can't be cast to Void and the conversion (HttpResponse<O> to O) did not run for the Publisher<Void> case.

I've updated HttpClientIntroductionAdvice to use .retrieve for Void as well, and I've altered retrieve to work with Void bodyType (a body that is null is not received well by the code).

@yawkat yawkat marked this pull request as ready for review November 21, 2022 13:55
@graemerocher graemerocher merged commit 4c50dd6 into 3.8.x Nov 21, 2022
@graemerocher graemerocher deleted the void-response-filter branch November 21, 2022 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants