3.2.0 backports of the fixes for dict-based child entity update implementation

spec/regression/logistics/tests/update-child-entities-dict.graphql

@@ -58,9 +58,13 @@ mutation updateMultiple {
             updateItems: [
                 { id: "id_test_0003", itemNumber: "updated03" }
                 { id: "id_test_0005", itemNumber: "updated05" }
-                { id: "id_test_0007", itemNumber: "updated07" }
                 { id: "id_test_0008", itemNumber: "updated08" }
                 { id: "id_test_0009", itemNumber: "updated09" }
+                # out of order to make sure the original order is preserved
+                { id: "id_test_0007", itemNumber: "updated07" }
+                # make sure this one does not end up in the list
+                # (the initial implementation of the dict-based update had a bug that it was)
+                { id: "nonexistant", itemNumber: "updated10" }
             ]
         }
     ) {

spec/regression/logistics/tests/update-child-entities.graphql

@@ -58,9 +58,13 @@ mutation updateMultiple {
             updateItems: [
                 { id: "id_test_0003", itemNumber: "updated03" }
                 { id: "id_test_0005", itemNumber: "updated05" }
-                { id: "id_test_0007", itemNumber: "updated07" }
                 { id: "id_test_0008", itemNumber: "updated08" }
                 { id: "id_test_0009", itemNumber: "updated09" }
+                # out of order to make sure the original order is preserved
+                { id: "id_test_0007", itemNumber: "updated07" }
+                # make sure this one does not end up in the list
+                # (the initial implementation of the dict-based update had a bug that it was)
+                { id: "nonexistant", itemNumber: "updated10" }
             ]
         }
     ) {

spec/regression/logistics/tests/update-empty-child-entities-list-dict.context.json

@@ -0,0 +1,4 @@
+{
+  "childEntityUpdatesViaDictStrategyThreshold": 1,
+  "authRoles": ["allusers"]
+}

spec/regression/logistics/tests/update-empty-child-entities-list-dict.graphql

@@ -0,0 +1,59 @@
+# in the .context.json, childEntityUpdatesViaDictStrategyThreshold is set to 1
+# so we will always use the dict strategy to update child entities
+
+# this test makes sure that updating child entities works (even though it does nothing)
+# when the child entity list in the database is empty
+# this regression test exists because of two bugs we had in the initial dict-based implementation
+# - an AQL error when there were updates but the collection was empty
+# - updates that did not correspond to actual were persisted as new items (just without id etc.)
+
+mutation clearItems {
+    updateDelivery(
+        input: { id: "@{ids/Delivery/1}", removeItems: ["id_init_0000", "id_init_0001"] }
+    ) {
+        items {
+            id
+            itemNumber
+        }
+    }
+}
+
+# to verify it is cleared in the db
+query afterClear {
+    Delivery(id: "@{ids/Delivery/1}") {
+        items {
+            id
+            itemNumber
+        }
+    }
+}
+
+mutation updateMultiple {
+    updateDelivery(
+        input: {
+            id: "@{ids/Delivery/1}"
+            updateItems: [
+                { id: "id_test_0003", itemNumber: "updated03" }
+                { id: "id_test_0005", itemNumber: "updated05" }
+                { id: "id_test_0009", itemNumber: "updated09" }
+                { id: "id_test_0007", itemNumber: "updated07" }
+                { id: "id_test_0008", itemNumber: "updated08" }
+            ]
+        }
+    ) {
+        items {
+            id
+            itemNumber
+        }
+    }
+}
+
+# just sanity check - should do nothign
+query afterUpdateMultiple {
+    Delivery(id: "@{ids/Delivery/1}") {
+        items {
+            id
+            itemNumber
+        }
+    }
+}

spec/regression/logistics/tests/update-empty-child-entities-list-dict.result.json

@@ -0,0 +1,30 @@
+{
+  "clearItems": {
+    "data": {
+      "updateDelivery": {
+        "items": []
+      }
+    }
+  },
+  "afterClear": {
+    "data": {
+      "Delivery": {
+        "items": []
+      }
+    }
+  },
+  "updateMultiple": {
+    "data": {
+      "updateDelivery": {
+        "items": []
+      }
+    }
+  },
+  "afterUpdateMultiple": {
+    "data": {
+      "Delivery": {
+        "items": []
+      }
+    }
+  }
+}

spec/regression/logistics/tests/update-empty-child-entities-list.context.json

@@ -0,0 +1,4 @@
+{
+  "childEntityUpdatesViaDictStrategyThreshold": 100,
+  "authRoles": ["allusers"]
+}

spec/regression/logistics/tests/update-empty-child-entities-list.graphql

@@ -0,0 +1,56 @@
+# this test makes sure that updating child entities works (even though it does nothing)
+# when the child entity list in the database is empty
+# this regression test exists because of two bugs we had in the initial dict-based implementation
+# - an AQL error when there were updates but the collection was empty
+# - updates that did not correspond to actual were persisted as new items (just without id etc.)
+
+mutation clearItems {
+    updateDelivery(
+        input: { id: "@{ids/Delivery/1}", removeItems: ["id_init_0000", "id_init_0001"] }
+    ) {
+        items {
+            id
+            itemNumber
+        }
+    }
+}
+
+# to verify it is cleared in the db
+query afterClear {
+    Delivery(id: "@{ids/Delivery/1}") {
+        items {
+            id
+            itemNumber
+        }
+    }
+}
+
+mutation updateMultiple {
+    updateDelivery(
+        input: {
+            id: "@{ids/Delivery/1}"
+            updateItems: [
+                { id: "id_test_0003", itemNumber: "updated03" }
+                { id: "id_test_0005", itemNumber: "updated05" }
+                { id: "id_test_0009", itemNumber: "updated09" }
+                { id: "id_test_0007", itemNumber: "updated07" }
+                { id: "id_test_0008", itemNumber: "updated08" }
+            ]
+        }
+    ) {
+        items {
+            id
+            itemNumber
+        }
+    }
+}
+
+# just sanity check - should do nothign
+query afterUpdateMultiple {
+    Delivery(id: "@{ids/Delivery/1}") {
+        items {
+            id
+            itemNumber
+        }
+    }
+}

spec/regression/logistics/tests/update-empty-child-entities-list.result.json

@@ -0,0 +1,30 @@
+{
+  "clearItems": {
+    "data": {
+      "updateDelivery": {
+        "items": []
+      }
+    }
+  },
+  "afterClear": {
+    "data": {
+      "Delivery": {
+        "items": []
+      }
+    }
+  },
+  "updateMultiple": {
+    "data": {
+      "updateDelivery": {
+        "items": []
+      }
+    }
+  },
+  "afterUpdateMultiple": {
+    "data": {
+      "Delivery": {
+        "items": []
+      }
+    }
+  }
+}

src/database/arangodb/aql-generator.ts

@@ -826,7 +826,9 @@ register(UpdateChildEntitiesQueryNode, (node, context) => {
         // temporary property to store the index of the child entity in the list
         aql`LET ${itemsWithIndexVar} = ${aqlExt.parenthesizeList(
             aql`FOR ${indexVar}`,
-            aql`IN 0..(LENGTH(${itemsVar}) - 1)`,
+            // 0..-1 would evaluate to [0, -1], so the ZIP would complain because the right side
+            // has more entries (2) than the left (0). RANGE() behaves the same
+            aql`IN LENGTH(${itemsVar}) > 0 ? 0..(LENGTH(${itemsVar}) - 1) : []`,
             aql`RETURN MERGE(NTH(${itemsVar}, ${indexVar}), { __index: ${indexVar} })`,
         )}`,
 
@@ -850,9 +852,13 @@ register(UpdateChildEntitiesQueryNode, (node, context) => {
         ),
         aql`})`,
 
-        // sort by the __index we stored, and unpack the dictionary into a list again
+        // sort by the __index we stored,
+        // filter out objects that were included in node.updates() but did not actually exist in node.originalList
+        // (for them, __index is not set)
+        // and unpack the dictionary into a list again
         aql`FOR ${itemVar}`,
         aql`IN VALUES(${updatedDictVar})`,
+        aql`FILTER ${itemVar}.__index != null`,
         aql`SORT ${itemVar}.__index`,
         aql`RETURN UNSET(${itemVar}, '__index')`,
     );

src/database/inmemory/js-generator.ts

@@ -528,7 +528,11 @@ register(UpdateChildEntitiesQueryNode, (node, context) => {
         // could be a complex expression, and we're using it multiple times -> store in a variable
         js`const ${itemsVar} = ${processNode(node.originalList, context)}`,
 
-        // add a __index property to each item so we can sort by this later
+        // the aql implementation needs an __index property to sort the object after applying
+        // the updates. the js implementation does not need to sort because {...obj, a: ...}
+        // keeps the order of properties in obj. However, we still need to filter out updates
+        // for which there are no items in originalList. To stay close to the aql implementation,
+        // we just also set an __index here.
         // regular field names cannot start with an underscore, so we're safe to use __index as a
         // temporary property to store the index of the child entity in the list
         // convert the list into a dict object like { 'id1': { ...}, 'id2': { ... } }
@@ -566,8 +570,10 @@ register(UpdateChildEntitiesQueryNode, (node, context) => {
         ),
         js`};`,
 
-        // sort by the __index we stored, and unpack the dictionary into a list again
-        js`return Object.values(${updatedDictVar}).map(({ __index, ...${itemVar} }) => ${itemVar});`,
+        // filter out objects that were included in node.updates() but did not actually exist in node.originalList
+        // (for them, __index is not set)
+        // and unpack the dictionary into a list again
+        js`return Object.values(${updatedDictVar}).filter(({ __index }) => __index !== undefined).map(({ __index, ...${itemVar} }) => ${itemVar});`,
     );
 });