DATAREDIS-512 - Skip repository index update checks on insert.

pom.xml

@@ -5,7 +5,7 @@
 
 	<groupId>org.springframework.data</groupId>
 	<artifactId>spring-data-redis</artifactId>
-	<version>1.8.0.BUILD-SNAPSHOT</version>
+	<version>1.8.0.DATAREDIS-512-SNAPSHOT</version>
 
 	<name>Spring Data Redis</name>
 

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

@@ -25,6 +25,7 @@
 import org.springframework.data.redis.util.ByteUtils;
 import org.springframework.util.Assert;
 import org.springframework.util.CollectionUtils;
+import org.springframework.util.ObjectUtils;
 
 /**
  * {@link IndexWriter} takes care of writing <a href="http://redis.io/topics/indexes">secondary index</a> structures to
@@ -57,13 +58,27 @@ public IndexWriter(RedisConnection connection, RedisConverter converter) {
 		this.converter = converter;
 	}
 
+	/**
+	 * Initially creates indexes.
+	 *
+	 * @param key must not be {@literal null}.
+	 * @param indexValues can be {@literal null}.
+	 */
+	public void createIndexes(Object key, Iterable<IndexedData> indexValues) {
+		createOrUpdateIndexes(key, indexValues, IndexWriteMode.CREATE);
+	}
+
 	/**
 	 * Updates indexes by first removing key from existing one and then persisting new index data.
-	 * 
+	 *
 	 * @param key must not be {@literal null}.
 	 * @param indexValues can be {@literal null}.
 	 */
 	public void updateIndexes(Object key, Iterable<IndexedData> indexValues) {
+		createOrUpdateIndexes(key, indexValues, IndexWriteMode.UPDATE);
+	}
+
+	private void createOrUpdateIndexes(Object key, Iterable<IndexedData> indexValues, IndexWriteMode writeMode) {
 
 		Assert.notNull(key, "Key must not be null!");
 		if (indexValues == null) {
@@ -72,7 +87,18 @@ public void updateIndexes(Object key, Iterable<IndexedData> indexValues) {
 
 		byte[] binKey = toBytes(key);
 
-		removeKeyFromExistingIndexes(binKey, indexValues);
+		if (ObjectUtils.nullSafeEquals(IndexWriteMode.UPDATE, writeMode)) {
+
+			if (indexValues.iterator().hasNext()) {
+				IndexedData data = indexValues.iterator().next();
+				if (data != null && data.getKeyspace() != null) {
+					removeKeyFromIndexes(data.getKeyspace(), binKey);
+				}
+			}
+
+			removeKeyFromExistingIndexes(binKey, indexValues);
+		}
+
 		addKeyToIndexes(binKey, indexValues);
 	}
 
@@ -123,8 +149,8 @@ private void removeKeyFromExistingIndexes(byte[] key, Iterable<IndexedData> inde
 	protected void removeKeyFromExistingIndexes(byte[] key, IndexedData indexedData) {
 
 		Assert.notNull(indexedData, "IndexedData must not be null!");
-		Set<byte[]> existingKeys = connection.keys(toBytes(indexedData.getKeyspace() + ":" + indexedData.getIndexName()
-				+ ":*"));
+		Set<byte[]> existingKeys = connection
+				.keys(toBytes(indexedData.getKeyspace() + ":" + indexedData.getIndexName() + ":*"));
 
 		if (!CollectionUtils.isEmpty(existingKeys)) {
 			for (byte[] existingKey : existingKeys) {
@@ -166,8 +192,8 @@ protected void addKeyToIndex(byte[] key, IndexedData indexedData) {
 			// keep track of indexes used for the object
 			connection.sAdd(ByteUtils.concatAll(toBytes(indexedData.getKeyspace() + ":"), key, toBytes(":idx")), indexKey);
 		} else {
-			throw new IllegalArgumentException(String.format("Cannot write index data for unknown index type %s",
-					indexedData.getClass()));
+			throw new IllegalArgumentException(
+					String.format("Cannot write index data for unknown index type %s", indexedData.getClass()));
 		}
 	}
 
@@ -185,10 +211,17 @@ private byte[] toBytes(Object source) {
 			return converter.getConversionService().convert(source, byte[].class);
 		}
 
-		throw new InvalidDataAccessApiUsageException(
-				String
-						.format(
-								"Cannot convert %s to binary representation for index key generation. Are you missing a Converter? Did you register a non PathBasedRedisIndexDefinition that might apply to a complex type?",
-								source.getClass()));
+		throw new InvalidDataAccessApiUsageException(String.format(
+				"Cannot convert %s to binary representation for index key generation. Are you missing a Converter? Did you register a non PathBasedRedisIndexDefinition that might apply to a complex type?",
+				source.getClass()));
+	}
+
+	/**
+	 * @author Christoph Strobl
+	 * @since 1.8
+	 */
+	private static enum IndexWriteMode {
+
+		CREATE, UPDATE
 	}
 }

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

@@ -25,7 +25,6 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.BeansException;
-import org.springframework.beans.factory.DisposableBean;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationContextAware;
 import org.springframework.context.ApplicationListener;
@@ -38,7 +37,6 @@
 import org.springframework.data.redis.connection.Message;
 import org.springframework.data.redis.connection.MessageListener;
 import org.springframework.data.redis.connection.RedisConnection;
-import org.springframework.data.redis.connection.RedisConnectionFactory;
 import org.springframework.data.redis.core.convert.CustomConversions;
 import org.springframework.data.redis.core.convert.KeyspaceConfiguration;
 import org.springframework.data.redis.core.convert.MappingRedisConverter;
@@ -164,8 +162,7 @@ public RedisKeyValueAdapter(RedisOperations<?, ?> redisOps, RedisConverter redis
 	/**
 	 * Default constructor.
 	 */
-	protected RedisKeyValueAdapter() {
-	}
+	protected RedisKeyValueAdapter() {}
 
 	/*
 	 * (non-Javadoc)
@@ -198,7 +195,7 @@ public Object doInRedis(RedisConnection connection) throws DataAccessException {
 				byte[] key = toBytes(rdo.getId());
 				byte[] objectKey = createKey(rdo.getKeyspace(), rdo.getId());
 
-				connection.del(objectKey);
+				boolean isNew = connection.del(objectKey) == 0;
 
 				connection.hMSet(objectKey, rdo.getBucket().rawMap());
 
@@ -215,7 +212,12 @@ public Object doInRedis(RedisConnection connection) throws DataAccessException {
 
 				connection.sAdd(toBytes(rdo.getKeyspace()), key);
 
-				new IndexWriter(connection, converter).updateIndexes(key, rdo.getIndexedData());
+				IndexWriter indexWriter = new IndexWriter(connection, converter);
+				if (isNew) {
+					indexWriter.createIndexes(key, rdo.getIndexedData());
+				} else {
+					indexWriter.updateIndexes(key, rdo.getIndexedData());
+				}
 				return null;
 			}
 		});

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

@@ -22,6 +22,7 @@
 
 import java.nio.charset.Charset;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.LinkedHashSet;
 
 import org.junit.Before;
@@ -110,8 +111,8 @@ public void removeKeyFromExistingIndexesShouldRemoveKeyFromAllExistingIndexesFor
 		byte[] indexKey1 = "persons:firstname:rand".getBytes(CHARSET);
 		byte[] indexKey2 = "persons:firstname:mat".getBytes(CHARSET);
 
-		when(connectionMock.keys(any(byte[].class))).thenReturn(
-				new LinkedHashSet<byte[]>(Arrays.asList(indexKey1, indexKey2)));
+		when(connectionMock.keys(any(byte[].class)))
+				.thenReturn(new LinkedHashSet<byte[]>(Arrays.asList(indexKey1, indexKey2)));
 
 		writer.removeKeyFromExistingIndexes(KEY_BIN, new StubIndxedData());
 
@@ -136,8 +137,8 @@ public void removeAllIndexesShouldDeleteAllIndexKeys() {
 		byte[] indexKey1 = "persons:firstname:rand".getBytes(CHARSET);
 		byte[] indexKey2 = "persons:firstname:mat".getBytes(CHARSET);
 
-		when(connectionMock.keys(any(byte[].class))).thenReturn(
-				new LinkedHashSet<byte[]>(Arrays.asList(indexKey1, indexKey2)));
+		when(connectionMock.keys(any(byte[].class)))
+				.thenReturn(new LinkedHashSet<byte[]>(Arrays.asList(indexKey1, indexKey2)));
 
 		writer.removeAllIndexes(KEYSPACE);
 
@@ -178,6 +179,42 @@ public byte[] convert(DummyObject source) {
 		verify(connectionMock).sAdd(eq(("persons:firstname:" + identityHexString).getBytes(CHARSET)), eq(KEY_BIN));
 	}
 
+	/**
+	 * @see DATAREDIS-512
+	 */
+	@Test
+	public void createIndexShouldNotTryToRemoveExistingValues() {
+
+		when(connectionMock.keys(any(byte[].class)))
+				.thenReturn(new LinkedHashSet<byte[]>(Arrays.asList("persons:firstname:rand".getBytes(CHARSET))));
+
+		writer.createIndexes(KEY_BIN,
+				Collections.<IndexedData> singleton(new SimpleIndexedPropertyValue(KEYSPACE, "firstname", "Rand")));
+
+		verify(connectionMock).sAdd(eq("persons:firstname:Rand".getBytes(CHARSET)), eq(KEY_BIN));
+		verify(connectionMock).sAdd(eq("persons:key-1:idx".getBytes(CHARSET)),
+				eq("persons:firstname:Rand".getBytes(CHARSET)));
+		verify(connectionMock, never()).sRem(any(byte[].class), eq(KEY_BIN));
+	}
+
+	/**
+	 * @see DATAREDIS-512
+	 */
+	@Test
+	public void updateIndexShouldRemoveExistingValues() {
+
+		when(connectionMock.keys(any(byte[].class)))
+				.thenReturn(new LinkedHashSet<byte[]>(Arrays.asList("persons:firstname:rand".getBytes(CHARSET))));
+
+		writer.updateIndexes(KEY_BIN,
+				Collections.<IndexedData> singleton(new SimpleIndexedPropertyValue(KEYSPACE, "firstname", "Rand")));
+
+		verify(connectionMock).sAdd(eq("persons:firstname:Rand".getBytes(CHARSET)), eq(KEY_BIN));
+		verify(connectionMock).sAdd(eq("persons:key-1:idx".getBytes(CHARSET)),
+				eq("persons:firstname:Rand".getBytes(CHARSET)));
+		verify(connectionMock, times(1)).sRem(any(byte[].class), eq(KEY_BIN));
+	}
+
 	static class StubIndxedData implements IndexedData {
 
 		@Override

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

@@ -64,8 +64,8 @@ public void setUp() {
 		template = new StringRedisTemplate(connectionFactory);
 		template.afterPropertiesSet();
 
-		RedisMappingContext mappingContext = new RedisMappingContext(new MappingConfiguration(new IndexConfiguration(),
-				new KeyspaceConfiguration()));
+		RedisMappingContext mappingContext = new RedisMappingContext(
+				new MappingConfiguration(new IndexConfiguration(), new KeyspaceConfiguration()));
 		mappingContext.afterPropertiesSet();
 
 		adapter = new RedisKeyValueAdapter(template, mappingContext);
@@ -272,6 +272,45 @@ public void keyExpiredEventShouldRemoveHelperStructures() {
 		assertThat(template.opsForSet().members("persons"), not(hasItem("1")));
 	}
 
+	/**
+	 * @see DATAREDIS-512
+	 */
+	@Test
+	public void putWritesIndexDataCorrectly() {
+
+		Person rand = new Person();
+		rand.age = 24;
+		rand.firstname = "rand";
+
+		adapter.put("rand", rand, "persons");
+
+		assertThat(template.hasKey("persons:firstname:rand"), is(true));
+		assertThat(template.hasKey("persons:rand:idx"), is(true));
+		assertThat(template.opsForSet().isMember("persons:rand:idx", "persons:firstname:rand"), is(true));
+
+		Person mat = new Person();
+		mat.age = 22;
+		mat.firstname = "mat";
+		adapter.put("mat", mat, "persons");
+
+		assertThat(template.hasKey("persons:firstname:rand"), is(true));
+		assertThat(template.hasKey("persons:firstname:mat"), is(true));
+		assertThat(template.hasKey("persons:rand:idx"), is(true));
+		assertThat(template.hasKey("persons:mat:idx"), is(true));
+		assertThat(template.opsForSet().isMember("persons:rand:idx", "persons:firstname:rand"), is(true));
+		assertThat(template.opsForSet().isMember("persons:mat:idx", "persons:firstname:mat"), is(true));
+
+		rand.firstname = "frodo";
+		adapter.put("rand", rand, "persons");
+
+		assertThat(template.hasKey("persons:firstname:rand"), is(false));
+		assertThat(template.hasKey("persons:firstname:mat"), is(true));
+		assertThat(template.hasKey("persons:firstname:frodo"), is(true));
+		assertThat(template.hasKey("persons:rand:idx"), is(true));
+		assertThat(template.opsForSet().isMember("persons:rand:idx", "persons:firstname:frodo"), is(true));
+		assertThat(template.opsForSet().isMember("persons:mat:idx", "persons:firstname:mat"), is(true));
+	}
+
 	@KeySpace("persons")
 	static class Person {
 

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

@@ -16,9 +16,12 @@
 
 package org.springframework.data.redis.core;
 
+import static org.mockito.Matchers.*;
 import static org.mockito.Mockito.*;
 
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.LinkedHashSet;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -27,11 +30,15 @@
 import org.mockito.runners.MockitoJUnitRunner;
 import org.springframework.data.redis.connection.RedisConnection;
 import org.springframework.data.redis.connection.jedis.JedisConnectionFactory;
+import org.springframework.data.redis.core.convert.Bucket;
+import org.springframework.data.redis.core.convert.RedisData;
+import org.springframework.data.redis.core.convert.SimpleIndexedPropertyValue;
 
 /**
  * Unit tests for {@link RedisKeyValueAdapter}.
  *
  * @author Mark Paluch
+ * @author Christoph Strobl
  */
 @RunWith(MockitoJUnitRunner.class)
 public class RedisKeyValueAdapterUnitTests {
@@ -66,4 +73,40 @@ public void destroyShouldNotDestroyConnectionFactory() throws Exception {
 
 		verify(jedisConnectionFactoryMock, never()).destroy();
 	}
+
+	/**
+	 * @see DATAREDIS-512
+	 */
+	@Test
+	public void putShouldRemoveExistingIndexValuesWhenUpdating() {
+
+		RedisData rd = new RedisData(Bucket.newBucketFromStringMap(Collections.singletonMap("_id", "1")));
+		rd.addIndexedData(new SimpleIndexedPropertyValue("persons", "firstname", "rand"));
+
+		when(redisConnectionMock.keys(any(byte[].class)))
+				.thenReturn(new LinkedHashSet<byte[]>(Arrays.asList("persons:firstname:rand".getBytes())));
+		when(redisConnectionMock.del((byte[][]) anyVararg())).thenReturn(1L);
+
+		redisKeyValueAdapter.put("1", rd, "persons");
+
+		verify(redisConnectionMock, times(1)).sRem(any(byte[].class), any(byte[].class));
+	}
+
+	/**
+	 * @see DATAREDIS-512
+	 */
+	@Test
+	public void putShouldNotTryToRemoveExistingIndexValuesWhenInsertingNew() {
+
+		RedisData rd = new RedisData(Bucket.newBucketFromStringMap(Collections.singletonMap("_id", "1")));
+		rd.addIndexedData(new SimpleIndexedPropertyValue("persons", "firstname", "rand"));
+
+		when(redisConnectionMock.sMembers(any(byte[].class)))
+				.thenReturn(new LinkedHashSet<byte[]>(Arrays.asList("persons:firstname:rand".getBytes())));
+		when(redisConnectionMock.del((byte[][]) anyVararg())).thenReturn(0L);
+
+		redisKeyValueAdapter.put("1", rd, "persons");
+
+		verify(redisConnectionMock, never()).sRem(any(byte[].class), (byte[][]) anyVararg());
+	}
 }