Clarify if ReaderInterceptor must be applied without an entity parameter

[chkal]

Given the following interceptor:

  @Provider
  public class MyReaderInterceptor implements ReaderInterceptor {

    public Object aroundReadFrom(ReaderInterceptorContext context) {
      // do stuff
    }

  }

And a resource like this:

  @Path("some-path")
  public class SomeResource {

    @POST
    public Response post(@FormParam("foobar") String foobar) {
      // do stuff
    }

  }

In this scenario JAX-RS implementation aren't consistent regarding whether the interceptor is invoked or not. This should be clarified in the spec.

The original use case is the CSRF protection implementation in the JSR 371 RI. It uses a special ReaderInterceptor which checks for the required CSRF token in the request body. This interceptor is NOT executed in CXF. Please see mvc-spec/ozark#202 for details.

[mkarg]

Proposal: We change the spec in 2.2-SNAPSHOT so it is clear about the fact that an interceptor MUST NOT be called around parsing the HTTP entity, but it MUST be called around invocation of an MessageBodyReader ONLY. Would that be sufficient?

[chkal]

Would that be sufficient?

No! 😄

See my latest response on the mailing list.

[spericas]

Personally, I feel that interceptors should be called regardless of how data is bound in a JAX-RS application. The JAX-RS processing pipeline (see spec appendix) should not be affected by a developer's choice of @FormParam or @Form or any new binding annotation we define in the future. If there's an HTTP entity that needs to be processed, there must be a reader/writer and an interceptor chain.

If implementations want to provide optimizations for the case when the length of the interceptor chain is 0, then that would be fine.

[mkarg]

Unfortunately this is not what JAX-RS 2.1 specification says currently:

3.3.2.1 Entity Parameters
The value of a parameter not annotated with @FormParam or any of the annotations listed in in Section 3.2, called the entity parameter, is mapped from the request entity body. Conversion between an entity body and a Java type is the responsibility of an entity provider, see Section 4.2. Resource methods MUST have at most one entity parameter.

This is pretty unambiguous about the fact that only entity parameters are to be created by MBRs, and that @FormParam is not an entity parameter. So in turn, @FormParam prevents invocation of MBRs (and ReaderInterceptors).

Appendix A Summary Of Annotations
Form Param ... Specifies that the value of a method parameter is to be extracted from a form parameter in a request entity body. The value of the annotation identifies the name of a form parameter. Note that whilst the annotation target allows use on fields and methods, the specification only requires support for use on resource method parameters.

This does not require that MBR (and ReaderInterceptor) is to be called, and IMHO it does so by good reason: MBRs are not a general means for HTTP entity parsing, these are solely providers for 3.3.2.1 entity parameter instances.

Conclusion: I am -1 for invoking MBR for @FormParam. I am +1 for changing Jersey. FormParam must have the freedom of completely separate parsing technology, just any other param types have.

[mkarg]

Having said that: The former is only valid in the context of v2.1.2 clarifications . I am +1 for rephrasing the mentioned paragraphs in v3.0 as a separate enhancement issue, so vendors and third party extension providers have a chance to get comfortable with the new situation then (i. e. explicitly mandating invocation of MBRs and Interceptors for non-3.3.2.1 entity parameter use cases, including @FormParam).

[chkal]

3.3.2.1 Entity Parameters
The value of a parameter not annotated with @FormParam or any of the annotations listed in in Section 3.2, called the entity parameter, is mapped from the request entity body. Conversion between an entity body and a Java type is the responsibility of an entity provider, see Section 4.2. Resource methods MUST have at most one entity parameter.

This is pretty unambiguous about the fact that only entity parameters are to be created by MBRs, and that @FormParam is not an entity parameter. So in turn, @FormParam prevents invocation of MBRs (and ReaderInterceptors).

I disagree.

This section states that:

• An entity parameter is a parameter not annotated with one if the @*Param annotations.
• The value of an entity parameter is created by invoking a corresponding entity provider.

It does not state that:

• The entity provider MUST only be invoked if there is actually an entity parameter for the matched resource method.

So in my view it would be perfectly fine for a JAX-RS implementation to use the entity provider API to create an entity if there is a request body even if the resource method doesn't declare an entity parameter.

Appendix A Summary Of Annotations
Form Param ... Specifies that the value of a method parameter is to be extracted from a form parameter in a request entity body. The value of the annotation identifies the name of a form parameter. Note that whilst the annotation target allows use on fields and methods, the specification only requires support for use on resource method parameters.

This does not require that MBR (and ReaderInterceptor) is to be called, and IMHO it does so by good reason: MBRs are not a general means for HTTP entity parsing, these are solely providers for 3.3.2.1 entity parameter instances.

Correct, it does not require it. But it is also not forbidden to do so.

I think it all boils down to the question of how a JAX-RS implementation should handle the case I described on the mailing list. Imagine a ReaderInterceptor which modifies the form parameter values submitted by a client. So what is the expected output if this:

@POST
public Response processForm( @FormParam("value") String value, Form form ) {
  System.out.println( "@FormParam:  " + value );
  System.out.println( "Form entity: " + form.asMap().getFirst( "value" ) );
}

I see three possible ways this situation could be handled.

1. The application could see two different values for the same parameter name. The original value submitted by the client for the @FormParam case and the value modified by the interceptor for the Form entity parameter.
2. The application could see the same value in both cases. This would only be possible if the @FormParam values are derived from a Form instance created from the entity provider. IMO this is the most consistent way.
3. Fail at deployment time because using @FormParam and an entity parameter at the same time is forbidden. I don't think that this makes a lot of sense if the entity parameter is a Form.

Obliviously, I prefer option (2), because it is the most consistent one. IMO the @FormParam binding is quite special, because it is the only one which needs to process the request body. Therefore, it must be treated differently. I think all this could be fixed by defining something like:

• If a resource method uses @FormParam bindings, a JAX-RS implementation MUST use the entity provider API to create a javax.ws.rs.core.Form and derive the values for the @FormParam bindings from this instance.
• If a resource methods uses @FormParam bindings, the only allowed entity parameter type is javax.ws.rs.core.Form. All other types MUST result in a deployment error.

I don't think that there is anything in the spec today that would prevent us to define the @FormParam and entity provider handling like this.

[mkarg]

It does not state that: The entity provider MUST only be invoked if there is actually an entity parameter for the matched resource method. So in my view it would be perfectly fine for a JAX-RS implementation to use the entity provider API to create an entity if there is a request body even if the resource method doesn't declare an entity paramete.

You are wrong here. 3.3.2.1 explains what to do with entity parameters. It is part of a list describing the contents a resource method signature is comprised of. Please do not cut single lines out of context. MBRs had been invented solely to provide a means to build entity parameters, nothing else. As I said, we can change that, but for now, it is simply a clear misuse by seeing it out of context. Regarding not forbidding: We invented MBRs for exatly that case, so we only described it in that very particular context. It simply is not a standalone Lego brick but we can make it one. We cannot simply say we interprete it differently in just a bug fix, we really must turn that into a feature, because it has side effects: As of 3.3, it is implied that only one single MBR runs for each call. After our change, any number can run. This could trouble vendors. So +1 for 3.0, but -1 for 2.x.

[chkal]

Don't get me wrong. My suggestion definitely goes beyond a simple clarification. Although I think that adding it in 2.x may work. But we can discuss the target version later after we find an agreement.

As of 3.3, it is implied that only one single MBR runs for each call. After our change, any number can run. This could trouble vendors. So +1 for 3.0, but -1 for 2.x.

I don't get this. Why would the suggestion allow multiple MBRs to be executed? My idea was that if there are @FormParam annotations, the entity parameter (if any) must be a Form. So there will be only one MBR involved.

[spericas]

I agree with @chkal 100%. As as I stated before, binding to Java should not alter how pipelines are configured and executed in JAX-RS. This cannot be more clear than when a JAX-RS client uses a Form entity and the corresponding JAX-RS service binds it to @FormParam(s) (if a client uses an interceptor to, say, encrypt data then the service should use the corresponding one to decrypt!).

Option (2) as described by @chkal above is the way to clarify this in the spec. Implementations could still provide optimizations as long as the behavior is not changed.

[mkarg]

I am neither +1 nor -1 about the next major specification implying changes to existing implementations, mandating the invocation of a MBR in case of no entity parameter being there. For me it is ok if we clearly say in 3.0 that a MBR<Form> (and any interceptors around that) MUST be invoked to feed @FormParam. Unfortunately this is not up to personal preferences but first about what the current spec actually wants: The spec 2.1 as unambiguous if you understand the intention of Chapter 3.3: It does not describe the pipeline, but it describes how to feed parameters. As a logical consequence, when there is no parameter, there is no pipeline. That is not simply an optimization. That is a very direct implementation of the literal words of chapter 3.3. Your interpretation is "http body needs pipeline, pipeline feeds params, don't care where to put it" (front to back) but this is not what the spec says. Chapter 3.3 talks about "there exists a parameter, so we need a pipeline, so we need a https body" (back to front).

To sum up, I need to say that a clarification of 2.x on the idea behind and the actual existing spec text, by all respect to rules of logic and textual semantics, can only be that MBR<Form> MUST NOT be invoked unless there is an entity parameter of type Form. Yes, this implies that the entity parameter really has a different value than what @FormParam will deliver!

For 3.0, I am happy to accept a complete rewrite of chapter 3 to allow the "front-to-back" or "pipeline always" interpretation.

[spericas]

I am neither +1 nor -1 about the next major specification implying changes to existing implementations, mandating the invocation of a MBR in case of no entity parameter being there. For me it is ok if we clearly say in 3.0 that a MBR<Form> (and any interceptors around that) MUST be invoked to feed @FormParam. Unfortunately this is not up to personal preferences but first about what the current spec actually wants: The spec 2.1 as unambiguous if you understand the intention of Chapter 3.3: It does not describe the pipeline, but it describes how to feed parameters. As a logical consequence, when there is no parameter, there is no pipeline.

What? This does not make any sense. The pipeline includes pre and post-matching filters. There is always a pipeline.

AFAICT, there's a single implementation that has decided to optimize this. As as I stated, the optimization should still valid when no interceptors are available. I think this is a rather simple clarification.

@andymc12 Would it be possible to suggest to the CXF team to only optimize in the absence of interceptors? That should make the behavior compatible with the other two major implementations and help MVC in the process.

[andymc12]

@spericas It looks like CXF uses the method signature to determine how to process in the incoming request. I'll continue to investigate an approach that allows CXF to invoke interceptors and MBRs in this scenario, but at this point it looks non-trivial.

That said, I think Markus's interpretation is correct. The text in section 3.3 of the spec talks about the parameter values determining the processing pipeline, not the content of the HTTP request.

3.3.2.1 Entity Parameters
The value of a parameter not annotated with @FormParam or any of the annotations listed in in Section 3.2, called the entity parameter, is mapped from the request entity body. Conversion between an entity body and a Java type is the responsibility of an entity provider, see Section 4.2. Resource methods MUST have at most one entity parameter.

My take (and @mkarg 's, IIUC) is that if there is no entity parameter on the method, then there is no requirement to invoke the entity provider (MBR) or it's processing pipeline (including the interceptors for any MBR).

If the user wants to manipulate the content of the form, they still can - but they would need to use ParamConverters instead of ReaderInterceptors - or else they would need specify an entity parameter on the method.

[spericas]

@spericas It looks like CXF uses the method signature to determine how to process in the incoming request. I'll continue to investigate an approach that allows CXF to invoke interceptors and MBRs in this scenario, but at this point it looks non-trivial.

Ok, great.

That said, I think Markus's interpretation is correct. The text in section 3.3 of the spec talks about the parameter values determining the processing pipeline, not the content of the HTTP request.

3.3.2.1 Entity Parameters
The value of a parameter not annotated with @FormParam or any of the annotations listed in in Section 3.2, called the entity parameter, is mapped from the request entity body. Conversion between an entity body and a Java type is the responsibility of an entity provider, see Section 4.2. Resource methods MUST have at most one entity parameter.

My take (and @mkarg 's, IIUC) is that if there is no entity parameter on the method, then there is no requirement to invoke the entity provider (MBR) or it's processing pipeline (including the interceptors for any MBR).

I don't think that is what it is saying. I interpret it as "If there's an entity parameter then an entity provider must be used". That is certainly not logically equivalent to "If there's NO entity parameter then NO entity provider must be used". In other words, you cannot go from A->B to not(A)->not(B).

[chkal]

My take (and @mkarg 's, IIUC) is that if there is no entity parameter on the method, then there is no requirement to invoke the entity provider (MBR) or it's processing pipeline (including the interceptors for any MBR).

I don't think that is what it is saying. I interpret it as "If there's an entity parameter then an entity provider must be used". That is certainly not logically equivalent to "If there's NO entity parameter then NO entity provider must be used". In other words, you cannot go from A->B to not(A)->not(B).

+1! That was also my interpretation. Of course the spec states that you need an MBR if there is an entity parameter. But saying that this implies that you must not invoke the MBR if there is no entity parameter doesn't make sense.

However, I agree that my proposal (2) may be more than a simple clarification. However, I strongly believe that my proposal makes sense and leads a much more consistent handling of request bodies.

[mkarg]

Again, I have no problem with making MBR invocation mandatory in the future, but for the sake of a clarification we have to stick with the original intention of the spec authors. And that original intention simply is method invocation: See Appendix C. It clearly shows that interceptors and MBRs only are invoked for the sake of method invocation, and method invocation is clearly described in chapters cited already, and it clearly says when to call what. It is obvious that all other cases are valid but not mandatory. So what CXF does is not an "optimization", it is what the spec wants, but simply literally, while Jersey implements a case not covered by the spec. You cannot turn it the other way and say "the undocumented case is the norm, and the literal case is an optimization"!

Update: The graphic in Appendix C even explicitly says that the invocation of the method is the normal case, while the invocation of the MBR and Interceptors is not! Look at the shaded background with the words (optional)! It allows Jersey to invoke the MBR, but the normal case (non-shaded) is to not invoke it!

Apparently we have 2:2 votes, so I would says, the spec should stay as it is and the outcome is:

• CXF is correct (literal interpretation of spec).
• Jersey is correct (liberal interpretation of spec).
• We do not change the spec, as we cannot find a proposal with no -1's.

Agreed?

[andymc12]

@mkarg I agree with your assessment, but I don't agree that the spec is "clear" - otherwise, we wouldn't be having this conversation. :)

In the mean time, for anybody who got here by googling something like "why don't my interceptors fire in CXF when they do in Jersey", the best way to ensure portability in this case is to use ParamConverters rather than interceptors when dealing with @FormParam parameters.

[chkal]

Update: The graphic in Appendix C even explicitly says that the invocation of the method is the normal case, while the invocation of the MBR and Interceptors is not! Look at the shaded background with the words (optional)! It allows Jersey to invoke the MBR, but the normal case (non-shaded) is to not invoke it!

Well, of course the MBR pipeline is optional. But my understanding of the "optional" was actually that you can only invoke the MBR pipeline if there is a request body. Of course you cannot invoke it for GET requests, so the MBR pipeline is optional.

But anyway, I think it is pretty obvious that the current spec wording isn't clear enough. As @andymc12 already mentioned: Otherwise this discussion wouldn't exist. 😆

Apparently we have 2:2 votes, so I would says, the spec should stay as it is [...]

As already mentioned before, I agree that fixing this issue is more than a simple clarification. So maybe we should adjust the title and tag of this issue to reflect this.

But I strongly believe that JAX-RS would behave much more consistent if we enhance the spec as proposed with option (2) in my previous post. Whether this change should get into 2.2 or 3.0 can be discussed later. IMO we should first find the optimal solution and then decide whether we can justify to put it into 2.2 or if we have to wait for 3.0.

Maybe I should file a new issue containing my proposal just to keep the discussion clean enough!? IMO this makes sense, because (2) actually defines how a JAX-RS implementation should obtain @FormParam values but has the nice side effect of fixing this issue.

[spericas]

Update: The graphic in Appendix C even explicitly says that the invocation of the method is the normal case, while the invocation of the MBR and Interceptors is not! Look at the shaded background with the words (optional)! It allows Jersey to invoke the MBR, but the normal case (non-shaded) is to not invoke it!

Well, of course the MBR pipeline is optional. But my understanding of the "optional" was actually that you can only invoke the MBR pipeline if there is a request body. Of course you cannot invoke it for GET requests, so the MBR pipeline is optional.

That is exactly right.

But anyway, I think it is pretty obvious that the current spec wording isn't clear enough. As @andymc12 already mentioned: Otherwise this discussion wouldn't exist. 😆

Agreed with @chkal and @andymc12 on this one.

Apparently we have 2:2 votes, so I would says, the spec should stay as it is [...]

As already mentioned before, I agree that fixing this issue is more than a simple clarification. So maybe we should adjust the title and tag of this issue to reflect this.

But I strongly believe that JAX-RS would behave much more consistent if we enhance the spec as proposed with option (2) in my previous post. Whether this change should get into 2.2 or 3.0 can be discussed later. IMO we should first find the optimal solution and then decide whether we can justify to put it into 2.2 or if we have to wait for 3.0.

As I stated before, I very much agree with option (2).

[mkarg]

+1 for separate issue with a clear proposal what the spec / Javadocs shall be changed like

[spericas]

@mkarg Sure, let's create another issue. Hopefully others will get involved in the discussion and we can move this forward.

[andymc12]

In one of @chkal's comments, there was the issue that CXF was showing different results for an @FormParam parameter than the same form property in the Form entity parameter. I agree that this is a bug, and we are fixing that in the CXF code base. This will get CXF to option (2).

Please note that there is still the main issue addressed here as to whether MBRs/ReaderInterceptors should be invoked if there is no entity parameter. Even with this change, CXF will not invoke the MBRs/ReaderInterceptors unless the resource method has an entity parameter - it sounds like this behavior is different than Jersey and RESTEasy.

[NicoNes]

Hi guys,

I'm facing a strange behavior with both jersey and resteasy about invocation of ReaderInterceptor. I was reading at your discussion hopping I will find an answer but it's not that clear.

Here is my case: consider following resource and providers registered in my JAX-RS application:

@Path("test")
public class TestResource {
	@GET
	public Response getMethodWithEntity(Object entity) {
		return Response.ok().build();
	}
}

public class TestFilter implements ContainerRequestFilter {

	public static final String HAS_ENTITY = "Has entity";

	@Override
	public void filter(ContainerRequestContext containerRequestContext ) throws IOException {
		requestContext.setProperty(HAS_ENTITY, containerRequestContext .hasEntity());
	}
}

public class TestReaderInterceptor implements ReaderInterceptor {

	@Override
	public Object aroundReadFrom(ReaderInterceptorContext context) throws IOException, WebApplicationException {
		boolean hasEntity = (boolean) context.getProperty(TestFilter.HAS_ENTITY);
		if (!hasEntity) {
			System.out.println("Invocation of ReaderInterceptor even if there is no request entity to read");
		}
		return context.proceed();
	}

}

When calling the GET method of TestResource from my browser without sending no request body I was
not expecting the TestReaderInterceptor.aroundReadFrom() to be invoked, but it is. (And to go further, if I register a MBR like following it will be invoked too)

public class TestMBR implements MessageBodyReader<Object> {
	@Override
	public boolean isReadable(Class<?> type, Type genericType, Annotation[] annotations, MediaType mediaType) {
		return true;
	}

	@Override
	public Object readFrom(Class<Object> type, Type genericType, Annotation[] annotations, MediaType mediaType,
			MultivaluedMap<String, String> httpHeaders, InputStream entityStream)
			throws IOException, WebApplicationException {
		return new Object();
	}
}

So based on the spec and what @chkal and @spericas said above

Well, of course the MBR pipeline is optional. But my understanding of the "optional" was actually that you can only invoke the MBR pipeline if there is a request body. Of course you cannot invoke it for GET requests, so the MBR pipeline is optional.

should the MBR pipeline and TestReaderInterceptor.aroundReadFrom() be invoked when there is no request body (containerRequestContext .hasEntity() returns false) ?

[chkal]

To be honest, I'm also surprised that the interceptor is called even for GET requests. However, I don't think the spec is very strict here. So IMO this is currently no violation of the spec.

[chkal]

And a second thought: I'm pretty sure that CXF won't invoke it in this case.

[NicoNes]

However, I don't think the spec is very strict here. So IMO this is currently no violation of the spec.

Yes but there is something to clarify here.
IMO, if MBR are here to map request body to JAVA type there is no need to invoke them when there is no request body.
This way, it will also be consistent with what is done for MBW. MBW pipeline is not invoked when there is no response body (containerResponseContext.hasEntity() returns false).

[spericas]

@NicoNes I tend to agree that it shouldn't be called in this case. Is this in general or a result of using a GET with an entity? Does it happen with PUT or POST as well?

[NicoNes]

@spericas It's general, it happens for GET, PUT and POST as well

[chkal]

I finally found some time to prepare a pull request for the idea I mentioned in #659 (comment). Please refer to #695 for details. If merged, the original problem described in #659 (comment) will be resolved automatically.

[spericas]

@chkal Do you agree that the issue reported by @NicoNes here is orthogonal to the @FormParam one that you're working on? We should create another issue IMO.

[chkal]

@spericas I agree. The original intent of this issue was to clarify if entity interceptors have to be applied if there is only a @FormParam but not an entity parameter. This will hopefully be fixed by #695.

@NicoNes Could you file a separate issue for the topic you brought up?

[chkal]

I just merged #695. So I guess we can close this now.

To sum up: The values of @FormParam bindings are resolved via a javax.ws.rs.core.Form instance which is created by executing the corresponding MBR. So all implementation must invoke the reader interceptors if there is at least one @FormParam.

Feel free to reopen this issue if you think that this needs more clarification.

[NicoNes]

@NicoNes Could you file a separate issue for the topic you brought up?

Yep just created it: #711

[mkarg]

Do we need to mention this in the spec, at least in the history appendix?