[finagle] do not fail full toggle map parsing when duplicate toggles …

…exist in a file

Problem:

Currently the finagle toggle map parser fails parsing of all elements in a
toggle file when a single duplicate entry is added. While this fail fast
behavior is good in some cases (especially when toggles are loaded locally),
it's not a great behavior in production systems where toggles are committed and
loaded separately. In such scenarios, if the file is already committed and old
version is unavailable to the system it is preferable to have the toggle map
load as much as it can to prevent breaking other toggles in the file.

Solution:

Add an opt in setting to keep the first entry when a duplicate is encountered
instead of failing to parse the whole thing

Differential Revision: https://phabricator.twitter.biz/D1117265

finagle-toggle/src/main/scala/com/twitter/finagle/toggle/JsonToggleMap.scala

@@ -2,8 +2,10 @@ package com.twitter.finagle.toggle
 
 import com.fasterxml.jackson.annotation.JsonProperty
 import com.fasterxml.jackson.core.JsonFactory
-import com.fasterxml.jackson.core.util.{DefaultIndenter, DefaultPrettyPrinter}
-import com.fasterxml.jackson.databind.{MappingJsonFactory, ObjectMapper}
+import com.fasterxml.jackson.core.util.DefaultIndenter
+import com.fasterxml.jackson.core.util.DefaultPrettyPrinter
+import com.fasterxml.jackson.databind.MappingJsonFactory
+import com.fasterxml.jackson.databind.ObjectMapper
 import com.fasterxml.jackson.module.scala.DefaultScalaModule
 import com.twitter.util.Try
 import java.net.URL
@@ -104,6 +106,25 @@ object JsonToggleMap {
    */
   object DescriptionIgnored extends DescriptionMode
 
+  /**
+   * How to treat the duplicate ids in arrays of toggles.
+   *
+   * @see [[FailParsingOnDuplicateId]] and [[KeepFirstOnDuplicateId]].
+   */
+  sealed abstract class DuplicateHandling
+
+  /**
+   * Requires toggles to have unique id field. Otherwise fail parsing.
+   */
+  object FailParsingOnDuplicateId extends DuplicateHandling
+
+  /**
+   * Keep the first and drop rest to remove duplicates. This is useful
+   * to preserve existing behavior when new duplicate entry is added to the end
+   * of a JSON file
+   */
+  object KeepFirstOnDuplicateId extends DuplicateHandling
+
   private[this] case class JsonToggle(
     @JsonProperty(required = true) id: String,
     @JsonProperty(required = true) fraction: Double,
@@ -112,7 +133,11 @@ object JsonToggleMap {
 
   private[this] case class JsonToggles(@JsonProperty(required = true) toggles: Seq[JsonToggle]) {
 
-    def toToggleMap(source: String, descriptionMode: DescriptionMode): ToggleMap = {
+    def toToggleMap(
+      source: String,
+      descriptionMode: DescriptionMode,
+      duplicateHandling: DuplicateHandling = FailParsingOnDuplicateId
+    ): ToggleMap = {
       val invalid = toggles.find { md =>
         descriptionMode match {
           case DescriptionRequired => md.description.isEmpty
@@ -137,9 +162,17 @@ object JsonToggleMap {
       val ids = metadata.map(_.id)
       val uniqueIds = ids.distinct
       if (ids.size != uniqueIds.size) {
-        throw new IllegalArgumentException(s"Duplicate Toggle ids found: ${ids.mkString(",")}")
+        duplicateHandling match {
+          case FailParsingOnDuplicateId =>
+            throw new IllegalArgumentException(s"Duplicate Toggle ids found: ${ids.mkString(",")}")
+          case KeepFirstOnDuplicateId =>
+            // Group by id and take the first entry of each group
+            val filteredMetadata = metadata.groupBy(_.id).values.map(_.head).toList
+            new ToggleMap.Immutable(filteredMetadata)
+        }
+      } else {
+        new ToggleMap.Immutable(metadata)
       }
-      new ToggleMap.Immutable(metadata)
     }
   }
 
@@ -151,10 +184,15 @@ object JsonToggleMap {
    * $example
    *
    * @param descriptionMode how to treat the "description" field for a toggle.
+   * @param duplicateHandling how to handle duplicates for a toggle in same file.
    */
-  def parse(json: String, descriptionMode: DescriptionMode): Try[ToggleMap] = Try {
+  def parse(
+    json: String,
+    descriptionMode: DescriptionMode,
+    duplicateHandling: DuplicateHandling
+  ): Try[ToggleMap] = Try {
     val jsonToggles = mapper.readValue(json, classOf[JsonToggles])
-    jsonToggles.toToggleMap("JSON String", descriptionMode)
+    jsonToggles.toToggleMap("JSON String", descriptionMode, duplicateHandling)
   }
 
   /**
@@ -167,10 +205,15 @@ object JsonToggleMap {
    * $example
    *
    * @param descriptionMode how to treat the "description" field for a toggle.
+   * @param duplicateHandling how to handle duplicates for a toggle in same file.
    */
-  def parse(url: URL, descriptionMode: DescriptionMode): Try[ToggleMap] = Try {
+  def parse(
+    url: URL,
+    descriptionMode: DescriptionMode,
+    duplicateHandling: DuplicateHandling
+  ): Try[ToggleMap] = Try {
     val jsonToggles = mapper.readValue(url, classOf[JsonToggles])
-    jsonToggles.toToggleMap(url.toString, descriptionMode)
+    jsonToggles.toToggleMap(url.toString, descriptionMode, duplicateHandling)
   }
 
   private case class Component(source: String, fraction: Double)

finagle-toggle/src/main/scala/com/twitter/finagle/toggle/StandardToggleMap.scala

@@ -3,10 +3,13 @@ package com.twitter.finagle.toggle
 import com.twitter.finagle.server.ServerInfo
 import com.twitter.finagle.stats.StatsReceiver
 import com.twitter.logging.Logger
-import com.twitter.util.{Return, Throw}
+import com.twitter.util.Return
+import com.twitter.util.Throw
 import java.net.URL
-import java.security.{DigestInputStream, MessageDigest}
-import java.util.concurrent.{ConcurrentHashMap, ConcurrentMap}
+import java.security.DigestInputStream
+import java.security.MessageDigest
+import java.util.concurrent.ConcurrentHashMap
+import java.util.concurrent.ConcurrentMap
 import scala.collection.JavaConverters._
 
 /**
@@ -76,13 +79,18 @@ object StandardToggleMap {
    *                      usage this should not be scoped so that the metrics
    *                      always end up scoped to "toggles/\$libraryName".
    */
-  def apply(libraryName: String, statsReceiver: StatsReceiver): ToggleMap.Mutable =
+  def apply(
+    libraryName: String,
+    statsReceiver: StatsReceiver,
+    duplicateHandling: JsonToggleMap.DuplicateHandling = JsonToggleMap.FailParsingOnDuplicateId
+  ): ToggleMap.Mutable =
     apply(
       libraryName,
       statsReceiver,
       ToggleMap.newMutable(s"Mutable($libraryName)"),
       ServerInfo(),
-      libs
+      libs,
+      duplicateHandling = duplicateHandling
     )
 
   /** exposed for testing */
@@ -91,13 +99,22 @@ object StandardToggleMap {
     statsReceiver: StatsReceiver,
     mutable: ToggleMap.Mutable,
     serverInfo: ServerInfo,
-    registry: ConcurrentMap[String, ToggleMap.Mutable]
+    registry: ConcurrentMap[String, ToggleMap.Mutable],
+    duplicateHandling: JsonToggleMap.DuplicateHandling
   ): ToggleMap.Mutable = {
     Toggle.validateId(libraryName)
 
     val svcsJson =
-      loadJsonConfig(s"$libraryName-service", serverInfo, JsonToggleMap.DescriptionIgnored)
-    val libsJson = loadJsonConfig(libraryName, serverInfo, JsonToggleMap.DescriptionRequired)
+      loadJsonConfig(
+        s"$libraryName-service",
+        serverInfo,
+        JsonToggleMap.DescriptionIgnored,
+        JsonToggleMap.FailParsingOnDuplicateId)
+    val libsJson = loadJsonConfig(
+      libraryName,
+      serverInfo,
+      JsonToggleMap.DescriptionRequired,
+      JsonToggleMap.FailParsingOnDuplicateId)
 
     val stacked = ToggleMap.of(
       mutable,
@@ -122,13 +139,14 @@ object StandardToggleMap {
   private[this] def loadJsonConfig(
     configName: String,
     serverInfo: ServerInfo,
-    descriptionMode: JsonToggleMap.DescriptionMode
+    descriptionMode: JsonToggleMap.DescriptionMode,
+    duplicateHandling: JsonToggleMap.DuplicateHandling
   ): ToggleMap = {
-    val withoutEnv = loadJsonConfigWithEnv(configName, descriptionMode)
+    val withoutEnv = loadJsonConfigWithEnv(configName, descriptionMode, duplicateHandling)
     val withEnv = serverInfo.environment match {
       case Some(env) =>
         val e = env.toString.toLowerCase
-        loadJsonConfigWithEnv(s"$configName-$e", descriptionMode)
+        loadJsonConfigWithEnv(s"$configName-$e", descriptionMode, duplicateHandling)
       case None =>
         NullToggleMap
     }
@@ -173,7 +191,8 @@ object StandardToggleMap {
 
   private[finagle] def loadJsonConfigWithEnv(
     configName: String,
-    descriptionMode: JsonToggleMap.DescriptionMode
+    descriptionMode: JsonToggleMap.DescriptionMode,
+    duplicateHandling: JsonToggleMap.DuplicateHandling
   ): ToggleMap = {
     val classLoader = getClass.getClassLoader
     val rscPath = s"com/twitter/toggles/configs/$configName.json"
@@ -184,7 +203,7 @@ object StandardToggleMap {
     } else {
       val rsc = selectResource(configName, rscs)
       log.debug(s"Toggle config resources found for $configName, using $rsc")
-      JsonToggleMap.parse(rsc, descriptionMode) match {
+      JsonToggleMap.parse(rsc, descriptionMode, duplicateHandling) match {
         case Throw(t) =>
           throw new IllegalArgumentException(
             s"Failure parsing Toggle config resources for $configName, from $rsc",

finagle-toggle/src/test/scala/com/twitter/finagle/toggle/JsonToggleMapTest.scala

@@ -1,14 +1,19 @@
 package com.twitter.finagle.toggle
 
-import com.twitter.util.{Return, Throw, Try}
+import com.twitter.util.Return
+import com.twitter.util.Throw
+import com.twitter.util.Try
 import org.scalacheck.Arbitrary.arbitrary
 import org.scalatestplus.scalacheck.ScalaCheckDrivenPropertyChecks
 import scala.collection.JavaConverters._
 import org.scalatest.funsuite.AnyFunSuite
 
 class JsonToggleMapTest extends AnyFunSuite with ScalaCheckDrivenPropertyChecks {
 
-  import JsonToggleMap.{DescriptionIgnored, DescriptionRequired}
+  import JsonToggleMap.DescriptionIgnored
+  import JsonToggleMap.DescriptionRequired
+  import JsonToggleMap.FailParsingOnDuplicateId
+  import JsonToggleMap.KeepFirstOnDuplicateId
 
   private def assertParseFails(input: String): Unit = {
     assertParseFails(input, DescriptionIgnored)
@@ -17,9 +22,10 @@ class JsonToggleMapTest extends AnyFunSuite with ScalaCheckDrivenPropertyChecks
 
   private def assertParseFails(
     input: String,
-    descriptionMode: JsonToggleMap.DescriptionMode
+    descriptionMode: JsonToggleMap.DescriptionMode,
+    duplicateHandling: JsonToggleMap.DuplicateHandling = JsonToggleMap.FailParsingOnDuplicateId
   ): Unit =
-    JsonToggleMap.parse(input, descriptionMode) match {
+    JsonToggleMap.parse(input, descriptionMode, duplicateHandling) match {
       case Return(_) => fail(s"Parsing should not succeed for $input")
       case Throw(_) => // expected
     }
@@ -38,19 +44,36 @@ class JsonToggleMapTest extends AnyFunSuite with ScalaCheckDrivenPropertyChecks
       |}""".stripMargin)
   }
 
+  val jsonWithDuplicates =
+    """
+      |{
+      |"toggles": [
+      |  {
+      |    "id": "com.twitter.duplicate",
+      |    "description": "cannot have duplicate ids even if other fields differ",
+      |    "fraction": 1.0
+      |  },
+      |  {
+      |    "id": "com.twitter.duplicate",
+      |    "description": "this is a duplicate",
+      |    "fraction": 0.1
+      |  }
+      |]
+      |}""".stripMargin
+
   test("parse invalid JSON string with duplicate ids") {
-    assertParseFails("""
-      |{"toggles": [
-      |    { "id": "com.twitter.duplicate",
-      |      "description": "cannot have duplicate ids even if other fields differ",
-      |      "fraction": 0.0
-      |    },
-      |    { "id": "com.twitter.duplicate",
-      |      "description": "this is a duplicate",
-      |      "fraction": 1.0
-      |    }
-      |  ]
-      |}""".stripMargin)
+    assertParseFails(jsonWithDuplicates)
+  }
+
+  test("parse JSON string with duplicate ids and keep first when KeepFirstOnDuplicateId is set") {
+    JsonToggleMap.parse(jsonWithDuplicates, DescriptionIgnored, KeepFirstOnDuplicateId) match {
+      case Throw(t) =>
+        fail(t)
+      case Return(tm) =>
+        assert(tm.iterator.size == 1)
+        assert(tm.apply("com.twitter.duplicate").isDefined)
+        assert(tm.apply("com.twitter.duplicate").isEnabled(1))
+    }
   }
 
   test("parse invalid JSON string with empty description") {
@@ -76,11 +99,11 @@ class JsonToggleMapTest extends AnyFunSuite with ScalaCheckDrivenPropertyChecks
       |}""".stripMargin
 
   test("parse JSON string with no description and is required") {
-    assertParseFails(jsonWithNoDescription, DescriptionRequired)
+    assertParseFails(jsonWithNoDescription, DescriptionRequired, FailParsingOnDuplicateId)
   }
 
   test("parse JSON string with no description and is ignored") {
-    JsonToggleMap.parse(jsonWithNoDescription, DescriptionIgnored) match {
+    JsonToggleMap.parse(jsonWithNoDescription, DescriptionIgnored, FailParsingOnDuplicateId) match {
       case Throw(t) =>
         fail(t)
       case Return(tm) =>
@@ -151,16 +174,18 @@ class JsonToggleMapTest extends AnyFunSuite with ScalaCheckDrivenPropertyChecks
   }
 
   test("parse valid JSON String") {
-    validateParsedJson(JsonToggleMap.parse(validInput, DescriptionIgnored))
-    validateParsedJson(JsonToggleMap.parse(validInput, DescriptionRequired))
+    validateParsedJson(
+      JsonToggleMap.parse(validInput, DescriptionIgnored, FailParsingOnDuplicateId))
+    validateParsedJson(
+      JsonToggleMap.parse(validInput, DescriptionRequired, FailParsingOnDuplicateId))
   }
 
   test("parse valid JSON String with empty toggles") {
     val in = """
         |{
         |  "toggles": [ ]
         |}""".stripMargin
-    JsonToggleMap.parse(in, DescriptionRequired) match {
+    JsonToggleMap.parse(in, DescriptionRequired, FailParsingOnDuplicateId) match {
       case Throw(t) =>
         fail(t)
       case Return(map) =>
@@ -177,8 +202,9 @@ class JsonToggleMapTest extends AnyFunSuite with ScalaCheckDrivenPropertyChecks
       .toSeq
 
     assert(1 == rscs.size)
-    validateParsedJson(JsonToggleMap.parse(rscs.head, DescriptionIgnored))
-    validateParsedJson(JsonToggleMap.parse(rscs.head, DescriptionRequired))
+    validateParsedJson(JsonToggleMap.parse(rscs.head, DescriptionIgnored, FailParsingOnDuplicateId))
+    validateParsedJson(
+      JsonToggleMap.parse(rscs.head, DescriptionRequired, FailParsingOnDuplicateId))
   }
 
   test("parse invalid JSON resource file") {
@@ -192,7 +218,7 @@ class JsonToggleMapTest extends AnyFunSuite with ScalaCheckDrivenPropertyChecks
       .toSeq
 
     assert(1 == rscs.size)
-    JsonToggleMap.parse(rscs.head, DescriptionIgnored) match {
+    JsonToggleMap.parse(rscs.head, DescriptionIgnored, FailParsingOnDuplicateId) match {
       case Return(_) => fail(s"Parsing should not succeed")
       case Throw(_) => // expected
     }