Bug 1556995 - Fix Glean sending of empty StringListMetricType

rocketsroger · Aug 14, 2019 · 6e0022f · 6e0022f
1 parent 1e8ba04
commit 6e0022f
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 1 deletion.
diff --git a/...glean/src/main/java/mozilla/components/service/glean/storages/StringListsStorageEngine.kt b/...glean/src/main/java/mozilla/components/service/glean/storages/StringListsStorageEngine.kt
@@ -111,7 +111,8 @@ internal open class StringListsStorageEngineImplementation(
     /**
      * Sets a string list in the desired stores. This function will replace the existing list or
      * create a new list if it doesn't already exist. To add or append to an existing list, use
-     * [add] function.
+     * [add] function. If an empty list is passed in, then an [ErrorType.InvalidValue] will be
+     * generated and the method will return without recording.
      *
      * @param metricData object with metric settings
      * @param value the string list value to record
@@ -132,6 +133,17 @@ internal open class StringListsStorageEngineImplementation(
             it.take(MAX_STRING_LENGTH)
         }
 
+        // Record an error when attempting to record a zero-length list and return.
+        if (stringList.count() == 0) {
+            recordError(
+                metricData,
+                ErrorType.InvalidValue,
+                "Attempt to set() an empty string list to ${metricData.identifier}",
+                logger
+            )
+            return
+        }
+
         if (stringList.count() > MAX_LIST_LENGTH_VALUE) {
             recordError(
                 metricData,

diff --git a/...n/src/test/java/mozilla/components/service/glean/storages/StringListsStorageEngineTest.kt b/...n/src/test/java/mozilla/components/service/glean/storages/StringListsStorageEngineTest.kt
@@ -16,6 +16,7 @@ import org.json.JSONArray
 import org.json.JSONObject
 import org.junit.Assert
 import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNull
 import org.junit.Assert.assertTrue
 import org.junit.Rule
 import org.junit.Test
@@ -201,6 +202,28 @@ class StringListsStorageEngineTest {
         assertEquals(1, testGetNumRecordedErrors(metric, ErrorType.InvalidValue))
     }
 
+    @Test
+    fun `set() records an error and returns when passed an empty list`() {
+        val metric = StringListMetricType(
+            disabled = false,
+            category = "telemetry",
+            lifetime = Lifetime.Ping,
+            name = "string_list_metric",
+            sendInPings = listOf("store1")
+        )
+
+        StringListsStorageEngine.set(metricData = metric, value = listOf())
+
+        // If nothing was stored, then the snapshot should be null
+        val snapshot = StringListsStorageEngine.getSnapshot(
+            storeName = "store1",
+            clearStore = false)
+        assertNull("Empty list must not be stored", snapshot)
+
+        // Verify the error was recorded
+        assertEquals(1, testGetNumRecordedErrors(metric, ErrorType.InvalidValue))
+    }
+
     @Test
     fun `string list deserializer should correctly parse string lists`() {
         val persistedSample = mapOf(