From e14ead0c0f0916cc8fdb8895e2cc481754a75583 Mon Sep 17 00:00:00 2001 From: Gavin King Date: Fri, 29 Nov 2024 16:17:14 +0100 Subject: [PATCH] HHH-16516 don't quote $ unnecessarily it's only needed on HSQLDB and remove unnecessary logging --- .../community/dialect/HSQLLegacyDialect.java | 1 + .../boot/model/naming/Identifier.java | 103 ++++++++++-------- .../org/hibernate/dialect/HSQLDialect.java | 1 + .../NormalizingIdentifierHelperImpl.java | 87 +++++---------- .../jdbc/env/spi/IdentifierHelperBuilder.java | 6 + .../autoquote/SpecialCharactersTest.java | 35 ++++++ 6 files changed, 127 insertions(+), 106 deletions(-) create mode 100644 hibernate-core/src/test/java/org/hibernate/orm/test/mapping/autoquote/SpecialCharactersTest.java diff --git a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/HSQLLegacyDialect.java b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/HSQLLegacyDialect.java index 09b61e963a..d512134a2c 100644 --- a/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/HSQLLegacyDialect.java +++ b/hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/HSQLLegacyDialect.java @@ -927,6 +927,7 @@ public class HSQLLegacyDialect extends Dialect { public IdentifierHelper buildIdentifierHelper(IdentifierHelperBuilder builder, DatabaseMetaData dbMetaData) throws SQLException { builder.setAutoQuoteInitialUnderscore(true); + builder.setAutoQuoteDollar(true); return super.buildIdentifierHelper(builder, dbMetaData); } diff --git a/hibernate-core/src/main/java/org/hibernate/boot/model/naming/Identifier.java b/hibernate-core/src/main/java/org/hibernate/boot/model/naming/Identifier.java index 61398e34f6..f9f3702aea 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/model/naming/Identifier.java +++ b/hibernate-core/src/main/java/org/hibernate/boot/model/naming/Identifier.java @@ -5,6 +5,7 @@ package org.hibernate.boot.model.naming; import java.util.Locale; +import java.util.Objects; import org.hibernate.dialect.Dialect; @@ -67,19 +68,19 @@ public class Identifier implements Comparable { /** * Means to generate an {@link Identifier} instance from its simple text form. *

- * If passed text is {@code null}, {@code null} is returned. + * If passed {@code text} is {@code null}, {@code null} is returned. *

- * If passed text is surrounded in quote markers, the generated Identifier - * is considered quoted. Quote markers include back-ticks (`), - * double-quotes (") and brackets ([ and ]). + * If passed {@code text} is surrounded in quote markers, the returned Identifier + * is considered quoted. Quote markers include back-ticks (`), double-quotes ("), + * and brackets ([ and ]). * * @param text The text form * @param quote Whether to quote unquoted text forms - * @param quoteOnNonIdentifierChar Controls whether to treat the result as quoted if text contains characters that are invalid for identifiers + * @param autoquote Whether to quote the result if it contains special characters * * @return The identifier form, or {@code null} if text was {@code null} */ - public static Identifier toIdentifier(String text, boolean quote, boolean quoteOnNonIdentifierChar) { + public static Identifier toIdentifier(String text, boolean quote, boolean autoquote) { if ( isBlank( text ) ) { return null; } @@ -102,24 +103,40 @@ public class Identifier implements Comparable { end--; quote = true; } - else if ( quoteOnNonIdentifierChar && !quote ) { - // Check the letters to determine if we must quote the text - char c = text.charAt( start ); - if ( !isLetter( c ) && c != '_' ) { - // SQL identifiers must begin with a letter or underscore - quote = true; - } - else { - for ( int i = start + 1; i < end; i++ ) { - c = text.charAt( i ); - if ( !isLetterOrDigit( c ) && c != '_' ) { - quote = true; - break; - } + else if ( autoquote && !quote ) { + quote = autoquote( text, start, end ); + } + return new Identifier( text.substring( start, end ), quote ); + } + + private static boolean autoquote(String text, int start, int end) { + // Check the letters to determine if we must quote the text + if ( !isLegalFirstChar( text.charAt( start ) ) ) { + // SQL identifiers must begin with a letter or underscore + return true; + } + else { + for ( int i = start + 1; i < end; i++ ) { + if ( !isLegalChar( text.charAt( i ) ) ) { + return true; } } } - return new Identifier( text.substring( start, end ), quote ); + return false; + } + + private static boolean isLegalChar(char current) { + return isLetterOrDigit( current ) + // every database also allows _ here + || current == '_' + // every database except HSQLDB also allows $ here + || current == '$'; + } + + private static boolean isLegalFirstChar(char first) { + return isLetter( first ) + // many databases also allow _ here + || first == '_'; } /** @@ -141,21 +158,22 @@ public class Identifier implements Comparable { public static boolean isQuoted(String name, int start, int end) { if ( start + 2 < end ) { - switch ( name.charAt( start ) ) { - case '`': - return name.charAt( end - 1 ) == '`'; - case '[': - return name.charAt( end - 1 ) == ']'; - case '"': - return name.charAt( end - 1 ) == '"'; - } + final char first = name.charAt( start ); + final char last = name.charAt( end - 1 ); + return switch ( first ) { + case '`' -> last == '`'; + case '[' -> last == ']'; + case '"' -> last == '"'; + default -> false; + }; + } + else { + return false; } - return false; } public static String unQuote(String name) { assert isQuoted( name ); - return name.substring( 1, name.length() - 1 ); } @@ -236,11 +254,9 @@ public class Identifier implements Comparable { } @Override - public boolean equals(Object o) { - if ( !(o instanceof Identifier that) ) { - return false; - } - return getCanonicalName().equals( that.getCanonicalName() ); + public boolean equals(Object object) { + return object instanceof Identifier that + && getCanonicalName().equals( that.getCanonicalName() ); } public boolean matches(String name) { @@ -251,16 +267,13 @@ public class Identifier implements Comparable { @Override public int hashCode() { - return isQuoted ? text.hashCode() : text.toLowerCase( Locale.ENGLISH ).hashCode(); + return isQuoted + ? text.hashCode() + : text.toLowerCase( Locale.ENGLISH ).hashCode(); } public static boolean areEqual(Identifier id1, Identifier id2) { - if ( id1 == null ) { - return id2 == null; - } - else { - return id1.equals( id2 ); - } + return Objects.equals( id1, id2 ); } public static Identifier quote(Identifier identifier) { @@ -270,7 +283,7 @@ public class Identifier implements Comparable { } @Override - public int compareTo(Identifier o) { - return getCanonicalName().compareTo( o.getCanonicalName() ); + public int compareTo(Identifier identifier) { + return getCanonicalName().compareTo( identifier.getCanonicalName() ); } } diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/HSQLDialect.java b/hibernate-core/src/main/java/org/hibernate/dialect/HSQLDialect.java index d1a7f2a569..c913615e81 100644 --- a/hibernate-core/src/main/java/org/hibernate/dialect/HSQLDialect.java +++ b/hibernate-core/src/main/java/org/hibernate/dialect/HSQLDialect.java @@ -712,6 +712,7 @@ public class HSQLDialect extends Dialect { public IdentifierHelper buildIdentifierHelper(IdentifierHelperBuilder builder, DatabaseMetaData dbMetaData) throws SQLException { builder.setAutoQuoteInitialUnderscore( true ); + builder.setAutoQuoteDollar( true ); return super.buildIdentifierHelper( builder, dbMetaData ); } diff --git a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/env/internal/NormalizingIdentifierHelperImpl.java b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/env/internal/NormalizingIdentifierHelperImpl.java index 85b4eeeb0c..7c23a18731 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/env/internal/NormalizingIdentifierHelperImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/env/internal/NormalizingIdentifierHelperImpl.java @@ -15,13 +15,10 @@ import org.hibernate.engine.jdbc.env.spi.IdentifierHelper; import org.hibernate.engine.jdbc.env.spi.JdbcEnvironment; import org.hibernate.engine.jdbc.env.spi.NameQualifierSupport; -import org.jboss.logging.Logger; - /** * @author Steve Ebersole */ public class NormalizingIdentifierHelperImpl implements IdentifierHelper { - private static final Logger log = Logger.getLogger( NormalizingIdentifierHelperImpl.class ); private final JdbcEnvironment jdbcEnvironment; @@ -30,6 +27,7 @@ public class NormalizingIdentifierHelperImpl implements IdentifierHelper { private final boolean globallyQuoteIdentifiersSkipColumnDefinitions; private final boolean autoQuoteKeywords; private final boolean autoQuoteInitialUnderscore; + private final boolean autoQuoteDollar; private final TreeSet reservedWords; private final IdentifierCaseStrategy unquotedCaseStrategy; private final IdentifierCaseStrategy quotedCaseStrategy; @@ -41,6 +39,7 @@ public class NormalizingIdentifierHelperImpl implements IdentifierHelper { boolean globallyQuoteIdentifiersSkipColumnDefinitions, boolean autoQuoteKeywords, boolean autoQuoteInitialUnderscore, + boolean autoQuoteDollar, TreeSet reservedWords, //careful, we intentionally omit making a defensive copy to not waste memory IdentifierCaseStrategy unquotedCaseStrategy, IdentifierCaseStrategy quotedCaseStrategy) { @@ -50,6 +49,7 @@ public class NormalizingIdentifierHelperImpl implements IdentifierHelper { this.globallyQuoteIdentifiersSkipColumnDefinitions = globallyQuoteIdentifiersSkipColumnDefinitions; this.autoQuoteKeywords = autoQuoteKeywords; this.autoQuoteInitialUnderscore = autoQuoteInitialUnderscore; + this.autoQuoteDollar = autoQuoteDollar; this.reservedWords = reservedWords; this.unquotedCaseStrategy = unquotedCaseStrategy == null ? IdentifierCaseStrategy.UPPER : unquotedCaseStrategy; this.quotedCaseStrategy = quotedCaseStrategy == null ? IdentifierCaseStrategy.MIXED : quotedCaseStrategy; @@ -57,32 +57,25 @@ public class NormalizingIdentifierHelperImpl implements IdentifierHelper { @Override public Identifier normalizeQuoting(Identifier identifier) { - log.tracef( "Normalizing identifier quoting [%s]", identifier ); - if ( identifier == null ) { return null; } - - if ( identifier.isQuoted() ) { + else if ( identifier.isQuoted() ) { return identifier; } - - if ( globallyQuoteIdentifiers ) { - log.tracef( "Forcing identifier [%s] to quoted for global quoting", identifier ); + else if ( mustQuote( identifier ) ) { return Identifier.toIdentifier( identifier.getText(), true ); } - - if ( autoQuoteKeywords && isReservedWord( identifier.getText() ) ) { - log.tracef( "Forcing identifier [%s] to quoted as recognized reserved word", identifier ); - return Identifier.toIdentifier( identifier.getText(), true ); + else { + return identifier; } + } - if ( autoQuoteInitialUnderscore && identifier.getText().startsWith("_") ) { - log.tracef( "Forcing identifier [%s] to quoted due to initial underscore", identifier ); - return Identifier.toIdentifier( identifier.getText(), true ); - } - - return identifier; + private boolean mustQuote(Identifier identifier) { + return globallyQuoteIdentifiers + || autoQuoteKeywords && isReservedWord( identifier.getText() ) + || autoQuoteInitialUnderscore && identifier.getText().startsWith( "_" ) + || autoQuoteDollar && identifier.getText().contains( "$" ); } @Override @@ -110,10 +103,7 @@ public class NormalizingIdentifierHelperImpl implements IdentifierHelper { @Override public String toMetaDataCatalogName(Identifier identifier) { - log.tracef( "Normalizing identifier quoting for catalog name [%s]", identifier ); - if ( !nameQualifierSupport.supportsCatalogs() ) { - log.trace( "Environment does not support catalogs; returning null" ); // null is used to tell DatabaseMetaData to not limit results based on catalog. return null; } @@ -133,53 +123,30 @@ public class NormalizingIdentifierHelperImpl implements IdentifierHelper { throw new IllegalArgumentException( "Identifier cannot be null; bad usage" ); } + final String text = identifier.getText(); if ( identifier instanceof DatabaseIdentifier ) { - return identifier.getText(); + return text; } - - if ( identifier.isQuoted() ) { - switch ( quotedCaseStrategy ) { - case UPPER: { - log.tracef( "Rendering quoted identifier [%s] in upper case for use in DatabaseMetaData", identifier ); - return identifier.getText().toUpperCase( Locale.ROOT ); - } - case LOWER: { - log.tracef( "Rendering quoted identifier [%s] in lower case for use in DatabaseMetaData", identifier ); - return identifier.getText().toLowerCase( Locale.ROOT ); - } - default: { - // default is mixed case - log.tracef( "Rendering quoted identifier [%s] in mixed case for use in DatabaseMetaData", identifier ); - return identifier.getText(); - } - } + else if ( identifier.isQuoted() ) { + return switch ( quotedCaseStrategy ) { + case UPPER -> text.toUpperCase( Locale.ROOT ); + case LOWER -> text.toLowerCase( Locale.ROOT ); + case MIXED -> text; // default + }; } else { - switch ( unquotedCaseStrategy ) { - case MIXED: { - log.tracef( "Rendering unquoted identifier [%s] in mixed case for use in DatabaseMetaData", identifier ); - return identifier.getText(); - } - case LOWER: { - log.tracef( "Rendering unquoted identifier [%s] in lower case for use in DatabaseMetaData", identifier ); - return identifier.getText().toLowerCase( Locale.ROOT ); - } - default: { - // default is upper case - log.tracef( "Rendering unquoted identifier [%s] in upper case for use in DatabaseMetaData", identifier ); - return identifier.getText().toUpperCase( Locale.ROOT ); - } - } + return switch ( unquotedCaseStrategy ) { + case MIXED -> text; + case LOWER -> text.toLowerCase( Locale.ROOT ); + case UPPER -> text.toUpperCase( Locale.ROOT ); // default + }; } } @Override public String toMetaDataSchemaName(Identifier identifier) { - log.tracef( "Normalizing identifier quoting for schema name [%s]", identifier ); - if ( !nameQualifierSupport.supportsSchemas() ) { // null is used to tell DatabaseMetaData to not limit results based on schema. - log.trace( "Environment does not support catalogs; returning null" ); return null; } @@ -195,8 +162,6 @@ public class NormalizingIdentifierHelperImpl implements IdentifierHelper { @Override public String toMetaDataObjectName(Identifier identifier) { - log.tracef( "Normalizing identifier quoting for object name [%s]", identifier ); - if ( identifier == null ) { // if this method was called, the value is needed throw new IllegalArgumentException( "null was passed as an object name" ); diff --git a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/env/spi/IdentifierHelperBuilder.java b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/env/spi/IdentifierHelperBuilder.java index 0fe2450260..5ff36157f2 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/env/spi/IdentifierHelperBuilder.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/env/spi/IdentifierHelperBuilder.java @@ -40,6 +40,7 @@ public class IdentifierHelperBuilder { private boolean skipGlobalQuotingForColumnDefinitions = false; private boolean autoQuoteKeywords = true; private boolean autoQuoteInitialUnderscore = false; + private boolean autoQuoteDollar = false; private IdentifierCaseStrategy unquotedCaseStrategy = IdentifierCaseStrategy.UPPER; private IdentifierCaseStrategy quotedCaseStrategy = IdentifierCaseStrategy.MIXED; @@ -150,6 +151,10 @@ public class IdentifierHelperBuilder { this.autoQuoteInitialUnderscore = autoQuoteInitialUnderscore; } + public void setAutoQuoteDollar(boolean autoQuoteDollar) { + this.autoQuoteDollar = autoQuoteDollar; + } + public NameQualifierSupport getNameQualifierSupport() { return nameQualifierSupport; } @@ -215,6 +220,7 @@ public class IdentifierHelperBuilder { skipGlobalQuotingForColumnDefinitions, autoQuoteKeywords, autoQuoteInitialUnderscore, + autoQuoteDollar, reservedWords, unquotedCaseStrategy, quotedCaseStrategy diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/mapping/autoquote/SpecialCharactersTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/mapping/autoquote/SpecialCharactersTest.java new file mode 100644 index 0000000000..45ed6fc843 --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/mapping/autoquote/SpecialCharactersTest.java @@ -0,0 +1,35 @@ +/* + * SPDX-License-Identifier: LGPL-2.1-or-later + * Copyright Red Hat Inc. and Hibernate Authors + */ +package org.hibernate.orm.test.mapping.autoquote; + +import jakarta.persistence.Column; +import jakarta.persistence.Entity; +import jakarta.persistence.Id; +import org.hibernate.testing.orm.junit.EntityManagerFactoryScope; +import org.hibernate.testing.orm.junit.Jpa; +import org.junit.jupiter.api.Test; + +@Jpa(annotatedClasses = SpecialCharactersTest.Simple.class) +public class SpecialCharactersTest { + @Test void test(EntityManagerFactoryScope scope) { + scope.getEntityManagerFactory(); + } + @Entity static class Simple { + @Id + long id; + @Column(name="NAME$NAME") + String nameWithDollar; + @Column(name="$NAME") + String nameWithInitialDollar; + @Column(name="NAME#NAME") + String nameWithHash; + @Column(name="NAME NAME") + String nameWithSpace; + @Column(name="NAME_NAME") + String nameWithUnderscore; + @Column(name="_NAME") + String nameWithInitialUnderscore; + } +}