From 6bad5392c913e9c4346666c60bab600cc82c28f5 Mon Sep 17 00:00:00 2001 From: Paddy Carver Date: Mon, 8 Feb 2021 10:44:30 -0800 Subject: [PATCH 1/2] Fix empty diags not getting associated with source. Right now, there's a bug that if a diagnostic comes back from the provider with an AttributePath set, but no steps in the AttributePath, Terraform _thinks_ it's an attribute-specific diagnostic and not a whole-resource diagnostic, but then doesn't associate it with any specific attribute, meaning the diagnostic doesn't get associated with the config at all. This PR changes things to check if there are any steps in the AttributePath before deciding this isn't a whole-resource diagnostic, and if there aren't, treats it as a whole-resource diagnostic, instead. See hashicorp/terraform-plugin-sdk#561 for more details on how this surfaces in the wild. --- plugin/convert/diagnostics.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/convert/diagnostics.go b/plugin/convert/diagnostics.go index 51cb2fe2fe..7eca328887 100644 --- a/plugin/convert/diagnostics.go +++ b/plugin/convert/diagnostics.go @@ -65,7 +65,7 @@ func ProtoToDiagnostics(ds []*proto.Diagnostic) tfdiags.Diagnostics { var newDiag tfdiags.Diagnostic // if there's an attribute path, we need to create a AttributeValue diagnostic - if d.Attribute != nil { + if d.Attribute != nil && len(d.Attribute.Steps) > 0 { path := AttributePathToPath(d.Attribute) newDiag = tfdiags.AttributeValue(severity, d.Summary, d.Detail, path) } else { From f5a1aed2f8defbb3b61fe764f53e00ec5375494f Mon Sep 17 00:00:00 2001 From: Paddy Carver Date: Thu, 18 Feb 2021 02:06:45 -0800 Subject: [PATCH 2/2] Test empty diagnostics. Add a test for a diagnostic with no steps to make sure it still gets associated with the resource in the config. Follow up to #27710 using @alisdair's suggested testing. --- plugin/convert/diagnostics_test.go | 44 ++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/plugin/convert/diagnostics_test.go b/plugin/convert/diagnostics_test.go index 5825269a58..c6957a7f1b 100644 --- a/plugin/convert/diagnostics_test.go +++ b/plugin/convert/diagnostics_test.go @@ -5,6 +5,8 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" proto "github.com/hashicorp/terraform/internal/tfplugin5" "github.com/hashicorp/terraform/tfdiags" "github.com/zclconf/go-cty/cty" @@ -357,3 +359,45 @@ func TestDiagnostics(t *testing.T) { }) } } + +// Test that a diagnostic with a present but empty attribute results in a +// whole body diagnostic. We verify this by inspecting the resulting Subject +// from the diagnostic when considered in the context of a config body. +func TestProtoDiagnostics_emptyAttributePath(t *testing.T) { + protoDiags := []*proto.Diagnostic{ + { + Severity: proto.Diagnostic_ERROR, + Summary: "error 1", + Detail: "error 1 detail", + Attribute: &proto.AttributePath{ + Steps: []*proto.AttributePath_Step{ + // this slice is intentionally left empty + }, + }, + }, + } + tfDiags := ProtoToDiagnostics(protoDiags) + + testConfig := `provider "test" { + foo = "bar" +}` + f, parseDiags := hclsyntax.ParseConfig([]byte(testConfig), "test.tf", hcl.Pos{Line: 1, Column: 1}) + if parseDiags.HasErrors() { + t.Fatal(parseDiags) + } + diags := tfDiags.InConfigBody(f.Body) + + if len(tfDiags) != 1 { + t.Fatalf("expected 1 diag, got %d", len(tfDiags)) + } + got := diags[0].Source().Subject + want := &tfdiags.SourceRange{ + Filename: "test.tf", + Start: tfdiags.SourcePos{Line: 1, Column: 1}, + End: tfdiags.SourcePos{Line: 1, Column: 1}, + } + + if !cmp.Equal(got, want, typeComparer, valueComparer) { + t.Fatal(cmp.Diff(got, want, typeComparer, valueComparer)) + } +}