[
https://issues.apache.org/jira/browse/COLLECTIONS-891?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18091105#comment-18091105
]
Ruiqi Dong commented on COLLECTIONS-891:
----------------------------------------
Thanks, I agree this is a contract/compatibility question and not just an
implementation detail.
The reason I read the current behavior as a bug is the contains(Object)
method's own Javadoc (lines 150-155 in IndexexCollection.java):
{code:java}
{@inheritDoc}
Note: uses the index for fast lookup.{code}
To me, \{@inheritDoc} means this method is inheriting the
Collection.contains(Object) contract: membership by object equality. The index
note then describes the implementation strategy, not a semantic change. The
patch keeps that property: it still uses the index to narrow the lookup to the
transformed-key bucket, and then applies the Collection membership check inside
that bucket.
The old behavior is problematic because contains(Object) is used by standard
Collection operations and callers that expect Collection semantics. With a
non-injective transformer, contains("1") can return true even when only "01" is
present, just because both map to the same key.
For the key-oriented lookup, IndexedCollection already exposes key-typed
methods: get(K) and values(K). In particular, values(key) != null answers
whether the index has values for that key. I also noticed the values(K) Javadoc
currently says "null if contains(key) == false"; that wording is probably part
of the ambiguity here, since it informally describes a key-based contains even
though contains(Object) inherits Collection semantics. I would be happy to
tighten that Javadoc to "null if the key is absent" if we keep the
Collection.contains direction.
That said, I understand the backward-compatibility concern. If the intended
contract is that contains(Object) is key-based for this specialized collection,
then I agree the implementation should probably stay as-is and the
contains(Object) Javadoc should explicitly document that it intentionally
differs from Collection.contains(Object). My preference is to keep Collection
semantics for contains(Object) and leave key lookup to get(K)/values(K), but I
am happy to follow the direction you prefer.
> 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)
