Tests for #3500 - #3547
Conversation
Review Verdict: Comment OnlyThe bipartite set-matching algorithm is correct and CI's required check is green, but the added tests never exercise the augmenting-path branch (the actual reason a matcher is used over a greedy loop) — so the core logic is unverified by the diff's own tests. Only non-blocking nits found; withholding approval so the test gap and the shared SDKv2/PF-path confirmation get a look. View session · Was this review helpful? Yes · No |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3547 +/- ##
==========================================
+ Coverage 70.18% 70.47% +0.28%
==========================================
Files 350 357 +7
Lines 38553 39446 +893
==========================================
+ Hits 27059 27798 +739
- Misses 9627 9702 +75
- Partials 1867 1946 +79 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![eon-pulumi-agent[bot]](https://avatars.githubusercontent.com/in/4080224?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this change, which replaces positional set-element matching in validInputsFromPlan with order-independent bipartite matching (Kuhn's augmenting-path algorithm) so reordered TypeSet elements — including nested sets — no longer produce spurious whole-set diffs. The linked issue is #3495; the diff touches pkg/tfbridge/detailed_diff.go and its test file.
What I checked:
- The matching algorithm is correct.
matchis a textbook augmenting-path search,seenis freshly allocated per top-level attempt and reused within a single augmentation,matchedInputForPlanis re-pointed on a successful path, and requiring every input to match combined with thelen(inputs) != len(plan)early return yields a correct perfect-bijection check. Empty sets and lists (still positional) behave correctly, and theSkipChildrenErrorreturn after a set match prevents the outer walk from double-descending. - The schema lookup in the recursion (
path.Index(planIndex)with the top-leveltfs) is correct:lookupSchemasresolves from the schema-map root using the full property path, so keepingtfsat the top level is required, not a bug. - Security: no exploit path. Set sizes come from the user's own program config and trusted state/plan output; there is no cross-trust-boundary input controlling them.
- CI: the required check (
Ensure test assets build cleanly) and lint are green; the large test matrix was still running at review time.
Findings are all non-blocking nits: the added tests exercise the reorder feature but never exercise the augmenting-path branch that is the reason a bipartite matcher is used; a worst-case complexity note; and a minor variable-shadowing readability nit.
One thing worth confirming rather than blocking on: MakeDetailedDiffV2 is shared runtime code reached from both the SDKv2 and Plugin Framework paths (pkg/pf/tfbridge/provider_diff.go), so this set-matching change applies to both. AGENTS.md asks to confirm new runtime behavior on both paths or document why it is path-specific; a PF-path or cross-test would close that out.
This is an automated low-risk assessment, not a human review.
|
|
||
| candidates := make([][]int, len(inputsList)) | ||
| for inputIndex, input := range inputsList { | ||
| for planIndex, plan := range planList { |
There was a problem hiding this comment.
Nit — logic
The inner loop variable plan shadows the closure's plan parameter. The shadowed value is what's used at the validInputsFromPlan(path.Index(planIndex), input, plan, ...) call, so this is not a bug, but it is easy to misread against the outer plan.
| for planIndex, plan := range planList { | |
| for inputIndex, input := range inputsList { | |
| for planIndex, planElem := range planList { | |
| if validInputsFromPlan(path.Index(planIndex), input, planElem, tfs, ps) { |
| if len(candidates[inputIndex]) == 0 { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit — perf
Set matching is now super-linear in set size: building candidates is O(n^2) full recursive validInputsFromPlan walks, and the augmenting-path matching adds up to O(n^3) worst case when many elements are mutually matchable (all-identical elements, near-empty objects, elements differing only in computed/null fields). This composes multiplicatively with nesting. The prior positional code was linear. Set contents come from user config/state, and this runs on every diff during preview/up.
Not a security issue (sizes aren't attacker-controlled across a trust boundary), and likely fine for typical set sizes, but resources with large sets (e.g. many firewall rules / IAM statements / DNS records) that are reorder- or duplicate-heavy could see a noticeable slowdown. Consider a size guard that falls back to the prior behavior above a threshold, and/or a benchmark to quantify the worst case.
| }, | ||
| }, | ||
| }, | ||
| want: true, |
There was a problem hiding this comment.
Nit — tests
The added cases (set does not require exact order, nested set does not require exact order, and TestDetailedDiffMatchesSetElementsWithReorderedNestedSets) all use sets of distinct elements ([a,b] vs [b,a]), so each input element has exactly one candidate plan index with no overlap. The candidate graph is a perfect one-to-one mapping and the augmenting-path recursion (|| match(matchedInputForPlan[planIndex], seen)) — the entire reason a bipartite matcher is used instead of a greedy loop — is never triggered. Deleting that recursive term would leave all these tests green.
Consider adding a case with partially-overlapping candidate sets (e.g. duplicate elements like [a,a], or elements matchable to multiple plan slots) so a first-fit greedy assignment would strand one element while augmenting-path succeeds. That locks in the non-greedy behavior this PR is really about.
2 participants
Running tests for #3500.