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 SpEL in @Query
#2826
Conversation
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.
Nice work. I have some remarks and questions and will do more tests later that week (being a little busy currently in the evenings).
And this needs to be added to the reactive parts as well.
src/test/java/org/springframework/data/elasticsearch/repositories/custommethod/QueryParam.java
Outdated
Show resolved
Hide resolved
src/test/java/org/springframework/data/elasticsearch/repositories/custommethod/QueryParam.java
Outdated
Show resolved
Hide resolved
...g/springframework/data/elasticsearch/repository/query/ElasticsearchStringQueryUnitTests.java
Outdated
Show resolved
Hide resolved
...g/springframework/data/elasticsearch/repository/query/ElasticsearchStringQueryUnitTests.java
Outdated
Show resolved
Hide resolved
...g/springframework/data/elasticsearch/repository/query/ElasticsearchStringQueryUnitTests.java
Outdated
Show resolved
Hide resolved
/** | ||
* Convert a collection into string for value part of the elasticsearch query. | ||
* <p> | ||
* The string should be wrapped with square brackets, with each element quoted therefore |
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.
quoted elements means Strings? What about other types, i.e. numeric ones?
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.
The non string types will be converted correctly by other converters in ConversionSerivce
. Take integers as the example:
{
"bool":{
"must":{
"terms":{
"age": [1, 2, 3]
}
}
}
}
I'll add more tests to prove this.
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.
And the comment is also changed to avoid this misleading.
Object targetElement = this.conversionService.convert( | ||
sourceElement, sourceType.elementTypeDescriptor(sourceElement), targetType); | ||
if (sourceElement instanceof String) { | ||
sb.add("\"" + targetElement + "\""); |
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.
targetElement
can be null
at this place; the +
and the valueOf
will convert this to `"null"; is that ok or should that produce an error?
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.
I've added the code to check that source
is not null
, and target
may have no need to check null again after that.
...n/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchQueryMethod.java
Outdated
Show resolved
Hide resolved
...n/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchQueryMethod.java
Outdated
Show resolved
Hide resolved
...n/antora/modules/ROOT/pages/elasticsearch/repositories/elasticsearch-repository-queries.adoc
Outdated
Show resolved
Hide resolved
Co-authored-by: Peter-Josef Meisch <pj.meisch@sothawo.com>
Review suggestions are accepted and more tests has been added. UPDATE: reactive parts is also finished. |
Closes #2083