Kong / unirest-java

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

Unirest 4.x - Adding a header with a null value #484

Closed haroon-sheikh closed 1 year ago

haroon-sheikh commented 1 year ago

Describe the bug When adding a header into kong.unirest.core.Headers with a null value, it succesfully adds one with an empty string.

We're in middle of upgrading to unirest 4.x and see this behaviour has changed. Is this an expected change?

To Reproduce

final Headers headers = new Headers();
headers.add("header1", "value");
headers.add("header2", (String) null);
assertEquals(1, headers.size());

Expected behavior It does not add a header with null value

Environmental Data:

ryber commented 1 year ago

Yes, I would expect it to behave this way. If you don't want a header, don't set it.

ryber commented 1 year ago

I guess, I'm a little confused about the request, do you not it to skip it? I double checked with the Unirest3 and that ALSO adds a empty header, so this has been the behavior for a while.

haroon-sheikh commented 1 year ago

Thanks @ryber for responding so quickly. Apologies, I did not explain the issue properly. We are expecting the header to be added, however the value set is different in both versions (null vs empty string).

Example:

Headers headers = new Headers();
headers.add("header1", "value");
headers.add("header2", (String) null);
System.out.println(headers);

// Output from v3
header1: value
header2: null
// Output from v4
header1: value
header2: 
ryber commented 1 year ago

Ah well, here is the thing. We are working with HTTP, and in that context, null does not exist. So what the Headers class does is irrelevant. What matters is what the result is when making a request. That part is the same, we send a empty value for the header.

As for why it changed, it probably has to do with differences between Apache HttpClient and the native Java HttpClient. Which I believe the Java client we moved to threw a NPE on null values, so the Headers class changed to never produce null (which is good .... null is bad).

In any case, we can add this as a part of the 3->4 migration guide. ... We take pull requests/