Merge pull request #2936 from github/koesie10/method-modeling-panel-validation

Add model validation in method modeling panel

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

@@ -0,0 +1,145 @@
+import { ModeledMethod } from "../modeled-method";
+import { MethodSignature } from "../method";
+import { assertNever } from "../../common/helpers-pure";
+
+export type ModeledMethodValidationError = {
+  title: string;
+  message: string;
+  actionText: string;
+  index: number;
+};
+
+/**
+ * This method will reset any properties which are not used for the specific type of modeled method.
+ *
+ * It will also set the `provenance` to `manual` since multiple modelings of the same method with a
+ * different provenance are not actually different.
+ *
+ * The returned canonical modeled method should only be used for comparisons. It should not be used
+ * for display purposes, saving the model, or any other purpose which requires the original modeled
+ * method to be preserved.
+ *
+ * @param modeledMethod The modeled method to canonicalize
+ */
+function canonicalizeModeledMethod(
+  modeledMethod: ModeledMethod,
+): ModeledMethod {
+  const methodSignature: MethodSignature = {
+    signature: modeledMethod.signature,
+    packageName: modeledMethod.packageName,
+    typeName: modeledMethod.typeName,
+    methodName: modeledMethod.methodName,
+    methodParameters: modeledMethod.methodParameters,
+  };
+
+  switch (modeledMethod.type) {
+    case "none":
+      return {
+        ...methodSignature,
+        type: "none",
+        input: "",
+        output: "",
+        kind: "",
+        provenance: "manual",
+      };
+    case "source":
+      return {
+        ...methodSignature,
+        type: "source",
+        input: "",
+        output: modeledMethod.output,
+        kind: modeledMethod.kind,
+        provenance: "manual",
+      };
+    case "sink":
+      return {
+        ...methodSignature,
+        type: "sink",
+        input: modeledMethod.input,
+        output: "",
+        kind: modeledMethod.kind,
+        provenance: "manual",
+      };
+    case "summary":
+      return {
+        ...methodSignature,
+        type: "summary",
+        input: modeledMethod.input,
+        output: modeledMethod.output,
+        kind: modeledMethod.kind,
+        provenance: "manual",
+      };
+    case "neutral":
+      return {
+        ...methodSignature,
+        type: "neutral",
+        input: "",
+        output: "",
+        kind: "",
+        provenance: "manual",
+      };
+    default:
+      assertNever(modeledMethod.type);
+  }
+}
+
+export function validateModeledMethods(
+  modeledMethods: ModeledMethod[],
+): ModeledMethodValidationError[] {
+  // Anything that is not modeled will not be saved, so we don't need to validate it
+  const consideredModeledMethods = modeledMethods.filter(
+    (modeledMethod) => modeledMethod.type !== "none",
+  );
+
+  const result: ModeledMethodValidationError[] = [];
+
+  // If the same model is present multiple times, only the first one makes sense, so we should give
+  // an error for any duplicates.
+  const seenModeledMethods = new Set<string>();
+  for (const modeledMethod of consideredModeledMethods) {
+    const canonicalModeledMethod = canonicalizeModeledMethod(modeledMethod);
+    const key = JSON.stringify(
+      canonicalModeledMethod,
+      // This ensures the keys are always in the same order
+      Object.keys(canonicalModeledMethod).sort(),
+    );
+
+    if (seenModeledMethods.has(key)) {
+      result.push({
+        title: "Duplicated classification",
+        message:
+          "This method has two identical or conflicting classifications.",
+        actionText: "Modify or remove the duplicated classification.",
+        index: modeledMethods.indexOf(modeledMethod),
+      });
+    } else {
+      seenModeledMethods.add(key);
+    }
+  }
+
+  const neutralModeledMethod = consideredModeledMethods.find(
+    (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
+
+    result.push({
+      title: "Conflicting classification",
+      message:
+        "This method has a neutral classification, which conflicts with other classifications.",
+      actionText: "Modify or remove the neutral classification.",
+      index: modeledMethods.indexOf(neutralModeledMethod),
+    });
+  }
+
+  // Sort by index so that the errors are always in the same order
+  result.sort((a, b) => a.index - b.index);
+
+  return result;
+}

extensions/ql-vscode/src/stories/method-modeling/MethodModeling.stories.tsx

@@ -69,3 +69,35 @@ MultipleModelingsModeledMultiple.args = {
   showMultipleModels: true,
   modelingStatus: "saved",
 };
+
+export const MultipleModelingsValidationFailedNeutral = Template.bind({});
+MultipleModelingsValidationFailedNeutral.args = {
+  method,
+  modeledMethods: [
+    createModeledMethod(method),
+    createModeledMethod({
+      ...method,
+      type: "neutral",
+    }),
+  ],
+  showMultipleModels: true,
+  modelingStatus: "unsaved",
+};
+
+export const MultipleModelingsValidationFailedDuplicate = Template.bind({});
+MultipleModelingsValidationFailedDuplicate.args = {
+  method,
+  modeledMethods: [
+    createModeledMethod(method),
+    createModeledMethod({
+      ...method,
+      type: "source",
+      input: "",
+      output: "ReturnValue",
+      kind: "remote",
+    }),
+    createModeledMethod(method),
+  ],
+  showMultipleModels: true,
+  modelingStatus: "unsaved",
+};

extensions/ql-vscode/src/view/common/Alert.tsx

@@ -85,11 +85,30 @@ type Props = {
 
   // Inverse the color scheme
   inverse?: boolean;
+
+  /**
+   * Role is used as the ARIA role. "alert" should only be set if the alert requires
+   * the user's immediate attention. "status" should be set if the alert is not
+   * important enough to require the user's immediate attention.
+   *
+   * Can be left out if the alert is not important enough to require the user's
+   * immediate attention. In this case, no ARIA role will be set and the alert
+   * will be read as normal text. The user will not be notified about any changes
+   * to the alert.
+   */
+  role?: "alert" | "status";
 };
 
-export const Alert = ({ type, title, message, actions, inverse }: Props) => {
+export const Alert = ({
+  type,
+  title,
+  message,
+  actions,
+  inverse,
+  role,
+}: Props) => {
   return (
-    <Container type={type} inverse={inverse}>
+    <Container type={type} inverse={inverse} role={role}>
       <Title>
         {getTypeText(type)}: {title}
       </Title>

extensions/ql-vscode/src/view/method-modeling/ModeledMethodAlert.tsx

@@ -0,0 +1,30 @@
+import { ModeledMethodValidationError } from "../../model-editor/shared/validation";
+import TextButton from "../common/TextButton";
+import { Alert } from "../common";
+import * as React from "react";
+import { useCallback } from "react";
+
+type Props = {
+  error: ModeledMethodValidationError;
+  setSelectedIndex: (index: number) => void;
+};
+
+export const ModeledMethodAlert = ({ error, setSelectedIndex }: Props) => {
+  const handleClick = useCallback(() => {
+    setSelectedIndex(error.index);
+  }, [error.index, setSelectedIndex]);
+
+  return (
+    <Alert
+      role="alert"
+      type="error"
+      title={error.title}
+      message={
+        <>
+          {error.message}{" "}
+          <TextButton onClick={handleClick}>{error.actionText}</TextButton>
+        </>
+      }
+    />
+  );
+};

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

@@ -6,6 +6,8 @@ import { styled } from "styled-components";
 import { MethodModelingInputs } from "./MethodModelingInputs";
 import { VSCodeButton } from "@vscode/webview-ui-toolkit/react";
 import { Codicon } from "../common";
+import { validateModeledMethods } from "../../model-editor/shared/validation";
+import { ModeledMethodAlert } from "./ModeledMethodAlert";
 
 export type MultipleModeledMethodsPanelProps = {
   method: Method;
@@ -19,9 +21,14 @@ const Container = styled.div`
   gap: 0.25rem;
 
   padding-bottom: 0.5rem;
+  border-top: 0.05rem solid var(--vscode-panelSection-border);
   border-bottom: 0.05rem solid var(--vscode-panelSection-border);
 `;
 
+const AlertContainer = styled.div`
+  margin-top: 0.5rem;
+`;
+
 const Footer = styled.div`
   display: flex;
   flex-direction: row;
@@ -54,6 +61,11 @@ export const MultipleModeledMethodsPanel = ({
     setSelectedIndex((previousIndex) => previousIndex + 1);
   }, []);
 
+  const validationErrors = useMemo(
+    () => validateModeledMethods(modeledMethods),
+    [modeledMethods],
+  );
+
   const handleAddClick = useCallback(() => {
     const newModeledMethod: ModeledMethod = {
       type: "none",
@@ -110,6 +122,17 @@ export const MultipleModeledMethodsPanel = ({
 
   return (
     <Container>
+      {validationErrors.length > 0 && (
+        <AlertContainer>
+          {validationErrors.map((error, index) => (
+            <ModeledMethodAlert
+              key={index}
+              error={error}
+              setSelectedIndex={setSelectedIndex}
+            />
+          ))}
+        </AlertContainer>
+      )}
       {modeledMethods.length > 0 ? (
         <MethodModelingInputs
           method={method}

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

@@ -252,6 +252,16 @@ describe(MultipleModeledMethodsPanel.name, () => {
       ).toHaveValue("source");
     });
 
+    it("does not show errors", () => {
+      render({
+        method,
+        modeledMethods,
+        onChange,
+      });
+
+      expect(screen.queryByRole("alert")).not.toBeInTheDocument();
+    });
+
     it("can update the first modeling", async () => {
       render({
         method,
@@ -351,6 +361,47 @@ describe(MultipleModeledMethodsPanel.name, () => {
         },
       ]);
     });
+
+    it("shows an error when adding a neutral modeling", async () => {
+      const { rerender } = render({
+        method,
+        modeledMethods,
+        onChange,
+      });
+
+      await userEvent.click(screen.getByLabelText("Add modeling"));
+
+      rerender(
+        <MultipleModeledMethodsPanel
+          method={method}
+          modeledMethods={
+            onChange.mock.calls[onChange.mock.calls.length - 1][0]
+          }
+          onChange={onChange}
+        />,
+      );
+
+      const modelTypeDropdown = screen.getByRole("combobox", {
+        name: "Model type",
+      });
+
+      await userEvent.selectOptions(modelTypeDropdown, "neutral");
+
+      rerender(
+        <MultipleModeledMethodsPanel
+          method={method}
+          modeledMethods={
+            onChange.mock.calls[onChange.mock.calls.length - 1][0]
+          }
+          onChange={onChange}
+        />,
+      );
+
+      expect(screen.getByRole("alert")).toBeInTheDocument();
+      expect(
+        screen.getByText("Error: Conflicting classification"),
+      ).toBeInTheDocument();
+    });
   });
 
   describe("with three modeled methods", () => {
@@ -562,4 +613,56 @@ describe(MultipleModeledMethodsPanel.name, () => {
       ]);
     });
   });
+
+  describe("with duplicate modeled methods", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        ...method,
+      }),
+      createModeledMethod({
+        ...method,
+      }),
+    ];
+
+    it("shows errors", () => {
+      render({
+        method,
+        modeledMethods,
+        onChange,
+      });
+
+      expect(screen.getByRole("alert")).toBeInTheDocument();
+    });
+
+    it("shows the correct error message", async () => {
+      render({
+        method,
+        modeledMethods,
+        onChange,
+      });
+
+      expect(
+        screen.getByText("Error: Duplicated classification"),
+      ).toBeInTheDocument();
+      expect(
+        screen.getByText(
+          "This method has two identical or conflicting classifications.",
+        ),
+      ).toBeInTheDocument();
+
+      expect(screen.getByText("1/2")).toBeInTheDocument();
+
+      const button = screen.getByText(
+        "Modify or remove the duplicated classification.",
+      );
+
+      await userEvent.click(button);
+
+      expect(screen.getByText("2/2")).toBeInTheDocument();
+
+      expect(
+        screen.getByText("Modify or remove the duplicated classification."),
+      ).toBeInTheDocument();
+    });
+  });
 });

extensions/ql-vscode/test/unit-tests/jest.config.ts

@@ -146,7 +146,7 @@ const config: Config = {
   // testLocationInResults: false,
 
   // The glob patterns Jest uses to detect test files
-  testMatch: ["**/*.test.[jt]s"],
+  testMatch: ["**/*.{test,spec}.[jt]s"],
 
   // An array of regexp pattern strings that are matched against all test paths, matched tests are skipped
   // testPathIgnorePatterns: [

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

@@ -0,0 +1,328 @@
+import { validateModeledMethods } from "../../../../src/model-editor/shared/validation";
+import { createModeledMethod } from "../../../factories/model-editor/modeled-method-factories";
+
+describe(validateModeledMethods.name, () => {
+  it("should not give an error with valid modeled methods", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        type: "source",
+        output: "ReturnValue",
+      }),
+      createModeledMethod({
+        type: "sink",
+        input: "Argument[this]",
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toHaveLength(0);
+  });
+
+  it("should not give an error with valid modeled methods and an unmodeled method", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        type: "source",
+        output: "ReturnValue",
+      }),
+      createModeledMethod({
+        type: "sink",
+        input: "Argument[this]",
+      }),
+      createModeledMethod({
+        type: "none",
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toHaveLength(0);
+  });
+
+  it("should not give an error with valid modeled methods and multiple unmodeled methods", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        type: "none",
+      }),
+      createModeledMethod({
+        type: "source",
+        output: "ReturnValue",
+      }),
+      createModeledMethod({
+        type: "sink",
+        input: "Argument[this]",
+      }),
+      createModeledMethod({
+        type: "none",
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toHaveLength(0);
+  });
+
+  it("should not give an error with a single neutral model", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        type: "neutral",
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toHaveLength(0);
+  });
+
+  it("should not give an error with a neutral model and an unmodeled method", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        type: "neutral",
+      }),
+      createModeledMethod({
+        type: "none",
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toHaveLength(0);
+  });
+
+  it("should give an error with exact duplicate modeled methods", () => {
+    const modeledMethods = [createModeledMethod(), createModeledMethod()];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toEqual([
+      {
+        index: 1,
+        title: expect.stringMatching(/duplicate/i),
+        message: expect.stringMatching(/identical/i),
+        actionText: expect.stringMatching(/remove/i),
+      },
+    ]);
+  });
+
+  it("should give an error with duplicate modeled methods with different provenance", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        provenance: "df-generated",
+      }),
+      createModeledMethod({
+        provenance: "manual",
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toEqual([
+      {
+        index: 1,
+        title: expect.stringMatching(/duplicate/i),
+        message: expect.stringMatching(/identical/i),
+        actionText: expect.stringMatching(/remove/i),
+      },
+    ]);
+  });
+
+  it("should give an error with duplicate modeled methods with different source unused fields", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        type: "source",
+        input: "Argument[this]",
+        output: "ReturnValue",
+      }),
+      createModeledMethod({
+        type: "source",
+        input: "Argument[1]",
+        output: "ReturnValue",
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toEqual([
+      {
+        index: 1,
+        title: expect.stringMatching(/duplicate/i),
+        message: expect.stringMatching(/identical/i),
+        actionText: expect.stringMatching(/remove/i),
+      },
+    ]);
+  });
+
+  it("should give an error with duplicate modeled methods with different sink unused fields", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        type: "sink",
+        input: "Argument[this]",
+        output: "ReturnValue",
+      }),
+      createModeledMethod({
+        type: "sink",
+        input: "Argument[this]",
+        output: "Argument[this]",
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toEqual([
+      {
+        index: 1,
+        title: expect.stringMatching(/duplicate/i),
+        message: expect.stringMatching(/identical/i),
+        actionText: expect.stringMatching(/remove/i),
+      },
+    ]);
+  });
+
+  it("should give an error with duplicate modeled methods with different summary unused fields", () => {
+    const supportedTrue = {
+      supported: true,
+    };
+    const supportedFalse = {
+      supported: false,
+    };
+
+    const modeledMethods = [
+      createModeledMethod({
+        type: "sink",
+        input: "Argument[this]",
+        output: "ReturnValue",
+        ...supportedTrue,
+      }),
+      createModeledMethod({
+        type: "sink",
+        input: "Argument[this]",
+        output: "Argument[this]",
+        ...supportedFalse,
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toEqual([
+      {
+        index: 1,
+        title: expect.stringMatching(/duplicate/i),
+        message: expect.stringMatching(/identical/i),
+        actionText: expect.stringMatching(/remove/i),
+      },
+    ]);
+  });
+
+  it("should give an error with duplicate modeled methods with different neutral unused fields", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        type: "neutral",
+        input: "Argument[this]",
+        output: "ReturnValue",
+      }),
+      createModeledMethod({
+        type: "neutral",
+        input: "Argument[1]",
+        output: "Argument[this]",
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toEqual([
+      {
+        index: 1,
+        title: expect.stringMatching(/duplicate/i),
+        message: expect.stringMatching(/identical/i),
+        actionText: expect.stringMatching(/remove/i),
+      },
+    ]);
+  });
+
+  it("should give an error with neutral combined with other models", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        type: "sink",
+      }),
+      createModeledMethod({
+        type: "neutral",
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toEqual([
+      {
+        index: 1,
+        title: expect.stringMatching(/conflicting/i),
+        message: expect.stringMatching(/neutral/i),
+        actionText: expect.stringMatching(/remove/i),
+      },
+    ]);
+  });
+
+  it("should give an error with duplicate neutral combined with other models", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        type: "neutral",
+      }),
+      createModeledMethod({
+        type: "sink",
+      }),
+      createModeledMethod({
+        type: "neutral",
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toEqual([
+      {
+        index: 0,
+        title: expect.stringMatching(/conflicting/i),
+        message: expect.stringMatching(/neutral/i),
+        actionText: expect.stringMatching(/remove/i),
+      },
+      {
+        index: 2,
+        title: expect.stringMatching(/duplicate/i),
+        message: expect.stringMatching(/identical/i),
+        actionText: expect.stringMatching(/remove/i),
+      },
+    ]);
+  });
+
+  it("should include unmodeled methods in the index", () => {
+    const modeledMethods = [
+      createModeledMethod({
+        type: "none",
+      }),
+      createModeledMethod({
+        type: "neutral",
+      }),
+      createModeledMethod({
+        type: "sink",
+      }),
+      createModeledMethod({
+        type: "neutral",
+      }),
+    ];
+
+    const errors = validateModeledMethods(modeledMethods);
+
+    expect(errors).toEqual([
+      {
+        index: 1,
+        title: expect.stringMatching(/conflicting/i),
+        message: expect.stringMatching(/neutral/i),
+        actionText: expect.stringMatching(/remove/i),
+      },
+      {
+        index: 3,
+        title: expect.stringMatching(/duplicate/i),
+        message: expect.stringMatching(/identical/i),
+        actionText: expect.stringMatching(/remove/i),
+      },
+    ]);
+  });
+});