Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork77
convert: Fix converting to nested dynamic map#47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
convert: Fix converting to nested dynamic map#47
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedFeb 25, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
478c755 toc6d1128CompareUh oh!
There was an error while loading.Please reload this page.
When converting an object tree to a nested dynamic map (for example,`cty.Map(cty.Map(cty.DynamicPseudoType))` or deeper), it is necessary tounify the types of the child elements of the map.Previously, we would only apply this process to the innermost leafobject's attributes, which could result in local unification butglobally incompatible types. This caused a panic when converting thesingle top-level object into a map with concrete type, as the types ofthe sub-trees were incompatible.This commit addresses this class of bugs in two ways: adding typeunification for multi-level map and object structures, and fallbackerror 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 sourceTerraform bugs which prompted this work.
c6d1128 to8be809fCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
📝
| @@ -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) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These changes throughout are for clarity, now that thepath parameter can be used multiple times (including in the call toconversionUnifyCollectionElements). Always mutating a copy seemed clearer to me than repeatedly mutating the passed parameter.
Co-Authored-By: Kristin Laemmert <mildwonkey@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
📝2️⃣
Uh oh!
There was an error while loading.Please reload this page.
This looks great to me@alisdair! I approve, but please get a second review. (not that I don't trustyour work, but mine) |
Re: the mismatched type error; if there is no valid conversion, we've already lost any path information and the 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 useconvert.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 toMismatchMessage. If I understand correctly, the relevant case here is wheregot is a map type andwant is an object type; it's currently handling only the opposite situation, going from object to map. (amismatchMessageStructuralFromCollections alongside the existingmismatchMessageCollectionsFromStructural, 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 ofthe child elements.This PR is very similar tozclconf#47, but handles lists of objects.
To correctly unify deep nested types, we need to unify all collectiontypes (maps, lists, sets), not just maps. This ensures that all cousintypes are unified correctly.This problem was first addressed for maps inzclconf#47 a year ago, but wedidn't realize at the time that the same bug could manifest in othercollection types.
To correctly unify deep nested types, we need to unify all collectiontypes (maps, lists, sets), not just maps. This ensures that all cousintypes are unified correctly.This problem was first addressed for maps inzclconf#47 a year ago, but wedidn't realize at the time that the same bug could manifest in othercollection types.
To correctly unify deep nested types, we need to unify all collectiontypes (maps, lists, sets), not just maps. This ensures that all cousintypes are unified correctly.This problem was first addressed for maps inzclconf#47 a year ago, but wedidn't realize at the time that the same bug could manifest in othercollection types.
Uh oh!
There was an error while loading.Please reload this page.
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:
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.