HTTP protocol version on the logging #1543

vitalijr2 · 2021-11-27T23:27:29Z

Fixes #1533

Changes:

logRequest does not write protocol version as we do not know what a client will use
logAndRebufferResponse tries to write a protocol version if a response has it or writes HTTP/unknown otherwise

A 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:

Default (hardcoded value)
Apache HTTP Client (hardcoded value)
HC5
Google HTTP Client (hardcoded value)
Java11
OkHttpClient

…te SLF4J unit-test

…ith full name

velo

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

core/src/main/java/feign/Client.java

velo · 2021-11-28T21:24:00Z

core/src/main/java/feign/Response.java

@@ -29,6 +29,7 @@
  private final Map<String, Collection<String>> headers;
  private final Body body;
  private final Request request;
+  private final String protocolVersion;


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

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.

core/src/test/java/feign/LoggerTest.java

googlehttpclient/src/main/java/feign/googlehttpclient/GoogleHttpClient.java

httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java

mock/src/main/java/feign/mock/MockClient.java

velo · 2021-11-28T21:35:18Z

java11/src/main/java/feign/http2client/Http2Client.java

@@ -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()


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.

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.

velo · 2021-11-28T21:36:14Z

okhttp/src/main/java/feign/okhttp/OkHttpClient.java

@@ -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()


same as java11

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

slf4j/src/test/java/feign/slf4j/Slf4jLoggerTest.java

…ack compatibility, replace string protocol version with enum, replace fragile conversion of alien enums by string case-insensitive comparision

vitalijr2 · 2021-11-29T12:21:00Z

There is another idea. After changes Request has constant-like field protocolVersion with value HTTP_1_1.

What about to add before logging of a request in SynchronousMethodHandler.executeAndDecode another call client.prepareRequest: the client can upgrade request's version if its wants.

velo

I liked, will leave to simmer for a few days, in case @kdavisk6 has anything to say about it

* [HTTP version] Add HTTP version to Response, the default client; Update SLF4J unit-test * [HTTP version] Mock client * [HTTP version] Apache HTTP Client * [HTTP version] protocol -> protocolVersion; Replace protocol number with full name * [HTTP version] Code style, rollback to old one * [HTTP version] Google HTTP Client * [HTTP version] HTTP_PROTOCOL -> HTTP_PROTOCOL_VERSION * [HTTP version] HC5 * [HTTP version] Java11 Client * [HTTP version] OkHttpClient * [HTTP version] Code style, rollback to old one * [HTTP version] Make some required changes: restore log messages for back compatibility, replace string protocol version with enum, replace fragile conversion of alien enums by string case-insensitive comparision * [HTTP version] Code style, rollback to old one; Remove unused constants * [HTTP version] Update imports * [HTTP version] Test coverage * [HTTP version] Fix license issue * [HTTP version] Beatify and simplify the unit-test

vitalijr2 added 11 commits November 27, 2021 21:27

[HTTP version] Add HTTP version to Response, the default client; Upda…

917a70a

…te SLF4J unit-test

[HTTP version] Mock client

8d50982

[HTTP version] Apache HTTP Client

ccfe371

[HTTP version] protocol -> protocolVersion; Replace protocol number w…

5ec727f

…ith full name

[HTTP version] Code style, rollback to old one

edd25e9

[HTTP version] Google HTTP Client

a50a0e5

[HTTP version] HTTP_PROTOCOL -> HTTP_PROTOCOL_VERSION

daef29d

[HTTP version] HC5

db253ac

[HTTP version] Java11 Client

3fbcca5

[HTTP version] OkHttpClient

66b1191

[HTTP version] Code style, rollback to old one

0d879e0

velo requested changes Nov 28, 2021

View reviewed changes

[HTTP version] Make some required changes: restore log messages for b…

366d2d2

…ack compatibility, replace string protocol version with enum, replace fragile conversion of alien enums by string case-insensitive comparision

vitalijr2 added 5 commits November 29, 2021 14:29

[HTTP version] Code style, rollback to old one; Remove unused constants

9c6191c

[HTTP version] Update imports

eb41a23

[HTTP version] Test coverage

20b31c2

[HTTP version] Fix license issue

df080b8

[HTTP version] Beatify and simplify the unit-test

acab19c

velo approved these changes Nov 30, 2021

View reviewed changes

velo added the ready to merge Will be merged if no other member ask for changes label Nov 30, 2021

velo merged commit 470631b into OpenFeign:master Nov 30, 2021

vitalijr2 deleted the http-version branch December 13, 2021 21:10

This was referenced Dec 23, 2021

Update feign monorepo to v11.8 opfab/operatorfabric-core#2316

Merged

Update dependency io.github.openfeign:feign-core to v11.8 HLTech/judge-d#230

Merged

This was referenced May 10, 2022

Update dependency io.github.openfeign:feign-httpclient to v11.8 hmcts/em-stitching-api#957

Merged

Update dependency io.github.openfeign:feign-jackson to v11 FigureTechnologies/asset-classification-libs#18

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP protocol version on the logging #1543

HTTP protocol version on the logging #1543

vitalijr2 commented Nov 27, 2021 •

edited

Loading

velo left a comment

velo Nov 28, 2021

vitalijr2 Nov 29, 2021 •

edited

Loading

velo Nov 28, 2021

vitalijr2 Nov 29, 2021

velo Nov 28, 2021

vitalijr2 Nov 29, 2021 •

edited

Loading

vitalijr2 commented Nov 29, 2021 •

edited

Loading

velo left a comment

HTTP protocol version on the logging #1543

HTTP protocol version on the logging #1543

Conversation

vitalijr2 commented Nov 27, 2021 • edited Loading

velo left a comment

Choose a reason for hiding this comment

velo Nov 28, 2021

Choose a reason for hiding this comment

vitalijr2 Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

velo Nov 28, 2021

Choose a reason for hiding this comment

vitalijr2 Nov 29, 2021

Choose a reason for hiding this comment

velo Nov 28, 2021

Choose a reason for hiding this comment

vitalijr2 Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

vitalijr2 commented Nov 29, 2021 • edited Loading

velo left a comment

Choose a reason for hiding this comment

vitalijr2 commented Nov 27, 2021 •

edited

Loading

vitalijr2 Nov 29, 2021 •

edited

Loading

vitalijr2 Nov 29, 2021 •

edited

Loading

vitalijr2 commented Nov 29, 2021 •

edited

Loading