App Config Provider Cleanup#49026
Open
mrm9084 wants to merge 8 commits into
Open
Conversation
mrm9084
requested review from
a team,
Netyyyy,
avanigupta,
moarychan,
nroutray,
rujche and
saragluna
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR performs a broad cleanup/refactor of the Spring Cloud Azure App Configuration provider implementation, consolidating duplicated listing logic, tightening concurrency/backoff behavior, improving immutability, and fixing a handful of correctness and documentation issues.
Changes:
- Consolidates settings/feature-flag paging into a single page-based listing path (
listSettingsByPage) and updates callers/tests accordingly. - Fixes and hardens replica discovery/backoff/refresh behavior (semaphore release safety, retry sleep, backoff calculation fix, null-safety).
- Improves maintainability via immutability, primitive boolean/int usage, safer label handling, and corrected JavaDocs/spelling/logging.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/AppConfigurationReplicaClient.java | Renames/centralizes page-based settings listing; removes duplicated feature-flag listing; improves error handling for watch checks. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/AppConfigurationReplicaClientsBuilder.java | Refactors client construction paths (endpoints vs connection strings), adds safer connection-string handling/redaction, and clarifies retry configuration. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/autofailover/ReplicaLookUp.java | Makes replica discovery more robust (concurrency-safe maps, semaphore release in finally, sleep between DNS retries, cleaner flow). |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/autofailover/SRVRecord.java | Implements Comparable, fixes protocol constant spelling, and updates SRV ordering logic. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/FeatureFlagClient.java | Uses listSettingsByPage, fixes context propagation for tracing, avoids inserting null features, and reuses mapper. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/BackoffTimeCalculator.java | Fixes backoff calculation (removes double-add of min backoff). |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/properties/AppConfigurationProperties.java | Simplifies duplicate-endpoint detection using a Set. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/configuration/WatchedConfigurationSettings.java | Makes settings container immutable by removing setters and finalizing fields. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/State.java | Makes refresh attempt tracking immutable (new State created per increment). |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/ConnectionManager.java | Simplifies available-client filtering and adds null-safety in backoff updates. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/AppConfigurationRefreshUtil.java | Uses primitive boolean and adjusts feature-flag refresh logic flow. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/AppConfigurationReplicaClientFactory.java | Removes static global connection map in favor of instance state. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/stores/AppConfigurationSecretClientManager.java | Replaces instance timeout field with constant and aligns JavaDoc. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/properties/FeatureFlagStore.java | Switches to primitives and simplifies empty-checks. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/properties/FeatureFlagKeyValueSelector.java | Avoids mutating incoming profiles list and uses safer label array creation. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/properties/AppConfigurationKeyValueSelector.java | Uses safer toArray overload and cleans imports/formatting. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/AppConfigurationApplicationSettingPropertySource.java | Avoids mutating label array view and replaces regex-based trimming with substring logic. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/AzureAppConfigDataLoader.java | Updates to use the consolidated page-based listing method. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/AzureAppConfigDataLocationResolver.java | Uses primitive boolean for enablement check. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/AzureAppConfigBootstrapRegistrar.java | Removes redundant customizer application (handled via factory customization). |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/AppConfigurationSnapshotPropertySource.java | Updates renamed feature-flag processing method call. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/StateHolder.java | Switches to primitives and removes an unnecessary cast. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/feature/entity/Feature.java | Corrects constructor JavaDoc parameter descriptions. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/test/java/com/azure/spring/cloud/appconfiguration/config/implementation/AppConfigurationReplicaClientTest.java | Updates tests to the consolidated listSettingsByPage path. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/test/java/com/azure/spring/cloud/appconfiguration/config/implementation/FeatureFlagClientTest.java | Updates mocks to use listSettingsByPage and simplifies context usage. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/test/java/com/azure/spring/cloud/appconfiguration/config/implementation/autofailover/SRVRecordTest.java | Updates expectations to match new SRV ordering behavior. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/test/java/com/azure/spring/cloud/appconfiguration/config/implementation/AppConfigurationReplicaClientBuilderTest.java | Aligns assertions with updated connection-string parsing/error messaging. |
| sdk/spring/spring-cloud-azure-appconfiguration-config/src/test/java/com/azure/spring/cloud/appconfiguration/config/implementation/stores/ConfigStoreTest.java | Import/format cleanup only. |
Comment on lines
+137
to
144
| if (!existingEndpoints.add(endpoint)) { | ||
| throw new IllegalArgumentException("Duplicate store name exists."); | ||
| } | ||
| existingEndpoints.put(endpoint, true); | ||
| } | ||
| } else if (StringUtils.hasText(store.getEndpoint())) { | ||
| if (existingEndpoints.containsKey(store.getEndpoint())) { | ||
| if (!existingEndpoints.add(store.getEndpoint())) { | ||
| throw new IllegalArgumentException("Duplicate store name exists."); | ||
| } |
Comment on lines
+216
to
+228
| private AppConfigurationReplicaClient buildClientFromEndpoint(String endpoint, String originEndpoint, | ||
| DefaultAzureCredential sharedCredential) { | ||
| ConfigurationClientBuilder builder = createBuilderInstance(); | ||
| // When credentialConfigured is false, no credential was provided via Spring Cloud Azure | ||
| // auto-config, so we fall back to DefaultAzureCredential. Prefer the shared instance | ||
| // when available; create a new one only for single-endpoint failover calls (null passed in). | ||
| if (!credentialConfigured) { | ||
| builder.credential(sharedCredential != null ? sharedCredential | ||
| : new DefaultAzureCredentialBuilder().build()); | ||
| } | ||
| builder.endpoint(endpoint); | ||
| return modifyAndBuildClient(builder, endpoint, originEndpoint, 0); | ||
| } |
Comment on lines
+224
to
+228
| : new DefaultAzureCredentialBuilder().build()); | ||
| } | ||
| builder.endpoint(endpoint); | ||
| return modifyAndBuildClient(builder, endpoint, originEndpoint, 0); | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
+278
to
282
| if (!variantsValue.isEmpty()) { | ||
| List<Map<String, Object>> sortedVariants = variantsValue.stream() | ||
| .filter(v -> allocatedVariants.contains(v.get("name"))) | ||
| .filter(v -> allocatedVariants.contains((String) v.get("name"))) | ||
| .sorted(Comparator.comparing(v -> (String) v.get("name"))) | ||
| .collect(Collectors.toList()); |
Comment on lines
+400
to
+409
| // Capture the policy passed to addPolicy and verify it contains the correct replica count | ||
| ArgumentCaptor<HttpPipelinePolicy> policyCaptor = ArgumentCaptor.forClass(HttpPipelinePolicy.class); | ||
| verify(builderMock, atLeastOnce()).addPolicy(policyCaptor.capture()); | ||
|
|
||
| List<HttpPipelinePolicy> policies = policyCaptor.getAllValues(); | ||
| // All captured policies should be BaseAppConfigurationPolicy instances | ||
| for (HttpPipelinePolicy policy : policies) { | ||
| assertTrue(policy instanceof BaseAppConfigurationPolicy, | ||
| "Expected BaseAppConfigurationPolicy but got " + policy.getClass().getSimpleName()); | ||
| } |
rujche
approved these changes
May 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
There is a number of issues that have built up over time that this pr fixes.
BooleanintobooleanminBackoffNanowas added twice.