Skip to content

feat: SDK-4210: Standardize BACKGROUND_THREADING feature flag key naming #2598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
abdulraqeeb33 merged 1 commit into from
Mar 26, 2026
Merged

feat: SDK-4210: Standardize BACKGROUND_THREADING feature flag key naming #2598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions ...ignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureFlag.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,23 @@ internal enum class FeatureActivationMode {
*/
internal enum class FeatureFlag(
val key: String,
val activationMode: FeatureActivationMode,
val activationMode: FeatureActivationMode
) {
// Threading mode is selected once per app startup to avoid mixed-mode behavior mid-session.
BACKGROUND_THREADING("BACKGROUND_THREADING", FeatureActivationMode.APP_STARTUP),
//
// Naming convention:
// SDK_<MMmmpp>_<FEATURE_DESCRIPTION>
//
// Example:
// SDK_050800_BACKGROUND_THREADING
//
SDK_050800_BACKGROUND_THREADING(
"SDK_050800_BACKGROUND_THREADING",
FeatureActivationMode.APP_STARTUP
),
;

fun isEnabledIn(enabledKeys: Set<String>): Boolean {
return enabledKeys.contains(key)
}
}
4 changes: 2 additions & 2 deletions ...alSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ internal class FeatureManager(
val nextStates = featureStates.toMutableMap()

for (feature in FeatureFlag.entries) {
val desiredState = enabledFeatureKeys.contains(feature.key)
val desiredState = feature.isEnabledIn(enabledFeatureKeys)
when (feature.activationMode) {
FeatureActivationMode.IMMEDIATE -> {
nextStates[feature] = desiredState
Expand Down Expand Up @@ -108,7 +108,7 @@ internal class FeatureManager(
enabled: Boolean,
) {
when (feature) {
FeatureFlag.BACKGROUND_THREADING ->
FeatureFlag.SDK_050800_BACKGROUND_THREADING ->
ThreadingMode.updateUseBackgroundThreading(
enabled = enabled,
source = "FeatureManager:${feature.activationMode}"
Expand Down
2 changes: 1 addition & 1 deletion ...nalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ internal class StartupService(
val useBackgroundThreading =
try {
val featureManager = services.getService<IFeatureManager>()
featureManager.isEnabled(FeatureFlag.BACKGROUND_THREADING)
featureManager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING)
} catch (t: Throwable) {
Logging.warn("OneSignal: Failed to resolve BACKGROUND_THREADING in StartupService. Falling back to legacy thread.", t)
Copy link
Copy Markdown

@claude claude bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Two catch-block warning messages still reference the old BACKGROUND_THREADING key name after the PR renamed the canonical key to SDK_050800_BACKGROUND_THREADING. A developer searching logcat for SDK_050800_BACKGROUND_THREADING when diagnosing startup threading failures would silently miss these warnings in StartupService.kt:24 and OneSignalImp.kt:185. Update both strings to match the new canonical key.

Extended reasoning...

This PR correctly renamed every FeatureFlag.BACKGROUND_THREADING enum reference to FeatureFlag.SDK_050800_BACKGROUND_THREADING, but the adjacent human-readable strings inside catch blocks were not updated to match.

Affected locations:

  • StartupService.kt line 24: the Logging.warn message still says BACKGROUND_THREADING in StartupService
  • OneSignalImp.kt line 185: the Logging.warn message still says BACKGROUND_THREADING feature

How the inconsistency manifests:
Both sites follow the same pattern: a try block calls featureManager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) (correctly updated) and a catch (t: Throwable) block logs a warning (not updated). When an exception occurs during flag resolution, the logged message says BACKGROUND_THREADING while every other trace in the system says SDK_050800_BACKGROUND_THREADING.

Concrete step-by-step proof:

  1. At app startup, StartupService.scheduleStart() calls featureManager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING).
  2. Suppose IFeatureManager is not yet registered and getService() throws — the catch block fires.
  3. Logcat receives: OneSignal: Failed to resolve BACKGROUND_THREADING in StartupService.
  4. A developer greps logcat for SDK_050800_BACKGROUND_THREADING to trace what happened — this warning is absent from results, misleading them into thinking the flag was never consulted.

Why existing code does not prevent it:
Log message strings are plain string literals with no compile-time binding to the enum key. Renaming the enum entry produces no build error or warning for stale adjacent strings.

Impact:
No functional impact — the fallback value false is still returned correctly. The impact is purely on debuggability: stale log strings can mislead engineers triaging threading startup failures.

Fix:
Update both log strings to reference the new canonical key name SDK_050800_BACKGROUND_THREADING.

false
Expand Down
2 changes: 1 addition & 1 deletion ...al/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ internal class OneSignalCrashUploaderWrapper(
@Suppress("TooGenericExceptionCaught")
override fun start() {
if (!OtelSdkSupport.isSupported) return
if (featureManager.isEnabled(FeatureFlag.BACKGROUND_THREADING)) {
if (featureManager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING)) {
OneSignalDispatchers.launchOnIO {
try {
uploader.start()
Expand Down
2 changes: 1 addition & 1 deletion OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ internal class OneSignalImp(
}

return try {
featureManager.isEnabled(FeatureFlag.BACKGROUND_THREADING)
featureManager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING)
} catch (t: Throwable) {
Logging.warn("OneSignal: Failed to resolve BACKGROUND_THREADING feature, defaulting to legacy mode.", t)
false
Expand Down
18 changes: 18 additions & 0 deletions ...SDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureFlagTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package com.onesignal.core.internal.features

import io.kotest.core.spec.style.FunSpec
import io.kotest.matchers.shouldBe

class FeatureFlagTests : FunSpec({
test("feature flags use SDK_050800 style key naming") {
val keyPattern = Regex("^SDK_[0-9]{6}_[A-Z0-9_]+$")

FeatureFlag.entries.forEach { feature ->
keyPattern.matches(feature.key) shouldBe true
}
}

test("BACKGROUND_THREADING uses the expected canonical key") {
FeatureFlag.SDK_050800_BACKGROUND_THREADING.key shouldBe "SDK_050800_BACKGROUND_THREADING"
}
})
16 changes: 8 additions & 8 deletions .../onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class FeatureManagerTests : FunSpec({
test("initial state should enable BACKGROUND_THREADING when feature is present") {
// Given
val initialModel = mockk<ConfigModel>()
every { initialModel.features } returns listOf(FeatureFlag.BACKGROUND_THREADING.key)
every { initialModel.features } returns listOf(FeatureFlag.SDK_050800_BACKGROUND_THREADING.key)
val configModelStore = mockk<ConfigModelStore>()
every { configModelStore.model } returns initialModel
every { configModelStore.subscribe(any()) } just runs
Expand All @@ -28,7 +28,7 @@ class FeatureManagerTests : FunSpec({
val manager = FeatureManager(configModelStore)

// Then
manager.isEnabled(FeatureFlag.BACKGROUND_THREADING) shouldBe true
manager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) shouldBe true
ThreadingMode.useBackgroundThreading shouldBe true
}

Expand All @@ -42,13 +42,13 @@ class FeatureManagerTests : FunSpec({
val manager = FeatureManager(configModelStore)

val updatedModel = mockk<ConfigModel>()
every { updatedModel.features } returns listOf(FeatureFlag.BACKGROUND_THREADING.key)
every { updatedModel.features } returns listOf(FeatureFlag.SDK_050800_BACKGROUND_THREADING.key)

// When
manager.onModelReplaced(updatedModel, ModelChangeTags.HYDRATE)

// Then
manager.isEnabled(FeatureFlag.BACKGROUND_THREADING) shouldBe false
manager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) shouldBe false
ThreadingMode.useBackgroundThreading shouldBe false
}

Expand All @@ -61,7 +61,7 @@ class FeatureManagerTests : FunSpec({
every { configModelStore.subscribe(any()) } just runs
val manager = FeatureManager(configModelStore)

every { model.features } returns listOf(FeatureFlag.BACKGROUND_THREADING.key)
every { model.features } returns listOf(FeatureFlag.SDK_050800_BACKGROUND_THREADING.key)

// When
manager.onModelUpdated(
Expand All @@ -72,14 +72,14 @@ class FeatureManagerTests : FunSpec({
)

// Then
manager.isEnabled(FeatureFlag.BACKGROUND_THREADING) shouldBe false
manager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) shouldBe false
ThreadingMode.useBackgroundThreading shouldBe false
}

test("onModelUpdated should keep startup mode when initial mode is enabled") {
// Given
val model = mockk<ConfigModel>()
every { model.features } returns listOf(FeatureFlag.BACKGROUND_THREADING.key)
every { model.features } returns listOf(FeatureFlag.SDK_050800_BACKGROUND_THREADING.key)
val configModelStore = mockk<ConfigModelStore>()
every { configModelStore.model } returns model
every { configModelStore.subscribe(any()) } just runs
Expand All @@ -96,7 +96,7 @@ class FeatureManagerTests : FunSpec({
)

// Then
manager.isEnabled(FeatureFlag.BACKGROUND_THREADING) shouldBe true
manager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) shouldBe true
ThreadingMode.useBackgroundThreading shouldBe true
}
})
2 changes: 1 addition & 1 deletion ...K/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class StartupServiceTests : FunSpec({
backgroundThreadingEnabled: Boolean = true,
): ServiceProvider {
val featureManager = mockk<IFeatureManager>()
every { featureManager.isEnabled(FeatureFlag.BACKGROUND_THREADING) } returns backgroundThreadingEnabled
every { featureManager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) } returns backgroundThreadingEnabled

val serviceBuilder = ServiceBuilder()
serviceBuilder.register(featureManager).provides<IFeatureManager>()
Expand Down
2 changes: 1 addition & 1 deletion ...ore/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapperTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class OneSignalCrashUploaderWrapperTest : FunSpec({

fun mockFeatureManager(backgroundThreadingEnabled: Boolean = true): IFeatureManager {
val featureManager = mockk<IFeatureManager>()
every { featureManager.isEnabled(FeatureFlag.BACKGROUND_THREADING) } returns backgroundThreadingEnabled
every { featureManager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) } returns backgroundThreadingEnabled
return featureManager
}

Expand Down
Loading