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

UpdateOneCommand implementation. #50

Merged
merged 4 commits into from
Jan 27, 2023
Merged

UpdateOneCommand implementation. #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe67c4d
UpdateOneCommand implementation.
maheshrajamani Jan 26, 2023
6ed3f6c
Added resolver test
maheshrajamani Jan 26, 2023
e9229cc
IT cases fix
maheshrajamani Jan 26, 2023
8f3e79d
IT cases fix
maheshrajamani Jan 26, 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
24 changes: 24 additions & 0 deletions src/main/java/io/stargate/sgv3/docsapi/StargateDocsV3Api.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,18 @@
}
}
"""),
@ExampleObject(
name = "updateOne",
summary = "`updateOne` command",
value =
"""
{
"updateOne": {
"filter": {"location": "London"},
"update" : {"$set" : {"location" : "New York"}}
}
}
"""),
@ExampleObject(
name = "deleteOne",
summary = "`deleteOne` command",
Expand Down Expand Up @@ -206,6 +218,18 @@
}
}
"""),
@ExampleObject(
name = "resultUpdateOne",
summary = "`updateOne` command result",
value =
"""
{
"status": {
Copy link
Contributor

@tatu-at-datastax tatu-at-datastax Jan 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This seems different from what Mongo returns as per https://www.mongodb.com/docs/manual/reference/method/db.collection.updateOne/, which says

The method returns a document that contains:

    'matchedCount' containing the number of matched documents
    'modifiedCount' containing the number of modified documents
    'upsertedId' containing the _id for the upserted document.

    A 'boolean' acknowledged as true if the operation ran with write concern or false if write concern was disabled

we can talk about this tomorrow, change in follow-up PR if needs changing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. The distinction between matchedCount and modifiedCount is somewhat important as well depending on how you've implemented updatedIds.

matchedCount is the number of documents that matched the filter. 0 or 1 for updateOne. And modifiedCount is the number of documents that changed.

You may have a case where matchedCount = 1 but modifiedCount = 0 if either (1) no properties in the document changed, for example $set: { answer: 42 } when answer is already 42, or (2) update operators didn't apply, for example $setOnInsert: { answer: 42 } if no upsert occurred.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point on details wrt modifiedCount. This needs to be considered on server side, so that update operations need to indicate if any changes were made.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd also advise against using updatedIds because that might mean we need to have separate return values for updateOne() and updateMany(). updateMany() may update millions of documents - sending all those ids over the network may be extremely slow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vkarpov15 Are you suggesting not to return updateIds for updateOne() also?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. For the sake of consistency between updateOne() and updateMany().

"updatedIds": ["1"]
}
}
}
"""),
@ExampleObject(
name = "resultInsert",
summary = "Insert command result",
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/io/stargate/sgv3/docsapi/api/model/command/Command.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import io.stargate.sgv3.docsapi.api.model.command.impl.FindOneCommand;
import io.stargate.sgv3.docsapi.api.model.command.impl.InsertManyCommand;
import io.stargate.sgv3.docsapi.api.model.command.impl.InsertOneCommand;
import io.stargate.sgv3.docsapi.api.model.command.impl.UpdateOneCommand;

/**
* POJO object (data no behavior) that represents a syntactically and grammatically valid command as
Expand Down Expand Up @@ -39,5 +40,6 @@
@JsonSubTypes.Type(value = FindOneAndUpdateCommand.class),
@JsonSubTypes.Type(value = InsertOneCommand.class),
@JsonSubTypes.Type(value = InsertManyCommand.class),
@JsonSubTypes.Type(value = UpdateOneCommand.class),
})
public interface Command {}
3 changes: 2 additions & 1 deletion ...va/io/stargate/sgv3/docsapi/api/model/command/deserializers/UpdateClauseDeserializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ private void getOperations(
while (fields.hasNext()) {
Map.Entry<String, JsonNode> updateField = fields.next();
JsonNode value = updateField.getValue();
if (!value.isValueNode()) throw new DocsException(ErrorCode.UNSUPPORTED_UPDATE_DATA_TYPE);
expressionList.add(
new UpdateOperation(updateField.getKey(), operator, jsonNodeValue(value)));
}
Expand All @@ -81,6 +80,8 @@ private static JsonNode jsonNodeValue(JsonNode node) {
case NUMBER:
case STRING:
case NULL:
case ARRAY:
case OBJECT:
return node;
default:
throw new DocsException(
Expand Down
19 changes: 19 additions & 0 deletions src/main/java/io/stargate/sgv3/docsapi/api/model/command/impl/UpdateOneCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package io.stargate.sgv3.docsapi.api.model.command.impl;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonTypeName;
import io.stargate.sgv3.docsapi.api.model.command.Filterable;
import io.stargate.sgv3.docsapi.api.model.command.ReadCommand;
import io.stargate.sgv3.docsapi.api.model.command.clause.filter.FilterClause;
import io.stargate.sgv3.docsapi.api.model.command.clause.update.UpdateClause;
import javax.validation.Valid;
import org.eclipse.microprofile.openapi.annotations.media.Schema;

@Schema(
description =
"Command that finds a single JSON document from a collection and updates the value provided in the update clause.")
@JsonTypeName("updateOne")
public record UpdateOneCommand(
@Valid @JsonProperty("filter") FilterClause filterClause,
@Valid @JsonProperty("update") UpdateClause updateClause)
implements ReadCommand, Filterable {}
8 changes: 6 additions & 2 deletions src/main/java/io/stargate/sgv3/docsapi/api/v3/CollectionResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import io.stargate.sgv3.docsapi.api.model.command.impl.FindOneCommand;
import io.stargate.sgv3.docsapi.api.model.command.impl.InsertManyCommand;
import io.stargate.sgv3.docsapi.api.model.command.impl.InsertOneCommand;
import io.stargate.sgv3.docsapi.api.model.command.impl.UpdateOneCommand;
import io.stargate.sgv3.docsapi.config.constants.OpenApiConstants;
import io.stargate.sgv3.docsapi.service.processor.CommandProcessor;
import javax.inject.Inject;
Expand Down Expand Up @@ -70,15 +71,17 @@ public CollectionResource(CommandProcessor commandProcessor) {
FindCommand.class,
FindOneAndUpdateCommand.class,
InsertOneCommand.class,
InsertManyCommand.class
InsertManyCommand.class,
UpdateOneCommand.class
}),
examples = {
@ExampleObject(ref = "deleteOne"),
@ExampleObject(ref = "findOne"),
@ExampleObject(ref = "find"),
@ExampleObject(ref = "findOneAndUpdate"),
@ExampleObject(ref = "insertOne"),
@ExampleObject(ref = "insertMany"),
@ExampleObject(ref = "deleteOne"),
@ExampleObject(ref = "updateOne"),
}))
@APIResponses(
@APIResponse(
Expand All @@ -95,6 +98,7 @@ public CollectionResource(CommandProcessor commandProcessor) {
@ExampleObject(ref = "resultInsert"),
@ExampleObject(ref = "resultError"),
@ExampleObject(ref = "resultDelete"),
@ExampleObject(ref = "resultUpdateOne"),
})))
@POST
public Uni<RestResponse<CommandResult>> postCommand(
Expand Down
44 changes: 44 additions & 0 deletions ...n/java/io/stargate/sgv3/docsapi/service/resolver/model/impl/UpdateOneCommandResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package io.stargate.sgv3.docsapi.service.resolver.model.impl;

import com.fasterxml.jackson.databind.ObjectMapper;
import io.stargate.sgv3.docsapi.api.model.command.CommandContext;
import io.stargate.sgv3.docsapi.api.model.command.impl.UpdateOneCommand;
import io.stargate.sgv3.docsapi.service.operation.model.Operation;
import io.stargate.sgv3.docsapi.service.operation.model.ReadOperation;
import io.stargate.sgv3.docsapi.service.operation.model.impl.ReadAndUpdateOperation;
import io.stargate.sgv3.docsapi.service.resolver.model.CommandResolver;
import io.stargate.sgv3.docsapi.service.resolver.model.impl.matcher.FilterableResolver;
import io.stargate.sgv3.docsapi.service.shredding.Shredder;
import io.stargate.sgv3.docsapi.service.updater.DocumentUpdater;
import javax.enterprise.context.ApplicationScoped;
import javax.inject.Inject;

/** Resolves the {@link UpdateOneCommand } */
@ApplicationScoped
public class UpdateOneCommandResolver extends FilterableResolver<UpdateOneCommand>
implements CommandResolver<UpdateOneCommand> {
private Shredder shredder;

@Inject
public UpdateOneCommandResolver(ObjectMapper objectMapper, Shredder shredder) {
super(objectMapper, true, true);
this.shredder = shredder;
}

@Override
public Class<UpdateOneCommand> getCommandClass() {
return UpdateOneCommand.class;
}

@Override
public Operation resolveCommand(CommandContext ctx, UpdateOneCommand command) {
ReadOperation readOperation = resolve(ctx, command);
DocumentUpdater documentUpdater = new DocumentUpdater(command.updateClause());
return new ReadAndUpdateOperation(ctx, readOperation, documentUpdater, false, shredder);
}

@Override
protected FilteringOptions getFilteringOption(UpdateOneCommand command) {
return new FilteringOptions(1, null, 1);
}
}
29 changes: 23 additions & 6 deletions ...o/stargate/sgv3/docsapi/api/model/command/deserializers/UpdateClauseDeserializerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package io.stargate.sgv3.docsapi.api.model.command.deserializers;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.catchThrowable;

import com.fasterxml.jackson.databind.ObjectMapper;
import io.quarkus.test.junit.QuarkusTest;
Expand All @@ -10,7 +9,6 @@
import io.stargate.sgv3.docsapi.api.model.command.clause.update.UpdateClause;
import io.stargate.sgv3.docsapi.api.model.command.clause.update.UpdateOperation;
import io.stargate.sgv3.docsapi.api.model.command.clause.update.UpdateOperator;
import io.stargate.sgv3.docsapi.exception.DocsException;
import javax.inject.Inject;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -95,13 +93,32 @@ public void mustHandleBoolean() throws Exception {
}

@Test
public void unsupportedFilterTypes() {
public void mustHandleArray() throws Exception {
String json = """
{"$set" : {"boolType": ["a"]}}
{"$set" : {"arrayType": ["a"]}}
""";

Throwable throwable = catchThrowable(() -> objectMapper.readValue(json, UpdateClause.class));
assertThat(throwable).isInstanceOf(DocsException.class);
final UpdateOperation operation =
new UpdateOperation(
"arrayType", UpdateOperator.SET, objectMapper.getNodeFactory().arrayNode(1).add("a"));
UpdateClause updateClause = objectMapper.readValue(json, UpdateClause.class);
assertThat(updateClause.updateOperations()).hasSize(1).contains(operation);
}

@Test
public void mustHandleSubdoc() throws Exception {
String json =
"""
{"$set" : {"subDocType": {"sub_doc_col" : "sub_doc_val"}}}}
""";

final UpdateOperation operation =
new UpdateOperation(
"subDocType",
UpdateOperator.SET,
objectMapper.getNodeFactory().objectNode().put("sub_doc_col", "sub_doc_val"));
UpdateClause updateClause = objectMapper.readValue(json, UpdateClause.class);
assertThat(updateClause.updateOperations()).hasSize(1).contains(operation);
}
}
}
Loading