Skip to content

Commit

Permalink
fix(config): incorrect handling of bracketed template keys with dots
Browse files Browse the repository at this point in the history
Partial resolution was mangling key segments with dots in them, as well
as nested template strings.
edvald authored and thsig committed Aug 12, 2020

Verified

This commit was signed with the committer’s verified signature.
1 parent 85ae9ee commit f1cdfee
Showing 6 changed files with 141 additions and 24 deletions.
53 changes: 42 additions & 11 deletions garden-service/src/config/config-context.ts
Original file line number Diff line number Diff line change
@@ -24,7 +24,8 @@ import { ModuleConfig } from "./module"
import { ModuleVersion } from "../vcs/vcs"
import { isPrimitive } from "util"

export type ContextKey = string[]
export type ContextKeySegment = string | number
export type ContextKey = ContextKeySegment[]

export interface ContextResolveOpts {
// Allow templates to be partially resolved (used to defer runtime template resolution, for example)
@@ -72,8 +73,8 @@ export abstract class ConfigContext {
}

resolve({ key, nodePath, opts }: ContextResolveParams): ContextResolveOutput {
const path = key.join(".")
const fullPath = nodePath.concat(key).join(".")
const path = renderKeyPath(key)
const fullPath = renderKeyPath(nodePath.concat(key))

// if the key has previously been resolved, return it directly
const resolved = this._resolvedValues[path]
@@ -100,7 +101,7 @@ export abstract class ConfigContext {
let value: any = this
let partial = false
let nextKey = key[0]
let lookupPath: string[] = []
let lookupPath: ContextKeySegment[] = []
let nestedNodePath = nodePath
let message: string | undefined = undefined

@@ -109,7 +110,7 @@ export abstract class ConfigContext {
lookupPath = key.slice(0, p + 1)
const remainder = key.slice(p + 1)
nestedNodePath = nodePath.concat(lookupPath)
const stackEntry = nestedNodePath.join(".")
const stackEntry = renderKeyPath(nestedNodePath)
available = null

if (typeof nextKey === "string" && nextKey.startsWith("_")) {
@@ -173,7 +174,7 @@ export abstract class ConfigContext {
if (message === undefined) {
message = chalk.red(`Could not find key ${chalk.white(nextKey)}`)
if (nestedNodePath.length > 1) {
message += chalk.red(" under ") + chalk.white(nestedNodePath.slice(0, -1).join("."))
message += chalk.red(" under ") + chalk.white(renderKeyPath(nestedNodePath.slice(0, -1)))
}
message += chalk.red(".")

@@ -203,17 +204,17 @@ export abstract class ConfigContext {
}

export class ScanContext extends ConfigContext {
foundKeys: KeyedSet<string[]>
foundKeys: KeyedSet<ContextKeySegment[]>

constructor() {
super()
this.foundKeys = new KeyedSet<string[]>((v) => v.join("."))
this.foundKeys = new KeyedSet<ContextKeySegment[]>((v) => renderKeyPath(v))
}

resolve({ key, nodePath }: ContextResolveParams) {
const fullKey = nodePath.concat(key)
this.foundKeys.add(fullKey)
return { resolved: "${" + fullKey.join(".") + "}" }
return { resolved: renderTemplateString(fullKey), partial: true }
}
}

@@ -671,7 +672,7 @@ export class TaskRuntimeContext extends ServiceRuntimeContext {
/**
* Used to defer and return the template string back, when allowPartial=true.
*/
class PassthroughContext extends ConfigContext {
export class PassthroughContext extends ConfigContext {
resolve(params: ContextResolveParams): ContextResolveOutput {
const opts = { ...(params.opts || {}), allowUndefined: params.opts.allowPartial || params.opts.allowUndefined }
const res = super.resolve({ ...params, opts })
@@ -682,7 +683,7 @@ class PassthroughContext extends ConfigContext {
// string, that can be resolved later.
const { key, nodePath } = params
const fullKey = nodePath.concat(key)
return { resolved: "${" + fullKey.join(".") + "}", partial: true }
return { resolved: renderTemplateString(fullKey), partial: true }
} else {
// If undefined values are allowed, we simply return undefined (We know allowUndefined is set here, because
// otherwise an error would have been thrown by `super.resolve()` above).
@@ -823,3 +824,33 @@ export class OutputConfigContext extends ModuleConfigContext {
})
}
}

/**
* Given all the segments of a template string, return a new template string that can be resolved later.
*/
function renderTemplateString(key: ContextKeySegment[]) {
return "${" + renderKeyPath(key) + "}"
}

/**
* Given all the segments of a template string, return a string path for the key.
*/
function renderKeyPath(key: ContextKeySegment[]): string {
// Note: We don't support bracket notation for the first part in a template string
if (key.length === 0) {
return ""
}
const stringSegments = key.map((segment) => "" + segment)
return (
stringSegments
.slice(1)
// Need to correctly handle key segments with dots in them, and nested templates
.reduce((output, segment) => {
if (segment.match(/[\.\$\{\}]/)) {
return `${output}[${JSON.stringify(segment)}]`
} else {
return `${output}.${segment}`
}
}, stringSegments[0])
)
}
2 changes: 1 addition & 1 deletion garden-service/src/config/provider.ts
Original file line number Diff line number Diff line change
@@ -127,7 +127,7 @@ export async function getProviderTemplateReferences(config: ProviderConfig) {

for (const key of references) {
if (key[0] === "providers") {
const providerName = key[1]
const providerName = key[1] as string
if (!providerName) {
throw new ConfigurationError(
deline`
8 changes: 4 additions & 4 deletions garden-service/src/outputs.ts
Original file line number Diff line number Diff line change
@@ -44,14 +44,14 @@ export async function resolveProjectOutputs(garden: Garden, log: LogEntry): Prom
continue
}
if (ref[0] === "providers") {
needProviders.push(ref[1])
needProviders.push(ref[1] as string)
} else if (ref[0] === "modules") {
needModules.push(ref[1])
needModules.push(ref[1] as string)
} else if (ref[0] === "runtime" && ref[2]) {
if (ref[1] === "services") {
needServices.push(ref[2])
needServices.push(ref[2] as string)
} else if (ref[1] === "tasks") {
needTasks.push(ref[2])
needTasks.push(ref[2] as string)
}
}
}
14 changes: 10 additions & 4 deletions garden-service/src/template-string.ts
Original file line number Diff line number Diff line change
@@ -9,7 +9,13 @@
import lodash from "lodash"
import { deepMap } from "./util/util"
import { GardenBaseError, ConfigurationError } from "./exceptions"
import { ConfigContext, ContextResolveOpts, ScanContext, ContextResolveOutput } from "./config/config-context"
import {
ConfigContext,
ContextResolveOpts,
ScanContext,
ContextResolveOutput,
ContextKeySegment,
} from "./config/config-context"
import { difference, flatten, uniq, isPlainObject, isNumber } from "lodash"
import { Primitive, StringMap, isPrimitive } from "./config/common"
import { profile } from "./util/profiling"
@@ -117,7 +123,7 @@ export const resolveTemplateStrings = profile(function $resolveTemplateStrings<T
/**
* Scans for all template strings in the given object and lists the referenced keys.
*/
export function collectTemplateReferences<T extends object>(obj: T): string[][] {
export function collectTemplateReferences<T extends object>(obj: T): ContextKeySegment[][] {
const context = new ScanContext()
resolveTemplateStrings(obj, context)
return uniq(context.foundKeys.entries()).sort()
@@ -144,7 +150,7 @@ export function throwOnMissingSecretKeys<T extends Object>(
secrets: StringMap,
prefix: string
) {
const allMissing: [string, string[]][] = [] // [[key, missing keys]]
const allMissing: [string, ContextKeySegment[]][] = [] // [[key, missing keys]]
for (const [key, config] of Object.entries(configs)) {
const missing = detectMissingSecretKeys(config, secrets)
if (missing.length > 0) {
@@ -190,7 +196,7 @@ export function throwOnMissingSecretKeys<T extends Object>(
* Collects template references to secrets in obj, and returns an array of any secret keys referenced in it that
* aren't present (or have blank values) in the provided secrets map.
*/
export function detectMissingSecretKeys<T extends object>(obj: T, secrets: StringMap): string[] {
export function detectMissingSecretKeys<T extends object>(obj: T, secrets: StringMap): ContextKeySegment[] {
const referencedKeys = collectTemplateReferences(obj)
.filter((ref) => ref[0] === "secrets")
.map((ref) => ref[1])
78 changes: 74 additions & 4 deletions garden-service/test/unit/src/config/config-context.ts
Original file line number Diff line number Diff line change
@@ -17,14 +17,16 @@ import {
ProviderConfigContext,
WorkflowConfigContext,
WorkflowStepConfigContext,
ScanContext,
PassthroughContext,
} from "../../../../src/config/config-context"
import { expectError, makeTestGardenA, TestGarden, projectRootA, makeTestGarden } from "../../../helpers"
import { join } from "path"
import { joi } from "../../../../src/config/common"
import { prepareRuntimeContext } from "../../../../src/runtime-context"
import { Service } from "../../../../src/types/service"
import stripAnsi = require("strip-ansi")
import { resolveTemplateString } from "../../../../src/template-string"
import { resolveTemplateString, resolveTemplateStrings } from "../../../../src/template-string"
import { fromPairs, keyBy } from "lodash"

type TestValue = string | ConfigContext | TestValues | TestValueFunction
@@ -129,7 +131,7 @@ describe("ConfigContext", () => {
const c = new TestContext({
nested: new NestedContext(),
})
await expectError(() => resolveKey(c, ["nested", "bla"]), "configuration")
await expectError(async () => resolveKey(c, ["nested", "bla"]), "configuration")
})

it("should show helpful error when unable to resolve nested key in map", async () => {
@@ -434,8 +436,10 @@ describe("ModuleConfigContext", () => {

context("runtimeContext is not set", () => {
it("should return runtime template strings unchanged if allowPartial=true", async () => {
expect(c.resolve({ key: ["runtime", "some", "key"], nodePath: [], opts: { allowPartial: true } })).to.eql({
resolved: "${runtime.some.key}",
expect(
c.resolve({ key: ["runtime", "some", "key.with.dots"], nodePath: [], opts: { allowPartial: true } })
).to.eql({
resolved: '${runtime.some["key.with.dots"]}',
})
})

@@ -716,3 +720,69 @@ describe("WorkflowStepConfigContext", () => {
)
})
})

describe("ScanContext", () => {
it("should collect found keys in an object", () => {
const context = new ScanContext()
const obj = {
a: "some ${templated.string}",
b: "${more.stuff}",
}
resolveTemplateStrings(obj, context)
expect(context.foundKeys.entries()).to.eql([
["templated", "string"],
["more", "stuff"],
])
})

it("should handle keys with dots correctly", () => {
const context = new ScanContext()
const obj = {
a: "some ${templated['key.with.dots']}",
b: "${more.stuff}",
}
resolveTemplateStrings(obj, context)
expect(context.foundKeys.entries()).to.eql([
["templated", "key.with.dots"],
["more", "stuff"],
])
})
})

describe("PassthroughContext", () => {
it("should return the template key back unresolved when allowPartial=true", () => {
const context = new PassthroughContext()
const obj = {
a: "some ${templated.string}",
b: "${more.stuff}",
}
const result = resolveTemplateStrings(obj, context, { allowPartial: true })
expect(result).to.eql(obj)
})

it("should handle keys with dots correctly", () => {
const context = new PassthroughContext()
const obj = {
a: "some ${templated['key.with.dots']}",
b: "${more.stuff}",
}
const result = resolveTemplateStrings(obj, context, { allowPartial: true })
expect(result).to.eql({
a: 'some ${templated["key.with.dots"]}',
b: "${more.stuff}",
})
})

it("should handle nested template keys correctly", () => {
const context = new PassthroughContext()
const obj = {
a: "some ${templated['${nested.key}']}",
b: "${more.stuff}",
}
const result = resolveTemplateStrings(obj, context, { allowPartial: true })
expect(result).to.eql({
a: 'some ${templated["${nested.key}"]}',
b: "${more.stuff}",
})
})
})
10 changes: 10 additions & 0 deletions garden-service/test/unit/src/template-string.ts
Original file line number Diff line number Diff line change
@@ -450,6 +450,16 @@ describe("resolveTemplateString", async () => {
expect(res).to.equal(true)
})

it("should handle member lookup with bracket notation, single quotes and dot in key name", async () => {
const res = resolveTemplateString("${foo['bar.baz']}", new TestContext({ foo: { "bar.baz": true } }))
expect(res).to.equal(true)
})

it("should handle member lookup with bracket notation, double quotes and dot in key name", async () => {
const res = resolveTemplateString('${foo.bar["bla.ble"]}', new TestContext({ foo: { bar: { "bla.ble": 123 } } }))
expect(res).to.equal(123)
})

it("should handle numeric member lookup with bracket notation", async () => {
const res = resolveTemplateString("${foo[1]}", new TestContext({ foo: [false, true] }))
expect(res).to.equal(true)

0 comments on commit f1cdfee

Please sign in to comment.