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

Hibernate reactive panache - validation of interceptor bindings #31846

Merged
merged 1 commit into from
Mar 15, 2023
Merged

Hibernate reactive panache - validation of interceptor bindings #31846

merged 1 commit into from
Mar 15, 2023

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Mar 14, 2023

  • the build only fails if any of [WithSession, WithTransaction, WithSessionOnDemand] is declared on a method that does not return Uni
  • if declared on a class then the methods that do not return Uni are effectively ignored

@mkouba
Hibernate reactive panache - validation of interceptor bindings
2239362 
- the build only fails if any of [WithSession, WithTransaction,
WithSessionOnDemand] is declared on a method that does not return Uni
- if declared on a class then the methods that do not return Uni are
effectively ignored
@mkouba
Copy link
Contributor Author

mkouba commented Mar 14, 2023

I believe that the new behavior makes more sense and is more practical in overall.

@mkouba mkouba added the area/hibernate-reactive Hibernate Reactive label Mar 14, 2023
@mkouba mkouba requested review from FroMage and DavideD March 14, 2023 20:10
@quarkus-bot

This comment has been minimized.

Sign in to view
DavideD
DavideD reviewed Mar 15, 2023
View reviewed changes
...e-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/WithSession.java
* A method annotated with this annotation must return either {@link io.smallrye.mutiny.Uni}. If declared on a class then all
* methods that are intercepted must return {@link io.smallrye.mutiny.Uni}.
* A method annotated with this annotation must return {@link io.smallrye.mutiny.Uni}. If declared on a class then all methods
* that return {@link io.smallrye.mutiny.Uni} are considered; all other methods are ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want this? I guess it's an inconvenience to have to annotate multiple methods if necessary, but isn't it better than having a method ignored because the user forgot that it must return a Uni?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it depends. I agree that the current behavior is a bit more correct.

But sometimes you need transaction/session for 80% of the methods but not for few package-private or protected methods, usually some utils inherited from a superclass. And in this case it's very inconvenient to annotate each method.

Copy link
Contributor

Choose a reason for hiding this comment

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

To me is the difference between adding a couple of annotations and having to debug an issue that's hard to spot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, I don't have a strong opinion, we could also add a property in the annotation to enable/disable strict validation

FroMage
FroMage approved these changes Mar 15, 2023
View reviewed changes
Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

That seems useful to me. +1

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 15, 2023

Failing Jobs - Building 2239362

Status Name Step Failures Logs Raw logs
✔️ Gradle Tests - JDK 11
⌛ Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.AnnotationProcessorSimpleModuleDevModeTest.main line 13 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

✖ io.quarkus.gradle.devmode.InjectQuarkusAppPropertiesDevModeTest.main line 19 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

✖ io.quarkus.gradle.devmode.MultiSourceProjectDevModeTest.main line 22 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

@mkouba mkouba merged commit 0a503ff into quarkusio:main Mar 15, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavideD DavideD DavideD approved these changes

@FroMage FroMage FroMage approved these changes

Assignees
No one assigned
Labels
area/hibernate-reactive Hibernate Reactive area/panache
Projects
None yet
Milestone
3.0.0.Beta1
Development

Successfully merging this pull request may close these issues.
3 participants
@mkouba @FroMage @DavideD