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

fix: EXPOSED-283 Date/Time defaults falsely trigger ALTER statements #1981

Merged
merged 5 commits into from
Feb 14, 2024

Conversation

joc-a
Copy link
Collaborator

@joc-a joc-a commented Feb 1, 2024

⚠️ Please review commit by commit. I want to rebase and merge instead of squash and merge.

Date/Time column type defaults false trigger ALTER statements because of a mismatch when comparing the default value obtained from the metadata (which Exposed sanitizes) with the formatted default value of the column type. The following are the mismatches that were fixed in this PR.

Mismatches for timestamp column type:

  1. PostgreSQL and PostgresSQLNG

Default value from metadata: '2024-02-07 12:42:26.251'::timestamp without time zone
Default value formatted by Exposed: '2024-02-07T12:42:26.251'

  1. Oracle and H2 Oracle

Default value from metadata: TIMESTAMP '2024-02-07 12:42:27.453
Default value formatted by Exposed: TIMESTAMP '2024-02-07 12:42:27.453'

  1. SQL Server

Default value from metadata: 2024-02-07T12:42:25.31
Default value formatted by Exposed: '2024-02-07T12:42:25.31'

  1. MySQL and MariaDB

Default value from metadata: 2024-02-07 12:42:24.508000
Default value formatted by Exposed: '2024-02-07T12:42:24.508'

  1. H2 (except for H2 Oracle)

Default value from metadata: 2024-02-07T12:42:23.494
Default value formatted by Exposed: '2024-02-07T12:42:23.494'

Mismatches for timestampWithTimeZone column type:

  1. PostgreSQL and PostgresSQLNG

Default value from metadata: '2024-02-07 14:55:28.1+00'::timestamp with time zone
Default value formatted by Exposed: '2024-02-07T23:55:28.100+09:00'

  1. Oracle

Default value from metadata: TIMESTAMP '2024-02-07 23:55:29.776000 +09:00
Default value formatted by Exposed: TIMESTAMP '2024-02-07 23:55:29.776000 +09:00'

  1. H2 Oracle

Default value from metadata: TIMESTAMP '2024-02-07 14:55:24.9
Default value formatted by Exposed: TIMESTAMP '2024-02-07T23:55:24.900+09:00'

  1. SQL Server and H2 (except for H2 Oracle)

Default value from metadata: 2024-02-07T23:55:26.812+09:00
Default value formatted by Exposed: '2024-02-07T23:55:26.812+09:00'

  1. MySQL 8

Default value from metadata: 2024-02-07 14:55:25.749000
Default value formatted by Exposed: '2024-02-07 23:55:25.749000+09:00'

Mismatches for datetime column type:

  1. PostgreSQL and PostgresSQLNG

Default value from metadata: '2024-02-08 00:02:34.935'::timestamp without time zone
Default value formatted by Exposed: '2024-02-08T00:02:34.935'

  1. Oracle and H2 Oracle

Default value from metadata: TIMESTAMP '2024-02-08 00:02:36.459
Default value formatted by Exposed: TIMESTAMP '2024-02-08 00:02:36.459'

  1. MySQL and MariaDB

Default value from metadata: 2024-02-08 00:02:45.394000
Default value formatted by Exposed: '2024-02-08T00:02:45.394'

  1. SQL Server and H2 (except for H2 Oracle)

Default value from metadata: 2024-02-08T00:02:33.662
Default value formatted by Exposed: '2024-02-08T00:02:33.662'

Mismatches for date column type:

  1. PostgreSQL and PostgresSQLNG

Default value from metadata: '2024-02-01'::date
Default value formatted by Exposed: '2024-02-01'

  1. Oracle and H2 Oracle

Default value from metadata: DATE '2024-02-01
Default value formatted by Exposed: DATE '2024-02-01'

  1. SQL Server and H2 (except for H2 Oracle)

Default value from metadata: 2024-02-01
Default value formatted by Exposed: '2024-02-01'

  1. MySQL and MariaDB

Default value from metadata: 2024-02-01
Default value formatted by Exposed: '2024-02-01'

Mismatches for time column type:

  1. PostgreSQL

Default value from metadata: '00:11:17.34'::time without time zone
Default value formatted by Exposed: '00:11:17.34'

  1. Oracle and H2 Oracle

Default value from metadata: TIMESTAMP '1900-01-01 00:11:12
Default value formatted by Exposed: TIMESTAMP '1900-01-01 00:11:12'

  1. SQL Server and H2 (except for H2 Oracle)

Default value from metadata: 00:11:16.088
Default value formatted by Exposed: '00:11:16.088'

  1. MySQL and MariaDB

Default value from metadata: 00:11:13
Default value formatted by Exposed: '00:11:13.431'

@joc-a joc-a force-pushed the joc/fix-time-related-default-issues branch 3 times, most recently from 94c99bc to 82e85ad Compare February 5, 2024 16:01
@joc-a joc-a changed the title fix: Date/Time defaults falsely trigger ALTER statements fix: EXPOSED-283 Date/Time defaults falsely trigger ALTER statements Feb 5, 2024
@joc-a joc-a force-pushed the joc/fix-time-related-default-issues branch 9 times, most recently from 313b282 to e11494f Compare February 8, 2024 22:23
@joc-a joc-a marked this pull request as ready for review February 8, 2024 23:12
@joc-a joc-a requested review from bog-walk and e5l February 8, 2024 23:12
@joc-a joc-a force-pushed the joc/fix-time-related-default-issues branch 3 times, most recently from 65fe85b to 1895002 Compare February 9, 2024 13:10
@bog-walk
Copy link
Member

@joc-a @e5l A though about valueAsDefaultString() and how much it duplicates valueToString():

The new functions create strings that match the metadata exactly and the only place valueAsDefaultString() seems used is in processForDefaultValue(), which does 3 things:

  • generates DDL for a column
  • registers column arguments for a parameterized statement
  • used for metadata comparisons

This means all generated SQL with defaults is going to now be different when logged, which works fine. So maybe it would be worth assessing if it would be possible to use this new logic directly inside of the original functions instead. Or at least directly inside the issue source, at LiteralOp, so that the 'correct' SQL string is being used at the first origin, and leave processForDefaultValue() unchanged:

class LiteralOp<T>(
    override val columnType: IColumnType,
    val value: T
) : ExpressionWithColumnType<T>() {
    override fun toQueryBuilder(queryBuilder: QueryBuilder): Unit = queryBuilder {
        // +columnType.valueToString(value)
        +columnType.valueAsDefaultString(value)
    }
}

I guess my overall thought is: Could valueAsDefaultString() be dropped? And either valueToString() gets refactored for literals or nonNullValueAsDefaultString() does all the heavy lifting? For the latter, it would keep the new logic, but could call a common utility function first to trim if needed and check Expression subtypes.

The majority of column types will definitely need to override nonNullValueAsDefaultString(), so I think it'd be great if the goal was to extract all column type logic from both SchemaUtils.dbDefaultToString() and processForDefaultValue(). So maintainers wouldn't have to search in 3 different files for default column behavior and we could one day do something simple like this at the main culprit line in SchemaUtils.addMissingColumnsStatements():

val incorrectDefaults = existingCol.defaultDbValue != col.dbDefaultValue?.let {
    columnType.nonNullValueAsDefaultString(it)
}

@e5l
Copy link
Member

e5l commented Feb 12, 2024

@joc-a, please check CI

@joc-a joc-a force-pushed the joc/fix-time-related-default-issues branch from 1895002 to a6e03fa Compare February 12, 2024 14:17
@joc-a
Copy link
Collaborator Author

joc-a commented Feb 13, 2024

@bog-walk My initial approach was to change the nonNullValueToString implementation to match the default value returned by the column's metadata. However, this approach hits a wall with some dialects for the timestamp with time zone column type, because the default value returned for some dialects omits the time zone entirely, and for other dialects, it is always stored using the UTC time zone (thereby losing the original specified time zone). Matching that behaviour would result in a loss of the information seen by users when they log the SQL statements generated by Exposed.
I do want to refactor this code and make some improvements after it is merged (to keep this PR limited to this fix).

…tement

-The default value, compared to the one from the metadata, used to be wrapped in single quotes, so there's a check to remove them now in `DataTypeProvider.dbDefaultToString` function.
-In the `sanitizedDefault` function in `JdbcDatabaseMetadataImpl`, the default value String obtained fromm the metadata for Oracle and H2_Oracle is now trimmed of its single quotes only when a single quote exists at the start AND end.
-In `ColumnType`, two new functions were added to deal with the String representation of a column's default value to match the default value obtained from the metadata (eg: `'2024-01-31 16:50:27.222'::timestamp without time zone` for PostgreSQL): `valueAsDefaultString` and `nonNullValueAsDefaultString`.
-In the `processForDefaultValue` function in `PostgreSQLDataTypeProvider`, `valueAsDefaultString` is now used when the type of the expression is LiteralOp.
-`KotlinInstantColumnType` and `JavaInstantColumnType` now override `nonNullValueAsDefaultString` to match the default value obtained from the metadata for PostgreSQL and H2 Oracle.
-Breaking change: In `nonNullValueToString` for `KotlinInstantColumnType` and `JavaDateColumnType`, the formatted String for MySQL did not match the format received from the metadata when `isFractionDateTimeSupported` is true, so a new formatter specific to that is now used.
-Fix failing tests in `ArrayColumnTypeTests` by overriding `nonNullValueToString` in `ArrayColumnType` and moving the logic for handling a non-null value from the overridden `valueToString` to `nonNullValueToString`.
…ement

-`KotlinLocalDateTimeColumnType`, `JavaLocalDateTimeColumnType`, and `DateColumnType` now override `nonNullValueAsDefaultString` to match the default value obtained from the metadata for PostgreSQL and H2 Oracle.
-Breaking change: In `nonNullValueToString` for `KotlinLocalDateTimeColumnType`, the formatted String for MySQL did not match the format received from the metadata when `isFractionDateTimeSupported` is true, so a new formatter specific to MySQL is now used.
-`KotlinLocalDateColumnType` and `JavaLocalDateColumnType` now override `nonNullValueAsDefaultString` to match the default value obtained from the metadata for PostgreSQL.
-In `DateColumnType`, `nonNullValueAsDefaultString` is modified to match the default value obtained from the metadata for PostgreSQL.
-`KotlinLocalTimeColumnType` and `JavaLocalTimeColumnType` now override `nonNullValueAsDefaultString` to match the default value obtained from the metadata for PostgreSQL and MySQL. The formatted String for MySQL (with fractional seconds) did not match the format received from the metadata (without fractional seconds), so a new formatter for MySQL is used which discards the fractional seconds.
…gers ALTER statement

-`KotlinOffsetDateTimeColumnType`, `JavaOffsetDateTimeColumnType`, and `DateTimeWithTimeZoneColumnType` now override `nonNullValueAsDefaultString` to match the default value obtained from the metadata for PostgreSQL, MySQL, and H2 Oracle.
@joc-a joc-a force-pushed the joc/fix-time-related-default-issues branch from a6e03fa to 902dbb9 Compare February 13, 2024 22:48
@joc-a joc-a merged commit 768b553 into main Feb 14, 2024
5 checks passed
@joc-a joc-a deleted the joc/fix-time-related-default-issues branch February 14, 2024 12:20
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