Kong / unirest-java

Unirest in Java: Simplified, lightweight HTTP client library.
http://kong.github.io/unirest-java/
MIT License
2.58k stars 591 forks source link

Configured HttpClient.Version ignored #511

Closed JamesBoon closed 5 months ago

JamesBoon commented 6 months ago

Describe the bug Setting the http version does nothing, as it is not read from the configuration in JavaClient.

To Reproduce

Unirest.config()
        .version(HttpClient.Version.HTTP_1_1);

HttpResponse<String> response = Unirest.post("https://httpbin.org/post")
        .body("test body")
        .asString();

// Should NOT contain "Connection", "Upgrade" and "Http2-Settings" header
System.out.println(response.getBody());

Expected behavior Http version 1.1 should be used if configured and no HTTP2 header sent.

Environmental Data:

Additional context I think changing the line .version(HttpClient.Version.HTTP_2) to something like .version(config.getVersion()) should fix the problem.

JamesBoon commented 6 months ago

In addition, it would be very practical if the version could be set individually for each request.
Just like this:

HttpResponse<String> response = Unirest.post("https://httpbin.org/post")
        .version(HttpClient.Version.HTTP_1_1)
        .body("test body")
        .asString();

Should I create a new feature request for this?

ryber commented 6 months ago

just released 4.2.5 with this fix. I just did the root config for now, per request is a but more work.

JamesBoon commented 6 months ago

Thank you! The commits look good.
What do you think, how long will it take until 4.2.5 will be available in Maven Central? EDIT: It is available - just not visible on the webpage :roll_eyes:

And should I create a new feature request for the "per request" configuration? Or do you find that feature (request) unnecessary?

ryber commented 6 months ago

Yea Maven central is a maze, the artifacts will typically show up for download in under 1 hour (I assume there is some cache that needs to expire), the website updates about once per day.

As for the per-request. We can just leave this issue open. But what I would like to understand is the reason folk would want to do that? It was my understanding (probably wrong) that HTTP2 was backwards compatible and degraded gracefully to 1.1. You would think the extra headers would just be ignored. What happened that caused you to even look at this?

JamesBoon commented 6 months ago

I have to make calls to an old legacy server that denies access as soon as I send the http2 headers. (Don't ask me why!)
It was actually a bit of a pain to find the cause for this behavior :sweat_smile:

JamesBoon commented 6 months ago

Seriously, @ryber, this is amazing. I just came here and thought I would ask you if you need any help with the implementation, like creating a PR. And now you're already done!

ryber commented 5 months ago

I'm trying to figure out if it actually deployed, maven central threw a weird error at the very end after it uploaded the artifacts. Its kind of a mess

JamesBoon commented 5 months ago

It seems it worked: https://repo1.maven.org/maven2/com/konghq/unirest-java-core/4.2.6/

ryber commented 5 months ago

Oh good, I think we can close this then if it works for you

JamesBoon commented 5 months ago

It works perfectly!