[
https://issues.apache.org/jira/browse/COLLECTIONS-892?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ruiqi Dong updated COLLECTIONS-892:
-----------------------------------
Description:
*Summary*
For `uniqueIndexedCollection(...)`, adding an object whose transformed key
already exists is supposed to fail with `IllegalArgumentException`.
The current implementation appends the object to the decorated collection
first, and only afterwards checks the uniqueness constraint while updating the
index. If the key collides, the exception is thrown too late: the decorated
collection has already been mutated.
*Affected code*
File:
`src/main/java/org/apache/commons/collections4/collection/IndexedCollection.java`
{code:java}
@Override
public boolean add(final C object) {
final boolean added = super.add(object);
if (added) {
addToIndex(object);
}
return added;
}
private void addToIndex(final C object) {
final K key = keyTransformer.apply(object);
if (uniqueIndex && index.containsKey(key)) {
throw new IllegalArgumentException("Duplicate key in uniquely indexed
collection.");
}
index.put(key, object);
} {code}
*Reproducer*
Add the following test to
`src/test/java/org/apache/commons/collections4/collection/IndexedCollectionTest.java`:
{code:java}
@Test
void testFailedUniqueAddDoesNotPolluteDecoratedCollection() {
final IndexedCollection<Integer, String> indexed =
decorateUniqueCollection(new ArrayList<>());
indexed.add("01");
assertThrows(IllegalArgumentException.class, () -> indexed.add("1"));
assertEquals(1, indexed.size());
assertEquals(asList("01"), new ArrayList<>(indexed));
} {code}
Both `"01"` and `"1"` map to the same integer key `1`.
Run:
{code:java}
mvn -q
-Dtest=org.apache.commons.collections4.collection.IndexedCollectionTest#testFailedUniqueAddDoesNotPolluteDecoratedCollection
test {code}
Observed behavior
{code:java}
expected: <1> but was: <2> {code}
*Expected behavior*
After the duplicate-key `add(...)` fails, the collection should still contain
only the original element `"01"`.
This is not merely about exception atomicity in the abstract.
`IndexedCollection` promises a unique index, but after a failed add:
- the decorated collection contains both values
- the index contains only the original key mapping
So the collection and its index immediately diverge, and the uniqueness
guarantee is no longer reflected by the actual collection state.
was:
# [BUG] `IndexedCollection.add()` mutates the decorated collection before
rejecting a duplicate unique index key
## Summary
For `uniqueIndexedCollection(...)`, adding an object whose transformed key
already exists is supposed to fail with `IllegalArgumentException`.
The current implementation appends the object to the decorated collection
first, and only afterwards checks the uniqueness constraint while updating the
index. If the key collides, the exception is thrown too late: the decorated
collection has already been mutated.
## Affected code
File:
`src/main/java/org/apache/commons/collections4/collection/IndexedCollection.java`
```java
@Override
public boolean add(final C object) {
finalbooleanadded = super.add(object);
if (added) {
addToIndex(object);
}
return added;
}
private void addToIndex(final C object) {
finalKkey = keyTransformer.apply(object);
if (uniqueIndex && index.containsKey(key)) {
thrownewIllegalArgumentException("Duplicate key in uniquely indexed
collection.");
}
index.put(key, object);
}
```
## Reproducer
Add the following test to
`src/test/java/org/apache/commons/collections4/collection/IndexedCollectionTest.java`:
```java
@Test
void testFailedUniqueAddDoesNotPolluteDecoratedCollection() {
finalIndexedCollection<Integer, String> indexed =
decorateUniqueCollection(newArrayList<>());
indexed.add("01");
assertThrows(IllegalArgumentException.class, () ->indexed.add("1"));
assertEquals(1, indexed.size());
assertEquals(asList("01"), newArrayList<>(indexed));
}
```
Both `"01"` and `"1"` map to the same integer key `1`.
Run:
```bash
mvn -q
-Dtest=org.apache.commons.collections4.collection.IndexedCollectionTest#testFailedUniqueAddDoesNotPolluteDecoratedCollection
test
```
## Observed behavior
```text
expected: <1> but was: <2>
```
## Expected behavior
After the duplicate-key `add(...)` fails, the collection should still contain
only the original element `"01"`.
## Why this is a bug
This is not merely about exception atomicity in the abstract.
`IndexedCollection` promises a unique index, but after a failed add:
- the decorated collection contains both values
- the index contains only the original key mapping
So the collection and its index immediately diverge, and the uniqueness
guarantee is no longer reflected by the actual collection state.
> IndexedCollection.add() mutates the decorated collection before rejecting a
> duplicate unique index key
> ------------------------------------------------------------------------------------------------------
>
> Key: COLLECTIONS-892
> URL: https://issues.apache.org/jira/browse/COLLECTIONS-892
> Project: Commons Collections
> Issue Type: Bug
> Components: Collection
> Reporter: Ruiqi Dong
> Priority: Minor
>
> *Summary*
> For `uniqueIndexedCollection(...)`, adding an object whose transformed key
> already exists is supposed to fail with `IllegalArgumentException`.
> The current implementation appends the object to the decorated collection
> first, and only afterwards checks the uniqueness constraint while updating
> the index. If the key collides, the exception is thrown too late: the
> decorated collection has already been mutated.
>
> *Affected code*
> File:
> `src/main/java/org/apache/commons/collections4/collection/IndexedCollection.java`
> {code:java}
> @Override
> public boolean add(final C object) {
> final boolean added = super.add(object);
> if (added) {
> addToIndex(object);
> }
> return added;
> }
> private void addToIndex(final C object) {
> final K key = keyTransformer.apply(object);
> if (uniqueIndex && index.containsKey(key)) {
> throw new IllegalArgumentException("Duplicate key in uniquely indexed
> collection.");
> }
> index.put(key, object);
> } {code}
> *Reproducer*
> Add the following test to
> `src/test/java/org/apache/commons/collections4/collection/IndexedCollectionTest.java`:
> {code:java}
> @Test
> void testFailedUniqueAddDoesNotPolluteDecoratedCollection() {
> final IndexedCollection<Integer, String> indexed =
> decorateUniqueCollection(new ArrayList<>());
> indexed.add("01");
> assertThrows(IllegalArgumentException.class, () -> indexed.add("1"));
> assertEquals(1, indexed.size());
> assertEquals(asList("01"), new ArrayList<>(indexed));
> } {code}
> Both `"01"` and `"1"` map to the same integer key `1`.
> Run:
> {code:java}
> mvn -q
> -Dtest=org.apache.commons.collections4.collection.IndexedCollectionTest#testFailedUniqueAddDoesNotPolluteDecoratedCollection
> test {code}
> Observed behavior
> {code:java}
> expected: <1> but was: <2> {code}
> *Expected behavior*
> After the duplicate-key `add(...)` fails, the collection should still contain
> only the original element `"01"`.
> This is not merely about exception atomicity in the abstract.
> `IndexedCollection` promises a unique index, but after a failed add:
> - the decorated collection contains both values
> - the index contains only the original key mapping
> So the collection and its index immediately diverge, and the uniqueness
> guarantee is no longer reflected by the actual collection state.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
