Skip to content

Commit

Permalink
fix!: EXPOSED-150 Auto-quoted column names change case across databas…
Browse files Browse the repository at this point in the history 
…es (#1841)

* fix!: EXPOSED-150 Auto-quoted column names change case across databases

Column and table names that are reserved keywords are automatically quoted before
being used in SQL statements. Databases that support upper case folding (H2, Oracle)
quote and upper case the identifiers, so attempting to use the tables across different
databases fails.

This fix ensures any reserved keywords used as identifiers are only quoted, so they
now retain whatever case the user provides them in, but it will be equivalent
across databases.

This broke some tests that checked for index name, as names like TABLE_column_IDX,
were being created in those databases. This was avoided by pulling inProperCase() out
of the buildString until the end of the name creation.

BREAKING CHANGE: [H2, Oracle] Reserved words will be treated as quoted identifiers
and no longer have their case automatically changed to upper case.

* fix!: EXPOSED-150 Auto-quoted column names change case across databases

Change table name of tests failing in Oracle due to using a quoted identifier.
These tests fail because of a JDBC driver bug specific to quoted table identifiers
beign used with an insert statement and preparedStatement(). Upgrading the driver
version to at least 21.1.0.0 resolves the issue but causes other failures (namely
batch insert / DML Returning errors). This will be investigated for the future,
but for now the tables are no longer named using reserved keywords.

* fix!: EXPOSED-150 Auto-quoted column names change case across databases

Add IdentifierManager cache for identities that have been checked against the
keywords list.

* fix!: EXPOSED-150 Auto-quoted column names change case across databases

Add global flag, preserveKeywordCasing, to DatabaseConfig to allow 
opt-out from new behavior.

Log warnings if table is created using identifiers that are in keywords list.

Restrict opt-in test to H2 for proper Database connection and configuration.

* fix!: EXPOSED-150 Auto-quoted column names change case across databases

Switch preserveKeywordCasing flag to have @OptIn and be false by default.

Adjust internal and private functions accordingly.

Adjust unit tests to test for when flag is opted-in.

Logged warnings only happen if keyword is identified and flag is not opted-in.

* fix!: EXPOSED-150 Auto-quoted column names change case across databases

Revert changed unit tests to use old behavior since flag set to false as default.
  • Loading branch information
@bog-walk
bog-walk authored Sep 22, 2023
1 parent 30a9ce0 commit 17a9dc8
Show file tree
Hide file tree
Showing 11 changed files with 231 additions and 26 deletions.
12 changes: 9 additions & 3 deletions exposed-core/api/exposed-core.api
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ public final class org/jetbrains/exposed/sql/Database$Companion {

public final class org/jetbrains/exposed/sql/DatabaseConfig {
public static final field Companion Lorg/jetbrains/exposed/sql/DatabaseConfig$Companion;
public synthetic fun <init> (Lorg/jetbrains/exposed/sql/SqlLogger;ZLjava/lang/Integer;IIJJZLjava/lang/Long;IZLorg/jetbrains/exposed/sql/vendors/DatabaseDialect;Lorg/jetbrains/exposed/sql/Schema;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public synthetic fun <init> (Lorg/jetbrains/exposed/sql/SqlLogger;ZLjava/lang/Integer;IIJJZLjava/lang/Long;IZLorg/jetbrains/exposed/sql/vendors/DatabaseDialect;Lorg/jetbrains/exposed/sql/Schema;IZLkotlin/jvm/internal/DefaultConstructorMarker;)V
public final fun getDefaultFetchSize ()Ljava/lang/Integer;
public final fun getDefaultIsolationLevel ()I
public final fun getDefaultMaxRepetitionDelay ()J
Expand All @@ -581,15 +581,16 @@ public final class org/jetbrains/exposed/sql/DatabaseConfig {
public final fun getKeepLoadedReferencesOutOfTransaction ()Z
public final fun getLogTooMuchResultSetsThreshold ()I
public final fun getMaxEntitiesToStoreInCachePerEntity ()I
public final fun getPreserveKeywordCasing ()Z
public final fun getSqlLogger ()Lorg/jetbrains/exposed/sql/SqlLogger;
public final fun getUseNestedTransactions ()Z
public final fun getWarnLongQueriesDuration ()Ljava/lang/Long;
}

public final class org/jetbrains/exposed/sql/DatabaseConfig$Builder {
public fun <init> ()V
public fun <init> (Lorg/jetbrains/exposed/sql/SqlLogger;ZLjava/lang/Integer;IIJJZLjava/lang/Long;IZLorg/jetbrains/exposed/sql/vendors/DatabaseDialect;Lorg/jetbrains/exposed/sql/Schema;I)V
public synthetic fun <init> (Lorg/jetbrains/exposed/sql/SqlLogger;ZLjava/lang/Integer;IIJJZLjava/lang/Long;IZLorg/jetbrains/exposed/sql/vendors/DatabaseDialect;Lorg/jetbrains/exposed/sql/Schema;IILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun <init> (Lorg/jetbrains/exposed/sql/SqlLogger;ZLjava/lang/Integer;IIJJZLjava/lang/Long;IZLorg/jetbrains/exposed/sql/vendors/DatabaseDialect;Lorg/jetbrains/exposed/sql/Schema;IZ)V
public synthetic fun <init> (Lorg/jetbrains/exposed/sql/SqlLogger;ZLjava/lang/Integer;IIJJZLjava/lang/Long;IZLorg/jetbrains/exposed/sql/vendors/DatabaseDialect;Lorg/jetbrains/exposed/sql/Schema;IZILkotlin/jvm/internal/DefaultConstructorMarker;)V
public final fun getDefaultFetchSize ()Ljava/lang/Integer;
public final fun getDefaultIsolationLevel ()I
public final fun getDefaultMaxRepetitionDelay ()J
Expand All @@ -601,6 +602,7 @@ public final class org/jetbrains/exposed/sql/DatabaseConfig$Builder {
public final fun getKeepLoadedReferencesOutOfTransaction ()Z
public final fun getLogTooMuchResultSetsThreshold ()I
public final fun getMaxEntitiesToStoreInCachePerEntity ()I
public final fun getPreserveKeywordCasing ()Z
public final fun getSqlLogger ()Lorg/jetbrains/exposed/sql/SqlLogger;
public final fun getUseNestedTransactions ()Z
public final fun getWarnLongQueriesDuration ()Ljava/lang/Long;
Expand All @@ -615,6 +617,7 @@ public final class org/jetbrains/exposed/sql/DatabaseConfig$Builder {
public final fun setKeepLoadedReferencesOutOfTransaction (Z)V
public final fun setLogTooMuchResultSetsThreshold (I)V
public final fun setMaxEntitiesToStoreInCachePerEntity (I)V
public final fun setPreserveKeywordCasing (Z)V
public final fun setSqlLogger (Lorg/jetbrains/exposed/sql/SqlLogger;)V
public final fun setUseNestedTransactions (Z)V
public final fun setWarnLongQueriesDuration (Ljava/lang/Long;)V
Expand Down Expand Up @@ -749,6 +752,9 @@ public final class org/jetbrains/exposed/sql/Exists : org/jetbrains/exposed/sql/
public fun toQueryBuilder (Lorg/jetbrains/exposed/sql/QueryBuilder;)V
}

public abstract interface annotation class org/jetbrains/exposed/sql/ExperimentalKeywordApi : java/lang/annotation/Annotation {
}

public abstract class org/jetbrains/exposed/sql/Expression {
public static final field Companion Lorg/jetbrains/exposed/sql/Expression$Companion;
public fun <init> ()V
Expand Down
24 changes: 19 additions & 5 deletions exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/DatabaseConfig.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ class DatabaseConfig private constructor(
val keepLoadedReferencesOutOfTransaction: Boolean,
val explicitDialect: DatabaseDialect?,
val defaultSchema: Schema?,
val logTooMuchResultSetsThreshold: Int
val logTooMuchResultSetsThreshold: Int,
val preserveKeywordCasing: Boolean,
) {

class Builder(
Expand Down Expand Up @@ -85,31 +86,35 @@ class DatabaseConfig private constructor(
* Useful when [eager loading](https://github.com/JetBrains/Exposed/wiki/DAO#eager-loading) is used.
*/
var keepLoadedReferencesOutOfTransaction: Boolean = false,

/**
* Set the explicit dialect for a database.
* This can be useful when working with unsupported dialects which have the same behavior as the one that
* Exposed supports.
*/
var explicitDialect: DatabaseDialect? = null,

/**
* Set the default schema for a database.
*/
var defaultSchema: Schema? = null,

/**
* Log too much result sets opened in parallel.
* The error log will contain the stacktrace of the place in the code where a new result set occurs, and it
* exceeds the threshold.
* 0 value means no log needed.
*/
var logTooMuchResultSetsThreshold: Int = 0,
/**
* Toggle whether table and column identifiers that are also keywords should retain their case sensitivity.
* Keeping user-defined case sensitivity (value set to `true`) may become the default in future releases.
*/
@ExperimentalKeywordApi
var preserveKeywordCasing: Boolean = false,
)

companion object {
operator fun invoke(body: Builder.() -> Unit = {}): DatabaseConfig {
val builder = Builder().apply(body)
@OptIn(ExperimentalKeywordApi::class)
return DatabaseConfig(
sqlLogger = builder.sqlLogger ?: Slf4jSqlDebugLogger,
useNestedTransactions = builder.useNestedTransactions,
Expand All @@ -124,8 +129,17 @@ class DatabaseConfig private constructor(
keepLoadedReferencesOutOfTransaction = builder.keepLoadedReferencesOutOfTransaction,
explicitDialect = builder.explicitDialect,
defaultSchema = builder.defaultSchema,
logTooMuchResultSetsThreshold = builder.logTooMuchResultSetsThreshold
logTooMuchResultSetsThreshold = builder.logTooMuchResultSetsThreshold,
preserveKeywordCasing = builder.preserveKeywordCasing,
)
}
}
}

@RequiresOptIn(
message = "This API is experimental and the behavior defined by setting this value to 'true' may become the default " +
"in future releases. Its usage must be marked with '@OptIn(org.jetbrains.exposed.sql.ExperimentalKeywordApi::class)' " +
"or '@org.jetbrains.exposed.sql.ExperimentalKeywordApi'."
)
@Target(AnnotationTarget.PROPERTY)
annotation class ExperimentalKeywordApi
21 changes: 19 additions & 2 deletions exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/Table.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1214,6 +1214,21 @@ open class Table(name: String = "") : ColumnSet(), DdlAware {
}
}

@Deprecated(
message = "This will be removed in future releases when the check becomes default in IdentifierManagerApi",
level = DeprecationLevel.WARNING
)
private fun String.warnIfUnflaggedKeyword() {
val warn = TransactionManager.currentOrNull()?.db?.identifierManager?.isUnflaggedKeyword(this) == true
if (warn) {
exposedLogger.warn(
"Keyword identifier used: '$this'. Case sensitivity may not be kept when quoted by default: '${inProperCase()}'. " +
"To keep case sensitivity, opt-in and set 'preserveKeywordCasing' to true in DatabaseConfig block."
)
}
}

@Suppress("CyclomaticComplexMethod")
override fun createStatement(): List<String> {
val createSequence = autoIncColumn?.autoIncColumnType?.autoincSeq?.let {
Sequence(
Expand All @@ -1233,9 +1248,11 @@ open class Table(name: String = "") : ColumnSet(), DdlAware {
if (currentDialect.supportsIfNotExists) {
append("IF NOT EXISTS ")
}
append(TransactionManager.current().identity(this@Table))
append(TransactionManager.current().identity(this@Table).also { tableName.warnIfUnflaggedKeyword() })
if (columns.isNotEmpty()) {
columns.joinTo(this, prefix = " (") { it.descriptionDdl(false) }
columns.joinTo(this, prefix = " (") { column ->
column.descriptionDdl(false).also { column.name.warnIfUnflaggedKeyword() }
}

if (columns.any { it.isPrimaryConstraintWillBeDefined }) {
primaryKeyConstraint()?.let { append(", $it") }
Expand Down
24 changes: 23 additions & 1 deletion ...sed-core/src/main/kotlin/org/jetbrains/exposed/sql/statements/api/IdentifierManagerApi.kt
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.jetbrains.exposed.sql.statements.api

import org.jetbrains.exposed.sql.transactions.TransactionManager
import org.jetbrains.exposed.sql.vendors.ANSI_SQL_2003_KEYWORDS
import org.jetbrains.exposed.sql.vendors.VENDORS_KEYWORDS
import org.jetbrains.exposed.sql.vendors.currentDialect
Expand Down Expand Up @@ -30,16 +31,35 @@ abstract class IdentifierManagerApi {
}

private val checkedIdentitiesCache = IdentifiersCache<Boolean>()
private val checkedKeywordsCache = IdentifiersCache<Boolean>()
private val shouldQuoteIdentifiersCache = IdentifiersCache<Boolean>()
private val identifiersInProperCaseCache = IdentifiersCache<String>()
private val quotedIdentifiersCache = IdentifiersCache<String>()

private fun String.isIdentifier() = !isEmpty() && first().isIdentifierStart() && all { it.isIdentifierStart() || it in '0'..'9' }
private fun Char.isIdentifierStart(): Boolean = this in 'a'..'z' || this in 'A'..'Z' || this == '_' || this in extraNameCharacters

private fun String.isAKeyword(): Boolean = checkedKeywordsCache.getOrPut(lowercase()) {
keywords.any { this.equals(it, true) }
}

@Deprecated(
message = "This will be removed in future releases when the behavior becomes default in IdentifierManagerApi",
level = DeprecationLevel.WARNING
)
internal fun isUnflaggedKeyword(identity: String): Boolean = identity.isAKeyword() && !shouldPreserveKeywordCasing

@Deprecated(
message = "This will be removed in future releases when the behavior becomes default in IdentifierManagerApi",
level = DeprecationLevel.WARNING
)
private val shouldPreserveKeywordCasing by lazy {
TransactionManager.currentOrNull()?.db?.config?.preserveKeywordCasing == true
}

fun needQuotes(identity: String): Boolean {
return checkedIdentitiesCache.getOrPut(identity.lowercase()) {
!identity.isAlreadyQuoted() && (keywords.any { identity.equals(it, true) } || !identity.isIdentifier())
!identity.isAlreadyQuoted() && (identity.isAKeyword() || !identity.isIdentifier())
}
}

Expand All @@ -51,6 +71,7 @@ abstract class IdentifierManagerApi {
val alreadyUpper = identity == identity.uppercase()
when {
alreadyQuoted -> false
identity.isAKeyword() && shouldPreserveKeywordCasing -> true
supportsMixedIdentifiers -> false
alreadyLower && isLowerCaseIdentifiers -> false
alreadyUpper && isUpperCaseIdentifiers -> false
Expand All @@ -67,6 +88,7 @@ abstract class IdentifierManagerApi {
alreadyQuoted && isUpperCaseQuotedIdentifiers -> identity.uppercase()
alreadyQuoted && isLowerCaseQuotedIdentifiers -> identity.lowercase()
supportsMixedIdentifiers -> identity
identity.isAKeyword() && shouldPreserveKeywordCasing -> identity
oracleVersion != OracleVersion.NonOracle -> identity.uppercase()
isUpperCaseIdentifiers -> identity.uppercase()
isLowerCaseIdentifiers -> identity.lowercase()
Expand Down
2 changes: 1 addition & 1 deletion exposed-money/src/test/kotlin/org/jetbrains/exposed/sql/money/MoneyTests.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ open class MoneyBaseTest : DatabaseTestsBase() {

@Test
fun testNullableCompositeColumnInsertAndSelect() {
val table = object : IntIdTable("Table") {
val table = object : IntIdTable("CompositeTable") {
val composite_money = compositeMoney(8, AMOUNT_SCALE, "composite_money").nullable()
}

Expand Down
Loading

0 comments on commit 17a9dc8

Please sign in to comment.