Adding Set element validation for ValuesAre #36

bendbennett · 2022-06-21T09:38:01Z

Closes: #12

detro

Left a bunch of comments, but overall this looks ready.

Once attr.Path is done, a lot of stuff will need rebasing and merging :)

detro · 2022-06-21T13:00:19Z

setvalidator/values_are.go

+		for _, validator := range v.valueValidators {
+			validator.Validate(ctx, request, resp)
+			if resp.Diagnostics.HasError() {
+				return
+			}
+		}


Question: what do you think of the idea of running all .valueValidators against elem before leaving the function?

I don't have a string argument pro/against, but I'm thinking (similarly to ComposeAggregateTestCheckFunc in our testing), getting a report of all the reasons my input is wrong upfront, can be useful.

Of course, it can also be seen as "overwhelming", so I'm on the fence but I thought I'd share my thinking.

For what its worth (and I think its a good idea, but can be part of a later iteration), helper/validation.All(), which was widely used aggregated errors:

https://github.com/hashicorp/terraform-plugin-sdk/blob/ab572994a48576754ec619691e23bd9afdd7042d/helper/validation/meta.go#L32-L43

I'm having a hard time imagining a case where a "sequential" validation must be performed, as if some later validator was panicking, then it should be fixed to not panic. Giving all diagnostics allows practitioners to fix their Terraform configurations knowing all the rules at once, similar to what @detro is mentioning above in testing, rather than needing to repeatedly validate to find the next issue.

Agreed, have amended to run all validators before exiting.

detro · 2022-06-21T13:01:52Z

setvalidator/values_are.go

+		descriptions = append(descriptions, validator.Description(ctx))
+	}
+
+	return fmt.Sprintf("value must satisfy all validations: %s", strings.Join(descriptions, " + "))


Suggested change

return fmt.Sprintf("value must satisfy all validations: %s", strings.Join(descriptions, " + "))

return fmt.Sprintf("Attribute value must satisfy the following: %s", strings.Join(descriptions, " + "))

Just a suggestion.

Think I'm gonna leave this as is if that's ok.

detro · 2022-06-21T13:04:27Z

setvalidator/values_are_test.go

+
+	type testCase struct {
+		val                 attr.Value
+		valuesAreValidators []tfsdk.AttributeValidator


Why not use the actual attribute validator here, instead of hosting only it's input?

Just curious.

Thought I'd follow the same pattern we're using in other validator tests (e.g., int64validator.at_least_test.go).

detro · 2022-06-21T13:06:10Z

validatordiag/diag.go

+func AttributeValueTerraformValueDiagnostic(path *tftypes.AttributePath, description string, value string) diag.Diagnostic {
+	return diag.NewAttributeErrorDiagnostic(
+		path,
+		"Invalid Attribute Value Terraform Value",
+		capitalize(description)+", err: "+value,
+	)
+}


As discussed in our 1-2-1, I think this conversion will be made unnecessary by @bflad work on attr.Path.

Plus I think it will be really confusing for users to see an error that says "we were unable to convert your value in a terraform value". How would they know what to make of it?

Indeed this should go away with the new attribute path work. My recommendation in the meantime would be prefer messaging that says to contact the provider developers, similar to a lot of what is in the framework today (and hopefully going away more and more as part of hashicorp/terraform-plugin-framework#172).

Have updated the message.

bendbennett · 2022-06-22T10:40:56Z

Following discussion with @detro we could consolidate the ValuesAre validators for List, Map and Set into a single generic validator if we don't require any type-specific validation.

bflad

Looks good to me 🚀

bflad · 2022-06-22T12:35:28Z

setvalidator/type_validation_test.go

+			gotSetElems, gotOk := validateSet(context.Background(), testCase.request, &tfsdk.ValidateAttributeResponse{})
+
+			if diff := cmp.Diff(gotSetElems, testCase.expectedSetElems); diff != "" {
+				t.Errorf("unexpected float64 difference: %s", diff)


Nit: Copypasta typo 🍝

bflad · 2022-06-22T12:41:37Z

setvalidator/values_are.go

+		for _, validator := range v.valueValidators {
+			validator.Validate(ctx, request, resp)
+			if resp.Diagnostics.HasError() {
+				return
+			}
+		}


For what its worth (and I think its a good idea, but can be part of a later iteration), helper/validation.All(), which was widely used aggregated errors:

https://github.com/hashicorp/terraform-plugin-sdk/blob/ab572994a48576754ec619691e23bd9afdd7042d/helper/validation/meta.go#L32-L43

I'm having a hard time imagining a case where a "sequential" validation must be performed, as if some later validator was panicking, then it should be fixed to not panic. Giving all diagnostics allows practitioners to fix their Terraform configurations knowing all the rules at once, similar to what @detro is mentioning above in testing, rather than needing to repeatedly validate to find the next issue.

bflad · 2022-06-22T12:46:21Z

validatordiag/diag.go

+func AttributeValueTerraformValueDiagnostic(path *tftypes.AttributePath, description string, value string) diag.Diagnostic {
+	return diag.NewAttributeErrorDiagnostic(
+		path,
+		"Invalid Attribute Value Terraform Value",
+		capitalize(description)+", err: "+value,
+	)
+}


Indeed this should go away with the new attribute path work. My recommendation in the meantime would be prefer messaging that says to contact the provider developers, similar to a lot of what is in the framework today (and hopefully going away more and more as part of hashicorp/terraform-plugin-framework#172).

…ELOG.md (#12)

github-actions · 2022-07-24T02:41:28Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

bendbennett requested a review from a team as a code owner June 21, 2022 09:38

detro approved these changes Jun 21, 2022

View reviewed changes

bflad added this to the v0.3.0 milestone Jun 22, 2022

bflad approved these changes Jun 22, 2022

View reviewed changes

bflad added enhancement New feature or request type/set types.Set validators labels Jun 22, 2022

bendbennett added 7 commits June 22, 2022 17:00

Adding Set element validation for ValuesAre (#12)

0635e3f

Renaming var (#12)

46824be

Rename test (#12)

ec677d4

Fix tests (#12)

651cf20

Updates following code review (#12)

eb25d3f

Adding doc.go and changelog entry (#12)

6759d60

Updating CHANGELOG.md (#12)

d106fc2

bendbennett force-pushed the bendbennett/issues-12 branch from 2013d59 to d106fc2 Compare June 22, 2022 16:00

Rename .changelog file to match PR number and remove updates to CHANG…

4353987

…ELOG.md (#12)

bendbennett merged commit b0eaeea into main Jun 23, 2022

bendbennett deleted the bendbennett/issues-12 branch June 23, 2022 11:33

github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Set element validation for ValuesAre #36

Adding Set element validation for ValuesAre #36

bendbennett commented Jun 21, 2022

detro left a comment

detro Jun 21, 2022

bflad Jun 22, 2022

bendbennett Jun 22, 2022

detro Jun 21, 2022

bendbennett Jun 22, 2022

detro Jun 21, 2022

bendbennett Jun 22, 2022

detro Jun 21, 2022

bflad Jun 22, 2022

bendbennett Jun 22, 2022

bendbennett commented Jun 22, 2022

bflad left a comment

bflad Jun 22, 2022

bflad Jun 22, 2022

bflad Jun 22, 2022

github-actions bot commented Jul 24, 2022

	return fmt.Sprintf("value must satisfy all validations: %s", strings.Join(descriptions, " + "))
	return fmt.Sprintf("Attribute value must satisfy the following: %s", strings.Join(descriptions, " + "))

Adding Set element validation for ValuesAre #36

Adding Set element validation for ValuesAre #36

Conversation

bendbennett commented Jun 21, 2022

detro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendbennett commented Jun 22, 2022

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 24, 2022