microtweak / conditional-validator

An extension package for Bean Validation 2.0 that adds conditional validations
MIT License
6 stars 1 forks source link

Class not found ConstraintLocation$ConstraintLocationKind when using hibernate-validator 6.0.23.Final #3

Closed Raffine64 closed 1 year ago

Raffine64 commented 1 year ago

due to HibernateValidatorConstraintDescriptorFactory, and new 6.0.x versions of hibernate-validator, there is a class not found : ConstraintLocation$ConstraintLocationKind.

The code is as follow, and doesn't take into account versions > 6.0.20 of 6.0.x versions:

if (version.gt("6.0.20")) {
            this.strategy = new Hv61XConstraintDescriptorImplStrategy();
        } else {
            if (!version.ge("6.0.6") || !version.le("6.0.20")) {
                throw new IllegalStateException("Hibernate Validator older then 6.0.6 is not supported");
            }

            this.strategy = new Hv60XConstraintDescriptorImplStrategy();
        }

usage of Hv61XConstraintDescriptorImplStrategy should be tested upon 6.1.x versions and usage of Hv60XConstraintDescriptorImplStrategy should be tested upon 6.0.x versions to avoid this kind of problems.

pseudo-code proposal:

if (version >= 6.1.x) { use 61XConstraintDescriptorImplStrategy }
else if (version >= 6.0.6) { use Hv60XConstraintDescriptorImplStrategy }
else {
throw throw new IllegalStateException("Hibernate Validator older than 6.0.6 is not supported");
}

see https://mvnrepository.com/artifact/org.hibernate.validator/hibernate-validator

Raffine64 commented 1 year ago

there is also a typo issue: Hibernate Validator older then 6.0.6 is not supported

Hibernate Validator older than 6.0.6 is not supported

salvadormarcos commented 1 year ago

Hello @Raffine64

First, thanks for using my project and for reporting this issue.

Whenever the Hibernate-Validator team makes changes to the ConstraintDescriptorImpl class there is a possibility that my project will break.

I'm looking at another approach to avoid future situations like this. Maybe have my own implementation of javax.validation.metadata.ConstraintDescriptor.

As soon as possible, I will let you know about the correction.

salvadormarcos commented 1 year ago

Hello @Raffine64

I created branch fix-class-not-found to test some project refactoring approaches. I believe I got into something solid with low dependency on internal APIs of HibernateValidator or Apache BVal.

I ran the unit tests with some versions of HV between 6.0.0 and 6.2.5 and everything worked as expected.

Right now I'm working on the interpolation of validation messages. I believe it is the last part to be corrected before releasing a new version.

In the meantime, could you download this branch locally and test that everything is still working in your environment?

Raffine64 commented 1 year ago

Hello @salvadormarcos

I tried to download the sources and clean install the project with maven, but I got a failed test:

[ERROR] Tests run: 6, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.418 s <<< FAILURE! - in com.github.microtweak.validator.conditional.internal.BeanValidationHelperTests [ERROR] findBuiltInValidatorClassByPrimitiveAndWrapperType Time elapsed: 0.011 s <<< FAILURE! org.opentest4j.AssertionFailedError: expected: <org.hibernate.validator.internal.constraintvalidators.bv.number.bound.MinValidatorForInteger> but was: <org.hibernate.validator.internal.constraintvalidators.bv.number.bound.MinValidatorForNumber> at com.github.microtweak.validator.conditional.internal.BeanValidationHelperTests.findBuiltInValidatorClassByPrimitiveAndWrapperType(BeanValidationHelperTests.java:79)

Could you fix the problem ?

salvadormarcos commented 1 year ago

This problem seems to be intermittent. I had to run the build a few times for the test to break. I will investigate further and fix this problem.

In the meantime, if you like, disable this test case and run "mvn clean install" without it. In this way, we were able to validate whether this new approach works in your environment.

salvadormarcos commented 1 year ago

Problem solved!!

The ordering of the validators was not prepared to handle primitive types. This caused the validators to be ordered incorrectly, intermittently bringing MinValidatorForNumber first than MinValidatorForInteger.

I believe that now you can run the tests.

Raffine64 commented 1 year ago

Thanks,

It passes the tests now.

May I also inform you on maven warnings you should address :

Warning 1 [WARNING] Source files encoding has not been set, using platform encoding UTF-8, i.e. build is platform dependent! => see https://stackoverflow.com/questions/3017695/how-to-configure-encoding-in-maven

Warning 2 | WARNING: | | The junit-platform-surefire-provider has been deprecated and is scheduled to | | be removed in JUnit Platform 1.4. Please use the built-in support in Maven | | Surefire >= 2.22.0 instead. | | » https://junit.org/junit5/docs/current/user-guide/#running-tests-build-maven |

Warning 3 [WARNING] Javadoc Warnings [WARNING] C:\JavaDevelopment\Workspaces\microtweak-conditional-validator-fix-class-not-found\validator-core\src\main\java\com\github\microtweak\validator\conditional\core\ConditionalValidate.java:24: warning: no @return C:\JavaDevelopment\Workspaces\microtweak-conditional-validator-fix-class-not-found\validator-core\src\main\java\com\github\microtweak\validator\conditional\core\ConditionalValidate.java:24: warning: no @return String message() default ""; ^ C:\JavaDevelopment\Workspaces\microtweak-conditional-validator-fix-class-not-found\validator-core\src\main\java\com\github\microtweak\validator\conditional\core\ConditionalValidate.java:30: warning: no @return Class<?>[] groups() default { }; ^ C:\JavaDevelopment\Workspaces\microtweak-conditional-validator-fix-class-not-found\validator-core\src\main\java\com\github\microtweak\validator\conditional\core\ConditionalValidate.java:36: warning: no @return Class<? extends Payload>[] payload() default { }; ^

Raffine64 commented 1 year ago

Hello @salvadormarcos

I've tested the new branch, but I got into problems.

I use hibernate-validator-6.0.23.Final and conditional-validator-hv deps.

but after debugging, it seems that it is unable to find the correct implementation, in my case for @Email. It says : Caused by: org.jboss.weld.exceptions.UnsatisfiedResolutionException: WELD-001334: Unsatisfied dependencies for type EmailValidator with qualifiers

what I do:

@ConditionalValidate
public class Contact {

    private String contactType;

    @EmailWhen(expression = "contactType == 'EMAIL_ADDRESS'")
    @URLWhen(expression = "contactType == 'WEBSITE_URL'", regexp = "^(http|https).*")
    private String value;

}

Do you have an idea about what the problem could be ?

salvadormarcos commented 1 year ago

Hello @Raffine64,

Are you using a CDI-based environment, correct? Apparently CDI is not getting the EmailValidator instance.

I'll make some changes to PlatformProvider and let you know when I have something to test

Raffine64 commented 1 year ago

Yes, it's a CDI-based environment.

salvadormarcos commented 1 year ago

Hello @Raffine64,

I made some changes to the PlatformProvider class. Running Weld 3.1.9.Final standalone, everything worked as expected.

Could you confirm that this change resolves the issue in your environment?

About maven's warnings: I'll fix them before releasing the final version. Thanks for linking the fixes

Raffine64 commented 1 year ago

Hello @salvadormarcos,

It seems to work as expected.

Thanks.

Raffine64 commented 1 year ago

Hello @salvadormarcos,

Do you plan to release the version ?

Thanks in advance

salvadormarcos commented 1 year ago

Hi @Raffine64,

The interpolation of validation messages is not working yet. I'm working on this issue.

As soon as I complete this part, I will release a new version.

And thanks for your feedback on changes to this branch.

salvadormarcos commented 1 year ago

Hello @Raffine64

I released version 0.0.3 on Maven Central. Could you verify that everything is working in your environment with this new version?

Raffine64 commented 1 year ago

Hello @salvadormarcos

It seems to work. Thanks !

Raffine64 commented 1 year ago

Hello @salvadormarcos

We saw a problem we had not seen when testing, regarding the keys from property bundle.

Exception message:java.util.MissingResourceException: Can't find resource for bundle java.util.PropertyResourceBundle

when trying with @Pattern for instance, we have the key. It's not the case if we use the @PatternWhen.

salvadormarcos commented 1 year ago

Hello,

This is a little weird... Both annotations use the same message {javax.validation.constraints.Pattern.message} and the project delegates message processing to the MessageInterpolator of the HV/BVal. I believe it's some particularity when converting @PatternWhen to @Pattern.

Could you send me the complete stacktrace and provide more details on how I reproduce this Exception?

Raffine64 commented 1 year ago

@salvadormarcos ,

sorry, the error message was from our JSF application.

we don't get an error in fact.

it just replaces the key definition by the message from the bundle.

we retrieve the message instead of the key when getting that from : ConstraintViolationException ConstraintViolation cve. cve.getMessageTemplate()

which should return something like {mymessages.validation.somethingNotValid}

for instance :

    @Pattern(regexp = "[0-9]{3}", message = "{validation.shortNumber}")
    private String valueNotConditional;

constraintViolation.getMessageTemplate() => {validation.shortNumber} constraintViolation.getMessage => Number must have length of maximum 3

but with :

@ConditionalValidate
public class TestObjectConditionalValidator {

    private String type;

    @PatternWhen(expression = "type == 'SHORT_NUMBER'", regexp = "[0-9]{3}", message = "{validation.shortNumber}")
    @PatternWhen(expression = "type == 'LONG_NUMBER'", regexp = "[0-9]{6}", message = "{validation.longNumber}")
    private String value;

constraintViolation.getMessageTemplate() =>Number must have length of maximum 3 constraintViolation.getMessage => Number must have length of maximum 3

Raffine64 commented 1 year ago

This problem seems to have been introduced in version 0.0.3.

I tested again against the branch, and it worked.

salvadormarcos commented 1 year ago

Hello,

This behavior occurs because I send the interpolated message to context.buildConstraintViolationWithTemplate.

Previously I passed the value of the "message" attribute of the validated constraint. In this format we would have getTemplateMessage() with {validation.shortNumber} and getMessage() with the interpolated message. The problem with this approach is that some parameters and ELs within messages will not be processed.

You can see an example of these messages in the org/hibernate/validator/ValidationMessages.properties file. The default message of javax.validation.constraints.DecimalMax.message is a good example.

Raffine64 commented 1 year ago

Hello @salvadormarcos

In my opinion, the solution should not compromise any behavior of the original validation annotation.

The fact we use an additional layer of "conditional" thing should be transparent to the annotation you would have used otherwise.

So the getTemplateMessage() cannot return the message itself, this breaks the contract. And also the parameters has to be passed along, you should not have a difference in behavior.

salvadormarcos commented 1 year ago

I agree with you, my initial idea was precisely to be a transparent layer reusing all the structure (validators, interpolation, etc.) of HV and BVal. Unfortunately the Bean Validation API doesn't offer much flexibility for us to add features beyond the specification (unless I create my own BV implementation 😁😉).

Message interpolation didn't work correctly in previous versions and I only noticed recently while testing the refactoring. I identified this behavior in HV and BVal: the templateMessage is replaced by the ResourceBundle message, however some parameters and ELs are kept in the final message.

The use of @DecimalMaxWhen I mentioned earlier returns me: must be less than ${inclusive == true ? 'or equal to ' : ''}{value}. This behavior already makes the use of several constraints unfeasible.

Using the HibernateConstraintValidatorContext interface (example) I can add these parameters to the message, but the EL still doesn't work. For BVal I couldn't find anything similar.

The least problematic solution was to send the already interpolated message to the context.buildConstraintViolationWithTemplate. Unfortunately it breaks the contract of getTemplateMessage(), but everything else still works.

salvadormarcos commented 1 year ago

By the way, if you know any other approach, we can add it to the project.

salvadormarcos commented 1 year ago

Hello @Raffine64,

Can I close this issue?

Raffine64 commented 1 year ago

Hello @salvadormarcos,

yes.