Skip to content

Prevent adding PlaceholderResolver multiple times#1276

Merged
bart-vmware merged 1 commit into
mainfrom
fix-duplicate-placeholder-resolvers
Apr 12, 2024
Merged

Prevent adding PlaceholderResolver multiple times#1276
bart-vmware merged 1 commit into
mainfrom
fix-duplicate-placeholder-resolvers

Conversation

@bart-vmware
Copy link
Copy Markdown
Member

@bart-vmware bart-vmware commented Apr 12, 2024

Description

Fixed: Prevent adding PlaceholderResolver multiple times, which breaks the ENV actuator.

Quality checklist

  • Your code complies with our Coding Style.
  • You've updated unit and/or integration tests for your change, where applicable.
  • You've updated documentation for your change, where applicable.
    If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.
  • You've added required license files and/or file headers (explaining where the code came from with proper attribution), where code is copied from StackOverflow, a blog, or OSS.

@bart-vmware bart-vmware marked this pull request as ready for review April 12, 2024 16:14
@bart-vmware bart-vmware requested a review from TimHess April 12, 2024 16:14
@TimHess
Copy link
Copy Markdown
Member

TimHess commented Apr 12, 2024

/azp run Steeltoe.All

@TimHess TimHess added Type/enhancement New feature or request Component/Configuration Issues related to Configuration providers ReleaseLine/4.x Identified as a feature/fix for the 4.x release line labels Apr 12, 2024
@TimHess TimHess added this to the 4.0.0-m1 milestone Apr 12, 2024
@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Copy Markdown
Member

@TimHess TimHess left a comment

Choose a reason for hiding this comment

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

LGTM as-is, only thought is we could log when it isn't added so the user knows they can remove the extra entry

@sonarqubecloud
Copy link
Copy Markdown

@bart-vmware bart-vmware merged commit 7121e8f into main Apr 12, 2024
@bart-vmware bart-vmware deleted the fix-duplicate-placeholder-resolvers branch April 12, 2024 21:05
@bart-vmware
Copy link
Copy Markdown
Member Author

bart-vmware commented Apr 12, 2024

LGTM as-is, only thought is we could log when it isn't added so the user knows they can remove the extra entry

Oh, I saw your comment after merging. We could, but AddPlaceholderResolver is sometimes called by other Steeltoe code: Bootstrap, Config Server, Management. In that case, the user can't fix it. I don't think we should warn in those cases. Also, it's pretty common to run registrations multiple times in ASP.NET. For example, AddOptions, AddLogging etc are called multiple times, just to make sure the basics are in place.

@fennekit
Copy link
Copy Markdown
Contributor

I think this also applies to the EncryptionResolver (see. EncryptionConfigurationExtensions.cs)

@bart-vmware
Copy link
Copy Markdown
Member Author

@fennekit Thanks, I think you're right.

I've created this PR because we've had internal reports from people explicitly calling .AddPlaceholderResolver() and .AddConfigServer() (where the latter implicitly calls this method as well), resulting in problems, such as the ENV endpoint not listing all of the configuration providers.

This problem is less likely to occur with EncryptionResolver because (as far as I'm aware) there's no other Steeltoe code implicitly calling AddEncryptionResolver. However, it doesn't hurt to apply the same safeguard there. Would you be interested in creating a PR for that?

@fennekit
Copy link
Copy Markdown
Contributor

@bart-vmware OK. I will create a PR in the coming week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component/Configuration Issues related to Configuration providers ReleaseLine/4.x Identified as a feature/fix for the 4.x release line Type/enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants