fix(parser): apply variable type constraints so optional() attributes materialise - #54
Merged
Merged
Conversation
… materialise The optional() support added in #46 (applyOptionalDefaults) only filled object attributes that had an explicit default, and only for a top-level object() type. It skipped bare optional(t) (no default) and did not descend into map(object(...)) / list(object(...)) wrappers. That is exactly the terraform-aws-modules/rds-aurora shape (issue #53): instances is typed map(object({ instance_class = optional(string), ... })), so a caller value of { one = {} } stayed an empty object, try(coalesce(each.value.instance_class, var.cluster_instance_class), null) errored on the missing attribute, try() swallowed it to null, and the resource fell back to the catalogue default class. Replace the hand-rolled extraction with the canonical Terraform mechanism: typeexpr.TypeConstraintWithDefaults, then Defaults.Apply + convert.Convert, applied once the var scope is fully layered, for both the root module and child-module inputs. This is a strict superset: it fills explicit optional defaults (including nested), materialises bare optionals as null, and handles map/list-of-object wrappers. Best-effort: unconvertible values pass through unchanged; any/untyped variables are skipped. Removes optional_defaults.go and its test; ports the explicit-default and nested-object coverage plus adds regression tests for the root and module-input rds-aurora paths.
andreacappelletti97
force-pushed
the
fix/module-variable-type-constraints
branch
from
July 28, 2026 18:06
13fa74a to
3ffe6b6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #53.
Root cause
c3x evaluated a variable's value but ignored its declared
typeconstraint, sooptional()object attributes were never filled. For an input typedmap(object({ instance_class = optional(string) })), a caller value of{ one = {} }stayed an empty object. Inside the module:each.value.instance_classerrored on the missing attribute, thecoalesceerrored,try()swallowed it tonull, and the resource fell back to the catalogue default class. That is why changingcluster_instance_classthrough terraform-aws-modules/rds-aurora produced no delta.Fix
Apply the declared type constraint once the var scope is fully layered, mirroring Terraform's own decoding (
typeexpr.TypeConstraintWithDefaults, thenDefaults.Apply+convert.Convert), for both the root module (parser.go) and child-module inputs (modules.go). Best-effort: unconvertible values pass through unchanged, andany/untyped variables are skipped so no useful structure is stripped.Verification
db.r6g.largetodb.r7g.2xlargenow moves the estimate correctly ($246.74 to $1,049.01), directly and through amoduleblock.go test ./...,go vet,gofmtclean. Nogo.modchange (typeexpr ships with hcl/v2).Boundary
This covers
optional()-typed inputs, the rds-aurora case. An input typedanywould still not materialise the attribute, but that also fails in real Terraform, so matching Terraform's behaviour is the correct boundary.