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 support in reactive routes #11864

Merged
merged 2 commits into from
Sep 8, 2020
Merged

Bean Validation support in reactive routes #11864

merged 2 commits into from
Sep 8, 2020

Conversation

cescoffier
Copy link
Member

Implement the support of bean validation with reactive routes:

  • parameters are checked
  • synchronous returns objects, and Uni<X> are validated - only @Valid is supported in this case

Multis are not supported, as the use case is not clear.

Ths PR also fixes the Quarkus Error Handler. It was not enforcing the status code that could have been configured in the context.

@cescoffier cescoffier mentioned this pull request Sep 3, 2020
Closed
@cescoffier cescoffier requested review from mkouba and gsmet September 3, 2020 17:23
gsmet
gsmet requested changes Sep 4, 2020
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I will work this afternoon on publishing the list of detected constraints.

mkouba
mkouba requested changes Sep 4, 2020
View reviewed changes
Copy link
Contributor

@mkouba mkouba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This PR contains a lot of unncessary formatting changes which makes it harder to review :-(

But overall it looks good and is definitely a useful addition!

@cescoffier cescoffier force-pushed the features/reactive-routes-bean-validation branch from 2d8d028 to a3841d4 Compare September 4, 2020 12:11
cescoffier
cescoffier commented Sep 4, 2020
View reviewed changes
...ns/vertx-web/deployment/src/main/java/io/quarkus/vertx/web/deployment/VertxWebProcessor.java
if (desc.isProducedResponseValidated()) {
// If the produced item needs to be validated, we inject the Validator
validatorField = invokerCreator.getFieldCreator("validator", Methods.VALIDATION_VALIDATOR)
.setModifiers(ACC_PUBLIC | ACC_FINAL);
Copy link
Member Author

Choose a reason for hiding this comment

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

The field is accessed from another class (a callback). Made it public final after discussion with @mkouba

mkouba
mkouba approved these changes Sep 4, 2020
View reviewed changes
Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

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

Looks good but we should probably wait for the new build item from @gsmet and then merge ;-).

@gsmet
Copy link
Member

gsmet commented Sep 4, 2020

@cescoffier I merged the BuildItem. Can you rebase and make use of it?

@cescoffier
The Quarkus Error Handler was not enforcing the status code that coul…
3ded180 
…d have been configured in the context.

When the application uses routingContext.fail(xxx, exception) the xxx (status) was ignored. This commits uses that status if configured.
@cescoffier cescoffier force-pushed the features/reactive-routes-bean-validation branch from a3841d4 to 84af783 Compare September 5, 2020 16:37
@cescoffier
Copy link
Member Author

@gsmet done.

@mkouba
Copy link
Contributor

mkouba commented Sep 7, 2020

I merged the BuildItem. Can you rebase and make use of it?

@gsmet Is there a reason why not to use DotNames directly in the build item? DotName#equals() is optimized and we could avoid all the DotName#toString() invocations which are IMO unnecessary in most cases...

@cescoffier
Copy link
Member Author

Anything else required on this?

@gsmet
Copy link
Member

gsmet commented Sep 7, 2020

@mkouba it's mostly to be consistent with other build items. AFAIK most of our build items are using String for class names and not DotName.

gsmet
gsmet requested changes Sep 7, 2020
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

@cescoffier just 2 minor comments: the one from Martin for the typo in the doc and mine for an incorrect comment.

Apart from that, it looks good to go and a very nice addition for 1.9!

@mkouba
Copy link
Contributor

mkouba commented Sep 8, 2020

it's mostly to be consistent with other build items.

@gsmet I don't think it's very practical, esp. in this case where you first convert dot names to strings and then the consumer converts dot name to string again. But it's a very minor "snag" ;-).

AFAIK most of our build items are using String for class names and not DotName.

Those I've created usually don't ;-).

@cescoffier
Fix quarkusio#10990
e330f52 
Add support for bean validation to reactive routes
Use the newly provided Build Item to retrieve the set of supported constraints.
@cescoffier cescoffier force-pushed the features/reactive-routes-bean-validation branch from 84af783 to e330f52 Compare September 8, 2020 06:16
@cescoffier cescoffier added this to the 1.9.0 - master milestone Sep 8, 2020
@cescoffier
Copy link
Member Author

CI issue not related to this PR (MongodbPanacheMockingTest).

@mkouba mkouba requested a review from gsmet September 8, 2020 14:39
@gsmet gsmet merged commit 6b6ad01 into quarkusio:master Sep 8, 2020
@gsmet
Copy link
Member

gsmet commented Sep 8, 2020

Merged, thanks!

@cescoffier cescoffier deleted the features/reactive-routes-bean-validation branch September 8, 2020 14:45
@dtrsan dtrsan mentioned this pull request Dec 6, 2021
Merged
@yrodiere yrodiere mentioned this pull request Mar 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand left review comments

@mkouba mkouba mkouba approved these changes

@gsmet gsmet gsmet approved these changes

Assignees
No one assigned
Labels
area/documentation area/vertx release/noteworthy-feature
Projects
None yet
Milestone
1.9.0.CR1
Development

Successfully merging this pull request may close these issues.
4 participants
@cescoffier @gsmet @mkouba @geoand