From dae9b49ed0ae0289a2a75fece197edc0f28c4c21 Mon Sep 17 00:00:00 2001 From: Yuqi Du Date: Wed, 8 Nov 2023 16:17:01 -0800 Subject: [PATCH 1/4] $in with empty array should find nothing --- .../model/impl/ExpressionBuilder.java | 22 ++++- .../operation/model/impl/FindOperation.java | 4 +- .../jsonapi/api/v1/FindIntegrationTest.java | 98 ++++++++++++++++++- 3 files changed, 119 insertions(+), 5 deletions(-) diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ExpressionBuilder.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ExpressionBuilder.java index 070aaa9767..4513055f02 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ExpressionBuilder.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ExpressionBuilder.java @@ -16,6 +16,12 @@ public class ExpressionBuilder { public static List> buildExpressions( LogicalExpression logicalExpression, DBFilterBase.IDFilter additionalIdFilter) { + // an empty filter should find everything + if (logicalExpression.isEmpty()) { + ArrayList> list = new ArrayList<>(); + list.add(null); + return list; + } // after validate in FilterClauseDeserializer, // partition key column key will not be nested under OR operator // so we can collect all id_conditions, then do a combination to generate separate queries @@ -43,8 +49,7 @@ private static List> buildExpressionWithId( if (idFilters.isEmpty() && additionalIdFilter == null) { // no idFilters in filter clause and no additionalIdFilter if (expressionWithoutId == null) { - expressionsWithId.add(null); // should find everything - return expressionsWithId; + return null; // should find nothing } else { return List.of(expressionWithoutId); } @@ -91,12 +96,17 @@ private static Expression buildExpressionRecursive( } conditionExpressions.add(subExpressionCondition); } + + boolean hasInFilterThisLevel = false; + boolean InFilterThisLevelWithEmptyArray = true; // second for loop, is to iterate all subComparisonExpression for (ComparisonExpression comparisonExpression : logicalExpression.comparisonExpressions) { for (DBFilterBase dbFilter : comparisonExpression.getDbFilters()) { if (dbFilter instanceof DBFilterBase.InFilter inFilter) { + hasInFilterThisLevel = true; List inFilterConditions = inFilter.getAll(); if (!inFilterConditions.isEmpty()) { + InFilterThisLevelWithEmptyArray = false; List> inConditionsVariables = inFilterConditions.stream().map(Variable::of).toList(); conditionExpressions.add(ExpressionUtils.orOf(inConditionsVariables)); @@ -115,6 +125,14 @@ private static Expression buildExpressionRecursive( if (conditionExpressions.isEmpty()) { return null; } + // when having an empty array $in, if $in occurs within an $and logic, entire $and should match + // nothing + if (hasInFilterThisLevel + && InFilterThisLevelWithEmptyArray + && logicalExpression.getLogicalRelation().equals(LogicalExpression.LogicalOperator.AND)) { + return null; + } + return ExpressionUtils.buildExpression( conditionExpressions, logicalExpression.getLogicalRelation().getOperator()); } diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/FindOperation.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/FindOperation.java index 92da09a0a7..3004483183 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/FindOperation.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/FindOperation.java @@ -394,7 +394,7 @@ public ReadDocument getNewDocument() { private List buildSelectQueries(DBFilterBase.IDFilter additionalIdFilter) { List> expressions = ExpressionBuilder.buildExpressions(logicalExpression, additionalIdFilter); - if (expressions == null) { // in filter, but with empty values, find nothing + if (expressions == null) { // find nothing return List.of(); } List queries = new ArrayList<>(expressions.size()); @@ -501,7 +501,7 @@ private List buildSortedSelectQueries( DBFilterBase.IDFilter additionalIdFilter) { List> expressions = ExpressionBuilder.buildExpressions(logicalExpression, additionalIdFilter); - if (expressions == null) { + if (expressions == null) { // find nothing return List.of(); } String[] columns = sortedDataColumns; diff --git a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindIntegrationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindIntegrationTest.java index 6b88fb5fb2..a069399db7 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindIntegrationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindIntegrationTest.java @@ -370,7 +370,7 @@ public void inConditionWithOtherCondition() { } @Test - public void inConditionEmptyArray() { + public void idInConditionEmptyArray() { String json = """ { @@ -393,6 +393,102 @@ public void inConditionEmptyArray() { .body("errors", is(nullValue())); } + @Test + public void nonIDInConditionEmptyArray() { + String json = + """ + { + "find": { + "filter" : { + "username" : {"$in" : []} + } + } + } + """; + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body(json) + .when() + .post(CollectionResource.BASE_PATH, namespaceName, collectionName) + .then() + .statusCode(200) + .body("data.documents", hasSize(0)) + .body("status", is(nullValue())) + .body("errors", is(nullValue())); + } + + @Test + public void nonIDInConditionEmptyArrayAnd() { + String json = + """ + { + "find": { + "filter" : { + "$and": [ + { + "age": { + "$in": [] + } + }, + { + "username": "user1" + } + ] + } + } + } + """; + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body(json) + .when() + .post(CollectionResource.BASE_PATH, namespaceName, collectionName) + .then() + .statusCode(200) + .body("data.documents", hasSize(0)) + .body("status", is(nullValue())) + .body("errors", is(nullValue())); + } + + @Test + public void nonIDInConditionEmptyArrayOr() { + String json = + """ + { + "find": { + "filter" : { + "$or": [ + { + "age": { + "$in": [] + } + }, + { + "username": "user1" + } + ] + } + } + } + """; + String expected1 = + "{\"_id\":\"doc1\", \"username\":\"user1\", \"active_user\":true, \"date\" : {\"$date\": 1672531200000}}"; + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body(json) + .when() + .post(CollectionResource.BASE_PATH, namespaceName, collectionName) + .then() + .statusCode(200) + .body("data.documents", hasSize(1)) + .body("status", is(nullValue())) + .body("errors", is(nullValue())) + .body("data.documents[0]", jsonEquals(expected1)); + } + @Test public void inOperatorEmptyArrayWithAdditionalFilters() { String json = From 5f5133783b544dc332d74c3caf8f9dc3b802d89a Mon Sep 17 00:00:00 2001 From: Yuqi Du Date: Wed, 8 Nov 2023 16:44:02 -0800 Subject: [PATCH 2/4] unit test --- .../model/impl/FindOperationTest.java | 31 ------------------- 1 file changed, 31 deletions(-) diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/FindOperationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/FindOperationTest.java index b6bbc9ede9..c247a337b9 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/FindOperationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/FindOperationTest.java @@ -117,10 +117,6 @@ public void findAll() throws Exception { Values.of(doc2)))); LogicalExpression implicitAnd = LogicalExpression.and(); - implicitAnd.comparisonExpressions.add(new ComparisonExpression(null, null, null)); - List filters = List.of(); - implicitAnd.comparisonExpressions.get(0).setDBFilters(filters); - FindOperation operation = FindOperation.unsorted( COMMAND_CONTEXT, @@ -1537,10 +1533,6 @@ public void findAllSort() throws Exception { Values.NULL))); LogicalExpression implicitAnd = LogicalExpression.and(); - implicitAnd.comparisonExpressions.add(new ComparisonExpression(null, null, null)); - List filters = List.of(); - implicitAnd.comparisonExpressions.get(0).setDBFilters(filters); - FindOperation operation = FindOperation.sorted( COMMAND_CONTEXT, @@ -1755,10 +1747,6 @@ public void findAllSortByDate() throws Exception { Values.of(1672531600000L)))); LogicalExpression implicitAnd = LogicalExpression.and(); - implicitAnd.comparisonExpressions.add(new ComparisonExpression(null, null, null)); - List filters = List.of(); - implicitAnd.comparisonExpressions.get(0).setDBFilters(filters); - FindOperation operation = FindOperation.sorted( COMMAND_CONTEXT, @@ -1954,10 +1942,6 @@ public void findAllSortWithSkip() throws Exception { Values.NULL))); LogicalExpression implicitAnd = LogicalExpression.and(); - implicitAnd.comparisonExpressions.add(new ComparisonExpression(null, null, null)); - List filters = List.of(); - implicitAnd.comparisonExpressions.get(0).setDBFilters(filters); - FindOperation operation = FindOperation.sorted( COMMAND_CONTEXT, @@ -2151,10 +2135,6 @@ public void findAllSortDescending() throws Exception { Values.NULL))); LogicalExpression implicitAnd = LogicalExpression.and(); - implicitAnd.comparisonExpressions.add(new ComparisonExpression(null, null, null)); - List filters = List.of(); - implicitAnd.comparisonExpressions.get(0).setDBFilters(filters); - FindOperation operation = FindOperation.sorted( COMMAND_CONTEXT, @@ -2551,10 +2531,6 @@ public void vectorSearch() throws Exception { Values.of(doc2)))); LogicalExpression implicitAnd = LogicalExpression.and(); - implicitAnd.comparisonExpressions.add(new ComparisonExpression(null, null, null)); - List filters = List.of(); - implicitAnd.comparisonExpressions.get(0).setDBFilters(filters); - FindOperation operation = FindOperation.vsearch( VECTOR_COMMAND_CONTEXT, @@ -2671,9 +2647,6 @@ public void vectorSearchWithSimilarityProjection() throws Exception { } """)); LogicalExpression implicitAnd = LogicalExpression.and(); - implicitAnd.comparisonExpressions.add(new ComparisonExpression(null, null, null)); - List filters = List.of(); - implicitAnd.comparisonExpressions.get(0).setDBFilters(filters); FindOperation operation = FindOperation.vsearch( @@ -2793,10 +2766,6 @@ public void vectorSearchWithSimilarityProjectionDotProduct() throws Exception { } """)); LogicalExpression implicitAnd = LogicalExpression.and(); - implicitAnd.comparisonExpressions.add(new ComparisonExpression(null, null, null)); - List filters = List.of(); - implicitAnd.comparisonExpressions.get(0).setDBFilters(filters); - FindOperation operation = FindOperation.vsearch( VECTOR_DOT_PRODUCT_COMMAND_CONTEXT, From a8cf25a3a9c2cf4158e6ea84182075bfd218e2fc Mon Sep 17 00:00:00 2001 From: Yuqi Du Date: Wed, 8 Nov 2023 18:20:15 -0800 Subject: [PATCH 3/4] fix test --- .../jsonapi/service/operation/model/impl/ExpressionBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ExpressionBuilder.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ExpressionBuilder.java index 4513055f02..6ab3a49565 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ExpressionBuilder.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ExpressionBuilder.java @@ -17,7 +17,7 @@ public class ExpressionBuilder { public static List> buildExpressions( LogicalExpression logicalExpression, DBFilterBase.IDFilter additionalIdFilter) { // an empty filter should find everything - if (logicalExpression.isEmpty()) { + if (logicalExpression.isEmpty() && additionalIdFilter == null) { ArrayList> list = new ArrayList<>(); list.add(null); return list; From 713850c245bffca2ae77a9b38f8aa63806b0bdf1 Mon Sep 17 00:00:00 2001 From: Yuqi Du Date: Thu, 9 Nov 2023 09:47:22 -0800 Subject: [PATCH 4/4] clean --- .../operation/model/impl/ExpressionBuilder.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ExpressionBuilder.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ExpressionBuilder.java index 6ab3a49565..30e3bbcf9e 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ExpressionBuilder.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ExpressionBuilder.java @@ -9,6 +9,7 @@ import io.stargate.sgv2.jsonapi.exception.ErrorCode; import io.stargate.sgv2.jsonapi.exception.JsonApiException; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.stream.Collectors; @@ -18,9 +19,7 @@ public static List> buildExpressions( LogicalExpression logicalExpression, DBFilterBase.IDFilter additionalIdFilter) { // an empty filter should find everything if (logicalExpression.isEmpty() && additionalIdFilter == null) { - ArrayList> list = new ArrayList<>(); - list.add(null); - return list; + return Collections.singletonList(null); } // after validate in FilterClauseDeserializer, // partition key column key will not be nested under OR operator @@ -98,7 +97,7 @@ private static Expression buildExpressionRecursive( } boolean hasInFilterThisLevel = false; - boolean InFilterThisLevelWithEmptyArray = true; + boolean inFilterThisLevelWithEmptyArray = true; // second for loop, is to iterate all subComparisonExpression for (ComparisonExpression comparisonExpression : logicalExpression.comparisonExpressions) { for (DBFilterBase dbFilter : comparisonExpression.getDbFilters()) { @@ -106,7 +105,7 @@ private static Expression buildExpressionRecursive( hasInFilterThisLevel = true; List inFilterConditions = inFilter.getAll(); if (!inFilterConditions.isEmpty()) { - InFilterThisLevelWithEmptyArray = false; + inFilterThisLevelWithEmptyArray = false; List> inConditionsVariables = inFilterConditions.stream().map(Variable::of).toList(); conditionExpressions.add(ExpressionUtils.orOf(inConditionsVariables)); @@ -128,7 +127,7 @@ private static Expression buildExpressionRecursive( // when having an empty array $in, if $in occurs within an $and logic, entire $and should match // nothing if (hasInFilterThisLevel - && InFilterThisLevelWithEmptyArray + && inFilterThisLevelWithEmptyArray && logicalExpression.getLogicalRelation().equals(LogicalExpression.LogicalOperator.AND)) { return null; }