diff --git a/extensions/ql-vscode/src/model-editor/shared/validation.ts b/extensions/ql-vscode/src/model-editor/shared/validation.ts new file mode 100644 index 00000000000..9559398c20c --- /dev/null +++ b/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(); + 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; +} diff --git a/extensions/ql-vscode/src/stories/method-modeling/MethodModeling.stories.tsx b/extensions/ql-vscode/src/stories/method-modeling/MethodModeling.stories.tsx index e611378ee84..1a6ee6c0b09 100644 --- a/extensions/ql-vscode/src/stories/method-modeling/MethodModeling.stories.tsx +++ b/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", +}; diff --git a/extensions/ql-vscode/src/view/common/Alert.tsx b/extensions/ql-vscode/src/view/common/Alert.tsx index 660d601d061..23a53fa6d1e 100644 --- a/extensions/ql-vscode/src/view/common/Alert.tsx +++ b/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 ( - + {getTypeText(type)}: {title} diff --git a/extensions/ql-vscode/src/view/method-modeling/ModeledMethodAlert.tsx b/extensions/ql-vscode/src/view/method-modeling/ModeledMethodAlert.tsx new file mode 100644 index 00000000000..111109d3534 --- /dev/null +++ b/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 ( + + {error.message}{" "} + {error.actionText} + + } + /> + ); +}; diff --git a/extensions/ql-vscode/src/view/method-modeling/MultipleModeledMethodsPanel.tsx b/extensions/ql-vscode/src/view/method-modeling/MultipleModeledMethodsPanel.tsx index 9dda8b9aad1..7befebdad18 100644 --- a/extensions/ql-vscode/src/view/method-modeling/MultipleModeledMethodsPanel.tsx +++ b/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 ( + {validationErrors.length > 0 && ( + + {validationErrors.map((error, index) => ( + + ))} + + )} {modeledMethods.length > 0 ? ( { ).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( + , + ); + + const modelTypeDropdown = screen.getByRole("combobox", { + name: "Model type", + }); + + await userEvent.selectOptions(modelTypeDropdown, "neutral"); + + rerender( + , + ); + + 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(); + }); + }); }); diff --git a/extensions/ql-vscode/test/unit-tests/jest.config.ts b/extensions/ql-vscode/test/unit-tests/jest.config.ts index 7e4cd63ef0b..74c207b2715 100644 --- a/extensions/ql-vscode/test/unit-tests/jest.config.ts +++ b/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: [ diff --git a/extensions/ql-vscode/test/unit-tests/model-editor/shared/validation.test.ts b/extensions/ql-vscode/test/unit-tests/model-editor/shared/validation.test.ts new file mode 100644 index 00000000000..474777544ef --- /dev/null +++ b/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), + }, + ]); + }); +});