Hi Igor,
On Fri, Nov 15, 2013 at 10:30 PM, <[email protected]> wrote: > Updated Branches: > refs/heads/wicket-6.x 4bb19a48f -> 49922cdd5 > > > WICKET-5418 properly handle groups in NonNull constraint > > > Project: http://git-wip-us.apache.org/repos/asf/wicket/repo > Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/49922cdd > Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/49922cdd > Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/49922cdd > > Branch: refs/heads/wicket-6.x > Commit: 49922cdd5ccba3b97c59106d3aea769b8468962e > Parents: 4bb19a4 > Author: Igor Vaynberg <[email protected]> > Authored: Fri Nov 15 12:30:22 2013 -0800 > Committer: Igor Vaynberg <[email protected]> > Committed: Fri Nov 15 12:30:34 2013 -0800 > > ---------------------------------------------------------------------- > .../wicket-bean-validation/pom.xml | 6 + > .../bean/validation/PropertyValidator.java | 212 ++++++++++--------- > .../PropertyValidatorRequiredTest.java | 196 +++++++++++++++++ > 3 files changed, 318 insertions(+), 96 deletions(-) > ---------------------------------------------------------------------- > > > > http://git-wip-us.apache.org/repos/asf/wicket/blob/49922cdd/wicket-experimental/wicket-bean-validation/pom.xml > ---------------------------------------------------------------------- > diff --git a/wicket-experimental/wicket-bean-validation/pom.xml > b/wicket-experimental/wicket-bean-validation/pom.xml > index 9f3895c..460e7fc 100644 > --- a/wicket-experimental/wicket-bean-validation/pom.xml > +++ b/wicket-experimental/wicket-bean-validation/pom.xml > @@ -25,5 +25,11 @@ > <groupId>org.apache.wicket</groupId> > <artifactId>wicket-core</artifactId> > </dependency> > + <dependency> > + <groupId>org.hibernate</groupId> > + <artifactId>hibernate-validator</artifactId> > + <version>4.3.1.Final</version> > + <scope>test</scope> > + </dependency> > </dependencies> > </project> > > > http://git-wip-us.apache.org/repos/asf/wicket/blob/49922cdd/wicket-experimental/wicket-bean-validation/src/main/java/org/apache/wicket/bean/validation/PropertyValidator.java > ---------------------------------------------------------------------- > diff --git > a/wicket-experimental/wicket-bean-validation/src/main/java/org/apache/wicket/bean/validation/PropertyValidator.java > b/wicket-experimental/wicket-bean-validation/src/main/java/org/apache/wicket/bean/validation/PropertyValidator.java > index 032a818..93bd4c4 100644 > --- > a/wicket-experimental/wicket-bean-validation/src/main/java/org/apache/wicket/bean/validation/PropertyValidator.java > +++ > b/wicket-experimental/wicket-bean-validation/src/main/java/org/apache/wicket/bean/validation/PropertyValidator.java > @@ -1,6 +1,10 @@ > package org.apache.wicket.bean.validation; > > +import java.util.ArrayList; > +import java.util.Arrays; > +import java.util.HashSet; > import java.util.Iterator; > +import java.util.List; > import java.util.Set; > > import javax.validation.ConstraintViolation; > @@ -18,40 +22,43 @@ import org.apache.wicket.validation.IValidatable; > import org.apache.wicket.validation.IValidator; > > /** > - * Validator that delegates to the bean validation framework. The > integration has to be first > - * configured using {@link BeanValidationConfiguration}. > + * Validator that delegates to the bean validation framework. The > integration > + * has to be first configured using {@link BeanValidationConfiguration}. > * > * <p> > - * The validator must be provided a {@link Property}, unless one can be > resolved from the component > - * implicitly. By default the configuration contains the {@link > DefaultPropertyResolver} so > - * {@link PropertyModel}s are supported out of the box - when attached to > a component with a > - * property model the property does not need to be specified explicitly. > + * The validator must be provided a {@link Property}, unless one can be > resolved > + * from the component implicitly. By default the configuration contains > the > + * {@link DefaultPropertyResolver} so {@link PropertyModel}s are > supported out > + * of the box - when attached to a component with a property model the > property > + * does not need to be specified explicitly. > * </p> > * > * <p> > - * The validator will set the required flag on the form component it is > attached to based on the > - * presence of the @NotNull annotation. Notice, the required flag will > only be set to {@code true}, > - * components with the required flag already set to {@code true} will not > have the flag set to > - * {@code false} by this validator. > + * The validator will set the required flag on the form component it is > attached > + * to based on the presence of the @NotNull annotation. Notice, the > required > + * flag will only be set to {@code true}, components with the required > flag > + * already set to {@code true} will not have the flag set to {@code > false} by > + * this validator. > * </p> > * > * <p> > - * The validator will allow {@link ITagModifier}s configured in {@link > BeanValidationConfiguration} > - * to mutate the markup tag of the component it is attached to. > + * The validator will allow {@link ITagModifier}s configured in > + * {@link BeanValidationConfiguration} to mutate the markup tag of the > component > + * it is attached to. > * </p> > * > * <p> > - * The validator specifies default error messages in the {@code > PropertyValidator.properties} file. > - * These values can be overridden in the application subclass' property > files globally or in the > - * page or panel properties locally. See this file for the default > messages supported. > + * The validator specifies default error messages in the > + * {@code PropertyValidator.properties} file. These values can be > overridden in > + * the application subclass' property files globally or in the page or > panel > + * properties locally. See this file for the default messages supported. > * </p> > * > * @author igor > * > * @param <T> > */ > -public class PropertyValidator<T> extends Behavior implements > IValidator<T> > -{ > +public class PropertyValidator<T> extends Behavior implements > IValidator<T> { > It seems you use different formatting style. Maybe because of the removed .settings/ folders... It is very hard for me to review (and learn from) this change. > private static final Class<?>[] EMPTY = new Class<?>[0]; > > private FormComponent<T> component; > @@ -67,49 +74,41 @@ public class PropertyValidator<T> extends Behavior > implements IValidator<T> > */ > private boolean requiredFlagSet; > > - public PropertyValidator(Class<?>... groups) > - { > + public PropertyValidator(Class<?>... groups) { > this(null, groups); > } > > - public PropertyValidator(IModel<Class<?>[]> groups) > - { > + public PropertyValidator(IModel<Class<?>[]> groups) { > this(null, groups); > } > > - public PropertyValidator(Property property, Class<?>... groups) > - { > + public PropertyValidator(Property property, Class<?>... groups) { > this(property, new GroupsModel(groups)); > } > > - public PropertyValidator(Property property, IModel<Class<?>[]> > groups) > - { > + public PropertyValidator(Property property, IModel<Class<?>[]> > groups) { > this.property_ = property; > this.groups_ = groups; > } > > - private Property getProperty() > - { > - if (property_ == null) > - { > - property_ = > BeanValidationConfiguration.get().resolveProperty(component); > - if (property_ == null) > - { > + private Property getProperty() { > + if (property_ == null) { > + property_ = > BeanValidationConfiguration.get().resolveProperty( > + component); > + if (property_ == null) { > throw new IllegalStateException( > - "Could not resolve Property from > component: " + > - component + > - ". Either specify the > Property in the constructor or use a model that works in combination with a > " + > - > IPropertyResolver.class.getSimpleName() + > - " to resolve the Property > automatically"); > + "Could not resolve > Property from component: " > + + component > + + ". > Either specify the Property in the constructor or use a model that works in > combination with a " > + + > IPropertyResolver.class.getSimpleName() > + + " to > resolve the Property automatically"); > } > } > return property_; > } > > - private Class<?>[] getGroups() > - { > - if (groups_ == null) > - { > + private Class<?>[] getGroups() { > + if (groups_ == null) { > return EMPTY; > } > return groups_.getObject(); > @@ -117,106 +116,127 @@ public class PropertyValidator<T> extends Behavior > implements IValidator<T> > > @SuppressWarnings("unchecked") > @Override > - public void bind(Component component) > - { > - if (this.component != null) > - { > + public void bind(Component component) { > + if (this.component != null) { > throw new IllegalStateException( // > - "This validator has already been added to > component: " + this.component + > - ". This validator does not support > reusing instances, please create a new one"); > + "This validator has already been > added to component: " > + + this.component > + + ". This > validator does not support reusing instances, please create a new one"); > } > > - if (!(component instanceof FormComponent)) > - { > - throw new > IllegalStateException(getClass().getSimpleName() + > - " can only be added to FormComponents"); > + if (!(component instanceof FormComponent)) { > + throw new > IllegalStateException(getClass().getSimpleName() > + + " can only be added to > FormComponents"); > } > > - // TODO add a validation key that appends the type so we > can have different messages for > - // @Size on String vs Collection - done but need to add a > key for each superclass/interface > + // TODO add a validation key that appends the type so we > can have > + // different messages for > + // @Size on String vs Collection - done but need to add a > key for each > + // superclass/interface > > - this.component = (FormComponent<T>)component; > + this.component = (FormComponent<T>) component; > } > > @Override > - public void onConfigure(Component component) > - { > + public void onConfigure(Component component) { > super.onConfigure(component); > - if (requiredFlagSet == false) > - { > - // "Required" flag is calculated upon component's > model property, so we must ensure, > - // that model object is accessible (i.e. component > is already added in a page). > + if (requiredFlagSet == false) { > + // "Required" flag is calculated upon component's > model property, so > + // we must ensure, > + // that model object is accessible (i.e. component > is already added > + // in a page). > requiredFlagSet = true; > - setComponentRequiredFlag(); > + if (isRequired()) { > + this.component.setRequired(true); > + } > } > } > > @Override > - public void detach(Component component) > - { > + public void detach(Component component) { > super.detach(component); > - if (groups_ != null) > - { > + if (groups_ != null) { > groups_.detach(); > } > } > > - /** > - * Marks the form component required if necessary > - */ > - private void setComponentRequiredFlag() > - { > + private List<NotNull> findNotNullConstraints() { > BeanValidationContext config = > BeanValidationConfiguration.get(); > Validator validator = config.getValidator(); > Property property = getProperty(); > > - // if the property has a NotNull constraint mark the form > component required > + List<NotNull> constraints = new ArrayList<NotNull>(); > + > + Iterator<ConstraintDescriptor<?>> it = new > ConstraintIterator( > + validator, property); > > - Iterator<ConstraintDescriptor<?>> it = new > ConstraintIterator(validator, property); > - while (it.hasNext()) > - { > + while (it.hasNext()) { > ConstraintDescriptor<?> desc = it.next(); > - if > (desc.getAnnotation().annotationType().equals(NotNull.class)) > - { > - component.setRequired(true); > - break; > + if > (desc.getAnnotation().annotationType().equals(NotNull.class)) { > + constraints.add((NotNull) > desc.getAnnotation()); > + } > + } > + > + return constraints; > + } > + > + boolean isRequired() { > + List<NotNull> constraints = findNotNullConstraints(); > + > + if (constraints.isEmpty()) { > + return false; > + } > + > + HashSet<Class<?>> validatorGroups = new > HashSet<Class<?>>(); > + validatorGroups.addAll(Arrays.asList(getGroups())); > + > + for (NotNull constraint : constraints) { > + if (constraint.groups().length == 0 && > validatorGroups.isEmpty()) { > + return true; > + } > + > + for (Class<?> constraintGroup : > constraint.groups()) { > + if > (validatorGroups.contains(constraintGroup)) { > + return true; > + } > } > } > + > + return false; > } > > @Override > @SuppressWarnings({ "rawtypes", "unchecked" }) > - public void onComponentTag(Component component, ComponentTag tag) > - { > + public void onComponentTag(Component component, ComponentTag tag) { > super.onComponentTag(component, tag); > > BeanValidationContext config = > BeanValidationConfiguration.get(); > Validator validator = config.getValidator(); > Property property = getProperty(); > > - // find any tag modifiers that apply to the constraints of > the property being validated > + // find any tag modifiers that apply to the constraints of > the property > + // being validated > // and allow them to modify the component tag > > - Iterator<ConstraintDescriptor<?>> it = new > ConstraintIterator(validator, property, > - getGroups()); > + Iterator<ConstraintDescriptor<?>> it = new > ConstraintIterator( > + validator, property, getGroups()); > > - while (it.hasNext()) > - { > + while (it.hasNext()) { > ConstraintDescriptor<?> desc = it.next(); > > - ITagModifier modifier = > config.getTagModifier(desc.getAnnotation().annotationType()); > + ITagModifier modifier = > config.getTagModifier(desc.getAnnotation() > + .annotationType()); > > - if (modifier != null) > - { > - > modifier.modify((FormComponent<?>)component, tag, desc.getAnnotation()); > + if (modifier != null) { > + modifier.modify((FormComponent<?>) > component, tag, > + desc.getAnnotation()); > } > } > } > > @SuppressWarnings("unchecked") > @Override > - public void validate(IValidatable<T> validatable) > - { > + public void validate(IValidatable<T> validatable) { > BeanValidationContext config = > BeanValidationConfiguration.get(); > Validator validator = config.getValidator(); > > @@ -224,14 +244,14 @@ public class PropertyValidator<T> extends Behavior > implements IValidator<T> > > // validate the value using the bean validator > > - Set<?> violations = > validator.validateValue(property.getOwner(), property.getName(), > - validatable.getValue(), getGroups()); > + Set<?> violations = > validator.validateValue(property.getOwner(), > + property.getName(), > validatable.getValue(), getGroups()); > > // iterate over violations and report them > > - for (ConstraintViolation<?> violation : > (Set<ConstraintViolation<?>>)violations) > - { > - > validatable.error(config.getViolationTranslator().convert(violation)); > + for (ConstraintViolation<?> violation : > (Set<ConstraintViolation<?>>) violations) { > + validatable.error(config.getViolationTranslator() > + .convert(violation)); > } > } > > > > http://git-wip-us.apache.org/repos/asf/wicket/blob/49922cdd/wicket-experimental/wicket-bean-validation/src/test/java/org/apache/wicket/bean/validation/PropertyValidatorRequiredTest.java > ---------------------------------------------------------------------- > diff --git > a/wicket-experimental/wicket-bean-validation/src/test/java/org/apache/wicket/bean/validation/PropertyValidatorRequiredTest.java > b/wicket-experimental/wicket-bean-validation/src/test/java/org/apache/wicket/bean/validation/PropertyValidatorRequiredTest.java > new file mode 100644 > index 0000000..e5ee0cc > --- /dev/null > +++ > b/wicket-experimental/wicket-bean-validation/src/test/java/org/apache/wicket/bean/validation/PropertyValidatorRequiredTest.java > @@ -0,0 +1,196 @@ > +package org.apache.wicket.bean.validation; > + > +import static org.junit.Assert.*; > + > +import javax.validation.constraints.NotNull; > + > +import org.apache.wicket.MarkupContainer; > +import org.apache.wicket.markup.IMarkupResourceStreamProvider; > +import org.apache.wicket.markup.html.WebPage; > +import org.apache.wicket.markup.html.form.Form; > +import org.apache.wicket.markup.html.form.FormComponent; > +import org.apache.wicket.markup.html.form.TextField; > +import org.apache.wicket.mock.MockApplication; > +import org.apache.wicket.model.PropertyModel; > +import org.apache.wicket.util.resource.IResourceStream; > +import org.apache.wicket.util.resource.StringResourceStream; > +import org.apache.wicket.util.tester.WicketTester; > +import org.apache.wicket.util.tester.WicketTesterScope; > +import org.junit.Rule; > +import org.junit.Test; > + > +public class PropertyValidatorRequiredTest { > + @Rule > + public static WicketTesterScope scope = new WicketTesterScope() { > + protected WicketTester create() { > + return new WicketTester(new TestApplication()); > + }; > + }; > + > + @Test > + public void test() { > + TestPage page = > scope.getTester().startPage(TestPage.class); > + > + // no group > + assertTrue(page.input1.isRequired()); > + assertFalse(page.input2.isRequired()); > + assertFalse(page.input3.isRequired()); > + assertFalse(page.input4.isRequired()); > + > + // group1 > + assertFalse(page.input5.isRequired()); > + assertTrue(page.input6.isRequired()); > + assertFalse(page.input7.isRequired()); > + assertTrue(page.input8.isRequired()); > + > + // group2 > + assertFalse(page.input9.isRequired()); > + assertFalse(page.input10.isRequired()); > + assertTrue(page.input11.isRequired()); > + assertTrue(page.input12.isRequired()); > + > + // group1+group2 > + assertFalse(page.input13.isRequired()); > + assertTrue(page.input14.isRequired()); > + assertTrue(page.input15.isRequired()); > + assertTrue(page.input16.isRequired()); > + > + // group3 > + assertFalse(page.input17.isRequired()); > + assertFalse(page.input18.isRequired()); > + assertFalse(page.input19.isRequired()); > + assertFalse(page.input20.isRequired()); > + > + } > + > + public static class TestApplication extends MockApplication { > + @Override > + protected void init() { > + super.init(); > + new BeanValidationConfiguration().configure(this); > + } > + } > + > + public static class TestPage extends WebPage implements > + IMarkupResourceStreamProvider { > + > + private TestBean bean = new TestBean(); > + private FormComponent<String> input1, input2, input3, > input4, input5, > + input6, input7, input8, input9, input10, > input11, input12, > + input13, input14, input15, input16, > input17, input18, input19, > + input20; > + > + public TestPage() { > + Form<?> form = new Form<Void>("form"); > + add(form); > + > + input1 = new TextField<String>("input1", new > PropertyModel<String>( > + this, "bean.property")) > + .add(new > PropertyValidator<String>()); > + input2 = new TextField<String>("input2", new > PropertyModel<String>( > + this, "bean.propertyOne")) > + .add(new > PropertyValidator<String>()); > + input3 = new TextField<String>("input3", new > PropertyModel<String>( > + this, "bean.propertyTwo")) > + .add(new > PropertyValidator<String>()); > + input4 = new TextField<String>("input4", new > PropertyModel<String>( > + this, "bean.propertyOneTwo")) > + .add(new > PropertyValidator<String>()); > + > + input5 = new TextField<String>("input5", new > PropertyModel<String>( > + this, "bean.property")).add(new > PropertyValidator<String>( > + GroupOne.class)); > + input6 = new TextField<String>("input6", new > PropertyModel<String>( > + this, "bean.propertyOne")) > + .add(new > PropertyValidator<String>(GroupOne.class)); > + input7 = new TextField<String>("input7", new > PropertyModel<String>( > + this, "bean.propertyTwo")) > + .add(new > PropertyValidator<String>(GroupOne.class)); > + input8 = new TextField<String>("input8", new > PropertyModel<String>( > + this, "bean.propertyOneTwo")) > + .add(new > PropertyValidator<String>(GroupOne.class)); > + > + input9 = new TextField<String>("input9", new > PropertyModel<String>( > + this, "bean.property")).add(new > PropertyValidator<String>( > + GroupTwo.class)); > + input10 = new TextField<String>("input10", > + new PropertyModel<String>(this, > "bean.propertyOne")) > + .add(new > PropertyValidator<String>(GroupTwo.class)); > + input11 = new TextField<String>("input11", > + new PropertyModel<String>(this, > "bean.propertyTwo")) > + .add(new > PropertyValidator<String>(GroupTwo.class)); > + input12 = new TextField<String>("input12", > + new PropertyModel<String>(this, > "bean.propertyOneTwo")) > + .add(new > PropertyValidator<String>(GroupTwo.class)); > + > + input13 = new TextField<String>("input13", > + new PropertyModel<String>(this, > "bean.property")) > + .add(new > PropertyValidator<String>(GroupOne.class, > + GroupTwo.class)); > + input14 = new TextField<String>("input14", > + new PropertyModel<String>(this, > "bean.propertyOne")) > + .add(new > PropertyValidator<String>(GroupOne.class, > + GroupTwo.class)); > + input15 = new TextField<String>("input15", > + new PropertyModel<String>(this, > "bean.propertyTwo")) > + .add(new > PropertyValidator<String>(GroupOne.class, > + GroupTwo.class)); > + input16 = new TextField<String>("input16", > + new PropertyModel<String>(this, > "bean.propertyOneTwo")) > + .add(new > PropertyValidator<String>(GroupOne.class, > + GroupTwo.class)); > + > + input17 = new TextField<String>("input17", > + new PropertyModel<String>(this, > "bean.property")) > + .add(new > PropertyValidator<String>(GroupThree.class)); > + input18 = new TextField<String>("input18", > + new PropertyModel<String>(this, > "bean.propertyOne")) > + .add(new > PropertyValidator<String>(GroupThree.class)); > + input19 = new TextField<String>("input19", > + new PropertyModel<String>(this, > "bean.propertyTwo")) > + .add(new > PropertyValidator<String>(GroupThree.class)); > + input20 = new TextField<String>("input20", > + new PropertyModel<String>(this, > "bean.propertyOneTwo")) > + .add(new > PropertyValidator<String>(GroupThree.class)); > + > + form.add(input1, input2, input3, input4, input5, > input6, input7, > + input8, input9, input10, input11, > input12, input13, > + input14, input15, input16, > input17, input18, input19, > + input20); > + > + } > + > + @Override > + public IResourceStream getMarkupResourceStream( > + MarkupContainer container, Class<?> > containerClass) { > + return new StringResourceStream( > + "<form wicket:id='form'><input > wicket:id='input1'/><input wicket:id='input2'/><input > wicket:id='input3'/><input wicket:id='input4'/><input > wicket:id='input5'/><input wicket:id='input6'/><input > wicket:id='input7'/><input wicket:id='input8'/><input > wicket:id='input9'/><input wicket:id='input10'/><input > wicket:id='input11'/><input wicket:id='input12'/><input > wicket:id='input13'/><input wicket:id='input14'/><input > wicket:id='input15'/><input wicket:id='input16'/><input > wicket:id='input17'/><input wicket:id='input18'/><input > wicket:id='input19'/><input wicket:id='input20'/></form>"); > + } > + > + } > + > + public static interface GroupOne { > + } > + > + public static interface GroupTwo { > + } > + > + public static interface GroupThree { > + } > + > + public static class TestBean { > + @NotNull > + String property; > + > + @NotNull(groups = { GroupOne.class }) > + String propertyOne; > + > + @NotNull(groups = { GroupTwo.class }) > + String propertyTwo; > + > + @NotNull(groups = { GroupOne.class, GroupTwo.class }) > + String propertyOneTwo; > + > + } > + > +} > >
