HHH-20658 Preserve index gaps for null elements of inverse indexed lists - #13073
HHH-20658 Preserve index gaps for null elements of inverse indexed lists#13073Develop-KIM wants to merge 3 commits into
Conversation
A null element of a @onetomany(mappedBy=...) list mapped with @OrderColumn has no row of its own, but it does occupy a position: the order column values around it should leave a gap, which ListInitializer pads back into a null element when the collection is loaded. Both ActionQueue implementations got this wrong, in different ways: - The graph-based queue planned a write-index operation for every entry, including nulls. AbstractOneToManyDecomposer#applyWriteIndexRestrictions then read the element identifier off the null entry and failed with "PropertyAccessException: Error accessing field [...] : null". Apply the same entry != null / entryExists guard the queued-additions path in that class already uses. - The legacy queue skipped null entries, but incremented nextIndex inside the same check, so a null left no gap and the order column was written compacted: a list persisted as [a, null, b] was stored with indices 0,1 instead of 0,2 and loaded back as [a, b]. Increment nextIndex for every entry, as OneToManyPersister#writeIndex did before 6.2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Develop-KIM <kimdonghwan913@gmail.com>
…gration guide Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Develop-KIM <kimdonghwan913@gmail.com>
mbellade
left a comment
There was a problem hiding this comment.
Thank you @Develop-KIM, the fix looks minimal and correct and would align with the behavior of owned @OneToMany associations, as well as with the read-side expectations.
The JPA spec says:
The persistence provider must maintain a contiguous (non-sparse) ordering
But it doesn't explicitly define the behavior with null values. Furthermore, for unowned lists it says:
Use of the OrderColumn annotation on the unowned side of a relationship is not portable between persistence providers
So it is unfortunate this has been the behavior since 6.2, but I think this change would make sense, though I would keep it in 8.0 and not back-port it to earlier versions. @sebersole wdyt?
|
@mbellade @sebersole gentle ping on this one — it's been sitting since the review in July. Nothing has changed on my side: the branch is still on |
mbellade
left a comment
There was a problem hiding this comment.
Sorry for the long wait @Develop-KIM, we try to address all incoming PRs in a timely manner but we're a small team so it's not always easy.
I left a comment for now for a scenario that probably needs further testing, but I'd still like @sebersole to take a look at the direction of this fix and confirm if the new behavior is in line with expectations - in the meantime I've updated the Jira to classify it as an "improvement" instead of a bug.
| if ( jdbcOperations.updateIndexPlan() != null | ||
| && entry != null | ||
| && collection.entryExists( entry, entryCount ) ) { |
There was a problem hiding this comment.
We should probably do something similar in decomponseUpdate: you only changed this behavior in the re-create path, but what if we're just updating an ordered List which now contains null entries?
Please add tests for this scenarios, we should verify that a list's update works even when pre-existing null entries were present and/or when new null elements are "added".
There was a problem hiding this comment.
You were right, and it turned out to break in two different ways depending on whether the collection is initialized.
If it isn't, the addition is queued and decomposeQueuedOperations numbers the queued entries by counting only the ones it writes, so a queued null takes up no position: starting from [a, b] and adding null then a child wrote positions 0,1,2 — the new child landed on the gap. The position now comes from where the addition sits in the queue rather than from a counter that only advances on a write.
If it is initialized, it never got that far. PersistentList#computeEntityListChangeSet reports a null slot as an addition of null, and that reaches applyWriteIndexRestrictions, which asks the identifier mapping for the id of a null element — PropertyAccessException on both of the scenarios you described. A null element adds no row, so it isn't reported as an addition any more, and applyUpdateChanges now skips entries that don't exist the same way decomposeRecreate does.
I split the tests along that seam, since the two paths are easy to conflate: a pre-existing null and a newly added one, each once through the queued path and once with the collection read first, all under both ActionQueue implementations. The two initialized cases fail on the previous commit with the exception above, and the queued ones with a wrong order column.
Full hibernate-core suite: 17333 tests, one failure in LockTest#testFindWithPessimisticWriteLockTimeoutException, which is a five-second assertTimeout that trips when the machine is loaded — it passes on its own both with and without this change.
No rush on the direction question for @sebersole, and the "improvement" reclassification makes sense to me.
|
…is updated The recreate path was the only one that left the gap, so a list kept its null elements when it was created and lost them again on the next update. Two places wrote the wrong index: `decomposeQueuedOperations` numbered the queued additions of an uninitialized collection by counting only the ones it wrote, so a queued null took up no position and the element after it was written over the gap it should have left. The position now comes from where the addition sits in the queue. An initialized collection reported a null slot as an addition of `null` in `PersistentList#computeEntityListChangeSet`, which then failed in `applyWriteIndexRestrictions` while reading the identifier of a null element. A null element adds no row, so it is no longer reported as an addition, and `applyUpdateChanges` now skips entries that do not exist, as `decomposeRecreate` already did. Tests cover a pre-existing null and a newly added one, for the queued and the initialized case, under both `ActionQueue` implementations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Develop-KIM <kimdonghwan913@gmail.com>


A null element of an inverse
@OneToManylist mapped with@OrderColumnhas no row of its own, but it still occupies a list position, so the order column around it should leave a gap —ListInitializer#readCollectionRowpads that gap back into a null when the collection is loaded. NeitherActionQueuedoes that today, and they fail differently.Graph-based queue (default since 8.0) — it throws rather than compacting:
decomposeRecreateplans a write-index operation for every entry, soapplyWriteIndexRestrictionsends up reading the element identifier off a null entry. The queued-additions path a bit further down in the same class already guards withentry != null && collection.entryExists( entry, ... ), so I used the same guard there.entryCountis already incremented outside the check, so the gap falls out of it.Legacy queue (
hibernate.flush.queue.type=legacy) — no exception, but the index is written compacted, which is what HHH-20658 reports.WriteIndexCoordinatorStandard#writeIndexincrementsnextIndexinside the null check, so[a, null, b]is stored with indices 0,1 instead of 0,2 and reloads as[a, b].OneToManyPersister#writeIndexincremented it for every entry through 6.1.7 and the increment moved inside the check in 6.2, so I moved it back out.Tests cover both queue types (
InverseListNullElementTest,InverseListNullElementLegacyTest), asserting the order column holds 0,2 and that the list round-trips as[a, null, b]. Both fail on main and pass with the fix;:hibernate-core:testis green on H2 (17330 tests).ListElementNullBasicTeststill passes — this doesn't change how owned@ElementCollectionlists drop nulls.I used Claude Code to help write this; I reviewed the change and verified it locally.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.
Please make sure that the following tasks are completed:
Tasks specific to HHH-20658 (Improvement):
documentation/src/main/asciidoc/userguidefor all features,documentation/src/main/asciidoc/introductionfor main features, links from existing documentationmigration-guide.adoc(breaking changes) andwhats-new.adoc(new features/improvements)https://hibernate.atlassian.net/browse/HHH-20658