HTTP protocol version on the logging #1543
Conversation
…te SLF4J unit-test
There was a problem hiding this comment.
Overall looks good. Main concern would be to revert all changes to expected log messages and make sure we would match what was there before.
I expecte with a default HTTP/1.1 on the Response, all should work as before, reducing backward compatibility problems.
Thanks for this work, looks really nice
| @@ -29,6 +29,7 @@ | |||
| private final Map<String, Collection<String>> headers; | |||
| private final Body body; | |||
| private final Request request; | |||
| private final String protocolVersion; | |||
There was a problem hiding this comment.
Wondering if want to make this an enum or we wan't to leave it open so people can have FTPES or any other protocols they want to
There was a problem hiding this comment.
I think that we can use enum: there is limited list of protocol versions - HTTP/1.0, HTTP/1.1, HTTP/2.0 (HTTP/2). In the future we can add new values if they are needed.
I have added Request.ProtocolVersion enum and Util.enumForName method that helps to find enum value by any of strings: HTTP_1_0, HTTP/1.0, HTTP_2, HTTP/2.0 etc.
googlehttpclient/src/main/java/feign/googlehttpclient/GoogleHttpClient.java
Outdated
Show resolved
Hide resolved
httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java
Outdated
Show resolved
Hide resolved
| @@ -124,6 +128,9 @@ protected Response toFeignResponse(Request request, HttpResponse<byte[]> httpRes | |||
| final OptionalLong length = httpResponse.headers().firstValueAsLong("Content-Length"); | |||
|
|
|||
| return Response.builder() | |||
| .protocolVersion(httpResponse.version().name() | |||
There was a problem hiding this comment.
this seems to fragile.
BTW, can you add a comment showing that is the expect value for version()
Also, I don't think it would be too back to have protocol = HTTP_1_1.
If you reallt think this name transformation is important, can we change this to a switch and hardcoded responses, just to make it easier to read.
There was a problem hiding this comment.
I agree.
I have replaced the string regexp-building by looking for proper enum value by its name() and toString() values. This solution covers all available in java11 values: HTTP_1_1 and HTTP_2.
| @@ -92,6 +96,9 @@ static Request toOkHttpRequest(feign.Request input) { | |||
| private static feign.Response toFeignResponse(Response response, feign.Request request) | |||
| throws IOException { | |||
| return feign.Response.builder() | |||
| .protocolVersion(response.protocol().name() | |||
There was a problem hiding this comment.
As I write above I have replaced it by looking for enum value. It covers some of available in OkHttp values: HTTP_1_0, HTTP_1_1 and HTTP_2. But there are other values: SPDY_3 (deprecated), H2_PRIOR_KNOWLEDGE, QUIC - they are replaced by null. Then Logger writes UNKNOWN as protocol version, see https://github.com/radio-rogal/feign/blob/366d2d218cb3e7f15504a9f5aa52e95eb70f91c9/core/src/main/java/feign/Logger.java#L108
…ack compatibility, replace string protocol version with enum, replace fragile conversion of alien enums by string case-insensitive comparision
|
There is another idea. After changes What about to add before logging of a request in SynchronousMethodHandler.executeAndDecode another call |
Fixes #1533
Changes:
logRequestdoes not write protocol version as we do not know what a client will uselogAndRebufferResponsetries to write a protocol version if a response has it or writes HTTP/unknown otherwiseA Feign client can add a protocol version when it creates a response.
Next Feign clients have hardcoded value of HTTP/1.1 or try to read it from HTTP client: