Clarification: ParameterConverter throwing IllegalArgumentException

[mkarg]

Apparently neither the spec PDF nor the JavaDocs specificy the common sense how an implementation MUST react upon IllegalArgumentException thrown by ParameterConverter.

Jersey for example is silently accepting that a converter was unable to parse a String but simply returns a default value. This is highly problematic, as (a) the common application programmer assumes by intention that incorrect parameters automatically lead to 400 Bad Request without any further action by him, and (b) the common application programmer cannot detect this situation at all, because maybe some request intentionally sents explicitly that same default value, so it is impossible to see whether the default was intentionally sent OR the converter failed due to illegal argument!

Due to that, I'd like that we clearly say in the JavaDocs of ParameterConverter::toString that IllegalArgumentException MUST prevent the resource method from being called, and instead a response of 400 Bad Request has to happen, and I propose that we ask all vendors to treat this as an implementation bug, not as a spec change.

Please discuss!

[chkal]

What about this paragraph in section 3.2?

A `WebApplicationException` thrown during construction of field or
property values using any of the 5 steps listed above is processed
directly as described in <<method_exc>>. Other exceptions thrown
during construction of field or property values using any of the 5 steps
listed above are treated as client errors: if the field or property is
annotated with `@MatrixParam`, `@QueryParam` or `@PathParam` then an
implementation MUST generate an instance of `NotFoundException` (404
status) that wraps the thrown exception and no entity; if the field or
property is annotated with `@HeaderParam` or `@CookieParam` then an
implementation MUST generate an instance of `BadRequestException` (400
status) that wraps the thrown exception and no entity. Exceptions MUST
be processed as described in <<method_exc>>.

According to my understanding this clearly states that a 404 or 400 status code is expected when ParameterConverter::toString throws a IllegalArgumentException. Or am I missing something?

[mkarg]

Good catch! So while I personally would prefer to always use 400, I am okay with 404, too.

So we do not need to change anything, if everybody is fine with the resolution that IllegalArgumentException MUST result in 404 / 400, hence it definitively is a bug that Jersey silently uses the default value.

@andymc12 @jansupol @ronsigal Agreed?

[ronsigal]

With @chkal's excellent catch, I agree. +1.

[spericas]

I concur.

[mkarg]

Thanks, everybody. According to our resultion I have filed Jersey issue #4636 and hope somebody chimes in with a bug fix. :-)

[jansupol]

To me this is in contradiction to what the ParameterConverter javadoc says:

 * @throws IllegalArgumentException if the supplied string cannot be parsed or is {@code null}.

When the supplied String is null, IllegalArgumentException is thrown and the @DefaultValue is used.

[mkarg]

When the supplied String is null, IllegalArgumentException is thrown and the @DefaultValue is used.

Where actually is written that the default value shall be used in that case?

[andymc12]

@jansupol I think that the javadoc text is consistent with the spec text @chkal mentioned. The key part in the spec text is "Other exceptions thrown during construction of field or property values using any of the 5 steps listed above are treated as client errors...".

The javadoc for @DefaultValue states:

Defines the default value of request meta-data that is bound using one of the following annotations: PathParam, QueryParam, MatrixParam, CookieParam, FormParam, or HeaderParam. The default value is used if the corresponding meta-data is not present in the request.

I take that to mean that @DefaultValue should only be used if no value is provided by the client (or if the value was stripped due to some request filter along the request chain). I don't see that default values should be used in error conditions. I think the right behavior should be to handle the IllegalArgumentException by returning a 404 in this case.

[jansupol]

The process is as follows:

1. the header param injection occurs.
2. the provided ParamConverter is used
3. when the param is null, the user defined ParamConverter can convert null into a value, or throw IllegalArgumentException
4. if the user defined ParamConverter did not work for the null value, which can only be determined by the IllegalArgumentException, the DefaultValue is to be used.

Or does someone have another execution chain?

[andymc12]

In @mkarg's scenario (as I understand it), the ParamConverter is throwing the IllegalArgumentException, not returning null. The javadocs seem to indicate that if a null string is passed in then the ParamConverter should throw an IllegalArgumentException rather than returning a null object, but I think that is a separate issue - we could add more clarification there - i.e. add something to say that if the user returns a null object, then the default value will be used.

But in this scenario, if the ParamConverter throws the exception, then the implementation should be going down the error path - it is bad input provided by the client, so the implementation should return a 4XX response.

So I would write the process like:

1. the header param injection occurs.
2. the provided ParamConverter is used
3. ~~when the param is null~~ regardless of what the input string is, the user defined ParamConverter can convert ~~null~~ the input string into a value or throw IllegalArgumentException
4. if the user defined ParamConverter ~~did not work for the null value~~ throws an IllegalArgumentException, the implementation must return a 4XX response ~~which can only be determined by the IllegalArgumentException~~. If the ParamConverter returns null, the DefaultValue is to be used. If the ParamConverter returns a non-null value, that value should be passed to the resource method.

[mkarg]

Yes, Andy, but in fact I would even go one step further looking at the section about default values in https://javaee.github.io/javaee-spec/javadocs/javax/ws/rs/ext/ParamConverter.html: @DefaultValue's value string actually is just the input to the converter in case the request did not contain the parameter according to that paragraph. This is essential to know, because a converter must have the freedom to replace foo by null without triggering usage of the default. Or vice versa, @DefaultValue(null) leads to null given into the converter only in case request did not contain the parameter, and the converter has the freedom to create foo from that! The default value annotation is for meta-data of requests, not for result values of converters!

[spericas]

@DefaultValue's value string actually is just the input to the converter in case the request did not contain the parameter according to that paragraph.

Right. This could be done eagerly or lazily.

This is essential to know, because a converter must have the freedom to replace foo by null without triggering usage of the default.

I suppose this is possible, although not sure what the use case would be.

Or vice versa, @DefaultValue(null) leads to null given into the converter only in case request did not contain the parameter, and the converter has the freedom to create foo from that!

Not following this one. Converters are required to throw an IllegalArgumentException if the parameter isnull. @DefaultValue(null) is counterintuitive and likely something we should disallow.

[mkarg]

This is essential to know, because a converter must have the freedom to replace foo by null without triggering usage of the default. I suppose this is possible, although not sure what the use case would be.

In case some fancy data format wants to transport the string NOTHING to effectively provide a parameter value of null which has a `@DefaultValue("Of Some Non-Null Value"), for example.

Or vice versa, @DefaultValue(null) leads to null given into the converter only in case request did not contain the parameter, and the converter has the freedom to create foo from that! Not following this one. Converters are required to throw an IllegalArgumentException if the parameter isnull. @DefaultValue(null) is counterintuitive and likely something we should disallow.

Good catch! Right, @DefaultValue(null) effectively is invalid due to that restriction and would produce IllegalArgumentException in turn. Maybe we should cleary say that in the JavaDocs of this annotation.

[andymc12]

I don't think @DefaultValue(null) is allowed by the compiler. When I try it, I get The value for annotation attribute DefaultValue.value must be a constant expressionJava(536871525). And according to O'Reilly Java In A Nutshell,

The values of annotation members must be non-null compile-time constant expressions that are assignment-compatible with the declared type of the member.

So I don't think we need to be concerned with that scenario.

[mkarg]

So I don't think we need to be concerned with that scenario.

Good catch!

[jbescos]

This conflicts with the validation.

For example this resource method is supposed to return 400 when no query parameter is specified:

        @GET
        public String test(@NotNull @QueryParam("query") String query) 

Internally ParamConverters$AbstractStringReader is reading the string value, and it throws an IllegalArgumentException because it is null.

That validation will not happen if we modify that SingleValueExtractor.java#L65 to rethrow the exception.