Skip to content

fix(plugin): EXPOSED-1033 Child tables that reference parent with index generate duplicate ALTER statements #2919

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

Open
Chantal Loncle (bog-walk) wants to merge 1 commit into
base: main
Choose a base branch
from
+40 −2
Open

fix(plugin): EXPOSED-1033 Child tables that reference parent with index generate duplicate ALTER statements #2919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions ...core/src/main/kotlin/org/jetbrains/exposed/v1/plugin/core/migration/MigrationGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ class MigrationGenerator(
.getClassesInPackage(config.tablesPackage)
.mapNotNull { it.tableOrNull() }
val sortedTables = SchemaUtils.sortTablesByReferences(foundTables.toList())
val generatedSQL = mutableSetOf<String>()

@bog-walk Chantal Loncle (bog-walk) Sep 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The most ideal scenario would be to apply the output statements below to a TestContainer to ensure the next method call is run against an accurate state database.
This solution covers the case when containers are not being used, by storing statements as they come for a future check. It might not be the most performant if there are many tables to check. But it should avoid more edge cases compared to calling the method once for all tables and attempting to separate the output logically (not all scripts will start with CREATE for example).
An alternative could be to attempt to create a variant of sortTablesByReferences() that returns grouped tables (that reference each other) to be handled together, for a potentially smaller temp store.

sortedTables.mapIndexedNotNull { index, table ->
transaction(database) {
addLogger(GeneratorSqlLogger())
Expand All @@ -120,12 +121,14 @@ class MigrationGenerator(
withLogs = config.debug,
)
if (statements.isNotEmpty()) {
val description = statements.first()
val newSQL = statements.subtract(generatedSQL)
generatedSQL.addAll(newSQL)
val description = newSQL.first()
.statementToFileDescription(config.useUpperCaseDescription)
val version = versionGenerator(index - ignored)
val fileName = "$version$description${config.fileExtension}"
val file = File(migrationsDirectory, fileName)
file.writeText(statements.joinToString(";\n", postfix = ";"))
file.writeText(newSQL.joinToString(";\n", postfix = ";"))
fileName
} else {
ignored++
Expand Down
35 changes: 35 additions & 0 deletions ...g/jetbrains/exposed/v1/plugin/core/migration/collision/MigrationGeneratorCollisionTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import kotlin.test.assertEquals

object Parent : Table("issue_2897_parent") {
val id = integer("id")
val label = varchar("label", 255).uniqueIndex()
override val primaryKey = PrimaryKey(id)
}

Expand Down Expand Up @@ -59,6 +60,7 @@ class MigrationGeneratorCollisionTest {
)

val generated = generator.generate()
val generatedFilenames = generated.map { it.substringAfter("__").substringBeforeLast('.') }

@bog-walk Chantal Loncle (bog-walk) Sep 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This test only covered the number of generated files and their SQL caused no issues because it's ok to execute CREATE TABLE IF NOT EXISTS multiple times. So multiple scripts held the parent table name description, as well as duplicate CREATE statements. This now checks that the lack of duplicates generates unique name descriptions.

val expectedTables = setOf(
Parent.tableName,
ChildOne.tableName,
Expand All @@ -74,6 +76,39 @@ class MigrationGeneratorCollisionTest {

assertEquals(expectedTables.size, generated.size, "All table migrations must be preserved: $generated")
assertEquals(expectedTables.size, generatedFiles.size)
assertEquals(expectedTables.size, generatedFilenames.distinct().size)
assertEquals(expectedTables, createdTables, "All table DDL must be preserved")
}

@Test
fun testDependentTablesDoNotDuplicateParentConstraints() {
val generator = MigrationGenerator(
config = MigrationConfig(
tablesPackage = this::class.java.packageName,
classpathUrls = listOf(this::class.java.protectionDomain.codeSource.location),
fileDirectory = migrationsDirectory,
fileVersionFormat = VersionFormat.TIMESTAMP_WITHOUT_SECONDS,
databaseUrl = "jdbc:h2:mem:${UUID.randomUUID()}",
databaseUser = "",
databasePassword = "",
),
logger = object : MigrationLogger {
override fun lifecycle(message: String) = Unit
override fun debug(message: String) = Unit
override val isDebugEnabled: Boolean = false
},
)

generator.generate()

val expectedTablesWithIndex = setOf(Parent.tableName)
val generatedFiles = migrationsDirectory.listFiles().orEmpty()
val alterTableRegex = Regex("""ALTER TABLE\s+"?([\w.]+)"?\s+ADD CONSTRAINT""", RegexOption.IGNORE_CASE)
val alteredTables = generatedFiles
.flatMap { file -> alterTableRegex.findAll(file.readText()).map { it.groupValues[1] }.toList() }
.map { it.substringAfterLast('.').lowercase() }

assertEquals(expectedTablesWithIndex.size, alteredTables.size, "Only files that generates table with index should contain its ALTER")
assertEquals(expectedTablesWithIndex, alteredTables.toSet())
}
}
Loading