Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validator doesn't inject CDI bean unless @ApplicationScoped #6094

Closed
sdaschner opened this issue Dec 11, 2019 · 7 comments · Fixed by #11598
Closed

Validator doesn't inject CDI bean unless @ApplicationScoped #6094

sdaschner opened this issue Dec 11, 2019 · 7 comments · Fixed by #11598
Labels
area/hibernate-validator Hibernate Validator good first issue Good for newcomers
Milestone

Comments

@sdaschner
Copy link
Contributor

sdaschner commented Dec 11, 2019

Describe the bug
A bean validator doesn't @Inject an application-scoped CDI bean if the validator itself is not @ApplicationScoped.
While it's described on https://quarkus.io/guides/validation that
validators are recommended to be application-scoped it doesn't read that
it's a requirement. There is no error, the bean is simply null.

Expected behavior
Same behavior as in stock EE 8 runtime with JAX-RS, Bean Validation & CDI: injection works without making the validator application-scoped.

While it's helpful that the documentation points out the scope, it would help developers who have experience in plain Enterprise Java to migrate to Quarkus.

To Reproduce
Steps to reproduce the behavior:

  1. Code a bean validator:
public class OrderValidator implements ConstraintValidator<ValidOrder, JsonObject> {

  @Inject
  Shop shop;
  ...

  public boolean isValid(JsonObject json, ConstraintValidatorContext context) {
    shop. ...
  }
}
  1. Integrate the validator in JAX-RS (here via @ValidOrder).
  2. Notice that isValid() is invoked, but shop is null which results in a NPE.
  3. Annotating OrderValidator with @ApplicationScoped resolves the issue.

Environment (please complete the following information):

  • Output of uname -a or ver: Linux archlinux 5.3.11-arch1-1 #1 SMP PREEMPT Tue, 12 Nov 2019 22:19:48 +0000 x86_64 GNU/Linux
  • Output of java -version: openjdk version "13.0.1" 2019-10-15 \n OpenJDK Runtime Environment AdoptOpenJDK (build 13.0.1+9) \n Eclipse OpenJ9 VM AdoptOpenJDK (build openj9-0.17.0, JRE 13 Linux amd64-64-Bit Compressed References 20191021_96 (JIT enabled, AOT enabled)
  • Quarkus version or git rev: 1.0.1.Final
@geoand geoand added the area/hibernate-validator Hibernate Validator label Dec 12, 2019
@geoand
Copy link
Contributor

geoand commented Dec 12, 2019

@gsmet maybe we can implicitly add make validator an application scoped bean implicitly by just detecting the implementations of ConstraintValidator?

@gsmet
Copy link
Member

gsmet commented Dec 16, 2019

I don't know. I did that at first when playing with HV + CDI and decided against it to avoid creating unnecessary beans as CV can also just be very simple classes with no injection.

I wasn't sure making them all beans was a good solution.

But there's one thing for sure, you need at least one scope annotation to make it a bean. Isn't it something expected in CDI?

@sdaschner
Copy link
Contributor Author

@gsmet I agree on avoiding the creation of unnecessary beans. Maybe that could be done as analysis during build time (whether things like injection are actually required).

However, in order to @Inject you don't need any scope annotation, at least not what works now with Hibernate Validator: https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#_dependency_injection

Also this is a common pattern that I see in projects, so it would definitely help migrating code to Quarkus (especially since there is no error, the injection point is simply null).

@sdaschner
Copy link
Contributor Author

Any updates on this? Happy to help, btw.

@gsmet
Copy link
Member

gsmet commented Aug 12, 2020

Well, it's normal the injection point is null. Your CV is just not a bean.

If you want to give it a shot, we would need something similar to: https://github.com/quarkusio/quarkus/blob/master/extensions/resteasy-server-common/deployment/src/main/java/io/quarkus/resteasy/server/common/deployment/ResteasyServerCommonProcessor.java#L335 in the Hibernate Validator extension to add a scope automatically to classes extending ConstraintValidator. I would probably also detect the lifecycle-related annotations.

@gsmet
Copy link
Member

gsmet commented Aug 13, 2020

@sdaschner once #11350 is in, could you make the same thing for ConstraintValidator and add a test?

Thanks!

@gsmet gsmet added the good first issue Good for newcomers label Aug 13, 2020
@sdaschner
Copy link
Contributor Author

@gsmet Sure, done. Happy that I could finally help with something on the build process :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/hibernate-validator Hibernate Validator good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants