Define how @FormParam values are obtained

[chkal]

The @FormParam binding annotation is special, because it is the only binding annotation that must consume the request body. Therefore, we need to define how @FormParam relates to other mechanisms accessing the request body, especially the MBR.

As both @FormParam and MBRs need to consume the request body, weird edge cases can occur.

• What happens if there is a @FormParam annotation AND the resource method declares an entity parameter of some arbitrary type. This would mean that two components need to process the entity body: the JAX-RS implementation to get the value for the @FormParam and the corresponding MBR to deserialize the entity parameter. The only way to achieve that would be to buffer the request body input stream, which is very bad for performance, especially for very large request bodies.
• What happens if a ReaderInterceptor modifies the request body? If the developer reads the form using a javax.ws.rs.core.Form entity parameter, the interceptor would be applied. But what if the developer uses a @FormParam annotation? Will he get the modified or the original value? What if the developer uses both ways? Will the values differ?

IMO the JAX-RS spec should define how this situation is handled. This is necessary to get consistent behavior between JAX-RS implementations and even within a single implementation.

This pull requests adds a corresponding description to the API docs of the @FormParam annotation. The text specifies that:

• If there is at least one @FormParam annotation, JAX-RS implementations must create a javax.ws.rs.core.Form using the entity provider API (which includes executing the full interceptor chain) and then read the form value from this instance. This ensures that developers get consistent behavior and that only one component (the MBR) is consuming the request body.
• On top of that the API docs clarify that if at least one @FormParam is used, no other entity parameter than javax.ws.rs.core.Form is allowed for the resource method. That's basically a consequence of the previous requirement.

Let me know what you think. Please note that all this originated from the discussions in #659. Please refer to the corresponding issue for details.

EDIT: The original proposal was adjusted: Instead of forbidding other entity parameter types (if there is at least one @FormParam) it is explicitly mentioned that it is optional.

[mkarg]

As the addition of "...entity provider..." here effectively overrides the explicit exception of section 3.3.2.1, we should remove that exception "...not annotated with @FormParam..." from the specification.

[spericas]

@chkal Looks good.

@mkarg Are you saying that a parameter annotated with @FormParam should be called an entity parameter? This is clearly an exception, but I don't like this suggestion since there could be many @FormParam and an entity param is just one.

[chkal]

@mkarg Could you elaborate a bit more? Are you saying that @FormParam annotated fields/parameters should be called an entity parameter? Although @FormParam values are obtained by consuming the request body, an entity parameter is something completely different in my opinion.

@spericas Feel free to approve the if you are +1 for the proposal.

@eclipse-ee4j/eclipse-jaxrs Any other opinions?

[mkarg]

@chkal Sorry for being silent on this for so long. I had a water in my house so I only could answer few threads. I will continue ASAP.

[spericas]

@chkal Approved now. My apologies, thought I had approved it already.

[mkarg]

Sorry for the very long delay.

@spericas Are you saying that a parameter annotated with @FormParam should be called an entity parameter?

I do not say that, but it is implied by the fact that the usage of entity providers is only covered by the spec's section talking about entity parameters and the fact that @chkal's change enforces that no entity parameters may be used when @FormParam is inplace. The current text of the spec does not explicitly allows using entity providers for @FormParam; moreover it currently explicitly forbids it in the quoted chapter.

So if we want @chkal's change, and if we want to end the dispute / confusion about what the spec means, a spec change is essential ontop of this PR.

[osvaldopina]

4j4

…

On Mon, Nov 5, 2018, 04:12 Markus KARG ***@***.*** wrote: ***@***.**** commented on this pull request. As the addition of "...entity provider..." here effectively overrides the explicit exception of section 3.3.2.1, we should remove that exception "...not annotated with @FormParam..." from the specification. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#695 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABN-UZM0t0poRWdw9rYTkwMKOqFPzsyzks5ur9bZgaJpZM4YNcZA> .

[osvaldopina]

👏😉👏👏😁👏😀😁qn😉😀👏👏👏👏

…

On Mon, Nov 5, 2018, 04:12 Markus KARG ***@***.*** wrote: ***@***.**** commented on this pull request. As the addition of "...entity provider..." here effectively overrides the explicit exception of section 3.3.2.1, we should remove that exception "...not annotated with @FormParam..." from the specification. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#695 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABN-UZM0t0poRWdw9rYTkwMKOqFPzsyzks5ur9bZgaJpZM4YNcZA> .

[spericas]

Sorry for the very long delay.
The current text of the spec does not explicitly allows using entity providers for @FormParam; moreover it currently explicitly forbids it in the quoted chapter.

We already had this discussion. Both @chkal and I explained very clearly why this statement is false. The spec says A -> B and you're concluding that not(A) -> not(B), which is different.

So if we want @chkal's change, and if we want to end the dispute / confusion about what the spec means, a spec change is essential ontop of this PR.

We can certainly elaborate this in this spec a bit more (in fact that is in @chkal's proposal above), but we wouldn't be changing the spec in a non-compatible way.

[chkal]

and the fact that @chkal's change enforces that no entity parameters may be used when @FormParam is inplace.

That's not correct. You can declare an entity parameter even if you are also using @FormParam, but in this case the entity parameter type is restricted to javax.ws.rs.core.Form.

moreover it currently explicitly forbids it in the quoted chapter.

I don't think that anything in this chapter forbids it. But maybe I'm missing something. Could you point us to the corresponding sentence?

[mkarg]

and the fact that @chkal's change enforces that no entity parameters may be used when @FormParam is inplace.

That's not correct. You can declare an entity parameter even if you are also using @FormParam, but in this case the entity parameter type is restricted to javax.ws.rs.core.Form.

Yes, I meant exactly that, sorry for being misleading. But what I wanted to say is that before your proposed change it was possible to have @FormParam being used while other types of entity parameters are used, but now after your change that is forbidden. Maybe I missed something in the spec that prevents applications from doing that before? So an existing application MUST get modified, or it won't work anymory. Hence, the proposed change is not backwards compatible. So IMHO we can do it in 3.0 but not in 2.2, as it impacts not only implementations (which would be ok for minor releases) but also the application (which is only ok in major releases).

moreover it currently explicitly forbids it in the quoted chapter.

I don't think that anything in this chapter forbids it. But maybe I'm missing something. Could you point us to the corresponding sentence?

Yes, we had that discussion before and we agreed that we disagree on the interpretation of the text. And we argreed that we need to clarify the text to make it unambiguous to people like me that entity providers may be used for other thing than entity parameters. That's why I asked for changes to the spec text in addition to this PR.

JAX-RS 2.1: 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 section clearly says that entity prividers are for mapping variables that are not annotated with @FormParam. That bold constraint makes simply no sense once we allow (or even enforce) the use of entity providers for exactly that parameters by the trick of enforcing them to be Form. If someone thinks different, then please tell me the sense of keeping that bold constraint? How shall this work "must not be @FormParam to do X but you can do X if its type is Y and its type is Y because you use @FormParam"? This forms a paradoxon. That's why I ask for a spec change that removes the bold constraint.

[spericas]

JAX-RS 2.1: 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 section clearly says that entity prividers are for mapping variables that are not annotated with @FormParam.

The first part is basically defining what an entity param is. A definition cannot possibly contradict anything that @chkal says.

The second part, again, it says that if an entity param then an entity provider must be used. This is not an if and only if, so it does not mean that entity providers must only be used for entity params.

[chkal]

But what I wanted to say is that before your proposed change it was possible to have @FormParam being used while other types of entity parameters are used, but now after your change that is forbidden.

I wonder if JAX-RS implementations actually support such a case. As already mentioned before, this would require that the entity stream (which could be really huge) is buffered somehow because it needs to be consumed twice.

[mkarg]

I actually do not care whether there actually exists an implementation that follows this ill approach, but only about the fact that it could exist. If it could exist, we must not restrict it in 2.2 but only in 3.0.

If we follow Santiago's interpretation that this chapter is not meant as it could be read (or: as it is interpreted by at least myself), we should really change the wording to make it unambiguously clear.

[chkal]

I wonder if JAX-RS implementations actually support such a case.

Answering my own question: I just did some tests to see how different JAX-RS implementations handle the case of a resource method that uses both a @FormParam AND an entity parameter:

• Glassfish/Jersey
  • Using @FormParam and a custom entity parameter type (with a corresponding MBR) works fine. I guess Jersey uses some kind of buffering to achieve this? @jansupol: Any idea?
• Wildfly/RESTEasy
  • @FormParam only works if there is no entity parameter. Even adding javax.ws.rs.core.Form breaks @FormParam. The value is null in this case.
• TomEE/CXF
  • @FormParam works only if there is no entity parameter OR a javax.ws.rs.core.Form parameter. Using other entity parameter types breaks @FormParam.

Three implementation, three different behaviors. So it looks like there was never any portable behavior for this case. 😆

However, I agree with @mkarg that my proposed wording would actually not allow using @FormParam with custom entity parameter types, which is actually supported by Jersey. And after thinking about this more, I don't see a good reason to forbid this case.

So we could replace this:

This implies, that javax.ws.rs.core.Form is the only allowed entity parameter type if there is at least one @FormParam annotation for a request.

With something like this.

If there is at least one @FormParam for a resource method, JAX-RS implementations MUST support a javax.ws.rs.core.Form entity parameter for the same method. Support for other entity parameter types is OPTIONAL.

@mkarg Do you agree that the proposed change could go into 2.2 in this case?

[mkarg]

@chkal As the new proposal seems to simply cover the existing behaviors, I do not see a reason against adding this line to 2.2. On the other hand, does it really bring an improvement to the situation? I thought what you wanted to achieve is to get not only predictable behavior, but same behavior? So I assume you will open another PR against 3.0 which contains the actually wanted "fix"?

[chkal]

As the new proposal seems to simply cover the existing behaviors, I do not see a reason against adding this line to 2.2.

Great! I'll update the PR accordingly.

I thought what you wanted to achieve is to get not only predictable behavior, but same behavior? So I assume you will open another PR against 3.0 which contains the actually wanted "fix"?

Well, I actually wanted to achieve that we get a single source of truth for form parameter values. If you now create a ReaderIntercetor which modifies the request body, you will see the corresponding change in both @FormParam and in your entity parameter. IMO it is very important to enforce this. Especially because it enforces that the ReaderInterceptor is called even if you are just using @FormParam.

I could imagine that users want to use both @FormParam and javax.ws.rs.core.Form in a single resource method. This could be useful if you have a very dynamic form for which you want to read the values from a map-like structure. So supporting this case seems reasonable.

However, I don't see any use case for using @FormParam with other entity parameter types. Especially because this would require buffering of the request body, so it can be consumed multiple times. I don't think that we need to forbid this combination (especially, because Jersey supports it). So making it optional is the best solution from my point of view.

I don't think that there is anything more we need to do for JAX-RS 3.0.

[mkarg]

@chkal Thank you for this explanation. I support this approach.

[mkarg]

@chkal Can you please modify the one line as we agreed upon earlier this week?

So we could replace this:
This implies, that javax.ws.rs.core.Form is the only allowed entity parameter type if there is at least one @FormParam annotation for a request.
With something like this.
If there is at least one @FormParam for a resource method, JAX-RS implementations MUST support a javax.ws.rs.core.Form entity parameter for the same method. Support for other entity parameter types is OPTIONAL.

[mkarg]

@asoldano @andymc12 I kindly like to ask you to check this proposal in-depth against your JAX-RS products and cast a review vote within the next days. This PR is open since several weeks and I really would like to have all known vendors on board when deciding about controversal discussions like this one. Thanks.

[chkal]

I just updated the API docs as discussed!

[spericas]

LGTM

[mkarg]

@chkal Merge?

[chkal]

@mkarg I agree. The PR is in review for almost 4 weeks, and we have enough approvals. So I'll merge this now.