Kong / unirest-java

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

Using multiple interceptors only produces the output from the last interceptor. #363

Closed haroon-sheikh closed 4 years ago

haroon-sheikh commented 4 years ago

Describe the bug When using configuring multiple interceptors, only the last one in the config produces output.

To Reproduce

  1. Configure two interceptors
Unirest.config()
        .interceptor(new Interceptor() {
            @Override
            public void onRequest(HttpRequest<?> request, Config config) {
                System.out.println("Output from first interceptor");
            }
        }).interceptor(new Interceptor() {
    @Override
    public void onRequest(HttpRequest<?> request, Config config) {
        System.out.println("Output from Second interceptor");
    }
});
Unirest.get("https://reqres.in/api/users?page=2").asJson();

Expected behavior Outcome from both interceptors

Output from first interceptor
Output from Second interceptor

but we only see the output from the 2nd interceptor.

Output from Second interceptor

Screenshots If applicable, add screenshots to help explain your problem.

Environmental Data:

Additional context Add any other context about the problem here.

ryber commented 4 years ago

There is in fact, only 1 interceptor, so in your example you're just replacing it. I realize the doc may not be clear about this and I'll update that.

You could make a compound interceptor that loops over them and does each.

haroon-sheikh commented 4 years ago

Thanks @ryber - is there a possibility of supporting multiple in future?

Also any tips/examples I can look at for creating a compound interceptor?

ryber commented 4 years ago

So I started to make an example compound interceptor and then realized it would be better just to use it internally to unirest all the time and support multiple interceptors with it.

done in 3.9.00

haroon-sheikh commented 4 years ago

Thanks @ryber - that was really quick. 👍 I've tested the changes on 3.9.00 and everything looks to work as expected, just wanted your view on this:

        Unirest.config()
                .interceptor(new Interceptor() {
                    @Override
                    public void onRequest(HttpRequest<?> request, Config config) {
                        System.out.println("Output from first interceptor");
                    }
                }).interceptor(new Interceptor() {
            @Override
            public void onRequest(HttpRequest<?> request, Config config) {
                System.out.println("Output from Second interceptor");
            }
        });
        Unirest.get("https://reqres.in/api/users?page=2").asJson();

When calling Unirest.config().interceptor("another interceptor");, should it add this new interceptor the existing list of interceptors or should it create a new list with Default + new interceptor only?

I'm setting the interceptors config as part of a constructor, and every time this object is created, it appends the same interceptors to the list and I see duplicated results onRequest/onResponse.

ryber commented 4 years ago

The configs are managed instances, so if you need adding new instances of the same interceptor it's going to keep adding new ones. It does check that the interceptor is already in the list or not so as long as you implement equals if will not add it twice.

haroon-sheikh commented 4 years ago

How do you mean by implementing an equals?

I'm just trying to make it so that it doesn't add same interceptor to list twice? Are you maybe able to add a unit test for this?

ryber commented 4 years ago

Two anonymous objects of the same interface in java are not the same. They are different instances. Like with any java object you could override equals/hash code to make them the same or you could pass in a singleton instance of the interceptor or you could set it up as a static variable.

In any case Configs in unirest are not designed to be messed with a lot. They should mostly be done once.

haroon-sheikh commented 4 years ago

Okay that makes sense. I'll have a play with static variables. :)

ryber commented 4 years ago

One thing to be aware of is that Unirest.config() is of course static itself. It will be the same instance of the config everywhere you use it in the JVM. So the recommendation is to do that on startup once, rather than before you invoke Unirest