Commit ca7ef551 authored by MozLando's avatar MozLando
Browse files

Merge #4959 



4959: Experiments: Make version comparisons more tolerant of invalid versions r=ekager a=mythmon

Fixes #4957




Co-authored-by: default avatarMike Cooper <mythmon@gmail.com>
parents fd5d8ffd 527521ed

components/service/experiments/src/main/java/mozilla/components/service/experiments/Experiment.kt

+4 −2
Original line number Diff line number Diff line
@@ -4,6 +4,8 @@ 


package mozilla.components.service.experiments



import mozilla.components.service.experiments.util.VersionString



/**

 * Represents an A/B test experiment,

 * independent of the underlying
@@ -83,11 +85,11 @@ internal data class Experiment( 
        /**

         * App minimum version, expected dotted numeric version E.g. 1.0.2, or 67.0.1

         */

        val appMinVersion: String?,

        val appMinVersion: VersionString?,

        /**

         * App maximum version, expected dotted numeric version E.g. 1.0.2, or 67.0.1

         */

        val appMaxVersion: String?,

        val appMaxVersion: VersionString?,

        /**

         * Locale language, as a regex.

         */

components/service/experiments/src/main/java/mozilla/components/service/experiments/ExperimentEvaluator.kt

+7 −6
Original line number Diff line number Diff line
@@ -87,7 +87,7 @@ internal class ExperimentEvaluator( 
    private fun matches(context: Context, experiment: Experiment): Boolean {

        val match = experiment.match

        val region = valuesProvider.getRegion(context)

        val version = valuesProvider.getVersion(context) ?: ""

        val version = VersionString(valuesProvider.getVersion(context) ?: "")

        val matchesRegion = (region == null) ||

            match.regions.isNullOrEmpty() ||

            match.regions.any { it == region }
@@ -95,14 +95,15 @@ internal class ExperimentEvaluator( 
        val matchesTag = (tag == null) ||

            match.debugTags.isNullOrEmpty() ||

            match.debugTags.any { it == tag }

        val matchesMinVersion = match.appMinVersion.isNullOrEmpty() ||

            VersionString(match.appMinVersion) <= VersionString(version)

        val matchesMaxVersion = match.appMaxVersion.isNullOrEmpty() ||

            VersionString(match.appMaxVersion) >= VersionString(version)



        val matchesMinVersion = match.appMinVersion == null ||

                version.isValid() && match.appMinVersion.isValid() && match.appMinVersion <= version

        val matchesMaxVersion = match.appMaxVersion == null ||

                version.isValid() && match.appMaxVersion.isValid() && match.appMaxVersion >= version



        return matchesRegion && matchesTag && matchesMinVersion && matchesMaxVersion &&

            matchesExperiment(match.appId, valuesProvider.getAppId(context)) &&

            matchesExperiment(match.appDisplayVersion, version) &&

            matchesExperiment(match.appDisplayVersion, version.toString()) &&

            matchesExperiment(match.localeLanguage, valuesProvider.getLanguage(context)) &&

            matchesExperiment(match.localeCountry, valuesProvider.getCountry(context)) &&

            matchesExperiment(match.deviceManufacturer, valuesProvider.getManufacturer(context)) &&

components/service/experiments/src/main/java/mozilla/components/service/experiments/JSONExperimentParser.kt

+12 −2
Original line number Diff line number Diff line
@@ -4,6 +4,7 @@ 


package mozilla.components.service.experiments



import mozilla.components.service.experiments.util.VersionString

import mozilla.components.support.ktx.android.org.json.putIfNotNull

import mozilla.components.support.ktx.android.org.json.toList

import mozilla.components.support.ktx.android.org.json.tryGetLong
@@ -46,8 +47,8 @@ internal class JSONExperimentParser { 
        val matcher = Experiment.Matcher(

                appId = matchObject.tryGetString(MATCH_APP_ID_KEY),

                appDisplayVersion = matchObject.tryGetString(MATCH_APP_DISPLAY_VERSION_KEY),

                appMinVersion = matchObject.tryGetString(MATCH_APP_MIN_VERSION_KEY),

                appMaxVersion = matchObject.tryGetString(MATCH_APP_MAX_VERSION_KEY),

                appMinVersion = matchObject.tryGetVersionString(MATCH_APP_MIN_VERSION_KEY),

                appMaxVersion = matchObject.tryGetVersionString(MATCH_APP_MAX_VERSION_KEY),

                localeLanguage = matchObject.tryGetString(MATCH_LOCALE_LANGUAGE_KEY),

                localeCountry = matchObject.tryGetString(MATCH_LOCALE_COUNTRY_KEY),

                deviceManufacturer = matchObject.tryGetString(MATCH_DEVICE_MANUFACTURER_KEY),
@@ -144,3 +145,12 @@ internal class JSONExperimentParser { 
        private const val MATCH_DEBUG_TAGS_KEY = "debug_tags"

    }

}



private fun JSONObject.tryGetVersionString(key: String): VersionString? {

    val s = tryGetString(key)

    if (s != null) {

        return VersionString(s)

    } else {

        return null

    }

}

components/service/experiments/src/main/java/mozilla/components/service/experiments/util/VersionString.kt

+6 −2
Original line number Diff line number Diff line
@@ -25,10 +25,10 @@ internal class VersionString( 


    @Suppress("ComplexMethod")

    override fun compareTo(other: VersionString): Int {

        if (!version.matches(VERSION_REGEX)) {

        if (!isValid()) {

            throw IllegalArgumentException("Unexpected format in VersionString")

        }

        if (!other.version.matches(VERSION_REGEX)) {

        if (!other.isValid()) {

            throw IllegalArgumentException("Unexpected format in VersionString")

        }

        val thisParts = version.split(".")
@@ -75,4 +75,8 @@ internal class VersionString( 
    override fun hashCode(): Int {

        return version.hashCode()

    }



    fun isValid(): Boolean = version.matches(VERSION_REGEX)



    override fun toString(): String = version

}

components/service/experiments/src/test/java/mozilla/components/service/experiments/ExperimentEvaluatorTest.kt

+131 −4
Original line number Diff line number Diff line
@@ -9,6 +9,7 @@ import android.content.SharedPreferences 
import android.content.pm.PackageInfo

import android.content.pm.PackageManager

import androidx.test.ext.junit.runners.AndroidJUnit4

import mozilla.components.service.experiments.util.VersionString

import mozilla.components.support.test.mock

import mozilla.components.support.test.robolectric.testContext

import org.junit.Assert
@@ -261,7 +262,7 @@ class ExperimentEvaluatorTest { 
                localeLanguage = "eng",

                appId = "test.appId",

                regions = listOf("USA", "GBR"),

                appMinVersion = "1.0.0",

                appMinVersion = VersionString("1.0.0"),

                deviceManufacturer = "unknown",

                deviceModel = "robolectric",

                localeCountry = "USA"
@@ -308,7 +309,7 @@ class ExperimentEvaluatorTest { 
                localeLanguage = "eng",

                appId = "test.appId",

                regions = listOf("USA", "GBR"),

                appMaxVersion = "2.0.0",

                appMaxVersion = VersionString("2.0.0"),

                deviceManufacturer = "unknown",

                deviceModel = "robolectric",

                localeCountry = "USA"
@@ -352,8 +353,8 @@ class ExperimentEvaluatorTest { 
                localeLanguage = "eng",

                appId = "test.appId",

                regions = listOf("USA", "GBR"),

                appMinVersion = "1.0.0",

                appMaxVersion = "2.0.0",

                appMinVersion = VersionString("1.0.0"),

                appMaxVersion = VersionString("2.0.0"),

                deviceManufacturer = "unknown",

                deviceModel = "robolectric",

                localeCountry = "USA"
@@ -395,6 +396,132 @@ class ExperimentEvaluatorTest { 
        assertNull(evaluator.evaluate(context, ExperimentDescriptor("testexperiment"), listOf(experiment), 20))

    }



    @Test

    fun `evaluate appMinVersion with invalid app version should never match`() {

        testReset(appId = "test.appId", versionName = "Invalid version")



        val experiment = createDefaultExperiment(

                id = "testexperiment",

                match = createDefaultMatcher(

                        localeLanguage = "eng",

                        appId = "test.appId",

                        regions = listOf("USA", "GBR"),

                        appMinVersion = VersionString("1.0.0"),

                        deviceManufacturer = "unknown",

                        deviceModel = "robolectric",

                        localeCountry = "USA"

                ),

                buckets = Experiment.Buckets(20, 70),

                lastModified = currentTime

        )



        val evaluator = ExperimentEvaluator()

        assertNull(evaluator.evaluate(context, ExperimentDescriptor("testexperiment"), listOf(experiment), 20))

    }



    @Test

    fun `evaluate appMaxVersion with invalid app version should never match`() {

        testReset(appId = "test.appId", versionName = "Invalid version")



        val experiment = createDefaultExperiment(

                id = "testexperiment",

                match = createDefaultMatcher(

                        localeLanguage = "eng",

                        appId = "test.appId",

                        regions = listOf("USA", "GBR"),

                        appMaxVersion = VersionString("1.0.0"),

                        deviceManufacturer = "unknown",

                        deviceModel = "robolectric",

                        localeCountry = "USA"

                ),

                buckets = Experiment.Buckets(20, 70),

                lastModified = currentTime

        )



        val evaluator = ExperimentEvaluator()

        assertNull(evaluator.evaluate(context, ExperimentDescriptor("testexperiment"), listOf(experiment), 20))

    }



    @Test

    fun `invalid app version doesn't prevent other matches`() {

        testReset(appId = "test.appId", versionName = "Invalid version")



        val experiment = createDefaultExperiment(

                id = "testexperiment",

                match = createDefaultMatcher(

                        localeLanguage = "eng",

                        appId = "test.appId",

                        regions = listOf("USA", "GBR"),

                        deviceManufacturer = "unknown",

                        deviceModel = "robolectric",

                        localeCountry = "USA"

                ),

                buckets = Experiment.Buckets(20, 70),

                lastModified = currentTime

        )



        val evaluator = ExperimentEvaluator()

        assertNotNull(evaluator.evaluate(context, ExperimentDescriptor("testexperiment"), listOf(experiment), 20))

    }



    @Test

    fun `invalid appMinVersion never matches`() {

        testReset(appId = "test.appId", versionName = "1.0.0")



        val experiment = createDefaultExperiment(

                id = "testexperiment",

                match = createDefaultMatcher(

                        localeLanguage = "eng",

                        appId = "test.appId",

                        regions = listOf("USA", "GBR"),

                        appMinVersion = VersionString("Invalid"),

                        deviceManufacturer = "unknown",

                        deviceModel = "robolectric",

                        localeCountry = "USA"

                ),

                buckets = Experiment.Buckets(20, 70),

                lastModified = currentTime

        )



        val evaluator = ExperimentEvaluator()

        assertNull(evaluator.evaluate(context, ExperimentDescriptor("testexperiment"), listOf(experiment), 20))



        packageInfo.versionName = "1.1.0"

        assertNull(evaluator.evaluate(context, ExperimentDescriptor("testexperiment"), listOf(experiment), 20))



        packageInfo.versionName = "0.9"

        assertNull(evaluator.evaluate(context, ExperimentDescriptor("testexperiment"), listOf(experiment), 20))

    }



    @Test

    fun `invalid appMaxVersion never matches`() {

        testReset(appId = "test.appId", versionName = "1.0.0")



        val experiment = createDefaultExperiment(

                id = "testexperiment",

                match = createDefaultMatcher(

                        localeLanguage = "eng",

                        appId = "test.appId",

                        regions = listOf("USA", "GBR"),

                        appMaxVersion = VersionString("Invalid"),

                        deviceManufacturer = "unknown",

                        deviceModel = "robolectric",

                        localeCountry = "USA"

                ),

                buckets = Experiment.Buckets(20, 70),

                lastModified = currentTime

        )



        val evaluator = ExperimentEvaluator()

        assertNull(evaluator.evaluate(context, ExperimentDescriptor("testexperiment"), listOf(experiment), 20))



        packageInfo.versionName = "1.1.0"

        assertNull(evaluator.evaluate(context, ExperimentDescriptor("testexperiment"), listOf(experiment), 20))



        packageInfo.versionName = "0.9"

        assertNull(evaluator.evaluate(context, ExperimentDescriptor("testexperiment"), listOf(experiment), 20))

    }



    @Test

    fun `evaluate deviceModel`() {

        testReset(appId = "test.appId", versionName = "test.version")