From 13b6b4894262da8db747f3c44b319f333425a7a0 Mon Sep 17 00:00:00 2001 From: Roman Andrushchenko Date: Fri, 3 Jul 2026 20:46:30 +0300 Subject: [PATCH] fix: Prevent IndexOutOfBoundsException in paging by enforcing item ID uniqueness ListMerger.reorderNonFinalItems assumed a 1:1 correspondence between target-list positions matching newIds and the new-item anchors. Duplicate item IDs (within a page, the same item across pages with live pagination, or externally intercepted values) broke that invariant and crashed with "Index N out of bounds for length N". Deduplicate by itemId at every point that produces the output list (getNonFinalItems, ListMerger input/result, and intercept), and guard the reorder loop, so the emitted list always has unique IDs. This also avoids Compose LazyColumn/LazyRow key-collision crashes downstream. --- README.md | 2 +- .../subject/paging/internal/ListMerger.kt | 21 +++++++-- .../paging/internal/PageRecordsState.kt | 13 ++++-- .../subject/paging/internal/ListMergerTest.kt | 45 +++++++++++++++++++ gradle-public.properties | 2 +- skills/container-store/README.md | 2 +- skills/container-store/SKILL.md | 4 +- store/README.md | 2 +- .../stores/common/CoreLoaderDelegate.kt | 4 +- .../store/stores/paged/PagedQueryStore.kt | 2 +- .../elveum/store/stores/paged/PagedStore.kt | 2 +- .../store/stores/simple/SimpleQueryStore.kt | 2 +- .../elveum/store/stores/simple/SimpleStore.kt | 2 +- 13 files changed, 84 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index ba30174..769bd63 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ The full documentation is available [here](https://docs.uandcode.com/container). Add the following line to your `build.gradle` file: ``` -implementation "com.elveum:container:3.2.0" +implementation "com.elveum:container:3.2.1" ``` ## Core Concepts diff --git a/container/src/main/java/com/elveum/container/subject/paging/internal/ListMerger.kt b/container/src/main/java/com/elveum/container/subject/paging/internal/ListMerger.kt index 5408edb..0ecf698 100644 --- a/container/src/main/java/com/elveum/container/subject/paging/internal/ListMerger.kt +++ b/container/src/main/java/com/elveum/container/subject/paging/internal/ListMerger.kt @@ -11,7 +11,12 @@ internal class ListMerger( nonFinalOldItems: List, nonFinalNewItems: List, ) { - val state = State(nonFinalOldItems, nonFinalNewItems) + // Items are identified by itemId(). Duplicate ids (within a page or from + // the same logical item appearing on more than one page) break the 1:1 + // position/anchor correspondence in reorderNonFinalItems() and previously + // caused an IndexOutOfBoundsException. Deduplicate incoming new items by + // id (first occurrence wins) so the merge always operates on unique ids. + val state = State(nonFinalOldItems, nonFinalNewItems.distinctById()) state.apply { // step 1 - remove items from targetList that exist in old but not in new @@ -26,9 +31,10 @@ internal class ListMerger( // when an anchor is encountered, flush all nonFinalNewItems up to that anchor val result = buildFinalList() - // step 4 - replace data in targetList by result list + // step 4 - replace data in targetList by result list, keeping ids + // unique so the target never accumulates duplicates across merges targetList.clear() - targetList += result + targetList += result.distinctById() } } @@ -47,7 +53,12 @@ internal class ListMerger( } } val anchors = nonFinalNewItems.filter { itemId(it) in existingIds } - for (i in nonFinalPositions.indices) { + // With unique ids the two counts match; guard against any residual + // asymmetry (e.g. a target list that already contained duplicates) so a + // stale duplicate can never cause an out-of-bounds access. Leftover + // duplicate positions are cleaned up by distinctById() in mergeFrom(). + val count = minOf(nonFinalPositions.size, anchors.size) + for (i in 0 until count) { targetList[nonFinalPositions[i]] = anchors[i] } } @@ -73,6 +84,8 @@ internal class ListMerger( return result } + private fun List.distinctById(): List = distinctBy { itemId(it) } + private inner class State( val nonFinalOldItems: List, val nonFinalNewItems: List, diff --git a/container/src/main/java/com/elveum/container/subject/paging/internal/PageRecordsState.kt b/container/src/main/java/com/elveum/container/subject/paging/internal/PageRecordsState.kt index 3d24a83..416ac8d 100644 --- a/container/src/main/java/com/elveum/container/subject/paging/internal/PageRecordsState.kt +++ b/container/src/main/java/com/elveum/container/subject/paging/internal/PageRecordsState.kt @@ -87,11 +87,15 @@ internal class PageRecordsState( fun intercept(container: Container>): Container> { return if (container.isSuccess()) { + // Enforce id-uniqueness on externally supplied values too: duplicate + // ids crash Compose LazyColumn/LazyRow (which use item id as key) and + // break the merger's invariants. See ListMerger for details. + val uniqueValue = container.value.distinctBy { config.itemId(it) } coroutineScope.launch { mutex.withLock { outputList.clear() - outputList.addAll(container.value) - outputListSnapshotFlow.update { container.value } + outputList.addAll(uniqueValue) + outputListSnapshotFlow.update { uniqueValue } } } container.map { it } // report other instance the loader handled the update manually @@ -189,7 +193,10 @@ internal class PageRecordsState( ?.getOrNull() ?: emptyList() } - + // The same logical item can appear on more than one page (common + // with live backend pagination). Deduplicate by id so the merger + // only ever sees unique ids. See ListMerger for details. + .distinctBy { config.itemId(it) } } private fun Iterable>.withHighestPriority( diff --git a/container/src/test/java/com/elveum/container/subject/paging/internal/ListMergerTest.kt b/container/src/test/java/com/elveum/container/subject/paging/internal/ListMergerTest.kt index ceb3e72..d2e3fa2 100644 --- a/container/src/test/java/com/elveum/container/subject/paging/internal/ListMergerTest.kt +++ b/container/src/test/java/com/elveum/container/subject/paging/internal/ListMergerTest.kt @@ -485,6 +485,51 @@ class ListMergerTest { ) } + @Test + fun duplicateIdsInNewItems_deduplicatedKeepingFirst() { + // nonFinalNewItems contains the same id twice; the output must not + // accumulate duplicates (first occurrence wins). + val base = mutableListOf() + val merger = createListMerger(base) + + merger.mergeFrom( + nonFinalOldItems = emptyList(), + nonFinalNewItems = listOf(updated(1), item(2), updated(1)), + ) + + assertEquals( + listOf(updated(1), item(2)), + base, + ) + } + + @Test + fun duplicateIdsInTarget_doesNotCrashAndIsDeduplicated() { + // Reproduces the production IndexOutOfBoundsException: the target list + // ends up holding a duplicate id, then the same page reloads reporting + // that id only once. targetList has more positions matching newIds than + // there are anchors. + val base = mutableListOf() + val merger = createListMerger(base) + + // Merge 1: duplicate id 1 -> without dedup the base would be [1, 2, 1] + merger.mergeFrom( + nonFinalOldItems = emptyList(), + nonFinalNewItems = listOf(item(1), item(2), item(1)), + ) + + // Merge 2: page reloads, id 1 now appears only once. + merger.mergeFrom( + nonFinalOldItems = listOf(item(1), item(2), item(1)), + nonFinalNewItems = listOf(updated(1), updated(2)), + ) + + assertEquals( + listOf(updated(1), updated(2)), + base, + ) + } + private fun createListMerger(target: MutableList) = ListMerger(target) { it.id } diff --git a/gradle-public.properties b/gradle-public.properties index 0681d3c..f94b794 100644 --- a/gradle-public.properties +++ b/gradle-public.properties @@ -6,7 +6,7 @@ android.nonTransitiveRClass=true # Maven Central publishing coordinates GROUP=com.elveum -VERSION_NAME=3.2.0 +VERSION_NAME=3.2.1 # POM metadata POM_DESCRIPTION=A library for simplifying state management and data loading in Android applications diff --git a/skills/container-store/README.md b/skills/container-store/README.md index 449269f..625a7a3 100644 --- a/skills/container-store/README.md +++ b/skills/container-store/README.md @@ -43,4 +43,4 @@ discovers skills (e.g. `~/.agents/skills/` for Codex), or paste | `references/api.md` | Every public import, `StoreResult` / `LoadRequest` semantics, typical operations | | `references/patterns.md` | Layer-by-layer patterns: data sources, repositories, ViewModels, Compose, DI, scoping | -The skill matches library version `3.2.0`. +The skill matches library version `3.2.1`. diff --git a/skills/container-store/SKILL.md b/skills/container-store/SKILL.md index 4bb7cef..c545d19 100644 --- a/skills/container-store/SKILL.md +++ b/skills/container-store/SKILL.md @@ -22,13 +22,13 @@ directly - no decompilation or dependency tree inspection needed. ## Dependency Setup -Maven coordinates: `com.elveum:store:3.2.0` (transitively brings +Maven coordinates: `com.elveum:store:3.2.1` (transitively brings `com.elveum:container`, whose types are part of the public API). ```toml # gradle/libs.versions.toml [versions] -store = "3.2.0" +store = "3.2.1" [libraries] store = { module = "com.elveum:store", version.ref = "store" } ``` diff --git a/store/README.md b/store/README.md index 522112e..1674693 100644 --- a/store/README.md +++ b/store/README.md @@ -38,7 +38,7 @@ the hood. Add the following line to your `build.gradle` file: ``` -implementation "com.elveum:store:3.2.0" +implementation "com.elveum:store:3.2.1" ``` The `store` artifact depends on `com.elveum:container`, so the Container diff --git a/store/src/main/java/com/elveum/store/internal/stores/common/CoreLoaderDelegate.kt b/store/src/main/java/com/elveum/store/internal/stores/common/CoreLoaderDelegate.kt index a28dbb4..e9465ba 100644 --- a/store/src/main/java/com/elveum/store/internal/stores/common/CoreLoaderDelegate.kt +++ b/store/src/main/java/com/elveum/store/internal/stores/common/CoreLoaderDelegate.kt @@ -22,8 +22,8 @@ internal interface CoreLoaderDelegate { * Default [CoreLoaderDelegate] implementing the cache-then-remote loading algorithm * shared by all store types: * - * - [LoadRequestSource.Default]: emit the cached value first (from [loader], falling back - * to the first value of [observer]), then fetch and emit the remote value, saving it if + * - [LoadRequestSource.Default]: emit the cached value first (from `loader`, falling back + * to the first value of `observer`), then fetch and emit the remote value, saving it if * it differs from the cached one. * - [LoadRequestSource.Fresh]: skip caches entirely and emit only the remote value. * - [LoadRequestSource.Offline]: emit only the cached value; never fetch. If there is no diff --git a/store/src/main/java/com/elveum/store/stores/paged/PagedQueryStore.kt b/store/src/main/java/com/elveum/store/stores/paged/PagedQueryStore.kt index 8f2bd64..890232c 100644 --- a/store/src/main/java/com/elveum/store/stores/paged/PagedQueryStore.kt +++ b/store/src/main/java/com/elveum/store/stores/paged/PagedQueryStore.kt @@ -74,7 +74,7 @@ public interface PagedQueryStore : * * See [BaseStore.optimisticUpdate] for more details. * - * See also [update] extension. + * See also: [updateIfSuccess][com.elveum.store.stores.base.updateIfSuccess] extension. */ override suspend fun optimisticUpdate(updater: suspend OptimisticUpdateScope>.(List) -> Unit) diff --git a/store/src/main/java/com/elveum/store/stores/paged/PagedStore.kt b/store/src/main/java/com/elveum/store/stores/paged/PagedStore.kt index 26aac87..35adbe3 100644 --- a/store/src/main/java/com/elveum/store/stores/paged/PagedStore.kt +++ b/store/src/main/java/com/elveum/store/stores/paged/PagedStore.kt @@ -55,7 +55,7 @@ public interface PagedStore : * * See [BaseStore.optimisticUpdate] for more details. * - * See also: [update] extension. + * See also: [updateIfSuccess][com.elveum.store.stores.base.updateIfSuccess] extension. */ override suspend fun optimisticUpdate(updater: suspend OptimisticUpdateScope>.(List) -> Unit) diff --git a/store/src/main/java/com/elveum/store/stores/simple/SimpleQueryStore.kt b/store/src/main/java/com/elveum/store/stores/simple/SimpleQueryStore.kt index e115fb4..5a0cbe6 100644 --- a/store/src/main/java/com/elveum/store/stores/simple/SimpleQueryStore.kt +++ b/store/src/main/java/com/elveum/store/stores/simple/SimpleQueryStore.kt @@ -64,7 +64,7 @@ public interface SimpleQueryStore : * * See [BaseStore.optimisticUpdate] for more details. * - * See also: [update] extension. + * See also: [updateIfSuccess][com.elveum.store.stores.base.updateIfSuccess] extension. */ override suspend fun optimisticUpdate(updater: suspend OptimisticUpdateScope.(T) -> Unit) diff --git a/store/src/main/java/com/elveum/store/stores/simple/SimpleStore.kt b/store/src/main/java/com/elveum/store/stores/simple/SimpleStore.kt index 3356f9e..875f93a 100644 --- a/store/src/main/java/com/elveum/store/stores/simple/SimpleStore.kt +++ b/store/src/main/java/com/elveum/store/stores/simple/SimpleStore.kt @@ -52,7 +52,7 @@ public interface SimpleStore : * * See [BaseStore.optimisticUpdate] for more details. * - * See also: [update] extension. + * See also: [updateIfSuccess][com.elveum.store.stores.base.updateIfSuccess] extension. */ override suspend fun optimisticUpdate(updater: suspend OptimisticUpdateScope.(T) -> Unit)