Repository: ChristopheBougere/asl-validator (ts)

Issue:
replace JSON path expression in mustNotHaveDuplicateFieldNamesAfterEvaluation with a traversal
**Describe the bug**
The JSON path expression used to identify the objects in the payload to check for duplicate fields throws an error when it encounters an array. This looks to be an issue in the jsonpath-plus library.

**To Reproduce**
Use the input for #104 (part of the repo now in `valid-parameters-issue104.json` and remove the try/catch added in `mustNotHaveDuplicateFieldNamesAfterEvaluation`

**Expected behavior**
Shouldn't throw a runtime exception from the jsonpath-plus library. Should instead correctly report the payload template as valid or invalid.

**Version:**
3.0.x

**Additional context**
I think the fix is to replace the jsonpath-plus specific expression `$..[Parameters,ResultSelector]..*[?(@property.match(/^.+\.\$$/))][0]^^` with a simple graph traversal. There's too much dark magic in the expression evaluation even without this array handling issue. 

Pull request:
fix(check): replace jsonpath-plus expression with traversal for dupe fields
closes #108 

Patch:
diff --git a/src/checks/duplicate-payload-template-fields.ts b/src/checks/duplicate-payload-template-fields.ts
index 0fa4076..045deca 100644
--- a/src/checks/duplicate-payload-template-fields.ts
+++ b/src/checks/duplicate-payload-template-fields.ts
@@ -9,28 +9,12 @@ export const mustNotHaveDuplicateFieldNamesAfterEvaluation: AslChecker = (defini
     // for each one, examine each node's children for field names
     // that will be in conflict after evaluation.
     const errorMessages: StateMachineError[] = [];
-    let valuesToInspect: Record<string, unknown>[] = [];
-    try {
-        valuesToInspect = JSONPath<Record<string, unknown>[]>({
-            json: definition,
-            // this uses some extensions to the original JSONPath spec supported by the lib
-            //
-            // extension 1: @property.match enables matching on the matched property name.
-            //              This finds and selects all nodes with a name ending in .$
-            // extension 2: ^ operator twice at the end selects the parent node instead of the value for the node
-            //
-            // also, don't miss the array offset [0] before selecting the grandparent.
-            // There may be multiple fields within a node that could be in conflict.
-            // We're checking all the keys in the parent node, so we only need a single
-            // result for that parent.
-            path: `$..[Parameters,ResultSelector]..*[?(@property.match(/^.+\\.\\$$/))][0]^^`
-        })
-    } catch (err: unknown) {
-        // need to revisit this since the JSONPath lib throws an error when it encounters an array
-        // Error: jsonPath: _$_property.match is not a function: _$_property.match(/^.+\.\$$/)
-    }
+    const valuesToInspect: Record<string, unknown>[] = JSONPath<Record<string, unknown>[]>({
+        json: definition,
+        path: `$..[Parameters,ResultSelector]`
+    })
 
-    valuesToInspect.forEach((parent) => {
+    const inspectKeys = (parent: Record<string, unknown>): void => {
         const keys: Record<string, number> = {};
         Object.keys(parent).forEach((key) => {
             const keyPostEval = key.endsWith(".$") ? key.substring(0, key.length - 2) : key;
@@ -42,7 +26,12 @@ export const mustNotHaveDuplicateFieldNamesAfterEvaluation: AslChecker = (defini
                 });
             }
             keys[keyPostEval] = current ? current + 1 : 1;
+            if (typeof parent[key] === "object") {
+                inspectKeys(parent[key] as Record<string, unknown>);
+            }
         });
-    });
+    }
+
+    valuesToInspect.forEach((parent) => inspectKeys(parent));
     return errorMessages;
 }


Test patch:
diff --git a/src/__tests__/definitions/invalid-duplicate-fields-nested.json b/src/__tests__/definitions/invalid-duplicate-fields-nested.json
new file mode 100644
index 0000000..6cc8839
--- /dev/null
+++ b/src/__tests__/definitions/invalid-duplicate-fields-nested.json
@@ -0,0 +1,25 @@
+{
+  "Comment": "Contains a duplicate field in the payload template. type.$ and type are in conflict.",
+  "StartAt": "Publish to Slack",
+  "States": {
+    "Publish to Slack": {
+      "Type": "Task",
+      "Resource": "arn:aws:lambda:::function:publish-to-slack-lambda",
+      "Parameters": {
+        "slackMessage": {
+          "channel.$": "$.slackErrorChannel",
+          "blocks": [
+            {
+              "type": "section",
+              "text": {
+                "type": "mrkdwn",
+                "type.$": "$.slackMessage.errorMessage"
+              }
+            }
+          ]
+        }
+      },
+      "End": true
+    }
+  }
+}
diff --git a/src/__tests__/definitions/invalid-duplicate-fields.json b/src/__tests__/definitions/invalid-duplicate-fields.json
new file mode 100644
index 0000000..73175a3
--- /dev/null
+++ b/src/__tests__/definitions/invalid-duplicate-fields.json
@@ -0,0 +1,26 @@
+{
+  "Comment": "Contains a duplicate field in the payload template. channel.$ and channel are in conflict.",
+  "StartAt": "Publish to Slack",
+  "States": {
+    "Publish to Slack": {
+      "Type": "Task",
+      "Resource": "arn:aws:lambda:::function:publish-to-slack-lambda",
+      "Parameters": {
+        "slackMessage": {
+          "channel.$": "$.slackErrorChannel",
+          "channel": "conflicting field",
+          "blocks": [
+            {
+              "type": "section",
+              "text": {
+                "type": "mrkdwn",
+                "text.$": "$.slackMessage.errorMessage"
+              }
+            }
+          ]
+        }
+      },
+      "End": true
+    }
+  }
+}