From e24b6edbe5aa5596b6d7570217893eeab15f2052 Mon Sep 17 00:00:00 2001 From: Philipp Page Date: Tue, 29 Jul 2025 09:40:45 +0200 Subject: [PATCH 1/2] fix(parameters): Correctly check for empty values in AppConfig Parameters Provider. --- .../appconfig/AppConfigProvider.java | 40 +++++++------ .../appconfig/AppConfigProviderTest.java | 60 ++++++++++--------- 2 files changed, 52 insertions(+), 48 deletions(-) diff --git a/powertools-parameters/powertools-parameters-appconfig/src/main/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProvider.java b/powertools-parameters/powertools-parameters-appconfig/src/main/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProvider.java index 5fd272c9b..ddb85c540 100644 --- a/powertools-parameters/powertools-parameters-appconfig/src/main/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProvider.java +++ b/powertools-parameters/powertools-parameters-appconfig/src/main/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProvider.java @@ -16,6 +16,8 @@ import java.util.HashMap; import java.util.Map; + +import software.amazon.awssdk.core.SdkBytes; import software.amazon.awssdk.services.appconfigdata.AppConfigDataClient; import software.amazon.awssdk.services.appconfigdata.model.GetLatestConfigurationRequest; import software.amazon.awssdk.services.appconfigdata.model.GetLatestConfigurationResponse; @@ -47,7 +49,7 @@ public class AppConfigProvider extends BaseProvider { private final HashMap establishedSessions = new HashMap<>(); AppConfigProvider(CacheManager cacheManager, TransformationManager transformationManager, - AppConfigDataClient client, String environment, String application) { + AppConfigDataClient client, String environment, String application) { super(cacheManager, transformationManager); this.client = client; this.application = application; @@ -63,7 +65,6 @@ public static AppConfigProviderBuilder builder() { return new AppConfigProviderBuilder(); } - /** * Retrieve the parameter value from the AppConfig parameter store.
* @@ -76,13 +77,12 @@ protected String getValue(String key) { // so that we can the initial token. If we already have a session, we can take // the next request token from there. EstablishedSession establishedSession = establishedSessions.getOrDefault(key, null); - String sessionToken = establishedSession != null ? - establishedSession.nextSessionToken : - client.startConfigurationSession(StartConfigurationSessionRequest.builder() - .applicationIdentifier(this.application) - .environmentIdentifier(this.environment) - .configurationProfileIdentifier(key) - .build()) + String sessionToken = establishedSession != null ? establishedSession.nextSessionToken + : client.startConfigurationSession(StartConfigurationSessionRequest.builder() + .applicationIdentifier(this.application) + .environmentIdentifier(this.environment) + .configurationProfileIdentifier(key) + .build()) .initialConfigurationToken(); // Get the configuration using the token @@ -93,16 +93,18 @@ protected String getValue(String key) { // Get the next session token we'll use next time we are asked for this key String nextSessionToken = response.nextPollConfigurationToken(); - // Get the value of the key. Note that AppConfig will return null if the value - // has not changed since we last asked for it in this session - in this case - // we return the value we stashed at last request. - String value = response.configuration() != null ? - response.configuration().asUtf8String() : // if we have a new value, use it - establishedSession != null ? - establishedSession.lastConfigurationValue : - // if we don't but we have a previous value, use that - null; // otherwise we've got no value - + // Get the value of the key. Note that AppConfig will return an empty value if the configuration has not changed + // since we last asked for it in this session - in this case we return the value we stashed at last request. + // https://docs.aws.amazon.com/appconfig/latest/userguide/appconfig-code-samples-using-API-read-configuration.html + SdkBytes configFromApi = response.configuration(); + String value; + if (configFromApi != null && configFromApi.asByteArray().length != 0) { + value = configFromApi.asUtf8String(); + } else if (establishedSession != null) { + value = establishedSession.lastConfigurationValue; + } else { + value = null; + } // Update the cache so we can get the next value later establishedSessions.put(key, new EstablishedSession(nextSessionToken, value)); diff --git a/powertools-parameters/powertools-parameters-appconfig/src/test/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProviderTest.java b/powertools-parameters/powertools-parameters-appconfig/src/test/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProviderTest.java index 039611875..c16bcfa2a 100644 --- a/powertools-parameters/powertools-parameters-appconfig/src/test/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProviderTest.java +++ b/powertools-parameters/powertools-parameters-appconfig/src/test/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProviderTest.java @@ -14,9 +14,9 @@ package software.amazon.lambda.powertools.parameters.appconfig; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; import static org.assertj.core.api.Assertions.assertThatRuntimeException; -import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.mockito.MockitoAnnotations.openMocks; import static software.amazon.lambda.powertools.parameters.transform.Transformer.json; @@ -27,6 +27,7 @@ import org.mockito.Captor; import org.mockito.Mock; import org.mockito.Mockito; + import software.amazon.awssdk.core.SdkBytes; import software.amazon.awssdk.services.appconfigdata.AppConfigDataClient; import software.amazon.awssdk.services.appconfigdata.model.GetLatestConfigurationRequest; @@ -36,7 +37,7 @@ import software.amazon.lambda.powertools.parameters.cache.CacheManager; import software.amazon.lambda.powertools.parameters.transform.TransformationManager; -public class AppConfigProviderTest { +class AppConfigProviderTest { private final String environmentName = "test"; private final String applicationName = "fakeApp"; @@ -53,7 +54,7 @@ public class AppConfigProviderTest { private AppConfigProvider provider; @BeforeEach - public void init() { + void init() { openMocks(this); provider = AppConfigProvider.builder() @@ -65,7 +66,6 @@ public void init() { .build(); } - /** * Tests repeated calls to the AppConfigProvider for the same key behave correctly. This is more complicated than * it seems, as the service itself will return no-data if the value of a property remains unchanged since the @@ -73,7 +73,7 @@ public void init() { * subsequent calls should once again return the new data. */ @Test - public void getValueRetrievesValue() { + void getValueRetrievesValue() { // Arrange StartConfigurationSessionResponse firstSession = StartConfigurationSessionResponse.builder() .initialConfigurationToken("token1") @@ -92,21 +92,29 @@ public void getValueRetrievesValue() { GetLatestConfigurationResponse thirdResponse = GetLatestConfigurationResponse.builder() .nextPollConfigurationToken("token4") .build(); + // Forth response returns empty, which means the provider should yield the previous value again + GetLatestConfigurationResponse forthResponse = GetLatestConfigurationResponse.builder() + .nextPollConfigurationToken("token5") + .configuration(SdkBytes.fromUtf8String("")) + .build(); Mockito.when(client.startConfigurationSession(startSessionRequestCaptor.capture())) .thenReturn(firstSession); Mockito.when(client.getLatestConfiguration(getLatestConfigurationRequestCaptor.capture())) - .thenReturn(firstResponse, secondResponse, thirdResponse); + .thenReturn(firstResponse, secondResponse, thirdResponse, forthResponse); // Act String returnedValue1 = provider.getValue(defaultTestKey); String returnedValue2 = provider.getValue(defaultTestKey); String returnedValue3 = provider.getValue(defaultTestKey); + String returnedValue4 = provider.getValue(defaultTestKey); // Assert assertThat(returnedValue1).isEqualTo(firstResponse.configuration().asUtf8String()); assertThat(returnedValue2).isEqualTo(secondResponse.configuration().asUtf8String()); assertThat(returnedValue3).isEqualTo(secondResponse.configuration() .asUtf8String()); // Third response is mocked to return null and should re-use previous value + assertThat(returnedValue4).isEqualTo(secondResponse.configuration() + .asUtf8String()); // Forth response is mocked to return empty and should re-use previous value assertThat(startSessionRequestCaptor.getValue().applicationIdentifier()).isEqualTo(applicationName); assertThat(startSessionRequestCaptor.getValue().environmentIdentifier()).isEqualTo(environmentName); assertThat(startSessionRequestCaptor.getValue().configurationProfileIdentifier()).isEqualTo(defaultTestKey); @@ -119,8 +127,7 @@ public void getValueRetrievesValue() { } @Test - public void getValueNoValueExists() { - + void getValueNoValueExists() { // Arrange StartConfigurationSessionResponse session = StartConfigurationSessionResponse.builder() .initialConfigurationToken("token1") @@ -136,9 +143,8 @@ public void getValueNoValueExists() { // Act String returnedValue = provider.getValue(defaultTestKey); - // Assert - assertThat(returnedValue).isEqualTo(null); + assertThat(returnedValue).isNull(); } /** @@ -146,7 +152,7 @@ public void getValueNoValueExists() { * work as expected. This means two separate configuration sessions should be established with AppConfig. */ @Test - public void multipleKeysRetrievalWorks() { + void multipleKeysRetrievalWorks() { // Arrange String param1Key = "key1"; StartConfigurationSessionResponse param1Session = StartConfigurationSessionResponse.builder() @@ -184,49 +190,45 @@ public void multipleKeysRetrievalWorks() { param1Session.initialConfigurationToken()); assertThat(getLatestConfigurationRequestCaptor.getAllValues().get(1).configurationToken()).isEqualTo( param2Session.initialConfigurationToken()); - } @Test - public void getMultipleValuesThrowsException() { - + void getMultipleValuesThrowsException() { // Act & Assert assertThatRuntimeException().isThrownBy(() -> provider.getMultipleValues("path")) .withMessage("Retrieving multiple parameter values is not supported with the AWS App Config Provider"); } @Test - public void testAppConfigProviderBuilderMissingEnvironment_throwsException() { - + void testAppConfigProviderBuilderMissingEnvironment_throwsException() { // Act & Assert assertThatIllegalStateException().isThrownBy(() -> AppConfigProvider.builder() - .withCacheManager(new CacheManager()) - .withApplication(applicationName) - .withClient(client) - .build()) + .withCacheManager(new CacheManager()) + .withApplication(applicationName) + .withClient(client) + .build()) .withMessage("No environment provided; please provide one"); } @Test - public void testAppConfigProviderBuilderMissingApplication_throwsException() { - + void testAppConfigProviderBuilderMissingApplication_throwsException() { // Act & Assert assertThatIllegalStateException().isThrownBy(() -> AppConfigProvider.builder() - .withCacheManager(new CacheManager()) - .withEnvironment(environmentName) - .withClient(client) - .build()) + .withCacheManager(new CacheManager()) + .withEnvironment(environmentName) + .withClient(client) + .build()) .withMessage("No application provided; please provide one"); } - @Test - public void testAppConfigProvider_withoutParameter_shouldHaveDefaultTransformationManager() { + @Test + void testAppConfigProvider_withoutParameter_shouldHaveDefaultTransformationManager() { // Act AppConfigProvider appConfigProvider = AppConfigProvider.builder() .withEnvironment("test") .withApplication("app") .build(); // Assert - assertDoesNotThrow(()->appConfigProvider.withTransformation(json)); + assertDoesNotThrow(() -> appConfigProvider.withTransformation(json)); } } From 3a45c657cced2af2990d0dc063551d8b2ef5eec4 Mon Sep 17 00:00:00 2001 From: Philipp Page Date: Tue, 29 Jul 2025 09:53:23 +0200 Subject: [PATCH 2/2] fix pmd findings. --- .../appconfig/AppConfigProvider.java | 4 +-- .../appconfig/AppConfigProviderTest.java | 30 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/powertools-parameters/powertools-parameters-appconfig/src/main/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProvider.java b/powertools-parameters/powertools-parameters-appconfig/src/main/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProvider.java index ddb85c540..37f07ae7a 100644 --- a/powertools-parameters/powertools-parameters-appconfig/src/main/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProvider.java +++ b/powertools-parameters/powertools-parameters-appconfig/src/main/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProvider.java @@ -46,7 +46,7 @@ public class AppConfigProvider extends BaseProvider { private final AppConfigDataClient client; private final String application; private final String environment; - private final HashMap establishedSessions = new HashMap<>(); + private final Map establishedSessions = new HashMap<>(); AppConfigProvider(CacheManager cacheManager, TransformationManager transformationManager, AppConfigDataClient client, String environment, String application) { @@ -118,7 +118,7 @@ protected Map getMultipleValues(String path) { "Retrieving multiple parameter values is not supported with the AWS App Config Provider"); } - private static class EstablishedSession { + private static final class EstablishedSession { private final String nextSessionToken; private final String lastConfigurationValue; diff --git a/powertools-parameters/powertools-parameters-appconfig/src/test/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProviderTest.java b/powertools-parameters/powertools-parameters-appconfig/src/test/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProviderTest.java index c16bcfa2a..da01d7d9a 100644 --- a/powertools-parameters/powertools-parameters-appconfig/src/test/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProviderTest.java +++ b/powertools-parameters/powertools-parameters-appconfig/src/test/java/software/amazon/lambda/powertools/parameters/appconfig/AppConfigProviderTest.java @@ -39,9 +39,9 @@ class AppConfigProviderTest { - private final String environmentName = "test"; - private final String applicationName = "fakeApp"; - private final String defaultTestKey = "key1"; + private static final String ENVIRONMENT_NAME = "test"; + private static final String DEFAULT_TEST_KEY = "key1"; + private static final String APPLICATION_NAME = "fakeApp"; @Mock AppConfigDataClient client; @@ -59,8 +59,8 @@ void init() { provider = AppConfigProvider.builder() .withClient(client) - .withApplication(applicationName) - .withEnvironment(environmentName) + .withApplication(APPLICATION_NAME) + .withEnvironment(ENVIRONMENT_NAME) .withCacheManager(new CacheManager()) .withTransformationManager(new TransformationManager()) .build(); @@ -103,10 +103,10 @@ void getValueRetrievesValue() { .thenReturn(firstResponse, secondResponse, thirdResponse, forthResponse); // Act - String returnedValue1 = provider.getValue(defaultTestKey); - String returnedValue2 = provider.getValue(defaultTestKey); - String returnedValue3 = provider.getValue(defaultTestKey); - String returnedValue4 = provider.getValue(defaultTestKey); + String returnedValue1 = provider.getValue(DEFAULT_TEST_KEY); + String returnedValue2 = provider.getValue(DEFAULT_TEST_KEY); + String returnedValue3 = provider.getValue(DEFAULT_TEST_KEY); + String returnedValue4 = provider.getValue(DEFAULT_TEST_KEY); // Assert assertThat(returnedValue1).isEqualTo(firstResponse.configuration().asUtf8String()); @@ -115,9 +115,9 @@ void getValueRetrievesValue() { .asUtf8String()); // Third response is mocked to return null and should re-use previous value assertThat(returnedValue4).isEqualTo(secondResponse.configuration() .asUtf8String()); // Forth response is mocked to return empty and should re-use previous value - assertThat(startSessionRequestCaptor.getValue().applicationIdentifier()).isEqualTo(applicationName); - assertThat(startSessionRequestCaptor.getValue().environmentIdentifier()).isEqualTo(environmentName); - assertThat(startSessionRequestCaptor.getValue().configurationProfileIdentifier()).isEqualTo(defaultTestKey); + assertThat(startSessionRequestCaptor.getValue().applicationIdentifier()).isEqualTo(APPLICATION_NAME); + assertThat(startSessionRequestCaptor.getValue().environmentIdentifier()).isEqualTo(ENVIRONMENT_NAME); + assertThat(startSessionRequestCaptor.getValue().configurationProfileIdentifier()).isEqualTo(DEFAULT_TEST_KEY); assertThat(getLatestConfigurationRequestCaptor.getAllValues().get(0).configurationToken()).isEqualTo( firstSession.initialConfigurationToken()); assertThat(getLatestConfigurationRequestCaptor.getAllValues().get(1).configurationToken()).isEqualTo( @@ -141,7 +141,7 @@ void getValueNoValueExists() { .thenReturn(response); // Act - String returnedValue = provider.getValue(defaultTestKey); + String returnedValue = provider.getValue(DEFAULT_TEST_KEY); // Assert assertThat(returnedValue).isNull(); @@ -204,7 +204,7 @@ void testAppConfigProviderBuilderMissingEnvironment_throwsException() { // Act & Assert assertThatIllegalStateException().isThrownBy(() -> AppConfigProvider.builder() .withCacheManager(new CacheManager()) - .withApplication(applicationName) + .withApplication(APPLICATION_NAME) .withClient(client) .build()) .withMessage("No environment provided; please provide one"); @@ -215,7 +215,7 @@ void testAppConfigProviderBuilderMissingApplication_throwsException() { // Act & Assert assertThatIllegalStateException().isThrownBy(() -> AppConfigProvider.builder() .withCacheManager(new CacheManager()) - .withEnvironment(environmentName) + .withEnvironment(ENVIRONMENT_NAME) .withClient(client) .build()) .withMessage("No application provided; please provide one");