DonalEvans commented on a change in pull request #7278: URL: https://github.com/apache/geode/pull/7278#discussion_r792193884
########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java ########## @@ -208,42 +207,26 @@ static int setOpStoreResult(RegionProvider regionProvider, RedisKey destinationK return result.size(); } - public Pair<BigInteger, List<Object>> sscan(GlobPattern matchPattern, int count, - BigInteger cursor) { - List<Object> returnList = new ArrayList<>(); - int size = members.size(); - BigInteger beforeCursor = new BigInteger("0"); - int numElements = 0; - int i = -1; - for (byte[] value : members) { - i++; - if (beforeCursor.compareTo(cursor) < 0) { - beforeCursor = beforeCursor.add(new BigInteger("1")); - continue; - } + public Pair<Integer, List<byte[]>> sscan(GlobPattern matchPattern, int count, + int cursor) { + long maximumCapacity = Math.min(count, scard() + 1); Review comment: This can be an `int` which removed the need for a cast on line 213. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -156,152 +215,172 @@ public void givenNonexistentKey_andCursorIsNotInteger_returnsInvalidCursorError( @Test public void givenExistentSetKey_andCursorIsNotAnInteger_returnsInvalidCursorError() { - jedis.set("a", "b"); + jedis.set(KEY, "b"); - assertThatThrownBy(() -> jedis.sendCommand("a", Protocol.Command.SSCAN, "a", "sjfls")) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, "sjfls")) .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNonexistentKey_returnsEmptyArray() { - ScanResult<String> result = jedis.sscan("nonexistent", "0"); + ScanResult<String> result = jedis.sscan("nonexistent", ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); assertThat(result.getResult()).isEmpty(); } + @Test + public void negativeCursor_doesNotError() { + List<String> set = initializeThreeMemberSet(); + + String cursor = "-100"; + ScanResult<String> result; + List<String> allEntries = new ArrayList<>(); + + do { + result = jedis.sscan(KEY, cursor); + allEntries.addAll(result.getResult()); + cursor = result.getCursor(); + } while (!result.isCompleteIteration()); + + assertThat(allEntries).hasSize(3); + assertThat(allEntries).containsExactlyInAnyOrderElementsOf(set); + } + @Test public void givenSetWithOneMember_returnsMember() { - jedis.sadd("a", "1"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, "1"); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactly("1"); + assertThat(result.getResult()).containsOnly(FIELD_ONE); } @Test - public void givenSetWithMultipleMembers_returnsAllMembers() { - jedis.sadd("a", "1", "2", "3"); - ScanResult<String> result = jedis.sscan("a", "0"); + public void givenSetWithMultipleMembers_returnsFewMembers() { + final List<String> initialMemberData = makeSet(); + jedis.sadd(KEY, initialMemberData.toArray(new String[initialMemberData.size()])); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); - assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactlyInAnyOrder("1", "2", "3"); + assertThat(result.getResult()).containsAnyElementsOf(initialMemberData); Review comment: This assertion should be `isSubsetOf()` ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -323,26 +402,204 @@ public void givenNegativeCursor_returnsMembersUsingAbsoluteValueOfCursor() { @Test public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, UNSIGNED_LONG_CAPACITY.add(BigInteger.ONE).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNegativeCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "-18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, NEGATIVE_LONG_CAPACITY.add(BigInteger.valueOf(-1)).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenInvalidRegexSyntax_returnsEmptyArray() { - jedis.sadd("a", "1"); + jedis.sadd(KEY, "1"); ScanParams scanParams = new ScanParams(); scanParams.count(1); scanParams.match("\\p"); ScanResult<byte[]> result = - jedis.sscan("a".getBytes(), "0".getBytes(), scanParams); + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); assertThat(result.getResult()).isEmpty(); } + + @Test + public void should_notReturnValue_givenValueWasRemovedBeforeSSCANISCalled() { + List<String> set = initializeThreeMemberSet(); + + jedis.srem(KEY, FIELD_THREE); + set.remove(FIELD_THREE); + + GeodeAwaitility.await().untilAsserted( + () -> assertThat(jedis.sismember(KEY, FIELD_THREE)).isFalse()); + + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); + + assertThat(result.getResult()).containsExactlyInAnyOrderElementsOf(set); + } + + @Test + public void should_notErrorGivenNonzeroCursorOnFirstCall() { + List<String> set = initializeThreeMemberSet(); + + ScanResult<String> result = jedis.sscan(KEY, "5"); + + assertThat(result.getResult()).isSubsetOf(set); + } + + @Test + public void should_notErrorGivenCountEqualToIntegerMaxValue() { + List<byte[]> set = initializeThreeMemberByteSet(); + + ScanParams scanParams = new ScanParams().count(Integer.MAX_VALUE); + + ScanResult<byte[]> result = + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); + assertThat(result.getResult()) + .containsExactlyInAnyOrderElementsOf(set); + } + + @Test + public void should_notErrorGivenCountGreaterThanIntegerMaxValue() { + initializeThreeMemberByteSet(); + + String greaterThanInt = String.valueOf(2L * Integer.MAX_VALUE); + List<Object> result = + uncheckedCast(jedis.sendCommand(KEY.getBytes(), Protocol.Command.SSCAN, + KEY.getBytes(), ZERO_CURSOR.getBytes(), + "COUNT".getBytes(), greaterThanInt.getBytes())); + + assertThat((byte[]) result.get(0)).isEqualTo(ZERO_CURSOR.getBytes()); + + List<byte[]> fields = uncheckedCast(result.get(1)); + assertThat(fields).containsExactlyInAnyOrder( + FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); + } + + /**** Concurrency ***/ + + @Test + public void should_returnAllConsistentlyPresentMembers_givenConcurrentThreadsAddingAndRemovingMembers() { + final List<String> initialMemberData = makeSet(); + jedis.sadd(KEY, initialMemberData.toArray(new String[initialMemberData.size()])); + final int iterationCount = 500; + + Jedis jedis1 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + Jedis jedis2 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + + new ConcurrentLoopingThreads(iterationCount, + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis1, initialMemberData), + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis2, initialMemberData), + (i) -> { + String field = "new_" + BASE_FIELD + i; + jedis.sadd(KEY, field); + jedis.srem(KEY, field); + }).run(); + + jedis1.close(); + jedis2.close(); + } + + @Test + public void should_notAlterUnderlyingData_givenMultipleConcurrentSscans() { + final List<String> initialMemberData = makeSet(); + jedis.sadd(KEY, initialMemberData.toArray(new String[initialMemberData.size()])); + final int iterationCount = 500; + + Jedis jedis1 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + Jedis jedis2 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + + new ConcurrentLoopingThreads(iterationCount, + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis1, initialMemberData), + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis2, initialMemberData)) + .run(); + + initialMemberData + .forEach((member) -> assertThat(jedis.sismember(KEY, member)).isTrue()); + + jedis1.close(); + jedis2.close(); + } + + private void multipleSScanAndAssertOnContentOfResultSet(int iteration, Jedis jedis, + final List<String> initialMemberData) { + + List<String> allEntries = new ArrayList<>(); + ScanResult<String> result; + String cursor = ZERO_CURSOR; + + do { + result = jedis.sscan(KEY, cursor); + cursor = result.getCursor(); + List<String> resultEntries = result.getResult(); + resultEntries + .forEach((entry) -> allEntries.add(entry)); + } while (!result.isCompleteIteration()); + + assertThat(allEntries).as("failed on iteration " + iteration) + .containsAll(initialMemberData); + } + + private void multipleSScanAndAssertOnSizeOfResultSet(Jedis jedis, Review comment: This method is never used and should be removed. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -323,26 +409,217 @@ public void givenNegativeCursor_returnsMembersUsingAbsoluteValueOfCursor() { @Test public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, UNSIGNED_LONG_CAPACITY.add(BigInteger.ONE).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNegativeCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "-18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, SIGNED_LONG_CAPACITY.add(BigInteger.valueOf(-1)).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenInvalidRegexSyntax_returnsEmptyArray() { Review comment: > This test is flawed, as there is no such thing as an invalid syntax for glob-style patterns. This test is just testing that given a non-matching pattern, an empty array is returned, so the name should reflect that. In fact, if the test is modified to the below, then it fails, as the element `p` matches and is returned: > > ``` > @Test > public void givenInvalidRegexSyntax_returnsEmptyArray() { > jedis.sadd(KEY, "\\p", "p"); > ScanParams scanParams = new ScanParams(); > scanParams.count(10); > scanParams.match("\\p"); > > ScanResult<byte[]> result = > jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); > > assertThat(result.getResult()).isEmpty(); > } > ``` This comment still applies. The test should be renamed to something along the lines of "givenNonMatchingPattern_returnsEmptyResult()" and the matching pattern changed to something without backslashes in it, as they just complicate things unnecessarily. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -156,152 +215,172 @@ public void givenNonexistentKey_andCursorIsNotInteger_returnsInvalidCursorError( @Test public void givenExistentSetKey_andCursorIsNotAnInteger_returnsInvalidCursorError() { - jedis.set("a", "b"); + jedis.set(KEY, "b"); - assertThatThrownBy(() -> jedis.sendCommand("a", Protocol.Command.SSCAN, "a", "sjfls")) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, "sjfls")) .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNonexistentKey_returnsEmptyArray() { - ScanResult<String> result = jedis.sscan("nonexistent", "0"); + ScanResult<String> result = jedis.sscan("nonexistent", ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); assertThat(result.getResult()).isEmpty(); } + @Test + public void negativeCursor_doesNotError() { + List<String> set = initializeThreeMemberSet(); + + String cursor = "-100"; + ScanResult<String> result; + List<String> allEntries = new ArrayList<>(); + + do { + result = jedis.sscan(KEY, cursor); + allEntries.addAll(result.getResult()); + cursor = result.getCursor(); + } while (!result.isCompleteIteration()); + + assertThat(allEntries).hasSize(3); + assertThat(allEntries).containsExactlyInAnyOrderElementsOf(set); + } + @Test public void givenSetWithOneMember_returnsMember() { - jedis.sadd("a", "1"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, "1"); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactly("1"); + assertThat(result.getResult()).containsOnly(FIELD_ONE); } @Test - public void givenSetWithMultipleMembers_returnsAllMembers() { - jedis.sadd("a", "1", "2", "3"); - ScanResult<String> result = jedis.sscan("a", "0"); + public void givenSetWithMultipleMembers_returnsFewMembers() { + final List<String> initialMemberData = makeSet(); Review comment: Since we're explicitly dealing with sets, `makeSet()` should probably return a Set rather than a List. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -156,152 +215,172 @@ public void givenNonexistentKey_andCursorIsNotInteger_returnsInvalidCursorError( @Test public void givenExistentSetKey_andCursorIsNotAnInteger_returnsInvalidCursorError() { - jedis.set("a", "b"); + jedis.set(KEY, "b"); - assertThatThrownBy(() -> jedis.sendCommand("a", Protocol.Command.SSCAN, "a", "sjfls")) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, "sjfls")) .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNonexistentKey_returnsEmptyArray() { - ScanResult<String> result = jedis.sscan("nonexistent", "0"); + ScanResult<String> result = jedis.sscan("nonexistent", ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); assertThat(result.getResult()).isEmpty(); } + @Test + public void negativeCursor_doesNotError() { + List<String> set = initializeThreeMemberSet(); + + String cursor = "-100"; + ScanResult<String> result; + List<String> allEntries = new ArrayList<>(); + + do { + result = jedis.sscan(KEY, cursor); + allEntries.addAll(result.getResult()); + cursor = result.getCursor(); + } while (!result.isCompleteIteration()); + + assertThat(allEntries).hasSize(3); + assertThat(allEntries).containsExactlyInAnyOrderElementsOf(set); + } + @Test public void givenSetWithOneMember_returnsMember() { - jedis.sadd("a", "1"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, "1"); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactly("1"); + assertThat(result.getResult()).containsOnly(FIELD_ONE); } @Test - public void givenSetWithMultipleMembers_returnsAllMembers() { - jedis.sadd("a", "1", "2", "3"); - ScanResult<String> result = jedis.sscan("a", "0"); + public void givenSetWithMultipleMembers_returnsFewMembers() { Review comment: This test might be better named something like "givenSetWithMultipleMembers_returnsSubsetOfMembers" ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -156,152 +215,172 @@ public void givenNonexistentKey_andCursorIsNotInteger_returnsInvalidCursorError( @Test public void givenExistentSetKey_andCursorIsNotAnInteger_returnsInvalidCursorError() { - jedis.set("a", "b"); + jedis.set(KEY, "b"); - assertThatThrownBy(() -> jedis.sendCommand("a", Protocol.Command.SSCAN, "a", "sjfls")) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, "sjfls")) .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNonexistentKey_returnsEmptyArray() { - ScanResult<String> result = jedis.sscan("nonexistent", "0"); + ScanResult<String> result = jedis.sscan("nonexistent", ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); assertThat(result.getResult()).isEmpty(); } + @Test + public void negativeCursor_doesNotError() { + List<String> set = initializeThreeMemberSet(); + + String cursor = "-100"; + ScanResult<String> result; + List<String> allEntries = new ArrayList<>(); + + do { + result = jedis.sscan(KEY, cursor); + allEntries.addAll(result.getResult()); + cursor = result.getCursor(); + } while (!result.isCompleteIteration()); + + assertThat(allEntries).hasSize(3); + assertThat(allEntries).containsExactlyInAnyOrderElementsOf(set); + } Review comment: Given that this test is just intending to show that a negative cursor value does not cause an error (behaviour with a negative cursor is undefined) we should only be asserting that no error is seen. This test should therefore be just: ``` @Test public void negativeCursor_doesNotError() { initializeThreeMemberSet(); assertThatNoException().isThrownBy(() -> jedis.sscan(KEY, "-100")); } ``` ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -55,95 +73,136 @@ public void tearDown() { @Test public void givenNoKeyArgument_returnsWrongNumberOfArgumentsError() { - assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SSCAN)) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN)) .hasMessageContaining("ERR wrong number of arguments for 'sscan' command"); } @Test - public void givenNoCursorArgument_returnsWrongNumberOfArgumentsError() { - assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SSCAN, "key")) + public void givenIncorrectOptionalArgumentAndKeyExists_returnsSyntaxError() { + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY)) .hasMessageContaining("ERR wrong number of arguments for 'sscan' command"); } Review comment: These tests should be replaced with a single test, "givenLessThanTwoArguments_returnsWrongNumberOfArgumentsError": ``` @Test public void givenLessThanTwoArguments_returnsWrongNumberOfArgumentsError() { assertAtLeastNArgs(jedis, Protocol.Command.SSCAN, 2); } ``` ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -323,26 +402,204 @@ public void givenNegativeCursor_returnsMembersUsingAbsoluteValueOfCursor() { @Test public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, UNSIGNED_LONG_CAPACITY.add(BigInteger.ONE).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNegativeCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "-18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, NEGATIVE_LONG_CAPACITY.add(BigInteger.valueOf(-1)).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenInvalidRegexSyntax_returnsEmptyArray() { - jedis.sadd("a", "1"); + jedis.sadd(KEY, "1"); ScanParams scanParams = new ScanParams(); scanParams.count(1); scanParams.match("\\p"); ScanResult<byte[]> result = - jedis.sscan("a".getBytes(), "0".getBytes(), scanParams); + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); assertThat(result.getResult()).isEmpty(); } + + @Test + public void should_notReturnValue_givenValueWasRemovedBeforeSSCANISCalled() { + List<String> set = initializeThreeMemberSet(); + + jedis.srem(KEY, FIELD_THREE); + set.remove(FIELD_THREE); + + GeodeAwaitility.await().untilAsserted( + () -> assertThat(jedis.sismember(KEY, FIELD_THREE)).isFalse()); + + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); + + assertThat(result.getResult()).containsExactlyInAnyOrderElementsOf(set); + } + + @Test + public void should_notErrorGivenNonzeroCursorOnFirstCall() { Review comment: This test should be changed to: ``` @Test public void should_notErrorGivenNonzeroCursorOnFirstCall() { initializeThreeMemberSet(); assertThatNoException().isThrownBy(() -> jedis.sscan(KEY, "5")); } ``` ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -323,26 +402,204 @@ public void givenNegativeCursor_returnsMembersUsingAbsoluteValueOfCursor() { @Test public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, UNSIGNED_LONG_CAPACITY.add(BigInteger.ONE).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNegativeCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "-18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, NEGATIVE_LONG_CAPACITY.add(BigInteger.valueOf(-1)).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenInvalidRegexSyntax_returnsEmptyArray() { - jedis.sadd("a", "1"); + jedis.sadd(KEY, "1"); ScanParams scanParams = new ScanParams(); scanParams.count(1); scanParams.match("\\p"); ScanResult<byte[]> result = - jedis.sscan("a".getBytes(), "0".getBytes(), scanParams); + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); assertThat(result.getResult()).isEmpty(); } + + @Test + public void should_notReturnValue_givenValueWasRemovedBeforeSSCANISCalled() { + List<String> set = initializeThreeMemberSet(); + + jedis.srem(KEY, FIELD_THREE); + set.remove(FIELD_THREE); + + GeodeAwaitility.await().untilAsserted( + () -> assertThat(jedis.sismember(KEY, FIELD_THREE)).isFalse()); + + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); + + assertThat(result.getResult()).containsExactlyInAnyOrderElementsOf(set); + } + + @Test + public void should_notErrorGivenNonzeroCursorOnFirstCall() { + List<String> set = initializeThreeMemberSet(); + + ScanResult<String> result = jedis.sscan(KEY, "5"); + + assertThat(result.getResult()).isSubsetOf(set); + } + + @Test + public void should_notErrorGivenCountEqualToIntegerMaxValue() { + List<byte[]> set = initializeThreeMemberByteSet(); + + ScanParams scanParams = new ScanParams().count(Integer.MAX_VALUE); + + ScanResult<byte[]> result = + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); + assertThat(result.getResult()) + .containsExactlyInAnyOrderElementsOf(set); + } + + @Test + public void should_notErrorGivenCountGreaterThanIntegerMaxValue() { + initializeThreeMemberByteSet(); + + String greaterThanInt = String.valueOf(2L * Integer.MAX_VALUE); + List<Object> result = + uncheckedCast(jedis.sendCommand(KEY.getBytes(), Protocol.Command.SSCAN, + KEY.getBytes(), ZERO_CURSOR.getBytes(), + "COUNT".getBytes(), greaterThanInt.getBytes())); + + assertThat((byte[]) result.get(0)).isEqualTo(ZERO_CURSOR.getBytes()); + + List<byte[]> fields = uncheckedCast(result.get(1)); + assertThat(fields).containsExactlyInAnyOrder( + FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); + } + + /**** Concurrency ***/ + + @Test + public void should_returnAllConsistentlyPresentMembers_givenConcurrentThreadsAddingAndRemovingMembers() { + final List<String> initialMemberData = makeSet(); + jedis.sadd(KEY, initialMemberData.toArray(new String[initialMemberData.size()])); Review comment: This should be `jedis.sadd(KEY, initialMemberData.toArray(new String[0]));` ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -323,26 +402,204 @@ public void givenNegativeCursor_returnsMembersUsingAbsoluteValueOfCursor() { @Test public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, UNSIGNED_LONG_CAPACITY.add(BigInteger.ONE).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNegativeCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "-18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, NEGATIVE_LONG_CAPACITY.add(BigInteger.valueOf(-1)).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenInvalidRegexSyntax_returnsEmptyArray() { - jedis.sadd("a", "1"); + jedis.sadd(KEY, "1"); ScanParams scanParams = new ScanParams(); scanParams.count(1); scanParams.match("\\p"); ScanResult<byte[]> result = - jedis.sscan("a".getBytes(), "0".getBytes(), scanParams); + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); assertThat(result.getResult()).isEmpty(); } + + @Test + public void should_notReturnValue_givenValueWasRemovedBeforeSSCANISCalled() { Review comment: For consistency, this should be "should_notReturnValue_givenValueWasRemovedBeforeSscanIsCalled" ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -156,152 +215,172 @@ public void givenNonexistentKey_andCursorIsNotInteger_returnsInvalidCursorError( @Test public void givenExistentSetKey_andCursorIsNotAnInteger_returnsInvalidCursorError() { - jedis.set("a", "b"); + jedis.set(KEY, "b"); - assertThatThrownBy(() -> jedis.sendCommand("a", Protocol.Command.SSCAN, "a", "sjfls")) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, "sjfls")) .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNonexistentKey_returnsEmptyArray() { - ScanResult<String> result = jedis.sscan("nonexistent", "0"); + ScanResult<String> result = jedis.sscan("nonexistent", ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); assertThat(result.getResult()).isEmpty(); } + @Test + public void negativeCursor_doesNotError() { + List<String> set = initializeThreeMemberSet(); + + String cursor = "-100"; + ScanResult<String> result; + List<String> allEntries = new ArrayList<>(); + + do { + result = jedis.sscan(KEY, cursor); + allEntries.addAll(result.getResult()); + cursor = result.getCursor(); + } while (!result.isCompleteIteration()); + + assertThat(allEntries).hasSize(3); + assertThat(allEntries).containsExactlyInAnyOrderElementsOf(set); + } + @Test public void givenSetWithOneMember_returnsMember() { - jedis.sadd("a", "1"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, "1"); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactly("1"); + assertThat(result.getResult()).containsOnly(FIELD_ONE); } @Test - public void givenSetWithMultipleMembers_returnsAllMembers() { - jedis.sadd("a", "1", "2", "3"); - ScanResult<String> result = jedis.sscan("a", "0"); + public void givenSetWithMultipleMembers_returnsFewMembers() { + final List<String> initialMemberData = makeSet(); + jedis.sadd(KEY, initialMemberData.toArray(new String[initialMemberData.size()])); Review comment: This should be: ``` jedis.sadd(KEY, initialMemberData.toArray(new String[0])); ``` From the IntelliJ inspection description: >In older Java versions using pre-sized array was recommended, as the reflection call which is necessary to create an array of proper size was quite slow. However since late updates of OpenJDK 6 this call was intrinsified, making the performance of the empty array version the same and sometimes even better, compared to the pre-sized version. Also passing pre-sized array is dangerous for a concurrent or synchronized collection as a data race is possible between the size and toArray call which may result in extra nulls at the end of the array, if the collection was concurrently shrunk during the operation. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -323,26 +402,204 @@ public void givenNegativeCursor_returnsMembersUsingAbsoluteValueOfCursor() { @Test public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, UNSIGNED_LONG_CAPACITY.add(BigInteger.ONE).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNegativeCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "-18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, NEGATIVE_LONG_CAPACITY.add(BigInteger.valueOf(-1)).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenInvalidRegexSyntax_returnsEmptyArray() { - jedis.sadd("a", "1"); + jedis.sadd(KEY, "1"); ScanParams scanParams = new ScanParams(); scanParams.count(1); scanParams.match("\\p"); ScanResult<byte[]> result = - jedis.sscan("a".getBytes(), "0".getBytes(), scanParams); + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); assertThat(result.getResult()).isEmpty(); } + + @Test + public void should_notReturnValue_givenValueWasRemovedBeforeSSCANISCalled() { + List<String> set = initializeThreeMemberSet(); + + jedis.srem(KEY, FIELD_THREE); + set.remove(FIELD_THREE); + + GeodeAwaitility.await().untilAsserted( + () -> assertThat(jedis.sismember(KEY, FIELD_THREE)).isFalse()); + + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); + + assertThat(result.getResult()).containsExactlyInAnyOrderElementsOf(set); + } + + @Test + public void should_notErrorGivenNonzeroCursorOnFirstCall() { + List<String> set = initializeThreeMemberSet(); + + ScanResult<String> result = jedis.sscan(KEY, "5"); + + assertThat(result.getResult()).isSubsetOf(set); + } + + @Test + public void should_notErrorGivenCountEqualToIntegerMaxValue() { + List<byte[]> set = initializeThreeMemberByteSet(); + + ScanParams scanParams = new ScanParams().count(Integer.MAX_VALUE); + + ScanResult<byte[]> result = + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); + assertThat(result.getResult()) + .containsExactlyInAnyOrderElementsOf(set); + } + + @Test + public void should_notErrorGivenCountGreaterThanIntegerMaxValue() { + initializeThreeMemberByteSet(); + + String greaterThanInt = String.valueOf(2L * Integer.MAX_VALUE); + List<Object> result = + uncheckedCast(jedis.sendCommand(KEY.getBytes(), Protocol.Command.SSCAN, + KEY.getBytes(), ZERO_CURSOR.getBytes(), + "COUNT".getBytes(), greaterThanInt.getBytes())); + + assertThat((byte[]) result.get(0)).isEqualTo(ZERO_CURSOR.getBytes()); + + List<byte[]> fields = uncheckedCast(result.get(1)); + assertThat(fields).containsExactlyInAnyOrder( + FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); + } + + /**** Concurrency ***/ + + @Test + public void should_returnAllConsistentlyPresentMembers_givenConcurrentThreadsAddingAndRemovingMembers() { + final List<String> initialMemberData = makeSet(); + jedis.sadd(KEY, initialMemberData.toArray(new String[initialMemberData.size()])); + final int iterationCount = 500; + + Jedis jedis1 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + Jedis jedis2 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + + new ConcurrentLoopingThreads(iterationCount, + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis1, initialMemberData), + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis2, initialMemberData), + (i) -> { + String field = "new_" + BASE_FIELD + i; + jedis.sadd(KEY, field); + jedis.srem(KEY, field); + }).run(); + + jedis1.close(); + jedis2.close(); + } + + @Test + public void should_notAlterUnderlyingData_givenMultipleConcurrentSscans() { + final List<String> initialMemberData = makeSet(); + jedis.sadd(KEY, initialMemberData.toArray(new String[initialMemberData.size()])); Review comment: This should be `jedis.sadd(KEY, initialMemberData.toArray(new String[0]));` ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -323,26 +402,204 @@ public void givenNegativeCursor_returnsMembersUsingAbsoluteValueOfCursor() { @Test public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, UNSIGNED_LONG_CAPACITY.add(BigInteger.ONE).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNegativeCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "-18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, NEGATIVE_LONG_CAPACITY.add(BigInteger.valueOf(-1)).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenInvalidRegexSyntax_returnsEmptyArray() { - jedis.sadd("a", "1"); + jedis.sadd(KEY, "1"); ScanParams scanParams = new ScanParams(); scanParams.count(1); scanParams.match("\\p"); ScanResult<byte[]> result = - jedis.sscan("a".getBytes(), "0".getBytes(), scanParams); + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); assertThat(result.getResult()).isEmpty(); } + + @Test + public void should_notReturnValue_givenValueWasRemovedBeforeSSCANISCalled() { + List<String> set = initializeThreeMemberSet(); + + jedis.srem(KEY, FIELD_THREE); + set.remove(FIELD_THREE); + + GeodeAwaitility.await().untilAsserted( + () -> assertThat(jedis.sismember(KEY, FIELD_THREE)).isFalse()); + + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); + + assertThat(result.getResult()).containsExactlyInAnyOrderElementsOf(set); Review comment: To be more explicit, this assert should be changed to: `assertThat(result.getResult()).doesNotContain(FIELD_THREE);` This also renders the `set` variable redundant, so it can be removed (although the call to `initializeThreeMemberSet()` will have to stay). ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -323,26 +402,204 @@ public void givenNegativeCursor_returnsMembersUsingAbsoluteValueOfCursor() { @Test public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, UNSIGNED_LONG_CAPACITY.add(BigInteger.ONE).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNegativeCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "-18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, NEGATIVE_LONG_CAPACITY.add(BigInteger.valueOf(-1)).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenInvalidRegexSyntax_returnsEmptyArray() { - jedis.sadd("a", "1"); + jedis.sadd(KEY, "1"); ScanParams scanParams = new ScanParams(); scanParams.count(1); scanParams.match("\\p"); ScanResult<byte[]> result = - jedis.sscan("a".getBytes(), "0".getBytes(), scanParams); + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); assertThat(result.getResult()).isEmpty(); } + + @Test + public void should_notReturnValue_givenValueWasRemovedBeforeSSCANISCalled() { + List<String> set = initializeThreeMemberSet(); + + jedis.srem(KEY, FIELD_THREE); + set.remove(FIELD_THREE); + + GeodeAwaitility.await().untilAsserted( + () -> assertThat(jedis.sismember(KEY, FIELD_THREE)).isFalse()); + + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); + + assertThat(result.getResult()).containsExactlyInAnyOrderElementsOf(set); + } + + @Test + public void should_notErrorGivenNonzeroCursorOnFirstCall() { + List<String> set = initializeThreeMemberSet(); + + ScanResult<String> result = jedis.sscan(KEY, "5"); + + assertThat(result.getResult()).isSubsetOf(set); + } + + @Test + public void should_notErrorGivenCountEqualToIntegerMaxValue() { + List<byte[]> set = initializeThreeMemberByteSet(); + + ScanParams scanParams = new ScanParams().count(Integer.MAX_VALUE); + + ScanResult<byte[]> result = + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); + assertThat(result.getResult()) + .containsExactlyInAnyOrderElementsOf(set); + } + + @Test + public void should_notErrorGivenCountGreaterThanIntegerMaxValue() { + initializeThreeMemberByteSet(); + + String greaterThanInt = String.valueOf(2L * Integer.MAX_VALUE); + List<Object> result = + uncheckedCast(jedis.sendCommand(KEY.getBytes(), Protocol.Command.SSCAN, + KEY.getBytes(), ZERO_CURSOR.getBytes(), + "COUNT".getBytes(), greaterThanInt.getBytes())); + + assertThat((byte[]) result.get(0)).isEqualTo(ZERO_CURSOR.getBytes()); + + List<byte[]> fields = uncheckedCast(result.get(1)); + assertThat(fields).containsExactlyInAnyOrder( + FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); + } + + /**** Concurrency ***/ + + @Test + public void should_returnAllConsistentlyPresentMembers_givenConcurrentThreadsAddingAndRemovingMembers() { + final List<String> initialMemberData = makeSet(); + jedis.sadd(KEY, initialMemberData.toArray(new String[initialMemberData.size()])); + final int iterationCount = 500; + + Jedis jedis1 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + Jedis jedis2 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + + new ConcurrentLoopingThreads(iterationCount, + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis1, initialMemberData), + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis2, initialMemberData), + (i) -> { + String field = "new_" + BASE_FIELD + i; + jedis.sadd(KEY, field); + jedis.srem(KEY, field); + }).run(); + + jedis1.close(); + jedis2.close(); + } + + @Test + public void should_notAlterUnderlyingData_givenMultipleConcurrentSscans() { + final List<String> initialMemberData = makeSet(); + jedis.sadd(KEY, initialMemberData.toArray(new String[initialMemberData.size()])); + final int iterationCount = 500; + + Jedis jedis1 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + Jedis jedis2 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + + new ConcurrentLoopingThreads(iterationCount, + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis1, initialMemberData), + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis2, initialMemberData)) + .run(); + + initialMemberData + .forEach((member) -> assertThat(jedis.sismember(KEY, member)).isTrue()); + + jedis1.close(); + jedis2.close(); + } + + private void multipleSScanAndAssertOnContentOfResultSet(int iteration, Jedis jedis, + final List<String> initialMemberData) { + + List<String> allEntries = new ArrayList<>(); + ScanResult<String> result; + String cursor = ZERO_CURSOR; + + do { + result = jedis.sscan(KEY, cursor); + cursor = result.getCursor(); + List<String> resultEntries = result.getResult(); + resultEntries + .forEach((entry) -> allEntries.add(entry)); + } while (!result.isCompleteIteration()); + + assertThat(allEntries).as("failed on iteration " + iteration) + .containsAll(initialMemberData); + } + + private void multipleSScanAndAssertOnSizeOfResultSet(Jedis jedis, + final List<String> initialMemberData) { + List<String> allEntries = new ArrayList<>(); + ScanResult<String> result; + String cursor = ZERO_CURSOR; + + do { + result = jedis.sscan(KEY, cursor); + cursor = result.getCursor(); + allEntries.addAll(result.getResult()); + } while (!result.isCompleteIteration()); + + List<String> allDistinctEntries = + allEntries + .stream() + .distinct() + .collect(Collectors.toList()); + + assertThat(allDistinctEntries.size()) + .isEqualTo(initialMemberData.size()); + } + + private List<String> initializeThreeMemberSet() { + List<String> set = new ArrayList<>(); + set.add(FIELD_ONE); + set.add(FIELD_TWO); + set.add(FIELD_THREE); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); + return set; + } + + private List<byte[]> initializeThreeMemberByteSet() { + List<byte[]> set = new ArrayList<>(); + set.add(FIELD_ONE.getBytes()); + set.add(FIELD_TWO.getBytes()); + set.add(FIELD_THREE.getBytes()); + jedis.sadd(KEY.getBytes(), FIELD_ONE.getBytes(), FIELD_TWO.getBytes(), FIELD_THREE.getBytes()); + return set; + } + + private List<String> makeSet() { + List<String> set = new ArrayList<>(); + for (int i = 0; i < SIZE_OF_SET; i++) { + set.add((BASE_FIELD + i)); + } + return set; + } + + private List<byte[]> makeByteSet() { Review comment: This method is never used and should be removed. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -323,26 +402,204 @@ public void givenNegativeCursor_returnsMembersUsingAbsoluteValueOfCursor() { @Test public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, UNSIGNED_LONG_CAPACITY.add(BigInteger.ONE).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNegativeCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "-18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, NEGATIVE_LONG_CAPACITY.add(BigInteger.valueOf(-1)).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenInvalidRegexSyntax_returnsEmptyArray() { - jedis.sadd("a", "1"); + jedis.sadd(KEY, "1"); ScanParams scanParams = new ScanParams(); scanParams.count(1); scanParams.match("\\p"); ScanResult<byte[]> result = - jedis.sscan("a".getBytes(), "0".getBytes(), scanParams); + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); assertThat(result.getResult()).isEmpty(); } + + @Test + public void should_notReturnValue_givenValueWasRemovedBeforeSSCANISCalled() { + List<String> set = initializeThreeMemberSet(); + + jedis.srem(KEY, FIELD_THREE); + set.remove(FIELD_THREE); + + GeodeAwaitility.await().untilAsserted( + () -> assertThat(jedis.sismember(KEY, FIELD_THREE)).isFalse()); + + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); + + assertThat(result.getResult()).containsExactlyInAnyOrderElementsOf(set); + } + + @Test + public void should_notErrorGivenNonzeroCursorOnFirstCall() { + List<String> set = initializeThreeMemberSet(); + + ScanResult<String> result = jedis.sscan(KEY, "5"); + + assertThat(result.getResult()).isSubsetOf(set); + } + + @Test + public void should_notErrorGivenCountEqualToIntegerMaxValue() { + List<byte[]> set = initializeThreeMemberByteSet(); + + ScanParams scanParams = new ScanParams().count(Integer.MAX_VALUE); + + ScanResult<byte[]> result = + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); + assertThat(result.getResult()) + .containsExactlyInAnyOrderElementsOf(set); + } + + @Test + public void should_notErrorGivenCountGreaterThanIntegerMaxValue() { + initializeThreeMemberByteSet(); + + String greaterThanInt = String.valueOf(2L * Integer.MAX_VALUE); + List<Object> result = + uncheckedCast(jedis.sendCommand(KEY.getBytes(), Protocol.Command.SSCAN, + KEY.getBytes(), ZERO_CURSOR.getBytes(), + "COUNT".getBytes(), greaterThanInt.getBytes())); + + assertThat((byte[]) result.get(0)).isEqualTo(ZERO_CURSOR.getBytes()); + + List<byte[]> fields = uncheckedCast(result.get(1)); + assertThat(fields).containsExactlyInAnyOrder( + FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); + } + + /**** Concurrency ***/ + + @Test + public void should_returnAllConsistentlyPresentMembers_givenConcurrentThreadsAddingAndRemovingMembers() { + final List<String> initialMemberData = makeSet(); + jedis.sadd(KEY, initialMemberData.toArray(new String[initialMemberData.size()])); + final int iterationCount = 500; + + Jedis jedis1 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + Jedis jedis2 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + + new ConcurrentLoopingThreads(iterationCount, + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis1, initialMemberData), + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis2, initialMemberData), + (i) -> { + String field = "new_" + BASE_FIELD + i; + jedis.sadd(KEY, field); + jedis.srem(KEY, field); + }).run(); + + jedis1.close(); + jedis2.close(); + } + + @Test + public void should_notAlterUnderlyingData_givenMultipleConcurrentSscans() { + final List<String> initialMemberData = makeSet(); + jedis.sadd(KEY, initialMemberData.toArray(new String[initialMemberData.size()])); + final int iterationCount = 500; + + Jedis jedis1 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + Jedis jedis2 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + + new ConcurrentLoopingThreads(iterationCount, + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis1, initialMemberData), + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis2, initialMemberData)) + .run(); + + initialMemberData + .forEach((member) -> assertThat(jedis.sismember(KEY, member)).isTrue()); Review comment: This would be better as: ``` assertThat(jedis.smembers(KEY)).containsExactlyInAnyOrderElementsOf(initialMemberData); ``` as we're currently only asserting that all the initial members are still present, but not that we haven't added any extra members somehow. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -156,152 +222,172 @@ public void givenNonexistentKey_andCursorIsNotInteger_returnsInvalidCursorError( @Test public void givenExistentSetKey_andCursorIsNotAnInteger_returnsInvalidCursorError() { - jedis.set("a", "b"); + jedis.set(KEY, "b"); - assertThatThrownBy(() -> jedis.sendCommand("a", Protocol.Command.SSCAN, "a", "sjfls")) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, "sjfls")) .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNonexistentKey_returnsEmptyArray() { - ScanResult<String> result = jedis.sscan("nonexistent", "0"); + ScanResult<String> result = jedis.sscan("nonexistent", ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); assertThat(result.getResult()).isEmpty(); } + @Test + public void givenNegativeCursor_returnsEntriesUsingAbsoluteValueOfCursor() { + List<String> set = initializeThreeMemberSet(); + + String cursor = "-100"; + ScanResult<String> result; + List<String> allEntries = new ArrayList<>(); + + do { + result = jedis.sscan(KEY, cursor); + allEntries.addAll(result.getResult()); + cursor = result.getCursor(); + } while (!result.isCompleteIteration()); + + assertThat(allEntries).hasSize(3); + assertThat(allEntries).containsExactlyInAnyOrderElementsOf(set); + } + @Test public void givenSetWithOneMember_returnsMember() { - jedis.sadd("a", "1"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, FIELD_ONE); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactly("1"); + assertThat(result.getResult()).containsExactly(FIELD_ONE); } @Test public void givenSetWithMultipleMembers_returnsAllMembers() { - jedis.sadd("a", "1", "2", "3"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactlyInAnyOrder("1", "2", "3"); + assertThat(result.getResult()).containsExactlyInAnyOrder(FIELD_ONE, FIELD_TWO, FIELD_THREE); } @Test public void givenCount_returnsAllMembersWithoutDuplicates() { - jedis.sadd("a", "1", "2", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); ScanParams scanParams = new ScanParams(); scanParams.count(1); - String cursor = "0"; + String cursor = ZERO_CURSOR; ScanResult<byte[]> result; List<byte[]> allMembersFromScan = new ArrayList<>(); do { - result = jedis.sscan("a".getBytes(), cursor.getBytes(), scanParams); + result = jedis.sscan(KEY.getBytes(), cursor.getBytes(), scanParams); allMembersFromScan.addAll(result.getResult()); cursor = result.getCursor(); } while (!result.isCompleteIteration()); - assertThat(allMembersFromScan).containsExactlyInAnyOrder("1".getBytes(), - "2".getBytes(), - "3".getBytes()); + assertThat(allMembersFromScan).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); } @Test @SuppressWarnings("unchecked") public void givenMultipleCounts_returnsAllEntriesWithoutDuplicates() { Review comment: > This test is flawed, as for small set sizes, native Redis will return all elements in one SSCAN regardless of the value of COUNT, and duplicate elements are allowed. A significant improvement would be to populate a set with 1000 elements, then call SSCAN once and assert that the number of elements returned is greater than or equal to the appropriate COUNT value and that they are a subset of the total set contents. This comment still applies ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -53,97 +73,143 @@ public void tearDown() { jedis.close(); } + @Test + public void givenLessThanTwoArguments_returnsWrongNumberOfArgumentsError() { + assertAtLeastNArgs(jedis, Protocol.Command.SSCAN, 2); + } + @Test public void givenNoKeyArgument_returnsWrongNumberOfArgumentsError() { - assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SSCAN)) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN)) .hasMessageContaining("ERR wrong number of arguments for 'sscan' command"); } @Test public void givenNoCursorArgument_returnsWrongNumberOfArgumentsError() { - assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SSCAN, "key")) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY)) .hasMessageContaining("ERR wrong number of arguments for 'sscan' command"); } @Test public void givenArgumentsAreNotOddAndKeyExists_returnsSyntaxError() { Review comment: >This test name could be more accurate as "givenIncorrectOptionalArgumentAndKeyExists_returnsSyntaxError" since the number of arguments isn't actually the relevant factor here, just that there is a syntax error of some kind. This comment still applies. This test name should be changed. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -156,152 +222,172 @@ public void givenNonexistentKey_andCursorIsNotInteger_returnsInvalidCursorError( @Test public void givenExistentSetKey_andCursorIsNotAnInteger_returnsInvalidCursorError() { - jedis.set("a", "b"); + jedis.set(KEY, "b"); - assertThatThrownBy(() -> jedis.sendCommand("a", Protocol.Command.SSCAN, "a", "sjfls")) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, "sjfls")) .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNonexistentKey_returnsEmptyArray() { - ScanResult<String> result = jedis.sscan("nonexistent", "0"); + ScanResult<String> result = jedis.sscan("nonexistent", ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); assertThat(result.getResult()).isEmpty(); } + @Test + public void givenNegativeCursor_returnsEntriesUsingAbsoluteValueOfCursor() { + List<String> set = initializeThreeMemberSet(); + + String cursor = "-100"; + ScanResult<String> result; + List<String> allEntries = new ArrayList<>(); + + do { + result = jedis.sscan(KEY, cursor); + allEntries.addAll(result.getResult()); + cursor = result.getCursor(); + } while (!result.isCompleteIteration()); + + assertThat(allEntries).hasSize(3); + assertThat(allEntries).containsExactlyInAnyOrderElementsOf(set); + } + @Test public void givenSetWithOneMember_returnsMember() { - jedis.sadd("a", "1"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, FIELD_ONE); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactly("1"); + assertThat(result.getResult()).containsExactly(FIELD_ONE); } @Test public void givenSetWithMultipleMembers_returnsAllMembers() { - jedis.sadd("a", "1", "2", "3"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactlyInAnyOrder("1", "2", "3"); + assertThat(result.getResult()).containsExactlyInAnyOrder(FIELD_ONE, FIELD_TWO, FIELD_THREE); } @Test public void givenCount_returnsAllMembersWithoutDuplicates() { Review comment: > This test is flawed, as for small set sizes, native Redis will return all elements in one SSCAN regardless of the value of COUNT, and duplicate elements are allowed. A significant improvement would be to populate a set with 1000 elements, then call SSCAN once and assert that the number of elements returned is greater than or equal to the appropriate COUNT value and that they are a subset of the total set contents. This comment still applies ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -323,26 +402,204 @@ public void givenNegativeCursor_returnsMembersUsingAbsoluteValueOfCursor() { @Test public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, UNSIGNED_LONG_CAPACITY.add(BigInteger.ONE).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNegativeCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "-18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, NEGATIVE_LONG_CAPACITY.add(BigInteger.valueOf(-1)).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenInvalidRegexSyntax_returnsEmptyArray() { - jedis.sadd("a", "1"); + jedis.sadd(KEY, "1"); ScanParams scanParams = new ScanParams(); scanParams.count(1); scanParams.match("\\p"); ScanResult<byte[]> result = - jedis.sscan("a".getBytes(), "0".getBytes(), scanParams); + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); assertThat(result.getResult()).isEmpty(); } + + @Test + public void should_notReturnValue_givenValueWasRemovedBeforeSSCANISCalled() { + List<String> set = initializeThreeMemberSet(); + + jedis.srem(KEY, FIELD_THREE); + set.remove(FIELD_THREE); + + GeodeAwaitility.await().untilAsserted( + () -> assertThat(jedis.sismember(KEY, FIELD_THREE)).isFalse()); + + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); + + assertThat(result.getResult()).containsExactlyInAnyOrderElementsOf(set); + } + + @Test + public void should_notErrorGivenNonzeroCursorOnFirstCall() { + List<String> set = initializeThreeMemberSet(); + + ScanResult<String> result = jedis.sscan(KEY, "5"); + + assertThat(result.getResult()).isSubsetOf(set); + } + + @Test + public void should_notErrorGivenCountEqualToIntegerMaxValue() { + List<byte[]> set = initializeThreeMemberByteSet(); + + ScanParams scanParams = new ScanParams().count(Integer.MAX_VALUE); + + ScanResult<byte[]> result = + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); + assertThat(result.getResult()) + .containsExactlyInAnyOrderElementsOf(set); + } + + @Test + public void should_notErrorGivenCountGreaterThanIntegerMaxValue() { + initializeThreeMemberByteSet(); + + String greaterThanInt = String.valueOf(2L * Integer.MAX_VALUE); + List<Object> result = + uncheckedCast(jedis.sendCommand(KEY.getBytes(), Protocol.Command.SSCAN, + KEY.getBytes(), ZERO_CURSOR.getBytes(), + "COUNT".getBytes(), greaterThanInt.getBytes())); + + assertThat((byte[]) result.get(0)).isEqualTo(ZERO_CURSOR.getBytes()); + + List<byte[]> fields = uncheckedCast(result.get(1)); + assertThat(fields).containsExactlyInAnyOrder( + FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); + } + + /**** Concurrency ***/ + + @Test + public void should_returnAllConsistentlyPresentMembers_givenConcurrentThreadsAddingAndRemovingMembers() { + final List<String> initialMemberData = makeSet(); + jedis.sadd(KEY, initialMemberData.toArray(new String[initialMemberData.size()])); + final int iterationCount = 500; + + Jedis jedis1 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + Jedis jedis2 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + + new ConcurrentLoopingThreads(iterationCount, + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis1, initialMemberData), + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis2, initialMemberData), + (i) -> { + String field = "new_" + BASE_FIELD + i; + jedis.sadd(KEY, field); + jedis.srem(KEY, field); + }).run(); + + jedis1.close(); + jedis2.close(); + } + + @Test + public void should_notAlterUnderlyingData_givenMultipleConcurrentSscans() { + final List<String> initialMemberData = makeSet(); + jedis.sadd(KEY, initialMemberData.toArray(new String[initialMemberData.size()])); + final int iterationCount = 500; + + Jedis jedis1 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + Jedis jedis2 = jedis.getConnectionFromSlot(SLOT_FOR_KEY); + + new ConcurrentLoopingThreads(iterationCount, + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis1, initialMemberData), + (i) -> multipleSScanAndAssertOnContentOfResultSet(i, jedis2, initialMemberData)) + .run(); + + initialMemberData + .forEach((member) -> assertThat(jedis.sismember(KEY, member)).isTrue()); + + jedis1.close(); + jedis2.close(); + } + + private void multipleSScanAndAssertOnContentOfResultSet(int iteration, Jedis jedis, + final List<String> initialMemberData) { + + List<String> allEntries = new ArrayList<>(); + ScanResult<String> result; + String cursor = ZERO_CURSOR; + + do { + result = jedis.sscan(KEY, cursor); + cursor = result.getCursor(); + List<String> resultEntries = result.getResult(); + resultEntries + .forEach((entry) -> allEntries.add(entry)); Review comment: This can be simplified to: ``` allEntries.addAll(result.getResult()); ``` ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -156,152 +222,172 @@ public void givenNonexistentKey_andCursorIsNotInteger_returnsInvalidCursorError( @Test public void givenExistentSetKey_andCursorIsNotAnInteger_returnsInvalidCursorError() { - jedis.set("a", "b"); + jedis.set(KEY, "b"); - assertThatThrownBy(() -> jedis.sendCommand("a", Protocol.Command.SSCAN, "a", "sjfls")) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, "sjfls")) .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNonexistentKey_returnsEmptyArray() { - ScanResult<String> result = jedis.sscan("nonexistent", "0"); + ScanResult<String> result = jedis.sscan("nonexistent", ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); assertThat(result.getResult()).isEmpty(); } + @Test + public void givenNegativeCursor_returnsEntriesUsingAbsoluteValueOfCursor() { + List<String> set = initializeThreeMemberSet(); + + String cursor = "-100"; + ScanResult<String> result; + List<String> allEntries = new ArrayList<>(); + + do { + result = jedis.sscan(KEY, cursor); + allEntries.addAll(result.getResult()); + cursor = result.getCursor(); + } while (!result.isCompleteIteration()); + + assertThat(allEntries).hasSize(3); + assertThat(allEntries).containsExactlyInAnyOrderElementsOf(set); + } + @Test public void givenSetWithOneMember_returnsMember() { - jedis.sadd("a", "1"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, FIELD_ONE); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactly("1"); + assertThat(result.getResult()).containsExactly(FIELD_ONE); } @Test public void givenSetWithMultipleMembers_returnsAllMembers() { - jedis.sadd("a", "1", "2", "3"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactlyInAnyOrder("1", "2", "3"); + assertThat(result.getResult()).containsExactlyInAnyOrder(FIELD_ONE, FIELD_TWO, FIELD_THREE); } @Test public void givenCount_returnsAllMembersWithoutDuplicates() { - jedis.sadd("a", "1", "2", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); ScanParams scanParams = new ScanParams(); scanParams.count(1); - String cursor = "0"; + String cursor = ZERO_CURSOR; ScanResult<byte[]> result; List<byte[]> allMembersFromScan = new ArrayList<>(); do { - result = jedis.sscan("a".getBytes(), cursor.getBytes(), scanParams); + result = jedis.sscan(KEY.getBytes(), cursor.getBytes(), scanParams); allMembersFromScan.addAll(result.getResult()); cursor = result.getCursor(); } while (!result.isCompleteIteration()); - assertThat(allMembersFromScan).containsExactlyInAnyOrder("1".getBytes(), - "2".getBytes(), - "3".getBytes()); + assertThat(allMembersFromScan).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); } @Test @SuppressWarnings("unchecked") public void givenMultipleCounts_returnsAllEntriesWithoutDuplicates() { - jedis.sadd("a", "1", "12", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); List<Object> result; List<byte[]> allEntries = new ArrayList<>(); - String cursor = "0"; + String cursor = ZERO_CURSOR; do { result = - (List<Object>) jedis.sendCommand("a", Protocol.Command.SSCAN, "a", cursor, "COUNT", "2", - "COUNT", "1"); + (List<Object>) jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, cursor, "COUNT", + FIELD_TWO, + "COUNT", FIELD_ONE); allEntries.addAll((List<byte[]>) result.get(1)); cursor = new String((byte[]) result.get(0)); - } while (!Arrays.equals((byte[]) result.get(0), "0".getBytes())); + } while (!Arrays.equals((byte[]) result.get(0), ZERO_CURSOR.getBytes())); - assertThat((byte[]) result.get(0)).isEqualTo("0".getBytes()); - assertThat(allEntries).containsExactlyInAnyOrder("1".getBytes(), - "12".getBytes(), - "3".getBytes()); + assertThat((byte[]) result.get(0)).isEqualTo(ZERO_CURSOR.getBytes()); + assertThat(allEntries).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); } @Test public void givenMatch_returnsAllMatchingMembersWithoutDuplicates() { - jedis.sadd("a", "1", "12", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); ScanParams scanParams = new ScanParams(); scanParams.match("1*"); ScanResult<byte[]> result = - jedis.sscan("a".getBytes(), "0".getBytes(), scanParams); + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactlyInAnyOrder("1".getBytes(), - "12".getBytes()); + assertThat(result.getResult()).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes()); } @Test @SuppressWarnings("unchecked") public void givenMultipleMatches_returnsMembersMatchingLastMatchParameter() { - jedis.sadd("a", "1", "12", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); - List<Object> result = (List<Object>) jedis.sendCommand("a", Protocol.Command.SSCAN, "a", "0", - "MATCH", "3*", "MATCH", "1*"); + List<Object> result = + (List<Object>) jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, ZERO_CURSOR, + "MATCH", "3*", "MATCH", "1*"); - assertThat((byte[]) result.get(0)).isEqualTo("0".getBytes()); - assertThat((List<byte[]>) result.get(1)).containsExactlyInAnyOrder("1".getBytes(), - "12".getBytes()); + assertThat((byte[]) result.get(0)).isEqualTo(ZERO_CURSOR.getBytes()); + assertThat((List<byte[]>) result.get(1)).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes()); } @Test public void givenMatchAndCount_returnsAllMembersWithoutDuplicates() { - jedis.sadd("a", "1", "12", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); ScanParams scanParams = new ScanParams(); scanParams.count(1); scanParams.match("1*"); ScanResult<byte[]> result; List<byte[]> allMembersFromScan = new ArrayList<>(); - String cursor = "0"; + String cursor = ZERO_CURSOR; do { - result = jedis.sscan("a".getBytes(), cursor.getBytes(), scanParams); + result = jedis.sscan(KEY.getBytes(), cursor.getBytes(), scanParams); allMembersFromScan.addAll(result.getResult()); cursor = result.getCursor(); } while (!result.isCompleteIteration()); - assertThat(allMembersFromScan).containsExactlyInAnyOrder("1".getBytes(), - "12".getBytes()); + assertThat(allMembersFromScan).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes()); } @Test @SuppressWarnings("unchecked") public void givenMultipleCountsAndMatches_returnsAllEntriesWithoutDuplicates() { Review comment: > This test would be better named "givenSetWithThreeMembersAndMultipleMatchAndCountArguments_returnsAllMatchingMembers" with the assertions changed to `containsOnly()` to allow duplicate entries. This comment still applies ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -156,152 +222,172 @@ public void givenNonexistentKey_andCursorIsNotInteger_returnsInvalidCursorError( @Test public void givenExistentSetKey_andCursorIsNotAnInteger_returnsInvalidCursorError() { - jedis.set("a", "b"); + jedis.set(KEY, "b"); - assertThatThrownBy(() -> jedis.sendCommand("a", Protocol.Command.SSCAN, "a", "sjfls")) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, "sjfls")) .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNonexistentKey_returnsEmptyArray() { - ScanResult<String> result = jedis.sscan("nonexistent", "0"); + ScanResult<String> result = jedis.sscan("nonexistent", ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); assertThat(result.getResult()).isEmpty(); } + @Test + public void givenNegativeCursor_returnsEntriesUsingAbsoluteValueOfCursor() { + List<String> set = initializeThreeMemberSet(); + + String cursor = "-100"; + ScanResult<String> result; + List<String> allEntries = new ArrayList<>(); + + do { + result = jedis.sscan(KEY, cursor); + allEntries.addAll(result.getResult()); + cursor = result.getCursor(); + } while (!result.isCompleteIteration()); + + assertThat(allEntries).hasSize(3); + assertThat(allEntries).containsExactlyInAnyOrderElementsOf(set); + } + @Test public void givenSetWithOneMember_returnsMember() { - jedis.sadd("a", "1"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, FIELD_ONE); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactly("1"); + assertThat(result.getResult()).containsExactly(FIELD_ONE); } @Test public void givenSetWithMultipleMembers_returnsAllMembers() { - jedis.sadd("a", "1", "2", "3"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactlyInAnyOrder("1", "2", "3"); + assertThat(result.getResult()).containsExactlyInAnyOrder(FIELD_ONE, FIELD_TWO, FIELD_THREE); } @Test public void givenCount_returnsAllMembersWithoutDuplicates() { - jedis.sadd("a", "1", "2", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); ScanParams scanParams = new ScanParams(); scanParams.count(1); - String cursor = "0"; + String cursor = ZERO_CURSOR; ScanResult<byte[]> result; List<byte[]> allMembersFromScan = new ArrayList<>(); do { - result = jedis.sscan("a".getBytes(), cursor.getBytes(), scanParams); + result = jedis.sscan(KEY.getBytes(), cursor.getBytes(), scanParams); allMembersFromScan.addAll(result.getResult()); cursor = result.getCursor(); } while (!result.isCompleteIteration()); - assertThat(allMembersFromScan).containsExactlyInAnyOrder("1".getBytes(), - "2".getBytes(), - "3".getBytes()); + assertThat(allMembersFromScan).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); } @Test @SuppressWarnings("unchecked") public void givenMultipleCounts_returnsAllEntriesWithoutDuplicates() { - jedis.sadd("a", "1", "12", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); List<Object> result; List<byte[]> allEntries = new ArrayList<>(); - String cursor = "0"; + String cursor = ZERO_CURSOR; do { result = - (List<Object>) jedis.sendCommand("a", Protocol.Command.SSCAN, "a", cursor, "COUNT", "2", - "COUNT", "1"); + (List<Object>) jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, cursor, "COUNT", + FIELD_TWO, + "COUNT", FIELD_ONE); allEntries.addAll((List<byte[]>) result.get(1)); cursor = new String((byte[]) result.get(0)); - } while (!Arrays.equals((byte[]) result.get(0), "0".getBytes())); + } while (!Arrays.equals((byte[]) result.get(0), ZERO_CURSOR.getBytes())); - assertThat((byte[]) result.get(0)).isEqualTo("0".getBytes()); - assertThat(allEntries).containsExactlyInAnyOrder("1".getBytes(), - "12".getBytes(), - "3".getBytes()); + assertThat((byte[]) result.get(0)).isEqualTo(ZERO_CURSOR.getBytes()); + assertThat(allEntries).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); } @Test public void givenMatch_returnsAllMatchingMembersWithoutDuplicates() { - jedis.sadd("a", "1", "12", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); ScanParams scanParams = new ScanParams(); scanParams.match("1*"); ScanResult<byte[]> result = - jedis.sscan("a".getBytes(), "0".getBytes(), scanParams); + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactlyInAnyOrder("1".getBytes(), - "12".getBytes()); + assertThat(result.getResult()).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes()); } @Test @SuppressWarnings("unchecked") public void givenMultipleMatches_returnsMembersMatchingLastMatchParameter() { Review comment: > This test is also flawed for the same reasons as earlier tests. A better name would be "givenSetWithThreeEntriesAndMultipleMatchArguments_returnsOnlyElementsMatchingLastMatchArgument" with the assertions changed to `containsOnly()` to allow duplicate entries. This comment still applies ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -156,152 +222,172 @@ public void givenNonexistentKey_andCursorIsNotInteger_returnsInvalidCursorError( @Test public void givenExistentSetKey_andCursorIsNotAnInteger_returnsInvalidCursorError() { - jedis.set("a", "b"); + jedis.set(KEY, "b"); - assertThatThrownBy(() -> jedis.sendCommand("a", Protocol.Command.SSCAN, "a", "sjfls")) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, "sjfls")) .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNonexistentKey_returnsEmptyArray() { - ScanResult<String> result = jedis.sscan("nonexistent", "0"); + ScanResult<String> result = jedis.sscan("nonexistent", ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); assertThat(result.getResult()).isEmpty(); } + @Test + public void givenNegativeCursor_returnsEntriesUsingAbsoluteValueOfCursor() { + List<String> set = initializeThreeMemberSet(); + + String cursor = "-100"; + ScanResult<String> result; + List<String> allEntries = new ArrayList<>(); + + do { + result = jedis.sscan(KEY, cursor); + allEntries.addAll(result.getResult()); + cursor = result.getCursor(); + } while (!result.isCompleteIteration()); + + assertThat(allEntries).hasSize(3); + assertThat(allEntries).containsExactlyInAnyOrderElementsOf(set); + } + @Test public void givenSetWithOneMember_returnsMember() { - jedis.sadd("a", "1"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, FIELD_ONE); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactly("1"); + assertThat(result.getResult()).containsExactly(FIELD_ONE); } @Test public void givenSetWithMultipleMembers_returnsAllMembers() { - jedis.sadd("a", "1", "2", "3"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactlyInAnyOrder("1", "2", "3"); + assertThat(result.getResult()).containsExactlyInAnyOrder(FIELD_ONE, FIELD_TWO, FIELD_THREE); } @Test public void givenCount_returnsAllMembersWithoutDuplicates() { - jedis.sadd("a", "1", "2", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); ScanParams scanParams = new ScanParams(); scanParams.count(1); - String cursor = "0"; + String cursor = ZERO_CURSOR; ScanResult<byte[]> result; List<byte[]> allMembersFromScan = new ArrayList<>(); do { - result = jedis.sscan("a".getBytes(), cursor.getBytes(), scanParams); + result = jedis.sscan(KEY.getBytes(), cursor.getBytes(), scanParams); allMembersFromScan.addAll(result.getResult()); cursor = result.getCursor(); } while (!result.isCompleteIteration()); - assertThat(allMembersFromScan).containsExactlyInAnyOrder("1".getBytes(), - "2".getBytes(), - "3".getBytes()); + assertThat(allMembersFromScan).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); } @Test @SuppressWarnings("unchecked") public void givenMultipleCounts_returnsAllEntriesWithoutDuplicates() { - jedis.sadd("a", "1", "12", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); List<Object> result; List<byte[]> allEntries = new ArrayList<>(); - String cursor = "0"; + String cursor = ZERO_CURSOR; do { result = - (List<Object>) jedis.sendCommand("a", Protocol.Command.SSCAN, "a", cursor, "COUNT", "2", - "COUNT", "1"); + (List<Object>) jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, cursor, "COUNT", + FIELD_TWO, + "COUNT", FIELD_ONE); allEntries.addAll((List<byte[]>) result.get(1)); cursor = new String((byte[]) result.get(0)); - } while (!Arrays.equals((byte[]) result.get(0), "0".getBytes())); + } while (!Arrays.equals((byte[]) result.get(0), ZERO_CURSOR.getBytes())); - assertThat((byte[]) result.get(0)).isEqualTo("0".getBytes()); - assertThat(allEntries).containsExactlyInAnyOrder("1".getBytes(), - "12".getBytes(), - "3".getBytes()); + assertThat((byte[]) result.get(0)).isEqualTo(ZERO_CURSOR.getBytes()); + assertThat(allEntries).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); } @Test public void givenMatch_returnsAllMatchingMembersWithoutDuplicates() { Review comment: > This test is also flawed for the same reasons as earlier tests. A better name would be "givenSetWithThreeEntriesAndMatch_returnsOnlyMatchingElements" with the assertions changed to `containsOnly()` to allow duplicate entries. This comment still applies ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -156,152 +222,172 @@ public void givenNonexistentKey_andCursorIsNotInteger_returnsInvalidCursorError( @Test public void givenExistentSetKey_andCursorIsNotAnInteger_returnsInvalidCursorError() { - jedis.set("a", "b"); + jedis.set(KEY, "b"); - assertThatThrownBy(() -> jedis.sendCommand("a", Protocol.Command.SSCAN, "a", "sjfls")) + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, "sjfls")) .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNonexistentKey_returnsEmptyArray() { - ScanResult<String> result = jedis.sscan("nonexistent", "0"); + ScanResult<String> result = jedis.sscan("nonexistent", ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); assertThat(result.getResult()).isEmpty(); } + @Test + public void givenNegativeCursor_returnsEntriesUsingAbsoluteValueOfCursor() { + List<String> set = initializeThreeMemberSet(); + + String cursor = "-100"; + ScanResult<String> result; + List<String> allEntries = new ArrayList<>(); + + do { + result = jedis.sscan(KEY, cursor); + allEntries.addAll(result.getResult()); + cursor = result.getCursor(); + } while (!result.isCompleteIteration()); + + assertThat(allEntries).hasSize(3); + assertThat(allEntries).containsExactlyInAnyOrderElementsOf(set); + } + @Test public void givenSetWithOneMember_returnsMember() { - jedis.sadd("a", "1"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, FIELD_ONE); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactly("1"); + assertThat(result.getResult()).containsExactly(FIELD_ONE); } @Test public void givenSetWithMultipleMembers_returnsAllMembers() { - jedis.sadd("a", "1", "2", "3"); - ScanResult<String> result = jedis.sscan("a", "0"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); + ScanResult<String> result = jedis.sscan(KEY, ZERO_CURSOR); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactlyInAnyOrder("1", "2", "3"); + assertThat(result.getResult()).containsExactlyInAnyOrder(FIELD_ONE, FIELD_TWO, FIELD_THREE); } @Test public void givenCount_returnsAllMembersWithoutDuplicates() { - jedis.sadd("a", "1", "2", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); ScanParams scanParams = new ScanParams(); scanParams.count(1); - String cursor = "0"; + String cursor = ZERO_CURSOR; ScanResult<byte[]> result; List<byte[]> allMembersFromScan = new ArrayList<>(); do { - result = jedis.sscan("a".getBytes(), cursor.getBytes(), scanParams); + result = jedis.sscan(KEY.getBytes(), cursor.getBytes(), scanParams); allMembersFromScan.addAll(result.getResult()); cursor = result.getCursor(); } while (!result.isCompleteIteration()); - assertThat(allMembersFromScan).containsExactlyInAnyOrder("1".getBytes(), - "2".getBytes(), - "3".getBytes()); + assertThat(allMembersFromScan).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); } @Test @SuppressWarnings("unchecked") public void givenMultipleCounts_returnsAllEntriesWithoutDuplicates() { - jedis.sadd("a", "1", "12", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); List<Object> result; List<byte[]> allEntries = new ArrayList<>(); - String cursor = "0"; + String cursor = ZERO_CURSOR; do { result = - (List<Object>) jedis.sendCommand("a", Protocol.Command.SSCAN, "a", cursor, "COUNT", "2", - "COUNT", "1"); + (List<Object>) jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, cursor, "COUNT", + FIELD_TWO, + "COUNT", FIELD_ONE); allEntries.addAll((List<byte[]>) result.get(1)); cursor = new String((byte[]) result.get(0)); - } while (!Arrays.equals((byte[]) result.get(0), "0".getBytes())); + } while (!Arrays.equals((byte[]) result.get(0), ZERO_CURSOR.getBytes())); - assertThat((byte[]) result.get(0)).isEqualTo("0".getBytes()); - assertThat(allEntries).containsExactlyInAnyOrder("1".getBytes(), - "12".getBytes(), - "3".getBytes()); + assertThat((byte[]) result.get(0)).isEqualTo(ZERO_CURSOR.getBytes()); + assertThat(allEntries).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes(), + FIELD_THREE.getBytes()); } @Test public void givenMatch_returnsAllMatchingMembersWithoutDuplicates() { - jedis.sadd("a", "1", "12", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); ScanParams scanParams = new ScanParams(); scanParams.match("1*"); ScanResult<byte[]> result = - jedis.sscan("a".getBytes(), "0".getBytes(), scanParams); + jedis.sscan(KEY.getBytes(), ZERO_CURSOR.getBytes(), scanParams); assertThat(result.isCompleteIteration()).isTrue(); - assertThat(result.getResult()).containsExactlyInAnyOrder("1".getBytes(), - "12".getBytes()); + assertThat(result.getResult()).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes()); } @Test @SuppressWarnings("unchecked") public void givenMultipleMatches_returnsMembersMatchingLastMatchParameter() { - jedis.sadd("a", "1", "12", "3"); + jedis.sadd(KEY, FIELD_ONE, FIELD_TWO, FIELD_THREE); - List<Object> result = (List<Object>) jedis.sendCommand("a", Protocol.Command.SSCAN, "a", "0", - "MATCH", "3*", "MATCH", "1*"); + List<Object> result = + (List<Object>) jedis.sendCommand(KEY, Protocol.Command.SSCAN, KEY, ZERO_CURSOR, + "MATCH", "3*", "MATCH", "1*"); - assertThat((byte[]) result.get(0)).isEqualTo("0".getBytes()); - assertThat((List<byte[]>) result.get(1)).containsExactlyInAnyOrder("1".getBytes(), - "12".getBytes()); + assertThat((byte[]) result.get(0)).isEqualTo(ZERO_CURSOR.getBytes()); + assertThat((List<byte[]>) result.get(1)).containsExactlyInAnyOrder(FIELD_ONE.getBytes(), + FIELD_TWO.getBytes()); } @Test public void givenMatchAndCount_returnsAllMembersWithoutDuplicates() { Review comment: > This test would be better named "givenSetWithThreeMembersAndMatchAndCount_returnsAllMatchingMembers" with the assertions changed to `containsOnly()` to allow duplicate entries. This comment still applies ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSScanIntegrationTest.java ########## @@ -323,26 +409,217 @@ public void givenNegativeCursor_returnsMembersUsingAbsoluteValueOfCursor() { @Test public void givenCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { - assertThatThrownBy(() -> jedis.sscan("a", "18446744073709551616")) - .hasMessageContaining(ERROR_CURSOR); + assertThatThrownBy( + () -> jedis.sscan(KEY, UNSIGNED_LONG_CAPACITY.add(BigInteger.ONE).toString())) + .hasMessageContaining(ERROR_CURSOR); } @Test public void givenNegativeCursorGreaterThanUnsignedLongCapacity_returnsCursorError() { Review comment: > It would be good to add tests that show the behaviour when the cursor value is outside the range `Long.MIN_VALUE > x > Long.MAX_VALUE`, and tests for the behaviour when the cursor in just inside that range. > > The behaviour of geode-for-redis differs from native Redis here, as they accept cursor values up to `UNSIGNED_LONG_CAPACITY` but we only accept ones up to `Long.MAX_VALUE`, so in order to test this, the test cases for failing with values outside the range should be put in `SScanIntegrationTest` (not the Abstract parent class). Also, looking at that class, the test `givenDifferentCursorThanSpecifiedByPreviousSscan_returnsAllMembers` is wrong and should be removed. This comment still applies -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org