Skip to content

fix(lib): ForExpression missing iteratorContext and OperatorExpression drops falsy right operands#112

Open
X-Guardian wants to merge 2 commits intoopen-constructs:mainfrom
X-Guardian:ForExpression-fix
Open

fix(lib): ForExpression missing iteratorContext and OperatorExpression drops falsy right operands#112
X-Guardian wants to merge 2 commits intoopen-constructs:mainfrom
X-Guardian:ForExpression-fix

Conversation

@X-Guardian
Copy link
Copy Markdown

@X-Guardian X-Guardian commented Apr 21, 2026

Related issue

Fixes #111

Description

Bug 1: ForExpression.resolve() does not set iteratorContext

DynamicListTerraformIterator._getForEachExpression() returns a Lazy that checks context.iteratorContext — returning the raw list for "FOR_EXPRESSION" and the map conversion for the default case. TerraformDynamicExpression.resolve() correctly sets context.iteratorContext = "FOR_EXPRESSION" before resolving, but ForExpression.resolve() does not.

Fix: Add context.iteratorContext = "FOR_EXPRESSION" in ForExpression.resolve():

Bug 2: OperatorExpression.resolve() uses truthiness check instead of undefined check

Line 325 uses this.right ? ... : undefined which treats all falsy values (0, false, "") as missing. The - operator branch (line 337) has the same bug.

Fix:

- const right = this.right ? this.resolveArg(context, this.right) : undefined;
+ const right = this.right !== undefined ? this.resolveArg(context, this.right) : undefined;
  case "-": {
-   if (right) {
+   if (right !== undefined) {
      // subtraction
      expr = `(${left} - ${right})`;
    } else {

Checklist

  • I have updated the PR title to match CDKTN's style guide
  • I have run the linter on my code locally
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if applicable
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works if applicable
  • New and existing unit tests pass locally with my changes

@X-Guardian X-Guardian marked this pull request as ready for review April 22, 2026 16:25
@X-Guardian X-Guardian requested a review from a team as a code owner April 22, 2026 16:25
public resolve(context: IResolveContext): string {
const suppressBraces = context.suppressBraces;
context.suppressBraces = true;
context.iteratorContext = "FOR_EXPRESSION";
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't believe this is the correct solution. It does work for your example and test case, but I believe it will fail if the input expression actually references a map.

I wonder if DynamicListTerraformIterator should resolve the for each expression with a value set at creation time rather than relying on the current context. Or perhaps the ForExpression needs more information at creation time about the input expression.

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.

CDKTN: ForExpression missing iteratorContext and OperatorExpression drops falsy right operands

2 participants