refactor: EXPOSED-561 Restructure code in MigrationUtils and SchemaUtils to avoid calling currentDialect.tableColumns(*tables) in MigrationUtils.statementsRequiredForDatabaseMigration twice

[joc-a]

Description

Detailed description:

• What:
  Restructure code in MigrationUtils and SchemaUtils.
• Why:
  To avoid calling currentDialect.tableColumns(*tables) in MigrationUtils.statementsRequiredForDatabaseMigration twice.
• How:
  SchemaUtils and MigrationUtils reside in separate modules and need to share non-public functions, so using a common abstract base class with protected functions in the exposed-core module is the most effective and clean solution. For the functions addMissingColumnsStatements and dropUnmappedColumnsStatements, two corresponding private functions with a different signature were created, with the difference being that the new functions expect existingTablesColumns to be passed as a parameter. Since the new private addMissingColumnsStatements function is common to both SchemaUtils and MigrationUtils, it was placed in BaseSchemaUtils, and the same thing was done for the logTimeSpent function.

---

Type of Change

Please mark the relevant options with an "X":

• Bug fix
• New feature
• Documentation update
• Code enhancement

Updates/remove existing public API methods:

• Is breaking change

Affected databases:

• MariaDB
• Mysql5
• Mysql8
• Oracle
• Postgres
• SqlServer
• H2
• SQLite

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

[obabichevjb]

I would say that in terms of OOP (at least from my current understanding of MigrationUtils's purpose) MigrationUtils should not extend SchemaUtils (BaseSchemaUtils) but should use it, because MigrationUtils is more abstract than SchemaUtils (SchemaUtils is responsible for manipulating with single db objects like tables, but MigrationUtils are supposed to move the whole DB from one state to another).

And as I can see MigrationUtils overrides nothing from base class.

[obabichevjb]

Not a problem of that particular PR, but this is a public method, and there is only one test that check that function returns empty list. Probably it would be nice to add one more test, that check for non empty result of that function)

[joc-a]

That case is tested, but not by directly calling dropUnmappedColumnsStatements, in testDropUnmappedColumns.

[obabichevjb]

And do we have any requests for such functionality? Looks like it's already used in statementsRequiredForDatabaseMigration that should returns All statements for migration. Probably, the method that checks columns of one (or multiple) tables for deleting could be a part of SchemaUtils. But it's discussable, just some thoughts on how it could be.

[obabichevjb]

Also not a problem of that PR, and can't say I finally understand how users will use it, but this class has a method generateMigrationScript that accepts the list of tables as an argument.

But if you want to create a script, and has hundreds of tables, it could be painful to collect all of them into collection and pass here.

If I use a migration tool, I'd expect that it should search for table objects automatically. As a variant for future improvements we could add one more method that accepts package, and searches for all the table objects (with reflection) within this package.

[joc-a]

Yes, this is already an improvement that I have on my to-do list.

[obabichevjb]

There is one another idea of how common part could be extracted.

At the current moment the whole SchemaUtils looks a bit overloaded. It has methods to generate DDL, and has methods to execute commands to manipulate DB.

We could try to split it into the way to extract DDL generation part into another class, let's call it for now DDLUtils; and keep inside SchemaUtils responsibility to execute these DDLs on the database.

In this case we will have common part with DDL statements generation, and two consumers of that class. SchemaUtils that directly performs it on the DB, and MigrationUtils that prepare it as a migration scripts.

[joc-a]

@obabichevjb In that case, how would we make the following function visible only to SchemaUtils and MigrationUtils? We don't want it to be public.

fun addMissingColumnsStatements(vararg tables: Table, existingTablesColumns: Map<Table, List<ColumnMetadata>>, withLogs: Boolean = true): List<String>