HV-864 - Cross-parameter constraint is disallowed by Annotation Processor

[nicolaferraro]

I've analyzed the Jira issue HV-864 and found out that cross-parameter constraints were not supported by hibernate annotation processor. In the example provided in Jira, the processor was trying to compare Object or Object[] to the return type of the method, not to the parameters.

I've implemented the support for cross-parameter constraints, added most of the checks and related test cases.

[hferentschik]

Hi @nibbio84, great that you want to help out. We have been talking about getting the annotation processor up to date, but so far we just could not find the time to do so. We were always hoping that someone from the community would step up to the task. Seems it has happened now :-)

I have not looked closer at your changes yet, but will do so asap. A couple of things to start with though.

1. For us to accept your contributions you need to "sign" the JBoss Contributor License Agreement (CLA). All you need it is to sign up for a free JBoss user account and then select Hibernate Validator from the list of JBoss projects in the CLA.

2. As it stands your code does not compile (see also CI job). I had a quick look at the problem and it seems you are now depending on Bean Validation API classes (eg javax.validation.constraintvalidation.ValidationTarget), but the pom.xml does not include the Bean Validation API. Adding

   <dependency>
       <groupId>javax.validation</groupId>
       <artifactId>validation-api</artifactId>
   </dependency>
   

gets the code to compile, but then some checkstyle violations fail the build.

Regarding the latter, in case you are using Idea, you can find a code formatting template here. If you are using Eclipse or any other IDE, most violations seems to be braces placement and missing/wrong license headers. It should be quite easy to get your IDE to do apply the right style.

Regarding the missing dependency, I guess it is reasonable to expect that the Bean Validation API is available, but so far the annotation processor is a dependency free jar. There are several advantages to this. The code is not tied to a specific Bean Validation API version and the setup of the annotation processor for the user is simpler, since only a single jar needs to be added to the processor classpath. I think it is possible to add the new functionality without a hard dependency to the BV API. You could just do string based checks for example. However, I am open for a discussion on whether the BV API is really needed.

Hope this make sense. Let us know what you think.

[nicolaferraro]

Thank you for the explanation, I didn't realize that the dependency on the validation-api was only with test scope (I'm using Eclipse, the problem appears only when compiling off-ide). I completely agree on the advantages of having a dependency-free jar.

Hope that Jenkins will compile the updated version.

[hferentschik]

not a big fan of variable names like oneXYZ. I think we avoided them as much as possible on the actual engine code base, but in the annotation processor we still might have more of these names left. Just saying ...

[gunnarmorling]

Yes, to be fair, it's really a left-over from my old style :)

[hferentschik]

Thank you for the explanation,

no worries

I didn't realize that the dependency on the validation-api was only with test scope (I'm using Eclipse, the problem appears only when compiling off-ide).

I guess it depends on the IDE. Idea complained as well. Either way, the reference is always the command line build. I don't care much what IDE one uses or through which hoops one has to jump to set the project up. In the bottom line, the build should be optimized for the command line. I am for example not very fond of all the Eclipse Maven metadata added to the main pom.xml. IMO it has nothing to do there. It's really a trade off between clean build and some more comfort for the main stream IDEs Idea and Eclipse.

I completely agree on the advantages of having a dependency-free jar.

Cool

Hope that Jenkins will compile the updated version.

It does and it works also locally for me now. Thanks for changing the style issues, it makes the patch much easier to review as well, since a lot of your previous changes were caused by re-formatting.

Overall this looks quite good to me now, but I let @gunnarmorling be the final judge, since the annotation processor was his initial baby.

[hferentschik]

Does this actually also take care of HV-801?

[nicolaferraro]

No, it doesn't.
I can take care of it. Shall I do it in current pull request?

[gunnarmorling]

I can take care of it. Shall I do it in current pull request?

No, if you are interested in tackling that one, too, I recommend to create a follow-up PR. We try to stick to a "one PR per feature" rule which eases reviewing, rebasing due to other concurrent changes etc.

[gunnarmorling]

Great commenting!

[gunnarmorling]

Ah, only the "li" tags should be closed :)

[gunnarmorling]

@nibbio84, added some more comments, mostly minor stuff, great work overall! Thanks a lot for this contribution, it's much appreciated!

[nicolaferraro]

Hi, thanks for the suggestion about the trailing whitespace, I didn't know how to solve it automatically :)

The "#" in properties files works on my mac, anyway, I replaced it with the unicode character. I've removed some redundant code and moved some strings to BeanValidationTypes.

[hferentschik]

I suggest to squash the commits into a single one making sure its message starts with the issue key. Right now some of the commits don't do that and all these follow up commits don't really add new features. It's all about the support of method constraints in the annotation processor.

[nicolaferraro]

Everything squashed.

[gunnarmorling]

Rebased and applied. Great work, @nibbio84, thanks a lot!