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

chore!: Change H2 Oracle longType and longAutoincType from NUMBER(19) to BIGINT and add CHECK constraint in Oracle #2273

Merged
merged 2 commits into from
Jan 24, 2025

Conversation

joc-a
Copy link
Collaborator

@joc-a joc-a commented Oct 14, 2024

Description

Detailed description:

  • What:
    The SQL type of H2 Oracle Long type was changed. A CHECK constraint was also added for Oracle.
  • Why:
  1. H2 enforces the range on its own when BIGINT is used.
  2. It is a preparation step for making detecting column type change for migration easier

Type of Change

Please mark the relevant options with an "X":

  • Bug fix
  • New feature
  • Documentation update
  • Improvement

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/modify-long-type branch from de57d51 to d2cf30f Compare October 14, 2024 17:22
@joc-a joc-a marked this pull request as ready for review October 14, 2024 18:32
@joc-a joc-a requested review from bog-walk and obabichevjb October 14, 2024 18:32
val name = column.name
val checkName = "${generatedSignedCheckPrefix}long_$name"
if (checkConstraints.none { it.first == checkName }) {
column.check(checkName) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if it's ok or not, but to me it looks a bit suspicious at the current moment.

We actually modify the table object, so user will have changes in table object if he tries to create statement for that table.

The method createStatement() does not look like the one that should modify table object, so some users may be surprised

Copy link
Collaborator Author

@joc-a joc-a Oct 16, 2024

Choose a reason for hiding this comment

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

@obabichevjb I agree. I didn't like this solution, to be frank, but I didn't have any other ideas. If you have any suggestions on how to do this in a cleaner way, please let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a good question, I'm not sure about what is the behavior we want to achieve.

If we want to guarantee that only long values would be stored in the database base and exception would be thrown otherwise, it could be problematic (like in the PR)

I tried to check SQLite, and I found out that if the column type INTEGER is used, then the column will store only Kotlin's Long values. But it would not complain if the user tried to save the value beyond Long.MIN_VALUE/Long.MAX_VALUE, it will just "cut" the values to the range.

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've decided to simply exclude SQLite from this expected behaviour simply because the raw SQL does not have that behaviour to begin with, and it is a hassle for Exposed to try to do it. If a user wants to enforce it, they can simply add their own custom check constraint like this:

val long = long("long_column").check { column ->
    fun typeOf(value: String) = object : ExpressionWithColumnType<String>() {
        override fun toQueryBuilder(queryBuilder: QueryBuilder) = queryBuilder { append("typeof($value)") }
        override val columnType: IColumnType<String> = TextColumnType()
    }
    val typeCondition = Expression.build { typeOf(column.name) eq stringLiteral("integer") }
    if (column.columnType.nullable) {
        column.isNull() or typeCondition
    } else {
        typeCondition
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@e5l What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @obabichevjb about modifying tables in createStatement, so I'm good if we're not doing that.

The one major thing to consider is a strict mode (like https://dev.mysql.com/doc/refman/8.4/en/sql-mode.html) or strict tables (https://www.sqlite.org/stricttables.html). We should mention these in the docs and check if we're working as expected if those are enabled.

@bog-walk bog-walk removed their request for review November 13, 2024 18:27
@joc-a joc-a force-pushed the joc/modify-long-type branch from d2cf30f to e668a55 Compare November 15, 2024 16:14
@joc-a joc-a force-pushed the joc/modify-long-type branch 5 times, most recently from bc28075 to 3f2e4b2 Compare November 28, 2024 11:44
@joc-a joc-a requested review from obabichevjb and e5l November 28, 2024 14:21
Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

left a comment

… to BIGINT and add CHECK constraint in Oracle and SQLite
@joc-a joc-a force-pushed the joc/modify-long-type branch from 3f2e4b2 to 48198aa Compare January 23, 2025 15:31
@joc-a joc-a force-pushed the joc/modify-long-type branch from 48198aa to 810c945 Compare January 24, 2025 14:30
…iour is that it is not possible to enforce the range without a special CHECK constraint that checks the type
@joc-a joc-a force-pushed the joc/modify-long-type branch from 810c945 to 753c534 Compare January 24, 2025 14:31
@joc-a joc-a changed the title chore!: Change H2 Oracle longType and longAutoincType from NUMBER(19) to BIGINT and add CHECK constraint in Oracle and SQLite chore!: Change H2 Oracle longType and longAutoincType from NUMBER(19) to BIGINT and add CHECK constraint in Oracle Jan 24, 2025
@joc-a joc-a merged commit a587ad9 into main Jan 24, 2025
5 checks passed
@joc-a joc-a deleted the joc/modify-long-type branch January 24, 2025 15:17
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