Skip to content

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

Open
Chantal Loncle (bog-walk) wants to merge 1 commit into
mainfrom
bog-walk/fix-plugin-duplicate-references
Open

fix(plugin): EXPOSED-1033 Child tables that reference parent with index generate duplicate ALTER statements#2919
Chantal Loncle (bog-walk) wants to merge 1 commit into
mainfrom
bog-walk/fix-plugin-duplicate-references

Conversation

@bog-walk

Copy link
Copy Markdown
Member

Description

Summary of the change: Check generated SQL from task generateMigrations to ensure duplicate statements are not written across multiple migration scripts.

Detailed description:

  • Why: The plugin task generateMigrations calls MigrationUtils.statementsRequiredForDatabaseMigration() for each detected table, so it can create a new migration script for that table. If this method was called on a group of tables that reference each other, it would ensure that no duplicate statements are generated. Since this method is instead called once per table, a child table generates all SQL needed to create the table it references, as well as itself. This causes no issue for basic tables, as CREATE TABLE IF NOT EXISTS can be called repeatedly. But if the table holds something like an index constraint, the child script will attempt to call the same parent's ALTER, resulting in constraint already exists issues.

A side effect of this is naming, as the script takes its descriptive name from the first compatible SQL statement. So the script may end up having a similar name distinguishable only by versioning. Take for example the following tables:

object TableA : IntIdTable("table_a") {
    val columnA = varchar("columnA", 255).uniqueIndex()
}

object TableB : IntIdTable("table_b") {
    val columnB = varchar("columnB", 255).uniqueIndex()
}

object TableC : IntIdTable("table_c") {
    val columnC = varchar("columnC", 255)
    val referenceA = reference("columnA", TableA)
    val referenceB = reference("columnB", TableB)
}

Running the task with default settings would generate the following files:

V20260908203419__CREATE_TABLE_TABLE_A.sql <-- create + alter for table_a ✅

V20260908203419.1__CREATE_TABLE_TABLE_B.sql <-- create + alter for table_b ✅

V20260908203419.2__CREATE_TABLE_TABLE_A.sql <-- create + alter for table_a + table_b + table_c ❌

  • How: Ideally, if only TestContainers were being used, generated SQL could be applied before each call to the migration method, ensuring steadfast accuracy for each new migration script. The solution needs to account for the immutable option when the real database is being used. Rather than attempt to parse/separate statements from a single call to the method, each call compares its output to a temporary store of previous outputs, to remove duplicates.

Type of Change

Please mark the relevant options with an "X":

  • Bug fix

Affected databases:

  • All

Checklist

  • Unit tests are in place
  • The build is green (including the Detekt check)
  • All public methods affected by my PR has up to date API docs
  • Documentation for my change is up to date

Related Issues

EXPOSED-1033

)

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

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.

.getClassesInPackage(config.tablesPackage)
.mapNotNull { it.tableOrNull() }
val sortedTables = SchemaUtils.sortTablesByReferences(foundTables.toList())
val generatedSQL = mutableSetOf<String>()

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant