fix(core): better Zod validation error messages (#5745)

<!--  Thanks for sending a pull request! Here are some tips for you:

1. If this is your first pull request, please read our contributor
guidelines in the
https://github.com/garden-io/garden/blob/main/CONTRIBUTING.md file.
2. Please label this pull request according to what type of issue you
are addressing (see "What type of PR is this?" below)
3. Ensure you have added or run the appropriate tests for your PR.
4. If the PR is unfinished, add `WIP:` at the beginning of the title or
use the GitHub Draft PR feature.
5. Please add at least two reviewers to the PR. Currently active
maintainers are: @edvald, @thsig, @eysi09, @shumailxyz, @stefreak,
@TimBeyer, @mkhq, and @vvagaytsev.
-->

**What this PR does / why we need it**:

Before this fix, we were simply passing through the default error
messages generated by Zod when validation failed. This resulted in
JSON-like error messages when a Zod schema validation error happened.

This was fixed by detecting Joi VS Zod validation errors, and adding a
little rendering logic for Zod errors to improve the output.

**Which issue(s) this PR fixes**:

Fixes #5446.

**Special notes for your reviewer**:

Before this fix, introducing an `exec` Test with a missing
`spec.command` would result in an error message like this:

![Screenshot 2024-02-19 at 12 06
18](https://github.com/garden-io/garden/assets/382137/ad038cdd-b624-42fe-a7e8-a66951bb5c9b)

After this fix:

![Screenshot 2024-02-19 at 12 05
33](https://github.com/garden-io/garden/assets/382137/41a5527b-70c9-4e4a-bb4f-04930bc5087b)

---------

Co-authored-by: Vladimir Vagaytsev <vladimir.vagaitsev@gmail.com>

core/package.json

@@ -27,6 +27,7 @@
   "dependencies": {
     "@codenamize/codenamize": "^1.1.1",
     "@hapi/joi": "git+https://github.com/garden-io/joi.git#master",
+    "@homebridge/node-pty-prebuilt-multiarch": "0.11.8",
     "@jsdevtools/readdir-enhanced": "^6.0.4",
     "@kubernetes/client-node": "github:garden-io/javascript#60f0ea6de09777ceabd5afc3e306801fd0b025cc",
     "@opentelemetry/api": "^1.6.0",
@@ -105,7 +106,6 @@
     "moment": "^2.30.1",
     "node-fetch": "^3.3.2",
     "node-forge": "^1.3.1",
-    "@homebridge/node-pty-prebuilt-multiarch": "0.11.8",
     "normalize-path": "^3.0.0",
     "normalize-url": "^8.0.0",
     "open": "^9.1.0",
@@ -276,4 +276,4 @@
     "fsevents": "^2.3.3"
   },
   "gitHead": "b0647221a4d2ff06952bae58000b104215aed922"
-}
+}

core/src/config/base.ts

@@ -48,6 +48,8 @@ The format of the files is determined by the configured file's extension:
 _NOTE: The default varfile format will change to YAML in Garden v0.13, since YAML allows for definition of nested objects and arrays._
 `.trim()
 
+export type ObjectPath = (string | number)[]
+
 export interface YamlDocumentWithSource extends Document {
   source: string
 }

core/src/config/common.ts

@@ -35,6 +35,7 @@ import {
   variableNameRegex,
   envVarRegex,
 } from "./constants.js"
+import { renderZodError } from "./zod.js"
 
 // Avoid chasing moved references
 export * from "./constants.js"
@@ -465,9 +466,8 @@ joi = joi.extend({
           if (!(error instanceof z.ZodError)) {
             throw error
           }
-          // TODO: customize the error output here to make it a bit nicer
-          const outputError = helpers.error("validation")
-          outputError.message = error.message
+          const outputError = helpers.error("zodValidation")
+          outputError.message = renderZodError(error)
           outputError.zodError = error
 
           if (error instanceof z.ZodError && error.issues.length > 0) {

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 { profile } from "../util/profiling.js"
-import type { BaseGardenResource, YamlDocumentWithSource } from "./base.js"
+import type { BaseGardenResource, ObjectPath, YamlDocumentWithSource } from "./base.js"
 import type { ParsedNode, Range } from "yaml"
 import { padEnd } from "lodash-es"
 import { styles } from "../logger/styles.js"
@@ -29,6 +29,7 @@ const joiOptions: Joi.ValidationOptions = {
   // Overriding some error messages to make them friendlier
   messages: {
     "any.unknown": `{{#label}} is not allowed at path ${joiPathPlaceholder}`,
+    "any.required": `{{#label}} is required at path ${joiPathPlaceholder}`,
     "object.missing": `object at ${joiPathPlaceholder} must contain at least one of {{#peersWithLabels}}`,
     "object.nand": `{{#mainWithLabel}} can\'t be specified simultaneously with {{#peersWithLabels}}`,
     "object.unknown": `key "{{#child}}" is not allowed at path ${joiPathPlaceholder}`,
@@ -41,7 +42,7 @@ const joiOptions: Joi.ValidationOptions = {
 
 export interface ConfigSource {
   yamlDoc?: YamlDocumentWithSource
-  basePath?: (string | number)[]
+  basePath?: ObjectPath
 }
 
 export interface ValidateOptions {
@@ -97,7 +98,7 @@ export interface ValidateConfigParams<T extends BaseGardenResource> {
   config: T
   schema: Joi.Schema
   projectRoot: string
-  yamlDocBasePath: (string | number)[]
+  yamlDocBasePath: ObjectPath
   ErrorClass?: typeof ConfigurationError
 }
 
@@ -130,45 +131,15 @@ export const validateSchema = profile(function $validateSchema<T>(
     return result.value
   }
 
-  const description = schema.describe()
-
   const yamlDoc = source?.yamlDoc
   const rawYaml = yamlDoc?.source
   const yamlBasePath = source?.basePath || []
 
   const errorDetails = error.details.map((e) => {
-    // render the key path in a much nicer way
-    let renderedPath = yamlBasePath.join(".")
-
-    if (e.path.length) {
-      let d = description
-
-      for (const part of e.path) {
-        if (d.keys && d.keys[part]) {
-          renderedPath = renderedPath ? renderedPath + "." + part : part.toString()
-          d = d.keys[part]
-        } else if (d.patterns) {
-          for (const p of d.patterns) {
-            if (part.toString().match(new RegExp(p.regex.slice(1, -1)))) {
-              renderedPath += `[${part}]`
-              d = p.rule
-              break
-            }
-          }
-        } else {
-          renderedPath += `[${part}]`
-        }
-      }
-    }
-
-    // a little hack to always use full key paths instead of just the label
-    e.message = e.message.replace(
-      joiLabelPlaceholderRegex,
-      renderedPath ? styles.bold.underline(renderedPath) : "value"
-    )
-    e.message = e.message.replace(joiPathPlaceholderRegex, styles.bold.underline(renderedPath || "."))
-    // FIXME: remove once we've customized the error output from AJV in customObject.jsonSchema()
-    e.message = e.message.replace(/should NOT have/g, "should not have")
+    e.message =
+      e.type === "zodValidation"
+        ? improveZodValidationErrorMessage(e, yamlBasePath)
+        : improveJoiValidationErrorMessage(e, schema, yamlBasePath)
 
     const node = yamlDoc?.getIn([...yamlBasePath, ...e.path], true) as ParsedNode | undefined
     const range = node?.range
@@ -195,7 +166,7 @@ export const validateSchema = profile(function $validateSchema<T>(
   }
 
   throw new ErrorClass({
-    message: `${msgPrefix}:\n${errorDescription}`,
+    message: `${msgPrefix}:\n\n${errorDescription}`,
   })
 })
 
@@ -204,6 +175,50 @@ export interface ArtifactSpec {
   target: string
 }
 
+function improveJoiValidationErrorMessage(item: Joi.ValidationErrorItem, schema: Joi.Schema, yamlBasePath: ObjectPath) {
+  // render the key path in a much nicer way
+  const description = schema.describe()
+  let renderedPath = yamlBasePath.join(".")
+  let msg = item.message
+  if (item.path.length) {
+    let d = description
+
+    for (const part of item.path) {
+      if (d.keys && d.keys[part]) {
+        renderedPath = renderedPath ? renderedPath + "." + part : part.toString()
+        d = d.keys[part]
+      } else if (d.patterns) {
+        for (const p of d.patterns) {
+          if (part.toString().match(new RegExp(p.regex.slice(1, -1)))) {
+            renderedPath += `[${part}]`
+            d = p.rule
+            break
+          }
+        }
+      } else {
+        renderedPath += `[${part}]`
+      }
+    }
+  }
+
+  // a little hack to always use full key paths instead of just the label
+  msg = msg.replace(joiLabelPlaceholderRegex, renderedPath ? styles.bold.underline(renderedPath) : "value")
+  msg = msg.replace(joiPathPlaceholderRegex, styles.bold.underline(renderedPath || "."))
+  // FIXME: remove once we've customized the error output from AJV in customObject.jsonSchema()
+  msg = msg.replace(/should NOT have/g, "should not have")
+
+  return msg
+}
+
+function improveZodValidationErrorMessage(item: Joi.ValidationErrorItem, yamlBasePath: ObjectPath) {
+  const path = [...yamlBasePath, ...item.path].join(".")
+  if (path.length > 0) {
+    return `At path ${styles.primary(path)}: ${item.message}`
+  } else {
+    return item.message
+  }
+}
+
 function addYamlContext({ rawYaml, range, message }: { rawYaml: string; range: Range; message: string }): string {
   // Get one line before the error location start, and the line including the error location end
   const toStart = rawYaml.slice(0, range[0])
@@ -242,5 +257,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 `\n${snippet}\n${marker}\n${styles.error.bold(message)}`
+  return `${snippet}\n${marker}\n${styles.error.bold(message)}`
 }

core/src/config/zod.ts

@@ -75,6 +75,24 @@ type GardenSchema = typeof z & {
   ) => z.ZodEffects<z.ZodArray<T, "many">, T["_output"][], T["_input"][]>
 }
 
+/**
+ * Improve on the default error messages generated by Zod (extend this function as needed).
+ *
+ * Note: The config path is added to the error message later in the flow (see the `validateSchema` function),
+ * where we prepend it with the YAML path if available.
+ */
+export function renderZodError(error: z.ZodError): string {
+  return error.issues
+    .map((i: Zod.ZodIssue) => {
+      if (i.message === "Required" && i["expected"] && i["received"]) {
+        return `Expected ${i["expected"]}, but received ${i["received"]}`
+      } else {
+        return i.message
+      }
+    })
+    .join("\n")
+}
+
 // This should be imported instead of z because we augment zod with custom methods
 const gardenZod = Object.assign(
   {

core/src/exceptions.ts

@@ -16,6 +16,7 @@ import indentString from "indent-string"
 import { constants } from "os"
 import dns from "node:dns"
 import { styles } from "./logger/styles.js"
+import type { ObjectPath } from "./config/base.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
@@ -303,9 +304,9 @@ export class CloudApiError extends GardenError {
 export class TemplateStringError extends GardenError {
   type = "template-string"
 
-  path?: (string | number)[]
+  path?: ObjectPath
 
-  constructor(params: GardenErrorParams & { path?: (string | number)[] }) {
+  constructor(params: GardenErrorParams & { path?: ObjectPath }) {
     super(params)
     this.path = params.path
   }

core/src/tasks/resolve-provider.ts

@@ -34,6 +34,7 @@ import { OtelTraced } from "../util/open-telemetry/decorators.js"
 import { LogLevel } from "../logger/logger.js"
 import type { Log } from "../logger/log-entry.js"
 import { styles } from "../logger/styles.js"
+import type { ObjectPath } from "../config/base.js"
 
 /**
  * Returns a provider log context with the provider name set.
@@ -189,7 +190,7 @@ export class ResolveProviderTask extends BaseTask<Provider> {
 
     const projectConfig = this.garden.getProjectConfig()
     const yamlDoc = projectConfig.internal.yamlDoc
-    let yamlDocBasePath: (string | number)[] = []
+    let yamlDocBasePath: ObjectPath = []
 
     if (yamlDoc) {
       projectConfig.providers.forEach((p, i) => {

core/src/template-string/template-string.ts

@@ -41,6 +41,7 @@ import { deepMap } from "../util/objects.js"
 import type { ConfigSource } from "../config/validation.js"
 import * as parser from "./parser.js"
 import { styles } from "../logger/styles.js"
+import type { ObjectPath } from "../config/base.js"
 
 const missingKeyExceptionType = "template-string-missing-key"
 const passthroughExceptionType = "template-string-passthrough"
@@ -70,8 +71,6 @@ function getValue(v: Primitive | undefined | ResolvedClause) {
   return isPlainObject(v) ? (<ResolvedClause>v).resolved : v
 }
 
-type ObjectPath = (string | number)[]
-
 export class TemplateError extends GardenError {
   type = "template"
 

core/test/unit/src/config/validation.ts

@@ -268,7 +268,6 @@ describe("validateSchema", () => {
         1  | apiVersion: 123
         -----------------^
         apiVersion must be a string
-
         ...
         3  | spec:
         4  |   foo: 123

core/test/unit/src/garden.ts

@@ -4190,7 +4190,7 @@ describe("Garden", () => {
       await expectError(() => garden.resolveModules({ log: garden.log }), {
         contains: [
           "Failed resolving one or more modules:",
-          "foo: Error validating outputs for module 'foo':\nfoo must be a string",
+          "foo: Error validating outputs for module 'foo':\n\nfoo must be a string",
         ],
       })
     })
@@ -4438,7 +4438,7 @@ describe("Garden", () => {
       await expectError(() => garden.resolveModules({ log: garden.log }), {
         contains: [
           "Failed resolving one or more modules:",
-          "foo: Error validating configuration for module 'foo' (base schema from 'base' plugin):\nbase is required",
+          "foo: Error validating configuration for module 'foo' (base schema from 'base' plugin):\n\nbase is required",
         ],
       })
     })
@@ -4503,7 +4503,7 @@ describe("Garden", () => {
       await expectError(() => garden.resolveModules({ log: garden.log }), {
         contains: [
           "Failed resolving one or more modules:",
-          "foo: Error validating outputs for module 'foo' (base schema from 'base' plugin):\nfoo must be a string",
+          "foo: Error validating outputs for module 'foo' (base schema from 'base' plugin):\n\nfoo must be a string",
         ],
       })
     })
@@ -4588,7 +4588,7 @@ describe("Garden", () => {
         await expectError(() => garden.resolveModules({ log: garden.log }), {
           contains: [
             "Failed resolving one or more modules:",
-            "foo: Error validating configuration for module 'foo' (base schema from 'base-a' plugin):\nbase is required",
+            "foo: Error validating configuration for module 'foo' (base schema from 'base-a' plugin):\n\nbase is required",
           ],
         })
       })
@@ -4666,7 +4666,7 @@ describe("Garden", () => {
         await expectError(() => garden.resolveModules({ log: garden.log }), {
           contains: [
             "Failed resolving one or more modules:",
-            "foo: Error validating outputs for module 'foo' (base schema from 'base-a' plugin):\nfoo must be a string",
+            "foo: Error validating outputs for module 'foo' (base schema from 'base-a' plugin):\n\nfoo must be a string",
           ],
         })
       })

core/test/unit/src/template-string.ts

@@ -1179,7 +1179,7 @@ describe("resolveTemplateString", () => {
         () => resolveTemplateString({ string: "${base64Decode(a)}", context: new TestContext({ a: 1234 }) }),
         {
           contains:
-            "Invalid template string (${base64Decode(a)}): Error validating argument 'string' for base64Decode helper function:\nvalue must be a string",
+            "Invalid template string (${base64Decode(a)}): Error validating argument 'string' for base64Decode helper function:\n\nvalue must be a string",
         }
       )
     })
@@ -1243,7 +1243,7 @@ describe("resolveTemplateString", () => {
           )
         }
 
-        it("using on incompatible argument types (string and object)", () => {
+        it("using an incompatible argument types (string and object)", () => {
           return expectArgTypeError({
             template: "${concat(a, b)}",
             testContextVars: {
@@ -1255,15 +1255,15 @@ describe("resolveTemplateString", () => {
           })
         })
 
-        it("using on unsupported argument types (number and object)", () => {
+        it("using an unsupported argument types (number and object)", () => {
           return expectArgTypeError({
             template: "${concat(a, b)}",
             testContextVars: {
               a: 123,
               b: ["a"],
             },
             errorMessage:
-              "Error validating argument 'arg1' for concat helper function:\nvalue must be one of [array, string]",
+              "Error validating argument 'arg1' for concat helper function:\n\nvalue must be one of [array, string]",
           })
         })
       })