DATACOUCH-616 - Fix parsing of String Query containing conditionals i… #265

mikereiche · 2020-09-22T23:45:07Z

…n quotes

For a query as below that has conditional portions, the parsing for
parameters was being done before the conditional portions were being
resolved, which left the parameters between quotes where they wre
not being recognized as query parameters.

@query("#{#n1ql.selectEntity} WHERE #{#n1ql.filter} " +
" #{#projectIds != null ? 'AND iata IN $1' : ''} ")
Long count(@param("projectIds") List projectIds)

You have read the Spring Data contribution guidelines.
There is a ticket in the bug tracker for the project in our JIRA.
You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.
You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

daschl · 2020-09-25T13:12:47Z

...ain/java/org/springframework/data/couchbase/repository/query/StringBasedN1qlQueryParser.java

@@ -192,11 +191,11 @@ private PlaceholderType checkPlaceholders(String statement) {
 		}

 		if (posCount > 0) {
-			return PlaceholderType.POSITIONAL;
+			return placeHolderType = PlaceholderType.POSITIONAL;


assignment and return in one ?

Yes. It used to be called int the constructor as
this.placeHolderType = checkPlaceholders(...)
Now it is public and gets called by StringN1qlQueryCreator - but the placeHolderType still needs to be set. I suppose it could be changed to not return a value - it was private, nothing else could be calling it for that.

I'll change it.

daschl · 2020-09-25T13:13:08Z

...ain/java/org/springframework/data/couchbase/repository/query/StringBasedN1qlQueryParser.java

@@ -103,7 +103,7 @@
 	private static final Logger LOGGER = LoggerFactory.getLogger(StringBasedN1qlQueryParser.class);
 	private final String statement;
 	private final QueryMethod queryMethod;
-	private final PlaceholderType placeHolderType;
+	private PlaceholderType placeHolderType;


you sure this is non final now? if so it maybe shold be volatile

It gets set when StringBasedN1qlQueryParser.checkPlaceHolders() is called by StringN1qlQueryCreator.getExpression(). To make it final, the code in getExpression() would need to be executed in the constructor. It can be done. Not sure that it is better. See next push.

daschl · 2020-09-25T13:14:52Z

src/main/java/org/springframework/data/couchbase/repository/query/StringN1qlQueryCreator.java

-		EvaluationContext evaluationContext = evaluationContextProvider.getEvaluationContext(
-				getQueryMethod().getParameters(), runtimeParameters);
-		N1QLExpression parsedStatement = x(this.queryParser.doParse(parser, evaluationContext, false));
+		boolean isCountQuery = queryMethod.getName().toLowerCase().startsWith("count"); // should be queryMethod.isCountQuery()


is this still a todo?

queryMethod.isCount() is in the datacouch-588 branch along with some others.

daschl

put in a couple comments which I dont think look right

JesusTheHun · 2020-09-25T15:07:21Z

Is it possible to backport this fix to future 3.2.11 if any ?

mikereiche · 2020-09-25T23:53:29Z

Is it possible to backport this fix to future 3.2.11 if any ?

Yes. The original issue was for 3.2.x

JesusTheHun · 2020-09-26T00:32:43Z

I'm the author of the issue. I was not sure you were going to make a new release for the 3.x branch though ;)
Thx

daschl

It would be good if we could have an integration test which covers this scenario, otherwise just minor nits

daschl · 2020-10-02T06:48:38Z

src/main/java/org/springframework/data/couchbase/core/ReactiveInsertByIdOperationSupport.java

@@ -98,7 +97,7 @@ private InsertOptions buildInsertOptions() {
 			} else if (durabilityLevel != DurabilityLevel.NONE) {
 				options.durability(durabilityLevel);
 			}
-			if (expiry != null) {
+			if (expiry != null && !expiry.equals(Duration.ofSeconds(0))) {


Nit: you can use Duration.ZERO;

daschl · 2020-10-02T06:48:45Z

src/main/java/org/springframework/data/couchbase/core/ReactiveUpsertByIdOperationSupport.java

@@ -98,7 +98,7 @@ private UpsertOptions buildUpsertOptions() {
 			} else if (durabilityLevel != DurabilityLevel.NONE) {
 				options.durability(durabilityLevel);
 			}
-			if (expiry != null) {
+			if (expiry != null && ! expiry.equals( Duration.ofSeconds(0))) {


Nit: you can use Duration.ZERO;

@query

…n quotes For a query as below that has conditional portions, the parsing for parameters was being done before the conditional portions were being resolved, which left the parameters between quotes where they wre not being recognized as query parameters. @query("#{#n1ql.selectEntity} WHERE #{#n1ql.filter} " + " #{#projectIds != null ? 'AND iata IN $1' : ''} ") Long count(@param("projectIds") List<String> projectIds)

@query

…n quotes For a query as below that has conditional portions, the parsing for parameters was being done before the conditional portions were being resolved, which left the parameters between quotes where they wre not being recognized as query parameters. @query("#{#n1ql.selectEntity} WHERE #{#n1ql.filter} " + " #{#projectIds != null ? 'AND iata IN $1' : ''} ") Long count(@param("projectIds") List projectIds)

mikereiche requested a review from daschl September 22, 2020 23:45

daschl reviewed Sep 25, 2020

View reviewed changes

daschl suggested changes Sep 25, 2020

View reviewed changes

mikereiche force-pushed the datacouch_616_string_query_parsing branch 2 times, most recently from f38dd09 to 3071763 Compare September 26, 2020 00:24

daschl suggested changes Oct 2, 2020

View reviewed changes

mikereiche force-pushed the datacouch_616_string_query_parsing branch from 3071763 to 6a58d84 Compare October 2, 2020 20:56

mikereiche closed this Oct 5, 2020

mp911de deleted the datacouch_616_string_query_parsing branch October 28, 2020 07:43

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

Positional parameters not detected inside SpEL expressions [DATACOUCH-616] #924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DATACOUCH-616 - Fix parsing of String Query containing conditionals i… #265

DATACOUCH-616 - Fix parsing of String Query containing conditionals i… #265

mikereiche commented Sep 22, 2020

daschl Sep 25, 2020

mikereiche Sep 25, 2020

daschl Sep 25, 2020

mikereiche Sep 25, 2020

daschl Sep 25, 2020

mikereiche Sep 25, 2020

daschl left a comment

JesusTheHun commented Sep 25, 2020 •

edited

Loading

mikereiche commented Sep 25, 2020

JesusTheHun commented Sep 26, 2020

daschl left a comment

daschl Oct 2, 2020

daschl Oct 2, 2020

DATACOUCH-616 - Fix parsing of String Query containing conditionals i… #265

DATACOUCH-616 - Fix parsing of String Query containing conditionals i… #265

Conversation

mikereiche commented Sep 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschl left a comment

Choose a reason for hiding this comment

JesusTheHun commented Sep 25, 2020 • edited Loading

mikereiche commented Sep 25, 2020

JesusTheHun commented Sep 26, 2020

daschl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesusTheHun commented Sep 25, 2020 •

edited

Loading