Fix dropping optional fields with schema defaults - #3484
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3484 +/- ##
==========================================
- Coverage 70.21% 70.21% -0.01%
==========================================
Files 350 350
Lines 38524 38530 +6
==========================================
+ Hits 27049 27053 +4
- Misses 9615 9617 +2
Partials 1860 1860 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
One Important finding: the null injection block in schema.go does not guard against getDefaultValue returning a non-nil error as interface{}, which would corrupt state for providers with failing DefaultFunc implementations. See the inline comment for the fix. One Nit on test comment framing.
Reviewed by Internal Trusted PR Reviewer
To install this agentic workflow, run
gh aw add pulumi-labs/gh-aw-internal/.github/workflows/gh-aw-pr-review.md@242988150273951aad5f67b008256266bdff6112
| // This handles the case where Read didn't populate the field but | ||
| // PlanResourceChange will apply the default, causing a null → default diff. | ||
| if ev.IsNull() && allowDrop && !etfs.Required() { | ||
| if dv := getDefaultValue(etfs, eps); dv != nil { |
There was a problem hiding this comment.
Important — bug
getDefaultValue can return a non-nil error as interface{} when DefaultFunc fails with an error other than ErrSchemaDefaultValue (see schema.go:~1855, which does return err). The dv != nil guard passes for errors, so resource.NewPropertyValue(dv) receives an error object and writes a corrupted value into the property map.
Before this PR, a null field fell through to v[k] = ev (storing null safely). This block regresses that for providers whose DefaultFunc returns a non-ErrSchemaDefaultValue error.
Add an explicit error guard:
if ev.IsNull() && allowDrop && !etfs.Required() {
if dv := getDefaultValue(etfs, eps); dv != nil {
if _, isErr := dv.(error); !isErr {
v[k] = resource.NewPropertyValue(dv)
continue
}
}
}Why this matters
getDefaultValue explicitly returns the raw error as interface{} for non-ErrSchemaDefaultValue failures. The existing isDefaultOrZeroValue caller is safe because comparing an error-typed dv to v.V simply returns false. The null injection path lacks this protection, introducing a state corruption regression.
| // With the fix for #2436, inputD is retained because its value | ||
| // matches the schema default. Previously it was dropped, causing | ||
| // a spurious diff on next preview. | ||
| "inputD": "input_d_default", |
There was a problem hiding this comment.
Nit — CLAUDE.md adherence
These comments reference the PR fix by issue number and describe prior behavior ("With the fix for #2436...", "Previously it was dropped"). The same pattern appears at lines 2795 and 3658. CLAUDE.md says to not reference the current task, fix, or callers — that context belongs in the PR description and rots as the codebase evolves.
Rephrase to describe the invariant instead:
| // With the fix for #2436, inputD is retained because its value | |
| // matches the schema default. Previously it was dropped, causing | |
| // a spurious diff on next preview. | |
| "inputD": "input_d_default", | |
| // inputD is retained: its value matches the schema default, and dropping it would | |
| // cause a spurious diff when PlanResourceChange re-applies the default on next preview. | |
| "inputD": "input_d_default", |
Why this matters
CLAUDE.md: "Don't reference the current task, fix, or callers ('used by X', 'added for the Y flow', 'handles the case from issue #123'), since those belong in the PR description and rot as the codebase evolves."
When extractSchemaInputsObject encounters an optional field whose value matches its schema default (e.g. `false` for `Default: false`), it was being dropped as a "zero value". This caused spurious diffs on the next preview when PlanResourceChange re-applied the same default. Additionally, when Read returns null for a field that has a schema default, inject the default value to prevent a null → default diff after import. Fixes #2436 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tValue - Add error type check to prevent getDefaultValue returning a raw error from corrupting state via resource.NewPropertyValue - Rephrase test comments to describe invariants instead of referencing issue numbers Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
11051ae to
651abfb
Compare
1 participant
Summary
extractSchemaInputsObjectdropping optional fields whose value matches the schema default (e.g.force_destroy: falsewithDefault: false). Previously these were treated as "zero values" and dropped, causing spurious diffs on next preview whenPlanResourceChangere-applied the same default.null → defaultdiff afterpulumi import.Fixes #2436
Test plan
TestExtractSchemaInputsDefaultInjectioncovering null injection, default retention, and no-default drop casesTestIsDefaultOrZeroValueBehaviorvalidating understanding of the helper functionTestExtractDefaultSecretInputs,TestExtractDefaultIntegerInputs, andTestExtractInputsFromOutputsSdkv2expectations🤖 Generated with Claude Code