Merge pull request #2965 from github/koesie10/modeled-method-validation-neutral

Fix neutral model validation to consider kind

extensions/ql-vscode/src/model-editor/shared/validation.ts

@@ -75,7 +75,7 @@ function canonicalizeModeledMethod(
         type: "neutral",
         input: "",
         output: "",
-        kind: "",
+        kind: modeledMethod.kind,
         provenance: "manual",
       };
     default:
@@ -117,24 +117,40 @@ export function validateModeledMethods(
     }
   }
 
-  const neutralModeledMethod = consideredModeledMethods.find(
+  const neutralModeledMethods = consideredModeledMethods.filter(
     (modeledMethod) => modeledMethod.type === "neutral",
   );
-  const hasNonNeutralModeledMethod = consideredModeledMethods.some(
-    (modeledMethod) => modeledMethod.type !== "neutral",
-  );
 
-  // If there is a neutral model and any other model, that is an error
-  if (neutralModeledMethod && hasNonNeutralModeledMethod) {
-    // Another validation will validate that only one neutral method is present, so we only need
-    // to return an error for the first one
+  const neutralModeledMethodsByKind = new Map<string, ModeledMethod[]>();
+  for (const neutralModeledMethod of neutralModeledMethods) {
+    if (!neutralModeledMethodsByKind.has(neutralModeledMethod.kind)) {
+      neutralModeledMethodsByKind.set(neutralModeledMethod.kind, []);
+    }
+
+    neutralModeledMethodsByKind
+      .get(neutralModeledMethod.kind)
+      ?.push(neutralModeledMethod);
+  }
+
+  for (const [
+    neutralModeledMethodKind,
+    neutralModeledMethods,
+  ] of neutralModeledMethodsByKind) {
+    const conflictingMethods = consideredModeledMethods.filter(
+      (method) => neutralModeledMethodKind === method.type,
+    );
+
+    if (conflictingMethods.length < 1) {
+      continue;
+    }
 
     result.push({
       title: "Conflicting classification",
-      message:
-        "This method has a neutral classification, which conflicts with other classifications.",
+      message: `This method has a neutral ${neutralModeledMethodKind} classification, which conflicts with other ${neutralModeledMethodKind} classifications.`,
       actionText: "Modify or remove the neutral classification.",
-      index: modeledMethods.indexOf(neutralModeledMethod),
+      // Another validation will validate that only one neutral method is present, so we only need
+      // to return an error for the first one
+      index: modeledMethods.indexOf(neutralModeledMethods[0]),
     });
   }
 

extensions/ql-vscode/src/view/method-modeling/__tests__/MultipleModeledMethodsPanel.spec.tsx

@@ -409,6 +409,24 @@ describe(MultipleModeledMethodsPanel.name, () => {
 
       await userEvent.selectOptions(modelTypeDropdown, "neutral");
 
+      expect(screen.queryByRole("alert")).not.toBeInTheDocument();
+
+      rerender(
+        <MultipleModeledMethodsPanel
+          method={method}
+          modeledMethods={
+            onChange.mock.calls[onChange.mock.calls.length - 1][1]
+          }
+          onChange={onChange}
+        />,
+      );
+
+      const kindDropdown = screen.getByRole("combobox", {
+        name: "Kind",
+      });
+
+      await userEvent.selectOptions(kindDropdown, "source");
+
       rerender(
         <MultipleModeledMethodsPanel
           method={method}

extensions/ql-vscode/test/unit-tests/model-editor/shared/validation.test.ts

@@ -246,6 +246,7 @@ describe(validateModeledMethods.name, () => {
       }),
       createModeledMethod({
         type: "neutral",
+        kind: "sink",
       }),
     ];
 
@@ -261,16 +262,34 @@ describe(validateModeledMethods.name, () => {
     ]);
   });
 
-  it("should give an error with duplicate neutral combined with other models", () => {
+  it("should not give an error with other neutral combined with other models", () => {
     const modeledMethods = [
-      createModeledMethod({
-        type: "neutral",
-      }),
       createModeledMethod({
         type: "sink",
       }),
       createModeledMethod({
         type: "neutral",
+        kind: "summary",
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toEqual([]);
+  });
+
+  it("should give an error with duplicate neutral combined with other models", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        type: "neutral",
+        kind: "summary",
+      }),
+      createModeledMethod({
+        type: "summary",
+      }),
+      createModeledMethod({
+        type: "neutral",
+        kind: "summary",
       }),
     ];
 
@@ -299,12 +318,14 @@ describe(validateModeledMethods.name, () => {
       }),
       createModeledMethod({
         type: "neutral",
+        kind: "sink",
       }),
       createModeledMethod({
         type: "sink",
       }),
       createModeledMethod({
         type: "neutral",
+        kind: "sink",
       }),
     ];