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-447 Eager loading does not work with composite PK entity #2177

Merged
merged 2 commits into from
Jul 28, 2024

Conversation

bog-walk
Copy link
Member

@bog-walk bog-walk commented Jul 26, 2024

Description

Summary of the change:
It is now possible to use eager loading with references involving CompositeIdTable.

Detailed description:

  • Why:
    Initial implementation of CompositeIdTable was already very large, so the necessary logic to use .load() and .with() was not included. preloadRelations() also relied heavily on inList, which was not capable of accepting an unknown-sized list of columns on the left hand side: PR feat: EXPOSED-446 Support N-column inList equality comparisons #2157

  • How:

    • Add inList overloads for List<CompositeID> and for deconstructed CompositeIdTable.id
    • Refactor preloadRelations() to handle CompositeIdTable references (both parent and child)
    • Add variant to warmupReferences() that can work with multiple-column ids.

Type of Change

Please mark the relevant options with an "X":

  • Bug fix

Affected databases:

  • All

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

Related Issues

EXPOSED-447

@bog-walk bog-walk requested review from e5l and obabichevjb July 26, 2024 03:35
Comment on lines 468 to 469
@Test
fun testPreloadReferencesOnCompositeIdEntities() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This test seems to be quite slow, I'm assuming because of the inTopLevelTransaction. I will split it locally into multiple tests and compare.

@@ -438,4 +439,143 @@ class CompositeIdTableEntityTest : DatabaseTestsBase() {
assertEqualCollections(publisherA.allOffices.toList(), listOf(officeB))
}
}

// fails because of bug in SQL Server notInList logic - see link to fix in PR details
// @Test
Copy link
Member

Choose a reason for hiding this comment

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

Could you use @ignore and log an issue instead of the comment?

List(this.size) { i ->
val component = id[this[i] as Column<Comparable<Any>>]
component.takeIf {
it !is EntityID<*> || this[i].columnType is EntityIDColumnType<*>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: I expect that it's just extra check, but is it possible that non-EntityId column (actually here is Comparable, but I expect that it's always columns) has EntityIDColumnType, does it make sense?

By the way removing of it !is EntityID<*> || does not break tests (at least fo H2 and Postgres)

Copy link
Member Author

@bog-walk bog-walk Jul 28, 2024

Choose a reason for hiding this comment

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

You're right. The tests were failing before (for case with double-wrapped EntityID<EntityID>) but not anymore, so I must have changed something in the logic lower down (hopefully it's not the tests that changed).

*/
@Suppress("UNCHECKED_CAST")
@JvmName("notInListCompositeEntityIds")
infix fun <ID : EntityID<CompositeID>> Column<ID>.notInList(list: Iterable<CompositeID>): InListOrNotInListBaseOp<List<*>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Idea says that there are no usages of that function.

Actually, if I comment this function, and the one above (infix fun List<Column<*>>.notInList(list: Iterable<CompositeID>): InListOrNotInListBaseOp<List<*>>) I get tests green (at least for H2 and Postgres), is it just a lack of tests yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are being tested in the test that was originally commented out before PR #2176 was merged.

@@ -954,12 +956,79 @@ abstract class EntityClass<ID : Comparable<ID>, out T : Entity<ID>>(
}
}

internal fun warmUpAllReferences(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably better to name warmUpCompositeIdReferences

I read this code first time, and I haven't expected that additionally to the method warmUpAllReferences there is another one warmUpReferences. That's definitely minor.

private fun getEntities(forUpdate: Boolean?, findQuery: SizedIterable<T>): List<T> = when (forUpdate) {
true -> findQuery.forUpdate()
false -> findQuery.notForUpdate()
else -> findQuery
}.toList()

private fun <R> List<T>.groupByReference(
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 always used with one argument, so it could be split into 2 independen functions.

fun Entity<*>.getReferenceId(
delegateRefColumn: Column<*>,
refColumns: Map<Column<*>, Column<*>>,
isNotCompositeIdReference: Boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also minor, but negatives in arguments a bit harder to read, usually better to name without them (isCompositeIdReference in this case)

null
} else {
CompositeID {
refColumns.values.forEachIndexed { i, parent ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like it would be good idea to add a function to build CompositeID. This piece of code I was able to extract to

private fun compositeId(entries: List<Pair<Column<EntityID<Comparable<Any>>>, Comparable<Any>>>) = CompositeID {
    entries.forEachIndexed { i, entry ->
        it[entry.first] = entry.second
    }
}

so the call of it looks like

compositeId(refColumns.values.zip(childValues) as List<Pair<Column<EntityID<Comparable<Any>>>, Comparable<Any>>>)

Typing unfortunately looks already not a human readable a bit =)

- Add inList variants for List<CompositeID> and for deconstructed CompositeIdTable.id
- Refactor preloadRelations() to handle CompositeIdTable references (parent and child)
- Add tests
- Uncomment inList test & split tests into 1 per reference type
- Remove extra check in inList variants for EntityID
- Rename internal function to warmUpCompositeIdReferences
- Splite groupByReference() into 2 overloads
- Rename variables that flag singleIdReferences
- Add internal CompositeID builder from reference maps
@bog-walk bog-walk force-pushed the bog-walk/fix-composite-eager-loading branch from 060914c to 3ad937a Compare July 28, 2024 22:55
@bog-walk bog-walk merged commit 5cea6fe into main Jul 28, 2024
5 checks passed
@bog-walk bog-walk deleted the bog-walk/fix-composite-eager-loading branch July 28, 2024 23:57
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.

3 participants