IntegerField has an incomplete API [1 day]

[schweinchendick]

Description

There is no way to communicate invalid input to the user or the Java backend.

Steps to Reproduce

Given the following code

        IntegerField test = new IntegerField();
        test.addValueChangeListener(event -> {
            Integer value = event.getValue();
            if (value == null)
                Notification.show("Why is it null?");
            else
                Notification.show(value.toString());
        });

1. Enter 1234 and leave the field (blur)
2. You see a notification of "1234"
3. Enter 123456789012 and leave the field (blur)
4. You see a notification of "Why is it null", while the visible UI value is still 123456789012

Steps not to reproduce

1. Enter 123456789012 and leave the field (blur)
2. You see no notification, while the visible UI value is still 123456789012
3. test.getValue() would return null in this case

Expected Results

There should be a way to communicate this error to the user or backend. How? The API is missing functionality for that.

Actual Results

The user is confused while staring at a number, which isn't the number the backend will work with from there. Neither is the field invalid nor is this state communicated to the user.

Browsers Affected

Any. It is not the fault of the browser.

Version

probably all versions.

Background

In the IntegerField.java there is a PARSER which swallows NumberFormatExceptions. It converts any invalid input silently to null. 123456789012 is not parse-able so this is somehow expected (from a programmer's perspective). But the programmer cannot react to that input in any way.

The same problem arises with any component implementing HasValue. The API is not written to handle invalid input. T getValue() does not allow to throw an exception. But there is always a possibility that converting the user's input into a semantic value does not work out.

This is heavily related to https://github.com/vaadin/vaadin-date-picker/issues/748.

But this is also true for any CustomField.generateModelValue(). The user can enter strange things that are not convertible to a valid value. There may be a workaround for custom field by implementing the "null object pattern" but you then have to return a different null object for every call. Otherwise the ValueChange is not triggered as the null-object still is equal to the former null-object.

Conclusion

The API must be extended to support exception handling. getValue() and generateModelValue() should allow to throw some ValueException which could then be handled by the framework. The exception could also arise directly from the client side and passed through to the Java backend.

Invalid user input shall not be discarded. It must be handled in a proper way. Either by the WebComponent, the Flow component or the programmer using the framework.
The current "solution" is like try {} catch (Exception e) { e.printStacktrace() }

[rolfsmeds]

Related: https://github.com/vaadin/vaadin-date-picker/issues/748 (considered blocked by vaadin/flow#8242)

[vursen]

I would break this down into two separate issue:

IntegerField, BigDecimalField suppress NumberFormatException when parsing the value.

It shouldn't technically be a problem to throw an exception in getValue() or more specifically in PARSER:

flow-components/vaadin-text-field-flow-parent/vaadin-text-field-flow/src/main/java/com/vaadin/flow/component/textfield/IntegerField.java

Lines 37 to 46 in b1a0a21

	private static final SerializableFunction<String, Integer> PARSER = valueFormClient -> {
	if (valueFormClient == null || valueFormClient.isEmpty()) {
	return null;
	}
	try {
	return Integer.parseInt(valueFormClient);
	} catch (NumberFormatException e) {
	return null;
	}
	};

It is worth considering that DatePicker, TimePicker, and some other Flow components already throw an exception when the filled date cannot be parsed:

flow-components/vaadin-time-picker-flow-parent/vaadin-time-picker-flow/src/main/java/com/vaadin/flow/component/timepicker/TimePicker.java

Lines 50 to 53 in b1a0a21

	private static final SerializableFunction<String, LocalTime> PARSER = valueFromClient -> {
	return valueFromClient == null || valueFromClient.isEmpty() ? null
	: LocalTime.parse(valueFromClient);
	};

From that perspective, IntegerField and BigDecimalField could just do the same: throw NumberFormatException when an invalid value is filled in the field. It would be pretty much in line with other Flow components. We need to keep in mind though that it will be a breaking change as exceptions may potentially be thrown somewhere the user wouldn't have expected them before.

Flow doesn't set min and max to MIN_INTEGER and MAX_INTEGER respectively for IntegerField by default.

There is another aspect of the issue described in the ticket. It is about integer limits. They are different in Java and Javascript:

• It is about 2^53 - 1 in Javascript
• It is usually about 2^32 in Java.

So there can be a situation where the user fills in an integer on the client-side that gets correctly processed by JavaScript because it is within the Javascript integer limit whereas it cannot be processed on the server-side because it is over the Java integer limit. Considering that, it could probably make sense to have Flow set the min and max attributes of the integer field to the Integer.MIN_INTEGER and Integer.MAX_INTEGER respectively to force the client-side to keep numbers within the Java integer limit as long as custom limits are not set.

[schweinchendick]

I see way more issues than two here.

The API is unfinished in a lot of places. Not only is it a problem in the described component, but also in the DatePicker. This is clearly shown in the related and linked (unresolved!) issue of DatePicker. But not only there. All CustomComponents have the same issue. That is what I described above. And I proved it in another issue, which I can't find right now.

The problem is not solved by throwing a RuntimeException. This has to be a checked exception. I mentioned that too, as I said ValueException except I didn't state that explicitly. On the other hand, the title expresses the missing API part. So we need an equivalent like public T getValue() throws ValueException whereas ValueException is not a RuntimeException.

Clearly this would be a breaking change. But unless it is implemented, it is a broken system! It is a very broken system, as there is not a single workaround. I - as a programmer/user of the API - can't do anything. Not a single HasValue is correct, except TextField because it just accepts anything.

[schweinchendick]

There it is: #1076 unresolved, and linked with similar problem.

And if you like, I can search for dozens of (still open) issues related to this problem, as it is always the same cause when the UI reacts weirdly due to swallowed exceptions.

[vursen]

The validation mechanism has been improved in v24 so that now field components properly handle non-parsable values as errors, which is reflected to their invalid flag. See vaadin/platform#3066 (comment) for more details.

@schweinchendick Can you please check if the issue still persists?

[vursen]

I'm closing the issue, as there has been no activity over a month. Feel free to reopen if the issue still persists.

[schweinchendick]

Yes, the issue still persists (Vaadin Flow 24.1.3), because the API is still incomplete. The current (new) implementation is just a bad workaround with obvious flaws.

You still get null as a value if the parsing fails, instead of an Exception. But at least you (developer or enduser) now see that the field is invalid. So one has to check that situation by hand:

    IntegerField test = new IntegerField();
    test.addValueChangeListener(event -> {
        Integer value = event.getValue();
        if (value == null)
            if (event.getSource().isInvalid())
                Notification.show(
                        "You entererd something bad. But I can't tell you what, because I have no access to the (string)value");
            else
                Notification.show("The good old empty string");
        else
            Notification.show(value.toString());
    });

The problem with the current workaround arises at the ValueChangeEvent. First try to enter "123456789012", observe the invalid field. Then give up and enter the empty string "", now wait for the ValueChangeEvent. You will never see any Notification as the value did not change from null (invalid) to null (empty representation). I can repeat that in many different variations, fields and CustomField.

The API is broken. Still. Now with an incomplete workaround. And this dates back at least since Vaadin 6.

[vursen]

Thank you for the feedback. It's a fair point that, currently, the components lack a mechanism that would allow distinguishing completely nonsensical input from input that is accepted on the client-side but still gets rejected by the server due to technical limitations, e.g. exceeding the Java Integer limit.

It's by design that the value falls back to null when it cannot be represented on the server, and that consequently there may be no value change event.

However, we could improve the validation mechanism to allow providing individual error messages in cases where the value was set to null as a result of a parse exception. I created an enhancement ticket for that. Note, it will require adding the error message support first.