From 347b359db37328f5be79ae10e1268b4cb4808cd0 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Thu, 9 Apr 2026 13:37:08 -0400 Subject: [PATCH 01/23] fix(ci): diff coverage uses PR base SHA and aggregate gate - Compare base.sha...head.sha on pull_request so the target branch matches the PR base. - Enforce >= threshold on aggregate touched executable lines; keep per-file detail informational. - Fail when main-source files in the diff cannot be mapped to JaCoCo. - Rename workflow step for clarity. Made-with: Cursor --- .github/workflows/ci.yml | 6 +- OneSignalSDK/coverage/checkCoverage.sh | 181 +++++++++++++++---------- 2 files changed, 114 insertions(+), 73 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a925c0706..6e857b194 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,9 +63,13 @@ jobs: else echo "bypass=false" >> $GITHUB_OUTPUT fi - - name: "[Diff Coverage] Check coverage" + - name: "[PR] Coverage on changed lines (min ${{ env.DIFF_COVERAGE_THRESHOLD }}%)" if: github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch' working-directory: OneSignalSDK + env: + # Exact PR diff (vs merge ref / wrong base branch) + DIFF_BASE_SHA: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || '' }} + DIFF_HEAD_SHA: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || '' }} run: | # Use the shared coverage check script for consistency # Generate markdown report for PR comments diff --git a/OneSignalSDK/coverage/checkCoverage.sh b/OneSignalSDK/coverage/checkCoverage.sh index 26708f0af..4b3eb461e 100755 --- a/OneSignalSDK/coverage/checkCoverage.sh +++ b/OneSignalSDK/coverage/checkCoverage.sh @@ -21,6 +21,9 @@ NC='\033[0m' # No Color # Configuration COVERAGE_THRESHOLD=${DIFF_COVERAGE_THRESHOLD:-80} BASE_BRANCH=${BASE_BRANCH:-origin/main} +# When set (e.g. GitHub PR), diff against these SHAs so we measure exactly the PR patch, not the merge ref +DIFF_BASE_SHA=${DIFF_BASE_SHA:-} +DIFF_HEAD_SHA=${DIFF_HEAD_SHA:-} GENERATE_MARKDOWN=${GENERATE_MARKDOWN:-false} # Set to 'true' for CI/CD to generate markdown report SKIP_COVERAGE_CHECK=${SKIP_COVERAGE_CHECK:-false} # Set to 'true' to bypass coverage check (still runs but doesn't fail) @@ -71,15 +74,27 @@ fi echo -e "${GREEN}✓ Coverage report generated${NC}\n" # Step 2: Check diff coverage using manual method (reliable path matching) -echo -e "${YELLOW}[2/3] Checking diff coverage against $BASE_BRANCH...${NC}" -echo -e "${YELLOW}Threshold: ${COVERAGE_THRESHOLD}%${NC}\n" +if [ -n "$DIFF_BASE_SHA" ] && [ -n "$DIFF_HEAD_SHA" ]; then + DIFF_RANGE="${DIFF_BASE_SHA}...${DIFF_HEAD_SHA}" + echo -e "${YELLOW}[2/3] Checking diff coverage for PR range ${DIFF_BASE_SHA:0:7}...${DIFF_HEAD_SHA:0:7}${NC}" +else + DIFF_RANGE="${BASE_BRANCH}...HEAD" + echo -e "${YELLOW}[2/3] Checking diff coverage against ${BASE_BRANCH}...HEAD${NC}" +fi +echo -e "${YELLOW}Threshold: ${COVERAGE_THRESHOLD}% (aggregate on changed executable lines)${NC}\n" # Get changed files (run from project root) -# Include committed changes, staged changes, and unstaged changes cd "$REPO_ROOT" -COMMITTED_FILES=$(git diff --name-only "$BASE_BRANCH"...HEAD 2>/dev/null | grep -E '\.(kt|java)$' || true) -STAGED_FILES=$(git diff --cached --name-only 2>/dev/null | grep -E '\.(kt|java)$' || true) -UNSTAGED_FILES=$(git diff --name-only 2>/dev/null | grep -E '\.(kt|java)$' || true) +if [ -n "$DIFF_BASE_SHA" ] && [ -n "$DIFF_HEAD_SHA" ]; then + COMMITTED_FILES=$(git diff --name-only "$DIFF_RANGE" 2>/dev/null | grep -E '\.(kt|java)$' || true) + STAGED_FILES="" + UNSTAGED_FILES="" +else + # Local / workflow_dispatch: committed vs base, plus working tree + COMMITTED_FILES=$(git diff --name-only "$BASE_BRANCH"...HEAD 2>/dev/null | grep -E '\.(kt|java)$' || true) + STAGED_FILES=$(git diff --cached --name-only 2>/dev/null | grep -E '\.(kt|java)$' || true) + UNSTAGED_FILES=$(git diff --name-only 2>/dev/null | grep -E '\.(kt|java)$' || true) +fi # Combine all, remove duplicates, and filter to OneSignalSDK files CHANGED_FILES=$(echo -e "$COMMITTED_FILES\n$STAGED_FILES\n$UNSTAGED_FILES" | grep -E '^OneSignalSDK/' | sort -u || true) @@ -102,6 +117,7 @@ else export MARKDOWN_REPORT export BASE_BRANCH export REPO_ROOT + export DIFF_RANGE python3 << PYEOF import xml.etree.ElementTree as ET import re @@ -116,47 +132,50 @@ generate_markdown = os.environ.get('GENERATE_MARKDOWN', 'false').lower() == 'tru markdown_report = os.environ.get('MARKDOWN_REPORT', 'diff_coverage.md') base_branch = os.environ.get('BASE_BRANCH', 'origin/main') repo_root_env = os.environ.get('REPO_ROOT') +diff_range = os.environ.get('DIFF_RANGE', '').strip() def get_changed_lines(file_path, project_root): """Get line numbers of added/modified lines from git diff""" try: - # First try to get diff from committed changes - result = subprocess.run( - ['git', 'diff', '--unified=0', base_branch + '...HEAD', '--', file_path], - capture_output=True, - text=True, - cwd=project_root - ) - - # If no committed changes, check staged changes - if result.returncode != 0 or not result.stdout.strip(): + if diff_range: result = subprocess.run( - ['git', 'diff', '--cached', '--unified=0', '--', file_path], + ['git', 'diff', '--unified=0', diff_range, '--', file_path], capture_output=True, text=True, cwd=project_root ) - - # If no staged changes, check unstaged changes - if result.returncode != 0 or not result.stdout.strip(): - result = subprocess.run( - ['git', 'diff', '--unified=0', '--', file_path], - capture_output=True, - text=True, - cwd=project_root - ) - - # If still nothing, try alternative base branch format - if result.returncode != 0 or not result.stdout.strip(): + if result.returncode != 0 or not result.stdout.strip(): + return None + else: result = subprocess.run( - ['git', 'diff', '--unified=0', base_branch, 'HEAD', '--', file_path], + ['git', 'diff', '--unified=0', base_branch + '...HEAD', '--', file_path], capture_output=True, text=True, cwd=project_root ) - - if result.returncode != 0 or not result.stdout.strip(): - return None + if result.returncode != 0 or not result.stdout.strip(): + result = subprocess.run( + ['git', 'diff', '--cached', '--unified=0', '--', file_path], + capture_output=True, + text=True, + cwd=project_root + ) + if result.returncode != 0 or not result.stdout.strip(): + result = subprocess.run( + ['git', 'diff', '--unified=0', '--', file_path], + capture_output=True, + text=True, + cwd=project_root + ) + if result.returncode != 0 or not result.stdout.strip(): + result = subprocess.run( + ['git', 'diff', '--unified=0', base_branch, 'HEAD', '--', file_path], + capture_output=True, + text=True, + cwd=project_root + ) + if result.returncode != 0 or not result.stdout.strip(): + return None changed_lines = set() current_new_line = None @@ -217,11 +236,15 @@ total_uncovered = 0 total_lines = 0 files_below_threshold = [] files_checked = [] +eligible_paths = [] markdown_output = [] if generate_markdown: markdown_output.append("## Diff Coverage Report (Changed Lines Only)\n") - markdown_output.append(f"**Threshold:** {threshold}%\n\n") + markdown_output.append( + f"**Gate:** aggregate coverage on **changed executable lines** must be ≥ **{threshold}%** " + "(JaCoCo line data for lines touched in the diff).\n\n" + ) markdown_output.append("### Changed Files Coverage\n\n") for changed_file in changed_files: @@ -237,7 +260,9 @@ for changed_file in changed_files: match = re.search(r'src/main/(java|kotlin)/(.+)/([^/]+\.(kt|java))$', path_part) if not match: continue - + + eligible_paths.append(changed_file) + package_path = match.group(2) filename = match.group(3) package_name = package_path.replace('/', '/') @@ -273,18 +298,14 @@ for changed_file in changed_files: coverage_pct = (file_covered / file_total * 100) if file_total > 0 else 100 if generate_markdown: - status = "✅" if coverage_pct >= threshold else "❌" - changed_info = f" ({len(changed_lines)} changed lines)" if changed_lines else " (all lines - could not determine changed lines)" - markdown_output.append(f"- {status} **{filename}**: {file_covered}/{file_total} changed lines ({coverage_pct:.1f}%){changed_info}") + changed_info = f" ({len(changed_lines)} touched lines in diff)" if changed_lines else " (all lines — could not resolve diff)" + markdown_output.append(f"- **{filename}**: {file_covered}/{file_total} touched executable lines ({coverage_pct:.1f}%){changed_info}") if coverage_pct < threshold: files_below_threshold.append((filename, coverage_pct, file_uncovered)) - markdown_output.append(f" - ⚠️ Below threshold: {file_uncovered} uncovered changed lines") + markdown_output.append(f" - {file_uncovered} uncovered touched lines in this file") else: - status = "✓" if coverage_pct >= threshold else "✗" - color = "" if coverage_pct >= threshold else "\033[0;31m" - reset = "\033[0m" if color else "" - changed_info = f" ({len(changed_lines)} changed lines)" if changed_lines else " (all lines - could not determine changed lines)" - print(f" {color}{status}{reset} {filename}: {file_covered}/{file_total} changed lines ({coverage_pct:.1f}%){changed_info}") + changed_info = f" ({len(changed_lines)} touched lines)" if changed_lines else " (all lines — could not resolve diff)" + print(f" {filename}: {file_covered}/{file_total} touched executable lines ({coverage_pct:.1f}%){changed_info}") if coverage_pct < threshold: files_below_threshold.append((filename, coverage_pct, file_uncovered)) break @@ -299,39 +320,43 @@ for changed_file in changed_files: if total_lines > 0: overall_coverage = ((total_lines - total_uncovered) / total_lines * 100) - + overall_pass = overall_coverage >= threshold + if generate_markdown: - markdown_output.append(f"\n### Overall Coverage (Changed Lines Only)\n") - markdown_output.append(f"**{total_lines - total_uncovered}/{total_lines}** changed lines covered ({overall_coverage:.1f}%)\n") - + markdown_output.append(f"\n### Overall (aggregate gate)\n") + markdown_output.append( + f"**{total_lines - total_uncovered}/{total_lines}** touched executable lines covered " + f"({overall_coverage:.1f}% — requires ≥ {threshold}%)\n" + ) if files_below_threshold: - markdown_output.append(f"\n### ❌ Coverage Check Failed\n") - markdown_output.append(f"Files below {threshold}% threshold:\n") + markdown_output.append(f"\n**Per-file detail** (informational; gate is aggregate above):\n") for filename, pct, uncovered in files_below_threshold: - markdown_output.append(f"- **{filename}**: {pct:.1f}% ({uncovered} uncovered changed lines)\n") - + markdown_output.append(f"- **{filename}**: {pct:.1f}% ({uncovered} uncovered touched lines)\n") + + if not overall_pass: + markdown_output.append(f"\n### ❌ Coverage Check Failed\n") + markdown_output.append( + f"Aggregate coverage on touched lines is **{overall_coverage:.1f}%** (minimum **{threshold}%**).\n" + ) + # Write markdown file with open(markdown_report, 'w') as f: f.write('\n'.join(markdown_output)) - + # Print to console print('\n'.join(markdown_output)) - - if files_below_threshold: - sys.exit(1) - else: - sys.exit(0) + + sys.exit(0 if overall_pass else 1) else: - print(f"\n Overall: {(total_lines - total_uncovered)}/{total_lines} changed lines covered ({overall_coverage:.1f}%)") - + print(f"\n Overall: {(total_lines - total_uncovered)}/{total_lines} touched executable lines covered ({overall_coverage:.1f}%)") + print(f" Gate: ≥ {threshold}% aggregate on touched lines — {'PASS' if overall_pass else 'FAIL'}") + if files_below_threshold: - print(f"\n Files below {threshold}% threshold:") + print(f"\n Per-file (informational):") for filename, pct, uncovered in files_below_threshold: - print(f" • {filename}: {pct:.1f}% ({uncovered} uncovered changed lines)") - sys.exit(1) - else: - print(f"\n ✓ All files meet {threshold}% threshold for changed lines") - sys.exit(0) + print(f" • {filename}: {pct:.1f}% ({uncovered} uncovered)") + + sys.exit(0 if overall_pass else 1) elif files_checked: # Files were found but had no executable lines if generate_markdown: @@ -343,14 +368,26 @@ elif files_checked: print("\n ✓ All checked files have no executable lines (or fully covered)") sys.exit(0) else: + if eligible_paths: + msg = ( + "Changed Kotlin/Java sources under `src/main` could not be matched to the merged JaCoCo report " + "(or the diff has no executable touched lines). Check module wiring and that unit tests run for those sources." + ) + if generate_markdown: + markdown_output.append(f"\n### ❌ No usable coverage for changed sources\n\n{msg}\n") + with open(markdown_report, 'w') as f: + f.write('\n'.join(markdown_output)) + print('\n'.join(markdown_output)) + else: + print(f"\n ✗ {msg}") + sys.exit(1) if generate_markdown: - markdown_output.append(f"\n### ⚠️ No Coverage Data\n") - markdown_output.append("No coverage data found for changed files\n") + markdown_output.append(f"\n### ✅ No main-source changes to gate\n") + markdown_output.append("No `src/main` Kotlin/Java changes in the diff.\n") with open(markdown_report, 'w') as f: f.write('\n'.join(markdown_output)) else: - print("\n ⚠ No coverage data found for changed files") - print(" This may mean files aren't being compiled or tested") + print("\n ✓ No main-source Kotlin/Java changes in the diff") sys.exit(0) PYEOF @@ -358,10 +395,10 @@ PYEOF if [ $CHECK_RESULT -eq 1 ]; then if [ "$GENERATE_MARKDOWN" != "true" ]; then if [ -n "$BYPASS_REASON" ]; then - echo -e "\n${YELLOW}⚠ Coverage below threshold (files below ${COVERAGE_THRESHOLD}%)${NC}" + echo -e "\n${YELLOW}⚠ Aggregate touched-line coverage below ${COVERAGE_THRESHOLD}%${NC}" echo -e "${YELLOW} Build will not fail due to bypass: $BYPASS_REASON${NC}\n" else - echo -e "\n${RED}✗ Coverage check failed (files below ${COVERAGE_THRESHOLD}% threshold)${NC}\n" + echo -e "\n${RED}✗ Coverage check failed (aggregate on touched lines < ${COVERAGE_THRESHOLD}%)${NC}\n" fi else # In markdown mode, update the report to indicate bypass if applicable From 769382528cb3cd5662951669d3be5781cb0b7574 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Tue, 14 Apr 2026 11:55:39 -0400 Subject: [PATCH 02/23] SDK-4363: Turbine remote SDK feature flags and foreground refresh - GET /apps/:app_id/sdk/features/android/:sdk_version with validated version label - FeatureFlagsJsonParser (kotlinx.serialization), metadata as sibling JsonObjects - FeatureFlagsRefreshService with ~10 min foreground polling; ConfigModelChangeTags.REMOTE_FEATURE_FLAGS - FeatureManager merges remote flags; IFeatureManager.remoteFeatureFlagMetadata() - SDK_BACKGROUND_THREADING / sdk_background_threading; CoreModule wiring and tests Made-with: Cursor --- OneSignalSDK/build.gradle | 18 +-- OneSignalSDK/onesignal/core/build.gradle | 2 + .../java/com/onesignal/core/CoreModule.kt | 5 + .../backend/IFeatureFlagsBackendService.kt | 28 +++++ .../impl/FeatureFlagsBackendService.kt | 90 +++++++++++++ .../backend/impl/FeatureFlagsJsonParser.kt | 100 +++++++++++++++ .../core/internal/config/ConfigModel.kt | 21 +++- .../internal/config/ConfigModelChangeTags.kt | 13 ++ .../config/impl/FeatureFlagsRefreshService.kt | 119 ++++++++++++++++++ .../core/internal/features/FeatureFlag.kt | 12 +- .../core/internal/features/FeatureManager.kt | 35 +++++- .../core/internal/startup/StartupService.kt | 2 +- .../crash/OneSignalCrashUploaderWrapper.kt | 2 +- .../com/onesignal/internal/OneSignalImp.kt | 2 +- .../impl/FeatureFlagsBackendServiceTest.kt | 50 ++++++++ .../impl/FeatureFlagsJsonParserTests.kt | 90 +++++++++++++ .../internal/features/FeatureFlagTests.kt | 8 +- .../internal/features/FeatureManagerTests.kt | 57 +++++++-- .../internal/startup/StartupServiceTests.kt | 3 +- .../OneSignalCrashUploaderWrapperTest.kt | 3 +- 20 files changed, 621 insertions(+), 39 deletions(-) create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IFeatureFlagsBackendService.kt create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTest.kt create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt diff --git a/OneSignalSDK/build.gradle b/OneSignalSDK/build.gradle index c70a36b00..c5a8437f5 100644 --- a/OneSignalSDK/build.gradle +++ b/OneSignalSDK/build.gradle @@ -17,6 +17,7 @@ buildscript { kotlinVersion = '1.9.25' dokkaVersion = '1.9.10' coroutinesVersion = '1.7.3' + kotlinxSerializationJsonVersion = '1.6.3' kotestVersion = '5.8.0' ioMockVersion = '1.13.2' // AndroidX Lifecycle and Activity versions @@ -38,14 +39,15 @@ buildscript { maven { url 'https://developer.huawei.com/repo/' } } sharedDeps = [ - "com.android.tools.build:gradle:$androidGradlePluginVersion", - "com.google.gms:google-services:$googleServicesGradlePluginVersion", - "com.huawei.agconnect:agcp:$huaweiAgconnectVersion", - "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlinVersion", - "org.jetbrains.dokka:dokka-gradle-plugin:$dokkaVersion", - "io.gitlab.arturbosch.detekt:detekt-gradle-plugin:$detektVersion", - "com.diffplug.spotless:spotless-plugin-gradle:$spotlessVersion", - "com.vanniktech.maven.publish:com.vanniktech.maven.publish.gradle.plugin:0.32.0" + "com.android.tools.build:gradle:$androidGradlePluginVersion", + "com.google.gms:google-services:$googleServicesGradlePluginVersion", + "com.huawei.agconnect:agcp:$huaweiAgconnectVersion", + "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlinVersion", + "org.jetbrains.kotlin:kotlin-serialization:$kotlinVersion", + "org.jetbrains.dokka:dokka-gradle-plugin:$dokkaVersion", + "io.gitlab.arturbosch.detekt:detekt-gradle-plugin:$detektVersion", + "com.diffplug.spotless:spotless-plugin-gradle:$spotlessVersion", + "com.vanniktech.maven.publish:com.vanniktech.maven.publish.gradle.plugin:0.32.0" ] } diff --git a/OneSignalSDK/onesignal/core/build.gradle b/OneSignalSDK/onesignal/core/build.gradle index bf1142512..41d78a3a1 100644 --- a/OneSignalSDK/onesignal/core/build.gradle +++ b/OneSignalSDK/onesignal/core/build.gradle @@ -1,6 +1,7 @@ plugins { id 'com.android.library' id 'kotlin-android' + id 'org.jetbrains.kotlin.plugin.serialization' id 'com.diffplug.spotless' id 'com.vanniktech.maven.publish' id 'io.gitlab.arturbosch.detekt' @@ -73,6 +74,7 @@ dependencies { implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlinVersion" implementation "org.jetbrains.kotlinx:kotlinx-coroutines-core:$coroutinesVersion" implementation "org.jetbrains.kotlinx:kotlinx-coroutines-android:$coroutinesVersion" + implementation "org.jetbrains.kotlinx:kotlinx-serialization-json:$kotlinxSerializationJsonVersion" // AndroidX Lifecycle and Activity implementation "androidx.lifecycle:lifecycle-viewmodel-ktx:$lifecycleVersion" diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/CoreModule.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/CoreModule.kt index 9d34231d6..9998fd213 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/CoreModule.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/CoreModule.kt @@ -4,12 +4,15 @@ import com.onesignal.common.modules.IModule import com.onesignal.common.services.ServiceBuilder import com.onesignal.core.internal.application.IApplicationService import com.onesignal.core.internal.application.impl.ApplicationService +import com.onesignal.core.internal.backend.IFeatureFlagsBackendService import com.onesignal.core.internal.backend.IParamsBackendService +import com.onesignal.core.internal.backend.impl.FeatureFlagsBackendService import com.onesignal.core.internal.backend.impl.ParamsBackendService import com.onesignal.core.internal.background.IBackgroundManager import com.onesignal.core.internal.background.impl.BackgroundManager import com.onesignal.core.internal.config.ConfigModelStore import com.onesignal.core.internal.config.impl.ConfigModelStoreListener +import com.onesignal.core.internal.config.impl.FeatureFlagsRefreshService import com.onesignal.core.internal.database.IDatabaseProvider import com.onesignal.core.internal.database.impl.DatabaseProvider import com.onesignal.core.internal.device.IDeviceService @@ -61,7 +64,9 @@ internal class CoreModule : IModule { builder.register().provides() builder.register().provides() builder.register().provides() + builder.register().provides() builder.register().provides() + builder.register().provides() // Operations builder.register().provides() diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IFeatureFlagsBackendService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IFeatureFlagsBackendService.kt new file mode 100644 index 000000000..fcad601a4 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IFeatureFlagsBackendService.kt @@ -0,0 +1,28 @@ +package com.onesignal.core.internal.backend + +import kotlinx.serialization.json.JsonObject + +/** + * Result of the dedicated SDK feature-flags endpoint (separate from [IParamsBackendService]). + * + * @param enabledKeys Feature keys that should be treated as enabled for this device/SDK. + * @param metadata Optional per-flag payload (e.g. weights), keyed by flag id. Parsed from sibling + * keys in the response JSON (see [com.onesignal.core.internal.backend.impl.FeatureFlagsJsonParser]). + */ +internal data class RemoteFeatureFlagsResult( + val enabledKeys: List, + val metadata: JsonObject?, +) { + companion object { + val EMPTY = RemoteFeatureFlagsResult(emptyList(), null) + } +} + +/** + * Fetches remote feature flags for the current app via **GET** + * `apps/{app_id}/sdk/features/{platform}/{sdk_version}` (Turbine; see + * [com.onesignal.core.internal.backend.impl.FeatureFlagsBackendService]). + */ +internal interface IFeatureFlagsBackendService { + suspend fun fetchRemoteFeatureFlags(appId: String): RemoteFeatureFlagsResult +} diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt new file mode 100644 index 000000000..0258a305b --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt @@ -0,0 +1,90 @@ +package com.onesignal.core.internal.backend.impl + +import com.onesignal.common.OneSignalUtils +import com.onesignal.core.internal.backend.IFeatureFlagsBackendService +import com.onesignal.core.internal.backend.RemoteFeatureFlagsResult +import com.onesignal.core.internal.http.IHttpClient +import com.onesignal.debug.LogLevel +import com.onesignal.debug.internal.logging.Logging +import java.net.URLEncoder +import java.nio.charset.StandardCharsets + +/** + * Turbine SDK feature flags endpoint ([OneSignal/turbine#1681](https://github.com/OneSignal/turbine/pull/1681)). + * + * **GET** `apps/{app_id}/sdk/features/{platform}/{sdk_version}` relative to + * [com.onesignal.core.internal.config.ConfigModel.apiUrl] (app-provided base URL). + * + * - **platform** is always **`android`** for this SDK client. + * - **sdk_version** is [OneSignalUtils.sdkVersion] (same label as the `SDK-Version` header segment), e.g. + * `050801` or `050801-beta`; see [isValidFeaturesSdkVersionLabel]. + * + * Response: `{ "features": [ "flag_key", ... ] }`. + */ +internal class FeatureFlagsBackendService( + private val http: IHttpClient, +) : IFeatureFlagsBackendService { + override suspend fun fetchRemoteFeatureFlags(appId: String): RemoteFeatureFlagsResult { + Logging.log(LogLevel.DEBUG, "FeatureFlagsBackendService.fetchRemoteFeatureFlags(appId=$appId)") + + val sdkVersion = OneSignalUtils.sdkVersion + if (!isValidFeaturesSdkVersionLabel(sdkVersion)) { + Logging.warn( + "FeatureFlagsBackendService: sdk version not usable for Turbine path (expected " + + "6-digit label optional -suffix, e.g. 050801 or 050801-beta): '$sdkVersion'", + ) + return RemoteFeatureFlagsResult.EMPTY + } + + val path = + buildFeatureFlagsGetPath( + appId = appId, + platform = TURBINE_FEATURES_PLATFORM_ANDROID, + sdkVersion = sdkVersion, + ) + + val response = http.get(path, null) + if (!response.isSuccess || response.payload.isNullOrBlank()) { + Logging.debug( + "FeatureFlagsBackendService: non-success or empty body, status=${response.statusCode}", + ) + return RemoteFeatureFlagsResult.EMPTY + } + + return FeatureFlagsJsonParser.parse(response.payload!!) + } + + companion object { + /** + * Turbine `:platform` segment for the OneSignal Android SDK (this client). + */ + const val TURBINE_FEATURES_PLATFORM_ANDROID = "android" + + /** + * Labels produced by [OneSignalUtils.formatVersion] / [OneSignalUtils.sdkVersion]: six digits, + * optionally `-` and a non-empty prerelease/build suffix (no slashes or whitespace). + */ + private val FEATURES_SDK_VERSION_LABEL_REGEX = Regex("""^\d{6}(-[^/\s]+)?$""") + + /** + * Returns true when [label] is safe to send as the Turbine `:sdk_version` path segment. + */ + fun isValidFeaturesSdkVersionLabel(label: String): Boolean = FEATURES_SDK_VERSION_LABEL_REGEX.matches(label) + + /** + * Path only (relative to API base), matching `/apps/:app_id/sdk/features/:platform/:sdk_version`. + */ + internal fun buildFeatureFlagsGetPath( + appId: String, + platform: String, + sdkVersion: String, + ): String { + val p = encodePathSegment(platform) + val v = encodePathSegment(sdkVersion) + return "apps/$appId/sdk/features/$p/$v" + } + + private fun encodePathSegment(value: String): String = + URLEncoder.encode(value, StandardCharsets.UTF_8.name()).replace("+", "%20") + } +} diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt new file mode 100644 index 000000000..8c7fdb7f9 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt @@ -0,0 +1,100 @@ +package com.onesignal.core.internal.backend.impl + +import com.onesignal.core.internal.backend.RemoteFeatureFlagsResult +import kotlinx.serialization.json.Json +import kotlinx.serialization.json.JsonArray +import kotlinx.serialization.json.JsonElement +import kotlinx.serialization.json.JsonObject +import kotlinx.serialization.json.JsonPrimitive +import kotlinx.serialization.json.buildJsonObject + +/** + * Parses the feature-flags API response with kotlinx.serialization. + * + * Expected shape: + * ```json + * { + * "features": ["feature_a", "feature_b"], + * "feature_a": { "weight": 0.1 }, + * "feature_b": { ... } + * } + * ``` + * + * Turbine returns only `features` ([OneSignal/turbine#1681](https://github.com/OneSignal/turbine/pull/1681)). + * If the same flag id also appears as a root property with a JSON object value, that object is copied + * into [RemoteFeatureFlagsResult.metadata] for forward-compatible extras (flags without a sibling + * object stay enabled via [RemoteFeatureFlagsResult.enabledKeys] only). + * + * Payload must be valid JSON (e.g. a comma between `"features": [...]` and the next property). + */ +internal object FeatureFlagsJsonParser { + /** + * RFC 8259–style JSON only (no lenient tokens like unquoted keys, `NaN`, trailing commas). + * That keeps [encodeMetadata] strings portable: the same text can be parsed later by + * `org.json.JSONObject`, Gson, or kotlinx without relying on kotlinx-only quirks. + */ + val format = + Json { + ignoreUnknownKeys = true + isLenient = false + allowSpecialFloatingPointValues = false + prettyPrint = false + } + + fun parse(payload: String): RemoteFeatureFlagsResult { + return try { + val root = format.parseToJsonElement(payload) as? JsonObject ?: return RemoteFeatureFlagsResult.EMPTY + parseRoot(root) + } catch (_: Throwable) { + RemoteFeatureFlagsResult.EMPTY + } + } + + private fun parseRoot(root: JsonObject): RemoteFeatureFlagsResult { + val featuresEl = root["features"] ?: return RemoteFeatureFlagsResult.EMPTY + val featuresArray = featuresEl as? JsonArray ?: return RemoteFeatureFlagsResult.EMPTY + val keys = + featuresArray.mapNotNull { el -> + (el as? JsonPrimitive) + ?.takeIf { it.isString } + ?.content + ?.takeIf { it.isNotBlank() } + } + if (keys.isEmpty()) { + return RemoteFeatureFlagsResult(emptyList(), null) + } + + val metadata = + buildJsonObject { + for (key in keys) { + when (val v = root[key]) { + is JsonObject -> put(key, v) + else -> Unit + } + } + } + val metaOut = if (metadata.isEmpty()) null else metadata + return RemoteFeatureFlagsResult(keys, metaOut) + } + + fun encodeMetadata(metadata: JsonObject?): String? = + metadata?.let { format.encodeToString(JsonElement.serializer(), it) } + + /** + * Decodes [ConfigModel.sdkRemoteFeatureFlagMetadata] (a JSON object of flag id → object) into a map. + * Non-object values are skipped so each entry stays a [JsonObject] for nested decoding (e.g. with `Json.decodeFromJsonElement`). + */ + fun parseStoredMetadataMap(raw: String?): Map { + if (raw.isNullOrBlank()) { + return emptyMap() + } + return try { + val root = format.parseToJsonElement(raw) as? JsonObject ?: return emptyMap() + root.entries.mapNotNull { (key, value) -> + (value as? JsonObject)?.let { key to it } + }.toMap() + } catch (_: Throwable) { + emptyMap() + } + } +} diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt index a88739e05..004c6cba8 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt @@ -300,6 +300,25 @@ class ConfigModel : Model() { setListProperty(::features.name, value) } + /** + * Feature keys from the dedicated SDK feature-flags HTTP endpoint (see [com.onesignal.core.internal.backend.IFeatureFlagsBackendService]). + * Unioned with [features] for [com.onesignal.core.internal.features.FeatureManager]. + */ + var sdkRemoteFeatureFlags: List + get() = getListProperty(::sdkRemoteFeatureFlags.name) { emptyList() } + set(value) { + setListProperty(::sdkRemoteFeatureFlags.name, value) + } + + /** + * JSON object string: flag id → metadata (e.g. `note` or structured values) from the feature-flags endpoint. + */ + var sdkRemoteFeatureFlagMetadata: String? + get() = getOptStringProperty(::sdkRemoteFeatureFlagMetadata.name) + set(value) { + setOptStringProperty(::sdkRemoteFeatureFlagMetadata.name, value) + } + /** * The outcomes parameters */ @@ -344,7 +363,7 @@ class ConfigModel : Model() { property: String, jsonArray: JSONArray, ): List<*>? { - if (property == ::features.name) { + if (property == ::features.name || property == ::sdkRemoteFeatureFlags.name) { return buildList { for (i in 0 until jsonArray.length()) { val featureName = jsonArray.optString(i, "") diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt new file mode 100644 index 000000000..1c634c28d --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt @@ -0,0 +1,13 @@ +package com.onesignal.core.internal.config + +/** + * [ModelChangeTags][com.onesignal.common.modeling.ModelChangeTags]-style values used only for + * [ConfigModel] / [ConfigModelStore] replace notifications. + */ +internal object ConfigModelChangeTags { + /** + * Remote feature-flags HTTP endpoint updated [ConfigModel] SDK flag fields only + * ([sdkRemoteFeatureFlags], [sdkRemoteFeatureFlagMetadata]). + */ + const val REMOTE_FEATURE_FLAGS = "REMOTE_FEATURE_FLAGS" +} diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt new file mode 100644 index 000000000..6e7ce3a8b --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt @@ -0,0 +1,119 @@ +package com.onesignal.core.internal.config.impl + +import com.onesignal.common.modeling.ISingletonModelStoreChangeHandler +import com.onesignal.common.modeling.ModelChangeTags +import com.onesignal.common.modeling.ModelChangedArgs +import com.onesignal.core.internal.config.ConfigModelChangeTags +import com.onesignal.common.threading.launchOnIO +import com.onesignal.core.internal.application.IApplicationLifecycleHandler +import com.onesignal.core.internal.application.IApplicationService +import com.onesignal.core.internal.backend.IFeatureFlagsBackendService +import com.onesignal.core.internal.backend.impl.FeatureFlagsJsonParser +import com.onesignal.core.internal.config.ConfigModel +import com.onesignal.core.internal.config.ConfigModelStore +import com.onesignal.core.internal.startup.IStartableService +import com.onesignal.debug.internal.logging.Logging +import kotlin.coroutines.coroutineContext +import kotlinx.coroutines.Job +import kotlinx.coroutines.delay +import kotlinx.coroutines.isActive + +/** + * Fetches remote SDK feature flags when the app is in the foreground, immediately on focus and then + * every [REFRESH_INTERVAL_MS] while the session stays in the foreground. Updates + * [ConfigModel.sdkRemoteFeatureFlags] / [ConfigModel.sdkRemoteFeatureFlagMetadata] so + * [com.onesignal.core.internal.features.FeatureManager] stays in sync. + */ +internal class FeatureFlagsRefreshService( + private val applicationService: IApplicationService, + private val configModelStore: ConfigModelStore, + private val featureFlagsBackend: IFeatureFlagsBackendService, +) : IStartableService, + IApplicationLifecycleHandler, + ISingletonModelStoreChangeHandler { + private var pollJob: Job? = null + + override fun start() { + applicationService.addApplicationLifecycleHandler(this) + configModelStore.subscribe(this) + if (applicationService.isInForeground) { + onFocus(firedOnSubscribe = true) + } + } + + override fun onFocus(firedOnSubscribe: Boolean) { + restartForegroundPolling() + } + + override fun onUnfocused() { + pollJob?.cancel() + pollJob = null + } + + override fun onModelUpdated( + args: ModelChangedArgs, + tag: String, + ) { + if (args.property != ConfigModel::appId.name) { + return + } + if (applicationService.isInForeground) { + restartForegroundPolling() + } + } + + override fun onModelReplaced( + model: ConfigModel, + tag: String, + ) { + if (tag == ConfigModelChangeTags.REMOTE_FEATURE_FLAGS) { + return + } + if (tag != ModelChangeTags.HYDRATE && tag != ModelChangeTags.NORMAL) { + return + } + if (model.appId.isNotEmpty() && applicationService.isInForeground) { + restartForegroundPolling() + } + } + + private fun restartForegroundPolling() { + pollJob?.cancel() + pollJob = + launchOnIO { + while (coroutineContext.isActive) { + if (!applicationService.isInForeground) { + break + } + val appId = configModelStore.model.appId + if (appId.isNotEmpty()) { + try { + fetchAndApply(appId) + } catch (t: Throwable) { + Logging.warn("FeatureFlagsRefreshService: fetch failed", t) + } + } + delay(REFRESH_INTERVAL_MS) + } + } + } + + private suspend fun fetchAndApply(appId: String) { + val result = featureFlagsBackend.fetchRemoteFeatureFlags(appId) + val current = configModelStore.model + val newMetaString = FeatureFlagsJsonParser.encodeMetadata(result.metadata) + if (result.enabledKeys == current.sdkRemoteFeatureFlags && newMetaString == current.sdkRemoteFeatureFlagMetadata) { + return + } + + val updated = ConfigModel() + updated.initializeFromModel(null, current) + updated.sdkRemoteFeatureFlags = result.enabledKeys + updated.sdkRemoteFeatureFlagMetadata = newMetaString + configModelStore.replace(updated, ConfigModelChangeTags.REMOTE_FEATURE_FLAGS) + } + + companion object { + private const val REFRESH_INTERVAL_MS = 600_000L + } +} diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureFlag.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureFlag.kt index f9bad1176..784589dc2 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureFlag.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureFlag.kt @@ -17,6 +17,8 @@ internal enum class FeatureActivationMode { /** * Backend-driven feature switches used by the SDK. + * + * [key] values are **lowercase** strings as returned from remote config / Turbine `features` arrays. */ internal enum class FeatureFlag( val key: String, @@ -24,14 +26,10 @@ internal enum class FeatureFlag( ) { // Threading mode is selected once per app startup to avoid mixed-mode behavior mid-session. // - // Naming convention: - // SDK__ + // Remote key (lowercase) must match backend / ConfigCat flag id. // - // Example: - // SDK_050800_BACKGROUND_THREADING - // - SDK_050800_BACKGROUND_THREADING( - "SDK_050800_BACKGROUND_THREADING", + SDK_BACKGROUND_THREADING( + "sdk_background_threading", FeatureActivationMode.APP_STARTUP ), ; diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt index 0a05fb5ae..982ace966 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt @@ -4,12 +4,21 @@ import com.onesignal.common.modeling.ISingletonModelStoreChangeHandler import com.onesignal.common.modeling.ModelChangeTags import com.onesignal.common.modeling.ModelChangedArgs import com.onesignal.common.threading.ThreadingMode +import com.onesignal.core.internal.backend.impl.FeatureFlagsJsonParser import com.onesignal.core.internal.config.ConfigModel +import com.onesignal.core.internal.config.ConfigModelChangeTags import com.onesignal.core.internal.config.ConfigModelStore import com.onesignal.debug.internal.logging.Logging +import kotlinx.serialization.json.JsonObject internal interface IFeatureManager { fun isEnabled(feature: FeatureFlag): Boolean + + /** + * Per-flag payloads from [com.onesignal.core.internal.backend.IFeatureFlagsBackendService]. + * Each value is a [JsonObject] so callers can decode nested fields or map to `@Serializable` types. + */ + fun remoteFeatureFlagMetadata(): Map } @Suppress("TooGenericExceptionCaught") @@ -31,13 +40,19 @@ internal class FeatureManager( override fun isEnabled(feature: FeatureFlag): Boolean = featureStates[feature] ?: false + override fun remoteFeatureFlagMetadata(): Map = + FeatureFlagsJsonParser.parseStoredMetadataMap(configModelStore.model.sdkRemoteFeatureFlagMetadata) + @Suppress("TooGenericExceptionCaught") override fun onModelReplaced( model: ConfigModel, tag: String, ) { Logging.debug("OneSignal: FeatureManager.onModelReplaced(tag=$tag)") - if (tag == ModelChangeTags.HYDRATE || tag == ModelChangeTags.NORMAL) { + if (tag == ModelChangeTags.HYDRATE || + tag == ModelChangeTags.NORMAL || + tag == ConfigModelChangeTags.REMOTE_FEATURE_FLAGS + ) { try { refreshEnabledFeatures(model, applyNextRunOnlyFeatures = false) } catch (t: Throwable) { @@ -51,7 +66,10 @@ internal class FeatureManager( args: ModelChangedArgs, tag: String, ) { - if (args.property == ConfigModel::features.name) { + if (args.property == ConfigModel::features.name || + args.property == ConfigModel::sdkRemoteFeatureFlags.name || + args.property == ConfigModel::sdkRemoteFeatureFlagMetadata.name + ) { Logging.debug("OneSignal: FeatureManager.onModelUpdated(property=${args.property}, tag=$tag)") try { refreshEnabledFeatures(configModelStore.model, applyNextRunOnlyFeatures = false) @@ -66,7 +84,12 @@ internal class FeatureManager( model: ConfigModel, applyNextRunOnlyFeatures: Boolean, ) { - val enabledFeatureKeys = (model.features + localFeatureOverrides).toSet() + val enabledFeatureKeys = + ( + model.features + + model.sdkRemoteFeatureFlags + + localFeatureOverrides + ).toSet() if (localFeatureOverrides.isNotEmpty()) { Logging.warn( "OneSignal: Local feature override enabled for testing only: $localFeatureOverrides", @@ -108,7 +131,7 @@ internal class FeatureManager( enabled: Boolean, ) { when (feature) { - FeatureFlag.SDK_050800_BACKGROUND_THREADING -> + FeatureFlag.SDK_BACKGROUND_THREADING -> ThreadingMode.updateUseBackgroundThreading( enabled = enabled, source = "FeatureManager:${feature.activationMode}" @@ -120,10 +143,10 @@ internal class FeatureManager( /** * Local-only test hook for forcing features ON without backend config. * Add feature keys here while testing locally, e.g.: - * setOf(FeatureFlag.BACKGROUND_THREADING.key) + * setOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) */ private val localFeatureOverrides: Set = emptySet() // private val localFeatureOverrides: Set = -// setOf(FeatureFlag.BACKGROUND_THREADING.key) +// setOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt index b195716cc..7caa7602d 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt @@ -19,7 +19,7 @@ internal class StartupService( val useBackgroundThreading = try { val featureManager = services.getService() - featureManager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) + featureManager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING) } catch (t: Throwable) { Logging.warn("OneSignal: Failed to resolve BACKGROUND_THREADING in StartupService. Falling back to legacy thread.", t) false diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt index 8fc34c5d9..5b8fe7497 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt @@ -50,7 +50,7 @@ internal class OneSignalCrashUploaderWrapper( @Suppress("TooGenericExceptionCaught") override fun start() { if (!OtelSdkSupport.isSupported) return - if (featureManager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING)) { + if (featureManager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING)) { OneSignalDispatchers.launchOnIO { try { uploader.start() diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt index 5cd9cb917..b58d28aba 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt @@ -180,7 +180,7 @@ internal class OneSignalImp( } return try { - featureManager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) + featureManager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING) } catch (t: Throwable) { Logging.warn("OneSignal: Failed to resolve BACKGROUND_THREADING feature, defaulting to legacy mode.", t) false diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTest.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTest.kt new file mode 100644 index 000000000..705f6f16c --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTest.kt @@ -0,0 +1,50 @@ +package com.onesignal.core.internal.backend.impl + +import com.onesignal.common.OneSignalUtils +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.shouldBe + +class FeatureFlagsBackendServiceTest : FunSpec({ + test("buildFeatureFlagsGetPath matches Turbine /apps/:app_id/sdk/features/:platform/:sdk_version") { + FeatureFlagsBackendService.buildFeatureFlagsGetPath( + appId = "14719551-23f1-4d20-8dab-81496ffca5ea", + platform = FeatureFlagsBackendService.TURBINE_FEATURES_PLATFORM_ANDROID, + sdkVersion = "050801", + ) shouldBe "apps/14719551-23f1-4d20-8dab-81496ffca5ea/sdk/features/android/050801" + } + + test("buildFeatureFlagsGetPath encodes prerelease sdk version label") { + FeatureFlagsBackendService.buildFeatureFlagsGetPath( + appId = "14719551-23f1-4d20-8dab-81496ffca5ea", + platform = FeatureFlagsBackendService.TURBINE_FEATURES_PLATFORM_ANDROID, + sdkVersion = "050801-beta", + ) shouldBe "apps/14719551-23f1-4d20-8dab-81496ffca5ea/sdk/features/android/050801-beta" + } + + test("isValidFeaturesSdkVersionLabel accepts only 6-digit labels with optional -suffix") { + val valid = + listOf( + "050801", + "050801-beta", + "050801-beta1", + "010203-rc.2", + "000000", + ) + val invalid = + listOf( + "5.8.1", + "05080", + "0508010", + "", + "050801-", + "050801/", + "v050801", + ) + valid.forEach { FeatureFlagsBackendService.isValidFeaturesSdkVersionLabel(it) shouldBe true } + invalid.forEach { FeatureFlagsBackendService.isValidFeaturesSdkVersionLabel(it) shouldBe false } + } + + test("OneSignalUtils.sdkVersion from BuildConfig matches Turbine label rules") { + FeatureFlagsBackendService.isValidFeaturesSdkVersionLabel(OneSignalUtils.sdkVersion) shouldBe true + } +}) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt new file mode 100644 index 000000000..1c41e0922 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt @@ -0,0 +1,90 @@ +package com.onesignal.core.internal.backend.impl + +import com.onesignal.core.internal.backend.RemoteFeatureFlagsResult +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.shouldBe +import kotlinx.serialization.json.jsonObject +import kotlinx.serialization.json.jsonPrimitive + +class FeatureFlagsJsonParserTests : FunSpec({ + test("parses features array with sibling flag objects (sample contract)") { + val payload = + """ + { + "features": ["feature_a", "feature_b"], + "feature_a": { "weight": 0.1 }, + "feature_b": { "enabled": true } + } + """.trimIndent() + + val r = FeatureFlagsJsonParser.parse(payload) + + r.enabledKeys shouldBe listOf("feature_a", "feature_b") + val meta = r.metadata!! + meta["feature_a"]!!.toString().contains("weight") shouldBe true + meta["feature_a"]!!.toString().contains("0.1") shouldBe true + meta["feature_b"]!!.toString().contains("enabled") shouldBe true + } + + test("omits metadata entry when flag has no sibling object") { + val payload = """{"features":["only_key"]}""" + + val r = FeatureFlagsJsonParser.parse(payload) + + r.enabledKeys shouldBe listOf("only_key") + r.metadata shouldBe null + } + + test("features list plus sibling object for only some ids (valid JSON with commas)") { + val payload = + """ + { + "features": ["feature_a", "feature_b"], + "feature_a": { "weight": 0.1 } + } + """.trimIndent() + + val r = FeatureFlagsJsonParser.parse(payload) + + r.enabledKeys shouldBe listOf("feature_a", "feature_b") + val meta = r.metadata!! + meta.containsKey("feature_a") shouldBe true + meta.containsKey("feature_b") shouldBe false + meta["feature_a"]!!.jsonObject["weight"]!!.jsonPrimitive.content shouldBe "0.1" + } + + test("invalid json returns empty") { + FeatureFlagsJsonParser.parse("{") shouldBe RemoteFeatureFlagsResult.EMPTY + } + + test("encodeMetadata round-trips for storage string") { + val payload = + """ + {"features":["x"],"x":{"weight":2.5}} + """.trimIndent() + val r = FeatureFlagsJsonParser.parse(payload) + val encoded = FeatureFlagsJsonParser.encodeMetadata(r.metadata)!! + encoded.contains("weight") shouldBe true + encoded.contains("2.5") shouldBe true + } + + test("missing features key returns empty") { + FeatureFlagsJsonParser.parse("""{"feature_a":{}}""") shouldBe RemoteFeatureFlagsResult.EMPTY + } + + test("features not an array returns empty") { + FeatureFlagsJsonParser.parse("""{"features":"bad"}""") shouldBe RemoteFeatureFlagsResult.EMPTY + } + + test("parseStoredMetadataMap splits root object into flag id to JsonObject") { + val map = FeatureFlagsJsonParser.parseStoredMetadataMap("""{"a":{"weight":1},"b":2}""") + map.keys shouldBe setOf("a") + map.getValue("a")["weight"]!!.toString() shouldBe "1" + } + + test("parseStoredMetadataMap blank or invalid returns empty") { + FeatureFlagsJsonParser.parseStoredMetadataMap(null) shouldBe emptyMap() + FeatureFlagsJsonParser.parseStoredMetadataMap("") shouldBe emptyMap() + FeatureFlagsJsonParser.parseStoredMetadataMap("{") shouldBe emptyMap() + } +}) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureFlagTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureFlagTests.kt index 2a7c77cfe..54cf41b3a 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureFlagTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureFlagTests.kt @@ -4,15 +4,15 @@ 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_]+$") + test("feature flag remote keys are lowercase with underscores") { + val keyPattern = Regex("^[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" + test("SDK_BACKGROUND_THREADING uses the expected remote key") { + FeatureFlag.SDK_BACKGROUND_THREADING.key shouldBe "sdk_background_threading" } }) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt index e7edae371..1b30bfe23 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt @@ -7,6 +7,7 @@ import com.onesignal.core.internal.config.ConfigModelStore import io.kotest.core.spec.style.FunSpec import io.kotest.matchers.shouldBe import io.mockk.every +import kotlinx.serialization.json.jsonPrimitive import io.mockk.just import io.mockk.mockk import io.mockk.runs @@ -16,10 +17,16 @@ class FeatureManagerTests : FunSpec({ ThreadingMode.useBackgroundThreading = false } + fun stubConfigModel(model: ConfigModel) { + every { model.sdkRemoteFeatureFlags } returns emptyList() + every { model.sdkRemoteFeatureFlagMetadata } returns null + } + test("initial state should enable BACKGROUND_THREADING when feature is present") { // Given val initialModel = mockk() - every { initialModel.features } returns listOf(FeatureFlag.SDK_050800_BACKGROUND_THREADING.key) + stubConfigModel(initialModel) + every { initialModel.features } returns listOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) val configModelStore = mockk() every { configModelStore.model } returns initialModel every { configModelStore.subscribe(any()) } just runs @@ -28,13 +35,29 @@ class FeatureManagerTests : FunSpec({ val manager = FeatureManager(configModelStore) // Then - manager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) shouldBe true + manager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING) shouldBe true + ThreadingMode.useBackgroundThreading shouldBe true + } + + test("initial state enables BACKGROUND_THREADING when key only exists on sdk remote flags") { + val initialModel = mockk() + stubConfigModel(initialModel) + every { initialModel.features } returns emptyList() + every { initialModel.sdkRemoteFeatureFlags } returns listOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) + val configModelStore = mockk() + every { configModelStore.model } returns initialModel + every { configModelStore.subscribe(any()) } just runs + + val manager = FeatureManager(configModelStore) + + manager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING) shouldBe true ThreadingMode.useBackgroundThreading shouldBe true } test("onModelReplaced should not switch threading mode after startup") { // Given val initialModel = mockk() + stubConfigModel(initialModel) every { initialModel.features } returns emptyList() val configModelStore = mockk() every { configModelStore.model } returns initialModel @@ -42,26 +65,28 @@ class FeatureManagerTests : FunSpec({ val manager = FeatureManager(configModelStore) val updatedModel = mockk() - every { updatedModel.features } returns listOf(FeatureFlag.SDK_050800_BACKGROUND_THREADING.key) + stubConfigModel(updatedModel) + every { updatedModel.features } returns listOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) // When manager.onModelReplaced(updatedModel, ModelChangeTags.HYDRATE) // Then - manager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) shouldBe false + manager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING) shouldBe false ThreadingMode.useBackgroundThreading shouldBe false } test("onModelUpdated should not switch threading mode after startup") { // Given val model = mockk() + stubConfigModel(model) every { model.features } returns emptyList() val configModelStore = mockk() every { configModelStore.model } returns model every { configModelStore.subscribe(any()) } just runs val manager = FeatureManager(configModelStore) - every { model.features } returns listOf(FeatureFlag.SDK_050800_BACKGROUND_THREADING.key) + every { model.features } returns listOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) // When manager.onModelUpdated( @@ -72,14 +97,15 @@ class FeatureManagerTests : FunSpec({ ) // Then - manager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) shouldBe false + manager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING) shouldBe false ThreadingMode.useBackgroundThreading shouldBe false } test("onModelUpdated should keep startup mode when initial mode is enabled") { // Given val model = mockk() - every { model.features } returns listOf(FeatureFlag.SDK_050800_BACKGROUND_THREADING.key) + stubConfigModel(model) + every { model.features } returns listOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) val configModelStore = mockk() every { configModelStore.model } returns model every { configModelStore.subscribe(any()) } just runs @@ -96,7 +122,22 @@ class FeatureManagerTests : FunSpec({ ) // Then - manager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) shouldBe true + manager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING) shouldBe true ThreadingMode.useBackgroundThreading shouldBe true } + + test("remoteFeatureFlagMetadata returns parsed JSON from config") { + val initialModel = mockk() + stubConfigModel(initialModel) + every { initialModel.features } returns emptyList() + every { initialModel.sdkRemoteFeatureFlagMetadata } returns """{"X":{"note":"y"}}""" + val configModelStore = mockk() + every { configModelStore.model } returns initialModel + every { configModelStore.subscribe(any()) } just runs + + val manager = FeatureManager(configModelStore) + + val meta = manager.remoteFeatureFlagMetadata() + meta.getValue("X")["note"]!!.jsonPrimitive.content shouldBe "y" + } }) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt index dc7fa72bb..391aef73e 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt @@ -26,7 +26,8 @@ class StartupServiceTests : FunSpec({ backgroundThreadingEnabled: Boolean = true, ): ServiceProvider { val featureManager = mockk() - every { featureManager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) } returns backgroundThreadingEnabled + every { featureManager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING) } returns backgroundThreadingEnabled + every { featureManager.remoteFeatureFlagMetadata() } returns emptyMap() val serviceBuilder = ServiceBuilder() serviceBuilder.register(featureManager).provides() diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapperTest.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapperTest.kt index 63ff63b74..b5004ba6e 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapperTest.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapperTest.kt @@ -41,7 +41,8 @@ class OneSignalCrashUploaderWrapperTest : FunSpec({ fun mockFeatureManager(backgroundThreadingEnabled: Boolean = true): IFeatureManager { val featureManager = mockk() - every { featureManager.isEnabled(FeatureFlag.SDK_050800_BACKGROUND_THREADING) } returns backgroundThreadingEnabled + every { featureManager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING) } returns backgroundThreadingEnabled + every { featureManager.remoteFeatureFlagMetadata() } returns emptyMap() return featureManager } From c63f686ef2061423771e33ba64f4a193b3c45093 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Tue, 14 Apr 2026 12:27:40 -0400 Subject: [PATCH 03/23] feature flag PR tweaks --- .../impl/FeatureFlagsBackendService.kt | 31 ++++----- .../backend/impl/FeatureFlagsJsonParser.kt | 63 ++++++++++++++++--- .../impl/TurbineSdkFeatureFlagsPath.kt | 61 ++++++++++++++++++ .../core/internal/features/FeatureManager.kt | 13 +++- .../impl/FeatureFlagsJsonParserTests.kt | 33 +++++++--- .../impl/TurbineSdkFeatureFlagsPathTest.kt | 26 ++++++++ .../internal/features/FeatureManagerTests.kt | 16 ++++- .../internal/startup/StartupServiceTests.kt | 2 +- .../OneSignalCrashUploaderWrapperTest.kt | 2 +- 9 files changed, 202 insertions(+), 45 deletions(-) create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPath.kt create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPathTest.kt diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt index 0258a305b..b6a6885ce 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt @@ -6,12 +6,13 @@ import com.onesignal.core.internal.backend.RemoteFeatureFlagsResult import com.onesignal.core.internal.http.IHttpClient import com.onesignal.debug.LogLevel import com.onesignal.debug.internal.logging.Logging -import java.net.URLEncoder -import java.nio.charset.StandardCharsets /** * Turbine SDK feature flags endpoint ([OneSignal/turbine#1681](https://github.com/OneSignal/turbine/pull/1681)). * + * HTTP, logging, and [OneSignalUtils] are platform-specific; path shape and validation live in + * [TurbineSdkFeatureFlagsPath], and JSON parsing in [FeatureFlagsJsonParser] (both KMP-friendly). + * * **GET** `apps/{app_id}/sdk/features/{platform}/{sdk_version}` relative to * [com.onesignal.core.internal.config.ConfigModel.apiUrl] (app-provided base URL). * @@ -37,21 +38,22 @@ internal class FeatureFlagsBackendService( } val path = - buildFeatureFlagsGetPath( + TurbineSdkFeatureFlagsPath.buildGetPath( appId = appId, platform = TURBINE_FEATURES_PLATFORM_ANDROID, sdkVersion = sdkVersion, ) val response = http.get(path, null) - if (!response.isSuccess || response.payload.isNullOrBlank()) { + val body = response.payload + if (!response.isSuccess || body.isNullOrBlank()) { Logging.debug( "FeatureFlagsBackendService: non-success or empty body, status=${response.statusCode}", ) return RemoteFeatureFlagsResult.EMPTY } - return FeatureFlagsJsonParser.parse(response.payload!!) + return FeatureFlagsJsonParser.parse(body) } companion object { @@ -60,31 +62,20 @@ internal class FeatureFlagsBackendService( */ const val TURBINE_FEATURES_PLATFORM_ANDROID = "android" - /** - * Labels produced by [OneSignalUtils.formatVersion] / [OneSignalUtils.sdkVersion]: six digits, - * optionally `-` and a non-empty prerelease/build suffix (no slashes or whitespace). - */ - private val FEATURES_SDK_VERSION_LABEL_REGEX = Regex("""^\d{6}(-[^/\s]+)?$""") - /** * Returns true when [label] is safe to send as the Turbine `:sdk_version` path segment. + * @see TurbineSdkFeatureFlagsPath.isValidFeaturesSdkVersionLabel */ - fun isValidFeaturesSdkVersionLabel(label: String): Boolean = FEATURES_SDK_VERSION_LABEL_REGEX.matches(label) + fun isValidFeaturesSdkVersionLabel(label: String): Boolean = TurbineSdkFeatureFlagsPath.isValidFeaturesSdkVersionLabel(label) /** * Path only (relative to API base), matching `/apps/:app_id/sdk/features/:platform/:sdk_version`. + * @see TurbineSdkFeatureFlagsPath.buildGetPath */ internal fun buildFeatureFlagsGetPath( appId: String, platform: String, sdkVersion: String, - ): String { - val p = encodePathSegment(platform) - val v = encodePathSegment(sdkVersion) - return "apps/$appId/sdk/features/$p/$v" - } - - private fun encodePathSegment(value: String): String = - URLEncoder.encode(value, StandardCharsets.UTF_8.name()).replace("+", "%20") + ): String = TurbineSdkFeatureFlagsPath.buildGetPath(appId, platform, sdkVersion) } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt index 8c7fdb7f9..92a43125a 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt @@ -26,6 +26,13 @@ import kotlinx.serialization.json.buildJsonObject * object stay enabled via [RemoteFeatureFlagsResult.enabledKeys] only). * * Payload must be valid JSON (e.g. a comma between `"features": [...]` and the next property). + * + * Flag ids from the `features` array are normalized with [Char.lowercaseChar] (Unicode case mapping, no + * `java.util.Locale`) so they align with [com.onesignal.core.internal.features.FeatureFlag.key]. Sibling + * objects are resolved with exact key, then canonical key, then a case-insensitive match against other root + * properties (excluding `features`). + * + * Uses only Kotlin stdlib + kotlinx.serialization (Kotlin Multiplatform-friendly). */ internal object FeatureFlagsJsonParser { /** @@ -41,6 +48,8 @@ internal object FeatureFlagsJsonParser { prettyPrint = false } + private const val FEATURES_PROPERTY = "features" + fun parse(payload: String): RemoteFeatureFlagsResult { return try { val root = format.parseToJsonElement(payload) as? JsonObject ?: return RemoteFeatureFlagsResult.EMPTY @@ -51,32 +60,66 @@ internal object FeatureFlagsJsonParser { } private fun parseRoot(root: JsonObject): RemoteFeatureFlagsResult { - val featuresEl = root["features"] ?: return RemoteFeatureFlagsResult.EMPTY + val featuresEl = root[FEATURES_PROPERTY] ?: return RemoteFeatureFlagsResult.EMPTY val featuresArray = featuresEl as? JsonArray ?: return RemoteFeatureFlagsResult.EMPTY - val keys = + val flagEntries = featuresArray.mapNotNull { el -> (el as? JsonPrimitive) ?.takeIf { it.isString } ?.content - ?.takeIf { it.isNotBlank() } - } - if (keys.isEmpty()) { + ?.trim() + ?.takeIf { it.isNotEmpty() } + ?.let { raw -> raw to canonicalFeatureFlagId(raw) } + }.distinctBy { it.second } + + if (flagEntries.isEmpty()) { return RemoteFeatureFlagsResult(emptyList(), null) } + val keys = flagEntries.map { it.second } + val metadata = buildJsonObject { - for (key in keys) { - when (val v = root[key]) { - is JsonObject -> put(key, v) - else -> Unit - } + for ((rawKey, canonicalKey) in flagEntries) { + findSiblingJsonObject(root, rawKey, canonicalKey)?.let { put(canonicalKey, it) } } } val metaOut = if (metadata.isEmpty()) null else metadata return RemoteFeatureFlagsResult(keys, metaOut) } + private fun findSiblingJsonObject( + root: JsonObject, + rawKeyFromFeaturesArray: String, + canonicalKey: String, + ): JsonObject? { + for (candidate in listOf(rawKeyFromFeaturesArray, canonicalKey)) { + if (candidate == FEATURES_PROPERTY) { + continue + } + when (val v = root[candidate]) { + is JsonObject -> return v + else -> Unit + } + } + for ((k, v) in root) { + if (k == FEATURES_PROPERTY) { + continue + } + if (k.equals(rawKeyFromFeaturesArray, ignoreCase = true) && v is JsonObject) { + return v + } + } + return null + } + + private fun canonicalFeatureFlagId(raw: String): String = + buildString(raw.length) { + for (c in raw) { + append(c.lowercaseChar()) + } + } + fun encodeMetadata(metadata: JsonObject?): String? = metadata?.let { format.encodeToString(JsonElement.serializer(), it) } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPath.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPath.kt new file mode 100644 index 000000000..89f0e9158 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPath.kt @@ -0,0 +1,61 @@ +package com.onesignal.core.internal.backend.impl + +/** + * Builds the Turbine-relative path for SDK feature flags and validates the SDK version label. + * + * Pure Kotlin only (no `java.*` / Android APIs) so this file can be reused from Kotlin Multiplatform `commonMain`. + */ +internal object TurbineSdkFeatureFlagsPath { + private val FEATURES_SDK_VERSION_LABEL_REGEX = Regex("""^\d{6}(-[^/\s]+)?$""") + + /** + * Labels expected in the `:sdk_version` path segment: six digits, optionally `-` and a non-empty suffix + * (no slashes or whitespace). + */ + fun isValidFeaturesSdkVersionLabel(label: String): Boolean = FEATURES_SDK_VERSION_LABEL_REGEX.matches(label) + + /** + * Path only (relative to API base), matching `apps/{app_id}/sdk/features/{platform}/{sdk_version}`. + * [platform] and [sdkVersion] are UTF-8 percent-encoded per RFC 3986 (unreserved bytes left as-is). + */ + fun buildGetPath( + appId: String, + platform: String, + sdkVersion: String, + ): String { + val p = percentEncodePathSegmentUtf8(platform) + val v = percentEncodePathSegmentUtf8(sdkVersion) + return "apps/$appId/sdk/features/$p/$v" + } + + /** + * Percent-encodes a UTF-8 string as a single path segment: unreserved bytes stay literal; everything else + * becomes `%HH` (uppercase hex). Space becomes `%20` (not `+`), matching typical REST path encoding. + */ + internal fun percentEncodePathSegmentUtf8(segment: String): String { + val bytes = segment.encodeToByteArray() + return buildString(bytes.size * 3) { + for (b in bytes) { + val u = b.toInt() and 0xff + if (isUnreservedByte(u)) { + append(u.toChar()) + } else { + append('%') + append(HEX_DIGITS[u shr 4]) + append(HEX_DIGITS[u and 15]) + } + } + } + } + + private fun isUnreservedByte(u: Int): Boolean = + u in 0x41..0x5A || + u in 0x61..0x7A || + u in 0x30..0x39 || + u == 0x2D || + u == 0x2E || + u == 0x5F || + u == 0x7E + + private const val HEX_DIGITS = "0123456789ABCDEF" +} diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt index 982ace966..2c9646605 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt @@ -17,8 +17,10 @@ internal interface IFeatureManager { /** * Per-flag payloads from [com.onesignal.core.internal.backend.IFeatureFlagsBackendService]. * Each value is a [JsonObject] so callers can decode nested fields or map to `@Serializable` types. + * + * `null` when no metadata has been stored yet ([ConfigModel.sdkRemoteFeatureFlagMetadata] null/blank). */ - fun remoteFeatureFlagMetadata(): Map + fun remoteFeatureFlagMetadata(): Map? } @Suppress("TooGenericExceptionCaught") @@ -40,8 +42,13 @@ internal class FeatureManager( override fun isEnabled(feature: FeatureFlag): Boolean = featureStates[feature] ?: false - override fun remoteFeatureFlagMetadata(): Map = - FeatureFlagsJsonParser.parseStoredMetadataMap(configModelStore.model.sdkRemoteFeatureFlagMetadata) + override fun remoteFeatureFlagMetadata(): Map? { + val raw = configModelStore.model.sdkRemoteFeatureFlagMetadata + if (raw.isNullOrBlank()) { + return null + } + return FeatureFlagsJsonParser.parseStoredMetadataMap(raw) + } @Suppress("TooGenericExceptionCaught") override fun onModelReplaced( diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt index 1c41e0922..f882af324 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt @@ -20,10 +20,10 @@ class FeatureFlagsJsonParserTests : FunSpec({ val r = FeatureFlagsJsonParser.parse(payload) r.enabledKeys shouldBe listOf("feature_a", "feature_b") - val meta = r.metadata!! - meta["feature_a"]!!.toString().contains("weight") shouldBe true - meta["feature_a"]!!.toString().contains("0.1") shouldBe true - meta["feature_b"]!!.toString().contains("enabled") shouldBe true + val meta = requireNotNull(r.metadata) + meta.getValue("feature_a").toString().contains("weight") shouldBe true + meta.getValue("feature_a").toString().contains("0.1") shouldBe true + meta.getValue("feature_b").toString().contains("enabled") shouldBe true } test("omits metadata entry when flag has no sibling object") { @@ -47,10 +47,27 @@ class FeatureFlagsJsonParserTests : FunSpec({ val r = FeatureFlagsJsonParser.parse(payload) r.enabledKeys shouldBe listOf("feature_a", "feature_b") - val meta = r.metadata!! + val meta = requireNotNull(r.metadata) meta.containsKey("feature_a") shouldBe true meta.containsKey("feature_b") shouldBe false - meta["feature_a"]!!.jsonObject["weight"]!!.jsonPrimitive.content shouldBe "0.1" + val weight = requireNotNull(meta.getValue("feature_a").jsonObject["weight"]).jsonPrimitive.content + weight shouldBe "0.1" + } + + test("normalizes feature ids to lowercase and resolves sibling with mismatched casing") { + val payload = + """ + { + "features": ["SDK_Background_Threading"], + "sdk_background_threading": { "weight": 0.5 } + } + """.trimIndent() + + val r = FeatureFlagsJsonParser.parse(payload) + + r.enabledKeys shouldBe listOf("sdk_background_threading") + val meta = requireNotNull(r.metadata) + meta.getValue("sdk_background_threading").jsonObject.getValue("weight").jsonPrimitive.content shouldBe "0.5" } test("invalid json returns empty") { @@ -63,7 +80,7 @@ class FeatureFlagsJsonParserTests : FunSpec({ {"features":["x"],"x":{"weight":2.5}} """.trimIndent() val r = FeatureFlagsJsonParser.parse(payload) - val encoded = FeatureFlagsJsonParser.encodeMetadata(r.metadata)!! + val encoded = requireNotNull(FeatureFlagsJsonParser.encodeMetadata(r.metadata)) encoded.contains("weight") shouldBe true encoded.contains("2.5") shouldBe true } @@ -79,7 +96,7 @@ class FeatureFlagsJsonParserTests : FunSpec({ test("parseStoredMetadataMap splits root object into flag id to JsonObject") { val map = FeatureFlagsJsonParser.parseStoredMetadataMap("""{"a":{"weight":1},"b":2}""") map.keys shouldBe setOf("a") - map.getValue("a")["weight"]!!.toString() shouldBe "1" + requireNotNull(map.getValue("a")["weight"]).toString() shouldBe "1" } test("parseStoredMetadataMap blank or invalid returns empty") { diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPathTest.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPathTest.kt new file mode 100644 index 000000000..13f16cb34 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPathTest.kt @@ -0,0 +1,26 @@ +package com.onesignal.core.internal.backend.impl + +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.shouldBe + +class TurbineSdkFeatureFlagsPathTest : FunSpec({ + test("percentEncodePathSegmentUtf8 leaves unreserved characters unchanged") { + TurbineSdkFeatureFlagsPath.percentEncodePathSegmentUtf8("android") shouldBe "android" + TurbineSdkFeatureFlagsPath.percentEncodePathSegmentUtf8("050801-beta") shouldBe "050801-beta" + TurbineSdkFeatureFlagsPath.percentEncodePathSegmentUtf8("aZ09-._~") shouldBe "aZ09-._~" + } + + test("percentEncodePathSegmentUtf8 encodes reserved and space as percent-hex") { + TurbineSdkFeatureFlagsPath.percentEncodePathSegmentUtf8("a/b") shouldBe "a%2Fb" + TurbineSdkFeatureFlagsPath.percentEncodePathSegmentUtf8("a b") shouldBe "a%20b" + TurbineSdkFeatureFlagsPath.percentEncodePathSegmentUtf8("café") shouldBe "caf%C3%A9" + } + + test("buildGetPath matches expected Turbine relative path") { + TurbineSdkFeatureFlagsPath.buildGetPath( + appId = "14719551-23f1-4d20-8dab-81496ffca5ea", + platform = "android", + sdkVersion = "050801-beta", + ) shouldBe "apps/14719551-23f1-4d20-8dab-81496ffca5ea/sdk/features/android/050801-beta" + } +}) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt index 1b30bfe23..af9e401f6 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt @@ -137,7 +137,19 @@ class FeatureManagerTests : FunSpec({ val manager = FeatureManager(configModelStore) - val meta = manager.remoteFeatureFlagMetadata() - meta.getValue("X")["note"]!!.jsonPrimitive.content shouldBe "y" + val meta = requireNotNull(manager.remoteFeatureFlagMetadata()) + requireNotNull(meta.getValue("X")["note"]).jsonPrimitive.content shouldBe "y" + } + + test("remoteFeatureFlagMetadata is null when config has no stored metadata") { + val initialModel = mockk() + stubConfigModel(initialModel) + every { initialModel.features } returns emptyList() + every { initialModel.sdkRemoteFeatureFlagMetadata } returns null + val configModelStore = mockk() + every { configModelStore.model } returns initialModel + every { configModelStore.subscribe(any()) } just runs + + FeatureManager(configModelStore).remoteFeatureFlagMetadata() shouldBe null } }) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt index 391aef73e..d8333ac8e 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt @@ -27,7 +27,7 @@ class StartupServiceTests : FunSpec({ ): ServiceProvider { val featureManager = mockk() every { featureManager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING) } returns backgroundThreadingEnabled - every { featureManager.remoteFeatureFlagMetadata() } returns emptyMap() + every { featureManager.remoteFeatureFlagMetadata() } returns null val serviceBuilder = ServiceBuilder() serviceBuilder.register(featureManager).provides() diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapperTest.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapperTest.kt index b5004ba6e..1f30da699 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapperTest.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapperTest.kt @@ -42,7 +42,7 @@ class OneSignalCrashUploaderWrapperTest : FunSpec({ fun mockFeatureManager(backgroundThreadingEnabled: Boolean = true): IFeatureManager { val featureManager = mockk() every { featureManager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING) } returns backgroundThreadingEnabled - every { featureManager.remoteFeatureFlagMetadata() } returns emptyMap() + every { featureManager.remoteFeatureFlagMetadata() } returns null return featureManager } From 3720ec5840d972de2a73ab492bddb78914a5b4f1 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Tue, 14 Apr 2026 13:11:39 -0400 Subject: [PATCH 04/23] fix(core): address PR 2612 review for FeatureFlagsRefreshService - Annotate pollJob with @Volatile for cross-thread visibility - Remove duplicate onFocus from start(); ApplicationService already fires on subscribe - Apply remote flags via in-place model property updates (REMOTE_FEATURE_FLAGS tag) to avoid clobbering concurrent params hydration with a stale full-model replace - Use OneSignalDispatchers.launchOnIO so CancellationException is not logged as an error - Compare enabled key sets (order-insensitive) before writing Made-with: Cursor --- .../config/impl/FeatureFlagsRefreshService.kt | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt index 6e7ce3a8b..9520a42fa 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt @@ -4,7 +4,7 @@ import com.onesignal.common.modeling.ISingletonModelStoreChangeHandler import com.onesignal.common.modeling.ModelChangeTags import com.onesignal.common.modeling.ModelChangedArgs import com.onesignal.core.internal.config.ConfigModelChangeTags -import com.onesignal.common.threading.launchOnIO +import com.onesignal.common.threading.OneSignalDispatchers import com.onesignal.core.internal.application.IApplicationLifecycleHandler import com.onesignal.core.internal.application.IApplicationService import com.onesignal.core.internal.backend.IFeatureFlagsBackendService @@ -14,6 +14,7 @@ import com.onesignal.core.internal.config.ConfigModelStore import com.onesignal.core.internal.startup.IStartableService import com.onesignal.debug.internal.logging.Logging import kotlin.coroutines.coroutineContext +import kotlin.jvm.Volatile import kotlinx.coroutines.Job import kotlinx.coroutines.delay import kotlinx.coroutines.isActive @@ -23,6 +24,9 @@ import kotlinx.coroutines.isActive * every [REFRESH_INTERVAL_MS] while the session stays in the foreground. Updates * [ConfigModel.sdkRemoteFeatureFlags] / [ConfigModel.sdkRemoteFeatureFlagMetadata] so * [com.onesignal.core.internal.features.FeatureManager] stays in sync. + * + * Remote fields are updated in place on the live [ConfigModel] (with [ConfigModelChangeTags.REMOTE_FEATURE_FLAGS]) + * so concurrent hydration cannot be overwritten by a stale full-model snapshot. */ internal class FeatureFlagsRefreshService( private val applicationService: IApplicationService, @@ -31,14 +35,13 @@ internal class FeatureFlagsRefreshService( ) : IStartableService, IApplicationLifecycleHandler, ISingletonModelStoreChangeHandler { + @Volatile private var pollJob: Job? = null override fun start() { applicationService.addApplicationLifecycleHandler(this) configModelStore.subscribe(this) - if (applicationService.isInForeground) { - onFocus(firedOnSubscribe = true) - } + // Foreground-at-subscribe is handled by [IApplicationService.addApplicationLifecycleHandler] (fires onFocus). } override fun onFocus(firedOnSubscribe: Boolean) { @@ -80,7 +83,7 @@ internal class FeatureFlagsRefreshService( private fun restartForegroundPolling() { pollJob?.cancel() pollJob = - launchOnIO { + OneSignalDispatchers.launchOnIO { while (coroutineContext.isActive) { if (!applicationService.isInForeground) { break @@ -102,15 +105,15 @@ internal class FeatureFlagsRefreshService( val result = featureFlagsBackend.fetchRemoteFeatureFlags(appId) val current = configModelStore.model val newMetaString = FeatureFlagsJsonParser.encodeMetadata(result.metadata) - if (result.enabledKeys == current.sdkRemoteFeatureFlags && newMetaString == current.sdkRemoteFeatureFlagMetadata) { + if (result.enabledKeys.toSet() == current.sdkRemoteFeatureFlags.toSet() && + newMetaString == current.sdkRemoteFeatureFlagMetadata + ) { return } - val updated = ConfigModel() - updated.initializeFromModel(null, current) - updated.sdkRemoteFeatureFlags = result.enabledKeys - updated.sdkRemoteFeatureFlagMetadata = newMetaString - configModelStore.replace(updated, ConfigModelChangeTags.REMOTE_FEATURE_FLAGS) + val tag = ConfigModelChangeTags.REMOTE_FEATURE_FLAGS + current.setListProperty(ConfigModel::sdkRemoteFeatureFlags.name, result.enabledKeys, tag) + current.setOptStringProperty(ConfigModel::sdkRemoteFeatureFlagMetadata.name, newMetaString, tag) } companion object { From 64a106837b142057fc2a0238c5335ae93e5c6fbc Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Mon, 20 Apr 2026 14:29:23 -0400 Subject: [PATCH 05/23] =?UTF-8?q?fix(core):=20PR=202612=20follow-ups=20?= =?UTF-8?q?=E2=80=94=20fetch=20outcome,=20cancellation,=20docs,=20key=20ca?= =?UTF-8?q?sing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - RemoteFeatureFlagsFetchOutcome Success/Unavailable; parseSuccessful distinguishes valid Turbine JSON (including empty features) from parse/contract failure; no cache wipe on Unavailable - Polling loop: rethrow CancellationException; catch Exception only for fetch errors - FeatureManager: drop dead REMOTE_FEATURE_FLAGS onModelReplaced branch; canonicalize flag keys - ConfigModelChangeTags + FeatureFlag KDoc/comment fixes - Tests: parseSuccessful, case-insensitive remote key Made-with: Cursor --- .../backend/IFeatureFlagsBackendService.kt | 14 ++++++++++++- .../impl/FeatureFlagsBackendService.kt | 16 +++++++++----- .../backend/impl/FeatureFlagsJsonParser.kt | 21 ++++++++++++------- .../internal/config/ConfigModelChangeTags.kt | 2 +- .../config/impl/FeatureFlagsRefreshService.kt | 17 +++++++++------ .../core/internal/features/FeatureFlag.kt | 2 +- .../core/internal/features/FeatureManager.kt | 19 ++++++++++------- .../impl/FeatureFlagsJsonParserTests.kt | 12 +++++++++++ .../internal/features/FeatureManagerTests.kt | 15 +++++++++++++ 9 files changed, 89 insertions(+), 29 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IFeatureFlagsBackendService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IFeatureFlagsBackendService.kt index fcad601a4..cc71fae45 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IFeatureFlagsBackendService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IFeatureFlagsBackendService.kt @@ -18,11 +18,23 @@ internal data class RemoteFeatureFlagsResult( } } +/** + * Outcome of [IFeatureFlagsBackendService.fetchRemoteFeatureFlags]. + * [Unavailable] means the client did not get a trustworthy response (HTTP error, invalid body, etc.); + * callers should keep previously cached flags. [Success] includes a valid HTTP parse, including an + * empty `features` array from the server. + */ +internal sealed class RemoteFeatureFlagsFetchOutcome { + data class Success(val result: RemoteFeatureFlagsResult) : RemoteFeatureFlagsFetchOutcome() + + data object Unavailable : RemoteFeatureFlagsFetchOutcome() +} + /** * Fetches remote feature flags for the current app via **GET** * `apps/{app_id}/sdk/features/{platform}/{sdk_version}` (Turbine; see * [com.onesignal.core.internal.backend.impl.FeatureFlagsBackendService]). */ internal interface IFeatureFlagsBackendService { - suspend fun fetchRemoteFeatureFlags(appId: String): RemoteFeatureFlagsResult + suspend fun fetchRemoteFeatureFlags(appId: String): RemoteFeatureFlagsFetchOutcome } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt index b6a6885ce..66a6c3b0e 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt @@ -2,7 +2,7 @@ package com.onesignal.core.internal.backend.impl import com.onesignal.common.OneSignalUtils import com.onesignal.core.internal.backend.IFeatureFlagsBackendService -import com.onesignal.core.internal.backend.RemoteFeatureFlagsResult +import com.onesignal.core.internal.backend.RemoteFeatureFlagsFetchOutcome import com.onesignal.core.internal.http.IHttpClient import com.onesignal.debug.LogLevel import com.onesignal.debug.internal.logging.Logging @@ -25,7 +25,7 @@ import com.onesignal.debug.internal.logging.Logging internal class FeatureFlagsBackendService( private val http: IHttpClient, ) : IFeatureFlagsBackendService { - override suspend fun fetchRemoteFeatureFlags(appId: String): RemoteFeatureFlagsResult { + override suspend fun fetchRemoteFeatureFlags(appId: String): RemoteFeatureFlagsFetchOutcome { Logging.log(LogLevel.DEBUG, "FeatureFlagsBackendService.fetchRemoteFeatureFlags(appId=$appId)") val sdkVersion = OneSignalUtils.sdkVersion @@ -34,7 +34,7 @@ internal class FeatureFlagsBackendService( "FeatureFlagsBackendService: sdk version not usable for Turbine path (expected " + "6-digit label optional -suffix, e.g. 050801 or 050801-beta): '$sdkVersion'", ) - return RemoteFeatureFlagsResult.EMPTY + return RemoteFeatureFlagsFetchOutcome.Unavailable } val path = @@ -50,10 +50,16 @@ internal class FeatureFlagsBackendService( Logging.debug( "FeatureFlagsBackendService: non-success or empty body, status=${response.statusCode}", ) - return RemoteFeatureFlagsResult.EMPTY + return RemoteFeatureFlagsFetchOutcome.Unavailable } - return FeatureFlagsJsonParser.parse(body) + val parsed = FeatureFlagsJsonParser.parseSuccessful(body) + return if (parsed != null) { + RemoteFeatureFlagsFetchOutcome.Success(parsed) + } else { + Logging.debug("FeatureFlagsBackendService: response body is not valid Turbine feature-flags JSON") + RemoteFeatureFlagsFetchOutcome.Unavailable + } } companion object { diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt index 92a43125a..b1f754a83 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt @@ -50,18 +50,25 @@ internal object FeatureFlagsJsonParser { private const val FEATURES_PROPERTY = "features" - fun parse(payload: String): RemoteFeatureFlagsResult { + fun parse(payload: String): RemoteFeatureFlagsResult = parseSuccessful(payload) ?: RemoteFeatureFlagsResult.EMPTY + + /** + * Parses a successful Turbine response body. Returns `null` if JSON is invalid or the payload does not + * match the expected contract (missing `features` array, wrong types). A valid `{"features":[]}` returns + * [RemoteFeatureFlagsResult] with an empty [RemoteFeatureFlagsResult.enabledKeys] list. + */ + fun parseSuccessful(payload: String): RemoteFeatureFlagsResult? { return try { - val root = format.parseToJsonElement(payload) as? JsonObject ?: return RemoteFeatureFlagsResult.EMPTY - parseRoot(root) + val root = format.parseToJsonElement(payload) as? JsonObject ?: return null + parseRootStrict(root) } catch (_: Throwable) { - RemoteFeatureFlagsResult.EMPTY + null } } - private fun parseRoot(root: JsonObject): RemoteFeatureFlagsResult { - val featuresEl = root[FEATURES_PROPERTY] ?: return RemoteFeatureFlagsResult.EMPTY - val featuresArray = featuresEl as? JsonArray ?: return RemoteFeatureFlagsResult.EMPTY + private fun parseRootStrict(root: JsonObject): RemoteFeatureFlagsResult? { + val featuresEl = root[FEATURES_PROPERTY] ?: return null + val featuresArray = featuresEl as? JsonArray ?: return null val flagEntries = featuresArray.mapNotNull { el -> (el as? JsonPrimitive) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt index 1c634c28d..6811c0332 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt @@ -7,7 +7,7 @@ package com.onesignal.core.internal.config internal object ConfigModelChangeTags { /** * Remote feature-flags HTTP endpoint updated [ConfigModel] SDK flag fields only - * ([sdkRemoteFeatureFlags], [sdkRemoteFeatureFlagMetadata]). + * ([ConfigModel.sdkRemoteFeatureFlags], [ConfigModel.sdkRemoteFeatureFlagMetadata]). */ const val REMOTE_FEATURE_FLAGS = "REMOTE_FEATURE_FLAGS" } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt index 9520a42fa..9a2942f04 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt @@ -8,6 +8,7 @@ import com.onesignal.common.threading.OneSignalDispatchers import com.onesignal.core.internal.application.IApplicationLifecycleHandler import com.onesignal.core.internal.application.IApplicationService import com.onesignal.core.internal.backend.IFeatureFlagsBackendService +import com.onesignal.core.internal.backend.RemoteFeatureFlagsFetchOutcome import com.onesignal.core.internal.backend.impl.FeatureFlagsJsonParser import com.onesignal.core.internal.config.ConfigModel import com.onesignal.core.internal.config.ConfigModelStore @@ -15,6 +16,7 @@ import com.onesignal.core.internal.startup.IStartableService import com.onesignal.debug.internal.logging.Logging import kotlin.coroutines.coroutineContext import kotlin.jvm.Volatile +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Job import kotlinx.coroutines.delay import kotlinx.coroutines.isActive @@ -69,9 +71,6 @@ internal class FeatureFlagsRefreshService( model: ConfigModel, tag: String, ) { - if (tag == ConfigModelChangeTags.REMOTE_FEATURE_FLAGS) { - return - } if (tag != ModelChangeTags.HYDRATE && tag != ModelChangeTags.NORMAL) { return } @@ -92,8 +91,10 @@ internal class FeatureFlagsRefreshService( if (appId.isNotEmpty()) { try { fetchAndApply(appId) - } catch (t: Throwable) { - Logging.warn("FeatureFlagsRefreshService: fetch failed", t) + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + Logging.warn("FeatureFlagsRefreshService: fetch failed", e) } } delay(REFRESH_INTERVAL_MS) @@ -102,7 +103,11 @@ internal class FeatureFlagsRefreshService( } private suspend fun fetchAndApply(appId: String) { - val result = featureFlagsBackend.fetchRemoteFeatureFlags(appId) + val result = + when (val outcome = featureFlagsBackend.fetchRemoteFeatureFlags(appId)) { + RemoteFeatureFlagsFetchOutcome.Unavailable -> return + is RemoteFeatureFlagsFetchOutcome.Success -> outcome.result + } val current = configModelStore.model val newMetaString = FeatureFlagsJsonParser.encodeMetadata(result.metadata) if (result.enabledKeys.toSet() == current.sdkRemoteFeatureFlags.toSet() && diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureFlag.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureFlag.kt index 784589dc2..324421a71 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureFlag.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureFlag.kt @@ -26,7 +26,7 @@ internal enum class FeatureFlag( ) { // Threading mode is selected once per app startup to avoid mixed-mode behavior mid-session. // - // Remote key (lowercase) must match backend / ConfigCat flag id. + // Remote key (lowercase) must match backend / Turbine flag id. // SDK_BACKGROUND_THREADING( "sdk_background_threading", diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt index 2c9646605..6e2dae7f1 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt @@ -6,7 +6,6 @@ import com.onesignal.common.modeling.ModelChangedArgs import com.onesignal.common.threading.ThreadingMode import com.onesignal.core.internal.backend.impl.FeatureFlagsJsonParser import com.onesignal.core.internal.config.ConfigModel -import com.onesignal.core.internal.config.ConfigModelChangeTags import com.onesignal.core.internal.config.ConfigModelStore import com.onesignal.debug.internal.logging.Logging import kotlinx.serialization.json.JsonObject @@ -56,10 +55,7 @@ internal class FeatureManager( tag: String, ) { Logging.debug("OneSignal: FeatureManager.onModelReplaced(tag=$tag)") - if (tag == ModelChangeTags.HYDRATE || - tag == ModelChangeTags.NORMAL || - tag == ConfigModelChangeTags.REMOTE_FEATURE_FLAGS - ) { + if (tag == ModelChangeTags.HYDRATE || tag == ModelChangeTags.NORMAL) { try { refreshEnabledFeatures(model, applyNextRunOnlyFeatures = false) } catch (t: Throwable) { @@ -93,9 +89,9 @@ internal class FeatureManager( ) { val enabledFeatureKeys = ( - model.features + - model.sdkRemoteFeatureFlags + - localFeatureOverrides + model.features.map { canonicalizeFeatureKey(it) } + + model.sdkRemoteFeatureFlags.map { canonicalizeFeatureKey(it) } + + localFeatureOverrides.map { canonicalizeFeatureKey(it) } ).toSet() if (localFeatureOverrides.isNotEmpty()) { Logging.warn( @@ -133,6 +129,13 @@ internal class FeatureManager( featureStates = nextStates } + private fun canonicalizeFeatureKey(key: String): String = + buildString(key.length) { + for (c in key) { + append(c.lowercaseChar()) + } + } + private fun applySideEffects( feature: FeatureFlag, enabled: Boolean, diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt index f882af324..286ff2339 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt @@ -74,6 +74,18 @@ class FeatureFlagsJsonParserTests : FunSpec({ FeatureFlagsJsonParser.parse("{") shouldBe RemoteFeatureFlagsResult.EMPTY } + test("parseSuccessful returns empty enabled keys for valid empty features array") { + val r = requireNotNull(FeatureFlagsJsonParser.parseSuccessful("""{"features":[]}""")) + r.enabledKeys shouldBe emptyList() + r.metadata shouldBe null + } + + test("parseSuccessful returns null for invalid json or contract") { + FeatureFlagsJsonParser.parseSuccessful("{") shouldBe null + FeatureFlagsJsonParser.parseSuccessful("""{"feature_a":{}}""") shouldBe null + FeatureFlagsJsonParser.parseSuccessful("""{"features":"bad"}""") shouldBe null + } + test("encodeMetadata round-trips for storage string") { val payload = """ diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt index af9e401f6..fc3aa6949 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt @@ -54,6 +54,21 @@ class FeatureManagerTests : FunSpec({ ThreadingMode.useBackgroundThreading shouldBe true } + test("initial state enables BACKGROUND_THREADING when remote key differs only by letter case") { + val initialModel = mockk() + stubConfigModel(initialModel) + every { initialModel.features } returns emptyList() + every { initialModel.sdkRemoteFeatureFlags } returns listOf("SDK_Background_Threading") + val configModelStore = mockk() + every { configModelStore.model } returns initialModel + every { configModelStore.subscribe(any()) } just runs + + val manager = FeatureManager(configModelStore) + + manager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING) shouldBe true + ThreadingMode.useBackgroundThreading shouldBe true + } + test("onModelReplaced should not switch threading mode after startup") { // Given val initialModel = mockk() From c7a27201e3dec5bc2c8cd23f96df763d05718276 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Tue, 21 Apr 2026 13:44:31 -0400 Subject: [PATCH 06/23] more logging --- .../impl/FeatureFlagsBackendService.kt | 35 ++++- .../config/impl/FeatureFlagsRefreshService.kt | 17 +- .../core/internal/http/HttpResponse.kt | 7 + .../impl/FeatureFlagsBackendServiceTests.kt | 145 ++++++++++++++++++ .../impl/FeatureFlagsJsonParserTests.kt | 5 + examples/demo/build.gradle.kts | 2 + 6 files changed, 203 insertions(+), 8 deletions(-) create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTests.kt diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt index 66a6c3b0e..f8231ce71 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt @@ -47,9 +47,12 @@ internal class FeatureFlagsBackendService( val response = http.get(path, null) val body = response.payload if (!response.isSuccess || body.isNullOrBlank()) { - Logging.debug( - "FeatureFlagsBackendService: non-success or empty body, status=${response.statusCode}", - ) + val msg = + "FeatureFlagsBackendService: non-success status=${response.statusCode} body=${bodySnippet(body)}" + // 4xx is likely a permanent misconfiguration (e.g. 403 Forbidden when the app is not + // enrolled for Turbine feature flags) and worth surfacing at WARN; other failures are + // typically transient (network blip, 5xx) and stay at DEBUG to avoid log spam. + if (response.isClientError) Logging.warn(msg) else Logging.debug(msg) return RemoteFeatureFlagsFetchOutcome.Unavailable } @@ -57,17 +60,41 @@ internal class FeatureFlagsBackendService( return if (parsed != null) { RemoteFeatureFlagsFetchOutcome.Success(parsed) } else { - Logging.debug("FeatureFlagsBackendService: response body is not valid Turbine feature-flags JSON") + Logging.warn( + "FeatureFlagsBackendService: response body is not valid Turbine feature-flags JSON: " + + bodySnippet(body), + ) RemoteFeatureFlagsFetchOutcome.Unavailable } } + /** + * Trim [body] to a short, single-line snippet safe for logcat. Caps at + * [LOG_BODY_SNIPPET_MAX_CHARS] so we never dump large payloads into logs. + */ + private fun bodySnippet(body: String?): String { + if (body.isNullOrEmpty()) return "" + val flattened = body.replace('\n', ' ').replace('\r', ' ') + return if (flattened.length <= LOG_BODY_SNIPPET_MAX_CHARS) { + flattened + } else { + flattened.take(LOG_BODY_SNIPPET_MAX_CHARS) + "…" + } + } + companion object { /** * Turbine `:platform` segment for the OneSignal Android SDK (this client). */ const val TURBINE_FEATURES_PLATFORM_ANDROID = "android" + /** + * Max chars of an HTTP response body included in diagnostic logs. Turbine error bodies + * (e.g. `{"errors":["Forbidden"]}`) are tiny, so 200 chars is plenty and bounds worst-case + * log size if an unexpected payload is returned. + */ + private const val LOG_BODY_SNIPPET_MAX_CHARS = 200 + /** * Returns true when [label] is safe to send as the Turbine `:sdk_version` path segment. * @see TurbineSdkFeatureFlagsPath.isValidFeaturesSdkVersionLabel diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt index 9a2942f04..41baa3d0e 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt @@ -97,6 +97,9 @@ internal class FeatureFlagsRefreshService( Logging.warn("FeatureFlagsRefreshService: fetch failed", e) } } + // TODO(revert-before-merge): makes the compiled poll interval obvious in logcat; remove + // for production. If you see 600_000ms here, the app is not using your local AAR. + Logging.debug("FeatureFlagsRefreshService: next fetch in ${REFRESH_INTERVAL_MS}ms") delay(REFRESH_INTERVAL_MS) } } @@ -110,9 +113,14 @@ internal class FeatureFlagsRefreshService( } val current = configModelStore.model val newMetaString = FeatureFlagsJsonParser.encodeMetadata(result.metadata) - if (result.enabledKeys.toSet() == current.sdkRemoteFeatureFlags.toSet() && - newMetaString == current.sdkRemoteFeatureFlagMetadata - ) { + val beforeKeys = current.sdkRemoteFeatureFlags.toSet() + val afterKeys = result.enabledKeys.toSet() + // TODO(revert-before-merge): verbose diff line for manual cold-start + mid-session flip testing. + Logging.debug( + "FeatureFlagsRefreshService: appId=$appId before=${beforeKeys.sorted()} " + + "after=${afterKeys.sorted()} changed=${beforeKeys != afterKeys}", + ) + if (afterKeys == beforeKeys && newMetaString == current.sdkRemoteFeatureFlagMetadata) { return } @@ -122,6 +130,7 @@ internal class FeatureFlagsRefreshService( } companion object { - private const val REFRESH_INTERVAL_MS = 600_000L + // Foreground polling cadence for remote feature flags (8 minutes). + private const val REFRESH_INTERVAL_MS = 480_000L } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/http/HttpResponse.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/http/HttpResponse.kt index fb62ccc24..64902b337 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/http/HttpResponse.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/http/HttpResponse.kt @@ -34,4 +34,11 @@ class HttpResponse( */ val isSuccess: Boolean get() = statusCode == HttpURLConnection.HTTP_OK || statusCode == HttpURLConnection.HTTP_ACCEPTED || statusCode == HttpURLConnection.HTTP_NOT_MODIFIED || statusCode == HttpURLConnection.HTTP_CREATED + + /** + * Whether the response is a client error (HTTP 4xx). Useful for distinguishing "probably + * permanent misconfiguration" from transient failures (5xx, network errors with statusCode=0). + */ + val isClientError: Boolean + get() = statusCode in HttpURLConnection.HTTP_BAD_REQUEST until HttpURLConnection.HTTP_INTERNAL_ERROR } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTests.kt new file mode 100644 index 000000000..26df536f6 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTests.kt @@ -0,0 +1,145 @@ +package com.onesignal.core.internal.backend.impl + +import com.onesignal.core.internal.backend.RemoteFeatureFlagsFetchOutcome +import com.onesignal.core.internal.http.HttpResponse +import com.onesignal.core.internal.http.IHttpClient +import com.onesignal.debug.ILogListener +import com.onesignal.debug.LogLevel +import com.onesignal.debug.OneSignalLogEvent +import com.onesignal.debug.internal.logging.Logging +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.collections.shouldHaveSize +import io.kotest.matchers.shouldBe +import io.mockk.coEvery +import io.mockk.mockk + +/** + * Verifies that [FeatureFlagsBackendService] surfaces enough diagnostic information for a + * developer to understand why a remote feature-flags fetch returned + * [RemoteFeatureFlagsFetchOutcome.Unavailable]. In particular: + * - 4xx failures (e.g. 403 Forbidden for apps not enrolled in Turbine) should log at [LogLevel.WARN] + * and include a body snippet so the Turbine error payload shows up in logcat by default. + * - 5xx / transient failures should stay at [LogLevel.DEBUG] to avoid noisy logs for flaky networks. + * - A 200 with an unexpected body shape should log at [LogLevel.WARN] with a body snippet since it + * indicates a real contract/config issue that would otherwise be silent. + */ +class FeatureFlagsBackendServiceTests : FunSpec({ + lateinit var capturedLogs: MutableList + lateinit var listener: ILogListener + var originalLogLevel: LogLevel = LogLevel.WARN + + beforeEach { + originalLogLevel = Logging.logLevel + Logging.logLevel = LogLevel.NONE + capturedLogs = mutableListOf() + listener = ILogListener { event -> capturedLogs.add(event) } + Logging.addListener(listener) + } + + afterEach { + Logging.removeListener(listener) + Logging.logLevel = originalLogLevel + } + + fun logsForService(): List = + capturedLogs.filter { it.entry.contains("FeatureFlagsBackendService:") } + + test("403 Forbidden logs at WARN with body snippet and returns Unavailable") { + val http = mockk() + coEvery { http.get(any(), any()) } returns HttpResponse(403, """{"errors":["Forbidden"]}""") + + val outcome = FeatureFlagsBackendService(http).fetchRemoteFeatureFlags("appId") + + outcome shouldBe RemoteFeatureFlagsFetchOutcome.Unavailable + val warns = logsForService().filter { it.level == LogLevel.WARN } + warns shouldHaveSize 1 + warns[0].entry.contains("status=403") shouldBe true + warns[0].entry.contains("""{"errors":["Forbidden"]}""") shouldBe true + } + + test("404 Not Found also logs at WARN with body snippet") { + val http = mockk() + coEvery { http.get(any(), any()) } returns HttpResponse(404, """{"errors":["Not Found"]}""") + + FeatureFlagsBackendService(http).fetchRemoteFeatureFlags("appId") shouldBe + RemoteFeatureFlagsFetchOutcome.Unavailable + val warns = logsForService().filter { it.level == LogLevel.WARN } + warns shouldHaveSize 1 + warns[0].entry.contains("status=404") shouldBe true + } + + test("500 server error stays at DEBUG to avoid noisy logs for transient failures") { + val http = mockk() + coEvery { http.get(any(), any()) } returns HttpResponse(500, "boom") + + FeatureFlagsBackendService(http).fetchRemoteFeatureFlags("appId") shouldBe + RemoteFeatureFlagsFetchOutcome.Unavailable + logsForService().any { it.level == LogLevel.WARN } shouldBe false + val debugs = logsForService().filter { it.level == LogLevel.DEBUG } + debugs.any { it.entry.contains("status=500") && it.entry.contains("body=boom") } shouldBe true + } + + test("network error (statusCode=0) stays at DEBUG") { + val http = mockk() + coEvery { http.get(any(), any()) } returns HttpResponse(0, null) + + FeatureFlagsBackendService(http).fetchRemoteFeatureFlags("appId") shouldBe + RemoteFeatureFlagsFetchOutcome.Unavailable + logsForService().any { it.level == LogLevel.WARN } shouldBe false + val debugs = logsForService().filter { it.level == LogLevel.DEBUG } + debugs.any { it.entry.contains("status=0") && it.entry.contains("body=") } shouldBe true + } + + test("200 with non-contract JSON body logs at WARN with body snippet") { + val http = mockk() + coEvery { http.get(any(), any()) } returns HttpResponse(200, """{"errors":["Forbidden"]}""") + + FeatureFlagsBackendService(http).fetchRemoteFeatureFlags("appId") shouldBe + RemoteFeatureFlagsFetchOutcome.Unavailable + val warns = + logsForService().filter { + it.level == LogLevel.WARN && it.entry.contains("not valid Turbine feature-flags JSON") + } + warns shouldHaveSize 1 + warns[0].entry.contains("""{"errors":["Forbidden"]}""") shouldBe true + } + + test("body snippet caps long payloads at 200 chars") { + val longBody = "x".repeat(1_000) + val http = mockk() + coEvery { http.get(any(), any()) } returns HttpResponse(403, longBody) + + FeatureFlagsBackendService(http).fetchRemoteFeatureFlags("appId") + val warn = logsForService().first { it.level == LogLevel.WARN } + // Snippet is truncated to 200 chars plus an ellipsis marker. + warn.entry.contains("x".repeat(200) + "…") shouldBe true + warn.entry.contains("x".repeat(201)) shouldBe false + } + + test("newlines in body are flattened so log entry stays single-line") { + val http = mockk() + coEvery { http.get(any(), any()) } returns HttpResponse(403, "line1\nline2\rline3") + + FeatureFlagsBackendService(http).fetchRemoteFeatureFlags("appId") + val warn = logsForService().first { it.level == LogLevel.WARN } + warn.entry.contains("body=line1 line2 line3") shouldBe true + warn.entry.contains("\n") shouldBe false + warn.entry.contains("\r") shouldBe false + } + + test("200 with valid empty features array is Success and does not log at WARN") { + val http = mockk() + coEvery { http.get(any(), any()) } returns HttpResponse(200, """{"features":[]}""") + + val outcome = FeatureFlagsBackendService(http).fetchRemoteFeatureFlags("appId") + outcome.shouldBeSuccessWithEmptyKeys() + logsForService().any { it.level == LogLevel.WARN } shouldBe false + } +}) + +private fun RemoteFeatureFlagsFetchOutcome.shouldBeSuccessWithEmptyKeys() { + check(this is RemoteFeatureFlagsFetchOutcome.Success) { + "expected Success, got $this" + } + this.result.enabledKeys shouldBe emptyList() +} diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt index 286ff2339..ba84d9e2d 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt @@ -86,6 +86,11 @@ class FeatureFlagsJsonParserTests : FunSpec({ FeatureFlagsJsonParser.parseSuccessful("""{"features":"bad"}""") shouldBe null } + test("parseSuccessful returns null for Turbine error body (e.g. 403 Forbidden)") { + FeatureFlagsJsonParser.parseSuccessful("""{"errors":["Forbidden"]}""") shouldBe null + FeatureFlagsJsonParser.parseSuccessful("""{"errors":["Not Found"]}""") shouldBe null + } + test("encodeMetadata round-trips for storage string") { val payload = """ diff --git a/examples/demo/build.gradle.kts b/examples/demo/build.gradle.kts index 159ff201c..3e21571e6 100644 --- a/examples/demo/build.gradle.kts +++ b/examples/demo/build.gradle.kts @@ -19,6 +19,8 @@ buildscript { allprojects { repositories { + // TODO(revert-before-merge): mavenLocal first so locally-published SDK builds + mavenLocal() google() mavenCentral() // Huawei maven From 8f47b9c3431354d4a61a1b9661aa9ddcd3501e01 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Tue, 21 Apr 2026 13:53:54 -0400 Subject: [PATCH 07/23] chore(core): drop revert-before-merge TODOs on kept diagnostics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep the "next fetch in …ms" and before/after diff debug logs in FeatureFlagsRefreshService, and keep mavenLocal() in the demo's allprojects repositories; just remove the TODO(revert-before-merge) markers now that these are staying. Made-with: Cursor --- .../core/internal/config/impl/FeatureFlagsRefreshService.kt | 3 --- examples/demo/build.gradle.kts | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt index 41baa3d0e..dfdbefcbc 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt @@ -97,8 +97,6 @@ internal class FeatureFlagsRefreshService( Logging.warn("FeatureFlagsRefreshService: fetch failed", e) } } - // TODO(revert-before-merge): makes the compiled poll interval obvious in logcat; remove - // for production. If you see 600_000ms here, the app is not using your local AAR. Logging.debug("FeatureFlagsRefreshService: next fetch in ${REFRESH_INTERVAL_MS}ms") delay(REFRESH_INTERVAL_MS) } @@ -115,7 +113,6 @@ internal class FeatureFlagsRefreshService( val newMetaString = FeatureFlagsJsonParser.encodeMetadata(result.metadata) val beforeKeys = current.sdkRemoteFeatureFlags.toSet() val afterKeys = result.enabledKeys.toSet() - // TODO(revert-before-merge): verbose diff line for manual cold-start + mid-session flip testing. Logging.debug( "FeatureFlagsRefreshService: appId=$appId before=${beforeKeys.sorted()} " + "after=${afterKeys.sorted()} changed=${beforeKeys != afterKeys}", diff --git a/examples/demo/build.gradle.kts b/examples/demo/build.gradle.kts index 3e21571e6..b0dc0f2a9 100644 --- a/examples/demo/build.gradle.kts +++ b/examples/demo/build.gradle.kts @@ -19,7 +19,7 @@ buildscript { allprojects { repositories { - // TODO(revert-before-merge): mavenLocal first so locally-published SDK builds + // mavenLocal first so locally-published SDK builds resolve over Central when testing against a snapshot. mavenLocal() google() mavenCentral() From ce19184b55570baa71d3270cd8e13223442675af Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Thu, 23 Apr 2026 12:15:35 -0400 Subject: [PATCH 08/23] Addressed comments --- .../impl/FeatureFlagsBackendService.kt | 8 ++- .../backend/impl/FeatureFlagsJsonParser.kt | 37 ++++++------- .../core/internal/config/ConfigModel.kt | 16 ++++-- .../internal/config/ConfigModelChangeTags.kt | 5 +- .../config/impl/FeatureFlagsRefreshService.kt | 53 +++++++++--------- .../impl/FeatureFlagsBackendServiceTest.kt | 50 ----------------- .../impl/FeatureFlagsBackendServiceTests.kt | 54 +++++++++++++++++++ examples/demo/build.gradle.kts | 2 - 8 files changed, 115 insertions(+), 110 deletions(-) delete mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTest.kt diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt index f8231ce71..0fbd26d5f 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt @@ -46,7 +46,7 @@ internal class FeatureFlagsBackendService( val response = http.get(path, null) val body = response.payload - if (!response.isSuccess || body.isNullOrBlank()) { + if (!response.isSuccess) { val msg = "FeatureFlagsBackendService: non-success status=${response.statusCode} body=${bodySnippet(body)}" // 4xx is likely a permanent misconfiguration (e.g. 403 Forbidden when the app is not @@ -55,6 +55,12 @@ internal class FeatureFlagsBackendService( if (response.isClientError) Logging.warn(msg) else Logging.debug(msg) return RemoteFeatureFlagsFetchOutcome.Unavailable } + if (body.isNullOrBlank()) { + Logging.warn( + "FeatureFlagsBackendService: empty body for success status=${response.statusCode}", + ) + return RemoteFeatureFlagsFetchOutcome.Unavailable + } val parsed = FeatureFlagsJsonParser.parseSuccessful(body) return if (parsed != null) { diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt index b1f754a83..560bd04a3 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt @@ -9,28 +9,21 @@ import kotlinx.serialization.json.JsonPrimitive import kotlinx.serialization.json.buildJsonObject /** - * Parses the feature-flags API response with kotlinx.serialization. + * **What this is:** strict JSON parsing for the Turbine SDK feature-flags response + * ([OneSignal/turbine#1681](https://github.com/OneSignal/turbine/pull/1681)). * - * Expected shape: - * ```json - * { - * "features": ["feature_a", "feature_b"], - * "feature_a": { "weight": 0.1 }, - * "feature_b": { ... } - * } - * ``` + * **Wire shape:** root object with a `features` array of **string** flag ids. Optional per-flag JSON + * objects may appear as **sibling root properties** (same name as the id); those are merged into + * [RemoteFeatureFlagsResult.metadata]. * - * Turbine returns only `features` ([OneSignal/turbine#1681](https://github.com/OneSignal/turbine/pull/1681)). - * If the same flag id also appears as a root property with a JSON object value, that object is copied - * into [RemoteFeatureFlagsResult.metadata] for forward-compatible extras (flags without a sibling - * object stay enabled via [RemoteFeatureFlagsResult.enabledKeys] only). + * **Optional metadata:** for a string entry `"foo"`, if the root also has a property `"foo"` (or + * case-insensitive match) whose value is a JSON object, that object is stored per-flag in + * [RemoteFeatureFlagsResult.metadata]. That lets a future API add per-flag config without a new + * top-level field; the SDK persists it in [com.onesignal.core.internal.config.ConfigModel.sdkRemoteFeatureFlagMetadata] + * so a later process or SDK version can read it without re-fetching. * - * Payload must be valid JSON (e.g. a comma between `"features": [...]` and the next property). - * - * Flag ids from the `features` array are normalized with [Char.lowercaseChar] (Unicode case mapping, no - * `java.util.Locale`) so they align with [com.onesignal.core.internal.features.FeatureFlag.key]. Sibling - * objects are resolved with exact key, then canonical key, then a case-insensitive match against other root - * properties (excluding `features`). + * **API surface:** [parseSuccessful] for HTTP 200 bodies; [parse] for lenient “best effort”; + * [encodeMetadata] / [parseStoredMetadataMap] for the persisted metadata column. * * Uses only Kotlin stdlib + kotlinx.serialization (Kotlin Multiplatform-friendly). */ @@ -53,9 +46,9 @@ internal object FeatureFlagsJsonParser { fun parse(payload: String): RemoteFeatureFlagsResult = parseSuccessful(payload) ?: RemoteFeatureFlagsResult.EMPTY /** - * Parses a successful Turbine response body. Returns `null` if JSON is invalid or the payload does not - * match the expected contract (missing `features` array, wrong types). A valid `{"features":[]}` returns - * [RemoteFeatureFlagsResult] with an empty [RemoteFeatureFlagsResult.enabledKeys] list. + * Parses a 200 response body. Returns `null` if the text is not JSON, not an object, or does not + * contain a `features` **array** of the expected element types. Returns an empty result for + * `{"features":[]}`. */ fun parseSuccessful(payload: String): RemoteFeatureFlagsResult? { return try { diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt index 004c6cba8..bfaf08eb8 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt @@ -291,8 +291,9 @@ class ConfigModel : Model() { } /** - * Remote feature switches controlled by backend. - * Presence of a feature name indicates enabled. + * Feature ids from the main OneSignal config / params response (legacy channel). + * Presence of a name means the feature is on. This list is distinct from [sdkRemoteFeatureFlags]; + * [com.onesignal.core.internal.features.FeatureManager] unions both (plus any local test overrides). */ var features: List get() = getListProperty(::features.name) { emptyList() } @@ -301,8 +302,10 @@ class ConfigModel : Model() { } /** - * Feature keys from the dedicated SDK feature-flags HTTP endpoint (see [com.onesignal.core.internal.backend.IFeatureFlagsBackendService]). - * Unioned with [features] for [com.onesignal.core.internal.features.FeatureManager]. + * Feature ids from the Turbine SDK feature-flags HTTP endpoint + * ([com.onesignal.core.internal.backend.IFeatureFlagsBackendService]), updated while the app is + * foreground. Unioned with [features] in [com.onesignal.core.internal.features.FeatureManager] + * when evaluating [com.onesignal.core.internal.features.FeatureFlag] entries. */ var sdkRemoteFeatureFlags: List get() = getListProperty(::sdkRemoteFeatureFlags.name) { emptyList() } @@ -311,7 +314,10 @@ class ConfigModel : Model() { } /** - * JSON object string: flag id → metadata (e.g. `note` or structured values) from the feature-flags endpoint. + * Compacted JSON object: canonical flag id → metadata object, from the same Turbine response + * as [sdkRemoteFeatureFlags]. Persisted so metadata survives process death and is available + * to [com.onesignal.core.internal.features.IFeatureManager.remoteFeatureFlagMetadata] without + * a new network fetch on the next launch. */ var sdkRemoteFeatureFlagMetadata: String? get() = getOptStringProperty(::sdkRemoteFeatureFlagMetadata.name) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt index 6811c0332..dbe741753 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt @@ -6,8 +6,9 @@ package com.onesignal.core.internal.config */ internal object ConfigModelChangeTags { /** - * Remote feature-flags HTTP endpoint updated [ConfigModel] SDK flag fields only - * ([ConfigModel.sdkRemoteFeatureFlags], [ConfigModel.sdkRemoteFeatureFlagMetadata]). + * A partial update from the Turbine feature-flags refresh: only + * [ConfigModel.sdkRemoteFeatureFlags] and [ConfigModel.sdkRemoteFeatureFlagMetadata] changed + * (in-place on the live model, not a full [ConfigModel] replace). */ const val REMOTE_FEATURE_FLAGS = "REMOTE_FEATURE_FLAGS" } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt index dfdbefcbc..7ff63499a 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt @@ -3,7 +3,6 @@ package com.onesignal.core.internal.config.impl import com.onesignal.common.modeling.ISingletonModelStoreChangeHandler import com.onesignal.common.modeling.ModelChangeTags import com.onesignal.common.modeling.ModelChangedArgs -import com.onesignal.core.internal.config.ConfigModelChangeTags import com.onesignal.common.threading.OneSignalDispatchers import com.onesignal.core.internal.application.IApplicationLifecycleHandler import com.onesignal.core.internal.application.IApplicationService @@ -11,15 +10,15 @@ import com.onesignal.core.internal.backend.IFeatureFlagsBackendService import com.onesignal.core.internal.backend.RemoteFeatureFlagsFetchOutcome import com.onesignal.core.internal.backend.impl.FeatureFlagsJsonParser import com.onesignal.core.internal.config.ConfigModel +import com.onesignal.core.internal.config.ConfigModelChangeTags import com.onesignal.core.internal.config.ConfigModelStore import com.onesignal.core.internal.startup.IStartableService import com.onesignal.debug.internal.logging.Logging -import kotlin.coroutines.coroutineContext -import kotlin.jvm.Volatile import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Job import kotlinx.coroutines.delay import kotlinx.coroutines.isActive +import kotlin.coroutines.coroutineContext /** * Fetches remote SDK feature flags when the app is in the foreground, immediately on focus and then @@ -37,7 +36,6 @@ internal class FeatureFlagsRefreshService( ) : IStartableService, IApplicationLifecycleHandler, ISingletonModelStoreChangeHandler { - @Volatile private var pollJob: Job? = null override fun start() { @@ -51,8 +49,10 @@ internal class FeatureFlagsRefreshService( } override fun onUnfocused() { - pollJob?.cancel() - pollJob = null + synchronized(this) { + pollJob?.cancel() + pollJob = null + } } override fun onModelUpdated( @@ -80,27 +80,28 @@ internal class FeatureFlagsRefreshService( } private fun restartForegroundPolling() { - pollJob?.cancel() - pollJob = - OneSignalDispatchers.launchOnIO { - while (coroutineContext.isActive) { - if (!applicationService.isInForeground) { - break - } - val appId = configModelStore.model.appId - if (appId.isNotEmpty()) { - try { - fetchAndApply(appId) - } catch (e: CancellationException) { - throw e - } catch (e: Exception) { - Logging.warn("FeatureFlagsRefreshService: fetch failed", e) + synchronized(this) { + pollJob?.cancel() + pollJob = + OneSignalDispatchers.launchOnIO { + while (coroutineContext.isActive) { + if (!applicationService.isInForeground) { + break + } + val appId = configModelStore.model.appId + if (appId.isNotEmpty()) { + try { + fetchAndApply(appId) + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + Logging.warn("FeatureFlagsRefreshService: fetch failed", e) + } } + delay(REFRESH_INTERVAL_MS) } - Logging.debug("FeatureFlagsRefreshService: next fetch in ${REFRESH_INTERVAL_MS}ms") - delay(REFRESH_INTERVAL_MS) } - } + } } private suspend fun fetchAndApply(appId: String) { @@ -113,10 +114,6 @@ internal class FeatureFlagsRefreshService( val newMetaString = FeatureFlagsJsonParser.encodeMetadata(result.metadata) val beforeKeys = current.sdkRemoteFeatureFlags.toSet() val afterKeys = result.enabledKeys.toSet() - Logging.debug( - "FeatureFlagsRefreshService: appId=$appId before=${beforeKeys.sorted()} " + - "after=${afterKeys.sorted()} changed=${beforeKeys != afterKeys}", - ) if (afterKeys == beforeKeys && newMetaString == current.sdkRemoteFeatureFlagMetadata) { return } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTest.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTest.kt deleted file mode 100644 index 705f6f16c..000000000 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTest.kt +++ /dev/null @@ -1,50 +0,0 @@ -package com.onesignal.core.internal.backend.impl - -import com.onesignal.common.OneSignalUtils -import io.kotest.core.spec.style.FunSpec -import io.kotest.matchers.shouldBe - -class FeatureFlagsBackendServiceTest : FunSpec({ - test("buildFeatureFlagsGetPath matches Turbine /apps/:app_id/sdk/features/:platform/:sdk_version") { - FeatureFlagsBackendService.buildFeatureFlagsGetPath( - appId = "14719551-23f1-4d20-8dab-81496ffca5ea", - platform = FeatureFlagsBackendService.TURBINE_FEATURES_PLATFORM_ANDROID, - sdkVersion = "050801", - ) shouldBe "apps/14719551-23f1-4d20-8dab-81496ffca5ea/sdk/features/android/050801" - } - - test("buildFeatureFlagsGetPath encodes prerelease sdk version label") { - FeatureFlagsBackendService.buildFeatureFlagsGetPath( - appId = "14719551-23f1-4d20-8dab-81496ffca5ea", - platform = FeatureFlagsBackendService.TURBINE_FEATURES_PLATFORM_ANDROID, - sdkVersion = "050801-beta", - ) shouldBe "apps/14719551-23f1-4d20-8dab-81496ffca5ea/sdk/features/android/050801-beta" - } - - test("isValidFeaturesSdkVersionLabel accepts only 6-digit labels with optional -suffix") { - val valid = - listOf( - "050801", - "050801-beta", - "050801-beta1", - "010203-rc.2", - "000000", - ) - val invalid = - listOf( - "5.8.1", - "05080", - "0508010", - "", - "050801-", - "050801/", - "v050801", - ) - valid.forEach { FeatureFlagsBackendService.isValidFeaturesSdkVersionLabel(it) shouldBe true } - invalid.forEach { FeatureFlagsBackendService.isValidFeaturesSdkVersionLabel(it) shouldBe false } - } - - test("OneSignalUtils.sdkVersion from BuildConfig matches Turbine label rules") { - FeatureFlagsBackendService.isValidFeaturesSdkVersionLabel(OneSignalUtils.sdkVersion) shouldBe true - } -}) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTests.kt index 26df536f6..c20b933b0 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTests.kt @@ -1,5 +1,6 @@ package com.onesignal.core.internal.backend.impl +import com.onesignal.common.OneSignalUtils import com.onesignal.core.internal.backend.RemoteFeatureFlagsFetchOutcome import com.onesignal.core.internal.http.HttpResponse import com.onesignal.core.internal.http.IHttpClient @@ -135,6 +136,59 @@ class FeatureFlagsBackendServiceTests : FunSpec({ outcome.shouldBeSuccessWithEmptyKeys() logsForService().any { it.level == LogLevel.WARN } shouldBe false } + + test("200 with empty body logs at WARN for contract anomaly and returns Unavailable") { + val http = mockk() + coEvery { http.get(any(), any()) } returns HttpResponse(200, null) + + val outcome = FeatureFlagsBackendService(http).fetchRemoteFeatureFlags("appId") + outcome shouldBe RemoteFeatureFlagsFetchOutcome.Unavailable + val warns = logsForService().filter { it.level == LogLevel.WARN } + warns.any { it.entry.contains("empty body for success status=200") } shouldBe true + } + + test("buildFeatureFlagsGetPath matches Turbine /apps/:app_id/sdk/features/:platform/:sdk_version") { + FeatureFlagsBackendService.buildFeatureFlagsGetPath( + appId = "14719551-23f1-4d20-8dab-81496ffca5ea", + platform = FeatureFlagsBackendService.TURBINE_FEATURES_PLATFORM_ANDROID, + sdkVersion = "050801", + ) shouldBe "apps/14719551-23f1-4d20-8dab-81496ffca5ea/sdk/features/android/050801" + } + + test("buildFeatureFlagsGetPath encodes prerelease sdk version label") { + FeatureFlagsBackendService.buildFeatureFlagsGetPath( + appId = "14719551-23f1-4d20-8dab-81496ffca5ea", + platform = FeatureFlagsBackendService.TURBINE_FEATURES_PLATFORM_ANDROID, + sdkVersion = "050801-beta", + ) shouldBe "apps/14719551-23f1-4d20-8dab-81496ffca5ea/sdk/features/android/050801-beta" + } + + test("isValidFeaturesSdkVersionLabel accepts only 6-digit labels with optional -suffix") { + val valid = + listOf( + "050801", + "050801-beta", + "050801-beta1", + "010203-rc.2", + "000000", + ) + val invalid = + listOf( + "5.8.1", + "05080", + "0508010", + "", + "050801-", + "050801/", + "v050801", + ) + valid.forEach { FeatureFlagsBackendService.isValidFeaturesSdkVersionLabel(it) shouldBe true } + invalid.forEach { FeatureFlagsBackendService.isValidFeaturesSdkVersionLabel(it) shouldBe false } + } + + test("OneSignalUtils.sdkVersion from BuildConfig matches Turbine label rules") { + FeatureFlagsBackendService.isValidFeaturesSdkVersionLabel(OneSignalUtils.sdkVersion) shouldBe true + } }) private fun RemoteFeatureFlagsFetchOutcome.shouldBeSuccessWithEmptyKeys() { diff --git a/examples/demo/build.gradle.kts b/examples/demo/build.gradle.kts index b0dc0f2a9..159ff201c 100644 --- a/examples/demo/build.gradle.kts +++ b/examples/demo/build.gradle.kts @@ -19,8 +19,6 @@ buildscript { allprojects { repositories { - // mavenLocal first so locally-published SDK builds resolve over Central when testing against a snapshot. - mavenLocal() google() mavenCentral() // Huawei maven From 0a3d18ebc6da138b5d7c37cbec365abe454436d5 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Thu, 23 Apr 2026 16:58:40 -0400 Subject: [PATCH 09/23] removing features node in ConfigModel --- .../internal/backend/IParamsBackendService.kt | 1 - .../backend/impl/ParamsBackendService.kt | 14 ------- .../core/internal/config/ConfigModel.kt | 17 ++------- .../config/impl/ConfigModelStoreListener.kt | 1 - .../core/internal/features/FeatureManager.kt | 6 +-- .../internal/features/FeatureManagerTests.kt | 37 ++++--------------- 6 files changed, 12 insertions(+), 64 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IParamsBackendService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IParamsBackendService.kt index 54f3cd85e..8773a23af 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IParamsBackendService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IParamsBackendService.kt @@ -35,7 +35,6 @@ internal class ParamsObject( var locationShared: Boolean? = null, var requiresUserPrivacyConsent: Boolean? = null, var opRepoExecutionInterval: Long? = null, - val features: List = emptyList(), var influenceParams: InfluenceParamsObject, var fcmParams: FCMParamsObject, val remoteLoggingParams: RemoteLoggingParamsObject, diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/ParamsBackendService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/ParamsBackendService.kt index ec0af8605..dfaaa027d 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/ParamsBackendService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/ParamsBackendService.kt @@ -68,19 +68,6 @@ internal class ParamsBackendService( ) } - val features = - responseJson.optJSONArray("features") - ?.let { featuresJson -> - buildList { - for (i in 0 until featuresJson.length()) { - val featureName = featuresJson.optString(i, "") - if (featureName.isNotBlank()) { - add(featureName) - } - } - } - } ?: emptyList() - return ParamsObject( googleProjectNumber = responseJson.safeString("android_sender_id"), enterprise = responseJson.safeBool("enterp"), @@ -97,7 +84,6 @@ internal class ParamsBackendService( requiresUserPrivacyConsent = responseJson.safeBool("requires_user_privacy_consent"), // TODO: New opRepoExecutionInterval = responseJson.safeLong("oprepo_execution_interval"), - features = features, influenceParams = influenceParams ?: InfluenceParamsObject(), fcmParams = fcmParams ?: FCMParamsObject(), remoteLoggingParams = remoteLoggingParams ?: RemoteLoggingParamsObject(), diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt index bfaf08eb8..616fa1916 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt @@ -290,22 +290,11 @@ class ConfigModel : Model() { setBooleanProperty(::clearGroupOnSummaryClick.name, value) } - /** - * Feature ids from the main OneSignal config / params response (legacy channel). - * Presence of a name means the feature is on. This list is distinct from [sdkRemoteFeatureFlags]; - * [com.onesignal.core.internal.features.FeatureManager] unions both (plus any local test overrides). - */ - var features: List - get() = getListProperty(::features.name) { emptyList() } - set(value) { - setListProperty(::features.name, value) - } - /** * Feature ids from the Turbine SDK feature-flags HTTP endpoint * ([com.onesignal.core.internal.backend.IFeatureFlagsBackendService]), updated while the app is - * foreground. Unioned with [features] in [com.onesignal.core.internal.features.FeatureManager] - * when evaluating [com.onesignal.core.internal.features.FeatureFlag] entries. + * foreground. [com.onesignal.core.internal.features.FeatureManager] unions these with any local + * test overrides when evaluating [com.onesignal.core.internal.features.FeatureFlag] entries. */ var sdkRemoteFeatureFlags: List get() = getListProperty(::sdkRemoteFeatureFlags.name) { emptyList() } @@ -369,7 +358,7 @@ class ConfigModel : Model() { property: String, jsonArray: JSONArray, ): List<*>? { - if (property == ::features.name || property == ::sdkRemoteFeatureFlags.name) { + if (property == ::sdkRemoteFeatureFlags.name) { return buildList { for (i in 0 until jsonArray.length()) { val featureName = jsonArray.optString(i, "") diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListener.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListener.kt index 9e5c4fc32..5dcfe83b7 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListener.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListener.kt @@ -95,7 +95,6 @@ internal class ConfigModelStoreListener( params.locationShared?.let { config.locationShared = it } params.requiresUserPrivacyConsent?.let { config.consentRequired = it } params.opRepoExecutionInterval?.let { config.opRepoExecutionInterval = it } - config.features = params.features params.influenceParams.notificationLimit?.let { config.influenceParams.notificationLimit = it } params.influenceParams.indirectNotificationAttributionWindow?.let { config.influenceParams.indirectNotificationAttributionWindow = it } params.influenceParams.iamLimit?.let { config.influenceParams.iamLimit = it } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt index 6e2dae7f1..84227f06d 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt @@ -69,8 +69,7 @@ internal class FeatureManager( args: ModelChangedArgs, tag: String, ) { - if (args.property == ConfigModel::features.name || - args.property == ConfigModel::sdkRemoteFeatureFlags.name || + if (args.property == ConfigModel::sdkRemoteFeatureFlags.name || args.property == ConfigModel::sdkRemoteFeatureFlagMetadata.name ) { Logging.debug("OneSignal: FeatureManager.onModelUpdated(property=${args.property}, tag=$tag)") @@ -89,8 +88,7 @@ internal class FeatureManager( ) { val enabledFeatureKeys = ( - model.features.map { canonicalizeFeatureKey(it) } + - model.sdkRemoteFeatureFlags.map { canonicalizeFeatureKey(it) } + + model.sdkRemoteFeatureFlags.map { canonicalizeFeatureKey(it) } + localFeatureOverrides.map { canonicalizeFeatureKey(it) } ).toSet() if (localFeatureOverrides.isNotEmpty()) { diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt index fc3aa6949..f2e2e6f07 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt @@ -22,27 +22,9 @@ class FeatureManagerTests : FunSpec({ every { model.sdkRemoteFeatureFlagMetadata } returns null } - test("initial state should enable BACKGROUND_THREADING when feature is present") { - // Given - val initialModel = mockk() - stubConfigModel(initialModel) - every { initialModel.features } returns listOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) - val configModelStore = mockk() - every { configModelStore.model } returns initialModel - every { configModelStore.subscribe(any()) } just runs - - // When - val manager = FeatureManager(configModelStore) - - // Then - manager.isEnabled(FeatureFlag.SDK_BACKGROUND_THREADING) shouldBe true - ThreadingMode.useBackgroundThreading shouldBe true - } - - test("initial state enables BACKGROUND_THREADING when key only exists on sdk remote flags") { + test("initial state enables BACKGROUND_THREADING when key is present in sdk remote flags") { val initialModel = mockk() stubConfigModel(initialModel) - every { initialModel.features } returns emptyList() every { initialModel.sdkRemoteFeatureFlags } returns listOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) val configModelStore = mockk() every { configModelStore.model } returns initialModel @@ -57,7 +39,6 @@ class FeatureManagerTests : FunSpec({ test("initial state enables BACKGROUND_THREADING when remote key differs only by letter case") { val initialModel = mockk() stubConfigModel(initialModel) - every { initialModel.features } returns emptyList() every { initialModel.sdkRemoteFeatureFlags } returns listOf("SDK_Background_Threading") val configModelStore = mockk() every { configModelStore.model } returns initialModel @@ -73,7 +54,6 @@ class FeatureManagerTests : FunSpec({ // Given val initialModel = mockk() stubConfigModel(initialModel) - every { initialModel.features } returns emptyList() val configModelStore = mockk() every { configModelStore.model } returns initialModel every { configModelStore.subscribe(any()) } just runs @@ -81,7 +61,7 @@ class FeatureManagerTests : FunSpec({ val updatedModel = mockk() stubConfigModel(updatedModel) - every { updatedModel.features } returns listOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) + every { updatedModel.sdkRemoteFeatureFlags } returns listOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) // When manager.onModelReplaced(updatedModel, ModelChangeTags.HYDRATE) @@ -95,18 +75,17 @@ class FeatureManagerTests : FunSpec({ // Given val model = mockk() stubConfigModel(model) - every { model.features } returns emptyList() val configModelStore = mockk() every { configModelStore.model } returns model every { configModelStore.subscribe(any()) } just runs val manager = FeatureManager(configModelStore) - every { model.features } returns listOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) + every { model.sdkRemoteFeatureFlags } returns listOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) // When manager.onModelUpdated( args = mockk { - every { property } returns ConfigModel::features.name + every { property } returns ConfigModel::sdkRemoteFeatureFlags.name }, tag = ModelChangeTags.NORMAL ) @@ -120,18 +99,18 @@ class FeatureManagerTests : FunSpec({ // Given val model = mockk() stubConfigModel(model) - every { model.features } returns listOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) + every { model.sdkRemoteFeatureFlags } returns listOf(FeatureFlag.SDK_BACKGROUND_THREADING.key) val configModelStore = mockk() every { configModelStore.model } returns model every { configModelStore.subscribe(any()) } just runs val manager = FeatureManager(configModelStore) - every { model.features } returns emptyList() + every { model.sdkRemoteFeatureFlags } returns emptyList() // When manager.onModelUpdated( args = mockk { - every { property } returns ConfigModel::features.name + every { property } returns ConfigModel::sdkRemoteFeatureFlags.name }, tag = ModelChangeTags.NORMAL ) @@ -144,7 +123,6 @@ class FeatureManagerTests : FunSpec({ test("remoteFeatureFlagMetadata returns parsed JSON from config") { val initialModel = mockk() stubConfigModel(initialModel) - every { initialModel.features } returns emptyList() every { initialModel.sdkRemoteFeatureFlagMetadata } returns """{"X":{"note":"y"}}""" val configModelStore = mockk() every { configModelStore.model } returns initialModel @@ -159,7 +137,6 @@ class FeatureManagerTests : FunSpec({ test("remoteFeatureFlagMetadata is null when config has no stored metadata") { val initialModel = mockk() stubConfigModel(initialModel) - every { initialModel.features } returns emptyList() every { initialModel.sdkRemoteFeatureFlagMetadata } returns null val configModelStore = mockk() every { configModelStore.model } returns initialModel From 68f2df3a07cf5eac12ce5bd7335f0e919a3f250f Mon Sep 17 00:00:00 2001 From: Nan Date: Fri, 24 Apr 2026 11:37:18 -0700 Subject: [PATCH 10/23] nit(style): fix spotless formatting and core detekt issues --- .../core/internal/backend/impl/FeatureFlagsJsonParser.kt | 3 +++ .../com/onesignal/core/internal/features/FeatureManager.kt | 2 +- .../onesignal/core/internal/features/FeatureManagerTests.kt | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt index 560bd04a3..963f327a5 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt @@ -59,6 +59,7 @@ internal object FeatureFlagsJsonParser { } } + @Suppress("ReturnCount") private fun parseRootStrict(root: JsonObject): RemoteFeatureFlagsResult? { val featuresEl = root[FEATURES_PROPERTY] ?: return null val featuresArray = featuresEl as? JsonArray ?: return null @@ -88,6 +89,7 @@ internal object FeatureFlagsJsonParser { return RemoteFeatureFlagsResult(keys, metaOut) } + @Suppress("ReturnCount") private fun findSiblingJsonObject( root: JsonObject, rawKeyFromFeaturesArray: String, @@ -127,6 +129,7 @@ internal object FeatureFlagsJsonParser { * Decodes [ConfigModel.sdkRemoteFeatureFlagMetadata] (a JSON object of flag id → object) into a map. * Non-object values are skipped so each entry stays a [JsonObject] for nested decoding (e.g. with `Json.decodeFromJsonElement`). */ + @Suppress("ReturnCount") fun parseStoredMetadataMap(raw: String?): Map { if (raw.isNullOrBlank()) { return emptyMap() diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt index 84227f06d..738186db2 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt @@ -90,7 +90,7 @@ internal class FeatureManager( ( model.sdkRemoteFeatureFlags.map { canonicalizeFeatureKey(it) } + localFeatureOverrides.map { canonicalizeFeatureKey(it) } - ).toSet() + ).toSet() if (localFeatureOverrides.isNotEmpty()) { Logging.warn( "OneSignal: Local feature override enabled for testing only: $localFeatureOverrides", diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt index f2e2e6f07..128a50b63 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt @@ -7,10 +7,10 @@ import com.onesignal.core.internal.config.ConfigModelStore import io.kotest.core.spec.style.FunSpec import io.kotest.matchers.shouldBe import io.mockk.every -import kotlinx.serialization.json.jsonPrimitive import io.mockk.just import io.mockk.mockk import io.mockk.runs +import kotlinx.serialization.json.jsonPrimitive class FeatureManagerTests : FunSpec({ beforeEach { From 28611447af3932316ad18bfe23a45e7a7e9f0808 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Mon, 27 Apr 2026 11:38:24 -0400 Subject: [PATCH 11/23] magic numbers --- .../impl/TurbineSdkFeatureFlagsPath.kt | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPath.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPath.kt index 89f0e9158..c533f8e50 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPath.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPath.kt @@ -34,28 +34,34 @@ internal object TurbineSdkFeatureFlagsPath { */ internal fun percentEncodePathSegmentUtf8(segment: String): String { val bytes = segment.encodeToByteArray() - return buildString(bytes.size * 3) { + return buildString(bytes.size * PCT_ENCODED_MAX_OUTPUT_CHARS_PER_INPUT_BYTE) { for (b in bytes) { - val u = b.toInt() and 0xff + val u = b.toInt() and BYTE_MASK if (isUnreservedByte(u)) { append(u.toChar()) } else { append('%') - append(HEX_DIGITS[u shr 4]) - append(HEX_DIGITS[u and 15]) + append(HEX_DIGITS[u shr HEX_NYBBLE_SHIFT]) + append(HEX_DIGITS[u and HEX_NYBBLE_MASK]) } } } } + /** Per RFC 3986 section 2.3 unreserved (ALPHA / DIGIT / "-" / "." / "_" / "~"). */ private fun isUnreservedByte(u: Int): Boolean = - u in 0x41..0x5A || - u in 0x61..0x7A || - u in 0x30..0x39 || - u == 0x2D || - u == 0x2E || - u == 0x5F || - u == 0x7E + u in 'A'.code..'Z'.code || + u in 'a'.code..'z'.code || + u in '0'.code..'9'.code || + u == '-'.code || + u == '.'.code || + u == '_'.code || + u == '~'.code + + private const val BYTE_MASK: Int = 0xFF + private const val PCT_ENCODED_MAX_OUTPUT_CHARS_PER_INPUT_BYTE: Int = 3 + private const val HEX_NYBBLE_MASK: Int = 0b1111 + private const val HEX_NYBBLE_SHIFT: Int = 4 private const val HEX_DIGITS = "0123456789ABCDEF" } From 87868520c4ee4c8c4d0d1bd1f3dc5cdbc85adfff Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Mon, 27 Apr 2026 14:07:09 -0400 Subject: [PATCH 12/23] fix(core): preserve feature flag cache against snapshot race and contract-violating responses ConfigModelStoreListener.fetchParams now re-sources sdkRemoteFeatureFlags{,Metadata} from the live model immediately before replace(), so concurrent in-place writes from FeatureFlagsRefreshService survive Model.initializeFromModel's data.clear()+putAll(). FeatureFlagsJsonParser.parseRootStrict now distinguishes an authoritative empty features array from one that filtered down to empty due to element-type violations; the latter returns null -> Unavailable so callers preserve the cached list rather than overwriting it with []. Adds regression tests for both paths. Addresses PR #2612 review comments. Made-with: Cursor --- .../backend/impl/FeatureFlagsJsonParser.kt | 5 +- .../config/impl/ConfigModelStoreListener.kt | 5 ++ .../impl/FeatureFlagsJsonParserTests.kt | 19 +++++ .../impl/ConfigModelStoreListenerTests.kt | 81 +++++++++++++++++++ 4 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListenerTests.kt diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt index 963f327a5..9a6a85231 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt @@ -74,7 +74,10 @@ internal object FeatureFlagsJsonParser { }.distinctBy { it.second } if (flagEntries.isEmpty()) { - return RemoteFeatureFlagsResult(emptyList(), null) + // `[]` is an authoritative empty config; a non-empty array that filtered down + // to empty is a contract violation. Null surfaces as Unavailable upstream so + // callers preserve the cached list instead of overwriting it with []. + return if (featuresArray.isEmpty()) RemoteFeatureFlagsResult(emptyList(), null) else null } val keys = flagEntries.map { it.second } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListener.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListener.kt index 5dcfe83b7..e15b6faa0 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListener.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListener.kt @@ -106,6 +106,11 @@ internal class ConfigModelStoreListener( params.remoteLoggingParams.logLevel?.let { config.remoteLoggingParams.logLevel = it } config.remoteLoggingParams.isEnabled = params.remoteLoggingParams.isEnabled + // Re-source from live model: replace() is a full data.clear()+putAll(), + // and these fields are written in-place by FeatureFlagsRefreshService. + config.sdkRemoteFeatureFlags = _configModelStore.model.sdkRemoteFeatureFlags + config.sdkRemoteFeatureFlagMetadata = _configModelStore.model.sdkRemoteFeatureFlagMetadata + _configModelStore.replace(config, ModelChangeTags.HYDRATE) success = true } catch (ex: BackendException) { diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt index ba84d9e2d..43983fce8 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt @@ -86,6 +86,25 @@ class FeatureFlagsJsonParserTests : FunSpec({ FeatureFlagsJsonParser.parseSuccessful("""{"features":"bad"}""") shouldBe null } + test("parseSuccessful returns null when features array is non-empty but every element is contract-violating") { + // Element-type violations on a non-empty array must be Unavailable, not Success(empty); + // otherwise callers would overwrite the cached list with []. + FeatureFlagsJsonParser.parseSuccessful("""{"features":[1,2,3]}""") shouldBe null + FeatureFlagsJsonParser.parseSuccessful("""{"features":[null]}""") shouldBe null + FeatureFlagsJsonParser.parseSuccessful("""{"features":[{}]}""") shouldBe null + FeatureFlagsJsonParser.parseSuccessful("""{"features":[[]]}""") shouldBe null + FeatureFlagsJsonParser.parseSuccessful("""{"features":[true,false]}""") shouldBe null + FeatureFlagsJsonParser.parseSuccessful("""{"features":[""]}""") shouldBe null + FeatureFlagsJsonParser.parseSuccessful("""{"features":[" "]}""") shouldBe null + } + + test("parseSuccessful keeps mixed arrays (drops invalid elements, keeps valid ones)") { + // Tolerance contract: any valid string id keeps the result; invalid siblings are dropped. + val r = requireNotNull(FeatureFlagsJsonParser.parseSuccessful("""{"features":["good", 1, null, {}, ""]}""")) + r.enabledKeys shouldBe listOf("good") + r.metadata shouldBe null + } + test("parseSuccessful returns null for Turbine error body (e.g. 403 Forbidden)") { FeatureFlagsJsonParser.parseSuccessful("""{"errors":["Forbidden"]}""") shouldBe null FeatureFlagsJsonParser.parseSuccessful("""{"errors":["Not Found"]}""") shouldBe null diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListenerTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListenerTests.kt new file mode 100644 index 000000000..c1ac8b6e6 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/config/impl/ConfigModelStoreListenerTests.kt @@ -0,0 +1,81 @@ +package com.onesignal.core.internal.config.impl + +import com.onesignal.core.internal.backend.IParamsBackendService +import com.onesignal.core.internal.backend.ParamsObject +import com.onesignal.core.internal.config.ConfigModelStore +import com.onesignal.mocks.IOMockHelper +import com.onesignal.mocks.IOMockHelper.awaitIO +import com.onesignal.mocks.MockPreferencesService +import com.onesignal.user.internal.subscriptions.ISubscriptionManager +import com.onesignal.user.internal.subscriptions.SubscriptionList +import com.onesignal.user.subscriptions.IPushSubscription +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.shouldBe +import io.mockk.coEvery +import io.mockk.every +import io.mockk.mockk + +/** + * Regression coverage for the snapshot/replace race in [ConfigModelStoreListener.fetchParams]: + * concurrent in-place writes to sdkRemoteFeatureFlags{,Metadata} from + * [FeatureFlagsRefreshService] must survive replace(). + */ +class ConfigModelStoreListenerTests : FunSpec({ + listener(IOMockHelper) + + test("fetchParams does not clobber sdkRemoteFeatureFlags written between snapshot and replace") { + // Real store so replace() goes through Model.initializeFromModel (data.clear()+putAll()). + val store = ConfigModelStore(MockPreferencesService()) + store.model.appId = "test-app-id" + store.model.sdkRemoteFeatureFlags = emptyList() + store.model.sdkRemoteFeatureFlagMetadata = null + + // Race trigger: locationShared is read between snapshot and replace, so its getter + // side-effect lands inside the window. Returning null short-circuits the let { } + // body; only the side-effect matters. + val params = mockk(relaxed = true) + every { params.locationShared } answers { + store.model.sdkRemoteFeatureFlags = listOf("sdk_background_threading") + store.model.sdkRemoteFeatureFlagMetadata = """{"sdk_background_threading":{"x":1}}""" + null + } + + val paramsBackend = mockk() + coEvery { paramsBackend.fetchParams(any(), any()) } returns params + + val pushSub = mockk(relaxed = true) + val subscriptionManager = mockk() + every { subscriptionManager.subscriptions } returns SubscriptionList(emptyList(), pushSub) + + val listener = ConfigModelStoreListener(store, paramsBackend, subscriptionManager) + listener.start() + awaitIO() + + // Without the fix, the stale snapshot wins and these are [] / null. + store.model.sdkRemoteFeatureFlags shouldBe listOf("sdk_background_threading") + store.model.sdkRemoteFeatureFlagMetadata shouldBe """{"sdk_background_threading":{"x":1}}""" + } + + test("fetchParams preserves sdkRemoteFeatureFlags already on the live model when no in-flight write occurs") { + // Smoke: no race -> existing values round-trip through snapshot + replace. + val store = ConfigModelStore(MockPreferencesService()) + store.model.appId = "test-app-id" + store.model.sdkRemoteFeatureFlags = listOf("sdk_background_threading") + store.model.sdkRemoteFeatureFlagMetadata = """{"sdk_background_threading":{"x":1}}""" + + val params = mockk(relaxed = true) + val paramsBackend = mockk() + coEvery { paramsBackend.fetchParams(any(), any()) } returns params + + val pushSub = mockk(relaxed = true) + val subscriptionManager = mockk() + every { subscriptionManager.subscriptions } returns SubscriptionList(emptyList(), pushSub) + + val listener = ConfigModelStoreListener(store, paramsBackend, subscriptionManager) + listener.start() + awaitIO() + + store.model.sdkRemoteFeatureFlags shouldBe listOf("sdk_background_threading") + store.model.sdkRemoteFeatureFlagMetadata shouldBe """{"sdk_background_threading":{"x":1}}""" + } +}) From 98e436dbb11f8afc83746ff761992c2f353a029f Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Mon, 27 Apr 2026 15:32:46 -0400 Subject: [PATCH 13/23] fix(core): dedup remote feature-flags fetch on same-appId hydrate `ConfigModelStoreListener.fetchParams` replaces the live `ConfigModel` with the HYDRATE tag during init, which fired `onModelReplaced` on `FeatureFlagsRefreshService` and triggered a second Turbine GET right after the focus-driven first one, even though the appId hadn't changed. Track the appId currently being polled and treat a re-trigger for the same appId as a no-op; genuine appId changes still cancel and restart. `onUnfocused` clears the cached appId so the next focus event re-arms polling. Pull the foreground cadence off the constructor and onto an `internal var refreshIntervalMs` (default 8 min, test-only override before `start`). The IoC reflection in `ServiceRegistrationReflection.resolve` cannot resolve a non-service `Long` parameter and was throwing "Could not instantiate service: ServiceRegistrationReflection@..." out of `StartupService.scheduleStart` once the service was registered. Made-with: Cursor --- .../config/impl/FeatureFlagsRefreshService.kt | 50 ++++- .../impl/FeatureFlagsRefreshServiceTests.kt | 190 ++++++++++++++++++ 2 files changed, 234 insertions(+), 6 deletions(-) create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshServiceTests.kt diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt index 7ff63499a..40407f821 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt @@ -22,12 +22,23 @@ import kotlin.coroutines.coroutineContext /** * Fetches remote SDK feature flags when the app is in the foreground, immediately on focus and then - * every [REFRESH_INTERVAL_MS] while the session stays in the foreground. Updates + * every [refreshIntervalMs] while the session stays in the foreground. Updates * [ConfigModel.sdkRemoteFeatureFlags] / [ConfigModel.sdkRemoteFeatureFlagMetadata] so * [com.onesignal.core.internal.features.FeatureManager] stays in sync. * * Remote fields are updated in place on the live [ConfigModel] (with [ConfigModelChangeTags.REMOTE_FEATURE_FLAGS]) * so concurrent hydration cannot be overwritten by a stale full-model snapshot. + * + * Polling is keyed on the active [ConfigModel.appId]: once a poll loop is running for a given + * appId, redundant triggers (e.g. [com.onesignal.common.modeling.ModelChangeTags.HYDRATE] replaces + * from [ConfigModelStoreListener.fetchParams] that write the same appId back) are a no-op so we + * don't double-fire the Turbine GET at startup. Genuine appId changes still cancel and restart. + * + * IMPORTANT: Constructor parameters must remain limited to types the IoC reflection can resolve + * via [com.onesignal.common.services.IServiceProvider] (registered services or `List`). + * Configuration knobs like [refreshIntervalMs] live as `internal var` instead so reflection in + * [com.onesignal.common.services.ServiceRegistrationReflection.resolve] still picks the only + * constructor and tests can still override the value before [start]. */ internal class FeatureFlagsRefreshService( private val applicationService: IApplicationService, @@ -36,8 +47,22 @@ internal class FeatureFlagsRefreshService( ) : IStartableService, IApplicationLifecycleHandler, ISingletonModelStoreChangeHandler { + /** + * Foreground polling cadence; defaults to [DEFAULT_REFRESH_INTERVAL_MS] (8 minutes). Test-only + * override (must be set before [start] / focus) – keep out of the constructor so the IoC's + * reflection-based resolver doesn't trip on a non-service `Long` parameter (see class KDoc). + */ + internal var refreshIntervalMs: Long = DEFAULT_REFRESH_INTERVAL_MS + private var pollJob: Job? = null + /** + * AppId currently being polled. Used to dedup redundant `restartForegroundPolling` triggers + * (e.g. HYDRATE replace from [ConfigModelStoreListener.fetchParams] that doesn't change the + * appId). Cleared in [onUnfocused] so the next focus event always re-arms polling. + */ + private var pollingAppId: String? = null + override fun start() { applicationService.addApplicationLifecycleHandler(this) configModelStore.subscribe(this) @@ -52,6 +77,7 @@ internal class FeatureFlagsRefreshService( synchronized(this) { pollJob?.cancel() pollJob = null + pollingAppId = null } } @@ -81,24 +107,36 @@ internal class FeatureFlagsRefreshService( private fun restartForegroundPolling() { synchronized(this) { + val appId = configModelStore.model.appId + if (appId.isEmpty()) { + pollJob?.cancel() + pollJob = null + pollingAppId = null + return + } + // Dedup: an active loop for the same appId is already covering us. + if (pollingAppId == appId) { + return + } pollJob?.cancel() + pollingAppId = appId pollJob = OneSignalDispatchers.launchOnIO { while (coroutineContext.isActive) { if (!applicationService.isInForeground) { break } - val appId = configModelStore.model.appId - if (appId.isNotEmpty()) { + val current = configModelStore.model.appId + if (current.isNotEmpty()) { try { - fetchAndApply(appId) + fetchAndApply(current) } catch (e: CancellationException) { throw e } catch (e: Exception) { Logging.warn("FeatureFlagsRefreshService: fetch failed", e) } } - delay(REFRESH_INTERVAL_MS) + delay(refreshIntervalMs) } } } @@ -125,6 +163,6 @@ internal class FeatureFlagsRefreshService( companion object { // Foreground polling cadence for remote feature flags (8 minutes). - private const val REFRESH_INTERVAL_MS = 480_000L + private const val DEFAULT_REFRESH_INTERVAL_MS = 480_000L } } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshServiceTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshServiceTests.kt new file mode 100644 index 000000000..5ceb40e77 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshServiceTests.kt @@ -0,0 +1,190 @@ +package com.onesignal.core.internal.config.impl + +import com.onesignal.common.modeling.ModelChangeTags +import com.onesignal.core.internal.application.IApplicationLifecycleHandler +import com.onesignal.core.internal.application.IApplicationService +import com.onesignal.core.internal.backend.IFeatureFlagsBackendService +import com.onesignal.core.internal.backend.RemoteFeatureFlagsFetchOutcome +import com.onesignal.core.internal.config.ConfigModel +import com.onesignal.core.internal.config.ConfigModelStore +import com.onesignal.mocks.IOMockHelper +import com.onesignal.mocks.IOMockHelper.awaitIO +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.shouldBe +import io.mockk.Runs +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.every +import io.mockk.just +import io.mockk.mockk +import io.mockk.slot + +/** + * Regression coverage for the duplicate Turbine feature-flags fetch at SDK startup. + * + * Two startables fire at init: + * 1. [FeatureFlagsRefreshService.start] -> [IApplicationService.addApplicationLifecycleHandler] + * synchronously delivers `onFocus(firedOnSubscribe = true)` while the app is foregrounded, + * which kicks off polling and issues fetch #1. + * 2. [ConfigModelStoreListener.fetchParams] (Android params) finishes and calls + * `_configModelStore.replace(config, ModelChangeTags.HYDRATE)`, which fires `onModelReplaced` + * on every subscriber. The replaced model carries the **same** `appId`, but pre-fix the + * listener still triggered a second poll, producing fetch #2 a few hundred ms after #1. + * + * The fix tracks the appId currently being polled and treats a re-trigger for the same appId as + * a no-op. Tests below assert both directions: same-appId hydrates are deduped, but a genuine + * appId change still cancels and re-fetches. + */ +class FeatureFlagsRefreshServiceTests : FunSpec({ + listener(IOMockHelper) + + fun mockBackend(): Pair Int> { + val backend = mockk() + var count = 0 + coEvery { backend.fetchRemoteFeatureFlags(any()) } answers { + count++ + RemoteFeatureFlagsFetchOutcome.Unavailable + } + return backend to { count } + } + + /** + * Builds an [IApplicationService] mock that mirrors production behavior at init: + * - `addApplicationLifecycleHandler` synchronously fires `onFocus(true)` (matches + * [com.onesignal.core.internal.application.impl.ApplicationService.addApplicationLifecycleHandler] + * when `current != null`). + * - `isInForeground` returns the supplied [foregroundSequence] in order; tests pick exactly + * the right pattern of `true`/`false` so that each polling loop runs one fetch then exits + * on the next iteration. Coupled with `refreshIntervalMs = 0L` this keeps tests + * deterministic without needing a virtual time scheduler. + * + * Per-trigger budget for [foregroundSequence]: + * - Initial `start()` -> `onFocus(true)` -> polling loop: **2** values (`true`, `false`). + * - `onModelReplaced` / `onModelUpdated` event handler condition: **1** value. + * - Each polling loop kicked off by an event handler when not deduped: **2** values. + */ + fun foregroundedAppService(vararg foregroundSequence: Boolean): IApplicationService { + val app = mockk() + val handlerSlot = slot() + every { app.addApplicationLifecycleHandler(capture(handlerSlot)) } answers { + handlerSlot.captured.onFocus(true) + } + every { app.removeApplicationLifecycleHandler(any()) } just Runs + every { app.isInForeground } returnsMany foregroundSequence.toList() + return app + } + + fun mockConfigStore(model: ConfigModel): ConfigModelStore { + val store = mockk() + every { store.model } returns model + every { store.subscribe(any()) } just Runs + every { store.unsubscribe(any()) } just Runs + return store + } + + test("HYDRATE replace with the same appId does not trigger a duplicate fetch") { + val model = ConfigModel().apply { appId = "appId-1" } + val store = mockConfigStore(model) + val (backend, fetchCount) = mockBackend() + // start: [true, false] (loop iter1=true, iter2=false break) + onModelReplaced check: [true] + val app = foregroundedAppService(true, false, true) + + val service = FeatureFlagsRefreshService(app, store, backend).apply { refreshIntervalMs = 0L } + + // Mirrors `OneSignalImp.internalInit` -> `startupService.scheduleStart()`: synchronously + // attaches the lifecycle handler, which fires `onFocus(true)` -> initial fetch. + service.start() + awaitIO() + fetchCount() shouldBe 1 + + // Mirrors `ConfigModelStoreListener.fetchParams` -> `replace(config, HYDRATE)` with the + // same appId already on the live model. Pre-fix this fired a second Turbine GET. + service.onModelReplaced(model, ModelChangeTags.HYDRATE) + awaitIO() + fetchCount() shouldBe 1 + } + + test("NORMAL replace with the same appId does not trigger a duplicate fetch") { + val model = ConfigModel().apply { appId = "appId-1" } + val store = mockConfigStore(model) + val (backend, fetchCount) = mockBackend() + val app = foregroundedAppService(true, false, true) + + val service = FeatureFlagsRefreshService(app, store, backend).apply { refreshIntervalMs = 0L } + service.start() + awaitIO() + fetchCount() shouldBe 1 + + service.onModelReplaced(model, ModelChangeTags.NORMAL) + awaitIO() + fetchCount() shouldBe 1 + } + + test("HYDRATE replace with a different appId cancels and re-fetches") { + val model = ConfigModel().apply { appId = "appId-1" } + val store = mockConfigStore(model) + val (backend, fetchCount) = mockBackend() + // start: [true, false] + onModelReplaced check: [true] + new poll loop: [true, false]. + val app = foregroundedAppService(true, false, true, true, false) + + val service = FeatureFlagsRefreshService(app, store, backend).apply { refreshIntervalMs = 0L } + service.start() + awaitIO() + fetchCount() shouldBe 1 + + // Genuine appId change (e.g. re-init with a different App ID). + model.appId = "appId-2" + service.onModelReplaced(model, ModelChangeTags.HYDRATE) + awaitIO() + fetchCount() shouldBe 2 + + coVerify(exactly = 1) { backend.fetchRemoteFeatureFlags("appId-1") } + coVerify(exactly = 1) { backend.fetchRemoteFeatureFlags("appId-2") } + } + + test("appId property update via onModelUpdated triggers a single re-fetch") { + val model = ConfigModel().apply { appId = "appId-1" } + val store = mockConfigStore(model) + val (backend, fetchCount) = mockBackend() + // start: [true, false] + onModelUpdated check: [true] + new poll loop: [true, false]. + val app = foregroundedAppService(true, false, true, true, false) + + val service = FeatureFlagsRefreshService(app, store, backend).apply { refreshIntervalMs = 0L } + service.start() + awaitIO() + fetchCount() shouldBe 1 + + model.appId = "appId-2" + service.onModelUpdated( + com.onesignal.common.modeling.ModelChangedArgs( + model = model, + path = ConfigModel::appId.name, + property = ConfigModel::appId.name, + oldValue = "appId-1", + newValue = "appId-2", + ), + ModelChangeTags.NORMAL, + ) + awaitIO() + fetchCount() shouldBe 2 + } + + test("unfocus then refocus restarts polling for the same appId") { + val model = ConfigModel().apply { appId = "appId-1" } + val store = mockConfigStore(model) + val (backend, fetchCount) = mockBackend() + // start: [true, false] + onFocus restart loop: [true, false]. + val app = foregroundedAppService(true, false, true, false) + + val service = FeatureFlagsRefreshService(app, store, backend).apply { refreshIntervalMs = 0L } + service.start() + awaitIO() + fetchCount() shouldBe 1 + + // onUnfocused must clear the cached pollingAppId so a subsequent refocus is not deduped. + service.onUnfocused() + service.onFocus(firedOnSubscribe = false) + awaitIO() + fetchCount() shouldBe 2 + } +}) From 74936f4622434c503ab735ed78d27e0c0bbcbc32 Mon Sep 17 00:00:00 2001 From: Nan Date: Fri, 17 Apr 2026 17:07:42 -0700 Subject: [PATCH 14/23] fix: login race causes subsequent calls to target previous user OneSignal.login() scheduled the user switch asynchronously via suspendifyOnIO, so the API call returned before the identity model was updated. Calls made right after login (e.g. addTag) applied to the OLD user because the switch hadn't happened yet. Split login into two phases: - switchUser(): synchronous under the login/logout lock. Swaps the local identity/properties/subscription models immediately so subsequent SDK calls see the new user. - enqueueLogin(): async. Enqueues the LoginUserOperation and awaits completion of the backend user creation. Because switchUser returns before enqueueLogin runs, there's a small window where RecoverFromDroppedLoginBug can observe the state (new externalId, local onesignalId, no LoginUserOperation in queue) and enqueue its own recovery login. When the real enqueueLogin then adds another login op, the executor crashes on a grouped batch containing two LoginUserOperations. Add a dedupe check in internalEnqueue: if a LoginUserOperation for the same onesignalId is already queued, drop the incoming one and wake its waiter optimistically with true so loginSuspend callers don't hang on enqueueAndWait. If the duplicate came from loadSavedOperations (addToStore = false), also remove it from the operation model store so we don't leave an orphan entry that lingers until the next cold start. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../internal/operations/impl/OperationRepo.kt | 22 +++++++++ .../com/onesignal/internal/OneSignalImp.kt | 13 +++-- .../onesignal/user/internal/LoginHelper.kt | 48 +++++++++++++------ .../user/internal/LoginHelperTests.kt | 12 +++-- 4 files changed, 73 insertions(+), 22 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt index 9b39566d1..2ead65c85 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt @@ -11,6 +11,7 @@ import com.onesignal.core.internal.startup.IStartableService import com.onesignal.core.internal.time.ITime import com.onesignal.debug.LogLevel import com.onesignal.debug.internal.logging.Logging +import com.onesignal.user.internal.operations.LoginUserOperation import com.onesignal.user.internal.operations.impl.states.NewRecordsState import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope @@ -161,6 +162,27 @@ internal class OperationRepo( return } + // Dedupe LoginUserOperation for the same user — prevents RecoverFromDroppedLoginBug + // and the real login() call from both enqueuing a login op during the timing window. + // Wake the waiter optimistically with `true`: the already-queued op will do the work, + // but we don't await its real result (that would require waiter chaining). enqueueAndWait + // callers like loginSuspend would otherwise hang forever. The only caller today is + // LoginHelper.enqueueLogin which just logs on failure, so the lost signal is acceptable. + // If the op came from loadSavedOperations (addToStore = false) it's also a stale + // duplicate in the store; remove it to avoid an orphan entry that lingers until the + // next cold start. + val op = queueItem.operation + if (op is LoginUserOperation && + queue.any { it.operation is LoginUserOperation && it.operation.onesignalId == op.onesignalId } + ) { + Logging.debug("OperationRepo: internalEnqueue - LoginUserOperation for onesignalId: ${op.onesignalId} already exists in the queue.") + if (!addToStore) { + _operationModelStore.remove(queueItem.operation.id) + } + queueItem.waiter?.wake(true) + return + } + if (index != null) { queue.add(index, queueItem) } else { diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt index b58d28aba..cfb9690c9 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt @@ -378,14 +378,20 @@ internal class OneSignalImp( if (isBackgroundThreadingEnabled) { waitForInit(operationName = "login") - suspendifyOnIO { loginHelper.login(externalId, jwtBearerToken) } } else { if (!isInitialized) { throw IllegalStateException("Must call 'initWithContext' before 'login'") } + } + + val context = loginHelper.switchUser(externalId, jwtBearerToken) ?: return + + if (isBackgroundThreadingEnabled) { + suspendifyOnIO { loginHelper.enqueueLogin(context) } + } else { Thread { runBlocking(runtimeIoDispatcher) { - loginHelper.login(externalId, jwtBearerToken) + loginHelper.enqueueLogin(context) } }.start() } @@ -646,7 +652,8 @@ internal class OneSignalImp( throw IllegalStateException("'initWithContext failed' before 'login'") } - loginHelper.login(externalId, jwtBearerToken) + val context = loginHelper.switchUser(externalId, jwtBearerToken) ?: return@withContext + loginHelper.enqueueLogin(context) } override suspend fun logoutSuspend() = diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/LoginHelper.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/LoginHelper.kt index 15939441b..6743e405f 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/LoginHelper.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/LoginHelper.kt @@ -13,20 +13,29 @@ class LoginHelper( private val configModel: ConfigModel, private val lock: Any, ) { - suspend fun login( + internal data class LoginEnqueueContext( + val appId: String, + val newIdentityOneSignalId: String, + val externalId: String, + val existingOneSignalId: String?, + ) + + /** + * Synchronously switches local user models under the login/logout lock so subsequent + * SDK calls (e.g. addTag) see the new user's identity immediately. Returns context + * needed for [enqueueLogin], or null if the user was already logged in with [externalId] + * (no switch needed). + */ + internal fun switchUser( externalId: String, jwtBearerToken: String? = null, - ) { - var currentIdentityExternalId: String? = null - var currentIdentityOneSignalId: String? = null - var newIdentityOneSignalId: String = "" - + ): LoginEnqueueContext? { synchronized(lock) { - currentIdentityExternalId = identityModelStore.model.externalId - currentIdentityOneSignalId = identityModelStore.model.onesignalId + val currentExternalId = identityModelStore.model.externalId + val currentOneSignalId = identityModelStore.model.onesignalId - if (currentIdentityExternalId == externalId) { - return + if (currentExternalId == externalId) { + return null } // TODO: Set JWT Token for all future requests. @@ -34,16 +43,25 @@ class LoginHelper( identityModel.externalId = externalId } - newIdentityOneSignalId = identityModelStore.model.onesignalId + val newOneSignalId = identityModelStore.model.onesignalId + val existingOneSignalId = + if (currentExternalId == null) currentOneSignalId else null + + return LoginEnqueueContext(configModel.appId, newOneSignalId, externalId, existingOneSignalId) } + } + /** + * Enqueues the [LoginUserOperation] and suspends until it completes. + */ + internal suspend fun enqueueLogin(context: LoginEnqueueContext) { val result = operationRepo.enqueueAndWait( LoginUserOperation( - configModel.appId, - newIdentityOneSignalId, - externalId, - if (currentIdentityExternalId == null) currentIdentityOneSignalId else null, + context.appId, + context.newIdentityOneSignalId, + context.externalId, + context.existingOneSignalId, ), ) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/LoginHelperTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/LoginHelperTests.kt index a501e73bc..bd59e0327 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/LoginHelperTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/LoginHelperTests.kt @@ -61,7 +61,8 @@ class LoginHelperTests : FunSpec({ // When runBlocking { - loginHelper.login(currentExternalId) + val context = loginHelper.switchUser(currentExternalId) + if (context != null) loginHelper.enqueueLogin(context) } // Then - should return early without any operations @@ -113,7 +114,8 @@ class LoginHelperTests : FunSpec({ // When runBlocking { - loginHelper.login(newExternalId) + val context = loginHelper.switchUser(newExternalId) + if (context != null) loginHelper.enqueueLogin(context) } // Then - should switch users and enqueue login operation @@ -178,7 +180,8 @@ class LoginHelperTests : FunSpec({ // When runBlocking { - loginHelper.login(newExternalId) + val context = loginHelper.switchUser(newExternalId) + if (context != null) loginHelper.enqueueLogin(context) } // Then - should provide existing OneSignal ID for anonymous user conversion @@ -239,7 +242,8 @@ class LoginHelperTests : FunSpec({ // When runBlocking { - loginHelper.login(newExternalId) + val context = loginHelper.switchUser(newExternalId) + if (context != null) loginHelper.enqueueLogin(context) } // Then - should still switch users but operation fails From ef813b1e458b7a02079ae2077f63b6a6fe67af26 Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 20 Apr 2026 08:46:13 -0700 Subject: [PATCH 15/23] Preserve existingOnesignalId when deduping LoginUserOperation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When dedupe fires, the existing queued op came from RecoverFromDroppedLoginBug (always has existingOnesignalId=null) and the incoming op from enqueueLogin may carry the anonymous user's UUID needed to merge their tags/subscriptions into the newly- identified user. Previously the dedupe kept whichever op was first, silently dropping the merge info when the recovery op won the race. Merge the incoming op's existingOnesignalId into the queued op in place before dropping the duplicate. LoginUserOperation.existingOnesignalId setter relaxed from private to internal to allow the in-place update. Also initialize the LoginUserOperations in the loadSavedOperations test properly — the old dedupe only compared ids so uninitialized ops worked incidentally; the new dedupe reads onesignalId. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../internal/operations/impl/OperationRepo.kt | 38 ++++++++++--------- .../internal/operations/LoginUserOperation.kt | 2 +- .../internal/operations/OperationRepoTests.kt | 4 +- 3 files changed, 24 insertions(+), 20 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt index 2ead65c85..f2f2eab16 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt @@ -162,25 +162,29 @@ internal class OperationRepo( return } - // Dedupe LoginUserOperation for the same user — prevents RecoverFromDroppedLoginBug - // and the real login() call from both enqueuing a login op during the timing window. - // Wake the waiter optimistically with `true`: the already-queued op will do the work, - // but we don't await its real result (that would require waiter chaining). enqueueAndWait - // callers like loginSuspend would otherwise hang forever. The only caller today is - // LoginHelper.enqueueLogin which just logs on failure, so the lost signal is acceptable. - // If the op came from loadSavedOperations (addToStore = false) it's also a stale - // duplicate in the store; remove it to avoid an orphan entry that lingers until the - // next cold start. + // Dedupe LoginUserOperation for the same user. Merge the incoming + // existingOnesignalId in so the anon-user conversion isn't lost when + // RecoverFromDroppedLoginBug wins the race (it enqueues with null). val op = queueItem.operation - if (op is LoginUserOperation && - queue.any { it.operation is LoginUserOperation && it.operation.onesignalId == op.onesignalId } - ) { - Logging.debug("OperationRepo: internalEnqueue - LoginUserOperation for onesignalId: ${op.onesignalId} already exists in the queue.") - if (!addToStore) { - _operationModelStore.remove(queueItem.operation.id) + if (op is LoginUserOperation) { + val existing = + queue.firstOrNull { + it.operation is LoginUserOperation && it.operation.onesignalId == op.onesignalId + } + if (existing != null) { + val existingOp = existing.operation as LoginUserOperation + if (op.existingOnesignalId != null && existingOp.existingOnesignalId == null) { + Logging.debug("OperationRepo: internalEnqueue - merged existingOnesignalId=${op.existingOnesignalId} into queued LoginUserOperation for onesignalId: ${op.onesignalId}.") + existingOp.existingOnesignalId = op.existingOnesignalId + } else { + Logging.debug("OperationRepo: internalEnqueue - LoginUserOperation for onesignalId: ${op.onesignalId} already exists in the queue.") + } + if (!addToStore) { + _operationModelStore.remove(queueItem.operation.id) + } + queueItem.waiter?.wake(true) + return } - queueItem.waiter?.wake(true) - return } if (index != null) { diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt index b283cc3da..4b707788f 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt @@ -48,7 +48,7 @@ class LoginUserOperation() : Operation(LoginUserOperationExecutor.LOGIN_USER) { */ var existingOnesignalId: String? get() = getOptStringProperty(::existingOnesignalId.name) - private set(value) { + internal set(value) { setOptStringProperty(::existingOnesignalId.name, value) } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt index 164949612..e90827026 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt @@ -100,8 +100,8 @@ class OperationRepoTests : FunSpec({ ), ) - val cachedOperation = LoginUserOperation() - val newOperation = LoginUserOperation() + val cachedOperation = LoginUserOperation("appId", "cached-onesignal-id", null, null) + val newOperation = LoginUserOperation("appId", "new-onesignal-id", null, null) val jsonArray = JSONArray() // cache the operation From dbe39d40cc0090c98217029443a59bde706093c1 Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 20 Apr 2026 09:35:48 -0700 Subject: [PATCH 16/23] Transfer waiter on dedupe and add merge-path test coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Change OperationQueueItem.waiter to var. On LoginUserOperation dedupe, transfer the incoming op's waiter to the already-queued item so the enqueueAndWait caller (loginSuspend) receives the real execution result instead of an optimistic true. Fall back to optimistic wake only in the unlikely case where both items already have waiters. - Add OperationRepoTests coverage for the dedupe+merge path with matching onesignalId and differing existingOnesignalId — the previous test setup used different onesignalIds so the merge branch had no regression guard. --- .../internal/operations/impl/OperationRepo.kt | 13 +++++--- .../internal/operations/OperationRepoTests.kt | 30 +++++++++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt index f2f2eab16..45ee8c05e 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt @@ -32,7 +32,7 @@ internal class OperationRepo( ) : IOperationRepo, IStartableService { internal class OperationQueueItem( val operation: Operation, - val waiter: WaiterWithValue? = null, + var waiter: WaiterWithValue? = null, val bucket: Int, var retries: Int = 0, ) { @@ -163,8 +163,9 @@ internal class OperationRepo( } // Dedupe LoginUserOperation for the same user. Merge the incoming - // existingOnesignalId in so the anon-user conversion isn't lost when + // existingOnesignalId so the anon-user conversion isn't lost when // RecoverFromDroppedLoginBug wins the race (it enqueues with null). + // Transfer the waiter so enqueueAndWait callers get the real result. val op = queueItem.operation if (op is LoginUserOperation) { val existing = @@ -174,15 +175,19 @@ internal class OperationRepo( if (existing != null) { val existingOp = existing.operation as LoginUserOperation if (op.existingOnesignalId != null && existingOp.existingOnesignalId == null) { - Logging.debug("OperationRepo: internalEnqueue - merged existingOnesignalId=${op.existingOnesignalId} into queued LoginUserOperation for onesignalId: ${op.onesignalId}.") + Logging.debug("OperationRepo: internalEnqueue - merging existingOnesignalId=${op.existingOnesignalId} into queued LoginUserOperation for onesignalId: ${op.onesignalId}.") existingOp.existingOnesignalId = op.existingOnesignalId } else { Logging.debug("OperationRepo: internalEnqueue - LoginUserOperation for onesignalId: ${op.onesignalId} already exists in the queue.") } + if (queueItem.waiter != null && existing.waiter == null) { + existing.waiter = queueItem.waiter + } else { + queueItem.waiter?.wake(true) + } if (!addToStore) { _operationModelStore.remove(queueItem.operation.id) } - queueItem.waiter?.wake(true) return } } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt index e90827026..bc96f5654 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt @@ -142,6 +142,36 @@ class OperationRepoTests : FunSpec({ operationRepo.queue.first().operation shouldBe cachedOperation } + test("enqueue dedupes LoginUserOperation for same onesignalId and merges existingOnesignalId") { + // Reproduces the RecoverFromDroppedLoginBug race: a recovery login op + // (existingOnesignalId=null) is already queued; an incoming enqueueLogin + // op for the same user carries the anon UUID needed for conversion. + // Dedup must merge the incoming existingOnesignalId into the queued op + // and transfer the waiter so the caller receives the real result. + val mocks = Mocks() + val operationRepo = mocks.operationRepo + + val queuedOp = LoginUserOperation("appId", "local-new", "alice", null) + queuedOp.id = UUID.randomUUID().toString() + synchronized(operationRepo.queue) { + operationRepo.queue.add(OperationQueueItem(queuedOp, bucket = 0)) + } + + val incomingOp = LoginUserOperation("appId", "local-new", "alice", "anon-uuid") + + // When — enqueue the incoming op (simulates enqueueLogin landing after recovery) + operationRepo.enqueue(incomingOp) + + // Yield so the launched coroutine in enqueue() can run + kotlinx.coroutines.runBlocking { delay(100) } + + // Then — queue has only the original op, with merged existingOnesignalId + operationRepo.queue.size shouldBe 1 + val merged = operationRepo.queue.first().operation as LoginUserOperation + merged.onesignalId shouldBe "local-new" + merged.existingOnesignalId shouldBe "anon-uuid" + } + test("containsInstanceOf") { // Given val mocks = Mocks() From af780de14028b2ef613388a19bbebf812613e7ee Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 20 Apr 2026 11:25:48 -0700 Subject: [PATCH 17/23] Fix FAIL_PAUSE_OPREPO hanging enqueueAndWait callers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an operation execution returns FAIL_PAUSE_OPREPO, the repo is paused and the ops are re-queued with their original waiters still attached. enqueueAndWait callers (loginSuspend) would suspend on the waiter forever — the op stays in the queue but never executes while paused, and the waiter is never woken. Wake the waiter with false and re-queue with waiter=null, matching the pattern from #2600. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../core/internal/operations/impl/OperationRepo.kt | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt index 45ee8c05e..8bcf7a9cb 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt @@ -333,9 +333,15 @@ internal class OperationRepo( Logging.error("Operation execution failed with eventual retry, pausing the operation repo: $operations") // keep the failed operation and pause the operation repo from executing paused = true - // add back all operations to the front of the queue to be re-executed. + // Unblock any enqueueAndWait callers so loginSuspend doesn't hang. + ops.forEach { it.waiter?.wake(false) } + // Re-queue with waiter = null: the operation is preserved for retry + // on next cold start, but the original waiter is detached since it + // was already woken above. synchronized(queue) { - ops.reversed().forEach { queue.add(0, it) } + ops.reversed().forEach { + queue.add(0, OperationQueueItem(it.operation, waiter = null, bucket = it.bucket, retries = it.retries)) + } } } } From 90bdb72c852eb911ad784500d33d66e85066fb01 Mon Sep 17 00:00:00 2001 From: Nan Date: Wed, 22 Apr 2026 10:49:36 -0700 Subject: [PATCH 18/23] tests: Use waitForInternalEnqueue helper in dedupe test Replace the ad-hoc runBlocking { delay(100) } in the new LoginUserOperation dedupe test with the same waitForInternalEnqueue helper every other test in the file uses. Deterministic instead of a fixed 100ms race against CI. --- .../onesignal/core/internal/operations/OperationRepoTests.kt | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt index bc96f5654..5be6d1ba1 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt @@ -161,9 +161,7 @@ class OperationRepoTests : FunSpec({ // When — enqueue the incoming op (simulates enqueueLogin landing after recovery) operationRepo.enqueue(incomingOp) - - // Yield so the launched coroutine in enqueue() can run - kotlinx.coroutines.runBlocking { delay(100) } + mocks.waitForInternalEnqueue() // Then — queue has only the original op, with merged existingOnesignalId operationRepo.queue.size shouldBe 1 From 6b87e29f61784f7754b7df795371a91053e5d076 Mon Sep 17 00:00:00 2001 From: Nan Date: Wed, 22 Apr 2026 11:02:04 -0700 Subject: [PATCH 19/23] fix: logout race causes subsequent calls to target previous user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirrors the login race fix for the logout flow. OneSignal.logout() scheduled the user switch asynchronously via suspendifyOnIO, so the API call returned before the identity/properties/subscription models were updated. Calls made right after logout() (e.g. addTag) targeted the pre-logout user. Split LogoutHelper.logout() into switchUser() (sync, under loginLogoutLock) and enqueueLogout() (async). OneSignalImp.logout() and logoutSuspend() call switchUser() synchronously, then launch enqueueLogout() in the coroutine scope — mirroring LoginHelper. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../com/onesignal/internal/OneSignalImp.kt | 13 ++++-- .../onesignal/user/internal/LogoutHelper.kt | 41 +++++++++++++------ .../user/internal/LogoutHelperTests.kt | 12 ++++-- 3 files changed, 47 insertions(+), 19 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt index cfb9690c9..57b08dcce 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt @@ -402,14 +402,20 @@ internal class OneSignalImp( if (isBackgroundThreadingEnabled) { waitForInit(operationName = "logout") - suspendifyOnIO { logoutHelper.logout() } } else { if (!isInitialized) { throw IllegalStateException("Must call 'initWithContext' before 'logout'") } + } + + val context = logoutHelper.switchUser() ?: return + + if (isBackgroundThreadingEnabled) { + suspendifyOnIO { logoutHelper.enqueueLogout(context) } + } else { Thread { runBlocking(runtimeIoDispatcher) { - logoutHelper.logout() + logoutHelper.enqueueLogout(context) } }.start() } @@ -666,6 +672,7 @@ internal class OneSignalImp( throw IllegalStateException("'initWithContext failed' before 'logout'") } - logoutHelper.logout() + val context = logoutHelper.switchUser() ?: return@withContext + logoutHelper.enqueueLogout(context) } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/LogoutHelper.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/LogoutHelper.kt index 8d9015c61..e0017d5b3 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/LogoutHelper.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/LogoutHelper.kt @@ -12,26 +12,43 @@ class LogoutHelper( private val configModel: ConfigModel, private val lock: Any, ) { - fun logout() { + internal data class LogoutEnqueueContext( + val appId: String, + val newOnesignalId: String, + ) + + /** + * Synchronously switches to a new device-scoped user under the login/logout lock + * so subsequent SDK calls (e.g. addTag) see the new anonymous user immediately. + * Returns context needed for [enqueueLogout], or null if no user was logged in + * (no switch needed). + */ + internal fun switchUser(): LogoutEnqueueContext? { synchronized(lock) { if (identityModelStore.model.externalId == null) { - return + return null } // Create new device-scoped user (clears external ID) userSwitcher.createAndSwitchToNewUser() - // Enqueue login operation for the new device-scoped user (no external ID) - operationRepo.enqueue( - LoginUserOperation( - configModel.appId, - identityModelStore.model.onesignalId, - null, - // No external ID for device-scoped user - ), - ) - // TODO: remove JWT Token for all future requests. + + return LogoutEnqueueContext(configModel.appId, identityModelStore.model.onesignalId) } } + + /** + * Enqueues the anonymous [LoginUserOperation] for the newly-created device-scoped user. + */ + internal fun enqueueLogout(context: LogoutEnqueueContext) { + operationRepo.enqueue( + LoginUserOperation( + context.appId, + context.newOnesignalId, + null, + // No external ID for device-scoped user + ), + ) + } } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/LogoutHelperTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/LogoutHelperTests.kt index 4921ed6bb..f78340868 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/LogoutHelperTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/LogoutHelperTests.kt @@ -53,7 +53,8 @@ class LogoutHelperTests : FunSpec({ ) // When - logoutHelper.logout() + val context = logoutHelper.switchUser() + if (context != null) logoutHelper.enqueueLogout(context) // Then - should return early without any operations verify(exactly = 0) { mockUserSwitcher.createAndSwitchToNewUser() } @@ -83,7 +84,8 @@ class LogoutHelperTests : FunSpec({ ) // When - logoutHelper.logout() + val context = logoutHelper.switchUser() + if (context != null) logoutHelper.enqueueLogout(context) // Then - should create new user and enqueue login operation for device-scoped user verify(exactly = 1) { mockUserSwitcher.createAndSwitchToNewUser() } @@ -122,7 +124,8 @@ class LogoutHelperTests : FunSpec({ ) // When - logoutHelper.logout() + val context = logoutHelper.switchUser() + if (context != null) logoutHelper.enqueueLogout(context) // Then - operations should happen in the correct order verifyOrder { @@ -157,7 +160,8 @@ class LogoutHelperTests : FunSpec({ val threads = (1..10).map { Thread { - logoutHelper.logout() + val context = logoutHelper.switchUser() + if (context != null) logoutHelper.enqueueLogout(context) } } From 9d6341a51abcf46747b52bfc146e5f94b89dbfd0 Mon Sep 17 00:00:00 2001 From: Nan Date: Wed, 22 Apr 2026 11:46:32 -0700 Subject: [PATCH 20/23] nit: update some comments for clarity --- .../core/internal/operations/impl/OperationRepo.kt | 9 ++++----- .../user/internal/operations/LoginUserOperation.kt | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt index 8bcf7a9cb..48333494f 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt @@ -32,7 +32,7 @@ internal class OperationRepo( ) : IOperationRepo, IStartableService { internal class OperationQueueItem( val operation: Operation, - var waiter: WaiterWithValue? = null, + var waiter: WaiterWithValue? = null, // waiter may transfer during operation de-dupe val bucket: Int, var retries: Int = 0, ) { @@ -162,10 +162,7 @@ internal class OperationRepo( return } - // Dedupe LoginUserOperation for the same user. Merge the incoming - // existingOnesignalId so the anon-user conversion isn't lost when - // RecoverFromDroppedLoginBug wins the race (it enqueues with null). - // Transfer the waiter so enqueueAndWait callers get the real result. + // Dedupe LoginUserOperation by onesignalId. val op = queueItem.operation if (op is LoginUserOperation) { val existing = @@ -174,12 +171,14 @@ internal class OperationRepo( } if (existing != null) { val existingOp = existing.operation as LoginUserOperation + // Preserve the anon-user conversion link if the queued op lacks it (e.g. RecoverFromDroppedLoginBug enqueued with null). if (op.existingOnesignalId != null && existingOp.existingOnesignalId == null) { Logging.debug("OperationRepo: internalEnqueue - merging existingOnesignalId=${op.existingOnesignalId} into queued LoginUserOperation for onesignalId: ${op.onesignalId}.") existingOp.existingOnesignalId = op.existingOnesignalId } else { Logging.debug("OperationRepo: internalEnqueue - LoginUserOperation for onesignalId: ${op.onesignalId} already exists in the queue.") } + // Transfer the waiter so enqueueAndWait callers see the queued op's real execution result. if (queueItem.waiter != null && existing.waiter == null) { existing.waiter = queueItem.waiter } else { diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt index 4b707788f..ca2f97945 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt @@ -48,7 +48,7 @@ class LoginUserOperation() : Operation(LoginUserOperationExecutor.LOGIN_USER) { */ var existingOnesignalId: String? get() = getOptStringProperty(::existingOnesignalId.name) - internal set(value) { + internal set(value) { // `internal` so OperationRepo can merge during de-dupe setOptStringProperty(::existingOnesignalId.name, value) } From e298b58c7d5dfa5b298e075ed642711a9c920c89 Mon Sep 17 00:00:00 2001 From: Nan Date: Wed, 22 Apr 2026 12:10:40 -0700 Subject: [PATCH 21/23] fix: skip dedup merge of local existingOnesignalId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guard the LoginUserOperation dedup+merge path with !IDManager.isLocalId. When upgrading from 5.0.0-5.1.7 with a dropped anon-user create, switchUser returns a local- existingOneSignalId. If RecoverFromDroppedLoginBug wins the race and its null-existingOnesignalId op is queued first, merging the local id flips canStartExecute to false and strands the op — the waiter (now transferred) never wakes, and the stuck op blocks future recovery. --- .../internal/operations/impl/OperationRepo.kt | 13 +++++++-- .../internal/operations/OperationRepoTests.kt | 29 +++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt index 48333494f..7237b1f16 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt @@ -1,5 +1,6 @@ package com.onesignal.core.internal.operations.impl +import com.onesignal.common.IDManager import com.onesignal.common.threading.WaiterWithValue import com.onesignal.core.internal.config.ConfigModelStore import com.onesignal.core.internal.operations.ExecutionResult @@ -172,9 +173,15 @@ internal class OperationRepo( if (existing != null) { val existingOp = existing.operation as LoginUserOperation // Preserve the anon-user conversion link if the queued op lacks it (e.g. RecoverFromDroppedLoginBug enqueued with null). - if (op.existingOnesignalId != null && existingOp.existingOnesignalId == null) { - Logging.debug("OperationRepo: internalEnqueue - merging existingOnesignalId=${op.existingOnesignalId} into queued LoginUserOperation for onesignalId: ${op.onesignalId}.") - existingOp.existingOnesignalId = op.existingOnesignalId + // Skip local ids: merging one would flip canStartExecute to false and strand the op, + // since a local id that never hit the backend will never receive an idTranslation. + val incomingExistingId = op.existingOnesignalId + if (incomingExistingId != null && + !IDManager.isLocalId(incomingExistingId) && + existingOp.existingOnesignalId == null + ) { + Logging.debug("OperationRepo: internalEnqueue - merging existingOnesignalId=$incomingExistingId into queued LoginUserOperation for onesignalId: ${op.onesignalId}.") + existingOp.existingOnesignalId = incomingExistingId } else { Logging.debug("OperationRepo: internalEnqueue - LoginUserOperation for onesignalId: ${op.onesignalId} already exists in the queue.") } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt index 5be6d1ba1..26e24dc7c 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt @@ -170,6 +170,35 @@ class OperationRepoTests : FunSpec({ merged.existingOnesignalId shouldBe "anon-uuid" } + test("enqueue dedupe does not merge a local existingOnesignalId onto the queued op") { + // Regression: when upgrading from 5.0.0-5.1.7, an anon user's backend-create + // may have been dropped, so its onesignalId is still "local-". If + // RecoverFromDroppedLoginBug wins the race (queued op has existingOnesignalId=null, + // canStartExecute=true), we must NOT overwrite it with the incoming op's local + // existingOnesignalId — doing so flips canStartExecute to false and strands the op + // forever (the local id will never receive an idTranslation). + val mocks = Mocks() + val operationRepo = mocks.operationRepo + + val queuedOp = LoginUserOperation("appId", "local-new", "alice", null) + queuedOp.id = UUID.randomUUID().toString() + synchronized(operationRepo.queue) { + operationRepo.queue.add(OperationQueueItem(queuedOp, bucket = 0)) + } + + val incomingOp = LoginUserOperation("appId", "local-new", "alice", "local-anon") + + // When + operationRepo.enqueue(incomingOp) + mocks.waitForInternalEnqueue() + + // Then — queue still has one op, existingOnesignalId stays null (canStartExecute stays true) + operationRepo.queue.size shouldBe 1 + val survivor = operationRepo.queue.first().operation as LoginUserOperation + survivor.existingOnesignalId shouldBe null + survivor.canStartExecute shouldBe true + } + test("containsInstanceOf") { // Given val mocks = Mocks() From dce31847abf447e53bb0eb780e02c6e64b721820 Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 27 Apr 2026 08:35:05 -0700 Subject: [PATCH 22/23] tests: cover OperationRepo dedupe when queued op already has a waiter When the queued LoginUserOperation carries a waiter, dedupe wakes the incoming enqueueAndWait caller immediately with true and the queued op's waiter receives the real execution result on SUCCESS. Existing dedupe coverage only exercised the waiter-transfer branch where the queued op had no waiter. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/operations/OperationRepoTests.kt | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt index 26e24dc7c..a9f7db4f5 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt @@ -29,6 +29,7 @@ import io.mockk.runs import io.mockk.slot import io.mockk.spyk import io.mockk.verify +import kotlinx.coroutines.CoroutineStart import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking @@ -199,6 +200,41 @@ class OperationRepoTests : FunSpec({ survivor.canStartExecute shouldBe true } + test("enqueue dedupe wakes both queued and incoming enqueueAndWait callers on SUCCESS") { + // A LoginUserOperation is queued via enqueueAndWait while the loop is not yet + // started, so it sits in the queue with its waiter attached. A second + // enqueueAndWait arrives for the same onesignalId. Dedupe wakes the incoming + // caller immediately with true; the queued op's waiter wakes with the real + // execution result when SUCCESS lands. + val mocks = Mocks() + val opRepo = mocks.operationRepo + val executeOperationsCall = mockExecuteOperations(opRepo) + + val queuedOp = LoginUserOperation("appId", "alice", "ext", "anon-uuid") + val incomingOp = LoginUserOperation("appId", "alice", "ext", "anon-uuid") + + // When — first enqueueAndWait runs (loop not started, op stays in queue). + // UNDISPATCHED so enqueueAndWait reaches its scope.launch + suspend before + // we send the incoming op below; otherwise the scope's single thread would + // see the incoming op's internalEnqueue first and dedupe direction reverses. + val queuedDone = WaiterWithValue() + launch(start = CoroutineStart.UNDISPATCHED) { + queuedDone.wake(opRepo.enqueueAndWait(queuedOp)) + } + mocks.waitForInternalEnqueue() + + // Incoming dedupes against the queued op and is woken immediately + val incomingResult = withTimeout(1_000) { opRepo.enqueueAndWait(incomingOp) } + // Run the loop; mocked executor SUCCESS wakes the queued op's waiter + opRepo.start() + executeOperationsCall.waitForWake() + val queuedResult = withTimeout(1_000) { queuedDone.waitForWake() } + + // Then + incomingResult shouldBe true + queuedResult shouldBe true + } + test("containsInstanceOf") { // Given val mocks = Mocks() From ea96c182715c8f40dfc9855cfb6baaadf03fe6cc Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 27 Apr 2026 16:05:38 -0700 Subject: [PATCH 23/23] nit: detekt --- OneSignalSDK/detekt/detekt-baseline-core.xml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/OneSignalSDK/detekt/detekt-baseline-core.xml b/OneSignalSDK/detekt/detekt-baseline-core.xml index 0530b2d0e..3f07ef1a1 100644 --- a/OneSignalSDK/detekt/detekt-baseline-core.xml +++ b/OneSignalSDK/detekt/detekt-baseline-core.xml @@ -179,6 +179,7 @@ LongMethod:LoginUserOperationExecutor.kt$LoginUserOperationExecutor$private suspend fun createUser( createUserOperation: LoginUserOperation, operations: List<Operation>, ): ExecutionResponse LongMethod:LoginUserOperationExecutor.kt$LoginUserOperationExecutor$private suspend fun loginUser( loginUserOp: LoginUserOperation, operations: List<Operation>, ): ExecutionResponse LongMethod:OperationRepo.kt$OperationRepo$internal suspend fun executeOperations(ops: List<OperationQueueItem>) + LongMethod:OperationRepo.kt$OperationRepo$private fun internalEnqueue( queueItem: OperationQueueItem, flush: Boolean, addToStore: Boolean, index: Int? = null, ) LongMethod:OutcomeEventsController.kt$OutcomeEventsController$private suspend fun sendAndCreateOutcomeEvent( name: String, weight: Float, // Note: this is optional sessionTime: Long, influences: List<Influence>, ): OutcomeEvent? LongMethod:OutcomeEventsController.kt$OutcomeEventsController$private suspend fun sendUniqueOutcomeEvent( name: String, sessionInfluences: List<Influence>, ): OutcomeEvent? LongMethod:OutcomeEventsRepository.kt$OutcomeEventsRepository$override suspend fun getAllEventsToSend(): List<OutcomeEventParams> @@ -609,6 +610,9 @@ UnusedPrivateMember:OperationRepo.kt$OperationRepo$private val _time: ITime UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("'initWithContext failed' before 'login'") UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("'initWithContext failed' before 'logout'") + UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("Must call 'initWithContext' before 'login'") + UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("Must call 'initWithContext' before 'logout'") + UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("Must call 'initWithContext' before use") UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw initFailureException ?: IllegalStateException("Initialization failed. Cannot proceed.")