Merge pull request #44 from davelopez/add_output_labels_rule

Custom Validation Rule: `workflow_outputs` must have a label

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,22 +139,22 @@ 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<Hover | null>;
   public abstract doComplete(workflowDocument: WorkflowDocument, position: Position): Promise<CompletionList | null>;
 
   protected abstract doValidation(workflowDocument: WorkflowDocument): Promise<Diagnostic[]>;
 
-  public setValidationContributors(contributors: ValidationContributor[]): void {
-    this._validationContributors = contributors;
+  public setValidationRules(validationRules: ValidationRule[]): void {
+    this._customValidationRules = validationRules;
   }
 
   public async validate(workflowDocument: WorkflowDocument): Promise<Diagnostic[]> {
     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;

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;
+  }
 }

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));

server/src/providers/validation/MissingPropertyValidation.ts

@@ -1,15 +1,15 @@
 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<Diagnostic[]> {
     const result: Diagnostic[] = [];
     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,
       });

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<Diagnostic[]> {
+    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);
+  }
+}

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);
+}

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;
+  }
+}

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.");
+      });
+    });
+  });
+});

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);
+    });
+  });
+});

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": {}
+}

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": []
         }
     }
 }