From 64b7eb65d83ba5a67dbcde46a940d00772be3786 Mon Sep 17 00:00:00 2001 From: maheshrajamani <99678631+maheshrajamani@users.noreply.github.com> Date: Mon, 20 Mar 2023 15:16:25 -0400 Subject: [PATCH] Bug fix for upsert if filter is based on non id filter --- .../model/impl/ReadAndUpdateOperation.java | 8 ++- .../v1/FindOneAndUpdateIntegrationTest.java | 70 ++++++++++++++++--- .../api/v1/UpdateManyIntegrationTest.java | 48 +++++++++++++ .../api/v1/UpdateOneIntegrationTest.java | 48 +++++++++++++ 4 files changed, 160 insertions(+), 14 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 21fa1ccc75..fb1751e8a0 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 @@ -153,10 +153,12 @@ private Uni processUpdate( documentUpdater().applyUpdates(readDocument.document().deepCopy(), isInsert); JsonNode updatedDocument = documentUpdaterResponse.document(); Uni updated = Uni.createFrom().nullItem(); + final WritableShreddedDocument writableShreddedDocument; if (documentUpdaterResponse.modified()) { - WritableShreddedDocument writableShreddedDocument = - shredder().shred(updatedDocument, readDocument.txnId()); + writableShreddedDocument = shredder().shred(updatedDocument, readDocument.txnId()); updated = updatedDocument(queryExecutor, writableShreddedDocument); + } else { + writableShreddedDocument = null; } final JsonNode documentToReturn = returnUpdatedDocument ? updatedDocument : originalDocument; @@ -168,7 +170,7 @@ private Uni processUpdate( v -> { if (readDocument.txnId() != null) modifiedCount.incrementAndGet(); return new UpdatedDocument( - readDocument.id(), + writableShreddedDocument.id(), readDocument.txnId() == null, returnDocumentInResponse ? documentToReturn : null, null); 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 c730a6ed84..fc5b1da476 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 @@ -5,6 +5,7 @@ import static net.javacrumbs.jsonunit.JsonMatchers.jsonEquals; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; import io.quarkus.test.common.QuarkusTestResource; @@ -175,24 +176,71 @@ public void findByIdReturnDocumentAfter() { } @Test - public void findByIdUpsert() { + public void findByColumnUpsert() { String json = """ { "findOneAndUpdate": { - "filter" : {"_id" : "afterDoc4"}, + "filter" : {"location" : "my_city"}, "update" : {"$set" : {"active_user": false}}, "options" : {"returnDocument" : "after", "upsert" : true} } } """; - String expected = + + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body(json) + .when() + .post(CollectionResource.BASE_PATH, keyspaceId.asInternal(), collectionName) + .then() + .statusCode(200) + .body("data.docs[0]", is(notNullValue())) + .body("status.upsertedId", is(notNullValue())) + .body("status.matchedCount", is(0)) + .body("status.modifiedCount", is(0)) + .body("errors", is(nullValue())); + + // assert state after update + json = """ { - "_id":"afterDoc4", - "active_user":false + "find": { + "filter" : {"location" : "my_city"} + } } """; + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body(json) + .when() + .post(CollectionResource.BASE_PATH, keyspaceId.asInternal(), collectionName) + .then() + .statusCode(200) + .body("data.docs[0]", is(notNullValue())); + } + + @Test + public void findByIdUpsert() { + String json = + """ + { + "findOneAndUpdate": { + "filter" : {"_id" : "afterDoc4"}, + "update" : {"$set" : {"active_user": false}}, + "options" : {"returnDocument" : "after", "upsert" : true} + } + } + """; + String expected = + """ + { + "_id":"afterDoc4", + "active_user":false + } + """; given() .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) @@ -211,12 +259,12 @@ public void findByIdUpsert() { // assert state after update json = """ - { - "find": { - "filter" : {"_id" : "afterDoc4"} - } - } - """; + { + "find": { + "filter" : {"_id" : "afterDoc4"} + } + } + """; given() .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) .contentType(ContentType.JSON) diff --git a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/UpdateManyIntegrationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/UpdateManyIntegrationTest.java index f7f9e227ba..a9eef184f6 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/UpdateManyIntegrationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/UpdateManyIntegrationTest.java @@ -5,6 +5,7 @@ import static net.javacrumbs.jsonunit.JsonMatchers.jsonEquals; import static org.hamcrest.Matchers.everyItem; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; import io.quarkus.test.common.QuarkusTestResource; @@ -359,6 +360,53 @@ public void updateManyByIdNoChange() { .body("data.docs[0]", jsonEquals(expected)); } + @Test + public void upsertManyByColumnUpsert() { + String json = + """ + { + "updateMany": { + "filter" : {"location" : "my_city"}, + "update" : {"$set" : {"active_user": false}}, + "options" : {"upsert" : true} + } + } + """; + + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body(json) + .when() + .post(CollectionResource.BASE_PATH, keyspaceId.asInternal(), collectionName) + .then() + .statusCode(200) + .body("status.upsertedId", is(notNullValue())) + .body("status.matchedCount", is(0)) + .body("status.modifiedCount", is(0)) + .body("errors", is(nullValue())); + + // assert state after update + json = + """ + { + "find": { + "filter" : {"location" : "my_city"} + } + } + """; + + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body(json) + .when() + .post(CollectionResource.BASE_PATH, keyspaceId.asInternal(), collectionName) + .then() + .statusCode(200) + .body("data.docs[0]", is(notNullValue())); + } + @Test public void updateManyUpsertAddFilterColumn() { insert(5); diff --git a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/UpdateOneIntegrationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/UpdateOneIntegrationTest.java index bf010de956..a5d8fdd5a1 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/UpdateOneIntegrationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/UpdateOneIntegrationTest.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.is; +import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; import io.quarkus.test.common.QuarkusTestResource; @@ -149,6 +150,53 @@ public void findByIdUpsert() { .body("data.docs[0]", jsonEquals(expected)); } + @Test + public void findByColumnUpsert() { + String json = + """ + { + "updateOne": { + "filter" : {"location" : "my_city"}, + "update" : {"$set" : {"active_user": false}}, + "options" : {"upsert" : true} + } + } + """; + + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body(json) + .when() + .post(CollectionResource.BASE_PATH, keyspaceId.asInternal(), collectionName) + .then() + .statusCode(200) + .body("status.upsertedId", is(notNullValue())) + .body("status.matchedCount", is(0)) + .body("status.modifiedCount", is(0)) + .body("errors", is(nullValue())); + + // assert state after update + json = + """ + { + "find": { + "filter" : {"location" : "my_city"} + } + } + """; + + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body(json) + .when() + .post(CollectionResource.BASE_PATH, keyspaceId.asInternal(), collectionName) + .then() + .statusCode(200) + .body("data.docs[0]", is(notNullValue())); + } + @Test public void findByIdAndColumnUpsert() { String json =