diff --git a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/UpdateOperation.java b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/UpdateOperation.java index 1df0da2abd..c34641230e 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/UpdateOperation.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/UpdateOperation.java @@ -20,7 +20,13 @@ protected UpdateOperation(List actions) { this.actions = actions; } - public List actions() { + /** + * @apiNote Doesn't return {@code List}, otherwise an operation like {@code + * actions().map(ActionWithLocator::locator)} may error trying to call a package-private + * implementation (e.g. {@code SetOperation.Action::locator}) + * @return List of actions that this update operation will apply to document + */ + public List actions() { return actions; } diff --git a/src/main/java/io/stargate/sgv2/jsonapi/exception/UpdateException.java b/src/main/java/io/stargate/sgv2/jsonapi/exception/UpdateException.java index b1ead8f10b..7124cef9dd 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/exception/UpdateException.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/exception/UpdateException.java @@ -20,6 +20,7 @@ public enum Code implements ErrorCode { UNKNOWN_TABLE_COLUMNS, UNSUPPORTED_COLUMN_TYPES, UNSUPPORTED_OVERLAPPING_UPDATE_OPERATIONS, + UNSUPPORTED_OVERLAPPING_UPSERT_PATHS, UNSUPPORTED_UPDATE_DATA_TYPE, // from ErrorCodeV1 UNSUPPORTED_UPDATE_FOR_PRIMARY_KEY_COLUMNS, UNSUPPORTED_UPDATE_OPERATIONS_FOR_TABLE, diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperation.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperation.java index 2c74be73cc..5ccb3a3c80 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperation.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperation.java @@ -5,13 +5,17 @@ import com.bpodgursky.jbool_expressions.Expression; import com.datastax.oss.driver.api.core.cql.SimpleStatement; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ObjectNode; import com.google.common.collect.Lists; import io.smallrye.mutiny.Uni; import io.stargate.sgv2.jsonapi.api.model.command.CommandContext; import io.stargate.sgv2.jsonapi.api.model.command.CommandResult; import io.stargate.sgv2.jsonapi.api.model.command.clause.sort.SortExpression; +import io.stargate.sgv2.jsonapi.api.model.command.clause.update.ActionWithLocator; +import io.stargate.sgv2.jsonapi.api.model.command.clause.update.SetOperation; import io.stargate.sgv2.jsonapi.api.request.RequestContext; import io.stargate.sgv2.jsonapi.exception.SchemaException; +import io.stargate.sgv2.jsonapi.exception.UpdateException; import io.stargate.sgv2.jsonapi.service.cql.builder.Query; import io.stargate.sgv2.jsonapi.service.cql.builder.QueryBuilder; import io.stargate.sgv2.jsonapi.service.cqldriver.executor.QueryExecutor; @@ -19,13 +23,13 @@ import io.stargate.sgv2.jsonapi.service.operation.builder.BuiltCondition; import io.stargate.sgv2.jsonapi.service.operation.filters.collection.CollectionFilter; import io.stargate.sgv2.jsonapi.service.operation.filters.collection.IDCollectionFilter; -import io.stargate.sgv2.jsonapi.service.operation.query.DBFilterBase; import io.stargate.sgv2.jsonapi.service.operation.query.DBLogicalExpression; import io.stargate.sgv2.jsonapi.service.projection.DocumentProjector; import io.stargate.sgv2.jsonapi.service.schema.collections.CollectionSchemaObject; import io.stargate.sgv2.jsonapi.service.schema.collections.spec.SuperShreddingMetadata; -import io.stargate.sgv2.jsonapi.service.shredding.collections.DocumentId; +import io.stargate.sgv2.jsonapi.util.PathMatchLocator; import java.util.*; +import java.util.function.Predicate; import java.util.function.Supplier; /** Operation that returns the documents or its key based on the filter condition. */ @@ -454,41 +458,102 @@ public Uni getDocuments( } /** - * An operation method which can return ReadDocument with an empty document, if the filter - * condition has _id filter it will return document with this field added. + * Creates a new empty document with no fields. Intended as a placeholder for when the find + * operation does not return any documents, but the caller still needs a document to work with + * (e.g. an upsert). + * + * @return A new empty document with no fields. */ - public ReadDocument getNewDocument() { + public ReadDocument newEmptyDocument() { + return ReadDocument.from(null, null, objectMapper().createObjectNode()); + } + /** + * Reconstructs a document from the filter used in the find operation, intended for upserts. + * + *

Contradictory filters (e.g. {@code {"a": 1, "a": 2}}) will cause an error to be thrown. + * + * @param pathFilter A predicate to filter which paths should be included in the reconstructed + * document. Contradictory paths not matched in the document won't cause an error. + * @return The reconstructed document as an {@code ObjectNode}. + */ + public ObjectNode reconstructDocumentFromFilter(Predicate pathFilter) { final var rootNode = objectMapper().createObjectNode(); - DocumentId documentId = null; + final var paths = new ArrayList(); + final var ops = new ArrayList(); + final var stack = new Stack(); stack.push(dbLogicalExpression); while (!stack.empty()) { var currentDbLogicalExpression = stack.pop(); - for (DBFilterBase filter : dbLogicalExpression.filters()) { - // every filter must be a collection filter, because we are making a new document, - // and we only do this for docs - if (filter instanceof IDCollectionFilter idFilter) { - documentId = idFilter.getSingularDocumentId(); - idFilter - .updateForNewDocument(objectMapper().getNodeFactory()) - .ifPresent(setOperation -> setOperation.updateDocument(rootNode)); - } else if (filter instanceof CollectionFilter collectionFilter) { - collectionFilter - .updateForNewDocument(objectMapper().getNodeFactory()) - .ifPresent(setOperation -> setOperation.updateDocument(rootNode)); - } else { - throw new IllegalArgumentException( - "Unsupported filter type in getNewDocument: %s" - .formatted(filter.getClass().getName())); + if (currentDbLogicalExpression.operator() != DBLogicalExpression.DBLogicalOperator.AND) { + continue; + } + + for (var filter : currentDbLogicalExpression.filters()) { + switch (filter) { + case CollectionFilter cf -> + cf.updateForNewDocument(objectMapper().getNodeFactory()) + .ifPresent( + op -> { + var beforeSize = paths.size(); + + op.actions().stream() + .map(ActionWithLocator::locator) + .filter(l -> pathFilter.test(l.path())) + .forEach(paths::add); + + if (paths.size() > beforeSize) { + ops.add(op); + } + }); + default -> + throw new IllegalArgumentException( + "Unsupported filter type in getNewDocument: %s" + .formatted(filter.getClass().getName())); } } currentDbLogicalExpression.subExpressions().forEach(stack::push); } - return ReadDocument.from(documentId, null, rootNode); + + validateUpsertPaths(paths); + ops.forEach(op -> op.updateDocument(rootNode)); + + return rootNode; + } + + /** + * Validates that the given paths do not overlap or contradict each other. + * + *

Throws an {@code UNSUPPORTED_OVERLAPPING_UPSERT_PATHS} on error. + * + * @param paths The list of paths to validate. + */ + private void validateUpsertPaths(List paths) { + Collections.sort(paths); + + for (var i = 0; i < paths.size() - 1; i++) { + final var current = paths.get(i); + final var next = paths.get(i + 1); + + if (current.compareTo(next) == 0) { + throwUnsupportedOverlappingUpsertPaths( + "Path '%s' is matched more than once".formatted(current)); + } + + if (next.isSubPathOf(current)) { + throwUnsupportedOverlappingUpsertPaths( + "Both paths '%s' and '%s' are matched".formatted(current, next)); + } + } + } + + private void throwUnsupportedOverlappingUpsertPaths(String message) { + throw UpdateException.Code.UNSUPPORTED_OVERLAPPING_UPSERT_PATHS.get( + errVars(commandContext.schemaObject(), map -> map.put("errorMessage", message))); } /** diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/collections/ReadAndUpdateCollectionOperation.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/collections/ReadAndUpdateCollectionOperation.java index 9cb22c9469..dc7bae66b4 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/collections/ReadAndUpdateCollectionOperation.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/collections/ReadAndUpdateCollectionOperation.java @@ -77,14 +77,7 @@ public Uni> execute( pageStateReference.set(findResponse.pageState()); final List docs = findResponse.docs(); if (upsert() && docs.isEmpty() && matchedCount.get() == 0) { - // TODO: creating the new document here, with the defaults from the filter, makes it - // harder because the new document created here may nto have an _id if there was - // none in the filter. A better approach may be to have the documentUpdater create - // the upsert document totally in once place. - // Currently creating to upsert document is in multiple places. To do this we would - // create UpdateOperations from the filter and give them to the document updated - // when it is created. - return Multi.createFrom().item(findCollectionOperation().getNewDocument()); + return Multi.createFrom().item(findCollectionOperation().newEmptyDocument()); } else { matchedCount.addAndGet(docs.size()); return Multi.createFrom().items(docs.stream()); @@ -182,7 +175,11 @@ private Uni processUpdate( JsonNode originalDocument = upsert ? null : readDocument.get(); DocumentUpdater.DocumentUpdaterResponse documentUpdaterResponse = - documentUpdater().apply(readDocument.get().deepCopy(), upsert); + documentUpdater() + .apply( + readDocument.get().deepCopy(), + upsert, + findCollectionOperation()::reconstructDocumentFromFilter); return documentUpdaterResponse .updateEmbeddingVector( diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/ArrayEqualsCollectionFilter.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/ArrayEqualsCollectionFilter.java index 0b152c7205..5ff1b9637c 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/ArrayEqualsCollectionFilter.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/ArrayEqualsCollectionFilter.java @@ -19,7 +19,10 @@ public ArrayEqualsCollectionFilter( @Override protected Optional jsonNodeForNewDocument(JsonNodeFactory nodeFactory) { - return Optional.of(toJsonNode(nodeFactory, arrayValue)); + if (Operator.MAP_EQUALS.equals(operator)) { + return Optional.of(toJsonNode(nodeFactory, arrayValue)); + } + return Optional.empty(); } // @Override diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/IsNullCollectionFilter.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/IsNullCollectionFilter.java index ab2850eee0..c36628ceee 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/IsNullCollectionFilter.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/IsNullCollectionFilter.java @@ -17,7 +17,10 @@ public IsNullCollectionFilter(String path, Operator operator) { @Override protected Optional jsonNodeForNewDocument(JsonNodeFactory nodeFactory) { - return Optional.of(toJsonNode(nodeFactory)); + if (Operator.CONTAINS.equals(operator)) { + return Optional.of(toJsonNode(nodeFactory)); + } + return Optional.empty(); } // @Override diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/MatchCollectionFilter.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/MatchCollectionFilter.java index 64a007cf42..576f9ede27 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/MatchCollectionFilter.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/MatchCollectionFilter.java @@ -29,7 +29,7 @@ public BuiltCondition get() { } protected Optional jsonNodeForNewDocument(JsonNodeFactory nodeFactory) { - return Optional.of(toJsonNode(nodeFactory, value)); + return Optional.empty(); } @Override diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/SubDocEqualsCollectionFilter.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/SubDocEqualsCollectionFilter.java index c197aeaae2..cfb0c46228 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/SubDocEqualsCollectionFilter.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/filters/collection/SubDocEqualsCollectionFilter.java @@ -28,7 +28,10 @@ public SubDocEqualsCollectionFilter( */ @Override protected Optional jsonNodeForNewDocument(JsonNodeFactory nodeFactory) { - return Optional.of(toJsonNode(nodeFactory, subDocValue)); + if (Operator.MAP_EQUALS.equals(operator)) { + return Optional.of(toJsonNode(nodeFactory, subDocValue)); + } + return Optional.empty(); } // @Override diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdater.java b/src/main/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdater.java index de60019225..b31079f6fd 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdater.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdater.java @@ -17,6 +17,8 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.function.Function; +import java.util.function.Predicate; /** Updates the document read from the database with the updates came as part of the request. */ public record DocumentUpdater( @@ -45,55 +47,69 @@ public static DocumentUpdater construct(ObjectNode replaceDocument) { return new DocumentUpdater(null, replaceDocument, replaceDocumentId, UpdateType.REPLACE); } + /** + * Some function which creates a document given a predicate to filter the fields to include. + * + *

Intended to be satisfied by {@link + * io.stargate.sgv2.jsonapi.service.operation.collections.FindCollectionOperation#reconstructDocumentFromFilter} + */ + @FunctionalInterface + public interface DocumentReconstructor extends Function, ObjectNode> {} + /** * This method is the entrance for first level update or replace. First level means it won't * vectorize if needed, but will warp an EmbeddingUpdateOperation in the DocumentUpdaterResponse * to do the following embedding update. * * @param readDocument Document to update - * @param docInserted True if document was just created (inserted); false if updating existing - * document + * @param isNew True if document was just created (inserted); false if updating existing document + * @param reconstructor A function which creates a reconstructed document from the search filter */ - public DocumentUpdaterResponse apply(JsonNode readDocument, boolean docInserted) { - ObjectNode docToUpdate = (ObjectNode) readDocument; + public DocumentUpdaterResponse apply( + JsonNode readDocument, boolean isNew, DocumentReconstructor reconstructor) { + final var docToUpdate = (ObjectNode) readDocument; + if (UpdateType.UPDATE == updateType) { - return update(docToUpdate, docInserted); + return update(docToUpdate, isNew, reconstructor); } else { - return replace(docToUpdate, docInserted); + return replace(docToUpdate, isNew, reconstructor); } } /** * Will be used for update commands. This is first level replace. This method will replace the * document, but won't re-vectorize yet(detail in updateEmbeddingVector method) - * - * @param docToUpdate - * @param docInserted - * @return */ - private DocumentUpdaterResponse update(ObjectNode docToUpdate, boolean docInserted) { - boolean modified = false; + private DocumentUpdaterResponse update( + ObjectNode docToUpdate, boolean isNew, DocumentReconstructor reconstructor) { + if (isNew) { + docToUpdate = reconstructor.apply(path -> true); + } + + boolean modified = isNew; List embeddingUpdateOperationList = new ArrayList<>(); - for (UpdateOperation updateOperation : updateOperations) { - if (updateOperation.shouldApplyIf(docInserted)) { - final UpdateOperation.UpdateOperationResult updateOperationResult = - updateOperation.updateDocument(docToUpdate); + + for (var updateOperation : updateOperations) { + if (updateOperation.shouldApplyIf(isNew)) { + final var updateOperationResult = updateOperation.updateDocument(docToUpdate); modified |= updateOperationResult.modified(); embeddingUpdateOperationList.addAll(updateOperationResult.embeddingUpdateOperations()); } } + return new DocumentUpdaterResponse(docToUpdate, modified, embeddingUpdateOperationList); } /** * Will be used for findOneAndReplace. This is first level replace. This method will replace the * document, but won't re-vectorize yet(detail in updateEmbeddingVector method) - * - * @param docToUpdate - * @param docInserted - * @return */ - private DocumentUpdaterResponse replace(ObjectNode docToUpdate, boolean docInserted) { + private DocumentUpdaterResponse replace( + ObjectNode docToUpdate, boolean isNew, DocumentReconstructor reconstructor) { + if (isNew) { + docToUpdate = reconstructor.apply(DocumentConstants.Fields.DOC_ID::equals); + } + // Do deep clone so we can remove _id field and check ObjectNode compareDoc = docToUpdate.deepCopy(); @@ -102,6 +118,10 @@ private DocumentUpdaterResponse replace(ObjectNode docToUpdate, boolean docInser // from the replacement document to use later, future work needed to so we can // reliably go back and forth between JsonNode and DocumentId without losing the benefits of // both. + // + // addendum (toptobes) - instead of removing the field from the doc we can just overwrite the + // _id in the replaceDocument (if idNode != null) as they're proven to be already equivalent + // (i.e. no compareDoc, no deep clone, and use .get() instead of .remove()) JsonNode idNode = compareDoc.remove(DocumentConstants.Fields.DOC_ID); // The replace document cannot specify an _id value that differs from the replaced document. diff --git a/src/main/resources/errors.yaml b/src/main/resources/errors.yaml index 38906b8fe8..709da192f3 100644 --- a/src/main/resources/errors.yaml +++ b/src/main/resources/errors.yaml @@ -925,6 +925,16 @@ request-errors: Resend the command using a single update operation for each column. + - scope: UPDATE + code: UNSUPPORTED_OVERLAPPING_UPSERT_PATHS + title: Paths cannot be inferred for upsert due to overlap + body: |- + The command included overlapping paths, making it impossible to infer the document structure for the upsert. + + ${errorMessage} + + Resend the command ensuring paths are not contradictory or overlapping. + # Note UNSUPPORTED_VECTORIZE_WHEN_MISSING_VECTORIZE_DEFINITION is a duplicate for Document scope, this one is used for Update scope. - scope: UPDATE code: UNSUPPORTED_VECTORIZE_WHEN_MISSING_VECTORIZE_DEFINITION diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperationTest.java index 18dde25f11..b264ea0b96 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperationTest.java @@ -2,6 +2,7 @@ import static io.restassured.RestAssured.given; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; @@ -29,6 +30,7 @@ import io.stargate.sgv2.jsonapi.api.model.command.CommandStatus; import io.stargate.sgv2.jsonapi.config.constants.DocumentConstants; import io.stargate.sgv2.jsonapi.exception.DatabaseException; +import io.stargate.sgv2.jsonapi.exception.UpdateException; import io.stargate.sgv2.jsonapi.service.cqldriver.executor.QueryExecutor; import io.stargate.sgv2.jsonapi.service.cqldriver.executor.VectorColumnDefinition; import io.stargate.sgv2.jsonapi.service.cqldriver.executor.VectorConfig; @@ -2994,6 +2996,175 @@ public void readFailureException() throws UnknownHostException { } } + @Nested + class BuildBaseDocument { + @Test + public void buildsFullDocumentWhenPredicateIsTrue() throws Exception { + var andExpr = new DBLogicalExpression(DBLogicalExpression.DBLogicalOperator.AND); + + andExpr.addFilter( + new IDCollectionFilter(IDCollectionFilter.Operator.EQ, DocumentId.fromString("user-1"))); + andExpr.addFilter( + new TextCollectionFilter("address.city", MapCollectionFilter.Operator.EQ, "NYC")); + andExpr.addFilter( + new TextCollectionFilter("address.post.code", MapCollectionFilter.Operator.EQ, "BTSME")); + andExpr.addFilter( + new BoolCollectionFilter("is_alive", MapCollectionFilter.Operator.EQ, true)); + + var operation = mkUnsortedOperation(andExpr); + var result = operation.reconstructDocumentFromFilter(path -> true); + + var expected = + """ + { + "_id": "user-1", + "address": { + "city": "NYC", + "post": { + "code": "BTSME" + } + }, + "is_alive": true + } + """; + assertThat(result).isEqualTo(objectMapper.readTree(expected)); + } + + @Test + public void filtersPathsBasedOnPredicate() throws Exception { + var andExpr = new DBLogicalExpression(DBLogicalExpression.DBLogicalOperator.AND); + + andExpr.addFilter( + new IDCollectionFilter(IDCollectionFilter.Operator.EQ, DocumentId.fromString("user-1"))); + andExpr.addFilter( + new TextCollectionFilter("address.city", MapCollectionFilter.Operator.EQ, "London")); + andExpr.addFilter( + new TextCollectionFilter("preferences.email", MapCollectionFilter.Operator.EQ, "true")); + + var operation = mkUnsortedOperation(andExpr); + var result = + operation.reconstructDocumentFromFilter( + path -> path.equals("_id") || path.startsWith("preferences")); + + var expected = + """ + { + "_id": "user-1", + "preferences": { + "email": "true" + } + } + """; + assertThat(result).isEqualTo(objectMapper.readTree(expected)); + } + + @Test + public void throwsExceptionOnOverlappingPaths() { + var andExpr = new DBLogicalExpression(DBLogicalExpression.DBLogicalOperator.AND); + + andExpr.addFilter( + new IDCollectionFilter(IDCollectionFilter.Operator.EQ, DocumentId.fromString("1"))); + andExpr.addFilter( + new TextCollectionFilter("conf.net", MapCollectionFilter.Operator.EQ, "wifi")); + andExpr.addFilter( + new TextCollectionFilter("conf.net.ip", MapCollectionFilter.Operator.EQ, "10.0.0.1")); + + var operation = mkUnsortedOperation(andExpr); + + assertThatThrownBy(() -> operation.reconstructDocumentFromFilter(path -> true)) + .isInstanceOf(UpdateException.class) + .hasMessageContaining("Both paths 'conf.net' and 'conf.net.ip' are matched"); + } + + @Test + public void bypassesExceptionIfOverlappingPathsFilteredOut() throws Exception { + var andExpr = new DBLogicalExpression(DBLogicalExpression.DBLogicalOperator.AND); + + andExpr.addFilter( + new IDCollectionFilter(IDCollectionFilter.Operator.EQ, DocumentId.fromString("1"))); + andExpr.addFilter( + new TextCollectionFilter("conf.net", MapCollectionFilter.Operator.EQ, "wifi")); + andExpr.addFilter( + new TextCollectionFilter("conf.net.ip", MapCollectionFilter.Operator.EQ, "10.0.0.1")); + + var operation = mkUnsortedOperation(andExpr); + var result = operation.reconstructDocumentFromFilter(path -> path.equals("_id")); + + var expected = + """ + { + "_id": "1" + } + """; + assertThat(result).isEqualTo(objectMapper.readTree(expected)); + } + + @Test + public void handlesArbitrarilyNestedAndExpressions() { + var depth = 50; + var top = new DBLogicalExpression(DBLogicalExpression.DBLogicalOperator.AND); + top.addFilter( + new IDCollectionFilter(IDCollectionFilter.Operator.EQ, DocumentId.fromString("user-1"))); + + var current = top; + for (int i = 1; i <= depth; i++) { + var nested = new DBLogicalExpression(DBLogicalExpression.DBLogicalOperator.AND); + nested.addFilter( + new TextCollectionFilter("field" + i, MapCollectionFilter.Operator.EQ, "value" + i)); + current.addSubExpressionReturnSub(nested); + current = nested; + } + + var operation = mkUnsortedOperation(top); + var result = operation.reconstructDocumentFromFilter(path -> true); + + var expected = objectMapper.createObjectNode(); + expected.put("_id", "user-1"); + for (int i = 1; i <= depth; i++) { + expected.put("field" + i, "value" + i); + } + assertThat(result).isEqualTo(expected); + } + + @Test + public void ignoresFiltersUnderOrSubExpressions() throws Exception { + var top = new DBLogicalExpression(DBLogicalExpression.DBLogicalOperator.AND); + top.addFilter( + new IDCollectionFilter(IDCollectionFilter.Operator.EQ, DocumentId.fromString("user-1"))); + + var orExpr = new DBLogicalExpression(DBLogicalExpression.DBLogicalOperator.OR); + orExpr.addFilter( + new TextCollectionFilter("address.city", MapCollectionFilter.Operator.EQ, "NYC")); + orExpr.addFilter( + new TextCollectionFilter("address.city", MapCollectionFilter.Operator.EQ, "LA")); + top.addSubExpressionReturnSub(orExpr); + + var operation = mkUnsortedOperation(top); + var result = operation.reconstructDocumentFromFilter(path -> true); + + var expected = + """ + { + "_id": "user-1" + } + """; + assertThat(result).isEqualTo(objectMapper.readTree(expected)); + } + + private FindCollectionOperation mkUnsortedOperation(DBLogicalExpression expression) { + return FindCollectionOperation.unsorted( + COMMAND_CONTEXT, + expression, + DocumentProjector.defaultProjector(), + null, + 20, + 20, + CollectionReadType.DOCUMENT, + objectMapper, + false); + } + } + MockRow resultRow(int index, String key, UUID txId, String doc) { return new MockRow( KEY_TXID_JSON_COLUMNS, diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdaterTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdaterTest.java index 97401a1bab..46a87065f4 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdaterTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdaterTest.java @@ -67,7 +67,7 @@ public void setUpdateCondition() throws Exception { UpdateOperator.SET, objectMapper.getNodeFactory().objectNode().put("location", "New York"))); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -96,7 +96,7 @@ public void setUpdateNewData() throws Exception { UpdateOperator.SET, objectMapper.getNodeFactory().objectNode().put("new_data", "data"))); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -125,7 +125,7 @@ public void setUpdateVector() throws Exception { UpdateOperator.SET, objectMapper.getNodeFactory().objectNode().put("new_data", "data"))); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -157,7 +157,7 @@ public void setVectorData() throws Exception { DocumentUpdaterUtils.updateClause( UpdateOperator.SET, (ObjectNode) objectMapper.readTree(vectorData))); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -177,7 +177,7 @@ public void unsetVectorData() throws Exception { UpdateOperator.UNSET, objectMapper.getNodeFactory().objectNode().put("$vector", ""))); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -206,7 +206,7 @@ public void setUpdateNumberData() throws Exception { UpdateOperator.SET, objectMapper.getNodeFactory().objectNode().put("new_data", 40))); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -234,7 +234,7 @@ public void unsetUpdateData() throws Exception { DocumentUpdaterUtils.updateClause( UpdateOperator.UNSET, objectMapper.getNodeFactory().objectNode().put("col", 1))); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -430,7 +430,7 @@ public void replaceDocument() throws Exception { } """)); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -465,7 +465,7 @@ public void replaceDocumentSameId() throws Exception { } """)); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -493,7 +493,7 @@ public void replaceWithDifferentId() throws Exception { catchThrowable( () -> { DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); }); assertThat(t) .isNotNull() @@ -518,7 +518,7 @@ public void replaceEmpty() throws Exception { DocumentUpdater documentUpdater = DocumentUpdater.construct((ObjectNode) objectMapper.readTree("{ }")); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -555,7 +555,7 @@ public void updateVectorize() throws Exception { JsonNode baseData = objectMapper.readTree(BASE_DOC_JSON); // location as London JsonNode expectedData1 = objectMapper.readTree(expected1); DocumentUpdater.DocumentUpdaterResponse firstResponse = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(firstResponse) .isNotNull() .satisfies( @@ -626,7 +626,7 @@ public void update_noVectorize() throws Exception { JsonNode baseData = objectMapper.readTree(BASE_DOC_JSON); // location as London JsonNode expectedData1 = objectMapper.readTree(expected1); DocumentUpdater.DocumentUpdaterResponse firstResponse = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(firstResponse) .isNotNull() .satisfies( @@ -660,7 +660,7 @@ public void update_notModified() throws Exception { JsonNode baseData = objectMapper.readTree(BASE_DOC_JSON); // location as London JsonNode expectedData1 = objectMapper.readTree(expected1); DocumentUpdater.DocumentUpdaterResponse firstResponse = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(firstResponse) .isNotNull() .satisfies( @@ -696,7 +696,7 @@ public void update_notModifiedVectorize() throws Exception { JsonNode baseData = objectMapper.readTree(BASE_DOC_JSON_VECTOR); // location as London JsonNode expectedData1 = objectMapper.readTree(expected1); DocumentUpdater.DocumentUpdaterResponse firstResponse = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(firstResponse) .isNotNull() .satisfies( @@ -732,7 +732,7 @@ public void update_modifiedVector() throws Exception { JsonNode baseData = objectMapper.readTree(BASE_DOC_JSON_VECTOR); // location as London JsonNode expectedData1 = objectMapper.readTree(expected1); DocumentUpdater.DocumentUpdaterResponse firstResponse = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(firstResponse) .isNotNull() .satisfies( @@ -769,7 +769,7 @@ public void update_vectorizeOverwriteVector() throws Exception { JsonNode baseData = objectMapper.readTree(BASE_DOC_JSON_VECTOR); // location as London JsonNode expectedData1 = objectMapper.readTree(expected1); DocumentUpdater.DocumentUpdaterResponse firstResponse = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(firstResponse) .isNotNull() .satisfies( @@ -839,7 +839,7 @@ public void update_vectorizeBlank() throws JsonProcessingException { JsonNode baseData = objectMapper.readTree(BASE_DOC_JSON_VECTOR); // location as London JsonNode expectedData1 = objectMapper.readTree(expected1); DocumentUpdater.DocumentUpdaterResponse firstResponse = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(firstResponse) .isNotNull() .satisfies( @@ -876,7 +876,7 @@ public void update_vectorizeNullValue() throws JsonProcessingException { JsonNode baseData = objectMapper.readTree(BASE_DOC_JSON_VECTOR); // location as London JsonNode expectedData1 = objectMapper.readTree(expected1); DocumentUpdater.DocumentUpdaterResponse firstResponse = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(firstResponse) .isNotNull() .satisfies( @@ -905,7 +905,7 @@ public void update_vectorizeNonTextualFailure() throws JsonProcessingException { catchThrowable( () -> { DocumentUpdater.DocumentUpdaterResponse firstResponse = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); }); assertThat(failure) .isInstanceOf(DocumentException.class) @@ -940,7 +940,7 @@ public void replaceDocument() throws Exception { } """)); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -1008,7 +1008,7 @@ public void replaceDocument_only_replace_vector() throws Exception { } """)); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -1042,7 +1042,7 @@ public void replaceDocument_vectorizeBlankTest() throws Exception { } """)); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -1079,7 +1079,7 @@ public void replaceDocument_vectorizeNonTextFailure() throws Exception { catchThrowable( () -> { DocumentUpdater.DocumentUpdaterResponse firstResponse = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); }); assertThat(failure) .isInstanceOf(DocumentException.class) @@ -1111,7 +1111,7 @@ public void replaceDocument_vectorizeNullValue() throws Exception { } """)); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -1154,7 +1154,7 @@ public void replaceDocument_allNull() throws Exception { } """)); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies( @@ -1189,7 +1189,7 @@ public void replaceDocument_willVectorizeEvenVectorizeHasNoDiff() throws Excepti } """)); DocumentUpdater.DocumentUpdaterResponse updatedDocument = - documentUpdater.apply(baseData, false); + documentUpdater.apply(baseData, false, null); assertThat(updatedDocument) .isNotNull() .satisfies(