-
Notifications
You must be signed in to change notification settings - Fork 34
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
tftypes: Introduce unknown value refinement support for Value
#448
base: main
Are you sure you want to change the base?
Conversation
"unknown-bool": { | ||
in: NewValue(Bool, UnknownValue), | ||
expected: "tftypes.Bool<unknown>", | ||
}, | ||
"unknown-bool-with-nullness-refinement": { | ||
in: NewValue(Bool, UnknownValue).Refine(refinement.Refinements{ | ||
refinement.KeyNullness: refinement.NewNullness(false), | ||
}), | ||
expected: `tftypes.Bool<unknown, not null>`, | ||
}, | ||
"unknown-string": { | ||
in: NewValue(String, UnknownValue), | ||
expected: "tftypes.String<unknown>", | ||
}, | ||
"unknown-string-with-multiple-refinements": { | ||
in: NewValue(String, UnknownValue).Refine(refinement.Refinements{ | ||
refinement.KeyNullness: refinement.NewNullness(false), | ||
refinement.KeyStringPrefix: refinement.NewStringPrefix("str://"), | ||
}), | ||
expected: `tftypes.String<unknown, not null, prefix = "str://">`, | ||
}, | ||
"unknown-number": { | ||
in: NewValue(Number, UnknownValue), | ||
expected: "tftypes.Number<unknown>", | ||
}, | ||
"unknown-number-with-multiple-refinements": { | ||
in: NewValue(Number, UnknownValue).Refine(refinement.Refinements{ | ||
refinement.KeyNullness: refinement.NewNullness(false), | ||
refinement.KeyNumberLowerBound: refinement.NewNumberLowerBound(big.NewFloat(5), true), | ||
refinement.KeyNumberUpperBound: refinement.NewNumberUpperBound(big.NewFloat(10), true), | ||
}), | ||
expected: `tftypes.Number<unknown, not null, lower bound = 5 (inclusive), upper bound = 10 (inclusive)>`, | ||
}, | ||
"unknown-number-float-with-multiple-refinements": { | ||
in: NewValue(Number, UnknownValue).Refine(refinement.Refinements{ | ||
refinement.KeyNullness: refinement.NewNullness(false), | ||
refinement.KeyNumberLowerBound: refinement.NewNumberLowerBound(big.NewFloat(5.67), true), | ||
refinement.KeyNumberUpperBound: refinement.NewNumberUpperBound(big.NewFloat(10.123), true), | ||
}), | ||
expected: `tftypes.Number<unknown, not null, lower bound = 5.67 (inclusive), upper bound = 10.123 (inclusive)>`, | ||
}, | ||
"unknown-list": { | ||
in: NewValue(List{ElementType: String}, UnknownValue), | ||
expected: "tftypes.List[tftypes.String]<unknown>", | ||
}, | ||
"unknown-list-with-multiple-refinements": { | ||
in: NewValue(List{ElementType: String}, UnknownValue).Refine(refinement.Refinements{ | ||
refinement.KeyNullness: refinement.NewNullness(false), | ||
refinement.KeyCollectionLengthLowerBound: refinement.NewCollectionLengthLowerBound(2), | ||
refinement.KeyCollectionLengthUpperBound: refinement.NewCollectionLengthUpperBound(7), | ||
}), | ||
expected: `tftypes.List[tftypes.String]<unknown, not null, length lower bound = 2, length upper bound = 7>`, | ||
}, |
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.
We can bikeshed the display of these attributes since there is no established pattern ATM (core displays the cty
methods when showing data consistency errors, but doesn't have a "user friendly" version of the data defined). Other thoughts:
tftypes.String<unknown, refinements = [not null, prefix = "str://"]>
tftypes.String<unknown, will not be null, will have prefix "str://">
// NewStringPrefix returns the StringPrefix unknown value refinement that indicates the final value will be prefixed with the specified | ||
// string value. String prefixes that exceed 256 characters in length will be truncated and empty string prefixes will not be encoded. This | ||
// refinement can only be applied to the String type. | ||
func NewStringPrefix(value string) Refinement { | ||
return StringPrefix{ | ||
value: value, | ||
} | ||
} |
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.
There is a potential subtle hazard here that you can read more about in the code that deals with the hazard over in the cty
implementation:
https://github.com/zclconf/go-cty/blob/main/cty/ctystrings/prefix.go#L29
To reframe that in more practical terms, the hazard is that if the promised string prefix is "hi"
and then the final string is "hí"
(that is: h
, i
, followed by U+0301 combining acute accent) then cty
's model of strings as a sequence of grapheme clusters (as opposed to unicode scalar values) will mean that the final value isn't considered to conform to the refinement, which will probably cause Terraform to complain that the refinements were inconsistent.
I don't recall whether Terraform actually checks this as part of its plan/apply consistency checks, but even if Terraform doesn't immediately catch and reject it the broken promise could still cause problems downstream with anything that was relying on the initial refinement.
In cty
I implemented a rather heavy rule that discards any trailing character that could potentially turn out to be the start of a multi-scalar-value sequence. I think that's probably overkill for the Terraform plugin libraries because these prefixes are presumably going to be chosen directly by the provider developer rather than the end-user anyway, and so it'd probably be sufficient to just warn about this in the documentation here.
To again make that more practical/concrete: it's fine and good to say NewStringPrefix("ami-")
because -
is not the start of any valid combining sequence. It's also fine to announce NewStringPrefix("ami")
if the provider dev knows that the final string is definitely not going to have a combining diacritic after it, which would be true for an AWS AMI ID. Where things get messy is if the character immediately after the prefix is end-user-controlled and could potentially begin with a combining character.
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 sharing that @apparentlymart! I've put this work down ATM, but I'll make a note for later to come back and decide what we want to do here. Like you said, at the very least documentation should be added.
I did run a quick test and Terraform does check the refinements during it's plan/apply data consistency check, so that's good!
--- FAIL: TestSchemaResource_basic (0.89s)
/Users/austin.valle/code/terraform-provider-corner/internal/framework6provider/schema_resource_test.go:19: Step 1/1 error: Error running apply: exit status 1
Error: Provider produced inconsistent result after apply
When applying changes to framework_schema.test, provider
"provider[\"registry.terraform.io/hashicorp/framework\"]" produced an
unexpected new value: .prefix_test: final value cty.StringVal("hí") does not
conform to planning placeholder
cty.UnknownVal(cty.String).Refine().NotNull().StringPrefixFull("hi").NewValue().
This is a bug in the provider, which should be reported in the provider's own
issue tracker.
Ref: hashicorp/terraform#30937
Ref: hashicorp/terraform-plugin-framework#869
This PR introduces value refinement support to the
tftypes
package. This allows Terraform providers to communicate unknown value refinements to/from Terraform core (Terraform 1.6+).Value refinements are additional constraints that can be applied to unknown values in Terraform that can be used to produce known results from unknown values. For example, with value refinements, providers can now indicate specific attributes will "definitely not be null" during
PlanResourceChange
, which would allow Terraform core to successfully evaluate expressions likecount
during plan, rather than returning an error like:TODOs left on this PR