Merged Error Handling Issues (d_error_handling_1)

This file contains all the error handling issues found in the d_error_handling_1 directory, merged into a single document for easier review and management.

ISSUE 1

File: /workspaces/terraform-provider-power-platform/internal/services/environment_groups/api_environment_group.go

Title

Missing Error Wrapping with Context

Problem

In several methods (for example, RemoveEnvironmentFromEnvironmentGroup, CreateEnvironmentGroup, UpdateEnvironmentGroup, GetEnvironmentsInEnvironmentGroup), errors from dependencies or API executions are simply returned, losing important context about the operation in which they occurred.

Impact

This makes it much harder to debug or trace errors, especially when the same error (e.g., HTTP 500) can occur in multiple functions. It results in reduced maintainability and supportability.

Severity: Medium

Location

tenantDto, err := client.TenantApi.GetTenant(ctx)
if err != nil {
    return err
}

...and elsewhere (return err with no context wrapping).

Code Issue

tenantDto, err := client.TenantApi.GetTenant(ctx)
if err != nil {
    return err
}

Fix

Wrap errors with additional context using fmt.Errorf("context: %w", err):

tenantDto, err := client.TenantApi.GetTenant(ctx)
if err != nil {
    return fmt.Errorf("failed to get tenant: %w", err)
}

Similarly, wrap all errors returned by API calls and downstream dependencies with a helpful message.

ISSUE 2

File: /workspaces/terraform-provider-power-platform/internal/services/environment_group_rule_set/api_environment_group_rule_set.go

Title

Use of Error Value After Control Flow in GetEnvironmentGroupRuleSet

Problem

In the GetEnvironmentGroupRuleSet function, when handling the http.StatusNoContent response, the code wraps and returns the err variable, which at that point is still the error value from a potentially successful API call. In this branch, err will probably be nil—there has been no new error between the API call and this status check. Wrapping and returning it may produce an ambiguous or misleading error.

Impact

This could lead to confusion and uninformative or misleading error messages for the caller or user, as a nil error may be wrapped and returned, producing unexpected or incorrect error output. Severity: Medium.

Location

if resp.HttpResponse.StatusCode == http.StatusNoContent {
    return nil, customerrors.WrapIntoProviderError(err, customerrors.ERROR_OBJECT_NOT_FOUND, "rule set '%s' not found")
}

Code Issue

if resp.HttpResponse.StatusCode == http.StatusNoContent {
    return nil, customerrors.WrapIntoProviderError(err, customerrors.ERROR_OBJECT_NOT_FOUND, "rule set '%s' not found")
}

Fix

Explicitly provide an informative error, not relying on wrapping a (likely) nil error:

if resp.HttpResponse.StatusCode == http.StatusNoContent {
    return nil, customerrors.WrapIntoProviderError(
        fmt.Errorf("rule set '%s' not found", environmentGroupId),
        customerrors.ERROR_OBJECT_NOT_FOUND,
        "rule set '%s' not found",
        environmentGroupId,
    )
}

ISSUE 3

File: /workspaces/terraform-provider-power-platform/internal/services/environment_group_rule_set/api_environment_group_rule_set.go

Title

Error Handling: Silent Failure in Delete Method

Problem

In the DeleteEnvironmentGroupRuleSet method, the HTTP response is ignored; only the error from client.Api.Execute is returned. This means HTTP status codes that are not mapped to an explicit error (but indicate possible API issues) could cause silent success or unhandled failures.

Impact

Potential silent failures or successes if the API response's HTTP status code is not properly handled by the underlying API client. It reduces reliability and observability for the caller. Severity: Medium.

Location

_, err = client.Api.Execute(ctx, nil, "DELETE", apiUrl.String(), nil, nil, []int{http.StatusOK}, nil)

return err

Code Issue

_, err = client.Api.Execute(ctx, nil, "DELETE", apiUrl.String(), nil, nil, []int{http.StatusOK}, nil)

return err

Fix

Check the result and response explicitly, or document/verify that all non-OK statuses will result in a non-nil error via the API client:

resp, err := client.Api.Execute(ctx, nil, "DELETE", apiUrl.String(), nil, nil, []int{http.StatusOK}, nil)
if err != nil {
    return err
}

// Optionally: inspect the response for additional guarantees or logging

return nil

ISSUE 4

File: /workspaces/terraform-provider-power-platform/internal/services/environment_group_rule_set/api_environment_group_rule_set.go

Title

Unnecessary Parameters Length Check After Create Call

Problem

In CreateEnvironmentGroupRuleSet, the code checks if environmentGroupRuleSet.Parameters has zero length, and returns an error if so. However, there is not enough surrounding code context about whether the Parameters field is guaranteed to be present and meaningful. If the backend API succeeds (returns 201 Created), returning an error due to empty Parameters risks masking success due to a detail of backend payload design or future changes.

Impact

This affects reliability and maintainability. Down the line, code may break or return false errors if the Parameters field is unused, deprecated, or simply empty according to business logic. Severity: Medium.

Location

if len(environmentGroupRuleSet.Parameters) == 0 {
    return nil, fmt.Errorf("no environment group ruleset found for environment group id %s", environmentGroupId)
}

Code Issue

if len(environmentGroupRuleSet.Parameters) == 0 {
    return nil, fmt.Errorf("no environment group ruleset found for environment group id %s", environmentGroupId)
}

Fix

Evaluate whether this check is truly necessary—preferably, rely on the HTTP status code and validated error-handling from the API response. If an additional check is required by business logic, include a comment to explain its necessity or consider handling this at a higher level.

// If empty Parameters is a valid API response, remove the following check.
// Otherwise, consider clarifying its necessity and error messaging.
if len(environmentGroupRuleSet.Parameters) == 0 {
    return nil, fmt.Errorf("no environment group ruleset parameters found for environment group id %s", environmentGroupId)
}

ISSUE 5

File: /workspaces/terraform-provider-power-platform/internal/services/environment_group_rule_set/resource_environment_group_rule_set.go

Title

Improper error message and mismatched type check in Configure

Problem

Error message in Configure returns "Expected *http.Client, got: %T", though we are expecting *api.ProviderClient.

Impact

Low.

• The message can mislead users and hinder debugging.

Location

resp.Diagnostics.AddError(
    "Unexpected Resource Configure Type",
    fmt.Sprintf("Expected *http.Client, got: %T. Please report this issue to the provider developers.", req.ProviderData),
)

Code Issue

resp.Diagnostics.AddError(
    "Unexpected Resource Configure Type",
    fmt.Sprintf("Expected *http.Client, got: %T. Please report this issue to the provider developers.", req.ProviderData),
)

Fix

Update Expected *http.Client to the correct type. For example:

resp.Diagnostics.AddError(
    "Unexpected Resource Configure Type",
    fmt.Sprintf("Expected *api.ProviderClient, got: %T. Please report this issue to the provider developers.", req.ProviderData),
)

ISSUE 6

File: /workspaces/terraform-provider-power-platform/internal/services/environment_group_rule_set/resource_environment_group_rule_set.go

Title

Potential resource drift or confusion with pointer model usage in Create/Update

Problem

In Create and Update functions, the code retrieves the plan or state into a pointer (*environmentGroupRuleSetResourceModel), but when setting resource state, it passes the pointer directly to resp.State.Set, which expects the concrete value, not a pointer. Some utilities will automatically dereference, but this may lead to subtle bugs or errors in diagnostics, especially if the plan is not fully valid at this point.

Impact

Medium.

• May result in incorrect behavior such as state not being stored properly or resource model being misrepresented.

Location

resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
//...
resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...)

Code Issue

resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
//...
resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...)

Fix

Dereference the pointer (once non-nil) when passing to Set:

resp.Diagnostics.Append(resp.State.Set(ctx, plan)...)

or (for plan as pointer):

if plan != nil {
    resp.Diagnostics.Append(resp.State.Set(ctx, *plan)...)
}

This may also clarify error messages and resource state behavior.