DATACMNS-1026 ExtensionAwareEvaluationContextProvider now returns all overloaded methods as functions #217

schauder · 2017-05-17T08:28:14Z

All overloaded methods are now available in SPeL expressions.

The key of the Map used for collecting methods is now the name and the length of the argument list.

It is also actually a MultiValueMap to allow multiple overloaded methods.

Among methods with identical argument list from different sources in the same extension (extension, root object, aliases) the last one in the order in parens wins.

If there is more than one method for an application the following rules are applied:

If there is one method with exact matching types in the argument list it is used. Otherwise, an exception is thrown.

…s all overloaded methods as functions. All overloaded methods are now available in SPeL expressions. The key of the Map used for collecting methods is now the name and the length of the argument list. It is also actually a MultiValueMap to allow multiple overloaded methods. Among methods with identical argument list from different sources in the same extension (extension, root object, aliases) the last one in the order in parens wins. If there is more than one method for an application the following rules are applied: If there is one method with exact matching types in the argument list it is used, otherwise an exception is thrown.

Minor improvements to Javadoc.

odrotbohm

Cosmetics. Looks good beyond that.

odrotbohm · 2017-05-17T09:07:39Z

src/main/java/org/springframework/data/repository/query/FunctionsMap.java

+		}
+
+		Optional<Function> exactMatch = candidates.stream().filter(f -> f.supportsExact(argumentTypes)).findFirst();
+		if (exactMatch.isPresent()) {


return exactMatch.orElseThrow(…)?

I thought about that, but then we return a Function instead of Optional<Function> and we end up sooner or later either rewrapping it in an Optional or doing null checks.

What we really need is
ifPresentOrElse but that is java9

I just removed the else to make it a little lighter on the eye.

I see. That makes sense. Maybe inverting the check here is an option, too. if (!exactMatch.isPresent()) { throw … }.

I also wonder: we don't really detect multiple exact matches here, do we? It just barks if we don't find any exact match, right? Actually that renders the exception message wrong as it's only thrown if there's no match at all, not if there are multiple ones matching.

Correct: multiple exact matches are not possible at this point because we drop those when we add elements to Functions this ensures the behavior of overwriting of methods from different sources (extension, root object, and aliases).

But we only check for an exact match if we have more than one match (candidates). So we throw the exception if we have multiple candidates and can't disambiguate between them.

We could improve the message to:

There are multiple matching methods for arguments of type , but no exact match. Make sure to provide only one matching overload or one with exactly those types.

What do you think?

But that's not what the code does is it? If we have multiple candidates and have multiple ones that potentially match exactly, we just pick the first one. Is that intended? I thought the idea was to also reject that case, wasn't it?

There can't be multiple exact matches. We don't add them to the Map contained in functions in the first place. see the addAll methods.

Ah, now that makes sense. Thanks!

odrotbohm · 2017-05-17T09:09:18Z

src/main/java/org/springframework/data/repository/query/FunctionsMap.java

+		}
+	}
+
+	@Data


Could this be @Value instead?

odrotbohm · 2017-05-17T09:13:43Z

src/main/java/org/springframework/data/repository/query/spi/Function.java

+	}
+
+	/**
+	 * Checks wither this {@code Function} has the same signature as another {@code Function}.


odrotbohm · 2017-05-17T09:21:07Z

src/main/java/org/springframework/data/util/MultiValueMapCollector.java

+	 * @param keyFunction {@link Function} to create a key from an element of the {@link java.util.stream.Stream}
+	 * @param valueFunction {@link Function} to create a value from an element of the {@link java.util.stream.Stream}
+	 */
+	public MultiValueMapCollector(Function<T, K> keyFunction, Function<T, V> valueFunction) {


@RequiredArgsConstructor? I guess a static factory method in StreamUtils would align nicely with the ones we have in place for unmodifiable lists and sets.

odrotbohm · 2017-05-17T09:21:29Z

src/main/java/org/springframework/data/util/MultiValueMapCollector.java

+	@Override
+	public BinaryOperator<MultiValueMap<K, V>> combiner() {
+
+		return (map1,map2) -> {


Space after comma.

odrotbohm · 2017-05-17T09:31:42Z

src/main/java/org/springframework/data/repository/query/FunctionsMap.java

+ * @author Jens Schauder
+ * @since 2.0
+ */
+class FunctionsMap {


What do you think of a plain Functions?

odrotbohm · 2017-05-17T09:34:59Z

.../java/org/springframework/data/repository/query/ExtensionAwareEvaluationContextProvider.java

 		 */
-		public Map<String, Function> getFunctions() {
+		FunctionsMap getFunctions() {


Wonder if it makes sense to extract an interface here so that clients can't modify the instance anymore, i.e. return something that doesn't expose the addAll(…) methods anymore.

This is just in a private class anyway. A separate interface sounds like overkill to me in this case.

odrotbohm · 2017-05-17T09:35:27Z

.../java/org/springframework/data/repository/query/ExtensionAwareEvaluationContextProvider.java

+			FunctionsMap functions = adapter.getFunctions();
+
+			Optional<Function> function = functions.get(name, argumentTypes);
+			return function.map(FunctionMethodExecutor::new);


Inline function?

odrotbohm · 2017-05-17T09:35:40Z

.../java/org/springframework/data/repository/query/ExtensionAwareEvaluationContextProvider.java

-					.findFirst().map(Entry::getValue).map(FunctionMethodExecutor::new);
+			FunctionsMap functions = adapter.getFunctions();
+
+			Optional<Function> function = functions.get(name, argumentTypes);


Inline functions?

odrotbohm · 2017-05-17T09:42:52Z

.../springframework/data/repository/query/ExtensionAwareEvaluationContextProviderUnitTests.java

+	@Test // DATACMNS-1026
+	public void overloadedMethodsGetResolved() throws Exception {
+
+		provider = new ExtensionAwareEvaluationContextProvider(


Shall we extract that setup step? It's repeated in every test below.

And formatting.

odrotbohm

Looks good to me, I'll take it from here.

…ll overloaded methods as functions. All overloaded methods are now available in SPeL expressions. Among methods with identical argument list from different sources in the same extension (extension, root object, aliases) the last one in the order in parens wins. If there is more than one method for an application the following rules are applied: if there is one method with exact matching types in the argument list it is used, otherwise an exception is thrown. Original pull request: #217.

Minor improvements to Javadoc. Original pull request: #217.

Minor refactorings here in there for clarity. Original pull request: #217.

schauder added 2 commits May 17, 2017 10:06

DATACMNS-1026 - Polishing.

4aa22d7

Minor improvements to Javadoc.

odrotbohm requested changes May 17, 2017

View reviewed changes

schauder added 2 commits May 17, 2017 13:39

DATACMNS-1026 - Changes from code review.

0502148

And formatting.

DATACMNS-1026 - Improved exception message for ambiguous methods.

c19510f

odrotbohm approved these changes May 17, 2017

View reviewed changes

odrotbohm self-assigned this May 17, 2017

odrotbohm pushed a commit that referenced this pull request May 18, 2017

DATACMNS-1026 - Polishing.

1bf318f

Minor improvements to Javadoc. Original pull request: #217.

odrotbohm added a commit that referenced this pull request May 18, 2017

DATACMNS-1026 - Polishing.

0e36b43

Minor refactorings here in there for clarity. Original pull request: #217.

odrotbohm closed this May 18, 2017

odrotbohm deleted the issue/DATACMNS-1026 branch May 18, 2017 05:07

spring-projects-issues mentioned this pull request Dec 30, 2020

ExtensionAwareEvaluationContextProvider should return all overloaded methods as functions [DATACMNS-1026] #1475

Closed

+              		}
+              	}
+              	@Data

DATACMNS-1026 ExtensionAwareEvaluationContextProvider now returns all overloaded methods as functions #217

DATACMNS-1026 ExtensionAwareEvaluationContextProvider now returns all overloaded methods as functions #217

Uh oh!

Conversation

schauder commented May 17, 2017

Uh oh!

odrotbohm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odrotbohm May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odrotbohm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

odrotbohm May 17, 2017 •

edited

Loading