Adding `AtLeastSumOf`, `AtMostSumOf`, `EqualToSumOf` Validators #29

bendbennett · 2022-05-27T15:59:01Z

Closes: #20

Couple of things:

If the attributes that are being summed are null this validator silently ignores that fact.
If all of the attributes that are being summed are unknown, no errors are emitted as it's not possible to perform the validation.
~~If this looks like it might be useful, let me know and I'll add atMostSumOf and any other permutations that you think are appropriate into the same PR.~~ Have added atLeastSumOf and equalToSumOf.

bflad · 2022-05-27T17:18:52Z

int64validator/at_least_sum_of_test.go

+			requestConfigRaw: map[string]tftypes.Value{
+				"one": tftypes.NewValue(tftypes.Number, 15),
+				"two": tftypes.NewValue(tftypes.Number, 15),
+			},


Drive-by note: It'd be great to ensure there are unit tests covering when the one/two attributes are null and/or unknown as well. Or all three are null/unknown.

Have added some additional tests.

These look great! Another good test case might be verifying that giving a path to an unexpected type (e.g. boolean attribute) always returns an error diagnostic. It appears the code should already be doing this, but it might be good to cover this for future code changes. Seems like it should be popping an additional attribute into the test schema, then referencing it in a new test case. 👍

Have added a test case for attributePath(s) referencing unexpected types in at_least|at_most|equal_to_sum_of_test.

detro

I think this is looking already good. I'd just include more testing for unknowns/nulls scenarios (what @bflad said) and I'd offer these validators in a set:

atLeastSumOf (i.e. Attr >= Sum)
atMostSumOf (i.e. Attr <= Sum)
sumOf (i.e. Attr = Sum)

detro

👩‍🍳 💋

bflad

Getting pumped for this. Another thing we may want to consider, similar to #32, is whether we should wait for hashicorp/terraform-plugin-framework#81 to land and release upstream, as it will necessitate a breaking change for anything accepting attribute paths (or hopefully attribute path "expressions" in this use case).

bflad · 2022-06-07T19:37:32Z

int64validator/at_least_sum_of.go

+			return
+		}
+
+		sumOfAttribs += attribToSum.Value


For code reader clarity, I'm not sure if its worth explicitly putting a conditional such as the below or a quick code comment why Value is safe to always add (because its zero-value is 0)

if attribToSum.IsNull() || attribToSum.IsUnknown() { continue } sumOfAttribs += attribToSum.Value

Per #29 (comment), it may be better to implement something like:

if attribToSum.IsNull() { continue } if attribToSum.IsUnknown() { return } sumOfAttribs += attribToSum.Value

Have added continue statements for Null and Unknown to at_least|at_most|equal_to_sum.

bflad · 2022-06-07T19:48:44Z

int64validator/at_least_sum_of.go

+//     - Is exclusively at least the sum of the given attributes.
+//
+// Null (unconfigured) and unknown (known after apply) values are skipped.
+func AtLeastSumOf(attributesToSum []*tftypes.AttributePath) tfsdk.AttributeValidator {


Something we should probably decide on as a group is a common parameter style for "multiple attribute"/"combination"/"schema" validators -- I think I lean towards preferring a variadic style so consumers can omit the wrapping slice type syntax (e.g. []*tftypes.AttributePath{}) and instead just list out the actual slice values, e.g.

// []*tftypes.AttributePath parameter int64validator.AtLeastSumOf([]*tftypes.AttributePath{ tftypes.NewAttributePath().WithAttributeName("one"), tftypes.NewAttributePath().WithAttributeName("two"), }) // ...*tftypes.AttributePath parameter int64validator.AtLeastSumOf( tftypes.NewAttributePath().WithAttributeName("one"), tftypes.NewAttributePath().WithAttributeName("two"), )

If for some reason they already have a slice, then it can be expanded, e.g.

var mySpecialAttributes []*tftypes.AttributePath = []*tftypes.AttributePath{/*...*/} int64validator.AtLeastSumOf(mySpecialAttributes...)

Agreed. Have amended accordingly.

bflad · 2022-06-07T20:10:51Z

int64validator/at_least_sum_of_test.go

+			requestConfigRaw: map[string]tftypes.Value{
+				"one": tftypes.NewValue(tftypes.Number, 15),
+				"two": tftypes.NewValue(tftypes.Number, 15),
+			},


These look great! Another good test case might be verifying that giving a path to an unexpected type (e.g. boolean attribute) always returns an error diagnostic. It appears the code should already be doing this, but it might be good to cover this for future code changes. Seems like it should be popping an additional attribute into the test schema, then referencing it in a new test case. 👍

bflad · 2022-06-07T20:13:12Z

int64validator/equal_to_sum_of.go

+// attribute value:
+//
+//     - Is a number, which can be represented by a 64-bit integer.
+//     - Is exclusively equal to the sum of the given attributes.


Nit: Does the word "exclusively" apply here? 😄

int64validator/at_most_sum_of_test.go

int64validator/equal_to_sum_of.go

bflad · 2022-06-22T12:54:08Z

FYI, moving this to v0.4.0 so we can release v0.3.0 with additional validators not dependent on hashicorp/terraform-plugin-framework#81, upgrade the framework dependency when its released, then focus immediately on these.

detro

I think you need to make use of path.Expressions here, instead of pure Paths. I left some details to why in the comment, but I'll also have an example of how I'm approaching this once I finish reworking the other PR.

int64validator/at_least_sum_of.go

int64validator/at_most_sum_of.go

int64validator/equal_to_sum_of.go

…f and EqualToSumOf validators (#20)

… EqualToSumOf validators (#20)

detro

The update to use the new .PathMatches looks great, but I think we need to iron out what's the best behaviour for unknowns, when you are essentially trying to sum stuff up, to meet the validator's target.

Good thing we have a zoom in 15min.

int64validator/at_least_sum_of.go

…` method

github-actions · 2022-08-20T02:39:37Z

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 May 27, 2022 15:59

bendbennett force-pushed the bendbennett/issues-20 branch from a11d52a to a56e62f Compare May 27, 2022 16:01

bflad reviewed May 27, 2022

View reviewed changes

detro reviewed May 30, 2022

View reviewed changes

bendbennett requested review from detro and bflad May 30, 2022 13:02

bendbennett force-pushed the bendbennett/issues-20 branch from 5937dfd to 49cdefb Compare May 30, 2022 13:27

detro approved these changes May 31, 2022

View reviewed changes

detro modified the milestones: v0.2.0, v0.3.0 May 31, 2022

bflad reviewed Jun 7, 2022

View reviewed changes

bflad mentioned this pull request Jun 7, 2022

Introducing schemavalidator package #32

Merged

bendbennett requested review from bflad and detro June 9, 2022 14:57

bflad added the enhancement New feature or request label Jun 22, 2022

bflad modified the milestones: v0.3.0, v0.4.0 Jun 22, 2022

bendbennett changed the title ~~Adding atLeastSumOfValidator~~ Adding AtLeastSumOf, AtMostSumOf, EqualToSumOf` Validators Jun 27, 2022

bendbennett changed the title ~~Adding AtLeastSumOf, AtMostSumOf, EqualToSumOf` Validators~~ Adding AtLeastSumOf, AtMostSumOf, EqualToSumOf Validators Jun 27, 2022

bendbennett force-pushed the bendbennett/issues-20 branch from 1ac220c to de23465 Compare July 6, 2022 09:24

detro reviewed Jul 6, 2022

View reviewed changes

int64validator/at_least_sum_of.go Outdated Show resolved Hide resolved

int64validator/at_most_sum_of.go Outdated Show resolved Hide resolved

int64validator/equal_to_sum_of.go Outdated Show resolved Hide resolved

bendbennett added 4 commits July 13, 2022 10:32

Adding atLeastSumOf, atMostSumOf and equalToSumOf int64 validators (#20)

9b4e4c0

Checking for summing of unknown attributes and using variadic args (#20)

4e3474b

Switching to using native framework path for AtLeastSumOf, AtMostSumO…

533f2df

…f and EqualToSumOf validators (#20)

Switching to using path expressions for AtLeastSumOf, AtMostSumOf and…

f37d1cb

… EqualToSumOf validators (#20)

bendbennett force-pushed the bendbennett/issues-20 branch from de23465 to f37d1cb Compare July 13, 2022 10:33

bendbennett requested a review from detro July 13, 2022 10:35

detro reviewed Jul 13, 2022

View reviewed changes

int64validator/at_least_sum_of.go Outdated Show resolved Hide resolved

Do not validate if any attributes are unknown (#20)

1f5151c

bflad reviewed Jul 13, 2022

View reviewed changes

int64validator/at_least_sum_of.go Outdated Show resolved Hide resolved

int64validator/at_least_sum_of.go Outdated Show resolved Hide resolved

int64validator/at_least_sum_of.go Outdated Show resolved Hide resolved

Ivan De Marino added 3 commits July 19, 2022 11:11

Updating dependencies, including [email protected]

aabdb17

PR review: making use of the new path.Expression `.MergeExpressions…

df358cd

…` method

Preparing CHANGELOG entry

66aea58

detro requested a review from bflad July 19, 2022 10:40

Rely on 'tfsdk.ValueAs' to do type validation

9c2061d

bflad mentioned this pull request Jul 19, 2022

Bump github.com/hashicorp/terraform-plugin-go from 0.11.0 to 0.12.0 #49

Closed

Ivan De Marino added 2 commits July 20, 2022 09:31

PR review

9fb4d53

Updated changelog entry to match other entires

5463f13

detro merged commit 3ef7858 into main Jul 20, 2022

detro deleted the bendbennett/issues-20 branch July 20, 2022 15:19

lostick mentioned this pull request Jul 23, 2022

Leverage AtLeastSumOf validator & deprecate tftypes.NewAttributePath hashicorp/terraform-provider-random#283

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2022

Adding AtLeastSumOf, AtMostSumOf, EqualToSumOf Validators #29

Adding AtLeastSumOf, AtMostSumOf, EqualToSumOf Validators #29

Uh oh!

Conversation

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

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!

detro left a comment • 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!

detro left a comment

Choose a reason for hiding this comment

Uh oh!

bflad left a comment

Choose a reason for hiding this comment

Uh oh!

bflad Jun 7, 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!

bflad Jun 7, 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bflad commented Jun 22, 2022

Uh oh!

detro left a comment

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!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Aug 20, 2022

Uh oh!

Uh oh!

Adding `AtLeastSumOf`, `AtMostSumOf`, `EqualToSumOf` Validators #29

Adding `AtLeastSumOf`, `AtMostSumOf`, `EqualToSumOf` Validators #29

bendbennett commented May 27, 2022 •

edited

Loading

detro left a comment •

edited

Loading

bflad Jun 7, 2022 •

edited

Loading

bflad Jun 7, 2022 •

edited

Loading