Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow setting additional Hibernate quoting options #30184

Merged
merged 1 commit into from
Jan 17, 2023
Merged

Allow setting additional Hibernate quoting options #30184

merged 1 commit into from
Jan 17, 2023

Conversation

brunolmfg
Copy link
Contributor

@brunolmfg brunolmfg commented Jan 4, 2023 •
edited
Loading

Fixes #28593

This implementation will allow to set additional quoting options available in Hibernate mapping properties.

Today, only enabling global quoting all identifiers is allowed by Hibernate ORM and Hibernate Reactive extensions. Now, it will be possible to enable the useful quoting options in both extensions setting the property .quote-identifiers.strategy with one of the available strategies:

Quoting option Description
none None identifiers will be quoted. (default)
all All identifiers will be quoted.
all-except-column-definitions All identifiers, except column definitions, will be quoted.
only-keywords Only the deemed keywords will be quoted.

The current property .database.globally-quoted-identifiers has been marked as deprecated in favor of .quote-identifiers.strategy.

Note: There is no documentation specific for globallyQuotedIdentifiers configuration, so the new property are absent either.

@quarkus-bot quarkus-bot bot added area/hibernate-orm Hibernate ORM area/hibernate-reactive Hibernate Reactive area/persistence OBSOLETE, DO NOT USE labels Jan 4, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Jan 4, 2023

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

@gastaldi gastaldi requested review from gsmet and Sanne January 4, 2023 23:45
@yrodiere yrodiere mentioned this pull request Jan 5, 2023
Closed
@Sanne Sanne force-pushed the allow-hibernate-quoting-options branch from dadac90 to 872d15e Compare January 5, 2023 12:26
@quarkus-bot

This comment has been minimized.

Sign in to view
@brunolmfg brunolmfg force-pushed the allow-hibernate-quoting-options branch from 872d15e to 4c8ea5e Compare January 5, 2023 13:15
@brunolmfg

This comment was marked as resolved.

Sign in to view
@brunolmfg brunolmfg force-pushed the allow-hibernate-quoting-options branch from 4c8ea5e to 5c2de01 Compare January 5, 2023 16:20
@brunolmfg
Copy link
Contributor Author

The existing case was extended by adding a column with a reserved name and column definition. So, that situation is resolved by activating globally-quoted-identifiers and globally-quoted-identifiers-skip-column-definitions in one test, and activating only auto-quote-keyword in another test.

@yrodiere @Sanne, the workflow can resume now if the changes are ok.

@brunolmfg brunolmfg force-pushed the allow-hibernate-quoting-options branch from 5c2de01 to a679264 Compare January 5, 2023 18:32
@brunolmfg
Copy link
Contributor Author

brunolmfg commented Jan 5, 2023 •
edited
Loading

I'm thinking if creating a config group for the quoting options would be a better approach.

quarkus.hibernate-orm.quote.all-identifiers=true
quarkus.hibernate-orm.quote.skip-column-definitions=true
quarkus.hibernate-orm.quote.auto-keywords=true
quarkus.hibernate-orm.quote.auto-initial-underscore=true

The first option can allow to remove or deprecate the quarkus.hibernate-orm.database.globally-quoted-identifiers configuration. Now the last option will be available when Quarkus supports Hibernate 6.x.

What do you think?

@Sanne
Copy link
Member

Sanne commented Jan 5, 2023

@brunolmfg +1 I like your idea of grouping the "quoting" properties

@quarkus-bot

This comment has been minimized.

Sign in to view
@brunolmfg brunolmfg force-pushed the allow-hibernate-quoting-options branch from a679264 to c279940 Compare January 6, 2023 14:01
@brunolmfg
Copy link
Contributor Author

I like your idea of grouping the "quoting" properties

I've created the group 'quote' for persistence unit configurations. Also, a warning will be emitted when enabling skip-column-definitions without all-identifiers and enabling auto-keyword and all-identifiers together. The property *.database.globally-quoted-identifiers has been deprecated in favor of *.quote.all-identifiers.

@brunolmfg brunolmfg force-pushed the allow-hibernate-quoting-options branch from c279940 to 6c419ac Compare January 9, 2023 14:35
@brunolmfg
Copy link
Contributor Author

Branch rebased with latest main. Waiting workflow if changes are ok.

@gsmet gsmet requested a review from yrodiere January 9, 2023 14:53
yrodiere
yrodiere reviewed Jan 9, 2023
View reviewed changes
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.

Tests look good, but I'm not sure about the configuration property structure, could you have a look at my comment?

...ent/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmConfigPersistenceUnit.java Outdated Show resolved Hide resolved
...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java Outdated Show resolved Hide resolved
...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java Outdated Show resolved Hide resolved
@brunolmfg brunolmfg force-pushed the allow-hibernate-quoting-options branch from 6c419ac to ab409bf Compare January 10, 2023 15:46
@brunolmfg
Copy link
Contributor Author

The branch was rebased with the latest main and changes discussed during the review.

@brunolmfg brunolmfg requested review from yrodiere and removed request for Sanne and gsmet January 10, 2023 16:01
@brunolmfg brunolmfg force-pushed the allow-hibernate-quoting-options branch from ab409bf to f3e702b Compare January 10, 2023 22:05
yrodiere
yrodiere approved these changes Jan 11, 2023
View reviewed changes
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.

Looking great, thanks!

I have a few suggestions about wording in the documentation, but I'm not a native English speaker, so... feel free to ignore.

...ent/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmConfigPersistenceUnit.java Outdated Show resolved Hide resolved
...ent/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmConfigPersistenceUnit.java Outdated Show resolved Hide resolved
@brunolmfg brunolmfg force-pushed the allow-hibernate-quoting-options branch from f3e702b to 79319f6 Compare January 11, 2023 11:27
@brunolmfg
Copy link
Contributor Author

I have a few suggestions about wording in the documentation, but I'm not a native English speaker, so... feel free to ignore.

I'm not too. 😅

But your suggestions seem clearer, so I've accepted and applied them.

@quarkus-bot

This comment has been minimized.

Sign in to view
@brunolmfg
Allow setting additional Hibernate quoting options
d0c7706 
Deprecate the property `globally-quoted-identifiers` indicating to use
`quote-identifiers.strategy` property. It can be possible to skip column
definitions when quoting identifiers or quote only the deemed keywords.
@brunolmfg brunolmfg force-pushed the allow-hibernate-quoting-options branch from 79319f6 to d0c7706 Compare January 11, 2023 12:37
@yrodiere yrodiere added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 11, 2023
@yrodiere yrodiere merged commit f07da48 into quarkusio:main Jan 17, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 17, 2023
@quarkus-bot quarkus-bot bot added this to the 2.17 - main milestone Jan 17, 2023
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jan 17, 2023
@yrodiere
Copy link
Member

Merged, thanks!

@yrodiere
Copy link
Member

yrodiere commented Jan 17, 2023 •
edited
Loading

I mentioned the deprecation in the migration guide: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-2.16#jpa--hibernate-orm https://github.com/quarkusio/quarkus/wiki/Migration-Guide-2.17#jpa--hibernate-orm

@brunolmfg brunolmfg deleted the allow-hibernate-quoting-options branch January 17, 2023 11:20
@brunolmfg
Copy link
Contributor Author

@yrodiere thanks for merging! But wouldn't the correct migration page be 2.17? Or do you intend to backport to 2.16 as well?

@yrodiere
Copy link
Member

@brunolmfg Damn, you're right. Good catch. I'll fix that.

@yrodiere
Copy link
Member

And no I don't intend to backport this, since it's more of a new feature.

Thanks for your work!

@brunolmfg
Copy link
Contributor Author

@yrodiere Considering the property quarkus.hibernate-orm.globally-quoted-identifiers was deprecated in 3.0, would we remove it in 3.2 – probably a LTS version – or in a future version?

@yrodiere
Copy link
Member

@brunolmfg Personally I would remove it in Quarkus 4. I don't see any rush.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yrodiere yrodiere yrodiere approved these changes

Assignees
No one assigned
Labels
area/hibernate-orm Hibernate ORM area/hibernate-reactive Hibernate Reactive area/persistence OBSOLETE, DO NOT USE kind/enhancement New feature or request
Projects
None yet
Milestone
3.0.0.Alpha4
Development

Successfully merging this pull request may close these issues.

Add globally_quoted_identifiers_skip_column_definitions to quarkus.hibernate-orm.database
3 participants
@brunolmfg @Sanne @yrodiere