-
Notifications
You must be signed in to change notification settings - Fork 3k
Use Arc features in Hibernate extensions for eager startup and active/inactive #48783
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
/cc @gsmet (hibernate-orm) |
🎊 PR Preview 28074c2 has been successfully built and deployed to https://quarkus-pr-main-48783-preview.surge.sh/version/main/guides/
|
36159d4
to
ee161f6
Compare
328bc46
to
ef96914
Compare
Hey @Sanne I remember discussing this with you: silently deactivating the PU when the datasource has no URL and proceeding with startup if the PU beans (session, session factory, ...) are not injected anywhere. I remember you had some conceerns, probably because this would be a very breaking change and/or simply inconvenient behavior. Well, if that was your opinion, I'm starting to think like you. For example in the Panache case, where the session might not even be injected anywhere, this would be pretty bad. So, I'm rolling this back a bit: I'll only silently deactivate the PU if there are no entity types in the PU (which we intend to make possible through #48732). In that case it's likely that Hibernate ORM is being used as a simple wrapper of JDBC with more convenient querying capabilities (with projections, ...), so I think it makes more sense. This means in the vast majority of cases we'll get pretty much the same behavior as today, so it's much safer, too. |
9ff633f
to
f6be893
Compare
This comment has been minimized.
This comment has been minimized.
Hey @marko-bekhta or @lucamolteni can I please ask one of you review this? If you skip the first commit it's almost a reasonable amount of code (and the vast majority is just tests that look very similar). EDIT: No need for both of you to review of course :) |
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.
Thanks! Looks nice 😃 🎉
I had some questions, but feel free to ignore those 😄
...st/java/io/quarkus/hibernate/orm/envers/config/ConfigOrmActiveFalseAndAuditedEntityTest.java
Outdated
Show resolved
Hide resolved
...te/search/orm/elasticsearch/test/configuration/ConfigOrmActiveFalseAndIndexedEntityTest.java
Outdated
Show resolved
Hide resolved
...arkus/hibernate/reactive/config/datasource/MultiplePUAsAlternativesWithBeanProducerTest.java
Show resolved
Hide resolved
...m/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmCdiProcessor.java
Show resolved
Hide resolved
...ment/src/test/java/io/quarkus/hibernate/orm/config/ConfigActiveFalseStaticInjectionTest.java
Outdated
Show resolved
Hide resolved
...ve/deployment/src/test/java/io/quarkus/hibernate/reactive/config/NoConfigNoEntitiesTest.java
Show resolved
Hide resolved
|
||
if (isDefaultPersistenceUnit) { | ||
if (PersistenceUnitUtil.isDefaultPersistenceUnit(persistenceUnitName)) { | ||
configurator.addQualifier(Default.class); | ||
} |
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.
for Search we always add the named qualifiers (just below) and for ORM those seem to be conditional, I wonder if we should be consistent 🤔
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 would like to, and if so I'd change what we do in ORM, but that seems out of scope. Want to create an issue?
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 addressed your comments @marko-bekhta , thank you!
* Dynamic retrieval of the persistence unit, such as through `CDI.getBeanContainer()`, `Arc.instance()`, or by injecting an `Instance<Session>`, causes an exception to be thrown. | ||
* Other Quarkus extensions that consume the persistence unit may cause application startup to fail. | ||
+ | ||
In this case, you must also deactivate those other extensions. |
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 added a small bit of documentation. Should be enough for now?
...bernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/HibernateOrmRecorder.java
Show resolved
Hide resolved
...m/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmCdiProcessor.java
Show resolved
Hide resolved
...ment/src/test/java/io/quarkus/hibernate/orm/config/ConfigActiveFalseStaticInjectionTest.java
Outdated
Show resolved
Hide resolved
|
||
if (isDefaultPersistenceUnit) { | ||
if (PersistenceUnitUtil.isDefaultPersistenceUnit(persistenceUnitName)) { | ||
configurator.addQualifier(Default.class); | ||
} |
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 would like to, and if so I'd change what we do in ORM, but that seems out of scope. Want to create an issue?
So that we can more easily see what the next commit changes.
…ate ORM is deactivated
…rnate Reactive too
…n is inactive Only actually attempting to retrieve the bean instance will trigger a failure. Co-Authored-By: marko-bekhta <[email protected]>
Status for workflow
|
Status for workflow
|
We're good to go, I just need an approval from someone with push access... @Sanne would you please do that? @marko-bekhta reviewed already and (I think) is happy :) |
Impressive changes thank you |
Draft for now.
To do:
Handle Hibernate Validator (remove=> Bad idea, this would be complex to implement, and also to use (if entity types are used outside of PUs, why should the PU active/inactive status matter?JPATraversableResolver
if ORM deactivated?)Migration guide entry: