From 068d354d23cfc7525e91b6dd9e13e0cee5ae4bd0 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sat, 15 Jun 2024 02:39:30 +0200 Subject: [PATCH 01/43] Refactor move validation rules to own package --- .../server-common/src/providers/validation/profiles.ts | 3 +-- .../validation/{ => rules}/MissingPropertyValidation.ts | 2 +- .../validation/{ => rules}/WorkflowOutputLabelValidation.ts | 4 ++-- .../server-common/src/providers/validation/rules/index.ts | 4 ++++ 4 files changed, 8 insertions(+), 5 deletions(-) rename server/packages/server-common/src/providers/validation/{ => rules}/MissingPropertyValidation.ts (92%) rename server/packages/server-common/src/providers/validation/{ => rules}/WorkflowOutputLabelValidation.ts (89%) create mode 100644 server/packages/server-common/src/providers/validation/rules/index.ts diff --git a/server/packages/server-common/src/providers/validation/profiles.ts b/server/packages/server-common/src/providers/validation/profiles.ts index 84c79a6..9097d3f 100644 --- a/server/packages/server-common/src/providers/validation/profiles.ts +++ b/server/packages/server-common/src/providers/validation/profiles.ts @@ -1,6 +1,5 @@ import { DiagnosticSeverity, ValidationProfile, ValidationRule } from "../../languageTypes"; -import { MissingPropertyValidationRule } from "./MissingPropertyValidation"; -import { WorkflowOutputLabelValidation } from "./WorkflowOutputLabelValidation"; +import { MissingPropertyValidationRule, WorkflowOutputLabelValidation } from "./rules"; /** * The default validation profile. diff --git a/server/packages/server-common/src/providers/validation/MissingPropertyValidation.ts b/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts similarity index 92% rename from server/packages/server-common/src/providers/validation/MissingPropertyValidation.ts rename to server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts index c0c702d..aa90ee0 100644 --- a/server/packages/server-common/src/providers/validation/MissingPropertyValidation.ts +++ b/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts @@ -1,5 +1,5 @@ import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; -import { ValidationRule, WorkflowDocument } from "../../languageTypes"; +import { ValidationRule, WorkflowDocument } from "../../../languageTypes"; /** * Validation rule to check that a particular property exists in a workflow. diff --git a/server/packages/server-common/src/providers/validation/WorkflowOutputLabelValidation.ts b/server/packages/server-common/src/providers/validation/rules/WorkflowOutputLabelValidation.ts similarity index 89% rename from server/packages/server-common/src/providers/validation/WorkflowOutputLabelValidation.ts rename to server/packages/server-common/src/providers/validation/rules/WorkflowOutputLabelValidation.ts index 1c54742..702ade3 100644 --- a/server/packages/server-common/src/providers/validation/WorkflowOutputLabelValidation.ts +++ b/server/packages/server-common/src/providers/validation/rules/WorkflowOutputLabelValidation.ts @@ -1,10 +1,10 @@ import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; -import { ValidationRule, WorkflowDocument } from "../../languageTypes"; +import { ValidationRule, WorkflowDocument } from "../../../languageTypes"; /** * Validation rule to check that all defined `workflow_outputs` have a `label`. */ -export class WorkflowOutputLabelValidation implements ValidationRule { +export class WorkflowOutputLabelValidationRule implements ValidationRule { constructor(readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error) {} validate(workflowDocument: WorkflowDocument): Promise { diff --git a/server/packages/server-common/src/providers/validation/rules/index.ts b/server/packages/server-common/src/providers/validation/rules/index.ts new file mode 100644 index 0000000..8fb9391 --- /dev/null +++ b/server/packages/server-common/src/providers/validation/rules/index.ts @@ -0,0 +1,4 @@ +import { MissingPropertyValidationRule } from "./MissingPropertyValidation"; +import { WorkflowOutputLabelValidationRule } from "./WorkflowOutputLabelValidation"; + +export { MissingPropertyValidationRule, WorkflowOutputLabelValidationRule as WorkflowOutputLabelValidation }; From bfeff489b17db24a3cb4ecbf93a18f94d8515fc9 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sat, 15 Jun 2024 14:15:29 +0200 Subject: [PATCH 02/43] Set default severity for MissingPropertyValidationRule --- .../src/providers/validation/rules/MissingPropertyValidation.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts b/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts index aa90ee0..b3e0aef 100644 --- a/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts +++ b/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts @@ -9,7 +9,7 @@ import { ValidationRule, WorkflowDocument } from "../../../languageTypes"; export class MissingPropertyValidationRule implements ValidationRule { constructor( readonly nodePath: string, - readonly severity?: DiagnosticSeverity | undefined + readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error ) {} validate(workflowDocument: WorkflowDocument): Promise { From 24962e3e9b327527fae635b6d2bc84c0387339af Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sat, 15 Jun 2024 14:31:51 +0200 Subject: [PATCH 03/43] Add MissingPropertyValidationRule tests --- .../tests/unit/customValidationRules.test.ts | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts index 95d53ef..b1f66be 100644 --- a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts +++ b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts @@ -1,5 +1,9 @@ +import { DiagnosticSeverity } from "@gxwf/server-common/src/languageTypes"; +import { + MissingPropertyValidationRule, + WorkflowOutputLabelValidation, +} from "@gxwf/server-common/src/providers/validation/rules"; import { createNativeWorkflowDocument } from "../testHelpers"; -import { WorkflowOutputLabelValidation } from "@gxwf/server-common/src/providers/validation/WorkflowOutputLabelValidation"; import { TestWorkflowProvider } from "../testWorkflowProvider"; describe("Custom Validation Rules", () => { @@ -41,4 +45,33 @@ describe("Custom Validation Rules", () => { }); }); }); + + describe("MissingPropertyValidation Rule", () => { + let rule: MissingPropertyValidationRule; + + beforeEach(() => { + rule = new MissingPropertyValidationRule("release"); + }); + + it("should not provide diagnostics when the property is present", async () => { + const wfContents = `{ + "a_galaxy_workflow": "true", + "release": "0.1", + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(0); + }); + + it("should provide error diagnostics when the property is missing", async () => { + const wfContents = `{ + "a_galaxy_workflow": "true", + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].message).toBe('Missing property "release".'); + expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); + }); + }); }); From 8b2966ac19a4a7b7e14ac0f1bb5e957a1a0594c0 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sat, 15 Jun 2024 14:33:00 +0200 Subject: [PATCH 04/43] add more missing properties rules to IWCValidationProfile --- .../server-common/src/providers/validation/profiles.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/packages/server-common/src/providers/validation/profiles.ts b/server/packages/server-common/src/providers/validation/profiles.ts index 9097d3f..98281d9 100644 --- a/server/packages/server-common/src/providers/validation/profiles.ts +++ b/server/packages/server-common/src/providers/validation/profiles.ts @@ -26,7 +26,10 @@ export class IWCValidationProfile implements ValidationProfile { protected static readonly ID = "iwc"; protected static readonly RULES = new Set([ new MissingPropertyValidationRule("release"), + new MissingPropertyValidationRule("creator"), + new MissingPropertyValidationRule("license"), new WorkflowOutputLabelValidation(DiagnosticSeverity.Error), + // Add more custom rules here... ]); public get id(): string { From e0bfe10ebea8b8e6a2e89b04d898ddce175f81fc Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sat, 15 Jun 2024 18:30:42 +0200 Subject: [PATCH 05/43] Increase test coverage for MissingPropertyValidationRule --- .../tests/unit/customValidationRules.test.ts | 145 ++++++++++++++++++ 1 file changed, 145 insertions(+) diff --git a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts index b1f66be..2ea9718 100644 --- a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts +++ b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts @@ -73,5 +73,150 @@ describe("Custom Validation Rules", () => { expect(diagnostics[0].message).toBe('Missing property "release".'); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); + + it("should provide error diagnostics when the property is present but has no value", async () => { + const wfContents = `{ + "a_galaxy_workflow": "true", + "release": "", + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].message).toBe('Missing value in property "release".'); + expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); + }); + + describe("when valueRequired is false", () => { + beforeEach(() => { + rule = new MissingPropertyValidationRule("release", false); + }); + + it("should not provide diagnostics when the property is present", async () => { + const wfContents = `{ + "a_galaxy_workflow": "true", + "release": "0.1", + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(0); + }); + + it("should provide diagnostics when the property is missing", async () => { + const wfContents = `{ + "a_galaxy_workflow": "true", + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].message).toBe('Missing property "release".'); + expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); + }); + + it("should not provide diagnostics when the property is present but has no value", async () => { + const wfContents = `{ + "a_galaxy_workflow": "true", + "release": "", + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(0); + }); + }); + + describe("when the property is an array", () => { + beforeEach(() => { + rule = new MissingPropertyValidationRule("creator"); + }); + + it("should not provide diagnostics when the property has a value", async () => { + const wfContents = `{ + "a_galaxy_workflow": "true", + "creator": [{ "name": "John Doe" }], + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(0); + }); + + it("should provide diagnostics when the property is present but has empty value", async () => { + const wfContents = `{ + "a_galaxy_workflow": "true", + "creator": [], + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].message).toBe('Missing value in property "creator".'); + expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); + }); + }); + + describe("when the property is an object", () => { + beforeEach(() => { + rule = new MissingPropertyValidationRule("steps"); + }); + + it("should not provide diagnostics when the property has a value", async () => { + const wfContents = `{ + "a_galaxy_workflow": "true", + "steps": { "0": { "tool_id": "tool1" } }, + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(0); + }); + + it("should provide diagnostics when the property is present but has empty value", async () => { + const wfContents = `{ + "a_galaxy_workflow": "true", + "steps": {}, + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].message).toBe('Missing value in property "steps".'); + expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); + }); + }); + + describe("when the property is nested", () => { + beforeEach(() => { + rule = new MissingPropertyValidationRule("steps/0/tool_id"); + }); + + it("should not provide diagnostics when the property has a value", async () => { + const wfContents = `{ + "a_galaxy_workflow": "true", + "steps": { "0": { "tool_id": "tool1" } }, + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(0); + }); + + it("should provide diagnostics when the property is missing", async () => { + const wfContents = `{ + "a_galaxy_workflow": "true", + "steps": { "0": {} }, + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].message).toBe('Missing property "steps/0/tool_id".'); + expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); + }); + + it("should provide diagnostics when the property is present but has empty value", async () => { + const wfContents = `{ + "a_galaxy_workflow": "true", + "steps": { "0": { "tool_id": "" } }, + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].message).toBe('Missing value in property "steps/0/tool_id".'); + expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); + }); + }); }); }); From 23fe7092b1027391f3c58e4ba80ee69eb8ed48c3 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sat, 15 Jun 2024 18:31:19 +0200 Subject: [PATCH 06/43] Add isNodeEmpty method to ASTNodeManager class --- .../packages/server-common/src/ast/nodeManager.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/server/packages/server-common/src/ast/nodeManager.ts b/server/packages/server-common/src/ast/nodeManager.ts index e9bbdd6..034c92a 100644 --- a/server/packages/server-common/src/ast/nodeManager.ts +++ b/server/packages/server-common/src/ast/nodeManager.ts @@ -190,4 +190,19 @@ export class ASTNodeManager { doVisit(this.root); } } + + public isNodeEmpty(node: ASTNode): boolean { + switch (node.type) { + case "object": + return node.properties.length === 0; + case "array": + return node.items.length === 0; + case "property": + return !node.valueNode || this.isNodeEmpty(node.valueNode); + case "string": + return node.value === ""; + default: + return false; + } + } } From 0380ac83c5c8c70929b88ff0b0facc9782bb1ccf Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sat, 15 Jun 2024 18:31:33 +0200 Subject: [PATCH 07/43] Add valueRequired parameter to MissingPropertyValidationRule --- .../rules/MissingPropertyValidation.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts b/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts index b3e0aef..03327c1 100644 --- a/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts +++ b/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts @@ -5,10 +5,16 @@ import { ValidationRule, WorkflowDocument } from "../../../languageTypes"; * Validation rule to check that a particular property exists in a workflow. * The property can be specified by a path, i.e: "prop1/prop2" will check * that 'prop1' contains a 'prop2' property defined. + * + * By default, the rule will also check that the property has a value, but this + * can be disabled by setting the `valueRequired` parameter to `false`. If the + * property is an object or an array, the rule will also check that it has at + * least one property or item. */ export class MissingPropertyValidationRule implements ValidationRule { constructor( readonly nodePath: string, + private valueRequired: boolean = true, readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error ) {} @@ -22,6 +28,17 @@ export class MissingPropertyValidationRule implements ValidationRule { severity: this.severity, }); } + + if (this.valueRequired && targetNode) { + const missingValue = workflowDocument.nodeManager.isNodeEmpty(targetNode); + if (missingValue) { + result.push({ + message: `Missing value in property "${this.nodePath}".`, + range: workflowDocument.nodeManager.getNodeRange(targetNode), + severity: this.severity, + }); + } + } return Promise.resolve(result); } } From fe09ee211dd7cfa8dcd1fd922b23081e22416674 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sat, 15 Jun 2024 19:15:38 +0200 Subject: [PATCH 08/43] Refactor MissingPropertyValidationRule to accept custom error messages --- .../tests/unit/customValidationRules.test.ts | 24 +++++++++++++++++++ .../src/providers/validation/profiles.ts | 21 +++++++++++++--- .../rules/MissingPropertyValidation.ts | 7 +++--- 3 files changed, 46 insertions(+), 6 deletions(-) diff --git a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts index 2ea9718..176565d 100644 --- a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts +++ b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts @@ -86,6 +86,30 @@ describe("Custom Validation Rules", () => { expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); + it("should provide warning diagnostics when the property is missing and severity is set to warning", async () => { + rule = new MissingPropertyValidationRule("release", true, DiagnosticSeverity.Warning); + const wfContents = `{ + "a_galaxy_workflow": "true", + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].message).toBe('Missing property "release".'); + expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Warning); + }); + + it("should display a custom message when provided", async () => { + rule = new MissingPropertyValidationRule("release", true, DiagnosticSeverity.Warning, "Custom message"); + const wfContents = `{ + "a_galaxy_workflow": "true", + }`; + const wfDocument = createNativeWorkflowDocument(wfContents); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].message).toBe("Custom message"); + expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Warning); + }); + describe("when valueRequired is false", () => { beforeEach(() => { rule = new MissingPropertyValidationRule("release", false); diff --git a/server/packages/server-common/src/providers/validation/profiles.ts b/server/packages/server-common/src/providers/validation/profiles.ts index 98281d9..2a260ef 100644 --- a/server/packages/server-common/src/providers/validation/profiles.ts +++ b/server/packages/server-common/src/providers/validation/profiles.ts @@ -25,9 +25,24 @@ export class BasicValidationProfile implements ValidationProfile { export class IWCValidationProfile implements ValidationProfile { protected static readonly ID = "iwc"; protected static readonly RULES = new Set([ - new MissingPropertyValidationRule("release"), - new MissingPropertyValidationRule("creator"), - new MissingPropertyValidationRule("license"), + new MissingPropertyValidationRule( + "release", + true, + DiagnosticSeverity.Error, + "The workflow must have a release version." + ), + new MissingPropertyValidationRule( + "creator", + true, + DiagnosticSeverity.Error, + "The workflow does not specify a creator." + ), + new MissingPropertyValidationRule( + "license", + true, + DiagnosticSeverity.Error, + "The workflow does not specify a license." + ), new WorkflowOutputLabelValidation(DiagnosticSeverity.Error), // Add more custom rules here... ]); diff --git a/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts b/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts index 03327c1..3bd4999 100644 --- a/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts +++ b/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts @@ -13,9 +13,10 @@ import { ValidationRule, WorkflowDocument } from "../../../languageTypes"; */ export class MissingPropertyValidationRule implements ValidationRule { constructor( - readonly nodePath: string, + private nodePath: string, private valueRequired: boolean = true, - readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error + private severity: DiagnosticSeverity = DiagnosticSeverity.Error, + private message?: string ) {} validate(workflowDocument: WorkflowDocument): Promise { @@ -23,7 +24,7 @@ export class MissingPropertyValidationRule implements ValidationRule { const targetNode = workflowDocument.nodeManager.getNodeFromPath(this.nodePath); if (!targetNode) { result.push({ - message: `Missing property "${this.nodePath}".`, + message: this.message ?? `Missing property "${this.nodePath}".`, range: workflowDocument.nodeManager.getDefaultRange(), severity: this.severity, }); From ff9ff91176b6d8b0d966e6c3ebb97b38b92f9449 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sun, 16 Jun 2024 00:10:43 +0200 Subject: [PATCH 09/43] Refactor and improve validation rules package Support custom profiles tailored for each language service. --- .../src/languageService.ts | 6 ++ server/gx-workflow-ls-format2/src/profiles.ts | 19 +++++ .../src/languageService.ts | 6 ++ server/gx-workflow-ls-native/src/profiles.ts | 19 +++++ .../server-common/src/languageTypes.ts | 39 ++++++++-- .../src/providers/validation/profiles.ts | 75 ++++++------------- server/packages/server-common/src/server.ts | 4 +- 7 files changed, 107 insertions(+), 61 deletions(-) create mode 100644 server/gx-workflow-ls-format2/src/profiles.ts create mode 100644 server/gx-workflow-ls-native/src/profiles.ts diff --git a/server/gx-workflow-ls-format2/src/languageService.ts b/server/gx-workflow-ls-format2/src/languageService.ts index edef90a..6b83b97 100644 --- a/server/gx-workflow-ls-format2/src/languageService.ts +++ b/server/gx-workflow-ls-format2/src/languageService.ts @@ -18,6 +18,7 @@ import { TYPES as YAML_TYPES } from "@gxwf/yaml-language-service/src/inversify.c import { YAMLLanguageService } from "@gxwf/yaml-language-service/src/yamlLanguageService"; import { inject, injectable } from "inversify"; import { GxFormat2WorkflowDocument } from "./gxFormat2WorkflowDocument"; +import { GxFormat2IWCValidationProfile } from "./profiles"; import { GalaxyWorkflowFormat2SchemaLoader } from "./schema"; import { GxFormat2CompletionService } from "./services/completionService"; import { GxFormat2HoverService } from "./services/hoverService"; @@ -77,6 +78,11 @@ export class GxFormat2WorkflowLanguageServiceImpl return this._completionService.doComplete(documentContext, position); } + protected override initializeValidationProfiles(): void { + super.initializeValidationProfiles(); + this.validationProfiles.set("iwc", new GxFormat2IWCValidationProfile()); + } + protected override async doValidation(documentContext: GxFormat2WorkflowDocument): Promise { const diagnostics = await this._yamlLanguageService.doValidation(documentContext.yamlDocument); for (const validator of this._validationServices) { diff --git a/server/gx-workflow-ls-format2/src/profiles.ts b/server/gx-workflow-ls-format2/src/profiles.ts new file mode 100644 index 0000000..f154f34 --- /dev/null +++ b/server/gx-workflow-ls-format2/src/profiles.ts @@ -0,0 +1,19 @@ +import { DiagnosticSeverity, ValidationProfile, ValidationRule } from "@gxwf/server-common/src/languageTypes"; +import { COMMON_IWC_WORKFLOW_RULES } from "@gxwf/server-common/src/providers/validation/profiles"; +import { WorkflowOutputLabelValidation } from "@gxwf/server-common/src/providers/validation/rules"; + +/** + * *Intergalactic Workflow Commission* (IWC) validation profile for Native Galaxy workflows. + * Defines custom validation rules to comply with the IWC best practices guidelines. + */ +export class GxFormat2IWCValidationProfile implements ValidationProfile { + protected static readonly RULES = new Set([ + ...COMMON_IWC_WORKFLOW_RULES, + new WorkflowOutputLabelValidation(DiagnosticSeverity.Error), + // Add more custom rules here... + ]); + + public get rules(): Set { + return GxFormat2IWCValidationProfile.RULES; + } +} diff --git a/server/gx-workflow-ls-native/src/languageService.ts b/server/gx-workflow-ls-native/src/languageService.ts index 47cb073..58cd69e 100644 --- a/server/gx-workflow-ls-native/src/languageService.ts +++ b/server/gx-workflow-ls-native/src/languageService.ts @@ -25,6 +25,7 @@ import { } from "vscode-json-languageservice"; import NativeWorkflowSchema from "../../../workflow-languages/schemas/native.schema.json"; import { NativeWorkflowDocument } from "./nativeWorkflowDocument"; +import { NativeIWCValidationProfile } from "./profiles"; const LANGUAGE_ID = "galaxyworkflow"; @@ -86,6 +87,11 @@ export class NativeWorkflowLanguageServiceImpl return completionResult; } + protected override initializeValidationProfiles(): void { + super.initializeValidationProfiles(); + this.validationProfiles.set("iwc", new NativeIWCValidationProfile()); + } + protected override async doValidation(workflowDocument: NativeWorkflowDocument): Promise { const nativeWorkflowDocument = workflowDocument as NativeWorkflowDocument; const schemaValidationResults = await this._jsonLanguageService.doValidation( diff --git a/server/gx-workflow-ls-native/src/profiles.ts b/server/gx-workflow-ls-native/src/profiles.ts new file mode 100644 index 0000000..d4521bb --- /dev/null +++ b/server/gx-workflow-ls-native/src/profiles.ts @@ -0,0 +1,19 @@ +import { DiagnosticSeverity, ValidationProfile, ValidationRule } from "@gxwf/server-common/src/languageTypes"; +import { COMMON_IWC_WORKFLOW_RULES } from "@gxwf/server-common/src/providers/validation/profiles"; +import { WorkflowOutputLabelValidation } from "@gxwf/server-common/src/providers/validation/rules"; + +/** + * *Intergalactic Workflow Commission* (IWC) validation profile for Native Galaxy workflows. + * Defines custom validation rules to comply with the IWC best practices guidelines. + */ +export class NativeIWCValidationProfile implements ValidationProfile { + protected static readonly RULES = new Set([ + ...COMMON_IWC_WORKFLOW_RULES, + new WorkflowOutputLabelValidation(DiagnosticSeverity.Error), + // Add more custom rules here... + ]); + + public get rules(): Set { + return NativeIWCValidationProfile.RULES; + } +} diff --git a/server/packages/server-common/src/languageTypes.ts b/server/packages/server-common/src/languageTypes.ts index 7e49115..9d969c1 100644 --- a/server/packages/server-common/src/languageTypes.ts +++ b/server/packages/server-common/src/languageTypes.ts @@ -67,6 +67,7 @@ import { ASTNodeManager } from "./ast/nodeManager"; import { ConfigService } from "./configService"; import { WorkflowDocument } from "./models/workflowDocument"; import { WorkflowTestsDocument } from "./models/workflowTestsDocument"; +import { NoOpValidationProfile } from "./providers/validation/profiles"; export { CleanWorkflowContentsParams, @@ -150,13 +151,12 @@ export interface ValidationRule { validate(documentContext: DocumentContext): Promise; } +export type ValidationProfileIdentifier = "basic" | "iwc"; + /** * Interface representing a validation profile which contains a set of custom rules. */ export interface ValidationProfile { - /** The unique identifier of this validation profile. */ - get id(): string; - /** The set of rules defining this validation profile. */ get rules(): Set; } @@ -197,7 +197,8 @@ export interface LanguageService { * Validates the document and reports all the diagnostics found. * An optional validation profile can be used to provide additional custom diagnostics. */ - validate(documentContext: T, useProfile?: ValidationProfile): Promise; + validate(documentContext: T, useProfile?: ValidationProfileIdentifier): Promise; + getValidationProfile(profileId: ValidationProfileIdentifier): ValidationProfile; setServer(server: GalaxyWorkflowLanguageServer): void; } @@ -208,8 +209,12 @@ export interface LanguageService { */ @injectable() export abstract class LanguageServiceBase implements LanguageService { - constructor(@unmanaged() public readonly languageId: string) {} protected server?: GalaxyWorkflowLanguageServer; + protected validationProfiles = new Map(); + + constructor(@unmanaged() public readonly languageId: string) { + this.initializeValidationProfiles(); + } public abstract parseDocument(document: TextDocument): T; public abstract format(document: TextDocument, range: Range, options: FormattingOptions): TextEdit[]; @@ -220,14 +225,26 @@ export abstract class LanguageServiceBase implements /** Performs basic syntax and semantic validation based on the document schema. */ protected abstract doValidation(documentContext: T): Promise; + /** + * Initializes the validation profiles for this language service. + * Subclasses should override this method to provide custom validation profiles. + * The default implementation does nothing. + */ + protected initializeValidationProfiles(): void { + const defaultProfile = new NoOpValidationProfile(); + this.validationProfiles.set("basic", defaultProfile); + this.validationProfiles.set("iwc", defaultProfile); + } + /** * Validates the document and reports all the diagnostics found. * An optional validation profile can be used to provide additional custom diagnostics. */ - public async validate(documentContext: T, useProfile?: ValidationProfile): Promise { + public async validate(documentContext: T, useProfile?: ValidationProfileIdentifier): Promise { const diagnostics = await this.doValidation(documentContext); if (useProfile) { - useProfile.rules.forEach(async (validationRule) => { + const profile = this.getValidationProfile(useProfile); + profile.rules.forEach(async (validationRule) => { const contributedDiagnostics = await validationRule.validate(documentContext); diagnostics.push(...contributedDiagnostics); }); @@ -235,6 +252,14 @@ export abstract class LanguageServiceBase implements return diagnostics; } + public getValidationProfile(profileId: ValidationProfileIdentifier): ValidationProfile { + const profile = this.validationProfiles.get(profileId); + if (!profile) { + throw new Error(`Validation profile not found for id: ${profileId}`); + } + return profile; + } + public setServer(server: GalaxyWorkflowLanguageServer): void { this.server = server; } diff --git a/server/packages/server-common/src/providers/validation/profiles.ts b/server/packages/server-common/src/providers/validation/profiles.ts index 2a260ef..051ec56 100644 --- a/server/packages/server-common/src/providers/validation/profiles.ts +++ b/server/packages/server-common/src/providers/validation/profiles.ts @@ -1,63 +1,36 @@ import { DiagnosticSeverity, ValidationProfile, ValidationRule } from "../../languageTypes"; -import { MissingPropertyValidationRule, WorkflowOutputLabelValidation } from "./rules"; +import { MissingPropertyValidationRule } from "./rules"; /** - * The default validation profile. + * The *NoOp* validation profile. * It doesn't apply any additional custom rules. */ -export class BasicValidationProfile implements ValidationProfile { - protected static readonly ID = "basic"; +export class NoOpValidationProfile implements ValidationProfile { protected static readonly NO_RULES = new Set([]); - public get id(): string { - return BasicValidationProfile.ID; - } - - public get rules(): Set { - return BasicValidationProfile.NO_RULES; - } -} - -/** - * *Intergalactic Workflow Commission* (IWC) validation profile. - * Defines custom validation rules to comply with the IWC best practices guidelines. - */ -export class IWCValidationProfile implements ValidationProfile { - protected static readonly ID = "iwc"; - protected static readonly RULES = new Set([ - new MissingPropertyValidationRule( - "release", - true, - DiagnosticSeverity.Error, - "The workflow must have a release version." - ), - new MissingPropertyValidationRule( - "creator", - true, - DiagnosticSeverity.Error, - "The workflow does not specify a creator." - ), - new MissingPropertyValidationRule( - "license", - true, - DiagnosticSeverity.Error, - "The workflow does not specify a license." - ), - new WorkflowOutputLabelValidation(DiagnosticSeverity.Error), - // Add more custom rules here... - ]); - - public get id(): string { - return IWCValidationProfile.ID; - } - public get rules(): Set { - return IWCValidationProfile.RULES; + return NoOpValidationProfile.NO_RULES; } } -/** Contains all the available validation profiles. */ -export const ValidationProfiles = new Map([ - ["basic", new BasicValidationProfile()], - ["iwc", new IWCValidationProfile()], +export const COMMON_IWC_WORKFLOW_RULES = new Set([ + new MissingPropertyValidationRule( + "release", + true, + DiagnosticSeverity.Error, + "The workflow must have a release version." + ), + new MissingPropertyValidationRule( + "creator", + true, + DiagnosticSeverity.Error, + "The workflow does not specify a creator." + ), + new MissingPropertyValidationRule( + "license", + true, + DiagnosticSeverity.Error, + "The workflow does not specify a license." + ), + // Add more common custom rules here... ]); diff --git a/server/packages/server-common/src/server.ts b/server/packages/server-common/src/server.ts index c712918..4becb7c 100644 --- a/server/packages/server-common/src/server.ts +++ b/server/packages/server-common/src/server.ts @@ -26,7 +26,6 @@ import { inject, injectable } from "inversify"; import { ConfigService } from "./configService"; import { CompletionHandler } from "./providers/completionHandler"; import { ServerEventHandler } from "./providers/handler"; -import { ValidationProfiles } from "./providers/validation/profiles"; @injectable() export class GalaxyWorkflowLanguageServerImpl implements GalaxyWorkflowLanguageServer { @@ -144,9 +143,8 @@ export class GalaxyWorkflowLanguageServerImpl implements GalaxyWorkflowLanguageS return; } const settings = await this.configService.getDocumentSettings(documentContext.textDocument.uri); - const validationProfile = ValidationProfiles.get(settings.validation.profile); const languageService = this.getLanguageServiceById(documentContext.languageId); - languageService.validate(documentContext, validationProfile).then((diagnostics) => { + languageService.validate(documentContext, settings.validation.profile).then((diagnostics) => { this.connection.sendDiagnostics({ uri: documentContext.textDocument.uri, diagnostics }); }); } From 3e03dd94e4f53aa7e59ffd63110e3cb2e29a24a1 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sun, 16 Jun 2024 01:04:47 +0200 Subject: [PATCH 10/43] Display profile name on diagnostics + refactoring --- server/gx-workflow-ls-format2/src/profiles.ts | 12 ++--- server/gx-workflow-ls-native/src/profiles.ts | 12 ++--- .../tests/unit/customValidationRules.test.ts | 6 +-- .../server-common/src/languageTypes.ts | 6 +++ .../src/providers/validation/profiles.ts | 51 +++++++++++-------- .../src/providers/validation/rules/index.ts | 2 +- 6 files changed, 51 insertions(+), 38 deletions(-) diff --git a/server/gx-workflow-ls-format2/src/profiles.ts b/server/gx-workflow-ls-format2/src/profiles.ts index f154f34..9e69dab 100644 --- a/server/gx-workflow-ls-format2/src/profiles.ts +++ b/server/gx-workflow-ls-format2/src/profiles.ts @@ -1,15 +1,13 @@ -import { DiagnosticSeverity, ValidationProfile, ValidationRule } from "@gxwf/server-common/src/languageTypes"; -import { COMMON_IWC_WORKFLOW_RULES } from "@gxwf/server-common/src/providers/validation/profiles"; -import { WorkflowOutputLabelValidation } from "@gxwf/server-common/src/providers/validation/rules"; +import { ValidationRule } from "@gxwf/server-common/src/languageTypes"; +import { IWCValidationProfile } from "@gxwf/server-common/src/providers/validation/profiles"; /** - * *Intergalactic Workflow Commission* (IWC) validation profile for Native Galaxy workflows. + * *Intergalactic Workflow Commission* (IWC) validation profile for gxformat2 Galaxy workflows. * Defines custom validation rules to comply with the IWC best practices guidelines. */ -export class GxFormat2IWCValidationProfile implements ValidationProfile { +export class GxFormat2IWCValidationProfile extends IWCValidationProfile { protected static readonly RULES = new Set([ - ...COMMON_IWC_WORKFLOW_RULES, - new WorkflowOutputLabelValidation(DiagnosticSeverity.Error), + ...super.RULES, // Add more custom rules here... ]); diff --git a/server/gx-workflow-ls-native/src/profiles.ts b/server/gx-workflow-ls-native/src/profiles.ts index d4521bb..49ba428 100644 --- a/server/gx-workflow-ls-native/src/profiles.ts +++ b/server/gx-workflow-ls-native/src/profiles.ts @@ -1,15 +1,15 @@ -import { DiagnosticSeverity, ValidationProfile, ValidationRule } from "@gxwf/server-common/src/languageTypes"; -import { COMMON_IWC_WORKFLOW_RULES } from "@gxwf/server-common/src/providers/validation/profiles"; -import { WorkflowOutputLabelValidation } from "@gxwf/server-common/src/providers/validation/rules"; +import { DiagnosticSeverity, ValidationRule } from "@gxwf/server-common/src/languageTypes"; +import { IWCValidationProfile } from "@gxwf/server-common/src/providers/validation/profiles"; +import { WorkflowOutputLabelValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; /** * *Intergalactic Workflow Commission* (IWC) validation profile for Native Galaxy workflows. * Defines custom validation rules to comply with the IWC best practices guidelines. */ -export class NativeIWCValidationProfile implements ValidationProfile { +export class NativeIWCValidationProfile extends IWCValidationProfile { protected static readonly RULES = new Set([ - ...COMMON_IWC_WORKFLOW_RULES, - new WorkflowOutputLabelValidation(DiagnosticSeverity.Error), + ...super.RULES, + new WorkflowOutputLabelValidationRule(DiagnosticSeverity.Error), // Add more custom rules here... ]); diff --git a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts index 176565d..b2db11d 100644 --- a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts +++ b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts @@ -1,17 +1,17 @@ import { DiagnosticSeverity } from "@gxwf/server-common/src/languageTypes"; import { MissingPropertyValidationRule, - WorkflowOutputLabelValidation, + WorkflowOutputLabelValidationRule, } from "@gxwf/server-common/src/providers/validation/rules"; import { createNativeWorkflowDocument } from "../testHelpers"; import { TestWorkflowProvider } from "../testWorkflowProvider"; describe("Custom Validation Rules", () => { describe("WorkflowOutputLabelValidation Rule", () => { - let rule: WorkflowOutputLabelValidation; + let rule: WorkflowOutputLabelValidationRule; beforeEach(() => { - rule = new WorkflowOutputLabelValidation(); + rule = new WorkflowOutputLabelValidationRule(); }); it("should not provide diagnostics when there are no steps", async () => { diff --git a/server/packages/server-common/src/languageTypes.ts b/server/packages/server-common/src/languageTypes.ts index 9d969c1..9ce71ed 100644 --- a/server/packages/server-common/src/languageTypes.ts +++ b/server/packages/server-common/src/languageTypes.ts @@ -159,6 +159,9 @@ export type ValidationProfileIdentifier = "basic" | "iwc"; export interface ValidationProfile { /** The set of rules defining this validation profile. */ get rules(): Set; + + /** The human-readable name of the profile. */ + name: string; } /** @@ -246,6 +249,9 @@ export abstract class LanguageServiceBase implements const profile = this.getValidationProfile(useProfile); profile.rules.forEach(async (validationRule) => { const contributedDiagnostics = await validationRule.validate(documentContext); + contributedDiagnostics.forEach((diagnostic) => { + diagnostic.source = profile.name; + }); diagnostics.push(...contributedDiagnostics); }); } diff --git a/server/packages/server-common/src/providers/validation/profiles.ts b/server/packages/server-common/src/providers/validation/profiles.ts index 051ec56..3823371 100644 --- a/server/packages/server-common/src/providers/validation/profiles.ts +++ b/server/packages/server-common/src/providers/validation/profiles.ts @@ -6,6 +6,7 @@ import { MissingPropertyValidationRule } from "./rules"; * It doesn't apply any additional custom rules. */ export class NoOpValidationProfile implements ValidationProfile { + public readonly name: string = "No Validation"; protected static readonly NO_RULES = new Set([]); public get rules(): Set { @@ -13,24 +14,32 @@ export class NoOpValidationProfile implements ValidationProfile { } } -export const COMMON_IWC_WORKFLOW_RULES = new Set([ - new MissingPropertyValidationRule( - "release", - true, - DiagnosticSeverity.Error, - "The workflow must have a release version." - ), - new MissingPropertyValidationRule( - "creator", - true, - DiagnosticSeverity.Error, - "The workflow does not specify a creator." - ), - new MissingPropertyValidationRule( - "license", - true, - DiagnosticSeverity.Error, - "The workflow does not specify a license." - ), - // Add more common custom rules here... -]); +export class IWCValidationProfile implements ValidationProfile { + public readonly name: string = "IWC Best Practices"; + + protected static readonly RULES: Set = new Set([ + new MissingPropertyValidationRule( + "release", + true, + DiagnosticSeverity.Error, + "The workflow must have a release version." + ), + new MissingPropertyValidationRule( + "creator", + true, + DiagnosticSeverity.Error, + "The workflow does not specify a creator." + ), + new MissingPropertyValidationRule( + "license", + true, + DiagnosticSeverity.Error, + "The workflow does not specify a license." + ), + // Add more custom rules here... + ]); + + public get rules(): Set { + return IWCValidationProfile.RULES; + } +} diff --git a/server/packages/server-common/src/providers/validation/rules/index.ts b/server/packages/server-common/src/providers/validation/rules/index.ts index 8fb9391..0a248f5 100644 --- a/server/packages/server-common/src/providers/validation/rules/index.ts +++ b/server/packages/server-common/src/providers/validation/rules/index.ts @@ -1,4 +1,4 @@ import { MissingPropertyValidationRule } from "./MissingPropertyValidation"; import { WorkflowOutputLabelValidationRule } from "./WorkflowOutputLabelValidation"; -export { MissingPropertyValidationRule, WorkflowOutputLabelValidationRule as WorkflowOutputLabelValidation }; +export { MissingPropertyValidationRule, WorkflowOutputLabelValidationRule }; From 9a9720cbc5877c3d0820e6f92b8cb037fa9bc5fb Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sun, 16 Jun 2024 01:16:57 +0200 Subject: [PATCH 11/43] Refactor move WorkflowOutputLabelValidationRule to native server --- server/gx-workflow-ls-native/src/profiles.ts | 2 +- .../src}/validation/rules/WorkflowOutputLabelValidation.ts | 2 +- .../tests/unit/customValidationRules.test.ts | 6 ++---- server/gx-workflow-ls-native/tsconfig.json | 1 - .../server-common/src/providers/validation/rules/index.ts | 3 +-- 5 files changed, 5 insertions(+), 9 deletions(-) rename server/{packages/server-common/src/providers => gx-workflow-ls-native/src}/validation/rules/WorkflowOutputLabelValidation.ts (93%) diff --git a/server/gx-workflow-ls-native/src/profiles.ts b/server/gx-workflow-ls-native/src/profiles.ts index 49ba428..2744cca 100644 --- a/server/gx-workflow-ls-native/src/profiles.ts +++ b/server/gx-workflow-ls-native/src/profiles.ts @@ -1,6 +1,6 @@ import { DiagnosticSeverity, ValidationRule } from "@gxwf/server-common/src/languageTypes"; import { IWCValidationProfile } from "@gxwf/server-common/src/providers/validation/profiles"; -import { WorkflowOutputLabelValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; +import { WorkflowOutputLabelValidationRule } from "./validation/rules/WorkflowOutputLabelValidation"; /** * *Intergalactic Workflow Commission* (IWC) validation profile for Native Galaxy workflows. diff --git a/server/packages/server-common/src/providers/validation/rules/WorkflowOutputLabelValidation.ts b/server/gx-workflow-ls-native/src/validation/rules/WorkflowOutputLabelValidation.ts similarity index 93% rename from server/packages/server-common/src/providers/validation/rules/WorkflowOutputLabelValidation.ts rename to server/gx-workflow-ls-native/src/validation/rules/WorkflowOutputLabelValidation.ts index 702ade3..7929da7 100644 --- a/server/packages/server-common/src/providers/validation/rules/WorkflowOutputLabelValidation.ts +++ b/server/gx-workflow-ls-native/src/validation/rules/WorkflowOutputLabelValidation.ts @@ -1,5 +1,5 @@ +import { ValidationRule, WorkflowDocument } from "@gxwf/server-common/src/languageTypes"; import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; -import { ValidationRule, WorkflowDocument } from "../../../languageTypes"; /** * Validation rule to check that all defined `workflow_outputs` have a `label`. diff --git a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts index b2db11d..0b9acc7 100644 --- a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts +++ b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts @@ -1,8 +1,6 @@ import { DiagnosticSeverity } from "@gxwf/server-common/src/languageTypes"; -import { - MissingPropertyValidationRule, - WorkflowOutputLabelValidationRule, -} from "@gxwf/server-common/src/providers/validation/rules"; +import { MissingPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; +import { WorkflowOutputLabelValidationRule } from "../../src/validation/rules/WorkflowOutputLabelValidation"; import { createNativeWorkflowDocument } from "../testHelpers"; import { TestWorkflowProvider } from "../testWorkflowProvider"; diff --git a/server/gx-workflow-ls-native/tsconfig.json b/server/gx-workflow-ls-native/tsconfig.json index 198d885..6a81fc7 100644 --- a/server/gx-workflow-ls-native/tsconfig.json +++ b/server/gx-workflow-ls-native/tsconfig.json @@ -11,7 +11,6 @@ "esModuleInterop": true, "sourceMap": true, "strict": true, - "rootDirs": ["src", "tests"], "paths": { "@schemas/*": ["../../workflow-languages/schemas/*"] } diff --git a/server/packages/server-common/src/providers/validation/rules/index.ts b/server/packages/server-common/src/providers/validation/rules/index.ts index 0a248f5..52ffab7 100644 --- a/server/packages/server-common/src/providers/validation/rules/index.ts +++ b/server/packages/server-common/src/providers/validation/rules/index.ts @@ -1,4 +1,3 @@ import { MissingPropertyValidationRule } from "./MissingPropertyValidation"; -import { WorkflowOutputLabelValidationRule } from "./WorkflowOutputLabelValidation"; -export { MissingPropertyValidationRule, WorkflowOutputLabelValidationRule }; +export { MissingPropertyValidationRule }; From deac9f1546c60e900145d432e3d81a27a35355a3 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sun, 16 Jun 2024 01:33:39 +0200 Subject: [PATCH 12/43] Add missing property validation rule for documentation and annotation --- server/gx-workflow-ls-format2/src/profiles.ts | 9 ++++++++- server/gx-workflow-ls-native/src/profiles.ts | 7 +++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/server/gx-workflow-ls-format2/src/profiles.ts b/server/gx-workflow-ls-format2/src/profiles.ts index 9e69dab..445d722 100644 --- a/server/gx-workflow-ls-format2/src/profiles.ts +++ b/server/gx-workflow-ls-format2/src/profiles.ts @@ -1,5 +1,6 @@ -import { ValidationRule } from "@gxwf/server-common/src/languageTypes"; +import { DiagnosticSeverity, ValidationRule } from "@gxwf/server-common/src/languageTypes"; import { IWCValidationProfile } from "@gxwf/server-common/src/providers/validation/profiles"; +import { MissingPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; /** * *Intergalactic Workflow Commission* (IWC) validation profile for gxformat2 Galaxy workflows. @@ -8,6 +9,12 @@ import { IWCValidationProfile } from "@gxwf/server-common/src/providers/validati export class GxFormat2IWCValidationProfile extends IWCValidationProfile { protected static readonly RULES = new Set([ ...super.RULES, + new MissingPropertyValidationRule( + "doc", + true, + DiagnosticSeverity.Error, + "The workflow is not documented. Documenting workflows helps users understand the purpose of the workflow." + ), // Add more custom rules here... ]); diff --git a/server/gx-workflow-ls-native/src/profiles.ts b/server/gx-workflow-ls-native/src/profiles.ts index 2744cca..522265b 100644 --- a/server/gx-workflow-ls-native/src/profiles.ts +++ b/server/gx-workflow-ls-native/src/profiles.ts @@ -1,5 +1,6 @@ import { DiagnosticSeverity, ValidationRule } from "@gxwf/server-common/src/languageTypes"; import { IWCValidationProfile } from "@gxwf/server-common/src/providers/validation/profiles"; +import { MissingPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; import { WorkflowOutputLabelValidationRule } from "./validation/rules/WorkflowOutputLabelValidation"; /** @@ -9,6 +10,12 @@ import { WorkflowOutputLabelValidationRule } from "./validation/rules/WorkflowOu export class NativeIWCValidationProfile extends IWCValidationProfile { protected static readonly RULES = new Set([ ...super.RULES, + new MissingPropertyValidationRule( + "annotation", + true, + DiagnosticSeverity.Error, + "The workflow is not annotated. Annotating workflows helps users understand the purpose of the workflow." + ), new WorkflowOutputLabelValidationRule(DiagnosticSeverity.Error), // Add more custom rules here... ]); From c3ef32cb70b8bc96b181d40665bc51796ef2473a Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sun, 16 Jun 2024 11:48:51 +0200 Subject: [PATCH 13/43] Refactor schema validation sources to include source information --- .../src/services/validation/schemaValidationService.ts | 5 ++++- server/gx-workflow-ls-native/src/languageService.ts | 3 +++ .../src/services/validation.ts | 3 +++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/server/gx-workflow-ls-format2/src/services/validation/schemaValidationService.ts b/server/gx-workflow-ls-format2/src/services/validation/schemaValidationService.ts index 4705302..eb11830 100644 --- a/server/gx-workflow-ls-format2/src/services/validation/schemaValidationService.ts +++ b/server/gx-workflow-ls-format2/src/services/validation/schemaValidationService.ts @@ -8,7 +8,7 @@ import { WorkflowValidator, } from "@gxwf/server-common/src/languageTypes"; import { SchemaNode, SchemaNodeResolver } from "../../schema"; -import { RecordSchemaNode, IdMapper } from "../../schema/definitions"; +import { IdMapper, RecordSchemaNode } from "../../schema/definitions"; export class GxFormat2SchemaValidationService implements WorkflowValidator { constructor(protected readonly schemaNodeResolver: SchemaNodeResolver) {} @@ -16,6 +16,9 @@ export class GxFormat2SchemaValidationService implements WorkflowValidator { public doValidation(workflowDocument: WorkflowDocument): Promise { const diagnostics: Diagnostic[] = []; this.collectSchemaDiagnostics(workflowDocument, diagnostics); + diagnostics.forEach((diagnostic) => { + diagnostic.source = "Format2 Schema"; + }); return Promise.resolve(diagnostics); } diff --git a/server/gx-workflow-ls-native/src/languageService.ts b/server/gx-workflow-ls-native/src/languageService.ts index 58cd69e..bf9e54f 100644 --- a/server/gx-workflow-ls-native/src/languageService.ts +++ b/server/gx-workflow-ls-native/src/languageService.ts @@ -100,6 +100,9 @@ export class NativeWorkflowLanguageServiceImpl this._documentSettings, this.schema ); + schemaValidationResults.forEach((diagnostic) => { + diagnostic.source = "Native Workflow Schema"; + }); return schemaValidationResults; } diff --git a/server/packages/workflow-tests-language-service/src/services/validation.ts b/server/packages/workflow-tests-language-service/src/services/validation.ts index 5e24874..2516d91 100644 --- a/server/packages/workflow-tests-language-service/src/services/validation.ts +++ b/server/packages/workflow-tests-language-service/src/services/validation.ts @@ -73,6 +73,9 @@ export class WorkflowTestsValidationServiceImpl implements WorkflowTestsValidati const schema = this.schemaProvider.getResolvedSchema(); const schemaValidation = getDiagnostics(schema); + schemaValidation.forEach((diagnostic) => { + diagnostic.source = "Workflow Tests Schema"; + }); const semanticValidation = await this.doSemanticValidation(documentContext); return schemaValidation.concat(semanticValidation); } From e919067f08953be0c9e39fd39edc0c42d88bd28d Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sun, 16 Jun 2024 19:31:59 +0200 Subject: [PATCH 14/43] Refactor and improve validation rules package --- .../src/languageService.ts | 25 ++++----- server/gx-workflow-ls-format2/src/profiles.ts | 27 +++++++-- .../schemaValidationService.ts | 7 +-- .../src/services/validation/index.ts | 4 -- .../validation/workflowValidationService.ts | 56 ------------------- server/gx-workflow-ls-native/src/profiles.ts | 29 ++++++++-- ...s => WorkflowOutputLabelValidationRule.ts} | 2 +- .../tests/unit/customValidationRules.test.ts | 2 +- .../server-common/src/languageTypes.ts | 6 +- .../src/providers/validation/profiles.ts | 28 ++++++++-- .../rules/MissingPropertyValidation.ts | 2 +- .../rules/StepErrorValidationRule.ts | 28 ++++++++++ .../rules/TestToolShedValidationRule.ts | 30 ++++++++++ .../src/providers/validation/rules/index.ts | 4 +- .../src/services/validation.ts | 1 + .../src/services/yamlValidation.ts | 2 - 16 files changed, 153 insertions(+), 100 deletions(-) rename server/gx-workflow-ls-format2/src/services/{validation => }/schemaValidationService.ts (94%) delete mode 100644 server/gx-workflow-ls-format2/src/services/validation/index.ts delete mode 100644 server/gx-workflow-ls-format2/src/services/validation/workflowValidationService.ts rename server/gx-workflow-ls-native/src/validation/rules/{WorkflowOutputLabelValidation.ts => WorkflowOutputLabelValidationRule.ts} (94%) create mode 100644 server/packages/server-common/src/providers/validation/rules/StepErrorValidationRule.ts create mode 100644 server/packages/server-common/src/providers/validation/rules/TestToolShedValidationRule.ts diff --git a/server/gx-workflow-ls-format2/src/languageService.ts b/server/gx-workflow-ls-format2/src/languageService.ts index 6b83b97..7c13f35 100644 --- a/server/gx-workflow-ls-format2/src/languageService.ts +++ b/server/gx-workflow-ls-format2/src/languageService.ts @@ -12,7 +12,6 @@ import { TYPES, TextDocument, TextEdit, - WorkflowValidator, } from "@gxwf/server-common/src/languageTypes"; import { TYPES as YAML_TYPES } from "@gxwf/yaml-language-service/src/inversify.config"; import { YAMLLanguageService } from "@gxwf/yaml-language-service/src/yamlLanguageService"; @@ -22,7 +21,7 @@ import { GxFormat2IWCValidationProfile } from "./profiles"; import { GalaxyWorkflowFormat2SchemaLoader } from "./schema"; import { GxFormat2CompletionService } from "./services/completionService"; import { GxFormat2HoverService } from "./services/hoverService"; -import { GxFormat2SchemaValidationService, WorkflowValidationService } from "./services/validation"; +import { GxFormat2SchemaValidationService } from "./services/schemaValidationService"; const LANGUAGE_ID = "gxformat2"; @@ -41,7 +40,7 @@ export class GxFormat2WorkflowLanguageServiceImpl private _schemaLoader: GalaxyWorkflowFormat2SchemaLoader; private _hoverService: GxFormat2HoverService; private _completionService: GxFormat2CompletionService; - private _validationServices: WorkflowValidator[]; + private _schemaValidationService: GxFormat2SchemaValidationService; constructor( @inject(YAML_TYPES.YAMLLanguageService) yamlLanguageService: YAMLLanguageService, @@ -52,10 +51,7 @@ export class GxFormat2WorkflowLanguageServiceImpl this._yamlLanguageService = yamlLanguageService; this._hoverService = new GxFormat2HoverService(this._schemaLoader.nodeResolver); this._completionService = new GxFormat2CompletionService(this._schemaLoader.nodeResolver); - this._validationServices = [ - new GxFormat2SchemaValidationService(this._schemaLoader.nodeResolver), - new WorkflowValidationService(), - ]; + this._schemaValidationService = new GxFormat2SchemaValidationService(this._schemaLoader.nodeResolver); } public override parseDocument(document: TextDocument): GxFormat2WorkflowDocument { @@ -84,12 +80,15 @@ export class GxFormat2WorkflowLanguageServiceImpl } protected override async doValidation(documentContext: GxFormat2WorkflowDocument): Promise { - const diagnostics = await this._yamlLanguageService.doValidation(documentContext.yamlDocument); - for (const validator of this._validationServices) { - const results = await validator.doValidation(documentContext); - diagnostics.push(...results); - } - return diagnostics; + const syntaxDiagnostics = await this._yamlLanguageService.doValidation(documentContext.yamlDocument); + syntaxDiagnostics.forEach((diagnostic) => { + diagnostic.source = "YAML Syntax"; + }); + const schemaDiagnostics = await this._schemaValidationService.doValidation(documentContext); + schemaDiagnostics.forEach((diagnostic) => { + diagnostic.source = "Format2 Schema"; + }); + return syntaxDiagnostics.concat(schemaDiagnostics); } public override getSymbols(documentContext: GxFormat2WorkflowDocument): DocumentSymbol[] { diff --git a/server/gx-workflow-ls-format2/src/profiles.ts b/server/gx-workflow-ls-format2/src/profiles.ts index 445d722..6585d97 100644 --- a/server/gx-workflow-ls-format2/src/profiles.ts +++ b/server/gx-workflow-ls-format2/src/profiles.ts @@ -1,21 +1,40 @@ import { DiagnosticSeverity, ValidationRule } from "@gxwf/server-common/src/languageTypes"; -import { IWCValidationProfile } from "@gxwf/server-common/src/providers/validation/profiles"; +import { + BasicCommonValidationProfile, + IWCCommonValidationProfile, +} from "@gxwf/server-common/src/providers/validation/profiles"; import { MissingPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; +/** + * Defines the minimal set of validation rules for gxformat2 Galaxy workflows. + */ +export class GxFormat2BasicValidationProfile extends BasicCommonValidationProfile { + public static readonly RULES = new Set([ + ...super.RULES, + // Add more custom rules specific to gxformat2 workflows here... + ]); + + public get rules(): Set { + return GxFormat2BasicValidationProfile.RULES; + } +} + /** * *Intergalactic Workflow Commission* (IWC) validation profile for gxformat2 Galaxy workflows. - * Defines custom validation rules to comply with the IWC best practices guidelines. + * This profile extends the basic validation profile and adds additional rules to comply + * with the IWC best practices guidelines. */ -export class GxFormat2IWCValidationProfile extends IWCValidationProfile { +export class GxFormat2IWCValidationProfile extends IWCCommonValidationProfile { protected static readonly RULES = new Set([ ...super.RULES, + ...GxFormat2BasicValidationProfile.RULES, new MissingPropertyValidationRule( "doc", true, DiagnosticSeverity.Error, "The workflow is not documented. Documenting workflows helps users understand the purpose of the workflow." ), - // Add more custom rules here... + // Add more custom rules specific to gxformat2 workflows here... ]); public get rules(): Set { diff --git a/server/gx-workflow-ls-format2/src/services/validation/schemaValidationService.ts b/server/gx-workflow-ls-format2/src/services/schemaValidationService.ts similarity index 94% rename from server/gx-workflow-ls-format2/src/services/validation/schemaValidationService.ts rename to server/gx-workflow-ls-format2/src/services/schemaValidationService.ts index eb11830..7ac026b 100644 --- a/server/gx-workflow-ls-format2/src/services/validation/schemaValidationService.ts +++ b/server/gx-workflow-ls-format2/src/services/schemaValidationService.ts @@ -7,8 +7,8 @@ import { WorkflowDocument, WorkflowValidator, } from "@gxwf/server-common/src/languageTypes"; -import { SchemaNode, SchemaNodeResolver } from "../../schema"; -import { IdMapper, RecordSchemaNode } from "../../schema/definitions"; +import { SchemaNode, SchemaNodeResolver } from "../schema"; +import { IdMapper, RecordSchemaNode } from "../schema/definitions"; export class GxFormat2SchemaValidationService implements WorkflowValidator { constructor(protected readonly schemaNodeResolver: SchemaNodeResolver) {} @@ -16,9 +16,6 @@ export class GxFormat2SchemaValidationService implements WorkflowValidator { public doValidation(workflowDocument: WorkflowDocument): Promise { const diagnostics: Diagnostic[] = []; this.collectSchemaDiagnostics(workflowDocument, diagnostics); - diagnostics.forEach((diagnostic) => { - diagnostic.source = "Format2 Schema"; - }); return Promise.resolve(diagnostics); } diff --git a/server/gx-workflow-ls-format2/src/services/validation/index.ts b/server/gx-workflow-ls-format2/src/services/validation/index.ts deleted file mode 100644 index 904820f..0000000 --- a/server/gx-workflow-ls-format2/src/services/validation/index.ts +++ /dev/null @@ -1,4 +0,0 @@ -import { GxFormat2SchemaValidationService } from "./schemaValidationService"; -import { WorkflowValidationService } from "./workflowValidationService"; - -export { GxFormat2SchemaValidationService, WorkflowValidationService }; diff --git a/server/gx-workflow-ls-format2/src/services/validation/workflowValidationService.ts b/server/gx-workflow-ls-format2/src/services/validation/workflowValidationService.ts deleted file mode 100644 index 9e1f0e9..0000000 --- a/server/gx-workflow-ls-format2/src/services/validation/workflowValidationService.ts +++ /dev/null @@ -1,56 +0,0 @@ -import { - Diagnostic, - DiagnosticSeverity, - WorkflowDocument, - WorkflowValidator, -} from "@gxwf/server-common/src/languageTypes"; - -export class WorkflowValidationService implements WorkflowValidator { - public doValidation(workflowDocument: WorkflowDocument): Promise { - const diagnostics: Diagnostic[] = [ - ...collectStepErrorDiagnostics(workflowDocument), - ...collectToolDiagnostics(workflowDocument), - ]; - return Promise.resolve(diagnostics); - } -} - -export function collectStepErrorDiagnostics(workflowDocument: WorkflowDocument): Diagnostic[] { - const diagnostics: Diagnostic[] = []; - const steps = workflowDocument.nodeManager.getStepNodes(true); - steps.forEach((step) => { - const errors = step.properties.find((p) => p.keyNode.value === "errors"); - if (errors) { - const range = workflowDocument.nodeManager.getNodeRange(errors); - diagnostics.push( - Diagnostic.create( - range, - `Tool step contains error indicated during Galaxy export - ${errors}`, - DiagnosticSeverity.Warning - ) - ); - } - }); - return diagnostics; -} - -export function collectToolDiagnostics(workflowDocument: WorkflowDocument): Diagnostic[] { - const diagnostics: Diagnostic[] = []; - const steps = workflowDocument.nodeManager.getStepNodes(true); - steps.forEach((step) => { - const tool_id = step.properties.find((p) => p.keyNode.value === "tool_id"); - if (tool_id) { - if (tool_id.valueNode?.value?.toString().includes("testtoolshed")) { - const range = workflowDocument.nodeManager.getNodeRange(tool_id); - diagnostics.push( - Diagnostic.create( - range, - `Step references a tool from the test tool shed, this should be replaced with a production tool`, - DiagnosticSeverity.Warning - ) - ); - } - } - }); - return diagnostics; -} diff --git a/server/gx-workflow-ls-native/src/profiles.ts b/server/gx-workflow-ls-native/src/profiles.ts index 522265b..f75d342 100644 --- a/server/gx-workflow-ls-native/src/profiles.ts +++ b/server/gx-workflow-ls-native/src/profiles.ts @@ -1,15 +1,34 @@ import { DiagnosticSeverity, ValidationRule } from "@gxwf/server-common/src/languageTypes"; -import { IWCValidationProfile } from "@gxwf/server-common/src/providers/validation/profiles"; +import { + BasicCommonValidationProfile, + IWCCommonValidationProfile, +} from "@gxwf/server-common/src/providers/validation/profiles"; import { MissingPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; -import { WorkflowOutputLabelValidationRule } from "./validation/rules/WorkflowOutputLabelValidation"; +import { WorkflowOutputLabelValidationRule } from "./validation/rules/WorkflowOutputLabelValidationRule"; + +/** + * Defines the minimal set of validation rules for Native Galaxy workflows. + */ +export class NativeBasicValidationProfile extends BasicCommonValidationProfile { + public static readonly RULES = new Set([ + ...super.RULES, + // Add more custom rules specific to native workflows here... + ]); + + public get rules(): Set { + return NativeBasicValidationProfile.RULES; + } +} /** * *Intergalactic Workflow Commission* (IWC) validation profile for Native Galaxy workflows. - * Defines custom validation rules to comply with the IWC best practices guidelines. + * This profile extends the basic validation profile and adds additional rules to comply + * with the IWC best practices guidelines. */ -export class NativeIWCValidationProfile extends IWCValidationProfile { +export class NativeIWCValidationProfile extends IWCCommonValidationProfile { protected static readonly RULES = new Set([ ...super.RULES, + ...NativeBasicValidationProfile.RULES, new MissingPropertyValidationRule( "annotation", true, @@ -17,7 +36,7 @@ export class NativeIWCValidationProfile extends IWCValidationProfile { "The workflow is not annotated. Annotating workflows helps users understand the purpose of the workflow." ), new WorkflowOutputLabelValidationRule(DiagnosticSeverity.Error), - // Add more custom rules here... + // Add more custom rules specific to native workflows here... ]); public get rules(): Set { diff --git a/server/gx-workflow-ls-native/src/validation/rules/WorkflowOutputLabelValidation.ts b/server/gx-workflow-ls-native/src/validation/rules/WorkflowOutputLabelValidationRule.ts similarity index 94% rename from server/gx-workflow-ls-native/src/validation/rules/WorkflowOutputLabelValidation.ts rename to server/gx-workflow-ls-native/src/validation/rules/WorkflowOutputLabelValidationRule.ts index 7929da7..baa0120 100644 --- a/server/gx-workflow-ls-native/src/validation/rules/WorkflowOutputLabelValidation.ts +++ b/server/gx-workflow-ls-native/src/validation/rules/WorkflowOutputLabelValidationRule.ts @@ -7,7 +7,7 @@ import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; export class WorkflowOutputLabelValidationRule implements ValidationRule { constructor(readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error) {} - validate(workflowDocument: WorkflowDocument): Promise { + public async validate(workflowDocument: WorkflowDocument): Promise { const result: Diagnostic[] = []; const stepNodes = workflowDocument.nodeManager.getStepNodes(); stepNodes.forEach((step) => { diff --git a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts index 0b9acc7..7540787 100644 --- a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts +++ b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts @@ -1,6 +1,6 @@ import { DiagnosticSeverity } from "@gxwf/server-common/src/languageTypes"; import { MissingPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; -import { WorkflowOutputLabelValidationRule } from "../../src/validation/rules/WorkflowOutputLabelValidation"; +import { WorkflowOutputLabelValidationRule } from "../../src/validation/rules/WorkflowOutputLabelValidationRule"; import { createNativeWorkflowDocument } from "../testHelpers"; import { TestWorkflowProvider } from "../testWorkflowProvider"; diff --git a/server/packages/server-common/src/languageTypes.ts b/server/packages/server-common/src/languageTypes.ts index 9ce71ed..4bfe33b 100644 --- a/server/packages/server-common/src/languageTypes.ts +++ b/server/packages/server-common/src/languageTypes.ts @@ -247,13 +247,13 @@ export abstract class LanguageServiceBase implements const diagnostics = await this.doValidation(documentContext); if (useProfile) { const profile = this.getValidationProfile(useProfile); - profile.rules.forEach(async (validationRule) => { - const contributedDiagnostics = await validationRule.validate(documentContext); + for (const rule of profile.rules) { + const contributedDiagnostics = await rule.validate(documentContext); contributedDiagnostics.forEach((diagnostic) => { diagnostic.source = profile.name; }); diagnostics.push(...contributedDiagnostics); - }); + } } return diagnostics; } diff --git a/server/packages/server-common/src/providers/validation/profiles.ts b/server/packages/server-common/src/providers/validation/profiles.ts index 3823371..02a4380 100644 --- a/server/packages/server-common/src/providers/validation/profiles.ts +++ b/server/packages/server-common/src/providers/validation/profiles.ts @@ -1,5 +1,5 @@ import { DiagnosticSeverity, ValidationProfile, ValidationRule } from "../../languageTypes"; -import { MissingPropertyValidationRule } from "./rules"; +import { MissingPropertyValidationRule, StepExportErrorValidationRule, TestToolshedValidationRule } from "./rules"; /** * The *NoOp* validation profile. @@ -14,7 +14,27 @@ export class NoOpValidationProfile implements ValidationProfile { } } -export class IWCValidationProfile implements ValidationProfile { +/** + * Common set of validation rules for basic validation of any workflow format. + */ +export class BasicCommonValidationProfile implements ValidationProfile { + public readonly name: string = "Basic Validation"; + + protected static readonly RULES: Set = new Set([ + new TestToolshedValidationRule(DiagnosticSeverity.Error), + new StepExportErrorValidationRule(DiagnosticSeverity.Error), + // Add common basic rules here... + ]); + + public get rules(): Set { + return BasicCommonValidationProfile.RULES; + } +} + +/** + * Common set of validation rules for IWC best practices. + */ +export class IWCCommonValidationProfile implements ValidationProfile { public readonly name: string = "IWC Best Practices"; protected static readonly RULES: Set = new Set([ @@ -36,10 +56,10 @@ export class IWCValidationProfile implements ValidationProfile { DiagnosticSeverity.Error, "The workflow does not specify a license." ), - // Add more custom rules here... + // Add more common rules here... ]); public get rules(): Set { - return IWCValidationProfile.RULES; + return IWCCommonValidationProfile.RULES; } } diff --git a/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts b/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts index 3bd4999..29ec299 100644 --- a/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts +++ b/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts @@ -19,7 +19,7 @@ export class MissingPropertyValidationRule implements ValidationRule { private message?: string ) {} - validate(workflowDocument: WorkflowDocument): Promise { + public async validate(workflowDocument: WorkflowDocument): Promise { const result: Diagnostic[] = []; const targetNode = workflowDocument.nodeManager.getNodeFromPath(this.nodePath); if (!targetNode) { diff --git a/server/packages/server-common/src/providers/validation/rules/StepErrorValidationRule.ts b/server/packages/server-common/src/providers/validation/rules/StepErrorValidationRule.ts new file mode 100644 index 0000000..269803b --- /dev/null +++ b/server/packages/server-common/src/providers/validation/rules/StepErrorValidationRule.ts @@ -0,0 +1,28 @@ +import { ValidationRule, WorkflowDocument } from "@gxwf/server-common/src/languageTypes"; +import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; + +/** + * Validation rule to check if step nodes contain export errors. + */ +export class StepExportErrorValidationRule implements ValidationRule { + constructor(readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error) {} + + public async validate(workflowDocument: WorkflowDocument): Promise { + const diagnostics: Diagnostic[] = []; + const steps = workflowDocument.nodeManager.getStepNodes(true); + steps.forEach((step) => { + const errors = step.properties.find((p) => p.keyNode.value === "errors"); + if (errors) { + const range = workflowDocument.nodeManager.getNodeRange(errors); + diagnostics.push( + Diagnostic.create( + range, + `Tool step contains error indicated during Galaxy export - ${errors}`, + DiagnosticSeverity.Warning + ) + ); + } + }); + return Promise.resolve(diagnostics); + } +} diff --git a/server/packages/server-common/src/providers/validation/rules/TestToolShedValidationRule.ts b/server/packages/server-common/src/providers/validation/rules/TestToolShedValidationRule.ts new file mode 100644 index 0000000..ceec159 --- /dev/null +++ b/server/packages/server-common/src/providers/validation/rules/TestToolShedValidationRule.ts @@ -0,0 +1,30 @@ +import { ValidationRule, WorkflowDocument } from "@gxwf/server-common/src/languageTypes"; +import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; + +/** + * Validation rule to check if step nodes contain a reference to a tool from the test Tool Shed. + */ +export class TestToolshedValidationRule implements ValidationRule { + constructor(readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error) {} + + public async validate(workflowDocument: WorkflowDocument): Promise { + const diagnostics: Diagnostic[] = []; + const steps = workflowDocument.nodeManager.getStepNodes(true); + steps.forEach((step) => { + const tool_id = step.properties.find((p) => p.keyNode.value === "tool_id"); + if (tool_id) { + if (tool_id.valueNode?.value?.toString().includes("testtoolshed")) { + const range = workflowDocument.nodeManager.getNodeRange(tool_id); + diagnostics.push( + Diagnostic.create( + range, + `Step references a tool from the test tool shed, this should be replaced with a production tool`, + DiagnosticSeverity.Warning + ) + ); + } + } + }); + return Promise.resolve(diagnostics); + } +} diff --git a/server/packages/server-common/src/providers/validation/rules/index.ts b/server/packages/server-common/src/providers/validation/rules/index.ts index 52ffab7..fa1a219 100644 --- a/server/packages/server-common/src/providers/validation/rules/index.ts +++ b/server/packages/server-common/src/providers/validation/rules/index.ts @@ -1,3 +1,5 @@ import { MissingPropertyValidationRule } from "./MissingPropertyValidation"; +import { StepExportErrorValidationRule } from "./StepErrorValidationRule"; +import { TestToolshedValidationRule } from "./TestToolShedValidationRule"; -export { MissingPropertyValidationRule }; +export { MissingPropertyValidationRule, StepExportErrorValidationRule, TestToolshedValidationRule }; diff --git a/server/packages/workflow-tests-language-service/src/services/validation.ts b/server/packages/workflow-tests-language-service/src/services/validation.ts index 2516d91..44d5f8d 100644 --- a/server/packages/workflow-tests-language-service/src/services/validation.ts +++ b/server/packages/workflow-tests-language-service/src/services/validation.ts @@ -80,6 +80,7 @@ export class WorkflowTestsValidationServiceImpl implements WorkflowTestsValidati return schemaValidation.concat(semanticValidation); } + // TODO: convert to rules async doSemanticValidation(documentContext: DocumentContext): Promise { const testDocument = documentContext as WorkflowTestsDocument; const inputDiagnostics = await this.validateWorkflowInputs(testDocument); diff --git a/server/packages/yaml-language-service/src/services/yamlValidation.ts b/server/packages/yaml-language-service/src/services/yamlValidation.ts index 57cec03..a74935d 100644 --- a/server/packages/yaml-language-service/src/services/yamlValidation.ts +++ b/server/packages/yaml-language-service/src/services/yamlValidation.ts @@ -20,8 +20,6 @@ export class YAMLValidation { return Promise.resolve([]); } const diagnostics: Diagnostic[] = [...yamlDocument.syntaxDiagnostics]; - // TODO: add schema validation diagnostics - return Promise.resolve(diagnostics); } } From 40bb41a17539d05b0fe90a4568f56a855a4b335d Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sun, 16 Jun 2024 20:23:02 +0200 Subject: [PATCH 15/43] Convert workflow tests documents semantic validation to rules --- .../WorkflowOutputLabelValidationRule.ts | 8 +-- .../rules/MissingPropertyValidation.ts | 12 ++-- .../rules/StepErrorValidationRule.ts | 8 +-- .../rules/TestToolShedValidationRule.ts | 8 +-- .../src/languageService.ts | 6 ++ .../src/profiles.ts | 47 ++++++++++++++ .../src/services/validation.ts | 64 +------------------ .../rules/WorkflowInputsValidationRule.ts | 29 +++++++++ .../rules/WorkflowOutputsValidationRule.ts | 29 +++++++++ 9 files changed, 131 insertions(+), 80 deletions(-) create mode 100644 server/packages/workflow-tests-language-service/src/profiles.ts create mode 100644 server/packages/workflow-tests-language-service/src/validation/rules/WorkflowInputsValidationRule.ts create mode 100644 server/packages/workflow-tests-language-service/src/validation/rules/WorkflowOutputsValidationRule.ts diff --git a/server/gx-workflow-ls-native/src/validation/rules/WorkflowOutputLabelValidationRule.ts b/server/gx-workflow-ls-native/src/validation/rules/WorkflowOutputLabelValidationRule.ts index baa0120..887b01d 100644 --- a/server/gx-workflow-ls-native/src/validation/rules/WorkflowOutputLabelValidationRule.ts +++ b/server/gx-workflow-ls-native/src/validation/rules/WorkflowOutputLabelValidationRule.ts @@ -1,4 +1,4 @@ -import { ValidationRule, WorkflowDocument } from "@gxwf/server-common/src/languageTypes"; +import { DocumentContext, ValidationRule } from "@gxwf/server-common/src/languageTypes"; import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; /** @@ -7,9 +7,9 @@ import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; export class WorkflowOutputLabelValidationRule implements ValidationRule { constructor(readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error) {} - public async validate(workflowDocument: WorkflowDocument): Promise { + public async validate(documentContext: DocumentContext): Promise { const result: Diagnostic[] = []; - const stepNodes = workflowDocument.nodeManager.getStepNodes(); + const stepNodes = documentContext.nodeManager.getStepNodes(); stepNodes.forEach((step) => { const workflowOutputs = step.properties.find((property) => property.keyNode.value === "workflow_outputs"); if (workflowOutputs && workflowOutputs.valueNode && workflowOutputs.valueNode.type === "array") { @@ -19,7 +19,7 @@ export class WorkflowOutputLabelValidationRule implements ValidationRule { if (!labelNode?.valueNode?.value) { result.push({ message: `Missing label in workflow output.`, - range: workflowDocument.nodeManager.getNodeRange(outputNode), + range: documentContext.nodeManager.getNodeRange(outputNode), severity: this.severity, }); } diff --git a/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts b/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts index 29ec299..7305da5 100644 --- a/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts +++ b/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts @@ -1,5 +1,5 @@ import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; -import { ValidationRule, WorkflowDocument } from "../../../languageTypes"; +import { DocumentContext, ValidationRule } from "../../../languageTypes"; /** * Validation rule to check that a particular property exists in a workflow. @@ -19,23 +19,23 @@ export class MissingPropertyValidationRule implements ValidationRule { private message?: string ) {} - public async validate(workflowDocument: WorkflowDocument): Promise { + public async validate(documentContext: DocumentContext): Promise { const result: Diagnostic[] = []; - const targetNode = workflowDocument.nodeManager.getNodeFromPath(this.nodePath); + const targetNode = documentContext.nodeManager.getNodeFromPath(this.nodePath); if (!targetNode) { result.push({ message: this.message ?? `Missing property "${this.nodePath}".`, - range: workflowDocument.nodeManager.getDefaultRange(), + range: documentContext.nodeManager.getDefaultRange(), severity: this.severity, }); } if (this.valueRequired && targetNode) { - const missingValue = workflowDocument.nodeManager.isNodeEmpty(targetNode); + const missingValue = documentContext.nodeManager.isNodeEmpty(targetNode); if (missingValue) { result.push({ message: `Missing value in property "${this.nodePath}".`, - range: workflowDocument.nodeManager.getNodeRange(targetNode), + range: documentContext.nodeManager.getNodeRange(targetNode), severity: this.severity, }); } diff --git a/server/packages/server-common/src/providers/validation/rules/StepErrorValidationRule.ts b/server/packages/server-common/src/providers/validation/rules/StepErrorValidationRule.ts index 269803b..96cfe58 100644 --- a/server/packages/server-common/src/providers/validation/rules/StepErrorValidationRule.ts +++ b/server/packages/server-common/src/providers/validation/rules/StepErrorValidationRule.ts @@ -1,4 +1,4 @@ -import { ValidationRule, WorkflowDocument } from "@gxwf/server-common/src/languageTypes"; +import { DocumentContext, ValidationRule } from "@gxwf/server-common/src/languageTypes"; import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; /** @@ -7,13 +7,13 @@ import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; export class StepExportErrorValidationRule implements ValidationRule { constructor(readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error) {} - public async validate(workflowDocument: WorkflowDocument): Promise { + public async validate(documentContext: DocumentContext): Promise { const diagnostics: Diagnostic[] = []; - const steps = workflowDocument.nodeManager.getStepNodes(true); + const steps = documentContext.nodeManager.getStepNodes(true); steps.forEach((step) => { const errors = step.properties.find((p) => p.keyNode.value === "errors"); if (errors) { - const range = workflowDocument.nodeManager.getNodeRange(errors); + const range = documentContext.nodeManager.getNodeRange(errors); diagnostics.push( Diagnostic.create( range, diff --git a/server/packages/server-common/src/providers/validation/rules/TestToolShedValidationRule.ts b/server/packages/server-common/src/providers/validation/rules/TestToolShedValidationRule.ts index ceec159..65b9ad7 100644 --- a/server/packages/server-common/src/providers/validation/rules/TestToolShedValidationRule.ts +++ b/server/packages/server-common/src/providers/validation/rules/TestToolShedValidationRule.ts @@ -1,4 +1,4 @@ -import { ValidationRule, WorkflowDocument } from "@gxwf/server-common/src/languageTypes"; +import { DocumentContext, ValidationRule } from "@gxwf/server-common/src/languageTypes"; import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; /** @@ -7,14 +7,14 @@ import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; export class TestToolshedValidationRule implements ValidationRule { constructor(readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error) {} - public async validate(workflowDocument: WorkflowDocument): Promise { + public async validate(documentContext: DocumentContext): Promise { const diagnostics: Diagnostic[] = []; - const steps = workflowDocument.nodeManager.getStepNodes(true); + const steps = documentContext.nodeManager.getStepNodes(true); steps.forEach((step) => { const tool_id = step.properties.find((p) => p.keyNode.value === "tool_id"); if (tool_id) { if (tool_id.valueNode?.value?.toString().includes("testtoolshed")) { - const range = workflowDocument.nodeManager.getNodeRange(tool_id); + const range = documentContext.nodeManager.getNodeRange(tool_id); diagnostics.push( Diagnostic.create( range, diff --git a/server/packages/workflow-tests-language-service/src/languageService.ts b/server/packages/workflow-tests-language-service/src/languageService.ts index 8885147..3741c23 100644 --- a/server/packages/workflow-tests-language-service/src/languageService.ts +++ b/server/packages/workflow-tests-language-service/src/languageService.ts @@ -16,6 +16,7 @@ import { TYPES as YAML_TYPES } from "@gxwf/yaml-language-service/src/inversify.c import { YAMLLanguageService } from "@gxwf/yaml-language-service/src/yamlLanguageService"; import { inject, injectable } from "inversify"; import { GxWorkflowTestsDocument } from "./document"; +import { TestDocumentIWCValidationProfile } from "./profiles"; import { WorkflowTestsCompletionService } from "./services/completion"; import { WorkflowTestsHoverService } from "./services/hover"; import { WorkflowTestsValidationService } from "./services/validation"; @@ -60,6 +61,11 @@ export class GxWorkflowTestsLanguageServiceImpl extends LanguageServiceBase { return this.validationService.doValidation(documentContext); } diff --git a/server/packages/workflow-tests-language-service/src/profiles.ts b/server/packages/workflow-tests-language-service/src/profiles.ts new file mode 100644 index 0000000..fbfcb36 --- /dev/null +++ b/server/packages/workflow-tests-language-service/src/profiles.ts @@ -0,0 +1,47 @@ +import { DiagnosticSeverity, ValidationRule } from "@gxwf/server-common/src/languageTypes"; +import { + BasicCommonValidationProfile, + IWCCommonValidationProfile, +} from "@gxwf/server-common/src/providers/validation/profiles"; +import { MissingPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; +import { WorkflowInputsValidationRule } from "./validation/rules/WorkflowInputsValidationRule"; +import { WorkflowOutputsValidationRule } from "./validation/rules/WorkflowOutputsValidationRule"; + +/** + * Defines the minimal set of validation rules for Native Galaxy workflows. + */ +export class TestDocumentBasicValidationProfile extends BasicCommonValidationProfile { + public static readonly RULES = new Set([ + ...super.RULES, + new WorkflowInputsValidationRule(DiagnosticSeverity.Error), + new WorkflowOutputsValidationRule(DiagnosticSeverity.Error), + // Add more custom rules specific to native workflows here... + ]); + + public get rules(): Set { + return TestDocumentBasicValidationProfile.RULES; + } +} + +/** + * *Intergalactic Workflow Commission* (IWC) validation profile for Native Galaxy workflows. + * This profile extends the basic validation profile and adds additional rules to comply + * with the IWC best practices guidelines. + */ +export class TestDocumentIWCValidationProfile extends IWCCommonValidationProfile { + protected static readonly RULES = new Set([ + ...super.RULES, + ...TestDocumentBasicValidationProfile.RULES, + new MissingPropertyValidationRule( + "doc", + true, + DiagnosticSeverity.Error, + "The workflow is not documented. Documenting workflows helps users understand the purpose of the workflow." + ), + // Add more custom rules specific to native workflows here... + ]); + + public get rules(): Set { + return TestDocumentIWCValidationProfile.RULES; + } +} diff --git a/server/packages/workflow-tests-language-service/src/services/validation.ts b/server/packages/workflow-tests-language-service/src/services/validation.ts index 44d5f8d..d419836 100644 --- a/server/packages/workflow-tests-language-service/src/services/validation.ts +++ b/server/packages/workflow-tests-language-service/src/services/validation.ts @@ -1,10 +1,4 @@ -import { - Diagnostic, - DiagnosticSeverity, - DocumentContext, - Range, - WorkflowTestsDocument, -} from "@gxwf/server-common/src/languageTypes"; +import { Diagnostic, DiagnosticSeverity, DocumentContext, Range } from "@gxwf/server-common/src/languageTypes"; import { inject, injectable } from "inversify"; import { ResolvedSchema } from "../schema/jsonSchema"; import { WorkflowTestsSchemaProvider } from "../schema/provider"; @@ -76,60 +70,6 @@ export class WorkflowTestsValidationServiceImpl implements WorkflowTestsValidati schemaValidation.forEach((diagnostic) => { diagnostic.source = "Workflow Tests Schema"; }); - const semanticValidation = await this.doSemanticValidation(documentContext); - return schemaValidation.concat(semanticValidation); - } - - // TODO: convert to rules - async doSemanticValidation(documentContext: DocumentContext): Promise { - const testDocument = documentContext as WorkflowTestsDocument; - const inputDiagnostics = await this.validateWorkflowInputs(testDocument); - const outputDiagnostics = await this.validateWorkflowOutputs(testDocument); - return inputDiagnostics.concat(outputDiagnostics); - } - - private async validateWorkflowInputs(testDocument: WorkflowTestsDocument): Promise { - const diagnostics: Diagnostic[] = []; - const workflowInputs = await testDocument.getWorkflowInputs(); - const documentInputNodes = testDocument.nodeManager.getAllPropertyNodesByName("job")[0]?.valueNode?.children ?? []; - documentInputNodes.forEach((inputNode) => { - if (inputNode.type !== "property") { - return; - } - const inputName = inputNode.keyNode.value as string; - const input = workflowInputs.find((i) => i.name === inputName); - if (!input) { - const range = Range.create( - testDocument.textDocument.positionAt(inputNode.offset), - testDocument.textDocument.positionAt(inputNode.offset + inputNode.length) - ); - const message = `Input "${inputName}" is not defined in the associated workflow.`; - diagnostics.push(Diagnostic.create(range, message, DiagnosticSeverity.Error)); - } - }); - return diagnostics; - } - - private async validateWorkflowOutputs(testDocument: WorkflowTestsDocument): Promise { - const diagnostics: Diagnostic[] = []; - const workflowOutputs = await testDocument.getWorkflowOutputs(); - const documentOutputNodes = - testDocument.nodeManager.getAllPropertyNodesByName("outputs")[0]?.valueNode?.children ?? []; - documentOutputNodes.forEach((outputNode) => { - if (outputNode.type !== "property") { - return; - } - const outputName = outputNode.keyNode.value as string; - const output = workflowOutputs.find((o) => o.name === outputName); - if (!output) { - const range = Range.create( - testDocument.textDocument.positionAt(outputNode.offset), - testDocument.textDocument.positionAt(outputNode.offset + outputNode.length) - ); - const message = `Output "${outputName}" is not defined in the associated workflow.`; - diagnostics.push(Diagnostic.create(range, message, DiagnosticSeverity.Error)); - } - }); - return diagnostics; + return schemaValidation; } } diff --git a/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowInputsValidationRule.ts b/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowInputsValidationRule.ts new file mode 100644 index 0000000..325e544 --- /dev/null +++ b/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowInputsValidationRule.ts @@ -0,0 +1,29 @@ +import { ValidationRule, WorkflowTestsDocument } from "@gxwf/server-common/src/languageTypes"; +import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; + +/** + * Validation rule to check if the inputs defined in the test document are present in the associated workflow. + */ +export class WorkflowInputsValidationRule implements ValidationRule { + constructor(readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error) {} + + public async validate(documentContext: WorkflowTestsDocument): Promise { + const diagnostics: Diagnostic[] = []; + const workflowInputs = await documentContext.getWorkflowInputs(); + const documentInputNodes = + documentContext.nodeManager.getAllPropertyNodesByName("job")[0]?.valueNode?.children ?? []; + documentInputNodes.forEach((inputNode) => { + if (inputNode.type !== "property") { + return; + } + const inputName = inputNode.keyNode.value as string; + const input = workflowInputs.find((i) => i.name === inputName); + if (!input) { + const range = documentContext.nodeManager.getNodeRange(inputNode); + const message = `Input "${inputName}" is not defined in the associated workflow.`; + diagnostics.push(Diagnostic.create(range, message, DiagnosticSeverity.Error)); + } + }); + return Promise.resolve(diagnostics); + } +} diff --git a/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowOutputsValidationRule.ts b/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowOutputsValidationRule.ts new file mode 100644 index 0000000..883c391 --- /dev/null +++ b/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowOutputsValidationRule.ts @@ -0,0 +1,29 @@ +import { ValidationRule, WorkflowTestsDocument } from "@gxwf/server-common/src/languageTypes"; +import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; + +/** + * Validation rule to check if the outputs defined in the test document are present in the associated workflow. + */ +export class WorkflowOutputsValidationRule implements ValidationRule { + constructor(readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error) {} + + public async validate(documentContext: WorkflowTestsDocument): Promise { + const diagnostics: Diagnostic[] = []; + const workflowOutputs = await documentContext.getWorkflowOutputs(); + const documentOutputNodes = + documentContext.nodeManager.getAllPropertyNodesByName("outputs")[0]?.valueNode?.children ?? []; + documentOutputNodes.forEach((outputNode) => { + if (outputNode.type !== "property") { + return; + } + const outputName = outputNode.keyNode.value as string; + const output = workflowOutputs.find((o) => o.name === outputName); + if (!output) { + const range = documentContext.nodeManager.getNodeRange(outputNode); + const message = `Output "${outputName}" is not defined in the associated workflow.`; + diagnostics.push(Diagnostic.create(range, message, DiagnosticSeverity.Error)); + } + }); + return Promise.resolve(diagnostics); + } +} From b0252aa60bc34c10b279041f3be873d12444ce1f Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sun, 16 Jun 2024 20:30:57 +0200 Subject: [PATCH 16/43] Setup basic validation profiles for all language services --- server/gx-workflow-ls-format2/src/languageService.ts | 3 ++- server/gx-workflow-ls-native/src/languageService.ts | 3 ++- .../workflow-tests-language-service/src/languageService.ts | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/server/gx-workflow-ls-format2/src/languageService.ts b/server/gx-workflow-ls-format2/src/languageService.ts index 7c13f35..f923c97 100644 --- a/server/gx-workflow-ls-format2/src/languageService.ts +++ b/server/gx-workflow-ls-format2/src/languageService.ts @@ -17,7 +17,7 @@ import { TYPES as YAML_TYPES } from "@gxwf/yaml-language-service/src/inversify.c import { YAMLLanguageService } from "@gxwf/yaml-language-service/src/yamlLanguageService"; import { inject, injectable } from "inversify"; import { GxFormat2WorkflowDocument } from "./gxFormat2WorkflowDocument"; -import { GxFormat2IWCValidationProfile } from "./profiles"; +import { GxFormat2BasicValidationProfile, GxFormat2IWCValidationProfile } from "./profiles"; import { GalaxyWorkflowFormat2SchemaLoader } from "./schema"; import { GxFormat2CompletionService } from "./services/completionService"; import { GxFormat2HoverService } from "./services/hoverService"; @@ -76,6 +76,7 @@ export class GxFormat2WorkflowLanguageServiceImpl protected override initializeValidationProfiles(): void { super.initializeValidationProfiles(); + this.validationProfiles.set("basic", new GxFormat2BasicValidationProfile()); this.validationProfiles.set("iwc", new GxFormat2IWCValidationProfile()); } diff --git a/server/gx-workflow-ls-native/src/languageService.ts b/server/gx-workflow-ls-native/src/languageService.ts index bf9e54f..1f0b35e 100644 --- a/server/gx-workflow-ls-native/src/languageService.ts +++ b/server/gx-workflow-ls-native/src/languageService.ts @@ -25,7 +25,7 @@ import { } from "vscode-json-languageservice"; import NativeWorkflowSchema from "../../../workflow-languages/schemas/native.schema.json"; import { NativeWorkflowDocument } from "./nativeWorkflowDocument"; -import { NativeIWCValidationProfile } from "./profiles"; +import { NativeBasicValidationProfile, NativeIWCValidationProfile } from "./profiles"; const LANGUAGE_ID = "galaxyworkflow"; @@ -89,6 +89,7 @@ export class NativeWorkflowLanguageServiceImpl protected override initializeValidationProfiles(): void { super.initializeValidationProfiles(); + this.validationProfiles.set("basic", new NativeBasicValidationProfile()); this.validationProfiles.set("iwc", new NativeIWCValidationProfile()); } diff --git a/server/packages/workflow-tests-language-service/src/languageService.ts b/server/packages/workflow-tests-language-service/src/languageService.ts index 3741c23..1bf1947 100644 --- a/server/packages/workflow-tests-language-service/src/languageService.ts +++ b/server/packages/workflow-tests-language-service/src/languageService.ts @@ -16,7 +16,7 @@ import { TYPES as YAML_TYPES } from "@gxwf/yaml-language-service/src/inversify.c import { YAMLLanguageService } from "@gxwf/yaml-language-service/src/yamlLanguageService"; import { inject, injectable } from "inversify"; import { GxWorkflowTestsDocument } from "./document"; -import { TestDocumentIWCValidationProfile } from "./profiles"; +import { TestDocumentBasicValidationProfile, TestDocumentIWCValidationProfile } from "./profiles"; import { WorkflowTestsCompletionService } from "./services/completion"; import { WorkflowTestsHoverService } from "./services/hover"; import { WorkflowTestsValidationService } from "./services/validation"; @@ -63,6 +63,7 @@ export class GxWorkflowTestsLanguageServiceImpl extends LanguageServiceBase Date: Sun, 16 Jun 2024 20:55:48 +0200 Subject: [PATCH 17/43] Refactor validation rule tests for wf test documents --- .../tests/unit/validation.test.ts | 66 ++++++++++++++----- .../tsconfig.json | 1 - 2 files changed, 51 insertions(+), 16 deletions(-) diff --git a/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts b/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts index c7f0692..7742e53 100644 --- a/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts +++ b/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts @@ -1,10 +1,17 @@ import { container } from "@gxwf/server-common/src/inversify.config"; -import { Diagnostic, DiagnosticSeverity, WorkflowDataProvider } from "@gxwf/server-common/src/languageTypes"; +import { + Diagnostic, + DiagnosticSeverity, + ValidationRule, + WorkflowDataProvider, +} from "@gxwf/server-common/src/languageTypes"; import { FAKE_WORKFLOW_DATA_PROVIDER } from "@gxwf/server-common/tests/testHelpers"; import { WorkflowTestsLanguageServiceContainerModule } from "@gxwf/workflow-tests-language-service/src/inversify.config"; import { WorkflowTestsValidationService } from "@gxwf/workflow-tests-language-service/src/services/validation"; import "reflect-metadata"; import { TYPES } from "../../src/types"; +import { WorkflowInputsValidationRule } from "../../src/validation/rules/WorkflowInputsValidationRule"; +import { WorkflowOutputsValidationRule } from "../../src/validation/rules/WorkflowOutputsValidationRule"; import { createGxWorkflowTestsDocument } from "../testHelpers"; describe("Workflow Tests Validation Service", () => { @@ -35,15 +42,30 @@ describe("Workflow Tests Validation Service", () => { expect(diagnostics[1].message).toBe('Missing property "outputs".'); expect(diagnostics[1].severity).toBe(DiagnosticSeverity.Warning); }); +}); + +describe("Workflow Tests Validation Rules", () => { + let rule: ValidationRule; - describe("Workflow Inputs/Outputs Validation", () => { - it("should pass validation when the inputs and outputs are defined in the workflow", async () => { + async function validate( + contents: string, + workflowDataProvider: WorkflowDataProvider = FAKE_WORKFLOW_DATA_PROVIDER + ): Promise { + const documentContext = createGxWorkflowTestsDocument(contents, workflowDataProvider); + return await rule.validate(documentContext); + } + + describe("WorkflowInputsValidationRule", () => { + beforeAll(() => { + rule = new WorkflowInputsValidationRule(); + }); + + it("should pass validation when a valid input is defined in the workflow", async () => { const testDocumentContents = ` -- doc: The docs - job: - My fake dataset: data/input.txt - outputs: - My output: out/output.txt`; + - doc: The docs + job: + My fake dataset: data/input.txt + `; const diagnostics = await validate(testDocumentContents); @@ -52,11 +74,10 @@ describe("Workflow Tests Validation Service", () => { it("should error when an input is not defined in the workflow", async () => { const testDocumentContents = ` -- doc: The docs - job: - Missing input: data/input.txt - outputs: - My output: out/output.txt`; + - doc: The docs + job: + Missing input: data/input.txt + `; const diagnostics = await validate(testDocumentContents); @@ -64,12 +85,27 @@ describe("Workflow Tests Validation Service", () => { expect(diagnostics[0].message).toBe('Input "Missing input" is not defined in the associated workflow.'); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); + }); + + describe("WorkflowOutputsValidationRule", () => { + beforeAll(() => { + rule = new WorkflowOutputsValidationRule(); + }); + + it("should pass validation when a valid outputs is defined in the workflow", async () => { + const testDocumentContents = ` + - doc: The docs + outputs: + My output: out/output.txt`; + + const diagnostics = await validate(testDocumentContents); + + expect(diagnostics).not.toBeNull(); + }); it("should error when an output is not defined in the workflow", async () => { const testDocumentContents = ` - doc: The docs - job: - My fake dataset: data/input.txt outputs: Missing output: out/output.txt`; diff --git a/server/packages/workflow-tests-language-service/tsconfig.json b/server/packages/workflow-tests-language-service/tsconfig.json index fe8c62d..ae51428 100644 --- a/server/packages/workflow-tests-language-service/tsconfig.json +++ b/server/packages/workflow-tests-language-service/tsconfig.json @@ -12,7 +12,6 @@ "sourceMap": true, "strict": true, "composite": true, - "rootDirs": ["src", "tests"], "rootDir": "../../../" }, "include": ["src/**/*", "../../../workflow-languages/schemas/*.json"], From 5b2e93b64ec40ff5311343f333af9309d38413db Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sun, 16 Jun 2024 21:01:37 +0200 Subject: [PATCH 18/43] Refactor tests --- .../tests/unit/customValidationRules.test.ts | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts index 7540787..27f8d78 100644 --- a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts +++ b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts @@ -1,14 +1,14 @@ -import { DiagnosticSeverity } from "@gxwf/server-common/src/languageTypes"; +import { DiagnosticSeverity, ValidationRule } from "@gxwf/server-common/src/languageTypes"; import { MissingPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; import { WorkflowOutputLabelValidationRule } from "../../src/validation/rules/WorkflowOutputLabelValidationRule"; import { createNativeWorkflowDocument } from "../testHelpers"; import { TestWorkflowProvider } from "../testWorkflowProvider"; describe("Custom Validation Rules", () => { - describe("WorkflowOutputLabelValidation Rule", () => { - let rule: WorkflowOutputLabelValidationRule; + let rule: ValidationRule; - beforeEach(() => { + describe("WorkflowOutputLabelValidation Rule", () => { + beforeAll(() => { rule = new WorkflowOutputLabelValidationRule(); }); @@ -45,9 +45,7 @@ describe("Custom Validation Rules", () => { }); describe("MissingPropertyValidation Rule", () => { - let rule: MissingPropertyValidationRule; - - beforeEach(() => { + beforeAll(() => { rule = new MissingPropertyValidationRule("release"); }); @@ -109,7 +107,7 @@ describe("Custom Validation Rules", () => { }); describe("when valueRequired is false", () => { - beforeEach(() => { + beforeAll(() => { rule = new MissingPropertyValidationRule("release", false); }); @@ -146,7 +144,7 @@ describe("Custom Validation Rules", () => { }); describe("when the property is an array", () => { - beforeEach(() => { + beforeAll(() => { rule = new MissingPropertyValidationRule("creator"); }); @@ -174,7 +172,7 @@ describe("Custom Validation Rules", () => { }); describe("when the property is an object", () => { - beforeEach(() => { + beforeAll(() => { rule = new MissingPropertyValidationRule("steps"); }); @@ -202,7 +200,7 @@ describe("Custom Validation Rules", () => { }); describe("when the property is nested", () => { - beforeEach(() => { + beforeAll(() => { rule = new MissingPropertyValidationRule("steps/0/tool_id"); }); From 9b65f9fece5b6388704cdc15200f594d053ff484 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sun, 16 Jun 2024 21:28:35 +0200 Subject: [PATCH 19/43] Adapt e2e tests --- client/tests/e2e/suite/extension.ga.e2e.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/client/tests/e2e/suite/extension.ga.e2e.ts b/client/tests/e2e/suite/extension.ga.e2e.ts index 1f387db..d5bafb9 100644 --- a/client/tests/e2e/suite/extension.ga.e2e.ts +++ b/client/tests/e2e/suite/extension.ga.e2e.ts @@ -52,7 +52,17 @@ suite("Native (JSON) Workflows", () => { await waitForDiagnostics(docUri); await assertDiagnostics(docUri, [ { - message: 'Missing property "release".', + message: "The workflow must have a release version.", + range: new vscode.Range(new vscode.Position(0, 0), new vscode.Position(0, 1)), + severity: vscode.DiagnosticSeverity.Error, + }, + { + message: "The workflow does not specify a creator.", + range: new vscode.Range(new vscode.Position(0, 0), new vscode.Position(0, 1)), + severity: vscode.DiagnosticSeverity.Error, + }, + { + message: "The workflow does not specify a license.", range: new vscode.Range(new vscode.Position(0, 0), new vscode.Position(0, 1)), severity: vscode.DiagnosticSeverity.Error, }, From 0730fd76df83b3df0ed332240360c87726ff1e3f Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Sun, 16 Jun 2024 21:31:53 +0200 Subject: [PATCH 20/43] Refactor rename validation rule from missing to required --- server/gx-workflow-ls-format2/src/profiles.ts | 4 ++-- server/gx-workflow-ls-native/src/profiles.ts | 4 ++-- .../tests/unit/customValidationRules.test.ts | 16 ++++++++-------- .../src/providers/validation/profiles.ts | 8 ++++---- ...tion.ts => RequiredPropertyValidationRule.ts} | 2 +- .../src/providers/validation/rules/index.ts | 4 ++-- .../src/profiles.ts | 8 ++++---- 7 files changed, 23 insertions(+), 23 deletions(-) rename server/packages/server-common/src/providers/validation/rules/{MissingPropertyValidation.ts => RequiredPropertyValidationRule.ts} (95%) diff --git a/server/gx-workflow-ls-format2/src/profiles.ts b/server/gx-workflow-ls-format2/src/profiles.ts index 6585d97..b45d27c 100644 --- a/server/gx-workflow-ls-format2/src/profiles.ts +++ b/server/gx-workflow-ls-format2/src/profiles.ts @@ -3,7 +3,7 @@ import { BasicCommonValidationProfile, IWCCommonValidationProfile, } from "@gxwf/server-common/src/providers/validation/profiles"; -import { MissingPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; +import { RequiredPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; /** * Defines the minimal set of validation rules for gxformat2 Galaxy workflows. @@ -28,7 +28,7 @@ export class GxFormat2IWCValidationProfile extends IWCCommonValidationProfile { protected static readonly RULES = new Set([ ...super.RULES, ...GxFormat2BasicValidationProfile.RULES, - new MissingPropertyValidationRule( + new RequiredPropertyValidationRule( "doc", true, DiagnosticSeverity.Error, diff --git a/server/gx-workflow-ls-native/src/profiles.ts b/server/gx-workflow-ls-native/src/profiles.ts index f75d342..8af2fdb 100644 --- a/server/gx-workflow-ls-native/src/profiles.ts +++ b/server/gx-workflow-ls-native/src/profiles.ts @@ -3,7 +3,7 @@ import { BasicCommonValidationProfile, IWCCommonValidationProfile, } from "@gxwf/server-common/src/providers/validation/profiles"; -import { MissingPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; +import { RequiredPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; import { WorkflowOutputLabelValidationRule } from "./validation/rules/WorkflowOutputLabelValidationRule"; /** @@ -29,7 +29,7 @@ export class NativeIWCValidationProfile extends IWCCommonValidationProfile { protected static readonly RULES = new Set([ ...super.RULES, ...NativeBasicValidationProfile.RULES, - new MissingPropertyValidationRule( + new RequiredPropertyValidationRule( "annotation", true, DiagnosticSeverity.Error, diff --git a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts index 27f8d78..c2696a9 100644 --- a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts +++ b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts @@ -1,5 +1,5 @@ import { DiagnosticSeverity, ValidationRule } from "@gxwf/server-common/src/languageTypes"; -import { MissingPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; +import { RequiredPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; import { WorkflowOutputLabelValidationRule } from "../../src/validation/rules/WorkflowOutputLabelValidationRule"; import { createNativeWorkflowDocument } from "../testHelpers"; import { TestWorkflowProvider } from "../testWorkflowProvider"; @@ -46,7 +46,7 @@ describe("Custom Validation Rules", () => { describe("MissingPropertyValidation Rule", () => { beforeAll(() => { - rule = new MissingPropertyValidationRule("release"); + rule = new RequiredPropertyValidationRule("release"); }); it("should not provide diagnostics when the property is present", async () => { @@ -83,7 +83,7 @@ describe("Custom Validation Rules", () => { }); it("should provide warning diagnostics when the property is missing and severity is set to warning", async () => { - rule = new MissingPropertyValidationRule("release", true, DiagnosticSeverity.Warning); + rule = new RequiredPropertyValidationRule("release", true, DiagnosticSeverity.Warning); const wfContents = `{ "a_galaxy_workflow": "true", }`; @@ -95,7 +95,7 @@ describe("Custom Validation Rules", () => { }); it("should display a custom message when provided", async () => { - rule = new MissingPropertyValidationRule("release", true, DiagnosticSeverity.Warning, "Custom message"); + rule = new RequiredPropertyValidationRule("release", true, DiagnosticSeverity.Warning, "Custom message"); const wfContents = `{ "a_galaxy_workflow": "true", }`; @@ -108,7 +108,7 @@ describe("Custom Validation Rules", () => { describe("when valueRequired is false", () => { beforeAll(() => { - rule = new MissingPropertyValidationRule("release", false); + rule = new RequiredPropertyValidationRule("release", false); }); it("should not provide diagnostics when the property is present", async () => { @@ -145,7 +145,7 @@ describe("Custom Validation Rules", () => { describe("when the property is an array", () => { beforeAll(() => { - rule = new MissingPropertyValidationRule("creator"); + rule = new RequiredPropertyValidationRule("creator"); }); it("should not provide diagnostics when the property has a value", async () => { @@ -173,7 +173,7 @@ describe("Custom Validation Rules", () => { describe("when the property is an object", () => { beforeAll(() => { - rule = new MissingPropertyValidationRule("steps"); + rule = new RequiredPropertyValidationRule("steps"); }); it("should not provide diagnostics when the property has a value", async () => { @@ -201,7 +201,7 @@ describe("Custom Validation Rules", () => { describe("when the property is nested", () => { beforeAll(() => { - rule = new MissingPropertyValidationRule("steps/0/tool_id"); + rule = new RequiredPropertyValidationRule("steps/0/tool_id"); }); it("should not provide diagnostics when the property has a value", async () => { diff --git a/server/packages/server-common/src/providers/validation/profiles.ts b/server/packages/server-common/src/providers/validation/profiles.ts index 02a4380..4bf44f9 100644 --- a/server/packages/server-common/src/providers/validation/profiles.ts +++ b/server/packages/server-common/src/providers/validation/profiles.ts @@ -1,5 +1,5 @@ import { DiagnosticSeverity, ValidationProfile, ValidationRule } from "../../languageTypes"; -import { MissingPropertyValidationRule, StepExportErrorValidationRule, TestToolshedValidationRule } from "./rules"; +import { RequiredPropertyValidationRule, StepExportErrorValidationRule, TestToolshedValidationRule } from "./rules"; /** * The *NoOp* validation profile. @@ -38,19 +38,19 @@ export class IWCCommonValidationProfile implements ValidationProfile { public readonly name: string = "IWC Best Practices"; protected static readonly RULES: Set = new Set([ - new MissingPropertyValidationRule( + new RequiredPropertyValidationRule( "release", true, DiagnosticSeverity.Error, "The workflow must have a release version." ), - new MissingPropertyValidationRule( + new RequiredPropertyValidationRule( "creator", true, DiagnosticSeverity.Error, "The workflow does not specify a creator." ), - new MissingPropertyValidationRule( + new RequiredPropertyValidationRule( "license", true, DiagnosticSeverity.Error, diff --git a/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts b/server/packages/server-common/src/providers/validation/rules/RequiredPropertyValidationRule.ts similarity index 95% rename from server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts rename to server/packages/server-common/src/providers/validation/rules/RequiredPropertyValidationRule.ts index 7305da5..47ae65f 100644 --- a/server/packages/server-common/src/providers/validation/rules/MissingPropertyValidation.ts +++ b/server/packages/server-common/src/providers/validation/rules/RequiredPropertyValidationRule.ts @@ -11,7 +11,7 @@ import { DocumentContext, ValidationRule } from "../../../languageTypes"; * property is an object or an array, the rule will also check that it has at * least one property or item. */ -export class MissingPropertyValidationRule implements ValidationRule { +export class RequiredPropertyValidationRule implements ValidationRule { constructor( private nodePath: string, private valueRequired: boolean = true, diff --git a/server/packages/server-common/src/providers/validation/rules/index.ts b/server/packages/server-common/src/providers/validation/rules/index.ts index fa1a219..111e796 100644 --- a/server/packages/server-common/src/providers/validation/rules/index.ts +++ b/server/packages/server-common/src/providers/validation/rules/index.ts @@ -1,5 +1,5 @@ -import { MissingPropertyValidationRule } from "./MissingPropertyValidation"; +import { RequiredPropertyValidationRule } from "./RequiredPropertyValidationRule"; import { StepExportErrorValidationRule } from "./StepErrorValidationRule"; import { TestToolshedValidationRule } from "./TestToolShedValidationRule"; -export { MissingPropertyValidationRule, StepExportErrorValidationRule, TestToolshedValidationRule }; +export { RequiredPropertyValidationRule, StepExportErrorValidationRule, TestToolshedValidationRule }; diff --git a/server/packages/workflow-tests-language-service/src/profiles.ts b/server/packages/workflow-tests-language-service/src/profiles.ts index fbfcb36..9f086d4 100644 --- a/server/packages/workflow-tests-language-service/src/profiles.ts +++ b/server/packages/workflow-tests-language-service/src/profiles.ts @@ -3,7 +3,7 @@ import { BasicCommonValidationProfile, IWCCommonValidationProfile, } from "@gxwf/server-common/src/providers/validation/profiles"; -import { MissingPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; +import { RequiredPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; import { WorkflowInputsValidationRule } from "./validation/rules/WorkflowInputsValidationRule"; import { WorkflowOutputsValidationRule } from "./validation/rules/WorkflowOutputsValidationRule"; @@ -30,13 +30,13 @@ export class TestDocumentBasicValidationProfile extends BasicCommonValidationPro */ export class TestDocumentIWCValidationProfile extends IWCCommonValidationProfile { protected static readonly RULES = new Set([ - ...super.RULES, ...TestDocumentBasicValidationProfile.RULES, - new MissingPropertyValidationRule( + // TODO: This rule needs to be updated to check for the presence of the `doc` property in each test. + new RequiredPropertyValidationRule( "doc", true, DiagnosticSeverity.Error, - "The workflow is not documented. Documenting workflows helps users understand the purpose of the workflow." + "The workflow test is not documented. Documenting workflows helps users understand the purpose of the workflow." ), // Add more custom rules specific to native workflows here... ]); From c21813b405db6c7189864dfcb3d31fd00402097a Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 18 Jun 2024 09:13:12 +0200 Subject: [PATCH 21/43] Consider null value as empty value --- server/packages/server-common/src/ast/nodeManager.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/packages/server-common/src/ast/nodeManager.ts b/server/packages/server-common/src/ast/nodeManager.ts index 034c92a..224ca90 100644 --- a/server/packages/server-common/src/ast/nodeManager.ts +++ b/server/packages/server-common/src/ast/nodeManager.ts @@ -201,6 +201,8 @@ export class ASTNodeManager { return !node.valueNode || this.isNodeEmpty(node.valueNode); case "string": return node.value === ""; + case "null": + return true; default: return false; } From 53d1f462d7be555dcfd8473ec7bb7a168bb7d97f Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 18 Jun 2024 09:15:20 +0200 Subject: [PATCH 22/43] Add RequiredArrayPropertyValidationRule + tests --- .../tests/unit/customValidationRules.test.ts | 18 ++-- .../RequiredArrayPropertyValidationRule.ts | 42 ++++++++++ .../rules/RequiredPropertyValidationRule.ts | 4 +- .../src/providers/validation/rules/index.ts | 8 +- .../src/profiles.ts | 13 ++- .../src/schema/adapter.ts | 2 +- .../tests/unit/validation.test.ts | 82 ++++++++++++++++++- 7 files changed, 147 insertions(+), 22 deletions(-) create mode 100644 server/packages/server-common/src/providers/validation/rules/RequiredArrayPropertyValidationRule.ts diff --git a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts index c2696a9..2d2dc3d 100644 --- a/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts +++ b/server/gx-workflow-ls-native/tests/unit/customValidationRules.test.ts @@ -44,7 +44,7 @@ describe("Custom Validation Rules", () => { }); }); - describe("MissingPropertyValidation Rule", () => { + describe("Required Propery Validation Rule", () => { beforeAll(() => { rule = new RequiredPropertyValidationRule("release"); }); @@ -66,7 +66,7 @@ describe("Custom Validation Rules", () => { const wfDocument = createNativeWorkflowDocument(wfContents); const diagnostics = await rule.validate(wfDocument); expect(diagnostics).toHaveLength(1); - expect(diagnostics[0].message).toBe('Missing property "release".'); + expect(diagnostics[0].message).toBe('Missing required property "release".'); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); @@ -78,7 +78,7 @@ describe("Custom Validation Rules", () => { const wfDocument = createNativeWorkflowDocument(wfContents); const diagnostics = await rule.validate(wfDocument); expect(diagnostics).toHaveLength(1); - expect(diagnostics[0].message).toBe('Missing value in property "release".'); + expect(diagnostics[0].message).toBe('Missing required value in property "release".'); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); @@ -90,7 +90,7 @@ describe("Custom Validation Rules", () => { const wfDocument = createNativeWorkflowDocument(wfContents); const diagnostics = await rule.validate(wfDocument); expect(diagnostics).toHaveLength(1); - expect(diagnostics[0].message).toBe('Missing property "release".'); + expect(diagnostics[0].message).toBe('Missing required property "release".'); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Warning); }); @@ -128,7 +128,7 @@ describe("Custom Validation Rules", () => { const wfDocument = createNativeWorkflowDocument(wfContents); const diagnostics = await rule.validate(wfDocument); expect(diagnostics).toHaveLength(1); - expect(diagnostics[0].message).toBe('Missing property "release".'); + expect(diagnostics[0].message).toBe('Missing required property "release".'); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); @@ -166,7 +166,7 @@ describe("Custom Validation Rules", () => { const wfDocument = createNativeWorkflowDocument(wfContents); const diagnostics = await rule.validate(wfDocument); expect(diagnostics).toHaveLength(1); - expect(diagnostics[0].message).toBe('Missing value in property "creator".'); + expect(diagnostics[0].message).toBe('Missing required value in property "creator".'); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); }); @@ -194,7 +194,7 @@ describe("Custom Validation Rules", () => { const wfDocument = createNativeWorkflowDocument(wfContents); const diagnostics = await rule.validate(wfDocument); expect(diagnostics).toHaveLength(1); - expect(diagnostics[0].message).toBe('Missing value in property "steps".'); + expect(diagnostics[0].message).toBe('Missing required value in property "steps".'); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); }); @@ -222,7 +222,7 @@ describe("Custom Validation Rules", () => { const wfDocument = createNativeWorkflowDocument(wfContents); const diagnostics = await rule.validate(wfDocument); expect(diagnostics).toHaveLength(1); - expect(diagnostics[0].message).toBe('Missing property "steps/0/tool_id".'); + expect(diagnostics[0].message).toBe('Missing required property "steps/0/tool_id".'); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); @@ -234,7 +234,7 @@ describe("Custom Validation Rules", () => { const wfDocument = createNativeWorkflowDocument(wfContents); const diagnostics = await rule.validate(wfDocument); expect(diagnostics).toHaveLength(1); - expect(diagnostics[0].message).toBe('Missing value in property "steps/0/tool_id".'); + expect(diagnostics[0].message).toBe('Missing required value in property "steps/0/tool_id".'); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); }); diff --git a/server/packages/server-common/src/providers/validation/rules/RequiredArrayPropertyValidationRule.ts b/server/packages/server-common/src/providers/validation/rules/RequiredArrayPropertyValidationRule.ts new file mode 100644 index 0000000..3e95f9e --- /dev/null +++ b/server/packages/server-common/src/providers/validation/rules/RequiredArrayPropertyValidationRule.ts @@ -0,0 +1,42 @@ +import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; +import { DocumentContext, ValidationRule } from "../../../languageTypes"; + +export class RequiredArrayPropertyValidationRule implements ValidationRule { + constructor( + private propertyName: string, + private valueRequired: boolean = true, + private severity: DiagnosticSeverity = DiagnosticSeverity.Error, + private message?: string + ) {} + + public async validate(documentContext: DocumentContext): Promise { + const result: Diagnostic[] = []; + if (documentContext.nodeManager.root?.type !== "array") { + return Promise.resolve(result); + } + for (const node of documentContext.nodeManager.root.items) { + if (node.type !== "object") { + continue; + } + const targetNode = node.properties.find((p) => p.keyNode.value === this.propertyName); + if (!targetNode) { + result.push({ + message: this.message ?? `Missing required property "${this.propertyName}".`, + range: documentContext.nodeManager.getNodeRange(node), + severity: this.severity, + }); + } + if (this.valueRequired && targetNode) { + const missingValue = documentContext.nodeManager.isNodeEmpty(targetNode); + if (missingValue) { + result.push({ + message: `Missing required value in property "${this.propertyName}".`, + range: documentContext.nodeManager.getNodeRange(targetNode), + severity: this.severity, + }); + } + } + } + return Promise.resolve(result); + } +} diff --git a/server/packages/server-common/src/providers/validation/rules/RequiredPropertyValidationRule.ts b/server/packages/server-common/src/providers/validation/rules/RequiredPropertyValidationRule.ts index 47ae65f..66986fb 100644 --- a/server/packages/server-common/src/providers/validation/rules/RequiredPropertyValidationRule.ts +++ b/server/packages/server-common/src/providers/validation/rules/RequiredPropertyValidationRule.ts @@ -24,7 +24,7 @@ export class RequiredPropertyValidationRule implements ValidationRule { const targetNode = documentContext.nodeManager.getNodeFromPath(this.nodePath); if (!targetNode) { result.push({ - message: this.message ?? `Missing property "${this.nodePath}".`, + message: this.message ?? `Missing required property "${this.nodePath}".`, range: documentContext.nodeManager.getDefaultRange(), severity: this.severity, }); @@ -34,7 +34,7 @@ export class RequiredPropertyValidationRule implements ValidationRule { const missingValue = documentContext.nodeManager.isNodeEmpty(targetNode); if (missingValue) { result.push({ - message: `Missing value in property "${this.nodePath}".`, + message: `Missing required value in property "${this.nodePath}".`, range: documentContext.nodeManager.getNodeRange(targetNode), severity: this.severity, }); diff --git a/server/packages/server-common/src/providers/validation/rules/index.ts b/server/packages/server-common/src/providers/validation/rules/index.ts index 111e796..294f5cf 100644 --- a/server/packages/server-common/src/providers/validation/rules/index.ts +++ b/server/packages/server-common/src/providers/validation/rules/index.ts @@ -1,5 +1,11 @@ +import { RequiredArrayPropertyValidationRule } from "./RequiredArrayPropertyValidationRule"; import { RequiredPropertyValidationRule } from "./RequiredPropertyValidationRule"; import { StepExportErrorValidationRule } from "./StepErrorValidationRule"; import { TestToolshedValidationRule } from "./TestToolShedValidationRule"; -export { RequiredPropertyValidationRule, StepExportErrorValidationRule, TestToolshedValidationRule }; +export { + RequiredArrayPropertyValidationRule, + RequiredPropertyValidationRule, + StepExportErrorValidationRule, + TestToolshedValidationRule, +}; diff --git a/server/packages/workflow-tests-language-service/src/profiles.ts b/server/packages/workflow-tests-language-service/src/profiles.ts index 9f086d4..785b908 100644 --- a/server/packages/workflow-tests-language-service/src/profiles.ts +++ b/server/packages/workflow-tests-language-service/src/profiles.ts @@ -3,12 +3,12 @@ import { BasicCommonValidationProfile, IWCCommonValidationProfile, } from "@gxwf/server-common/src/providers/validation/profiles"; -import { RequiredPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; +import { RequiredArrayPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; import { WorkflowInputsValidationRule } from "./validation/rules/WorkflowInputsValidationRule"; import { WorkflowOutputsValidationRule } from "./validation/rules/WorkflowOutputsValidationRule"; /** - * Defines the minimal set of validation rules for Native Galaxy workflows. + * Defines the minimal set of validation rules for Galaxy Workflow Tests Documents. */ export class TestDocumentBasicValidationProfile extends BasicCommonValidationProfile { public static readonly RULES = new Set([ @@ -24,19 +24,18 @@ export class TestDocumentBasicValidationProfile extends BasicCommonValidationPro } /** - * *Intergalactic Workflow Commission* (IWC) validation profile for Native Galaxy workflows. + * *Intergalactic Workflow Commission* (IWC) validation profile for Galaxy Workflow Tests Documents. * This profile extends the basic validation profile and adds additional rules to comply * with the IWC best practices guidelines. */ export class TestDocumentIWCValidationProfile extends IWCCommonValidationProfile { protected static readonly RULES = new Set([ ...TestDocumentBasicValidationProfile.RULES, - // TODO: This rule needs to be updated to check for the presence of the `doc` property in each test. - new RequiredPropertyValidationRule( + new RequiredArrayPropertyValidationRule( "doc", true, - DiagnosticSeverity.Error, - "The workflow test is not documented. Documenting workflows helps users understand the purpose of the workflow." + DiagnosticSeverity.Warning, + "The workflow test is not documented. Documenting the tests helps reviewers understand the purpose of the tests." ), // Add more custom rules specific to native workflows here... ]); diff --git a/server/packages/workflow-tests-language-service/src/schema/adapter.ts b/server/packages/workflow-tests-language-service/src/schema/adapter.ts index 9ca59f7..5648b64 100644 --- a/server/packages/workflow-tests-language-service/src/schema/adapter.ts +++ b/server/packages/workflow-tests-language-service/src/schema/adapter.ts @@ -227,7 +227,7 @@ enum ProblemType { } const ProblemTypeMessages: Record = { - [ProblemType.missingRequiredPropWarning]: 'Missing property "{0}".', + [ProblemType.missingRequiredPropWarning]: 'Missing required property "{0}".', [ProblemType.typeMismatchWarning]: 'Incorrect type. Expected "{0}".', [ProblemType.constWarning]: "Value must be {0}.", }; diff --git a/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts b/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts index 7742e53..ba93fb7 100644 --- a/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts +++ b/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts @@ -5,6 +5,7 @@ import { ValidationRule, WorkflowDataProvider, } from "@gxwf/server-common/src/languageTypes"; +import { RequiredArrayPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; import { FAKE_WORKFLOW_DATA_PROVIDER } from "@gxwf/server-common/tests/testHelpers"; import { WorkflowTestsLanguageServiceContainerModule } from "@gxwf/workflow-tests-language-service/src/inversify.config"; import { WorkflowTestsValidationService } from "@gxwf/workflow-tests-language-service/src/services/validation"; @@ -37,9 +38,9 @@ describe("Workflow Tests Validation Service", () => { const diagnostics = await validate(testDocumentContents); expect(diagnostics.length).toBe(2); - expect(diagnostics[0].message).toBe('Missing property "job".'); + expect(diagnostics[0].message).toBe('Missing required property "job".'); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Warning); - expect(diagnostics[1].message).toBe('Missing property "outputs".'); + expect(diagnostics[1].message).toBe('Missing required property "outputs".'); expect(diagnostics[1].severity).toBe(DiagnosticSeverity.Warning); }); }); @@ -70,6 +71,7 @@ describe("Workflow Tests Validation Rules", () => { const diagnostics = await validate(testDocumentContents); expect(diagnostics).not.toBeNull(); + expect(diagnostics.length).toBe(0); }); it("should error when an input is not defined in the workflow", async () => { @@ -101,6 +103,7 @@ describe("Workflow Tests Validation Rules", () => { const diagnostics = await validate(testDocumentContents); expect(diagnostics).not.toBeNull(); + expect(diagnostics.length).toBe(0); }); it("should error when an output is not defined in the workflow", async () => { @@ -116,4 +119,79 @@ describe("Workflow Tests Validation Rules", () => { expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); }); + + describe("RequiredArrayPropertyValidationRule", () => { + beforeAll(() => { + rule = new RequiredArrayPropertyValidationRule("doc"); + }); + + it("should pass validation when the required property is defined in all items of an array", async () => { + const testDocumentContents = ` +- doc: The docs1 +- doc: The docs2 +- doc: The docs3 +`; + const diagnostics = await validate(testDocumentContents); + + expect(diagnostics).not.toBeNull(); + expect(diagnostics.length).toBe(0); + }); + + it("should error when a required property is missing", async () => { + const testDocumentContents = `- job:`; + + const diagnostics = await validate(testDocumentContents); + + expect(diagnostics.length).toBe(1); + expect(diagnostics[0].message).toBe('Missing required property "doc".'); + }); + + it("should error when a required property is empty", async () => { + const testDocumentContents = `- doc:`; + + const diagnostics = await validate(testDocumentContents); + + expect(diagnostics.length).toBe(1); + expect(diagnostics[0].message).toBe('Missing required value in property "doc".'); + }); + + it("should error when a required property is missing in some items of an array", async () => { + const testDocumentContents = ` +- doc: The docs1 +- job: +- doc: The docs3 +`; + const diagnostics = await validate(testDocumentContents); + + expect(diagnostics.length).toBe(1); + expect(diagnostics[0].message).toBe('Missing required property "doc".'); + }); + + it("should error when a required property is missing or empty in some items of an array", async () => { + const testDocumentContents = ` +- doc: The docs1 +- doc: +- job: +`; + const diagnostics = await validate(testDocumentContents); + + expect(diagnostics.length).toBe(2); + expect(diagnostics[0].message).toBe('Missing required value in property "doc".'); + expect(diagnostics[1].message).toBe('Missing required property "doc".'); + }); + + it("should error when a required property is missing in all items of an array", async () => { + const testDocumentContents = ` +- job: +- job: +- job: +`; + const diagnostics = await validate(testDocumentContents); + + expect(diagnostics.length).toBe(3); + for (const diagnostic of diagnostics) { + expect(diagnostic.message).toBe('Missing required property "doc".'); + } + }); + }); }); From 6581ae4d1530c1e26666bc91c332966e83049796 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 19 Jun 2024 13:49:25 +0200 Subject: [PATCH 23/43] Refactor parsing of format2 workflow inputs and outputs --- .../src/gxFormat2WorkflowDocument.ts | 88 +++++++++++-------- 1 file changed, 49 insertions(+), 39 deletions(-) diff --git a/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts b/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts index 72168e5..50d1044 100644 --- a/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts +++ b/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts @@ -5,6 +5,8 @@ import { TextDocument, WorkflowDataType, WorkflowDocument, + WorkflowInput, + WorkflowOutput, } from "@gxwf/server-common/src/languageTypes"; import { YAMLDocument } from "@gxwf/yaml-language-service/src"; @@ -23,54 +25,49 @@ export class GxFormat2WorkflowDocument extends WorkflowDocument { } public getWorkflowInputs(): GetWorkflowInputsResult { - const result: GetWorkflowInputsResult = { inputs: [] }; - const inputs = this.nodeManager.getNodeFromPath("inputs"); - if (inputs?.type === "property") { - const inputList = inputs.valueNode?.children; - if (inputList) { - inputList.forEach((input) => { - if (input.type !== "property" || !input.keyNode) return; - const inputName = String(input.keyNode.value); - const inputType = this.extractInputType(input); - const inputDocNode = input.valueNode?.children?.find( - (prop) => prop.type === "property" && prop.keyNode.value === "doc" - ) as PropertyASTNode; - const inputDescription = String(inputDocNode?.valueNode?.value ?? ""); - result.inputs.push({ - name: inputName, - doc: inputDescription, - type: inputType, - }); - }); - } - } - return result; + return { + inputs: this.getAllPropertyNodesAtPath("inputs").map((input) => this.parseInputDefinition(input)), + }; } public getWorkflowOutputs(): GetWorkflowOutputsResult { - const result: GetWorkflowOutputsResult = { outputs: [] }; - const output = this.nodeManager.getNodeFromPath("outputs"); - if (output?.type === "property") { - const outputList = output.valueNode?.children; - if (outputList) { - outputList.forEach((output) => { - if (output.type !== "property" || !output.keyNode) return; - const outputName = String(output.keyNode.value); - const outputDocNode = output.valueNode?.children?.find( - (prop) => prop.type === "property" && prop.keyNode.value === "doc" - ) as PropertyASTNode; - const outputDoc = String(outputDocNode?.valueNode?.value ?? ""); - result.outputs.push({ - name: outputName, - doc: outputDoc, - }); + return { + outputs: this.getAllPropertyNodesAtPath("outputs").map((output) => this.parseOutputDefinition(output)), + }; + } + + private getAllPropertyNodesAtPath(path: string): PropertyASTNode[] { + const result: PropertyASTNode[] = []; + const nodeAtPath = this.nodeManager.getNodeFromPath(path); + if (nodeAtPath?.type === "property") { + const propertyNodes = nodeAtPath.valueNode?.children; + if (propertyNodes) { + propertyNodes.forEach((node) => { + if (node.type !== "property" || !node.keyNode) return; + result.push(node); }); } } return result; } - private extractInputType(input: PropertyASTNode): WorkflowDataType { + private parseInputDefinition(input: PropertyASTNode): WorkflowInput { + const inputName = String(input.keyNode.value); + const inputType = this.parseInputType(input); + const inputDocNode = input.valueNode?.children?.find( + (prop) => prop.type === "property" && prop.keyNode.value === "doc" + ) as PropertyASTNode; + const inputDescription = String(inputDocNode?.valueNode?.value ?? ""); + const inputDefinition: WorkflowInput = { + name: inputName, + doc: inputDescription, + type: inputType, + //TODO: default + }; + return inputDefinition; + } + + private parseInputType(input: PropertyASTNode): WorkflowDataType { let inputType: WorkflowDataType = "data"; const inputTypeNode = input.valueNode?.children?.find( (prop) => prop.type === "property" && prop.keyNode.value === "type" @@ -83,4 +80,17 @@ export class GxFormat2WorkflowDocument extends WorkflowDocument { } return inputType; } + + private parseOutputDefinition(output: PropertyASTNode): WorkflowOutput { + const outputName = String(output.keyNode.value); + const outputDocNode = output.valueNode?.children?.find( + (prop) => prop.type === "property" && prop.keyNode.value === "doc" + ) as PropertyASTNode; + const outputDoc = String(outputDocNode?.valueNode?.value ?? ""); + const outputDefinition = { + name: outputName, + doc: outputDoc, + }; + return outputDefinition; + } } From 24c494bf373a11a19fbc456a68d3e0eae876e3f1 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 19 Jun 2024 19:03:09 +0200 Subject: [PATCH 24/43] Fix completionService to suggest available properties for 'report' --- .../src/services/completionService.ts | 2 ++ .../tests/integration/completion.test.ts | 14 ++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/server/gx-workflow-ls-format2/src/services/completionService.ts b/server/gx-workflow-ls-format2/src/services/completionService.ts index 5764eba..0043f3d 100644 --- a/server/gx-workflow-ls-format2/src/services/completionService.ts +++ b/server/gx-workflow-ls-format2/src/services/completionService.ts @@ -104,6 +104,8 @@ export class GxFormat2CompletionService { }; result.push(item); }); + } else if (schemaRecord instanceof RecordSchemaNode) { + return this.getProposedItems(schemaRecord, textBuffer, exclude, offset); } } return result; diff --git a/server/gx-workflow-ls-format2/tests/integration/completion.test.ts b/server/gx-workflow-ls-format2/tests/integration/completion.test.ts index a4421f5..2174957 100644 --- a/server/gx-workflow-ls-format2/tests/integration/completion.test.ts +++ b/server/gx-workflow-ls-format2/tests/integration/completion.test.ts @@ -301,4 +301,18 @@ outputs:`; const completionLabels = getCompletionItemsLabels(completions); expect(completionLabels).toEqual(EXPECTED_COMPLETION_LABELS); }); + + it("should suggest the list of available properties for 'report' when the cursor is inside the property", async () => { + const template = ` +class: GalaxyWorkflow +report: + $`; + const EXPECTED_COMPLETION_LABELS = ["markdown"]; + const { contents, position } = parseTemplate(template); + + const completions = await getCompletions(contents, position); + + const completionLabels = getCompletionItemsLabels(completions); + expect(completionLabels).toEqual(EXPECTED_COMPLETION_LABELS); + }); }); From 7fcc8f70603cd8d3e9c8328bca0e3a1f7fabbe26 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 19 Jun 2024 20:13:42 +0200 Subject: [PATCH 25/43] Improve schema validation for format2 workflows - Validate primitive types - Validate enums --- .../src/schema/definitions.ts | 12 ++++--- .../src/services/completionService.ts | 2 +- .../src/services/schemaValidationService.ts | 35 +++++++++++++++++-- 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/server/gx-workflow-ls-format2/src/schema/definitions.ts b/server/gx-workflow-ls-format2/src/schema/definitions.ts index b05a01e..14b87df 100644 --- a/server/gx-workflow-ls-format2/src/schema/definitions.ts +++ b/server/gx-workflow-ls-format2/src/schema/definitions.ts @@ -259,7 +259,7 @@ export class FieldSchemaNode implements SchemaNode, IdMapper { } public get canBeObject(): boolean { - return this.canBeAny || this._allowedTypes.some((t) => this.isObjectType(t.typeName)); + return this.canBeAny || this._allowedTypes.some((t) => this.isRecordType(t.typeName)); } public matchesType(typeName: string): boolean { @@ -313,11 +313,15 @@ export class FieldSchemaNode implements SchemaNode, IdMapper { return undefined; } - private isPrimitiveType(typeName: string): boolean { - return FieldSchemaNode.definitions.primitiveTypes.has(typeName); + public get isPrimitiveType(): boolean { + return FieldSchemaNode.definitions.primitiveTypes.has(this.typeRef); } - private isObjectType(typeName: string): boolean { + public get isObjectType(): boolean { + return this.isRecordType(this.typeRef); + } + + private isRecordType(typeName: string): boolean { return FieldSchemaNode.definitions.records.has(typeName); } diff --git a/server/gx-workflow-ls-format2/src/services/completionService.ts b/server/gx-workflow-ls-format2/src/services/completionService.ts index 0043f3d..c419ce6 100644 --- a/server/gx-workflow-ls-format2/src/services/completionService.ts +++ b/server/gx-workflow-ls-format2/src/services/completionService.ts @@ -68,7 +68,7 @@ export class GxFormat2CompletionService { result.push(item); }); } else if (schemaNode instanceof FieldSchemaNode) { - if (this.schemaNodeResolver.definitions.primitiveTypes.has(schemaNode.typeRef)) { + if (schemaNode.isPrimitiveType) { const defaultValue = String(schemaNode.default ?? ""); if (defaultValue) { const item: CompletionItem = { diff --git a/server/gx-workflow-ls-format2/src/services/schemaValidationService.ts b/server/gx-workflow-ls-format2/src/services/schemaValidationService.ts index 7ac026b..d4425fe 100644 --- a/server/gx-workflow-ls-format2/src/services/schemaValidationService.ts +++ b/server/gx-workflow-ls-format2/src/services/schemaValidationService.ts @@ -1,5 +1,5 @@ import { ASTNodeManager } from "@gxwf/server-common/src/ast/nodeManager"; -import { ASTNode, ObjectASTNode } from "@gxwf/server-common/src/ast/types"; +import { ASTNode, ObjectASTNode, StringASTNode } from "@gxwf/server-common/src/ast/types"; import { Diagnostic, DiagnosticSeverity, @@ -8,7 +8,7 @@ import { WorkflowValidator, } from "@gxwf/server-common/src/languageTypes"; import { SchemaNode, SchemaNodeResolver } from "../schema"; -import { IdMapper, RecordSchemaNode } from "../schema/definitions"; +import { EnumSchemaNode, IdMapper, RecordSchemaNode } from "../schema/definitions"; export class GxFormat2SchemaValidationService implements WorkflowValidator { constructor(protected readonly schemaNodeResolver: SchemaNodeResolver) {} @@ -38,6 +38,10 @@ export class GxFormat2SchemaValidationService implements WorkflowValidator { parenSchemaNode?: SchemaNode ): void { const range = this.getRange(nodeManager, node); + const schemaRecord = this.schemaNodeResolver.getSchemaNodeByTypeRef(schemaNode.typeRef); + if (schemaRecord instanceof EnumSchemaNode && node.type === "string") { + this.validateEnumValue(node, schemaRecord, range, diagnostics); + } if (schemaNode instanceof RecordSchemaNode) { switch (node.type) { case "object": @@ -50,6 +54,22 @@ export class GxFormat2SchemaValidationService implements WorkflowValidator { } } } + private validateEnumValue( + node: StringASTNode, + schemaRecord: EnumSchemaNode, + range: Range, + diagnostics: Diagnostic[] + ): void { + if (!schemaRecord.symbols.includes(node.value)) { + diagnostics.push( + Diagnostic.create( + range, + `The value is not a valid '${schemaRecord.name}'. Allowed values are: ${schemaRecord.symbols.join(", ")}.`, + DiagnosticSeverity.Error + ) + ); + } + } private validateObjectNode( nodeManager: ASTNodeManager, @@ -67,6 +87,17 @@ export class GxFormat2SchemaValidationService implements WorkflowValidator { ); } if (nodeFound) { + if (schemaFieldNode.isPrimitiveType && propertyNode?.valueNode?.type) { + if (!schemaFieldNode.matchesType(propertyNode.valueNode.type)) { + diagnostics.push( + Diagnostic.create( + range, + `Type mismatch for field '${schemaFieldNode.name}'. Expected '${schemaFieldNode.typeRef}' but found '${propertyNode.valueNode.type}'.`, + DiagnosticSeverity.Error + ) + ); + } + } const childSchemaNode = this.schemaNodeResolver.getSchemaNodeByTypeRef(schemaFieldNode.typeRef); if (childSchemaNode && propertyNode.valueNode) { if (schemaFieldNode.canBeArray) { From 2b657969facda7e5aaa7c508e8f4c9f55fb3d0e4 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 19 Jun 2024 20:14:12 +0200 Subject: [PATCH 26/43] Add some tests for format2 validation --- .../tests/integration/validation.test.ts | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 server/gx-workflow-ls-format2/tests/integration/validation.test.ts diff --git a/server/gx-workflow-ls-format2/tests/integration/validation.test.ts b/server/gx-workflow-ls-format2/tests/integration/validation.test.ts new file mode 100644 index 0000000..d7a3ac8 --- /dev/null +++ b/server/gx-workflow-ls-format2/tests/integration/validation.test.ts @@ -0,0 +1,80 @@ +import { Diagnostic } from "@gxwf/server-common/src/languageTypes"; +import { GalaxyWorkflowFormat2SchemaLoader } from "../../src/schema"; +import { GxFormat2SchemaValidationService } from "../../src/services/schemaValidationService"; +import { createFormat2WorkflowDocument } from "../testHelpers"; + +describe("Schema validation", () => { + let validator: GxFormat2SchemaValidationService; + + function validateDocument(documentContents: string): Promise { + const document = createFormat2WorkflowDocument(documentContents); + return validator.doValidation(document); + } + + beforeAll(() => { + const schemaNodeResolver = new GalaxyWorkflowFormat2SchemaLoader().nodeResolver; + validator = new GxFormat2SchemaValidationService(schemaNodeResolver); + }); + + it("should validate a correct workflow", async () => { + const content = ` +class: GalaxyWorkflow +doc: | + Simple workflow that no-op cats a file. +inputs: + the_input: + type: File + doc: input doc +outputs: + the_output: + outputSource: cat/out_file1 +steps: + cat: + tool_id: cat1 + doc: cat doc + in: + input1: the_input + `; + const diagnostics = await validateDocument(content); + expect(diagnostics).toHaveLength(0); + }); + + it("should report error when a property has incorrect type value", async () => { + const content = ` +class: null +inputs: +outputs: +steps: + `; + const diagnostics = await validateDocument(content); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].message).toBe("Type mismatch for field 'class'. Expected 'string' but found 'null'."); + }); + + it("should report error for missing required properties", async () => { + const content = ` +class: GalaxyWorkflow + `; + const diagnostics = await validateDocument(content); + expect(diagnostics).toHaveLength(3); + expect(diagnostics[0].message).toBe("The 'steps' field is required."); + expect(diagnostics[1].message).toBe("The 'inputs' field is required."); + expect(diagnostics[2].message).toBe("The 'outputs' field is required."); + }); + + it("should report error for invalid enum value", async () => { + const content = ` +class: GalaxyWorkflow +inputs: + the_input: + type: unknown +outputs: +steps: + `; + const diagnostics = await validateDocument(content); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].message).toBe( + "The value is not a valid 'GalaxyType'. Allowed values are: integer, text, File, data, collection, null, boolean, int, long, float, double, string." + ); + }); +}); From 269feede8f07068c445e954a4f882a0862accde7 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 19 Jun 2024 20:16:59 +0200 Subject: [PATCH 27/43] Add InputTypeValidationRule --- .../src/gxFormat2WorkflowDocument.ts | 12 +++- .../rules/InputTypeValidationRule.ts | 55 +++++++++++++++++++ .../server-common/src/ast/nodeManager.ts | 15 ++++- .../packages/server-common/src/ast/types.ts | 4 +- 4 files changed, 82 insertions(+), 4 deletions(-) create mode 100644 server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts diff --git a/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts b/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts index 50d1044..e0fa11a 100644 --- a/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts +++ b/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts @@ -26,16 +26,24 @@ export class GxFormat2WorkflowDocument extends WorkflowDocument { public getWorkflowInputs(): GetWorkflowInputsResult { return { - inputs: this.getAllPropertyNodesAtPath("inputs").map((input) => this.parseInputDefinition(input)), + inputs: this.getRawInputNodes().map((input) => this.parseInputDefinition(input)), }; } public getWorkflowOutputs(): GetWorkflowOutputsResult { return { - outputs: this.getAllPropertyNodesAtPath("outputs").map((output) => this.parseOutputDefinition(output)), + outputs: this.getRawOutputNodes().map((output) => this.parseOutputDefinition(output)), }; } + public getRawInputNodes(): PropertyASTNode[] { + return this.getAllPropertyNodesAtPath("inputs"); + } + + public getRawOutputNodes(): PropertyASTNode[] { + return this.getAllPropertyNodesAtPath("outputs"); + } + private getAllPropertyNodesAtPath(path: string): PropertyASTNode[] { const result: PropertyASTNode[] = []; const nodeAtPath = this.nodeManager.getNodeFromPath(path); diff --git a/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts b/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts new file mode 100644 index 0000000..069926b --- /dev/null +++ b/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts @@ -0,0 +1,55 @@ +import { ValidationRule } from "@gxwf/server-common/src/languageTypes"; +import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; +import { GxFormat2WorkflowDocument } from "../../gxFormat2WorkflowDocument"; + +/** + * Validation rule to check that all inputs that define a default value have the correct type. + */ +export class InputTypeValidationRule implements ValidationRule { + constructor(readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error) {} + + public async validate(documentContext: GxFormat2WorkflowDocument): Promise { + const result: Diagnostic[] = []; + + const inputNodes = documentContext.getRawInputNodes(); + inputNodes.forEach((input) => { + let defaultTypeMatchesValue = true; + + const inputName = String(input.keyNode.value); + const inputTypeValue = documentContext.nodeManager.getPropertyValueByName(input, "type"); + const defaultValueNode = documentContext.nodeManager.getPropertyNodeByName(input, "default"); + const defaultValue = defaultValueNode.valueNode?.value; + + const defaultValueType = typeof defaultValue; + if (inputTypeValue && defaultValue) { + switch (inputTypeValue) { + case "int": + case "integer": + case "long": + case "double": + case "float": + defaultTypeMatchesValue = defaultValueType === "number"; + break; + case "boolean": + defaultTypeMatchesValue = defaultValueType === "boolean"; + break; + case "string": + defaultTypeMatchesValue = defaultValueType === "string"; + break; + case "null": + defaultTypeMatchesValue = defaultValueType === null; + break; + } + if (!defaultTypeMatchesValue) { + result.push({ + message: `Input '${inputName}' default value has invalid type. Expected ${inputTypeValue} but found ${defaultValueType}.`, + range: documentContext.nodeManager.getNodeRange(defaultValueNode), + severity: this.severity, + }); + } + } + }); + + return Promise.resolve(result); + } +} diff --git a/server/packages/server-common/src/ast/nodeManager.ts b/server/packages/server-common/src/ast/nodeManager.ts index 224ca90..be157ea 100644 --- a/server/packages/server-common/src/ast/nodeManager.ts +++ b/server/packages/server-common/src/ast/nodeManager.ts @@ -1,5 +1,5 @@ import { Position, Range, TextDocument } from "../languageTypes"; -import { ASTNode, NodePath, ObjectASTNode, ParsedDocument, PropertyASTNode, Segment } from "./types"; +import { ASTNode, NodePath, ObjectASTNode, ParsedDocument, PropertyASTNode, Segment, ValueTypes } from "./types"; import { getPropertyNodeFromPath } from "./utils"; export class ASTNodeManager { @@ -207,4 +207,17 @@ export class ASTNodeManager { return false; } } + + public getPropertyNodeByName(node: PropertyASTNode, propertyName: string): PropertyASTNode { + const targetProperty = node.valueNode?.children?.find( + (prop) => prop.type === "property" && prop.keyNode.value === propertyName + ) as PropertyASTNode; + return targetProperty; + } + + public getPropertyValueByName(node: PropertyASTNode, propertyName: string): ValueTypes | undefined { + const targetProperty = this.getPropertyNodeByName(node, propertyName); + const targetValue = targetProperty.valueNode?.value; + return targetValue; + } } diff --git a/server/packages/server-common/src/ast/types.ts b/server/packages/server-common/src/ast/types.ts index 2c3ae8c..b869597 100644 --- a/server/packages/server-common/src/ast/types.ts +++ b/server/packages/server-common/src/ast/types.ts @@ -7,13 +7,15 @@ export type ASTNode = | BooleanASTNode | NullASTNode; +export type ValueTypes = string | boolean | number | null; + export interface BaseASTNode { readonly type: "object" | "array" | "property" | "string" | "number" | "boolean" | "null"; readonly parent?: ASTNode; readonly offset: number; readonly length: number; readonly children?: ASTNode[]; - readonly value?: string | boolean | number | null; + readonly value?: ValueTypes; readonly internalNode: unknown; getNodeFromOffsetEndInclusive(offset: number): ASTNode | undefined; } From bb57bb713f7f5a0048979a1e0012ccd1c4c1565a Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 19 Jun 2024 20:17:38 +0200 Subject: [PATCH 28/43] Add InputTypeValidationRule to IWC profile for gxformat2 --- server/gx-workflow-ls-format2/src/profiles.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/gx-workflow-ls-format2/src/profiles.ts b/server/gx-workflow-ls-format2/src/profiles.ts index b45d27c..4caf1ac 100644 --- a/server/gx-workflow-ls-format2/src/profiles.ts +++ b/server/gx-workflow-ls-format2/src/profiles.ts @@ -4,6 +4,7 @@ import { IWCCommonValidationProfile, } from "@gxwf/server-common/src/providers/validation/profiles"; import { RequiredPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; +import { InputTypeValidationRule } from "./validation/rules/InputTypeValidationRule"; /** * Defines the minimal set of validation rules for gxformat2 Galaxy workflows. @@ -34,6 +35,7 @@ export class GxFormat2IWCValidationProfile extends IWCCommonValidationProfile { DiagnosticSeverity.Error, "The workflow is not documented. Documenting workflows helps users understand the purpose of the workflow." ), + new InputTypeValidationRule(DiagnosticSeverity.Error), // Add more custom rules specific to gxformat2 workflows here... ]); From 3f6336719a71e637d7491e12966cefed8e1e6c6e Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 19 Jun 2024 20:26:30 +0200 Subject: [PATCH 29/43] Add tests for InputTypeValidationRule --- .../rules/InputTypeValidationRule.ts | 2 +- .../tests/integration/validation.test.ts | 49 ++++++++++++++++++- 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts b/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts index 069926b..ced1137 100644 --- a/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts +++ b/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts @@ -42,7 +42,7 @@ export class InputTypeValidationRule implements ValidationRule { } if (!defaultTypeMatchesValue) { result.push({ - message: `Input '${inputName}' default value has invalid type. Expected ${inputTypeValue} but found ${defaultValueType}.`, + message: `Input '${inputName}' default value has invalid type. Expected '${inputTypeValue}' but found '${defaultValueType}'.`, range: documentContext.nodeManager.getNodeRange(defaultValueNode), severity: this.severity, }); diff --git a/server/gx-workflow-ls-format2/tests/integration/validation.test.ts b/server/gx-workflow-ls-format2/tests/integration/validation.test.ts index d7a3ac8..7256941 100644 --- a/server/gx-workflow-ls-format2/tests/integration/validation.test.ts +++ b/server/gx-workflow-ls-format2/tests/integration/validation.test.ts @@ -1,6 +1,7 @@ -import { Diagnostic } from "@gxwf/server-common/src/languageTypes"; +import { Diagnostic, ValidationRule } from "@gxwf/server-common/src/languageTypes"; import { GalaxyWorkflowFormat2SchemaLoader } from "../../src/schema"; import { GxFormat2SchemaValidationService } from "../../src/services/schemaValidationService"; +import { InputTypeValidationRule } from "../../src/validation/rules/InputTypeValidationRule"; import { createFormat2WorkflowDocument } from "../testHelpers"; describe("Schema validation", () => { @@ -77,4 +78,50 @@ steps: "The value is not a valid 'GalaxyType'. Allowed values are: integer, text, File, data, collection, null, boolean, int, long, float, double, string." ); }); + + describe("Custom Rules", () => { + let rule: ValidationRule; + + function validateRule(documentContents: string): Promise { + const document = createFormat2WorkflowDocument(documentContents); + return rule.validate(document); + } + + describe("InputTypeValidationRule", () => { + beforeAll(() => { + rule = new InputTypeValidationRule(); + }); + + it("should report error when input default value has invalid type", async () => { + const content = ` +class: GalaxyWorkflow +inputs: + the_input: + type: int + default: this is not a number +outputs: +steps: + `; + const diagnostics = await validateRule(content); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].message).toBe( + "Input 'the_input' default value has invalid type. Expected 'int' but found 'string'." + ); + }); + + it("should not report error when input default value has valid type", async () => { + const content = ` +class: GalaxyWorkflow +inputs: + the_input: + type: int + default: 42 +outputs: +steps: + `; + const diagnostics = await validateRule(content); + expect(diagnostics).toHaveLength(0); + }); + }); + }); }); From de2917619da01b91135f42f3997cd6e834f0154c Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 19 Jun 2024 20:48:47 +0200 Subject: [PATCH 30/43] Refactor use getPropertyNodeByName --- .../src/gxFormat2WorkflowDocument.ts | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts b/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts index e0fa11a..fc0fcde 100644 --- a/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts +++ b/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts @@ -62,24 +62,21 @@ export class GxFormat2WorkflowDocument extends WorkflowDocument { private parseInputDefinition(input: PropertyASTNode): WorkflowInput { const inputName = String(input.keyNode.value); const inputType = this.parseInputType(input); - const inputDocNode = input.valueNode?.children?.find( - (prop) => prop.type === "property" && prop.keyNode.value === "doc" - ) as PropertyASTNode; + const inputDocNode = this.nodeManager.getPropertyNodeByName(input, "doc"); const inputDescription = String(inputDocNode?.valueNode?.value ?? ""); + const defaultValueNode = this.nodeManager.getPropertyNodeByName(input, "default"); const inputDefinition: WorkflowInput = { name: inputName, doc: inputDescription, type: inputType, - //TODO: default + default: defaultValueNode?.valueNode?.value, }; return inputDefinition; } private parseInputType(input: PropertyASTNode): WorkflowDataType { let inputType: WorkflowDataType = "data"; - const inputTypeNode = input.valueNode?.children?.find( - (prop) => prop.type === "property" && prop.keyNode.value === "type" - ) as PropertyASTNode; + const inputTypeNode = this.nodeManager.getPropertyNodeByName(input, "type"); if (inputTypeNode) { inputType = String(inputTypeNode.valueNode?.value) as WorkflowDataType; } else { @@ -91,9 +88,7 @@ export class GxFormat2WorkflowDocument extends WorkflowDocument { private parseOutputDefinition(output: PropertyASTNode): WorkflowOutput { const outputName = String(output.keyNode.value); - const outputDocNode = output.valueNode?.children?.find( - (prop) => prop.type === "property" && prop.keyNode.value === "doc" - ) as PropertyASTNode; + const outputDocNode = this.nodeManager.getPropertyNodeByName(output, "doc"); const outputDoc = String(outputDocNode?.valueNode?.value ?? ""); const outputDefinition = { name: outputName, From cccddddfaac71c7d6d5355497adc0c855ad4d3cb Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 19 Jun 2024 20:50:11 +0200 Subject: [PATCH 31/43] Update WorkflowInput definition to include default value + tests --- .../tests/integration/document.test.ts | 1 + .../src/nativeWorkflowDocument.ts | 13 +++++++++---- .../tests/unit/nativeWorkflowDocument.test.ts | 14 +++++++------- shared/src/requestsDefinitions.ts | 1 + 4 files changed, 18 insertions(+), 11 deletions(-) diff --git a/server/gx-workflow-ls-format2/tests/integration/document.test.ts b/server/gx-workflow-ls-format2/tests/integration/document.test.ts index d449b3c..10cfe54 100644 --- a/server/gx-workflow-ls-format2/tests/integration/document.test.ts +++ b/server/gx-workflow-ls-format2/tests/integration/document.test.ts @@ -49,6 +49,7 @@ inputs: name: "text_param", doc: "", type: "text", + default: "text value", }, ]); }); diff --git a/server/gx-workflow-ls-native/src/nativeWorkflowDocument.ts b/server/gx-workflow-ls-native/src/nativeWorkflowDocument.ts index 287b059..fc0e4dc 100644 --- a/server/gx-workflow-ls-native/src/nativeWorkflowDocument.ts +++ b/server/gx-workflow-ls-native/src/nativeWorkflowDocument.ts @@ -5,6 +5,8 @@ import { TextDocument, WorkflowDataType, WorkflowDocument, + WorkflowInput, + WorkflowOutput, } from "@gxwf/server-common/src/languageTypes"; import { JSONDocument } from "vscode-json-languageservice"; import { ToolState, isWorkflowInputType, type ParameterInputToolState } from "./utils"; @@ -63,11 +65,13 @@ export class NativeWorkflowDocument extends WorkflowDocument { const toolStateValue = JSON.parse( toolStateNode?.valueNode?.value ? String(toolStateNode?.valueNode?.value) : "{}" ); - result.inputs.push({ + const inputDefinition: WorkflowInput = { name: String(labelValue), doc: String(annotationValue ?? ""), type: this.getInputType(stepTypeValue, toolStateValue), - }); + default: toolStateValue.default, + }; + result.inputs.push(inputDefinition); } }); return result; @@ -95,10 +99,11 @@ export class NativeWorkflowDocument extends WorkflowDocument { } const uuidNode = workflowOutput.properties.find((property) => property.keyNode.value === "uuid"); const uuidValue = String(uuidNode?.valueNode?.value); - result.outputs.push({ + const outputDefinition: WorkflowOutput = { name: String(labelValue), uuid: uuidValue, - }); + }; + result.outputs.push(outputDefinition); }); } }); diff --git a/server/gx-workflow-ls-native/tests/unit/nativeWorkflowDocument.test.ts b/server/gx-workflow-ls-native/tests/unit/nativeWorkflowDocument.test.ts index 96c30bf..7b0c29d 100644 --- a/server/gx-workflow-ls-native/tests/unit/nativeWorkflowDocument.test.ts +++ b/server/gx-workflow-ls-native/tests/unit/nativeWorkflowDocument.test.ts @@ -50,13 +50,13 @@ describe("NativeWorkflowDocument", () => { [ TestWorkflowProvider.workflows.validation.withOnlyInputs, [ - { doc: "", name: "Dataset Input", type: "data" }, - { doc: "", name: "Collection Input", type: "collection" }, - { doc: "", name: "Text Param", type: "text" }, - { doc: "", name: "Integer Param", type: "integer" }, - { doc: "", name: "Float Param", type: "float" }, - { doc: "", name: "Boolean Param", type: "boolean" }, - { doc: "", name: "Color Param", type: "color" }, + { default: undefined, doc: "", name: "Dataset Input", type: "data" }, + { default: undefined, doc: "", name: "Collection Input", type: "collection" }, + { default: undefined, doc: "", name: "Text Param", type: "text" }, + { default: 10, doc: "", name: "Integer Param", type: "integer" }, + { default: undefined, doc: "", name: "Float Param", type: "float" }, + { default: undefined, doc: "", name: "Boolean Param", type: "boolean" }, + { default: undefined, doc: "", name: "Color Param", type: "color" }, ], ], ])("returns the expected inputs", (wfContent: string, expectedInputs: WorkflowInput[]) => { diff --git a/shared/src/requestsDefinitions.ts b/shared/src/requestsDefinitions.ts index 41ff0be..48280fd 100644 --- a/shared/src/requestsDefinitions.ts +++ b/shared/src/requestsDefinitions.ts @@ -47,6 +47,7 @@ export interface WorkflowInput { name: string; type: WorkflowDataType; doc: string; + default?: unknown; } export interface GetWorkflowInputsResult { From e59845497e662d399504105c1e08211453267fda Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 19 Jun 2024 21:04:47 +0200 Subject: [PATCH 32/43] Update WorkflowInput definition to include optional flag + tests --- .../src/gxFormat2WorkflowDocument.ts | 2 ++ .../validation/rules/InputTypeValidationRule.ts | 2 +- .../tests/integration/document.test.ts | 3 +++ .../src/nativeWorkflowDocument.ts | 1 + .../tests/unit/nativeWorkflowDocument.test.ts | 14 +++++++------- .../packages/server-common/src/ast/nodeManager.ts | 4 ++-- shared/src/requestsDefinitions.ts | 1 + 7 files changed, 17 insertions(+), 10 deletions(-) diff --git a/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts b/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts index fc0fcde..245a0f4 100644 --- a/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts +++ b/server/gx-workflow-ls-format2/src/gxFormat2WorkflowDocument.ts @@ -65,11 +65,13 @@ export class GxFormat2WorkflowDocument extends WorkflowDocument { const inputDocNode = this.nodeManager.getPropertyNodeByName(input, "doc"); const inputDescription = String(inputDocNode?.valueNode?.value ?? ""); const defaultValueNode = this.nodeManager.getPropertyNodeByName(input, "default"); + const optionalValue = this.nodeManager.getPropertyValueByName(input, "optional"); const inputDefinition: WorkflowInput = { name: inputName, doc: inputDescription, type: inputType, default: defaultValueNode?.valueNode?.value, + optional: optionalValue === undefined ? undefined : optionalValue === true, }; return inputDefinition; } diff --git a/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts b/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts index ced1137..6798f63 100644 --- a/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts +++ b/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts @@ -18,7 +18,7 @@ export class InputTypeValidationRule implements ValidationRule { const inputName = String(input.keyNode.value); const inputTypeValue = documentContext.nodeManager.getPropertyValueByName(input, "type"); const defaultValueNode = documentContext.nodeManager.getPropertyNodeByName(input, "default"); - const defaultValue = defaultValueNode.valueNode?.value; + const defaultValue = defaultValueNode?.valueNode?.value; const defaultValueType = typeof defaultValue; if (inputTypeValue && defaultValue) { diff --git a/server/gx-workflow-ls-format2/tests/integration/document.test.ts b/server/gx-workflow-ls-format2/tests/integration/document.test.ts index 10cfe54..1e4440a 100644 --- a/server/gx-workflow-ls-format2/tests/integration/document.test.ts +++ b/server/gx-workflow-ls-format2/tests/integration/document.test.ts @@ -9,6 +9,7 @@ inputs: input_2: type: File doc: This is the input 2 + optional: false the_collection: type: collection doc: This is a collection @@ -34,6 +35,7 @@ inputs: name: "input_2", doc: "This is the input 2", type: "File", + optional: false, }, { name: "the_collection", @@ -50,6 +52,7 @@ inputs: doc: "", type: "text", default: "text value", + optional: true, }, ]); }); diff --git a/server/gx-workflow-ls-native/src/nativeWorkflowDocument.ts b/server/gx-workflow-ls-native/src/nativeWorkflowDocument.ts index fc0e4dc..83d5f75 100644 --- a/server/gx-workflow-ls-native/src/nativeWorkflowDocument.ts +++ b/server/gx-workflow-ls-native/src/nativeWorkflowDocument.ts @@ -70,6 +70,7 @@ export class NativeWorkflowDocument extends WorkflowDocument { doc: String(annotationValue ?? ""), type: this.getInputType(stepTypeValue, toolStateValue), default: toolStateValue.default, + optional: toolStateValue.optional, }; result.inputs.push(inputDefinition); } diff --git a/server/gx-workflow-ls-native/tests/unit/nativeWorkflowDocument.test.ts b/server/gx-workflow-ls-native/tests/unit/nativeWorkflowDocument.test.ts index 7b0c29d..0ab3134 100644 --- a/server/gx-workflow-ls-native/tests/unit/nativeWorkflowDocument.test.ts +++ b/server/gx-workflow-ls-native/tests/unit/nativeWorkflowDocument.test.ts @@ -50,13 +50,13 @@ describe("NativeWorkflowDocument", () => { [ TestWorkflowProvider.workflows.validation.withOnlyInputs, [ - { default: undefined, doc: "", name: "Dataset Input", type: "data" }, - { default: undefined, doc: "", name: "Collection Input", type: "collection" }, - { default: undefined, doc: "", name: "Text Param", type: "text" }, - { default: 10, doc: "", name: "Integer Param", type: "integer" }, - { default: undefined, doc: "", name: "Float Param", type: "float" }, - { default: undefined, doc: "", name: "Boolean Param", type: "boolean" }, - { default: undefined, doc: "", name: "Color Param", type: "color" }, + { default: undefined, doc: "", name: "Dataset Input", optional: false, type: "data" }, + { default: undefined, doc: "", name: "Collection Input", optional: false, type: "collection" }, + { default: undefined, doc: "", name: "Text Param", optional: false, type: "text" }, + { default: 10, doc: "", name: "Integer Param", optional: true, type: "integer" }, + { default: undefined, doc: "", name: "Float Param", optional: false, type: "float" }, + { default: undefined, doc: "", name: "Boolean Param", optional: false, type: "boolean" }, + { default: undefined, doc: "", name: "Color Param", optional: false, type: "color" }, ], ], ])("returns the expected inputs", (wfContent: string, expectedInputs: WorkflowInput[]) => { diff --git a/server/packages/server-common/src/ast/nodeManager.ts b/server/packages/server-common/src/ast/nodeManager.ts index be157ea..90561a6 100644 --- a/server/packages/server-common/src/ast/nodeManager.ts +++ b/server/packages/server-common/src/ast/nodeManager.ts @@ -208,7 +208,7 @@ export class ASTNodeManager { } } - public getPropertyNodeByName(node: PropertyASTNode, propertyName: string): PropertyASTNode { + public getPropertyNodeByName(node: PropertyASTNode, propertyName: string): PropertyASTNode | undefined { const targetProperty = node.valueNode?.children?.find( (prop) => prop.type === "property" && prop.keyNode.value === propertyName ) as PropertyASTNode; @@ -217,7 +217,7 @@ export class ASTNodeManager { public getPropertyValueByName(node: PropertyASTNode, propertyName: string): ValueTypes | undefined { const targetProperty = this.getPropertyNodeByName(node, propertyName); - const targetValue = targetProperty.valueNode?.value; + const targetValue = targetProperty?.valueNode?.value; return targetValue; } } diff --git a/shared/src/requestsDefinitions.ts b/shared/src/requestsDefinitions.ts index 48280fd..90bd7a3 100644 --- a/shared/src/requestsDefinitions.ts +++ b/shared/src/requestsDefinitions.ts @@ -48,6 +48,7 @@ export interface WorkflowInput { type: WorkflowDataType; doc: string; default?: unknown; + optional?: boolean; } export interface GetWorkflowInputsResult { From a2e9b59e182255bcefd8e2bcf1973a71f57eebfa Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 19 Jun 2024 22:32:51 +0200 Subject: [PATCH 33/43] Refactor extract utility function for type checking --- .../src/schema/index.ts | 4 +-- .../rules/InputTypeValidationRule.ts | 29 ++++--------------- server/packages/server-common/src/utils.ts | 28 ++++++++++++++++++ 3 files changed, 36 insertions(+), 25 deletions(-) create mode 100644 server/packages/server-common/src/utils.ts diff --git a/server/gx-workflow-ls-format2/src/schema/index.ts b/server/gx-workflow-ls-format2/src/schema/index.ts index a4239af..d1ebff3 100644 --- a/server/gx-workflow-ls-format2/src/schema/index.ts +++ b/server/gx-workflow-ls-format2/src/schema/index.ts @@ -1,5 +1,5 @@ +import { FieldSchemaNode, RecordSchemaNode, SchemaNode } from "./definitions"; import { GalaxyWorkflowFormat2SchemaLoader } from "./schemaLoader"; -import { RecordSchemaNode, FieldSchemaNode, SchemaNode } from "./definitions"; import { SchemaNodeResolver } from "./schemaNodeResolver"; -export { GalaxyWorkflowFormat2SchemaLoader, SchemaNodeResolver, SchemaNode, RecordSchemaNode, FieldSchemaNode }; +export { FieldSchemaNode, GalaxyWorkflowFormat2SchemaLoader, RecordSchemaNode, SchemaNode, SchemaNodeResolver }; diff --git a/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts b/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts index 6798f63..1a759bb 100644 --- a/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts +++ b/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts @@ -1,4 +1,5 @@ import { ValidationRule } from "@gxwf/server-common/src/languageTypes"; +import { isCompatibleType } from "@gxwf/server-common/src/utils"; import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; import { GxFormat2WorkflowDocument } from "../../gxFormat2WorkflowDocument"; @@ -13,36 +14,18 @@ export class InputTypeValidationRule implements ValidationRule { const inputNodes = documentContext.getRawInputNodes(); inputNodes.forEach((input) => { - let defaultTypeMatchesValue = true; - const inputName = String(input.keyNode.value); - const inputTypeValue = documentContext.nodeManager.getPropertyValueByName(input, "type"); + const inputType = documentContext.nodeManager.getPropertyValueByName(input, "type") as string; const defaultValueNode = documentContext.nodeManager.getPropertyNodeByName(input, "default"); const defaultValue = defaultValueNode?.valueNode?.value; const defaultValueType = typeof defaultValue; - if (inputTypeValue && defaultValue) { - switch (inputTypeValue) { - case "int": - case "integer": - case "long": - case "double": - case "float": - defaultTypeMatchesValue = defaultValueType === "number"; - break; - case "boolean": - defaultTypeMatchesValue = defaultValueType === "boolean"; - break; - case "string": - defaultTypeMatchesValue = defaultValueType === "string"; - break; - case "null": - defaultTypeMatchesValue = defaultValueType === null; - break; - } + + if (inputType && defaultValue) { + const defaultTypeMatchesValue = isCompatibleType(inputType, defaultValueType); if (!defaultTypeMatchesValue) { result.push({ - message: `Input '${inputName}' default value has invalid type. Expected '${inputTypeValue}' but found '${defaultValueType}'.`, + message: `Input '${inputName}' default value has invalid type. Expected '${inputType}' but found '${defaultValueType}'.`, range: documentContext.nodeManager.getNodeRange(defaultValueNode), severity: this.severity, }); diff --git a/server/packages/server-common/src/utils.ts b/server/packages/server-common/src/utils.ts new file mode 100644 index 0000000..7ae4422 --- /dev/null +++ b/server/packages/server-common/src/utils.ts @@ -0,0 +1,28 @@ +/** + * Check if the actual type can be mapped to the expected type. + * Usefull to validate properties types. + * @param expectedType The expected type. Usually a type supported by the schema. + * @param actualType The actual type. You can use the `typeof` operator to get this value. + */ +export function isCompatibleType(expectedType: string, actualType: string): boolean { + let isCompatible = true; + switch (expectedType) { + case "int": + case "integer": + case "long": + case "double": + case "float": + isCompatible = actualType === "number"; + break; + case "boolean": + isCompatible = actualType === "boolean"; + break; + case "string": + isCompatible = actualType === "string"; + break; + case "null": + isCompatible = actualType === null; + break; + } + return isCompatible; +} From 023917baae4067fa5699cacf9c8fdd3dcfaf32c6 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Wed, 19 Jun 2024 23:53:20 +0200 Subject: [PATCH 34/43] Update test file wf to include default value and optional flag --- test-data/json/validation/test_wf_05.ga | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-data/json/validation/test_wf_05.ga b/test-data/json/validation/test_wf_05.ga index 3bf04dd..8374f75 100644 --- a/test-data/json/validation/test_wf_05.ga +++ b/test-data/json/validation/test_wf_05.ga @@ -106,7 +106,7 @@ "top": 374 }, "tool_id": null, - "tool_state": "{\"parameter_type\": \"integer\", \"optional\": false}", + "tool_state": "{\"default\": 10, \"parameter_type\": \"integer\", \"optional\": true}", "tool_version": null, "type": "parameter_input", "uuid": "847d6938-c7aa-41fa-a139-611f4aaa900d", From 02998155a509501e7f905cae3305113877b59973 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 20 Jun 2024 12:07:42 +0200 Subject: [PATCH 35/43] Update isCompatibleType to use WorkflowDataType for expectedType parameter --- server/packages/server-common/src/utils.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/server/packages/server-common/src/utils.ts b/server/packages/server-common/src/utils.ts index 7ae4422..c82ca09 100644 --- a/server/packages/server-common/src/utils.ts +++ b/server/packages/server-common/src/utils.ts @@ -1,10 +1,12 @@ +import { WorkflowDataType } from "./languageTypes"; + /** * Check if the actual type can be mapped to the expected type. * Usefull to validate properties types. * @param expectedType The expected type. Usually a type supported by the schema. * @param actualType The actual type. You can use the `typeof` operator to get this value. */ -export function isCompatibleType(expectedType: string, actualType: string): boolean { +export function isCompatibleType(expectedType: WorkflowDataType, actualType: string): boolean { let isCompatible = true; switch (expectedType) { case "int": @@ -17,9 +19,13 @@ export function isCompatibleType(expectedType: string, actualType: string): bool case "boolean": isCompatible = actualType === "boolean"; break; + case "text": case "string": isCompatible = actualType === "string"; break; + case "File": + isCompatible = actualType === "string" || actualType === "object"; + break; case "null": isCompatible = actualType === null; break; From 1674bf46b7cdc8646695430a14452e3845041ad4 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 20 Jun 2024 12:08:24 +0200 Subject: [PATCH 36/43] Add check for invalid type in WorkflowInputsValidationRule --- .../packages/server-common/tests/testHelpers.ts | 13 +++++++++++++ .../rules/WorkflowInputsValidationRule.ts | 17 ++++++++++++++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/server/packages/server-common/tests/testHelpers.ts b/server/packages/server-common/tests/testHelpers.ts index 019143e..b3270c5 100644 --- a/server/packages/server-common/tests/testHelpers.ts +++ b/server/packages/server-common/tests/testHelpers.ts @@ -71,6 +71,19 @@ export const EXPECTED_WORKFLOW_INPUTS: WorkflowInput[] = [ doc: "This is a collection", type: "collection", }, + { + name: "My fake string", + doc: "This is an optional string with a default value", + type: "string", + default: "default string", + optional: true, + }, + { + name: "My fake number", + doc: "This is a required number parameter", + type: "int", + optional: false, + }, ]; export const EXPECTED_WORKFLOW_OUTPUTS: WorkflowOutput[] = [ diff --git a/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowInputsValidationRule.ts b/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowInputsValidationRule.ts index 325e544..3cb7dbd 100644 --- a/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowInputsValidationRule.ts +++ b/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowInputsValidationRule.ts @@ -1,4 +1,5 @@ import { ValidationRule, WorkflowTestsDocument } from "@gxwf/server-common/src/languageTypes"; +import { isCompatibleType } from "@gxwf/server-common/src/utils"; import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; /** @@ -12,9 +13,9 @@ export class WorkflowInputsValidationRule implements ValidationRule { const workflowInputs = await documentContext.getWorkflowInputs(); const documentInputNodes = documentContext.nodeManager.getAllPropertyNodesByName("job")[0]?.valueNode?.children ?? []; - documentInputNodes.forEach((inputNode) => { + for (const inputNode of documentInputNodes) { if (inputNode.type !== "property") { - return; + continue; } const inputName = inputNode.keyNode.value as string; const input = workflowInputs.find((i) => i.name === inputName); @@ -22,8 +23,18 @@ export class WorkflowInputsValidationRule implements ValidationRule { const range = documentContext.nodeManager.getNodeRange(inputNode); const message = `Input "${inputName}" is not defined in the associated workflow.`; diagnostics.push(Diagnostic.create(range, message, DiagnosticSeverity.Error)); + } else { + if (inputNode.valueNode) { + const inputType = input.type; + const inputTypeValue = inputNode.valueNode.type; + if (!isCompatibleType(inputType, inputTypeValue)) { + const range = documentContext.nodeManager.getNodeRange(inputNode); + const message = `Input "${inputName}" has an invalid type. Expected "${inputType}" but found "${inputTypeValue}".`; + diagnostics.push(Diagnostic.create(range, message, DiagnosticSeverity.Error)); + } + } } - }); + } return Promise.resolve(diagnostics); } } From d442ffc808c1e7f80c87507513251c71872275d6 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 20 Jun 2024 12:09:32 +0200 Subject: [PATCH 37/43] Increase test coverage for input validation --- .../tests/unit/validation.test.ts | 49 ++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts b/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts index ba93fb7..35d8b27 100644 --- a/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts +++ b/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts @@ -61,7 +61,7 @@ describe("Workflow Tests Validation Rules", () => { rule = new WorkflowInputsValidationRule(); }); - it("should pass validation when a valid input is defined in the workflow", async () => { + it("should pass validation when a valid input is defined in the workflow (inline)", async () => { const testDocumentContents = ` - doc: The docs job: @@ -74,6 +74,20 @@ describe("Workflow Tests Validation Rules", () => { expect(diagnostics.length).toBe(0); }); + it("should pass validation when a valid input is defined in the workflow", async () => { + const testDocumentContents = ` + - doc: The docs + job: + My fake dataset: + class: File + `; + + const diagnostics = await validate(testDocumentContents); + + expect(diagnostics).not.toBeNull(); + expect(diagnostics.length).toBe(0); + }); + it("should error when an input is not defined in the workflow", async () => { const testDocumentContents = ` - doc: The docs @@ -87,6 +101,39 @@ describe("Workflow Tests Validation Rules", () => { expect(diagnostics[0].message).toBe('Input "Missing input" is not defined in the associated workflow.'); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); + + it("should error when an input has an invalid type (inline)", async () => { + const testDocumentContents = ` + - doc: The docs + job: + My fake string: 5 + `; + + const diagnostics = await validate(testDocumentContents); + + expect(diagnostics.length).toBe(1); + expect(diagnostics[0].message).toBe( + 'Input "My fake string" has an invalid type. Expected "string" but found "number".' + ); + expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); + }); + + it("should error when an input has an invalid type", async () => { + const testDocumentContents = ` + - doc: The docs + job: + My fake number: + class: string + `; + + const diagnostics = await validate(testDocumentContents); + + expect(diagnostics.length).toBe(1); + expect(diagnostics[0].message).toBe( + 'Input "My fake number" has an invalid type. Expected "int" but found "object".' + ); + expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); + }); }); describe("WorkflowOutputsValidationRule", () => { From 5ae87d2cface055985691ee122daa09b44b66b5c Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 20 Jun 2024 12:42:21 +0200 Subject: [PATCH 38/43] Fix type validation for workflow inputs --- .../rules/InputTypeValidationRule.ts | 15 +++++-- server/packages/server-common/src/utils.ts | 8 ++++ shared/src/requestsDefinitions.ts | 41 ++++++++++++------- 3 files changed, 47 insertions(+), 17 deletions(-) diff --git a/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts b/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts index 1a759bb..56c6e44 100644 --- a/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts +++ b/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts @@ -1,5 +1,5 @@ -import { ValidationRule } from "@gxwf/server-common/src/languageTypes"; -import { isCompatibleType } from "@gxwf/server-common/src/utils"; +import { ValidationRule, WorkflowDataType } from "@gxwf/server-common/src/languageTypes"; +import { isCompatibleType, isWorkflowDataType } from "@gxwf/server-common/src/utils"; import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; import { GxFormat2WorkflowDocument } from "../../gxFormat2WorkflowDocument"; @@ -16,13 +16,22 @@ export class InputTypeValidationRule implements ValidationRule { inputNodes.forEach((input) => { const inputName = String(input.keyNode.value); const inputType = documentContext.nodeManager.getPropertyValueByName(input, "type") as string; + + if (!isWorkflowDataType(inputType)) { + result.push({ + message: `Input '${inputName}' has an invalid type. Expected a valid workflow data type but found '${inputType}'.`, + range: documentContext.nodeManager.getNodeRange(input), + severity: this.severity, + }); + } + const defaultValueNode = documentContext.nodeManager.getPropertyNodeByName(input, "default"); const defaultValue = defaultValueNode?.valueNode?.value; const defaultValueType = typeof defaultValue; if (inputType && defaultValue) { - const defaultTypeMatchesValue = isCompatibleType(inputType, defaultValueType); + const defaultTypeMatchesValue = isCompatibleType(inputType as WorkflowDataType, defaultValueType); if (!defaultTypeMatchesValue) { result.push({ message: `Input '${inputName}' default value has invalid type. Expected '${inputType}' but found '${defaultValueType}'.`, diff --git a/server/packages/server-common/src/utils.ts b/server/packages/server-common/src/utils.ts index c82ca09..8019952 100644 --- a/server/packages/server-common/src/utils.ts +++ b/server/packages/server-common/src/utils.ts @@ -1,3 +1,4 @@ +import { workflowDataTypes } from "../../../../shared/src/requestsDefinitions"; import { WorkflowDataType } from "./languageTypes"; /** @@ -32,3 +33,10 @@ export function isCompatibleType(expectedType: WorkflowDataType, actualType: str } return isCompatible; } + +export function isWorkflowDataType(type?: string): type is WorkflowDataType { + if (!type) { + return false; + } + return type in workflowDataTypes; +} diff --git a/shared/src/requestsDefinitions.ts b/shared/src/requestsDefinitions.ts index 90bd7a3..633b830 100644 --- a/shared/src/requestsDefinitions.ts +++ b/shared/src/requestsDefinitions.ts @@ -28,20 +28,33 @@ export interface TargetWorkflowDocumentParams { uri: string; } -export type WorkflowDataType = - | "color" //TODO: this type seems to be missing in format2 schema - | "null" - | "boolean" - | "int" - | "long" - | "float" - | "double" - | "string" - | "integer" - | "text" - | "File" - | "data" - | "collection"; +/** + * This contains all the supported data types for workflow inputs. + * + * **Important**: This definition must be kept in sync with the schema definition. + * + * Note: Is defined as a const object to be used as a map and to be able to use the keys as a union type. + * This way we can maintain a single source of truth for the supported data types and generate WorkflowDataType + * type dynamically from it. + */ +export const workflowDataTypes = { + boolean: true, + collection: true, + color: true, //TODO: this type seems to be missing in format2 schema + data: true, + double: true, + File: true, + float: true, + int: true, + integer: true, + long: true, + null: true, + string: true, + text: true, +} as const; + +// Extract the keys of the object to form the union type +export type WorkflowDataType = keyof typeof workflowDataTypes; export interface WorkflowInput { name: string; From d117fb8766a2bc5f4323d1fb69483ae16a20ea05 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 20 Jun 2024 12:52:08 +0200 Subject: [PATCH 39/43] Refactor InputTypeValidationRule to skip type validation for invalid types --- .../validation/rules/InputTypeValidationRule.ts | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts b/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts index 56c6e44..fa10849 100644 --- a/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts +++ b/server/gx-workflow-ls-format2/src/validation/rules/InputTypeValidationRule.ts @@ -1,4 +1,4 @@ -import { ValidationRule, WorkflowDataType } from "@gxwf/server-common/src/languageTypes"; +import { ValidationRule } from "@gxwf/server-common/src/languageTypes"; import { isCompatibleType, isWorkflowDataType } from "@gxwf/server-common/src/utils"; import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; import { GxFormat2WorkflowDocument } from "../../gxFormat2WorkflowDocument"; @@ -13,25 +13,21 @@ export class InputTypeValidationRule implements ValidationRule { const result: Diagnostic[] = []; const inputNodes = documentContext.getRawInputNodes(); - inputNodes.forEach((input) => { + for (const input of inputNodes) { const inputName = String(input.keyNode.value); const inputType = documentContext.nodeManager.getPropertyValueByName(input, "type") as string; if (!isWorkflowDataType(inputType)) { - result.push({ - message: `Input '${inputName}' has an invalid type. Expected a valid workflow data type but found '${inputType}'.`, - range: documentContext.nodeManager.getNodeRange(input), - severity: this.severity, - }); + // Skip type validation for invalid types. Those will be caught by the schema validation. + continue; } const defaultValueNode = documentContext.nodeManager.getPropertyNodeByName(input, "default"); const defaultValue = defaultValueNode?.valueNode?.value; - const defaultValueType = typeof defaultValue; if (inputType && defaultValue) { - const defaultTypeMatchesValue = isCompatibleType(inputType as WorkflowDataType, defaultValueType); + const defaultTypeMatchesValue = isCompatibleType(inputType, defaultValueType); if (!defaultTypeMatchesValue) { result.push({ message: `Input '${inputName}' default value has invalid type. Expected '${inputType}' but found '${defaultValueType}'.`, @@ -40,8 +36,7 @@ export class InputTypeValidationRule implements ValidationRule { }); } } - }); - + } return Promise.resolve(result); } } From 6ab97c0c508231f339c9e01498ae8708a33078e3 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 20 Jun 2024 15:24:14 +0200 Subject: [PATCH 40/43] Fix getAllPropertyNodesByName Don't require the property value to be of type object --- server/packages/server-common/src/ast/nodeManager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/packages/server-common/src/ast/nodeManager.ts b/server/packages/server-common/src/ast/nodeManager.ts index 90561a6..db2d485 100644 --- a/server/packages/server-common/src/ast/nodeManager.ts +++ b/server/packages/server-common/src/ast/nodeManager.ts @@ -109,7 +109,7 @@ export class ASTNodeManager { if (!root) return result; this.visit((node) => { - if (node.type === "property" && node.keyNode.value === name && node.valueNode?.type === "object") { + if (node.type === "property" && node.keyNode.value === name) { result.push(node); } return true; From 815b7d9dde06cbf1b8ac6c5c35a939ae4162f40a Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 20 Jun 2024 15:25:21 +0200 Subject: [PATCH 41/43] Validate required inputs in tests documents + tests --- .../server-common/tests/testHelpers.ts | 10 +++++ .../rules/WorkflowInputsValidationRule.ts | 36 ++++++++++++++-- .../tests/unit/validation.test.ts | 42 +++++++++++++++++-- 3 files changed, 81 insertions(+), 7 deletions(-) diff --git a/server/packages/server-common/tests/testHelpers.ts b/server/packages/server-common/tests/testHelpers.ts index b3270c5..3ac5e32 100644 --- a/server/packages/server-common/tests/testHelpers.ts +++ b/server/packages/server-common/tests/testHelpers.ts @@ -57,6 +57,7 @@ export const FAKE_DATASET_INPUT: WorkflowInput = { name: "My fake dataset", doc: "This is a simple dataset", type: "data", + optional: true, }; export const EXPECTED_WORKFLOW_INPUTS: WorkflowInput[] = [ @@ -65,11 +66,13 @@ export const EXPECTED_WORKFLOW_INPUTS: WorkflowInput[] = [ name: "Input dataset: fake", doc: "This is a simple dataset with a colon in the name", type: "File", + optional: true, }, { name: "My fake collection", doc: "This is a collection", type: "collection", + optional: true, }, { name: "My fake string", @@ -84,6 +87,13 @@ export const EXPECTED_WORKFLOW_INPUTS: WorkflowInput[] = [ type: "int", optional: false, }, + { + name: "My fake boolean", + doc: "This is a required boolean parameter with a default value", + type: "boolean", + default: true, + optional: false, + }, ]; export const EXPECTED_WORKFLOW_OUTPUTS: WorkflowOutput[] = [ diff --git a/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowInputsValidationRule.ts b/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowInputsValidationRule.ts index 3cb7dbd..bd418c3 100644 --- a/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowInputsValidationRule.ts +++ b/server/packages/workflow-tests-language-service/src/validation/rules/WorkflowInputsValidationRule.ts @@ -1,4 +1,5 @@ -import { ValidationRule, WorkflowTestsDocument } from "@gxwf/server-common/src/languageTypes"; +import { PropertyASTNode } from "@gxwf/server-common/src/ast/types"; +import { ValidationRule, WorkflowInput, WorkflowTestsDocument } from "@gxwf/server-common/src/languageTypes"; import { isCompatibleType } from "@gxwf/server-common/src/utils"; import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; @@ -11,13 +12,32 @@ export class WorkflowInputsValidationRule implements ValidationRule { public async validate(documentContext: WorkflowTestsDocument): Promise { const diagnostics: Diagnostic[] = []; const workflowInputs = await documentContext.getWorkflowInputs(); - const documentInputNodes = - documentContext.nodeManager.getAllPropertyNodesByName("job")[0]?.valueNode?.children ?? []; + const jobNodes = documentContext.nodeManager.getAllPropertyNodesByName("job"); + for (const jobNode of jobNodes) { + const jobDiagnostics = await this.validateInputsInJobNode(jobNode, workflowInputs, documentContext); + diagnostics.push(...jobDiagnostics); + } + return Promise.resolve(diagnostics); + } + + private async validateInputsInJobNode( + jobNode: PropertyASTNode, + workflowInputs: WorkflowInput[], + documentContext: WorkflowTestsDocument + ): Promise { + const diagnostics: Diagnostic[] = []; + if (!jobNode) { + // Skip validation if the job node is not present. This will be caught by schema validation. + return Promise.resolve(diagnostics); + } + const documentInputNodes = jobNode?.valueNode?.children ?? []; + const definedInputsInTestDocument = new Set(); for (const inputNode of documentInputNodes) { if (inputNode.type !== "property") { continue; } const inputName = inputNode.keyNode.value as string; + definedInputsInTestDocument.add(inputName); const input = workflowInputs.find((i) => i.name === inputName); if (!input) { const range = documentContext.nodeManager.getNodeRange(inputNode); @@ -35,6 +55,16 @@ export class WorkflowInputsValidationRule implements ValidationRule { } } } + const requiredWorkflowInputs = workflowInputs + .filter((i) => !i.optional && i.default === undefined) + .map((i) => i.name); + const range = documentContext.nodeManager.getNodeRange(jobNode); + for (const requiredInput of requiredWorkflowInputs) { + if (!definedInputsInTestDocument.has(requiredInput)) { + const message = `Input "${requiredInput}" is required but no value or default was provided.`; + diagnostics.push(Diagnostic.create(range, message, DiagnosticSeverity.Error)); + } + } return Promise.resolve(diagnostics); } } diff --git a/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts b/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts index 35d8b27..13e2172 100644 --- a/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts +++ b/server/packages/workflow-tests-language-service/tests/unit/validation.test.ts @@ -66,6 +66,7 @@ describe("Workflow Tests Validation Rules", () => { - doc: The docs job: My fake dataset: data/input.txt + My fake number: 5 `; const diagnostics = await validate(testDocumentContents); @@ -80,6 +81,7 @@ describe("Workflow Tests Validation Rules", () => { job: My fake dataset: class: File + My fake number: 5 `; const diagnostics = await validate(testDocumentContents); @@ -92,13 +94,14 @@ describe("Workflow Tests Validation Rules", () => { const testDocumentContents = ` - doc: The docs job: - Missing input: data/input.txt + My fake number: 5 + Unknown input: data/input.txt `; const diagnostics = await validate(testDocumentContents); expect(diagnostics.length).toBe(1); - expect(diagnostics[0].message).toBe('Input "Missing input" is not defined in the associated workflow.'); + expect(diagnostics[0].message).toBe('Input "Unknown input" is not defined in the associated workflow.'); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); @@ -106,6 +109,7 @@ describe("Workflow Tests Validation Rules", () => { const testDocumentContents = ` - doc: The docs job: + My fake number: 5 My fake string: 5 `; @@ -122,7 +126,8 @@ describe("Workflow Tests Validation Rules", () => { const testDocumentContents = ` - doc: The docs job: - My fake number: + My fake number: 5 + My fake string: class: string `; @@ -130,10 +135,39 @@ describe("Workflow Tests Validation Rules", () => { expect(diagnostics.length).toBe(1); expect(diagnostics[0].message).toBe( - 'Input "My fake number" has an invalid type. Expected "int" but found "object".' + 'Input "My fake string" has an invalid type. Expected "string" but found "object".' ); expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); }); + + it("should error when an input is required but not defined", async () => { + const testDocumentContents = ` + - doc: The docs + job: + `; + + const diagnostics = await validate(testDocumentContents); + + expect(diagnostics.length).toBe(1); + expect(diagnostics[0].message).toBe('Input "My fake number" is required but no value or default was provided.'); + expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); + }); + + it("should error when an input is required in multiple tests but not defined in one of them", async () => { + const testDocumentContents = ` + - doc: The docs + job: + My fake number: 5 + - doc: The docs + job: + `; + + const diagnostics = await validate(testDocumentContents); + + expect(diagnostics.length).toBe(1); + expect(diagnostics[0].message).toBe('Input "My fake number" is required but no value or default was provided.'); + expect(diagnostics[0].severity).toBe(DiagnosticSeverity.Error); + }); }); describe("WorkflowOutputsValidationRule", () => { From f75858d1ab7ac3600a59d0615d2cb7181d5031c9 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 20 Jun 2024 17:43:02 +0200 Subject: [PATCH 42/43] Refactor validation profiles to include descriptive names --- server/gx-workflow-ls-format2/src/profiles.ts | 1 + server/gx-workflow-ls-native/src/profiles.ts | 1 + server/packages/server-common/src/languageTypes.ts | 2 +- .../src/providers/validation/profiles.ts | 2 +- .../workflow-tests-language-service/src/profiles.ts | 11 ++++------- 5 files changed, 8 insertions(+), 9 deletions(-) diff --git a/server/gx-workflow-ls-format2/src/profiles.ts b/server/gx-workflow-ls-format2/src/profiles.ts index 4caf1ac..65ee38c 100644 --- a/server/gx-workflow-ls-format2/src/profiles.ts +++ b/server/gx-workflow-ls-format2/src/profiles.ts @@ -10,6 +10,7 @@ import { InputTypeValidationRule } from "./validation/rules/InputTypeValidationR * Defines the minimal set of validation rules for gxformat2 Galaxy workflows. */ export class GxFormat2BasicValidationProfile extends BasicCommonValidationProfile { + public readonly name: string = "GxFormat2 Validation"; public static readonly RULES = new Set([ ...super.RULES, // Add more custom rules specific to gxformat2 workflows here... diff --git a/server/gx-workflow-ls-native/src/profiles.ts b/server/gx-workflow-ls-native/src/profiles.ts index 8af2fdb..1a44e6e 100644 --- a/server/gx-workflow-ls-native/src/profiles.ts +++ b/server/gx-workflow-ls-native/src/profiles.ts @@ -10,6 +10,7 @@ import { WorkflowOutputLabelValidationRule } from "./validation/rules/WorkflowOu * Defines the minimal set of validation rules for Native Galaxy workflows. */ export class NativeBasicValidationProfile extends BasicCommonValidationProfile { + public readonly name: string = "Workflow Validation"; public static readonly RULES = new Set([ ...super.RULES, // Add more custom rules specific to native workflows here... diff --git a/server/packages/server-common/src/languageTypes.ts b/server/packages/server-common/src/languageTypes.ts index 4bfe33b..3c1fd1c 100644 --- a/server/packages/server-common/src/languageTypes.ts +++ b/server/packages/server-common/src/languageTypes.ts @@ -250,7 +250,7 @@ export abstract class LanguageServiceBase implements for (const rule of profile.rules) { const contributedDiagnostics = await rule.validate(documentContext); contributedDiagnostics.forEach((diagnostic) => { - diagnostic.source = profile.name; + diagnostic.source = diagnostic.source ?? profile.name; }); diagnostics.push(...contributedDiagnostics); } diff --git a/server/packages/server-common/src/providers/validation/profiles.ts b/server/packages/server-common/src/providers/validation/profiles.ts index 4bf44f9..53046e4 100644 --- a/server/packages/server-common/src/providers/validation/profiles.ts +++ b/server/packages/server-common/src/providers/validation/profiles.ts @@ -18,7 +18,7 @@ export class NoOpValidationProfile implements ValidationProfile { * Common set of validation rules for basic validation of any workflow format. */ export class BasicCommonValidationProfile implements ValidationProfile { - public readonly name: string = "Basic Validation"; + public readonly name: string = "Workflow Validator"; protected static readonly RULES: Set = new Set([ new TestToolshedValidationRule(DiagnosticSeverity.Error), diff --git a/server/packages/workflow-tests-language-service/src/profiles.ts b/server/packages/workflow-tests-language-service/src/profiles.ts index 785b908..ef8121d 100644 --- a/server/packages/workflow-tests-language-service/src/profiles.ts +++ b/server/packages/workflow-tests-language-service/src/profiles.ts @@ -1,8 +1,5 @@ -import { DiagnosticSeverity, ValidationRule } from "@gxwf/server-common/src/languageTypes"; -import { - BasicCommonValidationProfile, - IWCCommonValidationProfile, -} from "@gxwf/server-common/src/providers/validation/profiles"; +import { DiagnosticSeverity, ValidationProfile, ValidationRule } from "@gxwf/server-common/src/languageTypes"; +import { IWCCommonValidationProfile } from "@gxwf/server-common/src/providers/validation/profiles"; import { RequiredArrayPropertyValidationRule } from "@gxwf/server-common/src/providers/validation/rules"; import { WorkflowInputsValidationRule } from "./validation/rules/WorkflowInputsValidationRule"; import { WorkflowOutputsValidationRule } from "./validation/rules/WorkflowOutputsValidationRule"; @@ -10,9 +7,9 @@ import { WorkflowOutputsValidationRule } from "./validation/rules/WorkflowOutput /** * Defines the minimal set of validation rules for Galaxy Workflow Tests Documents. */ -export class TestDocumentBasicValidationProfile extends BasicCommonValidationProfile { +export class TestDocumentBasicValidationProfile implements ValidationProfile { + public readonly name: string = "Test Document Validator"; public static readonly RULES = new Set([ - ...super.RULES, new WorkflowInputsValidationRule(DiagnosticSeverity.Error), new WorkflowOutputsValidationRule(DiagnosticSeverity.Error), // Add more custom rules specific to native workflows here... From f4841ba5332e274d4e0222cdccc26b32d99e616d Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 20 Jun 2024 17:59:08 +0200 Subject: [PATCH 43/43] Refactor hoverService to use getNodeRange from nodeManager --- .../src/services/hoverService.ts | 6 +----- .../src/services/hover.ts | 6 +----- .../src/services/validation.ts | 12 +++--------- 3 files changed, 5 insertions(+), 19 deletions(-) diff --git a/server/gx-workflow-ls-format2/src/services/hoverService.ts b/server/gx-workflow-ls-format2/src/services/hoverService.ts index f04cea7..ef0cfbc 100644 --- a/server/gx-workflow-ls-format2/src/services/hoverService.ts +++ b/server/gx-workflow-ls-format2/src/services/hoverService.ts @@ -33,11 +33,7 @@ export class GxFormat2HoverService { } } - const hoverRange = Range.create( - textDocument.positionAt(hoverRangeNode.offset), - textDocument.positionAt(hoverRangeNode.offset + hoverRangeNode.length) - ); - + const hoverRange = nodeManager.getNodeRange(hoverRangeNode); const location = nodeManager.getPathFromNode(hoverRangeNode); const schemaNode = this.schemaNodeResolver.resolveSchemaContext(location); const contents = this.getHoverMarkdownContentsForNode(schemaNode); diff --git a/server/packages/workflow-tests-language-service/src/services/hover.ts b/server/packages/workflow-tests-language-service/src/services/hover.ts index 9e9205a..6032d7f 100644 --- a/server/packages/workflow-tests-language-service/src/services/hover.ts +++ b/server/packages/workflow-tests-language-service/src/services/hover.ts @@ -47,11 +47,7 @@ export class WorkflowTestsHoverServiceImpl implements WorkflowTestsHoverService return Promise.resolve(null); } - const hoverRange = Range.create( - documentContext.textDocument.positionAt(hoverRangeNode.offset), - documentContext.textDocument.positionAt(hoverRangeNode.offset + hoverRangeNode.length) - ); - + const hoverRange = documentContext.nodeManager.getNodeRange(hoverRangeNode); if (this.parentPropertyMatchesKey(node, "job")) { const inputHover = await this.getHoverForWorkflowInput(documentContext, node, hoverRange); if (inputHover) { diff --git a/server/packages/workflow-tests-language-service/src/services/validation.ts b/server/packages/workflow-tests-language-service/src/services/validation.ts index d419836..2d1de15 100644 --- a/server/packages/workflow-tests-language-service/src/services/validation.ts +++ b/server/packages/workflow-tests-language-service/src/services/validation.ts @@ -1,4 +1,4 @@ -import { Diagnostic, DiagnosticSeverity, DocumentContext, Range } from "@gxwf/server-common/src/languageTypes"; +import { Diagnostic, DiagnosticSeverity, DocumentContext } from "@gxwf/server-common/src/languageTypes"; import { inject, injectable } from "inversify"; import { ResolvedSchema } from "../schema/jsonSchema"; import { WorkflowTestsSchemaProvider } from "../schema/provider"; @@ -37,16 +37,10 @@ export class WorkflowTestsValidationServiceImpl implements WorkflowTestsValidati const property = astRoot.type === "object" ? astRoot.properties[0] : undefined; if (property && property.keyNode.value === "$schema") { const node = property.valueNode || property; - const range = Range.create( - documentContext.textDocument.positionAt(node.offset), - documentContext.textDocument.positionAt(node.offset + node.length) - ); + const range = documentContext.nodeManager.getNodeRange(node); addProblem(Diagnostic.create(range, errorMessage, severity)); } else { - const range = Range.create( - documentContext.textDocument.positionAt(astRoot.offset), - documentContext.textDocument.positionAt(astRoot.offset + 1) - ); + const range = documentContext.nodeManager.getDefaultRange(); addProblem(Diagnostic.create(range, errorMessage, severity)); } }