Skip to content

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

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

Closed
haroon-sheikh opened this issue Jul 28, 2020 · 9 comments

Comments

@haroon-sheikh
Copy link
Contributor

haroon-sheikh commented Jul 28, 2020

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:

  • Java Version - 11
  • Version [e.g. 3.8.06]

Additional context
Add any other context about the problem here.

@ryber
Copy link
Collaborator

ryber commented Jul 28, 2020

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
Copy link
Contributor Author

haroon-sheikh commented Jul 28, 2020

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 added a commit that referenced this issue Aug 2, 2020
@ryber
Copy link
Collaborator

ryber commented Aug 2, 2020

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

@ryber ryber closed this as completed Aug 2, 2020
@haroon-sheikh
Copy link
Contributor Author

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
Copy link
Collaborator

ryber commented Aug 3, 2020

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
Copy link
Contributor Author

haroon-sheikh commented Aug 3, 2020

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
Copy link
Collaborator

ryber commented Aug 3, 2020

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
Copy link
Contributor Author

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

@ryber
Copy link
Collaborator

ryber commented Aug 3, 2020

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

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

No branches or pull requests

2 participants