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

Don't fail Hibernate Validator when no RESTEasy Reactive request is in progress #19844

Merged
merged 1 commit into from
Sep 2, 2021
Merged

Don't fail Hibernate Validator when no RESTEasy Reactive request is in progress #19844

merged 1 commit into from
Sep 2, 2021

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Sep 1, 2021 •
edited
Loading

Relates to: #19834

@geoand geoand requested a review from gsmet September 1, 2021 16:37
@quarkus-bot

This comment has been minimized.

Sign in to view
@geoand geoand changed the title Don't fail Hibernate Validator when no RESTEasy Reactive request is i… Don't fail Hibernate Validator when no RESTEasy Reactive request is in flight Sep 1, 2021
@quarkus-bot quarkus-bot bot added the area/hibernate-validator Hibernate Validator label Sep 1, 2021
@geoand geoand mentioned this pull request Sep 1, 2021
Closed
@geoand geoand changed the title Don't fail Hibernate Validator when no RESTEasy Reactive request is in flight Don't fail Hibernate Validator when no RESTEasy Reactive request is in progress Sep 1, 2021
gsmet
gsmet reviewed Sep 1, 2021
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.

Added some questions.

...java/io/quarkus/hibernate/validator/runtime/jaxrs/ResteasyReactiveContextLocaleResolver.java
headers.getLength(); // this forces the creation of the actual object which will fail if there is no request in flight
return headers;
} catch (IllegalStateException e) {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

It's not possible to return an empty header object? I'm not sure I understand how the case of not having a request can happen and when the user will be confronted to it?

Copy link
Member

Choose a reason for hiding this comment

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

What I understand from the OP's case is that ResteasyReactiveRequestContext.getHttpHeaders() fails somehow. I'm wondering if we should return empty headers there and thus cover this case entirely.

Note that I have absolutely no knowledge of the RR codebase so I'm just thinking out loud, given the pattern above looks hackyish and fixing the issue at the source might be a more general solution.

Copy link
Contributor Author

@geoand geoand Sep 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

org.jboss.resteasy.reactive.server.injection.ContextProducers.getHttpHeaders() would indeed make sense to return an empty headers in this case. However it does not make sense for others in ContextProducers, so we need a consistent approach.

gastaldi
gastaldi reviewed Sep 1, 2021
View reviewed changes
integration-tests/pom.xml
@@ -123,6 +123,7 @@
<module>class-transformer</module>
<module>shared-library</module>
<module>hibernate-validator</module>
<module>hibernate-validator-resteasy-reactive</module>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this new module be listed in https://github.com/quarkusio/quarkus/blob/main/.github/native-tests.json?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe let's not add minutes to the native build if we don't think native will introduce issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my though exactly for not adding it

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 1, 2021

Failing Jobs - Building ee298ec

Status Name Step Failures Logs Raw logs
✖ JVM Tests - JDK 11 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 16

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/smallrye-reactive-messaging-kafka/deployment 
! Skipped: docs integration-tests/kubernetes/quarkus-standard-way-kafka integration-tests/reactive-messaging-kafka and 1 more

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingTestCase.testContinuousTestingScenario1 line 64 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <1> but was: <0>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)

@geoand
Copy link
Contributor Author

geoand commented Sep 2, 2021 •
edited
Loading

The failure is completely unrelated

gsmet
gsmet approved these changes Sep 2, 2021
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'm not sure about the pattern as that's typically the type of pattern that could bite us later but I'll trust you on that :).

@gsmet gsmet merged commit 05bebe3 into quarkusio:main Sep 2, 2021
@quarkus-bot quarkus-bot bot added this to the 2.3 - main milestone Sep 2, 2021
@geoand
Copy link
Contributor Author

geoand commented Sep 2, 2021

I'm not sure about the pattern as that's typically the type of pattern that could bite us later but I'll trust you on that :).

You know by know that I'll be the first one to say you were right if the future proves you correct :)

@geoand geoand deleted the #19834-validation branch September 2, 2021 14:30
@gsmet gsmet modified the milestones: 2.3 - main, 2.2.2.Final Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastaldi gastaldi gastaldi left review comments

@gsmet gsmet gsmet approved these changes

Assignees
No one assigned
Labels
area/hibernate-validator Hibernate Validator area/rest
Projects
None yet
Milestone
2.2.2.Final
Development

Successfully merging this pull request may close these issues.
3 participants
@geoand @gastaldi @gsmet