SingleValueExtractor makes it impossible to support java.util.Optional (or Guava Optional)

[glassfishrobot]

SingleValueExtractor makes it impossible to support java.util.Optional (or Guava Optional) through custom ParamConverter as it checks for null before forwarding the value to the ParamConverter (thus not delegating the null value handling to the custom ParamConverter).

Hence it is impossible to write a resource method like

**HelloResource.java**@GET
@Produces("text/plain")
public String hello(@QueryParam("name") final Optional<String> name) {
     return "Hello " + name.orElse("World") + "!";
}

While given a custom ParamConverterProvider like this one for example:

**OptionalParamConverterProvider.java**@Singleton
public class OptionalParamConverterProvider implements ParamConverterProvider {

    private final ServiceLocator locator;

    @Inject
    public OptionalParamConverterProvider(final ServiceLocator locator) {
        this.locator = locator;
    }

    @Override
    public <T> ParamConverter<T> getConverter(final Class<T> rawType, final Type genericType, final Annotation[] annotations) {

        final List<ClassTypePair> ctps = ReflectionHelper.getTypeArgumentAndClass(genericType);
        ClassTypePair ctp = (ctps.size() == 1) ? ctps.get(0) : null;
        if (ctp == null || ctp.rawClass() == String.class) {
            return new ParamConverter<T>() {
@Override
public T fromString(final String value) {
    return rawType.cast(Optional.ofNullable(value));
}

@Override
public String toString(final T value) throws IllegalArgumentException {
    return value.toString();
}
            };
        }

        final Set<ParamConverterProvider> converterProviders = Providers.getProviders(locator, ParamConverterProvider.class);
        for (ParamConverterProvider provider : converterProviders) {
            @SuppressWarnings("unchecked")
            ParamConverter<?> converter = provider.getConverter(ctp.rawClass(), ctp.type(), annotations);
            if (converter != null) {
return new ParamConverter<T>() {
    @Override
    public T fromString(final String value) {
        return rawType.cast(Optional.ofNullable(converter.fromString(value)));
    }

    @Override
    public String toString(final T value) throws IllegalArgumentException {
        return value.toString();
    }
};
            }
        }

        return null;
    }
}

Environment

java version "1.7.0_51"
Java(TM) SE Runtime Environment (build 1.7.0_51-b13)
Java HotSpot(TM) 64-Bit Server VM (build 24.51-b03, mixed mode)
OSX 10.9.4

Affected Versions

[2.11]

[glassfishrobot]

Reported by nicolasyanncouturier

[glassfishrobot]

nicolasyanncouturier said:
A pull request for this issue is available here: https://github.com/jersey/jersey/pull/98

[glassfishrobot]

@mpotociar said:
The suggested change in behavior cannot be supported, because according to JAX-RS API specification, a ParamConverter implementation is expected to throw an IllegalStateException when the input parameter is null. See here.

What you want to do with Optional can be easily supported by using a JAX-RS @DefaultValue annotation. Or is there a real-life use case that cannot be covered by the existing programming model?

[glassfishrobot]

@mpotociar said:
On a second thought, perhaps we could support this deviation from the expected standards, provided we do not break the implementations that adhere to the standard behavior and we can still pass JAX-RS TCK tests.

But I would still like to see a sound use case that cannot be solved using @DefaultValue before we even consider going into all this trouble.

[glassfishrobot]

nicolasyanncouturier said:
I understand the comment about the ParamConverter supposed to throw IllegalArgumentException according to the spec. And that's also why, in my proposed PR, I implemented this behavior in all of the Jersey built-in ParamConverter – as it is not the case in the current implementation – inside the server main sources (intentionally not in the test ParamConverter for Optional).

As I understand the spec, it shows that it is the ParamConverter-s' reponsibility to check for null values. However, with the current implementation of SingleValueExtractor all ParamConverter-s are not even given the chance to handle the null values that they would be passed according to the spec. The change I propose in SingleValueExtractor complies with the spec since all Jersey built-in ParamConverter now check for null values and throw IllegalArgumentException.

The interesting part is that, while still being compliant with the spec, this change also leaves open the possibility to handle null values differently than the spec imposes in one's own custom ParamConverter-s (f.ex. to provide custom support for Optional) (an that is then of course one's own responsibility, not Jersey's anymore)

I know about the @DefaultValue mechanism; however, my real-life use-cases are that no default value would make sense semantically speaking as regard to my domain and I would thus instead resort to null and loads of error-prone null-checking, to which the Optional monad brings an elegant solution. Hence my need for Optional support through a custom non-spec-compliant ParamConverter. However, note that my non-spec-compliant ParamConverter is not to be included into Jersey's server main sources as it is not spec-compliant (I could remove this "borderline" test-case code from the PR).

Generally: if one expects an optional Integer value coming as a @QueryParam, often no default value could play the role of "value is absent", and one would thus resort to a null-check. Now, if one has 7 of these parameters that is a big number of null-checks (or "default value that means 'absent'"-checks).

Specifically: my service is supposed to return all historical price changes for a list of real estate properties given its cadastre numbers: county nr. (required), municipality nr.(required), cadastral-unit nr. (required), property-unit nr. (required), section nr. (optional), lease nr. (optional).

A simplified non optional-based code example:

**Resource.java**@GET
@Produces("text/plain")
public List<Price> getPriceHistory(@QueryParam("cnr") final Integer countyNr, 
   @QueryParam("mnr") final Integer municipalityNr, 
   @QueryParam("cunr") final Integer cadastralUnitNr,
   @QueryParam("punr") final Integer propertyUnitNr, 
   @QueryParam("snr") final Integer sectionNr, 
   @QueryParam("lnr") final Integer leaseNr) {
     if(countyNr != null && municipalityNr != null && cadastralUnitNr != null && propertyUnitNr != null) {
         return service.getPriceHistory(countyNr, municipalityNr, cadastralUnitNr, propertyUnitNr, sectionNr, leaseNr);
     }
     return Collections.emptyList();
}

**Service.java**public List<Price> getPriceHistory(final Integer countyNr, 
   final Integer municipalityNr, 
   final Integer cadastralUnitNr,
   final Integer propertyUnitNr, 
   final Integer sectionNr, 
   final Integer leaseNr) {
    // ...
    // get the prices from a database
    // ...
    // naïve query building example for brievity
    if(sectionNr != null) {
    	query += " AND section_nr = " + sectionNumber;
    }
    if(leaseNr != null) {
    	query += " AND lease_nr = " + leaseNr;
    }
}

To compare with a simplified optional-based code example:

**Resource.java**@GET
@Produces("text/plain")
public List<Price> getPriceHistory(@QueryParam("cnr") final Optional<Integer> countyNr, 
   @QueryParam("mnr") final Optional<Integer> municipalityNr, 
   @QueryParam("cunr") final Optional<Integer> cadastralUnitNr,
   @QueryParam("punr") final Optional<Integer> propertyUnitNr, 
   @QueryParam("snr") final Optional<Integer> sectionNr, 
   @QueryParam("lnr") final Optional<Integer> leaseNr) {
    return Optionals.of(countyNr, municipalityNr, cadastralUnitNr, propertyUnitNr)
    .map(cnr, mnr, cunr, punr -> service.getPriceHistory(cnr, mnr, cunr, punr, sectionNr, leaseNr))
    .orElse(Collections.emptyList());
}

**Service.java**public List<Price> getPriceHistory(final Integer countyNr, 
   final Integer municipalityNr, 
   final Integer cadastralUnitNr,
   final Integer propertyUnitNr, 
   final Optional<Integer> sectionNr, 
   final Optional<Integer> leaseNr) {
    // ...
    // get the prices from a database
    // ...
    // naïve query building example for brievity
    query += sectionNumber.map(snr -> " AND section_nr = " + snr).orElse("")
           + leaseNumber.map(lnr -> " AND lease_nr = " + lnr).orElse("");
}

And of course, an intermediate - but less elegant - solution could be:

**Resource.java**@GET
@Produces("text/plain")
public List<Price> getPriceHistory(@QueryParam("cnr") final Integer countyNr, 
   @QueryParam("mnr") final Integer municipalityNr, 
   @QueryParam("cunr") final Integer cadastralUnitNr,
   @QueryParam("punr") final Integer propertyUnitNr, 
   @QueryParam("snr") final Integer sectionNr, 
   @QueryParam("lnr") final Integer leaseNr) {
    return Optionals.of(Optional.ofNullable(countyNr), 
        Optional.ofNullable(municipalityNr), 
        Optional.ofNullable(cadastralUnitNr), 
        Optional.ofNullable(propertyUnitNr))
    .map(cnr, mnr, cunr, punr -> service.getPriceHistory(cnr, 
         mnr, 
         cunr, 
         punr, 
         Optional.ofNullable(sectionNr), 
         Optional.ofNullable(leaseNr)))
    .orElse(Collections.emptyList());
}

[glassfishrobot]

@mpotociar said:
Pull request has been merged.

[glassfishrobot]

Marked as fixed on Friday, October 24th 2014, 3:46:11 am

[glassfishrobot]

This issue was imported from java.net JIRA JERSEY-2612