From fb15a9647d9da3f4df20748b1b8de0570e4a2579 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Mon, 20 Jul 2020 13:22:41 +0200 Subject: [PATCH] DATACMNS-1108 - Polishing. Move loop to Set lookup, update Javadoc and remove unused imports. Original Pull Request: #454 --- ...EvaluationContextExtensionInformation.java | 82 +++++++++++++------ .../data/spel/ExpressionDependencies.java | 44 +++++++++- ...tensionAwareEvaluationContextProvider.java | 5 +- .../spel/spi/EvaluationContextExtension.java | 4 +- .../data/spel/spi/ExtensionIdAware.java | 2 +- .../ReactiveEvaluationContextExtension.java | 4 +- .../spel/ExpressionDependenciesUnitTests.java | 16 +++- ...areEvaluationContextProviderUnitTests.java | 67 +++++++++++++-- 8 files changed, 182 insertions(+), 42 deletions(-) diff --git a/src/main/java/org/springframework/data/spel/EvaluationContextExtensionInformation.java b/src/main/java/org/springframework/data/spel/EvaluationContextExtensionInformation.java index abc70976a..06bc1f11b 100644 --- a/src/main/java/org/springframework/data/spel/EvaluationContextExtensionInformation.java +++ b/src/main/java/org/springframework/data/spel/EvaluationContextExtensionInformation.java @@ -28,9 +28,12 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Optional; +import java.util.Set; +import java.util.stream.Stream; import org.springframework.beans.BeanUtils; import org.springframework.data.spel.EvaluationContextExtensionInformation.ExtensionTypeInformation.PublicMethodAndFieldFilter; +import org.springframework.data.spel.ExpressionDependencies.ExpressionDependency; import org.springframework.data.spel.spi.EvaluationContextExtension; import org.springframework.data.spel.spi.Function; import org.springframework.data.util.Streamable; @@ -102,14 +105,13 @@ class EvaluationContextExtensionInformation { } /** - * Return {@literal true} if this extension provides {@link ExpressionDependencies.ExpressionDependency}. + * Return {@literal true} if this extension provides {@link ExpressionDependency}. * * @param dependency the expression dependency. - * @return {@literal true} if {@link ExpressionDependencies.ExpressionDependency} is provided by this root object or - * the extension itself. + * @return {@literal true} if {@link ExpressionDependency} is provided by this root object or the extension itself. * @since 2.4 */ - public boolean provides(ExpressionDependencies.ExpressionDependency dependency) { + public boolean provides(ExpressionDependency dependency) { // We don't know, extension declares Object getRootObject if (!rootObjectInformation.isPresent()) { @@ -159,13 +161,13 @@ class EvaluationContextExtensionInformation { } /** - * Return {@literal true} if this extension provides {@link ExpressionDependencies.ExpressionDependency}. + * Return {@literal true} if this extension provides {@link ExpressionDependency}. * * @param dependency the expression dependency. - * @return {@literal true} if {@link ExpressionDependencies.ExpressionDependency} is provided by this root object. + * @return {@literal true} if {@link ExpressionDependency} is provided by this root object. * @since 2.4 */ - public boolean provides(ExpressionDependencies.ExpressionDependency dependency) { + public boolean provides(ExpressionDependency dependency) { if (dependency.isPropertyOrField()) { return properties.containsKey(dependency.getSymbol()); @@ -199,12 +201,12 @@ class EvaluationContextExtensionInformation { static class PublicMethodAndFieldFilter implements MethodFilter, FieldFilter { - public static final PublicMethodAndFieldFilter STATIC = new PublicMethodAndFieldFilter(true); - public static final PublicMethodAndFieldFilter NON_STATIC = new PublicMethodAndFieldFilter(false); + static final PublicMethodAndFieldFilter STATIC = new PublicMethodAndFieldFilter(true); + static final PublicMethodAndFieldFilter NON_STATIC = new PublicMethodAndFieldFilter(false); private final boolean staticOnly; - public PublicMethodAndFieldFilter(boolean staticOnly) { + PublicMethodAndFieldFilter(boolean staticOnly) { this.staticOnly = staticOnly; } @@ -251,7 +253,7 @@ class EvaluationContextExtensionInformation { private static final RootObjectInformation NONE = new RootObjectInformation(Object.class); private final Map accessors; - private final Collection methods; + private final Methods methods; private final Collection fields; /** @@ -260,12 +262,12 @@ class EvaluationContextExtensionInformation { * * @param type must not be {@literal null}. */ - public RootObjectInformation(Class type) { + RootObjectInformation(Class type) { Assert.notNull(type, "Type must not be null!"); this.accessors = new HashMap<>(); - this.methods = new HashSet<>(); + this.methods = new Methods(); this.fields = new ArrayList<>(); if (Object.class.equals(type)) { @@ -293,7 +295,7 @@ class EvaluationContextExtensionInformation { * @param target can be {@literal null}. * @return the methods */ - public MultiValueMap getFunctions(Optional target) { + MultiValueMap getFunctions(Optional target) { return target.map(this::getFunctions).orElseGet(LinkedMultiValueMap::new); } @@ -307,7 +309,7 @@ class EvaluationContextExtensionInformation { * * @return the properties */ - public Map getProperties(Optional target) { + Map getProperties(Optional target) { return target.map(it -> { @@ -323,12 +325,17 @@ class EvaluationContextExtensionInformation { } /** - * Returns whether this root object information provides {@link ExpressionDependencies.ExpressionDependency}. + * Returns whether this root object information provides {@link ExpressionDependency}. * * @param dependency the expression dependency. - * @return {@literal true} if {@link ExpressionDependencies.ExpressionDependency} is provided by this root object. + * @return {@literal true} if {@link ExpressionDependency} is provided by this root object. + * @since 2.4 */ - public boolean provides(ExpressionDependencies.ExpressionDependency dependency) { + boolean provides(ExpressionDependency dependency) { + + if (!dependency.isMethod() && !dependency.isPropertyOrField()) { + return false; + } if (dependency.isPropertyOrField()) { @@ -346,14 +353,7 @@ class EvaluationContextExtensionInformation { } if (dependency.isMethod()) { - - for (Method method : methods) { - if (method.getName().equals(dependency.getSymbol())) { - return true; - } - } - - return false; + return methods.containsMethodName(dependency.getSymbol()); } return false; @@ -369,4 +369,34 @@ class EvaluationContextExtensionInformation { return map.isEmpty() ? Collections.emptyMap() : Collections.unmodifiableMap(map); } + + /** + * @since 2.4 + * @author Christpoh Strobl + */ + static class Methods { + + private final Collection methods; + private final Set methodNames; + + Methods() { + + this.methods = new HashSet<>(); + this.methodNames = new HashSet<>(); + } + + void add(Method method) { + + methods.add(method); + methodNames.add(method.getName()); + } + + boolean containsMethodName(String name) { + return methodNames.contains(name); + } + + public Stream stream() { + return methods.stream(); + } + } } diff --git a/src/main/java/org/springframework/data/spel/ExpressionDependencies.java b/src/main/java/org/springframework/data/spel/ExpressionDependencies.java index 34f6f6f4f..cdb3c8258 100644 --- a/src/main/java/org/springframework/data/spel/ExpressionDependencies.java +++ b/src/main/java/org/springframework/data/spel/ExpressionDependencies.java @@ -16,6 +16,7 @@ package org.springframework.data.spel; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Iterator; import java.util.LinkedHashSet; @@ -37,6 +38,7 @@ import org.springframework.util.ObjectUtils; * Value object capturing dependencies to a method or property/field that is referenced from a SpEL expression. * * @author Mark Paluch + * @author Christoph Strobl * @since 2.4 */ public class ExpressionDependencies implements Streamable { @@ -52,12 +54,47 @@ public class ExpressionDependencies implements Streamable dependencies) { + + if (dependencies.isEmpty()) { + return EMPTY; + } + + return new ExpressionDependencies(new ArrayList<>(new LinkedHashSet<>(dependencies))); + } + + /** + * Return an {@link ExpressionDependencies} object representing the merged collection of {@link ExpressionDependency + * dependencies} withing the given {@link ExpressionDependencies} collection. + * + * @return {@link ExpressionDependencies} holding a set of merged {@link ExpressionDependency dependencies} or + * {@link ExpressionDependencies#none() none} if the given {@link Iterable} is {@link Collection#isEmpty() + * empty}. + */ + public static ExpressionDependencies merged(Iterable dependencies) { + + if (!dependencies.iterator().hasNext()) { + return EMPTY; + } + + List dependencySet = new ArrayList<>(); + dependencies.forEach(it -> dependencySet.addAll(it.dependencies)); + return ExpressionDependencies.of(dependencySet); + } + /** * Discover all expression dependencies that are referenced in the {@link SpelNode expression root}. * @@ -65,7 +102,7 @@ public class ExpressionDependencies implements Streamable, EvaluationContextExtensionInformation> extensionInformationCache = new ConcurrentHashMap<>(); + private final Map extensionInformationCache = new ConcurrentHashMap<>(); private final Lazy> extensions; private ListableBeanFactory beanFactory; @@ -181,7 +182,7 @@ public class ExtensionAwareEvaluationContextProvider implements EvaluationContex * @return */ EvaluationContextExtensionInformation getOrCreateInformation(Class extension) { - return extensionInformationCache.computeIfAbsent(extension, + return extensionInformationCache.computeIfAbsent(ClassUtils.getUserClass(extension).getName(), type -> new EvaluationContextExtensionInformation(extension)); } diff --git a/src/main/java/org/springframework/data/spel/spi/EvaluationContextExtension.java b/src/main/java/org/springframework/data/spel/spi/EvaluationContextExtension.java index 7badce5a2..c5601dcf1 100644 --- a/src/main/java/org/springframework/data/spel/spi/EvaluationContextExtension.java +++ b/src/main/java/org/springframework/data/spel/spi/EvaluationContextExtension.java @@ -18,13 +18,13 @@ package org.springframework.data.spel.spi; import java.util.Collections; import java.util.Map; -import org.springframework.data.repository.query.ExtensionAwareQueryMethodEvaluationContextProvider; import org.springframework.expression.EvaluationContext; import org.springframework.lang.Nullable; /** * SPI to allow adding a set of properties and function definitions accessible via the root of an - * {@link EvaluationContext} provided by a {@link ExtensionAwareQueryMethodEvaluationContextProvider}. + * {@link EvaluationContext} provided by an + * {@link org.springframework.data.repository.query.ExtensionAwareQueryMethodEvaluationContextProvider}. *

* Extensions can be ordered by following Spring's {@link org.springframework.core.Ordered} conventions. * diff --git a/src/main/java/org/springframework/data/spel/spi/ExtensionIdAware.java b/src/main/java/org/springframework/data/spel/spi/ExtensionIdAware.java index d9557298d..48a7c97f6 100644 --- a/src/main/java/org/springframework/data/spel/spi/ExtensionIdAware.java +++ b/src/main/java/org/springframework/data/spel/spi/ExtensionIdAware.java @@ -29,7 +29,7 @@ public interface ExtensionIdAware { * Return the identifier of the extension. The id can be leveraged by users to fully qualify property lookups and thus * overcome ambiguities in case multiple extensions expose properties with the same name. * - * @return the extension id, must not be {@literal null}. + * @return the extension id, never {@literal null}. */ String getExtensionId(); } diff --git a/src/main/java/org/springframework/data/spel/spi/ReactiveEvaluationContextExtension.java b/src/main/java/org/springframework/data/spel/spi/ReactiveEvaluationContextExtension.java index 0a28b9eb5..47936f058 100644 --- a/src/main/java/org/springframework/data/spel/spi/ReactiveEvaluationContextExtension.java +++ b/src/main/java/org/springframework/data/spel/spi/ReactiveEvaluationContextExtension.java @@ -17,12 +17,12 @@ package org.springframework.data.spel.spi; import reactor.core.publisher.Mono; -import org.springframework.data.repository.query.ExtensionAwareQueryMethodEvaluationContextProvider; import org.springframework.expression.EvaluationContext; /** * SPI to resolve a {@link EvaluationContextExtension} to make it accessible via the root of an - * {@link EvaluationContext} provided by a {@link ExtensionAwareQueryMethodEvaluationContextProvider}. + * {@link EvaluationContext} provided by a + * {@link org.springframework.data.repository.query.ExtensionAwareQueryMethodEvaluationContextProvider}. *

* Extensions can be ordered by following Spring's {@link org.springframework.core.Ordered} conventions. * diff --git a/src/test/java/org/springframework/data/spel/ExpressionDependenciesUnitTests.java b/src/test/java/org/springframework/data/spel/ExpressionDependenciesUnitTests.java index 59e3f1c40..56cd6c45a 100644 --- a/src/test/java/org/springframework/data/spel/ExpressionDependenciesUnitTests.java +++ b/src/test/java/org/springframework/data/spel/ExpressionDependenciesUnitTests.java @@ -18,7 +18,6 @@ package org.springframework.data.spel; import static org.assertj.core.api.Assertions.*; import org.junit.jupiter.api.Test; - import org.springframework.expression.spel.SpelNode; import org.springframework.expression.spel.standard.SpelExpression; import org.springframework.expression.spel.standard.SpelExpressionParser; @@ -27,6 +26,7 @@ import org.springframework.expression.spel.standard.SpelExpressionParser; * Unit tests for {@link ExpressionDependencies}. * * @author Mark Paluch + * @author Christoph Strobl */ class ExpressionDependenciesUnitTests { @@ -74,6 +74,20 @@ class ExpressionDependenciesUnitTests { "principal"); } + @Test // DATACMNS-1108 + void shouldExtractAll() { + + String expression = "hello().hasRole(principal.emailAddress, principal.somethingElse).somethingElse()"; + + SpelExpression spelExpression = (SpelExpression) PARSER.parseExpression(expression); + SpelNode ast = spelExpression.getAST(); + + ExpressionDependencies dependencies = ExpressionDependencies.discover(ast, false); + + assertThat(dependencies).extracting(ExpressionDependencies.ExpressionDependency::getSymbol).containsOnly("hello", + "hasRole", "principal", "emailAddress", "somethingElse"); + } + @Test // DATACMNS-1108 void shouldMergeDependencies() { diff --git a/src/test/java/org/springframework/data/spel/ReactiveExtensionAwareEvaluationContextProviderUnitTests.java b/src/test/java/org/springframework/data/spel/ReactiveExtensionAwareEvaluationContextProviderUnitTests.java index aef72b19b..9e6550dc1 100644 --- a/src/test/java/org/springframework/data/spel/ReactiveExtensionAwareEvaluationContextProviderUnitTests.java +++ b/src/test/java/org/springframework/data/spel/ReactiveExtensionAwareEvaluationContextProviderUnitTests.java @@ -21,12 +21,10 @@ import reactor.core.publisher.Mono; import reactor.test.StepVerifier; import java.util.Arrays; -import java.util.Collections; import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; - import org.springframework.data.spel.spi.EvaluationContextExtension; import org.springframework.data.spel.spi.ReactiveEvaluationContextExtension; import org.springframework.expression.Expression; @@ -37,6 +35,7 @@ import org.springframework.expression.spel.standard.SpelExpressionParser; * Unit tests for {@link ReactiveExtensionAwareEvaluationContextProvider}. * * @author Mark Paluch + * @author Christoph Strobl */ class ReactiveExtensionAwareEvaluationContextProviderUnitTests { @@ -108,7 +107,7 @@ class ReactiveExtensionAwareEvaluationContextProviderUnitTests { ExpressionDependencies dependencies = ExpressionDependencies.discover(expression); ReactiveExtensionAwareEvaluationContextProvider provider = new ReactiveExtensionAwareEvaluationContextProvider( - Collections.singletonList(GenericReactiveExtension.INSTANCE)); + Arrays.asList(SampleReactiveExtension.INSTANCE, GenericReactiveExtension.INSTANCE)); provider.getEvaluationContextLater(new Object[0], dependencies).map(expression::getValue) // .as(StepVerifier::create) // @@ -116,6 +115,58 @@ class ReactiveExtensionAwareEvaluationContextProviderUnitTests { .verifyComplete(); assertThat(GenericModelRoot.creationCounter).hasValue(1); + assertThat(SecurityExpressionRoot.creationCounter).hasValue(0); + } + + @Test // DATACMNS-1108 + void doesNotLoadExtensionForDirectCall() { + + Expression expression = PARSER.parseExpression( + "T(org.springframework.data.spel.ReactiveExtensionAwareEvaluationContextProviderUnitTests.WithStaticRole).hasRole('ADMIN')"); + ExpressionDependencies dependencies = ExpressionDependencies.discover(expression); + + ReactiveExtensionAwareEvaluationContextProvider provider = new ReactiveExtensionAwareEvaluationContextProvider( + Arrays.asList(SampleReactiveExtension.INSTANCE, GenericReactiveExtension.INSTANCE)); + + provider.getEvaluationContextLater(new Object[0], dependencies).map(expression::getValue) // + .as(StepVerifier::create) // + .expectNext(true) // + .verifyComplete(); + + assertThat(GenericModelRoot.creationCounter).hasValue(1); + assertThat(SecurityExpressionRoot.creationCounter).hasValue(0); + } + + @Test // DATACMNS-1108 + void loadsExtensionEvenWhenRootObjectMethodMatches() { + + Expression expression = PARSER.parseExpression("principal.name"); + ExpressionDependencies dependencies = ExpressionDependencies.discover(expression); + + ReactiveExtensionAwareEvaluationContextProvider provider = new ReactiveExtensionAwareEvaluationContextProvider( + Arrays.asList(SampleReactiveExtension.INSTANCE, GenericReactiveExtension.INSTANCE)); + + provider.getEvaluationContextLater(new WithRole(), dependencies).map(expression::getValue) // + .as(StepVerifier::create) // + .expectNext("Walter") // + .verifyComplete(); + + assertThat(GenericModelRoot.creationCounter).hasValue(1); + assertThat(SecurityExpressionRoot.creationCounter).hasValue(1); + } + + public static class WithStaticRole { + + public static boolean hasRole(String arg) { + return arg.equals("ADMIN"); + } + } + + public static class WithRole { + + public Object getPrincipal() { + return new MyPrincipal("Jesse"); + } } /** @@ -216,14 +267,20 @@ class ReactiveExtensionAwareEvaluationContextProviderUnitTests { } public Object getPrincipal() { - return new MyPrincipal(); + return new MyPrincipal("Walter"); } } static class MyPrincipal { + private final String name; + + public MyPrincipal(String name) { + this.name = name; + } + public String getName() { - return "Walter"; + return name; } } }