Repository: Unleash/unleash-client-java
Pull request #180 (2022-11-24 12:27:07)

fix: getVariant counts enabled or not
As a change from earlier, this now increments yes/no as well as which variant was received in the MetricsBucket.

To reflect behaviour of Node SDK from https://github.com/Unleash/unleash-client-node/commit/38f35e658a34c510b4d01f8dd33866ff1119d96e onwards.


Patch:
diff --git a/src/main/java/io/getunleash/DefaultUnleash.java b/src/main/java/io/getunleash/DefaultUnleash.java
index 15b6f9d8..b6a3438f 100644
--- a/src/main/java/io/getunleash/DefaultUnleash.java
+++ b/src/main/java/io/getunleash/DefaultUnleash.java
@@ -98,8 +98,7 @@ public class DefaultUnleash implements Unleash {
 
     @Override
     public boolean isEnabled(
-            final String toggleName,
-            final BiPredicate<String, UnleashContext> fallbackAction) {
+            final String toggleName, final BiPredicate<String, UnleashContext> fallbackAction) {
         return isEnabled(toggleName, contextProvider.getContext(), fallbackAction);
     }
 
@@ -178,6 +177,8 @@ public class DefaultUnleash implements Unleash {
                         ? VariantUtil.selectVariant(featureToggle, context, defaultValue)
                         : defaultValue;
         metricService.countVariant(toggleName, variant.getName());
+        // Should count yes/no also when getting variant.
+        metricService.count(toggleName, enabled);
         return variant;
     }
 
diff --git a/src/main/java/io/getunleash/util/UnleashConfig.java b/src/main/java/io/getunleash/util/UnleashConfig.java
index f6c7f377..fbd75895 100644
--- a/src/main/java/io/getunleash/util/UnleashConfig.java
+++ b/src/main/java/io/getunleash/util/UnleashConfig.java
@@ -602,8 +602,24 @@ public class UnleashConfig {
             if (backupFile != null) {
                 return backupFile;
             } else {
-                String fileName = "unleash-" + appName + "-repo.json";
-                return System.getProperty("java.io.tmpdir") + File.separatorChar + fileName;
+                String fileName = "unleash-" + sanitizedAppName(appName) + "-repo.json";
+                String tmpDir = System.getProperty("java.io.tmpdir");
+                if (tmpDir == null) {
+                    throw new IllegalStateException(
+                            "'java.io.tmpdir' must not be empty, cause we write backup files into it.");
+                }
+                tmpDir = !tmpDir.endsWith(File.separator) ? tmpDir + File.separatorChar : tmpDir;
+                return tmpDir + fileName;
+            }
+        }
+
+        private String sanitizedAppName(String appName) {
+            if (null == appName) {
+                return "default";
+            } else if (appName.contains("/") || appName.contains("\\")) {
+                return appName.replace("/", "-").replace("\\", "-");
+            } else {
+                return appName;
             }
         }
 


Test patch:
diff --git a/src/test/java/io/getunleash/UnleashTest.java b/src/test/java/io/getunleash/UnleashTest.java
index a0cfd28f..f6bc7b98 100644
--- a/src/test/java/io/getunleash/UnleashTest.java
+++ b/src/test/java/io/getunleash/UnleashTest.java
@@ -5,6 +5,8 @@ import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.*;
 import static org.mockito.Mockito.*;
 
+import io.getunleash.event.EventDispatcher;
+import io.getunleash.metric.UnleashMetricService;
 import io.getunleash.repository.*;
 import io.getunleash.strategy.Strategy;
 import io.getunleash.strategy.UserWithIdStrategy;
@@ -256,6 +258,41 @@ public class UnleashTest {
         assertThat(result.isEnabled()).isTrue();
     }
 
+    @Test
+    public void getting_variant_when_disabled_should_increment_no_counter() {
+        UnleashContext context = UnleashContext.builder().userId("1").build();
+        UnleashMetricService metricService = mock(UnleashMetricService.class);
+        UnleashConfig config =
+                new UnleashConfig.Builder()
+                        .appName("test")
+                        .unleashAPI("http://localhost:4242/api/")
+                        .environment("test")
+                        .scheduledExecutor(mock(UnleashScheduledExecutor.class))
+                        .unleashContextProvider(contextProvider)
+                        .build();
+        Unleash thisUnleash =
+                new DefaultUnleash(
+                        config,
+                        toggleRepository,
+                        Collections.emptyMap(),
+                        contextProvider,
+                        new EventDispatcher(config),
+                        metricService);
+        // Set up a toggleName using UserWithIdStrategy
+        Map<String, String> params = new HashMap<>();
+        params.put("userIds", "123, 111, 121, 13");
+        ActivationStrategy strategy = new ActivationStrategy("userWithId", params);
+        FeatureToggle featureToggle = new FeatureToggle("test", false, asList(strategy));
+
+        when(toggleRepository.getToggle("test")).thenReturn(featureToggle);
+
+        final Variant result = thisUnleash.getVariant("test", context);
+
+        assertThat(result).isNotNull();
+        verify(metricService).count(anyString(), eq(false));
+        verify(metricService).countVariant(anyString(), eq(result.getName()));
+    }
+
     @Test
     public void get_default_empty_variant_when_disabled_and_no_default_value_is_specified() {
         UnleashContext context = UnleashContext.builder().userId("1").build();
diff --git a/src/test/java/io/getunleash/util/UnleashConfigTest.java b/src/test/java/io/getunleash/util/UnleashConfigTest.java
index 387d8cea..7d5bd812 100644
--- a/src/test/java/io/getunleash/util/UnleashConfigTest.java
+++ b/src/test/java/io/getunleash/util/UnleashConfigTest.java
@@ -60,11 +60,39 @@ public class UnleashConfigTest {
                 UnleashConfig.builder().appName("my-app").unleashAPI("http://unleash.org").build();
 
         assertThat(config.getAppName()).isEqualTo("my-app");
+        String tmpDir = System.getProperty("java.io.tmpdir");
+        tmpDir = tmpDir.endsWith(File.separator) ? tmpDir : tmpDir + File.separatorChar;
+        assertThat(config.getBackupFile()).isEqualTo(tmpDir + "unleash-my-app-repo.json");
+    }
+
+    @Test
+    public void should_generate_backupfile_app_name_with_slash() {
+        UnleashConfig config =
+                UnleashConfig.builder()
+                        .appName("fix/baz-123456")
+                        .unleashAPI("http://unleash.org")
+                        .build();
+
+        assertThat(config.getAppName()).isEqualTo("fix/baz-123456");
+        String tmpDir = System.getProperty("java.io.tmpdir");
+        tmpDir = tmpDir.endsWith(File.separator) ? tmpDir : tmpDir + File.separatorChar;
+        assertThat(config.getBackupFile())
+                .isEqualTo(tmpDir + "unleash-" + "fix-baz-123456" + "-repo.json");
+    }
+
+    @Test
+    public void should_generate_backupfile_app_name_with_backslash() {
+        UnleashConfig config =
+                UnleashConfig.builder()
+                        .appName("fix\\baz-123456")
+                        .unleashAPI("http://unleash.org")
+                        .build();
+
+        assertThat(config.getAppName()).isEqualTo("fix\\baz-123456");
+        String tmpDir = System.getProperty("java.io.tmpdir");
+        tmpDir = tmpDir.endsWith(File.separator) ? tmpDir : tmpDir + File.separatorChar;
         assertThat(config.getBackupFile())
-                .isEqualTo(
-                        System.getProperty("java.io.tmpdir")
-                                + File.separatorChar
-                                + "unleash-my-app-repo.json");
+                .isEqualTo(tmpDir + "unleash-" + "fix-baz-123456" + "-repo.json");
     }
 
     @Test