Skip to content

Commit

Permalink
Only mark as required if empty value would fail validation
Browse files Browse the repository at this point in the history
Also fixes #2261

Related to vaadin/framework#9000
  • Loading branch information
Legioth committed May 18, 2018
1 parent 81a9e62 commit 8fc5cd9
Show file tree
Hide file tree
Showing 5 changed files with 220 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
* properties. The Binder automatically adds BeanValidator which validates beans
* using JSR-303 specification. It assumes that JSR-303 bean validation
* implementation is present on the classpath.
*
*
* @author Vaadin Ltd
* @see Binder
* @see BeanValidator
Expand Down Expand Up @@ -61,9 +61,9 @@ public BeanValidationBinder(Class<BEAN> beanType) {
if (!BeanUtil.checkBeanValidationAvailable()) {
throw new IllegalStateException(
BeanValidationBinder.class.getSimpleName()
+ " cannot be used because a JSR-303 Bean Validation "
+ "implementation not found on the classpath or could not be initialized. Use "
+ Binder.class.getSimpleName() + " instead");
+ " cannot be used because a JSR-303 Bean Validation "
+ "implementation not found on the classpath or could not be initialized. Use "
+ Binder.class.getSimpleName() + " instead");
}
this.beanType = beanType;
}
Expand Down Expand Up @@ -146,7 +146,8 @@ private void configureRequired(BindingBuilder<BEAN, ?> binding,
}
if (propertyDescriptor.getConstraintDescriptors().stream()
.map(ConstraintDescriptor::getAnnotation)
.anyMatch(requiredConfigurator)) {
.anyMatch(constraint -> requiredConfigurator.test(constraint,
binding))) {
binding.getField().setRequiredIndicatorVisible(true);
}
}
Expand Down
10 changes: 9 additions & 1 deletion flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,10 @@ protected BindingBuilderImpl(Binder<BEAN> binder,
this.statusHandler = statusHandler;
}

Converter<FIELDVALUE, ?> getConverterValidatorChain() {
return converterValidatorChain;
}

@Override
public Binding<BEAN, TARGET> bind(ValueProvider<BEAN, TARGET> getter,
Setter<BEAN, TARGET> setter) {
Expand Down Expand Up @@ -987,7 +991,7 @@ public HasValue<?, FIELDVALUE> getField() {
*
* @return the found locale, not null
*/
protected Locale findLocale() {
protected static Locale findLocale() {
Locale locale = null;
if (UI.getCurrent() != null) {
locale = UI.getCurrent().getLocale();
Expand Down Expand Up @@ -1064,6 +1068,10 @@ private BindingValidationStatus<TARGET> doValidation() {
* @return the value context
*/
protected ValueContext createValueContext() {
return createValueContext(field);
}

static ValueContext createValueContext(HasValue<?, ?> field) {
if (field instanceof Component) {
return new ValueContext((Component) field, field);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@
package com.vaadin.flow.data.binder;

import java.lang.annotation.Annotation;
import java.util.Objects;

import javax.validation.constraints.NotEmpty;
import javax.validation.constraints.NotNull;
import javax.validation.constraints.Size;

import com.vaadin.flow.component.HasValue;
import com.vaadin.flow.function.SerializablePredicate;
import com.vaadin.flow.data.binder.Binder.BindingBuilder;
import com.vaadin.flow.function.SerializableBiPredicate;

/**
* This interface represents a predicate which returns {@code true} if bound
Expand All @@ -36,47 +39,58 @@
*
*/
public interface RequiredFieldConfigurator
extends SerializablePredicate<Annotation> {
extends SerializableBiPredicate<Annotation, BindingBuilder<?, ?>> {

/**
* Configurator which is aware of {@literal @NotNull} annotation presence
* for a property.
* for a property where the default value is <code>null</code>.
*/
RequiredFieldConfigurator NOT_NULL = annotation -> annotation
.annotationType().equals(NotNull.class);
RequiredFieldConfigurator NOT_NULL = (annotation,
binding) -> annotation.annotationType().equals(NotNull.class)
&& RequiredFieldConfiguratorUtil.testConvertedDefaultValue(
binding, Objects::isNull);

/**
* Configurator which is aware of {@literal @NotEmpty} annotation presence
* for a property.
* for a property where the default value is empty.
*/
RequiredFieldConfigurator NOT_EMPTY = annotation -> annotation
.annotationType().getName()
.equals("org.hibernate.validator.constraints.NotEmpty");
RequiredFieldConfigurator NOT_EMPTY = (annotation,
binding) -> (annotation.annotationType().equals(NotEmpty.class)
|| annotation.annotationType().getName()
.equals("org.hibernate.validator.constraints.NotEmpty"))
&& RequiredFieldConfiguratorUtil.testConvertedDefaultValue(
binding,
RequiredFieldConfiguratorUtil::hasZeroSize);

/**
* Configurator which is aware of {@literal Size} annotation with
* {@code min()> 0} presence for a property.
* {@code min()> 0} presence for a property where the size of the default
* value is 0.
*/
RequiredFieldConfigurator SIZE = annotation -> annotation
.annotationType().equals(Size.class)
&& ((Size) annotation).min() > 0;
RequiredFieldConfigurator SIZE = (annotation,
binding) -> annotation.annotationType().equals(Size.class)
&& ((Size) annotation).min() > 0
&& RequiredFieldConfiguratorUtil.testConvertedDefaultValue(
binding,
RequiredFieldConfiguratorUtil::hasZeroSize);

/**
* Default configurator which is combination of {@link #NOT_NULL},
* {@link #NOT_EMPTY} and {@link #SIZE} configurators.
*/
RequiredFieldConfigurator DEFAULT = NOT_NULL.chain(NOT_EMPTY)
.chain(SIZE);
/**
* Default configurator which is combination of {@link #NOT_NULL},
* {@link #NOT_EMPTY} and {@link #SIZE} configurators.
*/
RequiredFieldConfigurator DEFAULT = NOT_NULL.chain(NOT_EMPTY).chain(SIZE);

/**
* Returns a configurator that chains together this configurator with the
* given configurator.
*
* @param configurator the configurator to chain, , not null
* @return a chained configurator
*/
default RequiredFieldConfigurator chain(
RequiredFieldConfigurator configurator) {
return descriptor -> test(descriptor) || configurator.test(descriptor);
}
/**
* Returns a configurator that chains together this configurator with the
* given configurator.
*
* @param configurator
* the configurator to chain, , not null
* @return a chained configurator
*/
default RequiredFieldConfigurator chain(
RequiredFieldConfigurator configurator) {
return (annotation, binding) -> test(annotation, binding)
|| configurator.test(annotation, binding);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/*
* Copyright 2000-2017 Vaadin Ltd.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package com.vaadin.flow.data.binder;

import java.lang.reflect.Array;
import java.util.Collection;
import java.util.Map;
import java.util.function.Predicate;

import javax.validation.constraints.Size;

import com.vaadin.flow.component.HasValue;
import com.vaadin.flow.data.binder.Binder.BindingBuilder;
import com.vaadin.flow.data.binder.Binder.BindingBuilderImpl;
import com.vaadin.flow.data.binder.Binder.BindingImpl;
import com.vaadin.flow.data.converter.Converter;

/**
* Helper methods used by {@link RequiredFieldConfigurator}. The methods are
* extracted to a separate class to prevent populating the public API of the
* {@link RequiredFieldConfigurator} interface.
*
* @author Vaadin Ltd
*/
public class RequiredFieldConfiguratorUtil {
private RequiredFieldConfiguratorUtil() {
// Only static helpers
}

/**
* Checks whether the given object would be considered empty according to
* the {@link Size} constraint.
*
* @param value
* the value to check
* @return true if the value is supported by the size constraint, otherwise
* <code>false</code>
*/
public static boolean hasZeroSize(Object value) {
if (value instanceof CharSequence) {
return ((CharSequence) value).length() == 0;
}
if (value instanceof Collection<?>) {
return ((Collection<?>) value).isEmpty();
}
if (value instanceof Map<?, ?>) {
return ((Map<?, ?>) value).isEmpty();
}
if (value != null && value.getClass().isArray()) {
return Array.getLength(value) == 0;
}

return false;
}

/**
* Tests the converted default value of the provided binding builder if
* possible.
*
* @param binding
* the binding builder to test
* @param predicate
* predicate for testing the converted default value
* @return <code>true</code> if a converted default value is available and
* it passes the test; <code>false</code> if no converted default
* value is available or if it doesn't pass the test
*/
@SuppressWarnings({ "rawtypes", "unchecked" })
public static boolean testConvertedDefaultValue(
BindingBuilder<?, ?> binding, Predicate<Object> predicate) {
if (binding instanceof BindingBuilderImpl<?, ?, ?>) {
HasValue<?, ?> field = binding.getField();
Converter converter = ((BindingBuilderImpl<?, ?, ?>) binding)
.getConverterValidatorChain();

Result<?> result = converter.convertToModel(field.getEmptyValue(),
BindingImpl.createValueContext(field));

if (!result.isError()) {
Object convertedEmptyValue = result
.getOrThrow(IllegalStateException::new);
return predicate.test(convertedEmptyValue);
}
}
return false;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,22 @@
*/
package com.vaadin.flow.data.binder;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;

import java.io.Serializable;
import java.time.LocalDate;
import java.util.List;
import java.util.Set;

import javax.validation.constraints.Digits;
import javax.validation.constraints.Max;
import javax.validation.constraints.NotEmpty;
import javax.validation.constraints.NotNull;
import javax.validation.constraints.Size;

import org.hibernate.validator.constraints.NotEmpty;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
Expand All @@ -36,11 +41,6 @@
import com.vaadin.flow.data.converter.StringToIntegerConverter;
import com.vaadin.flow.tests.data.bean.BeanToValidate;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;

public class BeanBinderTest
extends BinderTestBase<Binder<BeanToValidate>, BeanToValidate> {

Expand Down Expand Up @@ -87,6 +87,9 @@ public static class RequiredConstraints implements Serializable {
@NotEmpty
private String lastname;

@Size(min = 1)
private int sizeless;

private SubConstraint subfield;

public String getFirstname() {
Expand Down Expand Up @@ -121,6 +124,14 @@ public void setSubfield(SubConstraint subfield) {
this.subfield = subfield;
}

public int getSizeless() {
return sizeless;
}

public void setSizeless(int sizeless) {
this.sizeless = sizeless;
}

public static class SubConstraint implements Serializable {

@NotNull
Expand Down Expand Up @@ -416,7 +427,28 @@ public void beanBinderWithBoxedType() {
}

@Test
public void firstName_isNotNullConstraint_fieldIsRequired() {
public void firstName_isNotNullConstraint_nullableFieldIsRequired() {
BeanValidationBinder<RequiredConstraints> binder = new BeanValidationBinder<>(
RequiredConstraints.class);
RequiredConstraints bean = new RequiredConstraints();

TestTextField field = new TestTextField() {
@Override
public String getEmptyValue() {
return null;
}
};
binder.bind(field, "firstname");
binder.setBean(bean);

Assert.assertTrue(
"@NotNull field with default value null should be required",
field.isRequiredIndicatorVisible());
testSerialization(binder);
}

@Test
public void firstName_isNotNullConstraint_textFieldIsNotRequired() {
BeanValidationBinder<RequiredConstraints> binder = new BeanValidationBinder<>(
RequiredConstraints.class);
RequiredConstraints bean = new RequiredConstraints();
Expand All @@ -425,7 +457,27 @@ public void firstName_isNotNullConstraint_fieldIsRequired() {
binder.bind(field, "firstname");
binder.setBean(bean);

Assert.assertTrue(field.isRequiredIndicatorVisible());
Assert.assertFalse(
"@NotNull field with default value \"\" should not be required",
field.isRequiredIndicatorVisible());
testSerialization(binder);
}

@Test
public void sizeless_sizeConstraint_fieldIsNotRequired() {
BeanValidationBinder<RequiredConstraints> binder = new BeanValidationBinder<>(
RequiredConstraints.class);
RequiredConstraints bean = new RequiredConstraints();

TestTextField field = new TestTextField();
binder.forField(field)
.withConverter(new StringToIntegerConverter("yada"))
.bind("sizeless");
binder.setBean(bean);

Assert.assertFalse(
"@Size(min=1) field with integer value should not be required",
field.isRequiredIndicatorVisible());
testSerialization(binder);
}

Expand Down

0 comments on commit 8fc5cd9

Please sign in to comment.