DonalEvans commented on code in PR #7513:
URL: https://github.com/apache/geode/pull/7513#discussion_r842139898
##########
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java:
##########
@@ -86,12 +93,19 @@ public static int smove(RedisKey sourceKey, RedisKey
destKey, byte[] member,
RegionProvider regionProvider) {
RedisSet source = regionProvider.getTypedRedisData(REDIS_SET, sourceKey,
false);
RedisSet destination = regionProvider.getTypedRedisData(REDIS_SET,
destKey, false);
- List<byte[]> memberList = new ArrayList<>();
- memberList.add(member);
- if (source.srem(memberList, regionProvider.getDataRegion(), sourceKey) ==
0) {
+ if (!source.sismember(member)) {
return 0;
}
- destination.sadd(memberList, regionProvider.getDataRegion(), destKey);
+ if (sourceKey.equals(destKey)) {
+ return 1;
+ }
+
+ List<byte[]> memberList = new ArrayList<>();
+ memberList.add(member);
+ RedisSet newSource = new RedisSet(source);
+ newSource.srem(memberList, regionProvider.getDataRegion(), sourceKey);
+ RedisSet newDestination = new RedisSet(destination);
+ newDestination.sadd(memberList, regionProvider.getDataRegion(), destKey);
Review Comment:
> Also, I think this test has two checks that it's doing but the wrong type
error on destination takes precedence and it might not be required as we have
other tests to do that.
Nonexistent key isn't an error condition, since it should just return 0
rather than an error message, so the test is only checking that we don't return
the WRONGTYPE error incorrectly, not that we return one of two possible errors.
I see why the TCL test failed with my changes though; it's because we'e
checking if the source set contains the member and returning early if it
doesn't rather than checking if the source is null and then checking if it
contains the member.
Right now we have one check with two possibilities for early return: the
source set exists but doesn't contain the member, or the source set doesn't
exist at all. In the second case, we should always return 0, regardless of what
type the destination set is, as that's what open source Redis does. In the
first case, we should check if the destination is the correct type before
checking if the member is contained in the source set. The below code and
additional integration test cover all the behaviour:
```
RedisSet source = regionProvider.getTypedRedisData(REDIS_SET, sourceKey,
false);
if (source.isNull()) {
return 0;
}
if (sourceKey.equals(destKey)) {
return 1;
}
RedisSet destination = regionProvider.getTypedRedisData(REDIS_SET,
destKey, false);
if (!source.sismember(member)) {
return 0;
}
List<byte[]> memberList = new ArrayList<>();
memberList.add(member);
RedisSet newSource = new RedisSet(source);
newSource.srem(memberList, regionProvider.getDataRegion(), sourceKey);
RedisSet newDestination = new RedisSet(destination);
newDestination.sadd(memberList, regionProvider.getDataRegion(), destKey);
return 1;
```
```
@Test
public void
smove_withNonExistentMemberInSourceAndDestinationNotASet_returnsWrongTypeError()
{
String nonExistentMember = "foo";
jedis.sadd(SOURCE_KEY, SOURCE_MEMBERS);
jedis.set(DESTINATION_KEY, "not a set");
assertThat(jedis.smove(NON_EXISTENT_SET_KEY, DESTINATION_KEY,
nonExistentMember))
.isEqualTo(0);
assertThatThrownBy(() -> jedis.sismember(DESTINATION_KEY,
nonExistentMember)).hasMessage(ERROR_WRONG_TYPE);
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
