HHH-16516 don't quote $ unnecessarily

it's only needed on HSQLDB

and remove unnecessary logging

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);
 	}
 

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<Identifier> {
 	/**
 	 * Means to generate an {@link Identifier} instance from its simple text form.
 	 * <p>
-	 * If passed text is {@code null}, {@code null} is returned.
+	 * If passed {@code text} is {@code null}, {@code null} is returned.
 	 * <p>
-	 * If passed text is surrounded in quote markers, the generated Identifier
+	 * If passed {@code text} is surrounded in quote markers, the returned Identifier
-	 * is considered quoted.  Quote markers include back-ticks (`),
+	 * is considered quoted. Quote markers include back-ticks (`), double-quotes ("),
-	 * double-quotes (") and brackets ([ and ]).
+	 * 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<Identifier> {
 			end--;
 			quote = true;
 		}
-		else if ( quoteOnNonIdentifierChar && !quote ) {
+		else if ( autoquote && !quote ) {
-			// Check the letters to determine if we must quote the text
+			quote = autoquote( text, start, end );
-			char c = text.charAt( start );
+		}
-			if ( !isLetter( c ) && c != '_' ) {
+		return new Identifier( text.substring( start, end ), quote );
-				// SQL identifiers must begin with a letter or underscore
+	}
-				quote = true;
+
-			}
+	private static boolean autoquote(String text, int start, int end) {
-			else {
+		// Check the letters to determine if we must quote the text
-				for ( int i = start + 1; i < end; i++ ) {
+		if ( !isLegalFirstChar( text.charAt( start ) ) ) {
-					c = text.charAt( i );
+			// SQL identifiers must begin with a letter or underscore
-					if ( !isLetterOrDigit( c ) && c != '_' ) {
+			return true;
-						quote = true;
+		}
-						break;
+		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<Identifier> {
 
 	public static boolean isQuoted(String name, int start, int end) {
 		if ( start + 2 < end ) {
-			switch ( name.charAt( start ) ) {
+			final char first = name.charAt( start );
-				case '`':
+			final char last = name.charAt( end - 1 );
-					return name.charAt( end - 1 ) == '`';
+			return switch ( first ) {
-				case '[':
+				case '`' -> last == '`';
-					return name.charAt( end - 1 ) == ']';
+				case '[' -> last == ']';
-				case '"':
+				case '"' -> last == '"';
-					return name.charAt( end - 1 ) == '"';
+				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<Identifier> {
 	}
 
 	@Override
-	public boolean equals(Object o) {
+	public boolean equals(Object object) {
-		if ( !(o instanceof Identifier that) ) {
+		return object instanceof Identifier that
-			return false;
+			&& getCanonicalName().equals( that.getCanonicalName() );
-		}
-		return getCanonicalName().equals( that.getCanonicalName() );
 	}
 
 	public boolean matches(String name) {
@@ -251,16 +267,13 @@ public class Identifier implements Comparable<Identifier> {
 
 	@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 Objects.equals( id1, id2 );
-			return id2 == null;
-		}
-		else {
-			return id1.equals( id2 );
-		}
 	}
 
 	public static Identifier quote(Identifier identifier) {
@@ -270,7 +283,7 @@ public class Identifier implements Comparable<Identifier> {
 	}
 
 	@Override
-	public int compareTo(Identifier o) {
+	public int compareTo(Identifier identifier) {
-		return getCanonicalName().compareTo( o.getCanonicalName() );
+		return getCanonicalName().compareTo( identifier.getCanonicalName() );
 	}
 }

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 );
 	}
 

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<String> 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<String> 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;
 		}
-
+		else if ( identifier.isQuoted() ) {
-		if ( identifier.isQuoted() ) {
 			return identifier;
 		}
-
+		else if ( mustQuote( identifier ) ) {
-		if ( globallyQuoteIdentifiers ) {
-			log.tracef( "Forcing identifier [%s] to quoted for global quoting", identifier );
 			return Identifier.toIdentifier( identifier.getText(), true );
 		}
-
+		else {
-		if ( autoQuoteKeywords && isReservedWord( identifier.getText() ) ) {
+			return identifier;
-			log.tracef( "Forcing identifier [%s] to quoted as recognized reserved word", identifier );
-			return Identifier.toIdentifier( identifier.getText(), true );
 		}
+	}
 
-		if ( autoQuoteInitialUnderscore && identifier.getText().startsWith("_") ) {
+	private boolean mustQuote(Identifier identifier) {
-			log.tracef( "Forcing identifier [%s] to quoted due to initial underscore", identifier );
+		return globallyQuoteIdentifiers
-			return Identifier.toIdentifier( identifier.getText(), true );
+			|| autoQuoteKeywords && isReservedWord( identifier.getText() )
-		}
+			|| autoQuoteInitialUnderscore && identifier.getText().startsWith( "_" )
-
+			|| autoQuoteDollar && identifier.getText().contains( "$" );
-		return identifier;
 	}
 
 	@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;
 		}
-
+		else if ( identifier.isQuoted() ) {
-		if ( identifier.isQuoted() ) {
+			return switch ( quotedCaseStrategy ) {
-			switch ( quotedCaseStrategy ) {
+				case UPPER -> text.toUpperCase( Locale.ROOT );
-				case UPPER: {
+				case LOWER -> text.toLowerCase( Locale.ROOT );
-					log.tracef( "Rendering quoted identifier [%s] in upper case for use in DatabaseMetaData", identifier );
+				case MIXED -> text; // default
-					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 {
-			switch ( unquotedCaseStrategy ) {
+			return switch ( unquotedCaseStrategy ) {
-				case MIXED: {
+				case MIXED -> text;
-					log.tracef( "Rendering unquoted identifier [%s] in mixed case for use in DatabaseMetaData", identifier );
+				case LOWER -> text.toLowerCase( Locale.ROOT );
-					return identifier.getText();
+				case UPPER -> text.toUpperCase( Locale.ROOT ); // default
-				}
+			};
-				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 );
-				}
-			}
 		}
 	}
 
 	@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" );

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

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;
+	}
+}