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.

Sign up for GitHub

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

Jump to bottom

Bean Validation - Method validation for CDI and JAX-RS #287

Merged
merged 5 commits into from
Dec 21, 2018
Merged

Bean Validation - Method validation for CDI and JAX-RS #287

merged 5 commits into from
Dec 21, 2018

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Dec 13, 2018 •
edited
Loading

Fixes #209 .

@mkouba
Copy link
Contributor

mkouba commented Dec 14, 2018

So there are several bugs, both in arc and in your code ;-). The first problem is that org.jboss.shamrock.beanvalidation.MethodValidated is not an interceptor binding (bug in your code). As a result, ValidationInterceptor has no bindings and is associated with all beans (this is a bug in arc and I'm going to send a PR in few mins).

Then the build is stuck in ComponentsProviderGenerator.generate() because we don't support circular dependencies in arc (another bug in arc - we should at least throw an exception -> PR in few mins).

Lastly, MethodValidatedAnnotationsTransformer.transform(TransformationContext) does not finish the transformation -> see also org.jboss.protean.arc.processor.Transformation.done(). I will definitely add some javadoc to make it more clear.

To sum it up - I'll send a PR and you should rebase and fix those issues after it's merged.

@mkouba
Copy link
Contributor

mkouba commented Dec 14, 2018

@gsmet #293

@gsmet gsmet changed the title [DO NOT MERGE] CDI bean method validation CDI bean method validation Dec 19, 2018
@gsmet
Copy link
Member Author

gsmet commented Dec 19, 2018

This is now ready for review.

@gsmet gsmet added the kind/enhancement New feature or request label Dec 19, 2018
@gsmet
Copy link
Member Author

gsmet commented Dec 19, 2018

@mkouba interested in your review if you have the time.

@gsmet gsmet added this to the 0.3.0 milestone Dec 20, 2018
@gsmet gsmet changed the title CDI bean method validation Bean Validation - Method validation for CDI and JAX-RS Dec 20, 2018
mkouba
mkouba reviewed Dec 20, 2018
View reviewed changes
...g/jboss/shamrock/beanvalidation/runtime/interceptor/AbstractMethodValidationInterceptor.java Outdated
* </p>
*/
@Inject
private Validator validator;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's always better to use package-private injection fields, or public/package-private injection initializers, or constructor injection in ArC. Private fields require reflection fallback...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will fix and force-push.

@mkouba
Copy link
Contributor

mkouba commented Dec 20, 2018

Looks good.

@gsmet
Copy link
Member Author

gsmet commented Dec 20, 2018

@mkouba I fixed the visibility issue, should be good to go now.

@gsmet
Register bean metadata for elements marked with @Valid
6abeb4a 
If the element ends up having a constraint, it was already working but
if the element is not constrained for whatever reason, we need to
register its metadata.
@gsmet
Minor cleanup
dd2d378
@gsmet
Upgrade Hibernate Validator to 6.1.0.Alpha2
ae04ba9
@gsmet
Implement CDI bean method validation
eefbd6b
@gsmet
Support JAX-RS validation as an optional feature of the bean-validation
1584bce 
extension
@mkouba mkouba merged commit b8ab83e into quarkusio:master Dec 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkouba mkouba mkouba approved these changes

Assignees
No one assigned
Labels
kind/enhancement New feature or request
Projects
None yet
Milestone
0.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@gsmet @mkouba