-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add .enabled/.active configuration properties to Hibernate ORM, Reactive and Envers extensions #27134
Conversation
So that an application using only programmatic bean retrieval will work correctly. This is useful for our own tests, but I think it makes sense in general too, and that's how Hibernate ORM's SessionFactory/Session beans are defined too.
…onScoped For the same reasons as Hibernate ORM: 1. So that it can be mocked 2. More importantly, so that it is instantiated on first use (i.e. at runtime) and so that we can throw an exception when the persistence unit was deactivated through configuration properties.
… build We'll need these at runtime in the next few commits.
…/.active on ORM's
Map<String, LazyPersistenceUnit> persistenceUnitsBuilder = new HashMap<>(); | ||
for (String persistenceUnitName : jpaConfigSupport.persistenceUnitNames) { | ||
persistenceUnitsBuilder.put(persistenceUnitName, new LazyPersistenceUnit(persistenceUnitName)); | ||
for (PersistenceUnitDescriptor descriptor : PersistenceUnitsHolder.getPersistenceUnitDescriptors()) { |
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.
We are sure that this will always be set properly? IIRC we ended up with injection because we had some race conditions. But it might have been with an old pattern and things might have changed.
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 know about that race condition and I don't see any failing test, so I can't say...
What I can say:
JPAConfigSupport
was introduced to "make JPAConfig less brittle"; not sure if it was about an actual bug or design concerns (best practices).JPAConfigSupport
initially contained more information than just persistence unit names; that is no longer the case.PersistenceUnitsHolder
is initialized at static init:Lines 73 to 79 in aec43cd
return new BeanContainerListener() { @Override public void created(BeanContainer beanContainer) { PersistenceUnitsHolder.initializeJpa(parsedPersistenceXmlDescriptors, scanner, additionalIntegrators, proxyDefinitions); } };
JPAConfig
is supposedly only used at runtime; that makes sense since session factories are a runtime construct. Here are the only uses I found:Lines 24 to 29 in bc4955b
/* RUNTIME_INIT for metrics */ public Consumer<MetricsFactory> consumeMetricsFactory() { return new Consumer<MetricsFactory>() { @Override public void accept(MetricsFactory metricsFactory) { JPAConfig jpaConfig = Arc.container().instance(JPAConfig.class).get(); Lines 98 to 104 in e9b4dce
public Supplier<SessionFactory> sessionFactorySupplier(String persistenceUnitName) { return new Supplier<SessionFactory>() { @Override public SessionFactory get() { SessionFactory sessionFactory = Arc.container().instance(JPAConfig.class).get() .getEntityManagerFactory(persistenceUnitName) .unwrap(SessionFactory.class); Lines 103 to 105 in aec43cd
public void startAllPersistenceUnits(BeanContainer beanContainer) { beanContainer.instance(JPAConfig.class).startAll(); } Lines 18 to 22 in 1aec4e3
@ApplicationScoped public class TransactionSessions { @Inject JPAConfig jpaConfig;
- Tests pass 🤷
If for some reason tests start failing because the constructor of JPAConfig
ends up being executed during static init (and before PersistenceUnitsHolder
is initialized), we can make JPAConfig
a synthetic bean and force runtime init. Or, worst case, we can make JPAConfig
@ApplicationScoped
, so that it's created on first use.
In fact, I can create a quick PR to make JPAConfig
a synthetic bean right now. It seems more correct, regardless of whether it's necessary, because that bean is fundamentally a runtime bean. I'll open that PR soon.
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 question. If you're sure things are fine, you can merge it.
Merged, thanks! I'll submit another PR to make the |
Actually, after a bit of digging, Singleton beans are initialized when they are first injected or retrieved programmatically, just like ApplicationScoped beans: quarkus/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/SingletonContext.java Line 8 in c4a5f99
Lines 24 to 38 in 50f5d3d
So as long as we only Lines 83 to 87 in 0bc9711
Which is better (well, at least more specific) than what we'd get if we attempted to use a runtime-initialized, synthetic CDI bean: Lines 116 to 128 in 0cef176
So, I'll leave |
Fixes #17516, and goes a bit further.
In a previous PR (#26840), I reworked the Hibernate Search extension so that it exposes two configuration properties:
quarkus.hibernate-search-orm.enabled
, to enable/disable the extension a build time, for all persistence units. Setting it tofalse
will disable almost all build steps of the extension. This is mostly useful for re-augmentation of Quarkus apps, to disable an extension that is not necessary in a given deployment.quarkus.hibernate-search-orm.active
/quarkus.hibernate-search-orm."some-pu".active
, to activate/deactivate the extension for a particular persistence unit. Setting it tofalse
will prevent Hibernate Search from starting at runtime for the corresponding persistence unit. This is mostly useful when entities are annotated with Hibernate Search annotations (@Indexed
) but for some reason one wants to disable Hibernate Search in a given persistence unit; that can happen e.g. when importing entity classes from a common company JAR, which is a surprisingly (and regrettably, if you ask me) common practice. Critically, this is also a runtime property, which I suppose can be useful for some exotic use cases.I did look into making the
.enabled
property available on a per-persistence unit basis, but honestly, it's much more complicated, so I personally would avoid that until we have a convincing use case.This PR adds similar configuration properties for other extensions:
quarkus.hibernate-orm.enabled
quarkus.hibernate-orm.active
/quarkus.hibernate-orm."some-pu".active
quarkus.hibernate-envers.enabled
quarkus.hibernate-envers.active
/quarkus.hibernate-envers."some-pu".active
In the case of Envers, unfortunately,
.active
is a build-time property. That's because Envers adds entities to the metamodel, and we cannot remove those at runtime (they are added during static init). Hopefully one day we'll be able to "veto" entities at runtime init through some SPI, but this SPI doesn't exist yet, so.active
will have to remain a build-time property until then.I noticed while working on the Envers extension that all configuration is global, with no way of customizing it on a per-persistence unit basis. I don't know if that's on purpose; maybe we should create another issue to address that?