feat: EXPOSED-755 Introduce Exposed Migration Gradle Plugin #2451
Conversation
nomisRev
changed the title
Introduces the Exposed Migration Gradle Plugin to simplify SQL migration generation using Exposed table definitions. The plugin supports TestContainers for database testing, Flyway for integration, and a highly configurable extension. Includes unit tests for plugin functionality and task behavior.
nomisRev
force-pushed
the
migration-gradle-plugin
branch
from
834b96a to
6da76a3
Compare
…to generalize for adding future functionality.
bog-walk
changed the title
There was a problem hiding this comment.
Thanks so much for putting all the work into this @nomisRev ! Left some comments.
Two extra thoughts I had when reviewing:
- I saw that the original module name was changed to drop 'migration'. Is the plan to do the same for the package -> 'org.jetbrains.exposed.plugin'? Or swap the order?
- We might need to adjust the tests. Exposed is setup to run all tests on at least 1 db, so I believe that all your tests are being ignored right now by test tasks. And looking on TC, I can't find any. Depending on the approach, it most likely needs to be setup at minimum like
spring-transaction(to ensure all new tests run, but only once & only during H2_V2 task):testImplementation()needs to be swapped out- Then in
TestDbDsl.kt/TestDb, the new module needs to be filtered out from any task that isn't running H2_V2. Similar to howval ignoresSpringTestsis handled.
| // Complex ALTER TABLE with multiple constraints | ||
| assertEquals( | ||
| "ALTER_TABLE_USERS_ADD_CONSTRAINT_USERS_EMAIL_UNIQUE", | ||
| """ | ||
| ALTER TABLE users | ||
| ADD CONSTRAINT users_email_unique UNIQUE (email) | ||
| """.trimIndent().statementToFileName() | ||
| ) |
There was a problem hiding this comment.
Just mentioning that this assert is identical to the simple one in test ALTER TABLE statements above, in case you meant to include more complex constraints like the comment states.
| // CREATE INDEX statements | ||
| normalizedStatement.startsWith("CREATE INDEX", ignoreCase = true) -> { | ||
| val indexInfo = extractIndexInfo(normalizedStatement) | ||
| "CREATE_INDEX_$indexInfo" | ||
| } |
There was a problem hiding this comment.
Some edge cases to mention:
If the goal is to cover the base DDL syntax generated out-of-the-box by Exposed, there are 2 cases that won't be caught here, resulting in CREATE_STATEMENT_hash names.
- Unique index + function/expression generates
CREATE UNIQUE INDEXsyntax in supported db (PostgreSQL, Oracle, MySQL, SQL Server) - Typed index generates for example
CREATE CLUSTERED INDEX(SQL Server only)
I can of course log an issue to cover these cases later or add as-needed. Just let me know
| // CREATE SEQUENCE statements | ||
| normalizedStatement.startsWith("CREATE SEQUENCE", ignoreCase = true) -> { | ||
| val sequenceName = extractSequenceName(normalizedStatement) | ||
| "CREATE_SEQUENCE_$sequenceName" | ||
| } |
There was a problem hiding this comment.
Another (maybe) edge case I was wondering about:
What if the generated Exposed migration statement(s) include creating a table that requires a sequence to be created first? So CREATE TABLE statement would be preceded by CREATE SEQUENCE in db that support it.
Imo the better descriptive migration name would still be CREATE_TABLE_name, but script will instead be named CREATE_SEQUENCE_name because it comes first.
I get that we can't cover every potential case and I'm not even sure if this may be common enough to warrant a special look-forward to catch the start of the (potential) second statement. Just thinking out loud.
| private fun URLClassLoader.getClassesInPackage(packageName: String): Sequence<KClass<*>> = | ||
| getResources(packageName.replace('.', '/')).asSequence().flatMap { resource -> |
There was a problem hiding this comment.
This made me think of our current configuration options in the spring boot starter database initializer, which potentially accepts a package to exclude (when searching for table objects). Do you think there could be a case when a user might prefer a way to exclude/ignore a specific (sub)package from generating migrations? And is this something you think we could feasibly cover later on if requested?
| // Check for VX__ format | ||
| val matcher = versionPattern.matcher(fileName) | ||
| if (matcher.matches()) { | ||
| hasXYFormat = false |
There was a problem hiding this comment.
Could you please confirm that I understand correctly the logic for when VX_Y__ format is used or not used?
Is it correct that this logic is based on the assumption that the user has (potentially) existing migration scripts that are all using the same format?
But that if there are different formats, the VX__ format will take priority (even if only 1 matches and it isn't the last)?
So if the existing scripts look like this:
- V1__text.sql
- V2__text.sql
- V3_0__text.sql
- V3_1__text.sqlThe next script will be V4__text.sql?
| require( | ||
| parameters.databaseUrl == null || | ||
| parameters.databaseUser == null || | ||
| parameters.databasePassword == null | ||
| ) { | ||
| "Database properties (url, user, password) must be provided when not using TestContainers" | ||
| } |
There was a problem hiding this comment.
I think this probably is meant to be an if block or condition needs to change to != null && ...?
Introduces the Exposed Migration Gradle Plugin to simplify SQL migration generation using Exposed table definitions. The plugin supports TestContainers for database testing, Flyway for integration, and a highly configurable extension. Includes unit tests for plugin functionality and task behavior.
Description
Summary of the change: Introduces the Exposed Migration Gradle Plugin to simplify SQL migration generation using Exposed table definitions. The plugin supports TestContainers for database testing, Flyway for integration, and a highly configurable extension. Includes unit tests for plugin functionality and task behavior.
Detailed description:
Type of Change
Please mark the relevant options with an "X":
Affected databases:
Checklist
Related Issues
EXPOSED-755