diff --git a/pom.xml b/pom.xml index e6d868411f..0e19d0f26e 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-redis - 1.8.0.BUILD-SNAPSHOT + 1.8.0.DATAREDIS-512-SNAPSHOT Spring Data Redis diff --git a/src/main/java/org/springframework/data/redis/core/IndexWriter.java b/src/main/java/org/springframework/data/redis/core/IndexWriter.java index b179180fd3..e25a7a52e8 100644 --- a/src/main/java/org/springframework/data/redis/core/IndexWriter.java +++ b/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 secondary index 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 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 indexValues) { + createOrUpdateIndexes(key, indexValues, IndexWriteMode.UPDATE); + } + + private void createOrUpdateIndexes(Object key, Iterable indexValues, IndexWriteMode writeMode) { Assert.notNull(key, "Key must not be null!"); if (indexValues == null) { @@ -72,7 +87,18 @@ public void updateIndexes(Object key, Iterable 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 inde protected void removeKeyFromExistingIndexes(byte[] key, IndexedData indexedData) { Assert.notNull(indexedData, "IndexedData must not be null!"); - Set existingKeys = connection.keys(toBytes(indexedData.getKeyspace() + ":" + indexedData.getIndexName() - + ":*")); + Set 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 } } diff --git a/src/main/java/org/springframework/data/redis/core/RedisKeyValueAdapter.java b/src/main/java/org/springframework/data/redis/core/RedisKeyValueAdapter.java index eb32fd1aea..481d7463f3 100644 --- a/src/main/java/org/springframework/data/redis/core/RedisKeyValueAdapter.java +++ b/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; } }); diff --git a/src/test/java/org/springframework/data/redis/core/IndexWriterUnitTests.java b/src/test/java/org/springframework/data/redis/core/IndexWriterUnitTests.java index ba512f9a18..53f1a65f49 100644 --- a/src/test/java/org/springframework/data/redis/core/IndexWriterUnitTests.java +++ b/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(Arrays.asList(indexKey1, indexKey2))); + when(connectionMock.keys(any(byte[].class))) + .thenReturn(new LinkedHashSet(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(Arrays.asList(indexKey1, indexKey2))); + when(connectionMock.keys(any(byte[].class))) + .thenReturn(new LinkedHashSet(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(Arrays.asList("persons:firstname:rand".getBytes(CHARSET)))); + + writer.createIndexes(KEY_BIN, + Collections. 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(Arrays.asList("persons:firstname:rand".getBytes(CHARSET)))); + + writer.updateIndexes(KEY_BIN, + Collections. 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 diff --git a/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterTests.java b/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterTests.java index 00e9299878..8084ee8d5c 100644 --- a/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterTests.java +++ b/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 { diff --git a/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterUnitTests.java b/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterUnitTests.java index 3a895b2364..6783ed65e4 100644 --- a/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterUnitTests.java +++ b/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(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(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()); + } }