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

Feature: AtLeastOneOf and ExactlyOneOf validation flags added to schema #225

Merged
merged 15 commits into from
Nov 4, 2019
+614 −30
Merged

Feature: AtLeastOneOf and ExactlyOneOf validation flags added to schema #225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e74c627
Start of AtMostOneOf
mbfrahry Oct 24, 2019
93e6a88
Added tests
mbfrahry Oct 24, 2019
01fa1ea
update error messages
megan07 Oct 24, 2019
dcb54b3
Adding extra validation and AtLeastOneOf
mbfrahry Oct 24, 2019
efe7f1c
AtMost -> Exactly
mbfrahry Oct 25, 2019
17cc721
removed duplicate keys from all keys
megan07 Oct 28, 2019
9a1d70d
fix bug in removeDuplicates
megan07 Oct 30, 2019
d832ec8
Final cleanup
mbfrahry Oct 30, 2019
2dd70af
Merge branch 'f-oneof-mm' of https://github.com/hashicorp/terraform-p…
mbfrahry Oct 30, 2019
990d7a5
better error messages
mbfrahry Oct 30, 2019
b2f6aa3
Add tests around unknown variable values
mbfrahry Nov 4, 2019
8c7dd1a
Add comment
mbfrahry Nov 4, 2019
9780cee
Update CHANGELOG.md
mbfrahry Nov 4, 2019
e81adb3
Merge branch 'master' into f-oneof-mm
mbfrahry Nov 4, 2019
cf5435b
Update CHANGELOG.md
mbfrahry Nov 4, 2019
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
# 1.3.0 (Unreleased)

FEATURES:

* helper/schema: Introduce `ExactlyOneOf` and `AtLeastOneOf` validation checks against shcema attributes ([#225](https://github.com/hashicorp/terraform-plugin-sdk/issues/225))

# 1.2.0 (October 25, 2019)

FEATURES:
Expand Down
182 changes: 152 additions & 30 deletions helper/schema/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,16 @@ type Schema struct {
// This will only check that they're set in the _config_. This will not
// raise an error for a malfunctioning resource that sets a conflicting
// key.
//
// ExactlyOneOf is a set of schema keys that, when set, only one of the
// keys in that list can be specified. It will error if none are
// specified as well.
//
// AtLeastOneOf is a set of schema keys that, when set, at least one of
// the keys in that list must be specified.
ConflictsWith []string
ExactlyOneOf []string
AtLeastOneOf []string

// When Deprecated is set, this attribute is deprecated.
//
Expand Down Expand Up @@ -749,36 +758,32 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro
return fmt.Errorf("%s: ConflictsWith cannot be set with Required", k)
}

if len(v.ConflictsWith) > 0 {
for _, key := range v.ConflictsWith {
parts := strings.Split(key, ".")
sm := topSchemaMap
var target *Schema
for _, part := range parts {
// Skip index fields
if _, err := strconv.Atoi(part); err == nil {
continue
}
if len(v.ExactlyOneOf) > 0 && v.Required {
return fmt.Errorf("%s: ExactlyOneOf cannot be set with Required", k)
}

var ok bool
if target, ok = sm[part]; !ok {
return fmt.Errorf("%s: ConflictsWith references unknown attribute (%s) at part (%s)", k, key, part)
}
if len(v.AtLeastOneOf) > 0 && v.Required {
return fmt.Errorf("%s: AtLeastOneOf cannot be set with Required", k)
}

if subResource, ok := target.Elem.(*Resource); ok {
sm = schemaMap(subResource.Schema)
}
}
if target == nil {
return fmt.Errorf("%s: ConflictsWith cannot find target attribute (%s), sm: %#v", k, key, sm)
}
if target.Required {
return fmt.Errorf("%s: ConflictsWith cannot contain Required attribute (%s)", k, key)
}
if len(v.ConflictsWith) > 0 {
err := checkKeysAgainstSchemaFlags(k, v.ConflictsWith, topSchemaMap)
if err != nil {
return fmt.Errorf("ConflictsWith: %+v", err)
}
}

if len(target.ComputedWhen) > 0 {
return fmt.Errorf("%s: ConflictsWith cannot contain Computed(When) attribute (%s)", k, key)
}
if len(v.ExactlyOneOf) > 0 {
err := checkKeysAgainstSchemaFlags(k, v.ExactlyOneOf, topSchemaMap)
if err != nil {
return fmt.Errorf("ExactlyOneOf: %+v", err)
}
}

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

Expand Down Expand Up @@ -845,6 +850,40 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro
return nil
}

func checkKeysAgainstSchemaFlags(k string, keys []string, topSchemaMap schemaMap) error {
for _, key := range keys {
parts := strings.Split(key, ".")
sm := topSchemaMap
var target *Schema
for _, part := range parts {
// Skip index fields
if _, err := strconv.Atoi(part); err == nil {
continue
}

var ok bool
if target, ok = sm[part]; !ok {
return fmt.Errorf("%s references unknown attribute (%s) at part (%s)", k, key, part)
}

if subResource, ok := target.Elem.(*Resource); ok {
sm = schemaMap(subResource.Schema)
}
}
if target == nil {
return fmt.Errorf("%s cannot find target attribute (%s), sm: %#v", k, key, sm)
}
if target.Required {
return fmt.Errorf("%s cannot contain Required attribute (%s)", k, key)
}

if len(target.ComputedWhen) > 0 {
return fmt.Errorf("%s cannot contain Computed(When) attribute (%s)", k, key)
}
}
return nil
}

func isValidFieldName(name string) bool {
re := regexp.MustCompile("^[a-z0-9_]+$")
return re.MatchString(name)
Expand Down Expand Up @@ -1350,12 +1389,22 @@ func (m schemaMap) validate(
// We're okay as long as we had a value set
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(
"%q: required field is not set", k)}
}

return nil, nil
}

Expand All @@ -1377,7 +1426,7 @@ func (m schemaMap) validate(
return nil, nil
}

err := m.validateConflictingAttributes(k, schema, c)
err = validateConflictingAttributes(k, schema, c)
if err != nil {
return nil, []error{err}
}
Expand Down Expand Up @@ -1407,7 +1456,7 @@ func isWhollyKnown(raw interface{}) bool {
}
return true
}
func (m schemaMap) validateConflictingAttributes(
func validateConflictingAttributes(
k string,
schema *Schema,
c *terraform.ResourceConfig) error {
Expand All @@ -1431,6 +1480,79 @@ func (m schemaMap) validateConflictingAttributes(
return nil
}

func removeDuplicates(elements []string) []string {
encountered := make(map[string]struct{}, 0)
result := []string{}

for v := range elements {
if _, ok := encountered[elements[v]]; !ok {
encountered[elements[v]] = struct{}{}
result = append(result, elements[v])
}
}

return result
}

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

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

allKeys := removeDuplicates(append(schema.ExactlyOneOf, k))
sort.Strings(allKeys)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to sort?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for nicer error messages. These have the potential to stack up on one another so if we sort them, they will line up nicely with one another

specified := make([]string, 0)
unknownVariableValueCount := 0
for _, exactlyOneOfKey := range allKeys {
if raw, ok := c.Get(exactlyOneOfKey); ok {
if raw == hcl2shim.UnknownVariableValue {
// This aims to do a best effort check that at least one value is specified whether
// it's known or not.
unknownVariableValueCount++
continue
}
specified = append(specified, exactlyOneOfKey)
}
}

if len(specified) == 0 && unknownVariableValueCount == 0 {
return fmt.Errorf("%q: one of `%s` must be specified", k, strings.Join(allKeys, ","))
}

if len(specified) > 1 {
return fmt.Errorf("%q: only one of `%s` can be specified, but `%s` were specified.", k, strings.Join(allKeys, ","), strings.Join(specified, ","))
}

return nil
}

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

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

allKeys := removeDuplicates(append(schema.AtLeastOneOf, k))
Copy link
Contributor

@appilon appilon Oct 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

my brain is tingling about this LOC in both functions, but if the tests pass I'm sure it all makes sense.

Copy link
Member Author

@mbfrahry mbfrahry Oct 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This was added to ensure that users coming from the ConflictsWith pattern where you specify every key but the attribute you're in are covered. For example:

"foo": {
  ConflictsWith: ["bar"]
}
"bar": {
  ConflictsWith:["foo"]
}

We wanted users to go a different route where you specify the whole list:

"foo": {
  AtLeastOneOf: ["foo", "bar"]
}
"bar": {
  AtLeastOneOf:["foo", "bar"]
}

but we don't have anyway to enforce that they use this pattern so we add the current attribute key and then remove duplicates to ensure our validation doesn't go sideways

sort.Strings(allKeys)
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above do we need to sort?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing it makes for nicer printout in the error message having the keys being sorted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly!


for _, atLeastOneOfKey := range allKeys {
if _, ok := c.Get(atLeastOneOfKey); ok {
// We can ignore hcl2shim.UnknownVariable by assuming it's been set and additional validation elsewhere
// will uncover this if it is in fact null.
return nil
}
}

return fmt.Errorf("%q: one of `%s` must be specified", k, strings.Join(allKeys, ","))
}

func (m schemaMap) validateList(
k string,
raw interface{},
Expand Down
Loading