ReflectiveBeanConverter does not work well for nullable fields and Value is missing toNullable

[agentgt]

The reflective bean converter generally works great however if there are fields that are not String that can be nullable (e.g. Instant, UUID, etc) it will fail to convert.

The reason this happens is because most form post submissions include all the fields as empty strings even if no value is inputted (obviously you can do some javascript hacks but that is not ideal).

Let says I have class like:

public record MyCommand(UUID someUUID) {}

(as a side note I'm not sure if the reflective bean converter works with records but I assume it does but you can translate it to a immutable bean mentally if not)

@POST("/command")
public ModelAndView command(MyCommand command) {
}

<form method="post"><input name="someUUID" value="" /> <input type="submit">submit</input>
</form>

When you submit it will essentially be sent as (lets assume url encoding for now): someUUID=

Thus someUUID is at first not null but "" (an empty string). Thus the Value converter will fail trying to convert the empty string to UUID. This is bad because often we need to validate other fields and can't just fail completely with a ProvisioningException

java.lang.IllegalArgumentException: Invalid UUID string:
        at java.base/java.util.UUID.fromString1(UUID.java:280)
        at java.base/java.util.UUID.fromString(UUID.java:258)
        at io.jooby.internal.converter.UUIDConverter.convert(UUIDConverter.java:19)
        at io.jooby.internal.ValueConverters.convert(ValueConverters.java:135)
        at io.jooby.DefaultContext.convert(DefaultContext.java:375)
        at io.jooby.internal.SingleValue.to(SingleValue.java:78)
        at io.jooby.internal.converter.ReflectiveBeanConverter.value(ReflectiveBeanConverter.java:187)
        at io.jooby.internal.converter.ReflectiveBeanConverter.inject(ReflectiveBeanConverter.java:117)
        at io.jooby.internal.converter.ReflectiveBeanConverter.newInstance(ReflectiveBeanConverter.java:72)
        at io.jooby.internal.converter.ReflectiveBeanConverter.convert(ReflectiveBeanConverter.java:52)
        at io.jooby.internal.ValueConverters.convert(ValueConverters.java:141)
        at io.jooby.DefaultContext.convert(DefaultContext.java:375)
        at io.jooby.internal.HashValue.to(HashValue.java:235)

This is because the ReflectiveBeanConverter does:

value.to(parameter.getType());

And that brings up a major point of issue I have with the Value API. There is no way to get a nullable out other than toOptional. The problem is Value.to by contract is NonNull (and I believe mostly by implementation).

Ideally io.jooby.Value should have a toNullable like:

  @Nullable <T> T toNullable(@Nonnull Class<T> type);

Then the ReflectiveBeanConverter would do:

value.toNullable(parameter.getType());

Which I think is the correct behavior for most people.

The work around is make your own bean converter and call ~~toOptional~~ value(), check if its an empty string and return null. You can't do this sadly through the ValueConverter API as null equals TypeMismatch (see below comments) and only in bean converters.

I rather the behavior be built in as I'm fairly sure it is the correct behavior.

[agentgt]

At an abstract level I'm basically saying Class types that come in as an empty String more often than not probably should be null.

Which actually brings up three conditions:

1. No Parameter was found. result: The value converter doesn't get called and thus is null.
2. A parameter was found but is a blank string. result: The value converter gets called but if it returns null it will be a TypeMismatchException even if null is an allowed value.
3. A parameter was found but the string is not empty and is not in the correct format. (e.g. corruption). This is a true TypeMismatchException.

Right now Jooby combines 2 and 3 as the same thing. It does simplify but I'm not sure it is correct behavior.

Really if the ValueConverter returns null it should be a MissingValueException.

From Value toOptional:

  @Nonnull default <T> Optional<T> toOptional(@Nonnull Class<T> type) {
    try {
      return Optional.ofNullable(to(type));
    } catch (MissingValueException x) {
      return Optional.empty();
    }
  }

DefaultContext

  @Override default @Nonnull <T> T convert(@Nonnull ValueNode value, @Nonnull Class<T> type) {
    T result = ValueConverters.convert(value, type, getRouter());
    if (result == null) {
      throw new TypeMismatchException(value.name(), type); // change this to MissingValueException
    }
    return result;
  }

[jknack]

Form/QueryString with empty string won't be set anymore.

With https://github.com/jooby-project/jooby/commit/ebc2671635864f79c65a7b009e2e0654a3fcc39a the reflective bean converter is bit more "clever". It doesn't create an instance when nothing is matched against the request.

This new reflective bean converter fix the #2525 issue.

Some examples:

Fails with uuid=

public record MyCommand(UUID uuid) {}

While this one: uuid=&name=some won't

public record MyCommand(UUID uuid, String name) {}

Reason is reflective bean converter found at least one matching variable and set it.

For supporting things like the first use case, I'm considering adding a new annotation:

@EmptyBean
public record MyCommand(UUID uuid) {}

With this annotation, form/query parsing will generate an orphan/useless/empty object.

Thoughts?

[agentgt]

Sorry @jknack I didn’t follow up on this. I have will try to test the new change this week before I go on vacation.