DatePicker allows setMax() to be bypassed and transfers invalid value server-side

mvysny commented 2 years ago

Description

setting DatePicker.setMax(LocalDate.now()) still allows the user to enter a future date via keyboard; DatePicker will then even transfer the date to the server-side and will not perform any server-side validation on the value.

Expected outcome

DatePicker should retain its older value, maybe firing invalidChangeListener on top of that.

Actual outcome

DatePicker fires value change listener with the invalid value.

Minimal reproducible example

@Route("")
public class MainView extends VerticalLayout {

    public MainView() {
        final DatePicker picker = new DatePicker("Enter future date");
        picker.setMax(LocalDate.now());
        picker.addValueChangeListener(e -> System.out.println(picker.getValue()));
        add(picker);
    }
}

Steps to reproduce

Clear steps describing how to reproduce the issue.

Paste the code above to Skeleton Starter
Type in a future date by hand via keyboard, for example 5/7/2023 (depending on the format of the locale your browser is using)
Observe the future date to be reported server-side.

Environment

Vaadin 14.8.7
Ubuntu 21.10

knoobie commented 2 years ago

Probably related to https://github.com/vaadin/flow/issues/7767

TatuLund commented 2 years ago

@knoobie You are right, the client side validation is disabled by default on purpose

https://github.com/vaadin/flow-components/blob/master/vaadin-date-picker-flow-parent/vaadin-date-picker-flow/src/main/java/com/vaadin/flow/component/datepicker/DatePicker.java#L335

vursen commented 2 years ago

Everything actually works as intended in this case. There are two different situations:

Input cannot be parsed.
Input can be parsed but doesn't satisfy some constraints, such as max for example.

Non-parsable input doesn't update the server-side value.

Parsable input does update the server-side value that is then validated against provided constraints and invalid is set accordingly:

https://user-images.githubusercontent.com/5039436/193764416-f3476f97-3a22-45fa-85ab-2b44558ff514.mov

(tested in Vaadin 14.8.7)

vursen commented 2 years ago

The current behavior is intended. The server-side value is synced on every user input change except when it cannot be cast to the server-side value type which is a limitation coming from using a strongly typed language. For example, an arbitrary foobar input string cannot be transformed into a LocalDate in the case of DatePicker but it is acceptable for EmailField whose value is of String type even though that string is not a valid email.

An example when syncing invalid values makes sense is when you would like to show an error popup having a mention of the invalid value like in the following example:

LocalDate today = LocalDate.now();

DatePicker paymentDatePicker = new DatePicker("Date of payment:");
paymentDatePicker.setMax(today);

Button submit = new Button("Submit", event -> {
    LocalDate paymentDate = paymentDatePicker.getValue();
    if (paymentDate.isAfter(today)) {
        Notification.show(paymentDate + " is not acceptable as the date of payment must be earlier than today");
        return;
    }

    ...
});

add(paymentDatePicker, submit);

Note, you can always check the value's validity status before doing anything with the value, e.g in the value change listener:

DatePicker datePicker = new DatePicker();
datePicker.addValueChangeListener(event -> {
    if (datePicker.isInvalid()) {
        ...
    }
});

We could consider changing the current behavior for all the field components in the future if the suggested behavior would be proven to cover the already existing use-cases.

vaadin / flow-components