Fix #164: order individual update operation actions as per spec (#221)

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

@@ -0,0 +1,9 @@
+package io.stargate.sgv2.jsonapi.api.model.command.clause.update;
+
+/**
+ * Interface needed to allow easy sorting by {@code path} property exposed by Action record types.
+ */
+public interface ActionWithPath {
+  /** @return Path that the action targets (dotted notation) */
+  String path();
+}

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

@@ -19,7 +19,7 @@ public class AddToSetOperation extends UpdateOperation {
   private List<AddToSetAction> actions;
 
   private AddToSetOperation(List<AddToSetAction> actions) {
-    this.actions = actions;
+    this.actions = sortByPath(actions);
   }
 
   public static AddToSetOperation construct(ObjectNode args) {
@@ -153,5 +153,6 @@ private boolean addToSet(ArrayNode set, JsonNode elementToAdd) {
   }
 
   /** Value class for per-field update operations. */
-  private record AddToSetAction(String path, JsonNode value, boolean each) {}
+  private record AddToSetAction(String path, JsonNode value, boolean each)
+      implements ActionWithPath {}
 }

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

@@ -16,10 +16,10 @@
  * explanation.
  */
 public class IncOperation extends UpdateOperation {
-  private final List<IncAction> updates;
+  private final List<IncAction> actions;
 
-  private IncOperation(List<IncAction> updates) {
-    this.updates = updates;
+  private IncOperation(List<IncAction> actions) {
+    this.actions = sortByPath(actions);
   }
 
   public static IncOperation construct(ObjectNode args) {
@@ -49,7 +49,7 @@ public static IncOperation construct(ObjectNode args) {
   public boolean updateDocument(ObjectNode doc, UpdateTargetLocator targetLocator) {
     // Almost always changes, except if adding zero; need to track
     boolean modified = false;
-    for (IncAction action : updates) {
+    for (IncAction action : actions) {
       final String path = action.path;
       final NumericNode toAdd = action.value;
 
@@ -92,5 +92,5 @@ private JsonNode addNumbers(ObjectNode doc, NumericNode nr1, NumericNode nr2) {
   }
 
   /** Value class for per-field update operations. */
-  private record IncAction(String path, NumericNode value) {}
+  private record IncAction(String path, NumericNode value) implements ActionWithPath {}
 }

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

@@ -14,7 +14,7 @@ public class PopOperation extends UpdateOperation {
   private List<PopAction> actions;
 
   private PopOperation(List<PopAction> actions) {
-    this.actions = actions;
+    this.actions = sortByPath(actions);
   }
 
   public static PopOperation construct(ObjectNode args) {
@@ -94,5 +94,5 @@ public boolean updateDocument(ObjectNode doc, UpdateTargetLocator targetLocator)
   }
 
   /** Value class for per-field Pop operation definitions. */
-  private record PopAction(String path, boolean removeFirst) {}
+  private record PopAction(String path, boolean removeFirst) implements ActionWithPath {}
 }

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

@@ -19,7 +19,7 @@ public class PushOperation extends UpdateOperation {
   private List<PushAction> actions;
 
   private PushOperation(List<PushAction> actions) {
-    this.actions = actions;
+    this.actions = sortByPath(actions);
   }
 
   public static PushOperation construct(ObjectNode args) {
@@ -186,5 +186,6 @@ public boolean equals(Object o) {
   }
 
   /** Value class for per-field update operations. */
-  private record PushAction(String path, JsonNode value, boolean each, Integer position) {}
+  private record PushAction(String path, JsonNode value, boolean each, Integer position)
+      implements ActionWithPath {}
 }

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

@@ -11,10 +11,10 @@
 
 /** Implementation of {@code $set} update operation used to assign values to document fields. */
 public class SetOperation extends UpdateOperation {
-  private final List<SetAction> additions;
+  private final List<SetAction> actions;
 
-  private SetOperation(List<SetAction> additions) {
-    this.additions = additions;
+  private SetOperation(List<SetAction> actions) {
+    this.actions = sortByPath(actions);
   }
 
   public static SetOperation construct(ObjectNode args) {
@@ -45,7 +45,7 @@ public static SetOperation construct(String filterPath, JsonNode value) {
   @Override
   public boolean updateDocument(ObjectNode doc, UpdateTargetLocator targetLocator) {
     boolean modified = false;
-    for (SetAction addition : additions) {
+    for (SetAction addition : actions) {
       UpdateTarget target = targetLocator.findOrCreate(doc, addition.path());
       JsonNode newValue = addition.value();
       JsonNode oldValue = target.valueNode();
@@ -59,16 +59,15 @@ public boolean updateDocument(ObjectNode doc, UpdateTargetLocator targetLocator)
     return modified;
   }
 
-  public Set<String> paths() {
-    return additions.stream().map(SetAction::path).collect(Collectors.toSet());
+  public Set<String> getPaths() {
+    return actions.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);
+    return (o instanceof SetOperation) && Objects.equals(this.actions, ((SetOperation) o).actions);
   }
 
-  private record SetAction(String path, JsonNode value) {}
+  private record SetAction(String path, JsonNode value) implements ActionWithPath {}
 }

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

@@ -2,46 +2,41 @@
 
 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;
+import java.util.stream.Collectors;
 
 /** Implementation of {@code $unset} update operation used to remove fields from documents. */
 public class UnsetOperation extends UpdateOperation {
-  private List<String> paths;
+  private List<UnsetAction> actions;
 
-  private UnsetOperation(List<String> paths) {
-    this.paths = paths;
+  private UnsetOperation(List<UnsetAction> actions) {
+    this.actions = sortByPath(actions);
   }
 
   public static UnsetOperation construct(ObjectNode args) {
     Iterator<String> it = args.fieldNames();
-    List<String> fieldNames = new ArrayList<>();
+    List<UnsetAction> actions = new ArrayList<>();
     while (it.hasNext()) {
-      fieldNames.add(validateUpdatePath(UpdateOperator.UNSET, it.next()));
+      actions.add(new UnsetAction(validateUpdatePath(UpdateOperator.UNSET, it.next())));
     }
-    return new UnsetOperation(fieldNames);
+    return new UnsetOperation(actions);
   }
 
   @Override
   public boolean updateDocument(ObjectNode doc, UpdateTargetLocator targetLocator) {
     boolean modified = false;
-    for (String path : paths) {
-      UpdateTarget target = targetLocator.findIfExists(doc, path);
+    for (UnsetAction action : actions) {
+      UpdateTarget target = targetLocator.findIfExists(doc, action.path());
       modified |= (target.removeValue() != null);
     }
     return modified;
   }
 
-  public Set<String> paths() {
-    return new HashSet<>(paths);
+  public Set<String> getPaths() {
+    return actions.stream().map(UnsetAction::path).collect(Collectors.toSet());
   }
 
-  // Just needed for tests
-  @Override
-  public boolean equals(Object o) {
-    return (o instanceof UnsetOperation) && Objects.equals(this.paths, ((UnsetOperation) o).paths);
-  }
+  private record UnsetAction(String path) implements ActionWithPath {}
 }

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

@@ -47,8 +47,8 @@ public List<UpdateOperation> buildOperations() {
     UnsetOperation unsetOp = (UnsetOperation) operationMap.get(UpdateOperator.UNSET);
 
     if ((setOp != null) && (unsetOp != null)) {
-      Set<String> paths = setOp.paths();
-      paths.retainAll(unsetOp.paths());
+      Set<String> paths = setOp.getPaths();
+      paths.retainAll(unsetOp.getPaths());
 
       if (!paths.isEmpty()) {
         throw new JsonApiException(

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

@@ -4,6 +4,9 @@
 import io.stargate.sgv2.jsonapi.config.constants.DocumentConstants;
 import io.stargate.sgv2.jsonapi.exception.ErrorCode;
 import io.stargate.sgv2.jsonapi.exception.JsonApiException;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
 
 /**
  * UpdateOperation represents one of update definitions from {@link UpdateClause} (like {@code $set}
@@ -56,4 +59,18 @@ protected static String validateNonModifierPath(UpdateOperator oper, String path
   protected static boolean looksLikeModifier(String path) {
     return path.startsWith("$");
   }
+
+  protected <P extends ActionWithPath> List<P> sortByPath(List<P> actions) {
+    Collections.sort(actions, NameComparator.INSTANCE);
+    return actions;
+  }
+
+  static class NameComparator implements Comparator<ActionWithPath> {
+    public static final NameComparator INSTANCE = new NameComparator();
+
+    @Override
+    public int compare(ActionWithPath o1, ActionWithPath o2) {
+      return o1.path().compareTo(o2.path());
+    }
+  }
 }

src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/command/clause/update/PushOperationTest.java

@@ -83,6 +83,26 @@ public void testPushToNonExistingNested() {
                                       """);
       assertThat(doc).isEqualTo(expected);
     }
+
+    // Test to ensure $push operations are done in alphabetic order by field
+    @Test
+    public void testPushToNonExistingOrdered() {
+      // Put targets in "wrong" order (different from expected execution order)
+      UpdateOperation oper =
+          UpdateOperator.PUSH.resolveOperation(
+              objectFromJson("{ \"subdoc.newArray\" : \"value\", \"array\": 3 }"));
+      assertThat(oper).isInstanceOf(PushOperation.class);
+      ObjectNode doc = objectFromJson("{ }");
+      assertThat(oper.updateDocument(doc, targetLocator)).isTrue();
+      ObjectNode expected =
+          objectFromJson(
+              """
+                      { "array": [ 3 ], "subdoc" : { "newArray": [ "value" ] } }
+                      """);
+      // Important: compare serializations as they indicate order of Object fields;
+      // ObjectNode.equals() uses order-insensitive comparison
+      assertThat(doc.toPrettyString()).isEqualTo(expected.toPrettyString());
+    }
   }
 
   @Nested

src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/command/clause/update/SetOperationTest.java

@@ -22,7 +22,7 @@ public class SetOperationTest extends UpdateOperationTestBase {
   class HappyPathRoot {
     @Test
     public void testSimpleSetOfExisting() {
-      // Remove 2 of 3 properties:
+      // Replace 2 of 3 properties:
       UpdateOperation oper =
           UpdateOperator.SET.resolveOperation(
               objectFromJson(
@@ -69,6 +69,26 @@ public void testSimpleSetOfNonExisting() {
       assertThat(doc).isEqualTo(expected);
     }
 
+    // Test for [json-api#164], order of $set additions alphabetic
+    @Test
+    public void testOrderedSetOfNonExisting() {
+      UpdateOperation oper =
+          UpdateOperator.SET.resolveOperation(
+              objectFromJson(
+                  """
+                                { "b" : 2, "a": 1}
+                                """));
+      assertThat(oper).isInstanceOf(SetOperation.class);
+      // Will append the new property so there is modification
+      ObjectNode doc = objectFromJson("{ }");
+      assertThat(oper.updateDocument(doc, targetLocator)).isTrue();
+      ObjectNode expected = objectFromJson("{ \"a\": 1, \"b\": 2 }");
+
+      // Important! Compare serialization as that preserves ordering: not ObjectNode
+      // which would use order-insensitive comparison
+      assertThat(doc.toPrettyString()).isEqualTo(expected.toPrettyString());
+    }
+
     @Test
     public void testSimpleSetWithoutChange() {
       UpdateOperation oper =

src/test/java/io/stargate/sgv2/jsonapi/service/operation/model/command/clause/update/UnsetOperationTest.java

@@ -12,7 +12,7 @@
 import io.stargate.sgv2.jsonapi.api.model.command.clause.update.UpdateOperator;
 import io.stargate.sgv2.jsonapi.exception.ErrorCode;
 import io.stargate.sgv2.jsonapi.exception.JsonApiException;
-import java.util.Arrays;
+import java.util.Set;
 import org.junit.jupiter.api.MethodOrderer;
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
@@ -34,7 +34,7 @@ public void testSimpleUnsetOfExisting() {
                     """));
       assertThat(oper)
           .isInstanceOf(UnsetOperation.class)
-          .hasFieldOrPropertyWithValue("paths", Arrays.asList("a", "b"));
+          .hasFieldOrPropertyWithValue("paths", Set.of("a", "b"));
       // Should indicate document being modified
       ObjectNode doc = defaultTestDocABC();
       assertThat(oper.updateDocument(doc, targetLocator)).isTrue();
@@ -56,7 +56,7 @@ public void testSimpleUnsetOfNonExisting() {
                     """));
       assertThat(oper)
           .isInstanceOf(UnsetOperation.class)
-          .hasFieldOrPropertyWithValue("paths", Arrays.asList("missing", "nosuchvalue"));
+          .hasFieldOrPropertyWithValue("paths", Set.of("missing", "nosuchvalue"));
       ObjectNode doc = defaultTestDocABC();
       // No modifications
       assertThat(oper.updateDocument(doc, targetLocator)).isFalse();