Skip to content

Conversation

@sebersole
Copy link
Contributor

@sebersole sebersole commented Sep 19, 2024

https://issues.redhat.com/browse/QUARKUS-3363
Fixes #13522

Handle ORM DialectSpecificSettings

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 19, 2024

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • description should not be empty, describe your intent or provide links to the issues this PR is fixing (using Fixes #NNNNN) or changelogs

This message is automatically generated by a bot.

@quarkus-bot quarkus-bot bot added the area/hibernate-orm Hibernate ORM label Sep 19, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 19, 2024

/cc @gsmet (hibernate-orm), @yrodiere (hibernate-orm)

@github-actions
Copy link

github-actions bot commented Sep 19, 2024

🙈 The PR is closed and the preview is expired.

Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I added a few comments below, we can discuss on Zulip if you want.

Regarding documentation, it would probably make sense to add a subsection under "Dialects > Supported databases"; see here for the source, here for the rendered version for this PR (updates take a few minutes/hours after you push).
Note the subsection we'd add to the documentation does not need to list all configuration properties -- that's already done in this automatically-generated list. We would just need to mention that some dialects can be configured in more details to match the DB configuration, and link to the list.

Regarding tests, I think your only solution ATM would be to add something under integration-tests/jpa-oracle, and integration-tests/jpa-mysql, and so on.

You can take inspiration from DefaultSchemaTest for your test; I picked this as an example because it overrides some settings compared to other tests, which I think you will need to do. Relevant classes:

EDIT: Actually, since we're talking about build-time properties here, this won't work. I think your only option would be to add a second persistence unit to application.properties. You may need to have this new PU start offline if the settings you need to test won't work with the DB we test against -- but that's no big deal, as you mainly want to make sure the Dialect's state matches the settings you set.

Starting a PU offline will require an additional config property though, to explicitly ask Hibernate ORM to "start offline"... which was the point of #13522, and the reason we're introducing all these dialect-specific settings.

Comment on lines 331 to 330
/**
* Configuration specific to the Hibernate ORM {@linkplain org.hibernate.dialect.MySQLDialect}
*/
MySQLDialectConfig mysql();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note we have use dedicated configuration for MariaDB in Quarkus, e.g. there is a db-kind named mariadb, distinct from mysql. It would probably make sense to do the same here?

FWIW you might be able to use inheritance with config groups if you want to avoid duplicating code... I think Guillaume added that support in his recent changes to the config annotation processor. We'll need to double checked by having a look at the resulting documentation, and of course through tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could just add MySQLDialectConfig mariadb();, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, for now this would work.

* Specifies the bytes per character to use based on the database's configured
* <a href="https://dev.mysql.com/doc/refman/8.0/en/charset-charsets.html">charset</a>.
*
* @see org.hibernate.cfg.DialectSpecificSettings#MYSQL_BYTES_PER_CHARACTER
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note:

  1. Such links won't appear in generated documentation.
  2. Application users are unlikely to reach this javadoc directly, they'll most likely read the rendered version included in the Quarkus documentation -- and in IDE plugins.

That's why we tend to do things like this:

* Refer to
* link:{hibernate-orm-docs-url}#basic-uuid[this section of the Hibernate ORM documentation]
* to see the possible UUID representations.

Reminder that Asciidoc syntax requires @asciidoctlet in the Javadoc comment.

hibernate-orm-docs-url is defined here, in case you want to add links to Hibernate ORM Javadocs -- but since nowadays you have a list of documentation properties in the Hibernate ORM user guide, I guess it could be more practical to link directly there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder that Asciidoc syntax requires @asciidoctlet in the Javadoc comment.

+1, sure just have not been using asciidoc formatting yet. But clearly I will need to start :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but since nowadays you have a list of documentation properties in the Hibernate ORM user guide, I guess it could be more practical to link directly there?

I think that's a good idea, Currently we don't generate url anchors for each setting as we'd need to, but https://hibernate.atlassian.net/browse/HHH-18654

@sebersole sebersole force-pushed the dialect-specific-settings branch from 0a3dade to 46aa8b3 Compare September 20, 2024 13:06
@sebersole sebersole changed the title Handle ORM DialectSpecificSettings QUARKUS-3363 - Disable JDBC Metadata Defaults in quarkus Sep 20, 2024
@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation labels Sep 21, 2024
}

private static SupportedDatabaseKind determineDatabaseKind(String name) {
if ( name.contains( "db2" ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DatabaseKind.isDB2(name) seems more appropriate? Same for others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because this is also called with the Dialect name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will not work well for db-kind then, because some database kinds (e.g. postgresql) have more than one corresponding string (e.g. pg). You'll probably need two different classes.

For what it's worth, we already have code that infers the "db product name" (the Hibernate ORM one) from the explicit dialect here:

if (dbKind.isPresent() && DatabaseKind.is(dbKind.get(), item.getDbKind())
// Set the default version based on the dialect when we don't have a datasource
// (i.e. for database multi-tenancy)
|| explicitDialect.isPresent() && item.getMatchingDialects().contains(explicitDialect.get())) {
dbProductName = item.getDatabaseProductName();

So maybe you should base your code on this dbProductName instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will not work well for db-kind then, because some database kinds (e.g. postgresql) have more than one corresponding string (e.g. pg).

Look at the callers. One works with a passed "database kind" and manually does the normalize which handles the case you were talking about (multiple names). The other works with the Dialect name.

So maybe you should base your code on this dbProductName instead?

Maybe, I'll take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this code handles (1) db-kind and (2) explicit Dialect name.

Are you saying that it is legal to specify a dbProductName but not db-kind?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the "problem" with using dbProductName is that, unless Quarkus does this somewhere, there is no simple mapping of dbProductName to Dialect or SupportedDatabaseKind.

Assuming that it is legal to have just dbProductName without either db-kind or dialect, we'd need a method that handles mapping dbProductName to a SupportedDatabaseKind

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no simple mapping of dbProductName to Dialect or SupportedDatabaseKind

Well, actually, I could leverage org.hibernate.dialect.Database to implement this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I added handling based on database product name as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Then if tests pass, we're all set.

FYI, the only way the database product name can be missing is if someone has no assigned datasource (because it's dynamic, see database multi-tenancy). Even then, users will need to set a dialect explicitly, and as long as they use one of the "standard" dialects, the database product name will be inferred from that dialect.

@yrodiere yrodiere changed the title QUARKUS-3363 - Disable JDBC Metadata Defaults in quarkus QUARKUS-3363 - Offline startup for Hibernate ORM in Quarkus Sep 24, 2024
@sebersole
Copy link
Contributor Author

Was the integration test I added run? I'm not sure which Action check runs those integration tests.

@yrodiere
Copy link
Member

Was the integration test I added run? I'm not sure which Action check runs those integration tests.

CI does not run on drafts, you need to mark as ready for review and add [WIP] to the title. I'll do it.

@yrodiere yrodiere changed the title QUARKUS-3363 - Offline startup for Hibernate ORM in Quarkus [WIP] QUARKUS-3363 - Offline startup for Hibernate ORM in Quarkus Sep 25, 2024
@yrodiere yrodiere marked this pull request as ready for review September 25, 2024 11:54
@yrodiere
Copy link
Member

Was the integration test I added run? I'm not sure which Action check runs those integration tests.

CI does not run on drafts, you need to mark as ready for review and add [WIP] to the title. I'll do it.

Though the recommended way of working if your code is not ready for review is to run GitHub Actions in your fork instead.

@quarkus-bot

This comment has been minimized.

Handle Hibernate's `DialectSpecificSettings` as supported Quarkus settings
Handle Hibernate's `DialectSpecificSettings` as supported Quarkus settings
@sebersole sebersole force-pushed the dialect-specific-settings branch from 72a6447 to a97c74f Compare September 25, 2024 12:41
@quarkus-bot

This comment has been minimized.

@sebersole
Copy link
Contributor Author

Imports are not sorted in /home/runner/work/quarkus/quarkus/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java

Um, I ran the formatter before I pushed. Even now, locally, I run the formatter and nothing changes.

What do I need to do here? I find it strange that the IJ format stuff does not handle this either?

@yrodiere
Copy link
Member

Imports are not sorted in /home/runner/work/quarkus/quarkus/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java

Um, I ran the formatter before I pushed. Even now, locally, I run the formatter and nothing changes.

What do I need to do here? I find it strange that the IJ format stuff does not handle this either?

How did you format code?

The formatter does not handle import order, that's a different setting.

If you installed the Eclipse Formatter plugin, you need to point to the import order config explicitly, it's a different file.
If you format from the command line, you must make sure to call the right goal/plugin.

@sebersole
Copy link
Contributor Author

I followed the directions from CONTRIBUTING :

Formatting

Open the Preferences window (or Settings depending on your edition), navigate to Plugins and install the [Adapter for Eclipse Code Formatter](https://plugins.jetbrains.com/plugin/6546-eclipse-code-formatter) from the Marketplace.

Restart your IDE, open the Preferences (or Settings) window again and navigate to Adapter for Eclipse Code Formatter section on the left pane.

Select Use Eclipse's Code Formatter, then change the Eclipse workspace/project folder or config file to point to the eclipse-format.xml file in the independent-projects/ide-config/src/main/resources directory. Make sure the Optimize Imports box is ticked. Then, select Import Order from file and make it point to the eclipse.importorder file in the independent-projects/ide-config/src/main/resources directory.

@sebersole
Copy link
Contributor Author

I "format" using mvnw -f extensions/hibernate-orm/deployment net.revelc.code.formatter:formatter-maven-plugin:2.24.1:format because that's what the actions suggested earlier.

@yrodiere
Copy link
Member

I "format" using mvnw -f extensions/hibernate-orm/deployment net.revelc.code.formatter:formatter-maven-plugin:2.24.1:format because that's what the actions suggested earlier.

I suppose the formatting plugin failed and suggested it. Here it's another plugin failing.

To run formatting + import sorting:

If you want to run the formatting without doing a full build, you can run `./mvnw process-sources`.

Or if you want to limit to one module:

./mvnw process-sources -f extensions/hibernate-orm

@yrodiere
Copy link
Member

This part should have ensured that imports get sorted when you run auto-formatting in your IDE:

Then, select Import Order from file and make it point to the eclipse.importorder file in the independent-projects/ide-config/src/main/resources directory.

There something wrong if CTRL+ALT+O / CTRL+ALT+L still doesn't format code correctly...

Handle Hibernate's `DialectSpecificSettings` as supported Quarkus settings

- test for mariadb
@sebersole sebersole force-pushed the dialect-specific-settings branch from 14b5d3c to 981a79e Compare September 25, 2024 13:47
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @sebersole , sorry I left this aside... were you waiting for feedback from me?

From what I can see you finished implementing most setting, and were trying to run tests.

Though I suspect in order to run tests, we will need to introduce the config property that QUARKUS-3363 actually is about (the dialect config just happens to be a prerequisite): something that tells Hibernate ORM to start offline, i.e. that sets hibernate.boot.allow_jdbc_metadata_access to false (on runtime startup), and probably triggers exceptions if users requested automatic schema creation/update/validation.

Comment on lines +11 to +13
quarkus.hibernate."offline".mariadb.bytesPerCharacter=1
quarkus.hibernate."offline".mariadb.noBackslashEscapes=true
quarkus.hibernate."offline".mariadb.storageEngine=MY_CUSTOM_ENGINE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
quarkus.hibernate."offline".mariadb.bytesPerCharacter=1
quarkus.hibernate."offline".mariadb.noBackslashEscapes=true
quarkus.hibernate."offline".mariadb.storageEngine=MY_CUSTOM_ENGINE
quarkus.hibernate-orm."offline".mariadb.bytesPerCharacter=1
quarkus.hibernate-orm."offline".mariadb.noBackslashEscapes=true
quarkus.hibernate-orm."offline".mariadb.storageEngine=MY_CUSTOM_ENGINE

lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Jul 29, 2025
* Offline test running (except for MY_CUSTOM_ENGINE)
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Jul 30, 2025
* Offline test running (except for MY_CUSTOM_ENGINE)
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Jul 30, 2025
* Offline test running (except for MY_CUSTOM_ENGINE)
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Jul 31, 2025
* Offline test running (except for MY_CUSTOM_ENGINE)
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Jul 31, 2025
* Offline test running (except for MY_CUSTOM_ENGINE)
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Jul 31, 2025
* Offline test running (except for MY_CUSTOM_ENGINE)
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Aug 1, 2025
* Offline test running (except for MY_CUSTOM_ENGINE)
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Aug 1, 2025
* Offline test running (except for MY_CUSTOM_ENGINE)
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Aug 1, 2025
…Hibernate Reactive quarkusio#48130

Added a `quarkus.hibernate-orm.database.start-offline` to avoid connecting to the database

* Disable schema validation
* Disable temporary table creation at startup, gives precedence to local temporary tables using Hibernate 7.1 local mutation strategies
* Disable schema management in offline mode for DevServices as well

New way to handle storage engine from both mysql and mariadb

Included initial draft by Steve Ebersole <[email protected]> quarkusio#43396

Co-authored-by: Steve Ebersole <[email protected]>
Co-authored-by: Yoann Rodière <[email protected]>
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Aug 4, 2025
…Hibernate Reactive quarkusio#48130

Added a `quarkus.hibernate-orm.database.start-offline` to avoid connecting to the database

* Disable schema validation
* Disable temporary table creation at startup, gives precedence to local temporary tables using Hibernate 7.1 local mutation strategies
* Disable schema management in offline mode for DevServices as well

New way to handle storage engine from both mysql and mariadb

Included initial draft by Steve Ebersole <[email protected]> quarkusio#43396

Co-authored-by: Steve Ebersole <[email protected]>
Co-authored-by: Yoann Rodière <[email protected]>
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Aug 4, 2025
…Hibernate Reactive quarkusio#48130

Added a `quarkus.hibernate-orm.database.start-offline` to avoid connecting to the database

* Disable schema validation
* Disable temporary table creation at startup, gives precedence to local temporary tables using Hibernate 7.1 local mutation strategies
* Disable schema management in offline mode for DevServices as well

New way to handle storage engine from both mysql and mariadb

Added tests for specific override of Dialect Settings

Removed unused hack MultiplePersistenceUnitsInconsistentStorageEnginesTest$H2DialectWithMySQLInTheName

Included initial draft by Steve Ebersole <[email protected]> quarkusio#43396

Co-authored-by: Steve Ebersole <[email protected]>
Co-authored-by: Yoann Rodière <[email protected]>
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Aug 4, 2025
…Hibernate Reactive quarkusio#48130

Added a `quarkus.hibernate-orm.database.start-offline` to avoid connecting to the database

* Disable schema validation
* Disable temporary table creation at startup, gives precedence to local temporary tables using Hibernate 7.1 local mutation strategies
* Disable schema management in offline mode for DevServices as well

New way to handle storage engine from both mysql and mariadb

Added tests for specific override of Dialect Settings

Removed unused hack MultiplePersistenceUnitsInconsistentStorageEnginesTest$H2DialectWithMySQLInTheName

Included initial draft by Steve Ebersole <[email protected]> quarkusio#43396

Co-authored-by: Steve Ebersole <[email protected]>
Co-authored-by: Yoann Rodière <[email protected]>
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Aug 5, 2025
…Hibernate Reactive quarkusio#48130

Added a `quarkus.hibernate-orm.database.start-offline` to avoid connecting to the database

* Disable schema validation
* Disable temporary table creation at startup, gives precedence to local temporary tables using Hibernate 7.1 local mutation strategies
* Disable schema management in offline mode for DevServices as well

New way to handle storage engine from both mysql and mariadb

Added tests for specific override of Dialect Settings

Removed unused hack MultiplePersistenceUnitsInconsistentStorageEnginesTest$H2DialectWithMySQLInTheName

Included initial draft by Steve Ebersole <[email protected]> quarkusio#43396

Co-authored-by: Steve Ebersole <[email protected]>
Co-authored-by: Yoann Rodière <[email protected]>
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Aug 5, 2025
…Hibernate Reactive quarkusio#48130

Added a `quarkus.hibernate-orm.database.start-offline` to avoid connecting to the database

* Disable schema validation
* Disable temporary table creation at startup, gives precedence to local temporary tables using Hibernate 7.1 local mutation strategies
* Disable schema management in offline mode for DevServices as well

New way to handle storage engine from both mysql and mariadb

Added tests for specific override of Dialect Settings

Removed unused hack MultiplePersistenceUnitsInconsistentStorageEnginesTest$H2DialectWithMySQLInTheName

Included initial draft by Steve Ebersole <[email protected]> quarkusio#43396

Co-authored-by: Steve Ebersole <[email protected]>
Co-authored-by: Yoann Rodière <[email protected]>
lucamolteni added a commit to lucamolteni/quarkus that referenced this pull request Aug 5, 2025
…Hibernate Reactive quarkusio#48130

Added a `quarkus.hibernate-orm.database.start-offline` to avoid connecting to the database

* Disable schema validation
* Disable temporary table creation at startup, gives precedence to local temporary tables using Hibernate 7.1 local mutation strategies
* Disable schema management in offline mode for DevServices as well

New way to handle storage engine from both mysql and mariadb

Added tests for specific override of Dialect Settings

Removed unused hack MultiplePersistenceUnitsInconsistentStorageEnginesTest$H2DialectWithMySQLInTheName

Register for reflection the local temp strategies otherwise Native compilation is broken (to be removed after https://hibernate.atlassian.net/browse/HHH-15525)

Included initial draft by Steve Ebersole <[email protected]> quarkusio#43396

Co-authored-by: Steve Ebersole <[email protected]>
Co-authored-by: Yoann Rodière <[email protected]>
mbellade pushed a commit to mbellade/quarkus that referenced this pull request Aug 7, 2025
…Hibernate Reactive quarkusio#48130

Added a `quarkus.hibernate-orm.database.start-offline` to avoid connecting to the database

* Disable schema validation
* Disable temporary table creation at startup, gives precedence to local temporary tables using Hibernate 7.1 local mutation strategies
* Disable schema management in offline mode for DevServices as well

New way to handle storage engine from both mysql and mariadb

Added tests for specific override of Dialect Settings

Removed unused hack MultiplePersistenceUnitsInconsistentStorageEnginesTest$H2DialectWithMySQLInTheName

Register for reflection the local temp strategies otherwise Native compilation is broken (to be removed after https://hibernate.atlassian.net/browse/HHH-15525)

Included initial draft by Steve Ebersole <[email protected]> quarkusio#43396

Co-authored-by: Steve Ebersole <[email protected]>
Co-authored-by: Yoann Rodière <[email protected]>
@yrodiere
Copy link
Member

Integrated into #49408, which supersedes this PR. Thank for all the work!

@yrodiere yrodiere closed this Aug 21, 2025
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Aug 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docstyle issues related for manual docstyle review area/documentation area/hibernate-orm Hibernate ORM triage/invalid This doesn't seem right

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Infer database capabilities from configuration, without opening a JDBC connection on runtime init

2 participants