Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strip dynamic types out of null and unknown values recursively #144

Merged
merged 3 commits into from
Nov 9, 2022

Conversation

liamcervante
Copy link
Contributor

@liamcervante liamcervante commented Nov 7, 2022

The original issue is surfaced in terraform as hashicorp/terraform#32109

The root cause is that go-cty is not stripping dynamic information out of null values recursively, it only checks if the value itself is dynamic but not for example if it is a collection and has a dynamic element type. The new function behaves basically like WithoutOptionalAttributesDeep except it's taking away dynamic types where possible. If both the in and out types are dynamic then the function has no choice but to also return dynamic.

This approach is the simplest from an architecture point of view, once we decide we are returning a null value then we recurse through the type and replace any dynamic types we would return. However, it's a bit inefficient as it recalculates things like unified types in objects and tuples when this work was already done. To fix that we could take the short circuit out entirely and have every conversion function handle null and unknown themselves, but that's a lot of changes needed and this is quite simple to implement and I don't think too negatively performant.

@liamcervante liamcervante marked this pull request as ready for review November 7, 2022 10:55
@@ -43,14 +43,14 @@ func getConversion(in cty.Type, out cty.Type, unsafe bool) conversion {
out = out.WithoutOptionalAttributesDeep()

if !isKnown {
return cty.UnknownVal(out), nil
return cty.UnknownVal(dynamicReplace(in.Type(), out)), nil
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apparentlymart - I specifically wanted to fix Null values with this, but I think it makes sense that unknown and null values behave the same. In case, we don't want that let me know and I'll revert the change to this line.

@apparentlymart
Copy link
Collaborator

Thanks! This makes sense to me. It is true that this is probably gonna be a bit expensive, but better to be correct for now and then we can look for opportunities to optimize later if we discover this is causing a real problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants