Adding Any/Or Generic Validator by bendbennett · Pull Request #43 · hashicorp/terraform-plugin-framework-validators

bendbennett · 2022-06-23T10:31:47Z

Closes: #28

As mentioned in #28 the implementation in this PR results in the following: "This has at least one particular quirk which may be problematic in that it could swallow unrecoverable errors, such as implementing an incorrect type validator intermixed with any valid one.". This seems acceptable given that the purpose of the Any validator is to determine whether at least one of the validators validates against the attribute but happy to discuss.

detro

I think I spotted a small issue with the handling of Any() and Warnings.

I have also left some suggestions around godoc for this package.

detro · 2022-06-23T15:44:39Z

+	for k, validator := range v.valueValidators {
+		validator.Validate(ctx, req, resp)
+		if k+1 > len(resp.Diagnostics) {
+			resp.Diagnostics = []diag.Diagnostic{}
+			return
+		}
+	}


Mmm, I think this might be a bit "brutal", in the sense that we swallow any kind of Diagnostic, even the ones that are Warning and not errors.

Maybe you can make use of helpers/validatordiag.ErrorsCount and helpers/validatordiag.WarningCount here?

Maybe a different way to go about this would be to feed a tailor made, empty Response in each validator, and return the first time response.Diagnostics.HasError() == false.

Ah, and of course the Diagnostics would need to be accumulated so that, in case we reach the bottom of the loop, we then want to attach the sum of all those Diagnostics to the original Response.

Have refactored to use your suggestion of response.Diagnostics.HasError() == false.
Diagnostics are accumulated and returned if none of the validators return without an error.

In terms of avoiding swallowing warnings, currently, it doesn't look like it's possible to selectively pull warnings out of diagnostics with something like response.Diagnostics.GetWarnings(). We could decorate diagnostics to make this possible or, modify diag.Diagnostics to add that function, for instance:

func (diags Diagnostics) GetWarnings() Diagnostics { var d Diagnostics for _, diag := range diags { if diag.Severity() == SeverityWarning { d = append(d, diag) } } return d }

Thoughts?

Yeah, that's why I added this to validatordiag:

// ErrorsCount returns the amount of diag.Diagnostic in diag.Diagnostics that are diag.SeverityError. func ErrorsCount(diags diag.Diagnostics) int { count := 0 for _, d := range diags { if diag.SeverityError == d.Severity() { count++ } } return count } // WarningsCount returns the amount of diag.Diagnostic in diag.Diagnostics that are diag.SeverityWarning. func WarningsCount(diags diag.Diagnostics) int { count := 0 for _, d := range diags { if diag.SeverityWarning == d.Severity() { count++ } } return count }

But I'd be keen to have it added to the Diagnostics type directly in the FW. Would be even better and more aptly located.

Mmm, but I think I see what you are trying to say. Counting != getting them extracted. Sorry, I was being dumb.

Co-authored-by: Ivan De Marino <ivan.demarino@hashicorp.com>

detro

LGTM, and if you would like to throw together a little PR to add those Warnings and Errors extractor from Diagnostics, I'd be happy to review that. Feels like a useful addition.

bflad · 2022-06-27T14:44:02Z

+// If the number of iterations (i.e., k + 1) is greater than the number of diagnostics in the response then
+// at least one of the validations has passed and, we return without any diagnostics.


Does this apply for the allValidator?

Docs needed updating following code changes. Fixed.

bflad · 2022-06-27T14:45:38Z

@@ -0,0 +1,69 @@
+package metavalidator


It looks like a few of the other packages were recently switched to the _test testing package convention so they can verify things via only exported functionality. Not sure if this should be updated as well.

Have moved the tests to their own metavalidator_test pkg.

bflad · 2022-06-27T14:48:33Z

+
+		validator.Validate(ctx, req, validatorResp)
+		if !validatorResp.Diagnostics.HasError() {
+			resp.Diagnostics = []diag.Diagnostic{}


Rather than returning empty diagnostics, it feels appropriate to return the validatorResp.Diagnostics here in case there are warning diagnostics with the passing validator.

Suggested change

resp.Diagnostics = []diag.Diagnostic{}

// Ensure any warning diagnostics from the passing validator are returned

resp.Diagnostics = validatorResp.Diagnostics

Believe we need hashicorp/terraform-plugin-framework#392 so that we can extract the warnings from diagnostics and return them. Otherwise if we're using Any() with All() it's possible that we'd have errors from a previous iteration that would be returned, such as the following:

Any( All(validator1, validator2, validator3), All(validator4, validator5,validator6), )

Have just range(d) over the diagnostics and inspected the Severity in order to extract warning diags.

My personal semantics opinion here is that given the above A || B, where A may contain conflicting logic than B and where B passes, that returning any warnings from A could potentially be confusing for practitioners. If provider developers want the ability to also return all warnings from other logical branches, then we could either introduce an "options" type parameter to allow triggering the variant behavior, or create a separate variant validator.

If we did want to return warnings from other logical branches, then the logic would need to fully remove any early return behavior and use a variable for tracking whether any given validator was "passing", performing the warnings extraction at the end of function based on that tracking variable to remove error diagnostics to prevent any sort of validator ordering semantics within Any().

Have created a separate AnyWithAllWarningsValidator but more than happy to change the name if something else would suit better.

bflad · 2022-06-27T14:50:24Z

+	"github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator"
+)
+
+func TestAnyValidator(t *testing.T) {


To ensure behaviors with warning diagnostics, we might need to create a testing validator that always returns a warning (but no error) diagnostic. Then the unit testing can verify (e.g. via cmp.Diff the returned diagnostics) that a passing validator with a warning diagnostic has its warning still returned.

~~Believe we need hashicorp/terraform-plugin-framework#392 so that we can extract the warnings from diagnostics and return them.~~ Have just range(d) over the diagnostics and inspected the Severity in order to extract warning diags.

Have created a testing validator to exercise the case you describe @bflad.

bendbennett · 2022-06-28T08:57:16Z

+		if !validatorResp.Diagnostics.HasError() {
+			diagWarnings := diag.Diagnostics{}
+
+			for _, d := range resp.Diagnostics {


This change will return all of the accumulated warning diagnostics from previously failed validations along with any warning from the validator that passes as discussed - "For the Warning for Any, I think it’s non desirable to swallow those".

Have created a separate AnyWithAllWarningsValidator but more than happy to change the name if something else would suit better.

detro

I had written my comments 3h ago and forgot to push submit 🤦

detro · 2022-06-28T12:08:39Z

+		if !validatorResp.Diagnostics.HasError() {
+			resp.Diagnostics = validatorResp.Diagnostics
+			return
+		}


Doesn't this also means that, any previous validator warnings are swollen?

Yes. There's a newly added AnyWithAllWarningsValidator that will accumulate all errors and warnings from all validators and return all warnings from all validators if any of the validators pass.

detro · 2022-06-28T12:11:11Z

+		diagWarnings := diag.Diagnostics{}
+
+		for _, d := range resp.Diagnostics {
+			if d.Severity() == diag.SeverityWarning {
+				diagWarnings.Append(d)
+			}
+		}
+
+		resp.Diagnostics = diagWarnings
+		return


Suggested change

diagWarnings := diag.Diagnostics{}

for _, d := range resp.Diagnostics {

if d.Severity() == diag.SeverityWarning {

diagWarnings.Append(d)

}

}

resp.Diagnostics = diagWarnings

return

resp.Diagnostics = resp.Diagnostics.Warnings()

😉

I have also noticed that the return wasn't really necessary there, given it's the end of the function.

I left that in, in case @bflad had any further comments. ~~Will refactor once terraform-plugin-framework containing the convenience functions has been tagged.~~ Have gone ahead and pulled the sha containing the convenience functions and refactored.

Also refactored all of the tests to use path.Root() rather than tftypes.NewAttributePath().WithAttributeName().

detro · 2022-06-28T12:17:20Z

+// If the diagnostics returned from the validator do not contain an error we return any warning diagnostics from the
+// passing validator.


The mention of "from the passing validator", kinda hints at how it works internally without being super explicit.

What if it explicitly mentions something like "it passes once it reaches a validator that returns no errors"? This way the sentence "we return any warning diagnostics from the passing validator" has more context.

I've expanded the docs a bit. See what you think.

detro · 2022-06-28T12:18:47Z

+	return v.Description(ctx)
+}
+
+// Validate performs the validation.


This is sort of complementary to the doc of Any(), and I think it would be useful if one mentions the other, and it explains this whole "sequencing" that both validators do.

I have just read also the doc you added to each of the wrapper functions. I think my argument is less strong, as in there (that is ultimately the public entry point), the docs is pretty clear about what those validators do internally.

👍

Again, have expanded the docs.

…e path.Root() to replace tftypes.NewAttributePath().WithAttrrtibuteName() (#28)

detro

🚀

bflad

A few minor things and pending terraform-plugin-framework v0.10.0 release, but otherwise looks good to me 🚀 Excellent work.

bflad · 2022-06-29T19:00:55Z

 require (
 	github.com/google/go-cmp v0.5.8
-	github.com/hashicorp/terraform-plugin-framework v0.9.0
+	github.com/hashicorp/terraform-plugin-framework v0.9.1-0.20220627174514-5a338a7dd906


Just putting a note here so we either rebase this after the dependabot PR for v0.10.0 or update this PR for v0.10.0 when its released.

bflad · 2022-06-29T19:07:50Z

+				stringvalidator.LengthAtLeast(5),
+			},
+			expectError: true,
+			// We can't test the diags returned as they are in the /internal/reflect pkg.


Aye this has bitten me in the past as well -- I think we should introduce an Equal() method on diag.Diagnostics so go-cmp can use the underlying (diag.Diagnostic).Equal() equality checking, rather than it needing to be Go type specific. I'll raise an issue upstream.

bflad · 2022-06-29T19:21:59Z

@@ -0,0 +1,47 @@
+package validatordiag


I believe this file shouldn't be recreated in preference of the new helper/validatordiag/diag.go 👍

Co-authored-by: Brian Flad <bflad417@gmail.com>

github-actions · 2022-08-05T02:40:49Z

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 added 4 commits June 23, 2022 10:28

Adding Any validator (#28)

c9b6e4a

Adding All validator for use with Any validator (#28)

6b5b1d2

Adding .changelog file (#28)

899ed35

Rewording test (#28)

e6f0d72

bendbennett added this to the v0.3.0 milestone Jun 23, 2022

bendbennett marked this pull request as ready for review June 23, 2022 10:36

bendbennett requested a review from a team as a code owner June 23, 2022 10:36

bendbennett added the enhancement New feature or request label Jun 23, 2022

detro reviewed Jun 23, 2022

View reviewed changes

bendbennett and others added 2 commits June 24, 2022 09:18

Apply suggestions from code review

9d3f44b

Co-authored-by: Ivan De Marino <ivan.demarino@hashicorp.com>

Checking diagnostics for error with each validator (#28)

a4a0b46

bendbennett force-pushed the bendbennett/issues-28 branch from 609dcac to a4a0b46 Compare June 24, 2022 08:25

detro approved these changes Jun 24, 2022

View reviewed changes

bendbennett mentioned this pull request Jun 27, 2022

Add convenience functions for diagnostics ErrorsCount, WarningsCount, Errors and Warnings hashicorp/terraform-plugin-framework#391

Closed

bflad reviewed Jun 27, 2022

View reviewed changes

bendbennett added 2 commits June 27, 2022 17:09

Updated following code review (#28)

ea34895

Returning all accumulated warnings when Any() validation passes (#28)

5fee37f

bendbennett commented Jun 28, 2022

View reviewed changes

bendbennett requested review from bflad and detro June 28, 2022 09:00

bendbennett added 3 commits June 28, 2022 10:01

Linting (#28)

61189e8

Adding AnyWithAllWarningsValidator (#28)

dc9ea85

Updating CHANGELOG and formatting (#28)

8b2420e

detro approved these changes Jun 28, 2022

View reviewed changes

bendbennett added 2 commits June 29, 2022 09:00

Refactoring to use diags.Warnings convenience func and updating to us…

9740423

…e path.Root() to replace tftypes.NewAttributePath().WithAttrrtibuteName() (#28)

Updating docs (#28)

5629801

bendbennett requested a review from detro June 29, 2022 08:15

Merge remote-tracking branch 'origin/main' into bendbennett/issues-28

8641bee

detro approved these changes Jun 29, 2022

View reviewed changes

bflad removed this from the v0.3.0 milestone Jun 29, 2022

bflad added this to the v0.4.0 milestone Jun 29, 2022

bflad approved these changes Jun 29, 2022

View reviewed changes

bendbennett and others added 4 commits June 30, 2022 09:41

Apply suggestions from code review

5ec8fa1

Co-authored-by: Brian Flad <bflad417@gmail.com>

Removing unneeded diag file (#28)

bef92ae

Adding tests for type mismatch (#28)

b5932e2

Removing duplicated interface check (#28)

e1d7670

bendbennett merged commit c9b8b39 into main Jul 5, 2022

bendbennett deleted the bendbennett/issues-28 branch July 5, 2022 15:31

github-actions Bot locked as resolved and limited conversation to collaborators Aug 5, 2022

		// If the number of iterations (i.e., k + 1) is greater than the number of diagnostics in the response then
		// at least one of the validations has passed and, we return without any diagnostics.

	resp.Diagnostics = []diag.Diagnostic{}
	// Ensure any warning diagnostics from the passing validator are returned
	resp.Diagnostics = validatorResp.Diagnostics

		// If the diagnostics returned from the validator do not contain an error we return any warning diagnostics from the
		// passing validator.

Conversation

bendbennett commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

detro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

detro Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

detro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendbennett Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendbennett Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendbennett Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

detro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendbennett Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bendbennett commented Jun 23, 2022 •

edited

Loading

detro Jun 24, 2022 •

edited

Loading

bendbennett Jun 27, 2022 •

edited

Loading

bendbennett Jun 27, 2022 •

edited

Loading

bendbennett Jun 28, 2022 •

edited

Loading

bendbennett Jun 29, 2022 •

edited

Loading