-
Notifications
You must be signed in to change notification settings - Fork 3k
DRAFT Panache Support multiple persistence unit in Hibernate Reactive #50084
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered inline
...rc/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/AbstractJpaOperations.java
Outdated
Show resolved
Hide resolved
context.removeLocal(SESSION_ON_DEMAND_OPENED_KEY); | ||
return closeSession(); | ||
// TODO Luca this is just wrong | ||
return closeSession(DEFAULT_PERSISTENCE_UNIT_NAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this should close all currently opened sessions, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used in WithSessionOnDemandInterceptor
I think we could assume that the interceptor works only for the default persistence unit.
Or else we could add the Persistence Unit name to the @WithSessionOnDemand
annotation so that the the interceptor knows about it. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's debatable.
My opinion is that this says "open a session automatically when asked", and then later (this is lazy), someone opens a session, at which point we know which session and which PU.
If we only allow a single session open at all times, then this should close that session that we opened, no matter its PU.
If we allow any number of sessions for different PUs open within a @WithSessionOnDemand
then they should all be closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So wait, actually in both of the cases I mention, it would close all the currently opened sessions :)
...te-reactive-panache/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/Panache.java
Outdated
Show resolved
Hide resolved
...e-panache/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/PanacheEntityBase.java
Outdated
Show resolved
Hide resolved
public default Uni<Void> flush() { | ||
return INSTANCE.flush(); | ||
// TODO Luca not sure about this should it flush all the sessions? | ||
// TODO Luca this will crash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question :-/
Yeah, flush all the sessions.
If needed, add an overload for flushing a single PU?
There was a problem hiding this 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 why I wrote this comment as this method might flush just the specific session/pu the entity is attached and I think it makes more sense WDYT? ~~
EDIT I remembered now, this
in repository is a repository not an entity, so it can't work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the new version I flush all the PUs, please take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually… a repository targets a single entity, so it should flush that entity's PU by default, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FroMage yes but nowhere we save in the repository the type of the entity if I'm not mistaken and it gets erased at runtime. If you think we can find the type of ENTITY
somewhere can you please me point me how?
...nache/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/PanacheRepositoryBase.java
Outdated
Show resolved
Hide resolved
...-reactive-panache/src/test/java/io/quarkus/it/panache/reactive/PanacheFunctionalityTest.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/AbstractJpaOperations.java
Outdated
Show resolved
Hide resolved
08d41a4
to
577434e
Compare
* Integration test for multiple reactive persistence units and Panache Backported from ORM the handling of different persistence units in entity in Panache * `@WithSessionOnDemand` works only with the default persistence unit * withSession overload to take the PU name * Execute update without entity runs on default session * Repository flush() flushes all PUs
577434e
to
500587e
Compare
I've tackled most of the comments and the current tests are passing I still need to add a few more test for the named persistence unit overload |
🎊 PR Preview de98066 has been successfully built and deployed to https://quarkus-pr-main-50084-preview.surge.sh/version/main/guides/
|
* This method flushes *all* persistence units. | ||
* Ideally, it should flush only the session associated with the repository’s entity, | ||
* but we don’t currently track the entity inside the repository. | ||
* |
There was a problem hiding this 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 why this is the case, is it the case for the ORM version?
context.removeLocal(SESSION_ON_DEMAND_OPENED_KEY); | ||
return closeSession(); | ||
// TODO Luca this is just wrong | ||
return closeSession(DEFAULT_PERSISTENCE_UNIT_NAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So wait, actually in both of the cases I mention, it would close all the currently opened sessions :)
* @return the current {@link Mutiny.Session} | ||
*/ | ||
public default Uni<Mutiny.Session> getSession() { | ||
return INSTANCE.getSession(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so this is the missing piece of the puzzle!
In the ORM version, we have:
@GenerateBridge
default Session getSession() {
throw implementationInjectionMissing();
}
Which is how we can pin a repo to its entity's PU.
Now, if you do the same here, you can obtain the proper session for the repo, and implement flush()
exactly as ORM does:
default void flush() {
getSession().flush();
}
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly! It's what I've written in this comment
* Used by Panache repositories.
*
* This method flushes *all* persistence units.
* Ideally, it should flush only the session associated with the repository’s entity,
* but we don’t currently track the entity inside the repository.
*
* In Panache blocking, Quarkus injects the current EntityManager.
* In reactive mode, however, there is not yet an injectable Mutiny.Session
* (see https://github.com/quarkusio/quarkus/issues/47462).
So when #47462 is done, it should be easier to do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's not related to stateless sessions. This should be a change done in this PR. The same change was done for the ORM support of multiple PUs.
This is mostly a back port from the blocking version.
This reactive version shows a few problems and I wanted to discuss it with the team before going forward
Please share your thoughts about this, especially with the changes of the public API, as the synchronous
isPersistent
returns now a Uni@FroMage @yrodiere
This fixes this reproducer provided by a user