Remove `rawNames` from `makeObjectTerraformInputs` #1851

iwahbe · 2024-04-10T13:48:04Z

It never makes sense to pass rawNames=true to makeObjectTerraformInputs since that implies we have an object type with properties, but that we should not do property name translations. This is never the case.

This PR introduces a subtle change in behavior when walking values of unknown type:

Before this PR, we assumed that an untyped resource.PropertyMap was an object (name translation is performed) but all nested resource.PropertyMap values were then assumed to be a map (name translation is not performed).

After this PR, we treat all untyped resource.PropertyMap values as maps. IMO this makes more sense: the rule is we don't apply name translation unless we know a value is an object (and can thus generate naming tables for it).

I don't expect that this change will break users since user's should never hit untyped values. If they do, neither before or after will work consistently.

Addressing the test change:

"object_property_value" was used to test object names but never given a type. In light of the above change, we needed to make it typed to get the expected name translation.

Fixes #1830

This PR is best reviewed on a commit by commit basis. Each commit passes tests and includes a description of what it does (if anything). The commits build, with each commit able to remove rawNames from one additional place.

codecov · 2024-04-10T13:52:40Z

Codecov Report

Attention: Patch coverage is 82.29167% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 60.43%. Comparing base (17265a0) to head (7df97a6).
Report is 1 commits behind head on master.

Files	Patch %	Lines
pkg/tfbridge/schema.go	82.53%	9 Missing and 2 partials ⚠️
pkg/tfbridge/diff.go	81.25%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1851      +/-   ##
==========================================
- Coverage   60.48%   60.43%   -0.05%     
==========================================
  Files         310      310              
  Lines       42835    42825      -10     
==========================================
- Hits        25909    25883      -26     
- Misses      15453    15465      +12     
- Partials     1473     1477       +4

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

t0yv0 · 2024-04-10T14:47:21Z

After this PR, we treat all untyped resource.PropertyMap values as maps. IMO this makes more sense: the rule is we don't apply name translation unless we know a value is an object (and can thus generate naming tables for it).

I think that indeed makes a lot more sense. But it might break existing programs out there. I don't know how often we hit the "lost the type" case, I suspect it's a lot less than a year ago where we had bugs in matching the right type to the right input. So we can move forward with this but recommend calling out explicitly in release notes.

pkg/tfbridge/schema.go

t0yv0 · 2024-04-10T15:44:16Z

pkg/tfbridge/diff.go

+				for k, e := range v.ObjectValue() {
+					var elementPath string
+					if strings.ContainsAny(string(k), `."[]`) {
+						elementPath = fmt.Sprintf(`%s.["%s"]`, path, strings.ReplaceAll(string(k), `"`, `\"`))


Umm.. How is elementPath used? Should this be a type distinct from string that encapsulates stepping down like this?

I'm pretty sure it should be a resource.PropertyPath. I didn't change it because I didn't want to expand the scope of this PR any more. I'll stack another PR on top of this to refactor elementPath.

t0yv0 · 2024-04-10T16:14:40Z

I appreciate cleaning up rawNames as this was very difficult for me to grok, but I believe the only purpose of that indeed was to track through the recursion whether we are dealing with an object or a map, or else we lost the schema.

I am very hesitant with touching this code without adding a ton of coverage first. This is a true minefield that can cause a lot of downstream pain. I'm trying to read existing tests now to see if I can spot anything or anything we could add.

One evidence is that the PR had a mismatched if err handling but did that get caught by the tests?

One confusing property of how this function is written is how it's handling MaxItemsOne=1 interaction with the types. I think it might just work but it's not obvious to me.. I like code really obvious, like in F# and Rust.... the reason it works is MIO wraps the value in an array which then forces the schema lookup to do elemSchemas which forces the recursion down to Elem, which might just be what we need here.

Let me read the tests again and do some case analysis.

iwahbe · 2024-04-10T16:44:02Z

I appreciate cleaning up rawNames as this was very difficult for me to grok, but I believe the only purpose of that indeed was to track through the recursion whether we are dealing with an object or a map, or else we lost the schema.

That is the purpose, but rawNames as used is not fit for purpose. If we have a type then we can use that to determine if we are looking at a map or an object, and if we have no type then we have lost the schema.

I am very hesitant with touching this code without adding a ton of coverage first. This is a true minefield that can cause a lot of downstream pain. I'm trying to read existing tests now to see if I can spot anything or anything we could add.

I'm pretty sure I fixed a couple of lurking bugs around nested objects. I'll try to add some tests for them.

One evidence is that the PR had a mismatched if err handling but did that get caught by the tests?

That was a cosmetic error:

			obj, err = ctx.makeObjectTerraformInputs(oldObject, v.ObjectValue(),
				tfflds, psflds)
			if err != nil {
+				return nil, err
-				return obj, err

			}

Since ctx.makeObjectTerraformInputs always returns nil, err if err != nil, no test could have caught it.

pkg/tfbridge/schema_test.go

t0yv0 · 2024-04-10T20:46:26Z

Consider rebasing on #1853 and first having commits that add test cases, and then having the change in behavior match the change in tests, that'd be awesome (as well as separate changes that refactor from changes that change behavior)!

This may be separate but having re-read these test cases, they don't really cover that many variations which is a pity. Having this covered would be interesting:

I'm curious what happens with nils and unknowns in blocks in particular. This is coming up for cross-tests as well so maybe will be adding cases as part of that. A lot of things look quite suspect.

iwahbe · 2024-04-11T14:30:13Z

single-nested block is covered

list-nested block

set-nested block

map attribue

set attribute

list attribute

I'm going to open a PR to cover this list and get existing behavior under test before I merge here.

Followup to #1853. Since #1851 also edits these tests, I thought they should also have (and require) correct types.

It never makes sense to pass `rawNames=true` to `makeObjectTerraformInputs` since that implies we have an object type with properties, but that we should not do property name translations. This is never the case. This PR introduces a subtle change in behavior when walking values of unknown type: Before this PR, we assumed that an untyped `resource.PropertyMap` was an object (name translation is performed) but all nested `resource.PropertyMap` values were then assumed to be a map (name translation is not performed). After this PR, we treat all untyped `resource.PropertyMap` values as maps. IMO this makes more sense: the rule is we don't apply name translation unless we know a value is an object (and can thus generate naming tables for it). I don't expect that this change will break users since user's should never hit untyped values. If they do, neither before or after will work consistently. Addressing the test change: "object_property_value" was used to test object names but never given a type. In light of the above change, we needed to make it typed to get the expected name translation.

This is a pure refactor and doesn't have behavioral effects.

…Outputs` This has the same effect as 261df01, and needs the same test case adjustment. This is a "breaking change" to public functions, but we don't expect that these functions will be used outside of the bridge and we have made breaking changes to them in the past (#1642).

~This fixes a bug in path traversals with nested `MaxItems: 1` values, but I don't know if that has ever effected customers.~ This would fix a bug in path traversals through maps, but SDKv2 does not currently support complex types under maps (hashicorp/terraform-plugin-sdk#62), so this is a pure refactor.

This is a pure refactor and doesn't have behavioral effects. At this point, `getInfoFromTerraformName` is only falled with `rawNames = false` so we can safely remove it.

iwahbe · 2024-04-12T16:08:23Z

@t0yv0 After the escapade that ended in hashicorp/terraform-plugin-sdk#62, I believe that only 2 commits introduce semantic changes, both of which update existing tests, demonstrating their behavior. The rest are pure refactors. Each commit has a commit message that details if it is a pure refactor or what behavioral change is expected. Given the tests in #1853, #1855 and 7df97a6 (which doesn't change with the rest of this PR), I think we have enough coverage to merge.

t0yv0 · 2024-04-12T17:37:58Z

pkg/tfbridge/schema_test.go

+
+			// NOTE: This input does not match the schema's type. The tfValue *should* be wrapped
+			// in a []any{}. [MakeTerraformOutputs] handles this case, but it shouldn't need to
+			// and tf should never return it.


Maybe it handles it because it's trying to do MaxItems=1 mismatch toleration or something. Hard to know.

t0yv0 · 2024-04-12T17:39:33Z

pkg/tfbridge/schema_test.go

 			tfValue: map[string]interface{}{
 				"property_a": "a",
 				"property_b": true,
 			},
+			tfType: &schema.Schema{


Hmm, strange test case it didn't have tfType before so it was trying to parse untyped?

t0yv0 · 2024-04-12T17:40:45Z

pkg/tfbridge/schema_test.go

@@ -3129,6 +3129,32 @@ func Test_makeTerraformInputsNoDefaults(t *testing.T) {
 			//nolint:lll
 			expect: autogold.Expect(map[string]interface{}{"nested_resource": []interface{}{map[string]interface{}{"configuration": map[string]interface{}{"configurationValue": true}}}}),
 		},
+		{


t0yv0 · 2024-04-12T17:42:33Z

pkg/tfbridge/schema_test.go

@@ -3082,7 +3098,7 @@ func Test_makeTerraformInputsNoDefaults(t *testing.T) {
 				},
 			}),
 			//nolint:lll
-			expect: autogold.Expect(map[string]interface{}{"property_a": "a", "property_b": true, "property_c": map[string]interface{}{"nested_property_a": true}}),
+			expect: autogold.Expect(map[string]interface{}{"property_a": "a", "property_b": true, "property_c": map[string]interface{}{"nestedPropertyA": true}}),


This is an observable change, and the tests is showing it. map_property_value indicates a Map with unknown element types, which is supported in TF, and the change is that the keys are now passed as-is instead of inflected to Pulumi name notation, which makes perfect sense.

t0yv0

Thanks for wrangling difficult code!

This is almost perfect form for legacy refactoring, one thing I'd do is add tests at the bottom of the commit stack and make changes on top, that'd be perfect form.

Anyhow I think the change in behavior over TypeMap without a type looks reasonable. The new behavior is much more intuitive.

We have not quite reached consensus that we need to freeze development on this codebase and only make changes that are motivated directly by customer-visible scenarios that cannot be satisfied without the change, so I'm on balance for shipping this, but at some point I believe we'll need to consider "freeze" the bridge in the above manner since in foundational codebases that are used everywhere every change is fraught with breaking something.

iwahbe requested review from t0yv0, guineveresaenger and VenelinMartinov April 10, 2024 13:48

iwahbe self-assigned this Apr 10, 2024

t0yv0 reviewed Apr 10, 2024

View reviewed changes

pkg/tfbridge/schema.go Show resolved Hide resolved

t0yv0 reviewed Apr 10, 2024

View reviewed changes

pkg/tfbridge/schema.go Outdated Show resolved Hide resolved

iwahbe force-pushed the iwahbe/refactor-out-rawNames branch from 4d68b50 to af1dccf Compare April 10, 2024 15:12

iwahbe requested a review from t0yv0 April 10, 2024 15:15

t0yv0 reviewed Apr 10, 2024

View reviewed changes

pkg/tfbridge/schema_test.go Outdated Show resolved Hide resolved

iwahbe force-pushed the iwahbe/refactor-out-rawNames branch from af1dccf to 52675e5 Compare April 11, 2024 15:49

iwahbe mentioned this pull request Apr 11, 2024

Refactor TestTerraformOutputsWithSecretsSupported #1855

Merged

iwahbe added a commit that referenced this pull request Apr 12, 2024

Refactor TestTerraformOutputsWithSecretsSupported (#1855)

47052e2

Followup to #1853. Since #1851 also edits these tests, I thought they should also have (and require) correct types.

iwahbe force-pushed the iwahbe/refactor-out-rawNames branch from 52675e5 to 61e0336 Compare April 12, 2024 11:21

iwahbe added 4 commits April 12, 2024 14:15

Remove rawNames from *conversionContext.makeTerraformInput

991e816

This is a pure refactor and doesn't have behavioral effects.

Remove rawNames from *conversionContext.applyDefaults

e1d6178

This is a pure refactor and doesn't have behavioral effects.

iwahbe force-pushed the iwahbe/refactor-out-rawNames branch from 61e0336 to 1e5e820 Compare April 12, 2024 12:16

iwahbe added 2 commits April 12, 2024 17:47

Remove rawNames from getInfoFromTerraformName

f13bd3c

This is a pure refactor and doesn't have behavioral effects. At this point, `getInfoFromTerraformName` is only falled with `rawNames = false` so we can safely remove it.

iwahbe force-pushed the iwahbe/refactor-out-rawNames branch from 1e5e820 to f13bd3c Compare April 12, 2024 15:49

iwahbe requested a review from t0yv0 April 12, 2024 15:52

Add some additional test cases for "Test_makeTerraformInputsNoDefaults"

7df97a6

t0yv0 reviewed Apr 12, 2024

View reviewed changes

t0yv0 self-requested a review April 12, 2024 17:43

t0yv0 approved these changes Apr 12, 2024

View reviewed changes

iwahbe merged commit 69f75b8 into master Apr 13, 2024
11 checks passed

iwahbe deleted the iwahbe/refactor-out-rawNames branch April 13, 2024 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `rawNames` from `makeObjectTerraformInputs` #1851

Remove `rawNames` from `makeObjectTerraformInputs` #1851

iwahbe commented Apr 10, 2024 •

edited

codecov bot commented Apr 10, 2024 •

edited

t0yv0 commented Apr 10, 2024

t0yv0 Apr 10, 2024

iwahbe Apr 10, 2024

t0yv0 Apr 10, 2024

t0yv0 commented Apr 10, 2024

iwahbe commented Apr 10, 2024 •

edited

t0yv0 commented Apr 10, 2024

iwahbe commented Apr 11, 2024 •

edited

iwahbe commented Apr 12, 2024

t0yv0 Apr 12, 2024

t0yv0 Apr 12, 2024

t0yv0 Apr 12, 2024

t0yv0 Apr 12, 2024

t0yv0 left a comment

Remove rawNames from makeObjectTerraformInputs #1851

Remove rawNames from makeObjectTerraformInputs #1851

Conversation

iwahbe commented Apr 10, 2024 • edited

codecov bot commented Apr 10, 2024 • edited

Codecov Report

t0yv0 commented Apr 10, 2024

t0yv0 Apr 10, 2024

Choose a reason for hiding this comment

iwahbe Apr 10, 2024

Choose a reason for hiding this comment

t0yv0 Apr 10, 2024

Choose a reason for hiding this comment

t0yv0 commented Apr 10, 2024

iwahbe commented Apr 10, 2024 • edited

t0yv0 commented Apr 10, 2024

iwahbe commented Apr 11, 2024 • edited

iwahbe commented Apr 12, 2024

t0yv0 Apr 12, 2024

Choose a reason for hiding this comment

t0yv0 Apr 12, 2024

Choose a reason for hiding this comment

t0yv0 Apr 12, 2024

Choose a reason for hiding this comment

t0yv0 Apr 12, 2024

Choose a reason for hiding this comment

t0yv0 left a comment

Choose a reason for hiding this comment

Remove `rawNames` from `makeObjectTerraformInputs` #1851

Remove `rawNames` from `makeObjectTerraformInputs` #1851

iwahbe commented Apr 10, 2024 •

edited

codecov bot commented Apr 10, 2024 •

edited

iwahbe commented Apr 10, 2024 •

edited

iwahbe commented Apr 11, 2024 •

edited