Improve ValidationError Error() (#2056)

Improve ValidationError Error() to expose AttributePath. It appears that
ValidationError is the bridge representation of any and all underlying
TF Diagnostics. Allowing all the information to be printed can help save
time when trying to understand unrecoverable error messages emitted from
TF internals.

pkg/tfbridge/adapt_check_failures.go

@@ -67,20 +67,13 @@ func parseCheckError(
 	schemaInfos map[string]*SchemaInfo,
 	err error,
 ) (*CheckFailurePath, CheckFailureReason, string) {
-	if parts := requiredFieldRegex.FindStringSubmatch(err.Error()); len(parts) == 2 {
-		name := parts[1]
-		pp := NewCheckFailurePath(schemaMap, schemaInfos, name)
-		return &pp, MissingKey, err.Error()
-	}
-	if parts := conflictsWithRegex.FindStringSubmatch(err.Error()); len(parts) == 2 {
-		name := parts[1]
-		pp := NewCheckFailurePath(schemaMap, schemaInfos, name)
-		return &pp, MiscFailure, err.Error()
-	}
 	if d := (*diagnostics.ValidationError)(nil); errors.As(err, &d) {
 		failType := MiscFailure
-		if strings.Contains(d.Summary, "Invalid or unknown key") {
+		switch {
+		case strings.Contains(d.Summary, "Invalid or unknown key"):
 			failType = InvalidKey
+		case strings.Contains(d.Summary, "required field is not set"):
+			failType = MissingKey
 		}
 		pp := formatAttributePathAsPropertyPath(schemaMap, schemaInfos, d.AttributePath)
 		s := d.Summary
@@ -89,6 +82,16 @@ func parseCheckError(
 		}
 		return pp, failType, s
 	}
+	if parts := requiredFieldRegex.FindStringSubmatch(err.Error()); len(parts) == 2 {
+		name := parts[1]
+		pp := NewCheckFailurePath(schemaMap, schemaInfos, name)
+		return &pp, MissingKey, err.Error()
+	}
+	if parts := conflictsWithRegex.FindStringSubmatch(err.Error()); len(parts) == 2 {
+		name := parts[1]
+		pp := NewCheckFailurePath(schemaMap, schemaInfos, name)
+		return &pp, MiscFailure, err.Error()
+	}
 	// If there is no way to identify a propertyPath, still report a generic CheckFailure.
 	return nil, MiscFailure, err.Error()
 }

pkg/tfbridge/provider_test.go

@@ -717,10 +717,10 @@ func testCheckFailuresV1(t *testing.T, failures []*pulumirpc.CheckFailure) {
 }
 
 func testCheckFailuresV2(t *testing.T, failures []*pulumirpc.CheckFailure) {
-	assert.Equal(t, "Conflicting configuration arguments: \"conflicting_property\": conflicts with "+
+	assert.Equal(t, "Conflicting configuration arguments. \"conflicting_property\": conflicts with "+
 		"conflicting_property2. Examine values at 'name.conflictingProperty'.", failures[0].Reason)
 	assert.Equal(t, "", failures[0].Property)
-	assert.Equal(t, "Conflicting configuration arguments: \"conflicting_property2\": conflicts with "+
+	assert.Equal(t, "Conflicting configuration arguments. \"conflicting_property2\": conflicts with "+
 		"conflicting_property. Examine values at 'name.conflictingProperty2'.", failures[1].Reason)
 	assert.Equal(t, "", failures[1].Property)
 	assert.Equal(t, "Missing required argument. The argument \"array_property_value\" is required"+

pkg/tfshim/diagnostics/error.go

@@ -1,20 +1,52 @@
 package diagnostics
 
 import (
+	"bytes"
 	"fmt"
+
 	"github.com/hashicorp/go-cty/cty"
 )
 
-// ValidationError wraps validation errors reported by shims (currently shim v2 and tf5)
+// ValidationError wraps diagnostics reported by shims (currently shim v2 and tf5).
 type ValidationError struct {
 	AttributePath cty.Path
 	Summary       string
 	Detail        string
 }
 
+// Last-resort error printout.
+//
+// To display nice error messages to Pulumi users the bridge should recognize ValidationError structs and reformat
+// AttributePath in terms of the Pulumi provider as appropriate, so this method should not be called on paths that are
+// expected to be user-visible.
+//
+// However this method is currently called in unrecoverable situations when underlying TF machinery fails. Opt to expose
+// all the information here to facilitate debugging.
 func (e ValidationError) Error() string {
+	var buf bytes.Buffer
+	if len(e.AttributePath) > 0 {
+		fmt.Fprintf(&buf, "[")
+		for i, p := range e.AttributePath {
+			switch p := p.(type) {
+			case cty.IndexStep:
+				if p.Key.Type() == cty.String {
+					fmt.Fprintf(&buf, "[%q]", p.Key.AsString())
+				} else if p.Key.Type() == cty.Number {
+					fmt.Fprintf(&buf, "[%v]", p.Key.AsBigFloat().String())
+				}
+			case cty.GetAttrStep:
+				if i > 0 {
+					fmt.Fprintf(&buf, ".")
+				}
+				fmt.Fprintf(&buf, p.Name)
+			}
+		}
+		fmt.Fprintf(&buf, "] ")
+	}
 	if e.Detail != "" {
-		return fmt.Sprintf("%s: %s", e.Summary, e.Detail)
+		fmt.Fprintf(&buf, "%s: %s", e.Summary, e.Detail)
+	} else {
+		fmt.Fprintf(&buf, e.Summary)
 	}
-	return e.Summary
+	return buf.String()
 }

pkg/tfshim/diagnostics/error_test.go

@@ -0,0 +1,44 @@
+package diagnostics
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/hashicorp/go-cty/cty"
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidationError(t *testing.T) {
+	type testCase struct {
+		e   ValidationError
+		exp string
+	}
+	testCases := []testCase{
+		{
+			ValidationError{Summary: "Bad input"},
+			"Bad input",
+		},
+		{
+			ValidationError{Summary: "Bad input", Detail: "the input is very bad"},
+			"Bad input: the input is very bad",
+		},
+		{
+			ValidationError{Summary: "Bad input", AttributePath: cty.GetAttrPath("foo").IndexInt(0)},
+			"[foo[0]] Bad input",
+		},
+		{
+			ValidationError{
+				Summary:       "Bad input",
+				Detail:        "the input is very bad",
+				AttributePath: cty.GetAttrPath("foo").IndexString("bar").GetAttr("baz"),
+			},
+			"[foo[\"bar\"].baz] Bad input: the input is very bad",
+		},
+	}
+	for i, tc := range testCases {
+		tc := tc
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			require.Equal(t, tc.exp, tc.e.Error())
+		})
+	}
+}