Repository: Unleash/unleash-client-java
Pull request #48 (2018-06-12 17:51:23)

Expose list of feature names.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/17452399/badge)](https://coveralls.io/builds/17452399)

Coverage decreased (-0.2%) to 95.03% when pulling **34b7000ee2f968614549db79b18f22f440493532 on chadjosephson-octanner:master** into **3ba8b257bf78b30f5c60a4e05c48f8bb23b9335f on Unleash:master**.

chadjosephson-octanner: This pull request is meant to expose the list of features available, the list of enabled features, and the list of enabled features for a given context. 
ivarconr: Thanks for the contribution @chadjosephson-octanner. I can understand the need for getting the status of multiple toggles at once, but it would be nice if you could elaborate a bit on what you intend to use the `getEnabledToggleNames` for. 

However there are a few things we should consider: 
1. Many companies uses Unleash across many applications and services. A typical convention is to prefix toggles to get a logical separation (e.g. `app.toggleNamne`) Should these `getEnabledToggleNames` accept a "name filter", so that the application can query only toggles relevant for that application? 
2. One side-effect of calling `isEnabled` is that the client will count yes/no for the toggle being tested. Have the application used the toggle if it calls `getEnabledToggleNames`? Do we have to omit metrics in that scenario? The intention of metrics in unleash is to keep track of which application is using which toggle and what the feature toggle evaluates to. Another use case for metrics is to detect unused toggles (no clients querying `isEnabled` for them anymore).


chadjosephson-octanner: Thank you for reviewing the proposed changes @ivarconr.

The use-case these proposed changes addresses relates to how our application stack is structured. We deliver a single page application backed by many micro-service endpoints. The intent is to have the layout microservice deliver a list of feature toggles to the front-end rather than having each microservice query Unleash. 

This could be accomplished by maintaining a list of potential feature toggles in the layout microservice and querying each flag individually.  What I don't like about that solution is the layout microservice would need to know about all potential features for all microservices and the UI. That would create unnecessary dependencies between services.  In my opinion it would be much simpler to ask Unleash to return a list of all enabled feature toggles. 

Let me know if this concept is in-line with Unleash. In the meantime I will address specific things you have brought up in the review. Thanks again for your time.


chadjosephson-octanner: Regarding your two considerations:

1. I would leave using the application name to filter as a question for you. At the moment it wouldn't be of any value to my project.

2. I can see how my changes will cause the metrics to have no value. Knowing which features are no longer being queried is useful information to have and I am not sure how to reconcile existing metrics and my changes.
ivarconr: Adding the `getToggleNames` is a no-brainer, and we should just do that. 

Given that you get this from the Unleash API, how important is it that the client actually provides you with the `getAllEnabledToggles` implementation?  (It would be fairly simple to have the "stream->filter->collect" impl on your side anyway). 

My concern is that it is not obvious for the users that this will reduce the value of the metrics.

What do you think? 
ivarconr: Another approach could be to add the method `Map<String, boolean> evaluateAllToggles()`, which in my opinion is more explicit, but it does not solve your use case directly. 
ivarconr: And a big thanks for taking time to reach out and suggest improvements. I am always happy when users talk about their needs and find ways to improve the API. 

I will make sure to merge the changes and get a release out asap.

Patch:
diff --git a/src/main/java/no/finn/unleash/DefaultUnleash.java b/src/main/java/no/finn/unleash/DefaultUnleash.java
index 325544ea..546b3692 100644
--- a/src/main/java/no/finn/unleash/DefaultUnleash.java
+++ b/src/main/java/no/finn/unleash/DefaultUnleash.java
@@ -97,6 +97,10 @@ public final class DefaultUnleash implements Unleash {
         return Optional.ofNullable(toggleRepository.getToggle(toggleName));
     }
 
+    public List<String> getFeatureToggleNames() {
+        return toggleRepository.getFeatureNames();
+    }
+
     public void count(final String toggleName, boolean enabled) {
         metricService.count(toggleName, enabled);
     }
diff --git a/src/main/java/no/finn/unleash/FakeUnleash.java b/src/main/java/no/finn/unleash/FakeUnleash.java
index 70d52f39..e1eccbc9 100644
--- a/src/main/java/no/finn/unleash/FakeUnleash.java
+++ b/src/main/java/no/finn/unleash/FakeUnleash.java
@@ -1,7 +1,9 @@
 package no.finn.unleash;
 
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
 
 public final class FakeUnleash implements Unleash {
     private boolean enableAll = false;
@@ -24,6 +26,11 @@ public final class FakeUnleash implements Unleash {
         }
     }
 
+    @Override
+    public List<String> getFeatureToggleNames() {
+        return features.keySet().stream().collect(Collectors.toList());
+    }
+
     public void enableAll() {
         disableAll = false;
         enableAll = true;
diff --git a/src/main/java/no/finn/unleash/Unleash.java b/src/main/java/no/finn/unleash/Unleash.java
index 2bce6d0f..f886dd48 100644
--- a/src/main/java/no/finn/unleash/Unleash.java
+++ b/src/main/java/no/finn/unleash/Unleash.java
@@ -1,5 +1,7 @@
 package no.finn.unleash;
 
+import java.util.List;
+
 public interface Unleash {
     boolean isEnabled(String toggleName);
 
@@ -12,4 +14,6 @@ public interface Unleash {
     default boolean isEnabled(String toggleName, UnleashContext context, boolean defaultSetting) {
         return isEnabled(toggleName, defaultSetting);
     }
+
+    List<String> getFeatureToggleNames();
 }
diff --git a/src/main/java/no/finn/unleash/repository/FeatureToggleRepository.java b/src/main/java/no/finn/unleash/repository/FeatureToggleRepository.java
index 117858f3..326b4baf 100644
--- a/src/main/java/no/finn/unleash/repository/FeatureToggleRepository.java
+++ b/src/main/java/no/finn/unleash/repository/FeatureToggleRepository.java
@@ -2,11 +2,12 @@ package no.finn.unleash.repository;
 
 import no.finn.unleash.util.UnleashConfig;
 import no.finn.unleash.util.UnleashScheduledExecutor;
-import no.finn.unleash.util.UnleashScheduledExecutorImpl;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
-import java.util.concurrent.*;
+import java.util.List;
+import java.util.stream.Collectors;
+
 import no.finn.unleash.FeatureToggle;
 import no.finn.unleash.UnleashException;
 
@@ -50,4 +51,9 @@ public final class FeatureToggleRepository implements ToggleRepository {
     public FeatureToggle getToggle(String name) {
         return toggleCollection.getToggle(name);
     }
+
+    @Override
+    public List<String> getFeatureNames() {
+        return toggleCollection.getFeatures().stream().map(toggle -> toggle.getName()).collect(Collectors.toList());
+    }
 }
diff --git a/src/main/java/no/finn/unleash/repository/ToggleRepository.java b/src/main/java/no/finn/unleash/repository/ToggleRepository.java
index 023766b1..27e77e76 100644
--- a/src/main/java/no/finn/unleash/repository/ToggleRepository.java
+++ b/src/main/java/no/finn/unleash/repository/ToggleRepository.java
@@ -2,6 +2,10 @@ package no.finn.unleash.repository;
 
 import no.finn.unleash.FeatureToggle;
 
+import java.util.List;
+
 public interface ToggleRepository {
     FeatureToggle getToggle(String name);
+
+    List<String> getFeatureNames();
 }


Test patch:
diff --git a/src/test/java/no/finn/unleash/FakeUnleashTest.java b/src/test/java/no/finn/unleash/FakeUnleashTest.java
index a2f87ac8..d1dc9593 100644
--- a/src/test/java/no/finn/unleash/FakeUnleashTest.java
+++ b/src/test/java/no/finn/unleash/FakeUnleashTest.java
@@ -2,6 +2,9 @@ package no.finn.unleash;
 
 import org.junit.Test;
 
+import java.util.Arrays;
+import java.util.List;
+
 import static org.hamcrest.core.Is.is;
 import static org.junit.Assert.*;
 
@@ -81,4 +84,16 @@ public class FakeUnleashTest {
         assertThat(fakeUnleash.isEnabled("t2", true), is(false));
 
     }
+
+
+    @Test
+    public void should_get_all_feature_names() {
+        FakeUnleash fakeUnleash = new FakeUnleash();
+        fakeUnleash.enable("t1", "t2");
+        fakeUnleash.disable("t2");
+
+        List<String> expected = Arrays.asList(new String[]{"t1", "t2"});
+        assertTrue(fakeUnleash.getFeatureToggleNames().containsAll(expected));
+    }
+
 }
diff --git a/src/test/java/no/finn/unleash/UnleashTest.java b/src/test/java/no/finn/unleash/UnleashTest.java
index a0ed3c86..45989121 100644
--- a/src/test/java/no/finn/unleash/UnleashTest.java
+++ b/src/test/java/no/finn/unleash/UnleashTest.java
@@ -14,6 +14,7 @@ import org.junit.Test;
 
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.anyMap;
 import static org.mockito.Mockito.mock;
@@ -159,4 +160,11 @@ public class UnleashTest {
 
         assertThat(((DefaultUnleash)unleash).getFeatureToggleDefinition("another toggle"), is(Optional.empty()));
     }
+
+    @Test
+    public void get_feature_names_should_return_list_of_feature_names() {
+        when(toggleRepository.getFeatureNames()).thenReturn(Arrays.asList("toggleFeatureName1", "toggleFeatureName2"));
+        assertTrue(2 == unleash.getFeatureToggleNames().size());
+        assertTrue("toggleFeatureName2".equals(unleash.getFeatureToggleNames().get(1)));
+    }
 }
diff --git a/src/test/java/no/finn/unleash/repository/FeatureToggleRepositoryTest.java b/src/test/java/no/finn/unleash/repository/FeatureToggleRepositoryTest.java
index 9de7a166..7a721902 100644
--- a/src/test/java/no/finn/unleash/repository/FeatureToggleRepositoryTest.java
+++ b/src/test/java/no/finn/unleash/repository/FeatureToggleRepositoryTest.java
@@ -12,7 +12,6 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 
-import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.*;
@@ -87,9 +86,27 @@ public class FeatureToggleRepositoryTest {
         assertTrue(toggleRepository.getToggle("toggleFetcherCalled").isEnabled());
     }
 
-    private ToggleCollection populatedToggleCollection(FeatureToggle featureToggle) {
+    @Test
+    public void get_feature_names_should_return_list_of_names() {
+        UnleashConfig config = mock(UnleashConfig.class);
+        UnleashScheduledExecutor executor = mock(UnleashScheduledExecutor.class);
+        ToggleFetcher toggleFetcher = mock(ToggleFetcher.class);
+
+        ToggleBackupHandler toggleBackupHandler = mock(ToggleBackupHandler.class);
+        ToggleCollection toggleCollection = populatedToggleCollection(
+                new FeatureToggle("toggleFeatureName1", true, Arrays.asList(new ActivationStrategy("custom", null))),
+                new FeatureToggle("toggleFeatureName2", true, Arrays.asList(new ActivationStrategy("custom", null)))
+        );
+        when(toggleBackupHandler.read()).thenReturn(toggleCollection);
+
+        ToggleRepository toggleRepository = new FeatureToggleRepository(config, executor, toggleFetcher, toggleBackupHandler);
+        assertTrue(2 == toggleRepository.getFeatureNames().size());
+        assertTrue("toggleFeatureName2".equals(toggleRepository.getFeatureNames().get(1)));
+    }
+
+    private ToggleCollection populatedToggleCollection(FeatureToggle... featureToggles) {
         List<FeatureToggle> list = new ArrayList();
-        list.add(featureToggle);
+        list.addAll(Arrays.asList(featureToggles));
         return new ToggleCollection(list);
 
     }