Skip to content
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.

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

feat: EXPOSED-733 Detect column type change for migrations in H2 #2419

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joc-a
Copy link
Collaborator

@joc-a joc-a commented Feb 21, 2025

Description

Detailed description:

  • What:
    Column type changes in H2 are now detected when generating database migration statements.
  • How:
    ColumnMetadata now has a new property, sqlType. To get this type, the column types pre-fetched (if the database supports that) in the extractColumns function in JdbcDatabaseMetadataImpl. If pre-fetching is not supported, the new getColumnType function queries the database for information about the column (future PRs).
    Some SQL types are mapped to a different type in the database than one might expect, so normalizing the type name is done in those cases. Let me know if you think the normalizedColumnType function in H2 should be moved to DatabaseDialect and turned into an open function that users can override.
    When checking for column differences, the type of the existing column is compared with the type of the Exposed table's column using the new function areEquivalentColumnTypes.

Next up: PostgreSQL 🚀


Type of Change

Please mark the relevant options with an "X":

  • Bug fix
  • New feature
  • Documentation update

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

@joc-a joc-a force-pushed the joc/exposed-733-H2 branch 2 times, most recently from f223c7d to 4ccfeb7 Compare February 24, 2025 15:49
@joc-a joc-a force-pushed the joc/exposed-733-H2 branch 3 times, most recently from 971360e to fefb2e8 Compare February 25, 2025 10:01
@@ -373,9 +376,9 @@ object SchemaUtils {

val incorrectCaseSensitiveName = existingCol.name.inProperCase() != col.nameUnquoted().inProperCase()

val incorrectSizeOrScale = isIncorrectSizeOrScale(existingCol, columnType)
val incorrectSizeOrScale = if (incorrectType) false else isIncorrectSizeOrScale(existingCol, columnType)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The size and scale are only compared if the types are confirmed to be equivalent because the size and scale differences are taken into account when comparing the types. This might change later as we refine this feature.

@joc-a joc-a marked this pull request as ready for review February 25, 2025 10:46
@@ -6,6 +6,8 @@ package org.jetbrains.exposed.sql
data class ColumnDiff(
/** Whether there is a mismatch between nullability of the existing column and the defined column. */
val nullability: Boolean,
/** Whether there is a mismatch between type of the existing column and the defined column. */
val type: Boolean,
Copy link
Member

Choose a reason for hiding this comment

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

should we store old and new types directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure why that's necessary. Could you please elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

The ColumnDiff class represents the difference between definition and metadata. It would be useful to have both types here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ColumnDiff class, as it is now, represents whether there is a difference between the Exposed definition and the metadata. It answers a yes or no question. All the properties in the class are of type Boolean. The way it works is that we check if any of the properties are true, and in that case the modifyColumn statement is invoked, which contains the ALTER statement that modifies the column to have the required type if needed. If I add both types to this class, they would actually not be used anywhere, because the comparison between the two types is already done a step before we create an instance of this class. This class is simply a result of that comparison. This is the approach we currently have for all the differences that we currently detect for columns (nullability, defaults, auto-increment, etc.). Please let me know if you had envisioned an entirely different approach to the one we currently have.

@joc-a joc-a force-pushed the joc/exposed-733-H2 branch from fefb2e8 to af17501 Compare February 28, 2025 11:06
@joc-a joc-a requested a review from e5l February 28, 2025 13:19
@joc-a joc-a force-pushed the joc/exposed-733-H2 branch from af17501 to 63f83fc Compare March 5, 2025 11:41
@joc-a joc-a requested a review from e5l March 5, 2025 11:47
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.

3 participants