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.

Sign up for GitHub

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

Jump to bottom

Adds skipDefaultValidator API #18549

Merged
merged 13 commits into from Feb 2, 2024
Merged

Adds skipDefaultValidator API #18549

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
89da0de
Adds skipDefaultValidator API
tepi Jan 26, 2024
2f65f53
Simplify API
tepi Jan 29, 2024
d6de853
Add tests
tepi Jan 29, 2024
cbdcfae
Rename method
tepi Jan 29, 2024
367580f
Change API to allow toggling skip default validators for Binder and B…
tepi Jan 30, 2024
bfee228
Cleanup
tepi Jan 30, 2024
b964c86
API naming changes; Add tests for dynamic disable/enable
tepi Jan 30, 2024
d0f483f
Merge branch 'main' into feat/17178-disable-default-validators-api
tepi Jan 30, 2024
1e15501
Fix javadoc error
tepi Jan 31, 2024
642b5be
Merge branch 'feat/17178-disable-default-validators-api' of github.co…
tepi Jan 31, 2024
beb1bf5
Change plural validators to validator for binding
tepi Jan 31, 2024
64079fb
Formatting
tepi Jan 31, 2024
75a8083
Merge branch 'main' into feat/17178-disable-default-validators-api
tepi Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
60 changes: 55 additions & 5 deletions flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java
View file
Open in desktop
Expand Up @@ -255,6 +255,13 @@ default BindingValidationStatus<TARGET> validate() {
*/
public boolean isValidatorsDisabled();

/**
* Returns if default validator of bound field is skipped or not.
*
* @return A boolean value.
*/
boolean isSkipDefaultValidator();
tepi marked this conversation as resolved.
Show resolved Hide resolved

/**
* Define whether the value should be converted back to the presentation
* in the field when a converter is used in binding.
Expand Down Expand Up @@ -824,6 +831,16 @@ default BindingBuilder<BEAN, TARGET> withStatusLabel(HasText label) {
BindingBuilder<BEAN, TARGET> withValidationStatusHandler(
BindingValidationStatusHandler handler);

/**
* Tells Binder to skip executing the default validators (e.g. min/max
* validators in DatePicker) for this binding.
*
* @return this binding, for chaining
* @see Binder#skipDefaultValidators() for faster way to skip default
* validators for all bound fields.
*/
BindingBuilder<BEAN, TARGET> skipDefaultValidator();

/**
* Sets the field to be required. This means two things:
* <ol>
Expand Down Expand Up @@ -938,6 +955,8 @@ protected static class BindingBuilderImpl<BEAN, FIELDVALUE, TARGET>

private boolean asRequiredSet;

private boolean skipDefaultValidator;

/**
* Creates a new binding builder associated with the given field.
* Initializes the builder with the given converter chain and status
Expand All @@ -960,6 +979,15 @@ protected BindingBuilderImpl(Binder<BEAN> binder,
this.binder = binder;
this.converterValidatorChain = converterValidatorChain;
this.statusHandler = statusHandler;

if (field instanceof HasValidator hasValidator) {
withValidator((val,
ctx) -> getBinder().skipDefaultValidators
|| binding.isSkipDefaultValidator()
? ValidationResult.ok()
: hasValidator.getDefaultValidator()
.apply(val, ctx));
}
}

Converter<FIELDVALUE, ?> getConverterValidatorChain() {
Expand Down Expand Up @@ -1103,6 +1131,13 @@ public BindingBuilder<BEAN, TARGET> withValidationStatusHandler(
return this;
}

@Override
public BindingBuilder<BEAN, TARGET> skipDefaultValidator() {
checkUnbound();
this.skipDefaultValidator = true;
return this;
}

@Override
public BindingBuilder<BEAN, TARGET> asRequired(
ErrorMessageProvider errorMessageProvider) {
Expand Down Expand Up @@ -1238,6 +1273,8 @@ protected static class BindingImpl<BEAN, FIELDVALUE, TARGET>

private Registration onValidationStatusChange;

private boolean skipDefaultValidator;

public BindingImpl(BindingBuilderImpl<BEAN, FIELDVALUE, TARGET> builder,
ValueProvider<BEAN, TARGET> getter,
Setter<BEAN, TARGET> setter) {
Expand All @@ -1247,6 +1284,8 @@ public BindingImpl(BindingBuilderImpl<BEAN, FIELDVALUE, TARGET> builder,
this.asRequiredSet = builder.asRequiredSet;
converterValidatorChain = ((Converter<FIELDVALUE, TARGET>) builder.converterValidatorChain);

skipDefaultValidator = builder.skipDefaultValidator;

onValueChange = getField().addValueChangeListener(
event -> handleFieldValueChange(event));

Expand Down Expand Up @@ -1566,6 +1605,11 @@ public boolean isValidatorsDisabled() {
return validatorsDisabled;
}

@Override
public boolean isSkipDefaultValidator() {
return skipDefaultValidator;
}

@Override
public void setConvertBackToPresentation(
boolean convertBackToPresentation) {
Expand Down Expand Up @@ -1724,6 +1768,8 @@ void setIdentity() {

private boolean fieldsValidationStatusChangeListenerEnabled = true;

private boolean skipDefaultValidators;

/**
* Creates a binder using a custom {@link PropertySet} implementation for
* finding and resolving property names for
Expand Down Expand Up @@ -1877,12 +1923,8 @@ public <FIELDVALUE> BindingBuilder<BEAN, FIELDVALUE> forField(
// clear previous errors for this field and any bean level validation
clearError(field);
getStatusLabel().ifPresent(label -> label.setText(""));

return createBinding(field, createNullRepresentationAdapter(field),
this::handleValidationStatus)
.withValidator(field instanceof HasValidator
? ((HasValidator) field).getDefaultValidator()
: Validator.alwaysPass());
this::handleValidationStatus);
}

/**
Expand Down Expand Up @@ -2709,6 +2751,14 @@ private List<ValidationResult> validateBean(BEAN bean) {
Collections::unmodifiableList));
}

/**
* Tells Binder to skip executing the default validators (e.g. min/max
* validators in DatePicker) of all bound fields by default.
*/
public void skipDefaultValidators() {
Copy link
Contributor

@knoobie knoobie Jan 29, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for consistency with the other fields I'm kinda missing setSkipDefaultValidators(boolean)

Otherwise it's also impossible to-renable the validators

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. We discussed this internally earlier today and concluded to keep it as simple as possible, so (at least for now) you can do one-time setting of skip for either whole binder or individual bindings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@knoobie Adding this method is possible of course, but I don't understand when exactly this would be needed to change this behaviour dynamically. Swapping this for the whole binder / form sounds as too much heavy logic. I would rather recommend to put the business logic into custom Binder's validators if one really needs to change validation logic on-the-fly. However, I may miss an experience of using Binder in more advanced cases, so could you please share your thoughts how setSkipDefaultValidators(boolean) could be used?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only thing that I could think of would be highly dynamic forms or even form builder that do things based on given objects and re-create all bindings.. but not sure how widely that's used.

I'm probably just biased because I Like side-effect free code.. or let's say, code that can be changed back once I messed-up.. now this method is a once used and it's impossible to change back (thinking unit tests for the binder for example) and it does not match all other flags the binder has, where it's possible to change them at "runtime" making it inconsistent to the API design. Personally I don't think there is much logic to consider by just replacing the direct access to the boolean with the getter from it, ensuring you are always getting the correct value the user supplied last - or do I miss something?

TL;DR: I'm not seeing the benefits of an inconsistent API

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed with @tepi on daily and it makes sense for a sake in consistent API to add setSkipDefaultValidators(boolean) methods as you suggested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reconsidering!

this.skipDefaultValidators = true;
}

/**
* Sets the label to show the binder level validation errors not related to
* any specific field.
Expand Down
74 changes: 73 additions & 1 deletion flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java
View file
Open in desktop
Expand Up @@ -39,6 +39,7 @@
import org.junit.Test;
import org.junit.rules.ExpectedException;

import com.vaadin.flow.component.HasValidation;
import com.vaadin.flow.component.HasValue;
import com.vaadin.flow.component.UI;
import com.vaadin.flow.data.binder.Binder.Binding;
Expand All @@ -51,7 +52,6 @@
import com.vaadin.flow.data.validator.IntegerRangeValidator;
import com.vaadin.flow.data.validator.NotEmptyValidator;
import com.vaadin.flow.data.validator.StringLengthValidator;
import com.vaadin.flow.function.ValueProvider;
import com.vaadin.flow.internal.CurrentInstance;
import com.vaadin.flow.tests.data.bean.Person;
import com.vaadin.flow.tests.data.bean.Sex;
Expand Down Expand Up @@ -1542,6 +1542,78 @@ public void two_asRequired_fields_without_initial_values_readBean() {
isEmptyString());
}

@Test
public void skipDefaultValidators_testNoneSkippedByDefault() {
TestTextFieldDefaultValidator field1 = new TestTextFieldDefaultValidator(
(val, ctx) -> ValidationResult.error("fail_1"));
TestTextFieldDefaultValidator field2 = new TestTextFieldDefaultValidator(
(val, ctx) -> ValidationResult.error("fail_2"));

binder.forField(field1).bind(Person::getFirstName,
Person::setFirstName);
binder.forField(field2).bind(Person::getLastName, Person::setLastName);

BinderValidationStatus<Person> status = binder.validate();
Assert.assertTrue(
"Validation should have two errors. "
+ "Default validators should be run.",
status.getValidationErrors().size() == 2);
}

@Test
public void skipDefaultValidators_testAllSkipped_whenBinderSkipSet() {
TestTextFieldDefaultValidator field1 = new TestTextFieldDefaultValidator(
(val, ctx) -> ValidationResult.error("fail_1"));
TestTextFieldDefaultValidator field2 = new TestTextFieldDefaultValidator(
(val, ctx) -> ValidationResult.error("fail_2"));
binder.skipDefaultValidators();

binder.forField(field1).bind(Person::getFirstName,
Person::setFirstName);
binder.forField(field2).bind(Person::getLastName, Person::setLastName);

BinderValidationStatus<Person> status = binder.validate();
Assert.assertTrue(
"Validation should not have errors. "
+ "Default validator should be skipped.",
status.getValidationErrors().isEmpty());
}

@Test
public void skipDefaultValidator_testSingleBindingSkipped_whenBinderSkipNotSet() {
TestTextFieldDefaultValidator field1 = new TestTextFieldDefaultValidator(
(val, ctx) -> ValidationResult.error("fail_1"));
TestTextFieldDefaultValidator field2 = new TestTextFieldDefaultValidator(
(val, ctx) -> ValidationResult.error("fail_2"));

binder.forField(field1).bind(Person::getFirstName,
Person::setFirstName);
binder.forField(field2).skipDefaultValidator().bind(Person::getLastName,
Person::setLastName);

BinderValidationStatus<Person> status = binder.validate();
Assert.assertTrue(
"Validation should have one error. "
+ "Only one default validators should be skipped.",
status.getValidationErrors().size() == 1);
}

public class TestTextFieldDefaultValidator extends TestTextField
implements HasValidator<String> {

private final Validator<String> defaultValidator;

public TestTextFieldDefaultValidator(
Validator<String> defaultValidator) {
this.defaultValidator = defaultValidator;
}

@Override
public Validator<String> getDefaultValidator() {
return defaultValidator;
}
}

@Test
public void refreshValueFromBean() {
Binding<Person, String> binding = binder.bind(nameField,
Expand Down