-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Value Expressions for Repository Query methods #4683
Conversation
Deprecations, missing Override annotations, test fix.
9f01de3
to
414ef64
Compare
d2c3a9d
to
e219fc9
Compare
This will also fix #2764 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some since
and deprecation
tags are off and we should update the documentation.
|
||
return new ValueExpressionEvaluator() { | ||
|
||
@Override | ||
public <T> T evaluate(String expressionString) { | ||
ValueExpression expression = valueExpressionDelegate.parse(expressionString); | ||
ValueEvaluationContext evaluationContext = valueEvaluationContextProvider.getEvaluationContext(accessor.getValues(), | ||
expression.getExpressionDependencies()); | ||
return (T) expression.evaluate(evaluationContext); | ||
} | ||
}; | ||
} | ||
|
||
/** | ||
* Obtain a {@link Mono publisher} emitting the {@link ValueExpressionEvaluator} suitable to evaluate expressions | ||
* backed by the given dependencies. | ||
* | ||
* @param dependencies must not be {@literal null}. | ||
* @param accessor must not be {@literal null}. | ||
* @return a {@link Mono} emitting the {@link ValueExpressionEvaluator} when ready. | ||
* @since 4.3 | ||
*/ | ||
protected Mono<ValueExpressionEvaluator> getValueExpressionEvaluatorLater(ExpressionDependencies dependencies, | ||
MongoParameterAccessor accessor) { | ||
|
||
return valueEvaluationContextProvider.getEvaluationContextLater(accessor.getValues(), dependencies) | ||
.map(evaluationContext -> { | ||
|
||
return new ValueExpressionEvaluator() { | ||
@Override | ||
public <T> T evaluate(String expressionString) { | ||
|
||
ValueExpression expression = valueExpressionDelegate.parse(expressionString); | ||
|
||
return (T) expression.evaluate(evaluationContext); | ||
} | ||
}; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move the inner ValueExpressionEvaluator
to a dedicated class that holds the evaluation context, so we can use for both?
@@ -406,7 +410,7 @@ public void capturingExpressionDependenciesShouldNotThrowParseErrorForSpelOnlyJs | |||
String json = "?#{ true ? { 'name': #name } : { 'name' : #name + 'trouble' } }"; | |||
|
|||
new ParameterBindingDocumentCodec().captureExpressionDependencies(json, (index) -> args[index], | |||
new SpelExpressionParser()); | |||
ValueExpressionParser.create(SpelExpressionParser::new)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might make sense to also add some tests to the binding parser and check if the current test setup is making use of value expressions already.
@Test // DATAMONGO-1244 | ||
public void shouldSupportExpressionsInCustomQueriesWithNestedObject() { | ||
|
||
ConvertingParameterAccessor accessor = StubParameterAccessor.getAccessor(converter, true, "param1", "param2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why's param2
gone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because the method declares 2 parameters and previously we passed in 3 arguments.
org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accessor); | ||
org.springframework.data.mongodb.core.query.Query reference = new BasicQuery("{'lastname' : 'bar'}"); | ||
|
||
assertThat(query.getQueryObject()).isEqualTo(reference.getQueryObject()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do we expect when the property is missing and does not have a default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it should break (and it does right now, I'll add a test for that)
No description provided.