Skip to content

fix: handling computed attributes with NonNullState plan modifier#266

Open
diya-dhan wants to merge 2 commits intomainfrom
issue_260
Open

fix: handling computed attributes with NonNullState plan modifier#266
diya-dhan wants to merge 2 commits intomainfrom
issue_260

Conversation

@diya-dhan
Copy link
Contributor

@diya-dhan diya-dhan commented Feb 16, 2026

Purpose

Closes #260 #261

When performing a GET operation on any SAP managed application, the attribute saml2Configuration.digestAlgorithm unless the application is updated via a PUT call. This leads to an inconsistent apply when updates are executed.

This is overcome by modifying the plan modifier of the attribute such that the null value can be updated. The plan modifiers for a few other attributes have been modified as well.

Does this introduce a breaking change?

[ ] Yes
[x] No

Pull Request Type

What kind of change does this Pull Request introduce?

[x] Bugfix
[ ] Feature
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

  • Test the code via automated test
make test

What to Check

Verify that the following are valid:

  • Automated tests are executed successfully

Checklist for reviewer

The following organizational tasks must be completed before merging this PR:

  • The PR status on the Project board is set (typically "in review").
  • The PR has the matching labels assigned to it.
  • If the PR closes an issue, the issue is referenced.
  • Possible follow-up issues are created and linked.

@github-actions github-actions bot added this to the 0.5.0-beta1 milestone Feb 16, 2026
@github-actions github-actions bot added the bug Something isn't working label Feb 16, 2026
@sonarqubecloud
Copy link

@lechnerc77 lechnerc77 requested a review from Copilot February 17, 2026 09:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes inconsistent Terraform apply behavior for computed SAP-managed attributes (notably saml2Configuration.digestAlgorithm) by allowing null state to be planned/updated correctly.

Changes:

  • Switches multiple computed/optional attributes to UseNonNullStateForUnknown() plan modifiers.
  • Adds UseNonNullStateForUnknown() plan modifiers for several Int32 and String attributes.
  • Adjusts SAML2 config state mapping to only set certain fields when non-empty.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
sci/provider/type_application.go Updates SAML2 attribute mapping to avoid forcing empty values into state.
sci/provider/resource_application.go Replaces UseStateForUnknown() with UseNonNullStateForUnknown() and adds missing plan modifiers to improve null-handling for computed attributes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +345 to +347
if len((saml2Res.DefaultNameIdFormat)) > 0 {
saml2Config.DefaultNameIdFormat = types.StringValue(reflect.ValueOf(saml2Res.DefaultNameIdFormat).String())
}
Copy link

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reflect.ValueOf(...).String() is risky here: if DefaultNameIdFormat is not a string kind (e.g., a *string or custom type), this will not yield the desired value (and can produce a placeholder like "<*string Value>"). Since this field was previously passed directly to types.StringValue(...), set it directly (or explicitly dereference/convert based on the actual type) and drop the redundant extra parentheses in len((...)). This avoids potential incorrect state values for default_name_id_format.

Copilot uses AI. Check for mistakes.
saml2Config.SamlMetadataUrl = types.StringValue(saml2Res.SamlMetadataUrl)
}

// Saml Digest Algorithm
Copy link

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The acronym should be capitalized as SAML in these comments for consistency (SAML Digest Algorithm, SAML Default NameId Format).

Copilot uses AI. Check for mistakes.
saml2Config.DigestAlgorithm = types.StringValue(saml2Res.DigestAlgorithm)
}

// Saml Default NameId Format
Copy link

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The acronym should be capitalized as SAML in these comments for consistency (SAML Digest Algorithm, SAML Default NameId Format).

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Inconsistent result after apply

2 participants