From bbbf22d8e4d834ca6c09943b5730eb00a6478826 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 20 Nov 2018 16:46:30 -0800 Subject: [PATCH] configs/configschema: Block.StaticValidateTraversal method This allows basic static validation of a traversal against a schema, to verify that it represents a valid path through the structural parts of the schema. The main purpose of this is to produce better error messages (using our knowledge of the schema) than we'd be able to achieve by just relying on HCL expression evaluation errors. This is particularly important for nested blocks because it may not be obvious whether one is represented internally by a set or a list, and incorrect usage would otherwise produce a confusing HCL-oriented error message. --- configs/configschema/validate_traversal.go | 173 +++++++++++++++ .../configschema/validate_traversal_test.go | 200 ++++++++++++++++++ 2 files changed, 373 insertions(+) create mode 100644 configs/configschema/validate_traversal.go create mode 100644 configs/configschema/validate_traversal_test.go diff --git a/configs/configschema/validate_traversal.go b/configs/configschema/validate_traversal.go new file mode 100644 index 000000000..351d7677e --- /dev/null +++ b/configs/configschema/validate_traversal.go @@ -0,0 +1,173 @@ +package configschema + +import ( + "fmt" + "sort" + + "github.com/hashicorp/hcl2/hcl" + "github.com/hashicorp/hcl2/hcl/hclsyntax" + "github.com/zclconf/go-cty/cty" + + "github.com/hashicorp/terraform/helper/didyoumean" + "github.com/hashicorp/terraform/tfdiags" +) + +// StaticValidateTraversal checks whether the given traversal (which must be +// relative) refers to a construct in the receiving schema, returning error +// diagnostics if any problems are found. +// +// This method is "optimistic" in that it will not return errors for possible +// problems that cannot be detected statically. It is possible that an +// traversal which passed static validation will still fail when evaluated. +func (b *Block) StaticValidateTraversal(traversal hcl.Traversal) tfdiags.Diagnostics { + if !traversal.IsRelative() { + panic("StaticValidateTraversal on absolute traversal") + } + if len(traversal) == 0 { + return nil + } + + var diags tfdiags.Diagnostics + + next := traversal[0] + after := traversal[1:] + + var name string + switch step := next.(type) { + case hcl.TraverseAttr: + name = step.Name + case hcl.TraverseIndex: + // No other traversal step types are allowed directly at a block. + // If it looks like the user was trying to use index syntax to + // access an attribute then we'll produce a specialized message. + key := step.Key + if key.Type() == cty.String && key.IsKnown() && !key.IsNull() { + maybeName := key.AsString() + if hclsyntax.ValidIdentifier(maybeName) { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Invalid index operation`, + Detail: fmt.Sprintf(`Only attribute access is allowed here. Did you mean to access attribute %q using the dot operator?`, maybeName), + Subject: &step.SrcRange, + }) + return diags + } + } + // If it looks like some other kind of index then we'll use a generic error. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Invalid index operation`, + Detail: `Only attribute access is allowed here, using the dot operator.`, + Subject: &step.SrcRange, + }) + return diags + default: + // No other traversal types should appear in a normal valid traversal, + // but we'll handle this with a generic error anyway to be robust. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Invalid operation`, + Detail: `Only attribute access is allowed here, using the dot operator.`, + Subject: next.SourceRange().Ptr(), + }) + return diags + } + + if attrS, exists := b.Attributes[name]; exists { + // For attribute validation we will just apply the rest of the + // traversal to an unknown value of the attribute type and pass + // through HCL's own errors, since we don't want to replicate all of + // HCL's type checking rules here. + val := cty.UnknownVal(attrS.Type) + _, hclDiags := after.TraverseRel(val) + diags = diags.Append(hclDiags) + return diags + } + + if blockS, exists := b.BlockTypes[name]; exists { + moreDiags := blockS.staticValidateTraversal(name, after) + diags = diags.Append(moreDiags) + return diags + } + + // If we get here then the name isn't valid at all. We'll collect up + // all of the names that _are_ valid to use as suggestions. + var suggestions []string + for name := range b.Attributes { + suggestions = append(suggestions, name) + } + for name := range b.BlockTypes { + suggestions = append(suggestions, name) + } + sort.Strings(suggestions) + suggestion := didyoumean.NameSuggestion(name, suggestions) + if suggestion != "" { + suggestion = fmt.Sprintf(" Did you mean %q?", suggestion) + } + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Unsupported attribute`, + Detail: fmt.Sprintf(`This object has no argument, nested block, or exported attribute named %q.%s`, name, suggestion), + Subject: next.SourceRange().Ptr(), + }) + + return diags +} + +func (b *NestedBlock) staticValidateTraversal(typeName string, traversal hcl.Traversal) tfdiags.Diagnostics { + if b.Nesting == NestingSingle { + // Single blocks are easy: just pass right through. + return b.Block.StaticValidateTraversal(traversal) + } + + if len(traversal) == 0 { + // It's always valid to access a nested block's attribute directly. + return nil + } + + var diags tfdiags.Diagnostics + next := traversal[0] + after := traversal[1:] + + switch b.Nesting { + + case NestingSet: + // Can't traverse into a set at all, since it does not have any keys + // to index with. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Cannot index a set value`, + Detail: fmt.Sprintf(`Block type %q is represented by a set of objects, and set elements do not have addressable keys. To find elements matching specific criteria, use a "for" expression with an "if" clause.`, typeName), + Subject: next.SourceRange().Ptr(), + }) + return diags + + case NestingList: + if _, ok := next.(hcl.TraverseIndex); ok { + moreDiags := b.Block.StaticValidateTraversal(after) + diags = diags.Append(moreDiags) + } else { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Invalid operation`, + Detail: fmt.Sprintf(`Block type %q is represented by a list of objects, so it must be indexed using a numeric key, like .%s[0].`, typeName, typeName), + Subject: next.SourceRange().Ptr(), + }) + } + return diags + + case NestingMap: + // Both attribute and index steps are valid for maps, so we'll just + // pass through here and let normal evaluation catch an + // incorrectly-typed index key later, if present. + moreDiags := b.Block.StaticValidateTraversal(after) + diags = diags.Append(moreDiags) + return diags + + default: + // Invalid nesting type is just ignored. It's checked by + // InternalValidate. (Note that we handled NestingSingle separately + // back at the start of this function.) + return nil + } +} diff --git a/configs/configschema/validate_traversal_test.go b/configs/configschema/validate_traversal_test.go new file mode 100644 index 000000000..19adaa191 --- /dev/null +++ b/configs/configschema/validate_traversal_test.go @@ -0,0 +1,200 @@ +package configschema + +import ( + "testing" + + "github.com/hashicorp/hcl2/hcl" + "github.com/hashicorp/hcl2/hcl/hclsyntax" + "github.com/zclconf/go-cty/cty" +) + +func TestStaticValidateTraversal(t *testing.T) { + attrs := map[string]*Attribute{ + "str": {Type: cty.String, Optional: true}, + "list": {Type: cty.List(cty.String), Optional: true}, + "dyn": {Type: cty.DynamicPseudoType, Optional: true}, + } + schema := &Block{ + Attributes: attrs, + BlockTypes: map[string]*NestedBlock{ + "single_block": { + Nesting: NestingSingle, + Block: Block{ + Attributes: attrs, + }, + }, + "list_block": { + Nesting: NestingList, + Block: Block{ + Attributes: attrs, + }, + }, + "set_block": { + Nesting: NestingSet, + Block: Block{ + Attributes: attrs, + }, + }, + "map_block": { + Nesting: NestingMap, + Block: Block{ + Attributes: attrs, + }, + }, + }, + } + + tests := []struct { + Traversal string + WantError string + }{ + { + `obj`, + ``, + }, + { + `obj.str`, + ``, + }, + { + `obj.str.nonexist`, + `Unsupported attribute: This value does not have any attributes.`, + }, + { + `obj.list`, + ``, + }, + { + `obj.list[0]`, + ``, + }, + { + `obj.list.nonexist`, + `Unsupported attribute: This value does not have any attributes.`, + }, + { + `obj.dyn`, + ``, + }, + { + `obj.dyn.anything_goes`, + ``, + }, + { + `obj.dyn[0]`, + ``, + }, + { + `obj.nonexist`, + `Unsupported attribute: This object has no argument, nested block, or exported attribute named "nonexist".`, + }, + { + `obj[1]`, + `Invalid index operation: Only attribute access is allowed here, using the dot operator.`, + }, + { + `obj["str"]`, // we require attribute access for the first step to avoid ambiguity with resource instance indices + `Invalid index operation: Only attribute access is allowed here. Did you mean to access attribute "str" using the dot operator?`, + }, + { + `obj.atr`, + `Unsupported attribute: This object has no argument, nested block, or exported attribute named "atr". Did you mean "str"?`, + }, + { + `obj.single_block`, + ``, + }, + { + `obj.single_block.str`, + ``, + }, + { + `obj.single_block.nonexist`, + `Unsupported attribute: This object has no argument, nested block, or exported attribute named "nonexist".`, + }, + { + `obj.list_block`, + ``, + }, + { + `obj.list_block[0]`, + ``, + }, + { + `obj.list_block[0].str`, + ``, + }, + { + `obj.list_block[0].nonexist`, + `Unsupported attribute: This object has no argument, nested block, or exported attribute named "nonexist".`, + }, + { + `obj.list_block.str`, + `Invalid operation: Block type "list_block" is represented by a list of objects, so it must be indexed using a numeric key, like .list_block[0].`, + }, + { + `obj.set_block`, + ``, + }, + { + `obj.set_block[0]`, + `Cannot index a set value: Block type "set_block" is represented by a set of objects, and set elements do not have addressable keys. To find elements matching specific criteria, use a "for" expression with an "if" clause.`, + }, + { + `obj.set_block.str`, + `Cannot index a set value: Block type "set_block" is represented by a set of objects, and set elements do not have addressable keys. To find elements matching specific criteria, use a "for" expression with an "if" clause.`, + }, + { + `obj.map_block`, + ``, + }, + { + `obj.map_block.anything`, + ``, + }, + { + `obj.map_block["anything"]`, + ``, + }, + { + `obj.map_block.anything.str`, + ``, + }, + { + `obj.map_block["anything"].str`, + ``, + }, + { + `obj.map_block.anything.nonexist`, + `Unsupported attribute: This object has no argument, nested block, or exported attribute named "nonexist".`, + }, + } + + for _, test := range tests { + t.Run(test.Traversal, func(t *testing.T) { + traversal, parseDiags := hclsyntax.ParseTraversalAbs([]byte(test.Traversal), "", hcl.Pos{Line: 1, Column: 1}) + for _, diag := range parseDiags { + t.Error(diag.Error()) + } + + // We trim the "obj." portion from the front since StaticValidateTraversal + // only works with relative traversals. + traversal = traversal[1:] + + diags := schema.StaticValidateTraversal(traversal) + if test.WantError == "" { + if diags.HasErrors() { + t.Errorf("unexpected error: %s", diags.Err().Error()) + } + } else { + if diags.HasErrors() { + if got := diags.Err().Error(); got != test.WantError { + t.Errorf("wrong error\ngot: %s\nwant: %s", got, test.WantError) + } + } else { + t.Errorf("wrong error\ngot: \nwant: %s", test.WantError) + } + } + }) + } +}