Inconsistent handling of invalid and empty strings for numeric parameters

[joschi]

This is a follow-up of the discussion with @jbescos in #4651 (comment).

The handling of invalid strings and empty strings is inconsistent for numeric query parameters.

Intuitively, I would expect the strings "" (empty string) and "not-a-number" (invalid string when trying to parse a numeric value) to lead to the same result, but Jersey currently handles them differently.

This also leads to an unidiomatic use of the Optional<T> container for which support was introduced just recently in Jersey 2.34.

The following example (using the Jersey testing framework) illustrates the issue:

import org.glassfish.jersey.server.ResourceConfig;
import org.glassfish.jersey.test.JerseyTest;
import org.junit.Test;

import javax.ws.rs.*;
import javax.ws.rs.core.*;
import java.util.Optional;

import static org.junit.Assert.assertEquals;

public class OptionalTest extends JerseyTest {
    @Override
    protected Application configure() {
        return new ResourceConfig(TestResource.class);
    }

    @Test
    public void testOptionalInteger() {
        String paramMissing = target("/optionalInteger").request().get(String.class);
        assertEquals("default", paramMissing);

        String paramProvided = target("/optionalInteger").queryParam("i", 42).request().get(String.class);
        assertEquals("i was 42", paramProvided);

        Response paramInvalidResponse = target("/optionalInteger").queryParam("i", "not-a-number").request().get();
        assertEquals(404, paramInvalidResponse.getStatus());

        Response paramEmptyResponse = target("/optionalInteger").queryParam("i", "").request().get();
        assertEquals(404, paramEmptyResponse.getStatus()); // 🛑 <-- Fails with response status: 200, response body: "null")
    }

    @Test
    public void testInteger() {
        String paramMissing = target("/integer").request().get(String.class);
        assertEquals("null", paramMissing);

        String paramProvided = target("/integer").queryParam("i", 42).request().get(String.class);
        assertEquals("i was 42", paramProvided);

        Response paramInvalidResponse = target("/integer").queryParam("i", "not-a-number").request().get();
        assertEquals(404, paramInvalidResponse.getStatus());

        Response paramEmptyResponse = target("/integer").queryParam("i", "").request().get();
        assertEquals(404, paramEmptyResponse.getStatus()); // 🛑 <-- Fails with response status: 200, response body: "null")
    }

    @Test
    public void testPrimitiveInt() {
        String paramMissing = target("/int").request().get(String.class);
        assertEquals("i was 0", paramMissing);

        String paramProvided = target("/int").queryParam("i", 42).request().get(String.class);
        assertEquals("i was 42", paramProvided);

        Response paramInvalidResponse = target("/int").queryParam("i", "not-a-number").request().get();
        assertEquals(404, paramInvalidResponse.getStatus());

        String paramEmpty = target("/int").queryParam("i", "").request().get(String.class);
        assertEquals("i was 0", paramEmpty);
    }


    @Produces(MediaType.TEXT_PLAIN)
    @Path("/")
    public static class TestResource {
        @GET
        @Path("/optionalInteger")
        public String optionalInt(@QueryParam("i") Optional<Integer> i) {
            if (i == null) {
                return "null";
            }
            return i.map(param -> "i was " + param).orElse("default");
        }

        @GET
        @Path("/integer")
        public String integer(@QueryParam("i") Integer i) {
            if (i == null) {
                return "null";
            }
            return "i was " + i;
        }

        @GET
        @Path("/int")
        public String primitiveInt(@QueryParam("i") int i) {
            return "i was " + i;
        }
    }
}

[jbescos]

This is happening because of org.glassfish.jersey.internal.inject.ParamConverters#fromString

There is special treatment for empty strings:

        @Override
        public T fromString(final String value) {
            if (value == null) {
                throw new IllegalArgumentException(LocalizationMessages.METHOD_PARAMETER_CANNOT_BE_NULL("value"));
            }
            try {
                return _fromString(value);
            } catch (final InvocationTargetException ex) {
                // if the value is an empty string, return null
                if (value.isEmpty()) {
                    return null;
                }
                final Throwable cause = ex.getCause();
                if (cause instanceof WebApplicationException) {
                    throw (WebApplicationException) cause;
                } else {
                    throw new ExtractorException(cause);
                }
            } catch (final Exception ex) {
                throw new ProcessingException(ex);
            }
        }

The execution is catch in the section catch (final InvocationTargetException ex)

There is a check for empty values:

                // if the value is an empty string, return null
                if (value.isEmpty()) {
                    return null;
                }

It returns null, but when value is not empty and it is not a number it re-throws an exception.

I have to check whether this is fine or not.

[jbescos]

It looks it was introduced in 2012 in this commit:
Issue JERSEY-1220 fixed: javaee/jersey@7652780

I cannot find that issue 1220 to get more context about this change, but it looks this is the correct behavior so we can close this @joschi

[joschi]

@jbescos Thanks for looking into this!

As @dsvensson mentioned #4555 (comment), this behavior makes sense for strings but not for other types which have to be converted from strings to the actual type, such as numeric types or UUIDs.

I stand by the statement that any implementation which will return null for a parameter of type Optional<T> is inherently broken and will invite a lot of bugs because people just don't expect an Optional being null (instead of Optional#empty()).

[joschi]

@jbescos Is there a way to override the providers/ParamConverters which are shipped by Jersey so that we could write or keep our own implementations for Optional<T> and Optional{Double,Int,Long}?

• https://github.com/dropwizard/dropwizard/tree/v2.0.25/dropwizard-jersey/src/main/java/io/dropwizard/jersey/optional
• https://github.com/dropwizard/dropwizard/blob/v2.0.25/dropwizard-jersey/src/test/java/io/dropwizard/jersey/optional/OptionalQueryParamResourceTest.java

[jbescos]

I think it makes sense that only applies for Strings. But it is strange that it only enters in that catch if it is not String. So, if I want to modify that behavior, I need to literally remove that part that was added in this commit javaee/jersey@7652780

I guess it was added because of one reason.

I can try to remove it and run all the tests, to see what happens.

[jbescos]

There are some tests failing:

[ERROR] Failures: 
[ERROR]   CookieParamStringConstructorTest.testStringConstructorListEmptyGet:163->AbstractTest._test:80 expected:<content> but was:<null>
[ERROR]   HeaderParamStringConstructorTest.testStringConstructorListEmptyGet:179 expected:<content> but was:<null>
[ERROR]   MatrixParamStringConstructorTest.testStringConstructorListEmptyGet:156->AbstractTest._test:80 expected:<content> but was:<null>
[ERROR]   QueryParamStringConstructorTest.testStringConstructorListEmptyGet:168->AbstractTest._test:80 expected:<content> but was:<null>
[ERROR]   QueryParamStringConstructorTest.testStringConstructorNullGet:175->AbstractTest._test:84->AbstractTest._test:80 expected:<content> but was:<null>

I focused in one that expects BigDecimals as input query parameters. When empty string is sent, it is expected to receive the request in the @GET method.

It looks the way it is working now it is the expected behavior.

[mleegwt]

Should this issue have a milestone? This issue blocks version updates in Dropwizard. Vulnerability checkers therefore correctly detect existing vulnerabilities. Would assigning a milestone help with improved focus?

[trnl]

Colleagues, I wonder if any work is done on it? I can probably take a look otherwise.

[jbescos]

Colleagues, I wonder if any work is done on it? I can probably take a look otherwise.

I created a PR with the suggested changes, but that made some tests to fail. See my comment: #4790 (comment)

It looks the current behavior is correct.

You can check out my PR if you want to try: #4850

[trnl]

Thank you @jbescos, will check it out.

[easbar]

Any updates on this? The mentioned PR #4850 was closed in the mean time.

[jbescos]

@easbar, the conclusion right now is that the current behavior is correct, because there are already tests verifying it. Unless somebody finds in the spec that it should work in other way, I don't expect changes here.

[joschi]

the conclusion right now is that the current behavior is correct, because there are already tests verifying it.

That's a non sequitur, isn't it? The tests can also verify incorrect behavior. 😉

The Javadoc for ParamConverter<T>#fromString(String) says:

T fromString(String value)
Parse the supplied value and create an instance of T.

Parameters:
value - the string value.

Returns:
the newly created instance of T.

Throws:
IllegalArgumentException - if the supplied string cannot be parsed or is null.

The last part ("Throws") means that the behavior of the current implementation is incorrect since the empty string is not a valid integer number and thus cannot be parsed and should result in the mentioned exception which would cause an HTTP error response.

Instead of failing, the current implementation constructs a null instance for the parameters (see also testOptionalInteger() and testInteger() tests in my first message).

[jbescos]

@jansupol @senivam what do you think?.

I have created a new PR with the changes requested: #5184

There are a couple of FIXMEs that I need to investigate.

[jbescos]

I removed that PR and I created a new one that looks better:
#5190

[jansupol]

@joschi
The following statements are quite different. The first sends some text which is then required to be converted to a number, which would correctly return 404.

Response paramInvalidResponse = target("/optionalInteger").queryParam("i", "not-a-number").request().get();
assertEquals(404, paramInvalidResponse.getStatus());

The second sends null to optional:

Response paramEmptyResponse = target("/optionalInteger").queryParam("i", "").request().get();
assertEquals(404, paramEmptyResponse.getStatus()); // 🛑 <-- Fails with response status: 200, response body: "null")

What makes you think that optional cannot be null?

[joschi]

The second sends null to optional:

Why is it null in the first place? The query parameter is defined, so shouldn't it be the empty string?

What makes you think that optional cannot be null?

The semantics of the Optional<T> class. 😓

The purpose of java.util.Optional<T> (as well as the primitive wrappers OptionalDouble, OptionalInt, and OptionalLong) is to avoid null references.

If Jersey doesn't guarantee that a variable of type Optional<T> is never null, then I think support for Optional<T> should be removed altogether and leave it up to the developers by wrapping the potential null reference with Optional.ofNullable(...).

[jansupol]

I agree with what you say, Optional should not be null.

I meant it differently, and I was not clear: Why do you think the query argument cannot be an empty String "", i.e. handled as null (So that the Optional is empty, but the return status would not be 404?

[joschi]

Why do you think the query argument cannot be an empty String "", i.e. handled as null (So that the Optional is empty, but the return status would not be 404?

@jansupol Thanks for your clarification! ❤️

You have to see this in context: The empty string is an invalid number and cannot be converted to a numeric type, so it should have the same result as the string "invalid-number".

If the query parameter was not provided at all, the value should be null (or in case of an optional it should be empty).
If the query parameter was provided and was empty, it should be the empty string which in turn is not a valid number and should return 404 (or whatever the default response for invalid types is).

Does that make sense? I'm just looking for consistency in the API and Jersey's implementation here. 😅

[jansupol]

The empty string is an invalid number

The thing is that the URI request and query parameters are always a text stream. so the empty string is at the same time an empty number, i.e. null. I cannot think of any simpler/better way to express the null number value.

The other point is that for the Jersey framework general functionality it seems better to match the endpoint with null number (empty optional) than return 404. When the endpoint is reached, it can programatically react on an empty query paramater by any response, 404 included if needed.