DATAKV-137 - Fix cached query execution.

We now make sure that cached SpelCriteria's use the appropriate EvaluationContext containing parameters from the request instead of using the EvaluationContext of the cached query.

Author: mp911de

src/main/java/org/springframework/data/keyvalue/repository/query/KeyValuePartTreeQuery.java

@@ -44,6 +44,7 @@
  * 
  * @author Christoph Strobl
  * @author Oliver Gierke
+ * @author Mark Paluch
  */
 public class KeyValuePartTreeQuery implements RepositoryQuery {
 
@@ -134,15 +135,17 @@ protected KeyValueQuery<?> prepareQuery(KeyValueQuery<?> instance, Object[] para
 		ParametersParameterAccessor accessor = new ParametersParameterAccessor(getQueryMethod().getParameters(),
 				parameters);
 
-		KeyValueQuery<?> query = new KeyValueQuery(instance.getCritieria());
+		Object criteria = instance.getCritieria();
 
-		if (instance.getCritieria() instanceof SpelExpression) {
+		if (criteria instanceof SpelCriteria || criteria instanceof SpelExpression) {
 
+			SpelExpression spelExpression = getSpelExpression(criteria);
 			EvaluationContext context = this.evaluationContextProvider.getEvaluationContext(getQueryMethod().getParameters(),
 					parameters);
-			query = new KeyValueQuery(new SpelCriteria((SpelExpression) instance.getCritieria(), context));
+			criteria = new SpelCriteria(spelExpression, context);
 		}
-
+		
+		KeyValueQuery<?> query = new KeyValueQuery(criteria);
 		Pageable pageable = accessor.getPageable();
 		Sort sort = accessor.getSort();
 
@@ -153,6 +156,19 @@ protected KeyValueQuery<?> prepareQuery(KeyValueQuery<?> instance, Object[] para
 		return query;
 	}
 
+	private SpelExpression getSpelExpression(Object criteria) {
+		
+		if(criteria instanceof SpelExpression){
+			return (SpelExpression) criteria;
+		}
+		
+		if(criteria instanceof SpelCriteria){
+			return getSpelExpression(((SpelCriteria) criteria).getExpression());
+		}
+
+		throw new IllegalStateException(String.format("Cannot retrieve SpelExpression from %s", criteria));
+	}
+
 	public KeyValueQuery<?> createQuery(ParameterAccessor accessor) {
 
 		PartTree tree = new PartTree(getQueryMethod().getName(), getQueryMethod().getEntityInformation().getJavaType());

src/test/java/org/springframework/data/keyvalue/repository/query/CachingKeyValuePartTreeQueryUnitTests.java

@@ -0,0 +1,85 @@
+/*
+ * Copyright 2016 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.data.keyvalue.repository.query;
+
+import static org.hamcrest.core.IsNot.*;
+import static org.hamcrest.core.IsSame.*;
+import static org.junit.Assert.*;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.*;
+
+import java.lang.reflect.Method;
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+import org.springframework.data.keyvalue.Person;
+import org.springframework.data.keyvalue.core.KeyValueOperations;
+import org.springframework.data.keyvalue.core.SpelCriteria;
+import org.springframework.data.projection.ProjectionFactory;
+import org.springframework.data.repository.core.RepositoryMetadata;
+import org.springframework.data.repository.query.DefaultEvaluationContextProvider;
+import org.springframework.data.repository.query.QueryMethod;
+
+/**
+ * Unit tests for {@link CachingKeyValuePartTreeQuery}.
+ * 
+ * @author Mark Paluch
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class CachingKeyValuePartTreeQueryUnitTests {
+
+	@Mock KeyValueOperations kvOpsMock;
+	@Mock RepositoryMetadata metadataMock;
+	@Mock ProjectionFactory projectionFactoryMock;
+
+	@Before
+	public void setUp() throws Exception {
+
+		when(metadataMock.getDomainType()).thenReturn((Class) Person.class);
+		when(metadataMock.getReturnedDomainClass(any(Method.class))).thenReturn((Class) Person.class);
+	}
+
+	/**
+	 * @see DATAKV-137
+	 */
+	@Test
+	@SuppressWarnings({ "unchecked", "rawtypes" })
+	public void cachedSpelExpressionShouldBeReusedWithNewContext() throws NoSuchMethodException, SecurityException {
+
+		QueryMethod qm = new QueryMethod(Repo.class.getMethod("findByFirstname", String.class), metadataMock,
+				projectionFactoryMock);
+
+		KeyValuePartTreeQuery query = new CachingKeyValuePartTreeQuery(qm, DefaultEvaluationContextProvider.INSTANCE,
+				kvOpsMock, SpelQueryCreator.class);
+
+		Object[] args = new Object[] { "foo" };
+
+		SpelCriteria first = (SpelCriteria) query.prepareQuery(args).getCritieria();
+		SpelCriteria second = (SpelCriteria) query.prepareQuery(args).getCritieria();
+
+		assertThat(first.getExpression(), sameInstance(second.getExpression()));
+		assertThat(first.getContext(), not(sameInstance(second.getContext())));
+	}
+
+	static interface Repo {
+
+		List<Person> findByFirstname(String firstname);
+	}
+}

src/test/java/org/springframework/data/map/AbstractRepositoryUnitTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2014-2015 the original author or authors.
+ * Copyright 2014-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -43,6 +43,7 @@
  * @author Christoph Strobl
  * @author Oliver Gierke
  * @author Thomas Darimont
+ * @author Mark Paluch
  */
 public abstract class AbstractRepositoryUnitTests<T extends AbstractRepositoryUnitTests.PersonRepository> {
 
@@ -61,7 +62,7 @@ public abstract class AbstractRepositoryUnitTests<T extends AbstractRepositoryUn
 	public void setup() {
 
 		KeyValueOperations operations = new KeyValueTemplate(new MapKeyValueAdapter());
-		KeyValueRepositoryFactory keyValueRepositoryFactory = new KeyValueRepositoryFactory(operations);
+		KeyValueRepositoryFactory keyValueRepositoryFactory = createKeyValueRepositoryFactory(operations);
 
 		this.repository = getRepository(keyValueRepositoryFactory);
 	}
@@ -77,15 +78,29 @@ public void findBy() {
 		assertThat(repository.findByAge(19), hasItems(CERSEI, JAIME));
 	}
 
+	/**
+	 * @see DATAKV-137
+	 */
+	@Test
+	public void findByFirstname() {
+
+		repository.save(LENNISTERS);
+
+		assertThat(repository.findByFirstname(CERSEI.getFirstname()), hasItems(CERSEI));
+		assertThat(repository.findByFirstname(JAIME.getFirstname()), hasItems(JAIME));
+	}
+
 	/**
 	 * @see DATACMNS-525
+	 * @see DATAKV-137
 	 */
 	@Test
 	public void combindedFindUsingAnd() {
 
 		repository.save(LENNISTERS);
 
 		assertThat(repository.findByFirstnameAndAge(JAIME.getFirstname(), 19), hasItem(JAIME));
+		assertThat(repository.findByFirstnameAndAge(TYRION.getFirstname(), 17), hasItem(TYRION));
 	}
 
 	/**
@@ -120,13 +135,15 @@ public void findByConnectingOr() {
 
 	/**
 	 * @see DATACMNS-525
+	 * @see DATAKV-137
 	 */
 	@Test
 	public void singleEntityExecution() {
 
 		repository.save(LENNISTERS);
 
 		assertThat(repository.findByAgeAndFirstname(TYRION.getAge(), TYRION.getFirstname()), is(TYRION));
+		assertThat(repository.findByAgeAndFirstname(CERSEI.getAge(), CERSEI.getFirstname()), is(CERSEI));
 	}
 
 	/**
@@ -183,6 +200,10 @@ public void projectsResultToDynamicInterface() {
 		assertThat(result.get(0).getFirstname(), is(CERSEI.getFirstname()));
 	}
 
+	protected KeyValueRepositoryFactory createKeyValueRepositoryFactory(KeyValueOperations operations) {
+		return new KeyValueRepositoryFactory(operations);
+	}
+	
 	protected abstract T getRepository(KeyValueRepositoryFactory factory);
 
 	public static interface PersonRepository extends CrudRepository<Person, String>, KeyValueRepository<Person, String> {

src/test/java/org/springframework/data/map/CachingQuerySimpleKeyValueRepositoryUnitTests.java

@@ -0,0 +1,45 @@
+/*
+ * Copyright 2016 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.data.map;
+
+import static org.hamcrest.Matchers.hasItems;
+import static org.junit.Assert.assertThat;
+
+import org.junit.Test;
+import org.springframework.data.keyvalue.core.KeyValueOperations;
+import org.springframework.data.keyvalue.repository.query.CachingKeyValuePartTreeQuery;
+import org.springframework.data.keyvalue.repository.query.SpelQueryCreator;
+import org.springframework.data.keyvalue.repository.support.KeyValueRepositoryFactory;
+import org.springframework.data.keyvalue.repository.support.SimpleKeyValueRepository;
+import org.springframework.data.map.AbstractRepositoryUnitTests.PersonRepository;
+
+/**
+ * Unit tests for {@link SimpleKeyValueRepository} using {@link CachingKeyValuePartTreeQuery} and {@link SpelQueryCreator}.
+ * 
+ * @author Mark Paluch
+ */
+public class CachingQuerySimpleKeyValueRepositoryUnitTests extends AbstractRepositoryUnitTests<PersonRepository> {
+
+	@Override
+	protected KeyValueRepositoryFactory createKeyValueRepositoryFactory(KeyValueOperations operations) {
+		return new KeyValueRepositoryFactory(operations, SpelQueryCreator.class, CachingKeyValuePartTreeQuery.class);
+	}
+
+	@Override
+	protected PersonRepository getRepository(KeyValueRepositoryFactory factory) {
+		return factory.getRepository(PersonRepository.class);
+	}
+}