Fix critical error handling issues: retry loops, state management, and diagnostic reporting

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

@@ -0,0 +1,5 @@
+kind: fixed
+body: 'fix: improve error handling for retry loops, state management, validators, and diagnostic reporting across 9 critical components to prevent silent failures and data corruption'
+time: 2025-06-04T17:12:06.598599816Z
+custom:
+    Issue: "812"

internal/provider/provider_test.go

@@ -183,6 +183,11 @@ func TestUnitPowerPlatformProvider_PartnerId_Valid(t *testing.T) {
 
 	mocks.ActivateEnvironmentHttpMocks()
 
+	httpmock.RegisterRegexpResponder("GET", regexp.MustCompile(`^https://api\.bap\.microsoft\.com/providers/Microsoft\.BusinessAppPlatform/scopes/admin/environments/(00000000-0000-0000-0000-000000000001|00000000-0000-0000-0000-000000000002)\?%24expand=permissions%2Cproperties.capacity%2Cproperties%2FbillingPolicy&api-version=2023-06-01$`),
+		func(req *http.Request) (*http.Response, error) {
+			return httpmock.NewStringResponse(http.StatusOK, httpmock.File("../services/environment/tests/datasource/Validate_Read/get_environments.json").String()), nil
+		})
+
 	httpmock.RegisterResponder("GET", `https://api.bap.microsoft.com/providers/Microsoft.BusinessAppPlatform/scopes/admin/environments?%24expand=properties%2FbillingPolicy%2Cproperties%2FcopilotPolicies&api-version=2023-06-01`,
 		func(req *http.Request) (*http.Response, error) {
 			return httpmock.NewStringResponse(http.StatusOK, httpmock.File("../services/environment/tests/datasource/Validate_Read/get_environments.json").String()), nil

internal/services/authorization/api_user.go

@@ -310,6 +310,9 @@ func (client *client) CreateDataverseUser(ctx context.Context, environmentId, aa
 		retryCount--
 	}
 	if err != nil {
+		if retryCount == 0 {
+			return nil, fmt.Errorf("failed to create Dataverse user after maximum retries: %w", err)
+		}
 		return nil, err
 	}
 

internal/services/authorization/datasource_securityroles.go

@@ -117,7 +117,11 @@ func (d *SecurityRolesDataSource) Read(ctx context.Context, req datasource.ReadR
 	defer exitContext()
 
 	var state SecurityRolesListDataSourceModel
-	resp.State.Get(ctx, &state)
+	diags := resp.State.Get(ctx, &state)
+	resp.Diagnostics.Append(diags...)
+	if resp.Diagnostics.HasError() {
+		return
+	}
 
 	if state.EnvironmentId.ValueString() == "" {
 		resp.Diagnostics.AddError("environment_id connot be an empty string", "environment_id connot be an empty string")
@@ -150,7 +154,7 @@ func (d *SecurityRolesDataSource) Read(ctx context.Context, req datasource.ReadR
 		})
 	}
 
-	diags := resp.State.Set(ctx, &state)
+	diags = resp.State.Set(ctx, &state)
 	resp.Diagnostics.Append(diags...)
 	if resp.Diagnostics.HasError() {
 		return

internal/services/currencies/datasource_currencies.go

@@ -119,7 +119,11 @@ func (d *DataSource) Read(ctx context.Context, req datasource.ReadRequest, resp
 	defer exitContext()
 
 	var state DataSourceModel
-	resp.State.Get(ctx, &state)
+	diags := resp.State.Get(ctx, &state)
+	resp.Diagnostics.Append(diags...)
+	if resp.Diagnostics.HasError() {
+		return
+	}
 
 	currencies, err := d.CurrenciesClient.GetCurrenciesByLocation(ctx, state.Location.ValueString())
 	if err != nil {
@@ -139,7 +143,7 @@ func (d *DataSource) Read(ctx context.Context, req datasource.ReadRequest, resp
 		})
 	}
 
-	diags := resp.State.Set(ctx, &state)
+	diags = resp.State.Set(ctx, &state)
 	resp.Diagnostics.Append(diags...)
 	if resp.Diagnostics.HasError() {
 		return

internal/services/environment/datasource_environments.go

@@ -268,8 +268,13 @@ func (d *EnvironmentsDataSource) Read(ctx context.Context, req datasource.ReadRe
 		defaultCurrency, err := d.EnvironmentClient.GetDefaultCurrencyForEnvironment(ctx, env.Name)
 		if err != nil {
 			if !errors.Is(err, customerrors.ErrEnvironmentUrlNotFound) {
-				resp.Diagnostics.AddWarning(fmt.Sprintf("Error when reading default currency for environment %s", env.Name), err.Error())
+				resp.Diagnostics.AddError(
+					fmt.Sprintf("Unexpected error when reading default currency for environment %s", env.Name),
+					err.Error(),
+				)
+				return
 			}
+			// Non-critical error (environment URL not found), just skip currency.
 		} else {
 			currencyCode = defaultCurrency.IsoCurrencyCode
 		}

internal/services/environment_settings/models.go

@@ -94,7 +94,7 @@ func convertFromEnvironmentSettingsModel(ctx context.Context, environmentSetting
 	if auditSettingsObject != nil && helpers.IsKnown(auditSettingsObject) {
 		objectValue, ok := auditSettingsObject.(basetypes.ObjectValue)
 		if !ok {
-			return nil, errors.New("failed to convert audit settings to ObjectValue")
+			return nil, fmt.Errorf("failed to convert audit settings to ObjectValue, got %T: %+v", auditSettingsObject, auditSettingsObject)
 		}
 
 		var auditAndLogsSourceModel AuditSettingsSourceModel

internal/services/locations/api_locations.go

@@ -5,6 +5,7 @@ package locations
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 	"net/url"
 
@@ -35,5 +36,8 @@ func (client *client) GetLocations(ctx context.Context) (locationDto, error) {
 
 	var locations locationDto
 	_, err := client.Api.Execute(ctx, nil, "GET", apiUrl.String(), nil, nil, []int{http.StatusOK}, &locations)
-	return locations, err
+	if err != nil {
+		return locations, fmt.Errorf("failed to get locations: %w", err)
+	}
+	return locations, nil
 }

internal/services/tenant/datasource_tenant.go

@@ -104,7 +104,11 @@ func (d *DataSource) Read(ctx context.Context, req datasource.ReadRequest, resp
 		FedRAMPHighCertificationRequired: types.BoolValue(tenant.FedRAMPHighCertificationRequired),
 	}
 
-	resp.State.Set(ctx, &state)
+	diags := resp.State.Set(ctx, &state)
+	resp.Diagnostics.Append(diags...)
+	if resp.Diagnostics.HasError() {
+		return
+	}
 }
 
 func (d *DataSource) Configure(ctx context.Context, req datasource.ConfigureRequest, resp *datasource.ConfigureResponse) {

internal/services/tenant_settings/resource_tenant_settings.go

@@ -568,9 +568,9 @@ func (r *TenantSettingsResource) Delete(ctx context.Context, req resource.Delete
 	}
 
 	// restore to previous state
-	previousBytes, diag := req.Private.GetKey(ctx, "original_settings")
-	if diag.HasError() {
-		diag.Append(diag...)
+	previousBytes, diags := req.Private.GetKey(ctx, "original_settings")
+	if diags.HasError() {
+		resp.Diagnostics.Append(diags...)
 		return
 	}
 

internal/validators/other_field_required_when_value_of_validator.go

@@ -65,10 +65,18 @@ func (av OtherFieldRequiredWhenValueOfValidator) Validate(ctx context.Context, r
 	}
 
 	currentFieldValue := ""
-	_ = req.Config.GetAttribute(ctx, req.Path, &currentFieldValue)
+	diags := req.Config.GetAttribute(ctx, req.Path, &currentFieldValue)
+	res.Diagnostics.Append(diags...)
+	if res.Diagnostics.HasError() {
+		return
+	}
 
 	if (av.CurrentFieldValueRegex != nil && av.CurrentFieldValueRegex.MatchString(currentFieldValue)) || (av.CurrentFieldValueRegex == nil && currentFieldValue != "") {
-		paths, _ := req.Config.PathMatches(ctx, av.OtherFieldExpression)
+		paths, diags := req.Config.PathMatches(ctx, av.OtherFieldExpression)
+		res.Diagnostics.Append(diags...)
+		if res.Diagnostics.HasError() {
+			return
+		}
 		if paths == nil || len(paths) != 1 {
 			res.Diagnostics.AddError("Other field required when value of validator should have exactly one match", "")
 			return