Unirest.config().setRequestHeader adds header instead of setting it

[philippendrulat]

When I use Unirest.config().setRequestHeader(name, value) I expect Unirest to set the header on all requests. If the header is already there, I want to override it.
In previous Versions, this was the behavior. The new Version adds the same header again and again.

Is this behavior intended or is it a bug? And if it's intended, how do I override an existing header without clearing all other headers?

[ryber]

The HTTP spec says that there can be multiple of the same header, so appending is what I would expect. The previous behavior was a bug. It is possible to directly manipulate the Header (it's just a TreeMap<String, List<String>>) through getHeader but that is kind of awkward and breaks the build pattern. I understand the use case so I'm adding a headerReplace method

        Unirest.config().setDefaultHeader("foo", "bar");

        Unirest.get(MockServer.GET)
                .headerReplace("foo", "qux")
                .headerReplace("fruit", "mango")
                .asObject(RequestCapture.class)
                .getBody()
                .assertHeaderSize("foo", 1)
                .assertHeaderSize("fruit", 1)
                .assertHeader("foo", "qux")
                .assertHeader("fruit", "mango");

Should be on 3.2.01 in mvn central in about 20-30 minutes.

[Catscratch]

Hi. I got the same problem.

The HTTP spec says that there can be multiple of the same header, so appending is what I would expect.

That's right. But only in HTTP. In JAVA a setter should always overwrite. What you want is an Adder (addHeader...).

So from Java developer perspective I would assume:

• setDefaultHeader: overwrites the old one with the new value
• addDefaultHeader: will set (if nothing already in the map) or append (if map already contains entries)

I wouldn't touch the builder pattern with "headerReplace" because it makes no sense at all. Why would you ever replace the header in one building line?

[ryber]

the original method is just header, in fact the entire original unirest builder pattern avoids java words like set or get most of the time. This is because Unirest was originally part of a family of unirests, there were ones for PHP and Python and such.

I also want the method to align with IDE auto-complete and having it right next to .header( is nice for that purpose. I picked replace because this is what it's called in the TreeMap

Honestly I kind of hate the name and it's still early so we can change it, but I don't think converting just one of the methods to start to be Java-y works.

Would headerSet be any better? It didn't seem to imply exactly what was going to happen as well as replace.

[Catscratch]

I only reference to Unirest.config().setDefaultHeader.

So my suggestion would be:

• Unirest.config().setDefaultHeader(...) -> overwrite
• Unirest.config().addDefaultHeader(...) -> append

Unirest.header(...).header(...) should stay as it is. :-)

[ryber]

I think what is being asked for though is for the local builder to override the configured default header.

so

//Normally use JSON
Unirest.config().setDefaultHeader("Accept" "application/json");

// Except for this weird one
Unirest.get("http://somexmlservice").header("Accept", "application/xml")

right @philippendrulat ??

[Catscratch]

Philipp is a colleague of mine. And he wants excactly what I asked for. ;-)

Change a specific default header.

[ryber]

ah, so... can you help me understand why you would change the default configuration so much? It's really designed to be done once.

I don't have a problem adding the method you suggest BTW, just trying to understand

[Catscratch]

Of course.

We have some kind of request/response headers to track communication between our services. Therefore every request/response is passed through a javax.ws.rs.container.ContainerRequestFilter. There we modify header etc.

What we need on Unirest-Calls like Unirest.get(...) is a unique identifier in a specific header field. Now we can go through the entire code and add an addHeader(...) on each request or (better) can modify these requests in the Filter to only write the code once.

Maybe you got another good solution using Unirest?

[ryber]

So the only real issue is that Unirest's default configuration is static, so in a heavily used system you could end up replacing the header in the middle of a request. Unless you bind the configuration to the request in some way perhaps in a threadlocal.

Doing that has risks as well, Unirest creates a bunch of monitor threads, and Apache Http Client itself creates worker threads and it needs to be shut down properly.

You could spawn a entire client for each request as long as you shut it down at the end, or have the filter keep a static instance of the Apache client around and reuse it for each request.

[Catscratch]

Hm. Maybe you're right. Heavy load indeed is a problem. Especially on concurrent Unirest calls from different threads. I'll talk to Philipp tomorrow. Thanks for the hint!

[ryber]

I went ahead and added the add vs set methods on the config.

Kong unirest used a single monolithic client. As of 3.0 we now support multiple configurations so It would be possible to spawn a new config per request, but if you do that I think you should try to keep the Apache client around.

[ryber]

I wonder if there is a good use case for using Suppliers here. rather than literal values for some things. Particularly headers?

What if you could do

Unirest.config().addDefaultHeader("trace", () -> threadLocal.get())

[philippendrulat]

I believe this use case is so unique that it would clutter the Unirest API.
We indeed forgot the threading problems of our solution.

I think you could use InheritableThreadLocal to push variables into the thread and its sub threads.

We are currently supplying our own HttpClient to unirest since we want to implement Tokenhandling and renewal of expired tokens. By attaching a default headers variable to the InheritableThreadLocal we are able to keep track of the default headers ourself.

After further discussion we really like the solution and we could make it work.
We would create a static InheritableThreadLocal (threadLocal) in the RequestFilter and add Unirest.config().setDefaultHeader("trace", () -> threadLocal.get()) in the static part of the filter class. This would solve our threading issues

[ryber]

I figured. I want to use it myself https://github.com/OpenUnirest/unirest-java/blob/master/CHANGELOG.md#3203