-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Hibernate ORM tries to connect to the database on startup even with schema validation disabled #48130
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
These internals are changing in Hibernate ORM 7
Because: 1. We currently disallow this for identifier generators only through a custom initializer for the identifier generator factory service, but that service is disappearing in Hibernate ORM 7.0.0.Beta1, leaving us only the setting `hibernate.cdi.extensions`. to disallow CDI for identifier generators -- and anything else that impacts metadata creation. 2. This is needed for quarkusio#40897, which will move more of metadata creation to build time -- where CDI is just not available. 3. Implementations of affected components needing access to CDI at runtime (so not during metadata creation) can still do so by calling `Arc.container()` to retrieve the relevant beans.
…equired dependencies With Derby moving to hibernate-community-dialects, Quarkus will need to add this dependency automatically.
…ilder Some of these changes probably pre-date Hibernate ORM 7.0, we just forgot to implement them.
Co-Authored-By: Jan Schatteman <[email protected]>
And remove Hibernate Commons Annotations, which isn't needed by any Hibernate project anymore.
…ibernate Search 8.0.0.Beta1
* Including verification for checkVersion * Including asserts on schema management
/cc @gsmet (hibernate-orm,hibernate-search), @marko-bekhta (hibernate-search) |
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! LGTM overall, though I have concerns about the temporary table thing -- see below.
To be merged after #41310 as it's based on @yrodiere Hibernate 7 branch, creating a Draft PR
FWIW if you're only going to address #30002 (and not #13522) in this PR, you can probably base your work on main
. I don't expect anything here to require the upgrade to ORM 7, and this would allow merging your PR more quickly.
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 entirely sure the offline mode should change the behaviour of dev mode. I think it's for deployment scenarios.
It definitely should IMO. As a rule, explicit configuration should never be ignored.
If people want Hibernate ORM to start offline in every mode, that's fine, and it can definitely make sense if they don't use dev services. Even when using dev services, I personally would want my app to behave as closely as possible to the way it will behave in production.
If people want Hibernate ORM to start offline only in prod mode, they can use %prod.quarkus.hibernate-orm.start-offline = true
.
throwable -> assertThat(throwable) | ||
.hasNoSuppressedExceptions() | ||
.hasMessageContaining( | ||
"Version check `quarkus.hibernate-orm.schema-management.strategy` cannot be any different than `none` when using offline mode `quarkus.hibernate-orm.database.start-offline=true`")); |
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.
Wording seems strange, especially the "any different"... Also it has nothing to do with the version check?
I'd recommend something like this:
"Version check `quarkus.hibernate-orm.schema-management.strategy` cannot be any different than `none` when using offline mode `quarkus.hibernate-orm.database.start-offline=true`")); | |
"Schema management strategy `quarkus.hibernate-orm.schema-management.strategy` only be set to `none` when using offline mode `quarkus.hibernate-orm.database.start-offline=true`")); |
@WithName("version-check.enabled") | ||
@ConfigDocDefault("`true` if the dialect was set automatically by Quarkus, `false` if it was set explicitly") | ||
@ConfigDocDefault("`true` if the dialect was set automatically by Quarkus, `false` if it was set explicitly, `false` in offlineMode") |
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.
Okay, three different possibilities is starting to get ambiguous :)
Maybe:
@ConfigDocDefault("`true` if the dialect was set automatically by Quarkus, `false` if it was set explicitly, `false` in offlineMode") | |
@ConfigDocDefault("`false` if the dialect was set explicitly or if starting offline (see `start-offline`), `true` otherwise") |
Optional<Boolean> versionCheckEnabled(); | ||
|
||
/** | ||
* This value will avoid connecting to a database to fetch JDBC metadata. |
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 at this point it's more about disabling/disallowing these other features? Or at least that will probably be more meaningful to application developers.
* This value will avoid connecting to a database to fetch JDBC metadata. | |
* Instructs Hibernate ORM to avoid connecting to the database on startup (for schema management, temporary table creation, DB version checking, ...). | |
* <p> |
// Allow detection of driver/database capabilities on runtime init if required | ||
// (was disabled during static init) | ||
runtimeSettingsBuilder.put(JdbcSettings.ALLOW_METADATA_ON_BOOT, !startsOffline); | ||
runtimeSettingsBuilder.put(GlobalTemporaryTableStrategy.CREATE_ID_TABLES, !startsOffline); |
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 will probably need more thoughts. If we don't create the tables, presumably something will go wrong when those tables are used.
So:
- When starting offline, should we consider simply overriding the temporary table kind to
local
? AFAIK H2 supports local temporary tables, and that was even the default before ORM 6.6: https://hibernate.atlassian.net/browse/HHH-18146 . - If instead we keep "persistent" temporary tables:
- Should we document this in Quarkus, and explain how to work around the problem?
- Should we set the other setting, another setting,
DROP_ID_TABLES
, tofalse
as well? If someone else created the tables, they probably wouldn't want us to drop them... - Should probably report the problem to Hibernate ORM, so that "temporary" table creation is integrated into schema management (hbm2ddl)? That makes a lot more sense to me...
I'd personally go with the first option, which puts the problem away in the short term, and file a ticket about investigating the second option later.. Because regardless,
Note: Keep in mind the "temporary tables" are only used in some specific scenarios, so existing tests may not detect any problems, yet there will be problems.
Fixes: #30002 (comment)
To be merged after #41310 as it's based on @yrodiere Hibernate 7 branch, creating a Draft PR
Based on the original work of https://github.com/xdev-software https://github.com/quarkusio/quarkus/pull/47695/files
This handles some of the issues addressed in #30002 (comment)
Done:
io.quarkus.hibernate.orm.runtime.HibernateOrmRuntimeConfigPersistenceUnit.HibernateOrmConfigPersistenceUnitDatabase#versionCheckEnabled should default to false when asked to start offline, and lead to an exception with a clear message when it's explicitly set to true while starting offline.
Except for the one for dev services for a few reasons: