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

Java 11 Http2Client Returns "OK" Status Message on 4XX Responses #1382

Closed
JKomoroski opened this issue Apr 15, 2021 · 2 comments · Fixed by #1550
Closed

Java 11 Http2Client Returns "OK" Status Message on 4XX Responses #1382

JKomoroski opened this issue Apr 15, 2021 · 2 comments · Fixed by #1550
Labels
bug Unexpected or incorrect behavior

Comments

@JKomoroski
Copy link
Contributor

If a server returns a 404 or 400 error (possibly other errors), and the FeignClient instance is backed using the Java 11 Http2 Client, the response will be set to OK unless a Reason-Phrase header is present in the response. Other client implementations appear to parse the status line of the response to generate this reason phrase.

I'm not certain if parsing the status line is possible, and I don't think this is actually standardized but it feels wrong getting a 404 OK or 400 OK.

The offending code appears to be:
Http2Client.java Line 78

Example Builder Used to duplicate:

Feign.builder()
  .client(new Http2Client())
  .target(MyClient.class, hostUrl);

I found this when I swapped out a client and had some tests that were checking for Feign Exception message contents.

@kdavisk6 kdavisk6 added the bug Unexpected or incorrect behavior label Apr 22, 2021
@kdavisk6
Copy link
Member

@JKomoroski

Thanks for bringing that to our attention. This is definitely a bug. We'll see when we can get to it, or if you are so inclined, submit a PR yourself.

@JKomoroski
Copy link
Contributor Author

JKomoroski commented Apr 22, 2021

@kdavisk6
Hmmm.... So I started putting in a fix then I looked at the Response model and saw the following:

  /**
   * Nullable and not set when using http/2
   *
   * See https://github.com/http2/http2-spec/issues/202
   */
  public String reason() {
    return reason;
  }

Are we married to this comment?
If so, we can simply null the field when a reason phrase header is not present, otherwise we can make best effort to populate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants