DATAREDIS-471 - Allow removal of complex types.

Allow removing complex types with all belonging nested structures and make sure existing indexes are updated correctly by removing the entity id from the according sets.

Author: christophstrobl

src/main/java/org/springframework/data/redis/core/RedisKeyValueAdapter.java

@@ -18,6 +18,7 @@
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -411,58 +412,28 @@ public void update(final PartialUpdate<?> update) {
 			@Override
 			public Void doInRedis(RedisConnection connection) throws DataAccessException {
 
-				List<byte[]> pathsToRemove = new ArrayList<byte[]>(update.getPropertyUpdates().size());
+				RedisUpdateObject redisUpdateObject = new RedisUpdateObject(redisKey, keyspace, id);
 
 				for (PropertyUpdate pUpdate : update.getPropertyUpdates()) {
 
 					String propertyPath = pUpdate.getPropertyPath();
 
-					if (UpdateCommand.DEL.equals(pUpdate.getCmd())) {
-
-						byte[] existingValue = connection.hGet(redisKey, toBytes(propertyPath));
-						pathsToRemove.add(toBytes(propertyPath));
-
-						byte[] existingValueIndexKey = existingValue != null
-								? ByteUtils.concatAll(toBytes(keyspace), (":" + propertyPath).getBytes(), ":".getBytes(), existingValue)
-								: null;
-
-						if (existingValue != null) {
-
-							if (connection.exists(existingValueIndexKey)) {
-								connection.sRem(existingValueIndexKey, toBytes(id));
-							}
-						}
-					}
-
-					if (pUpdate.getValue() instanceof Collection || pUpdate.getValue() instanceof Map
+					if (UpdateCommand.DEL.equals(pUpdate.getCmd()) || pUpdate.getValue() instanceof Collection
+							|| pUpdate.getValue() instanceof Map
 							|| (pUpdate.getValue() != null && pUpdate.getValue().getClass().isArray()) || (pUpdate.getValue() != null
 									&& !converter.getConversionService().canConvert(pUpdate.getValue().getClass(), byte[].class))) {
 
-						Set<byte[]> existingFields = connection.hKeys(redisKey);
-
-						for (byte[] hkey : existingFields) {
-
-							if (asString(hkey).startsWith(pUpdate.getPropertyPath() + ".")) {
-								pathsToRemove.add(hkey);
-
-								byte[] existingValue = connection.hGet(redisKey, toBytes(hkey));
-								byte[] existingValueIndexKey = existingValue != null ? ByteUtils.concatAll(toBytes(keyspace),
-										(":" + propertyPath).getBytes(), ":".getBytes(), existingValue) : null;
-
-								if (existingValue != null) {
-
-									if (connection.exists(existingValueIndexKey)) {
-										connection.sRem(existingValueIndexKey, toBytes(id));
-									}
-								}
-							}
-						}
-
+						redisUpdateObject = fetchDeletePathsFromHashAndUpdateIndex(redisUpdateObject, propertyPath, connection);
 					}
 				}
 
-				if (!pathsToRemove.isEmpty()) {
-					connection.hDel(redisKey, pathsToRemove.toArray(new byte[pathsToRemove.size()][]));
+				if (!redisUpdateObject.fieldsToRemove.isEmpty()) {
+					connection.hDel(redisKey,
+							redisUpdateObject.fieldsToRemove.toArray(new byte[redisUpdateObject.fieldsToRemove.size()][]));
+				}
+
+				for (byte[] index : redisUpdateObject.indexesToUpdate) {
+					connection.sRem(index, toBytes(redisUpdateObject.targetId));
 				}
 
 				if (!rdo.getBucket().isEmpty()) {
@@ -497,6 +468,48 @@ public Void doInRedis(RedisConnection connection) throws DataAccessException {
 		});
 	}
 
+	private RedisUpdateObject fetchDeletePathsFromHashAndUpdateIndex(RedisUpdateObject redisUpdateObject, String path,
+			RedisConnection connection) {
+
+		redisUpdateObject.addFieldToRemove(toBytes(path));
+		byte[] value = connection.hGet(redisUpdateObject.targetKey, toBytes(path));
+
+		if (value != null && value.length > 0) {
+
+			byte[] existingValueIndexKey = value != null
+					? ByteUtils.concatAll(toBytes(redisUpdateObject.keyspace), toBytes((":" + path)), toBytes(":"), value) : null;
+
+			if (connection.exists(existingValueIndexKey)) {
+				redisUpdateObject.addIndexToUpdate(existingValueIndexKey);
+			}
+			return redisUpdateObject;
+		}
+
+		Set<byte[]> existingFields = connection.hKeys(redisUpdateObject.targetKey);
+
+		for (byte[] field : existingFields) {
+
+			if (asString(field).startsWith(path + ".")) {
+
+				redisUpdateObject.addFieldToRemove(field);
+				value = connection.hGet(redisUpdateObject.targetKey, toBytes(field));
+
+				if (value != null) {
+
+					byte[] existingValueIndexKey = value != null
+							? ByteUtils.concatAll(toBytes(redisUpdateObject.keyspace), toBytes(":"), field, toBytes(":"), value)
+							: null;
+
+					if (connection.exists(existingValueIndexKey)) {
+						redisUpdateObject.addIndexToUpdate(existingValueIndexKey);
+					}
+				}
+			}
+		}
+
+		return redisUpdateObject;
+	}
+
 	/**
 	 * Execute {@link RedisCallback} via underlying {@link RedisOperations}.
 	 * 
@@ -684,4 +697,33 @@ private boolean isKeyExpirationMessage(Message message) {
 		}
 	}
 
+	/**
+	 * Container holding update information like fields to remove from the Redis Hash.
+	 *
+	 * @author Christoph Strobl
+	 */
+	private static class RedisUpdateObject {
+
+		private final String keyspace;
+		private final Object targetId;
+		private final byte[] targetKey;
+
+		private Set<byte[]> fieldsToRemove = new LinkedHashSet<byte[]>();
+		private Set<byte[]> indexesToUpdate = new LinkedHashSet<byte[]>();
+
+		RedisUpdateObject(byte[] targetKey, String keyspace, Object targetId) {
+
+			this.targetKey = targetKey;
+			this.keyspace = keyspace;
+			this.targetId = targetId;
+		}
+
+		void addFieldToRemove(byte[] field) {
+			fieldsToRemove.add(field);
+		}
+
+		void addIndexToUpdate(byte[] indexName) {
+			indexesToUpdate.add(indexName);
+		}
+	}
 }

src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterTests.java

@@ -22,6 +22,7 @@
 import static org.junit.Assert.*;
 
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Date;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -358,6 +359,120 @@ public void updateShouldAlterIndexDataOnNestedObjectPathCorrectly() {
 		assertThat(template.hasKey("persons:address.country:tear"), is(true));
 	}
 
+	/**
+	 * @see DATAREDIS-471
+	 */
+	@Test
+	public void updateShouldRemoveComplexObjectCorrectly() {
+
+		Person rand = new Person();
+		rand.address = new Address();
+		rand.address.country = "andor";
+		rand.address.city = "emond's field";
+
+		adapter.put("1", rand, "persons");
+
+		PartialUpdate<Person> update = new PartialUpdate<Person>("1", Person.class) //
+				.del("address");
+
+		adapter.update(update);
+
+		assertThat(template.opsForHash().hasKey("persons:1", "address.country"), is(false));
+		assertThat(template.opsForHash().hasKey("persons:1", "address.city"), is(false));
+		assertThat(template.opsForSet().isMember("persons:address.country:andor", "1"), is(false));
+	}
+
+	/**
+	 * @see DATAREDIS-471
+	 */
+	@Test
+	public void updateShouldRemoveSimpleListValuesCorrectly() {
+
+		Person rand = new Person();
+		rand.nicknames = Arrays.asList("lews therin", "dragon reborn");
+
+		adapter.put("1", rand, "persons");
+
+		PartialUpdate<Person> update = new PartialUpdate<Person>("1", Person.class) //
+				.del("nicknames");
+
+		adapter.update(update);
+
+		assertThat(template.opsForHash().hasKey("persons:1", "nicknames.[0]"), is(false));
+		assertThat(template.opsForHash().hasKey("persons:1", "nicknames.[1]"), is(false));
+	}
+
+	/**
+	 * @see DATAREDIS-471
+	 */
+	@Test
+	public void updateShouldRemoveComplexListValuesCorrectly() {
+
+		Person mat = new Person();
+		mat.firstname = "mat";
+		mat.nicknames = Collections.singletonList("prince of ravens");
+
+		Person perrin = new Person();
+		perrin.firstname = "mat";
+		perrin.nicknames = Collections.singletonList("lord of the two rivers");
+
+		Person rand = new Person();
+		rand.coworkers = Arrays.asList(mat, perrin);
+
+		adapter.put("1", rand, "persons");
+
+		PartialUpdate<Person> update = new PartialUpdate<Person>("1", Person.class) //
+				.del("coworkers");
+
+		adapter.update(update);
+
+		assertThat(template.opsForHash().hasKey("persons:1", "coworkers.[0].firstname"), is(false));
+		assertThat(template.opsForHash().hasKey("persons:1", "coworkers.[0].nicknames.[0]"), is(false));
+		assertThat(template.opsForHash().hasKey("persons:1", "coworkers.[1].firstname"), is(false));
+		assertThat(template.opsForHash().hasKey("persons:1", "coworkers.[1].nicknames.[0]"), is(false));
+	}
+
+	/**
+	 * @see DATAREDIS-471
+	 */
+	@Test
+	public void updateShouldRemoveSimpleMapValuesCorrectly() {
+
+		Person rand = new Person();
+		rand.physicalAttributes = Collections.singletonMap("eye-color", "grey");
+
+		adapter.put("1", rand, "persons");
+
+		PartialUpdate<Person> update = new PartialUpdate<Person>("1", Person.class) //
+				.del("physicalAttributes");
+
+		adapter.update(update);
+
+		assertThat(template.opsForHash().hasKey("persons:1", "physicalAttributes.[eye-color]"), is(false));
+	}
+
+	/**
+	 * @see DATAREDIS-471
+	 */
+	@Test
+	public void updateShouldRemoveComplexMapValuesCorrectly() {
+
+		Person tam = new Person();
+		tam.firstname = "tam";
+
+		Person rand = new Person();
+		rand.relatives = Collections.singletonMap("stepfather", tam);
+
+		adapter.put("1", rand, "persons");
+
+		PartialUpdate<Person> update = new PartialUpdate<Person>("1", Person.class) //
+				.del("relatives");
+
+		adapter.update(update);
+
+		assertThat(template.opsForHash().hasKey("persons:1", "relatives.[stepfather].firstname"), is(false));
+	}
+
 	@KeySpace("persons")
 	static class Person {