Repository navigation
Conversation
|
I have read the CLA Document and I hereby sign the CLA |
jsteinich
left a comment
There was a problem hiding this comment.
Thanks for picking this up. The guard is in the right place: keysToSnakeCase is shared, so resources, providers and backends that get a null value stop crashing too. Every recursive call goes through the new early return, so it also covers the null array elements and nested values that #522 mentions. I checked the callers, and this only turns cases that used to throw into null in the output. Nothing that synthesized before comes out differently.
The one thing blocking this is the HCL output path (SYNTH_HCL_OUTPUT, which CI also runs the integration tests with). There, the same variable still loses its default or crashes. Both spots are outside the diff, so I've put them here instead of inline.
packages/cdktn/src/terraform-variable.ts:197 (blocking)
synthesizeHclAttributes() filters with !!value?.value, so default: null is dropped from the HCL output:
variable "v" {
type = any
}The JSON output now has "default": null, which makes the variable optional, but the HCL version is required. So the same app behaves differently depending on output mode. The same filter also drops default: false, 0 and "". That was already broken, but the same change fixes it:
Object.entries(attrs).filter(([_, value]) => value?.value !== undefined),packages/cdktn/src/hcl/render.ts:560 (blocking)
With the filter fixed, null reaches renderFuzzyJsonExpression, which has the same typeof null === "object" problem and calls Object.entries(null). This already crashes today for default: Token.nullValue() (the workaround in #522) and for a null nested in an object default, even with this PR applied. A null check before the object branch fixes it:
if (jsonExpression === null) {
return "null";
}
if (typeof jsonExpression === "object") {I tried both changes locally. null, false, 0, Token.nullValue() and { someKey: null, l: [null, { innerKey: null }] } all render as expected (default = null, default = false, …), and the existing cdktn unit tests and snapshots still pass.
PR title (nit)
cdktn isn't one of the allowed commit scopes, so the Lint PR check will fail on it. Changes to the core library use lib: could you rename it to fix(lib): Handle null variable defaults?
|
The HCL attribute filter in terraform-variable.ts now keeps every value that is not undefined, and renderFuzzyJsonExpression in hcl/render.ts returns null for a null value, so null defaults render in HCL. The PR title is unchanged in this round; the rename to fix(lib) still needs to be made on the PR. |
Summary
TerraformVariablewithdefault: nullcrashed during synthesis becausekeysToSnakeCasetreated null as an object. This change returns null and undefined unchanged before recursing, preserving the null default in synthesized output.Closes: #522
Testing
npx jest packages/cdktn/test/variable.test.ts --runInBand -t "null default value"Regression test: fails on base twice (/Cannot convert undefined or null to object/), passes on fix twice (
npx jest packages/cdktn/test/variable.test.ts --runInBand -t "null default value")Fixes #522