[
https://issues.apache.org/jira/browse/COLLECTIONS-891?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18090275#comment-18090275
]
Gary D. Gregory commented on COLLECTIONS-891:
---------------------------------------------
It's not clear to me whether the logic in this ticket and PR breaks the intent
or the class. It certainly changes the behavior of one method drastically.
Here is the current Javadoc:
{code:java}
/**
* An IndexedCollection is a Map-like view onto a Collection. It accepts a
* keyTransformer to define how the keys are converted from the values.
* <p>
* Modifications made to this decorator modify the index as well as the
* decorated {@link Collection}. However, modifications to the underlying
* {@link Collection} will not update the index and it will get out of sync.
* </p>
* <p>
* If modification of the decorated {@link Collection} is unavoidable, then a
* call to {@link #reindex()} will update the index to the current contents of
* the {@link Collection}.
* </p>
*
* @param <K> the type of object in the index.
* @param <C> the type of object in the collection.
* @since 4.0
*/
{code}
In particular:
{quote}It accepts a keyTransformer to define how the keys are converted from
the values."
{quote}
which reads better IMO by flipping the text into a from-to description:
{quote}It accepts a keyTransformer to convert the values into keys.
{quote}
[~rickydong] writes:
{quote}`IndexedCollection` is still a `Collection`, so `contains( x )` should
answer whether an equal object is present in the collection.
{quote}
This is indeed a Collection but a _specialized_ collection, where many methods
are reimplemented with _specialized_ behavior.
The question here is how does _this_ class' specialization should affect the
{{contains()}} method.
I don't see anything in the class or method Javadoc that indicates the behavior
should be different from the current code. While this has to be balanced with
the {{Collection}} contract, I am quite concerned that changing this behavior
would have bother unintended consequences for call sites and doesn't allow for
the old behavior.
Discuss!
> IndexedCollection.contains() can return true for objects that were never added
> ------------------------------------------------------------------------------
>
> Key: COLLECTIONS-891
> URL: https://issues.apache.org/jira/browse/COLLECTIONS-891
> Project: Commons Collections
> Issue Type: Bug
> Components: Collection
> Reporter: Ruiqi Dong
> Priority: Minor
>
> *Summary*
> `IndexedCollection` is still a `Collection`, so `contains( x )` should answer
> whether an equal object is present in the collection.
>
> The current implementation answers a different question: whether the
> collection contains *any* element whose transformed key matches
> `keyTransformer.apply( x )`.
> That produces false positives whenever two different objects map to the same
> key.
>
> *Affected code*
> File:
> `src/main/java/org/apache/commons/collections4/collection/IndexedCollection.java`
> {code:java}
> @SuppressWarnings("unchecked")
> @Override
> public boolean contains(final Object object) {
> return index.containsKey(keyTransformer.apply((C) object));
> } {code}
> *Reproducer*
> Add the following test to
> `src/test/java/org/apache/commons/collections4/collection/IndexedCollectionTest.java`:
> {code:java}
> @Test
> void testContainsUsesObjectEqualityNotOnlyTransformedKey() {
> final Collection<String> coll = makeUniqueTestCollection();
> coll.add("01");
> assertFalse(coll.contains("1"));
> } {code}
> This uses the existing `IntegerTransformer`, so both `"01"` and `"1"` map to
> the same key `1`, but the objects are not equal.
>
> Run:
> {code:java}
> mvn -q
> -Dtest=org.apache.commons.collections4.collection.IndexedCollectionTest#testContainsUsesObjectEqualityNotOnlyTransformedKey
> test {code}
> *Observed behavior*
> {code:java}
> expected: <false> but was: <true> {code}
> *Expected behavior*
> `contains("1")` should be `false`, because only `"01"` was added to the
> collection.
>
> The class is a `Collection` decorator, not just a key index. Its `contains`
> method must preserve collection membership semantics. Matching on transformed
> keys instead of object equality breaks the `Collection.contains` contract
> whenever the transformer is not injective.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
