From e30967585373d35f8fd57ec3ea9bb599235df6b0 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 15 Mar 2018 16:02:23 -0700 Subject: [PATCH] tfdiags: Contextual diagnostics The usual usage of diagnostics requires us to pass around source location information to everywhere that might generate a diagnostic, and that is always the best way to get the most precise diagnostic source locations. However, it's impractical to require source location information to be retained in every Terraform subsystem, and so this new idea of "contextual diagnostics" allows us to separate the generation of a diagnostic from the resolution of its source location, instead resolving the location information as a post-processing step once the call stack unwinds to a place where there is enough context to find it. This is necessarily a less precise approach than reading the source ranges directly from the configuration AST, but gives us an alternative to no diagnostics at all in portions of Terraform where full location information is not available. This is a best-effort sort of thing which will get as precise as it can but may return a range in a parent block if the precise location of a particular attribute cannot be found. Diagnostics that rely on this mechanism should include some other contextual information in the detail message to make up for any loss of precision that results. --- tfdiags/contextual.go | 263 +++++++++++++++++++++++++++++++++++++ tfdiags/contextual_test.go | 139 ++++++++++++++++++++ tfdiags/diagnostic_base.go | 27 ++++ 3 files changed, 429 insertions(+) create mode 100644 tfdiags/contextual.go create mode 100644 tfdiags/contextual_test.go create mode 100644 tfdiags/diagnostic_base.go diff --git a/tfdiags/contextual.go b/tfdiags/contextual.go new file mode 100644 index 000000000..de392c54b --- /dev/null +++ b/tfdiags/contextual.go @@ -0,0 +1,263 @@ +package tfdiags + +import ( + "github.com/hashicorp/hcl2/hcl" + "github.com/zclconf/go-cty/cty" + "github.com/zclconf/go-cty/cty/gocty" +) + +// The "contextual" family of diagnostics are designed to allow separating +// the detection of a problem from placing that problem in context. For +// example, some code that is validating an object extracted from configuration +// may not have access to the configuration that generated it, but can still +// report problems within that object which the caller can then place in +// context by calling IsConfigBody on the returned diagnostics. +// +// When contextual diagnostics are used, the documentation for a method must +// be very explicit about what context is implied for any diagnostics returned, +// to help ensure the expected result. + +// contextualFromConfig is an interface type implemented by diagnostic types +// that can elaborate themselves when given information about the configuration +// body they are embedded in. +// +// Usually this entails extracting source location information in order to +// populate the "Subject" range. +type contextualFromConfigBody interface { + ElaborateFromConfigBody(hcl.Body) Diagnostic +} + +// InConfigBody returns a copy of the receiver with any config-contextual +// diagnostics elaborated in the context of the given body. +func (d Diagnostics) InConfigBody(body hcl.Body) Diagnostics { + if len(d) == 0 { + return nil + } + + ret := make(Diagnostics, len(d)) + for i, srcDiag := range d { + if cd, isCD := srcDiag.(contextualFromConfigBody); isCD { + ret[i] = cd.ElaborateFromConfigBody(body) + } else { + ret[i] = srcDiag + } + } + + return ret +} + +// AttributeValue returns a diagnostic about an attribute value in an implied current +// configuration context. This should be returned only from functions whose +// interface specifies a clear configuration context that this will be +// resolved in. +// +// The given path is relative to the implied configuration context. To describe +// a top-level attribute, it should be a single-element cty.Path with a +// cty.GetAttrStep. It's assumed that the path is returning into a structure +// that would be produced by our conventions in the configschema package; it +// may return unexpected results for structures that can't be represented by +// configschema. +// +// Since mapping attribute paths back onto configuration is an imprecise +// operation (e.g. dynamic block generation may cause the same block to be +// evaluated multiple times) the diagnostic detail should include the attribute +// name and other context required to help the user understand what is being +// referenced in case the identified source range is not unique. +// +// The returned attribute will not have source location information until +// context is applied to the containing diagnostics using diags.InConfigBody. +// After context is applied, the source location is the value assigned to the +// named attribute, or the containing body's "missing item range" if no +// value is present. +func AttributeValue(severity Severity, summary, detail string, attrPath cty.Path) Diagnostic { + return &attributeDiagnostic{ + diagnosticBase: diagnosticBase{ + severity: severity, + summary: summary, + detail: detail, + }, + attrPath: attrPath, + } +} + +type attributeDiagnostic struct { + diagnosticBase + attrPath cty.Path + subject *SourceRange // populated only after ElaborateFromConfigBody +} + +// ElaborateFromConfigBody finds the most accurate possible source location +// for a diagnostic's attribute path within the given body. +// +// Backing out from a path back to a source location is not always entirely +// possible because we lose some information in the decoding process, so +// if an exact position cannot be found then the returned diagnostic will +// refer to a position somewhere within the containing body, which is assumed +// to be better than no location at all. +// +// If possible it is generally better to report an error at a layer where +// source location information is still available, for more accuracy. This +// is not always possible due to system architecture, so this serves as a +// "best effort" fallback behavior for such situations. +func (d *attributeDiagnostic) ElaborateFromConfigBody(body hcl.Body) Diagnostic { + if len(d.attrPath) < 1 { + // Should never happen, but we'll allow it rather than crashing. + return d + } + + if d.subject != nil { + // Don't modify an already-elaborated diagnostic. + return d + } + + ret := *d + + // This function will often end up re-decoding values that were already + // decoded by an earlier step. This is non-ideal but is architecturally + // more convenient than arranging for source location information to be + // propagated to every place in Terraform, and this happens only in the + // presence of errors where performance isn't a concern. + + traverse := d.attrPath[:len(d.attrPath)-1] + final := d.attrPath[len(d.attrPath)-1] + + // If we have more than one step then we'll first try to traverse to + // a child body corresponding to the requested path. + for i := 0; i < len(traverse); i++ { + step := traverse[i] + + switch tStep := step.(type) { + case cty.GetAttrStep: + + var next cty.PathStep + if i < (len(traverse) - 1) { + next = traverse[i+1] + } + + // Will be indexing into our result here? + var indexType cty.Type + var indexVal cty.Value + if nextIndex, ok := next.(cty.IndexStep); ok { + indexVal = nextIndex.Key + indexType = indexVal.Type() + i++ // skip over the index on subsequent iterations + } + + var blockLabelNames []string + if indexType == cty.String { + // Map traversal means we expect one label for the key. + blockLabelNames = []string{"key"} + } + + // For intermediate steps we expect to be referring to a child + // block, so we'll attempt decoding under that assumption. + content, _, contentDiags := body.PartialContent(&hcl.BodySchema{ + Blocks: []hcl.BlockHeaderSchema{ + { + Type: tStep.Name, + LabelNames: blockLabelNames, + }, + }, + }) + if contentDiags.HasErrors() { + subject := SourceRangeFromHCL(body.MissingItemRange()) + ret.subject = &subject + return &ret + } + filtered := make([]*hcl.Block, 0, len(content.Blocks)) + for _, block := range content.Blocks { + if block.Type == tStep.Name { + filtered = append(filtered, block) + } + } + if len(filtered) == 0 { + } + + switch indexType { + case cty.NilType: // no index at all + if len(filtered) != 1 { + subject := SourceRangeFromHCL(body.MissingItemRange()) + ret.subject = &subject + return &ret + } + body = filtered[0].Body + case cty.Number: + var idx int + err := gocty.FromCtyValue(indexVal, &idx) + if err != nil || idx >= len(filtered) { + subject := SourceRangeFromHCL(body.MissingItemRange()) + ret.subject = &subject + return &ret + } + body = filtered[idx].Body + case cty.String: + key := indexVal.AsString() + var block *hcl.Block + for _, candidate := range filtered { + if candidate.Labels[0] == key { + block = candidate + break + } + } + if block == nil { + // No block with this key, so we'll just indicate a + // missing item in the containing block. + subject := SourceRangeFromHCL(body.MissingItemRange()) + ret.subject = &subject + return &ret + } + body = block.Body + default: + // Should never happen, because only string and numeric indices + // are supported by cty collections. + subject := SourceRangeFromHCL(body.MissingItemRange()) + ret.subject = &subject + return &ret + } + + default: + // For any other kind of step, we'll just return our current body + // as the subject and accept that this is a little inaccurate. + subject := SourceRangeFromHCL(body.MissingItemRange()) + ret.subject = &subject + return &ret + } + } + + // Default is to indicate a missing item in the deepest body we reached + // while traversing. + subject := SourceRangeFromHCL(body.MissingItemRange()) + ret.subject = &subject + + // Once we get here, "final" should be a GetAttr step that maps to an + // attribute in our current body. + finalStep, isAttr := final.(cty.GetAttrStep) + if !isAttr { + return &ret + } + + content, _, contentDiags := body.PartialContent(&hcl.BodySchema{ + Attributes: []hcl.AttributeSchema{ + { + Name: finalStep.Name, + Required: true, + }, + }, + }) + if contentDiags.HasErrors() { + return &ret + } + + if attr, ok := content.Attributes[finalStep.Name]; ok { + subject = SourceRangeFromHCL(attr.Expr.Range()) + ret.subject = &subject + } + + return &ret +} + +func (d *attributeDiagnostic) Source() Source { + return Source{ + Subject: d.subject, + } +} diff --git a/tfdiags/contextual_test.go b/tfdiags/contextual_test.go new file mode 100644 index 000000000..4171c7c84 --- /dev/null +++ b/tfdiags/contextual_test.go @@ -0,0 +1,139 @@ +package tfdiags + +import ( + "testing" + + "github.com/go-test/deep" + "github.com/hashicorp/hcl2/hcl" + "github.com/hashicorp/hcl2/hcl/hclsyntax" + "github.com/zclconf/go-cty/cty" +) + +func TestAttributeValue(t *testing.T) { + testConfig := ` +foo { + bar = "hi" +} +foo { + bar = "bar" +} +bar { + bar = "woot" +} +baz "a" { + bar = "beep" +} +baz "b" { + bar = "boop" +} +` + f, parseDiags := hclsyntax.ParseConfig([]byte(testConfig), "test.tf", hcl.Pos{Line: 1, Column: 1}) + if len(parseDiags) != 0 { + t.Fatal(parseDiags) + } + + body := f.Body + var diags Diagnostics + diags = diags.Append(AttributeValue( + Error, + "foo[0].bar", + "detail", + cty.Path{ + cty.GetAttrStep{Name: "foo"}, + cty.IndexStep{Key: cty.NumberIntVal(0)}, + cty.GetAttrStep{Name: "bar"}, + }, + )) + diags = diags.Append(AttributeValue( + Error, + "foo[1].bar", + "detail", + cty.Path{ + cty.GetAttrStep{Name: "foo"}, + cty.IndexStep{Key: cty.NumberIntVal(1)}, + cty.GetAttrStep{Name: "bar"}, + }, + )) + diags = diags.Append(AttributeValue( + Error, + "bar.bar", + "detail", + cty.Path{ + cty.GetAttrStep{Name: "bar"}, + cty.GetAttrStep{Name: "bar"}, + }, + )) + diags = diags.Append(AttributeValue( + Error, + `baz["a"].bar`, + "detail", + cty.Path{ + cty.GetAttrStep{Name: "baz"}, + cty.IndexStep{Key: cty.StringVal("a")}, + cty.GetAttrStep{Name: "bar"}, + }, + )) + diags = diags.Append(AttributeValue( + Error, + `baz["b"].bar`, + "detail", + cty.Path{ + cty.GetAttrStep{Name: "baz"}, + cty.IndexStep{Key: cty.StringVal("b")}, + cty.GetAttrStep{Name: "bar"}, + }, + )) + // Attribute value with subject already populated should not be disturbed. + // (in a real case, this might've been passed through from a deeper function + // in the call stack, for example.) + diags = diags.Append(&attributeDiagnostic{ + diagnosticBase: diagnosticBase{ + summary: "preexisting", + detail: "detail", + }, + subject: &SourceRange{ + Filename: "somewhere_else.tf", + }, + }) + + gotDiags := diags.InConfigBody(body) + + wantRanges := map[string]*SourceRange{ + `foo[0].bar`: { + Filename: "test.tf", + Start: SourcePos{Line: 3, Column: 9, Byte: 15}, + End: SourcePos{Line: 3, Column: 13, Byte: 19}, + }, + `foo[1].bar`: { + Filename: "test.tf", + Start: SourcePos{Line: 6, Column: 9, Byte: 36}, + End: SourcePos{Line: 6, Column: 14, Byte: 41}, + }, + `bar.bar`: { + Filename: "test.tf", + Start: SourcePos{Line: 9, Column: 9, Byte: 58}, + End: SourcePos{Line: 9, Column: 15, Byte: 64}, + }, + `baz["a"].bar`: { + Filename: "test.tf", + Start: SourcePos{Line: 12, Column: 9, Byte: 85}, + End: SourcePos{Line: 12, Column: 15, Byte: 91}, + }, + `baz["b"].bar`: { + Filename: "test.tf", + Start: SourcePos{Line: 15, Column: 9, Byte: 112}, + End: SourcePos{Line: 15, Column: 15, Byte: 118}, + }, + `preexisting`: { + Filename: "somewhere_else.tf", + }, + } + gotRanges := make(map[string]*SourceRange) + for _, diag := range gotDiags { + gotRanges[diag.Description().Summary] = diag.Source().Subject + } + + for _, problem := range deep.Equal(gotRanges, wantRanges) { + t.Error(problem) + } +} diff --git a/tfdiags/diagnostic_base.go b/tfdiags/diagnostic_base.go new file mode 100644 index 000000000..6de82ee82 --- /dev/null +++ b/tfdiags/diagnostic_base.go @@ -0,0 +1,27 @@ +package tfdiags + +// diagnosticBase can be embedded in other diagnostic structs to get +// default implementations of Severity and Description. This type also +// has a default implementation of Source that returns no source location +// information, so embedders should generally override that method to +// return more useful results. +type diagnosticBase struct { + severity Severity + summary string + detail string +} + +func (d diagnosticBase) Severity() Severity { + return d.severity +} + +func (d diagnosticBase) Description() Description { + return Description{ + Summary: d.summary, + Detail: d.detail, + } +} + +func (d diagnosticBase) Source() Source { + return Source{} +}