convert: Fix converting to nested dynamic map #47

alisdair · 2020-02-25T17:31:17Z

When converting an object tree to a nested dynamic map (for example, cty.Map(cty.Map(cty.DynamicPseudoType)) or deeper), it is necessary to unify the types of the child elements of the map.

Previously, we would only apply this process to the innermost leaf object's attributes, which could result in local unification but globally incompatible types. This caused a panic when converting the single top-level object into a map with concrete type, as the types of the sub-trees were incompatible.

This commit addresses this class of bugs in two ways: adding type unification for multi-level map and object structures, and fallback error handling to prevent panics for this class of failure.

The additional type unification process has several steps:

First, we add a second unify & convert step at the end of the process, immediately before constructing the final map. This second unification ensures that not only sibling elements but also cousins are of the same type.
We also ensure that generated type information is preserved for empty collections, by returning an empty map of the detected type, instead of the dynamic type. This prevents a multi-step unification bouncing back and forth between concrete and dynamic element types.
Finally, we add a specific unification procedure for map-to-map conversions. This inspects and unifies the element types for the maps which are being converted. For example, a populated map(string) and an empty map(any) will be unified to a map(string) type.

Tests are extended to cover reduced configurations from the three source Terraform bugs which prompted this work.

TODO

I'm not satisfied with the error that is printed for Terraform bug 24167:

Error: Invalid value for input variable

  on variables.tf line 1:
   1: variable "x" {

The given value is not valid for variable "x": object is required.

I don't yet understand why the path to the specific sub-element isn't displayed.

Edit: this is explained by James in a comment below. Fixing it in this PR seems out of scope.

codecov · 2020-02-25T17:41:49Z

Codecov Report

Merging #47 into master will increase coverage by 0.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
+ Coverage   69.92%   70.10%   +0.17%     
==========================================
  Files          78       78              
  Lines        6987     7064      +77     
==========================================
+ Hits         4886     4952      +66     
- Misses       1668     1673       +5     
- Partials      433      439       +6

Impacted Files	Coverage Δ
cty/convert/conversion_collection.go	`82.71% <0.00%> (-0.62%)`	⬇️
cty/convert/mismatch_msg.go	`70.87% <0.00%> (+1.94%)`	⬆️
cty/convert/unify.go	`81.03% <0.00%> (-0.05%)`	⬇️
cty/convert/compare_types.go	`100.00% <0.00%> (+2.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 089490b...e1048fb. Read the comment docs.

cty/convert/public_test.go

When converting an object tree to a nested dynamic map (for example, `cty.Map(cty.Map(cty.DynamicPseudoType))` or deeper), it is necessary to unify the types of the child elements of the map. Previously, we would only apply this process to the innermost leaf object's attributes, which could result in local unification but globally incompatible types. This caused a panic when converting the single top-level object into a map with concrete type, as the types of the sub-trees were incompatible. This commit addresses this class of bugs in two ways: adding type unification for multi-level map and object structures, and fallback error handling to prevent panics for this class of failure. The additional type unification process has several steps: - First, we add a second unify & convert step at the end of the process, immediately before constructing the final map. This second unification ensures that not only sibling elements but also cousins are of the same type. - We also ensure that generated type information is preserved for empty collections, by returning an empty map of the detected type, instead of the dynamic type. This prevents a multi-step unification bouncing back and forth between concrete and dynamic element types. - Finally, we add a specific unification procedure for map-to-map conversions. This inspects and unifies the element types for the maps which are being converted. For example, a populated map(string) and an empty map(any) will be unified to a map(string) type. Tests are extended to cover reduced configurations from the three source Terraform bugs which prompted this work.

alisdair

📝

alisdair · 2020-03-03T14:44:20Z

cty/convert/conversion_collection.go

@@ -15,18 +15,18 @@ func conversionCollectionToList(ety cty.Type, conv conversion) conversion {
 	return func(val cty.Value, path cty.Path) (cty.Value, error) {
 		elems := make([]cty.Value, 0, val.LengthInt())
 		i := int64(0)
-		path = append(path, nil)
+		elemPath := append(path.Copy(), nil)


These changes throughout are for clarity, now that the path parameter can be used multiple times (including in the call to conversionUnifyCollectionElements). Always mutating a copy seemed clearer to me than repeatedly mutating the passed parameter.

Co-Authored-By: Kristin Laemmert <[email protected]>

alisdair

📝2️⃣

cty/convert/unify.go

mildwonkey · 2020-03-03T14:54:34Z

This looks great to me @alisdair! I approve, but please get a second review. (not that I don't trust your work, but mine)

jbardin · 2020-03-03T15:18:05Z

Re: the mismatched type error; if there is no valid conversion, we've already lost any path information and the Convert function only knows that it's not possible, and simplified types are displayed in the message.

One possibility is making the mismatch errors more detailed and provide the nested type information so the user can at least try to deduce why it failed, but that may be too noisy in the general case. The much larger change would be to thread diagnostics back through getConversion to provide precise errors.

apparentlymart

Thanks for digging into this! The explanation and fix here both make sense to me.

r.e. the unhelpful error message, IIRC the current implementation of those error messages in Terraform is to use convert.MismatchMessage after the fact, which can (for some limited cases, currently) describe a more detailed diff between the wanted type and the given type. The error message you saw here is the fallback for when none of the special cases it knows about apply.

We could potentially improve it by adding additional rules to MismatchMessage. If I understand correctly, the relevant case here is where got is a map type and want is an object type; it's currently handling only the opposite situation, going from object to map. (a mismatchMessageStructuralFromCollections alongside the existing mismatchMessageCollectionsFromStructural, perhaps.)

That improvement seems orthogonal to this one though, so I'm going to land this PR now and maybe we can iterate on the mismatch message implementation in separate PRs.

When converting a list of objects, it is necessary to unify the types of the child elements. This PR is very similar to zclconf#47, but handles lists of objects.

To correctly unify deep nested types, we need to unify all collection types (maps, lists, sets), not just maps. This ensures that all cousin types are unified correctly. This problem was first addressed for maps in zclconf#47 a year ago, but we didn't realize at the time that the same bug could manifest in other collection types.

alisdair force-pushed the alisdair/fix-convert-nested-dynamic-map branch from 478c755 to c6d1128 Compare February 25, 2020 22:06

alisdair marked this pull request as ready for review February 25, 2020 22:21

mildwonkey reviewed Mar 3, 2020

View reviewed changes

cty/convert/public_test.go Outdated Show resolved Hide resolved

alisdair force-pushed the alisdair/fix-convert-nested-dynamic-map branch from c6d1128 to 8be809f Compare March 3, 2020 14:47

alisdair commented Mar 3, 2020

View reviewed changes

Update cty/convert/public_test.go

65f6e09

Co-Authored-By: Kristin Laemmert <[email protected]>

alisdair commented Mar 3, 2020

View reviewed changes

cty/convert/unify.go Outdated Show resolved Hide resolved

Fix potential panic in unreachable code

e1048fb

apparentlymart approved these changes Mar 3, 2020

View reviewed changes

apparentlymart merged commit aa51091 into zclconf:master Mar 3, 2020

alisdair deleted the alisdair/fix-convert-nested-dynamic-map branch March 3, 2020 19:28

mildwonkey mentioned this pull request May 18, 2020

Cannot use type any in object() hashicorp/terraform#24801

Closed

mildwonkey mentioned this pull request May 18, 2020

convert: fix panic when converting nested objects #53

Merged

alisdair mentioned this pull request Mar 4, 2021

Optional attributes fixes #88

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

convert: Fix converting to nested dynamic map #47

convert: Fix converting to nested dynamic map #47

alisdair commented Feb 25, 2020 •

edited

Loading

codecov bot commented Feb 25, 2020 •

edited

Loading

alisdair left a comment

alisdair Mar 3, 2020

alisdair left a comment

mildwonkey commented Mar 3, 2020

jbardin commented Mar 3, 2020

apparentlymart left a comment

convert: Fix converting to nested dynamic map #47

convert: Fix converting to nested dynamic map #47

Conversation

alisdair commented Feb 25, 2020 • edited Loading

TODO

codecov bot commented Feb 25, 2020 • edited Loading

Codecov Report

alisdair left a comment

Choose a reason for hiding this comment

alisdair Mar 3, 2020

Choose a reason for hiding this comment

alisdair left a comment

Choose a reason for hiding this comment

mildwonkey commented Mar 3, 2020

jbardin commented Mar 3, 2020

apparentlymart left a comment

Choose a reason for hiding this comment

alisdair commented Feb 25, 2020 •

edited

Loading

codecov bot commented Feb 25, 2020 •

edited

Loading