Skip to content

Commit

Permalink
fix(core): improve project config validation
Browse files Browse the repository at this point in the history
We now validate that `project.environments` is an array and throw an
informative error if it isn't.

Previously, if `project.environments` was mistakenly formatted as a
map instead of an array, a cryptic type error would be printed ("Cannot
read property name of undefined"). We now clearly indicate the problem
to the user.

Additionally, the `joiSparseArray` schema can now properly handle
non-array inputs without failing cryptically.
  • Loading branch information
thsig committed Nov 17, 2021
1 parent 3fdcffb commit c08747e
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 11 deletions.
4 changes: 2 additions & 2 deletions core/src/config/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import Joi from "@hapi/joi"
import Ajv from "ajv"
import { splitLast } from "../util/util"
import { deline, dedent } from "../util/string"
import { cloneDeep } from "lodash"
import { cloneDeep, isArray } from "lodash"
import { joiPathPlaceholder } from "./validation"
import { DEFAULT_API_VERSION } from "../constants"

Expand Down Expand Up @@ -415,7 +415,7 @@ joi = joi.extend({
type: "sparseArray",
coerce: {
method(value) {
return { value: value && value.filter((v: any) => v !== undefined && v !== null) }
return { value: isArray(value) && value.filter((v: any) => v !== undefined && v !== null) }
},
},
})
Expand Down
8 changes: 1 addition & 7 deletions core/src/config/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,7 @@ export const environmentSchema = () =>
})

export const environmentsSchema = () =>
joi
.alternatives(
joiSparseArray(environmentSchema()).unique("name"),
// Allow a string as a shorthand for { name: foo }
joiSparseArray(joiUserIdentifier())
)
.description("A list of environments to configure for the project.")
joiSparseArray(environmentSchema()).unique("name").description("A list of environments to configure for the project.")

export interface SourceConfig {
name: string
Expand Down
15 changes: 13 additions & 2 deletions core/src/garden.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import { TaskGraph, GraphResults, ProcessTasksOpts } from "./task-graph"
import { getLogger } from "./logger/logger"
import { PluginActionHandlers, GardenPlugin } from "./types/plugin/plugin"
import { loadConfigResources, findProjectConfig, prepareModuleResource, GardenResource } from "./config/base"
import { DeepPrimitiveMap, StringMap, PrimitiveMap } from "./config/common"
import { DeepPrimitiveMap, StringMap, PrimitiveMap, joiSparseArray, joi } from "./config/common"
import { BaseTask } from "./tasks/base"
import { LocalConfigStore, ConfigStore, GlobalConfigStore, LinkedSource } from "./config-store"
import { getLinkedSources, ExternalSourceType } from "./util/ext-source-util"
Expand Down Expand Up @@ -99,7 +99,7 @@ import { ProviderConfigContext } from "./config/template-contexts/provider"
import { getSecrets } from "./enterprise/get-secrets"
import { killSyncDaemon } from "./plugins/kubernetes/mutagen"
import { ConfigContext } from "./config/template-contexts/base"
import { validateSchema } from "./config/validation"
import { validateSchema, validateWithPath } from "./config/validation"

export interface ActionHandlerMap<T extends keyof PluginActionHandlers> {
[actionName: string]: PluginActionHandlers[T]
Expand Down Expand Up @@ -1232,6 +1232,17 @@ export async function resolveGardenParams(currentDirectory: string, opts: Garden
const gitHandler = new GitHandler(projectRoot, gardenDirPath, [], treeCache)
const vcsBranch = (await gitHandler.getBranchName(log, projectRoot)) || ""

// Since we iterate/traverse them before fully validating them (which we do after resolving template strings), we
// validdate that `config.environments` and `config.providers` are both arrays.
// This prevents cryptic type errors when the user mistakely writes down e.g. a map instead of an array.
validateWithPath({
config: config.environments,
schema: joiSparseArray(joi.object()),
configType: "environments",
path: config.path,
projectRoot: config.path,
})

const defaultEnvironmentName = resolveTemplateString(
config.defaultEnvironment,
new DefaultEnvironmentContext({
Expand Down
6 changes: 6 additions & 0 deletions core/test/data/test-project-malformed-environments/garden.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Project
name: test-project-malformed-environments
environments:
name: local # This should result in an exception, because `environments` is a map instead of a array.
providers:
- name: test-plugin
8 changes: 8 additions & 0 deletions core/test/unit/src/garden.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,14 @@ describe("Garden", () => {
await expectError(async () => TestGarden.factory(projectRootA, { environmentName: "garden-bla" }), "parameter")
})

it("should throw if project.environments is not an array", async () => {
const projectRoot = join(dataDir, "test-project-malformed-environments")
await expectError(
async () => TestGarden.factory(projectRoot),
(err) => expect(err.message).to.match(/Error validating environments: value must be an array/)
)
})

it("should set .garden as the default cache dir", async () => {
const projectRoot = join(dataDir, "test-project-empty")
const garden = await TestGarden.factory(projectRoot, { plugins: [testPlugin] })
Expand Down

0 comments on commit c08747e

Please sign in to comment.