Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for 416 and 417 #419

Closed
wants to merge 9 commits into from
Closed

Fix for 416 and 417 #419

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4dd173f
Fix for 416 and 417
maheshrajamani May 9, 2023
4d3d550
Fixed the IT test case to only check if upserted id is not null
maheshrajamani May 9, 2023
25cd323
Review comment from PR #419
maheshrajamani May 9, 2023
c213aa3
Http status code change (#415)
maheshrajamani May 9, 2023
b94e724
Fix for 416 and 417
maheshrajamani May 9, 2023
f175406
Fixed the IT test case to only check if upserted id is not null
maheshrajamani May 9, 2023
9ba577c
Review comment from PR #419
maheshrajamani May 9, 2023
a709c8d
Merge branch 'fix-416-417' of https://github.com/stargate/jsonapi int…
maheshrajamani May 9, 2023
fb5cca8
Fix for 416 and 417
maheshrajamani May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions ...in/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,11 @@ private Uni<UpdatedDocument> processUpdate(
}

// otherwise shred
JsonNode updatedDocument = documentUpdaterResponse.document();
final WritableShreddedDocument writableShreddedDocument =
shredder().shred(updatedDocument, readDocument.txnId());
shredder().shred(documentUpdaterResponse.document(), readDocument.txnId());

// Have to do this because shredder adds _id field to the document if it doesn't exist
JsonNode updatedDocument = writableShreddedDocument.docJsonNode();

// update the document
return updatedDocument(queryExecutor, writableShreddedDocument)
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/stargate/sgv2/jsonapi/service/shredding/Shredder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public WritableShreddedDocument shred(JsonNode doc, UUID txId) {
validateDocument(documentLimits, docWithId, docJson);

final WritableShreddedDocument.Builder b =
WritableShreddedDocument.builder(new DocValueHasher(), docId, txId, docJson);
WritableShreddedDocument.builder(new DocValueHasher(), docId, txId, docJson, docWithId);

// And now let's traverse the document, _including DocumentId so it will also
// be indexed along with other fields.
Expand Down
14 changes: 9 additions & 5 deletions src/main/java/io/stargate/sgv2/jsonapi/service/shredding/model/WritableShreddedDocument.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public record WritableShreddedDocument(
/** Optional transaction id used for optimistic locking */
UUID txID,
String docJson,
JsonNode docJsonNode,
Set<JsonPath> existKeys,
Map<JsonPath, String> subDocEquals,
Map<JsonPath, Integer> arraySize,
Expand All @@ -39,8 +40,9 @@ public record WritableShreddedDocument(
Map<JsonPath, String> queryTextValues,
Map<JsonPath, Date> queryTimestampValues,
Set<JsonPath> queryNullValues) {
public static Builder builder(DocValueHasher hasher, DocumentId id, UUID txID, String docJson) {
return new Builder(hasher, id, txID, docJson);
public static Builder builder(
DocValueHasher hasher, DocumentId id, UUID txID, String docJson, JsonNode docJsonNode) {
return new Builder(hasher, id, txID, docJson, docJsonNode);
}

/**
Expand All @@ -58,7 +60,7 @@ public static class Builder implements ShredListener {
private final UUID txID;

private final String docJson;

private final JsonNode docJsonNode;
private final Set<JsonPath> existKeys;

private Map<JsonPath, String> subDocEquals;
Expand All @@ -73,12 +75,13 @@ public static class Builder implements ShredListener {
private Map<JsonPath, Date> queryTimestampValues;
private Set<JsonPath> queryNullValues;

public Builder(DocValueHasher hasher, DocumentId id, UUID txID, String docJson) {
public Builder(
DocValueHasher hasher, DocumentId id, UUID txID, String docJson, JsonNode docJsonNode) {
this.hasher = hasher;
this.id = id;
this.txID = txID;
this.docJson = Objects.requireNonNull(docJson);

this.docJsonNode = docJsonNode;
existKeys = new LinkedHashSet<>(); // retain document order
}

Expand All @@ -92,6 +95,7 @@ public WritableShreddedDocument build() {
id,
txID,
docJson,
docJsonNode,
existKeys,
_nonNull(subDocEquals),
_nonNull(arraySize),
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdater.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ private boolean replace(ObjectNode docToUpdate, boolean docInserted) {
}
// remove all data and add _id as first field
docToUpdate.removeAll();
docToUpdate.set(DocumentConstants.Fields.DOC_ID, idNode);
if (idNode != null) docToUpdate.set(DocumentConstants.Fields.DOC_ID, idNode);
docToUpdate.setAll(replaceDocument());
// return modified flag as true
return true;
Expand Down
47 changes: 47 additions & 0 deletions src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindOneAndReplaceIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -329,6 +330,52 @@ public void withUpsert() {
.body("data.documents[0]", jsonEquals(expected));
}

@Test
public void withUpsertNoId() {
String json =
"""
{
"findOneAndReplace": {
"filter" : {"username" : "username2"},
"replacement" : {"username": "username2", "status" : true },
"options" : {"returnDocument" : "after", "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("data.document._id", is(notNullValue()))
.body("status.matchedCount", is(0))
.body("status.modifiedCount", is(0))
.body("status.upsertedId", is(notNullValue()))
.body("errors", is(nullValue()));

// assert state after update
json =
"""
{
"find": {
"filter" : {"username" : "username2"}
}
}
""";
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]._id", is(notNullValue()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be also good to check that the document's _id is a string

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

}

@Test
public void byIdWithDifferentId() {
String document =
Expand Down
1 change: 1 addition & 0 deletions src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindOneAndUpdateIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ public void byColumnUpsert() {
.then()
.statusCode(200)
.body("data.document", is(notNullValue()))
.body("data.document._id", is(notNullValue()))
.body("status.upsertedId", is(notNullValue()))
.body("status.matchedCount", is(0))
.body("status.modifiedCount", is(0))
Expand Down