From 16767af6902eaeb4bb48942bf6b35d95449e1081 Mon Sep 17 00:00:00 2001 From: Jonathan Almeida Date: Tue, 23 Aug 2022 23:40:41 -0400 Subject: [PATCH] Close #9829: Move IO work in FxaPushSupportFeature to a coroutine The `FxaPushSupportFeature` needs to generate, read and write a push scope (identifier) to disk. This work was meant to be done lazily and not during app startup. However, if a Sync account is setup then the IO work is done immediately during the initial account manager initialization at startup. In this patch, we delegate this IO work to a new `PushScopeProperty` that retrieves the value from disk using a provided `CoroutineScope`. --- components/feature/accounts-push/README.md | 5 +- components/feature/accounts-push/build.gradle | 6 +++ .../accounts/push/FxaPushSupportFeature.kt | 53 +++++++------------ .../accounts/push/cache/PushScopeProperty.kt | 43 +++++++++++++++ .../accounts/push/cache/ScopeProperty.kt | 18 +++++++ .../push/FxaPushSupportFeatureTest.kt | 23 ++++++-- docs/changelog.md | 3 ++ 7 files changed, 113 insertions(+), 38 deletions(-) create mode 100644 components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/cache/PushScopeProperty.kt create mode 100644 components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/cache/ScopeProperty.kt diff --git a/components/feature/accounts-push/README.md b/components/feature/accounts-push/README.md index 3fcdee26f50..385500c2323 100644 --- a/components/feature/accounts-push/README.md +++ b/components/feature/accounts-push/README.md @@ -51,7 +51,10 @@ in order to fix the problem. Therefore, if FxA and Push are used together, include the support feature as well: ```kotlin -FxaPushSupportFeature(context, fxaAccountManager, autoPushFeature) +val feature = FxaPushSupportFeature(context, fxaAccountManager, autoPushFeature) + +// initialize the feature; this can be done immediately if needed. +feature.initalize() ``` ## License diff --git a/components/feature/accounts-push/build.gradle b/components/feature/accounts-push/build.gradle index 1d19273d7da..0cba60fcbed 100644 --- a/components/feature/accounts-push/build.gradle +++ b/components/feature/accounts-push/build.gradle @@ -27,6 +27,12 @@ android { } } +tasks.withType(org.jetbrains.kotlin.gradle.tasks.KotlinCompile).configureEach { + kotlinOptions { + freeCompilerArgs += "-Xopt-in=kotlinx.coroutines.ExperimentalCoroutinesApi" + } +} + dependencies { implementation project(':service-firefox-accounts') implementation project(':support-ktx') diff --git a/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt b/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt index 6ab0958b45e..d99752f7161 100644 --- a/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt +++ b/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/FxaPushSupportFeature.kt @@ -13,6 +13,7 @@ import androidx.lifecycle.ProcessLifecycleOwner import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.DelicateCoroutinesApi import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Dispatchers.Main import kotlinx.coroutines.GlobalScope import kotlinx.coroutines.launch import mozilla.components.concept.base.crash.Breadcrumb @@ -24,6 +25,7 @@ import mozilla.components.concept.sync.DeviceConstellation import mozilla.components.concept.sync.DeviceConstellationObserver import mozilla.components.concept.sync.DevicePushSubscription import mozilla.components.concept.sync.OAuthAccount +import mozilla.components.feature.accounts.push.cache.PushScopeProperty import mozilla.components.feature.push.AutoPushFeature import mozilla.components.feature.push.AutoPushSubscription import mozilla.components.feature.push.PushScope @@ -32,7 +34,6 @@ import mozilla.components.service.fxa.manager.ext.withConstellation import mozilla.components.support.base.log.logger.Logger import mozilla.components.support.base.utils.SharedPreferencesCache import org.json.JSONObject -import java.util.UUID import mozilla.components.concept.sync.AccountObserver as SyncAccountObserver internal const val PREFERENCE_NAME = "mozac_feature_accounts_push" @@ -48,61 +49,49 @@ internal const val PREF_FXA_SCOPE = "fxa_push_scope" * @param accountManager The FxaAccountManager. * @param pushFeature The [AutoPushFeature] if that is setup for observing push events. * @param crashReporter Instance of `CrashReporting` to record unexpected caught exceptions. + * @param coroutineScope The scope in which IO work within the feature should be performed on. * @param owner the lifecycle owner for the observer. Defaults to [ProcessLifecycleOwner]. * @param autoPause whether to stop notifying the observer during onPause lifecycle events. * Defaults to false so that observers are always notified. */ class FxaPushSupportFeature( private val context: Context, - accountManager: FxaAccountManager, - pushFeature: AutoPushFeature, + private val accountManager: FxaAccountManager, + private val pushFeature: AutoPushFeature, private val crashReporter: CrashReporting? = null, - owner: LifecycleOwner = ProcessLifecycleOwner.get(), - autoPause: Boolean = false + private val coroutineScope: CoroutineScope = CoroutineScope(Dispatchers.IO), + private val owner: LifecycleOwner = ProcessLifecycleOwner.get(), + private val autoPause: Boolean = false ) { /** * A unique scope for the FxA push subscription that is generated once and stored in SharedPreferences. * - * This scope is randomly generated and unique to the account on that particular device. + * This scope is randomly generated and unique to the app install. * (why this uuid? Note it is *not* reset on logout!) - * (isn't this racey? Different observers reference it) - * (maybe you just want the acct uid?) */ - private val fxaPushScope: String by lazy { - val prefs = preference(context) - - // Generate a unique scope if one doesn't exist. - val randomUuid = UUID.randomUUID().toString().replace("-", "") - - // Return a scope in the format example: "fxa_push_scope_a62d5f27c9d74af4996d057f0e0e9c38" - val scope = PUSH_SCOPE_PREFIX + randomUuid - - if (!prefs.contains(PREF_FXA_SCOPE)) { - prefs.edit().putString(PREF_FXA_SCOPE, scope).apply() - - return@lazy scope - } + private val pushScope = PushScopeProperty(context, coroutineScope) - // The default string is non-null, so we can safely cast. - prefs.getString(PREF_FXA_SCOPE, scope) as String - } - - init { - val autoPushObserver = AutoPushObserver(accountManager, pushFeature, fxaPushScope) + /** + * Initialize the support feature to launch the appropriate observers. + */ + fun initialize() = coroutineScope.launch { + val autoPushObserver = AutoPushObserver(accountManager, pushFeature, pushScope.value()) val accountObserver = AccountObserver( context, pushFeature, - fxaPushScope, + pushScope.value(), crashReporter, owner, autoPause ) - accountManager.register(accountObserver) + coroutineScope.launch(Main) { + accountManager.register(accountObserver) - pushFeature.register(autoPushObserver, owner, autoPause) + pushFeature.register(autoPushObserver, owner, autoPause) + } } companion object { @@ -128,7 +117,6 @@ internal class AccountObserver( @OptIn(DelicateCoroutinesApi::class) // GlobalScope usage override fun onAuthenticated(account: OAuthAccount, authType: AuthType) { - val constellationObserver = ConstellationObserver( context = context, push = push, @@ -395,7 +383,6 @@ internal class VerificationDelegate( internal data class VerificationState(val timestamp: Long, val totalCount: Int) -@VisibleForTesting internal fun preference(context: Context) = context.getSharedPreferences(PREFERENCE_NAME, Context.MODE_PRIVATE) internal fun AutoPushSubscription.into() = DevicePushSubscription( diff --git a/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/cache/PushScopeProperty.kt b/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/cache/PushScopeProperty.kt new file mode 100644 index 00000000000..44d9c433538 --- /dev/null +++ b/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/cache/PushScopeProperty.kt @@ -0,0 +1,43 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package mozilla.components.feature.accounts.push.cache + +import android.content.Context +import android.content.SharedPreferences +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.withContext +import mozilla.components.feature.accounts.push.FxaPushSupportFeature +import mozilla.components.feature.accounts.push.PREF_FXA_SCOPE +import mozilla.components.feature.accounts.push.preference +import mozilla.components.feature.push.PushScope +import java.util.UUID + +/** + * An implementation of a [ScopeProperty] that generates and stores a scope in [SharedPreferences]. + */ +internal class PushScopeProperty( + private val context: Context, + private val coroutineScope: CoroutineScope, +) : ScopeProperty { + + override suspend fun value(): PushScope = withContext(coroutineScope.coroutineContext) { + val prefs = preference(context) + + // Generate a unique scope if one doesn't exist. + val randomUuid = UUID.randomUUID().toString().replace("-", "") + + // Return a scope in the format example: "fxa_push_scope_a62d5f27c9d74af4996d057f0e0e9c38" + val scope = FxaPushSupportFeature.PUSH_SCOPE_PREFIX + randomUuid + + if (!prefs.contains(PREF_FXA_SCOPE)) { + prefs.edit().putString(PREF_FXA_SCOPE, scope).apply() + + return@withContext scope + } + + // The default string is non-null, so we can safely cast. + prefs.getString(PREF_FXA_SCOPE, scope) as String + } +} diff --git a/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/cache/ScopeProperty.kt b/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/cache/ScopeProperty.kt new file mode 100644 index 00000000000..30e40308883 --- /dev/null +++ b/components/feature/accounts-push/src/main/java/mozilla/components/feature/accounts/push/cache/ScopeProperty.kt @@ -0,0 +1,18 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package mozilla.components.feature.accounts.push.cache + +import mozilla.components.feature.push.PushScope + +/** + * A [ScopeProperty] implementation generates and holds the [PushScope]. + */ +interface ScopeProperty { + + /** + * Returns the [PushScope] value. + */ + suspend fun value(): PushScope +} diff --git a/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/FxaPushSupportFeatureTest.kt b/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/FxaPushSupportFeatureTest.kt index 80a509b53af..6242cfa13d8 100644 --- a/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/FxaPushSupportFeatureTest.kt +++ b/components/feature/accounts-push/src/test/java/mozilla/components/feature/accounts/push/FxaPushSupportFeatureTest.kt @@ -12,9 +12,11 @@ import mozilla.components.service.fxa.manager.FxaAccountManager import mozilla.components.support.test.any import mozilla.components.support.test.mock import mozilla.components.support.test.robolectric.testContext +import mozilla.components.support.test.rule.MainCoroutineRule import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Before +import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.mockito.ArgumentMatchers.anyBoolean @@ -25,18 +27,31 @@ import org.robolectric.RobolectricTestRunner @RunWith(RobolectricTestRunner::class) class FxaPushSupportFeatureTest { + @get:Rule + val coroutineTestRule = MainCoroutineRule() + private val pushFeature: AutoPushFeature = mock() private val accountManager: FxaAccountManager = mock() + private lateinit var feature: FxaPushSupportFeature + @Before fun setup() { preference(testContext).edit().remove(PREF_FXA_SCOPE).apply() `when`(pushFeature.config).thenReturn(mock()) + + feature = FxaPushSupportFeature( + context = testContext, + accountManager = accountManager, + pushFeature = pushFeature, + crashReporter = null, + coroutineScope = coroutineTestRule.scope + ) } @Test fun `account observer registered`() { - FxaPushSupportFeature(testContext, accountManager, pushFeature) + feature.initialize() verify(accountManager).register(any()) verify(pushFeature).register(any(), any(), anyBoolean()) @@ -44,7 +59,7 @@ class FxaPushSupportFeatureTest { @Test fun `feature generates and caches a scope`() { - FxaPushSupportFeature(testContext, accountManager, pushFeature) + feature.initialize() assertTrue(preference(testContext).contains(PREF_FXA_SCOPE)) } @@ -53,7 +68,7 @@ class FxaPushSupportFeatureTest { fun `feature does not generate a scope if one already exists`() { preference(testContext).edit().putString(PREF_FXA_SCOPE, "testScope").apply() - FxaPushSupportFeature(testContext, accountManager, pushFeature) + feature.initialize() val cachedScope = preference(testContext).getString(PREF_FXA_SCOPE, "") assertEquals("testScope", cachedScope!!) @@ -61,7 +76,7 @@ class FxaPushSupportFeatureTest { @Test fun `feature generates a partially predictable push scope`() { - FxaPushSupportFeature(testContext, accountManager, pushFeature) + feature.initialize() val cachedScope = preference(testContext).getString(PREF_FXA_SCOPE, "") diff --git a/docs/changelog.md b/docs/changelog.md index 608987f9c67..bd2d660bd7a 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -11,6 +11,9 @@ permalink: /changelog/ * [Gecko](https://github.com/mozilla-mobile/android-components/blob/main/buildSrc/src/main/java/Gecko.kt) * [Configuration](https://github.com/mozilla-mobile/android-components/blob/main/.config.yml) +* **feature-accounts-push**: + * ⚠️ **This is a breaking change**: `FxaPushSupportFeature` now requires to be explicitly started with `initialize`. + * The constructor for `FxaPushSupportFeature` has a `coroutineScope` parameter that defaults to a `CoroutineScope(Dispatchers.IO)`. # 105.0.0 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v104.0.0...v105.0.0)