StringToIntegerConverter makes TextField throw NullPointerException on null Integer value

[mvysny]

Vaadin 14.0.9. Binding a null Integer field to a TextField via StringToIntegerConverter fails with NPE. This is the most basic use-case which simply must work out of the box.

[mvysny]

Simple code:

public class IntHolder {
  private Integer myvalue;
  // + getter/setter
}
binder = new Binder(IntHolder.class);
binder.forField(new TextField()).withConverter(new StringToIntegerConverter("Not an integer")).bind("myvalue");
binder.readBean(new IntHolder());

fails with

java.lang.NullPointerException: Null value is not supported

	at java.util.Objects.requireNonNull(Objects.java:228)
	at com.vaadin.flow.component.AbstractSinglePropertyField.lambda$static$460eb7c0$1(AbstractSinglePropertyField.java:56)
	at com.vaadin.flow.component.AbstractSinglePropertyField$TypeHandler.lambda$createWriter$db98900a$1(AbstractSinglePropertyField.java:108)
	at com.vaadin.flow.component.AbstractSinglePropertyField.setPresentationValue(AbstractSinglePropertyField.java:359)
	at com.vaadin.flow.component.internal.AbstractFieldSupport.applyValue(AbstractFieldSupport.java:220)
	at com.vaadin.flow.component.internal.AbstractFieldSupport.setValue(AbstractFieldSupport.java:188)
	at com.vaadin.flow.component.internal.AbstractFieldSupport.setValue(AbstractFieldSupport.java:133)
	at com.vaadin.flow.component.AbstractField.setValue(AbstractField.java:181)
	at com.vaadin.flow.component.textfield.TextField.setValue(TextField.java:439)
	at com.vaadin.flow.component.textfield.TextField.setValue(TextField.java:32)
	at com.vaadin.flow.data.binder.Binder$BindingImpl.initFieldValue(Binder.java:1131)
	at com.vaadin.flow.data.binder.Binder$BindingImpl.access$200(Binder.java:972)
	at com.vaadin.flow.data.binder.Binder.lambda$readBean$2(Binder.java:1702)
	at java.util.ArrayList.forEach(ArrayList.java:1257)
	at com.vaadin.flow.data.binder.Binder.readBean(Binder.java:1695)

[mvysny]

Workaround: patch the Converter to return an empty string instead of null:

public class StringToIntegerConverter2 extends StringToIntegerConverter {
    public StringToIntegerConverter2() {
        super("Not an integer");
    }

    @Override
    public String convertToPresentation(Integer value, ValueContext context) {
        final String result = super.convertToPresentation(value, context);
        return result == null ? "" : result;
    }
}

[Legioth]

You're supposed to define an explicit withNullRepresentation("") or similar if there can be null values from the business logic. The framework cannot know how you want nulls to be handled (or whether they are expected at all), so it's the responsibility of the application developer to explicitly define those.

[mvysny]

The framework cannot know how you want nulls to be handled (or whether they are expected at all), so it's the responsibility of the application developer to explicitly define those.

@Legioth not true, broken already by Binder since it uses withNullRepresentation("") implicitly when there are no other converters and TextField is used. Which is good dev experience, otherwise I would have to call withNullRepresentation("") for nearly every binding.

Making the simplest conversion code fail with NPE is bad dev experience. Forcing the dev to always write withNullRepresentation("") when StringToIntegerConverter is used, is bad dev experience. This needs to be fixed.

If you hate accepting nulls for TextFields and you want to preserve Converters to pass nulls, then at least consider failing with a different exception, e.g. Passing null to TextField binding is not allowed. Please consider using withNullRepresentation("").

[Legioth]

The problem is that it's also a potentially bad experience to do "magic" things on the developer's behalf. Just ask anyone who has ever used any dependency injection framework :)

Good point about the automatic null representation in cases when there isn't any converter at all. The idea there is that defining a converter puts the binding in "manual" mode without any magic at all whereas simple bindings without any explicit conversion logic can be slightly magic.

You are right that we could at the very least somehow improve the error message for this particular case. I'm not 100% sure how to do that since TextField.setValue() cannot assume that its value has been set through Binder. Maybe we should make Binder catch any exception from setValue and rethrow with a more specific message in the case of passing null to a field with a non-null getEmptyValue()?

We could also consider whether there would be some alternative way of defining explicit null conversion with less boilerplate, e.g. as an optional parameter to the converter constructor. This would on the other hand have a negative impact on code readability.

[Legioth]

For reference, there are also related discussions for the identical functionality in Vaadin 8: vaadin/framework#8664 & vaadin/framework#11109

[mvysny]

@Legioth very good point with magic+DI, I avoid DI like a plague.

Maybe we should make Binder catch any exception from setValue and rethrow with a more specific message in the case of passing null to a field with a non-null getEmptyValue()?

That sounds really good. Rethrowing as RuntimeException changes exception type which should be avoided. Rethrowing only in special case as described above sounds good to me, and navigates the dev to the correct solution.

We could also consider whether there would be some alternative way of defining explicit null conversion with less boilerplate, e.g. as an optional parameter to the converter constructor. This would on the other hand have a negative impact on code readability.

Yeah, StringToIntegerConverter already has 4 constructors and with optional boolean param there would be too many. We could add converter.withNullValue("") but there's no point since we can already call binding.withNullRepresentation(""). I think rethrow is the best solution.

[Legioth]

#6757 adds a mention of null representations to the exception message.