Add checks to make sure Update Operators do not use conflicting/overl…

…apping paths (#267)

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/UpdateClause.java

@@ -5,10 +5,12 @@
 import io.stargate.sgv2.jsonapi.api.model.command.deserializers.UpdateClauseDeserializer;
 import io.stargate.sgv2.jsonapi.exception.ErrorCode;
 import io.stargate.sgv2.jsonapi.exception.JsonApiException;
+import io.stargate.sgv2.jsonapi.util.PathMatchLocator;
 import java.util.ArrayList;
 import java.util.EnumMap;
 import java.util.List;
 import java.util.Set;
+import java.util.TreeMap;
 import java.util.stream.Collectors;
 import org.eclipse.microprofile.openapi.annotations.enums.SchemaType;
 import org.eclipse.microprofile.openapi.annotations.media.Schema;
@@ -37,27 +39,51 @@ public record UpdateClause(EnumMap<UpdateOperator, ObjectNode> updateOperationDe
    */
   public List<UpdateOperation> buildOperations() {
     // First, resolve operations individually; this will handle basic validation
-    var operationMap = new EnumMap<UpdateOperator, UpdateOperation>(UpdateOperator.class);
+    final 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
-
-    UpdateOperation<?> setOp = operationMap.get(UpdateOperator.SET);
-    UpdateOperation<?> unsetOp = operationMap.get(UpdateOperator.UNSET);
+    // Then handle cross-operation validation: first, exact path conflicts:
+    final var actionMap = new TreeMap<PathMatchLocator, UpdateOperator>();
+    operationMap
+        .entrySet()
+        .forEach(
+            e -> {
+              final UpdateOperator type = e.getKey();
+              List<ActionWithLocator> actions = e.getValue().actions();
+              for (ActionWithLocator action : actions) {
+                UpdateOperator prevType = actionMap.put(action.locator(), type);
+                if (prevType != null) {
+                  throw new JsonApiException(
+                      ErrorCode.UNSUPPORTED_UPDATE_OPERATION_PARAM,
+                      "Update operators '%s' and '%s' must not refer to same path: '%s'"
+                          .formatted(prevType.operator(), type.operator(), action.locator()));
+                }
+              }
+            });
 
-    if ((setOp != null) && (unsetOp != null)) {
-      Set<String> paths = getPaths(setOp);
-      paths.retainAll(getPaths(unsetOp));
+    // Second: check for sub-paths -- efficiently done by using alphabetic-sorted paths
+    // and comparing adjacent ones; for each pair see if later one is longer one and
+    // starts with dot.
+    // For example: "path.x" vs "path.x.some.more" we notice latter has suffix ".some.more"
+    // and is thereby conflicting.
+    if (!actionMap.isEmpty()) {
+      var it = actionMap.entrySet().iterator();
+      var prev = it.next();
 
-      if (!paths.isEmpty()) {
-        throw new JsonApiException(
-            ErrorCode.UNSUPPORTED_UPDATE_OPERATION_PARAM,
-            "Update operators '$set' and '$unset' must not refer to same path: '%s'"
-                .formatted(paths.iterator().next()));
+      while (it.hasNext()) {
+        var curr = it.next();
+        PathMatchLocator prevLoc = prev.getKey();
+        PathMatchLocator currLoc = curr.getKey();
+        // So, if parent/child, parent always first so this check is enough
+        if (currLoc.isSubPathOf(prevLoc)) {
+          throw new JsonApiException(
+              ErrorCode.UNSUPPORTED_UPDATE_OPERATION_PARAM,
+              "Update operator path conflict due to overlap: '%s' (%s) vs '%s' (%s)"
+                  .formatted(
+                      prevLoc, prev.getValue().operator(), currLoc, curr.getValue().operator()));
+        }
       }
     }
 

src/main/java/io/stargate/sgv2/jsonapi/util/PathMatchLocator.java

@@ -16,8 +16,13 @@
  *   <li>{@link JsonNode} when extracting value using {@link #findValueIn} (used for Sorting and
  *       possibly Projection)
  * </ul>
+ *
+ * <p>Implements {@link Comparable} so that locators are naturally sorted in "Segment-aware" way:
+ * meaning that sorting is segment-by-segment, alphabetically, so that parent path will be sorted
+ * immediately before its first child path (if any). This sorting is used to verify that update
+ * operations' locator paths do not overlap in ancestors/descendants dimensions.
  */
-public class PathMatchLocator {
+public class PathMatchLocator implements Comparable<PathMatchLocator> {
   private static final Pattern DOT = Pattern.compile(Pattern.quote("."));
 
   private static final Pattern INDEX_SEGMENT = Pattern.compile("0|[1-9][0-9]*");
@@ -35,6 +40,27 @@ public String path() {
     return dotPath;
   }
 
+  /**
+   * Method that will check whether this locator represents a "sub-path" of given locator: this is
+   * the case if the "parent" path is a proper prefix of this path, followed by a comma and path
+   * segment(s). For example: if this locator has path {@code a.b.c} and {@code possibleParent} has
+   * path {@code a.b} then method would return true (as suffix is {@code .c}).
+   *
+   * <p>Note: if paths are the same, will NOT be considered a sub-path (returns {@code false}).
+   *
+   * @param possibleParent Locator to check against
+   * @return True if this locator has a path that is sub-path of path of {@code possibleParent}
+   */
+  public boolean isSubPathOf(PathMatchLocator possibleParent) {
+    String parentPath = possibleParent.path();
+    String thisPath = path();
+    final int parentLen = parentPath.length();
+
+    return thisPath.startsWith(parentPath)
+        && parentLen < thisPath.length()
+        && thisPath.charAt(parentLen) == '.';
+  }
+
   /**
    * Factory method for constructing path; also does minimal verification of path: currently only
    * verification is to ensure there are no empty segments.
@@ -241,4 +267,26 @@ public boolean equals(Object o) {
   public int hashCode() {
     return dotPath.hashCode();
   }
+
+  @Override
+  public String toString() {
+    return dotPath;
+  }
+
+  @Override
+  public int compareTo(PathMatchLocator other) {
+    // Instead of simple alphabetic sorting of dotPath, do segment-aware to
+    // ensure parent/children are sorted next to each other
+    final String[] s1 = this.segments;
+    final String[] s2 = other.segments;
+
+    for (int i = 0, end = Math.min(s1.length, s2.length); i < end; ++i) {
+      int diff = s1[i].compareTo(s2[i]);
+      if (diff != 0) {
+        return diff;
+      }
+    }
+    // If same prefix sort longer one after shorter one
+    return s1.length - s2.length;
+  }
 }