Skip to content

Commit

Permalink
fix: EXPOSED-91 NPE in existingIndices() with function index (SQLite,…
Browse files Browse the repository at this point in the history
… MySQL) (JetBrains#1791)

existingIndices() in JdbcDatabaseMetadataImpl requires a COLUMN_NAME value from
every table index retrieved through metadata. If an index is function-based, PostgreSQL
and Oracle return String values of the function definitions, but MySQL and SQLite
return null values.

The !! operator has been removed and null values handles to ensure that the correct
indices are added to the returned map value, to be used for example by functions
like createMissingTablesAndColumns.

**Note:**
- PostgreSQL functional indices follow same rules as partial indices, in that
they must be dropped using DROP INDEX, not ALTER TABLE. Vendor dialect dropIndex()
had to be refactored to join these 2 conditions.
  • Loading branch information
bog-walk authored and saral committed Oct 3, 2023
1 parent 86823ae commit e94514a
Show file tree
Hide file tree
Showing 9 changed files with 52 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,9 @@ data class Index(

override fun createStatement(): List<String> = listOf(currentDialect.createIndex(this))
override fun modifyStatement(): List<String> = dropStatement() + createStatement()
override fun dropStatement(): List<String> = listOf(currentDialect.dropIndex(table.nameInDatabaseCase(), indexName, unique, filterCondition != null))
override fun dropStatement(): List<String> = listOf(
currentDialect.dropIndex(table.nameInDatabaseCase(), indexName, unique, filterCondition != null || functions != null)
)

/** Returns `true` if the [other] index has the same columns and uniqueness as this index, but a different name, `false` otherwise */
fun onlyNameDiffer(other: Index): Boolean = indexName != other.indexName && columns == other.columns && unique == other.unique
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -936,7 +936,7 @@ interface DatabaseDialect {
fun createIndex(index: Index): String

/** Returns the SQL command that drops the specified [indexName] from the specified [tableName]. */
fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartial: Boolean): String
fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartialOrFunctional: Boolean): String

/** Returns the SQL command that modifies the specified [column]. */
fun modifyColumn(column: Column<*>, columnDiff: ColumnDiff): List<String>
Expand Down Expand Up @@ -1187,6 +1187,8 @@ abstract class VendorDialect(
when (it) {
is Column<*> -> t.identity(it)
is Function<*> -> indexFunctionToString(it)
// returned by existingIndices() mapping String metadata to stringLiteral()
is LiteralOp<*> -> it.value.toString().trim('"')
else -> {
exposedLogger.warn("Unexpected defining key field will be passed as String: $it")
it.toString()
Expand Down Expand Up @@ -1222,7 +1224,7 @@ abstract class VendorDialect(
return "CREATE INDEX $name ON $table $columns USING $type$filterCondition"
}

override fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartial: Boolean): String {
override fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartialOrFunctional: Boolean): String {
return "ALTER TABLE ${identifierManager.quoteIfNecessary(tableName)} DROP CONSTRAINT ${identifierManager.quoteIfNecessary(indexName)}"
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ open class MysqlDialect : VendorDialect(dialectName, MysqlDataTypeProvider, Mysq
return super.createIndex(index)
}

override fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartial: Boolean): String =
override fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartialOrFunctional: Boolean): String =
"ALTER TABLE ${identifierManager.quoteIfNecessary(tableName)} DROP INDEX ${identifierManager.quoteIfNecessary(indexName)}"

override fun setSchema(schema: Schema): String = "USE ${schema.identifier}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ open class OracleDialect : VendorDialect(dialectName, OracleDataTypeProvider, Or

override fun isAllowedAsColumnDefault(e: Expression<*>): Boolean = true

override fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartial: Boolean): String {
override fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartialOrFunctional: Boolean): String {
return "DROP INDEX ${identifierManager.quoteIfNecessary(indexName)}"
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +324,8 @@ open class PostgreSQLDialect : VendorDialect(dialectName, PostgreSQLDataTypeProv
return "CREATE INDEX $name ON $table USING $type $columns$filterCondition"
}

override fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartial: Boolean): String {
return if (isUnique && !isPartial) {
override fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartialOrFunctional: Boolean): String {
return if (isUnique && !isPartialOrFunctional) {
"ALTER TABLE IF EXISTS ${identifierManager.quoteIfNecessary(tableName)} DROP CONSTRAINT IF EXISTS ${identifierManager.quoteIfNecessary(indexName)}"
} else {
"DROP INDEX IF EXISTS ${identifierManager.quoteIfNecessary(indexName)}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,8 @@ open class SQLServerDialect : VendorDialect(dialectName, SQLServerDataTypeProvid
return "CREATE $type INDEX $name ON $table $columns$filterCondition"
}

override fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartial: Boolean): String {
return if (isUnique && !isPartial) {
override fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartialOrFunctional: Boolean): String {
return if (isUnique && !isPartialOrFunctional) {
"ALTER TABLE ${identifierManager.quoteIfNecessary(tableName)} DROP CONSTRAINT IF EXISTS ${identifierManager.quoteIfNecessary(indexName)}"
} else {
"DROP INDEX IF EXISTS ${identifierManager.quoteIfNecessary(indexName)} ON ${identifierManager.quoteIfNecessary(tableName)}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ open class SQLiteDialect : VendorDialect(dialectName, SQLiteDataTypeProvider, SQ
}
}

override fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartial: Boolean): String {
override fun dropIndex(tableName: String, indexName: String, isUnique: Boolean, isPartialOrFunctional: Boolean): String {
return "DROP INDEX IF EXISTS ${identifierManager.quoteIfNecessary(indexName)}"
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,19 +204,35 @@ class JdbcDatabaseMetadataImpl(database: String, val metadata: DatabaseMetaData)
val tmpIndices = hashMapOf<Triple<String, Boolean, Op.TRUE?>, MutableList<String>>()

while (rs.next()) {
rs.getString("INDEX_NAME")?.let {
val column = transaction.db.identifierManager.quoteIdentifierWhenWrongCaseOrNecessary(rs.getString("COLUMN_NAME")!!)
val isUnique = !rs.getBoolean("NON_UNIQUE")
val isPartial = if (rs.getString("FILTER_CONDITION").isNullOrEmpty()) null else Op.TRUE
tmpIndices.getOrPut(Triple(it, isUnique, isPartial)) { arrayListOf() }.add(column)
rs.getString("INDEX_NAME")?.let { indexName ->
// if index is function-based, SQLite & MySQL return null column_name metadata
val columnNameMetadata = rs.getString("COLUMN_NAME") ?: when (currentDialect) {
is MysqlDialect, is SQLiteDialect -> "\"\""
else -> null
}
columnNameMetadata?.let { columnName ->
val column = transaction.db.identifierManager.quoteIdentifierWhenWrongCaseOrNecessary(columnName)
val isUnique = !rs.getBoolean("NON_UNIQUE")
val isPartial = if (rs.getString("FILTER_CONDITION").isNullOrEmpty()) null else Op.TRUE
tmpIndices.getOrPut(Triple(indexName, isUnique, isPartial)) { arrayListOf() }.add(column)
}
}
}
rs.close()
val tColumns = table.columns.associateBy { transaction.identity(it) }
tmpIndices.filterNot { it.key.first in pkNames }
.mapNotNull { (index, columns) ->
columns.distinct().mapNotNull { cn -> tColumns[cn] }.takeIf { c -> c.size == columns.size }
?.let { c -> Index(c, index.second, index.first, filterCondition = index.third) }
val (functionBased, columnBased) = columns.distinct().partition { cn -> tColumns[cn] == null }
columnBased.map { cn -> tColumns[cn]!! }
.takeIf { c -> c.size + functionBased.size == columns.size }
?.let { c ->
Index(
c, index.second, index.first,
filterCondition = index.third,
functions = functionBased.map { stringLiteral(it) }.ifEmpty { null },
functionsTable = if (functionBased.isNotEmpty()) table else null
)
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,19 +135,14 @@ class CreateIndexTests : DatabaseTestsBase() {
kotlin.test.assertEquals(totalIndexCount, 3, "Indexes expected to be created")
}

fun getIndices(): List<Index> {
db.dialect.resetCaches()
return currentDialect.existingIndices(partialIndexTable)[partialIndexTable].orEmpty()
}

val dropIndex = Index(columns = listOf(partialIndexTable.value, partialIndexTable.name), unique = false).dropStatement().first()
kotlin.test.assertTrue(dropIndex.startsWith("DROP INDEX "), "Unique partial index must be created and dropped as index")
val dropUniqueConstraint = Index(columns = listOf(partialIndexTable.anotherValue), unique = true).dropStatement().first()
kotlin.test.assertTrue(dropUniqueConstraint.startsWith("ALTER TABLE "), "Unique index must be created and dropped as constraint")

execInBatch(listOf(dropUniqueConstraint, dropIndex))

assertEquals(getIndices().size, 1)
assertEquals(getIndices(partialIndexTable).size, 1)
SchemaUtils.drop(partialIndexTable)
}
}
Expand Down Expand Up @@ -180,19 +175,14 @@ class CreateIndexTests : DatabaseTestsBase() {

assertEquals(2, tester.indices.count { it.filterCondition != null })

fun getIndices(): List<Index> {
db.dialect.resetCaches()
return currentDialect.existingIndices(tester)[tester].orEmpty()
}

var indices = getIndices()
var indices = getIndices(tester)
assertEquals(3, indices.size)

val uniqueWithPartial = Index(listOf(tester.team), true, "team_only_index", null, Op.TRUE).dropStatement().first()
val dropStatements = indices.map { it.dropStatement().first() }
expect(Unit) { execInBatch(dropStatements + uniqueWithPartial) }

indices = getIndices()
indices = getIndices(tester)
assertEquals(0, indices.size)

// test for non-unique partial index with type
Expand Down Expand Up @@ -255,11 +245,21 @@ class CreateIndexTests : DatabaseTestsBase() {
if (!isOldMySql()) {
SchemaUtils.createMissingTablesAndColumns()
assertTrue(tester.exists())
assertEquals(3, tester.indices.size)

val dropStatements = tester.indices.map { it.dropStatement().first() }
var indices = getIndices(tester)
assertEquals(3, indices.size)

val dropStatements = indices.map { it.dropStatement().first() }
expect(Unit) { execInBatch(dropStatements) }

indices = getIndices(tester)
assertEquals(0, indices.size)
}
}
}

private fun Transaction.getIndices(table: Table): List<Index> {
db.dialect.resetCaches()
return currentDialect.existingIndices(table)[table].orEmpty()
}
}

0 comments on commit e94514a

Please sign in to comment.