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: Allow column reference in default expressions for MySQL and MariaDB #2159

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

joc-a
Copy link
Collaborator

@joc-a joc-a commented Jul 17, 2024

MySQL and MariaDB are the only supported databases that allow referencing another column in a default expression, but for this to work, the referenced column must be one that occurs earlier in the table definition. This was not working because Exposed was sorting the columns by column name for the insert statement. Not sorting fixes this issue.

@joc-a joc-a force-pushed the joc/allow-column-reference-in-default-expressions branch from 0d7e201 to c8f1354 Compare July 17, 2024 12:49
@joc-a joc-a marked this pull request as ready for review July 17, 2024 14:30
@joc-a joc-a requested a review from obabichevjb July 17, 2024 14:30
MySQL and MariaDB are the only supported databases that allow referencing another column in a default expression, but for this to work, the referenced column must be one that occurs earlier in the table definition. This was not working because Exposed was sorting the columns by column name for the insert statement. Not sorting fixes this issue.
@joc-a joc-a force-pushed the joc/allow-column-reference-in-default-expressions branch from c8f1354 to 393da3e Compare July 17, 2024 14:35
@obabichevjb
Copy link
Collaborator

It looks good to me,

The only reason to make sorting I see is stable DDL generation. Sorting allows to get all the columns in the same order independently from how it was added to the table object.

I expect that if somebody already has generated tables, may get diff in DDL after that change. If you write a test that check that there is no change in ddl between two identical tables but with different columns order (if it is really so) it would be nice.

@joc-a
Copy link
Collaborator Author

joc-a commented Jul 18, 2024

It looks good to me,

The only reason to make sorting I see is stable DDL generation. Sorting allows to get all the columns in the same order independently from how it was added to the table object.

I expect that if somebody already has generated tables, may get diff in DDL after that change. If you write a test that check that there is no change in ddl between two identical tables but with different columns order (if it is really so) it would be nice.

@Test
fun testSameTableWithDifferentColumnOrder() {
    val tester1 = object : Table("tester") {
        val aColumn = integer("aColumn")
        val bColumn = integer("bColumn")
        val cColumn = integer("cColumn")
    }
    val tester2 = object : Table("tester") {
        val cColumn = integer("cColumn")
        val bColumn = integer("bColumn")
        val aColumn = integer("aColumn")
    }
    withDb {
        assertEquals(tester1.ddl, tester2.ddl)
    }
}

This would fail even before the changes in this PR, but it's not clear to me why we would ever need identical DDL for two identical tables with different column order. Could you please clarify?

@obabichevjb
Copy link
Collaborator

But this one does not fail now:

@Test
fun testSameTableWithDifferentColumnOrder() {
    val tester1 = object : Table("tester") {
        val aColumn = integer("aColumn")
        val bColumn = integer("bColumn")
        val cColumn = integer("cColumn")
    }
    val tester2 = object : Table("tester") {
        val cColumn = integer("cColumn")
        val bColumn = integer("bColumn")
        val aColumn = integer("aColumn")
    }
    withTables(tester1) {
        assertEqualLists(emptyList(),  SchemaUtils.statementsRequiredToActualizeScheme(tester2))
    }
}

I checked, in your branch it does not fail also, so it 's not a breaking change finally. I should check it earlier)

@joc-a
Copy link
Collaborator Author

joc-a commented Jul 18, 2024

@obabichevjb Oh, I see what you mean. Yeah, that's because detecting the difference between the Exposed table and the table in the database does not depend on the order of the columns. Thank you for the extra check!

@joc-a joc-a merged commit eef61fc into main Jul 18, 2024
5 checks passed
@joc-a joc-a deleted the joc/allow-column-reference-in-default-expressions branch July 18, 2024 11:53
DonRobo referenced this pull request in DonRobo/home-former Aug 4, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[org.jetbrains.exposed:exposed-jdbc](https://github.com/JetBrains/Exposed)
| `0.52.0` -> `0.53.0` |
[![age](https://developer.mend.io/api/mc/badges/age/maven/org.jetbrains.exposed:exposed-jdbc/0.53.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/org.jetbrains.exposed:exposed-jdbc/0.53.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/org.jetbrains.exposed:exposed-jdbc/0.52.0/0.53.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/org.jetbrains.exposed:exposed-jdbc/0.52.0/0.53.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
|
[org.jetbrains.exposed:exposed-core](https://github.com/JetBrains/Exposed)
| `0.52.0` -> `0.53.0` |
[![age](https://developer.mend.io/api/mc/badges/age/maven/org.jetbrains.exposed:exposed-core/0.53.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/org.jetbrains.exposed:exposed-core/0.53.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/org.jetbrains.exposed:exposed-core/0.52.0/0.53.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/org.jetbrains.exposed:exposed-core/0.52.0/0.53.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>JetBrains/Exposed
(org.jetbrains.exposed:exposed-jdbc)</summary>

###
[`v0.53.0`](https://github.com/JetBrains/Exposed/blob/HEAD/CHANGELOG.md#0530)

[Compare
Source](https://github.com/JetBrains/Exposed/compare/0.52.0...0.53.0)

Infrastructure:

-   SQLite driver 3.46.0.1
-   Spring Framework 6.1.11
-   Spring Boot 3.3.2
-   junit-bom 5.10.3

Features:

- feat: Add time extension function for temporal expressions in Kotlin
and Java by [@&#8203;joc-a](https://github.com/joc-a) in
[https://github.com/JetBrains/Exposed/pull/2121](https://github.com/JetBrains/Exposed/pull/2121)
- feat: EXPOSED-435 Allow insertReturning() to set isIgnore = true by
[@&#8203;bog-walk](https://github.com/bog-walk) in
[https://github.com/JetBrains/Exposed/pull/2148](https://github.com/JetBrains/Exposed/pull/2148)
- feat: EXPOSED-77 Support entity class for table with composite primary
key by [@&#8203;bog-walk](https://github.com/bog-walk) in
[https://github.com/JetBrains/Exposed/pull/1987](https://github.com/JetBrains/Exposed/pull/1987)
- feat: EXPOSED-446 Support N-column inList equality comparisons by
[@&#8203;bog-walk](https://github.com/bog-walk) in
[https://github.com/JetBrains/Exposed/pull/2157](https://github.com/JetBrains/Exposed/pull/2157)
- feat: EXPOSED-450 Merge command: PostgreSQL improvements by
[@&#8203;obabichevjb](https://github.com/obabichevjb) in
[https://github.com/JetBrains/Exposed/pull/2161](https://github.com/JetBrains/Exposed/pull/2161)
- feat: EXPOSED-388 Support for column type converters by
[@&#8203;obabichevjb](https://github.com/obabichevjb) in
[https://github.com/JetBrains/Exposed/pull/2143](https://github.com/JetBrains/Exposed/pull/2143)
- Adding comment text for a query SQL by
[@&#8203;xJoeWoo](https://github.com/xJoeWoo) in
[https://github.com/JetBrains/Exposed/pull/2088](https://github.com/JetBrains/Exposed/pull/2088)
- feat: EXPOSED-459 Open AbstractQuery.copyTo() to allow custom Query
class extension by [@&#8203;bog-walk](https://github.com/bog-walk) in
[https://github.com/JetBrains/Exposed/pull/2173](https://github.com/JetBrains/Exposed/pull/2173)
- feat: EXPOSED-461 Add time column in Joda-Time module by
[@&#8203;joc-a](https://github.com/joc-a) in
[https://github.com/JetBrains/Exposed/pull/2175](https://github.com/JetBrains/Exposed/pull/2175)

Bug fixes:

- fix: EXPOSED-424 ClassCastException exception when using
`fetchBatchedResults` with `alias` by
[@&#8203;joc-a](https://github.com/joc-a) in
[https://github.com/JetBrains/Exposed/pull/2140](https://github.com/JetBrains/Exposed/pull/2140)
- fix: EXPOSED-407 compositeMoney() nullability definition issues by
[@&#8203;bog-walk](https://github.com/bog-walk) in
[https://github.com/JetBrains/Exposed/pull/2137](https://github.com/JetBrains/Exposed/pull/2137)
- fix: EXPOSED-415 SchemaUtils incorrectly generates ALTER statements
for existing nullable columns by
[@&#8203;obabichevjb](https://github.com/obabichevjb) in
[https://github.com/JetBrains/Exposed/pull/2136](https://github.com/JetBrains/Exposed/pull/2136)
- fix: EXPOSED-363 LocalTime and literal(LocalTime) are not the same by
[@&#8203;joc-a](https://github.com/joc-a) in
[https://github.com/JetBrains/Exposed/pull/2152](https://github.com/JetBrains/Exposed/pull/2152)
- fix: EXPOSED-432 CurrentDate default is generated as null in MariaDB
by [@&#8203;joc-a](https://github.com/joc-a) in
[https://github.com/JetBrains/Exposed/pull/2149](https://github.com/JetBrains/Exposed/pull/2149)
- fix: Allow column reference in default expressions for MySQL and
MariaDB by [@&#8203;joc-a](https://github.com/joc-a) in
[https://github.com/JetBrains/Exposed/pull/2159](https://github.com/JetBrains/Exposed/pull/2159)
- fix: EXPOSED-430 Insert and BatchInsert do not return default values
by [@&#8203;obabichevjb](https://github.com/obabichevjb) in
[https://github.com/JetBrains/Exposed/pull/2158](https://github.com/JetBrains/Exposed/pull/2158)
- fix: EXPOSED-452 Flaky H2\_Oracle test
`testTimestampWithTimeZoneDefaults` by
[@&#8203;joc-a](https://github.com/joc-a) in
[https://github.com/JetBrains/Exposed/pull/2169](https://github.com/JetBrains/Exposed/pull/2169)
- EXPOSED-457 The column default value always compares unequal by
[@&#8203;obabichevjb](https://github.com/obabichevjb) in
[https://github.com/JetBrains/Exposed/pull/2170](https://github.com/JetBrains/Exposed/pull/2170)
- EXPOSED-409 Custom primary key. Access to the primary key fails with
ClassCastException by
[@&#8203;obabichevjb](https://github.com/obabichevjb) in
[https://github.com/JetBrains/Exposed/pull/2151](https://github.com/JetBrains/Exposed/pull/2151)
- fix: EXPOSED-447 Eager loading does not work with composite PK entity
by [@&#8203;bog-walk](https://github.com/bog-walk) in
[https://github.com/JetBrains/Exposed/pull/2177](https://github.com/JetBrains/Exposed/pull/2177)

Docs:

- chore: Add migration sample by
[@&#8203;joc-a](https://github.com/joc-a) in
[https://github.com/JetBrains/Exposed/pull/2144](https://github.com/JetBrains/Exposed/pull/2144)
- docs: Change repetitionAttempts to maxAttempts in website docs by
[@&#8203;bog-walk](https://github.com/bog-walk) in
[https://github.com/JetBrains/Exposed/pull/2164](https://github.com/JetBrains/Exposed/pull/2164)
- docs: EXPOSED-445 Add documentation for DSL & DAO composite primary
keys by [@&#8203;bog-walk](https://github.com/bog-walk) in
[https://github.com/JetBrains/Exposed/pull/2165](https://github.com/JetBrains/Exposed/pull/2165)
- docs: EXPOSED-419 Rework the getting started tutorial by
[@&#8203;vnikolova](https://github.com/vnikolova) in
[https://github.com/JetBrains/Exposed/pull/2160](https://github.com/JetBrains/Exposed/pull/2160)
- Configure API documentation for Exposed by
[@&#8203;e5l](https://github.com/e5l) in
[https://github.com/JetBrains/Exposed/pull/2171](https://github.com/JetBrains/Exposed/pull/2171)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these
updates again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job
log](https://developer.mend.io/github/DonRobo/home-former).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoiZGV2ZWxvcCIsImxhYmVscyI6W119-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

2 participants