Skip to content

Commit

Permalink
Fix #4750: Fixes SDK 31 support (#4752)
Browse files Browse the repository at this point in the history
## Explanation
Fixes #4750

This PR fixes issues that were introduced in #4747 when trying to use
the app on an actual API 31 device. The problems come from two breaking
changes in Android 12:
- Public activities, services, and providers must be marked as exported
(this only affected our SplashActivity).
- PendingIntents must be explicitly marked as mutable/immutable (this
only affects WorkManager, but that's tricky for us).

The WorkManager issue was fixed in version 2.7.0 (where we currently
depend on version 2.4.0), but we can't actually updated in a way that
would be low-risk to cherry-pick (since it would require updating our
Kotlin SDK to 1.5.x instead of 1.4.x). A safer option is to disable
WorkManager outright, but while doing this I discovered that we have
unrestricted getInstance() calls that will force WorkManager to get
created regardless. This was fixed by introducing a new
analytics-specific application creation listener and ensuring those do
not get called for SDK 31+. #4751 is tracking fixing this longer term.
Approximately 9% of our users will have analytics disabled until we
properly fix it & push a new release.

Note that I opted for a fix-forward instead of rolling back #4747 since
these are small PRs that need to be easily cherry-picked, and it's
guaranteed that the target SDK change be isolated to SDK 31 devices (so
developers would still be able to use the app on lower versions).

Proper testing for ensuring this works requires manual testing, for now
(as we don't have automated end-to-end tests). I manually verified that
the app opens and can be used on Android 12. One test exemption was
added for the new listener since it's an interface and thus not
testable.

A new regex check was also added to ensure WorkManager.getInstance calls
always go through the new listener to avoid future situations where they
can accidentally trigger an instance creation when it's not wanted.

## Essential Checklist
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
This is mainly an infrastructural change as it's literally fixing
whether the app can open at all on SDK 31+ devices, so there's not much
to show.
  • Loading branch information
BenHenning committed Nov 23, 2022
1 parent fdd2956 commit 0818246
Show file tree
Hide file tree
Showing 17 changed files with 101 additions and 23 deletions.
1 change: 1 addition & 0 deletions app/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@
android:name=".app.splash.SplashActivity"
android:label="@string/app_name"
android:screenOrientation="portrait"
android:exported="true"
android:theme="@style/SplashScreenTheme">
<intent-filter>
<action android:name="android.intent.action.MAIN" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,14 @@ abstract class AbstractOppiaApplication(
// in Bazel.
AppCompatDelegate.setCompatVectorFromResourcesEnabled(true)
}
FirebaseApp.initializeApp(applicationContext)
WorkManager.initialize(applicationContext, workManagerConfiguration)
// The current WorkManager version doesn't work in SDK 31+, so disable it.
// TODO(#4751): Re-enable WorkManager for S+.
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.S) {
FirebaseApp.initializeApp(applicationContext)
WorkManager.initialize(applicationContext, workManagerConfiguration)
val workManager = WorkManager.getInstance(applicationContext)
component.getAnalyticsStartupListenerStartupListeners().forEach { it.onCreate(workManager) }
}
component.getApplicationStartupListeners().forEach(ApplicationStartupListener::onCreate)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import androidx.work.Configuration
import dagger.BindsInstance
import org.oppia.android.app.activity.ActivityComponentImpl
import org.oppia.android.domain.oppialogger.ApplicationStartupListener
import org.oppia.android.domain.oppialogger.analytics.AnalyticsStartupListener
import javax.inject.Provider

/**
Expand All @@ -26,5 +27,7 @@ interface ApplicationComponent : ApplicationInjector {

fun getApplicationStartupListeners(): Set<ApplicationStartupListener>

fun getAnalyticsStartupListenerStartupListeners(): Set<AnalyticsStartupListener>

fun getWorkManagerConfiguration(): Configuration
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import dagger.BindsInstance
import dagger.Component
import dagger.Module
import dagger.Provides
import dagger.multibindings.Multibinds
import org.junit.Before
import org.junit.Rule
import org.junit.Test
Expand Down Expand Up @@ -51,6 +52,7 @@ import org.oppia.android.domain.hintsandsolution.HintsAndSolutionProdModule
import org.oppia.android.domain.onboarding.ExpirationMetaDataRetrieverModule
import org.oppia.android.domain.oppialogger.LogStorageModule
import org.oppia.android.domain.oppialogger.LoggingIdentifierModule
import org.oppia.android.domain.oppialogger.analytics.AnalyticsStartupListener
import org.oppia.android.domain.oppialogger.analytics.ApplicationLifecycleModule
import org.oppia.android.domain.oppialogger.analytics.CpuPerformanceSnapshotterModule
import org.oppia.android.domain.platformparameter.PlatformParameterModule
Expand Down Expand Up @@ -164,6 +166,12 @@ class DateTimeUtilTest {
fun provideGlobalLogLevel(): LogLevel = LogLevel.VERBOSE
}

@Module
interface AnalyticsStartupListenerTestModule {
@Multibinds
fun provideAnalyticsListenerSet(): Set<AnalyticsStartupListener>
}

// TODO(#89): Move this to a common test application component.
@Singleton
@Component(
Expand All @@ -190,7 +198,7 @@ class DateTimeUtilTest {
LoggingIdentifierModule::class, ApplicationLifecycleModule::class,
SyncStatusModule::class, TestingBuildFlavorModule::class,
EventLoggingConfigurationModule::class, ActivityRouterModule::class,
CpuPerformanceSnapshotterModule::class
CpuPerformanceSnapshotterModule::class, AnalyticsStartupListenerTestModule::class
]
)
interface TestApplicationComponent : ApplicationComponent {
Expand Down
1 change: 1 addition & 0 deletions domain/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ kt_android_library(
"//domain/src/main/java/org/oppia/android/domain/classify:classification_result",
"//domain/src/main/java/org/oppia/android/domain/oppialogger:oppia_logger",
"//domain/src/main/java/org/oppia/android/domain/oppialogger:startup_listener",
"//domain/src/main/java/org/oppia/android/domain/oppialogger/analytics:analytics_startup_listener",
"//domain/src/main/java/org/oppia/android/domain/oppialogger/exceptions:controller",
"//domain/src/main/java/org/oppia/android/domain/oppialogger/logscheduler:metric_log_scheduling_worker_factory",
"//domain/src/main/java/org/oppia/android/domain/oppialogger/loguploader:worker_factory",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package org.oppia.android.domain.oppialogger.analytics

import androidx.work.WorkManager

/**
* Analytics-specific application startup listener that receives an instance of [WorkManager] to
* perform analytics-specific initialization.
*/
interface AnalyticsStartupListener {
/**
* Called on application creation with the singleton, application-wide [WorkManager] that should
* be used for scheduling background analytics tasks.
*/
fun onCreate(workManager: WorkManager)
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@ Library for providing logging analytics to the Oppia android app.
load("@dagger//:workspace_defs.bzl", "dagger_rules")
load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_android_library")

kt_android_library(
name = "analytics_startup_listener",
srcs = [
"AnalyticsStartupListener.kt",
],
visibility = ["//:oppia_api_visibility"],
deps = [
"//third_party:androidx_work_work-runtime-ktx",
],
)

kt_android_library(
name = "controller",
srcs = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ kt_android_library(
deps = [
":worker",
"//domain/src/main/java/org/oppia/android/domain/oppialogger:startup_listener",
"//domain/src/main/java/org/oppia/android/domain/oppialogger/analytics:analytics_startup_listener",
"//domain/src/main/java/org/oppia/android/domain/oppialogger/logscheduler:metric_log_scheduling_worker",
"//third_party:androidx_work_work-runtime-ktx",
"//utility/src/main/java/org/oppia/android/util/logging:log_uploader",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
package org.oppia.android.domain.oppialogger.loguploader

import android.content.Context
import androidx.work.Constraints
import androidx.work.Data
import androidx.work.NetworkType
import androidx.work.PeriodicWorkRequest
import androidx.work.WorkManager
import org.oppia.android.domain.oppialogger.ApplicationStartupListener
import org.oppia.android.domain.oppialogger.analytics.AnalyticsStartupListener
import org.oppia.android.domain.oppialogger.logscheduler.MetricLogSchedulingWorker
import org.oppia.android.util.logging.LogUploader
import org.oppia.android.util.logging.MetricLogScheduler
Expand All @@ -22,14 +21,13 @@ import javax.inject.Inject
* on application creation.
*/
class LogReportWorkManagerInitializer @Inject constructor(
private val context: Context,
private val logUploader: LogUploader,
private val metricLogScheduler: MetricLogScheduler,
@PerformanceMetricsCollectionHighFrequencyTimeIntervalInMinutes
performanceMetricsCollectionHighFrequencyTimeInterval: PlatformParameterValue<Int>,
@PerformanceMetricsCollectionLowFrequencyTimeIntervalInMinutes
performanceMetricCollectionLowFrequencyTimeInterval: PlatformParameterValue<Int>
) : ApplicationStartupListener {
) : AnalyticsStartupListener {

private val logReportWorkerConstraints = Constraints.Builder()
.setRequiredNetworkType(NetworkType.CONNECTED)
Expand Down Expand Up @@ -126,8 +124,7 @@ class LogReportWorkManagerInitializer @Inject constructor(
.setConstraints(logReportWorkerConstraints)
.build()

override fun onCreate() {
val workManager = WorkManager.getInstance(context)
override fun onCreate(workManager: WorkManager) {
logUploader.enqueueWorkRequestForEvents(workManager, workRequestForUploadingEvents)
logUploader.enqueueWorkRequestForExceptions(workManager, workRequestForUploadingExceptions)
logUploader.enqueueWorkRequestForPerformanceMetrics(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package org.oppia.android.domain.oppialogger.loguploader
import dagger.Binds
import dagger.Module
import dagger.multibindings.IntoSet
import org.oppia.android.domain.oppialogger.ApplicationStartupListener
import org.oppia.android.domain.oppialogger.analytics.AnalyticsStartupListener

/** Provides [LogUploadWorker] related dependencies. */
@Module
Expand All @@ -13,5 +13,5 @@ interface LogReportWorkerModule {
@IntoSet
fun bindLogReportWorkRequest(
logReportWorkManagerInitializer: LogReportWorkManagerInitializer
): ApplicationStartupListener
): AnalyticsStartupListener
}
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
package org.oppia.android.domain.platformparameter.syncup

import android.annotation.SuppressLint
import android.content.Context
import androidx.annotation.VisibleForTesting
import androidx.work.Constraints
import androidx.work.Data
import androidx.work.ExistingPeriodicWorkPolicy
import androidx.work.NetworkType
import androidx.work.PeriodicWorkRequest
import androidx.work.WorkManager
import org.oppia.android.domain.oppialogger.ApplicationStartupListener
import org.oppia.android.domain.oppialogger.analytics.AnalyticsStartupListener
import org.oppia.android.util.platformparameter.PlatformParameterValue
import org.oppia.android.util.platformparameter.SyncUpWorkerTimePeriodHours
import java.util.UUID
Expand All @@ -21,9 +20,8 @@ import javax.inject.Inject
* from the remote service on application creation.
*/
class PlatformParameterSyncUpWorkManagerInitializer @Inject constructor(
private val context: Context,
@SyncUpWorkerTimePeriodHours private val workRequestRepeatInterval: PlatformParameterValue<Int>
) : ApplicationStartupListener {
) : AnalyticsStartupListener {

private val OPPIA_PLATFORM_PARAMETER_WORK_REQUEST_NAME = "OPPIA_PLATFORM_PARAMETER_WORK_REQUEST"

Expand Down Expand Up @@ -53,8 +51,7 @@ class PlatformParameterSyncUpWorkManagerInitializer @Inject constructor(
.setConstraints(platformParameterSyncUpWorkerConstraints)
.build()

override fun onCreate() {
val workManager = WorkManager.getInstance(context)
override fun onCreate(workManager: WorkManager) {
workManager.enqueueUniquePeriodicWork(
OPPIA_PLATFORM_PARAMETER_WORK_REQUEST_NAME,
ExistingPeriodicWorkPolicy.KEEP,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package org.oppia.android.domain.platformparameter.syncup
import dagger.Binds
import dagger.Module
import dagger.multibindings.IntoSet
import org.oppia.android.domain.oppialogger.ApplicationStartupListener
import org.oppia.android.domain.oppialogger.analytics.AnalyticsStartupListener

/** Provides [PlatformParameterSyncUpWorker] related dependencies. */
@Module
Expand All @@ -13,5 +13,5 @@ interface PlatformParameterSyncUpWorkerModule {
@IntoSet
fun bindLogUploadWorkRequest(
platformParameterSyncUpWorkManagerInitializer: PlatformParameterSyncUpWorkManagerInitializer
): ApplicationStartupListener
): AnalyticsStartupListener
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import androidx.work.Constraints
import androidx.work.Data
import androidx.work.DelegatingWorkerFactory
import androidx.work.NetworkType
import androidx.work.WorkManager
import androidx.work.testing.SynchronousExecutor
import androidx.work.testing.WorkManagerTestInitHelper
import com.google.common.truth.Truth.assertThat
Expand Down Expand Up @@ -113,7 +114,7 @@ class LogReportWorkManagerInitializerTest {

@Test
fun testWorkRequest_onCreate_enqueuesRequest_verifyRequestId() {
logReportWorkManagerInitializer.onCreate()
logReportWorkManagerInitializer.onCreate(WorkManager.getInstance(context))
testCoroutineDispatchers.runCurrent()

val enqueuedEventWorkRequestId = logReportWorkManagerInitializer.getWorkRequestForEventsId()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,12 @@ class PlatformParameterSyncUpWorkManagerInitializerTest {

@Test
fun testWorkRequest_onCreate_enqueuesRequest_verifyRequestId() {
syncUpWorkManagerInitializer.onCreate()
val workManager = WorkManager.getInstance(context)
syncUpWorkManagerInitializer.onCreate(workManager)
testCoroutineDispatchers.runCurrent()

val enqueuedSyncUpWorkRequestId = syncUpWorkManagerInitializer.getSyncUpWorkRequestId()

val workManager = WorkManager.getInstance(context)
// Get all the WorkRequestInfo which have been tagged with "PlatformParameterSyncUpWorker.TAG"
val workInfoList = workManager.getWorkInfosByTag(PlatformParameterSyncUpWorker.TAG).get()
// There should be only one such work request having "PlatformParameterSyncUpWorker.TAG" tag
Expand Down Expand Up @@ -136,7 +136,7 @@ class PlatformParameterSyncUpWorkManagerInitializerTest {

@Test
fun testWorkRequest_verifyWorkRequestPeriodicity() {
syncUpWorkManagerInitializer.onCreate()
syncUpWorkManagerInitializer.onCreate(WorkManager.getInstance(context))
testCoroutineDispatchers.runCurrent()

val syncUpWorkerTimePeriodInMs = syncUpWorkManagerInitializer.getSyncUpWorkerTimePeriod()
Expand Down
7 changes: 7 additions & 0 deletions scripts/assets/file_content_validation_checks.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -400,3 +400,10 @@ file_content_checks {
exempted_file_patterns: "testing.+?\\.kt"
required_content_regex: "decorateWithScreenName\\("
}
file_content_checks {
file_path_regex: ".+?.kt"
prohibited_content_regex: "WorkManager.getInstance"
failure_message: "Use AnalyticsStartupListener to retrieve an instance of WorkManager rather than fetching one using getInstance (as the latter may create a WorkManager if one isn't already present, and the application may intend to disable it)."
exempted_file_name: "app/src/main/java/org/oppia/android/app/application/AbstractOppiaApplication.kt"
exempted_file_patterns: ".+?Test\\.kt"
}
1 change: 1 addition & 0 deletions scripts/assets/test_file_exemptions.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,7 @@ exempted_file_path: "domain/src/main/java/org/oppia/android/domain/onboarding/te
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/oppialogger/ApplicationIdSeed.kt"
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/oppialogger/ApplicationStartupListener.kt"
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/oppialogger/LogStorageModule.kt"
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/AnalyticsStartupListener.kt"
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/CpuPerformanceLoggingTimePeriodMillis.kt"
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/LearnerAnalyticsInactivityLimitMillis.kt"
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/oppialogger/exceptions/UncaughtExceptionLoggerModule.kt"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,10 @@ class RegexPatternValidationCheckTest {
"Only colors from color_palette.xml may be used in component_colors.xml."
private val doesNotReferenceColorFromColorDefs =
"Only colors from color_defs.xml may be used in color_palette.xml."
private val doesNotUseWorkManagerGetInstance =
"Use AnalyticsStartupListener to retrieve an instance of WorkManager rather than fetching one" +
" using getInstance (as the latter may create a WorkManager if one isn't already present, " +
"and the application may intend to disable it)."
private val wikiReferenceNote =
"Refer to https://github.com/oppia/oppia-android/wiki/Static-Analysis-Checks" +
"#regexpatternvalidation-check for more details on how to fix this."
Expand Down Expand Up @@ -2260,6 +2264,31 @@ class RegexPatternValidationCheckTest {
assertThat(outContent.toString().trim()).isEqualTo(REGEX_CHECK_PASSED_OUTPUT_INDICATOR)
}

@Test
fun testFileContent_referenceGetInstance_fileContentIsNotCorrect() {
val prohibitedContent =
"""
val workManager = WorkManager.getInstance(context)
""".trimIndent()
tempFolder.newFolder("testfiles", "app", "src", "main", "java", "org", "oppia", "android")
val stringFilePath = "app/src/main/java/org/oppia/android/SomeInitializer.kt"
tempFolder.newFile("testfiles/$stringFilePath").writeText(prohibitedContent)

val exception = assertThrows(Exception::class) {
runScript()
}

// Verify that all patterns are properly detected & prohibited.
assertThat(exception).hasMessageThat().contains(REGEX_CHECK_FAILED_OUTPUT_INDICATOR)
assertThat(outContent.toString().trim())
.isEqualTo(
"""
$stringFilePath:1: $doesNotUseWorkManagerGetInstance
$wikiReferenceNote
""".trimIndent()
)
}

/** Runs the regex_pattern_validation_check. */
private fun runScript() {
main(File(tempFolder.root, "testfiles").absolutePath)
Expand Down

0 comments on commit 0818246

Please sign in to comment.