Check for previous null values in filtered diffs

[stevendborrelli]

Description of your changes

Summary

filteredDiffExists in pkg/controller/external_tfpluginfw.go discarded any
diff where the planned value (Value1) was null, on the assumption that a null
planned value means "field was never specified." This is correct for fields that
were never set, but it is wrong for Optional non-Computed attributes that a
user explicitly removes: the prior state is non-null, the planned state is null,
and that transition is a real diff that must trigger an Update.

for example:

1. User removes a nested attribute from their Claim
2. Composition patches null attribute to the MR's spec.forProvider
3. Upjet computes proposed state attribute = null (correct)
4. Upjet sends proposed state to TF provider's PlanResourceChange
5. TF provider returns planned state with attribute = null (correct)
6. Upjet diffs planned state vs prior state: Value1 (planned) = null, Value2 (prior) = non-null
7. filteredDiffExists filters out the diff because Value1 is null
8. Upjet concludes no diff exists — no Update is triggered
9. Block remains on the cluster

The result was that removing an optional nested attribute through a Crossplane
claim had no effect — Upjet concluded the resource was up-to-date and never
called the provider's Update RPC.

Fix

Add a second keep-condition to the filter: retain a diff when the planned value
(Value1) is null and the prior value (Value2) is non-null. This is the
unambiguous signature of an explicit removal.

The existing behavior is preserved for all other cases:

• Value1 nil (child attributes of a null parent object) → still filtered
• Value1 unknown (computed fields) → still filtered
• Both values null (field never specified) → still filtered

Fixes #

I have:

• Read and followed Upjet's contribution process.
• Run make reviewable to ensure this PR is ready for review.
• Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Added table-driven tests in TestFilteredDiffExists cover the core removal case,
the "never specified" case that must stay filtered, and the nested-object removal
scenario where child-attribute diffs have nil Value1 but the parent-level diff
carries the null/non-null transition.

[coderabbitai]

Warning

Rate limit exceeded

@stevendborrelli has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 56 minutes and 57 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 89aac6e0-d1aa-4e34-bb06-5fdb641f7b75

📥 Commits

Reviewing files that changed from the base of the PR and between c743f23 and 1b008da.

📒 Files selected for processing (2)

• pkg/controller/external_tfpluginfw.go
• pkg/controller/external_tfpluginfw_test.go

📝 Walkthrough

Walkthrough

filteredDiffExists was changed to treat explicit removals as real diffs: it now keeps diffs when the planned value is explicitly null while the prior value was non-null, in addition to the existing behavior that preserves known, non-null planned values. Tests exercising these cases were added.

Changes

Terraform Plan Diff Filtering

Layer / File(s)	Summary
Core Logic pkg/controller/external_tfpluginfw.go	filteredDiffExists now preserves diffs where Planned is explicitly null and Prior is non-null (explicit optional-attribute removals), along with existing preservation of known, non-null planned values.
Tests / Verification pkg/controller/external_tfpluginfw_test.go	TestFilteredDiffExists added: table-driven cases cover empty diffs, planned non-null changes, no-op null/null and unknown planned values, explicit planned-null/prior-non-null removals, and a nested-object removal scenario.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested labels: backport release-2.2

Suggested reviewers:

• ulucinar
• sergenyalcin
• erhancagirici

Thanks for the change — any preference on adding a comment in-code explaining the explicit-removal rationale for future readers?

🚥 Pre-merge checks | ✅ 7

✅ Passed checks (7 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: adding logic to check for previous null values when filtering diffs, which directly addresses the core fix in the PR.
Description check	✅ Passed	The description is well-detailed and directly related to the changeset, explaining the problem, the root cause, the fix, and how it was tested.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Configuration Api Breaking Changes	✅ Passed	Changes are in pkg/controller/, not pkg/config/. The custom check applies only to pkg/config/** modifications, so it does not apply here.
Generated Code Manual Edits	✅ Passed	No files matching the zz_*.go pattern were modified. All changes are to hand-written source files (external_tfpluginfw.go and its test file), not auto-generated code.
Template Breaking Changes	✅ Passed	No .tmpl template files were modified. Changes are to hand-written runtime library code (pkg/controller/external_tfpluginfw.go), not generated templates. Check is not applicable to this PR.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Upbound-CLA]

All committers have signed the CLA.