Optional query parameter handling error when parameter value is empty string

[jason...@skiddoo.com.au]

Hi there

I met an issue when I use optional query parameter. For example, I have a service method like:

ServiceCall<NotUsed, Done> updateNationality(Optional<Country> country)

And the service is defined like:

Service.restCall(Method.PUT, "/user/nationality?country", this::updateNationality)

If I get a request like PUT /nationality, it works fine then nationality is clear. However, if the request is /nationality?country=, I get this error:

Caused by: java.lang.NullPointerException: null

at java.util.Objects.requireNonNull(Objects.java:203) ~[na:1.8.0_72]

at java.util.Optional.<init>(Optional.java:96) ~[na:1.8.0_72]

at java.util.Optional.of(Optional.java:108) ~[na:1.8.0_72]

at com.lightbend.lagom.javadsl.api.deser.PathParamSerializers$2.deserialize(PathParamSerializers.java:58) ~[lagom-javadsl-api_2.11-1.3.3.jar:1.3.3]

at com.lightbend.lagom.javadsl.api.deser.PathParamSerializers$2.deserialize(PathParamSerializers.java:47) ~[lagom-javadsl-api_2.11-1.3.3.jar:1.3.3]

at com.lightbend.lagom.internal.javadsl.api.ServiceReader$$anon$1$$anonfun$7.apply(ServiceReader.scala:240) ~[lagom-javadsl-api_2.11-1.3.3.jar:1.3.3]

at com.lightbend.lagom.internal.javadsl.api.ServiceReader$$anon$1$$anonfun$7.apply(ServiceReader.scala:239) ~[lagom-javadsl-api_2.11-1.3.3.jar:1.3.3]

I think the PathParamSerializer should interpret empty string as empty Optional.

[Mick Jermsurawong]

Hi Jason,

I assume null probably comes with the enum method of Country that couldn't find the matching name "of empty string"?

Can your path param use `Optional.ofNullable` instead? That should then give you empty Optional

Best,

Mick

[jason...@skiddoo.com.au]

Hi Mick

The null value comes from the method of returning Country from String, like :

withPathParamSerializer(Country.class, PathParamSerializers.required("Country", Country::fromCode, Country::getCode)

Regarding your suggestion, I guess I cannot add PathParamSerializer for Optional<Country> because it's not a Class.

Anyway, there could be workaround. But it would be nice if Lagom takes care of empty parameter value and handle Optional parameter nicely.

Regards

Jason

[Mick Jermsurawong]

Actually i'm wondering if the current implementation is helpful to discourage less idiomatic query.

Supposed we extend this to multiple query strings, having url that looks like /user/nationality?country=&city=&street= seems redundant and should be discouraged in my opinion

Also supposed a `MyCountry` enum that maps empty string "" into `UNKNOWN` enum, the proposed serialization will not be able to handle well. i.e it is not necessarily that empty string should default to empty option.

[James Roper]

I don't think there's a straight forward answer.

What if you want to differentiate between an empty string, and the parameter not being present?  Consider a search API, where you can search for users by first and/or last name.  If there's no lastName parameter, then that means you don't want to restrict the search by lastName.  But some people don't have a last name, and maybe you want to search for users with no last name, in which case, you will send a query of "search?lastName=", and you would expect that that would return only users that don't have a last name.  In which case, Optional.of("") must be passed to the API, rather than Optional.empty().

Another thing is if you don't support empty parameters, then if one comes in, that often might indicate a bug - a value was meant to be included but when the client built the URL, it accidentally forgot to include the value.  If that's the case, it's better to fail than to simply ignore the missing value.

So I think I definitely prefer the current behaviour.

Of course, you can implement your own path param serializer that does treat empty parameters as empty by doing this:

class OptionalCountrySerializer implements PathParamSerializer<Optional<Country>> {
    @Override
    public PSequence<String> serialize(Optional<Country> parameter) {
        return parameter.map(country -> TreePVector.singleton(country.getCode())).orElse(TreePVector.empty());
    }

    @Override
    public Optional<Country> deserialize(PSequence<String> parameters) {
        if (parameters.isEmpty()) {
            return Optional.empty();
        } else {
            return Optional.ofNullable(Country.fromCode(parameters.get(0)));
        }
    }
};

And then to use this in your descriptor (you need to use Guava's TypeToken to construct a generic type):

withPathParamSerializer(new com.google.common.reflect.TypeToken<Optional<Country>>() {}.getType(), new OptionalCountrySerializer())

Regards,

James

--
You received this message because you are subscribed to the Google Groups "Lagom Framework Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to lagom-framework+unsubscribe@googlegroups.com.
To post to this group, send email to lagom-framework@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/lagom-framework/33515eb0-055f-4f90-86d2-66fc18aa2c7a%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

James Roper
Software Engineer

Lightbend – Build reactive apps!
Twitter: @jroper

[Tim Moore]

It doesn't seem like this should throw a NullPointerException, though... that could be a bug.

To view this discussion on the web visit https://groups.google.com/d/msgid/lagom-framework/CABY0rKPc7choFBN3CnLHChE5Gkks_kWbYnu30y7cEF%3DisR1vYQ%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Tim Moore

Senior Engineer, Lagom, Lightbend, Inc.

tim....@lightbend.com
+61 420 981 589
Skype: timothy.m.moore

[jason...@skiddoo.com.au]

Thank you all for elaborating what's encouraged and discouraged.

However, as I am implementing an endpoint, I don't want to throw error or return 500 if the caller sends empty parameter value. This should be handled gracefully.

Similar problem happens if the parameter type is something like Integer, for example

ServiceCall<NotUsed, UserList> getUsers(Optional<Integer> page)

If the callers sends /users?page=, a NumberFormatException is thrown, which is not ideal. I think at least Lagom needs to catch those errors and return 400

[James Roper]

Issue created:

https://github.com/lagom/lagom/issues/713

--

You received this message because you are subscribed to the Google Groups "Lagom Framework Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to lagom-framework+unsubscribe@googlegroups.com.
To post to this group, send email to lagom-framework@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/lagom-framework/59b0b54e-4584-412d-b610-b74967ff70da%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[jason...@skiddoo.com.au]

Thank you, James

Cheers

Jason

On Wednesday, May 3, 2017 at 5:19:43 PM UTC+10, James Roper wrote:

Issue created:

https://github.com/lagom/lagom/issues/713

On 3 May 2017 at 13:17, <jason...@skiddoo.com.au> wrote:

Thank you all for elaborating what's encouraged and discouraged.

However, as I am implementing an endpoint, I don't want to throw error or return 500 if the caller sends empty parameter value. This should be handled gracefully.

Similar problem happens if the parameter type is something like Integer, for example

ServiceCall<NotUsed, UserList> getUsers(Optional<Integer> page)

If the callers sends /users?page=, a NumberFormatException is thrown, which is not ideal. I think at least Lagom needs to catch those errors and return 400

--
You received this message because you are subscribed to the Google Groups "Lagom Framework Users" group.

To unsubscribe from this group and stop receiving emails from it, send an email to lagom-framewo...@googlegroups.com.
To post to this group, send email to lagom-f...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/lagom-framework/59b0b54e-4584-412d-b610-b74967ff70da%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.