OldCache violates the Map contract

[ben-manes]

When testing for https://github.com/oracle/coherence/issues/16#issuecomment-1025074742, I used Guava's testlib to validate conformance to the Collections Framework contracts. Running these tests against the existing OldCache uncovered a bug in the iterator.

CollectionIteratorTester.testIterator_unknownOrderRemoveSupported

``` junit.framework.AssertionFailedError: failed with stimuli [hasNext, hasNext, next, hasNext, remove] at com.google.common.collect.testing.Helpers.fail(Helpers.java:248) at com.google.common.collect.testing.AbstractIteratorTester.compareResultsForThisListOfStimuli(AbstractIteratorTester.java:370) at com.google.common.collect.testing.AbstractIteratorTester.recurse(AbstractIteratorTester.java:344) at com.google.common.collect.testing.AbstractIteratorTester.recurse(AbstractIteratorTester.java:349) at com.google.common.collect.testing.AbstractIteratorTester.recurse(AbstractIteratorTester.java:349) at com.google.common.collect.testing.AbstractIteratorTester.recurse(AbstractIteratorTester.java:349) at com.google.common.collect.testing.AbstractIteratorTester.recurse(AbstractIteratorTester.java:349) at com.google.common.collect.testing.AbstractIteratorTester.recurse(AbstractIteratorTester.java:349) at com.google.common.collect.testing.AbstractIteratorTester.test(AbstractIteratorTester.java:317) at com.google.common.collect.testing.testers.CollectionIteratorTester.runIteratorTest(CollectionIteratorTester.java:133) at com.google.common.collect.testing.testers.CollectionIteratorTester.testIterator_unknownOrderRemoveSupported(CollectionIteratorTester.java:111) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:568) at junit.framework.TestCase.runTest(TestCase.java:177) at junit.framework.TestCase.runBare(TestCase.java:142) at junit.framework.TestResult$1.protect(TestResult.java:122) at junit.framework.TestResult.runProtected(TestResult.java:142) at junit.framework.TestResult.run(TestResult.java:125) at junit.framework.TestCase.run(TestCase.java:130) at junit.framework.TestSuite.runTest(TestSuite.java:241) at junit.framework.TestSuite.run(TestSuite.java:236) at junit.framework.TestSuite.runTest(TestSuite.java:241) at junit.framework.TestSuite.run(TestSuite.java:236) at junit.framework.TestSuite.runTest(TestSuite.java:241) at junit.framework.TestSuite.run(TestSuite.java:236) at junit.framework.TestSuite.runTest(TestSuite.java:241) at junit.framework.TestSuite.run(TestSuite.java:236) at junit.framework.TestSuite.runTest(TestSuite.java:241) at junit.framework.TestSuite.run(TestSuite.java:236) at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:90) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69) at com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38) at com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11) at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35) at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:235) at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:54) Caused by: junit.framework.AssertionFailedError: Target threw exception when reference did not at com.google.common.collect.testing.Helpers.fail(Helpers.java:248) at com.google.common.collect.testing.AbstractIteratorTester.internalExecuteAndCompare(AbstractIteratorTester.java:442) at com.google.common.collect.testing.AbstractIteratorTester.access$600(AbstractIteratorTester.java:44) at com.google.common.collect.testing.AbstractIteratorTester$8.executeAndCompare(AbstractIteratorTester.java:553) at com.google.common.collect.testing.AbstractIteratorTester.compareResultsForThisListOfStimuli(AbstractIteratorTester.java:367) ... 37 more Caused by: java.lang.IllegalStateException at com.tangosol.util.FilterEnumerator.remove(FilterEnumerator.java:157) at com.google.common.collect.testing.AbstractIteratorTester$1.execute(AbstractIteratorTester.java:469) at com.google.common.collect.testing.AbstractIteratorTester.internalExecuteAndCompare(AbstractIteratorTester.java:398) ... 40 more ```

There were also failures due to the custom string representation. Unfortunately there is not a setting to ignore that, as it does seem reasonable for your use-case when debugging. Source code below.

OldCacheMapTests

```java import com.google.common.collect.testing.MapTestSuiteBuilder; import com.google.common.collect.testing.TestStringMapGenerator; import com.google.common.collect.testing.features.CollectionFeature; import com.google.common.collect.testing.features.CollectionSize; import com.google.common.collect.testing.features.MapFeature; import com.tangosol.net.cache.OldCache; import java.util.Map; import java.util.Map.Entry; import junit.framework.TestCase; import junit.framework.TestSuite; /** Guava testlib map tests. */ public final class OldCacheMapTests extends TestCase { public static TestSuite suite() { return MapTestSuiteBuilder .using(generator()) .named("OldCache") .withFeatures( CollectionSize.ANY, MapFeature.GENERAL_PURPOSE, MapFeature.ALLOWS_NULL_KEYS, MapFeature.ALLOWS_NULL_VALUES, CollectionFeature.SUPPORTS_ITERATOR_REMOVE) .createTestSuite(); } private static TestStringMapGenerator generator() { return new TestStringMapGenerator() { @Override protected Map create(Entry[] entries) { var cache = new OldCache(); for (var entry : entries) { cache.put(entry.getKey(), entry.getValue()); } return cache; } }; } } ```

[aseovic]

Other than documenting difference in behavior, I'm not sure we can do much about this.

The issue is that the call to hasNext advances underlying iterator and pre-fetches the value to be returned by the subsequent call to next, in order to filter out expired entries. In the process, it explicitly disables remove and causes it to throw IllegalStateException, because at that point underlying iterator is already at the next value, and calling remove would remove wrong value.

It's not ideal, but it's only an issue if you call hasNext between the calls to next and remove, so in most typical usage scenarios it shouldn't be a problem.

[ben-manes]

fwiw, I only just realized you can suppress individual tests. You might consider incorporating their suite as a freebie, but disabling those two that don't fit. That is what fastutils did for their usage.

Caffeine has the same need to peek ahead and passes this test case. Maybe it's iterator implementation would be a helpful reference.

[ben-manes]

To disable some of the Guava tests see FastUtil's setup. This uses suppressing(Method method).

/**
 * Prevents the given methods from being run as part of the test suite.
 *
 * <p><em>Note:</em> in principle this should never need to be used, but it might be useful if the
 * semantics of an implementation disagree in unforeseen ways with the semantics expected by a
 * test, or to keep dependent builds clean in spite of an erroneous test.
 */
public B suppressing(Method... methods)