Merge pull request #607 from mrm9084/AppConfigFeatureFixes

App Config Feature Fixes

spring-cloud-azure-appconfiguration-config/src/main/java/com/microsoft/azure/spring/cloud/config/AzureCloudConfigRefresh.java

@@ -141,18 +141,23 @@ private boolean refresh(ConfigStore store, String storeSuffix, String watchedKey
         if (items != null && !items.isEmpty()) {
             etag = items.get(0).getETag();
         }
-
+        
         if (StateHolder.getState(storeNameWithSuffix) == null) {
+            // Should never be the case as Property Source should set the state, but if
+            // etag != null return true.
+            if (etag != null) {
+                return true;
+            }
             return false;
         }
-
+        
         if (!etag.equals(StateHolder.getState(storeNameWithSuffix).getETag())) {
             LOGGER.trace("Some keys in store [{}] matching [{}] is updated, will send refresh event.",
                     store.getEndpoint(), watchedKeyNames);
-            if (eventDataInfo.isEmpty()) {
-                eventDataInfo = watchedKeyNames;
+            if (this.eventDataInfo.isEmpty()) {
+                this.eventDataInfo = watchedKeyNames;
             } else {
-                eventDataInfo += ", " + watchedKeyNames;
+                this.eventDataInfo += ", " + watchedKeyNames;
             }
 
             // Don't need to refresh here will be done in Property Source

spring-cloud-azure-appconfiguration-config/src/main/java/com/microsoft/azure/spring/cloud/config/AzureConfigPropertySource.java

@@ -238,7 +238,8 @@ private String getKeyVaultEntry(String value) {
      * @param featureSet Feature Flag info to be set to this property source.
      */
     void initFeatures(FeatureSet featureSet) {
-        properties.put(FEATURE_MANAGEMENT_KEY, mapper.convertValue(featureSet, LinkedHashMap.class));
+        properties.put(FEATURE_MANAGEMENT_KEY,
+                mapper.convertValue(featureSet.getFeatureManagement(), LinkedHashMap.class));
     }
 
     /**

spring-cloud-azure-appconfiguration-config/src/test/java/com/microsoft/azure/spring/cloud/config/AzureConfigCloudRefreshTest.java

@@ -6,6 +6,7 @@
 package com.microsoft.azure.spring.cloud.config;
 
 import static com.microsoft.azure.spring.cloud.config.Constants.CONFIGURATION_SUFFIX;
+import static com.microsoft.azure.spring.cloud.config.Constants.FEATURE_SUFFIX;
 import static com.microsoft.azure.spring.cloud.config.TestConstants.TEST_CONN_STRING;
 import static com.microsoft.azure.spring.cloud.config.TestConstants.TEST_ETAG;
 import static com.microsoft.azure.spring.cloud.config.TestConstants.TEST_STORE_NAME;
@@ -25,13 +26,13 @@
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
-import org.powermock.api.mockito.PowerMockito;
 import org.powermock.modules.junit4.PowerMockRunner;
 import org.springframework.cloud.endpoint.event.RefreshEvent;
 import org.springframework.context.ApplicationEventPublisher;
@@ -61,6 +62,8 @@ public class AzureConfigCloudRefreshTest {
 
     @Mock
     private ClientStore clientStoreMock;
+
+    private static final String WATCHED_KEYS = "/application/*";
 
     @Before
     public void setup() {
@@ -69,12 +72,12 @@ public void setup() {
         ConfigStore store = new ConfigStore();
         store.setEndpoint(TEST_STORE_NAME);
         store.setConnectionString(TEST_CONN_STRING);
-        store.setWatchedKey("/application/*");
-        
+        store.setWatchedKey(WATCHED_KEYS);
+
         properties = new AzureCloudConfigProperties();
         properties.setStores(Arrays.asList(store));
 
-        properties.setCacheExpiration(Duration.ofSeconds(-60));
+        properties.setCacheExpiration(Duration.ofMinutes(-60));
 
         contextsMap = new ConcurrentHashMap<>();
         contextsMap.put(TEST_STORE_NAME, Arrays.asList(TEST_ETAG));
@@ -88,39 +91,50 @@ public void setup() {
         item.setKey("fake-etag/application/test.key");
         item.setETag("fake-etag");
 
+        when(clientStoreMock.watchedKeyNames(Mockito.any(), Mockito.any())).thenReturn(WATCHED_KEYS);
+
         configRefresh = new AzureCloudConfigRefresh(properties, contextsMap, clientStoreMock);
     }
 
+    @After
+    public void cleanupMethod() {
+        StateHolder.setState(TEST_STORE_NAME + CONFIGURATION_SUFFIX,  new ConfigurationSetting());
+        StateHolder.setState(TEST_STORE_NAME + FEATURE_SUFFIX,  new ConfigurationSetting());
+    }
+
     @Test
-    public void firstCallShouldPublishEvent() throws Exception {
-        PowerMockito.whenNew(Date.class).withNoArguments().thenReturn(date);
+    public void nonUpdatedEtagShouldntPublishEvent() throws Exception {
+        StateHolder.setState(TEST_STORE_NAME + CONFIGURATION_SUFFIX, initialResponse().get(0));
+        StateHolder.setState(TEST_STORE_NAME + FEATURE_SUFFIX, initialResponse().get(0));
+
         configRefresh.setApplicationEventPublisher(eventPublisher);
 
         when(clientStoreMock.listSettingRevisons(Mockito.any(), Mockito.anyString())).thenReturn(initialResponse());
 
-        when(date.after(Mockito.any(Date.class))).thenReturn(true);
-        configRefresh.refreshConfigurations();
+        configRefresh.refreshConfigurations().get();
         verify(eventPublisher, times(0)).publishEvent(any(RefreshEvent.class));
     }
 
     @Test
     public void updatedEtagShouldPublishEvent() throws Exception {
-        PowerMockito.whenNew(Date.class).withNoArguments().thenReturn(date);
-        when(clientStoreMock.listSettingRevisons(Mockito.any(), Mockito.anyString())).thenReturn(initialResponse())
-                .thenReturn(updatedResponse());
+        StateHolder.setState(TEST_STORE_NAME + CONFIGURATION_SUFFIX, initialResponse().get(0));
+        StateHolder.setState(TEST_STORE_NAME + FEATURE_SUFFIX, initialResponse().get(0));
+
+        when(clientStoreMock.listSettingRevisons(Mockito.any(), Mockito.anyString())).thenReturn(initialResponse());
         configRefresh.setApplicationEventPublisher(eventPublisher);
 
-        when(date.after(Mockito.any(Date.class))).thenReturn(true);
-
         // The first time an action happens it can't update
         assertFalse(configRefresh.refreshConfigurations().get());
         verify(eventPublisher, times(0)).publishEvent(any(RefreshEvent.class));
 
-        StateHolder.setState(TEST_STORE_NAME + CONFIGURATION_SUFFIX, new ConfigurationSetting());
+        when(clientStoreMock.listSettingRevisons(Mockito.any(), Mockito.anyString())).thenReturn(updatedResponse());
 
         // If there is a change it should update
         assertTrue(configRefresh.refreshConfigurations().get());
         verify(eventPublisher, times(1)).publishEvent(any(RefreshEvent.class));
+
+        StateHolder.setState(TEST_STORE_NAME + CONFIGURATION_SUFFIX, updatedResponse().get(0));
+        StateHolder.setState(TEST_STORE_NAME + FEATURE_SUFFIX, updatedResponse().get(0));
 
         HashMap<String, String> map = new HashMap<String, String>();
         map.put("store1_configuration", "fake-etag-updated");
@@ -138,14 +152,14 @@ public void updatedEtagShouldPublishEvent() throws Exception {
 
     @Test
     public void notRefreshTime() throws Exception {
-        properties.setCacheExpiration(Duration.ofSeconds(60));
-        AzureCloudConfigRefresh watchLargeDelay = new AzureCloudConfigRefresh(properties, contextsMap, clientStoreMock);
+        StateHolder.setState(TEST_STORE_NAME + CONFIGURATION_SUFFIX, initialResponse().get(0));
+        StateHolder.setState(TEST_STORE_NAME + FEATURE_SUFFIX, initialResponse().get(0));
 
-        PowerMockito.whenNew(Date.class).withNoArguments().thenReturn(date);
-        watchLargeDelay.setApplicationEventPublisher(eventPublisher);
+        properties.setCacheExpiration(Duration.ofMinutes(60));
+        AzureCloudConfigRefresh watchLargeDelay = new AzureCloudConfigRefresh(properties, contextsMap, clientStoreMock);
 
-        when(date.after(Mockito.any(Date.class))).thenReturn(true);
-        watchLargeDelay.refreshConfigurations();
+        watchLargeDelay.setApplicationEventPublisher(eventPublisher);
+        watchLargeDelay.refreshConfigurations().get();
 
         // The first time an action happens it can update
         verify(eventPublisher, times(0)).publishEvent(any(RefreshEvent.class));

spring-cloud-azure-appconfiguration-config/src/test/java/com/microsoft/azure/spring/cloud/config/AzureConfigPropertySourceTest.java

@@ -61,7 +61,6 @@
 import reactor.core.publisher.Flux;
 import reactor.core.publisher.Mono;
 
-
 public class AzureConfigPropertySourceTest {
     private static final String EMPTY_CONTENT_TYPE = "";
 
@@ -86,7 +85,7 @@ public class AzureConfigPropertySourceTest {
 
     private static final ConfigurationSetting featureItem = createItem(".appconfig.featureflag/", "Alpha",
             FEATURE_VALUE, FEATURE_LABEL, FEATURE_FLAG_CONTENT_TYPE);
-    
+
     private static final ConfigurationSetting featureItem2 = createItem(".appconfig.featureflag/", "Beta",
             FEATURE_BOOLEAN_VALUE, FEATURE_LABEL, FEATURE_FLAG_CONTENT_TYPE);
 
@@ -103,6 +102,7 @@ public class AzureConfigPropertySourceTest {
     private static ObjectMapper mapper = new ObjectMapper();
 
     private AzureCloudConfigProperties azureProperties;
+
     @Mock
     private ClientStore clientStoreMock;
 
@@ -129,9 +129,9 @@ public class AzureConfigPropertySourceTest {
 
     @Rule
     public ExpectedException expected = ExpectedException.none();
-    
+
     private AppConfigProviderProperties appProperties;
-    
+
     private KeyVaultCredentialProvider tokenCredentialProvider = null;
 
     @BeforeClass
@@ -242,7 +242,8 @@ public void testFeatureFlagCanBeInitedAndQueried() throws IOException {
         feature.setEnabledFor(filters);
         featureSetExpected.addFeature("Alpha", feature);
         featureSetExpected.addFeature("Beta", true);
-        LinkedHashMap<?, ?> convertedValue = mapper.convertValue(featureSetExpected, LinkedHashMap.class);
+        LinkedHashMap<?, ?> convertedValue = mapper.convertValue(featureSetExpected.getFeatureManagement(),
+                LinkedHashMap.class);
 
         assertEquals(convertedValue, propertySource.getProperty(FEATURE_MANAGEMENT_KEY));
     }
@@ -279,7 +280,8 @@ public void testFeatureFlagBuildError() throws IOException {
         feature.setEnabledFor(filters);
         featureSetExpected.addFeature("Alpha", feature);
         featureSetExpected.addFeature("Beta", true);
-        LinkedHashMap<?, ?> convertedValue = mapper.convertValue(featureSetExpected, LinkedHashMap.class);
+        LinkedHashMap<?, ?> convertedValue = mapper.convertValue(featureSetExpected.getFeatureManagement(),
+                LinkedHashMap.class);
 
         assertEquals(convertedValue, propertySource.getProperty(FEATURE_MANAGEMENT_KEY));
     }

spring-cloud-azure-feature-management/src/main/java/com/microsoft/azure/spring/cloud/feature/manager/FeatureManagementConfiguration.java

@@ -9,15 +9,13 @@
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 
-import com.microsoft.azure.spring.cloud.feature.manager.entities.FeatureSet;
-
 @Configuration
-@EnableConfigurationProperties({ FeatureManagementConfigProperties.class, FeatureSet.class })
+@EnableConfigurationProperties({ FeatureManagementConfigProperties.class })
 public class FeatureManagementConfiguration {
 
     @Bean
-    public FeatureManager featureManager(FeatureManagementConfigProperties properties, FeatureSet featureSet) {
-        return new FeatureManager(properties, featureSet);
+    public FeatureManager featureManager(FeatureManagementConfigProperties properties) {
+        return new FeatureManager(properties);
     }
 
 }

spring-cloud-azure-feature-management/src/main/java/com/microsoft/azure/spring/cloud/feature/manager/FeatureManager.java

@@ -5,39 +5,51 @@
  */
 package com.microsoft.azure.spring.cloud.feature.manager;
 
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.NoSuchBeanDefinitionException;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.context.properties.ConfigurationProperties;
 import org.springframework.context.ApplicationContext;
 import org.springframework.stereotype.Component;
 import org.springframework.util.ReflectionUtils;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
 import com.microsoft.azure.spring.cloud.feature.manager.entities.Feature;
 import com.microsoft.azure.spring.cloud.feature.manager.entities.FeatureFilterEvaluationContext;
-import com.microsoft.azure.spring.cloud.feature.manager.entities.FeatureSet;
 
 import reactor.core.publisher.Mono;
 
 /**
  * Holds information on Feature Management properties and can check if a given feature is
  * enabled.
  */
-@Component("FeatureManager")
-public class FeatureManager {
+@SuppressWarnings("serial")
+@Component("FeatureManagement")
+@ConfigurationProperties(prefix = "feature-management")
+public class FeatureManager extends HashMap<String, Object> {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(FeatureManager.class);
 
-    private FeatureSet featureManagement;
-
     @Autowired
     private ApplicationContext context;
 
     private FeatureManagementConfigProperties properties;
 
-    public FeatureManager(FeatureManagementConfigProperties properties, FeatureSet featureManagement) {
+    private HashMap<String, Feature> featureManagement;
+
+    private HashMap<String, Boolean> onOff;
+
+    private ObjectMapper mapper = new ObjectMapper();
+
+    public FeatureManager(FeatureManagementConfigProperties properties) {
         this.properties = properties;
-        this.featureManagement = featureManagement;
+        featureManagement = new HashMap<String, Feature>();
+        onOff = new HashMap<String, Boolean>();
     }
 
     /**
@@ -55,13 +67,12 @@ public Mono<Boolean> isEnabledAsync(String feature) {
 
     private boolean checkFeatures(String feature) {
         boolean enabled = false;
-        if (featureManagement == null || featureManagement.getFeatureManagement() == null ||
-                featureManagement.getOnOff() == null) {
+        if (featureManagement == null || onOff == null) {
             return false;
         }
 
-        Feature featureItem = featureManagement.getFeatureManagement().get(feature);
-        Boolean boolFeature = featureManagement.getOnOff().get(feature);
+        Feature featureItem = featureManagement.get(feature);
+        Boolean boolFeature = onOff.get(feature);
 
         if (boolFeature != null) {
             return boolFeature;
@@ -89,4 +100,69 @@ private boolean checkFeatures(String feature) {
         }
         return enabled;
     }
+
+    @SuppressWarnings("unchecked")
+    private void addToFeatures(Map<? extends String, ? extends Object> features, String key, String combined) {
+        Object featureKey = features.get(key);
+        if (!combined.isEmpty() && !combined.endsWith(".")) {
+            combined += ".";
+        }
+        if (featureKey instanceof Boolean) {
+            onOff.put(combined + key, (Boolean) featureKey);
+        } else {
+            Feature feature = null;
+            try {
+                feature = mapper.convertValue(featureKey, Feature.class);
+            } catch (IllegalArgumentException e) {
+                LOGGER.error("Found invalid feature {} with value {}.", combined + key, featureKey.toString());
+            }
+
+            // When coming from a file "feature.flag" is not a possible flag name
+            if (feature != null && feature.getEnabledFor() == null && feature.getKey() == null) {
+                if (LinkedHashMap.class.isAssignableFrom(featureKey.getClass())) {
+                    features = (LinkedHashMap<String, Object>) featureKey;
+                    for (String fKey : features.keySet()) {
+                        addToFeatures(features, fKey, combined + key);
+                    }
+                }
+            } else {
+                if (feature != null) {
+                    feature.setKey(key);
+                    featureManagement.put(key, feature);
+                }
+            }
+        }
+    }
+
+    @SuppressWarnings("unchecked")
+    @Override
+    public void putAll(Map<? extends String, ? extends Object> m) {
+        if (m == null) {
+            return;
+        }
+
+        if (m.size() == 1 && m.get("featureManagement") != null) {
+            m = (Map<? extends String, ? extends Object>) m.get("featureManagement");
+        }
+
+        for (String key : m.keySet()) {
+            addToFeatures(m, key, "");
+        }
+    }
+
+    /**
+     * @return the featureManagement
+     */
+    HashMap<String, Feature> getFeatureManagement() {
+        return featureManagement;
+    }
+
+    /**
+     * @return the onOff
+     */
+    HashMap<String, Boolean> getOnOff() {
+        return onOff;
+    }
+
+
 }