Merged Pointer Handling Issues

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

ISSUE 1

File: /workspaces/terraform-provider-power-platform/internal/services/admin_management_application/api_admin_management_application.go

Title

Return Pointer to Local Variable (DTO Struct)

Problem

Returning a pointer to a local variable for a DTO is not strictly unsafe in Go, but it usually warrants explicit attention, especially with larger structs or concurrency.

Impact

Low. For small structs like DTOs this is commonly acceptable, but it's worth noting as the function API could inadvertently propagate local variable lifetime surprises during future refactoring.

Location

In each function returning *adminManagementApplicationDto.

Code Issue

var adminApp adminManagementApplicationDto
_, err := client.Api.Execute(ctx, nil, "GET", apiUrl.String(), nil, nil, []int{http.StatusOK}, &adminApp)

return &adminApp, err

Fix

Ensure this pattern is intentional and add a small comment if kept. For robust code, prefer to clarify by documenting, or for larger structs, perhaps prefer returning by value if suitable.

// Returning pointer to local variable is OK here (small DTO), but document reasoning
return &adminApp, err

ISSUE 2

File: /workspaces/terraform-provider-power-platform/internal/services/analytics_data_export/api_analytics_data_exports.go

Title

Ambiguous Usage of *[]AnalyticsDataDto as Return Type in GetAnalyticsDataExport

Problem

The function GetAnalyticsDataExport returns a *[]AnalyticsDataDto (pointer to a slice), which is not idiomatic Go. Slices are already reference types and returning a pointer to a slice rarely provides benefit. This usage can introduce confusion and potential misuse.

Impact

The code is less idiomatic and could create confusion about ownership, mutability, and nilness. Returning a pointer to a slice may also increase the risk of bugs and makes client code harder to reason about. Severity: Medium.

Location

func (client *Client) GetAnalyticsDataExport(ctx context.Context) (*[]AnalyticsDataDto, error)

Code Issue

func (client *Client) GetAnalyticsDataExport(ctx context.Context) (*[]AnalyticsDataDto, error)

And:

return &adr.Value, nil

Fix

Return a plain slice type:

func (client *Client) GetAnalyticsDataExport(ctx context.Context) ([]AnalyticsDataDto, error) {
    ...
    return adr.Value, nil
}

Update all usages accordingly.

ISSUE 3

File: /workspaces/terraform-provider-power-platform/internal/services/powerapps/api_powerapps.go

Title

Unnecessary Use of Pointer for receiver

Problem

The GetPowerApps method is defined on a pointer receiver *client despite the struct's fields either being pointers themselves or unexported, and no apparent mutation occurs.

Impact

Unnecessarily using pointer receivers can be avoided for better clarity and to signal immutability. Severity: Low.

Location

func (client *client) GetPowerApps(ctx context.Context) ([]powerAppBapiDto, error)

Code Issue

func (client *client) GetPowerApps(ctx context.Context) ([]powerAppBapiDto, error)

Fix

If mutation isn't needed, use value receiver:

func (client client) GetPowerApps(ctx context.Context) ([]powerAppBapiDto, error)

Alternatively, if mutation is (or will be) needed, this can be left as-is. Consider reviewing if pointer receivers are necessary.

ISSUE 4

File: /workspaces/terraform-provider-power-platform/internal/services/solution_checker_rules/client.go

Title

Method Receiver Should Be Pointer to Avoid Unintended Copy

Problem

The method GetSolutionCheckerRules uses a non-pointer receiver (c *client). This is correct. However, if client is used without pointer semantics (because the constructor returns a value), the receiver and method call will work on a copy rather than the original, potentially leading to bugs if the method ever modifies state. The current constructor returns a value, so clients may end up using non-pointer semantics by mistake. This is a code structure issue.

Impact

Potential unintended copies and inconsistent usage. If the struct gets fields that need mutation or synchronization, could cause bugs. Severity: low to medium.

Location

• Constructor and receiver usage for methods on client

Code Issue

func newSolutionCheckerRulesClient(apiClient *api.Client) client {
 return client{
  Api:               apiClient,
  environmentClient: environment.NewEnvironmentClient(apiClient),
 }
}

func (c *client) GetSolutionCheckerRules(ctx context.Context, environmentId string) ([]ruleDto, error) {
 //...
}

Fix

Return a pointer from the constructor, and ensure all usages are as pointer. This is considered good Go client API practice.

func NewSolutionCheckerRulesClient(apiClient *api.Client) *Client {
 return &Client{
  Api:               apiClient,
  environmentClient: environment.NewEnvironmentClient(apiClient),
 }
}

func (c *Client) GetSolutionCheckerRules(ctx context.Context, environmentId string) ([]ruleDto, error) {
 //...
}

ISSUE 5

File: /workspaces/terraform-provider-power-platform/internal/services/environment/datasource_environments.go

Title

Inconsistent Attribute Pointer Usage in Schema Definition

Problem

In the schema definition section (inside Schema method), most attributes are value types. However, for certain attributes such as billing_policy_id and currency_code, they are created as pointers to StringAttribute (e.g., &schema.StringAttribute{...}), which is unnecessary and inconsistent since other similar attributes are values and the documentation for Terraform Plugin Framework suggests value type usage unless mutability is required.

Impact

Unnecessary use of pointers leads to inconsistent code and possible confusion for maintainers; it also may increase risk of accidental nil dereference. Severity: Low.

Location

"billing_policy_id": &schema.StringAttribute{
    MarkdownDescription: "Billing policy id (guid) for pay-as-you-go environments using Azure subscription billing",
    Computed:            true,
},
...
"currency_code": &schema.StringAttribute{
    MarkdownDescription: "Currency name (EUR, USE, GBP etc.)",
    Computed:            true,
},

Code Issue

"billing_policy_id": &schema.StringAttribute{
    MarkdownDescription: "Billing policy id (guid) for pay-as-you-go environments using Azure subscription billing",
    Computed:            true,
},
...
"currency_code": &schema.StringAttribute{
    MarkdownDescription: "Currency name (EUR, USE, GBP etc.)",
    Computed:            true,
},

Fix

Change field assignments to non-pointer value usages, like so:

"billing_policy_id": schema.StringAttribute{
    MarkdownDescription: "Billing policy id (guid) for pay-as-you-go environments using Azure subscription billing",
    Computed:            true,
},
...
"currency_code": schema.StringAttribute{
    MarkdownDescription: "Currency name (EUR, USE, GBP etc.)",
    Computed:            true,
},

This makes the code consistent with the rest of the schema and follows the best practices for attribute assignment.

ISSUE 6

File: /workspaces/terraform-provider-power-platform/internal/services/tenant_settings/dto.go

Title

Repetitive If-Null-And-Unknown Pattern Leads to Boilerplate

Problem

Throughout the DTO conversion logic, code is repeatedly written for null and unknown checks before assigning or converting values:

if !value.IsNull() && !value.IsUnknown() {
    target = value.ValueBoolPointer()
}

This leads to excessive boilerplate and makes the code harder to read and maintain.

Impact

• Reduced code maintainability.
• More places for subtle bugs if new fields are added or checks are missed.
• Contributes to unnecessarily bloated and less readable functions.
• Increases technical debt over time. Severity: medium.

Location

• All major conversion functions: e.g., convertFromTenantSettingsModel, convertTeamsIntegrationModel, convertPowerAppsModel, etc.

Code Issue

if !tenantSettings.DisableTrialEnvironmentCreationByNonAdminUsers.IsNull() && !tenantSettings.DisableTrialEnvironmentCreationByNonAdminUsers.IsUnknown() {
    tenantSettingsDto.DisableTrialEnvironmentCreationByNonAdminUsers = tenantSettings.DisableTrialEnvironmentCreationByNonAdminUsers.ValueBoolPointer()
}

Fix

Abstract common check-use patterns into reusable helper functions, such as:

func getBoolPointer(v basetypes.BoolValue) *bool {
    if !v.IsNull() && !v.IsUnknown() {
        return v.ValueBoolPointer()
    }
    return nil
}

// Usage:
tenantSettingsDto.DisableTrialEnvironmentCreationByNonAdminUsers = getBoolPointer(tenantSettings.DisableTrialEnvironmentCreationByNonAdminUsers)

This enables more concise and reliable code, and helps enforce best practices for null/unknown-safe conversions.

ISSUE 7

File: /workspaces/terraform-provider-power-platform/internal/services/connection/resource_connection.go

Title

Unnecessary use of double-pointer for ResourceModel variable plan

Problem

The code uses double-pointer syntax for plan (i.e., var plan *ResourceModel) and then passes its address around (e.g., &plan), but Go's Terraform Plugin Framework typically expects a value or a single pointer. This can lead to confusion and inconsistent state handling.

Impact

Low: This is mostly a readability/maintainability issue. It may introduce subtle bugs if pointer semantics are misunderstood, but does not currently break the code.

Location

Create, Update methods:

Code Issue

var plan *ResourceModel
resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
// ... then plan usage

Fix

Use a non-pointer or a single pointer appropriately for plan with Terraform plugin calls.

var plan ResourceModel
resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...) // pass address of value, not address of pointer
// ... use plan fields as plan.Foo

If you need plan to be a pointer, then don't take &plan (it's already a pointer):

var plan *ResourceModel
resp.Diagnostics.Append(req.Plan.Get(ctx, plan)...) // just pass plan

Choose the style based on framework expectations and keep it consistent throughout the file.

ISSUE 8

File: /workspaces/terraform-provider-power-platform/internal/services/data_record/resource_data_record.go

Title

Potential Data Loss: Use of Pointer to DataRecordResourceModel in Update/Read

Problem

The functions Update and Read use pointers for *DataRecordResourceModel when reading state/plan. The rest of the code and Terraform conventions expect passing by value to ensure correct zero-value/unknown behavior, and because the struct is not especially large.

Impact

• Severity: Medium
• Can cause nil dereference panics if the struct is not initialized.
• Can result in partial or incorrect updates as pointer fields may not accurately reflect unknowns or non-set fields.

Location

Read, Update function signatures and variable declarations.

Code Issue

var state *DataRecordResourceModel
resp.Diagnostics.Append(req.State.Get(ctx, &state)...)
// ...
var plan *DataRecordResourceModel
resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)

Fix

Use value types instead of pointers, and pass value addresses:

var state DataRecordResourceModel
resp.Diagnostics.Append(req.State.Get(ctx, &state)...)

var plan DataRecordResourceModel
resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)

This ensures state is always properly initialized and compatible with Terraform conventions.

ISSUE 9

File: /workspaces/terraform-provider-power-platform/internal/customtypes/uuid.go

Title

Missing documentation/comments on exported functions

Problem

The file defines multiple exported functions (e.g., NewUUIDNull, NewUUIDUnknown, NewUUIDValue, NewUUIDPointerValue, NewUUIDValueMust, NewUUIDPointerValueMust) that do not have Go-style documentation comments. This makes it less clear for users and developers as to the intent and usage of each function, especially in a public API.

Impact

This is a low-severity code structure, maintainability, and readability issue. Lack of documentation reduces maintainability, readability, and can make onboarding new developers harder, or cause external users to misuse the exported API.

Location

Applies to all exported functions in this file.

Code Issue

func NewUUIDNull() UUID {
 return UUID{
  StringValue: basetypes.NewStringNull(),
 }
}

(And similar for the others.)

Fix

Add Go-style documentation comments immediately preceding each exported function, explaining its purpose, parameters, and return values.

// NewUUIDNull returns a UUID representing a null value.
func NewUUIDNull() UUID {
 return UUID{
  StringValue: basetypes.NewStringNull(),
 }
}

// NewUUIDUnknown returns a UUID representing an unknown value.
func NewUUIDUnknown() UUID {
 return UUID{
  StringValue: basetypes.NewStringUnknown(),
 }
}

// NewUUIDValue returns a UUID initialized with the given string value.
// If the string is not a valid UUID, the returned value may be invalid.
func NewUUIDValue(value string) UUID {
 return UUID{
  StringValue: basetypes.NewStringValue(value),
 }
}

Rememeber to:

1. Unit Tests
2. Linter
3. Regenerate Dos
4. Run Changie