Add a ParamConverterProvider for java.util.Optional parameters

[Gaff]

Would it be possible to include a ParamCovnerterProvider that supports Optional<X> types?

I notice DropWizard have some: https://github.com/dropwizard/dropwizard/blob/master/dropwizard-jersey/src/main/java/io/dropwizard/jersey/optional/OptionalParamBinder.java - but presumably this would be a generally useful feature!

[Gaff]

@jansupol wow thanks for doing this! Much appreciated.

[joschi]

@jansupol @jbescos Thanks for implementing this upstream!

Too bad you didn't implement providers for the other optional classes such as OptionalDouble, OptionalInt, and OptionalLong too, but we've got you covered: io.dropwizard.jersey.optional. 😉

In the upstream implementation, I think there was an unfortunate decision to return null if the value couldn't be converted into the type wrapped by the Optional<T> container:

jersey/core-common/src/main/java/org/glassfish/jersey/internal/inject/ParamConverters.java

Lines 282 to 286 in 52c8aa4

	/*
	* In this case we don't send Optional.empty() because 'value' is not null.
	* But we return null because the provider didn't find how to parse it.
	*/
	return null;

This means that users will have to check all Optional<T> parameters for null before interacting with them, otherwise they'll get a NullPointerException and clients would receive a response with status code 500 (Internal Server Error).

In my humble opinion this is counter-intuitive to the idiomatic usages of the Optional<T> type.

Unfortunately the test cases don't cover the erroneous cases:

jersey/tests/e2e-server/src/test/java/org/glassfish/jersey/tests/e2e/server/OptionalParamConverterTest.java

Lines 79 to 87 in 52c8aa4

	@Test
	public void fromOptionalInt() {
	Response empty = target("/OptionalResource/fromInteger").request().get();
	Response notEmpty = target("/OptionalResource/fromInteger").queryParam(PARAM_NAME, 1).request().get();
	assertEquals(200, empty.getStatus());
	assertEquals(Integer.valueOf(0), empty.readEntity(Integer.class));
	assertEquals(200, notEmpty.getStatus());
	assertEquals(Integer.valueOf(1), notEmpty.readEntity(Integer.class));
	}

In order to properly cover the error cases, the resource method would have to be changed as follows:

Original:

jersey/tests/e2e-server/src/test/java/org/glassfish/jersey/tests/e2e/server/OptionalParamConverterTest.java

Lines 47 to 51 in 52c8aa4

	@GET
	@Path("/fromInteger")
	public Response fromInteger(@QueryParam(PARAM_NAME) Optional<Integer> data) {
	return Response.ok(data.orElse(0)).build();
	}

Changed version with correct error handling:

@GET
@Path("/fromInteger")
public Response fromInteger(@QueryParam(PARAM_NAME) Optional<Integer> data) {
    if (data == null) {
        // parameter is present but couldn't be converted to an integer
        return Response.status(400, "Invalid integer parameter");
    }
    return Response.ok(data.orElse(0)).build();
}

The version with the correct error handling doesn't provide any advantage over using a plain Integer parameter with @DefaultValue which would look as follows:

@GET
@Path("/fromInteger")
public Response fromInteger(@QueryParam(PARAM_NAME) @DefaultValue("0") Integer data) {
    if (data == null) {
        // parameter is present but couldn't be converted to an integer
        return Response.status(400, "Invalid integer parameter");
    }
    return Response.ok(data).build();
}

[jbescos]

Hi @joshchi, thanks for your feedback, but I have few questions.

@get

@Path("/fromInteger")
public Response fromInteger(@QueryParam(PARAM_NAME) @DefaultValue("0") Integer data) {
    if (data == null) {
        // parameter is present but couldn't be converted to an integer
        return Response.status(400, "Invalid integer parameter");
    }
    return Response.ok(data).build();
}

With that you cannot change the default value with runtime data. For example, lets suppose the input is a Date, you can not use by default new Date(). Or maybe the value is coming from a property, database, etc.

For the other observation:
In the upstream implementation, I think there was an unfortunate decision to return null if the value couldn't be converted into the type wrapped by the Optional<T> container

For clarification, there are 3 scenarios:

• The value is null. In this case Optional.empty() is passed to the resource.
• The value is not null and parameter can be converted. In this case Optional.of(val) is returned.
• The value is not null but Jersey does not know how to convert it. From @joschi comment it looks that null is passed to the resource causing NullPointerException. This is the point we are discussing here, and you are suggesting to return Optional.empty() if I understood you well.

Jersey iterates through all ParamConverters till it finds one that is not null. Basically if the ParamConverter does not know how to convert it, it returns null and it lets the next candidate to handle it. From this point of view I think it is doing it well.

I agree user should never check optional == null, but for the case you mention, I think Jersey should fail before reaching the resource with a 400 error, instead of passing an Optional.empty() as argument.

[joschi]

@jbescos Thanks for your response!

I agree user should never check optional == null, but for the case you mention, I think Jersey should fail before reaching the resource with a 400 error, instead of passing an Optional.empty() as argument.

You're right, Jersey would let the request fail and not return null for the Optional<T> parameter but return a response with HTTP status 404 (Not Found) if the parameter converter(s) returned null.

This being said, the behavior seems a bit inconsistent across providers, e. g. Integer and Boolean:

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 paramEmpty = target("/int").request().get(String.class);
        assertEquals("default", paramEmpty);

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

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

    @Test
    public void testOptionalBoolean() {
        String paramEmpty = target("/bool").request().get(String.class);
        assertEquals("default", paramEmpty);

        String paramProvided = target("/bool").queryParam("b", "true").request().get(String.class);
        assertEquals("b was true", paramProvided);

        String paramInvalid = target("/bool").queryParam("b", "not-a-number").request().get(String.class);
        assertEquals("b was false", paramInvalid);
    }

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

        @GET
        @Path("/bool")
        public String optionalBool(@QueryParam("b") Optional<Boolean> b) {
            if (b == null) {
                return "null";
            }
            return b.map(param -> "b was " + param).orElse("default");
        }
    }
}

[jbescos]

That is probably because Integer.parseInteger fails with exception when the input is not a number.

But for the case of Boolean.parseBoolean, it returns false in case the input is not "true".

I guess if you remove the Optional from the test resources it should behave in the same way, passing the boolean as false when the input is "not-a-number".

[joschi]

@jbescos Sorry to bring this up again but it looks like an empty input string ("") is being handled differently than an invalid input string ("not-a-number") or a missing input string for these cases.

Same example as before but this time with a check for the behavior when passing an empty string for the Optional<Integer> parameter:

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("/int").request().get(String.class);
        assertEquals("default", paramMissing);

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

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

        Response paramEmpty = target("/int").queryParam("i", "").request().get();
        assertEquals(404, paramEmpty.getStatus()); // ⚠️ This is failing
    }

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

This seems to be true for all numeric types (Double, Long, BigInteger, etc.).

[jbescos]

@joschi, from the implementation point of view it is consistent with the next behavior when there is no Optional:

        @GET
        @Path("/int2")
        public String int2(@QueryParam("i") Integer i) {
            if (i == null) {
                return "null";
            }
            return i + "";
        }
        
    @Test
    public void testInteger() {
        Response paramEmpty2 = target("/int2").queryParam("i", "").request().get();
        assertEquals(404, paramEmpty2.getStatus()); // ⚠️ This is failing
    }

It will return 200 and the Integer i will be null.

It seems Jersey is validating that the Integer is not empty before parseInt it. Although it looks inconsistent with the other test you pointed out before:

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

Why Jersey validates the empty string but it is not validating when the string is not a number?. This is other issue that would require some discussion and analysis. Maybe it is written in the specification that it must behave in that way.

Feel free to open a new issue to address this and we will check it.

[joschi]

@joschi, from the implementation point of view it is consistent with the next behavior when there is no Optional:

Which leaves us again with (#4651 (comment)):

This means that users will have to check all Optional<T> parameters for null before interacting with them, otherwise they'll get a NullPointerException and clients would receive a response with status code 500 (Internal Server Error).

In my humble opinion this is counter-intuitive to the idiomatic usages of the Optional<T> type.

This being said, I think I'll follow up with your suggestion to open a new issue. 👍

Why Jersey validates the empty string but it is not validating when the string is not a number?. This is other issue that would require some discussion and analysis. Maybe it is written in the specification that it must behave in that way.

Feel free to open a new issue to address this and we will check it.