Skip to content

fix(functions): resolve nested ternary CEL expressions in params - #11093

Open
Om-singhaI wants to merge 2 commits into
firebase:mainfrom
Om-singhaI:fix-cel-nested-ternary
Open

Om-singhaI wants to merge 2 commits into
firebase:mainfrom
Om-singhaI:fix-cel-nested-ternary

Conversation

@Om-singhaI

Copy link
Copy Markdown

Description

Fixes #7755.

A v1 runWith option built from a nested thenElse makes firebase-functions emit a chained CEL ternary into functions.yaml, and the emulator then fails to load the function:

FirebaseError: CEL tried to evaluate param."xxx" ? "aaa" : params.PROJECT_ID == "yyy" in a context which only permits literal values

The cause is in src/deploy/functions/cel.ts. The ternary regexps used a greedy (.+), so the last " ? " and " : " in the string ended up as the delimiters and the condition swallowed the first ternary whole. The two sibling forms are worse, because their conditions are anchored on params.X tokens and still parse, so only the branches split in the wrong place and they quietly return the wrong value with no error. On main, {{ params.FLAG ? "a" : params.OTHER ? "b" : "c" }} with FLAG false and OTHER true returns "c" where "b" is correct.

A regexp can't express this: ternaries nest to any depth, the SDK emits them without parentheses, and a quoted literal is allowed to contain either token. So this replaces them with a left to right scan that tracks quoting and pairs delimiters by depth. The condition it produces is dispatched to the comparison evaluators that already exist, which leaves comparison semantics, type coercion checks and error messages untouched. Branch evaluation is now recursive so a branch can be a ternary itself, and wantType is threaded down unchanged, so every leaf is still type checked exactly as before.

Three shapes behave differently from main:

  • Delimiters that can't be paired up now raise. Before, a " ? " with no matching " : " fell through to isComparisonExpression(), and resolveComparison() takes no wantType, so a string field came back as a boolean.
  • A ternary that isn't the whole expression is rejected, since the body now has to start with {{ and end with }}. resolveString() only ever hands the resolver whole {{ ... }} substrings, so that matches what it passes.
  • A value holding a double quote alongside a " ? " or a " : " is ambiguous. The SDK builds string operands as "${value}" without escaping, and nothing can tell a value's own quote from a real one. Where a split would cut a branch in half the expression is rejected, so most of these raise where main resolved them, and in rarer nested shapes the two disagree on the value instead. Expressions whose quotes line up are unaffected, and a " : " with no " ? " anywhere is left to the comparison evaluators exactly as before.

That structural check runs when the expression is split rather than only on the branch that gets selected, so a broken expression raises whichever way the params point. Resolution stays lazy about param values, so a missing param in a branch that is never selected still doesn't raise.

Scenarios Tested

Unit tests in src/deploy/functions/cel.spec.ts, beside the source as CONTRIBUTING asks, plus one in src/deploy/functions/params.spec.ts for the resolveString() path. cel.spec.ts is at 62 passing, up from 41. Reverting only cel.ts to main and rerunning both specs gives 15 failures, 14 and 1, including the exact error from the issue, so the new cases pin the fix rather than the implementation.

Covered:

  • the expression from the issue, asserted for all three values of PROJECT_ID, and the same shape with number branches
  • nesting in either branch, and a chain three levels deep with each of the four arms selected in turn
  • the boolean param and dual param forms that silently returned the wrong branch
  • params, lists and literals as branches of a nested ternary, with type checking
  • quoted branches holding " ? " or " : ", in the true branch and in the false branch
  • values holding double quotes, in the condition and in either branch, as a list element, and written by hand as \"
  • a nested ternary interpolated into a longer string through resolveString(), and one sitting next to a second expression, which is how serviceAccount and vpcConnector reach the resolver
  • the rejected shapes above, plus a missing delimiter, an empty branch, {{ }}, a bare colon, the double question mark form, a missing param in a nested branch and in a nested condition, a branch of the wrong type, and loose whitespace such as {{ params.FOO == 22 ? 10 : 0 }}

Also ran npx mocha src/deploy/functions/build.spec.ts alongside the two above, 114 passing, since that module imports cel.ts, plus npm run test:compile, which is clean. prettier --check passes on the changed files. eslint reports no errors and no new warnings.

Sample Commands

No commands or flags change. To run the affected tests:

npx mocha src/deploy/functions/cel.spec.ts src/deploy/functions/params.spec.ts

The ternary regexps used a greedy (.+) for the right hand side of the
comparison and for each branch, so the last " ? " and " : " in the string
were treated as the delimiters. A nested ternary like

  {{ params.PROJECT_ID == "xxx" ? "aaa" : params.PROJECT_ID == "yyy" ? "bbb" : "ccc" }}

parsed as though the condition were params.PROJECT_ID == ("xxx" ? "aaa" :
params.PROJECT_ID == "yyy"), and the emulator then failed to load the
function with "CEL tried to evaluate param. ... in a context which only
permits literal values". The two sibling ternary forms were worse. Their
conditions still parsed, so they selected the wrong branch and returned a
plausible wrong value with no error at all.

A regexp can't express this. Ternaries nest to any depth and the SDK emits
them without parentheses, so pairing each " ? " with the " : " that belongs
to it means counting, and a quoted string literal is allowed to contain
either token. Both need a left to right scan, so this adds a small
splitTernary() helper that tracks quoting and delimiter depth. The condition
it returns is dispatched to the existing comparison evaluators, which leaves
their semantics, type checks and error messages alone, and branches now
resolve recursively so that a branch can be a ternary itself.

Quoted branches containing " ? " or " : " now parse correctly too, which
falls out of the same scan. A backslash inside a literal escapes whatever
follows it, so the quotes that preprocessLists() writes into a list branch
don't pull the scan out of step.

The SDK builds string operands as "${value}" without escaping, so a value
holding a double quote leaves quotes in the body that delimit nothing. The
scan spots those, since a quote that really does open or close a literal sits
next to a space, a bracket or a comma. Once they show up the pairing is
ambiguous, so the scan collects every candidate " : " and takes the first one
whose two branches are both whole. A candidate that cuts a value in half
leaves a branch holding a delimiter that pairs with nothing, and when no
candidate survives that test the body is rejected instead of resolving to a
truncated value.

A " : " with nothing to pair it to anywhere in the body was never a ternary,
so it stays with the comparison evaluators that have always handled it. One
that does pair with a " ? " and still doesn't line up is now an error rather
than something those evaluators get to reinterpret, which used to hand a
string field a boolean.

resolveLiteral() now reports a list it can't parse with this module's own
error type, so a bad split can't surface as a raw SyntaxError from JSON.parse.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request replaces the regular-expression-based parsing of CEL ternary expressions with a manual scanner and parser to support nested ternaries of any depth and correctly handle string literals containing double quotes or delimiters. It also adds comprehensive unit tests for these scenarios. The review feedback points out a redundant check in splitTernary where rescanned.kind === "misquoted" is evaluated, which can be simplified since scanTernary with ignoreQuotes set to true will never return a misquoted status.

return { kind: "none" };
}
// The second scan doesn't track quoting, so it can't come back misquoted.
return rescanned.kind === "misquoted" ? { kind: "none" } : rescanned;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Since rescanned is obtained by calling scanTernary(body, true) with ignoreQuotes set to true, it can never return { kind: 'misquoted' }. Therefore, the check rescanned.kind === 'misquoted' is redundant and will always evaluate to false. We can simplify this statement to just return rescanned;.

Suggested change
return rescanned.kind === "misquoted" ? { kind: "none" } : rescanned;
return rescanned;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're right that the case can't happen at runtime, which is what the comment above the line says. The check is still doing work for the compiler though: scanTernary is declared as returning TernaryScan, which is wider than this function's return type, so return rescanned; fails npm run test:compile with TS2322: Type 'TernarySplit | { kind: "misquoted"; }' is not assignable to type 'TernarySplit'. I'd rather keep the narrowing than widen the return type or assert past it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Emulator fails to load functions with nested TernalyExpression for a runWith parameter

2 participants