App Config Spring - Refresh Refactor

[mrm9084]

Description

• Refactors how the state is controlled to work with newer systems instead of being static code.
• Removes a few code warnings

All SDK Contribution checklist:

• The pull request does not introduce [breaking changes]
• [] CHANGELOG is updated for new features, bug fixes or other significant changes.
• I have read the contribution guidelines.

General Guidelines and Best Practices

• Title of the pull request is clear and informative.
• There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

• Pull request includes test coverage for the included changes.

[Copilot]

Pull request overview

This pull request refactors the state management in the Azure App Configuration Spring library, moving from a static singleton pattern to an instance-based approach managed by Spring's dependency injection framework.

Changes:

• Refactored StateHolder from a static singleton to an instance-based class registered in Spring's BootstrapContext and promoted to the ApplicationContext
• Updated AppConfigurationRefreshUtil to accept StateHolder as a constructor parameter instead of using static methods
• Modified tests to use instance-based StateHolder instead of static methods, splitting monolithic tests into focused unit tests
• Applied minor code quality improvements including replacing .size() > 0 with .isEmpty() and improving variable naming

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
StateHolder.java	Converted from package-private singleton to public instance-based class; removed static methods and singleton pattern
StateHolderTest.java	Refactored from single monolithic test to multiple focused tests using instance-based StateHolder
AppConfigurationRefreshUtil.java	Added StateHolder constructor parameter; converted static refresh methods to instance methods
AppConfigurationRefreshUtilTest.java	Updated to use mocked StateHolder instances instead of static method mocking
AppConfigurationPullRefresh.java	Added StateHolder constructor parameter and injected dependency
AppConfigurationPullRefreshTest.java	Updated constructor calls to include StateHolder mock; reorganized imports
AzureAppConfigBoostrapRegistrar.java	Added StateHolder registration in BootstrapContext with promotion to ApplicationContext
AzureAppConfigDataLoader.java	Modified to retrieve StateHolder from BootstrapContext instead of creating new instance
AppConfigurationWatchAutoConfiguration.java	Updated bean creation to retrieve StateHolder from BootstrapContext and pass to dependencies
ConnectionManager.java	Simplified client tracking by replacing currentReplica with lastActiveClient
ConfigStore.java	Improved variable naming (endpoint → validationEndpoint) and validation logic
AppConfigurationProperties.java	Replaced .size() > 0 with .isEmpty() for better code style
AppConfigurationReplicaClient.java	Replaced .size() > 0 with .isEmpty() for better code style
AppConfigurationApplicationSettingPropertySource.java	Added @OverRide annotation and removed unnecessary return statement from void method

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.

Comments suppressed due to low confidence (2)

sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/StateHolder.java:156

• expireState dereferences oldState without a null check. If expireState is called for an endpoint that hasn't had setState(...) called yet (or state was cleared/never initialized), this will throw a NullPointerException. Add a guard (e.g., return early when oldState == null) so expiring an unknown endpoint is a no-op.

    public void expireState(String originEndpoint) {
        State oldState = state.get(originEndpoint);
        long wait = (long) (new SecureRandom().nextDouble() * MAX_JITTER);

        long timeLeft = (int) ((oldState.getNextRefreshCheck().toEpochMilli() - (Instant.now().toEpochMilli())) / 1000);
        if (wait < timeLeft) {

sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/StateHolder.java:195

• updateNextRefreshTime calls getNextRefreshCheck(nextForcedRefresh, ...) without handling nextForcedRefresh == null, which will NPE inside getNextRefreshCheck. Consider initializing nextForcedRefresh in the constructor (e.g., Instant.now()) and/or treating null as "expired" and setting a new value before calling getNextRefreshCheck.

    public void updateNextRefreshTime(Duration refreshInterval, Long defaultMinBackoff) {
        if (refreshInterval != null) {
            Instant newForcedRefresh = getNextRefreshCheck(nextForcedRefresh,
                clientRefreshAttempts, refreshInterval.getSeconds(), defaultMinBackoff);

            if (newForcedRefresh.compareTo(nextForcedRefresh) != 0) {
                clientRefreshAttempts += 1;
            }
            nextForcedRefresh = newForcedRefresh;
        }

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/StateHolder.java:195

• updateNextRefreshTime calls getNextRefreshCheck(nextForcedRefresh, ...) without handling nextForcedRefresh == null. If setNextForcedRefresh was never called (or was called with null), this will NPE when refreshInterval != null. Consider initializing nextForcedRefresh when absent (e.g., Instant.now()) or short-circuiting when it’s null.

    public void updateNextRefreshTime(Duration refreshInterval, Long defaultMinBackoff) {
        if (refreshInterval != null) {
            Instant newForcedRefresh = getNextRefreshCheck(nextForcedRefresh,
                clientRefreshAttempts, refreshInterval.getSeconds(), defaultMinBackoff);

            if (newForcedRefresh.compareTo(nextForcedRefresh) != 0) {
                clientRefreshAttempts += 1;
            }
            nextForcedRefresh = newForcedRefresh;
        }

[Copilot]

expireState will throw a NullPointerException if the endpoint has no entry in state (i.e., oldState is null). Since expireRefreshInterval is a public API entry point and can be called before a store is loaded/initialized (or for an unknown endpoint), please guard for oldState == null and no-op (or log debug) in that case.

[Copilot]

The JavaDoc describes this as "Thread-safe", but clientRefreshAttempts and nextForcedRefresh are mutated/read without synchronization/volatile/atomic guarantees. Since this instance is shared across refresh paths, consider using atomic/volatile (or synchronization) for these fields, or adjust the thread-safety claim accordingly.