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

fix: EXPOSED-415 SchemaUtils incorrectly generates ALTER statements f… #2136

Merged
merged 4 commits into from
Jul 10, 2024
Merged

fix: EXPOSED-415 SchemaUtils incorrectly generates ALTER statements f… #2136

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ecc8a94
fix: EXPOSED-415 SchemaUtils incorrectly generates ALTER statements f…
obabichevjb Jun 24, 2024
a363d9f
Review issues: fix binary colum default values and ddl generation
obabichevjb Jun 28, 2024
fd7ae29
Revert binary column
obabichevjb Jun 28, 2024
dba3d98
Review issues again but without binary column fix
obabichevjb Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/SchemaUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,10 +316,11 @@ object SchemaUtils {
val incorrectNullability = existingCol.nullable != columnType.nullable
// Exposed doesn't support changing sequences on columns
val incorrectAutoInc = existingCol.autoIncrement != columnType.isAutoInc && col.autoIncColumnType?.autoincSeq == null
val incorrectDefaults = existingCol.defaultDbValue != col.dbDefaultValue?.let {
dataTypeProvider.dbDefaultToString(col, it)
}

val incorrectDefaults = isIncorrectDefault(dataTypeProvider, existingCol, col)

val incorrectCaseSensitiveName = existingCol.name.inProperCase() != col.nameUnquoted().inProperCase()

ColumnDiff(incorrectNullability, incorrectAutoInc, incorrectDefaults, incorrectCaseSensitiveName)
}.filterValues { it.hasDifferences() }

Expand All @@ -343,6 +344,30 @@ object SchemaUtils {
return statements
}

/**
* For DDL purposes we do not segregate the cases when the default value was not specified, and when it
* was explicitly set to `null`.
*/
private fun isIncorrectDefault(dataTypeProvider: DataTypeProvider, columnMeta: ColumnMetadata, column: Column<*>): Boolean {
val isExistingColumnDefaultNull = columnMeta.defaultDbValue == null
val isDefinedColumnDefaultNull = column.dbDefaultValue == null ||
(column.dbDefaultValue is LiteralOp<*> && (column.dbDefaultValue as? LiteralOp<*>)?.value == null)

return when {
// Bot values are null-like, no DDL update is needed
obabichevjb marked this conversation as resolved.
Show resolved Hide resolved
isExistingColumnDefaultNull && isDefinedColumnDefaultNull -> false
// Only one of the values is null-like, DDL update is needed
isExistingColumnDefaultNull != isDefinedColumnDefaultNull -> true

else -> {
val columnDefaultValue = column.dbDefaultValue?.let {
dataTypeProvider.dbDefaultToString(column, it)
}
columnMeta.defaultDbValue != columnDefaultValue
}
}
}

private fun addMissingColumnConstraints(vararg tables: Table, withLogs: Boolean): List<String> {
val existingColumnConstraint = logTimeSpent("Extracting column constraints", withLogs) {
currentDialect.columnConstraints(*tables)
Expand Down
63 changes: 47 additions & 16 deletions ...dbc/src/main/kotlin/org/jetbrains/exposed/sql/statements/jdbc/JdbcDatabaseMetadataImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import org.jetbrains.exposed.sql.statements.api.ExposedDatabaseMetadata
import org.jetbrains.exposed.sql.statements.api.IdentifierManagerApi
import org.jetbrains.exposed.sql.transactions.TransactionManager
import org.jetbrains.exposed.sql.vendors.*
import org.jetbrains.exposed.sql.vendors.H2Dialect.H2CompatibilityMode
import java.math.BigDecimal
import java.sql.DatabaseMetaData
import java.sql.ResultSet
Expand Down Expand Up @@ -179,27 +178,59 @@ class JdbcDatabaseMetadataImpl(database: String, val metadata: DatabaseMetaData)
)
}

private fun sanitizedDefault(defaultValue: String): String {
/**
* Here is the table of default values which are returned from the column `"COLUMN_DEF"` depending on how it was configured:
*
* - Not set: `varchar("any", 128).nullable()`
* - Set null: `varchar("any", 128).nullable().default(null)`
* - Set "NULL": `varchar("any", 128).nullable().default("NULL")`
* ```
* DB Not set Set null Set "NULL"
* SqlServer null "(NULL)" "('NULL')"
* SQLite null "NULL" "'NULL'"
* Postgres null "NULL::character varying" "'NULL'::character varying"
* PostgresNG null "NULL::character varying" "'NULL'::character varying"
* Oracle null "NULL " "'NULL' "
obabichevjb marked this conversation as resolved.
Show resolved Hide resolved
* MySql5 null null "NULL"
* MySql8 null null "NULL"
* MariaDB3 "NULL" "NULL" "'NULL'"
* MariaDB2 "NULL" "NULL" "'NULL'"
* H2V1 null "NULL" "'NULL'"
* H2V1 (MySql) null "NULL" "'NULL'"
* H2V2 null "NULL" "'NULL'"
* H2V2 (MySql) null "NULL" "'NULL'"
* H2V2 (MariaDB) null "NULL" "'NULL'"
* H2V2 (PSQL) null "NULL" "'NULL'"
* H2V2 (Oracle) null "NULL" "'NULL'"
* H2V2 (SqlServer) null "NULL" "'NULL'"
* ```
* According to this table there is no simple rule of what is the default value. It should be checked
* for each DB (or groups of DBs) specifically.
* In the case of MySql and MariaDB it's also not possible to say whether was default value skipped or
* explicitly set to `null`.
*
* @return `null` - if the value was set to `null` or not configured. `defaultValue` in other case.
*/
private fun sanitizedDefault(defaultValue: String): String? {
val dialect = currentDialect
val h2Mode = dialect.h2Mode
return when {
dialect is SQLServerDialect -> defaultValue.trim('(', ')', '\'')
dialect is OracleDialect || h2Mode == H2CompatibilityMode.Oracle -> defaultValue.trim().let {
if (it.startsWith('\'') && it.endsWith('\'')) it.trim('\'') else it
dialect is SQLServerDialect -> defaultValue.trim('(', ')').extractNullAndStringFromDefaultValue()
dialect is MariaDBDialect -> when (defaultValue) {
"NULL" -> null
else -> defaultValue.extractNullAndStringFromDefaultValue()
}
dialect is MysqlDialect || h2Mode == H2CompatibilityMode.MySQL || h2Mode == H2CompatibilityMode.MariaDB -> defaultValue.substringAfter(
"b'"
).trim('\'')
Comment on lines -190 to -192
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we have good enough test coverage to ok removing some of these branches. The h2Modes are probably not going to be an issue, but removing MariaDB from this substring check may cause problems in a few users.

One example I can think of:
In both MySQL and MariaDB, Exposed takes the opinion to store boolean values in BOOLEAN column type instead of BIT. But let's say a user has an existing table already in the DB that uses BIT (or chooses to create one themselves), and they are using their own custom bool() to register and work with this BIT column using Kotlin booleans.
Attempting something like the following would no longer show the same behavior in both DB:

// represents the existing table in db
exec("CREATE TABLE IF NOT EXISTS tester (bool BIT DEFAULT b'0' NOT NULL)")

// represents the identical Exposed definition
object TestTable : Table("tester") {
    val bool = bool("bool").default(false)
}

// when calling SchemaUtils.createMissingTables....
// MySQL - correctly does not generate ALTER statement
// MariaDB - incorrectly does generate ALTER statement

I'd suggest a third branch (if value starts with b' or something) in the new MariaDB logic that does what the original logic did (i.e. what the new MySQL branch does now). Not sure if the mysql H2 modes would also be affected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added check of 'b for MariaDB also, but without proper test actually.

But what I've found during testing of this is that binary column is almost untested and quite broken.
I added test to check binary column ddl generation, and insert/reading values (with binary literal also, and the binary literal actually). So now it looks better, but PR become a bit bigger..


dialect is PostgreSQLDialect || h2Mode == H2CompatibilityMode.PostgreSQL -> when {
defaultValue.startsWith('\'') && defaultValue.endsWith('\'') -> defaultValue.trim('\'')
else -> defaultValue
}

else -> defaultValue.trim('\'')
dialect is OracleDialect -> defaultValue.trim().extractNullAndStringFromDefaultValue()
dialect is MysqlDialect -> defaultValue.substringAfter("b'").trim('\'')
else -> defaultValue.extractNullAndStringFromDefaultValue()
}
}

private fun String.extractNullAndStringFromDefaultValue() = when {
this.startsWith("NULL") -> null
this.startsWith('\'') && this.endsWith('\'') -> this.trim('\'')
else -> this
}

private val existingIndicesCache = HashMap<Table, List<Index>>()

@Suppress("CyclomaticComplexMethod")
Expand Down
71 changes: 71 additions & 0 deletions ...tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/ddl/ColumnDefinitionTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,4 +186,75 @@ class ColumnDefinitionTests : DatabaseTestsBase() {
result2.close()
}
}

@Test
fun testNoChangesOnCreateMissingNullableColumns() {
obabichevjb marked this conversation as resolved.
Show resolved Hide resolved
val testerWithDefaults = object : Table("tester") {
val defaultNullNumber = integer("default_null_number").nullable().default(null)
val defaultNullWord = varchar("default_null_word", 8).nullable().default(null)
val nullNumber = integer("null_number").nullable()
val nullWord = varchar("null_word", 8).nullable()
val defaultNumber = integer("default_number").default(999).nullable()
val defaultWord = varchar("default_word", 8).default("Hello").nullable()
}

val testerWithoutDefaults = object : Table("tester") {
val defaultNullNumber = integer("default_null_number").nullable()
val defaultNullWord = varchar("default_null_word", 8).nullable()
val nullNumber = integer("null_number").nullable()
val nullWord = varchar("null_word", 8).nullable()
val defaultNumber = integer("default_number").default(999).nullable()
val defaultWord = varchar("default_word", 8).default("Hello").nullable()
}

listOf(
testerWithDefaults to testerWithDefaults,
testerWithDefaults to testerWithoutDefaults,
testerWithoutDefaults to testerWithDefaults,
testerWithoutDefaults to testerWithoutDefaults
).forEach { (existingTable, definedTable) ->
withTables(existingTable) {
SchemaUtils.statementsRequiredToActualizeScheme(definedTable).also {
assertTrue(it.isEmpty())
}
}
}
}

@Test
fun testChangesOnCreateMissingNullableColumns() {
val testerWithDefaults = object : Table("tester") {
val defaultNullString = varchar("default_null_string", 8).nullable().default("NULL")
val defaultNumber = integer("default_number").default(999).nullable()
val defaultWord = varchar("default_word", 8).default("Hello").nullable()
}

val testerWithoutDefaults = object : Table("tester") {
val defaultNullString = varchar("default_null_string", 8).nullable()
val defaultNumber = integer("default_number").nullable()
val defaultWord = varchar("default_word", 8).nullable()
}

listOf(
testerWithDefaults to testerWithoutDefaults,
testerWithoutDefaults to testerWithDefaults,
).forEach { (existingTable, definedTable) ->
withTables(excludeSettings = listOf(TestDB.SQLITE), existingTable) {
SchemaUtils.statementsRequiredToActualizeScheme(definedTable).also {
assertTrue(it.isNotEmpty())
}
}
}

listOf(
testerWithDefaults to testerWithDefaults,
testerWithoutDefaults to testerWithoutDefaults
).forEach { (existingTable, definedTable) ->
withTables(excludeSettings = listOf(TestDB.SQLITE), existingTable) {
SchemaUtils.statementsRequiredToActualizeScheme(definedTable).also {
assertTrue(it.isEmpty())
}
}
}
}
}
Loading