Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server Side Validation is removed on blur #4804

Closed
knoobie opened this issue Mar 14, 2023 · 17 comments
Closed

Server Side Validation is removed on blur #4804

knoobie opened this issue Mar 14, 2023 · 17 comments
Assignees
Labels
needs discussion No decision yet, discussion needed validation

Comments

@knoobie
Copy link
Contributor

knoobie commented Mar 14, 2023

Description

Using the server side API to add validation errors on fields is overwritten once the user blurs the field.

Expected outcome

All fields are still shown as invalid and error message is present even when user entered and left a field.
grafik

Minimal reproducible example

public class BugExample extends Div {

  public BugExample() {

    var field1 = new ComboBox<>("Type", "A", "B", "C", "D");
    var field2 = new ComboBox<>("Collection", "A", "B", "C", "D");
    var field3 = new ComboBox<>("Version", "A", "B", "C", "D");

    Binder<MyBean> binder = new Binder<>();
    binder.forField(field1).asRequired("Required.").bind(MyBean::getField1, MyBean::setField1);
    binder.forField(field2).asRequired("Required.").bind(MyBean::getField2, MyBean::setField2);
    binder.forField(field3).asRequired("Required.").bind(MyBean::getField3, MyBean::setField3);

    binder.setBean(new MyBean("A", null, null));

    new CombinationCheck().register(binder, field1, field2, field3);


    var button = new Button("Validate after filling", e -> {
      if(binder.validate().isOk()) {
        System.out.println("All Good");
      }else {
        System.out.println("Binder not happy");
      }
    });

    add(field1, field2, field3, button);
  }

  @Data
  @AllArgsConstructor
  public static class MyBean {
    private String field1;
    private String field2;
    private String field3;
  }

  public static class CombinationCheck implements Serializable {

    private Binder<?> binder;
    private ComboBox<String> field1;
    private ComboBox<String> field2;
    private ComboBox<String> field3;

    public void register(Binder<?> binder,
                         ComboBox<String> field1,
                         ComboBox<String> field2,
                         ComboBox<String> field3) {
      this.binder = binder;
      this.field1 = field1;
      this.field2 = field2;
      this.field3 = field3;

      // listeners to show combination problems
      field1.addValueChangeListener(e -> {
        if (null == e.getValue()) {
          return;
        }

        if (hasField2Value() && hasField3Value()) {
          check();
        }
      });
      field2.addValueChangeListener(e -> {
        if (null == e.getValue()) {
          return;
        }

        if (hasField1Value() && hasField3Value()) {
          check();
        }
      });
      field3.addValueChangeListener(e -> {
        if (null == e.getValue()) {
          return;
        }

        if (hasField2Value() && hasField3Value()) {
          check();
        }
      });
    }

    private void check() {
      boolean isValid = "A".equals(field1.getValue()) && "A".equals(field2.getValue()) && "A".equals(field3.getValue());

      if (isValid) {
        removeFeedback(field3);
        removeFeedback(field2);
        removeFeedback(field1);
      } else {
        showFeedback(field3);
        showFeedback(field2);
        showFeedback(field1);
      }
    }

    private void removeFeedback(ComboBox<?> component) {
      if (component.hasThemeName(ErrorLevel.INFO.name().toLowerCase(Locale.ROOT))) {
        // remove 'info' level field errors
        binder.getValidationErrorHandler().clearError(component);
      }
    }

    private void showFeedback(ComboBox<?> component) {
      if (component.hasThemeName(ErrorLevel.ERROR.name().toLowerCase(Locale.ROOT))) {
        // don't overwrite hard errors that are not of type 'INFO'
        return;
      }

      binder.getValidationErrorHandler().handleError(component,
        ValidationResult.create("Notice: Combination of Fields is not recommended.", ErrorLevel.INFO));
    }

    private boolean hasField1Value() {
      return this.field1.getValue() != null;
    }

    private boolean hasField2Value() {
      return this.field2.getValue() != null;
    }

    private boolean hasField3Value() {
      return this.field3.getValue() != null;
    }
  }
}

Steps to reproduce

  1. Add this class to a view

  2. Select 'A', 'A', 'A' --> everything is good (matching the check / recommendation)
    grafik

  3. Select 'B' somewhere --> all fields should show an error now (working!)
    grafik

  4. Blur the field where you changed the value to 'B' --> field is instantly marked as correct, even tho the server side wasn't asked / notified again (where the check would say: nope; still bad)
    grafik

Environment

Vaadin version(s): 24.0.0

Browsers

Firefox

@vursen
Copy link
Contributor

vursen commented Mar 14, 2023

Here is another example by @cdir.

The invalid state gets overridden because of an extra validation round which is triggered by the client-side validated event coming right after value-changed:

TextField field1 = new TextField();
TextField field2 = new TextField();
field1.addValueChangeListener(e -> {
     field1.setInvalid(true);
     field1.setErrorMessage("error");
     field2.setInvalid(true);
     field2.setErrorMessage("error");
});
add(field1);
add(field2);

#4390 (comment):

When you enter something in field1, a validation message should appear. But the client,side validation overwrites the invalid flag immediately.

In my real case, there is some business logic that sets an error message and the invalid flag when the input value is not valid.

With Vaadin 24, the feature flag is still present but ignored and the client-side validation is always enabled. Therefore, all server-side validation is overridden.

@rolfsmeds rolfsmeds added the needs discussion No decision yet, discussion needed label Mar 15, 2023
@simasch
Copy link

simasch commented Mar 17, 2023

We have the same issue in a CustomField that consists of a Select and a TextField.
The validation message gets lost.

@vursen
Copy link
Contributor

vursen commented Mar 23, 2023

In general, we recommend using Binder for cross-field validation. It seems that @knoobie's example can be rewritten with Binder so that it won't require direct invalid state manipulations:

@Route("example-bug")
public class ExampleBug extends Div {

    public ExampleBug() {
        var field1 = new ComboBox<>("Type", "A", "B", "C", "D");
        var field2 = new ComboBox<>("Collection", "A", "B", "C", "D");
        var field3 = new ComboBox<>("Version", "A", "B", "C", "D");

        Binder<MyBean> binder = new Binder<>();

        Validator<String> validator = (value, context) -> {
            if ("A".equals(field1.getValue()) &&
                    "A".equals(field2.getValue()) &&
                    "A".equals(field3.getValue())) {
                return ValidationResult.create("Notice: Combination of Fields is not recommended.", ErrorLevel.INFO);
            }

            return ValidationResult.ok();
        };

        var field1Binding = binder
                .forField(field1)
                .asRequired("Required")
                .withValidator(validator)
                .bind(MyBean::getField1, MyBean::setField1);

        var field2Binding = binder
                .forField(field2)
                .asRequired("Required")
                .withValidator(validator)
                .bind(MyBean::getField2, MyBean::setField2);

        var field3Binding = binder
                .forField(field3)
                .asRequired("Required")
                .withValidator(validator)
                .bind(MyBean::getField3, MyBean::setField3);

        field1.addValueChangeListener(event -> {
            if (field2.getValue() != null) {
                field2Binding.validate();
            }
            if (field3.getValue() != null) {
                field3Binding.validate();
            }
        });

        field2.addValueChangeListener(event -> {
            if (field1.getValue() != null) {
                field1Binding.validate();
            }
            if (field3.getValue() != null) {
                field3Binding.validate();
            }
        });

        field3.addValueChangeListener(event -> {
            if (field1.getValue() != null) {
                field1Binding.validate();
            }
            if (field2.getValue() != null) {
                field2Binding.validate();
            }
        });

        binder.setBean(new MyBean("A", null, null));

        var button = new Button("Validate after filling", e -> {
            if (binder.validate().isOk()) {
                System.out.println("All Good");
            } else {
                System.out.println("Binder not happy");
            }
        });

        add(field1, field2, field3, button);
    }

    public static class MyBean {
        private String field1;
        private String field2;
        private String field3;

        public MyBean(String field1, String field2, String field3) {
            this.field1 = field1;
            this.field2 = field2;
            this.field3 = field3;
        }

        public void setField1(String field1) {
            this.field1 = field1;
        }

        public String getField1() {
            return field1;
        }

        public void setField2(String field2) {
            this.field2 = field2;
        }

        public String getField2() {
            return field2;
        }

        public void setField3(String field3) {
            this.field3 = field3;
        }

        public String getField3() {
            return field3;
        }
    }
}

@simasch @cdir @knoobie Do you have any use cases in mind that cannot be achieved with Binder?

@cdir
Copy link

cdir commented Mar 23, 2023

@vursen we cannot use Binder because we use our own binding framework. How does the binder set the validation, maybe we can use the same principle as a workaround?

@knoobie
Copy link
Contributor Author

knoobie commented Mar 23, 2023

I can probably rewrite my code to rely "correctly" on Binder, but this feels like a big step backwards, huge breaking change and increases the already quite complex forms and binder combinations I'm using significant. For me the server-side is king. If the server decides that the field is invalid, it should stay like this until the server and all registered listener were asked again and say otherwise - this model is the main reason I'm using and relying on Vaadin Flow.

Technically speaking, I'm not even manipulating the fields' state directly, but using the Binder already but probably not as intended ;)

      binder.getValidationErrorHandler().handleError(component,
        ValidationResult.create("Notice: Combination of Fields is not recommended.", ErrorLevel.INFO));

So you are telling us, that the usage of special / public Binder features (like shown above), setErrorMessage and setInvalid on Fields is not allowed anymore? All those methods are public for literally a decade (thinking back to V8), are really really often used by developers to create their own implementations on top of the fields or binder. Those are not marked deprecated or anything else - they just work differently / or not at all, if I understand you correctly. That's going to create a big backslash once more people are trying to migrate. Fields and how they interact with the Binder are literally the soul of Vaadin Flow.

@yuriy-fix
Copy link
Contributor

@knoobie , thank you for clarification!
The Flow components now have a logic for validation on blur that was highly anticipated in several tickets. The API of handleError is not deprecated and still can be used. The problem is that in this particular use-case described in the ticket the addValueChangeListener won't be called after blur to run check() as value was applied already before blurring the field. However, the validation itself is still applied through validators afterwards. So the timing for running check() is the issue in this case.
In order to resolve the timing issue without using Binder validators (which are chained) you would need to consider adding one more listener, for example:

field1.addClientValidatedEventListener(e -> {
    if (null == field1.getValue()) {
        return;
    }

    if (hasField2Value() && hasField3Value()) {
        check();
    }
});

The reason of why we are asking for use-cases is because we want to figure out what API could be added in order to achieve desired goals.

@knoobie
Copy link
Contributor Author

knoobie commented Mar 23, 2023

Thanks for the additional insights @yuriy-fix!

The problem with the Binder / adding those validations directly in the chain, comes from e.g. loose coupling and seperation of concern in complex applications. Especially those "info level" validations are more like a business requirement instead of e.g. normal technical requirement (not null / correct range) and are handled (in our case) way later in the form creation instead of the binder creation. We use for example a generic re-useable binder model and forms and only add those business requirement "validations" later once we know which form the binder and form is build for. At this stage of the circle, all BIndings are already finalized.

For such use-case having an API to allow additional Validator in the finalized Binding are needed, which are currently not available (not even a Binder::getBinding(HasValue) method) and therefore no Binding::addValidatoror Binding::removeValidator

In order to resolve the timing issue without using Binder validators (which are chained) you would need to consider adding one more listener.

Adding three listeners in my example fixes the issue above (replacing addValueChange with addClientValidated does not work 100% and creates flickering of error / not-error message depending on the action).

Just to confirm: What is the recommended and long-term supported implementation? Do you recommended to find a way to fully use the binder or rely on the client validated events in conjunction with value change (which feels kinda brittle) listeners?

@vursen
Copy link
Contributor

vursen commented Mar 23, 2023

@cdir: we cannot use Binder because we use our own binding framework. How does the binder set the validation, maybe we can use the same principle as a workaround?

Basically, there are two events that trigger Flow components to validate: ValueChangeEvent and ClientValidatedEvent.

Every field component internally listens to these events and runs constraint validation when they happen:

addValueChangeListener(e -> validate());
addClientValidatedEventListener(event -> validate());

At the end of the constraint validation, the component updates its invalid state:

When binding Binder to a component, Binder adds its own listeners to those two events on top of the component ones, which leads to the following sequence:

Constraint validation --> an invalid state update --> Binder validation --> an invalid state update

Binder updates the component's invalid state based on the chain of validators which includes


In your case – where you have a totally custom binder implementation – the current workaround would be to make sure you run your custom validation on both ValueChangeEvent and ClientValidatedEvent. Here is an example:

public class ExampleView extends Div {
    TextField textField;

    public ExampleView() {
        textField = new TextField();
        textField.setMinLength(10);
        textField.setMaxLength(20);
        textField.addValueChangeListener(event -> validate());
        textField.addClientValidatedEventListener(event -> validate());
        add(textField);
    }

    private void validate() {
        String value = textField.getValue();

        ValidationResult constraintValidationResult = textField
            .getDefaultValidator()
            .apply(value, null);

        // Check for the constraint validation result
        if (constraintValidationResult.isError()) {
            textField.setInvalid(true);
            return;
        }

        // Your custom validation rules
        if ("invalid".equals(textField.getValue())) {
            textField.setInvalid(true);
            return;
        }

        textField.setInvalid(false);
    }
}

@cdir
Copy link

cdir commented Mar 23, 2023

@vursen I think that is no option. Our own binding works like this:

valueChange --> update business model --> trigger internal 'model changed event' --> all fields are updated --> whole business model is validated --> update all validation messages

In the value change listener I only have the option to call the 'model change event'. Every model change event will validate the whole business model because we have quite a lot validations which refer to more complex state of the whole model. Assuming I call my model change event in valueChanged and clientValidationEvent, my own model change event is called several times. When I get the point of 'client validation event' correctly it is triggered every time a value changed in the client. That means I get at least two 'model change events'. Additionally there might be other fields that get updated when changing a value. For every updated field I get an additional event.

@leluna
Copy link

leluna commented Apr 24, 2023

Is there any updates on this issue? We are discovering more and more weird constellations caused by the discrepancy between the valid state in the UI and the model, especially in combination with required state. It is becoming increasingly confusing for our users when a field is marked as invalid/valid and why.

Our work around right now is basically to store the validation messages and the severity level (error = invalid) in a separate attribute, and completely ignore the client validation result. This feels like a really hacky "solution".

Similar to @knoobie s comment, our validations are all business requirements, including the required fields. The only technical validations are formatting validations. Ideally, our UI component should reflect the state of the business model at all time. In my opinion, client validation may add to server validations, but never remove or override any.

@rolfsmeds
Copy link
Contributor

We are looking into options for providing a solution for these custom validation use cases.

@yuriy-fix
Copy link
Contributor

Dear @knoobie,
Sorry for late reply! Binder is a long-term supported implementation. addValueChange with addClientValidated are mentioned as those allow flexibility with the validation solution. Those are used internally to trigger validation as described in the comment above. I have created an enhancement request for Flow that will allow implementing the desired behaviour without using mentioned listeners, but only using Binder API.

Dear @cdir,
With custom Binding mentioned it seems that you don't want the field to control the invalid state on its own, but instead rely on custom model validation. A more suitable solution in this case would be to override the validate:

class CustomDatePicker extends DatePicker {

    @Override
    protected void validate() {
        var requiredValidation = ValidationUtil.checkRequired(isRequired(), getValue(),
                getEmptyValue());

        boolean isFieldInvalid = requiredValidation.isError() || getDefaultValidator().apply(getValue(), null).isError();

        // Notify about updated invalid state if needed...
        // fireEvent(...isFieldInvalid...);

        // Set invalid state and error message if needed... For example, only when value is null:
        if (getValue() == null) {
            setErrorMessage("Custom error message");
            setInvalid(isFieldInvalid);
        }
    }
}

With getting result from getDefaultValidator you will be able to check what is the internal validity state of the field without applying it. I have added requiredValidation to the example as well, in case you don't have an explicit required check in the model validation.
With such implementation you are able to use value change listener for calling the 'model change event' and implement custom validation in the later stages of model updates (considering the results from getDefaultValidator if needed).

Dear @leluna,
Sorry for the confusion caused! The validation logic of the input fields is chained as described in #4804 (comment) and was recently updated to support highly anticipated validation on blur. It's intended to work out of the box with internal field constraints (min, max etc.) and Binder Validators. Hopefully the enhancement mentioned will make it more flexible as described in the comment above. For the custom validation logic when field shouldn't control the invalid state, it should be possible to override the validate as described above.
If the solutions mentioned don't suit your case, could you please provide more details on your use-case or share what API / behaviour you would like to see?
For example, would the convenience API for disallowing the field to set invalid state help in this case?

@leluna
Copy link

leluna commented May 8, 2023

Hi @yuriy-fix , thanks for the helpful reply!

I think being able to disallow the field to set invalid state would be exactly what we need. I think #16420 would probably also solve the issue, if we could remove the default validator at a later point?

The problem with overriding the validate method is that we would have to extend all components. Currently, we are mostly using the components as is.

@cdir
Copy link

cdir commented May 11, 2023

Hi @yuriy-fix,

as @leluna mentioned, overriding every component we use is not an acceptable solution. I think it should be possible to disable client side validation.

I am even more confused by HasValue.setRequiredIndicatorVisible(boolean). The JavaDoc says:

In case HTML element has its own (client-side)
validation it should be disabled when
<code>setRequiredIndicatorVisible(true)</code> is called and re-enabled
back on <code>setRequiredIndicatorVisible(false)</code>.

With this, I would expect to get no validation from the field. However, the field actually sets the invalid flag and validates that it should not be empty.

Right now, we only have the option to write our own validation message to the field through a second property and read it in an additional ClientValidatedEventListener to set it again. Not a satisfactory situation.

I also noticed that in a TextField, this listener is called on every keystroke when the field is invalid. In a ComboBox, on the other hand, the listener is not called when I set a value but only when I exit the field. This also seems to be inconsistent and leads to many unnecessary requests, especially in the TextField.

@knoobie
Copy link
Contributor Author

knoobie commented May 11, 2023

this listener is called on every keystroke when the field is invalid

Related issues: #4645 and #4986

@cdir
Copy link

cdir commented Jun 13, 2023

From my point of view, this is a bug or regression with Vaadin 24. With Vaadin 23, we could easily run a validation in the business logic and write to the field. Now we need ugly workarounds to achieve the same behavior which also affect the performance (although not much).

Either fix this or implement #5074 which seems to be a reasonable solution.

@vursen
Copy link
Contributor

vursen commented Jun 29, 2023

Hey!

I'm here to inform you that we've introduced a manual validation mode that disables the component's built-in validation, allowing you to have fully manual control over the component's invalid state and error messages. The feature will be included in the upcoming 24.2.0.alpha2 version.

The idea is that you simply enable this mode for your existing fields and it will prevent all the conflicts and overrides described in this ticket:

TextField textField = new TextField();
textField.setManualValidation(true);
textField.addValueChangeListener(event -> {
    // Run your custom validation logic on ValueChangeEvent as before
});

Feel free to reopen the ticket or create a new one if the issue hasn't been resolved for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion No decision yet, discussion needed validation
Projects
None yet
Development

No branches or pull requests

7 participants