fix: Apply inProperCase() to CHECK constraint names#2826
fix: Apply inProperCase() to CHECK constraint names#2826Rashin Arab (rasharab) wants to merge 1 commit into
Conversation
0e33c0a to
46715d8
Compare
CHECK constraint names generated for unsigned/signed integer columns use tableNameWithSchemaSanitized which preserves the raw Kotlin class name casing. On PostgreSQL, unquoted identifiers are folded to lowercase on storage, so mixed-case constraint names in DDL don't match what pg_constraint.conname stores. Apply .inProperCase() to the CHECK constraint name in CheckConstraint.from(), matching the pattern already used for FK constraint names and Index names. Add test with a mixed-case table name (MixedCaseUIntTable) to verify the CHECK constraint name is properly cased in DDL output. Fixes JetBrains#2825
46715d8 to
51a3bd9
Compare
|
Chantal Loncle (@bog-walk) Can you take a look and see if this is okay (when you have time)? |
There was a problem hiding this comment.
Hi Rashin Arab (@rasharab) and thanks for catching this naming convention inconsistency. It's looking good to me so far. I was initially concerned that, while this may not be a breaking API change, that it might technically count as a behavioural breaking change. So I'm running regression tests, which so far seem to all be passing.
If you are able to, please add something like the following, to simulate a case where the user previously created a database table using the old check constraint name. In both exposed-migration-jdbc and exposed-migration-r2dbc DatabaseMigrationTests.kt. To ensure that this naming change will not trigger migration statements, for example:
@OptIn(InternalApi::class)
@Test
fun testUnsignedCheckConstraintNameCaseRegression() {
withDb(excludeSettings = TestDB.ALL_MYSQL_MARIADB) {
val previousConstraintName = "chk_MixedCaseUInt_unsigned_integer_unsignedCol"
val currentDdl = MixedCaseUIntTable.createStatement().single()
val currentCheckClause = currentDdl.substringAfter("CHECK ")
val previousDdl = "${currentDdl.substringBefore(" CONSTRAINT ")} CONSTRAINT $previousConstraintName CHECK $currentCheckClause"
try {
exec(previousDdl)
commit()
val rerun = MigrationUtils.statementsRequiredForDatabaseMigration(MixedCaseUIntTable, withLogs = false)
assertTrue(rerun.isEmpty())
} finally {
SchemaUtils.drop(MixedCaseUIntTable)
}
}
}Lastly, unfortunately our GH <-> TC (CI/CD) integration is not currently working, so there are some failing tests that are not visible to you as part of the PR workflow. These are any tests that test directly for generated SQL using hard-coded strings. Please do a project search for the following tests and add .inProperCase() to the plain string constraint names:
testDefaults01()[the datetime ones across multiple test modules]testTimestampWithTimeZoneDefaults()testCustomCheckConstraintName()<-- feel free to add@Suppress("MaximumLineLength")to avoid any detekt issues here
Edit: It's actually many many failing tests. Basically anywhere that you find a hard-coded " CONSTRAINT " in a test assert, would need to have the name chained with .inProperCase().
Let me know if anything else doesn't work or is causing problems for you.
| withTables(MixedCaseUIntTable) { | ||
| if (currentDialectTest is MysqlDialect) return@withTables // MySQL uses UNSIGNED type instead of CHECK |
There was a problem hiding this comment.
Please simplify this with:
withTables(excludeSettings = TestDB.ALL_MYSQL_MARIADB, MixedCaseUIntTable) {
val checkConstraints = MixedCaseUIntTable.checkConstraints()
...
}Please also copy this test over to the same file/location in exposed-r2dbc-tests.
Problem
CHECK constraint names generated for unsigned/signed integer columns (e.g.
uinteger(),ushort(),ubyte(),ulong()) usetableNameWithSchemaSanitized, which preserves the raw Kotlin class name casing. On PostgreSQL, unquoted identifiers are folded to lowercase on storage, so mixed-case constraint names in DDL don't match whatpg_constraint.connamestores.For example,
uinteger("onboardingStates")onOrgMemberModelgenerates:but PostgreSQL stores it as
chk_orgmembermodel_unsigned_integer_onboardingstates.This causes problems for any migration tooling that checks
pg_constraint.connamefor idempotentIF NOT EXISTSguards — the mixed-case lookup fails to find the existing constraint.Root Cause
Other constraint types already handle casing correctly:
ForeignKeyConstraint.fkName, line 118-123): appliescutIfNecessaryAndQuote(...).inProperCase()Index.indexName, line 280-291): appliesbuildString { ... }.inProperCase()CheckConstraint.from, line 247): only appliescutIfNecessaryAndQuote(name)— missing.inProperCase()The CHECK constraint name is built from
generatedUnsignedCheckPrefix/generatedSignedCheckPrefix, which usetableNameWithSchemaSanitized(rawtableName.unquoted()) without any casing normalization.Fix
Apply
.inProperCase()to the CHECK constraint name inCheckConstraint.from(), matching the pattern already used for FK and Index constraints:Also added a test with a mixed-case table name (
MixedCaseUIntTable) that verifies the CHECK constraint name usesinProperCase(). Test passes on H2 (uppercase) and will pass on PostgreSQL (lowercase).Fixes #2825