From bf7e44069222bad9de5b01c4ffeb64c611bfc8b0 Mon Sep 17 00:00:00 2001 From: Christopher Schleiden Date: Mon, 21 Nov 2022 17:55:24 -0800 Subject: [PATCH] Expression validation --- .../src/complete.expressions.test.ts | 2 +- .../src/context-providers/config.ts | 2 +- .../src/context-providers/default.ts | 8 +- .../expression-validation/error-dictionary.ts | 38 ++++++ .../src/test-utils/cursor-position.ts | 5 +- .../src/test-utils/document.ts | 5 + .../src/validate.expressions.test.ts | 57 ++++++++ actions-languageservice/src/validate.test.ts | 92 ++++++------- actions-languageservice/src/validate.ts | 123 ++++++++++++++---- 9 files changed, 248 insertions(+), 84 deletions(-) create mode 100644 actions-languageservice/src/expression-validation/error-dictionary.ts create mode 100644 actions-languageservice/src/test-utils/document.ts create mode 100644 actions-languageservice/src/validate.expressions.test.ts diff --git a/actions-languageservice/src/complete.expressions.test.ts b/actions-languageservice/src/complete.expressions.test.ts index fd6eee0..1bc0114 100644 --- a/actions-languageservice/src/complete.expressions.test.ts +++ b/actions-languageservice/src/complete.expressions.test.ts @@ -4,7 +4,7 @@ import {ContextProviderConfig} from "./context-providers/config"; import {getPositionFromCursor} from "./test-utils/cursor-position"; const contextProviderConfig: ContextProviderConfig = { - getContext: async (context: string) => { + getContext: (context: string) => { switch (context) { case "github": return new data.Dictionary({ diff --git a/actions-languageservice/src/context-providers/config.ts b/actions-languageservice/src/context-providers/config.ts index 08c441f..e77057a 100644 --- a/actions-languageservice/src/context-providers/config.ts +++ b/actions-languageservice/src/context-providers/config.ts @@ -3,7 +3,7 @@ import {Dictionary} from "@github/actions-expressions/data/dictionary"; import {ExpressionData, Pair} from "@github/actions-expressions/data/expressiondata"; export type ContextProviderConfig = { - getContext: (name: string) => Promise; + getContext: (name: string) => data.Dictionary | undefined; }; /** diff --git a/actions-languageservice/src/context-providers/default.ts b/actions-languageservice/src/context-providers/default.ts index cc964e9..aa10e1a 100644 --- a/actions-languageservice/src/context-providers/default.ts +++ b/actions-languageservice/src/context-providers/default.ts @@ -1,16 +1,16 @@ import {data} from "@github/actions-expressions"; import {ContextProviderConfig} from "./config"; -export async function getContext(names: string[], config: ContextProviderConfig | undefined): Promise { +export function getContext(names: string[], config: ContextProviderConfig | undefined): data.Dictionary { const context = new data.Dictionary(); for (const contextName of names) { let value: data.Dictionary | undefined; - value = await getDefaultContext(contextName); + value = getDefaultContext(contextName); if (!value) { - value = await config?.getContext(contextName); + value = config?.getContext(contextName); } if (!value) { @@ -23,7 +23,7 @@ export async function getContext(names: string[], config: ContextProviderConfig return context; } -async function getDefaultContext(name: string): Promise { +function getDefaultContext(name: string): data.Dictionary | undefined { switch (name) { case "runner": return objectToDictionary({ diff --git a/actions-languageservice/src/expression-validation/error-dictionary.ts b/actions-languageservice/src/expression-validation/error-dictionary.ts new file mode 100644 index 0000000..df96c42 --- /dev/null +++ b/actions-languageservice/src/expression-validation/error-dictionary.ts @@ -0,0 +1,38 @@ +import {data} from "@github/actions-expressions"; +import {isDictionary} from "@github/actions-expressions/data/dictionary"; +import {ExpressionData, Pair} from "@github/actions-expressions/data/expressiondata"; + +export class AccessError extends Error { + constructor(message: string, public readonly keyName: string) { + super(message); + } +} + +export class ErrorDictionary extends data.Dictionary { + constructor(...pairs: Pair[]) { + super(...pairs); + } + + get(key: string): ExpressionData | undefined { + const value = super.get(key); + if (value) { + return value; + } + + throw new AccessError(`Invalid context access: ${key}`, key); + } +} + +export function wrapDictionary(d: data.Dictionary): ErrorDictionary { + const e = new ErrorDictionary(); + + for (const {key, value} of d.pairs()) { + if (isDictionary(value)) { + e.add(key, wrapDictionary(value)); + } else { + e.add(key, value); + } + } + + return e; +} diff --git a/actions-languageservice/src/test-utils/cursor-position.ts b/actions-languageservice/src/test-utils/cursor-position.ts index 1dceb57..51d2ed4 100644 --- a/actions-languageservice/src/test-utils/cursor-position.ts +++ b/actions-languageservice/src/test-utils/cursor-position.ts @@ -1,9 +1,10 @@ -import {TextDocument, Position} from "vscode-languageserver-textdocument"; +import {Position, TextDocument} from "vscode-languageserver-textdocument"; +import {createDocument} from "./document"; // Calculates the position of the cursor and the document without that cursor // Cursor is represented by a `|` character export function getPositionFromCursor(input: string): [TextDocument, Position] { - const doc = TextDocument.create("test://test/test.yaml", "yaml", 0, input); + const doc = createDocument("test.yaml", input); const cursorIndex = doc.getText().indexOf("|"); if (cursorIndex === -1) { diff --git a/actions-languageservice/src/test-utils/document.ts b/actions-languageservice/src/test-utils/document.ts new file mode 100644 index 0000000..7e4ecc2 --- /dev/null +++ b/actions-languageservice/src/test-utils/document.ts @@ -0,0 +1,5 @@ +import {TextDocument} from "vscode-languageserver-textdocument"; + +export function createDocument(fileName: string, content: string): TextDocument { + return TextDocument.create("test://test/" + fileName, "yaml", 0, content); +} diff --git a/actions-languageservice/src/validate.expressions.test.ts b/actions-languageservice/src/validate.expressions.test.ts new file mode 100644 index 0000000..0cf6b5d --- /dev/null +++ b/actions-languageservice/src/validate.expressions.test.ts @@ -0,0 +1,57 @@ +import {DiagnosticSeverity} from "vscode-languageserver-types"; +import {createDocument} from "./test-utils/document"; +import {validate} from "./validate"; + +describe("expression validation", () => { + it("access invalid context field", async () => { + const result = await validate( + createDocument( + "wf.yaml", + "on: push\nrun-name: name-${{ github.does-not-exist }}\njobs:\n build:\n runs-on: ubuntu-latest\n steps:\n - run: echo" + ) + ); + + expect(result).toEqual([ + { + message: "Context might be invalid: does-not-exist", + range: { + end: { + character: 43, + line: 1 + }, + start: { + character: 10, + line: 1 + } + }, + severity: DiagnosticSeverity.Warning + } + ]); + }); + + it("access invalid nested context field", async () => { + const result = await validate( + createDocument( + "wf.yaml", + "on: push\nrun-name: name-${{ github.does-not-exist.again }}\njobs:\n build:\n runs-on: ubuntu-latest\n steps:\n - run: echo" + ) + ); + + expect(result).toEqual([ + { + message: "Context might be invalid: does-not-exist", + range: { + end: { + character: 49, + line: 1 + }, + start: { + character: 10, + line: 1 + } + }, + severity: DiagnosticSeverity.Warning + } + ]); + }); +}); diff --git a/actions-languageservice/src/validate.test.ts b/actions-languageservice/src/validate.test.ts index 7caac1c..06616e0 100644 --- a/actions-languageservice/src/validate.test.ts +++ b/actions-languageservice/src/validate.test.ts @@ -1,68 +1,60 @@ -import {parseWorkflow} from "@github/actions-workflow-parser"; -import {TemplateValidationError} from "@github/actions-workflow-parser/templates/template-validation-error"; -import {nullTrace} from "./nulltrace"; +import {Diagnostic} from "vscode-languageserver-types"; +import {createDocument} from "./test-utils/document"; +import {validate} from "./validate"; describe("validation", () => { - it("valid workflow", () => { - const result = parseWorkflow( - "wf.yaml", - [ - { - name: "wf.yaml", - content: "on: push\njobs:\n build:\n runs-on: ubuntu-latest" - } - ], - nullTrace - ); + it("valid workflow", async () => { + const result = await validate(createDocument("wf.yaml", "on: push\njobs:\n build:\n runs-on: ubuntu-latest")); - expect(result.context.errors.getErrors().length).toBe(0); + expect(result.length).toBe(0); }); - it("missing jobs key", () => { - const result = parseWorkflow( - "wf.yaml", - [ - { - name: "wf.yaml", - content: "on: push" - } - ], - nullTrace - ); + it("missing jobs key", async () => { + const result = await validate(createDocument("wf.yaml", "on: push")); - expect(result.context.errors.getErrors().length).toBe(1); - expect(result.context.errors.getErrors()[0]).toEqual( - new TemplateValidationError("Required property is missing: jobs", "wf.yaml (Line: 1, Col: 1)", undefined, { - start: [1, 1], - end: [1, 9] - }) - ); + expect(result.length).toBe(1); + expect(result[0]).toEqual({ + message: "Required property is missing: jobs", + range: { + start: { + line: 0, + character: 0 + }, + end: { + line: 0, + character: 8 + } + } + } as Diagnostic); }); - it("extraneous key", () => { - const result = parseWorkflow( - "wf.yaml", - [ - { - name: "wf.yaml", - content: `on: push + it("extraneous key", async () => { + const result = await validate( + createDocument( + "wf.yaml", + `on: push unknown-key: foo jobs: build: runs-on: ubuntu-latest steps: - run: echo` - } - ], - nullTrace + ) ); - expect(result.context.errors.getErrors().length).toBe(1); - expect(result.context.errors.getErrors()[0]).toEqual( - new TemplateValidationError("Unexpected value 'unknown-key'", "wf.yaml (Line: 2, Col: 1)", undefined, { - start: [2, 1], - end: [2, 12] - }) - ); + expect(result.length).toBe(1); + expect(result[0]).toEqual({ + message: "Unexpected value 'unknown-key'", + range: { + end: { + character: 11, + line: 1 + }, + start: { + character: 0, + line: 1 + } + } + } as Diagnostic); }); }); diff --git a/actions-languageservice/src/validate.ts b/actions-languageservice/src/validate.ts index 64fc67f..daa8eac 100644 --- a/actions-languageservice/src/validate.ts +++ b/actions-languageservice/src/validate.ts @@ -1,9 +1,23 @@ -import {convertWorkflowTemplate, parseWorkflow, ParseWorkflowResult} from "@github/actions-workflow-parser"; +import {Evaluator, Lexer, Parser} from "@github/actions-expressions"; +import {Expr} from "@github/actions-expressions/ast"; +import { + convertWorkflowTemplate, + isBasicExpression, + parseWorkflow, + ParseWorkflowResult +} from "@github/actions-workflow-parser"; +import {splitAllowedContext} from "@github/actions-workflow-parser/templates/tokens/expression-token"; +import {TemplateToken} from "@github/actions-workflow-parser/templates/tokens/template-token"; import {TokenRange} from "@github/actions-workflow-parser/templates/tokens/token-range"; import {File} from "@github/actions-workflow-parser/workflows/file"; import {TextDocument} from "vscode-languageserver-textdocument"; -import {Diagnostic, Range} from "vscode-languageserver-types"; +import {Diagnostic, DiagnosticSeverity, Range} from "vscode-languageserver-types"; + +import {ContextProviderConfig} from "./context-providers/config"; +import {getContext} from "./context-providers/default"; +import {AccessError, wrapDictionary} from "./expression-validation/error-dictionary"; import {nullTrace} from "./nulltrace"; +import {ValueProviderConfig} from "./value-providers/config"; /** * Validates a workflow file @@ -12,53 +26,56 @@ import {nullTrace} from "./nulltrace"; * @returns Array of diagnostics */ export async function validate( - textDocument: TextDocument + textDocument: TextDocument, // TODO: Support multiple files, context for API calls + valueProviderConfig?: ValueProviderConfig, + contextProviderConfig?: ContextProviderConfig ): Promise { const file: File = { name: textDocument.uri, content: textDocument.getText() }; + const diagnostics: Diagnostic[] = []; + try { const result: ParseWorkflowResult = parseWorkflow(file.name, [file], nullTrace); - if (result.value) { // Errors will be updated in the context convertWorkflowTemplate(result.context, result.value); } - // For now map parser errors directly to diagnostics - return result.context.errors.getErrors().map(error => { - let range = mapRange(error.range); - if (!range) { - // Use default range - range = { - start: { - line: 1, - character: 1 - }, - end: { - line: 1, - character: 1 - } - }; - } + // Validate expressions + validateExpressions(diagnostics, result, contextProviderConfig); - return { + // For now map parser errors directly to diagnostics + for (const error of result.context.errors.getErrors()) { + let range = mapRange(error.range); + + diagnostics.push({ message: error.rawMessage, range - }; - }); + }); + } } catch (e) { // TODO: Handle error here - return []; } + + return diagnostics; } -function mapRange(range: TokenRange | undefined): Range | undefined { +function mapRange(range: TokenRange | undefined): Range { if (!range) { - return undefined; + return { + start: { + line: 1, + character: 1 + }, + end: { + line: 1, + character: 1 + } + }; } return { @@ -72,3 +89,57 @@ function mapRange(range: TokenRange | undefined): Range | undefined { } }; } + +function validateExpressions( + diagnotics: Diagnostic[], + result: ParseWorkflowResult, + contextProviderConfig: ContextProviderConfig | undefined +) { + if (!result.value) { + return; + } + + // Iterate over the parsed workflow + for (const token of TemplateToken.traverse(result.value)) { + if (isBasicExpression(token)) { + // Validate the expression + for (const expression of token.originalExpressions || [token]) { + const allowedContexts = token.definition?.readerContext || []; + const {namedContexts, functions} = splitAllowedContext(allowedContexts); + + let expr: Expr | undefined; + + try { + const l = new Lexer(expression.expression); + const lr = l.lex(); + + const p = new Parser(lr.tokens, namedContexts, functions); + expr = p.parse(); + } catch { + // Ignore any error here, we should've caught this earlier in the parsing process + continue; + } + + try { + const context = getContext(namedContexts, contextProviderConfig); + + const e = new Evaluator(expr, wrapDictionary(context)); + e.evaluate(); + + // Any invalid context access would've thrown an error via the `ErrorDictionary`, for now we don't have to check the actual + // result of the evaluation. + } catch (e) { + if (e instanceof AccessError) { + diagnotics.push({ + message: `Context access might be invalid: ${e.keyName}`, + severity: DiagnosticSeverity.Warning, + range: mapRange(expression.range) + }); + } else { + // Ignore error + } + } + } + } + } +}