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.

Sign up for GitHub

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

Jump to bottom

RequiredWith schema check and changes in validation for optional schema fields #342

Merged
merged 6 commits into from
Apr 14, 2020
Merged

RequiredWith schema check and changes in validation for optional schema fields #342

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
81ae466
Added implementation for AllOf validation and moved validation functi…
Feb 24, 2020
6cc84bf
Added tests for AllOf setting and corrected tests for AtLeastOneOf in…
Feb 24, 2020
0b04412
Corrected formatting of helper/schema/schema.go
Feb 24, 2020
61abb29
Fixed formatting issues in helper/schema/schema_test.go
tmeckel Mar 21, 2020
e193bdb
Renamed AllOf to RequiredWith and made RequiredWith usable with other…
tmeckel Apr 9, 2020
e65a621
Moved validateExactlyOneAttribute and validateAtLeastOneAttribute bac…
tmeckel Apr 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 46 additions & 10 deletions helper/schema/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,9 +223,12 @@ type Schema struct {
//
// AtLeastOneOf is a set of schema keys that, when set, at least one of
// the keys in that list must be specified.
//
// RequiredWith is a set of schema keys that must be set simultaneously.
ConflictsWith []string
ExactlyOneOf []string
AtLeastOneOf []string
RequiredWith []string

// When Deprecated is set, this attribute is deprecated.
//
Expand Down Expand Up @@ -773,6 +776,13 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro
}
}

if len(v.RequiredWith) > 0 {
err := checkKeysAgainstSchemaFlags(k, v.RequiredWith, topSchemaMap)
if err != nil {
return fmt.Errorf("RequiredWith: %+v", err)
}
}

if len(v.ExactlyOneOf) > 0 {
err := checkKeysAgainstSchemaFlags(k, v.ExactlyOneOf, topSchemaMap)
if err != nil {
Expand Down Expand Up @@ -1390,16 +1400,6 @@ func (m schemaMap) validate(
ok = raw != nil
}

err := validateExactlyOneAttribute(k, schema, c)
if err != nil {
return nil, []error{err}
}

err = validateAtLeastOneAttribute(k, schema, c)
if err != nil {
return nil, []error{err}
}

if !ok {
if schema.Required {
return nil, []error{fmt.Errorf(
Expand All @@ -1414,6 +1414,21 @@ func (m schemaMap) validate(
"%q: this field cannot be set", k)}
}

err := validateRequiredWithAttribute(k, schema, c)
if err != nil {
return nil, []error{err}
}

err = validateExactlyOneAttribute(k, schema, c)
Copy link
Member

Choose a reason for hiding this comment

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

The validateExactlyOneAttribute and validateAtLeastOneAttribute do want to be where they were before because they are "Required" in the sense that one of whatever is in that list must be set. But that validation error will never occur since we get out of the function if the key hasn't been set and it's not Required. You can see how that happens from the AtLeastOneOf test that was updated down below.

The RequiredWith validation does make sense here since the keys in that list are only Required when one of keys in that list is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually that's why I initially filed the PR: before the discussion here, it made no sense to me checking anything on an optional property which has no value. That's why my explanation in the chapter Semantic changes in validation for optional schema fields above.

After yours and @appilon's explanation how you guys want to use the various validation "operators" I fully understand why you come up with the request to change the implementation.

But the question remains: how can I define validation inside the schema that a property needs other properties but only if it has a value? If you always validate the property regardless it it has a value or not, how do I implement my scenario?

Copy link
Member

Choose a reason for hiding this comment

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

I noted in the very last sentence of my last comment that you're right that RequiredWith needs to happen right where it is. This is especially true because of the reason you just stated

Does RequiredWith hit all the checks you need now or are we still missing some functionality that warrants another validator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was aware what you said about the position of RequiredWith but my question included also the two other validation functions (validateExactlyOneAttribute and validateAtLeastOneAttribute).

If you leave them at the old position, form my understanding, there's no possibility to model the requirement, that a validation is only performed on an optional attribute when it has a value (or a default value).

I know that this collides with your statements about how you wanna use the AtLeastOne attribute to ensure that at least one of the a set of properties has been set for a set of optional attributes.

So, my question, how to model the requirement, that a validation is only performed when a property has a value? Is this even possible in the current configuration? Do miss something? Especially when I take into account the other options in the schema, like Optional, Required etc.

Copy link
Member

Choose a reason for hiding this comment

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

I need a little more confirmation on what exactly you are looking for. Are you looking for ExactlyOneOf/AtLeastOneOf validation to be only be run when a value has been set? Or are where these checks preventing any form of validation from being run?

If you're looking for ExactlyOneOf functionality for only when an attribute has been set than you should just use ConflictsWith. That ensures that only one value in that set of keys can be set at a time and only is checked when the key in question is set.

AtLeastOneOf validation is a little trickier since its goal is to have at least one attribute in a list of keys to be set. If you only want that validation for when a key is set then the whole point is moot because the key is just Optional at that point.

Do you have a real world example you can share that would better help me see what you're trying to achieve?

Regardless, making changes to ExactlyOneOf/AtLeastOneOf is outside the scope of adding RequiredWith. We should move them back and open an issue on missing functionality so we can track that in a single issue and move forward on getting this piece in since many developers (myself included) could make good use of RequiredWith today.

Copy link
Member

@mbfrahry mbfrahry Apr 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So my proposal would be to add validation functions like AtLeastOneOf or ExactlyOneOf (or both, or with other names) to the Resource and move the tests for AtLeastOneOf or ExactlyOneOf for schema properties after the test if a value is specified.

Moving all the ConflictsWith/ExactlyOneOf/AtLeastOneOf to the Resource level has been on my radar for quite some time actually but time has not been on my side and from the brief time I looked into it, it's non-trivial.

Copy link
Contributor Author

@tmeckel tmeckel Apr 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I apologize if I'm not seeing how all these pieces fit together but I have not looked into the azure devops api and am doing a best effort solution on the information you're providing me.

If I sounded harsh I apologize. It wasn't definitely my intention to disqualify your efforts to bring up alternate solutions. On the contrary, they gave me a deeper insight into how the SDK works. Thanks for the extensive discussion! Highly appreciated.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, much of provider development is still tribal knowledge and learning by doing. I wish this wasn't the case but we do have plans in the future to ease the learning curve.

Happy to discuss further and you can @ me on a PR if you'd like me to review what solution you ultimately end up going with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I guess I'll use the RequiredWith constrained for the current implementation of the DevOps provider. Cheapest way to go and, according to HCL, the easiest to understand for the user of the provider. When do you think the PR will be merged?

Aside I'm happy to know, that I can @ you on the DevOps PR or some other people here when I might file another PR for the SDK or if I've an issue with it. 👍 👍 Always hard to figure out who's in charge for a project on GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that that is the way you should go with RequiredWith!

It's on the sdk team now to get it merged and released but they know it's ready to go!

if err != nil {
return nil, []error{err}
}

err = validateAtLeastOneAttribute(k, schema, c)
if err != nil {
return nil, []error{err}
}

// If the value is unknown then we can't validate it yet.
// In particular, this avoids spurious type errors where downstream
// validation code sees UnknownVariableValue as being just a string.
Expand Down Expand Up @@ -1494,6 +1509,27 @@ func removeDuplicates(elements []string) []string {
return result
}

func validateRequiredWithAttribute(
k string,
schema *Schema,
c *terraform.ResourceConfig) error {

if len(schema.RequiredWith) == 0 {
return nil
}

allKeys := removeDuplicates(append(schema.RequiredWith, k))
sort.Strings(allKeys)

for _, key := range allKeys {
if _, ok := c.Get(key); !ok {
return fmt.Errorf("%q: all of `%s` must be specified", k, strings.Join(allKeys, ","))
}
}

return nil
}

func validateExactlyOneAttribute(
k string,
schema *Schema,
Expand Down
273 changes: 272 additions & 1 deletion helper/schema/schema_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6537,7 +6537,7 @@ func TestValidateAtLeastOneOfAttributes(t *testing.T) {
},

Config: map[string]interface{}{},
Err: true,
Err: false,
tmeckel marked this conversation as resolved.
Show resolved Hide resolved
},

"Only Unknown Variable Value": {
Expand Down Expand Up @@ -6635,3 +6635,274 @@ func TestValidateAtLeastOneOfAttributes(t *testing.T) {
})
}
}

func TestValidateRequiredWithAttributes(t *testing.T) {
cases := map[string]struct {
Key string
Schema map[string]*Schema
Config map[string]interface{}
Err bool
}{

"two attributes specified": {
Key: "whitelist",
Schema: map[string]*Schema{
"whitelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"blacklist"},
},
"blacklist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist"},
},
},

Config: map[string]interface{}{
"whitelist": true,
"blacklist": true,
},
Err: false,
},

"one attributes specified": {
Key: "whitelist",
Schema: map[string]*Schema{
"whitelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"blacklist"},
},
"blacklist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist"},
},
},

Config: map[string]interface{}{
"whitelist": true,
},
Err: true,
},

"no attributes specified": {
Key: "whitelist",
Schema: map[string]*Schema{
"whitelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"blacklist"},
},
"blacklist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist"},
},
},

Config: map[string]interface{}{},
Err: false,
},

"two attributes of three specified": {
Key: "whitelist",
Schema: map[string]*Schema{
"whitelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"purplelist"},
},
"blacklist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "purplelist"},
},
"purplelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist"},
},
},

Config: map[string]interface{}{
"whitelist": true,
"purplelist": true,
},
Err: false,
},

"three attributes of three specified": {
Key: "whitelist",
Schema: map[string]*Schema{
"whitelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"blacklist", "purplelist"},
},
"blacklist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "purplelist"},
},
"purplelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "blacklist"},
},
},

Config: map[string]interface{}{
"whitelist": true,
"purplelist": true,
"blacklist": true,
},
Err: false,
},

"one attributes of three specified": {
Key: "whitelist",
Schema: map[string]*Schema{
"whitelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"blacklist", "purplelist"},
},
"blacklist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "purplelist"},
},
"purplelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "blacklist"},
},
},

Config: map[string]interface{}{
"purplelist": true,
},
Err: true,
},

"no attributes of three specified": {
Key: "whitelist",
Schema: map[string]*Schema{
"whitelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "blacklist", "purplelist"},
},
"blacklist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "blacklist", "purplelist"},
},
"purplelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "blacklist", "purplelist"},
},
},

Config: map[string]interface{}{},
Err: false,
},

"Only Unknown Variable Value": {
Schema: map[string]*Schema{
"whitelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "blacklist", "purplelist"},
},
"blacklist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "blacklist", "purplelist"},
},
"purplelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "blacklist", "purplelist"},
},
},

Config: map[string]interface{}{
"whitelist": hcl2shim.UnknownVariableValue,
},

Err: true,
},

"only unknown list value": {
Schema: map[string]*Schema{
"whitelist": {
Type: TypeList,
Optional: true,
Elem: &Schema{Type: TypeString},
RequiredWith: []string{"whitelist", "blacklist"},
},
"blacklist": {
Type: TypeList,
Optional: true,
Elem: &Schema{Type: TypeString},
RequiredWith: []string{"whitelist", "blacklist"},
},
},

Config: map[string]interface{}{
"whitelist": []interface{}{hcl2shim.UnknownVariableValue},
},

Err: true,
},

"Unknown Variable Value and Known Value": {
Schema: map[string]*Schema{
"whitelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "blacklist", "purplelist"},
},
"blacklist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "blacklist", "purplelist"},
},
"purplelist": {
Type: TypeBool,
Optional: true,
RequiredWith: []string{"whitelist", "blacklist", "purplelist"},
},
},

Config: map[string]interface{}{
"whitelist": hcl2shim.UnknownVariableValue,
"blacklist": true,
},

Err: true,
},
}

for tn, tc := range cases {
t.Run(tn, func(t *testing.T) {
c := terraform.NewResourceConfigRaw(tc.Config)
_, es := schemaMap(tc.Schema).Validate(c)
if len(es) > 0 != tc.Err {
if len(es) == 0 {
t.Fatalf("expected error")
}

for _, e := range es {
t.Fatalf("didn't expect error, got error: %+v", e)
}

t.FailNow()
}
})
}
}