Skip to content

Commit

Permalink
improvement(template): return partially resolved conditionals unchanged
Browse files Browse the repository at this point in the history 
Previously, we would collapse conditionals. This could cause issues
upon later resolution with a broader context, because the reference to
the intended key could be lost.
  • Loading branch information
@edvald @thsig
edvald authored and thsig committed Oct 8, 2020
1 parent 1e3dce0 commit d30b856
Show file tree
Hide file tree
Showing 7 changed files with 165 additions and 253 deletions.
49 changes: 10 additions & 39 deletions core/src/config/config-context.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { isString, mapValues } from "lodash"
import { PrimitiveMap, joiIdentifierMap, joiStringMap, joiPrimitive, DeepPrimitiveMap, joiVariables } from "./common"
import { Provider, GenericProviderConfig, ProviderMap } from "./provider"
import { ConfigurationError } from "../exceptions"
import { resolveTemplateString } from "../template-string"
import { resolveTemplateString, TemplateStringMissingKeyError } from "../template-string"
import { Garden } from "../garden"
import { joi } from "../config/common"
import { KeyedSet } from "../util/keyed-set"
Expand All @@ -28,8 +28,6 @@ export type ContextKey = ContextKeySegment[]
export interface ContextResolveOpts {
// Allow templates to be partially resolved (used to defer runtime template resolution, for example)
allowPartial?: boolean
// Allow undefined values to be returned without throwing an error
allowUndefined?: boolean
// a list of previously resolved paths, used to detect circular references
stack?: string[]
}
Expand Down Expand Up @@ -178,14 +176,18 @@ export abstract class ConfigContext {
}
}

if (opts.allowUndefined) {
return { resolved: undefined, message }
} else {
throw new ConfigurationError(message, {
if (opts.allowPartial) {
// If we're allowing partial strings, we throw the error immediately to end the resolution flow. The error
// is caught in the surrounding template resolution code.
throw new TemplateStringMissingKeyError(message, {
nodePath,
fullPath,
opts,
})
} else {
// Otherwise we return the undefined value, so that any logical expressions can be evaluated appropriately.
// The template resolver will throw the error later if appropriate.
return { resolved: undefined, message }
}
}

Expand Down Expand Up @@ -450,9 +452,6 @@ export class WorkflowConfigContext extends EnvironmentConfigContext {
)
public secrets: PrimitiveMap

// We ignore step references here, and keep for later resolution
public steps: Map<string, WorkflowStepContext | ErrorContext> | PassthroughContext

constructor(garden: Garden) {
super({
projectName: garden.projectName,
Expand All @@ -465,7 +464,6 @@ export class WorkflowConfigContext extends EnvironmentConfigContext {
const fullEnvName = garden.namespace ? `${garden.namespace}.${garden.environmentName}` : garden.environmentName
this.environment = new EnvironmentContext(this, garden.environmentName, fullEnvName, garden.namespace)
this.project = new ProjectContext(this, garden.projectName)
this.steps = new PassthroughContext()
}
}

Expand Down Expand Up @@ -695,34 +693,7 @@ export class TaskRuntimeContext extends ServiceRuntimeContext {
public outputs: PrimitiveMap
}

/**
* Used to defer and return the template string back, when allowPartial=true.
*/
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 })

if (res.resolved === undefined) {
if (params.opts.allowPartial) {
// If value can't be resolved and allowPartial is set, we defer the resolution by returning another template
// string, that can be resolved later.
const { key, nodePath } = params
const fullKey = nodePath.concat(key)
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).
return res
}
} else {
// Value successfully resolved
return res
}
}
}

class RuntimeConfigContext extends PassthroughContext {
class RuntimeConfigContext extends ConfigContext {
@schema(
joiIdentifierMap(ServiceRuntimeContext.getSchema())
.required()
Expand Down
2 changes: 1 addition & 1 deletion core/src/tasks/resolve-module.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export class ResolveModuleConfigTask extends BaseTask {
async resolveDependencies() {
const rawConfigs = keyBy(await this.garden.getRawModuleConfigs(), "name")

const templateRefs = await getModuleTemplateReferences(this.moduleConfig)
const templateRefs = getModuleTemplateReferences(this.moduleConfig)
const deps = templateRefs.filter((d) => d[1] !== this.moduleConfig.name)

return deps.map((d) => {
Expand Down
87 changes: 59 additions & 28 deletions core/src/template-string-parser.pegjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,21 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/

{
const {
buildBinaryExpression,
buildLogicalExpression,
getKey,
getValue,
isArray,
isPrimitive,
optionalSuffix,
missingKeyErrorType,
resolveNested,
TemplateStringError,
} = options
}

TemplateString
= a:(FormatString)+ b:TemplateString? { return [...a, ...(b || [])] }
/ a:Prefix b:(FormatString)+ c:TemplateString? { return [a, ...b, ...(c || [])] }
Expand All @@ -14,25 +29,37 @@ TemplateString

FormatString
= FormatStart e:Expression end:FormatEnd {
if (e && e._error) {
if (e && e._error && e._error.type !== missingKeyErrorType) {
return e
}

// Need to provide the optional suffix as a variable because of a parsing bug in pegjs
const allowUndefined = options.allowUndefined || end[1] === options.optionalSuffix

if (options.getValue(e) === undefined && !allowUndefined) {
const _error = new options.TemplateStringError(e.message || "Unable to resolve one or more keys.", {
text: text(),
})
return { _error }
const allowUndefined = end[1] === optionalSuffix

if (getValue(e) === undefined) {
if (allowUndefined) {
if (e && e._error) {
return { ...e, _error: undefined }
} else {
return e
}
} else if (options.allowPartial) {
return text()
} else if (e && e._error) {
return e
} else {
const _error = new TemplateStringError(e.message || "Unable to resolve one or more keys.", {
text: text(),
})
return { _error }
}
}
return e
}

InvalidFormatString
= Prefix? FormatStart .* {
throw new options.TemplateStringError("Unable to parse as valid template string.")
throw new TemplateStringError("Unable to parse as valid template string.")
}

FormatStart
Expand All @@ -54,8 +81,8 @@ MemberExpression
= head:Identifier
tail:(
"[" __ e:PrimaryExpression __ "]" {
if (e.resolved && !options.isPrimitive(e.resolved)) {
const _error = new options.TemplateStringError(
if (e.resolved && !isPrimitive(e.resolved)) {
const _error = new TemplateStringError(
`Expression in bracket must resolve to a primitive (got ${typeof e}).`,
{ text: e.resolved }
)
Expand All @@ -77,7 +104,7 @@ PrimaryExpression
}
/ v:StringLiteral {
// Allow nested template strings in literals
return options.resolveNested(v)
return resolveNested(v)
}
/ key:MemberExpression {
for (const part of key) {
Expand All @@ -86,7 +113,11 @@ PrimaryExpression
}
}
key = key.map((part) => part.resolved || part)
return options.getKey(key, { allowUndefined: true })
try {
return getKey(key, { allowPartial: options.allowPartial })
} catch (err) {
return { _error: err }
}
}
/ "(" __ e:Expression __ ")" {
return e
Expand All @@ -102,9 +133,9 @@ UnaryExpression
}

if (operator === "typeof") {
return typeof options.getValue(v)
return typeof getValue(v)
} else if (operator === "!") {
return !options.getValue(v)
return !getValue(v)
}
}

Expand All @@ -121,12 +152,12 @@ ContainsExpression
return tail
}

head = options.getValue(head)
tail = options.getValue(tail)
head = getValue(head)
tail = getValue(tail)

if (!options.isPrimitive(tail)) {
if (!isPrimitive(tail)) {
return {
_error: new options.TemplateStringError(
_error: new TemplateStringError(
`The right-hand side of a 'contains' operator must be a string, number, boolean or null (got ${typeof tail}).`
)
}
Expand All @@ -135,7 +166,7 @@ ContainsExpression
const headType = head === null ? "null" : typeof head

if (headType === "object") {
if (options.lodash.isArray(head)) {
if (isArray(head)) {
return head.includes(tail)
} else {
return head.hasOwnProperty(tail)
Expand All @@ -144,7 +175,7 @@ ContainsExpression
return head.includes(tail.toString())
} else {
return {
_error: new options.TemplateStringError(
_error: new TemplateStringError(
`The left-hand side of a 'contains' operator must be a string, array or object (got ${headType}).`
)
}
Expand All @@ -158,7 +189,7 @@ ContainsOperator
MultiplicativeExpression
= head:ContainsExpression
tail:(__ MultiplicativeOperator __ ContainsExpression)*
{ return options.buildBinaryExpression(head, tail); }
{ return buildBinaryExpression(head, tail); }

MultiplicativeOperator
= $("*" !"=")
Expand All @@ -169,7 +200,7 @@ AdditiveExpression
= head:MultiplicativeExpression
// Note: We require a whitespace around these operators to disambiguate from identifiers with dashes
tail:(WhiteSpace+ AdditiveOperator WhiteSpace+ MultiplicativeExpression)*
{ return options.buildBinaryExpression(head, tail); }
{ return buildBinaryExpression(head, tail); }

AdditiveOperator
= $("+" ![+=])
Expand All @@ -178,7 +209,7 @@ AdditiveOperator
RelationalExpression
= head:AdditiveExpression
tail:(__ RelationalOperator __ AdditiveExpression)*
{ return options.buildBinaryExpression(head, tail); }
{ return buildBinaryExpression(head, tail); }

RelationalOperator
= "<="
Expand All @@ -189,7 +220,7 @@ RelationalOperator
EqualityExpression
= head:RelationalExpression
tail:(__ EqualityOperator __ RelationalExpression)*
{ return options.buildBinaryExpression(head, tail); }
{ return buildBinaryExpression(head, tail); }

EqualityOperator
= "=="
Expand All @@ -198,15 +229,15 @@ EqualityOperator
LogicalANDExpression
= head:EqualityExpression
tail:(__ LogicalANDOperator __ EqualityExpression)*
{ return options.buildLogicalExpression(head, tail); }
{ return buildLogicalExpression(head, tail); }

LogicalANDOperator
= "&&"

LogicalORExpression
= head:LogicalANDExpression
tail:(__ LogicalOROperator __ LogicalANDExpression)*
{ return options.buildLogicalExpression(head, tail); }
{ return buildLogicalExpression(head, tail); }

LogicalOROperator
= "||"
Expand All @@ -226,7 +257,7 @@ ConditionalExpression
return alternate
}

return options.getValue(test) ? consequent : alternate
return getValue(test) ? consequent : alternate
}
/ LogicalORExpression

Expand Down
15 changes: 10 additions & 5 deletions core/src/template-string.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/

import lodash from "lodash"
import { GardenBaseError, ConfigurationError } from "./exceptions"
import {
ConfigContext,
Expand All @@ -23,10 +22,16 @@ import { isArray } from "util"

export type StringOrStringPromise = Promise<string> | string

const missingKeyErrorType = "template-string-missing-key"

class TemplateStringError extends GardenBaseError {
type = "template-string"
}

export class TemplateStringMissingKeyError extends GardenBaseError {
type = missingKeyErrorType
}

let _parser: any

function getParser() {
Expand Down Expand Up @@ -64,13 +69,13 @@ export function resolveTemplateString(string: string, context: ConfigContext, op
},
getValue,
resolveNested: (nested: string) => resolveTemplateString(nested, context, opts),
// Some utilities to pass to the parser
buildBinaryExpression,
buildLogicalExpression,
lodash,
isArray,
ConfigurationError,
TemplateStringError,
allowUndefined: opts.allowUndefined,
missingKeyErrorType,
allowPartial: !!opts.allowPartial,
optionalSuffix: "}?",
isPrimitive,
})
Expand Down Expand Up @@ -158,7 +163,7 @@ export const resolveTemplateStrings = profile(function $resolveTemplateStrings<T
*/
export function collectTemplateReferences<T extends object>(obj: T): ContextKeySegment[][] {
const context = new ScanContext()
resolveTemplateStrings(obj, context, { allowPartial: true, allowUndefined: true })
resolveTemplateStrings(obj, context, { allowPartial: true })
return uniq(context.foundKeys.entries()).sort()
}

Expand Down
Loading

0 comments on commit d30b856

Please sign in to comment.