rest icon indicating copy to clipboard operation
rest copied to clipboard

[Spec] `abortWith` and `WebApplicationException` MUST be allowed *everywhere*

Open mkarg opened this issue 5 years ago • 4 comments

JAX-RS currently officially supports abortWith and WebApplicationException only in particular places (e. g. in resource methods). While it does work in some implementations to e. g. throw WebApplicationException from within a CDI interceptor, this simply is not guaranteed to work in all implementations. The same is true for caching the ContainerRequestContext e. g. to abortWith at a later time in a CDI interceptor.

To better integrate with CDI, I'd like to propose that the JAX-RS specification shall clearly enforce that all implementations MUST allow abortWith on cached ContainerRequestContexts and throwing WebApplicationException outside of that few locations mentioned in the current specification.

(See also #825)

Commiters, please discuss:

  • Do we want this?
  • In what specification version?
  • Who provides the pull request?
  • Who provides the TCK?
  • Who provides the compliant implementation?

mkarg avatar Jan 01 '20 11:01 mkarg

Regarding abortWith: This method is provided by ContainerRequestContext, which is actually a parameter object for the ContainerRequestFilter API. The API docs of ContainerRequestContext state:

A mutable class that provides request-specific information for the filter, such as [...]

I guess from this wording it is clear, that the context object should be used only be the filter itself. Actually the API docs of the method even state:

Abort the filter chain with a response. This method breaks the filter chain processing and returns the provided response back to the client.

I don't think that it makes much sense to explicitly allow calling this method from outside the filter chain. Especially because this creates some weird corner cases like resource methods which call abortWith AND also return some result. However, of course we could make the expected usage more explicit.

Regarding WebApplicationException: In my view we should really clarify how CDI resource methods are invoked as part of our improved CDI integration. If we explicitly enforce that implementations request an instance of the resource class from the CDI container and then invoke the resource method on this instance, we get a few benfits:

  • It much clearer that the scope of the CDI resource is correctly respected. This was already discussed several times before, because the current wording in the spec is a bit vague.
  • It is clear that if there is an CDI interceptor active for the resource, the instance received from the CDI container will be a proxy which handles the invocation of the interceptor correctly. Therefore, the JAX-RS implementation cannot even know if an exception was thrown by the resource or by an interceptor.

Any other thoughts?

chkal avatar Jan 02 '20 06:01 chkal

Regarding the filter change: Well, the idea of this issue is to change the spec text. ;-) The idea is that the context is not a filter chain context anymore but a CDI-injectable request and response context, so it provides access to the request and response (incl. all properties) and it allows to break the request and response processing (i. e. what the spec means with "filter chain"). This makes a lot of sense, as CDI extensions can be written which completely replace the filtering (in fact I do not see any need for particular filter interfaces and contexts once we would have that, as filters are nothing else than sorted resource method interceptors).

mkarg avatar Jan 02 '20 08:01 mkarg

as filters are nothing else than sorted resource method interceptors

Have you thought about @PreMatching filters?

completely replace the filtering

I believe that users would have been very unhappy should they need to refactor their existing application filters to CDI extensions upon migration to a newer API.

jansupol avatar Jan 07 '20 19:01 jansupol

completely replace the filtering I believe that users would have been very unhappy should they need to refactor their existing application filters to CDI extensions upon migration to a newer API.

I didn't meant to actually remove the existing functionality, but I meant that new filters could be written by solely using CDI.

mkarg avatar Jan 07 '20 19:01 mkarg