From 4d6a2c8fc67152c0cfc4cf5644151847ea2932a8 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Fri, 19 May 2023 09:56:09 -0700 Subject: [PATCH 1/3] Fix #433: allow _id with $setOnInsert --- .../command/clause/update/SetOperation.java | 12 +++- .../v1/FindOneAndUpdateIntegrationTest.java | 65 +++++++++++++++++++ .../clause/update/SetOperationTest.java | 23 ++++++- 3 files changed, 97 insertions(+), 3 deletions(-) diff --git a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/SetOperation.java b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/SetOperation.java index 93bf7ea684..174b17b141 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/SetOperation.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/SetOperation.java @@ -2,6 +2,7 @@ import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.ObjectNode; +import io.stargate.sgv2.jsonapi.config.constants.DocumentConstants; import io.stargate.sgv2.jsonapi.util.JsonUtil; import io.stargate.sgv2.jsonapi.util.PathMatch; import io.stargate.sgv2.jsonapi.util.PathMatchLocator; @@ -30,7 +31,10 @@ public static SetOperation constructSet(ObjectNode args) { return construct(args, false, UpdateOperator.SET); } - /** Override method used to create an operation that $sets a single property */ + /** + * Override method used to create an operation that $sets a single property (never used for + * $setOnInsert) + */ public static SetOperation constructSet(String filterPath, JsonNode value) { List additions = new ArrayList<>(); String path = validateUpdatePath(UpdateOperator.SET, filterPath); @@ -51,7 +55,11 @@ private static SetOperation construct( var it = args.fields(); while (it.hasNext()) { var entry = it.next(); - String path = validateUpdatePath(operator, entry.getKey()); + // 19-May-2023, tatu: As per [json-api#433] need to allow _id override on $setOnInsert + String path = entry.getKey(); + if (!onlyOnInsert || !DocumentConstants.Fields.DOC_ID.equals(path)) { + path = validateUpdatePath(operator, path); + } additions.add(new Action(PathMatchLocator.forPath(path), entry.getValue())); } return new SetOperation(additions, onlyOnInsert); 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 334983cead..f0b2b75b44 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 @@ -1261,6 +1261,71 @@ public void byIdUpsertAndAddOnInsert() { .statusCode(200) .body("data.documents[0]", jsonEquals(expected)); } + + @Test + public void useGivenDocIdOnInsert() { + String json = + """ + { + "findOneAndUpdate": { + "filter" : {"_id" : "noSuchItem"}, + "update" : { + "$set" : { + "active_user": true, + "extra": 13 + }, + "$setOnInsert" : { + "_id": "setOnInsertDoc2", + "new_user": true + } + }, + "options" : {"returnDocument" : "after", "upsert" : true} + } + } + """; + + // On Insert (for Upsert) should apply both $set and $setOnInsert + String expected = + """ + { + "_id": "setOnInsertDoc2", + "active_user": true, + "new_user": true, + "extra": 13 + } + """; + 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.document", jsonEquals(expected)) + .body("status.upsertedId", is("setOnInsertDoc2")) + .body("status.matchedCount", is(0)) + .body("status.modifiedCount", is(0)) + .body("errors", is(nullValue())); + + // And verify that the document was inserted as expected: + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body( + """ + { + "find": { + "filter" : {"_id" : "setOnInsertDoc2"} + } + } + """) + .when() + .post(CollectionResource.BASE_PATH, namespaceName, collectionName) + .then() + .statusCode(200) + .body("data.documents[0]", jsonEquals(expected)); + } } @Nested diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/command/clause/update/SetOperationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/command/clause/update/SetOperationTest.java index ea5e1da03e..491b399d53 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/command/clause/update/SetOperationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/command/clause/update/SetOperationTest.java @@ -346,7 +346,7 @@ public void testChangeForObjectWithDifferentFieldOrder() { @Nested class InvalidCasesRoot { @Test - public void testNoReplacingDocId() { + public void testNoReplacingDocIdWithSet() { Exception e = catchException( () -> { @@ -363,6 +363,27 @@ public void testNoReplacingDocId() { .hasFieldOrPropertyWithValue("errorCode", ErrorCode.UNSUPPORTED_UPDATE_FOR_DOC_ID) .hasMessage(ErrorCode.UNSUPPORTED_UPDATE_FOR_DOC_ID.getMessage() + ": $set"); } + + // As per [json-api#433]: Ok to override _id for $setOnInsert + @Test + public void testReplaceDocIdWithSetOnInsert() { + UpdateOperation oper = + UpdateOperator.SET_ON_INSERT.resolveOperation( + objectFromJson(""" + { "_id": 1 } + """)); + assertThat(oper).isInstanceOf(SetOperation.class); + // Should indicate document being modified + ObjectNode doc = + objectFromJson(""" + { "_id": 0, "a": 1 } + """); + assertThat(oper.updateDocument(doc)).isTrue(); + assertThat(doc) + .isEqualTo(fromJson(""" + { "_id": 1, "a": 1 } + """)); + } } @Nested From 3b72eebb573ae6c30ebaab3ee0bd676f36ecdffa Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Fri, 19 May 2023 10:43:48 -0700 Subject: [PATCH 2/3] Add IT for updateOne too --- .../api/v1/UpdateOneIntegrationTest.java | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) 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 ec72d6f236..f1a4f55af0 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 @@ -179,6 +179,62 @@ public void byIdUpsert() { .body("data.documents[0]", jsonEquals(expected)); } + @Test + public void byIdUpsertSetOnInsert() { + String json = + """ + { + "updateOne": { + "filter" : {"_id" : "no-such-doc"}, + "update" : { + "$set" : {"active_user": true}, + "$setOnInsert" : {"_id": "upsertSetOnInsert1"} + }, + "options" : {"upsert" : true} + } + } + """; + + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body(json) + .when() + .post(CollectionResource.BASE_PATH, namespaceName, collectionName) + .then() + .statusCode(200) + .body("status.upsertedId", is("upsertSetOnInsert1")) + .body("status.matchedCount", is(0)) + .body("status.modifiedCount", is(0)) + .body("errors", is(nullValue())); + + // assert state after update + json = + """ + { + "find": { + "filter" : {"_id" : "upsertSetOnInsert1"} + } + } + """; + String expected = + """ + { + "_id": "upsertSetOnInsert1", + "active_user": true + } + """; + 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[0]", jsonEquals(expected)); + } + @Test public void byColumnUpsert() { String json = From 0f421fd92c616535597fb021362cb391f61cfbad Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Fri, 19 May 2023 10:53:24 -0700 Subject: [PATCH 3/3] Add test for UpdateMany as well --- .../api/v1/UpdateManyIntegrationTest.java | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) 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 9ce40e446e..aabc1ad725 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 @@ -333,6 +333,63 @@ public void upsert() { .body("data.documents[0]", jsonEquals(expected)); } + @Test + public void upsertWithSetOnInsert() { + insert(2); + String json = + """ + { + "updateMany": { + "filter" : {"_id": "no-such-doc"}, + "update" : { + "$set" : {"active_user": true}, + "$setOnInsert" : {"_id": "docX"} + }, + "options" : {"upsert" : true} + } + } + """; + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body(json) + .when() + .post(CollectionResource.BASE_PATH, namespaceName, collectionName) + .then() + .statusCode(200) + .body("status.upsertedId", is("docX")) + .body("status.matchedCount", is(0)) + .body("status.modifiedCount", is(0)) + .body("status.moreData", nullValue()) + .body("errors", is(nullValue())); + + // assert upsert + String expected = + """ + { + "_id": "docX", + "active_user": true + } + """; + json = + """ + { + "find": { + "filter" : {"_id" : "docX"} + } + } + """; + 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[0]", jsonEquals(expected)); + } + @Test public void byIdNoChange() { insert(2);