Fix empty string credentials validation issue, increase test coverage…

… of credential validation (#7690) (#14279) Signed-off-by: Modular Magician <magic-modules@google.com>
hashicorp · Apr 11, 2023 · 6330964 · 6330964
1 parent 43bfd88
commit 6330964
Show file tree

Hide file tree

Showing 4 changed files with 131 additions and 60 deletions.
diff --git a/.changelog/7690.txt b/.changelog/7690.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+provider: fixed bug where `credentials` field could not be set as an empty string
+```
diff --git a/google/framework_provider_test.go b/google/framework_provider_test.go
@@ -57,48 +57,69 @@ func TestFrameworkProvider_impl(t *testing.T) {
 	var _ provider.ProviderWithMetaSchema = New("test")
 }
 
-func TestFrameworkProvider_loadCredentialsFromFile(t *testing.T) {
-	cv := CredentialsValidator()
-
-	req := validator.StringRequest{
-		ConfigValue: types.StringValue(testFakeCredentialsPath),
-	}
-
-	resp := validator.StringResponse{
-		Diagnostics: diag.Diagnostics{},
-	}
-
-	cv.ValidateString(context.Background(), req, &resp)
-
-	if resp.Diagnostics.WarningsCount() > 0 {
-		t.Errorf("Expected 0 warnings, got %d", resp.Diagnostics.WarningsCount())
-	}
-	if resp.Diagnostics.HasError() {
-		t.Errorf("Expected 0 errors, got %d", resp.Diagnostics.ErrorsCount())
-	}
-}
-
-func TestFrameworkProvider_loadCredentialsFromJSON(t *testing.T) {
-	contents, err := ioutil.ReadFile(testFakeCredentialsPath)
-	if err != nil {
-		t.Fatalf("Unexpected error: %s", err)
-	}
-	cv := CredentialsValidator()
-
-	req := validator.StringRequest{
-		ConfigValue: types.StringValue(string(contents)),
-	}
-
-	resp := validator.StringResponse{
-		Diagnostics: diag.Diagnostics{},
+func TestFrameworkProvider_CredentialsValidator(t *testing.T) {
+	cases := map[string]struct {
+		ConfigValue          func(t *testing.T) types.String
+		ExpectedWarningCount int
+		ExpectedErrorCount   int
+	}{
+		"configuring credentials as a path to a credentials JSON file is valid": {
+			ConfigValue: func(t *testing.T) types.String {
+				return types.StringValue(testFakeCredentialsPath) // Path to a test fixture
+			},
+		},
+		"configuring credentials as a path to a non-existant file is NOT valid": {
+			ConfigValue: func(t *testing.T) types.String {
+				return types.StringValue("./this/path/doesnt/exist.json") // Doesn't exist
+			},
+			ExpectedErrorCount: 1,
+		},
+		"configuring credentials as a credentials JSON string is valid": {
+			ConfigValue: func(t *testing.T) types.String {
+				contents, err := ioutil.ReadFile(testFakeCredentialsPath)
+				if err != nil {
+					t.Fatalf("Unexpected error: %s", err)
+				}
+				stringContents := string(contents)
+				return types.StringValue(stringContents)
+			},
+		},
+		"configuring credentials as an empty string is valid": {
+			ConfigValue: func(t *testing.T) types.String {
+				return types.StringValue("")
+			},
+		},
+		"leaving credentials unconfigured is valid": {
+			ConfigValue: func(t *testing.T) types.String {
+				return types.StringNull()
+			},
+		},
 	}
 
-	cv.ValidateString(context.Background(), req, &resp)
-	if resp.Diagnostics.WarningsCount() > 0 {
-		t.Errorf("Expected 0 warnings, got %d", resp.Diagnostics.WarningsCount())
-	}
-	if resp.Diagnostics.HasError() {
-		t.Errorf("Expected 0 errors, got %d", resp.Diagnostics.ErrorsCount())
+	for tn, tc := range cases {
+		t.Run(tn, func(t *testing.T) {
+			// Arrange
+			req := validator.StringRequest{
+				ConfigValue: tc.ConfigValue(t),
+			}
+
+			resp := validator.StringResponse{
+				Diagnostics: diag.Diagnostics{},
+			}
+
+			cv := CredentialsValidator()
+
+			// Act
+			cv.ValidateString(context.Background(), req, &resp)
+
+			// Assert
+			if resp.Diagnostics.WarningsCount() > tc.ExpectedWarningCount {
+				t.Errorf("Expected %d warnings, got %d", tc.ExpectedWarningCount, resp.Diagnostics.WarningsCount())
+			}
+			if resp.Diagnostics.ErrorsCount() > tc.ExpectedErrorCount {
+				t.Errorf("Expected %d errors, got %d", tc.ExpectedErrorCount, resp.Diagnostics.ErrorsCount())
+			}
+		})
 	}
 }
 

diff --git a/google/framework_validators.go b/google/framework_validators.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator"
 	"github.com/hashicorp/terraform-plugin-framework/schema/validator"
+	"github.com/hashicorp/terraform-plugin-framework/types"
 
 	googleoauth "golang.org/x/oauth2/google"
 )
@@ -36,7 +37,7 @@ func (v credentialsValidator) MarkdownDescription(ctx context.Context) string {
 
 // ValidateString performs the validation.
 func (v credentialsValidator) ValidateString(ctx context.Context, request validator.StringRequest, response *validator.StringResponse) {
-	if request.ConfigValue.IsNull() || request.ConfigValue.IsUnknown() {
+	if request.ConfigValue.IsNull() || request.ConfigValue.IsUnknown() || request.ConfigValue.Equal(types.StringValue("")) {
 		return
 	}
 

diff --git a/google/provider_test.go b/google/provider_test.go
@@ -2,6 +2,7 @@ package google
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"log"
@@ -165,27 +166,72 @@ func AccTestPreCheck(t *testing.T) {
 	}
 }
 
-func TestProvider_loadCredentialsFromFile(t *testing.T) {
-	ws, es := validateCredentials(testFakeCredentialsPath, "")
-	if len(ws) != 0 {
-		t.Errorf("Expected %d warnings, got %v", len(ws), ws)
-	}
-	if len(es) != 0 {
-		t.Errorf("Expected %d errors, got %v", len(es), es)
+func TestProvider_validateCredentials(t *testing.T) {
+	cases := map[string]struct {
+		ConfigValue      func(t *testing.T) interface{}
+		ValueNotProvided bool
+		ExpectedWarnings []string
+		ExpectedErrors   []error
+	}{
+		"configuring credentials as a path to a credentials JSON file is valid": {
+			ConfigValue: func(t *testing.T) interface{} {
+				return testFakeCredentialsPath // Path to a test fixture
+			},
+		},
+		"configuring credentials as a path to a non-existant file is NOT valid": {
+			ConfigValue: func(t *testing.T) interface{} {
+				return "./this/path/doesnt/exist.json" // Doesn't exist
+			},
+			ExpectedErrors: []error{
+				// As the file doesn't exist, so the function attempts to parse it as a JSON
+				errors.New("JSON credentials are not valid: invalid character '.' looking for beginning of value"),
+			},
+		},
+		"configuring credentials as a credentials JSON string is valid": {
+			ConfigValue: func(t *testing.T) interface{} {
+				contents, err := ioutil.ReadFile(testFakeCredentialsPath)
+				if err != nil {
+					t.Fatalf("Unexpected error: %s", err)
+				}
+				return string(contents)
+			},
+		},
+		"configuring credentials as an empty string is valid": {
+			ConfigValue: func(t *testing.T) interface{} {
+				return ""
+			},
+		},
+		"leaving credentials unconfigured is valid": {
+			ValueNotProvided: true,
+		},
 	}
-}
 
-func TestProvider_loadCredentialsFromJSON(t *testing.T) {
-	contents, err := ioutil.ReadFile(testFakeCredentialsPath)
-	if err != nil {
-		t.Fatalf("Unexpected error: %s", err)
-	}
-	ws, es := validateCredentials(string(contents), "")
-	if len(ws) != 0 {
-		t.Errorf("Expected %d warnings, got %v", len(ws), ws)
-	}
-	if len(es) != 0 {
-		t.Errorf("Expected %d errors, got %v", len(es), es)
+	for tn, tc := range cases {
+		t.Run(tn, func(t *testing.T) {
+			// Arrange
+			var configValue interface{}
+			if !tc.ValueNotProvided {
+				configValue = tc.ConfigValue(t)
+			}
+
+			// Act
+			// Note: second argument is currently unused by the function but is necessary to fulfill the SchemaValidateFunc type's function signature
+			ws, es := validateCredentials(configValue, "")
+
+			// Assert
+			if len(ws) != len(tc.ExpectedWarnings) {
+				t.Errorf("Expected %d warnings, got %d: %v", len(tc.ExpectedWarnings), len(ws), ws)
+			}
+			if len(es) != len(tc.ExpectedErrors) {
+				t.Errorf("Expected %d errors, got %d: %v", len(tc.ExpectedErrors), len(es), es)
+			}
+
+			if len(tc.ExpectedErrors) > 0 {
+				if es[0].Error() != tc.ExpectedErrors[0].Error() {
+					t.Errorf("Expected first error to be \"%s\", got \"%s\"", tc.ExpectedErrors[0], es[0])
+				}
+			}
+		})
 	}
 }