From f6d32f120c6160ca70e047eb905e4140296c3dde Mon Sep 17 00:00:00 2001 From: Mahesh Rajamani <99678631+maheshrajamani@users.noreply.github.com> Date: Mon, 24 Apr 2023 14:56:17 -0400 Subject: [PATCH] `findOneAndUpdate()` and `findOneAndReplace()` to return document when no data change (#392) --- .../model/impl/ReadAndUpdateOperation.java | 13 ++- .../v1/FindOneAndReplaceIntegrationTest.java | 3 +- .../v1/FindOneAndUpdateIntegrationTest.java | 39 +++++++ .../impl/ReadAndUpdateOperationTest.java | 103 ++++++++++++++++++ 4 files changed, 155 insertions(+), 3 deletions(-) diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperation.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperation.java index e9dd939528..048f65c3c1 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperation.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperation.java @@ -162,8 +162,17 @@ private Uni processUpdate( // if no changes return null item DocumentUpdater.DocumentUpdaterResponse documentUpdaterResponse = documentUpdater().apply(readDocument.document().deepCopy(), upsert); - if (!documentUpdaterResponse.modified()) { - return Uni.createFrom().nullItem(); + + // In case no change to document and not an upsert document, short circuit and return + if (!documentUpdaterResponse.modified() && !upsert) { + // If no change return the original document Issue #390 + if (returnDocumentInResponse) { + resultProjection.applyProjection(originalDocument); + return Uni.createFrom() + .item(new UpdatedDocument(readDocument.id(), upsert, originalDocument, null)); + } else { + return Uni.createFrom().nullItem(); + } } // otherwise shred diff --git a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindOneAndReplaceIntegrationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindOneAndReplaceIntegrationTest.java index 3af1ed64fa..eb677175b3 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindOneAndReplaceIntegrationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindOneAndReplaceIntegrationTest.java @@ -176,7 +176,8 @@ public void byIdWithIdNoChange() { .post(CollectionResource.BASE_PATH, namespaceName, collectionName) .then() .statusCode(200) - .body("data.docs", hasSize(0)) + .body("data.docs", hasSize(1)) + .body("data.docs[0]", jsonEquals(document)) .body("status.matchedCount", is(1)) .body("status.modifiedCount", is(0)) .body("errors", is(nullValue())); diff --git a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindOneAndUpdateIntegrationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindOneAndUpdateIntegrationTest.java index de7180113f..170e624b77 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindOneAndUpdateIntegrationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindOneAndUpdateIntegrationTest.java @@ -4,6 +4,7 @@ import static io.stargate.sgv2.common.IntegrationTestUtils.getAuthToken; import static net.javacrumbs.jsonunit.JsonMatchers.jsonEquals; import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; @@ -87,6 +88,44 @@ public void byIdAndSet() { .body("data.docs[0]", jsonEquals(expected)); } + @Test + public void byIdAndSetNoChange() { + String document = + """ + { + "_id": "doc3", + "username": "admin", + "active_user" : true + } + """; + insertDoc(document); + + String json = + """ + { + "findOneAndUpdate": { + "filter" : {"_id" : "doc3"}, + "sort": { "username": 1 }, + "update" : {"$set" : {"username": "admin"}}, + "options": {"returnDocument": "before"} + } + } + """; + 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.docs", hasSize(1)) + .body("data.docs[0]", jsonEquals(document)) + .body("status.matchedCount", is(1)) + .body("status.modifiedCount", is(0)) + .body("errors", is(nullValue())); + } + @Test public void byIdAndSetNotFound() { String json = diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperationTest.java index 6551669a83..f49e942f29 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperationTest.java @@ -218,6 +218,109 @@ public void happyPath() throws Exception { assertThat(result.errors()).isNull(); } + @Test + public void noChange() throws Exception { + String collectionReadCql = + "SELECT key, tx_id, doc_json FROM \"%s\".\"%s\" WHERE key = ? LIMIT 1" + .formatted(KEYSPACE_NAME, COLLECTION_NAME); + + UUID tx_id = UUID.randomUUID(); + String doc1 = + """ + { + "_id": "doc1", + "username": "user1" + } + """; + + ValidatingStargateBridge.QueryAssert selectQueryAssert = + withQuery( + collectionReadCql, + Values.of( + CustomValueSerializers.getDocumentIdValue(DocumentId.fromString("doc1")))) + .withPageSize(1) + .withColumnSpec( + List.of( + QueryOuterClass.ColumnSpec.newBuilder() + .setName("key") + .setType(TypeSpecs.tuple(TypeSpecs.TINYINT, TypeSpecs.VARCHAR)) + .build(), + QueryOuterClass.ColumnSpec.newBuilder() + .setName("tx_id") + .setType(TypeSpecs.UUID) + .build(), + QueryOuterClass.ColumnSpec.newBuilder() + .setName("doc_json") + .setType(TypeSpecs.VARCHAR) + .build())) + .returning( + List.of( + List.of( + Values.of( + CustomValueSerializers.getDocumentIdValue( + DocumentId.fromString("doc1"))), + Values.of(tx_id), + Values.of(doc1)))); + + String collectionUpdateCql = UPDATE.formatted(KEYSPACE_NAME, COLLECTION_NAME); + JsonNode jsonNode = objectMapper.readTree(doc1); + WritableShreddedDocument shredDocument = shredder.shred(jsonNode); + DBFilterBase.IDFilter filter = + new DBFilterBase.IDFilter( + DBFilterBase.IDFilter.Operator.EQ, DocumentId.fromString("doc1")); + FindOperation findOperation = + new FindOperation( + COMMAND_CONTEXT, + List.of(filter), + DocumentProjector.identityProjector(), + null, + 1, + 1, + ReadType.DOCUMENT, + objectMapper, + null, + 0, + 0); + String updateClause = + """ + { "$set" : { "username" : "user1" }} + """; + DocumentUpdater documentUpdater = + DocumentUpdater.construct(objectMapper.readValue(updateClause, UpdateClause.class)); + ReadAndUpdateOperation operation = + new ReadAndUpdateOperation( + COMMAND_CONTEXT, + findOperation, + documentUpdater, + true, + false, + false, + shredder, + DocumentProjector.identityProjector(), + 1, + 3); + + Supplier execute = + operation + .execute(queryExecutor) + .subscribe() + .withSubscriber(UniAssertSubscriber.create()) + .awaitItem() + .getItem(); + + // assert query execution + selectQueryAssert.assertExecuteCount().isOne(); + + // then result + CommandResult result = execute.get(); + assertThat(result.status()) + .hasSize(2) + .containsEntry(CommandStatus.MATCHED_COUNT, 1) + .containsEntry(CommandStatus.MODIFIED_COUNT, 0); + assertThat(result.errors()).isNull(); + assertThat(result.data().docs()).hasSize(1); + } + @Test public void happyPathWithSort() throws Exception { String collectionReadCql =