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
• Now contains a feature management bug fix for the week of daylight savings time where there is a day with less than 24 hours.

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/test/java/com/azure/spring/cloud/appconfiguration/config/implementation/AppConfigurationRefreshUtilTest.java:240

• Many tests in AppConfigurationRefreshUtilTest.java still use the removed no-argument constructor new AppConfigurationRefreshUtil() and mock static StateHolder methods (e.g., StateHolder.getLoadState(), StateHolder.getState(), StateHolder.getCurrentState(), StateHolder.getStateFeatureFlag()). Since AppConfigurationRefreshUtil no longer has a no-argument constructor (it now requires a StateHolder instance), and the static methods on StateHolder have been removed, these tests will fail to compile. Affected tests include: refreshStoresCheckSettingsTestNotEnabled, refreshStoresCheckSettingsTestNotLoaded, refreshStoresCheckSettingsTestNotRefreshTime, refreshStoresCheckSettingsTestFailedRequest, refreshStoresCheckSettingsTestRefreshTimeNoChange, refreshStoresPushRefreshEnabledPrimary, refreshStoresPushRefreshEnabledSecondary, refreshStoresCheckSettingsTestTriggerRefresh, refreshStoresCheckFeatureFlagTestNotLoaded, refreshStoresCheckFeatureFlagTestNotRefreshTime, refreshStoresCheckFeatureFlagTestNoChange, refreshStoresCheckFeatureFlagTestTriggerRefresh, and refreshAllWithWatchedConfigurationSettingsTest. Each of these tests needs to be updated to use refreshUtil (the instance-based field) instead of new AppConfigurationRefreshUtil(), and mock the currentStateMock instead of using MockedStatic<StateHolder>.

        try (MockedStatic<StateHolder> stateHolderMock = Mockito.mockStatic(StateHolder.class)) {
            stateHolderMock.when(() -> StateHolder.getLoadState(endpoint)).thenReturn(false);
            stateHolderMock.when(() -> StateHolder.getState(endpoint)).thenReturn(newState);

            // Monitor is disabled
            RefreshEventData eventData = new AppConfigurationRefreshUtil().refreshStoresCheck(clientFactoryMock,
                Duration.ofMinutes(10),
                (long) 60, replicaLookUpMock);
            assertFalse(eventData.getDoRefresh());
            verify(clientOriginMock, times(0)).getWatchKey(Mockito.anyString(), Mockito.anyString(),
                Mockito.any(Context.class));
        }
    }

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

[Copilot]

Line 44 calls context.get(AppConfigurationReplicaClientFactory.class) but its return value is discarded — the factory was already retrieved with context.getOrElse(...) on line 41 into clientFactory. This is dead code that has no effect and creates unnecessary confusion for readers.

[Copilot]

Pull request overview

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

[Copilot]

In watchedConfigurationSettingsWithChangeDetectedTest, a disabledFeatureStore is created and setEnabled(false) is called on it, but there is no when(connectionManagerMock.getFeatureFlagStore()).thenReturn(disabledFeatureStore) stub. Compare this with the similar test watchedConfigurationSettingsNoChangeTest at line 654-656 which correctly stubs this call. While the test likely passes because refreshStoresCheck returns early when doRefresh is true (before reaching connection.getFeatureFlagStore()), the setup is incomplete and leaves the disabledFeatureStore variable unused. This should be stubbed for correctness and test intent clarity.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 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:197

• updateNextRefreshTime calls getNextRefreshCheck(nextForcedRefresh, ...) when refreshInterval != null, but nextForcedRefresh can be null (per getNextForcedRefresh() JavaDoc and when setNextForcedRefresh hasn’t been called). This can cause a NullPointerException inside getNextRefreshCheck (and also in newForcedRefresh.compareTo(nextForcedRefresh)). Consider initializing nextForcedRefresh when first needed (e.g., treat null as expired and set to Instant.now()/Instant.EPOCH), or guard null explicitly 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]

BootstrapContext#get(StateHolder.class) will throw if StateHolder was never registered (e.g., when App Configuration config-data isn’t resolvable/used or stores are disabled). Since this call happens before the null checks for clientFactory/replicaLookUp, the auto-configuration can fail during startup instead of returning null. Use context.getOrElse(StateHolder.class, null) (or isRegistered check) and only require it when the other dependencies are present; fall back to constructing AppConfigurationRefreshUtil with a new StateHolder if needed.

Suggested change

	AppConfigurationReplicaClientFactory clientFactory = context.getOrElse(AppConfigurationReplicaClientFactory.class, null);
	ReplicaLookUp replicaLookUp = context.getOrElse(ReplicaLookUp.class, null);
	StateHolder stateHolder = context.get(StateHolder.class);
	if (clientFactory == null || replicaLookUp == null) {
	return null;
	}
	AppConfigurationReplicaClientFactory clientFactory =
	context.getOrElse(AppConfigurationReplicaClientFactory.class, null);
	ReplicaLookUp replicaLookUp = context.getOrElse(ReplicaLookUp.class, null);
	StateHolder stateHolder = context.getOrElse(StateHolder.class, null);
	if (clientFactory == null || replicaLookUp == null) {
	return null;
	}
	if (stateHolder == null) {
	stateHolder = new StateHolder();
	}

[Copilot]

This DST-specific weekly recurrence logic isn’t covered by a unit test. Since there is already a comprehensive RecurrenceEvaluatorTest, add a test case that crosses a DST transition (e.g., America/New_York around the spring-forward week) and asserts the expected previous occurrence/match behavior so this regression stays protected.

[Copilot]

PR description indicates a user-visible bug fix (DST week) and a refresh refactor, but the checklist shows CHANGELOG isn’t updated. Please add an entry to the appropriate module CHANGELOG(s) for the fix/refactor per Azure SDK release guidelines.