Commit eda2ba63 authored by MickeyMoz's avatar MickeyMoz
Browse files

Merge #4111 

4111:  Bug 1556995 - Fix Glean sending of empty StringListMetricType r=travis79 a=travis79

This addresses an issue where Glean currently accepts an empty list when calling `set()` on a `StringListMetricType`, which would cause the empty metric to be collected and sent in the ping.
---
<!-- Text above this line will be added to the commit once "bors" merges this PR -->

### Pull Request checklist
<!-- Before submitting the PR, please address each item -->
- [ ] **Quality**: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
- [ ] **Tests**: This PR includes thorough tests or an explanation of why it does not
- [ ] **Changelog**: This PR includes [a changelog entry](https://github.com/mozilla-mobile/android-components/blob/master/docs/changelog.md) or does not need one
- [ ] **Accessibility**: The code in this PR follows [accessibility best practices](https://github.com/mozilla-mobile/shared-docs/blob/master/android/accessibility_guide.md) or does not include any user facing features

### After merge
- [ ] **Milestone**: Make sure issues closed by this pull request are added to the [milestone](https://github.com/mozilla-mobile/android-components/milestones) of the version currently in development.
- [ ] **Breaking Changes**: If this is a breaking change, please push a draft PR on [Reference Browser](https://github.com/mozilla-mobile/reference-browser

) to address the breaking issues.


Co-authored-by: default avatarTravis Long <tlong@mozilla.com>
parents da75090e 02804e45

components/service/glean/src/main/java/mozilla/components/service/glean/storages/StringListsStorageEngine.kt

+13 −1
Original line number Diff line number Diff line
@@ -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,

components/service/glean/src/test/java/mozilla/components/service/glean/storages/StringListsStorageEngineTest.kt

+23 −0
Original line number Diff line number Diff line
@@ -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(