Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List Params support in CEL #5137

Merged
merged 25 commits into from Dec 19, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
a1a2983
first commit: param lists
Berlioz Oct 17, 2022
ace8a2b
love too have initial trivial test case be broken
Berlioz Oct 17, 2022
7c2dbd2
maybe fix it so it actually works
Berlioz Oct 18, 2022
faa1eeb
handle list comparisons
Berlioz Oct 19, 2022
42e6334
Merge branch 'master' into list_params
Berlioz Oct 20, 2022
af42549
actually support a real list param
Berlioz Oct 20, 2022
5bd2940
um, placeholder input methods?
Berlioz Oct 26, 2022
d628d9f
.env lists as comma seperated strings instead of serialized objects
Berlioz Nov 1, 2022
1c6df4e
-what if the previous commit but actually working
Berlioz Nov 1, 2022
cf66cd0
smaller style fixes
Berlioz Nov 3, 2022
0fba2ef
Merge branch 'master' into list_params
Berlioz Nov 3, 2022
5cf4480
some work on input methods
Berlioz Nov 8, 2022
54dab9b
Merge remote-tracking branch 'origin' into list_params
Berlioz Nov 8, 2022
37ba78f
Merge remote-tracking branch 'origin' into list_params
Berlioz Nov 16, 2022
8782c04
assorted changes per reviews
Berlioz Nov 16, 2022
bd77603
eslint fix
Berlioz Nov 16, 2022
655a546
Merge remote-tracking branch 'origin' into list_params
Berlioz Dec 1, 2022
7807eb4
Merge remote-tracking branch 'origin' into list_params
Berlioz Dec 19, 2022
afcf6f3
Update src/deploy/functions/cel.ts
Berlioz Dec 19, 2022
b2de4f6
Update src/deploy/functions/params.ts
Berlioz Dec 19, 2022
c8aa2ef
Update src/deploy/functions/params.ts
Berlioz Dec 19, 2022
8cc6f89
Update src/deploy/functions/params.ts
Berlioz Dec 19, 2022
b54d489
Update src/deploy/functions/params.ts
Berlioz Dec 19, 2022
eb54228
more error messages filled in
Berlioz Dec 19, 2022
65c5635
eslint fix
Berlioz Dec 19, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 18 additions & 4 deletions src/deploy/functions/build.ts
Expand Up @@ -52,8 +52,9 @@ export interface RequiredApi {
// expressions.
// `Expression<Foo> == Expression<Foo>` is an Expression<boolean>
// `Expression<boolean> ? Expression<T> : Expression<T>` is an Expression<T>
export type Expression<T extends string | number | boolean> = string; // eslint-disable-line
export type Expression<T extends string | number | boolean | string[]> = string; // eslint-disable-line
export type Field<T extends string | number | boolean> = T | Expression<T> | null;
export type ListField = Expression<string[]> | (string | Expression<string>)[] | null;

// A service account must either:
// 1. Be a project-relative email that ends with "@" (e.g. database-users@)
Expand Down Expand Up @@ -234,7 +235,7 @@ export type Endpoint = Triggered & {

// defaults to ["us-central1"], overridable in firebase-tools with
// process.env.FIREBASE_FUNCTIONS_DEFAULT_REGION
region?: string[];
region?: ListField;

// The Cloud project associated with this endpoint.
project: string;
Expand Down Expand Up @@ -314,6 +315,7 @@ function envWithTypes(
string: true,
boolean: true,
number: true,
list: true,
};
for (const param of definedParams) {
if (param.name === envName) {
Expand All @@ -322,18 +324,28 @@ function envWithTypes(
string: true,
boolean: false,
number: false,
list: false,
};
} else if (param.type === "int") {
providedType = {
string: false,
boolean: false,
number: true,
list: false,
};
} else if (param.type === "boolean") {
providedType = {
string: false,
boolean: true,
number: false,
list: false,
};
} else if (param.type === "list") {
providedType = {
string: false,
boolean: false,
number: false,
list: true,
};
}
}
Expand Down Expand Up @@ -414,9 +426,11 @@ export function toBackend(
for (const endpointId of Object.keys(build.endpoints)) {
const bdEndpoint = build.endpoints[endpointId];

let regions = bdEndpoint.region;
if (typeof regions === "undefined") {
let regions: string[] = [];
if (!bdEndpoint.region) {
regions = [api.functionsDefaultRegion];
} else {
regions = params.resolveList(bdEndpoint.region, paramValues);
}
for (const region of regions) {
const trigger = discoverTrigger(bdEndpoint, region, r);
Expand Down
140 changes: 124 additions & 16 deletions src/deploy/functions/cel.ts
Expand Up @@ -10,8 +10,8 @@ type TernaryExpression = CelExpression;
type LiteralTernaryExpression = CelExpression;
type DualTernaryExpression = CelExpression;

type Literal = string | number | boolean;
type L = "string" | "number" | "boolean";
type Literal = string | number | boolean | string[];
type L = "string" | "number" | "boolean" | "string[]";

const paramRegexp = /params\.(\S+)/;
const CMP = /((?:!=)|(?:==)|(?:>=)|(?:<=)|>|<)/.source; // !=, ==, >=, <=, >, <
Expand All @@ -28,6 +28,15 @@ const ternaryRegexp = new RegExp(
);
const literalTernaryRegexp = /{{ params\.(\S+) \? (.+) : (.+) }/;

/**
* An array equality test for use on resolved list literal ParamValues only;
* skips a lot of the null/undefined/object-y/nested-list checks that something
* like Underscore's isEqual() would make because args have to be string[].
*/
function listEquals(a: string[], b: string[]): boolean {
return a.every((item) => b.includes(item)) && b.every((item) => a.includes(item));
}

/**
* Determines if something is a string that looks vaguely like a CEL expression.
* No guarantees as to whether it'll actually evaluate.
Expand Down Expand Up @@ -75,6 +84,10 @@ export function resolveExpression(
expr: CelExpression,
params: Record<string, ParamValue>
): Literal {
// N.B: List literals [] can contain CEL inside them, so we need to process them
// first and resolve them. This isn't (and can't be) recursive, but the fact that
// we only support string[] types mostly saves us here.
expr = preprocessLists(wantType, expr, params);
// N.B: Since some of these regexps are supersets of others--anything that is
// params\.(\S+) is also (.+)--the order in which they are tested matters
if (isIdentityExpression(expr)) {
Expand All @@ -94,11 +107,72 @@ export function resolveExpression(
}
}

/**
* Replaces all lists in a CEL expression string, which can contain string-type CEL
* subexpressions or references to params, with their literal resolved values.
* Not recursive.
*/
function preprocessLists(
wantType: L,
expr: CelExpression,
params: Record<string, ParamValue>
): CelExpression {
let rv = expr;
const listMatcher = /\[[^\[\]]*\]/g;
let match: RegExpMatchArray | null;
while ((match = listMatcher.exec(expr)) != null) {
const list = match[0];
const resolved = resolveList("string", list, params);
rv = rv.replace(list, JSON.stringify(resolved));
}
return rv;
}

/**
* A List in Functions CEL is a []-bracketed string with comma-seperated values that can be:
* - A double quoted string literal
* - A reference to a param value (params.FOO) which must resolve with type string
* - A sub-CEL expression {{ params.BAR == 0 ? "a" : "b" }} which must resolve with type string
*/
function resolveList(
wantType: "string",
list: string,
params: Record<string, ParamValue>
): string[] {
if (!list.startsWith("[") || !list.endsWith("]")) {
throw new ExprParseError("");
Berlioz marked this conversation as resolved.
Show resolved Hide resolved
} else if (list === "[]") {
return [];
}
const rv: string[] = [];
const entries = list.slice(1, -1).split(",");

for (const entry of entries) {
const trimmed = entry.trim();
if (trimmed.startsWith('"') && trimmed.endsWith('"')) {
rv.push(trimmed.slice(1, -1));
} else if (trimmed.startsWith("{{") && trimmed.endsWith("}}")) {
rv.push(resolveExpression("string", trimmed, params) as string);
} else {
const paramMatch = paramRegexp.exec(trimmed);
if (!paramMatch) {
throw new ExprParseError(`Malformed list component ${trimmed}`);
} else if (!(paramMatch[1] in params)) {
throw new ExprParseError("");
}
rv.push(resolveParamListOrLiteral("string", trimmed, params) as string);
}
}

return rv;
}

function assertType(wantType: L, paramName: string, paramValue: ParamValue) {
if (
(wantType === "string" && !paramValue.legalString) ||
(wantType === "number" && !paramValue.legalNumber) ||
(wantType === "boolean" && !paramValue.legalBoolean)
(wantType === "boolean" && !paramValue.legalBoolean) ||
(wantType === "string[]" && !paramValue.legalList)
) {
throw new ExprParseError(`Illegal type coercion of param ${paramName} to type ${wantType}`);
}
Expand All @@ -111,6 +185,8 @@ function readParamValue(wantType: L, paramName: string, paramValue: ParamValue):
return paramValue.asNumber();
} else if (wantType === "boolean") {
return paramValue.asBoolean();
} else if (wantType === "string[]") {
return paramValue.asList();
} else {
assertExhaustive(wantType);
}
Expand Down Expand Up @@ -154,9 +230,9 @@ function resolveComparison(
const test = function (a: Literal, b: Literal): boolean {
switch (cmp) {
case "!=":
return a !== b;
return Array.isArray(a) ? !listEquals(a, b as string[]) : a !== b;
case "==":
return a === b;
return Array.isArray(a) ? listEquals(a, b as string[]) : a === b;
case ">=":
return a >= b;
case "<=":
Expand Down Expand Up @@ -187,6 +263,14 @@ function resolveComparison(
} else if (lhsVal.legalBoolean) {
rhs = resolveLiteral("boolean", match[3]);
return test(lhsVal.asBoolean(), rhs);
} else if (lhsVal.legalList) {
if (!["==", "!="].includes(cmp)) {
throw new ExprParseError(
`Unsupported comparison operation ${cmp} on list operands in expression ${expr}`
);
}
rhs = resolveLiteral("string[]", match[3]);
return test(lhsVal.asList(), rhs);
} else {
throw new ExprParseError(
`Could not infer type of param ${lhsName} used in comparison operation`
Expand All @@ -210,9 +294,9 @@ function resolveDualComparison(
const test = function (a: Literal, b: Literal): boolean {
switch (cmp) {
case "!=":
return a !== b;
return Array.isArray(a) ? !listEquals(a, b as string[]) : a !== b;
case "==":
return a === b;
return Array.isArray(a) ? listEquals(a, b as string[]) : a === b;
case ">=":
return a >= b;
case "<=":
Expand Down Expand Up @@ -263,6 +347,18 @@ function resolveDualComparison(
);
}
return test(lhsVal.asBoolean(), rhsVal.asBoolean());
} else if (lhsVal.legalList) {
if (!rhsVal.legalList) {
throw new ExprParseError(
`CEL comparison expression ${expr} has type mismatch between the operands`
);
}
if (!["==", "!="].includes(cmp)) {
throw new ExprParseError(
`Unsupported comparison operation ${cmp} on list operands in expression ${expr}`
);
}
return test(lhsVal.asList(), rhsVal.asList());
} else {
throw new ExprParseError(
`could not infer type of param ${lhsName} used in comparison operation`
Expand All @@ -286,9 +382,9 @@ function resolveTernary(
const comparisonExpr = `{{ params.${match[1]} ${match[2]} ${match[3]} }}`;
const isTrue = resolveComparison(comparisonExpr, params);
if (isTrue) {
return resolveParamOrLiteral(wantType, match[4], params);
return resolveParamListOrLiteral(wantType, match[4], params);
} else {
return resolveParamOrLiteral(wantType, match[5], params);
return resolveParamListOrLiteral(wantType, match[5], params);
}
}

Expand All @@ -304,13 +400,12 @@ function resolveDualTernary(
if (!match) {
throw new ExprParseError("Malformed CEL ternary expression '" + expr + "'");
}

const comparisonExpr = `{{ params.${match[1]} ${match[2]} params.${match[3]} }}`;
const isTrue = resolveDualComparison(comparisonExpr, params);
if (isTrue) {
return resolveParamOrLiteral(wantType, match[4], params);
return resolveParamListOrLiteral(wantType, match[4], params);
} else {
return resolveParamOrLiteral(wantType, match[5], params);
return resolveParamListOrLiteral(wantType, match[5], params);
}
}

Expand Down Expand Up @@ -342,13 +437,13 @@ function resolveLiteralTernary(
}

if (paramValue.asBoolean()) {
return resolveParamOrLiteral(wantType, match[2], params);
return resolveParamListOrLiteral(wantType, match[2], params);
} else {
return resolveParamOrLiteral(wantType, match[3], params);
return resolveParamListOrLiteral(wantType, match[3], params);
}
}

function resolveParamOrLiteral(
function resolveParamListOrLiteral(
wantType: L,
field: string,
params: Record<string, ParamValue>
Expand All @@ -371,7 +466,20 @@ function resolveLiteral(wantType: L, value: string): Literal {
);
}

if (wantType === "number") {
if (wantType === "string[]") {
// N.B: value being a literal list that can just be JSON.parsed should be guaranteed
// by the preprocessLists() invocation at the beginning of CEL resolution
const parsed = JSON.parse(value);
if (!Array.isArray(parsed)) {
throw new ExprParseError("");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we include more descriptive parser error message?

}
for (const shouldBeString of parsed) {
if (typeof shouldBeString !== "string") {
throw new ExprParseError("");
}
}
return parsed as string[];
} else if (wantType === "number") {
if (isNaN(+value)) {
throw new ExprParseError("CEL literal " + value + " does not seem to be a number");
}
Expand Down