Refactor detailed diff v2 to short circuit on nulls and unknowns #2496

VenelinMartinov · 2024-10-17T15:27:22Z

This PR changes the logic in the detailed diff v2 calculation to short-circuit on encountering nils and unknowns.

Previously, when we encountered a null or unknown in either news or olds we would still recurse down and compare the non-nil values versus a nil value in order to get any replacements which might be happening. We would then simplify the diff later to trim these extra diffs but would propagate the replacement to the right level.

We would now instead short-circuit on encountering a null or unknown and walk the non-null value to find any properties which can trigger a replacement. This makes it much easier to handle sets in #2451 as recursing into sets is not necessary, as they are only compared by hashes.

This change is not meant to change any behaviour and is tested in #2405

codecov · 2024-10-17T15:32:48Z

Codecov Report

Attention: Patch coverage is 88.23529% with 12 lines in your changes missing coverage. Please review.

Project coverage is 62.60%. Comparing base (0e0d2c4) to head (e13ae47).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/tfbridge/detailed_diff.go	88.23%	8 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2496      +/-   ##
==========================================
+ Coverage   62.58%   62.60%   +0.01%     
==========================================
  Files         380      380              
  Lines       51265    51294      +29     
==========================================
+ Hits        32086    32112      +26     
- Misses      17363    17365       +2     
- Partials     1816     1817       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

iwahbe · 2024-10-18T11:55:44Z

pkg/tfbridge/detailed_diff.go

+	if !isPresent(val) {
+		return false
+	}
+	switch propType {


The logic in isTypeShapeMismatched only works when val doesn't have any marker types, so lets assert that.

Suggested change

switch propType {

contract.Assertf(!val.IsComputed() && !val.IsSecret() && !val.IsOutput(),

"Unexpected marked value %#v", val)

switch propType {

iwahbe · 2024-10-18T12:01:53Z

pkg/tfbridge/detailed_diff.go

+	if !visitor(path, val) {
+		return false
+	}
+


Again, I'd like to see an assert that we are not hitting marker types.

Just was commenting on another PR, but this is not a complete walker on PropertyValue.

E.G.

https://github.com/pulumi/pulumi-terraform-bridge/blob/master/unstable/propertyvalue/propertyvalue.go#L42

If we had a complete walker on property value we would not need to double-test it. I know Go is about "a bit more duplication is better than a bit more sharing", but if we can reuse that makes coverage goals easier to achieve.

iwahbe · 2024-10-18T12:07:28Z

pkg/tfbridge/detailed_diff.go

+	return true
+}
+
+func willTriggerReplacement(


Can you comment on why we need willTriggerReplacement and willTriggerReplacementRecursive.

I think this code decides presentation. We already decided if the plan is replace or not. This code decides which change to blame for replacing so it's easier to see visually why a replace is happening. Is that correct VM?

iwahbe · 2024-10-18T12:08:27Z

pkg/tfbridge/detailed_diff.go

+	return isForceNew(tfs, ps)
+}
+
+func willTriggerReplacementRecursive(


The name for this function needs to describe what it does, not how its' implemented.

iwahbe · 2024-10-18T12:09:17Z

pkg/tfbridge/detailed_diff.go

+	if tfs.Type() != shim.TypeList && tfs.Type() != shim.TypeSet && tfs.Type() != shim.TypeMap {
+		return false
+	}


I'm surprised that this is (1) only effected by one level up and (2) doesn't include ForceNew on objects.

I'm not sure I understand these - can you please expand on this?

I'm surprised that we don't search all the way up the schema tree, not just one level up.

I think I was wrong about not including objects. tfs.Type() doesn't express objects.

Yeah I was wondering this too. We can probably get a away with something approximate as it's presentation-layer only but worth backlogging at least.

iwahbe · 2024-10-18T12:12:43Z

pkg/tfbridge/detailed_diff.go

+	oldMap := old.ObjectValue()
+	newMap := new.ObjectValue()


Just so I understand, this is ok because of isTypeShapeMismatched?

iwahbe · 2024-10-18T12:28:37Z

Reviewing the tests added in #2405, they look sufficient for nested forcenew in top level and singly nested cases. Can you point me to where multiple nesting levels are tested? I want to see a test that validates our behavior on a schema like this:

List{
  ForceNew: true,
  Elem: Object{
    Fields: {
       "f0": String,
    }
  },
}

I couldn't find tests for more then one level of nesting.

t0yv0 · 2024-10-18T17:32:26Z

pkg/tfbridge/detailed_diff.go

+func willTriggerReplacement(
+	path propertyPath, rootTFSchema shim.SchemaMap, rootPulumiSchema map[string]*info.Schema,
+) bool {
+	// A change on a property might trigger a replacement if:


The function seems to do what the comment says. The rule sounds a little un-intuitive but looks like you've explored this thoroughly in tests so perhaps that's that?

If a property P is forcenew, but P.a.b.c.d has changed, I am guessing that the overall plan is a replace plan, is that right? If we are displaying the detailed diff as a.b.c.d has changed we are probably within rights to blame d for replacing?

t0yv0 · 2024-10-18T17:49:52Z

pkg/tfbridge/detailed_diff.go

@@ -264,13 +294,48 @@ func (differ detailedDiffer) makePlainPropDiff(
 	return nil
 }

+// makeShortCircuitDiff is used for properties that are nil or computed in either the old or new state.


This is .. tricky.. If a property is Computed or unknown, we do not know if it will trigger a replacement or not, until we know it. It might trigger a replacement, or it might not. Any implementation here takes a guess and we need to probably call this out in a comment that we are guessing. I would look up current Pulumi behavior for this case and copy how we're guessing to continue in the tradition we have.

t0yv0 · 2024-10-18T17:52:21Z

pkg/tfbridge/detailed_diff_test.go

-	differ := detailedDiffer{
-		tfs: shimv2.NewSchemaMap(sdkv2Schema),
-	}
+	tfs := shimv2.NewSchemaMap(sdkv2Schema)


The changes to tests look very unconvincing 🙏

You're going for no change in behavior in this PR?

Can you point me to a test case that illustrates this "short-circuit" diff so I can build up some intuition? Perhaps this all is under tests already. Thank you!

Do not want to block on this but would like to understand better.

refactor detailed diff v2 to short circuit on nulls and unknowns

629a5ff

VenelinMartinov marked this pull request as ready for review October 17, 2024 17:37

VenelinMartinov requested review from t0yv0, guineveresaenger and iwahbe October 17, 2024 17:39

This was referenced Oct 17, 2024

Move property path code to separate file #2497

Open

Improve set detailed diffs #2451

Open

VenelinMartinov added 2 commits October 18, 2024 12:00

move type mismatch check and short-circuit logic

49ec2be

rename

e13ae47

iwahbe reviewed Oct 18, 2024

View reviewed changes

t0yv0 reviewed Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor detailed diff v2 to short circuit on nulls and unknowns #2496

Refactor detailed diff v2 to short circuit on nulls and unknowns #2496

VenelinMartinov commented Oct 17, 2024 •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

iwahbe Oct 18, 2024

iwahbe Oct 18, 2024

t0yv0 Oct 18, 2024

iwahbe Oct 18, 2024

t0yv0 Oct 18, 2024

iwahbe Oct 18, 2024

iwahbe Oct 18, 2024

VenelinMartinov Oct 18, 2024

iwahbe Oct 18, 2024

t0yv0 Oct 18, 2024

iwahbe Oct 18, 2024

iwahbe commented Oct 18, 2024

t0yv0 Oct 18, 2024

t0yv0 Oct 18, 2024

t0yv0 Oct 18, 2024

Refactor detailed diff v2 to short circuit on nulls and unknowns #2496

Are you sure you want to change the base?

Refactor detailed diff v2 to short circuit on nulls and unknowns #2496

Conversation

VenelinMartinov commented Oct 17, 2024 • edited Loading

codecov bot commented Oct 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwahbe commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VenelinMartinov commented Oct 17, 2024 •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading