-
Notifications
You must be signed in to change notification settings - Fork 3k
Introduce Panache 2 #50058
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?
Introduce Panache 2 #50058
Conversation
|
🎊 PR Preview 4eeb571 has been successfully built and deployed to https://quarkus-pr-main-50058-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
|
Let's progress with CI, if we're going to merge this unannounced before we finish it in the main branch. I suppose we can start with the review process. @lucamolteni this will likely conflict with #50084 (comment) since I've added support for stateless sessions to HR and it's in the same ATM, I've added So, who wants the joy of reviewing this, @yrodiere @gsmet (definitely optional, just because he was the poor soul who reviewed Panache 1 😅) @lucamolteni @mkouba @gavinking ? Docs are not finished, but they're in a good enough state to understand the extension. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@lucamolteni I'm having intermitent test failures here because I tried to collect the persistence units for a given entity and I think at the time you added support for ORM/HR mix, the default PU for ORM and HR had a different name perhaps? Now they're both |
Yes exactly |
Thanks. Hopefully this passes CI now. |
Yes:
The idea is that less common cases are longer, but every type is discoverable from
That is certainly a valid option, and I agree it looks better. On the other hand, the fact that we have these four methods in the same place means we can have them in a single interface: public interface PanacheEntityMarker {
default PanacheManagedBlockingEntity managedBlocking() { … }
default PanacheManagedReactiveEntity managedReactive() { … }
default PanacheStatelessReactiveEntity statelessReactive() { … }
default PanacheStatelessBlockingEntity statelessBlocking() { … }
}And same for repositories which all extend |
This comment has been minimized.
This comment has been minimized.
|
Continuing the discussion for the API design for switching, here are some ideas: package io.quarkus.hibernate.panache;
// That's what we have now
// Allows for abstraction
interface CurrentOption {
interface Switcher {
MB mb();
SB sb();
MR mr();
SR sr();
}
interface MB extends Switcher {}
interface SB extends Switcher {}
interface MR extends Switcher {}
interface SR extends Switcher {}
}
// What we have now plus tri-shortcuts for every subtype
// Allows for abstraction
// Allows for shortcuts such as MyEntity.reactive() or MyEntity.stateless() or MyEntity.reactive().stateless()
interface CurrentOptionWithTriShortcuts {
interface Switcher {
MB mb();
SB sb();
MR mr();
SR sr();
}
interface MB extends Switcher {
MB m();
MR r();
SB s();
}
interface SB extends Switcher {
SB b();
SR r();
MB m();
}
interface MR extends Switcher {
MR m();
MB b();
SR s();
}
interface SR extends Switcher {
SR s();
SB b();
SB m();
}
}
// Allows abstraction
// Allows blocking().stateless() but not shortcuts
interface SwitcherTwoLevels {
interface Switcher2<ManagedTarget, StatelessTarget> {
ManagedTarget managed();
StatelessTarget stateless();
}
interface Switcher {
Switcher2<MB, SB> blocking();
Switcher2<MR, SR> reactive();
}
interface MB extends Switcher {}
interface SB extends Switcher {}
interface MR extends Switcher {}
interface SR extends Switcher {}
}
// Allows abstraction
// Allows switcher().blocking().stateless()
// Allows shortcuts such as MyEntity.reactive() or MyEntity.stateless() or MyEntity.reactive().stateless()
interface SwitcherThreeLevels {
interface Switcher2<ManagedTarget, StatelessTarget> {
ManagedTarget managed();
StatelessTarget stateless();
}
interface Switcher1 {
Switcher2<MB, SB> blocking();
Switcher2<MR, SR> reactive();
}
interface Switcher {
Switcher1 switcher();
}
interface MB extends Switcher {
MB m();
MR r();
SB s();
}
interface SB extends Switcher {
SB b();
SR r();
MB m();
}
interface MR extends Switcher {
MR m();
MB b();
SR s();
}
interface SR extends Switcher {
SR s();
SB b();
SB m();
}
} |
This comment has been minimized.
This comment has been minimized.
|
Rebased on the latest PR merged by @lucamolteni with support for multiple PUs in HR. |
This comment has been minimized.
This comment has been minimized.
| /** | ||
| * Represents an entity with stateless blocking operations. | ||
| */ | ||
| public interface Stateless extends PanacheStatelessBlockingEntity { |
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 think we need an alternative to this name. It is really confusing as it makes it seem like the entity is stateless, which it obviously isn't.
It also comes down to future-proofing the name, as StatelessSession is being added to Jakarta Persistence, with a different name: jakartaee/persistence#675
Christian and I are in favor of EntityStore for the Jakarta Persistence equivalent of StatelessSession. Following that naming, we could have:
Session(Hibernate) /EntityManager(Jakarta Persistence) /Managed(entity) /Managed(repository)StatelessSession(Hibernate) /EntityStore(Jakarta Persistence) /Stored(entity) /Store(?) (repository)
So an entity could be "managed" or just "stored".
I concede it's not great, but I would argue it's still an improvement?
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 don't really care about the name, I just went with Managed vs Stateless because that's what ORM called it.
I really dislike Store and all related because it just doesn't evoke anything to me. But then perhaps Stateless also didn't at first, and I should have gone with Unmanaged.
Perhaps we're looking at it the wrong way, and it should be entity vs active record?
Entity,EntityRepository,EntitySessionActiveRecord,ActiveRecordRepository,ActiveRecordSession
Or just Record if we want short?
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 don't really care about the name, I just went with Managed vs Stateless because that's what ORM called it.
Note ORM doesn't call entities anything, they're just entities. The session is statelesss. If we really wanted to fit that we'd need to go with something like StatelesslyHandled or something like that -- very ugly.
I really dislike Store and all related because it just doesn't evoke anything to me. But then perhaps Stateless also didn't at first, and I should have gone with Unmanaged.
I'd really hate going with something that strays too far from what Jakarta Persistence will use... but I guess Unmanaged wouldn't be the end of the world.
Perhaps we're looking at it the wrong way, and it should be entity vs active record?
-1 for ActiveRecord since I'm not convinced what we're doing can still be called active record...
Definitely -1 for Record as it'll get confused with both DB records (which this isn't, not really) and Java records (which this definitely isn't).
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.
Well, I don't really care how Jakarta Persistence calls sessions, because Panache doesn't define any session. It has entities and repositories.
So, perhaps it's Managed vs Detached? Though again, that's about entities, not repositories.
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.
Naming is hard, otherwise @gavinking would not have come up with something so weird as EntityAgent 😅
I mean, I don't want to lose 10% of my entity every time I made a transaction with its agent 🤣
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 mean the distinction between your two types of repositories is the same as the distinction between the two types of sessions: the access pattern.
You can "not care" about it, but you'll still get people who need to use both sessions and repositories (because no, repositories are not enough to do everything). And there having two naming schemes (Hibernate + Jakarta Persistence) will already be pretty confusing, so I don't think adding a third one will help anyone.
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 mean, I don't care because I can't name them, they already have names: Session and StatelessSession, but that has no effect on the layers that I do expose, which are repositories and entities.
Also, I'm either not following or agreeing with your distinction between the two types of sessions. One (managed) contains a database session, which is not unlike a transaction, really, in that it binds a set of local operations over a database that at some point are sent to the database. And it does that by tracking a number of Java instances and replicating their changes over to the DB by translating changes into SQL.
The other one (stateless) has no tracking, and does not observe entity changes. IMO it's just a dumb translator of operations from one higher level to SQL. I don't even see how it's a session, but probably that's because of its internals.
In any case, those two concepts are entirely orthogonal to types of repositories or entities.
Repositories have the exact same set of operations and don't give a care if the entity is automatically tracked or not. I mean, besides flush and update. Same for entity operations.
And as far as entities are concerned, the only difference is that one is tracked and the other is manual.
Interestingly, Jakarta Data decided to make blocking stateless entities the default, and Repository here applies only to that.
Perhaps we could pick the same default for PanacheEntity instead of "managed blocking" and go for:
PanacheEntity(stateless),TrackedEntity(managed)PanacheRepository(stateless),TrackingRepository(managed)
Tracked, Managed, Auto, AutoSync… they all describe the underlying mechanism, where Stateless (dumb) actually just refers to the absence of that mechanism.
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 won't continue the conversation on the fundamental differences between Session and StatelessSession, because I know Gavin tried to argue about that before, and if he couldn't convince you, I certainly can't :)
Perhaps we could pick the same default for
PanacheEntityinstead of "managed blocking" and go for:* `PanacheEntity` (stateless), `TrackedEntity` (managed) * `PanacheRepository` (stateless), `TrackingRepository` (managed)Tracked, Managed, Auto, AutoSync… they all describe the underlying mechanism, where Stateless (dumb) actually just refers to the absence of that mechanism.
Bikeshedding aside (and there will be bikeshedding), defaulting to StatelessSession is definitely an idea. We know it doesn't perform very well in many cases, but we also know it's way simpler and the best choice for straightforward applications, e.g. demos or CRUD applications. We also know it provides fewer ways of shooting yourself in the foot, in particular when using Reactive.
I am concerned about migration though, because people coming from Panache 1 will experience big behavior changes in any endpoint that is not a single DB operation.
WDYT @gavinking? Any opinion on promoting StatelessSession as the first thing people should try?
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 won't continue the conversation on the fundamental differences between Session and StatelessSession, because I know Gavin tried to argue about that before, and if he couldn't convince you, I certainly can't :)
I don't think we disagree on the differences between them (one tracks, one does not). I think we disagree that users care about having a different name for persist/insert and remove/delete. I have even contemplated using the same names in Panache Next to sidestep the problem, but I think I haven't, though I remain on the fence.
Perhaps it comes from the user's point of view. An ORM implementor (you and Gavin) already knows everything about JPA and managed entities and expects persist/remove and auto-tracking. A proficient user will think they understand JPA and managed entities (but won't, really), and will still expect persist/remove and auto-tracking.
A person new to Hibernate/JPA will mostly not be familiar with managed entities and will never guess what persist/remove do, or even find them. They expect insert/delete. Perhaps that's why Jakarta Data start focused on that.
This is not very important, though IMO, besides those aspects:
- What are we selling first (managed or not)
- Who are our main users (for Panache, it was definitely NOT Hibernate experts)
| * Interface representing an entity query, which abstracts the use of paging, getting the number of results, and | ||
| * operating on {@link List} or {@link Stream}. |
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.
Does PanacheQuery still bring a lot of benefits now that Hibernate ORM itself (and Jakarta Data, and...) supports paging, restrictions, and more?
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.
Not necessarily, but this was meant as an easier upgrade path because there's otherwise no common type for queries across ORM/HR and SelectionQuery has a lot of baggage and complicated APIs we may want to hide, and also projections are handled differently.
We could remove it, but that'd be a BIG change that would delay things further.
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, makes sense. I take it its use is optional then? People can just use Hibernate ORM queries if they want.
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.
Well, not with the untyped APIs, no. But with @Find and @Query sure.
| String value() default DEFAULT_PERSISTENCE_UNIT_NAME; | ||
|
|
||
| /** | ||
| * WARNING: this is temporary, it will be removed in the future. |
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.
Not sure the warning is useful unless there is already a better alternative, in which case @deprecated would be more fitting.
If you mean you want to remove this and move to @Transactional before we advertise Panache 2, maybe a TODO/FIXME?
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.
Well, no, I can't remove this otherwise nobody can use stateless sessions with this. But I could make it deprecated, even though we could say the same about value we just added and the entire annotation, really 🤷
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.
Can we please avoid hibernate-panache? It should be hibernate-orm-panache, or hibernate-orm-panache2, or... But hibernate-panache just dismisses other Hibernate project :(
By the way... What's your stance on simply quarkus-data-hibernate-orm?
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.
Well this is ORM + HR, so I went with Hibernate with Panache because I can't very well say hibernate-orm-and-hr-panache.
But otherwise you're right that it dismisses other projects… a bit like Apache, to me and a lot of people is Apache Httpd and not the foundation. To most people (outside the Hibernate team), Hibernate is ORM.
quarkus-data-hibernate-orm is pretty useless IMO, compared to quarkus-data, I don't see the point in mentioning Hibernate ORM there 🤷
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.
quarkus-data-hibernate-orm is pretty useless IMO, compared to quarkus-data, I don't see the point in mentioning Hibernate ORM there 🤷
Mainly to leave room for a non-Hibernate ORM implementation if it ever becomes necessary.
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.
Oh really? Damn, I hope not.
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.
Same, but you know: hope for the best, plan for the worst.
|
CI failure is mine, I guess I merged something wrong. Also, for some reason, some methods accept a |
|
Yeah, hopefully I fixed the merge mistakes now. |
This comment has been minimized.
This comment has been minimized.
|
Note that I've added some checks in HR/Panache that you can't currently open two sessions for the same PU at the same time. So that's currently different to ORM where I think you currently can. But we have an issue open about this: #50170 |
This comment has been minimized.
This comment has been minimized.
Not needed for @WithLazySession as this works for both types of sessions
This allows supporting ORM and HR at the same time
…ocking, one reactive)
Status for workflow
|
Status for workflow
|
| Status | Name | Step | Failures | Logs | Raw logs | Build scan |
|---|---|---|---|---|---|---|
| ✔️ | JVM Integration Tests - JDK 17 | Logs | Raw logs | 🚧 | ||
| ❌ | JVM Integration Tests - JDK 17 Windows | Build |
Failures | Logs | Raw logs | 🚧 |
| ✔️ | JVM Integration Tests - JDK 21 | Logs | Raw logs | 🚧 | ||
| ✔️ | JVM Integration Tests - JDK 25 | Logs | Raw logs | 🚧 |
Full information is available in the Build summary check run.
You can consult the Develocity build scans.
Failures
⚙️ JVM Integration Tests - JDK 17 Windows #
- Failing: integration-tests/jfr-reactive
📦 integration-tests/jfr-reactive
❌ io.quarkus.jfr.it.JfrTest. - History - More details - Source on GitHub
org.junit.jupiter.engine.execution.ConditionEvaluationException: Failed to evaluate condition [io.quarkus.test.junit.QuarkusTestExtension]: Internal error: Test class was loaded with an unexpected classloader (QuarkusClassLoader:Quarkus Base Runtime ClassLoader: TEST for JUnitQuarkusTest-no-profile (QuarkusTest)@6e0d16a4) or the thread context classloader (jdk.internal.loader.ClassLoaders$AppClassLoader@c387f44) was incorrect.
at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179)
at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179)
at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
at java.base/java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1602)
at java.base/java.util.stream.Referenc...
❌ io.quarkus.jfr.it.JfrTest.blockingTest line 56 - History - More details - Source on GitHub
java.lang.AssertionError:
1 expectation failed.
JSON path start.resourceClass doesn't match.
Expected: is "io.quarkus.jfr.it.AppResource"
Actual: null
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
Flaky tests - Develocity
⚙️ JVM Tests - JDK 25
📦 test-framework/jacoco/runtime
❌ io.quarkus.jacoco.runtime.DataFileWatchTest.waitForPreexistingDataFileThatNeverChanges - History
Expecting value to be false but was true-org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError:
Expecting value to be false but was true
at io.quarkus.jacoco.runtime.DataFileWatchTest.waitForPreexistingDataFileThatNeverChanges(DataFileWatchTest.java:241)
at java.base/java.lang.reflect.Method.invoke(Method.java:565)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1604)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1604)
⚙️ JVM Tests - JDK 17 Windows
📦 extensions/micrometer-opentelemetry/deployment
❌ io.quarkus.micrometer.opentelemetry.deployment.compatibility.MicrometerTimedInterceptorTest.testTimeMethod_AsyncFailed - History
Stream has no elements-java.lang.IllegalArgumentException
java.lang.IllegalArgumentException: Stream has no elements
at io.quarkus.micrometer.opentelemetry.deployment.common.MetricDataFilter.lambda$lastReading$2(MetricDataFilter.java:213)
at java.base/java.util.Optional.orElseThrow(Optional.java:403)
at io.quarkus.micrometer.opentelemetry.deployment.common.MetricDataFilter.lastReading(MetricDataFilter.java:213)
at io.quarkus.micrometer.opentelemetry.deployment.common.MetricDataFilter.lastReadingDataPoint(MetricDataFilter.java:231)
at io.quarkus.micrometer.opentelemetry.deployment.compatibility.MicrometerTimedInterceptorTest.testTimeMethod_AsyncFailed(MicrometerTimedInterceptorTest.java:150)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:534)
⚙️ MicroProfile TCKs Tests
📦 tcks/microprofile-lra
❌ org.eclipse.microprofile.lra.tck.TckRecoveryTests.testCancelWhenParticipantIsUnavailable - History
Expecting the metric Compensated callback was called Expected: a value equal to or greater than <1> but: <0> was less than <1>-java.lang.AssertionError
java.lang.AssertionError:
Expecting the metric Compensated callback was called
Expected: a value equal to or greater than <1>
but: <0> was less than <1>
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.eclipse.microprofile.lra.tck.TckRecoveryTests.assertMetricCallbackCalled(TckRecoveryTests.java:210)
at org.eclipse.microprofile.lra.tck.TckRecoveryTests.testCancelWhenParticipantIsUnavailable(TckRecoveryTests.java:195)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

Uh oh!
There was an error while loading. Please reload this page.