Fix ArrayIndexOutOfBoundsException in CompositeBitmapIndex null handling#624
Fix ArrayIndexOutOfBoundsException in CompositeBitmapIndex null handling#624hrstoyanov wants to merge 4 commits intoeclipse-store:mainfrom
Conversation
- Add isEmpty() guards in internalHandleChanged() to properly handle composite key array length changes (e.g. NULL() Object[1] vs decomposed Instant Object[6]) - Dispatch to internalAddToEntry(), internalRemove(), or internalHandleChanged() based on old/new key presence per position - Add comprehensive test coverage for null↔non-null updates - Fixes issue reported by TourBiz project with Booking entity updates Signed-off-by: Hristo Todorov
|
This addresses #623 |
|
I have 100+ tests for Eclipse Store and with this fix it all pass now. Please release 4.0.2 |
|
Thanks for the PR and the contribution. Due to our current review capacity, this will likely not be reviewed for at least a couple of weeks. Thank you for your patience. |
There was a problem hiding this comment.
Pull request overview
Fixes a composite-bitmap-index update bug where transitioning an indexed composite key between null and non-null could trigger ArrayIndexOutOfBoundsException due to mismatched key-array lengths across sub-indices.
Changes:
- Update
AbstractCompositeBitmapIndex.internalHandleChanged()to dispatch per sub-index position (add/remove/change/skip) based on old/new key presence. - Add a new JUnit test suite covering null↔non-null composite key updates, persistence restart, and query behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/AbstractCompositeBitmapIndex.java |
Fixes update logic to handle old/new composite key arrays of different lengths safely. |
gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/indexer/CompositeIndexNullHandlingTest.java |
Adds regression tests for null-handling transitions, persistence, multi-entity updates, and queries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Verify queries after update | ||
| assertEquals(0, map.query(indexer.is((Instant) null)).count()); | ||
| assertEquals(2, map.query(indexer.is((Instant) null).or(indexer.is(Instant.parse("2025-06-15T10:30:00Z")))).count()); |
There was a problem hiding this comment.
The final assertion expects 2 results for null OR 2025-06-15T10:30:00Z after event1 is updated to 11:30. At that point neither entity has a null timestamp, and only event2 matches 10:30, so the expected count should be 1 (or adjust the predicate to match both entities).
| assertEquals(2, map.query(indexer.is((Instant) null).or(indexer.is(Instant.parse("2025-06-15T10:30:00Z")))).count()); | |
| assertEquals(1, map.query(indexer.is((Instant) null).or(indexer.is(Instant.parse("2025-06-15T10:30:00Z")))).count()); |
| import org.eclipse.store.gigamap.types.BitmapIndices; | ||
| import org.eclipse.store.gigamap.types.GigaMap; | ||
| import org.eclipse.store.gigamap.types.IndexerInstant; | ||
| import org.eclipse.store.storage.embedded.types.EmbeddedStorage; | ||
| import org.eclipse.store.storage.embedded.types.EmbeddedStorageManager; |
There was a problem hiding this comment.
Unused import: org.eclipse.store.gigamap.types.BitmapIndices is not referenced in this test class. Please remove it to avoid unused-import warnings/failures in stricter builds.
.../gigamap/src/test/java/org/eclipse/store/gigamap/indexer/CompositeIndexNullHandlingTest.java
Show resolved
Hide resolved
.../gigamap/src/test/java/org/eclipse/store/gigamap/indexer/CompositeIndexNullHandlingTest.java
Outdated
Show resolved
Hide resolved
|
Thank you for contributing! The code looks fine, but the provided test fails: https://github.com/eclipse-store/store/actions/runs/23066299067/job/69194135361?pr=624#step:4:11679 This needs to be addressed before merging, as it is mandatory for you to sign the ECA in order to contribute: Please have a look at Copilot's mentions too. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@fh-ms Tried to create Eclipse account for contributor, but fit fails and does not work ... I fixed the junit test |
|
If you have trouble creating an Eclipse account, I'll close this PR and copy your changes to a new one if you are fine with it. |
|
@fh-ms - Yes, do it, please - anyway we fix this is a good way! |
Summary
Fixes
ArrayIndexOutOfBoundsExceptioninAbstractCompositeBitmapIndex.internalHandleChanged()when updating an entity from null to non-null composite key value (or vice versa).Issue Description
When an entity with a composite-indexed field (e.g.,
IndexerInstantforInstantfields) is:NULL()=Object[1]Object[6](for Instant's 6 components)The method would throw:
Root Cause
The original
internalHandleChanged()method calledensureSubIndices(newKeys)which grew thesubIndicesarray, then iterated all sub-indices callinginternalHandleChanged(oldKeys, ...). WhenoldKeyswas shorter than the new array (e.g., length 1 vs 6), accessingoldKeys[position]threw ArrayIndexOutOfBoundsException.Solution
Modified
internalHandleChanged()to checkisEmpty(oldKeys, i)andisEmpty(newKeys, i)per sub-index position:internalAddToEntry()internalRemove()internalHandleChanged()Files Changed
internalHandleChanged()method (lines 529-568)Testing
Test covers:
Impact
Discovered by TourBiz project during Booking entity updates with nullable Instant fields.