DATAKV-115 - Make derived finder execution thread-safe.

We now make sure separate SpelExpression and EvaluationContext for each and every derived finder execution to assert the expression context cannot be accidentally overridden.

Original pull request: #16.

Author: christophstrobl

src/main/java/org/springframework/data/keyvalue/core/SpelCriteria.java

@@ -0,0 +1,59 @@
+/*
+ * 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.core;
+
+import org.springframework.expression.EvaluationContext;
+import org.springframework.expression.spel.standard.SpelExpression;
+import org.springframework.expression.spel.support.StandardEvaluationContext;
+
+/**
+ * {@link SpelCriteria} allows to pass on a {@link SpelExpression} and {@link EvaluationContext} to the actual query
+ * processor. This decouples the {@link SpelExpression} from the context it is used in.
+ * 
+ * @author Christoph Strobl
+ */
+public class SpelCriteria {
+
+	private final SpelExpression expression;
+	private final EvaluationContext context;
+
+	/**
+	 * Creates new {@link SpelCriteria}.
+	 * 
+	 * @param expression must not be {@literal null}.
+	 * @param context can be {@literal null} and will be defaulted to {@link StandardEvaluationContext}.
+	 */
+	public SpelCriteria(SpelExpression expression, EvaluationContext context) {
+
+		this.expression = expression;
+		this.context = context == null ? new StandardEvaluationContext() : context;
+	}
+
+	/**
+	 * @return never {@literal null}.
+	 */
+	public EvaluationContext getContext() {
+		return context;
+	}
+
+	/**
+	 * @return never {@literal null}.
+	 */
+	public SpelExpression getExpression() {
+		return expression;
+	}
+
+}

src/main/java/org/springframework/data/keyvalue/core/SpelCriteriaAccessor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2014 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.
@@ -18,6 +18,7 @@
 import org.springframework.data.keyvalue.core.query.KeyValueQuery;
 import org.springframework.expression.spel.standard.SpelExpression;
 import org.springframework.expression.spel.standard.SpelExpressionParser;
+import org.springframework.expression.spel.support.StandardEvaluationContext;
 import org.springframework.util.Assert;
 
 /**
@@ -26,7 +27,7 @@
  * @author Christoph Strobl
  * @author Oliver Gierke
  */
-class SpelCriteriaAccessor implements CriteriaAccessor<SpelExpression> {
+class SpelCriteriaAccessor implements CriteriaAccessor<SpelCriteria> {
 
 	private final SpelExpressionParser parser;
 
@@ -40,20 +41,25 @@ public SpelCriteriaAccessor(SpelExpressionParser parser) {
 	}
 
 	@Override
-	public SpelExpression resolve(KeyValueQuery<?> query) {
+	public SpelCriteria resolve(KeyValueQuery<?> query) {
 
 		if (query.getCritieria() == null) {
 			return null;
 		}
 
 		if (query.getCritieria() instanceof SpelExpression) {
-			return (SpelExpression) query.getCritieria();
+			return new SpelCriteria((SpelExpression) query.getCritieria(), new StandardEvaluationContext());
 		}
 
 		if (query.getCritieria() instanceof String) {
-			return parser.parseRaw((String) query.getCritieria());
+			return new SpelCriteria(parser.parseRaw((String) query.getCritieria()), new StandardEvaluationContext());
+		}
+
+		if (query.getCritieria() instanceof SpelCriteria) {
+			return (SpelCriteria) query.getCritieria();
 		}
 
 		throw new IllegalArgumentException("Cannot create SpelCriteria for " + query.getCritieria());
 	}
+
 }

src/main/java/org/springframework/data/keyvalue/core/SpelQueryEngine.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2014 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.
@@ -35,7 +35,7 @@
  * @author Oliver Gierke
  * @param <T>
  */
-class SpelQueryEngine<T extends KeyValueAdapter> extends QueryEngine<KeyValueAdapter, SpelExpression, Comparator<?>> {
+class SpelQueryEngine<T extends KeyValueAdapter> extends QueryEngine<KeyValueAdapter, SpelCriteria, Comparator<?>> {
 
 	private static final SpelExpressionParser PARSER = new SpelExpressionParser();
 
@@ -51,7 +51,7 @@ public SpelQueryEngine() {
 	 * @see org.springframework.data.keyvalue.core.QueryEngine#execute(java.lang.Object, java.lang.Object, int, int, java.io.Serializable)
 	 */
 	@Override
-	public Collection<?> execute(SpelExpression criteria, Comparator<?> sort, int offset, int rows, Serializable keyspace) {
+	public Collection<?> execute(SpelCriteria criteria, Comparator<?> sort, int offset, int rows, Serializable keyspace) {
 		return sortAndFilterMatchingRange(getAdapter().getAllOf(keyspace), criteria, sort, offset, rows);
 	}
 
@@ -60,12 +60,12 @@ public Collection<?> execute(SpelExpression criteria, Comparator<?> sort, int of
 	 * @see org.springframework.data.keyvalue.core.QueryEngine#count(java.lang.Object, java.io.Serializable)
 	 */
 	@Override
-	public long count(SpelExpression criteria, Serializable keyspace) {
+	public long count(SpelCriteria criteria, Serializable keyspace) {
 		return filterMatchingRange(getAdapter().getAllOf(keyspace), criteria, -1, -1).size();
 	}
 
 	@SuppressWarnings({ "unchecked", "rawtypes" })
-	private List<?> sortAndFilterMatchingRange(Iterable<?> source, SpelExpression criteria, Comparator sort, int offset,
+	private List<?> sortAndFilterMatchingRange(Iterable<?> source, SpelCriteria criteria, Comparator sort, int offset,
 			int rows) {
 
 		List<?> tmp = IterableConverter.toList(source);
@@ -76,7 +76,7 @@ private List<?> sortAndFilterMatchingRange(Iterable<?> source, SpelExpression cr
 		return filterMatchingRange(tmp, criteria, offset, rows);
 	}
 
-	private static <S> List<S> filterMatchingRange(Iterable<S> source, SpelExpression criteria, int offset, int rows) {
+	private static <S> List<S> filterMatchingRange(Iterable<S> source, SpelCriteria criteria, int offset, int rows) {
 
 		List<S> result = new ArrayList<S>();
 
@@ -90,10 +90,11 @@ private static <S> List<S> filterMatchingRange(Iterable<S> source, SpelExpressio
 
 			if (!matches) {
 				try {
-					matches = criteria.getValue(candidate, Boolean.class);
+					matches = criteria.getExpression().getValue(criteria.getContext(), candidate, Boolean.class);
 				} catch (SpelEvaluationException e) {
-					criteria.getEvaluationContext().setVariable("it", candidate);
-					matches = criteria.getValue() == null ? false : criteria.getValue(Boolean.class);
+					criteria.getContext().setVariable("it", candidate);
+					matches = criteria.getExpression().getValue(criteria.getContext()) == null ? false : criteria.getExpression()
+							.getValue(criteria.getContext(), Boolean.class);
 				}
 			}
 

src/main/java/org/springframework/data/keyvalue/repository/query/KeyValuePartTreeQuery.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.
@@ -23,6 +23,7 @@
 import org.springframework.data.domain.Sort;
 import org.springframework.data.keyvalue.core.IterableConverter;
 import org.springframework.data.keyvalue.core.KeyValueOperations;
+import org.springframework.data.keyvalue.core.SpelCriteria;
 import org.springframework.data.keyvalue.core.query.KeyValueQuery;
 import org.springframework.data.repository.query.EvaluationContextProvider;
 import org.springframework.data.repository.query.ParameterAccessor;
@@ -93,7 +94,7 @@ public QueryMethod getQueryMethod() {
 	public Object execute(Object[] parameters) {
 
 		ParameterAccessor accessor = new ParametersParameterAccessor(getQueryMethod().getParameters(), parameters);
-		KeyValueQuery<?> query = prepareQuery(parameters, accessor);
+		KeyValueQuery<?> query = prepareQuery(parameters);
 		ResultProcessor processor = queryMethod.getResultProcessor().withDynamicProjection(accessor);
 
 		return processor.processResult(doExecute(parameters, query));
@@ -114,8 +115,8 @@ protected Object doExecute(Object[] parameters, KeyValueQuery<?> query) {
 
 			Iterable<?> result = this.keyValueOperations.find(query, queryMethod.getEntityInformation().getJavaType());
 
-			long count = queryMethod.isSliceQuery() ? 0
-					: keyValueOperations.count(query, queryMethod.getEntityInformation().getJavaType());
+			long count = queryMethod.isSliceQuery() ? 0 : keyValueOperations.count(query, queryMethod.getEntityInformation()
+					.getJavaType());
 
 			return new PageImpl(IterableConverter.toList(result), page, count);
 
@@ -133,14 +134,24 @@ protected Object doExecute(Object[] parameters, KeyValueQuery<?> query) {
 	}
 
 	@SuppressWarnings({ "rawtypes", "unchecked" })
-	private KeyValueQuery<?> prepareQuery(Object[] parameters, ParameterAccessor accessor) {
+	protected KeyValueQuery<?> prepareQuery(Object[] parameters) {
+
+		ParametersParameterAccessor accessor = new ParametersParameterAccessor(getQueryMethod().getParameters(), parameters);
 
 		if (this.query == null) {
 			this.query = createQuery(accessor);
 		}
 
 		KeyValueQuery<?> q = new KeyValueQuery(this.query.getCritieria());
 
+		if (this.query.getCritieria() instanceof SpelExpression) {
+
+			EvaluationContext context = this.evaluationContextProvider.getEvaluationContext(getQueryMethod().getParameters(),
+					parameters);
+			SpelCriteria spelCriteria = new SpelCriteria((SpelExpression) this.query.getCritieria(), context);
+			q = new KeyValueQuery(spelCriteria);
+		}
+
 		if (accessor.getPageable() != null) {
 			q.setOffset(accessor.getPageable().getOffset());
 			q.setRows(accessor.getPageable().getPageSize());
@@ -153,12 +164,6 @@ private KeyValueQuery<?> prepareQuery(Object[] parameters, ParameterAccessor acc
 
 		q.setSort(sort != null ? sort : query.getSort());
 
-		if (q.getCritieria() instanceof SpelExpression) {
-			EvaluationContext context = this.evaluationContextProvider.getEvaluationContext(getQueryMethod().getParameters(),
-					parameters);
-			((SpelExpression) q.getCritieria()).setEvaluationContext(context);
-		}
-
 		return q;
 	}
 

src/test/java/org/springframework/data/keyvalue/core/SpelQueryEngineUnitTests.java

@@ -16,7 +16,6 @@
 package org.springframework.data.keyvalue.core;
 
 import static org.hamcrest.Matchers.*;
-import static org.hamcrest.Matchers.contains;
 import static org.junit.Assert.*;
 import static org.mockito.Matchers.*;
 import static org.mockito.Mockito.*;
@@ -33,14 +32,12 @@
 import org.mockito.Mock;
 import org.mockito.runners.MockitoJUnitRunner;
 import org.springframework.data.annotation.Id;
-import org.springframework.data.keyvalue.core.query.KeyValueQuery;
 import org.springframework.data.keyvalue.repository.query.SpelQueryCreator;
 import org.springframework.data.projection.SpelAwareProxyProjectionFactory;
 import org.springframework.data.repository.core.RepositoryMetadata;
 import org.springframework.data.repository.query.ParametersParameterAccessor;
 import org.springframework.data.repository.query.QueryMethod;
 import org.springframework.data.repository.query.parser.PartTree;
-import org.springframework.expression.spel.standard.SpelExpression;
 import org.springframework.expression.spel.support.StandardEvaluationContext;
 
 /**
@@ -77,8 +74,8 @@ public void queriesEntitiesWithNullProperty() throws Exception {
 
 		doReturn(people).when(adapter).getAllOf(anyString());
 
-		assertThat((Collection<Person>) engine.execute(createQueryForMethodWithArgs("findByFirstname", "bob"), null, -1, -1,
-				anyString()), contains(BOB_WITH_FIRSTNAME));
+		assertThat((Collection<Person>) engine.execute(createQueryForMethodWithArgs("findByFirstname", "bob"), null, -1,
+				-1, anyString()), contains(BOB_WITH_FIRSTNAME));
 	}
 
 	/**
@@ -92,7 +89,7 @@ public void countsEntitiesWithNullProperty() throws Exception {
 		assertThat(engine.count(createQueryForMethodWithArgs("findByFirstname", "bob"), anyString()), is(1L));
 	}
 
-	private static SpelExpression createQueryForMethodWithArgs(String methodName, Object... args) throws Exception {
+	private static SpelCriteria createQueryForMethodWithArgs(String methodName, Object... args) throws Exception {
 
 		List<Class<?>> types = new ArrayList<Class<?>>(args.length);
 
@@ -105,13 +102,10 @@ private static SpelExpression createQueryForMethodWithArgs(String methodName, Ob
 		doReturn(method.getReturnType()).when(metadata).getReturnedDomainClass(method);
 
 		PartTree partTree = new PartTree(method.getName(), method.getReturnType());
-		SpelQueryCreator creator = new SpelQueryCreator(partTree, new ParametersParameterAccessor(
-				new QueryMethod(method, metadata, new SpelAwareProxyProjectionFactory()).getParameters(), args));
+		SpelQueryCreator creator = new SpelQueryCreator(partTree, new ParametersParameterAccessor(new QueryMethod(method,
+				metadata, new SpelAwareProxyProjectionFactory()).getParameters(), args));
 
-		KeyValueQuery<SpelExpression> query = creator.createQuery();
-		query.getCritieria().setEvaluationContext(new StandardEvaluationContext(args));
-
-		return query.getCritieria();
+		return new SpelCriteria(creator.createQuery().getCritieria(), new StandardEvaluationContext(args));
 	}
 
 	static interface PersonRepository {

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

@@ -0,0 +1,73 @@
+/*
+ * Copyright 2015-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.*;
+import static org.mockito.Mockito.*;
+
+import java.lang.reflect.Method;
+import java.util.List;
+
+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.projection.ProjectionFactory;
+import org.springframework.data.repository.core.RepositoryMetadata;
+import org.springframework.data.repository.query.DefaultEvaluationContextProvider;
+import org.springframework.data.repository.query.QueryMethod;
+
+/**
+ * @author Christoph Strobl
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class KeyValuePartTreeQueryUnitTests {
+
+	@Mock KeyValueOperations kvOpsMock;
+	@Mock RepositoryMetadata metadataMock;
+	@Mock ProjectionFactory projectionFactoryMock;
+
+	/**
+	 * @see DATAKV-115
+	 */
+	@Test
+	@SuppressWarnings({ "unchecked", "rawtypes" })
+	public void spelExpressionAndContextShouldNotBeReused() throws NoSuchMethodException, SecurityException {
+
+		when(metadataMock.getDomainType()).thenReturn((Class) Person.class);
+		when(metadataMock.getReturnedDomainClass(any(Method.class))).thenReturn((Class) Person.class);
+
+		QueryMethod qm = new QueryMethod(Repo.class.getMethod("findByFirstname", String.class), metadataMock,
+				projectionFactoryMock);
+
+		KeyValuePartTreeQuery query = new KeyValuePartTreeQuery(qm, DefaultEvaluationContextProvider.INSTANCE, kvOpsMock,
+				SpelQueryCreator.class);
+
+		Object[] args = new Object[] { "foo" };
+
+		assertThat(query.prepareQuery(args).getCritieria(), not(sameInstance(query.prepareQuery(args).getCritieria())));
+	}
+
+	static interface Repo {
+
+		List<Person> findByFirstname(String firstname);
+	}
+}