improvement: Point to YAML file for template string error messages if…

… possible (#6692) * feat: improve template string error messages Include a snippet to the template string in a YAML file, and point to YAML file if available. * refactor: Use named parameters instead of positional in `loadAndValidateYaml` * fix: line number calculation
garden-io · Dec 10, 2024 · a9c205b · a9c205b
1 parent 884241b
commit a9c205b
Show file tree

Hide file tree

Showing 8 changed files with 164 additions and 45 deletions.
diff --git a/core/src/config/base.ts b/core/src/config/base.ts
@@ -54,6 +54,7 @@ export type ObjectPath = (string | number)[]
 
 export interface YamlDocumentWithSource extends Document {
   source: string
+  filename: string | undefined
 }
 
 export function getEffectiveConfigFileLocation(actionConfig: BaseActionConfig): string {
@@ -125,11 +126,17 @@ export const allConfigKinds = ["Module", "Workflow", "Project", configTemplateKi
  * @param sourceDescription - A description of the location of the yaml file, e.g. "bar.yaml at directory /foo/".
  * @param version - YAML standard version. Defaults to "1.2"
  */
-export async function loadAndValidateYaml(
-  content: string,
-  sourceDescription: string,
-  version: DocumentOptions["version"] = "1.2"
-): Promise<YamlDocumentWithSource[]> {
+export async function loadAndValidateYaml({
+  content,
+  sourceDescription,
+  filename,
+  version = "1.2",
+}: {
+  content: string
+  sourceDescription: string
+  filename: string | undefined
+  version?: DocumentOptions["version"]
+}): Promise<YamlDocumentWithSource[]> {
   try {
     return Array.from(parseAllDocuments(content, { merge: true, strict: false, version }) || []).map((doc) => {
       if (doc.errors.length > 0) {
@@ -142,6 +149,7 @@ export async function loadAndValidateYaml(
 
       const docWithSource = doc as YamlDocumentWithSource
       docWithSource.source = content
+      docWithSource.filename = filename
 
       return docWithSource
     })
@@ -197,7 +205,11 @@ export async function validateRawConfig({
   projectRoot: string
   allowInvalid?: boolean
 }) {
-  let rawSpecs = await loadAndValidateYaml(rawConfig, `${basename(configPath)} in directory ${dirname(configPath)}`)
+  let rawSpecs = await loadAndValidateYaml({
+    content: rawConfig,
+    sourceDescription: `${basename(configPath)} in directory ${dirname(configPath)}`,
+    filename: configPath,
+  })
 
   // Ignore empty resources
   rawSpecs = rawSpecs.filter(Boolean)

diff --git a/core/src/config/validation.ts b/core/src/config/validation.ts
@@ -11,7 +11,7 @@ import { ConfigurationError } from "../exceptions.js"
 import { relative } from "path"
 import { uuidv4 } from "../util/random.js"
 import type { BaseGardenResource, ObjectPath, YamlDocumentWithSource } from "./base.js"
-import type { ParsedNode, Range } from "yaml"
+import type { ParsedNode } from "yaml"
 import { padEnd } from "lodash-es"
 import { styles } from "../logger/styles.js"
 
@@ -130,8 +130,6 @@ export function validateSchema<T>(
     return result.value
   }
 
-  const yamlDoc = source?.yamlDoc
-  const rawYaml = yamlDoc?.source
   const yamlBasePath = source?.path || []
 
   const errorDetails = error.details.map((e) => {
@@ -140,15 +138,16 @@ export function validateSchema<T>(
         ? improveZodValidationErrorMessage(e, yamlBasePath)
         : improveJoiValidationErrorMessage(e, schema, yamlBasePath)
 
-    const node = yamlDoc?.getIn([...yamlBasePath, ...e.path], true) as ParsedNode | undefined
-    const range = node?.range
-
-    if (rawYaml && yamlDoc?.contents && range) {
-      try {
-        e.message = addYamlContext({ rawYaml, range, message: e.message })
-      } catch {
-        // ignore
-      }
+    try {
+      e.message = addYamlContext({
+        source: {
+          ...source,
+          path: [...yamlBasePath, ...e.path],
+        },
+        message: e.message,
+      })
+    } catch {
+      // ignore
     }
 
     return e
@@ -218,7 +217,25 @@ function improveZodValidationErrorMessage(item: Joi.ValidationErrorItem, yamlBas
   }
 }
 
-function addYamlContext({ rawYaml, range, message }: { rawYaml: string; range: Range; message: string }): string {
+export function addYamlContext({
+  source: { yamlDoc, path },
+  message,
+}: {
+  source: ConfigSource
+  message: string
+}): string {
+  if (!yamlDoc) {
+    return message
+  }
+
+  const node = yamlDoc.getIn(path, true) as ParsedNode | undefined
+  const range = node?.range
+  const rawYaml = yamlDoc.source
+
+  if (!node || !range || !rawYaml) {
+    return message
+  }
+
   // Get one line before the error location start, and the line including the error location end
   const toStart = rawYaml.slice(0, range[0])
   const lineNumber = toStart.split("\n").length + 1
@@ -256,5 +273,5 @@ function addYamlContext({ rawYaml, range, message }: { rawYaml: string; range: R
   const errorLineOffset = range[0] - errorLineStart + linePrefixLength + 2
   const marker = styles.error("-".repeat(errorLineOffset)) + styles.error.bold("^")
 
-  return `${snippet}\n${marker}\n${styles.error.bold(message)}`
+  return `${yamlDoc.filename ? `${styles.secondary(`${yamlDoc.filename}:${lineNumber - (snippetLines - 1)}`)}\n` : ""}${snippet}\n${marker}\n${styles.error.bold(message)}`
 }
diff --git a/core/src/exceptions.ts b/core/src/exceptions.ts
@@ -18,7 +18,7 @@ import dns from "node:dns"
 import { styles } from "./logger/styles.js"
 import type { ExecaError } from "execa"
 import type { Location } from "./template-string/ast.js"
-import type { ConfigSource } from "./config/validation.js"
+import { addYamlContext, type ConfigSource } from "./config/validation.js"
 
 // Unfortunately, NodeJS does not provide a list of all error codes, so we have to maintain this list manually.
 // See https://nodejs.org/docs/latest-v18.x/api/dns.html#error-codes
@@ -314,14 +314,25 @@ export class TemplateStringError extends GardenError {
   originalMessage: string
 
   constructor(params: GardenErrorParams & { loc: Location; yamlSource: ConfigSource }) {
-    const path = params.yamlSource.path
-    const pathDescription = path.length > 0 ? ` at path ${styles.accent(path.join("."))}` : ""
-    const prefix = `Invalid template string (${styles.accent(
-      truncate(params.loc.source.rawTemplateString, { length: 200 }).replace(/\n/g, "\\n")
-    )})${pathDescription}: `
-    const message = params.message.startsWith(prefix) ? params.message : prefix + params.message
-
-    super({ ...params, message })
+    let enriched: string = params.message
+    try {
+      // TODO: Use Location information from parser to point to the specific part
+      enriched = addYamlContext({ source: params.yamlSource, message: params.message })
+    } catch {
+      // ignore
+    }
+
+    if (enriched === params.message) {
+      const { path } = params.yamlSource
+
+      const pathDescription = path.length > 0 ? ` at path ${styles.accent(path.join("."))}` : ""
+      const prefix = `Invalid template string (${styles.accent(
+        truncate(params.loc.source.rawTemplateString, { length: 200 }).replace(/\n/g, "\\n")
+      )})${pathDescription}: `
+      enriched = params.message.startsWith(prefix) ? params.message : prefix + params.message
+    }
+
+    super({ ...params, message: enriched })
     this.loc = params.loc
     this.originalMessage = params.message
   }

diff --git a/core/src/plugins/kubernetes/kubernetes-type/common.ts b/core/src/plugins/kubernetes/kubernetes-type/common.ts
@@ -368,7 +368,11 @@ async function readKustomizeManifests(
       log,
       args: ["build", kustomizePath, ...extraArgs],
     })
-    const manifests = await parseKubernetesManifests(kustomizeOutput, `kustomize output of ${action.longDescription()}`)
+    const manifests = await parseKubernetesManifests(
+      kustomizeOutput,
+      `kustomize output of ${action.longDescription()}`,
+      undefined
+    )
     return manifests.map((manifest, index) => ({
       declaration: {
         type: "kustomize",
@@ -427,7 +431,8 @@ async function readFileManifests(
         const resolved = ctx.resolveTemplateStrings(str, { allowPartial: true, unescape: true })
         const manifests = await parseKubernetesManifests(
           resolved,
-          `${basename(absPath)} in directory ${dirname(absPath)} (specified in ${action.longDescription()})`
+          `${basename(absPath)} in directory ${dirname(absPath)} (specified in ${action.longDescription()})`,
+          absPath
         )
         return manifests.map((manifest, index) => ({
           declaration: {
@@ -449,10 +454,14 @@ async function readFileManifests(
  * @param str raw string containing Kubernetes manifests in YAML format
  * @param sourceDescription description of where the YAML string comes from, e.g. "foo.yaml in directory /bar"
  */
-async function parseKubernetesManifests(str: string, sourceDescription: string): Promise<KubernetesResource[]> {
+async function parseKubernetesManifests(
+  str: string,
+  sourceDescription: string,
+  filename: string | undefined
+): Promise<KubernetesResource[]> {
   // parse yaml with version 1.1 by default, as Kubernetes still uses this version.
   // See also https://github.com/kubernetes/kubernetes/issues/34146
-  const docs = await loadAndValidateYaml(str, sourceDescription, "1.1")
+  const docs = await loadAndValidateYaml({ content: str, sourceDescription, filename, version: "1.1" })
 
   // TODO: We should use schema validation to make sure that apiVersion, kind and metadata are always defined as required by the type.
   const manifests = docs.map((d) => d.toJS()) as KubernetesResource[]

diff --git a/core/test/unit/src/config/base.ts b/core/test/unit/src/config/base.ts
@@ -610,7 +610,11 @@ describe("loadAndValidateYaml", () => {
       name: foo
     `
 
-    const yamlDocs = await loadAndValidateYaml(yaml, "foo.yaml in directory bar")
+    const yamlDocs = await loadAndValidateYaml({
+      content: yaml,
+      sourceDescription: "foo.yaml in directory bar",
+      filename: "bar/foo.yaml",
+    })
 
     expect(yamlDocs).to.have.length(1)
     expect(yamlDocs[0].source).to.equal(yaml)
@@ -633,7 +637,11 @@ describe("loadAndValidateYaml", () => {
       name: doc3
     `
 
-    const yamlDocs = await loadAndValidateYaml(yaml, "foo.yaml in directory bar")
+    const yamlDocs = await loadAndValidateYaml({
+      content: yaml,
+      sourceDescription: "foo.yaml in directory bar",
+      filename: "bar/foo.yaml",
+    })
 
     expect(yamlDocs).to.have.length(3)
 
@@ -662,7 +670,11 @@ describe("loadAndValidateYaml", () => {
       newYamlOctalNumber: 0o777
     `
 
-    const yamlDocs = await loadAndValidateYaml(yaml, "foo.yaml in directory bar")
+    const yamlDocs = await loadAndValidateYaml({
+      content: yaml,
+      sourceDescription: "foo.yaml in directory bar",
+      filename: "bar/foo.yaml",
+    })
 
     expect(yamlDocs).to.have.length(1)
     expect(yamlDocs[0].source).to.equal(yaml)
@@ -684,7 +696,11 @@ describe("loadAndValidateYaml", () => {
       newYamlOctalNumber: 0o777
     `
 
-    const yamlDocs = await loadAndValidateYaml(yaml, "foo.yaml in directory bar")
+    const yamlDocs = await loadAndValidateYaml({
+      content: yaml,
+      sourceDescription: "foo.yaml in directory bar",
+      filename: "bar/foo.yaml",
+    })
 
     expect(yamlDocs).to.have.length(1)
     expect(yamlDocs[0].source).to.equal(yaml)
@@ -704,7 +720,12 @@ describe("loadAndValidateYaml", () => {
     `
 
     // we use the version parameter to force the yaml 1.1 standard
-    const yamlDocs = await loadAndValidateYaml(yaml, "foo.yaml in directory bar", "1.1")
+    const yamlDocs = await loadAndValidateYaml({
+      content: yaml,
+      sourceDescription: "foo.yaml in directory bar",
+      filename: "bar/foo.yaml",
+      version: "1.1",
+    })
 
     expect(yamlDocs).to.have.length(1)
     expect(yamlDocs[0].source).to.equal(yaml)
@@ -720,7 +741,12 @@ describe("loadAndValidateYaml", () => {
     `
 
     await expectError(
-      () => loadAndValidateYaml(yaml, "foo.yaml in directory bar"),
+      () =>
+        loadAndValidateYaml({
+          content: yaml,
+          sourceDescription: "foo.yaml in directory bar",
+          filename: "bar/foo.yaml",
+        }),
       (err) => {
         expect(err.message).to.eql(dedent`
           Could not parse foo.yaml in directory bar as valid YAML: YAMLException: unidentified alias "bar" (1:10)

diff --git a/core/test/unit/src/config/validation.ts b/core/test/unit/src/config/validation.ts
@@ -306,7 +306,11 @@ describe("validateSchema", () => {
       name: bar
     `
 
-    const yamlDocs = await loadAndValidateYaml(yaml, "foo.yaml in directory bar")
+    const yamlDocs = await loadAndValidateYaml({
+      content: yaml,
+      sourceDescription: "foo.yaml in directory bar",
+      filename: "bar/foo.yaml",
+    })
     const yamlDoc = yamlDocs[1]
 
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -324,6 +328,7 @@ describe("validateSchema", () => {
         expect(stripAnsi(err.message)).to.equal(dedent`
         Validation error:
 
+        bar/foo.yaml:10
         ...
         9   | spec:
         10  |   foo: 456
@@ -347,7 +352,8 @@ describe("validateSchema", () => {
     `
 
     const yamlDoc = parseDocument(yaml) as YamlDocumentWithSource
-    yamlDoc["source"] = yaml
+    yamlDoc.source = yaml
+    yamlDoc.filename = "foo/bar.yaml"
 
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     const config: any = {
@@ -364,6 +370,7 @@ describe("validateSchema", () => {
         expect(stripAnsi(err.message)).to.equal(dedent`
         Validation error:
 
+        foo/bar.yaml:4
         ...
         3  | spec:
         4  |   foo: 123

diff --git a/core/test/unit/src/template-string.ts b/core/test/unit/src/template-string.ts
@@ -26,6 +26,7 @@ import {
   UnresolvableValue,
   visitAll,
 } from "../../../src/template-string/static-analysis.js"
+import { loadAndValidateYaml } from "../../../src/config/base.js"
 
 describe("resolveTemplateString", () => {
   it("should return a non-templated string unchanged", () => {
@@ -368,6 +369,41 @@ describe("resolveTemplateString", () => {
     })
   })
 
+  it("if available, should include yaml context in error message", async () => {
+    const command = "${resol${part}ed}"
+    const yamlDoc = await loadAndValidateYaml({
+      content: dedent`
+      name: test,
+      kind: Build
+      spec:
+        command: '${command}'
+    `,
+      sourceDescription: "test",
+      filename: "bar/foo.yaml",
+    })
+    void expectError(
+      () =>
+        resolveTemplateString({
+          string: command,
+          context: new GenericContext({}),
+          source: {
+            yamlDoc: yamlDoc[0],
+            path: ["spec", "command"],
+          },
+        }),
+      {
+        contains: dedent`
+          bar/foo.yaml:4
+          ...
+          3  | spec:
+          4  |   command: '\${resol\${part}ed}
+          ----------------^
+          unable to parse as valid template string.
+        `,
+      }
+    )
+  })
+
   it("should handle a single-quoted string", () => {
     const res = resolveTemplateString({ string: "${'foo'}", context: new GenericContext({}) })
     expect(res).to.equal("foo")

diff --git a/plugins/pulumi/src/helpers.ts b/plugins/pulumi/src/helpers.ts
@@ -262,10 +262,11 @@ export async function applyConfig(params: PulumiParams & { previewDirPath?: stri
   let stackConfigFileExists: boolean
   try {
     const fileData = await readFile(stackConfigPath)
-    const stackConfigDocs = await loadAndValidateYaml(
-      fileData.toString(),
-      `${basename(stackConfigPath)} in directory ${dirname(stackConfigPath)}`
-    )
+    const stackConfigDocs = await loadAndValidateYaml({
+      content: fileData.toString(),
+      sourceDescription: `${basename(stackConfigPath)} in directory ${dirname(stackConfigPath)}`,
+      filename: stackConfigPath,
+    })
     stackConfig = stackConfigDocs[0].toJS()
     stackConfigFileExists = true
   } catch (err) {