helidon-io / helidon

Java libraries for writing microservices
https://helidon.io
Apache License 2.0
3.52k stars 564 forks source link

Request filter is skipped for Helidon-provided endpoints and non-existent endpoints; response filter is invoked for all #9445

Open tjquinno opened 2 weeks ago

tjquinno commented 2 weeks ago

Environment Details


Problem Description

In an MP app, a request filter is skipped for Helidon-provided endpoints such as /metrics, /health, and /openapi. It is also skipped for non-existent endpoints.

A response filter, on the other hand, is invoked for all of the above.

Steps to reproduce

  1. Expand the attached zip file.
  2. Build and run the project which contains a class that implements both ContainerRequestFilter and ContainerResponseFilter.
  3. curl http://localhost:8080/greet. Both filter methods log a message.
  4. curl http://localhost:8080/metrics. The request succeeds but only the response filter is invoked.
  5. curl http://localhost:8080/bad. The request fails with the expected 404; only the response filter is invoked.

MPFilterExample.zip

spericas commented 1 week ago

@tjquinno This is working as designed once the Helidon integration with Jakarta REST is understood. The CustomFilter is a global filter because it has no name binding annotations. All global filters need to be invoked according to the Jakarta REST spec (this enables some filters, for example, to convert a 404 to a 200 if necessary).

  1. curl http://localhost:8080/metrics calls the response filter because /metrics is first tried with Jersey and then with the Helidon WebServer. The status code in the filter will be 404. Once the 404 is processed by the Helidon WebServer, the actual endpoint is found and called (converting the 404 to a 200).
  2. curl http://localhost:8080/bad calls the response filter for the same reason described in (1). However, since /bad is unknown to the WebServer, the result is still a 404.

The existing behavior in (1) and (2) can be altered by name binding the filter with the resource in question (or registering it manually to avoid making it global). That filter will never be called if the resource is not found. See Name Binding.

Could the Helidon WebServer avoid calling '/metrics' when it recognizes the endpoint as valid? This is a question for @tomas-langer: seems like the current approach is to always try Jersey first and the WebServer second.

tjquinno commented 15 hours ago

@spericas I wanted to restate what you said to make sure I understand.

Helidon MP delegates each request to Jersey to give it a chance to process the request.

IIUC Jersey invokes the Jakarta REST request filter (CustomFilter in the attached project) only for requests that it recognizes (matches) based on the Jakarta REST resources it knows about. But it invokes the response filter for all requests it was asked to try, matched or not.

Because Helidon MP first asks Jersey to try each request (including /metrics and /health), and because the resources known to Jersey do not match those paths, then Jersey does not invoke the request filter in those cases but Jersey does invoke the response filter in those cases. And the status in the Jersey response context is indeed 404.

After the Jersey processing (including the response filter invocations) has completed, Helidon checks the result from delegating the request to Jersey, sees that Jersey did not handle the /metrics or /health request, and so tries to handle the request with its own webserver. That works, the status is set to 200, and the response is populated with the metrics or health output and sent to the client.

That explains what users are seeing and that's what Jersey is required to do by the Jakarta REST spec.

You've explained how name binding could change this. IIUC the @PreMatching annotation applied to the filter class would also allow the request filter to be invoked for every request Jersey is asked to handle, but the request context passed to the filter in that case is different because the filter receives both matched and unmatched requests.

spericas commented 1 hour ago

@tjquinno Yes, @PreMatching should invoke the request filters with less context info.