diff --git a/server/src/languageTypes.ts b/server/src/languageTypes.ts index 32f40dd..e38ae36 100644 --- a/server/src/languageTypes.ts +++ b/server/src/languageTypes.ts @@ -130,7 +130,7 @@ export interface HoverContentContributor { /** * Interface for contributing additional diagnostics to the validation process. */ -export interface ValidationContributor { +export interface ValidationRule { /** * Validates the given workflow document and provides diagnostics. * @param workflowDocument The workflow document @@ -139,7 +139,7 @@ export interface ValidationContributor { } export abstract class WorkflowLanguageService { - protected _validationContributors: ValidationContributor[] = []; + protected _customValidationRules: ValidationRule[] = []; public abstract format(document: TextDocument, range: Range, options: FormattingOptions): TextEdit[]; public abstract parseWorkflowDocument(document: TextDocument): WorkflowDocument; public abstract doHover(workflowDocument: WorkflowDocument, position: Position): Promise; @@ -147,14 +147,14 @@ export abstract class WorkflowLanguageService { protected abstract doValidation(workflowDocument: WorkflowDocument): Promise; - public setValidationContributors(contributors: ValidationContributor[]): void { - this._validationContributors = contributors; + public setValidationRules(validationRules: ValidationRule[]): void { + this._customValidationRules = validationRules; } public async validate(workflowDocument: WorkflowDocument): Promise { const diagnostics = await this.doValidation(workflowDocument); - this._validationContributors.forEach(async (contributor) => { - const contributedDiagnostics = await contributor.validate(workflowDocument); + this._customValidationRules.forEach(async (validationRule) => { + const contributedDiagnostics = await validationRule.validate(workflowDocument); diagnostics.push(...contributedDiagnostics); }); return diagnostics; diff --git a/server/src/models/nativeWorkflowDocument.ts b/server/src/models/nativeWorkflowDocument.ts index b16bc6d..52cf929 100644 --- a/server/src/models/nativeWorkflowDocument.ts +++ b/server/src/models/nativeWorkflowDocument.ts @@ -1,4 +1,4 @@ -import { JSONDocument } from "vscode-json-languageservice"; +import { JSONDocument, ObjectASTNode } from "vscode-json-languageservice"; import { getPropertyNodeFromPath } from "../jsonUtils"; import { TextDocument, Range, Position, ASTNode, WorkflowDocument } from "../languageTypes"; @@ -72,4 +72,22 @@ export class NativeWorkflowDocument extends WorkflowDocument { if (!root) return null; return getPropertyNodeFromPath(root, path); } + + public override getStepNodes(): ObjectASTNode[] { + const root = this._jsonDocument.root; + if (!root) { + return []; + } + const result: ObjectASTNode[] = []; + const stepsNode = this.getNodeFromPath("steps"); + if (stepsNode && stepsNode.type === "property" && stepsNode.valueNode && stepsNode.valueNode.type === "object") { + stepsNode.valueNode.properties.forEach((stepProperty) => { + const stepNode = stepProperty.valueNode; + if (stepNode && stepNode.type === "object") { + result.push(stepNode); + } + }); + } + return result; + } } diff --git a/server/src/models/workflowDocument.ts b/server/src/models/workflowDocument.ts index bb135a8..fcb010d 100644 --- a/server/src/models/workflowDocument.ts +++ b/server/src/models/workflowDocument.ts @@ -1,4 +1,4 @@ -import { TextDocument, Range, Position, ASTNode } from "../languageTypes"; +import { TextDocument, Range, Position, ASTNode, ObjectASTNode } from "../languageTypes"; import { URI } from "vscode-uri"; /** @@ -28,6 +28,8 @@ export abstract class WorkflowDocument { public abstract getNodeFromPath(path: string): ASTNode | null; + public abstract getStepNodes(): ObjectASTNode[]; + /** Returns a small Range at the beginning of the document */ public getDefaultRange(): Range { return Range.create(this.textDocument.positionAt(0), this.textDocument.positionAt(1)); diff --git a/server/src/providers/validation/MissingPropertyValidation.ts b/server/src/providers/validation/MissingPropertyValidation.ts index 9302385..d82daf3 100644 --- a/server/src/providers/validation/MissingPropertyValidation.ts +++ b/server/src/providers/validation/MissingPropertyValidation.ts @@ -1,7 +1,7 @@ import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; -import { ValidationContributor, WorkflowDocument } from "../../languageTypes"; +import { ValidationRule, WorkflowDocument } from "../../languageTypes"; -export class MissingPropertyValidationRule implements ValidationContributor { +export class MissingPropertyValidationRule implements ValidationRule { constructor(readonly nodePath: string, readonly severity?: DiagnosticSeverity | undefined) {} validate(workflowDocument: WorkflowDocument): Promise { @@ -9,7 +9,7 @@ export class MissingPropertyValidationRule implements ValidationContributor { const targetNode = workflowDocument.getNodeFromPath(this.nodePath); if (!targetNode) { result.push({ - message: `Property '${this.nodePath}' is missing`, + message: `Missing property "${this.nodePath}".`, range: workflowDocument.getDefaultRange(), severity: this.severity, }); diff --git a/server/src/providers/validation/WorkflowOutputLabelValidation.ts b/server/src/providers/validation/WorkflowOutputLabelValidation.ts new file mode 100644 index 0000000..7c3a75b --- /dev/null +++ b/server/src/providers/validation/WorkflowOutputLabelValidation.ts @@ -0,0 +1,29 @@ +import { Diagnostic, DiagnosticSeverity } from "vscode-languageserver-types"; +import { ValidationRule, WorkflowDocument } from "../../languageTypes"; + +export class WorkflowOutputLabelValidation implements ValidationRule { + constructor(readonly severity: DiagnosticSeverity = DiagnosticSeverity.Error) {} + + validate(workflowDocument: WorkflowDocument): Promise { + const result: Diagnostic[] = []; + const stepNodes = workflowDocument.getStepNodes(); + stepNodes.forEach((step) => { + const workflowOutputs = step.properties.find((property) => property.keyNode.value === "workflow_outputs"); + if (workflowOutputs && workflowOutputs.valueNode && workflowOutputs.valueNode.type === "array") { + workflowOutputs.valueNode.items.forEach((outputNode) => { + if (outputNode.type === "object") { + const labelNode = outputNode.properties.find((property) => property.keyNode.value === "label"); + if (!labelNode?.valueNode?.value) { + result.push({ + message: `Missing label in workflow output.`, + range: workflowDocument.getNodeRange(outputNode), + severity: this.severity, + }); + } + } + }); + } + }); + return Promise.resolve(result); + } +} diff --git a/server/tests/testHelpers.ts b/server/tests/testHelpers.ts index 305cc57..878f286 100644 --- a/server/tests/testHelpers.ts +++ b/server/tests/testHelpers.ts @@ -1,6 +1,7 @@ import { ASTNode, getLanguageService, JSONDocument, JSONPath } from "vscode-json-languageservice"; import { TextDocument } from "../src/languageTypes"; import * as Json from "jsonc-parser"; +import { NativeWorkflowDocument } from "../src/models/nativeWorkflowDocument"; export function toJsonDocument(contents: string): { textDoc: TextDocument; jsonDoc: JSONDocument } { const textDoc = TextDocument.create("foo://bar/file.json", "json", 0, contents); @@ -23,3 +24,8 @@ export function getNodeValue(node: ASTNode): unknown { export function getNodePath(node: ASTNode): JSONPath { return Json.getNodePath(node); } + +export function createNativeWorkflowDocument(contents: string): NativeWorkflowDocument { + const { textDoc, jsonDoc } = toJsonDocument(contents); + return new NativeWorkflowDocument(textDoc, jsonDoc); +} diff --git a/server/tests/testWorkflowProvider.ts b/server/tests/testWorkflowProvider.ts new file mode 100644 index 0000000..7b94c6f --- /dev/null +++ b/server/tests/testWorkflowProvider.ts @@ -0,0 +1,43 @@ +import * as fs from "fs"; +import * as path from "path"; + +const TEST_DATA_PATH = path.join(__dirname, "..", "..", "test-data"); + +interface TestJsonWorkflows { + /** Workflows for testing validation issues. */ + validation: { + /** Invalid workflow without steps. */ + withoutSteps: string; + /** Valid workflow with 1 step. */ + withOneStep: string; + /** Invalid workflow with 3 steps. The steps are missing UUID and workflow_outputs. */ + withThreeSteps: string; + /** Workflow with 1 step. The step has 2 workflow_outputs without labels. */ + withoutWorkflowOutputLabels: string; + /** Workflow with 1 step. The step has 2 workflow_outputs with labels. */ + withWorkflowOutputLabels: string; + }; +} + +export class TestWorkflowProvider { + private static _jsonWorkflows: TestJsonWorkflows = { + validation: { + withoutSteps: fs.readFileSync(path.join(TEST_DATA_PATH, "json", "validation", "test_wf_00.ga"), "utf-8"), + withOneStep: fs.readFileSync(path.join(TEST_DATA_PATH, "json", "validation", "test_wf_01.ga"), "utf-8"), + withThreeSteps: fs.readFileSync(path.join(TEST_DATA_PATH, "json", "validation", "test_wf_02.ga"), "utf-8"), + withoutWorkflowOutputLabels: fs.readFileSync( + path.join(TEST_DATA_PATH, "json", "validation", "test_wf_03.ga"), + "utf-8" + ), + withWorkflowOutputLabels: fs.readFileSync( + path.join(TEST_DATA_PATH, "json", "validation", "test_wf_04.ga"), + "utf-8" + ), + }, + }; + + /** Workflows in native JSON format. */ + public static get nativeJson(): TestJsonWorkflows { + return this._jsonWorkflows; + } +} diff --git a/server/tests/unit/customValidationRules.test.ts b/server/tests/unit/customValidationRules.test.ts new file mode 100644 index 0000000..9eef510 --- /dev/null +++ b/server/tests/unit/customValidationRules.test.ts @@ -0,0 +1,44 @@ +import { createNativeWorkflowDocument } from "../testHelpers"; +import { WorkflowOutputLabelValidation } from "../../src/providers/validation/WorkflowOutputLabelValidation"; +import { TestWorkflowProvider } from "../testWorkflowProvider"; + +describe("Custom Validation Rules", () => { + describe("WorkflowOutputLabelValidation Rule", () => { + let rule: WorkflowOutputLabelValidation; + + beforeEach(() => { + rule = new WorkflowOutputLabelValidation(); + }); + + it("should not provide diagnostics when there are no steps", async () => { + const wfDocument = createNativeWorkflowDocument(TestWorkflowProvider.nativeJson.validation.withoutSteps); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(0); + }); + + it("should not provide diagnostics when there are no workflow_outputs in the steps", async () => { + const wfDocument = createNativeWorkflowDocument(TestWorkflowProvider.nativeJson.validation.withThreeSteps); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(0); + }); + + it("should not provide diagnostics when the steps contains workflow_outputs with label", async () => { + const wfDocument = createNativeWorkflowDocument( + TestWorkflowProvider.nativeJson.validation.withWorkflowOutputLabels + ); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(0); + }); + + it("should provide diagnostics when the steps contains workflow_outputs without label", async () => { + const wfDocument = createNativeWorkflowDocument( + TestWorkflowProvider.nativeJson.validation.withoutWorkflowOutputLabels + ); + const diagnostics = await rule.validate(wfDocument); + expect(diagnostics).toHaveLength(2); + diagnostics.forEach((diagnostic) => { + expect(diagnostic.message).toBe("Missing label in workflow output."); + }); + }); + }); +}); diff --git a/server/tests/unit/nativeWorkflowDocument.test.ts b/server/tests/unit/nativeWorkflowDocument.test.ts new file mode 100644 index 0000000..c1bd22d --- /dev/null +++ b/server/tests/unit/nativeWorkflowDocument.test.ts @@ -0,0 +1,17 @@ +import { createNativeWorkflowDocument } from "../testHelpers"; +import { TestWorkflowProvider } from "../testWorkflowProvider"; + +describe("NativeWorkflowDocument", () => { + describe("getStepNodes", () => { + it.each([ + ["", 0], + [TestWorkflowProvider.nativeJson.validation.withoutSteps, 0], + [TestWorkflowProvider.nativeJson.validation.withOneStep, 1], + [TestWorkflowProvider.nativeJson.validation.withThreeSteps, 3], + ])("returns the expected number of steps", (wf_content: string, expectedNumSteps: number) => { + const wfDocument = createNativeWorkflowDocument(wf_content); + const stepNodes = wfDocument.getStepNodes(); + expect(stepNodes).toHaveLength(expectedNumSteps); + }); + }); +}); diff --git a/test-data/json/validation/test_wf_00.ga b/test-data/json/validation/test_wf_00.ga new file mode 100644 index 0000000..bcb73ce --- /dev/null +++ b/test-data/json/validation/test_wf_00.ga @@ -0,0 +1,6 @@ +{ + "a_galaxy_workflow": "true", + "format-version": "0.1", + "name": "Test Workflow Without Steps", + "steps": {} +} diff --git a/test-data/json/validation/test_wf_01.ga b/test-data/json/validation/test_wf_01.ga index b45ce03..a0a51e1 100644 --- a/test-data/json/validation/test_wf_01.ga +++ b/test-data/json/validation/test_wf_01.ga @@ -1,87 +1,18 @@ { "a_galaxy_workflow": "true", - "annotation": "simple workflow", "format-version": "0.1", - "name": "TestWorkflow1", + "name": "Test Workflow Minimum Valid", "steps": { "0": { - "annotation": "input1 description", "id": 0, - "input_connections": {}, - "inputs": [ - { - "description": "input1 description", - "name": "WorkflowInput1" - } - ], - "name": "Input dataset", - "outputs": [], - "position": { - "left": 199.55555772781372, - "top": 200.66666460037231 - }, - "tool_errors": null, - "tool_id": null, - "tool_state": "{\"name\": \"WorkflowInput1\"}", - "tool_version": null, + "name": "Test Step", "type": "data_input", - "user_outputs": [] - }, - "1": { - "annotation": "", - "id": 1, + "annotation": "Step description", + "uuid": "692d2674-5e70-4e01-ad12-4ce5572c39e5", "input_connections": {}, - "inputs": [ - { - "description": "", - "name": "WorkflowInput2" - } - ], - "name": "Input dataset", - "outputs": [], - "position": { - "left": 206.22221422195435, - "top": 327.33335161209106 - }, - "tool_errors": null, - "tool_id": null, - "tool_state": "{\"name\": \"WorkflowInput2\"}", - "tool_version": null, - "type": "data_input", - "user_outputs": [] - }, - "2": { - "annotation": "", - "id": 2, - "input_connections": { - "input1": { - "id": 0, - "output_name": "output" - }, - "queries_0|input2": { - "id": 1, - "output_name": "output" - } - }, "inputs": [], - "name": "Concatenate datasets", - "outputs": [ - { - "name": "out_file1", - "type": "input" - } - ], - "position": { - "left": 419.33335876464844, - "top": 200.44446563720703 - }, - "post_job_actions": {}, - "tool_errors": null, - "tool_id": "cat1", - "tool_state": "{\"__page__\": 0, \"__rerun_remap_job_id__\": null, \"input1\": \"null\", \"queries\": \"[{\\\"input2\\\": null, \\\"__index__\\\": 0}]\"}", - "tool_version": "1.0.0", - "type": "tool", - "user_outputs": [] + "outputs": [], + "workflow_outputs": [] } } } diff --git a/test-data/json/validation/test_wf_02.ga b/test-data/json/validation/test_wf_02.ga new file mode 100644 index 0000000..0b9827b --- /dev/null +++ b/test-data/json/validation/test_wf_02.ga @@ -0,0 +1,87 @@ +{ + "a_galaxy_workflow": "true", + "annotation": "The steps are missing UUIDs and workflow_outputs", + "format-version": "0.1", + "name": "Test Workflow with 3 steps", + "steps": { + "0": { + "annotation": "input1 description", + "id": 0, + "input_connections": {}, + "inputs": [ + { + "description": "input1 description", + "name": "WorkflowInput1" + } + ], + "name": "Input dataset", + "outputs": [], + "position": { + "left": 199.55555772781372, + "top": 200.66666460037231 + }, + "tool_errors": null, + "tool_id": null, + "tool_state": "{\"name\": \"WorkflowInput1\"}", + "tool_version": null, + "type": "data_input", + "user_outputs": [] + }, + "1": { + "annotation": "", + "id": 1, + "input_connections": {}, + "inputs": [ + { + "description": "", + "name": "WorkflowInput2" + } + ], + "name": "Input dataset", + "outputs": [], + "position": { + "left": 206.22221422195435, + "top": 327.33335161209106 + }, + "tool_errors": null, + "tool_id": null, + "tool_state": "{\"name\": \"WorkflowInput2\"}", + "tool_version": null, + "type": "data_input", + "user_outputs": [] + }, + "2": { + "annotation": "", + "id": 2, + "input_connections": { + "input1": { + "id": 0, + "output_name": "output" + }, + "queries_0|input2": { + "id": 1, + "output_name": "output" + } + }, + "inputs": [], + "name": "Concatenate datasets", + "outputs": [ + { + "name": "out_file1", + "type": "input" + } + ], + "position": { + "left": 419.33335876464844, + "top": 200.44446563720703 + }, + "post_job_actions": {}, + "tool_errors": null, + "tool_id": "cat1", + "tool_state": "{\"__page__\": 0, \"__rerun_remap_job_id__\": null, \"input1\": \"null\", \"queries\": \"[{\\\"input2\\\": null, \\\"__index__\\\": 0}]\"}", + "tool_version": "1.0.0", + "type": "tool", + "user_outputs": [] + } + } +} diff --git a/test-data/json/validation/test_wf_03.ga b/test-data/json/validation/test_wf_03.ga new file mode 100644 index 0000000..2def6a9 --- /dev/null +++ b/test-data/json/validation/test_wf_03.ga @@ -0,0 +1,28 @@ +{ + "a_galaxy_workflow": "true", + "annotation": "Workflow without labels in the workflow_outputs", + "format-version": "0.1", + "name": "Test Workflow", + "steps": { + "0": { + "id": 0, + "name": "Test Step", + "type": "data_input", + "annotation": "Step description", + "uuid": "692d2674-5e70-4e01-ad12-4ce5572c39e5", + "input_connections": {}, + "inputs": [], + "outputs": [], + "workflow_outputs": [ + { + "output_name": "output1", + "uuid": "7f08baab-5426-427e-9640-85815d809261" + }, + { + "output_name": "output2", + "uuid": "b58fce9c-e507-4714-abfc-739607e02eed" + } + ] + } + } +} diff --git a/test-data/json/validation/test_wf_04.ga b/test-data/json/validation/test_wf_04.ga new file mode 100644 index 0000000..58aec62 --- /dev/null +++ b/test-data/json/validation/test_wf_04.ga @@ -0,0 +1,30 @@ +{ + "a_galaxy_workflow": "true", + "annotation": "Valid workflow with labels in the workflow_outputs", + "format-version": "0.1", + "name": "Test Workflow", + "steps": { + "0": { + "id": 0, + "name": "Test Step", + "type": "data_input", + "annotation": "Step description", + "uuid": "692d2674-5e70-4e01-ad12-4ce5572c39e5", + "input_connections": {}, + "inputs": [], + "outputs": [], + "workflow_outputs": [ + { + "label": "The first output", + "output_name": "output1", + "uuid": "7f08baab-5426-427e-9640-85815d809261" + }, + { + "label": "The second output", + "output_name": "output2", + "uuid": "b58fce9c-e507-4714-abfc-739607e02eed" + } + ] + } + } +} diff --git a/workflow-languages/schemas/native.schema.json b/workflow-languages/schemas/native.schema.json index 27b72db..9ae077a 100644 --- a/workflow-languages/schemas/native.schema.json +++ b/workflow-languages/schemas/native.schema.json @@ -59,9 +59,8 @@ "$ref": "#/definitions/tags" }, "uuid": { - "type": "string", - "markdownDescription": "UUID associated with the workflow.", - "pattern": "^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$" + "$ref": "#/definitions/uuid", + "markdownDescription": "UUID associated with the workflow." }, "steps": { "$ref": "#/definitions/steps" @@ -226,9 +225,8 @@ "enum": ["tool", "subworkflow", "parameter_input", "data_input", "data_collection_input"] }, "uuid": { - "type": "string", - "markdownDescription": "UUID associated with the workflow step.", - "pattern": "^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$" + "$ref": "#/definitions/uuid", + "markdownDescription": "UUID associated with the workflow step." }, "workflow_outputs": { "$ref": "#/definitions/workflowOutputs" @@ -303,7 +301,24 @@ }, "workflowOutputs": { "type": "array", - "items": {} + "items": { + "$ref": "#/definitions/workflowOutput" + } + }, + "workflowOutput": { + "type": "object", + "properties": { + "label": { + "type": "string" + }, + "output_name": { + "type": "string" + }, + "uuid": { + "$ref": "#/definitions/uuid", + "markdownDescription": "UUID associated with the workflow output." + } + } }, "position": { "type": "object", @@ -319,6 +334,10 @@ }, "optionalString": { "type": ["string", "null"] + }, + "uuid": { + "type": "string", + "pattern": "^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$" } } }