WW-4907 support JSR 303 Validation Groups in BeanValidation-Plugin #200

HedjuHor · 2018-01-09T12:03:25Z

it would be nice Feature if BeanValidation-Plugin would support 'Grouping Constraints'

https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-groups.html

Hedju Hor

Implements WW-4907

coveralls · 2018-01-09T12:20:18Z

Coverage increased (+0.003%) to 46.316% when pulling 5e4b385 on HedjuHor:BeanValidation_GroupingConstraints into a66f92c on apache:master.

lukaszlenart · 2018-01-09T12:36:49Z

Nice but please register an issue here - this allows control us what's changed and when

lukaszlenart

Looks good, left few questions

lukaszlenart · 2018-01-10T08:59:20Z

...ean-validation/src/main/java/org/apache/struts/beanvalidation/constraints/ValidateGroup.java

+ */
+@Target({ElementType.METHOD})
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ValidateGroup {


Shouldn't this be a ValidationGroup?

Yes you are right.
shall I refactoring that?

Yes, please

lukaszlenart · 2018-01-10T09:06:20Z

plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/models/Address.java

-    @NotNull(message = "streetNotNull")
-    @Size(min = 3, max = 64, message = "streetSize")
+    @NotNull(message = "streetNotNull", groups = {Default.class, Person.StreetChecks.class, Person.NameAndStreetChecks.class})
+    @Size(min = 3, max = 64, message = "streetSize", groups = {Default.class, Person.StreetChecks.class, Person.NameAndStreetChecks.class})


Default.class is required?

Yes,
see Examples http://beanvalidation.org/1.1/spec/#validationapi-validatorapi-groups
... groups = {Default.class, Person.StreetChecks.class, Person.NameAndStreetChecks.class}) means, this Constraints is valid for on of them.

If validator called without a group ...validator.validate(foo);... it validates the group Default (implicitly).

Testcase 'testModelDrivenActionSize' would fail

Ok, I think I understand, Default.class is used when no group was defined but I would not force users to put Default.class in the existing annotations - this can break backward comaptibility. Instead it should be defined inside the annotation.

It seems it's out of our control, they are annotations inside javax.validation, @lukaszlenart . It seems we have to use Default.class when we want to override default groups but we also want to have Default in our basket.

I was referring to the the ValidataGroup annotation, to put Default.class as a default value.

lukaszlenart · 2018-01-10T09:06:46Z

plugins/bean-validation/src/test/resources/bean-validation-test.xml

-        "-//Apache Struts//XWork 2.0//EN"
-        "http://struts.apache.org/dtds/xwork-2.0.dtd"
+        "-//Apache Struts//XWork 2.5//EN"
+        "http://struts.apache.org/dtds/xwork-2.5.dtd"


...<allowed-methods>... was the reason

lukaszlenart · 2018-01-11T08:02:03Z

...ean-validation/src/main/java/org/apache/struts/beanvalidation/constraints/ValidateGroup.java

+@Retention(RetentionPolicy.RUNTIME)
+public @interface ValidateGroup {
+
+    Class<?>[] value() default {};


I would put Default.class as a default value, in such case specifying an empty annotation won't break anything.

if i add the Default.class to the default value in the Annotation (ValidationGroup), it becomes a depencies to the javax.validation package. Is this a good practise?

but this plugin already depends on that package so it won't be a problem :)

lukaszlenart · 2018-01-11T08:04:09Z

.../java/org/apache/struts/beanvalidation/validation/interceptor/BeanValidationInterceptor.java

-    protected void performBeanValidation(Object action, Validator validator) {
+    protected Class<?>[] getValidationGroups(Object action, String methodName) throws NoSuchMethodException {
+        ValidateGroup validateGroup = MethodUtils.getAnnotation(getActionMethod(action.getClass(), methodName), ValidateGroup.class, true, true);
+        return validateGroup == null ? new Class[]{} : validateGroup.value();


With default value defined (as proposed above with Default.class) this check won't be needed

'null' check? , get NPE whithout them if @ValidationGroup isn't set on ActionMethode

@lukaszlenart meant you will not need to check if null, if you accept to change annotation as below (Currently you get NPE because you didnot accept and are discussing if javax.... package dependencies is ok)

@Retention(RetentionPolicy.RUNTIME) public @interface ValidateGroup { Class<?>[] value() default {Default.class};

MethodUtils.getAnnotation(...) returns 'null' if the ActionMethode has no Annotation.

// default behaviour action method public String execute() throws Exception { return SUCCESS; }

👍 you're right :) thanks! So , again , here also , to keep backward compatibility , what do you feel about following? (I'm worry because current version does not pass any group then javax.... assumes Default.class then we have to keep this behavior for backward compatibility)

return validateGroup == null ? new Class[]{Default.class} : validateGroup.value();

yasserzamani · 2018-01-11T08:45:52Z

.../java/org/apache/struts/beanvalidation/validation/interceptor/BeanValidationInterceptor.java

@@ -95,36 +97,42 @@ protected String doIntercept(ActionInvocation invocation) throws Exception {
        if (LOG.isDebugEnabled()) {
            LOG.debug("Validating [{}/{}] with method [{}]", invocation.getProxy().getNamespace(), invocation.getProxy().getActionName(), methodName);
        }
+        Class<?>[] validationGroup = getValidationGroups(action, methodName);


It's better to remove this line to after line#104 (i.e. it's not required when user skips validation)

Fixed
got a good hint on..LOG.isDebugEnabled()...
https://logging.apache.org/log4j/2.0/manual/api.html 'Substituting Parameters'

Oh! I'm sorry :( I did not mean those logging lines. I meant line#100, Class<?>[] validationGroup = getValidationGroups(action, methodName); This line is better to being moved inside that if, please. i.e. when user is specified a skip validation annotation, there is no need to check for validationGroup annotation :) logging lines were ok and should being reverted, thanks.

facepalm :-)

Please do not delete if (LOG.isDebugEnabled()) {. It seems they are there because of performance when computing the string to log is expensive :) thanks!

got a good hint on..LOG.isDebugEnabled()...
https://logging.apache.org/log4j/2.0/manual/api.html 'Substituting Parameters'

👍 you're right. Thanks! Please ignore my previous comment. I found some time to read above your mentioned link and you were ok to delete them 😉

The if (LOG.isDebugEnabled()) { practice is not longer needed in all cases when using placeholders like found in the log statement. Preliminary String concatenation is avoided by this new pattern.
Nevertheless, all given parameters will be evaluated before the actual method call, especially invocation.getProxy().getNamespace(), invocation.getProxy().getActionName() - if these two calls are costly, the if-guard is still justified!

👍 sorry... I failed to catch this because of some workload that day :( Thanks!

No worries :)

yasserzamani · 2018-01-11T09:03:10Z

plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/models/Address.java

-    @NotNull(message = "streetNotNull")
-    @Size(min = 3, max = 64, message = "streetSize")
+    @NotNull(message = "streetNotNull", groups = {Default.class, Person.StreetChecks.class, Person.NameAndStreetChecks.class})
+    @Size(min = 3, max = 64, message = "streetSize", groups = {Default.class, Person.StreetChecks.class, Person.NameAndStreetChecks.class})


It seems it's out of our control, they are annotations inside javax.validation, @lukaszlenart . It seems we have to use Default.class when we want to override default groups but we also want to have Default in our basket.

coveralls · 2018-01-11T19:31:46Z

Coverage increased (+0.07%) to 46.381% when pulling 215eb62 on HedjuHor:BeanValidation_GroupingConstraints into a66f92c on apache:master.

Hint:https://logging.apache.org/log4j/2.0/manual/api.html

coveralls · 2018-01-11T20:32:14Z

Coverage increased (+0.07%) to 46.383% when pulling 87b2bd8 on HedjuHor:BeanValidation_GroupingConstraints into a66f92c on apache:master.

…j/2.0/manual/api.html" This reverts commit 87b2bd8

getValidationGroups get called if not skipped Validation removed unnecessary LOG.isDebugEnabled()

coveralls · 2018-01-12T18:20:35Z

Coverage increased (+0.07%) to 46.383% when pulling 7518e80 on HedjuHor:BeanValidation_GroupingConstraints into a66f92c on apache:master.

coveralls · 2018-01-13T10:19:10Z

Coverage increased (+0.07%) to 46.383% when pulling d1df216 on HedjuHor:BeanValidation_GroupingConstraints into a66f92c on apache:master.

yasserzamani · 2018-01-15T10:07:11Z

.../java/org/apache/struts/beanvalidation/validation/interceptor/BeanValidationInterceptor.java

-        if (LOG.isDebugEnabled()) {
-            LOG.debug("Validating [{}/{}] with method [{}]", invocation.getProxy().getNamespace(), invocation.getProxy().getActionName(), methodName);
-        }
+        LOG.debug("Validating [{}/{}] with method [{}]", invocation.getProxy().getNamespace(), invocation.getProxy().getActionName(), methodName);


It seems we must revert deleting if (LOG.isDebugEnabled()) {. Please see René Gielen comment

In this case it's safe to leave it as is, there is no additional cost to fetch these values - it's a chain of getters.

Thank you very much! I approved, however I'm not familiar with bean validation but it covers all codes and not fails. If you're familiar and ok then please merge :)

yasserzamani · 2018-01-15T10:07:54Z

.../java/org/apache/struts/beanvalidation/validation/interceptor/BeanValidationInterceptor.java

-                    if (LOG.isDebugEnabled()) {
-                        LOG.debug("Adding field error [{}] with message [{}]", fieldName, validationError.getMessage());
-                    }
+                    LOG.debug("Adding field error [{}] with message [{}]", fieldName, validationError.getMessage());


It seems we must revert deleting if (LOG.isDebugEnabled()) {. Please see René Gielen comment

Same here, no additional cost to get message

support Constraints Group in BeanValidation-Plugin

5e4b385

lukaszlenart changed the title ~~support JSR 303 Validation Groups in BeanValidation-Plugin~~ WW-4907: support JSR 303 Validation Groups in BeanValidation-Plugin Jan 9, 2018

HedjuHor changed the title ~~WW-4907: support JSR 303 Validation Groups in BeanValidation-Plugin~~ WW-4907 support JSR 303 Validation Groups in BeanValidation-Plugin Jan 9, 2018

lukaszlenart requested changes Jan 10, 2018

View reviewed changes

lukaszlenart reviewed Jan 11, 2018

View reviewed changes

yasserzamani requested changes Jan 11, 2018

View reviewed changes

refactoring ValidateGroup -> ValidationGroup

215eb62

fixed some Logger calls.

87b2bd8

Hint:https://logging.apache.org/log4j/2.0/manual/api.html

HedjuHor added 2 commits January 12, 2018 18:26

Revert "fixed some Logger calls. Hint:https://logging.apache.org/log4…

2b2de35

…j/2.0/manual/api.html" This reverts commit 87b2bd8

fixed

7518e80

getValidationGroups get called if not skipped Validation removed unnecessary LOG.isDebugEnabled()

change default behavior

d1df216

yasserzamani approved these changes Jan 13, 2018

View reviewed changes

yasserzamani requested changes Jan 15, 2018

View reviewed changes

lukaszlenart approved these changes Jan 15, 2018

View reviewed changes

yasserzamani approved these changes Jan 15, 2018

View reviewed changes

lukaszlenart merged commit 9bf89f8 into apache:master Jan 15, 2018

HedjuHor deleted the BeanValidation_GroupingConstraints branch January 15, 2018 20:15

WW-4907 support JSR 303 Validation Groups in BeanValidation-Plugin #200

WW-4907 support JSR 303 Validation Groups in BeanValidation-Plugin #200

Uh oh!

Conversation

HedjuHor commented Jan 9, 2018 • edited by lukaszlenart Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaszlenart commented Jan 9, 2018

Uh oh!

lukaszlenart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HedjuHor Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HedjuHor Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HedjuHor Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HedjuHor Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

HedjuHor commented Jan 9, 2018 •

edited by lukaszlenart

Loading

coveralls commented Jan 9, 2018 •

edited

Loading

HedjuHor Jan 10, 2018 •

edited

Loading

HedjuHor Jan 12, 2018 •

edited

Loading

HedjuHor Jan 12, 2018 •

edited

Loading

HedjuHor Jan 12, 2018 •

edited

Loading

coveralls commented Jan 11, 2018 •

edited

Loading

coveralls commented Jan 11, 2018 •

edited

Loading

coveralls commented Jan 12, 2018 •

edited

Loading

coveralls commented Jan 13, 2018 •

edited

Loading

lukaszlenart Jan 15, 2018 •

edited

Loading