FFM-6125 - Nullpointer in MetricsProcessor (#130)

* FFM-6125 - Nullpointer in MetricsProcessor

What
When passing in a null target to an xxxVariation() function we get a NullPointerException.
This change fixes the MetricsProcessor to handle nulls correctly and corrects a log statement to use toString() instead of deferencing target directly.

Why
Logger statement and metrics processing code don't perform any null checks.

Testing
Unit testing: added new unit tests to check that nulls can be passed in without error.
Manual testing with GettingStarted sample

Author: andybharness

README.md

@@ -22,7 +22,7 @@ For a sample FF Java SDK project, see our [test Java project](https://github.com
 
 To use this SDK, make sure you've:
 
-- Installed[JDK 8](https://openjdk.java.net/install/) or a newer version<br>
+- Installed [JDK 8](https://openjdk.java.net/install/) or a newer version<br>
 - Installed Maven or Gradle or an alternative build automation tool for your application
 
 To follow along with our test code sample, make sure you’ve:
@@ -69,7 +69,7 @@ The first step is to install the FF SDK as a dependency in your application usin
 
 Refer to the [Harness Feature Flag Java Server SDK](https://mvnrepository.com/artifact/io.harness/ff-java-server-sdk) to identify the latest version for your build automation tool.
 
-This section lists dependencies for Maven and Gradle and uses the 1.1.8 version as an example:
+This section lists dependencies for Maven and Gradle and uses the 1.1.9 version as an example:
 
 #### Maven
 
@@ -78,14 +78,14 @@ Add the following Maven dependency in your project's pom.xml file:
 <dependency>
     <groupId>io.harness</groupId>
     <artifactId>ff-java-server-sdk</artifactId>
-    <version>1.1.8</version>
+    <version>1.1.9</version>
 </dependency>
 ```
 
 #### Gradle
 
 ```
-implementation group: 'io.harness', name: 'ff-java-server-sdk', version: '1.1.8'
+implementation group: 'io.harness', name: 'ff-java-server-sdk', version: '1.1.9'
 ```
 
 ### Code Sample

examples/pom.xml

@@ -6,7 +6,7 @@
 
     <groupId>io.harness.featureflags</groupId>
     <artifactId>examples</artifactId>
-    <version>1.1.9-SNAPSHOT</version>
+    <version>1.1.9</version>
 
     <properties>
         <maven.compiler.source>8</maven.compiler.source>
@@ -33,7 +33,7 @@
         <dependency>
             <groupId>io.harness</groupId>
             <artifactId>ff-java-server-sdk</artifactId>
-            <version>1.1.9-SNAPSHOT</version>
+            <version>1.1.9</version>
         </dependency>
 
         <dependency>

pom.xml

@@ -6,7 +6,7 @@
 
     <groupId>io.harness</groupId>
     <artifactId>ff-java-server-sdk</artifactId>
-    <version>1.1.9-SNAPSHOT</version>
+    <version>1.1.9</version>
     <packaging>jar</packaging>
     <name>Harness Feature Flag Java Server SDK</name>
     <description>Harness Feature Flag Java Server SDK</description>

src/main/java/io/harness/cf/client/api/MetricsProcessor.java

@@ -68,14 +68,15 @@ public synchronized void pushToQueue(Target target, String featureName, Variatio
       executor.submit(this::runOneIteration);
     }
 
-    log.debug(
-        "Flag: " + featureName + " Target: " + target.getIdentifier() + " Variation: " + variation);
-
-    uniqueTargetSet.add(target);
+    log.debug("Flag: " + featureName + " Target: " + target + " Variation: " + variation);
 
     Target metricTarget = globalTarget;
-    if (!config.isGlobalTargetEnabled()) {
-      metricTarget = target;
+
+    if (target != null) {
+      uniqueTargetSet.add(target);
+      if (!config.isGlobalTargetEnabled()) {
+        metricTarget = target;
+      }
     }
 
     frequencyMap.incrementAndGet(new MetricEvent(featureName, metricTarget, variation));

src/test/java/io/harness/cf/client/api/CfClientTest.java

@@ -162,7 +162,19 @@ private static Stream<Arguments> variantsToTest() {
         Arguments.of("boolVariation", (Consumer<CfClient>) CfClientTest::testBoolVariant),
         Arguments.of("stringVariation", (Consumer<CfClient>) CfClientTest::testStringVariant),
         Arguments.of("numberVariation", (Consumer<CfClient>) CfClientTest::testNumberVariant),
-        Arguments.of("jsonVariant", (Consumer<CfClient>) CfClientTest::testJsonVariant));
+        Arguments.of("jsonVariant", (Consumer<CfClient>) CfClientTest::testJsonVariant),
+        Arguments.of(
+            "boolVariationWithNullTarget",
+            (Consumer<CfClient>) CfClientTest::testBoolVariantWithNullTarget),
+        Arguments.of(
+            "stringVariationWithNullTarget",
+            (Consumer<CfClient>) CfClientTest::testStringVariantWithNullTarget),
+        Arguments.of(
+            "numberVariationWithNullTarget",
+            (Consumer<CfClient>) CfClientTest::testNumberVariantWithNullTarget),
+        Arguments.of(
+            "jsonVariantWithNullTarget",
+            (Consumer<CfClient>) CfClientTest::testJsonVariantWithNullTarget));
   }
 
   @ParameterizedTest(name = "{0}")
@@ -172,7 +184,7 @@ void shouldGetVariantInPollingMode(String description, Consumer<CfClient> flagCa
     BaseConfig config =
         BaseConfig.builder()
             .pollIntervalInSeconds(1)
-            .analyticsEnabled(false)
+            .analyticsEnabled(true)
             .streamEnabled(false)
             .build();
 
@@ -216,6 +228,26 @@ private static void testJsonVariant(CfClient client) {
     assertEquals("on", value.get("value").getAsString());
   }
 
+  private static void testBoolVariantWithNullTarget(CfClient client) {
+    boolean value = client.boolVariation("simplebool", null, false);
+    assertTrue(value);
+  }
+
+  private static void testStringVariantWithNullTarget(CfClient client) {
+    String value = client.stringVariation("simplestring", null, "DEFAULT");
+    assertEquals("on-string", value);
+  }
+
+  private static void testNumberVariantWithNullTarget(CfClient client) {
+    double value = client.numberVariation("simplenumber", null, 0.123);
+    assertEquals(1, value);
+  }
+
+  private static void testJsonVariantWithNullTarget(CfClient client) {
+    JsonObject value = client.jsonVariation("simplejson", null, new JsonObject());
+    assertEquals("on", value.get("value").getAsString());
+  }
+
   static class TestWebServerDispatcher extends Dispatcher {
     private final AtomicInteger version = new AtomicInteger(2);