WIP: Fix #53: Add validation for $set, $unset Update operations not to allow changing _id

src/main/java/io/stargate/sgv3/docsapi/api/model/command/clause/update/SetOperation.java

@@ -0,0 +1,46 @@
+package io.stargate.sgv3.docsapi.api.model.command.clause.update;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+public class SetOperation extends UpdateOperation {
+  private final List<SetAction> additions;
+
+  private SetOperation(List<SetAction> additions) {
+    this.additions = additions;
+  }
+
+  public static SetOperation construct(ObjectNode args) {
+    List<SetAction> additions = new ArrayList<>();
+    var it = args.fields();
+    while (it.hasNext()) {
+      var entry = it.next();
+      String path = validateSetPath(UpdateOperator.SET, entry.getKey());
+      additions.add(new SetAction(path, entry.getValue()));
+    }
+    return new SetOperation(additions);
+  }
+
+  @Override
+  public void updateDocument(ObjectNode doc) {
+    additions.forEach(addition -> doc.set(addition.path, addition.value));
+  }
+
+  public Set<String> paths() {
+    return additions.stream().map(SetAction::path).collect(Collectors.toSet());
+  }
+
+  // Just needed for tests
+  @Override
+  public boolean equals(Object o) {
+    return (o instanceof SetOperation)
+        && Objects.equals(this.additions, ((SetOperation) o).additions);
+  }
+
+  private record SetAction(String path, JsonNode value) {}
+}

src/main/java/io/stargate/sgv3/docsapi/api/model/command/clause/update/UnsetOperation.java

@@ -0,0 +1,41 @@
+package io.stargate.sgv3.docsapi.api.model.command.clause.update;
+
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+
+public class UnsetOperation extends UpdateOperation {
+  private List<String> paths;
+
+  private UnsetOperation(List<String> paths) {
+    this.paths = paths;
+  }
+
+  public static UnsetOperation construct(ObjectNode args) {
+    Iterator<String> it = args.fieldNames();
+    List<String> fieldNames = new ArrayList<>();
+    while (it.hasNext()) {
+      fieldNames.add(validateSetPath(UpdateOperator.UNSET, it.next()));
+    }
+    return new UnsetOperation(fieldNames);
+  }
+
+  @Override
+  public void updateDocument(ObjectNode doc) {
+    paths.stream().forEach(path -> doc.remove(path));
+  }
+
+  public Set<String> paths() {
+    return new HashSet<>(paths);
+  }
+
+  // Just needed for tests
+  @Override
+  public boolean equals(Object o) {
+    return (o instanceof UnsetOperation) && Objects.equals(this.paths, ((UnsetOperation) o).paths);
+  }
+}

src/main/java/io/stargate/sgv3/docsapi/api/model/command/clause/update/UpdateClause.java

@@ -1,16 +1,63 @@
 package io.stargate.sgv3.docsapi.api.model.command.clause.update;
 
 import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.node.ObjectNode;
 import io.stargate.sgv3.docsapi.api.model.command.deserializers.UpdateClauseDeserializer;
+import io.stargate.sgv3.docsapi.exception.DocsException;
+import io.stargate.sgv3.docsapi.exception.ErrorCode;
+import java.util.ArrayList;
+import java.util.EnumMap;
 import java.util.List;
+import java.util.Set;
 import org.eclipse.microprofile.openapi.annotations.enums.SchemaType;
 import org.eclipse.microprofile.openapi.annotations.media.Schema;
 
+/**
+ * Value type read from incoming JSON and used for resolving into actual {@link UpdateOperation}s
+ * with validation and additional context.
+ */
 @JsonDeserialize(using = UpdateClauseDeserializer.class)
 @Schema(
     type = SchemaType.OBJECT,
     implementation = Object.class,
-    example = """
-             {"$set" : {"location": "New York"}
+    example =
+        """
+             {"$set" : {"location": "New York"},
+              "$unset" : {"new_data": 1}
               """)
-public record UpdateClause(List<UpdateOperation> updateOperations) {}
+public record UpdateClause(EnumMap<UpdateOperator, ObjectNode> updateOperationDefs) {
+  /**
+   * Method that will validate update operation definitions of the clause and construct an ordered
+   * set of {@link UpdateOperation}s.
+   *
+   * @return List of update operations to execute
+   */
+  public List<UpdateOperation> buildOperations() {
+    // First, resolve operations individually; this will handle basic validation
+    var operationMap = new EnumMap<UpdateOperator, UpdateOperation>(UpdateOperator.class);
+    updateOperationDefs
+        .entrySet()
+        .forEach(e -> operationMap.put(e.getKey(), e.getKey().resolveOperation(e.getValue())));
+
+    // Then handle cross-operation validation
+
+    // First: verify $set and $unset do NOT have overlapping keys
+
+    SetOperation setOp = (SetOperation) operationMap.get(UpdateOperator.SET);
+    UnsetOperation unsetOp = (UnsetOperation) operationMap.get(UpdateOperator.UNSET);
+
+    if ((setOp != null) && (unsetOp != null)) {
+      Set<String> paths = setOp.paths();
+      paths.retainAll(unsetOp.paths());
+
+      if (!paths.isEmpty()) {
+        throw new DocsException(
+            ErrorCode.UNSUPPORTED_UPDATE_OPERATION_PARAM,
+            "Update operators '$set' and '$unset' must not refer to same path: '%s'"
+                .formatted(paths.iterator().next()));
+      }
+    }
+
+    return new ArrayList<>(operationMap.values());
+  }
+}

src/main/java/io/stargate/sgv3/docsapi/api/model/command/clause/update/UpdateOperation.java

@@ -1,13 +1,28 @@
 package io.stargate.sgv3.docsapi.api.model.command.clause.update;
 
-import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import io.stargate.sgv3.docsapi.config.constants.DocumentConstants;
+import io.stargate.sgv3.docsapi.exception.DocsException;
+import io.stargate.sgv3.docsapi.exception.ErrorCode;
 
 /**
- * UpdateOperation represents a unit of data to be updated. A update clause can have list of *
- * UpdateOperation.
- *
- * @param path
- * @param operator
- * @param value
+ * UpdateOperation represents one of update definitions from {@link UpdateClause} (like {@code $set}
+ * or {@code $unset}): single operation can contain multiple actual changes.
  */
-public record UpdateOperation(String path, UpdateOperator operator, JsonNode value) {}
+public abstract class UpdateOperation {
+  public abstract void updateDocument(ObjectNode doc);
+
+  /**
+   * Shared validation method used by {@code $set} and {@code $unset} operations to ensure they are
+   * not used to modify paths that are not allowed (specifically Document's primary id, {@code
+   * _id}).
+   */
+  protected static String validateSetPath(UpdateOperator oper, String path) {
+    if (DocumentConstants.Fields.DOC_ID.equals(path)) {
+      throw new DocsException(
+          ErrorCode.UNSUPPORTED_UPDATE_FOR_DOC_ID,
+          ErrorCode.UNSUPPORTED_UPDATE_FOR_DOC_ID.getMessage() + ": " + oper.operator());
+    }
+    return path;
+  }
+}

src/main/java/io/stargate/sgv3/docsapi/api/model/command/clause/update/UpdateOperator.java

@@ -1,19 +1,48 @@
 package io.stargate.sgv3.docsapi.api.model.command.clause.update;
 
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import io.stargate.sgv3.docsapi.exception.DocsException;
+import io.stargate.sgv3.docsapi.exception.ErrorCode;
 import java.util.HashMap;
 import java.util.Map;
 
 /** List of update operator that's supported in update. */
 public enum UpdateOperator {
-  SET("$set"),
-  UNSET("$unset");
+  // First operators that are supported
+
+  SET("$set") {
+    @Override
+    public UpdateOperation resolveOperation(ObjectNode arguments) {
+      return SetOperation.construct(arguments);
+    }
+  },
+  UNSET("$unset") {
+    @Override
+    public UpdateOperation resolveOperation(ObjectNode arguments) {
+      return UnsetOperation.construct(arguments);
+    }
+  },
+
+  // Then operators that we recognize but do not (yet) support
+
+  INC("$inc");
 
   private String operator;
 
   UpdateOperator(String operator) {
     this.operator = operator;
   }
 
+  public String operator() {
+    return operator;
+  }
+
+  public UpdateOperation resolveOperation(ObjectNode arguments) {
+    throw new DocsException(
+        ErrorCode.UNSUPPORTED_UPDATE_OPERATION,
+        "Unsupported update operator '%s'".formatted(operator));
+  }
+
   private static Map<String, UpdateOperator> operatorMap = new HashMap<>();
 
   static {

src/main/java/io/stargate/sgv3/docsapi/api/model/command/deserializers/UpdateClauseDeserializer.java

@@ -1,19 +1,17 @@
 package io.stargate.sgv3.docsapi.api.model.command.deserializers;
 
-import com.fasterxml.jackson.core.JacksonException;
 import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.databind.DeserializationContext;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.node.ObjectNode;
 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 io.stargate.sgv3.docsapi.exception.ErrorCode;
 import java.io.IOException;
-import java.util.ArrayList;
+import java.util.EnumMap;
 import java.util.Iterator;
-import java.util.List;
 import java.util.Map;
 
 /** {@link StdDeserializer} for the {@link UpdateClause}. */
@@ -26,67 +24,39 @@ public UpdateClauseDeserializer() {
   /** {@inheritDoc} */
   @Override
   public UpdateClause deserialize(
-      JsonParser jsonParser, DeserializationContext deserializationContext)
-      throws IOException, JacksonException {
+      JsonParser jsonParser, DeserializationContext deserializationContext) throws IOException {
     JsonNode filterNode = deserializationContext.readTree(jsonParser);
-    if (!filterNode.isObject()) throw new DocsException(ErrorCode.UNSUPPORTED_UPDATE_DATA_TYPE);
+    if (!filterNode.isObject()) {
+      throw new DocsException(
+          ErrorCode.UNSUPPORTED_UPDATE_DATA_TYPE,
+          "Unsupported update data type for UpdateClause (must be JSON Object): "
+              + filterNode.getNodeType());
+    }
+    final EnumMap<UpdateOperator, ObjectNode> updateDefs = new EnumMap<>(UpdateOperator.class);
     Iterator<Map.Entry<String, JsonNode>> fieldIter = filterNode.fields();
-    List<UpdateOperation> expressionList = new ArrayList<>();
     while (fieldIter.hasNext()) {
       Map.Entry<String, JsonNode> entry = fieldIter.next();
-      if (entry.getKey().startsWith("$")) {
-        // Using aggregation pipeline
-        getOperations(entry, expressionList);
-      } else {
+      final String operName = entry.getKey();
+      if (!operName.startsWith("$")) {
         throw new DocsException(
             ErrorCode.UNSUPPORTED_UPDATE_OPERATION,
-            "Unsupported update operator " + entry.getKey());
+            "Invalid update operator '%s' (must start with '$')".formatted(operName));
       }
-    }
-    return new UpdateClause(expressionList);
-  }
-
-  /**
-   * The key of the entry will be update type like $set, $unset The value of the entry will be
-   * object of field to be updated "$set" : {"field1" : val1, "field2" : val2 }
-   *
-   * @param entry
-   * @return
-   */
-  private void getOperations(
-      Map.Entry<String, JsonNode> entry, List<UpdateOperation> expressionList) {
-    try {
-      UpdateOperator operator = UpdateOperator.getUpdateOperator(entry.getKey());
-      if (entry.getValue().isObject()) {
-        final Iterator<Map.Entry<String, JsonNode>> fields = entry.getValue().fields();
-        while (fields.hasNext()) {
-          Map.Entry<String, JsonNode> updateField = fields.next();
-          JsonNode value = updateField.getValue();
-          expressionList.add(
-              new UpdateOperation(updateField.getKey(), operator, jsonNodeValue(value)));
-        }
-      } else {
-        throw new DocsException(ErrorCode.UNSUPPORTED_UPDATE_DATA_TYPE);
+      UpdateOperator oper = UpdateOperator.getUpdateOperator(operName);
+      if (oper == null) {
+        throw new DocsException(
+            ErrorCode.UNSUPPORTED_UPDATE_OPERATION,
+            "Unrecognized update operator '%s'".formatted(operName));
       }
-    } catch (IllegalArgumentException e) {
-      throw new DocsException(
-          ErrorCode.UNSUPPORTED_UPDATE_OPERATION, "Unsupported update operation " + entry.getKey());
-    }
-  }
-
-  private static JsonNode jsonNodeValue(JsonNode node) {
-    switch (node.getNodeType()) {
-      case BOOLEAN:
-      case NUMBER:
-      case STRING:
-      case NULL:
-      case ARRAY:
-      case OBJECT:
-        return node;
-      default:
+      JsonNode operationArg = entry.getValue();
+      if (!operationArg.isObject()) {
         throw new DocsException(
-            ErrorCode.UNSUPPORTED_FILTER_DATA_TYPE,
-            String.format("Unsupported NodeType %s", node.getNodeType()));
+            ErrorCode.UNSUPPORTED_UPDATE_DATA_TYPE,
+            "Unsupported update data type for Operator '%s' (must be JSON Object): %s"
+                .formatted(operName, operationArg.getNodeType()));
+      }
+      updateDefs.put(oper, (ObjectNode) operationArg);
     }
+    return new UpdateClause(updateDefs);
   }
 }

src/main/java/io/stargate/sgv3/docsapi/config/constants/DocumentConstants.java

@@ -0,0 +1,9 @@
+package io.stargate.sgv3.docsapi.config.constants;
+
+public interface DocumentConstants {
+  /** Names of "special" fields in Documents */
+  interface Fields {
+    /** Primary key for Documents stored; has special handling for many operations. */
+    String DOC_ID = "_id";
+  }
+}

src/main/java/io/stargate/sgv3/docsapi/exception/ErrorCode.java

@@ -26,7 +26,11 @@ public enum ErrorCode {
 
   UNSUPPORTED_UPDATE_DATA_TYPE("Unsupported update data type"),
 
-  UNSUPPORTED_UPDATE_OPERATION("Unsupported update operator");
+  UNSUPPORTED_UPDATE_OPERATION("Unsupported update operation"),
+
+  UNSUPPORTED_UPDATE_OPERATION_PARAM("Unsupported update operation parameter"),
+
+  UNSUPPORTED_UPDATE_FOR_DOC_ID("Cannot use operator with '_id' field");
 
   private final String message;
 

src/main/java/io/stargate/sgv3/docsapi/service/resolver/model/impl/FindOneAndUpdateCommandResolver.java

@@ -33,7 +33,7 @@ public Class<FindOneAndUpdateCommand> getCommandClass() {
   @Override
   public Operation resolveCommand(CommandContext ctx, FindOneAndUpdateCommand command) {
     ReadOperation readOperation = resolve(ctx, command);
-    DocumentUpdater documentUpdater = new DocumentUpdater(command.updateClause());
+    DocumentUpdater documentUpdater = DocumentUpdater.construct(command.updateClause());
     return new ReadAndUpdateOperation(ctx, readOperation, documentUpdater, true, shredder);
   }
 

src/main/java/io/stargate/sgv3/docsapi/service/resolver/model/impl/UpdateOneCommandResolver.java

@@ -33,7 +33,7 @@ public Class<UpdateOneCommand> getCommandClass() {
   @Override
   public Operation resolveCommand(CommandContext ctx, UpdateOneCommand command) {
     ReadOperation readOperation = resolve(ctx, command);
-    DocumentUpdater documentUpdater = new DocumentUpdater(command.updateClause());
+    DocumentUpdater documentUpdater = DocumentUpdater.construct(command.updateClause());
     return new ReadAndUpdateOperation(ctx, readOperation, documentUpdater, false, shredder);
   }