Merge branch 'main' into copilot/fix-839

Author: mawasile

.changes/unreleased/changed_error_messages.yaml

@@ -0,0 +1,4 @@
+kind: changed
+body: Improved error message formatting and consistency for better user experience across environment templates, tenant isolation policy, and tenant settings resources
+custom:
+  Issue: 823

.changes/unreleased/fixed-20250603-164644.yaml

@@ -0,0 +1,5 @@
+kind: fixed
+body: Fixed duplicated constants and literals issues across codebase to improve maintainability
+time: 2025-06-03T16:46:44.448551164Z
+custom:
+    Issue: "798"

.changes/unreleased/fixed-20250604-133404.yaml

@@ -0,0 +1,5 @@
+kind: fixed
+body: Fixed recursive retry loops and missing return statements in environment services that could lead to infinite loops and data corruption. Added proper error wrapping for better debugging context.
+time: 2025-06-04T13:34:04.220409865Z
+custom:
+    Issue: "808"

.changes/unreleased/fixed-20250604-170539.yaml

@@ -0,0 +1,5 @@
+kind: fixed
+body: 'fix: add proper API error wrapping and data conversion context across multiple services'
+time: 2025-06-04T17:05:39.915274906Z
+custom:
+    Issue: "810"

.changes/unreleased/fixed-20250604-175056.yaml

@@ -0,0 +1,5 @@
+kind: fixed
+body: Fixed missing return statements after error handling in three datasources to prevent potential panics and undefined behavior
+time: 2025-06-04T17:50:56.587490569Z
+custom:
+    Issue: "814"

.changes/unreleased/fixed-20250605-102519.yaml

@@ -0,0 +1,5 @@
+kind: fixed
+body: 'Fixed validation and modifiers issues: improved error handling in validators and plan modifiers, corrected error messages for SHA256 checksums, and enhanced diagnostic context'
+time: 2025-06-05T10:25:19.227038423Z
+custom:
+    Issue: "821"

.changes/unreleased/fixed-20250605-121743.yaml

@@ -0,0 +1,5 @@
+kind: fixed
+body: 'Fix configuration and constants issues: CAE challenge detection strings, RX PowerApps Advisor API domain constants, and AuxiliaryTenantIDs type safety conversion'
+time: 2025-06-05T12:17:43.646455328Z
+custom:
+    Issue: "824"

.changes/unreleased/fixed-20250610-154740.yaml

@@ -0,0 +1,5 @@
+kind: fixed
+body: Fix recursive retry loops and missing return statements after AddError in environment services
+time: 2025-06-10T15:47:40.435353291Z
+custom:
+    Issue: "808"

.changes/unreleased/fixed_type_safety_formatting_issues.yaml

@@ -0,0 +1,4 @@
+kind: fixed
+body: Fixed type safety and formatting issues including missing string validation, interface assertions, and escaped newline formatting
+custom:
+  Issue: 825

examples/resources/powerplatform_billing_policy/terraform.tf

@@ -5,7 +5,7 @@ terraform {
     }
     azurerm = {
       source  = "hashicorp/azurerm"
-      version = "4.32.0"
+      version = "4.33.0"
     }
     azurecaf = {
       source = "aztfmod/azurecaf"

internal/api/client.go

@@ -83,8 +83,8 @@ func IsCaeChallengeResponse(resp *http.Response) bool {
 	if resp.StatusCode == http.StatusUnauthorized {
 		wwwAuthenticate := resp.Header.Get("WWW-Authenticate")
 		if wwwAuthenticate != "" {
-			return strings.Contains(wwwAuthenticate, "claims=") &&
-				strings.Contains(wwwAuthenticate, "insufficient_claims")
+			return strings.Contains(wwwAuthenticate, constants.CAE_CHALLENGE_CLAIMS_INDICATOR) &&
+				strings.Contains(wwwAuthenticate, constants.CAE_CHALLENGE_INSUFFICIENT_CLAIMS_INDICATOR)
 		}
 	}
 

internal/api/client_test.go

@@ -39,10 +39,10 @@ func TestUnitApiClient_GetConfig(t *testing.T) {
 	}
 
 	switch err.(type) {
-	case customerrors.UrlFormatError:
+	case *customerrors.UrlFormatError:
 		return
 	default:
-		t.Errorf("Expected error type %s but got %s", reflect.TypeOf(customerrors.UrlFormatError{}), reflect.TypeOf(err))
+		t.Errorf("Expected error type %s but got %s", reflect.TypeOf(&customerrors.UrlFormatError{}), reflect.TypeOf(err))
 	}
 }
 

internal/constants/constants.go

@@ -141,6 +141,7 @@ const (
 
 const (
 	DEFAULT_RESOURCE_OPERATION_TIMEOUT_IN_MINUTES = 20 * time.Minute
+	MAX_RETRY_COUNT                               = 10
 )
 
 const (
@@ -180,6 +181,18 @@ const (
 	AZAPI_PROVIDER_VERSION_CONSTRAINT    = ">= 1.15.0"
 )
 
+const (
+	ADMIN_MANAGEMENT_APP_API_VERSION = "2020-10-01"
+	ENTERPRISE_POLICY_API_VERSION    = "2019-10-01"
+	BAP_API_VERSION                  = "2023-06-01"
+	BAP_2021_API_VERSION             = "2021-04-01"
+	BAP_2022_API_VERSION             = "2022-05-01"
+	APPLICATION_API_VERSION          = "2022-03-01-preview"
+	ENVIRONMENT_GROUP_API_VERSION    = "2021-10-01-preview"
+	CONNECTORS_API_VERSION           = "2019-05-01"
+	TENANT_SETTINGS_API_VERSION      = "2020-08-01"
+)
+
 const (
 	SOLUTION_CHECKER_RULESET_ID = "0ad12346-e108-40b8-a956-9a8f95ea18c9"
 )
@@ -188,6 +201,10 @@ const (
 	NO_MANAGEMENT_APPLICATION_ERROR_MSG = "authorization has been denied for this request. Make sure that your service principal is registered as an admin management application: https://learn.microsoft.com/en-us/power-platform/admin/powerplatform-api-create-service-principal#registering-an-admin-management-application"
 )
 
+const (
+	CAE_CHALLENGE_CLAIMS_INDICATOR              = "claims="
+	CAE_CHALLENGE_INSUFFICIENT_CLAIMS_INDICATOR = "insufficient_claims"
+)
 // Error codes for provider errors.
 const (
 	ERROR_OBJECT_NOT_FOUND             = "OBJECT_NOT_FOUND"

internal/customerrors/url_format_error.go

@@ -7,21 +7,21 @@ import (
 	"fmt"
 )
 
-var _ error = UrlFormatError{}
+var _ error = (*UrlFormatError)(nil)
 
 type UrlFormatError struct {
 	Url string
 	Err error
 }
 
 func NewUrlFormatError(url string, err error) error {
-	return UrlFormatError{
+	return &UrlFormatError{
 		Err: err,
 		Url: url,
 	}
 }
 
-func (e UrlFormatError) Error() string {
+func (e *UrlFormatError) Error() string {
 	errorMsg := ""
 	if e.Err != nil {
 		errorMsg = e.Err.Error()
@@ -30,6 +30,6 @@ func (e UrlFormatError) Error() string {
 	return fmt.Sprintf("Request url must be an absolute url: '%s' : '%s'", e.Url, errorMsg)
 }
 
-func (e UrlFormatError) Unwrap() error {
+func (e *UrlFormatError) Unwrap() error {
 	return e.Err
 }

internal/customtypes/uuid_value.go

@@ -18,7 +18,7 @@ import (
 
 const (
 	UUIDTypeErrorInvalidStringHeader  = "Invalid UUID String Value"
-	UUIDTypeErrorInvalidStringDetails = `A string value was provided that is not valid UUID string format.\n\nGiven Value: %s\n`
+	UUIDTypeErrorInvalidStringDetails = "A string value was provided that is not valid UUID string format.\n\nGiven Value: %s\n"
 )
 
 var (

internal/helpers/cert.go

@@ -22,7 +22,7 @@ func GetCertificateRawFromCertOrFilePath(certificate, certificateFilePath string
 	if certificateFilePath != "" {
 		pfx, err := os.ReadFile(certificateFilePath)
 		if err != nil {
-			return "", err
+			return "", fmt.Errorf("failed to read certificate file '%s': %w", certificateFilePath, err)
 		}
 		certAsBase64 := base64.StdEncoding.EncodeToString(pfx)
 		return strings.TrimSpace(certAsBase64), nil
@@ -38,7 +38,7 @@ func ConvertBase64ToCert(b64, password string) ([]*x509.Certificate, crypto.Priv
 
 	certs, key, err := convertByteToCert(pfx, password)
 	if err != nil {
-		return nil, nil, err
+		return nil, nil, fmt.Errorf("failed to convert certificate bytes: %w", err)
 	}
 
 	return certs, key, nil
@@ -62,7 +62,7 @@ func convertByteToCert(certData []byte, password string) ([]*x509.Certificate, c
 
 	key, cert, _, err := pkcs12.DecodeChain(certData, password)
 	if err != nil {
-		return nil, nil, err
+		return nil, nil, fmt.Errorf("failed to decode PKCS12 certificate chain: %w", err)
 	}
 
 	if cert == nil {

internal/helpers/hash_test.go

@@ -13,24 +13,29 @@ import (
 func TestUnitCalculateSHA256(t *testing.T) {
 	t.Parallel()
 
+	const (
+		sameContent      = "same"
+		differentContent = "different"
+	)
+
 	tdir := t.TempDir()
 	file1 := tdir + "/test.txt"
 	file2 := tdir + "/test2.txt"
 	file3 := tdir + "/test3.txt"
 	file4 := tdir + "/test4.txt"
 	file5 := tdir + "/test5"
 
-	err := os.WriteFile(file1, []byte("same"), 0644)
+	err := os.WriteFile(file1, []byte(sameContent), 0644)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	err = os.WriteFile(file2, []byte("same"), 0644)
+	err = os.WriteFile(file2, []byte(sameContent), 0644)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	err = os.WriteFile(file3, []byte("different"), 0644)
+	err = os.WriteFile(file3, []byte(differentContent), 0644)
 	if err != nil {
 		t.Fatal(err)
 	}

internal/helpers/uri.go

@@ -5,7 +5,10 @@ package helpers
 
 import (
 	"fmt"
+	"net/url"
 	"strings"
+
+	"github.com/microsoft/terraform-provider-power-platform/internal/constants"
 )
 
 // BuildEnvironmentHostUri builds the host uri for the environmentid
@@ -29,3 +32,26 @@ func BuildTenantHostUri(tenantId, powerPlatformUrl string) string {
 
 	return fmt.Sprintf("%s.%s.tenant.%s", envId, realm, powerPlatformUrl)
 }
+
+// BuildApiUrl builds a URL for API endpoints with a given host, path, and query parameters.
+func BuildApiUrl(host, path string, query url.Values) string {
+	apiUrl := &url.URL{
+		Scheme: constants.HTTPS,
+		Host:   host,
+		Path:   path,
+	}
+	if query != nil {
+		apiUrl.RawQuery = query.Encode()
+	}
+	return apiUrl.String()
+}
+
+// BuildDataverseApiUrl builds a URL for Dataverse API endpoints.
+func BuildDataverseApiUrl(environmentHost, path string, query url.Values) string {
+	return BuildApiUrl(environmentHost, path, query)
+}
+
+// BuildBapiUrl builds a URL for BAPI endpoints.
+func BuildBapiUrl(bapiHost, path string, query url.Values) string {
+	return BuildApiUrl(bapiHost, path, query)
+}

internal/modifiers/set_bool_value_unknown_if_checksum_change_modifier.go

@@ -61,7 +61,7 @@ func (d *setBoolValueToUnknownIfChecksumsChangeModifier) hasChecksumChanged(ctx
 
 	value, err := helpers.CalculateSHA256(attribute.ValueString())
 	if err != nil {
-		resp.Diagnostics.AddError(fmt.Sprintf("Error calculating MD5 checksum for %s", attribute), err.Error())
+		resp.Diagnostics.AddError(fmt.Sprintf("Error calculating SHA256 checksum for %q", attribute), err.Error())
 	}
 
 	return value != attributeChecksum.ValueString()

internal/modifiers/set_string_value_unknown_if_checksum_change_modifier.go

@@ -54,14 +54,20 @@ func (d *setStringValueToUnknownIfChecksumsChangeModifier) hasChecksumChanged(ct
 	var attribute types.String
 	diags := req.Plan.GetAttribute(ctx, path.Root(attributeName), &attribute)
 	resp.Diagnostics.Append(diags...)
+	if diags.HasError() {
+		return false
+	}
 
 	var attributeChecksum types.String
 	diags = req.State.GetAttribute(ctx, path.Root(checksumAttributeName), &attributeChecksum)
 	resp.Diagnostics.Append(diags...)
+	if diags.HasError() {
+		return false
+	}
 
 	value, err := helpers.CalculateSHA256(attribute.ValueString())
 	if err != nil {
-		resp.Diagnostics.AddError(fmt.Sprintf("Error calculating MD5 checksum for %s", attribute), err.Error())
+		resp.Diagnostics.AddError(fmt.Sprintf("Error calculating SHA256 checksum for attribute %q", attributeName), err.Error())
 	}
 
 	return value != attributeChecksum.ValueString()

internal/modifiers/sync_attribute_plan_modifier.go

@@ -40,18 +40,17 @@ func (d *syncAttributePlanModifier) PlanModifyString(ctx context.Context, req pl
 		return
 	}
 
-	if settingsFile.IsNull() {
-		resp.PlanValue = types.StringNull()
-	} else if settingsFile.IsUnknown() {
+	if settingsFile.IsNull() || settingsFile.IsUnknown() {
 		resp.PlanValue = types.StringNull()
 	} else {
 		value, err := helpers.CalculateSHA256(settingsFile.ValueString())
 		if err != nil {
-			resp.Diagnostics.AddError(fmt.Sprintf("Error calculating MD5 checksum for %s", d.syncAttribute), err.Error())
+			resp.Diagnostics.AddError(fmt.Sprintf("Error calculating SHA256 checksum for %s", d.syncAttribute), err.Error())
 			return
 		}
 
 		if value == "" {
+			resp.Diagnostics.AddError(fmt.Sprintf("Checksum is empty for %s", d.syncAttribute), "Calculated SHA256 checksum resulted in an empty value, which is unexpected.")
 			resp.PlanValue = types.StringUnknown()
 		} else {
 			resp.PlanValue = types.StringValue(value)

internal/provider/provider.go

@@ -355,7 +355,11 @@ func configureUseMsi(ctx context.Context, p *PowerPlatformProvider, clientId str
 	// Convert the slice to an array
 	auxiliaryTenantIDsList := make([]string, len(auxiliaryTenantIDs.Elements()))
 	for i, v := range auxiliaryTenantIDs.Elements() {
-		auxiliaryTenantIDsList[i] = v.String()
+		if str, ok := v.(types.String); ok {
+			auxiliaryTenantIDsList[i] = str.ValueString()
+		} else {
+			tflog.Warn(ctx, fmt.Sprintf("Element at index %d in auxiliaryTenantIDs is not of type types.String. Skipping.", i))
+		}
 	}
 	p.Config.AuxiliaryTenantIDs = auxiliaryTenantIDsList
 	p.Config.UseMsi = true

internal/provider/provider_test.go

@@ -76,9 +76,9 @@ func TestUnitPowerPlatformProviderHasChildDataSources_Basic(t *testing.T) {
 	}
 	datasources := provider.NewPowerPlatformProvider(context.Background())().(*provider.PowerPlatformProvider).DataSources(context.Background())
 
-	require.Equal(t, len(expectedDataSources), len(datasources), "There are an unexpected number of registered data sources")
+	require.Equalf(t, len(expectedDataSources), len(datasources), "Expected %d data sources, got %d", len(expectedDataSources), len(datasources))
 	for _, d := range datasources {
-		require.Contains(t, expectedDataSources, d(), "An unexpected data source was registered")
+		require.Containsf(t, expectedDataSources, d(), "Data source %+v was not expected", d())
 	}
 }
 
@@ -109,9 +109,9 @@ func TestUnitPowerPlatformProviderHasChildResources_Basic(t *testing.T) {
 	}
 	resources := provider.NewPowerPlatformProvider(context.Background())().(*provider.PowerPlatformProvider).Resources(context.Background())
 
-	require.Equal(t, len(expectedResources), len(resources), "There are an unexpected number of registered resources")
+	require.Equalf(t, len(expectedResources), len(resources), "Expected %d resources, got %d", len(expectedResources), len(resources))
 	for _, r := range resources {
-		require.Contains(t, expectedResources, r(), "An unexpected resource was registered")
+		require.Containsf(t, expectedResources, r(), "Resource %+v was not expected", r())
 	}
 }