trimmingConvert breaks asRequred

[DALDEI]

Vaadin 8.6.0 Kotlin 1.3.10

simply:

bind(binder).asRequired().trimmingConverter().bind(SSMParameter::name) Generates NPE on blank text values if SSMParamater::name is not nullable

or bind(binder).trimmingConverter().asRequired().bind(SSMParameter::name)

Happily allows blank or empty text values

This is due to trimmingConverter() producing a null value in convertToModel()

Result.ok( value?.trim () )

It appears that value becomes null due to a prior ValidationResult.ok() and thus is passed through as null.

A hacky workaround that validates this (better tbd)

 bind(binder).trimmingConverter().asRequired {v, _ ->
            if( v.isNullOrBlank() )
              com.vaadin.data.ValidationResult.error("Must not be blank")
            else
              com.vaadin.data.ValidationResult.ok()

          }.bind(SSMParameter::value)

Possibly due to changes in Vaadin ? (using vaadin 8.6.0)

[mvysny]

You're right! Reproduced on Vaadin 8.6.2; the bug is reproducible even when the trimmingConverter() is removed from the chain, so it has to be a bug in asRequired() (since TextField always emits non-null values). I'll create a bug report to the Vaadin guys. EDIT: I just realized that the problem lies in the BinderUtilsKt.bind() since it calls withNullRepresentation() automatically for TextField.

[mvysny]

The problem is that we need withNullRepresentation() with nullable field types, since we have to convert null bean property value into an empty string (since TextField rejects nulls and would fail with an exception). However we must not call withNullRepresentation() for non-nullable fields (since that will take empty field value, convert it to null and pass it to asRequired() (which does not check for nulls and will pass null as a valid value). I'd say the problem is twofold:

• asRequired() validator returns OK for null. Filed in as a Vaadin bug: https://github.com/vaadin/framework/issues/11401
• Karibu-DSL's bind() must not call withNullRepresentation() on non-nullable fields. The problem is that bind() does not know at call time whether the target bean type is nullable or not.

[mvysny]

We could make withNullRepresentation() one-way (only convert nulls to empty values), but that could raise much confusion as of why the nullable fields are populated with empty strings. Therefore I don't believe that's a good way.

Instead we can move the withNullRepresentation() call to the bind(KProperty1) method, where it could be possible to detect the type nullability. Let me try this.

[mvysny]

The problem with type nullability is that it requires kotlin-reflect.jar on the classpath, which is a whopping 2,5 megabyte jar :( I'd pretty much wish to avoid depending on such a large jar file if possible... So if Vaadin folk will decide to fix asRequired() then I'd vote for keeping things as they are... What do you think?

[mvysny]

Also a workaround would be to move withNullRepresentation() from initial HasValue.bind() to the final BindingBuilder.bind(KProperty1) method (it would be always invoked, regardless of the nullability of the type, hence avoiding the need for kotlin-reflect.jar), but that's a bit fragile since then the withNullRepresentation() would not be called when you'd call bind(String) or bind(getter, setter) methods...

[mvysny]

Also filed https://github.com/vaadin/flow/issues/17277 .

[mvysny]

Workaround is to use a different asRequired() function, such as:

fun <T, V> BindingBuilder<T, V>.asRequiredNonNull(errorMessage: String): BindingBuilder<T, V> =
    asRequired { value, ctx -> if (value != null && value != field.emptyValue) ValidationResult.ok() else ValidationResult.error(errorMessage) }