From 016c3eadb6db49aba4c99842dbc385814764f1d3 Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 09:59:15 +0100 Subject: [PATCH 01/17] Update Gradle wrapper and dependency versions - Gradle 9.5.0 -> 9.6.1 (wrapper scripts regenerated) - Gradle plugin 9.2.1 -> 9.3.1 - Kotlin 2.4.0 -> 2.4.10 - Kover 0.9.8 -> 0.9.9 - lifecycle-runtime 2.10.0 -> 2.11.0 - Compose BOM 2026.05.01 -> 2026.06.01 Co-Authored-By: Claude Opus 5 (1M context) --- gradle/libs.versions.toml | 10 +++++----- gradle/wrapper/gradle-wrapper.properties | 2 +- gradlew | 4 ++-- gradlew.bat | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index bd92e6e..7a7fd7e 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -1,19 +1,19 @@ [versions] compileSdk = "37" targetSdk = "37" -gradle = "9.2.1" -kotlin = "2.4.0" +gradle = "9.3.1" +kotlin = "2.4.10" dokka = "2.2.0" -kover = "0.9.8" +kover = "0.9.9" dataStore = "1.2.1" mockitoCore = "5.23.0" mockitoKotlin = "6.3.0" robolectric = "4.16.1" coreKtx = "1.19.0" appcompat = "1.7.1" -lifecycleRuntime = "2.10.0" +lifecycleRuntime = "2.11.0" activityCompose = "1.13.0" -composeBom = "2026.05.01" +composeBom = "2026.06.01" testExtJunit = "1.3.0" espressoCore = "3.7.0" junit = "4.13.2" diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties index f193d72..8747320 100644 --- a/gradle/wrapper/gradle-wrapper.properties +++ b/gradle/wrapper/gradle-wrapper.properties @@ -1,6 +1,6 @@ distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists -distributionUrl=https\://services.gradle.org/distributions/gradle-9.5.0-all.zip +distributionUrl=https\://services.gradle.org/distributions/gradle-9.6.1-all.zip networkTimeout=10000 retries=0 retryBackOffMs=500 diff --git a/gradlew b/gradlew index b9bb139..249efbb 100755 --- a/gradlew +++ b/gradlew @@ -20,7 +20,7 @@ ############################################################################## # -# Gradle start up script for POSIX generated by Gradle. +# gradlew start up script for POSIX generated by Gradle. # # Important for running: # @@ -29,7 +29,7 @@ # bash, then to run this script, type that shell name before the whole # command line, like: # -# ksh Gradle +# ksh gradlew # # Busybox and similar reduced shells will NOT work, because this script # requires all of these POSIX shell features: diff --git a/gradlew.bat b/gradlew.bat index 24c62d5..a51ec4f 100644 --- a/gradlew.bat +++ b/gradlew.bat @@ -19,7 +19,7 @@ @if "%DEBUG%"=="" @echo off @rem ########################################################################## @rem -@rem Gradle startup script for Windows +@rem gradlew startup script for Windows @rem @rem ########################################################################## @@ -72,7 +72,7 @@ echo location of your Java installation. 1>&2 -@rem Execute Gradle +@rem Execute gradlew @rem endlocal doesn't take effect until after the line is parsed and variables are expanded @rem which allows us to clear the local environment before executing the java command endlocal & "%JAVA_EXE%" %DEFAULT_JVM_OPTS% %JAVA_OPTS% %GRADLE_OPTS% "-Dorg.gradle.appname=%APP_BASE_NAME%" -jar "%APP_HOME%\gradle\wrapper\gradle-wrapper.jar" %* & call :exitWithErrorLevel From 5c4f1dd15bf21c123c08e10d386ac3a15498c5ae Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 10:16:45 +0100 Subject: [PATCH 02/17] Split coroutineContext into dispatcher + injectable scope, inject DataStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements COROUTINE-API-REDESIGN.md, taking the wider of the two options so the testability goal is actually delivered rather than deferred. One constructor parameter was doing two unrelated jobs: choosing the dispatcher for suspend functions AND owning fire-and-forget launches. Splitting them fixes three problems: - suspend functions no longer reparent onto a foreign Job, so clearPrefs() and the suspending writes are cancelled by their caller as they should be - the default SupervisorJob is now per-instance; the process-wide companion vals on both classes are deleted - consumers can inject a scope, so async write failures can reach a CoroutineExceptionHandler instead of the default handler BaseDataStoreHelper now has two constructors. The primary takes the DataStore directly, which is what makes subclasses unit-testable; the convenience constructor keeps the old (context, preferenceName) signature and builds the store via PreferenceDataStoreFactory. Existing subclasses compile unchanged — the 1,621-line existing suite passes untouched. readValueBlocking drops its runBlocking context: DataStore is already main-safe, so the IO hop bought nothing and the Job it carried made the read cancellable process-wide by accident. Adds BaseDataStoreHelperInjectionTest covering the injected DataStore, the injected scope, per-instance scope isolation, and caller-cancellable clearPrefs on both helpers. The two cancellation tests were verified against a mutated build: both fail under the old foreign-Job behaviour. BREAKING CHANGE: coroutineContext -> dispatcher (+ optional scope), and the companion supervisorJob properties are gone. See "Migrating to 2.0" in the README — in particular, any logout-style sequence that relied on prefs writes surviving its caller's cancellation now needs an application-lifetime scope or withContext(NonCancellable). Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 6 +- COROUTINE-API-REDESIGN.md | 272 ++++++++++++++ .../duck/prefshelper/BaseDataStoreHelper.kt | 94 +++-- .../com/duck/prefshelper/BasePrefsHelper.kt | 21 +- README.md | 116 ++++++ app/build.gradle.kts | 1 + .../app/BaseDataStoreHelperInjectionTest.kt | 340 ++++++++++++++++++ gradle/libs.versions.toml | 2 + 8 files changed, 799 insertions(+), 53 deletions(-) create mode 100644 COROUTINE-API-REDESIGN.md create mode 100644 app/src/test/java/com/duck/app/BaseDataStoreHelperInjectionTest.kt diff --git a/CLAUDE.md b/CLAUDE.md index 8b114d8..e94633c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -43,7 +43,7 @@ The library provides two abstract base classes that consumers extend: - Wraps Android `SharedPreferences` with type-safe getters/setters - Synchronous API for reads, async writes via `edit {}` - Subclasses must provide `sharedPreferences` instance -- Uses coroutine context (defaults to `Dispatchers.IO + SupervisorJob`) for `clearPrefs()` +- Takes a `dispatcher: CoroutineDispatcher = Dispatchers.IO` (no `Job`), used by `clearPrefs()`. Because the dispatcher carries no Job, `clearPrefs()` is cancelled when its caller is cancelled — this is deliberate, see the 2.0 migration note in the README. - Preferred usage is the `*Pref` property delegate factories (e.g. `var flag by booleanPref(KEY, defaultValue = false)`). Each type has a non-nullable overload `(key, defaultValue)` and a nullable overload `(key)`. Temporal types (`Date`, `LocalDateTime`, `LocalDate`, `LocalTime`) are nullable-only and preserve the existing -1L sentinel. ### BaseDataStoreHelper @@ -51,7 +51,9 @@ The library provides two abstract base classes that consumers extend: - Wraps Jetpack `DataStore` with type-safe methods - Returns `Flow` for reactive reads, plus blocking `readXxxValue()` methods (2s timeout) - Both suspend and async write methods. The `writeXxxAsync` methods launch on `scope` and **return the `Job`**, so callers (and tests) can `.join()` to await completion instead of guessing with a delay. Delegate setters discard the `Job` (property setters return `Unit`), so they remain genuinely fire-and-forget. -- Subclasses pass `Context` and preference name to constructor +- **Two constructors.** Primary takes `(dataStore: DataStore, dispatcher, scope)` — the injectable path, used by tests. Secondary convenience constructor takes `(context, preferenceName, dispatcher, scope)` and builds the store via `PreferenceDataStoreFactory.create(produceFile = { context.applicationContext.preferencesDataStoreFile(name) })`. Subclasses written against the old `(context, name)` signature are unchanged. +- `dispatcher` (no `Job`) is where `suspend` functions work; `scope` is where the `*Async` writes launch and is injectable so consumers can supply an app-lifetime scope with a `CoroutineExceptionHandler`. The default `scope` is per-instance — there is no longer a shared companion `SupervisorJob`. +- As with DataStore itself, only one live instance per backing file per process — keep subclasses singletons. - Null values remove the key from storage - Preferred usage is the `*Pref` delegate + `*PrefFlow` alias pair (e.g. `var userId by intPref(KEY, defaultValue = -1)` paired with `val userIdFlow = intPrefFlow(KEY, defaultValue = -1)`). Delegate setters route through the existing `*Async` writes so callers don't need to build their own `CoroutineScope`. diff --git a/COROUTINE-API-REDESIGN.md b/COROUTINE-API-REDESIGN.md new file mode 100644 index 0000000..c6115d1 --- /dev/null +++ b/COROUTINE-API-REDESIGN.md @@ -0,0 +1,272 @@ +# Coroutine API redesign — injectable scope + correct cancellation + +**Status:** implemented on `feature/coroutine-api-redesign`, 2026-07-31 — see [Outcome](#outcome) +**Drafted:** 2026-07-30 +**Provenance:** came out of a main-thread-safety audit of the repo layer in +`AIM-Capture-Android`. Line references below were verified against the working tree on that date — +re-check them if the files have moved on. Design was reviewed by a second opinion; the two "gotcha" +sections exist because that review found problems with the first draft. + +--- + +## Outcome + +Built as **scope + injectable DataStore** — the wider of the two options in +[The testability catch](#the-testability-catch), so goal 2 is genuinely delivered rather than +deferred. Everything below is the original proposal, kept as the design record. Where the +implementation deviates from it: + +| Proposal said | Built instead | Why | +|---|---|---| +| `dataStore: DataStore? = null` param alongside `context` | **Two constructors** — primary `(dataStore, dispatcher, scope)`, secondary `(context, preferenceName, dispatcher, scope)` | `context` is used for nothing else in the class, so requiring it on the injected path is dead weight. Existing `super(context, "name")` subclasses stay source-compatible. | +| Keep the `Context.dataStoreInstance` extension delegate | Deleted; the convenience constructor uses `PreferenceDataStoreFactory.create(produceFile = { context.applicationContext.preferencesDataStoreFile(name) })` | The delegate's singleton guard was per *helper instance*, not process-wide, so it was never buying the protection it looked like it was. `SingleProcessDataStore`'s own file-lock check still catches duplicate instances. | +| `@PublishedApi internal val dispatcher` on **both** classes | `@PublishedApi internal` on `BaseDataStoreHelper`, plain `protected` on `BasePrefsHelper` | No `inline` function in `BasePrefsHelper` touches it, so `protected` is both sufficient and the smaller diff from the status quo. | + +Two things the proposal flagged that turned out fine: the existing 1,621-line test suite passes +**unchanged** against the new API, and `scope` moving from a body property to a constructor property +does not reintroduce the `IllegalAccessError` gotcha (that only bites members captured inside an +anonymous object emitted by an inline function — a direct `this.scope` read in an inlined +`protected inline` body is legal). + +Still outstanding, and deliberately out of this repo: the AIM app's logout call site (below), and +the version bump + JitPack tag. + +--- + +## Goals + +1. **Injectable scope.** Consumers should be able to hand in a `CoroutineScope` they own — e.g. an + application-lifetime scope from Koin with a `CoroutineExceptionHandler` — so that fire-and-forget + prefs writes report failures instead of crashing the process. +2. **Testability.** Subclasses should be unit-testable with deterministic coroutines. + +Goal 1 is achievable with the change below. **Goal 2 is not, without an extra step** — see +[The testability catch](#the-testability-catch). Read that before estimating. + +--- + +## Diagnosis + +One constructor parameter is doing two unrelated jobs: + +```kotlin +// BasePrefsHelper.kt:36-37 and BaseDataStoreHelper.kt:52-55 +coroutineContext: CoroutineContext = Dispatchers.IO + supervisorJob +``` + +It simultaneously decides: + +- **(a)** which dispatcher `suspend` functions hop to — `withContext(coroutineContext)`, at + `BasePrefsHelper.kt:47` and `BaseDataStoreHelper.kt:108`, `:159` +- **(b)** which Job owns fire-and-forget launches — `BaseDataStoreHelper.kt:74`, + `protected val scope = CoroutineScope(coroutineContext)`, used by the `*Async` helpers at `:129`, + `:148`, `:173` + +Conflating them causes three concrete problems. + +### 1. `suspend` functions ignore their caller's cancellation + +`withContext(context)` where `context` carries a Job **reparents** the block onto that Job instead of +the caller's. So `clearPrefs()` and `writeValue()` behave like `NonCancellable` with respect to +whoever called them. A `suspend` function should be cancellable by its caller; this one isn't. + +A `suspend` function should only ever be handed a **dispatcher** — no Job. + +### 2. The default `SupervisorJob` is shared process-wide + +```kotlin +// BasePrefsHelper.kt:461 and BaseDataStoreHelper.kt:1184 (companion objects) +val supervisorJob = SupervisorJob() +``` + +A companion `val` means **one Job shared across every instance of that class in the process**. Cancel +it once and every prefs helper in the app silently stops doing async work, permanently, with no +error. Nothing cancels it today, so it currently behaves as a harmless keep-alive parent — but it is +a live footgun, and it makes the blocking reads at `:212` cancellable process-wide by accident. + +### 3. There is no way to inject a scope + +Which is the thing that prompted all this. A consumer can pass a context, but then it also +(unavoidably) changes where `suspend` functions dispatch, and it still can't attach an exception +handler to the launches without also reparenting the suspend calls. + +--- + +## Proposed API + +Split the parameter in two. + +```kotlin +abstract class BaseDataStoreHelper( + context: Context, + preferenceName: String, + @PublishedApi internal val dispatcher: CoroutineDispatcher = Dispatchers.IO, + protected val scope: CoroutineScope = CoroutineScope(dispatcher + SupervisorJob()), +) { +``` + +```kotlin +abstract class BasePrefsHelper( + @PublishedApi internal val dispatcher: CoroutineDispatcher = Dispatchers.IO, +) { +``` + +- `dispatcher` — used by `suspend` functions via `withContext(dispatcher)`. No Job, so caller + cancellation propagates correctly. +- `scope` — used for fire-and-forget launches. Injectable; this is the goal-1 payload. +- The default `SupervisorJob()` becomes **per-instance**, replacing the shared companion one. +- A default parameter referencing an earlier parameter (`scope` defaulting to an expression over + `dispatcher`) is legal Kotlin and compiles. + +`BasePrefsHelper` has no launches and no `scope` today, so it only needs `dispatcher`. Adding a +`scope` there too is optional — do it only if you want symmetry, not because anything needs it. + +### Visibility constraint + +`dispatcher` is touched by `protected inline` functions (`writeValue` at `:159`, the +`readValueBlocking` family at `:211`/`:225`/`:235`). Keeping it `@PublishedApi internal` — matching +today's `coroutineContext` — is the safe option. `protected` would also satisfy a `protected inline` +caller, but `@PublishedApi internal` is a smaller diff from the status quo. `scope` stays +`protected` and already works from the `protected inline` `*Async` helpers. + +--- + +## Change list + +### `BaseDataStoreHelper.kt` + +| Line | Now | Becomes | +|------|-----|---------| +| 52-55 | `coroutineContext: CoroutineContext = Dispatchers.IO + supervisorJob` | `dispatcher` + `scope` params as above | +| 74 | `protected val scope = CoroutineScope(coroutineContext)` | delete — `scope` is now a constructor property | +| 108 | `clearPrefs() = withContext(coroutineContext)` | `withContext(dispatcher)` | +| 159 | `writeValue(...) = withContext(coroutineContext)` | `withContext(dispatcher)` | +| 212 | `runBlocking(coroutineContext) { withTimeoutOrNull(2.seconds) { … } }` | `runBlocking { withTimeoutOrNull(2.seconds) { … } }` — drop the context entirely | +| 1184 | companion `val supervisorJob = SupervisorJob()` | delete (after checking for consumer references) | + +On `:212` specifically: DataStore is already main-safe, so the IO dispatch buys nothing, and passing +a Job into `runBlocking` is the footgun described above. + +### `BasePrefsHelper.kt` + +| Line | Now | Becomes | +|------|-----|---------| +| 36-37 | `coroutineContext: CoroutineContext = Dispatchers.IO + supervisorJob` | `dispatcher: CoroutineDispatcher = Dispatchers.IO` | +| 47 | `clearPrefs() = withContext(coroutineContext)` | `withContext(dispatcher)` | +| 461 | companion `val supervisorJob = SupervisorJob()` | delete (after checking for consumer references) | + +Also update the class KDoc on both — it currently documents the `coroutineContext` parameter and the +shared-`supervisorJob` default. + +--- + +## The testability catch + +**Injecting a scope does not, on its own, make DataStore-backed subclasses unit-testable.** + +`preferencesDataStore` creates its **own** internal `Dispatchers.IO` scope and performs real file +I/O. An injected `TestScope` with virtual time cannot make `dataStore.edit { }` deterministic, +because the work doesn't run on the scope we injected. So the change above delivers goal 1 and +quietly fails goal 2. + +To actually get goal 2, the **`DataStore` instance itself must be injectable**: either a constructor +parameter defaulting to the current delegate, or constructed via +`PreferenceDataStoreFactory.create(scope = …)` so tests can supply their own. + +**Decision required.** Two options: + +- **Scope-only (recommended first pass).** Smaller, contained, delivers the error-handling and + Koin-ownership win. Be explicit that unit tests aren't arriving in this round. +- **Scope + injectable DataStore.** Delivers real tests, but it is a wider API change and a bigger + piece of work. Reasonable as a follow-up once the split above has landed. + +`BasePrefsHelper` (SharedPreferences) does not have this problem — it's synchronous and already +testable with a fake `SharedPreferences`. + +--- + +## Consumer impact + +### Known consumers + +- `AIM-Capture-Android` — `NormalPrefs`, `DevicePrefs` (both `BasePrefsHelper`), `NormalDataStore` + (`BaseDataStoreHelper`), and `CameraDataStore` in the `:insta360Helper` module. +- Other Appoly apps that depend on `PrefsHelperBase` — enumerate before releasing. + +Most subclasses **don't pass the parameter at all** and rely on the default, so they are unaffected +by the signature change. Only call sites that explicitly pass `coroutineContext` need editing. + +### Required change at the logout call site — do not skip this + +Fixing problem 1 (making `suspend` functions properly caller-cancellable) has a real consequence. +`PrefsHelper.onLogout()` in the AIM app runs four sequential clears: + +```kotlin +suspend fun onLogout() { + normalPrefs.clearPrefs() + normalDataStore.clearPrefs() + RetrofitClient.resetClients() + cameraDataStore.clearPrefs() +} +``` + +Today the foreign-Job `withContext` makes those effectively uncancellable, which is accidentally why +logout always completes. Its caller is `SettingsScreenModel.logout()` running on `screenModelScope`, +which **is cancelled when the settings screen is disposed**. After this change, navigating away +mid-logout could leave SharedPreferences cleared but DataStore and the Retrofit clients not — a +partially logged-out state. + +**Fix it in the same release:** run logout on an application-lifetime scope (the AIM app now has one +in `appModule` as `named("appScope")`), or wrap the body in `withContext(NonCancellable)`. Pick one +deliberately — this is a genuine behaviour change, not a refactor. + +### Direct `supervisorJob` references + +`grep` all consumers for `supervisorJob` before deleting the companion properties. Nothing is +expected to reference them, but confirm rather than assume. + +--- + +## Release strategy + +**Major version bump with a migration note. No deprecated back-compat shim.** + +A secondary constructor preserving the old `coroutineContext` parameter would have to derive a +dispatcher back out of a `CoroutineContext` — exactly the fragile extraction this split exists to +avoid — and you own every known consumer, so the compatibility burden isn't worth carrying. + +The migration note should cover: + +1. `coroutineContext` → `dispatcher` (+ optional `scope`). +2. `suspend` functions are now caller-cancellable — audit anywhere you relied on a prefs write + surviving the caller's cancellation, and see the logout note above. +3. The shared `supervisorJob` is gone; the default Job is now per-instance. + +--- + +## Checklist + +- [x] Decide scope-only vs scope + injectable DataStore — chose **scope + injectable DataStore** +- [x] `BaseDataStoreHelper`: split constructor, delete the body `scope`, fix `clearPrefs`, nullable `writeValue`, `readValueBlocking` +- [x] `BasePrefsHelper`: `dispatcher` param, fix `clearPrefs` +- [x] Delete both companion `supervisorJob` properties (grepped — no references in this repo) +- [x] Update class KDoc on both +- [x] Update the library's own `app/` sample module if it passes a context — it doesn't; no change needed +- [x] Grep consumers for `coroutineContext` / `supervisorJob` usage — clean within this repo +- [ ] Fix the AIM app's logout call site in the same release — **not in this repo** +- [x] Migration note in README (`## Migrating to 2.0`) +- [ ] Major version bump, tag + JitPack release, then bump `prefshelperVersion` in consumers +- [x] Deterministic tests proving the injectable DataStore delivers goal 2 + +--- + +## Notes for whoever picks this up + +There is an **uncommitted** change in `AIM-Capture-Android` +(`data/local/prefs/NormalDataStore.kt`) that works around the current API by passing a custom +`coroutineContext` to the superclass to attach an exception handler. It is a stopgap and becomes +obsolete once this redesign lands — expect to delete it and inject the app scope instead. + +One thing that is **not** a defect, despite looking like one: `withContext(coroutineContext)` is a +real dispatcher switch, not a no-op. `coroutineContext` is a constructor property that *shadows* +`kotlin.coroutines.coroutineContext`. This was misdiagnosed once already. diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt index f06190a..2aa1fef 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt @@ -11,9 +11,10 @@ import androidx.datastore.preferences.core.doublePreferencesKey import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.intPreferencesKey import androidx.datastore.preferences.core.longPreferencesKey +import androidx.datastore.preferences.core.PreferenceDataStoreFactory import androidx.datastore.preferences.core.stringPreferencesKey -import androidx.datastore.preferences.preferencesDataStore -import com.duck.prefshelper.BaseDataStoreHelper.Companion.supervisorJob +import androidx.datastore.preferences.preferencesDataStoreFile +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job @@ -29,7 +30,6 @@ import kotlin.time.Duration.Companion.seconds import java.time.LocalDate import java.time.LocalDateTime import java.time.LocalTime -import kotlin.coroutines.CoroutineContext import kotlin.properties.ReadWriteProperty import kotlin.reflect.KProperty @@ -40,38 +40,55 @@ import kotlin.reflect.KProperty * Provides generic methods to read and write various data types. * Includes methods for blocking reads with timeouts. * - * BaseDataStoreHelper uses a coroutine context for background operations. - * By default, it uses [Dispatchers.IO] + [supervisorJob]. If you need custom job management, - * pass a context with your own Job or SupervisorJob. Avoid passing a new Job per instance - * unless you manage its lifecycle explicitly. + * Coroutine usage is split across two independent parameters: * - * @param context Application context - * @param preferenceName Name of the preferences data store - * @param coroutineContext Coroutine context for async operations + * - [dispatcher] decides where `suspend` functions do their work ([withContext]). It carries no + * [Job], so cancelling the caller correctly cancels the work. + * - [scope] owns the fire-and-forget `*Async` writes. Inject an application-lifetime scope — for + * example one provided by your DI container with a `CoroutineExceptionHandler` attached — if you + * want async write failures reported rather than thrown into a bare [SupervisorJob]. + * + * The default [scope] is created per instance, so cancelling one helper's scope never affects + * another's. + * + * Each instance owns exactly one [DataStore]. As with the DataStore library itself, there must + * only ever be a single active [DataStore] per file in a process — keep subclasses singletons. + * + * @param dataStore The [DataStore] backing this helper + * @param dispatcher The [CoroutineDispatcher] `suspend` functions run on, defaults to [Dispatchers.IO] + * @param scope The [CoroutineScope] the `*Async` writes are launched in, defaults to a per-instance + * scope of [dispatcher] + [SupervisorJob] */ abstract class BaseDataStoreHelper( - context: Context, - preferenceName: String, - @PublishedApi internal val coroutineContext: CoroutineContext = Dispatchers.IO + supervisorJob + @PublishedApi internal val dataStore: DataStore, + @PublishedApi internal val dispatcher: CoroutineDispatcher = Dispatchers.IO, + protected val scope: CoroutineScope = CoroutineScope(dispatcher + SupervisorJob()), ) { /** - * DataStore instance - */ - private val Context.dataStoreInstance: DataStore by preferencesDataStore(name = preferenceName) - - /** - * DataStore reference - * - * Internal visibility allows testing while keeping it hidden from library consumers. - * This is accessed by inline functions which require it to be internal or public. - */ - @PublishedApi - internal val dataStore: DataStore = context.dataStoreInstance - - /** - * Coroutine scope for async operations - */ - protected val scope = CoroutineScope(coroutineContext) + * Creates a helper backed by a Preferences DataStore file named [preferenceName]. + * + * This is the convenience path for production code. To supply your own [DataStore] — for + * deterministic unit tests, or to configure migrations or a corruption handler — use the + * primary constructor instead. + * + * @param context Any [Context]; the application context is used internally + * @param preferenceName Name of the preferences data store file + * @param dispatcher The [CoroutineDispatcher] `suspend` functions and the DataStore's own IO run on + * @param scope The [CoroutineScope] the `*Async` writes are launched in + */ + constructor( + context: Context, + preferenceName: String, + dispatcher: CoroutineDispatcher = Dispatchers.IO, + scope: CoroutineScope = CoroutineScope(dispatcher + SupervisorJob()), + ) : this( + dataStore = PreferenceDataStoreFactory.create( + scope = CoroutineScope(dispatcher + SupervisorJob()), + produceFile = { context.applicationContext.preferencesDataStoreFile(preferenceName) }, + ), + dispatcher = dispatcher, + scope = scope, + ) //Todo: remove Deprecated methods in future release /** @@ -105,7 +122,7 @@ abstract class BaseDataStoreHelper( * * @return Unit */ - open suspend fun clearPrefs(): Unit = withContext(coroutineContext) { + open suspend fun clearPrefs(): Unit = withContext(dispatcher) { dataStore.edit { preferences -> preferences.clear() } @@ -156,7 +173,7 @@ abstract class BaseDataStoreHelper( * @param value The value to store or null to delete */ @JvmName("writeNullableValue") - protected suspend inline fun writeValue(key: Preferences.Key, value: T?) = withContext(coroutineContext) { + protected suspend inline fun writeValue(key: Preferences.Key, value: T?) = withContext(dispatcher) { if (value == null) removeKey(key) else writeValue(key, value) } @@ -205,11 +222,18 @@ abstract class BaseDataStoreHelper( /** * Generic method to read value from the data store in a blocking way * + * Blocks the calling thread until the read completes or 2 seconds elapse. The read itself runs + * on the [DataStore]'s own scope, so no dispatcher is imposed here. + * + * Never call this from a thread that the [DataStore]'s scope is itself confined to (for example + * an injected store built on a single-threaded test dispatcher) — the read cannot make progress + * and the call will stall for the full timeout and return null. + * * @param key The key to read the value for * @return The value or null if not present */ protected inline fun readValueBlocking(key: Preferences.Key): T? = - runBlocking(coroutineContext) { + runBlocking { withTimeoutOrNull(2.seconds) { dataStore.data.first()[key] } @@ -1179,8 +1203,4 @@ abstract class BaseDataStoreHelper( * [Flow] accessor for a nullable [Enum] preference — alias for [readEnum]. */ protected inline fun > enumPrefFlow(key: String): Flow = readEnum(key) - - companion object { - val supervisorJob = SupervisorJob() - } } diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt index 93ccae3..80a8550 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt @@ -5,9 +5,8 @@ import android.os.Build import android.util.Log import androidx.annotation.RequiresApi import androidx.core.content.edit -import com.duck.prefshelper.BasePrefsHelper.Companion.supervisorJob +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.withContext import java.time.LocalDate import java.time.LocalDateTime @@ -16,7 +15,6 @@ import java.time.ZoneOffset import java.util.Date import kotlin.contracts.ExperimentalContracts import kotlin.contracts.contract -import kotlin.coroutines.CoroutineContext import kotlin.properties.ReadWriteProperty import kotlin.reflect.KProperty @@ -26,15 +24,14 @@ import kotlin.reflect.KProperty * Provides common functionality for storing and retrieving preferences of various types * including String, Int, Long, Date, Boolean, LocalDateTime, LocalDate, LocalTime, and Enum. * - * BasePrefsHelper uses a coroutine context for background operations. - * By default, it uses [Dispatchers.IO] + [supervisorJob]. If you need custom job management, - * pass a context with your own Job or SupervisorJob. Avoid passing a new Job per instance - * unless you manage its lifecycle explicitly. + * Reads and writes are synchronous; only [clearPrefs] does blocking work, and it hops to + * [dispatcher] to do it. The dispatcher carries no [kotlinx.coroutines.Job], so cancelling the + * caller correctly cancels the clear. * - * @param coroutineContext The [CoroutineContext] to use for suspend functions, defaults to [Dispatchers.IO] with a [SupervisorJob] + * @param dispatcher The [CoroutineDispatcher] `suspend` functions run on, defaults to [Dispatchers.IO] */ abstract class BasePrefsHelper( - protected val coroutineContext: CoroutineContext = Dispatchers.IO + supervisorJob + protected val dispatcher: CoroutineDispatcher = Dispatchers.IO, ) { /** * The [SharedPreferences] instance to use @@ -44,7 +41,7 @@ abstract class BasePrefsHelper( /** * Clear all preferences */ - open suspend fun clearPrefs(): Unit = withContext(coroutineContext) { + open suspend fun clearPrefs(): Unit = withContext(dispatcher) { sharedPreferences.edit(commit = true) { clear() } } @@ -456,8 +453,4 @@ abstract class BasePrefsHelper( override fun setValue(thisRef: Any?, property: KProperty<*>, value: T?) = setEnum(key, value) } } - - companion object { - val supervisorJob = SupervisorJob() - } } \ No newline at end of file diff --git a/README.md b/README.md index 0b34bdc..971aed5 100644 --- a/README.md +++ b/README.md @@ -63,6 +63,70 @@ class AppPrefs(context: Context) : BaseDataStoreHelper(context, "app_prefs") { } ``` +### Coroutines: `dispatcher` and `scope` + +Both base classes take a `dispatcher`; `BaseDataStoreHelper` also takes a `scope`. They do two separate jobs and neither defaults will surprise you: + +| Parameter | Used for | Default | +|-----------|----------|---------| +| `dispatcher` | Where `suspend` functions do their work (`withContext(dispatcher)`) | `Dispatchers.IO` | +| `scope` | Where fire-and-forget `*Async` writes (and the delegate setters) launch | Per-instance `CoroutineScope(dispatcher + SupervisorJob())` | + +`dispatcher` deliberately carries no `Job`, so a `suspend` call like `clearPrefs()` is cancelled when its caller is cancelled — see the migration note below, this matters. + +Inject `scope` when you want async write failures to go somewhere. A bare `SupervisorJob` swallows nothing but has no handler, so an exception from a fire-and-forget write reaches the thread's default handler and takes the process with it. Hand in an application-lifetime scope with a `CoroutineExceptionHandler` instead: + +```kotlin +// Koin, but any DI container works the same way +single(named("appScope")) { + CoroutineScope( + SupervisorJob() + Dispatchers.IO + CoroutineExceptionHandler { _, t -> + Crashlytics.recordException(t) + }, + ) +} + +single { AppPrefs(get(), get(named("appScope"))) } + +class AppPrefs( + context: Context, + appScope: CoroutineScope, +) : BaseDataStoreHelper(context, "app_prefs", scope = appScope) { + // … +} +``` + +### Injecting a `DataStore` for tests + +`BaseDataStoreHelper`'s primary constructor takes the `DataStore` directly, so tests can supply one backed by a temp file and a test scheduler and drop the sleep-and-hope pattern entirely. Forward both constructors from your subclass: + +```kotlin +class AppPrefs : BaseDataStoreHelper { + constructor(context: Context) : super(context, "app_prefs") + + constructor( + dataStore: DataStore, + dispatcher: CoroutineDispatcher, + scope: CoroutineScope, + ) : super(dataStore, dispatcher, scope) + + var userId by intPref(KEY_USER_ID, defaultValue = -1) + // … +} +``` + +Then, in a test: + +```kotlin +val store = PreferenceDataStoreFactory.create( + scope = CoroutineScope(UnconfinedTestDispatcher(testScheduler) + Job()), + produceFile = { tempFolder.newFile("test.preferences_pb") }, +) +val prefs = AppPrefs(store, StandardTestDispatcher(testScheduler), backgroundScope) +``` + +Give every test a fresh file — DataStore throws if two live instances point at the same one — and cancel the store's scope in teardown to release the file lock. + ### Composing a single façade When an app stores preferences across several backing files (e.g. a per-user SharedPreferences plus a device-wide DataStore), it's convenient to wrap them in one façade so consumers only inject one type and cross-cutting operations (like "clear everything on logout") live in one place. Each property on the façade can re-expose a sub-helper's delegate via a Kotlin property reference (`by subHelper::property`): @@ -93,6 +157,58 @@ Both `BasePrefsHelper` and `BaseDataStoreHelper` support `String`, `Int`, `Long` Each type exposes a non-nullable delegate `*Pref(key, defaultValue)` and a nullable delegate `*Pref(key)` (assigning `null` clears the stored value on DataStore, or stores the sentinel on SharedPreferences for temporal types). `BaseDataStoreHelper` additionally exposes matching `*PrefFlow` accessors for reactive reads. +## Migrating to 2.0 + +2.0 splits the single `coroutineContext` constructor parameter, which was doing two unrelated jobs, into a `dispatcher` and a `scope`. There is no back-compat shim: deriving a dispatcher back out of an arbitrary `CoroutineContext` is exactly the fragile guesswork this change exists to remove. + +**Most subclasses need no change at all.** If you extend `BasePrefsHelper()` or `BaseDataStoreHelper(context, "name")` without passing a context, you are already on the new defaults. + +### 1. `coroutineContext` → `dispatcher` (+ optional `scope`) + +```kotlin +// Before +class AppPrefs(context: Context) : BaseDataStoreHelper( + context, "app_prefs", Dispatchers.IO + SupervisorJob() + handler, +) + +// After +class AppPrefs(context: Context, appScope: CoroutineScope) : BaseDataStoreHelper( + context, "app_prefs", dispatcher = Dispatchers.IO, scope = appScope, +) +``` + +Attaching a `CoroutineExceptionHandler` is now the `scope`'s job, not the dispatcher's — which is the point. + +### 2. `suspend` functions are now caller-cancellable — audit your call sites + +This is a real behaviour change, not a refactor. Previously `withContext(coroutineContext)` reparented the work onto a foreign `Job`, so `clearPrefs()` and the suspending writes behaved like `NonCancellable`: they finished even if the caller was cancelled. They no longer do. + +The failure mode this creates is a partial logout. Given: + +```kotlin +suspend fun onLogout() { + userPrefs.clearPrefs() + appPrefs.clearPrefs() + apiClient.reset() +} +``` + +if that runs on a screen-scoped coroutine (`viewModelScope`, `screenModelScope`) and the user navigates away mid-call, you can now end up with SharedPreferences cleared and DataStore not. Fix it deliberately — either run logout on an application-lifetime scope, or wrap the body: + +```kotlin +suspend fun onLogout() = withContext(NonCancellable) { … } +``` + +Anywhere else you relied on a prefs write surviving its caller's cancellation needs the same treatment. + +### 3. The shared `supervisorJob` is gone + +`BasePrefsHelper.Companion.supervisorJob` and `BaseDataStoreHelper.Companion.supervisorJob` have been deleted. They were `companion val`s — one `Job` shared by every helper instance in the process, so cancelling it once silently and permanently stopped async writes everywhere. The default `Job` is now created per instance. Nothing is expected to reference these, but grep for `supervisorJob` before upgrading. + +### 4. `readValueBlocking` no longer runs on `Dispatchers.IO` + +DataStore is already main-safe, so the hop bought nothing and the `Job` it carried made the blocking read cancellable process-wide by accident. It now uses a plain `runBlocking`. One caveat: never call it from a thread the backing `DataStore`'s own scope is confined to — the read can't make progress and you'll block for the full 2-second timeout and get `null`. + ## R8 / ProGuard / Minification PrefsHelper is fully compatible with R8 (including full mode) and requires **no consumer ProGuard rules** (the shipped `consumer-rules.pro` is intentionally empty). Preference keys are always explicit string arguments you pass to the `*Pref(...)` delegates — they are **never** derived from Kotlin property names via reflection. R8 is free to rename, merge, and repackage your `BasePrefsHelper`/`BaseDataStoreHelper` subclasses and their properties without changing any persisted key. Enum values are stored by `Enum.name` (preserved by R8's default Android rules), so enum prefs survive obfuscation. diff --git a/app/build.gradle.kts b/app/build.gradle.kts index d5bee8e..82de954 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -99,6 +99,7 @@ dependencies { testImplementation(libs.mockito.kotlin) testImplementation(libs.androidx.test.ext.junit) testImplementation(libs.robolectric) + testImplementation(libs.kotlinx.coroutines.test) debugImplementation(libs.androidx.compose.ui.tooling) } diff --git a/app/src/test/java/com/duck/app/BaseDataStoreHelperInjectionTest.kt b/app/src/test/java/com/duck/app/BaseDataStoreHelperInjectionTest.kt new file mode 100644 index 0000000..19d2fb7 --- /dev/null +++ b/app/src/test/java/com/duck/app/BaseDataStoreHelperInjectionTest.kt @@ -0,0 +1,340 @@ +package com.duck.app + +import android.content.Context +import android.content.SharedPreferences +import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.PreferenceDataStoreFactory +import androidx.datastore.preferences.core.Preferences +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import com.duck.prefshelper.BaseDataStoreHelper +import com.duck.prefshelper.BasePrefsHelper +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.CoroutineStart +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.Job +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.isActive +import kotlinx.coroutines.job +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.TestCoroutineScheduler +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import org.junit.runner.RunWith +import java.io.File + +/** + * Proves the coroutine-API redesign delivers injectable [DataStore], [CoroutineDispatcher], and + * [CoroutineScope], plus caller-cancellable suspend work — without the sleeps/polling the + * convenience-constructor suite still needs. + */ +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(AndroidJUnit4::class) +class BaseDataStoreHelperInjectionTest { + + @get:Rule + val tempFolder = TemporaryFolder() + + /** Scopes that own active DataStore file locks — cancelled in [tearDown]. */ + private val storeScopes = mutableListOf() + + private var fileCounter = 0 + + @After + fun tearDown() { + storeScopes.forEach { it.cancel() } + storeScopes.clear() + } + + /** + * Fresh preferences file per call — DataStore throws if two active instances share a file. + * Returns a path that does not yet exist ([TemporaryFolder.newFile] would pre-create it). + */ + private fun newPreferencesFile(): File = + File(tempFolder.root, "test_${fileCounter++}.preferences_pb") + + /** + * Builds a [DataStore] on an [UnconfinedTestDispatcher] so edits complete eagerly inside + * [runTest] with no polling. + */ + private fun newDataStore(testScheduler: TestCoroutineScheduler): DataStore { + val storeScope = CoroutineScope(UnconfinedTestDispatcher(testScheduler) + Job()) + storeScopes += storeScope + return PreferenceDataStoreFactory.create( + scope = storeScope, + produceFile = { newPreferencesFile() }, + ) + } + + // region 1 — Injected DataStore: deterministic write/read + + /** + * Injected [DataStore] + unconfined scopes make `write` then `read`/`first()` complete + * deterministically — no [kotlinx.coroutines.delay] or polling required. + */ + @Test + fun testInjectedDataStoreWriteThenReadIsDeterministic() = runTest { + val dispatcher = UnconfinedTestDispatcher(testScheduler) + val helper = InjectableDataStoreHelper( + dataStore = newDataStore(testScheduler), + dispatcher = dispatcher, + scope = CoroutineScope(dispatcher + SupervisorJob()), + ) + + helper.testWriteString("deterministic_key", "deterministic_value") + + assertEquals("deterministic_value", helper.testReadStringFlow("deterministic_key").first()) + assertEquals("deterministic_value", helper.testReadStringValue("deterministic_key")) + } + + // endregion + + // region 2 — Injected helper scope owns *Async launches + + /** + * `*Async` writes launch in the injected [BaseDataStoreHelper] [scope], not an internal one. + * With a paused [StandardTestDispatcher], the write only lands after [advanceUntilIdle], and + * the returned [Job] can be [Job.join]ed. + */ + @Test + fun testAsyncWriteLaunchesInInjectedHelperScope() = runTest { + val storeDispatcher = UnconfinedTestDispatcher(testScheduler) + val helperDispatcher = UnconfinedTestDispatcher(testScheduler) + // Paused dispatcher: launched async work does not run until the scheduler is advanced. + val helperScopeDispatcher = StandardTestDispatcher(testScheduler) + val helperScope = CoroutineScope(helperScopeDispatcher + Job()) + + val helper = InjectableDataStoreHelper( + dataStore = newDataStore(testScheduler), + dispatcher = helperDispatcher, + scope = helperScope, + ) + + val job = helper.testWriteStringAsync("async_key", "async_value") + assertTrue("Returned Job should still be active before the helper scope is advanced", job.isActive) + assertNull( + "Write must not land before the injected scope's dispatcher is advanced", + helper.testReadStringFlow("async_key").first(), + ) + + advanceUntilIdle() + job.join() + + assertTrue("Returned Job should be complete after join()", job.isCompleted) + assertEquals("async_value", helper.testReadStringFlow("async_key").first()) + } + + // endregion + + // region 3 — Default scope is per-instance (regression for deleted companion SupervisorJob) + + /** + * Regression guard for the deleted shared companion `supervisorJob`: two helpers built with + * the default [scope] argument must not share a Job. Cancelling instance A's scope must not + * prevent instance B's `*Async` writes from completing. + */ + @Test + fun testDefaultScopeIsPerInstanceNotShared() = runTest { + // Default scope uses Dispatchers.IO + SupervisorJob per instance — real threads, so join() + // is the await (no delay/polling). DataStore still uses an unconfined test scope for + // deterministic file I/O under runTest. + val helperA = InjectableDataStoreHelper(dataStore = newDataStore(testScheduler)) + val helperB = InjectableDataStoreHelper(dataStore = newDataStore(testScheduler)) + + assertFalse( + "Default scopes must be independent instances", + helperA.exposedScope === helperB.exposedScope, + ) + assertFalse( + "Default scopes must not share a Job (regression for companion supervisorJob)", + helperA.exposedScope.coroutineContext.job === helperB.exposedScope.coroutineContext.job, + ) + + helperA.exposedScope.cancel() + assertFalse(helperA.exposedScope.isActive) + + // B must still complete async writes after A was cancelled. + helperB.testWriteStringAsync("independent_key", "still_works").join() + assertEquals("still_works", helperB.testReadStringFlow("independent_key").first()) + assertTrue(helperB.exposedScope.isActive) + } + + // endregion + + // region 4 — clearPrefs() is caller-cancellable (DataStore) + + /** + * Headline behaviour change: [BaseDataStoreHelper.clearPrefs] uses `withContext(dispatcher)` + * (no Job on the dispatcher), so cancelling the caller cancels the clear. Under the old + * foreign-Job `withContext(coroutineContext)` the clear would have completed regardless. + * + * [CoroutineStart.UNDISPATCHED] runs until the first suspension (`withContext`) without + * advancing the test scheduler — then we cancel before [advanceUntilIdle]. + */ + @Test + fun testClearPrefsIsCallerCancellable() = runTest { + val clearDispatcher = StandardTestDispatcher(testScheduler) + val helper = InjectableDataStoreHelper( + dataStore = newDataStore(testScheduler), + dispatcher = clearDispatcher, + scope = CoroutineScope(UnconfinedTestDispatcher(testScheduler) + SupervisorJob()), + ) + + // Seed via writeString (withContext on the paused dispatcher), then advance so it lands. + val seedJob = launch(start = CoroutineStart.UNDISPATCHED) { + helper.testWriteString("keep_key", "keep_me") + } + advanceUntilIdle() + seedJob.join() + assertEquals("keep_me", helper.testReadStringFlow("keep_key").first()) + + val clearJob = launch(start = CoroutineStart.UNDISPATCHED) { + helper.clearPrefs() + } + // Suspended inside withContext(clearDispatcher) — body not yet run. + assertTrue(clearJob.isActive) + clearJob.cancel() + assertTrue(clearJob.isCancelled) + + advanceUntilIdle() + + assertEquals( + "Cancelled clear must not wipe prefs", + "keep_me", + helper.testReadStringFlow("keep_key").first(), + ) + } + + /** + * Positive path companion to [testClearPrefsIsCallerCancellable]: when the clear job is not + * cancelled, [advanceUntilIdle] must actually clear — so the cancel test cannot pass vacuously. + */ + @Test + fun testClearPrefsRunsWhenNotCancelled() = runTest { + val clearDispatcher = StandardTestDispatcher(testScheduler) + val helper = InjectableDataStoreHelper( + dataStore = newDataStore(testScheduler), + dispatcher = clearDispatcher, + scope = CoroutineScope(UnconfinedTestDispatcher(testScheduler) + SupervisorJob()), + ) + + val seedJob = launch(start = CoroutineStart.UNDISPATCHED) { + helper.testWriteString("clear_key", "will_go") + } + advanceUntilIdle() + seedJob.join() + assertEquals("will_go", helper.testReadStringFlow("clear_key").first()) + + val clearJob = launch(start = CoroutineStart.UNDISPATCHED) { + helper.clearPrefs() + } + advanceUntilIdle() + clearJob.join() + + assertNull(helper.testReadStringFlow("clear_key").first()) + } + + // endregion + + // region 5 — Same cancellation guard for BasePrefsHelper + + /** + * [BasePrefsHelper.clearPrefs] is also `withContext(dispatcher)` — caller cancellation must + * prevent the clear. Real Robolectric [SharedPreferences], not mocks, so we assert on stored + * state rather than verify() interactions. + */ + @Test + fun testPrefsHelperClearPrefsIsCallerCancellable() = runTest { + val clearDispatcher = StandardTestDispatcher(testScheduler) + val prefs = newSharedPreferences("prefs_cancel_${fileCounter++}") + val helper = InjectablePrefsHelper(prefs, clearDispatcher) + + helper.setString("keep_key", "keep_me") + assertEquals("keep_me", helper.getString("keep_key")) + assertTrue(helper.contains("keep_key")) + + val clearJob = launch(start = CoroutineStart.UNDISPATCHED) { + helper.clearPrefs() + } + assertTrue(clearJob.isActive) + clearJob.cancel() + assertTrue(clearJob.isCancelled) + + advanceUntilIdle() + + assertTrue("Cancelled clear must leave the key present", helper.contains("keep_key")) + assertEquals("keep_me", helper.getString("keep_key")) + } + + /** + * Positive path companion to [testPrefsHelperClearPrefsIsCallerCancellable]. + */ + @Test + fun testPrefsHelperClearPrefsRunsWhenNotCancelled() = runTest { + val clearDispatcher = StandardTestDispatcher(testScheduler) + val prefs = newSharedPreferences("prefs_clear_${fileCounter++}") + val helper = InjectablePrefsHelper(prefs, clearDispatcher) + + helper.setString("clear_key", "will_go") + assertTrue(helper.contains("clear_key")) + + val clearJob = launch(start = CoroutineStart.UNDISPATCHED) { + helper.clearPrefs() + } + advanceUntilIdle() + clearJob.join() + + assertFalse(helper.contains("clear_key")) + } + + // endregion + + private fun newSharedPreferences(name: String): SharedPreferences { + val context = InstrumentationRegistry.getInstrumentation().targetContext + return context.getSharedPreferences(name, Context.MODE_PRIVATE).also { it.edit().clear().commit() } + } + + // region Test subclasses + + /** + * Thin test surface over [BaseDataStoreHelper] for the primary (injected-[DataStore]) constructor. + * Exposes [exposedScope] so tests can cancel the default per-instance scope (#3). + */ + private class InjectableDataStoreHelper( + dataStore: DataStore, + dispatcher: CoroutineDispatcher = Dispatchers.IO, + scope: CoroutineScope = CoroutineScope(dispatcher + SupervisorJob()), + ) : BaseDataStoreHelper(dataStore, dispatcher, scope) { + + val exposedScope: CoroutineScope get() = scope + + suspend fun testWriteString(key: String, value: String?) = writeString(key, value) + fun testWriteStringAsync(key: String, value: String?) = writeStringAsync(key, value) + fun testReadStringValue(key: String) = readStringValue(key) + fun testReadStringFlow(key: String) = readString(key) + } + + /** + * [BasePrefsHelper] over a real Robolectric [SharedPreferences] with an injectable dispatcher. + */ + private class InjectablePrefsHelper( + override val sharedPreferences: SharedPreferences, + dispatcher: CoroutineDispatcher, + ) : BasePrefsHelper(dispatcher) + + // endregion +} diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 7a7fd7e..9f26415 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -6,6 +6,7 @@ kotlin = "2.4.10" dokka = "2.2.0" kover = "0.9.9" dataStore = "1.2.1" +coroutines = "1.11.0" mockitoCore = "5.23.0" mockitoKotlin = "6.3.0" robolectric = "4.16.1" @@ -34,6 +35,7 @@ androidx-test-espresso-core = { module = "androidx.test.espresso:espresso-core", junit = { module = "junit:junit", version.ref = "junit" } androidx-dataStore = { group = "androidx.datastore", name = "datastore-preferences", version.ref = "dataStore" } +kotlinx-coroutines-test = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-test", version.ref = "coroutines" } android-documentation-plugin = { module = "org.jetbrains.dokka:android-documentation-plugin", version.ref = "dokka" } From 1ea588ff6b8603c5f29efc1eec793e82ad1017f0 Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 10:20:06 +0100 Subject: [PATCH 03/17] Remove long-deprecated *Bool methods The four protected boolean helpers on BaseDataStoreHelper have carried @Deprecated annotations and a "remove in a future release" TODO for several versions. A major bump is the release to do it in. writeBool(key, Boolean) -> writeBoolean(key, value) writeBool(key, Boolean?) -> writeBoolean(key, value) readNullableBool(key) -> readBoolean(key) readBool(key) -> readBoolean(key, true) Nothing in this repo referenced them. Documented in the README migration note, including the trap in the last one: readBool defaulted to true, and readBoolean(key) alone is the nullable overload, so a mechanical replace-with that drops the argument changes behaviour silently. Also fixes the documentation command in CLAUDE.md. Dokka 2.2.0 removed the V1 dokkaHtml task, so the documented command has been failing since the Dokka upgrade; dokkaGenerateHtml is the replacement. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 4 +-- .../duck/prefshelper/BaseDataStoreHelper.kt | 27 ------------------- README.md | 15 ++++++++++- 3 files changed, 16 insertions(+), 30 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e94633c..81a0e90 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,8 +27,8 @@ LocalDateTime, LocalDate, LocalTime, and Enums. ./gradlew :app:koverHtmlReportDebug # HTML -> app/build/reports/kover/htmlDebug/index.html ./gradlew :app:koverVerifyDebug # fails below the coverage floor (minBound in app/build.gradle.kts) -# Generate documentation -./gradlew :PrefsHelper:dokkaHtml +# Generate documentation (Dokka 2.x V2 task; the old `dokkaHtml` V1 task no longer exists) +./gradlew :PrefsHelper:dokkaGenerateHtml # Clean build ./gradlew clean diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt index 2aa1fef..04cef25 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt @@ -90,33 +90,6 @@ abstract class BaseDataStoreHelper( scope = scope, ) - //Todo: remove Deprecated methods in future release - /** - * Add Boolean to the data store - */ - @Deprecated("Use writeBoolean instead", ReplaceWith("writeBoolean(key, value)")) - protected suspend fun writeBool(key: String, value: Boolean) = - writeBoolean(key, value) - - /** - * Add Nullable Boolean to the data store - */ - @Deprecated("Use writeBoolean instead", ReplaceWith("writeBoolean(key, value)")) - protected suspend fun writeBool(key: String, value: Boolean?) = writeBoolean(key, value) - - /** - * Reading the Boolean from the data store - */ - @Deprecated("Use readBoolean instead", ReplaceWith("readBoolean(key)")) - protected fun readNullableBool(key: String): Flow = readBoolean(key) - - /** - * Reading the Boolean from the data store - */ - @Deprecated("Use readBoolean(key, default) instead", ReplaceWith("readBoolean(key, true)")) - protected fun readBool(key: String): Flow = readBoolean(key, true) - //end Deprecated methods - /** * Clear all preferences in the data store. * diff --git a/README.md b/README.md index 971aed5..c0cdb38 100644 --- a/README.md +++ b/README.md @@ -205,7 +205,20 @@ Anywhere else you relied on a prefs write surviving its caller's cancellation ne `BasePrefsHelper.Companion.supervisorJob` and `BaseDataStoreHelper.Companion.supervisorJob` have been deleted. They were `companion val`s — one `Job` shared by every helper instance in the process, so cancelling it once silently and permanently stopped async writes everywhere. The default `Job` is now created per instance. Nothing is expected to reference these, but grep for `supervisorJob` before upgrading. -### 4. `readValueBlocking` no longer runs on `Dispatchers.IO` +### 4. The long-deprecated `*Bool` methods are gone + +`BaseDataStoreHelper`'s four `protected` boolean methods, deprecated since before 1.x and carrying a "remove in a future release" note, have been deleted. Their replacements have been available the whole time: + +| Removed | Replacement | +|---------|-------------| +| `writeBool(key, value: Boolean)` | `writeBoolean(key, value)` | +| `writeBool(key, value: Boolean?)` | `writeBoolean(key, value)` | +| `readNullableBool(key)` | `readBoolean(key)` | +| `readBool(key)` | `readBoolean(key, true)` | + +Mind the last one: `readBool(key)` defaulted to **`true`**, not `false`, and `readBoolean(key)` on its own is the *nullable* overload returning `Flow`. Pass the default explicitly — an IDE "replace with" that drops the `true` will silently change behaviour. + +### 5. `readValueBlocking` no longer runs on `Dispatchers.IO` DataStore is already main-safe, so the hop bought nothing and the `Job` it carried made the blocking read cancellable process-wide by accident. It now uses a plain `runBlocking`. One caveat: never call it from a thread the backing `DataStore`'s own scope is confined to — the read can't make progress and you'll block for the full 2-second timeout and get `null`. From e7b5fe12f125c6c3e8cc98a2082f77b33a008ff1 Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 10:39:31 +0100 Subject: [PATCH 04/17] Remove the coroutine redesign spec doc now it's implemented MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The proposal has shipped and its content now lives where consumers will actually look for it: the migration guide in the README and the class KDoc on both helpers. The design record isn't lost — the doc, including the Outcome section recording where the implementation deviated from the proposal and why, is retrievable from history: git show 5c4f1dd:COROUTINE-API-REDESIGN.md Co-Authored-By: Claude Opus 5 (1M context) --- COROUTINE-API-REDESIGN.md | 272 -------------------------------------- 1 file changed, 272 deletions(-) delete mode 100644 COROUTINE-API-REDESIGN.md diff --git a/COROUTINE-API-REDESIGN.md b/COROUTINE-API-REDESIGN.md deleted file mode 100644 index c6115d1..0000000 --- a/COROUTINE-API-REDESIGN.md +++ /dev/null @@ -1,272 +0,0 @@ -# Coroutine API redesign — injectable scope + correct cancellation - -**Status:** implemented on `feature/coroutine-api-redesign`, 2026-07-31 — see [Outcome](#outcome) -**Drafted:** 2026-07-30 -**Provenance:** came out of a main-thread-safety audit of the repo layer in -`AIM-Capture-Android`. Line references below were verified against the working tree on that date — -re-check them if the files have moved on. Design was reviewed by a second opinion; the two "gotcha" -sections exist because that review found problems with the first draft. - ---- - -## Outcome - -Built as **scope + injectable DataStore** — the wider of the two options in -[The testability catch](#the-testability-catch), so goal 2 is genuinely delivered rather than -deferred. Everything below is the original proposal, kept as the design record. Where the -implementation deviates from it: - -| Proposal said | Built instead | Why | -|---|---|---| -| `dataStore: DataStore? = null` param alongside `context` | **Two constructors** — primary `(dataStore, dispatcher, scope)`, secondary `(context, preferenceName, dispatcher, scope)` | `context` is used for nothing else in the class, so requiring it on the injected path is dead weight. Existing `super(context, "name")` subclasses stay source-compatible. | -| Keep the `Context.dataStoreInstance` extension delegate | Deleted; the convenience constructor uses `PreferenceDataStoreFactory.create(produceFile = { context.applicationContext.preferencesDataStoreFile(name) })` | The delegate's singleton guard was per *helper instance*, not process-wide, so it was never buying the protection it looked like it was. `SingleProcessDataStore`'s own file-lock check still catches duplicate instances. | -| `@PublishedApi internal val dispatcher` on **both** classes | `@PublishedApi internal` on `BaseDataStoreHelper`, plain `protected` on `BasePrefsHelper` | No `inline` function in `BasePrefsHelper` touches it, so `protected` is both sufficient and the smaller diff from the status quo. | - -Two things the proposal flagged that turned out fine: the existing 1,621-line test suite passes -**unchanged** against the new API, and `scope` moving from a body property to a constructor property -does not reintroduce the `IllegalAccessError` gotcha (that only bites members captured inside an -anonymous object emitted by an inline function — a direct `this.scope` read in an inlined -`protected inline` body is legal). - -Still outstanding, and deliberately out of this repo: the AIM app's logout call site (below), and -the version bump + JitPack tag. - ---- - -## Goals - -1. **Injectable scope.** Consumers should be able to hand in a `CoroutineScope` they own — e.g. an - application-lifetime scope from Koin with a `CoroutineExceptionHandler` — so that fire-and-forget - prefs writes report failures instead of crashing the process. -2. **Testability.** Subclasses should be unit-testable with deterministic coroutines. - -Goal 1 is achievable with the change below. **Goal 2 is not, without an extra step** — see -[The testability catch](#the-testability-catch). Read that before estimating. - ---- - -## Diagnosis - -One constructor parameter is doing two unrelated jobs: - -```kotlin -// BasePrefsHelper.kt:36-37 and BaseDataStoreHelper.kt:52-55 -coroutineContext: CoroutineContext = Dispatchers.IO + supervisorJob -``` - -It simultaneously decides: - -- **(a)** which dispatcher `suspend` functions hop to — `withContext(coroutineContext)`, at - `BasePrefsHelper.kt:47` and `BaseDataStoreHelper.kt:108`, `:159` -- **(b)** which Job owns fire-and-forget launches — `BaseDataStoreHelper.kt:74`, - `protected val scope = CoroutineScope(coroutineContext)`, used by the `*Async` helpers at `:129`, - `:148`, `:173` - -Conflating them causes three concrete problems. - -### 1. `suspend` functions ignore their caller's cancellation - -`withContext(context)` where `context` carries a Job **reparents** the block onto that Job instead of -the caller's. So `clearPrefs()` and `writeValue()` behave like `NonCancellable` with respect to -whoever called them. A `suspend` function should be cancellable by its caller; this one isn't. - -A `suspend` function should only ever be handed a **dispatcher** — no Job. - -### 2. The default `SupervisorJob` is shared process-wide - -```kotlin -// BasePrefsHelper.kt:461 and BaseDataStoreHelper.kt:1184 (companion objects) -val supervisorJob = SupervisorJob() -``` - -A companion `val` means **one Job shared across every instance of that class in the process**. Cancel -it once and every prefs helper in the app silently stops doing async work, permanently, with no -error. Nothing cancels it today, so it currently behaves as a harmless keep-alive parent — but it is -a live footgun, and it makes the blocking reads at `:212` cancellable process-wide by accident. - -### 3. There is no way to inject a scope - -Which is the thing that prompted all this. A consumer can pass a context, but then it also -(unavoidably) changes where `suspend` functions dispatch, and it still can't attach an exception -handler to the launches without also reparenting the suspend calls. - ---- - -## Proposed API - -Split the parameter in two. - -```kotlin -abstract class BaseDataStoreHelper( - context: Context, - preferenceName: String, - @PublishedApi internal val dispatcher: CoroutineDispatcher = Dispatchers.IO, - protected val scope: CoroutineScope = CoroutineScope(dispatcher + SupervisorJob()), -) { -``` - -```kotlin -abstract class BasePrefsHelper( - @PublishedApi internal val dispatcher: CoroutineDispatcher = Dispatchers.IO, -) { -``` - -- `dispatcher` — used by `suspend` functions via `withContext(dispatcher)`. No Job, so caller - cancellation propagates correctly. -- `scope` — used for fire-and-forget launches. Injectable; this is the goal-1 payload. -- The default `SupervisorJob()` becomes **per-instance**, replacing the shared companion one. -- A default parameter referencing an earlier parameter (`scope` defaulting to an expression over - `dispatcher`) is legal Kotlin and compiles. - -`BasePrefsHelper` has no launches and no `scope` today, so it only needs `dispatcher`. Adding a -`scope` there too is optional — do it only if you want symmetry, not because anything needs it. - -### Visibility constraint - -`dispatcher` is touched by `protected inline` functions (`writeValue` at `:159`, the -`readValueBlocking` family at `:211`/`:225`/`:235`). Keeping it `@PublishedApi internal` — matching -today's `coroutineContext` — is the safe option. `protected` would also satisfy a `protected inline` -caller, but `@PublishedApi internal` is a smaller diff from the status quo. `scope` stays -`protected` and already works from the `protected inline` `*Async` helpers. - ---- - -## Change list - -### `BaseDataStoreHelper.kt` - -| Line | Now | Becomes | -|------|-----|---------| -| 52-55 | `coroutineContext: CoroutineContext = Dispatchers.IO + supervisorJob` | `dispatcher` + `scope` params as above | -| 74 | `protected val scope = CoroutineScope(coroutineContext)` | delete — `scope` is now a constructor property | -| 108 | `clearPrefs() = withContext(coroutineContext)` | `withContext(dispatcher)` | -| 159 | `writeValue(...) = withContext(coroutineContext)` | `withContext(dispatcher)` | -| 212 | `runBlocking(coroutineContext) { withTimeoutOrNull(2.seconds) { … } }` | `runBlocking { withTimeoutOrNull(2.seconds) { … } }` — drop the context entirely | -| 1184 | companion `val supervisorJob = SupervisorJob()` | delete (after checking for consumer references) | - -On `:212` specifically: DataStore is already main-safe, so the IO dispatch buys nothing, and passing -a Job into `runBlocking` is the footgun described above. - -### `BasePrefsHelper.kt` - -| Line | Now | Becomes | -|------|-----|---------| -| 36-37 | `coroutineContext: CoroutineContext = Dispatchers.IO + supervisorJob` | `dispatcher: CoroutineDispatcher = Dispatchers.IO` | -| 47 | `clearPrefs() = withContext(coroutineContext)` | `withContext(dispatcher)` | -| 461 | companion `val supervisorJob = SupervisorJob()` | delete (after checking for consumer references) | - -Also update the class KDoc on both — it currently documents the `coroutineContext` parameter and the -shared-`supervisorJob` default. - ---- - -## The testability catch - -**Injecting a scope does not, on its own, make DataStore-backed subclasses unit-testable.** - -`preferencesDataStore` creates its **own** internal `Dispatchers.IO` scope and performs real file -I/O. An injected `TestScope` with virtual time cannot make `dataStore.edit { }` deterministic, -because the work doesn't run on the scope we injected. So the change above delivers goal 1 and -quietly fails goal 2. - -To actually get goal 2, the **`DataStore` instance itself must be injectable**: either a constructor -parameter defaulting to the current delegate, or constructed via -`PreferenceDataStoreFactory.create(scope = …)` so tests can supply their own. - -**Decision required.** Two options: - -- **Scope-only (recommended first pass).** Smaller, contained, delivers the error-handling and - Koin-ownership win. Be explicit that unit tests aren't arriving in this round. -- **Scope + injectable DataStore.** Delivers real tests, but it is a wider API change and a bigger - piece of work. Reasonable as a follow-up once the split above has landed. - -`BasePrefsHelper` (SharedPreferences) does not have this problem — it's synchronous and already -testable with a fake `SharedPreferences`. - ---- - -## Consumer impact - -### Known consumers - -- `AIM-Capture-Android` — `NormalPrefs`, `DevicePrefs` (both `BasePrefsHelper`), `NormalDataStore` - (`BaseDataStoreHelper`), and `CameraDataStore` in the `:insta360Helper` module. -- Other Appoly apps that depend on `PrefsHelperBase` — enumerate before releasing. - -Most subclasses **don't pass the parameter at all** and rely on the default, so they are unaffected -by the signature change. Only call sites that explicitly pass `coroutineContext` need editing. - -### Required change at the logout call site — do not skip this - -Fixing problem 1 (making `suspend` functions properly caller-cancellable) has a real consequence. -`PrefsHelper.onLogout()` in the AIM app runs four sequential clears: - -```kotlin -suspend fun onLogout() { - normalPrefs.clearPrefs() - normalDataStore.clearPrefs() - RetrofitClient.resetClients() - cameraDataStore.clearPrefs() -} -``` - -Today the foreign-Job `withContext` makes those effectively uncancellable, which is accidentally why -logout always completes. Its caller is `SettingsScreenModel.logout()` running on `screenModelScope`, -which **is cancelled when the settings screen is disposed**. After this change, navigating away -mid-logout could leave SharedPreferences cleared but DataStore and the Retrofit clients not — a -partially logged-out state. - -**Fix it in the same release:** run logout on an application-lifetime scope (the AIM app now has one -in `appModule` as `named("appScope")`), or wrap the body in `withContext(NonCancellable)`. Pick one -deliberately — this is a genuine behaviour change, not a refactor. - -### Direct `supervisorJob` references - -`grep` all consumers for `supervisorJob` before deleting the companion properties. Nothing is -expected to reference them, but confirm rather than assume. - ---- - -## Release strategy - -**Major version bump with a migration note. No deprecated back-compat shim.** - -A secondary constructor preserving the old `coroutineContext` parameter would have to derive a -dispatcher back out of a `CoroutineContext` — exactly the fragile extraction this split exists to -avoid — and you own every known consumer, so the compatibility burden isn't worth carrying. - -The migration note should cover: - -1. `coroutineContext` → `dispatcher` (+ optional `scope`). -2. `suspend` functions are now caller-cancellable — audit anywhere you relied on a prefs write - surviving the caller's cancellation, and see the logout note above. -3. The shared `supervisorJob` is gone; the default Job is now per-instance. - ---- - -## Checklist - -- [x] Decide scope-only vs scope + injectable DataStore — chose **scope + injectable DataStore** -- [x] `BaseDataStoreHelper`: split constructor, delete the body `scope`, fix `clearPrefs`, nullable `writeValue`, `readValueBlocking` -- [x] `BasePrefsHelper`: `dispatcher` param, fix `clearPrefs` -- [x] Delete both companion `supervisorJob` properties (grepped — no references in this repo) -- [x] Update class KDoc on both -- [x] Update the library's own `app/` sample module if it passes a context — it doesn't; no change needed -- [x] Grep consumers for `coroutineContext` / `supervisorJob` usage — clean within this repo -- [ ] Fix the AIM app's logout call site in the same release — **not in this repo** -- [x] Migration note in README (`## Migrating to 2.0`) -- [ ] Major version bump, tag + JitPack release, then bump `prefshelperVersion` in consumers -- [x] Deterministic tests proving the injectable DataStore delivers goal 2 - ---- - -## Notes for whoever picks this up - -There is an **uncommitted** change in `AIM-Capture-Android` -(`data/local/prefs/NormalDataStore.kt`) that works around the current API by passing a custom -`coroutineContext` to the superclass to attach an exception handler. It is a stopgap and becomes -obsolete once this redesign lands — expect to delete it and inject the app scope instead. - -One thing that is **not** a defect, despite looking like one: `withContext(coroutineContext)` is a -real dispatcher switch, not a no-op. `coroutineContext` is a constructor property that *shadows* -`kotlin.coroutines.coroutineContext`. This was misdiagnosed once already. From 76c98751f6a6ec5610e008d5a02ae44cc60f8a70 Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 10:50:38 +0100 Subject: [PATCH 05/17] Tighten the 2.0 migration guide - Fix a misleading line: it said subclasses were fine if they didn't "pass a context", but BaseDataStoreHelper(context, "name") does pass a context. The thing that matters is not passing a coroutine context. - Call out that item 2 (caller-cancellable suspend functions) is the only change that can alter behaviour without a compile error. The rest are removed or retyped symbols that fail the build immediately, so that's where a reader's attention should go. - Add the 2.0.0-beta01 coordinate for testing ahead of the release. - Note the injectable DataStore as a new capability, since a reader skimming only the migration guide would otherwise miss it. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index c0cdb38..4889882 100644 --- a/README.md +++ b/README.md @@ -161,7 +161,15 @@ Each type exposes a non-nullable delegate `*Pref(key, defaultValue)` and a nulla 2.0 splits the single `coroutineContext` constructor parameter, which was doing two unrelated jobs, into a `dispatcher` and a `scope`. There is no back-compat shim: deriving a dispatcher back out of an arbitrary `CoroutineContext` is exactly the fragile guesswork this change exists to remove. -**Most subclasses need no change at all.** If you extend `BasePrefsHelper()` or `BaseDataStoreHelper(context, "name")` without passing a context, you are already on the new defaults. +**Most subclasses need no change at all.** If you extend `BasePrefsHelper()` or `BaseDataStoreHelper(context, "name")` without passing a coroutine context of your own, you are already on the new defaults. + +**Only one item on this list can change behaviour without the compiler telling you: item 2.** Everything else is a removed or retyped symbol, so it fails the build and you'll find it immediately. Read item 2 properly. + +A beta is published for testing ahead of the 2.0.0 release: + +```kotlin +implementation("com.github.projectdelta6:PrefsHelperBase:2.0.0-beta01") +``` ### 1. `coroutineContext` → `dispatcher` (+ optional `scope`) @@ -222,6 +230,10 @@ Mind the last one: `readBool(key)` defaulted to **`true`**, not `false`, and `re DataStore is already main-safe, so the hop bought nothing and the `Job` it carried made the blocking read cancellable process-wide by accident. It now uses a plain `runBlocking`. One caveat: never call it from a thread the backing `DataStore`'s own scope is confined to — the read can't make progress and you'll block for the full 2-second timeout and get `null`. +### 6. New, not breaking: the `DataStore` is injectable + +`BaseDataStoreHelper`'s primary constructor now takes a `DataStore` directly. Nothing forces you to use it — the `(context, preferenceName)` convenience constructor is unchanged — but it's what finally makes DataStore-backed subclasses unit-testable without sleeps. See [Injecting a `DataStore` for tests](#injecting-a-datastore-for-tests). + ## R8 / ProGuard / Minification PrefsHelper is fully compatible with R8 (including full mode) and requires **no consumer ProGuard rules** (the shipped `consumer-rules.pro` is intentionally empty). Preference keys are always explicit string arguments you pass to the `*Pref(...)` delegates — they are **never** derived from Kotlin property names via reflection. R8 is free to rename, merge, and repackage your `BasePrefsHelper`/`BaseDataStoreHelper` subclasses and their properties without changing any persisted key. Enum values are stored by `Enum.name` (preserved by R8's default Android rules), so enum prefs survive obfuscation. From bd31460a1a374fbefaa55e0f068e899a581ba831 Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 10:58:39 +0100 Subject: [PATCH 06/17] Align jitpack.yml with this project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The file was copied from another project and had drifted from what this repo actually needs. - Drop `sdk install maven` / `mvn -v`. This is a pure Gradle project; the release build was downloading Maven 3.9.16 every time and never using it. - Bump the SDKMAN JDK from 17 to 21.0.12, matching .github/workflows/ci.yml. CI and the release build were running different JDKs, which meant a green CI was not evidence the release would build — JitPack was the only place a JDK-specific break could surface, and only at release time. - Add an explicit `install:` scoped to :PrefsHelper. JitPack's default runs `assemble`, which built the :app sample module as well — 96 of 186 tasks for something that is never published. Takes the build from ~1m53s to ~12s. GROUP and VERSION are JitPack-provided env vars and are passed through, or the artifact would publish as "unspecified". Only :PrefsHelper applies maven-publish, so it is still the single published module and the consumer coordinate is unchanged. Verified locally on Corretto 21.0.12 with JitPack's own argument form, and the command was read off the build log of the 2.0.0-beta01 release rather than guessed. Co-Authored-By: Claude Opus 5 (1M context) --- jitpack.yml | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/jitpack.yml b/jitpack.yml index 1b99ca8..e457be2 100644 --- a/jitpack.yml +++ b/jitpack.yml @@ -1,8 +1,34 @@ +# JitPack release build configuration. +# +# JitPack builds a tag the first time someone requests it, then caches the +# result — so this runs about once per release. +# +# `jdk:` selects JitPack's base image, which tops out below what AGP needs, so +# before_install upgrades via SDKMAN and that is the JDK that actually runs +# Gradle. Pinned to 21 to match .github/workflows/ci.yml — when CI and the +# release build run different JDKs, a green CI stops being evidence that the +# release will build. Keep the two in step. This only affects the toolchain; +# the artifact still targets JVM 11 (see compileOptions / jvmTarget in +# PrefsHelper/build.gradle.kts). +# +# `install:` overrides JitPack's default, which would be: +# ./gradlew clean -Pgroup=$GROUP -Pversion=$VERSION -xtest -xlint assemble publishToMavenLocal +# That `assemble` builds the :app sample module too — over half the build for +# something that is never published. Scoping to :PrefsHelper takes the build +# from ~1m53s to ~12s. GROUP and VERSION are JitPack-provided environment +# variables and MUST be passed through, or the artifact publishes as +# "unspecified". +# +# Only :PrefsHelper applies maven-publish, so it remains the single published +# module and consumers keep resolving it as com.github.projectdelta6:PrefsHelperBase. + jdk: - openjdk11 + before_install: - sdk update - - sdk install java 17.0.6-amzn - - sdk use java 17.0.6-amzn - - sdk install maven - - mvn -v \ No newline at end of file + - sdk install java 21.0.12-amzn + - sdk use java 21.0.12-amzn + +install: + - ./gradlew clean :PrefsHelper:publishToMavenLocal -Pgroup=$GROUP -Pversion=$VERSION -xtest -xlint From 9a5898770fed01244076fca7a13ded6462181260 Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 11:10:55 +0100 Subject: [PATCH 07/17] Address PR #10 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pin the convenience constructor's DataStore to Dispatchers.IO ------------------------------------------------------------ The convenience constructor was building its store with `scope = CoroutineScope(dispatcher + SupervisorJob())`, so a caller-supplied dispatcher also owned DataStore's internal actor. That was a silent change from the old `preferencesDataStore` delegate, which always used its own IO scope — and it combined badly with readValueBlocking's plain runBlocking: a confined dispatcher (Main, single-threaded, a paused test dispatcher) would deadlock the store against its own blocking read. The coupling bought nothing. Passing a test dispatcher to the convenience constructor never made anything deterministic — that is precisely why the injectable primary constructor exists. Pinned to Dispatchers.IO, restoring pre-2.0 isolation exactly, and documented so it doesn't get "tidied" back. Fix the migration guide banner ------------------------------ It claimed only item 2 changes behaviour without a compile error. Item 5 (readValueBlocking dropping its dispatcher hop) is equally silent. Now names both, while still pointing at item 2 as the one that can corrupt state. Also ---- - Document that an injected scope does NOT own DataStore's internal actor, so a CoroutineExceptionHandler on it won't see DataStore's own failures. - Add write-cancellation tests. The 2.0 claim covers clearPrefs *and* the suspending writes, but only clearPrefs was tested. Mutation-checked: restoring the foreign Job on the nullable writeValue overload fails exactly the new guard. - README: use a non-precreated unique path in the DataStore test example, matching the injection test. The old `newFile` form does actually work (verified), but it depends on non-contractual DataStore internals and contradicted our own test. - CLAUDE.md: two test classes -> three, named. - jitpack.yml: correct the timing claim in the comment. The "~12s" was a warm local build; on JitPack wall time is dominated by the Gradle distribution download. The real, measured win is 158 tasks -> 31. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 4 +- .../duck/prefshelper/BaseDataStoreHelper.kt | 16 +++++- README.md | 19 +++++-- .../app/BaseDataStoreHelperInjectionTest.kt | 55 +++++++++++++++++++ jitpack.yml | 14 +++-- 5 files changed, 94 insertions(+), 14 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 81a0e90..fec8867 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -15,7 +15,7 @@ LocalDateTime, LocalDate, LocalTime, and Enums. # Build the library ./gradlew :PrefsHelper:build -# Run all tests (both test classes live in :app/src/test and run on the JVM — +# Run all tests (all three test classes live in :app/src/test and run on the JVM — # BaseDataStoreHelperTest runs under Robolectric, so no device/emulator is needed) ./gradlew :app:testDebugUnitTest @@ -63,7 +63,7 @@ Both classes support: `String`, `Int`, `Long`, `Boolean`, `LocalDateTime`, `Loca - **Inline reified factories that access `protected` members**: an `inline fun ` that returns an anonymous object (e.g. a `ReadWriteProperty`) calling `protected` methods on `BaseDataStoreHelper` throws `IllegalAccessError` at runtime — the anonymous class is emitted inside the *subclass* at inline time, losing JVM-level protected access. Fix: split into a thin `inline` + `reified` wrapper that forwards to a non-inline `@PublishedApi internal` helper which owns the anonymous object. See `enumPref` / `enumPrefInternal` in `BaseDataStoreHelper.kt`. `BasePrefsHelper` is unaffected because its get/set accessors are `public`. -- **Tests are JVM-only (Robolectric), not instrumented**: both test classes live in `app/src/test`. `BaseDataStoreHelperTest` uses a real `DataStore` but runs on the JVM via Robolectric (`@RunWith(AndroidJUnit4::class)` delegates to `RobolectricTestRunner` off-device). This is deliberate: **Kover cannot instrument on-device tests**, so DataStore coverage would read ~0% if the tests were instrumented — running them under Robolectric makes the `koverVerifyDebug` floor meaningful across both helpers. Robolectric's SDK is pinned to 36 in `app/src/test/resources/robolectric.properties` because `targetSdk = 37` has no Robolectric image yet. +- **Tests are JVM-only (Robolectric), not instrumented**: all three test classes live in `app/src/test` (`BasePrefsHelperTest`, `BaseDataStoreHelperTest`, and `BaseDataStoreHelperInjectionTest`). `BaseDataStoreHelperTest` uses a real `DataStore` but runs on the JVM via Robolectric (`@RunWith(AndroidJUnit4::class)` delegates to `RobolectricTestRunner` off-device). This is deliberate: **Kover cannot instrument on-device tests**, so DataStore coverage would read ~0% if the tests were instrumented — running them under Robolectric makes the `koverVerifyDebug` floor meaningful across both helpers. Robolectric's SDK is pinned to 36 in `app/src/test/resources/robolectric.properties` because `targetSdk = 37` has no Robolectric image yet. ## Project Structure diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt index 04cef25..4a6dc40 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt @@ -71,10 +71,20 @@ abstract class BaseDataStoreHelper( * deterministic unit tests, or to configure migrations or a corruption handler — use the * primary constructor instead. * + * The store this creates always runs its own internal actor on [Dispatchers.IO], regardless of + * [dispatcher] — matching what the `preferencesDataStore` delegate did before 2.0. Binding it to + * a caller-supplied [dispatcher] instead would be a silent behaviour change, and a confined + * dispatcher (`Main`, single-threaded, a paused test dispatcher) would then deadlock + * [readValueBlocking] against its own store. Use the primary constructor if you genuinely want + * to control where the store schedules its work. + * * @param context Any [Context]; the application context is used internally * @param preferenceName Name of the preferences data store file - * @param dispatcher The [CoroutineDispatcher] `suspend` functions and the DataStore's own IO run on - * @param scope The [CoroutineScope] the `*Async` writes are launched in + * @param dispatcher The [CoroutineDispatcher] `suspend` functions run on. Does **not** affect the + * store's own IO — see above. + * @param scope The [CoroutineScope] the `*Async` writes are launched in. Note this does not own + * the store's internal actor either, so a `CoroutineExceptionHandler` here sees failures from the + * `*Async` writes, not from DataStore itself. */ constructor( context: Context, @@ -83,7 +93,7 @@ abstract class BaseDataStoreHelper( scope: CoroutineScope = CoroutineScope(dispatcher + SupervisorJob()), ) : this( dataStore = PreferenceDataStoreFactory.create( - scope = CoroutineScope(dispatcher + SupervisorJob()), + scope = CoroutineScope(Dispatchers.IO + SupervisorJob()), produceFile = { context.applicationContext.preferencesDataStoreFile(preferenceName) }, ), dispatcher = dispatcher, diff --git a/README.md b/README.md index 4889882..7bf85ae 100644 --- a/README.md +++ b/README.md @@ -96,6 +96,8 @@ class AppPrefs( } ``` +Be clear about what that handler does and doesn't cover: `scope` owns the `*Async` writes and the delegate setters, so it sees their failures. It does **not** own DataStore's own internal actor, which has its own `SupervisorJob` — a corrupted-file or disk error surfacing from inside DataStore won't reach this handler. Injecting a scope is not "now I catch everything prefs-related". + ### Injecting a `DataStore` for tests `BaseDataStoreHelper`'s primary constructor takes the `DataStore` directly, so tests can supply one backed by a temp file and a test scheduler and drop the sleep-and-hope pattern entirely. Forward both constructors from your subclass: @@ -118,15 +120,22 @@ class AppPrefs : BaseDataStoreHelper { Then, in a test: ```kotlin +@get:Rule val tempFolder = TemporaryFolder() +private var fileCount = 0 + val store = PreferenceDataStoreFactory.create( scope = CoroutineScope(UnconfinedTestDispatcher(testScheduler) + Job()), - produceFile = { tempFolder.newFile("test.preferences_pb") }, + // Not tempFolder.newFile(...) — that pre-creates the file, and throws if + // produceFile is ever invoked twice. Hand DataStore a path it owns. + produceFile = { File(tempFolder.root, "test_${fileCount++}.preferences_pb") }, ) val prefs = AppPrefs(store, StandardTestDispatcher(testScheduler), backgroundScope) ``` Give every test a fresh file — DataStore throws if two live instances point at the same one — and cancel the store's scope in teardown to release the file lock. +The library's own `BaseDataStoreHelperInjectionTest` is a worked example of this if you want something to copy. + ### Composing a single façade When an app stores preferences across several backing files (e.g. a per-user SharedPreferences plus a device-wide DataStore), it's convenient to wrap them in one façade so consumers only inject one type and cross-cutting operations (like "clear everything on logout") live in one place. Each property on the façade can re-expose a sub-helper's delegate via a Kotlin property reference (`by subHelper::property`): @@ -163,12 +172,12 @@ Each type exposes a non-nullable delegate `*Pref(key, defaultValue)` and a nulla **Most subclasses need no change at all.** If you extend `BasePrefsHelper()` or `BaseDataStoreHelper(context, "name")` without passing a coroutine context of your own, you are already on the new defaults. -**Only one item on this list can change behaviour without the compiler telling you: item 2.** Everything else is a removed or retyped symbol, so it fails the build and you'll find it immediately. Read item 2 properly. +**Two items on this list change behaviour without the compiler telling you: items 2 and 5.** The rest are removed or retyped symbols, so they fail the build and you'll find them immediately. Item 2 is the one that can corrupt state — read it properly. Item 5 only bites a specific threading arrangement. A beta is published for testing ahead of the 2.0.0 release: ```kotlin -implementation("com.github.projectdelta6:PrefsHelperBase:2.0.0-beta01") +implementation("com.github.projectdelta6:PrefsHelperBase:2.0.0-beta03") ``` ### 1. `coroutineContext` → `dispatcher` (+ optional `scope`) @@ -232,7 +241,9 @@ DataStore is already main-safe, so the hop bought nothing and the `Job` it carri ### 6. New, not breaking: the `DataStore` is injectable -`BaseDataStoreHelper`'s primary constructor now takes a `DataStore` directly. Nothing forces you to use it — the `(context, preferenceName)` convenience constructor is unchanged — but it's what finally makes DataStore-backed subclasses unit-testable without sleeps. See [Injecting a `DataStore` for tests](#injecting-a-datastore-for-tests). +`BaseDataStoreHelper`'s primary constructor now takes a `DataStore` directly. Nothing forces you to use it — the `(context, preferenceName)` convenience constructor is unchanged, in signature *and* in scheduling — but it's what finally makes DataStore-backed subclasses unit-testable without sleeps. See [Injecting a `DataStore` for tests](#injecting-a-datastore-for-tests). + +The convenience constructor deliberately keeps the store's own internal actor on `Dispatchers.IO` whatever `dispatcher` you pass, exactly as the old `preferencesDataStore` delegate did. Binding the store to a caller-supplied dispatcher would be a silent behaviour change, and a confined one (`Main`, single-threaded, a paused test dispatcher) would deadlock `readValueBlocking` against its own store. If you actually want to control where the store schedules, use the primary constructor. ## R8 / ProGuard / Minification diff --git a/app/src/test/java/com/duck/app/BaseDataStoreHelperInjectionTest.kt b/app/src/test/java/com/duck/app/BaseDataStoreHelperInjectionTest.kt index 19d2fb7..d6e38dd 100644 --- a/app/src/test/java/com/duck/app/BaseDataStoreHelperInjectionTest.kt +++ b/app/src/test/java/com/duck/app/BaseDataStoreHelperInjectionTest.kt @@ -248,6 +248,61 @@ class BaseDataStoreHelperInjectionTest { assertNull(helper.testReadStringFlow("clear_key").first()) } + /** + * The redesign's claim is that `clearPrefs` *and the suspending writes* became caller-cancellable, + * so the writes need their own guard rather than riding on the clearPrefs tests. + * + * The nullable `writeValue` overload is the one that routes through `withContext(dispatcher)`, + * so it's the path that changed. Same shape as the clearPrefs pair: cancel before advancing and + * the write must not land. + */ + @Test + fun testSuspendingWriteIsCallerCancellable() = runTest { + val writeDispatcher = StandardTestDispatcher(testScheduler) + val helper = InjectableDataStoreHelper( + dataStore = newDataStore(testScheduler), + dispatcher = writeDispatcher, + scope = CoroutineScope(UnconfinedTestDispatcher(testScheduler) + SupervisorJob()), + ) + + val writeJob = launch(start = CoroutineStart.UNDISPATCHED) { + helper.testWriteString("cancelled_write", "should_not_land") + } + // Suspended inside withContext(writeDispatcher) — the edit has not run. + assertTrue(writeJob.isActive) + writeJob.cancel() + assertTrue(writeJob.isCancelled) + + advanceUntilIdle() + + assertNull( + "Cancelled write must not reach the store", + helper.testReadStringFlow("cancelled_write").first(), + ) + } + + /** + * Positive path companion to [testSuspendingWriteIsCallerCancellable], so that test cannot pass + * merely because the write never worked. + */ + @Test + fun testSuspendingWriteLandsWhenNotCancelled() = runTest { + val writeDispatcher = StandardTestDispatcher(testScheduler) + val helper = InjectableDataStoreHelper( + dataStore = newDataStore(testScheduler), + dispatcher = writeDispatcher, + scope = CoroutineScope(UnconfinedTestDispatcher(testScheduler) + SupervisorJob()), + ) + + val writeJob = launch(start = CoroutineStart.UNDISPATCHED) { + helper.testWriteString("landed_write", "should_land") + } + advanceUntilIdle() + writeJob.join() + + assertEquals("should_land", helper.testReadStringFlow("landed_write").first()) + } + // endregion // region 5 — Same cancellation guard for BasePrefsHelper diff --git a/jitpack.yml b/jitpack.yml index e457be2..78297ff 100644 --- a/jitpack.yml +++ b/jitpack.yml @@ -13,11 +13,15 @@ # # `install:` overrides JitPack's default, which would be: # ./gradlew clean -Pgroup=$GROUP -Pversion=$VERSION -xtest -xlint assemble publishToMavenLocal -# That `assemble` builds the :app sample module too — over half the build for -# something that is never published. Scoping to :PrefsHelper takes the build -# from ~1m53s to ~12s. GROUP and VERSION are JitPack-provided environment -# variables and MUST be passed through, or the artifact publishes as -# "unspecified". +# That `assemble` builds the :app sample module too — something that is never +# published. Scoping to :PrefsHelper drops the build from 158 executed tasks to +# 31 (measured: 2.0.0-beta01 vs 2.0.0-beta02 build logs). Wall time barely moves +# on JitPack, because a cold build there is dominated by downloading the Gradle +# distribution and resolving dependencies, not by task execution — the win is +# in work done and things that can break, not minutes. +# +# GROUP and VERSION are JitPack-provided environment variables and MUST be +# passed through, or the artifact publishes as "unspecified". # # Only :PrefsHelper applies maven-publish, so it remains the single published # module and consumers keep resolving it as com.github.projectdelta6:PrefsHelperBase. From cf903c5f720c1d548bc333af982317a37790f0b7 Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 11:32:43 +0100 Subject: [PATCH 08/17] Make produceFile idempotent in the DataStore test example MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DataStore's produceFile is contractually required to yield the same file on every invocation. Both the injection test and the README example called a counter-incrementing helper from inside the lambda, so a second invocation would have handed DataStore a different path — a worse failure than the tempFolder.newFile form this replaced, which would at least have thrown. Current DataStore resolves produceFile once via a lazy, so neither form misbehaves today. This is about not depending on that, and about the README being a safe thing to copy. Also renames the region the write-cancellation tests live in; they had been added under "region 4 — clearPrefs() is caller-cancellable". Mutation check re-run after the change: restoring the foreign Job on either clearPrefs or the nullable writeValue still fails exactly its own guard, so the rework didn't weaken the tests. Both raised by Grok's re-review as optional nits. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 9 ++++++--- .../com/duck/app/BaseDataStoreHelperInjectionTest.kt | 10 +++++++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 7bf85ae..596792d 100644 --- a/README.md +++ b/README.md @@ -123,11 +123,14 @@ Then, in a test: @get:Rule val tempFolder = TemporaryFolder() private var fileCount = 0 +// Resolve the path up front and close over it — produceFile must yield the same +// file every time. Not tempFolder.newFile(...): that pre-creates the file and +// throws if it's ever invoked twice. +val file = File(tempFolder.root, "test_${fileCount++}.preferences_pb") + val store = PreferenceDataStoreFactory.create( scope = CoroutineScope(UnconfinedTestDispatcher(testScheduler) + Job()), - // Not tempFolder.newFile(...) — that pre-creates the file, and throws if - // produceFile is ever invoked twice. Hand DataStore a path it owns. - produceFile = { File(tempFolder.root, "test_${fileCount++}.preferences_pb") }, + produceFile = { file }, ) val prefs = AppPrefs(store, StandardTestDispatcher(testScheduler), backgroundScope) ``` diff --git a/app/src/test/java/com/duck/app/BaseDataStoreHelperInjectionTest.kt b/app/src/test/java/com/duck/app/BaseDataStoreHelperInjectionTest.kt index d6e38dd..ddc8234 100644 --- a/app/src/test/java/com/duck/app/BaseDataStoreHelperInjectionTest.kt +++ b/app/src/test/java/com/duck/app/BaseDataStoreHelperInjectionTest.kt @@ -74,9 +74,13 @@ class BaseDataStoreHelperInjectionTest { private fun newDataStore(testScheduler: TestCoroutineScheduler): DataStore { val storeScope = CoroutineScope(UnconfinedTestDispatcher(testScheduler) + Job()) storeScopes += storeScope + // Resolve the path up front and close over it: produceFile is contractually required to + // yield the same file every time. Calling newPreferencesFile() inside the lambda would + // hand DataStore a different path on any second invocation. + val file = newPreferencesFile() return PreferenceDataStoreFactory.create( scope = storeScope, - produceFile = { newPreferencesFile() }, + produceFile = { file }, ) } @@ -248,6 +252,10 @@ class BaseDataStoreHelperInjectionTest { assertNull(helper.testReadStringFlow("clear_key").first()) } + // endregion + + // region 4b — Suspending writes are caller-cancellable + /** * The redesign's claim is that `clearPrefs` *and the suspending writes* became caller-cancellable, * so the writes need their own guard rather than riding on the clearPrefs tests. From 7d448a8ab3b84d428a413594f26c04106c7d54cc Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 11:36:55 +0100 Subject: [PATCH 09/17] README housekeeping - The DataStore usage section still described writes going to "the library's internal scope". That scope is injectable as of 2.0, so it now says so and links to the section explaining it. - Install section gains the two facts a consumer needs before adding the dependency: minSdk 21, and that datastore-preferences is an api dependency so DataStore is visible without declaring it. Plus a pointer to the migration guide, since 2.x is breaking. - Make the NonCancellable logout snippet valid Kotlin rather than a bare ellipsis inside braces, matching the comment style used in the other snippets. All three internal anchors verified to resolve against the heading slugs. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 596792d..aae660d 100644 --- a/README.md +++ b/README.md @@ -24,6 +24,10 @@ dependencies { } ``` +`minSdk 21`. `datastore-preferences` is exposed as an `api` dependency, so `DataStore` is visible to you without declaring it yourself. + +Upgrading from 1.x? See [Migrating to 2.0](#migrating-to-20) — it is a breaking release. + ## Usage ### `BasePrefsHelper` (SharedPreferences) @@ -47,7 +51,7 @@ class UserPrefs(context: Context) : BasePrefsHelper() { ### `BaseDataStoreHelper` (DataStore) -Subclass `BaseDataStoreHelper`, pass a `Context` and DataStore name. Use `*Pref` for imperative read/write and the matching `*PrefFlow` for reactive observation — the delegate setter dispatches to the library's internal scope so you don't need to open your own `CoroutineScope`. +Subclass `BaseDataStoreHelper`, pass a `Context` and DataStore name. Use `*Pref` for imperative read/write and the matching `*PrefFlow` for reactive observation — the delegate setter dispatches to the helper's `scope`, so you don't need to open your own `CoroutineScope`. That scope is injectable if you want to own it; see [Coroutines: `dispatcher` and `scope`](#coroutines-dispatcher-and-scope). ```kotlin class AppPrefs(context: Context) : BaseDataStoreHelper(context, "app_prefs") { @@ -216,7 +220,9 @@ suspend fun onLogout() { if that runs on a screen-scoped coroutine (`viewModelScope`, `screenModelScope`) and the user navigates away mid-call, you can now end up with SharedPreferences cleared and DataStore not. Fix it deliberately — either run logout on an application-lifetime scope, or wrap the body: ```kotlin -suspend fun onLogout() = withContext(NonCancellable) { … } +suspend fun onLogout() = withContext(NonCancellable) { + // … the clears, as before +} ``` Anywhere else you relied on a prefs write surviving its caller's cancellation needs the same treatment. From 8ad89bfcb38ae49e33a90a0cbb10fbf0e7692d17 Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 11:45:56 +0100 Subject: [PATCH 10/17] Give both helpers the same set of supported types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BasePrefsHelper gains Double; BaseDataStoreHelper gains Date. Both were previously supported by only one of the two, which made the "supported types" story awkward to explain and forced consumers to pick a backend partly on type coverage. SharedPreferences has no double primitive, so BasePrefsHelper stores a Double as its raw IEEE-754 bit pattern via putLong, read back with Double.fromBits. This round-trips every value exactly, including the denormals and infinities a Float-based encoding would mangle. The one sharp edge is documented on setDouble: reading that key with getLong returns the bit pattern, not the number. BaseDataStoreHelper stores a Date as epoch millis in a longPreferencesKey and removes the key on null, matching how it already handles its other temporal types rather than importing BasePrefsHelper's -1L sentinel. That means DataStore can represent Date(-1L) distinctly from absent, where SharedPreferences cannot — documented rather than papered over. Both additions get the full surface: read/write, async write, Flow accessors and blocking reads on the DataStore side, plus non-null and nullable *Pref delegates on both. Tests: 183 -> 199, all passing, coverage gate holding. Date coverage includes the epoch and pre-epoch boundaries, which is exactly where a sentinel-based scheme would have failed. Docs: README "Supported types" now states the shared list plus a table of the storage differences that genuinely remain between the backends; same in CLAUDE.md. Also drops the beta coordinate from the README, since those builds won't outlive the merge. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 6 +- .../duck/prefshelper/BaseDataStoreHelper.kt | 95 +++++++++++++++++++ .../com/duck/prefshelper/BasePrefsHelper.kt | 53 +++++++++++ README.md | 27 ++++-- .../com/duck/app/BaseDataStoreHelperTest.kt | 59 ++++++++++++ .../java/com/duck/app/BasePrefsHelperTest.kt | 86 +++++++++++++++++ 6 files changed, 315 insertions(+), 11 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index fec8867..ea88e1d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,7 +57,11 @@ The library provides two abstract base classes that consumers extend: - Null values remove the key from storage - Preferred usage is the `*Pref` delegate + `*PrefFlow` alias pair (e.g. `var userId by intPref(KEY, defaultValue = -1)` paired with `val userIdFlow = intPrefFlow(KEY, defaultValue = -1)`). Delegate setters route through the existing `*Async` writes so callers don't need to build their own `CoroutineScope`. -Both classes support: `String`, `Int`, `Long`, `Boolean`, `LocalDateTime`, `LocalDate`, `LocalTime`, `Enum<*>`. `BasePrefsHelper` additionally supports `Date`; `BaseDataStoreHelper` additionally supports `Double`. +Both classes support the same types as of 2.0: `String`, `Int`, `Long`, `Double`, `Boolean`, `Date`, `LocalDateTime`, `LocalDate`, `LocalTime`, `Enum<*>`. + +Storage conventions still differ by backend, and deliberately so: +- **`Double` on `BasePrefsHelper`** is stored as raw IEEE-754 bits via `putLong`/`Double.fromBits`, because `SharedPreferences` has no double primitive. Reading that key with `getLong` returns the bit pattern, not the number. +- **`Date` on `BaseDataStoreHelper`** is epoch millis in a `longPreferencesKey`, and null removes the key — unlike `BasePrefsHelper`, whose temporal types use the `-1L` sentinel and therefore cannot represent `Date(-1L)` distinctly from null. ## Gotchas diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt index 4a6dc40..6caeeaf 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt @@ -30,6 +30,7 @@ import kotlin.time.Duration.Companion.seconds import java.time.LocalDate import java.time.LocalDateTime import java.time.LocalTime +import java.util.Date import kotlin.properties.ReadWriteProperty import kotlin.reflect.KProperty @@ -364,6 +365,68 @@ abstract class BaseDataStoreHelper( protected fun readDoubleValue(key: String, default: Double): Double = readValueBlocking(doublePreferencesKey(key), default) + /** + * Add [Date] to the data store + * + * Stored as epoch milliseconds in a [Long]. If value is null, the key will be removed — note + * this differs from `BasePrefsHelper`, which writes a -1L sentinel for temporal types. + * + * @param key The key to store the value under + * @param value The value to store + */ + protected suspend fun writeDate(key: String, value: Date?) = + writeValue(longPreferencesKey(key), value?.time) + + /** + * Add [Date] to the data store asynchronously + * + * If value is null, the key will be removed. + * + * @param key The key to store the value under + * @param value The value to store + * @return The [Job] for the launched write — call [Job.join] to await completion. + */ + protected fun writeDateAsync(key: String, value: Date?): Job = + writeValueAsync(longPreferencesKey(key), value?.time) + + /** + * Read a [Date] preference as a [Flow]. + * + * @param key The key to read the value for + * @return A [Flow] emitting the [Date] stored under the key, or null if the key does not exist + */ + protected fun readDate(key: String): Flow = + readValue(longPreferencesKey(key)).map { it?.let { millis -> Date(millis) } } + + /** + * Read a [Date] preference as a [Flow] with a non-null default. + * + * @param key The key to read the value for + * @param default The default value to return if the key does not exist + * @return A [Flow] emitting the [Date] stored under the key, or the default value + */ + protected fun readDate(key: String, default: Date): Flow = + readValue(longPreferencesKey(key)).map { it?.let { millis -> Date(millis) } ?: default } + + /** + * Read a [Date] preference in a blocking way. + * + * @param key The key to read the value for + * @return The [Date] or null if the key does not exist + */ + protected fun readDateValue(key: String): Date? = + readValueBlocking(longPreferencesKey(key))?.let { Date(it) } + + /** + * Read a [Date] preference in a blocking way with a default value. + * + * @param key The key to read the value for + * @param default The default value to return if the key does not exist + * @return The [Date] stored under the key, or the default value + */ + protected fun readDateValue(key: String, default: Date): Date = + readValueBlocking(longPreferencesKey(key))?.let { Date(it) } ?: default + /** * Add [String] data to data Store * @@ -974,6 +1037,38 @@ abstract class BaseDataStoreHelper( */ protected fun doublePrefFlow(key: String): Flow = readDouble(key) + /** + * Create a property delegate for a [Date] preference. + */ + protected fun datePref(key: String, defaultValue: Date): ReadWriteProperty = + object : ReadWriteProperty { + override fun getValue(thisRef: Any?, property: KProperty<*>): Date = readDateValue(key, defaultValue) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Date) { + writeDateAsync(key, value) + } + } + + /** + * Create a property delegate for a nullable [Date] preference. + */ + protected fun datePref(key: String): ReadWriteProperty = + object : ReadWriteProperty { + override fun getValue(thisRef: Any?, property: KProperty<*>): Date? = readDateValue(key) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Date?) { + writeDateAsync(key, value) + } + } + + /** + * [Flow] accessor for a [Date] preference — alias for [readDate]. + */ + protected fun datePrefFlow(key: String, defaultValue: Date): Flow = readDate(key, defaultValue) + + /** + * [Flow] accessor for a nullable [Date] preference — alias for [readDate]. + */ + protected fun datePrefFlow(key: String): Flow = readDate(key) + /** * Create a property delegate for a [Boolean] preference. */ diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt index 80a8550..6a29700 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt @@ -118,6 +118,33 @@ abstract class BasePrefsHelper( return sharedPreferences.getLong(key, defValue) } + /** + * Set a [Double] preference + * + * [SharedPreferences] has no double primitive, so the value is stored as its raw IEEE-754 bit + * pattern in a [Long]. Always read it back with [getDouble] — calling [getLong] on the same key + * returns the bit pattern, not the number. + * + * @param key The key to store the value under + * @param value The value to store + */ + fun setDouble(key: String, value: Double) { + sharedPreferences.edit { + putLong(key, value.toRawBits()) + } + } + + /** + * Get a [Double] preference + * + * @param key The key to get the value for + * @param defValue The default value to return if the key does not exist, defaults to 0.0 + */ + fun getDouble(key: String, defValue: Double = 0.0): Double { + if (!sharedPreferences.contains(key)) return defValue + return Double.fromBits(sharedPreferences.getLong(key, 0L)) + } + /** * Set a [Date] preference * @@ -346,6 +373,32 @@ abstract class BasePrefsHelper( } } + /** + * Create a property delegate for a [Double] preference. + * + * @param key The key to read/write the value for + * @param defaultValue Value returned if the key is absent + */ + protected fun doublePref(key: String, defaultValue: Double): ReadWriteProperty = + object : ReadWriteProperty { + override fun getValue(thisRef: Any?, property: KProperty<*>): Double = getDouble(key, defaultValue) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Double) = setDouble(key, value) + } + + /** + * Create a property delegate for a nullable [Double] preference. + * + * Returns null when the key is absent. Assigning null removes the key. + */ + protected fun doublePref(key: String): ReadWriteProperty = + object : ReadWriteProperty { + override fun getValue(thisRef: Any?, property: KProperty<*>): Double? = + if (contains(key)) getDouble(key) else null + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Double?) { + if (value == null) sharedPreferences.edit { remove(key) } else setDouble(key, value) + } + } + /** * Create a property delegate for a [Boolean] preference. * diff --git a/README.md b/README.md index aae660d..1bec306 100644 --- a/README.md +++ b/README.md @@ -166,12 +166,21 @@ class PrefsHelper(context: Context) { ## Supported types -Both `BasePrefsHelper` and `BaseDataStoreHelper` support `String`, `Int`, `Long`, `Boolean`, `LocalDateTime`, `LocalDate`, `LocalTime`, and `Enum<*>`. In addition: +`BasePrefsHelper` and `BaseDataStoreHelper` support the same set of types as of 2.0: -- `BasePrefsHelper` supports `Date`. -- `BaseDataStoreHelper` supports `Double`. +`String`, `Int`, `Long`, `Double`, `Boolean`, `Date`, `LocalDateTime`, `LocalDate`, `LocalTime`, and `Enum<*>`. -Each type exposes a non-nullable delegate `*Pref(key, defaultValue)` and a nullable delegate `*Pref(key)` (assigning `null` clears the stored value on DataStore, or stores the sentinel on SharedPreferences for temporal types). `BaseDataStoreHelper` additionally exposes matching `*PrefFlow` accessors for reactive reads. +Each type exposes a non-nullable delegate `*Pref(key, defaultValue)` and a nullable delegate `*Pref(key)`. `BaseDataStoreHelper` additionally exposes matching `*PrefFlow` accessors for reactive reads. + +The types match, but the two backends still store and clear them differently — that's inherent to `SharedPreferences` versus `DataStore`, not an oversight: + +| | `BasePrefsHelper` | `BaseDataStoreHelper` | +|---|---|---| +| Assigning `null` | Removes the key for `String`/`Int`/`Long`/`Double`/`Boolean`; writes a `-1L` sentinel for the temporal types | Always removes the key | +| `Double` storage | Raw IEEE-754 bits in a `Long` — `SharedPreferences` has no double primitive, so never read the key back with `getLong` | Native `doublePreferencesKey` | +| `Date` storage | Epoch millis, with `-1L` doubling as "absent" | Epoch millis in a `Long`, absent means absent | + +That last row matters if you store pre-epoch dates: `BasePrefsHelper` cannot distinguish `Date(-1L)` from null, `BaseDataStoreHelper` can. ## Migrating to 2.0 @@ -181,12 +190,6 @@ Each type exposes a non-nullable delegate `*Pref(key, defaultValue)` and a nulla **Two items on this list change behaviour without the compiler telling you: items 2 and 5.** The rest are removed or retyped symbols, so they fail the build and you'll find them immediately. Item 2 is the one that can corrupt state — read it properly. Item 5 only bites a specific threading arrangement. -A beta is published for testing ahead of the 2.0.0 release: - -```kotlin -implementation("com.github.projectdelta6:PrefsHelperBase:2.0.0-beta03") -``` - ### 1. `coroutineContext` → `dispatcher` (+ optional `scope`) ```kotlin @@ -254,6 +257,10 @@ DataStore is already main-safe, so the hop bought nothing and the `Job` it carri The convenience constructor deliberately keeps the store's own internal actor on `Dispatchers.IO` whatever `dispatcher` you pass, exactly as the old `preferencesDataStore` delegate did. Binding the store to a caller-supplied dispatcher would be a silent behaviour change, and a confined one (`Main`, single-threaded, a paused test dispatcher) would deadlock `readValueBlocking` against its own store. If you actually want to control where the store schedules, use the primary constructor. +### 7. New, not breaking: the two helpers now support the same types + +`BasePrefsHelper` gains `Double`, `BaseDataStoreHelper` gains `Date`. Nothing existing changes — these are additions, and both follow their helper's established conventions. See [Supported types](#supported-types) for the storage differences that remain between the two backends. + ## R8 / ProGuard / Minification PrefsHelper is fully compatible with R8 (including full mode) and requires **no consumer ProGuard rules** (the shipped `consumer-rules.pro` is intentionally empty). Preference keys are always explicit string arguments you pass to the `*Pref(...)` delegates — they are **never** derived from Kotlin property names via reflection. R8 is free to rename, merge, and repackage your `BasePrefsHelper`/`BaseDataStoreHelper` subclasses and their properties without changing any persisted key. Enum values are stored by `Enum.name` (preserved by R8's default Android rules), so enum prefs survive obfuscation. diff --git a/app/src/test/java/com/duck/app/BaseDataStoreHelperTest.kt b/app/src/test/java/com/duck/app/BaseDataStoreHelperTest.kt index a2fca41..22cfa1d 100644 --- a/app/src/test/java/com/duck/app/BaseDataStoreHelperTest.kt +++ b/app/src/test/java/com/duck/app/BaseDataStoreHelperTest.kt @@ -88,6 +88,58 @@ class BaseDataStoreHelperTest { assertEquals(3.14, value ?: 0.0, 0.01) } + @Test + fun testWriteAndReadDate() = runBlocking { + val date = java.util.Date(1_700_000_000_000L) + dataStoreHelper.testWriteDate("date_key", date) + assertEquals(date, dataStoreHelper.testReadDateValue("date_key")) + } + + /** + * Date is stored as epoch millis, so it must survive the epoch itself and pre-epoch (negative) + * values — the case a naive -1L sentinel would corrupt. + */ + @Test + fun testDateBoundaryValues() = runBlocking { + val epoch = java.util.Date(0L) + dataStoreHelper.testWriteDate("epoch", epoch) + assertEquals(epoch, dataStoreHelper.testReadDateValue("epoch")) + + val preEpoch = java.util.Date(-1L) + dataStoreHelper.testWriteDate("pre_epoch", preEpoch) + assertEquals(preEpoch, dataStoreHelper.testReadDateValue("pre_epoch")) + } + + @Test + fun testWriteNullDateRemovesKey() = runBlocking { + dataStoreHelper.testWriteDate("null_date_key", java.util.Date(1_700_000_000_000L)) + assertNotNull(dataStoreHelper.testReadDateValue("null_date_key")) + + dataStoreHelper.testWriteDate("null_date_key", null) + assertNull(dataStoreHelper.testReadDateValue("null_date_key")) + } + + @Test + fun testReadDateFlow() = runBlocking { + val date = java.util.Date(1_600_000_000_000L) + dataStoreHelper.testWriteDate("date_flow_key", date) + assertEquals(date, dataStoreHelper.testReadDateFlow("date_flow_key").first()) + } + + @Test + fun testReadDateValueWithDefault() = runBlocking { + val fallback = java.util.Date(42L) + assertEquals(fallback, dataStoreHelper.testReadDateValueWithDefault("no_such_date", fallback)) + assertEquals(fallback, dataStoreHelper.testReadDateFlowWithDefault("no_such_date", fallback).first()) + } + + @Test + fun testWriteDateAsync() = runBlocking { + val date = java.util.Date(1_500_000_000_000L) + dataStoreHelper.testWriteDateAsync("date_async_key", date).join() + assertEquals(date, dataStoreHelper.testReadDateValue("date_async_key")) + } + @Test fun testWriteAndReadBoolean() = runBlocking { dataStoreHelper.testWriteBoolean("bool_key", true) @@ -905,6 +957,13 @@ class BaseDataStoreHelperTest { fun testReadDoubleFlowWithDefault(key: String, default: Double) = readDouble(key, default) fun testWriteDoubleAsync(key: String, value: Double?) = writeDoubleAsync(key, value) + suspend fun testWriteDate(key: String, value: java.util.Date?) = writeDate(key, value) + fun testReadDateValue(key: String) = readDateValue(key) + fun testReadDateValueWithDefault(key: String, default: java.util.Date) = readDateValue(key, default) + fun testReadDateFlow(key: String) = readDate(key) + fun testReadDateFlowWithDefault(key: String, default: java.util.Date) = readDate(key, default) + fun testWriteDateAsync(key: String, value: java.util.Date?) = writeDateAsync(key, value) + suspend fun testWriteBoolean(key: String, value: Boolean?) = writeBoolean(key, value) fun testReadBooleanValue(key: String) = readBooleanValue(key) fun testReadBooleanValueWithDefault(key: String, default: Boolean) = readBooleanValue(key, default) diff --git a/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt b/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt index 0dfbd04..827a0aa 100644 --- a/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt +++ b/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt @@ -87,6 +87,49 @@ class BasePrefsHelperTest { verify(mockEditor).apply() } + @Test + fun testSetDouble() { + prefsHelper.setDouble("key", 3.14) + // SharedPreferences has no double primitive — stored as raw IEEE-754 bits in a Long. + verify(mockEditor).putLong("key", 3.14.toRawBits()) + verify(mockEditor).apply() + } + + @Test + fun testGetDouble() { + `when`(mockSharedPreferences.contains("key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("key", 0L)).thenReturn(3.14.toRawBits()) + assertEquals(3.14, prefsHelper.getDouble("key"), 0.0) + } + + @Test + fun testGetDoubleReturnsDefaultWhenKeyAbsent() { + `when`(mockSharedPreferences.contains("key")).thenReturn(false) + assertEquals(2.5, prefsHelper.getDouble("key", 2.5), 0.0) + } + + /** + * The raw-bits encoding must round-trip every double exactly, including the values a + * lossy Float-based encoding would mangle. Mirrors the Double coverage + * `BaseDataStoreHelperTest` has, so both helpers are held to the same standard. + */ + @Test + fun testDoubleBitEncodingRoundTripsExactly() { + val values = listOf( + 0.0, + -0.0, + 3.14159265358979, + Double.MAX_VALUE, + Double.MIN_VALUE, + Double.POSITIVE_INFINITY, + Double.NEGATIVE_INFINITY, + ) + for (value in values) { + assertEquals(value, Double.fromBits(value.toRawBits()), 0.0) + } + assertTrue(Double.fromBits(Double.NaN.toRawBits()).isNaN()) + } + @Test fun testGetBoolean() { `when`(mockSharedPreferences.getBoolean("key", false)).thenReturn(true) @@ -460,6 +503,47 @@ class BasePrefsHelperTest { verify(mockEditor).apply() } + // Double delegate + @Test + fun testDoublePrefDelegateGet() { + `when`(mockSharedPreferences.contains("double_key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("double_key", 0L)).thenReturn(9.75.toRawBits()) + assertEquals(9.75, prefsHelper.doubleValue, 0.0) + } + + @Test + fun testDoublePrefDelegateFallsBackToDefault() { + `when`(mockSharedPreferences.contains("double_key")).thenReturn(false) + assertEquals(1.5, prefsHelper.doubleValue, 0.0) + } + + @Test + fun testDoublePrefDelegateSet() { + prefsHelper.doubleValue = 2.25 + verify(mockEditor).putLong("double_key", 2.25.toRawBits()) + verify(mockEditor).apply() + } + + @Test + fun testNullableDoublePrefReturnsNullWhenAbsent() { + `when`(mockSharedPreferences.contains("nullable_double_key")).thenReturn(false) + assertNull(prefsHelper.nullableDouble) + } + + @Test + fun testNullableDoublePrefReturnsValueWhenPresent() { + `when`(mockSharedPreferences.contains("nullable_double_key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("nullable_double_key", 0L)).thenReturn((-0.5).toRawBits()) + assertEquals(-0.5, prefsHelper.nullableDouble!!, 0.0) + } + + @Test + fun testNullableDoublePrefRemovesKeyOnNullAssignment() { + prefsHelper.nullableDouble = null + verify(mockEditor).remove("nullable_double_key") + verify(mockEditor).apply() + } + // Boolean delegate @Test fun testBooleanPrefDelegateGet() { @@ -603,6 +687,8 @@ class BasePrefsHelperTest { var maybeInt by intPref("maybe_int") var longValue by longPref("long_key", defaultValue = 5L) var nullableLong by longPref("nullable_long_key") + var doubleValue by doublePref("double_key", defaultValue = 1.5) + var nullableDouble by doublePref("nullable_double_key") var boolValue by booleanPref("bool_key", defaultValue = true) var nullableBool by booleanPref("nullable_bool_key") var dateValue by datePref("date_delegate_key") From 7f221cb4a3bca6ad5eab9f7ec685abfc8be44884 Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 12:38:05 +0100 Subject: [PATCH 11/17] Add Float, Set, ByteArray, Instant and Set to both helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Float and Set were the real gaps: both are first-class on SharedPreferences AND DataStore, so their absence was an oversight rather than a design decision. ByteArray, Instant and Set round out the set. Backend-specific encodings, all documented at the call site: - Float and Set are native on both backends. - ByteArray is native on DataStore; Base64 (NO_WRAP) in a String on SharedPreferences, which has no binary type. Undecodable data logs and returns null instead of throwing. - Instant is epoch millis: null removes the key on DataStore, and uses the established -1L sentinel on SharedPreferences alongside the other temporal types. - Set is a set of Enum.name on both. Names that no longer match a constant are dropped on read, so deleting an enum value doesn't break existing installs. Set copies on both read and write for SharedPreferences. The platform documents getStringSet's result as one callers must not modify, and keeps a reference to the set it is handed on write — both directions are traps, so both are copied. Tested from both sides. enumSetPref uses the same thin-inline + @PublishedApi-internal split as enumPref, for the IllegalAccessError gotcha in CLAUDE.md. This is verified, not assumed: collapsing the split so the inline function returns the anonymous object directly makes exactly the three enumSetPref delegate tests fail with IllegalAccessError, and nothing else. Noted in CLAUDE.md that only a subclass-in-another-module delegate test catches this. New BasePrefsHelperRealPrefsTest covers what mocks structurally cannot: the Double bit encoding, Base64, and the Set copies against a real Robolectric SharedPreferences. That also closes the gap flagged when Double was added — its round-trip was previously only proven by composition. Tests 199 -> 298, all passing, coverage gate holding. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 11 +- .../duck/prefshelper/BaseDataStoreHelper.kt | 462 +++++++++++++++ .../com/duck/prefshelper/BasePrefsHelper.kt | 243 ++++++++ README.md | 23 +- .../com/duck/app/BaseDataStoreHelperTest.kt | 531 ++++++++++++++++++ .../duck/app/BasePrefsHelperRealPrefsTest.kt | 228 ++++++++ .../java/com/duck/app/BasePrefsHelperTest.kt | 213 +++++++ 7 files changed, 1700 insertions(+), 11 deletions(-) create mode 100644 app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt diff --git a/CLAUDE.md b/CLAUDE.md index ea88e1d..515769d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,15 +57,20 @@ The library provides two abstract base classes that consumers extend: - Null values remove the key from storage - Preferred usage is the `*Pref` delegate + `*PrefFlow` alias pair (e.g. `var userId by intPref(KEY, defaultValue = -1)` paired with `val userIdFlow = intPrefFlow(KEY, defaultValue = -1)`). Delegate setters route through the existing `*Async` writes so callers don't need to build their own `CoroutineScope`. -Both classes support the same types as of 2.0: `String`, `Int`, `Long`, `Double`, `Boolean`, `Date`, `LocalDateTime`, `LocalDate`, `LocalTime`, `Enum<*>`. +Both classes support the same types as of 2.0: `String`, `Int`, `Long`, `Float`, `Double`, `Boolean`, `ByteArray`, `Set`, `Date`, `Instant`, `LocalDateTime`, `LocalDate`, `LocalTime`, `Enum<*>`, `Set`. Storage conventions still differ by backend, and deliberately so: - **`Double` on `BasePrefsHelper`** is stored as raw IEEE-754 bits via `putLong`/`Double.fromBits`, because `SharedPreferences` has no double primitive. Reading that key with `getLong` returns the bit pattern, not the number. -- **`Date` on `BaseDataStoreHelper`** is epoch millis in a `longPreferencesKey`, and null removes the key — unlike `BasePrefsHelper`, whose temporal types use the `-1L` sentinel and therefore cannot represent `Date(-1L)` distinctly from null. +- **`ByteArray` on `BasePrefsHelper`** is Base64 (`NO_WRAP`) in a String, since SharedPreferences has no binary type. Undecodable data logs and returns null rather than throwing. +- **`Set` on `BasePrefsHelper`** copies on both read and write. `SharedPreferences.getStringSet` documents its result as one callers must not modify, and the platform keeps a reference to the set it is handed — both directions are trapped, so both are copied. +- **`Date`/`Instant` on `BaseDataStoreHelper`** are epoch millis in a `longPreferencesKey` and null removes the key — unlike `BasePrefsHelper`, whose temporal types use the `-1L` sentinel and therefore cannot represent `-1L` millis distinctly from null. +- **`Set`** is stored as a set of `Enum.name` on both. Unknown names are dropped on read so deleting an enum constant doesn't break existing installs. ## Gotchas -- **Inline reified factories that access `protected` members**: an `inline fun ` that returns an anonymous object (e.g. a `ReadWriteProperty`) calling `protected` methods on `BaseDataStoreHelper` throws `IllegalAccessError` at runtime — the anonymous class is emitted inside the *subclass* at inline time, losing JVM-level protected access. Fix: split into a thin `inline` + `reified` wrapper that forwards to a non-inline `@PublishedApi internal` helper which owns the anonymous object. See `enumPref` / `enumPrefInternal` in `BaseDataStoreHelper.kt`. `BasePrefsHelper` is unaffected because its get/set accessors are `public`. +- **Inline reified factories that access `protected` members**: an `inline fun ` that returns an anonymous object (e.g. a `ReadWriteProperty`) calling `protected` methods on `BaseDataStoreHelper` throws `IllegalAccessError` at runtime — the anonymous class is emitted inside the *subclass* at inline time, losing JVM-level protected access. Fix: split into a thin `inline` + `reified` wrapper that forwards to a non-inline `@PublishedApi internal` helper which owns the anonymous object. See `enumPref` / `enumPrefInternal` and `enumSetPref` / `enumSetPrefInternal` in `BaseDataStoreHelper.kt`. `BasePrefsHelper` is unaffected because its get/set accessors are `public`. + +This only reproduces when the delegate is used **from a subclass in another module**, which is why `BaseDataStoreHelperTest`'s `TestDataStoreHelper` (in `:app`) declares `delegate`-prefixed properties for both enum delegates. A test that calls `readEnumSetValue` directly will not catch a regression here. - **Tests are JVM-only (Robolectric), not instrumented**: all three test classes live in `app/src/test` (`BasePrefsHelperTest`, `BaseDataStoreHelperTest`, and `BaseDataStoreHelperInjectionTest`). `BaseDataStoreHelperTest` uses a real `DataStore` but runs on the JVM via Robolectric (`@RunWith(AndroidJUnit4::class)` delegates to `RobolectricTestRunner` off-device). This is deliberate: **Kover cannot instrument on-device tests**, so DataStore coverage would read ~0% if the tests were instrumented — running them under Robolectric makes the `koverVerifyDebug` floor meaningful across both helpers. Robolectric's SDK is pinned to 36 in `app/src/test/resources/robolectric.properties` because `targetSdk = 37` has no Robolectric image yet. diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt index 6caeeaf..d6e96b9 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt @@ -7,12 +7,15 @@ import androidx.annotation.RequiresApi import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.Preferences import androidx.datastore.preferences.core.booleanPreferencesKey +import androidx.datastore.preferences.core.byteArrayPreferencesKey import androidx.datastore.preferences.core.doublePreferencesKey import androidx.datastore.preferences.core.edit +import androidx.datastore.preferences.core.floatPreferencesKey import androidx.datastore.preferences.core.intPreferencesKey import androidx.datastore.preferences.core.longPreferencesKey import androidx.datastore.preferences.core.PreferenceDataStoreFactory import androidx.datastore.preferences.core.stringPreferencesKey +import androidx.datastore.preferences.core.stringSetPreferencesKey import androidx.datastore.preferences.preferencesDataStoreFile import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope @@ -27,6 +30,7 @@ import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeoutOrNull import kotlin.time.Duration.Companion.seconds +import java.time.Instant import java.time.LocalDate import java.time.LocalDateTime import java.time.LocalTime @@ -365,6 +369,248 @@ abstract class BaseDataStoreHelper( protected fun readDoubleValue(key: String, default: Double): Double = readValueBlocking(doublePreferencesKey(key), default) + /** + * Adding [Float] to the data store + * + * If value is null, the key will be removed. + * + * @param key The key to store the value under + * @param value The value to store + */ + protected suspend fun writeFloat(key: String, value: Float?) = + writeValue(floatPreferencesKey(key), value) + + /** + * Add [Float] to the data store asynchronously + * + * @param key The key to store the value under + * @param value The value to store + * @return The [Job] for the launched write — call [Job.join] to await completion. + */ + protected fun writeFloatAsync(key: String, value: Float?): Job = + writeValueAsync(floatPreferencesKey(key), value) + + /** + * Reading the [Float] value from the data store + * + * @param key The key to read the value for + * @return Flow of the value or null if not present + */ + protected fun readFloat(key: String): Flow = + readValue(floatPreferencesKey(key)) + + /** + * Reading the [Float] value from the data store with a default + * + * @param key The key to read the value for + * @param default The default value to return if the key is not present + * @return Flow of the value + */ + protected fun readFloat(key: String, default: Float): Flow = + readValue(floatPreferencesKey(key), default) + + /** + * Read [Float] value in a blocking way from the data store preferences + * + * @param key The key to read the value for + * @return The value or null if not present + */ + protected fun readFloatValue(key: String): Float? = + readValueBlocking(floatPreferencesKey(key)) + + /** + * Read [Float] value in a blocking way from the data store preferences with a default + * + * @param key The key to read the value for + * @param default The default value to return if the key is not present + * @return The value + */ + protected fun readFloatValue(key: String, default: Float): Float = + readValueBlocking(floatPreferencesKey(key), default) + + /** + * Add a [Set] of [String] to the data store + * + * If value is null, the key will be removed. + * + * @param key The key to store the value under + * @param value The value to store + */ + protected suspend fun writeStringSet(key: String, value: Set?) = + writeValue(stringSetPreferencesKey(key), value) + + /** + * Add a [Set] of [String] to the data store asynchronously + * + * @param key The key to store the value under + * @param value The value to store + * @return The [Job] for the launched write — call [Job.join] to await completion. + */ + protected fun writeStringSetAsync(key: String, value: Set?): Job = + writeValueAsync(stringSetPreferencesKey(key), value) + + /** + * Reading the [Set] of [String] value from the data store + * + * @param key The key to read the value for + * @return Flow of the value or null if not present + */ + protected fun readStringSet(key: String): Flow?> = + readValue(stringSetPreferencesKey(key)) + + /** + * Reading the [Set] of [String] value from the data store with a default + * + * @param key The key to read the value for + * @param default The default value to return if the key is not present + * @return Flow of the value + */ + protected fun readStringSet(key: String, default: Set): Flow> = + readValue(stringSetPreferencesKey(key), default) + + /** + * Read a [Set] of [String] in a blocking way from the data store preferences + * + * @param key The key to read the value for + * @return The value or null if not present + */ + protected fun readStringSetValue(key: String): Set? = + readValueBlocking(stringSetPreferencesKey(key)) + + /** + * Read a [Set] of [String] in a blocking way from the data store preferences with a default + * + * @param key The key to read the value for + * @param default The default value to return if the key is not present + * @return The value + */ + protected fun readStringSetValue(key: String, default: Set): Set = + readValueBlocking(stringSetPreferencesKey(key), default) + + /** + * Add a [ByteArray] to the data store + * + * If value is null, the key will be removed. + * + * @param key The key to store the value under + * @param value The value to store + */ + protected suspend fun writeByteArray(key: String, value: ByteArray?) = + writeValue(byteArrayPreferencesKey(key), value) + + /** + * Add a [ByteArray] to the data store asynchronously + * + * @param key The key to store the value under + * @param value The value to store + * @return The [Job] for the launched write — call [Job.join] to await completion. + */ + protected fun writeByteArrayAsync(key: String, value: ByteArray?): Job = + writeValueAsync(byteArrayPreferencesKey(key), value) + + /** + * Reading the [ByteArray] value from the data store + * + * @param key The key to read the value for + * @return Flow of the value or null if not present + */ + protected fun readByteArray(key: String): Flow = + readValue(byteArrayPreferencesKey(key)) + + /** + * Reading the [ByteArray] value from the data store with a default + * + * @param key The key to read the value for + * @param default The default value to return if the key is not present + * @return Flow of the value + */ + protected fun readByteArray(key: String, default: ByteArray): Flow = + readValue(byteArrayPreferencesKey(key), default) + + /** + * Read a [ByteArray] in a blocking way from the data store preferences + * + * @param key The key to read the value for + * @return The value or null if not present + */ + protected fun readByteArrayValue(key: String): ByteArray? = + readValueBlocking(byteArrayPreferencesKey(key)) + + /** + * Read a [ByteArray] in a blocking way from the data store preferences with a default + * + * @param key The key to read the value for + * @param default The default value to return if the key is not present + * @return The value + */ + protected fun readByteArrayValue(key: String, default: ByteArray): ByteArray = + readValueBlocking(byteArrayPreferencesKey(key), default) + + /** + * Add an [Instant] to the data store + * + * Stored as epoch milliseconds in a [Long]. If value is null, the key will be removed. + * + * @param key The key to store the value under + * @param value The value to store + */ + @RequiresApi(Build.VERSION_CODES.O) + protected suspend fun writeInstant(key: String, value: Instant?) = + writeValue(longPreferencesKey(key), value?.toEpochMilli()) + + /** + * Add an [Instant] to the data store asynchronously + * + * @param key The key to store the value under + * @param value The value to store + * @return The [Job] for the launched write — call [Job.join] to await completion. + */ + @RequiresApi(Build.VERSION_CODES.O) + protected fun writeInstantAsync(key: String, value: Instant?): Job = + writeValueAsync(longPreferencesKey(key), value?.toEpochMilli()) + + /** + * Read an [Instant] preference as a [Flow]. + * + * @param key The key to read the value for + * @return A [Flow] emitting the [Instant] stored under the key, or null if the key does not exist + */ + @RequiresApi(Build.VERSION_CODES.O) + protected fun readInstant(key: String): Flow = + readValue(longPreferencesKey(key)).map { it?.let { millis -> Instant.ofEpochMilli(millis) } } + + /** + * Read an [Instant] preference as a [Flow] with a non-null default. + * + * @param key The key to read the value for + * @param default The default value to return if the key does not exist + * @return A [Flow] emitting the [Instant] stored under the key, or the default value + */ + @RequiresApi(Build.VERSION_CODES.O) + protected fun readInstant(key: String, default: Instant): Flow = + readValue(longPreferencesKey(key)).map { it?.let { millis -> Instant.ofEpochMilli(millis) } ?: default } + + /** + * Read an [Instant] preference in a blocking way. + * + * @param key The key to read the value for + * @return The [Instant] or null if the key does not exist + */ + @RequiresApi(Build.VERSION_CODES.O) + protected fun readInstantValue(key: String): Instant? = + readValueBlocking(longPreferencesKey(key))?.let { Instant.ofEpochMilli(it) } + + /** + * Read an [Instant] preference in a blocking way with a default value. + * + * @param key The key to read the value for + * @param default The default value to return if the key does not exist + * @return The [Instant] stored under the key, or the default value + */ + @RequiresApi(Build.VERSION_CODES.O) + protected fun readInstantValue(key: String, default: Instant): Instant = + readValueBlocking(longPreferencesKey(key))?.let { Instant.ofEpochMilli(it) } ?: default + /** * Add [Date] to the data store * @@ -1037,6 +1283,125 @@ abstract class BaseDataStoreHelper( */ protected fun doublePrefFlow(key: String): Flow = readDouble(key) + /** + * Create a property delegate for a [Float] preference. + */ + protected fun floatPref(key: String, defaultValue: Float): ReadWriteProperty = + object : ReadWriteProperty { + override fun getValue(thisRef: Any?, property: KProperty<*>): Float = readFloatValue(key, defaultValue) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Float) { + writeFloatAsync(key, value) + } + } + + /** + * Create a property delegate for a nullable [Float] preference. + */ + protected fun floatPref(key: String): ReadWriteProperty = + object : ReadWriteProperty { + override fun getValue(thisRef: Any?, property: KProperty<*>): Float? = readFloatValue(key) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Float?) { + writeFloatAsync(key, value) + } + } + + /** + * [Flow] accessor for a [Float] preference — alias for [readFloat]. + */ + protected fun floatPrefFlow(key: String, defaultValue: Float): Flow = readFloat(key, defaultValue) + + /** + * [Flow] accessor for a nullable [Float] preference — alias for [readFloat]. + */ + protected fun floatPrefFlow(key: String): Flow = readFloat(key) + + /** + * Create a property delegate for a [Set] of [String] preference. + */ + protected fun stringSetPref(key: String, defaultValue: Set): ReadWriteProperty> = + object : ReadWriteProperty> { + override fun getValue(thisRef: Any?, property: KProperty<*>): Set = + readStringSetValue(key, defaultValue) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Set) { + writeStringSetAsync(key, value) + } + } + + /** + * Create a property delegate for a nullable [Set] of [String] preference. + */ + protected fun stringSetPref(key: String): ReadWriteProperty?> = + object : ReadWriteProperty?> { + override fun getValue(thisRef: Any?, property: KProperty<*>): Set? = readStringSetValue(key) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Set?) { + writeStringSetAsync(key, value) + } + } + + /** + * [Flow] accessor for a [Set] of [String] preference — alias for [readStringSet]. + */ + protected fun stringSetPrefFlow(key: String, defaultValue: Set): Flow> = + readStringSet(key, defaultValue) + + /** + * [Flow] accessor for a nullable [Set] of [String] preference — alias for [readStringSet]. + */ + protected fun stringSetPrefFlow(key: String): Flow?> = readStringSet(key) + + /** + * Create a property delegate for a nullable [ByteArray] preference. + */ + protected fun byteArrayPref(key: String): ReadWriteProperty = + object : ReadWriteProperty { + override fun getValue(thisRef: Any?, property: KProperty<*>): ByteArray? = readByteArrayValue(key) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: ByteArray?) { + writeByteArrayAsync(key, value) + } + } + + /** + * [Flow] accessor for a nullable [ByteArray] preference — alias for [readByteArray]. + */ + protected fun byteArrayPrefFlow(key: String): Flow = readByteArray(key) + + /** + * Create a property delegate for an [Instant] preference. + */ + @RequiresApi(Build.VERSION_CODES.O) + protected fun instantPref(key: String, defaultValue: Instant): ReadWriteProperty = + object : ReadWriteProperty { + override fun getValue(thisRef: Any?, property: KProperty<*>): Instant = + readInstantValue(key, defaultValue) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Instant) { + writeInstantAsync(key, value) + } + } + + /** + * Create a property delegate for a nullable [Instant] preference. + */ + @RequiresApi(Build.VERSION_CODES.O) + protected fun instantPref(key: String): ReadWriteProperty = + object : ReadWriteProperty { + override fun getValue(thisRef: Any?, property: KProperty<*>): Instant? = readInstantValue(key) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Instant?) { + writeInstantAsync(key, value) + } + } + + /** + * [Flow] accessor for an [Instant] preference — alias for [readInstant]. + */ + @RequiresApi(Build.VERSION_CODES.O) + protected fun instantPrefFlow(key: String, defaultValue: Instant): Flow = readInstant(key, defaultValue) + + /** + * [Flow] accessor for a nullable [Instant] preference — alias for [readInstant]. + */ + @RequiresApi(Build.VERSION_CODES.O) + protected fun instantPrefFlow(key: String): Flow = readInstant(key) + /** * Create a property delegate for a [Date] preference. */ @@ -1272,6 +1637,103 @@ abstract class BaseDataStoreHelper( } } + /** + * Add a [Set] of [Enum] to the data store + * + * Stored as a set of [Enum.name] values. If value is null, the key will be removed. + * + * @param key The key to store the value under + * @param value The value to store + */ + protected suspend fun writeEnumSet(key: String, value: Set>?) = + writeStringSet(key, value?.mapTo(LinkedHashSet()) { it.name }) + + /** + * Add a [Set] of [Enum] to the data store asynchronously + * + * @param key The key to store the value under + * @param value The value to store + * @return The [Job] for the launched write — call [Job.join] to await completion. + */ + protected fun writeEnumSetAsync(key: String, value: Set>?): Job = + writeStringSetAsync(key, value?.mapTo(LinkedHashSet()) { it.name }) + + /** + * Read a [Set] of [Enum] preference as a [Flow]. + * + * Stored names that no longer match a constant of [T] are dropped, so removing an enum constant + * does not break reads of previously stored data. + * + * @param key The key to read the value for + * @param default The value emitted if the key does not exist, defaults to an empty set + */ + protected inline fun > readEnumSet( + key: String, + default: Set = emptySet(), + ): Flow> { + val constants = T::class.java.enumConstants + return readStringSet(key).map { names -> + if (names == null) default + else names.mapNotNullTo(LinkedHashSet()) { name -> constants?.firstOrNull { it.name == name } } + } + } + + /** + * Read a [Set] of [Enum] preference in a blocking way. + * + * @param key The key to read the value for + * @param default The value returned if the key does not exist, defaults to an empty set + */ + protected inline fun > readEnumSetValue( + key: String, + default: Set = emptySet(), + ): Set { + val names = readStringSetValue(key) ?: return default + val constants = T::class.java.enumConstants + return names.mapNotNullTo(LinkedHashSet()) { name -> constants?.firstOrNull { it.name == name } } + } + + /** + * Create a property delegate for a [Set] of [Enum] preference. + * + * @param key The key to read/write the value for + * @param defaultValue Value returned if the key is absent + */ + protected inline fun > enumSetPref( + key: String, + defaultValue: Set = emptySet(), + ): ReadWriteProperty> = enumSetPrefInternal(key, defaultValue, T::class.java) + + /** + * Non-inline implementation, for the same reason as [enumPrefInternal] — an anonymous class + * emitted inside the subclass would lose JVM-level access to the protected helpers. + */ + @PublishedApi + internal fun > enumSetPrefInternal( + key: String, + defaultValue: Set, + enumClass: Class, + ): ReadWriteProperty> { + val constants = enumClass.enumConstants + return object : ReadWriteProperty> { + override fun getValue(thisRef: Any?, property: KProperty<*>): Set { + val names = readStringSetValue(key) ?: return defaultValue + return names.mapNotNullTo(LinkedHashSet()) { name -> constants?.firstOrNull { it.name == name } } + } + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Set) { + writeEnumSetAsync(key, value) + } + } + } + + /** + * [Flow] accessor for a [Set] of [Enum] preference — alias for [readEnumSet]. + */ + protected inline fun > enumSetPrefFlow( + key: String, + defaultValue: Set = emptySet(), + ): Flow> = readEnumSet(key, defaultValue) + /** * [Flow] accessor for an [Enum] preference — alias for [readEnum]. */ diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt index 6a29700..e1491fd 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt @@ -2,12 +2,14 @@ package com.duck.prefshelper import android.content.SharedPreferences import android.os.Build +import android.util.Base64 import android.util.Log import androidx.annotation.RequiresApi import androidx.core.content.edit import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext +import java.time.Instant import java.time.LocalDate import java.time.LocalDateTime import java.time.LocalTime @@ -145,6 +147,57 @@ abstract class BasePrefsHelper( return Double.fromBits(sharedPreferences.getLong(key, 0L)) } + /** + * Set a [Float] preference + * + * @param key The key to store the value under + * @param value The value to store + */ + fun setFloat(key: String, value: Float) { + sharedPreferences.edit { + putFloat(key, value) + } + } + + /** + * Get a [Float] preference + * + * @param key The key to get the value for + * @param defValue The default value to return if the key does not exist, defaults to 0f + */ + fun getFloat(key: String, defValue: Float = 0f): Float { + return sharedPreferences.getFloat(key, defValue) + } + + /** + * Set a [Set] of [String] preference + * + * A copy is stored rather than the caller's instance: [SharedPreferences] keeps a reference to + * the set it is handed, and mutating it afterwards corrupts the stored value. + * + * @param key The key to store the value under + * @param value The value to store + */ + fun setStringSet(key: String, value: Set) { + sharedPreferences.edit { + putStringSet(key, LinkedHashSet(value)) + } + } + + /** + * Get a [Set] of [String] preference + * + * Returns a defensive copy. [SharedPreferences.getStringSet] documents its result as one you + * must not modify, with undefined behaviour if you do — this returns a set you own instead. + * + * @param key The key to get the value for + * @param defValue The default value to return if the key does not exist, defaults to an empty set + */ + fun getStringSet(key: String, defValue: Set = emptySet()): Set { + val stored = sharedPreferences.getStringSet(key, null) ?: return defValue + return LinkedHashSet(stored) + } + /** * Set a [Date] preference * @@ -168,6 +221,65 @@ abstract class BasePrefsHelper( return if(time == -1L) null else Date(time) } + /** + * Set an [Instant] preference + * + * Stored as epoch milliseconds, using the same -1L sentinel as the other temporal types here. + * + * @param key The key to store the value under + * @param value The value to store + */ + @RequiresApi(Build.VERSION_CODES.O) + fun setInstant(key: String, value: Instant?) { + sharedPreferences.edit { + putLong(key, value?.toEpochMilli() ?: -1L) + } + } + + /** + * Get an [Instant] preference + * + * @param key The key to get the value for + * @return The Instant stored under the key, or null if the key does not exist or the value is null + */ + @RequiresApi(Build.VERSION_CODES.O) + fun getInstant(key: String): Instant? { + val millis = sharedPreferences.getLong(key, -1L) + return if (millis == -1L) null else Instant.ofEpochMilli(millis) + } + + /** + * Set a [ByteArray] preference + * + * [SharedPreferences] has no binary type, so the value is stored Base64-encoded + * ([Base64.NO_WRAP]) in a [String]. Assigning null removes the key. + * + * @param key The key to store the value under + * @param value The value to store, or null to remove the key + */ + fun setByteArray(key: String, value: ByteArray?) { + sharedPreferences.edit { + if (value == null) remove(key) else putString(key, Base64.encodeToString(value, Base64.NO_WRAP)) + } + } + + /** + * Get a [ByteArray] preference + * + * @param key The key to get the value for + * @return The bytes stored under the key, or null if the key does not exist or the stored value + * is not valid Base64 + */ + fun getByteArray(key: String): ByteArray? { + val encoded = sharedPreferences.getString(key, null) ?: return null + return try { + Base64.decode(encoded, Base64.NO_WRAP) + } catch (e: IllegalArgumentException) { + Log.w("BasePrefsHelper", "Could not decode Base64 ByteArray for \"$key\"", e) + null + } + } + /** * Set a [Boolean] preference * @@ -275,6 +387,39 @@ abstract class BasePrefsHelper( setString(key, value?.name ?: "") } + /** + * Set a [Set] of [Enum] preference + * + * Stored as a set of [Enum.name] values. + * + * @param key The key to store the value under + * @param value The value to store + */ + fun setEnumSet(key: String, value: Set>) { + setStringSet(key, value.mapTo(LinkedHashSet()) { it.name }) + } + + /** + * Get a [Set] of [Enum] preference + * + * Stored names that no longer match a constant of [T] are dropped rather than throwing, so + * removing an enum constant does not break reads of previously stored data. + * + * @param key The key to get the value for + * @param default The value to return if the key does not exist, defaults to an empty set + */ + inline fun > getEnumSet(key: String, default: Set = emptySet()): Set { + val names = getStringSet(key, emptySet()) + if (names.isEmpty()) return default + return try { + val constants = T::class.java.enumConstants ?: return default + names.mapNotNullTo(LinkedHashSet()) { name -> constants.firstOrNull { it.name == name } } + } catch (e: Exception) { + Log.w("BasePrefsHelper", "Could not get Enum set of ${T::class.simpleName} for \"$key\"", e) + default + } + } + /** * Get an [Enum] preference * @@ -399,6 +544,69 @@ abstract class BasePrefsHelper( } } + /** + * Create a property delegate for a [Float] preference. + * + * @param key The key to read/write the value for + * @param defaultValue Value returned if the key is absent + */ + protected fun floatPref(key: String, defaultValue: Float): ReadWriteProperty = + object : ReadWriteProperty { + override fun getValue(thisRef: Any?, property: KProperty<*>): Float = getFloat(key, defaultValue) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Float) = setFloat(key, value) + } + + /** + * Create a property delegate for a nullable [Float] preference. + * + * Returns null when the key is absent. Assigning null removes the key. + */ + protected fun floatPref(key: String): ReadWriteProperty = + object : ReadWriteProperty { + override fun getValue(thisRef: Any?, property: KProperty<*>): Float? = + if (contains(key)) getFloat(key) else null + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Float?) { + if (value == null) sharedPreferences.edit { remove(key) } else setFloat(key, value) + } + } + + /** + * Create a property delegate for a [Set] of [String] preference. + * + * @param key The key to read/write the value for + * @param defaultValue Value returned if the key is absent + */ + protected fun stringSetPref(key: String, defaultValue: Set): ReadWriteProperty> = + object : ReadWriteProperty> { + override fun getValue(thisRef: Any?, property: KProperty<*>): Set = getStringSet(key, defaultValue) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Set) = setStringSet(key, value) + } + + /** + * Create a property delegate for a nullable [Set] of [String] preference. + * + * Returns null when the key is absent. Assigning null removes the key. + */ + protected fun stringSetPref(key: String): ReadWriteProperty?> = + object : ReadWriteProperty?> { + override fun getValue(thisRef: Any?, property: KProperty<*>): Set? = + if (contains(key)) getStringSet(key) else null + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Set?) { + if (value == null) sharedPreferences.edit { remove(key) } else setStringSet(key, value) + } + } + + /** + * Create a property delegate for a [ByteArray] preference. + * + * Returns null when the key is absent. Assigning null removes the key. + */ + protected fun byteArrayPref(key: String): ReadWriteProperty = + object : ReadWriteProperty { + override fun getValue(thisRef: Any?, property: KProperty<*>): ByteArray? = getByteArray(key) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: ByteArray?) = setByteArray(key, value) + } + /** * Create a property delegate for a [Boolean] preference. * @@ -425,6 +633,18 @@ abstract class BasePrefsHelper( } } + /** + * Create a property delegate for a nullable [Instant] preference. + * + * Assigning null stores the -1L sentinel (matching [setInstant]/[getInstant]). + */ + @RequiresApi(Build.VERSION_CODES.O) + protected fun instantPref(key: String): ReadWriteProperty = + object : ReadWriteProperty { + override fun getValue(thisRef: Any?, property: KProperty<*>): Instant? = getInstant(key) + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Instant?) = setInstant(key, value) + } + /** * Create a property delegate for a nullable [Date] preference. * @@ -506,4 +726,27 @@ abstract class BasePrefsHelper( override fun setValue(thisRef: Any?, property: KProperty<*>, value: T?) = setEnum(key, value) } } + + /** + * Create a property delegate for a [Set] of [Enum] preference. + * + * Stored names that no longer match a constant of [T] are dropped on read. + * + * @param key The key to read/write the value for + * @param defaultValue Value returned if the key is absent + */ + protected inline fun > enumSetPref( + key: String, + defaultValue: Set = emptySet(), + ): ReadWriteProperty> { + val constants = T::class.java.enumConstants + return object : ReadWriteProperty> { + override fun getValue(thisRef: Any?, property: KProperty<*>): Set { + val names = getStringSet(key, emptySet()) + if (names.isEmpty()) return defaultValue + return names.mapNotNullTo(LinkedHashSet()) { name -> constants?.firstOrNull { it.name == name } } + } + override fun setValue(thisRef: Any?, property: KProperty<*>, value: Set) = setEnumSet(key, value) + } + } } \ No newline at end of file diff --git a/README.md b/README.md index 1bec306..fec56af 100644 --- a/README.md +++ b/README.md @@ -168,19 +168,24 @@ class PrefsHelper(context: Context) { `BasePrefsHelper` and `BaseDataStoreHelper` support the same set of types as of 2.0: -`String`, `Int`, `Long`, `Double`, `Boolean`, `Date`, `LocalDateTime`, `LocalDate`, `LocalTime`, and `Enum<*>`. +`String`, `Int`, `Long`, `Float`, `Double`, `Boolean`, `ByteArray`, `Set`, `Date`, `Instant`, `LocalDateTime`, `LocalDate`, `LocalTime`, `Enum<*>`, and `Set`. -Each type exposes a non-nullable delegate `*Pref(key, defaultValue)` and a nullable delegate `*Pref(key)`. `BaseDataStoreHelper` additionally exposes matching `*PrefFlow` accessors for reactive reads. +Each type exposes a non-nullable delegate `*Pref(key, defaultValue)` and a nullable delegate `*Pref(key)`. `BaseDataStoreHelper` additionally exposes matching `*PrefFlow` accessors for reactive reads. The `java.time` types require API 26 (`@RequiresApi(O)`), as they did before. -The types match, but the two backends still store and clear them differently — that's inherent to `SharedPreferences` versus `DataStore`, not an oversight: +The types match, but the two backends store and clear some of them differently — inherent to `SharedPreferences` versus `DataStore`, not an oversight: | | `BasePrefsHelper` | `BaseDataStoreHelper` | |---|---|---| -| Assigning `null` | Removes the key for `String`/`Int`/`Long`/`Double`/`Boolean`; writes a `-1L` sentinel for the temporal types | Always removes the key | -| `Double` storage | Raw IEEE-754 bits in a `Long` — `SharedPreferences` has no double primitive, so never read the key back with `getLong` | Native `doublePreferencesKey` | -| `Date` storage | Epoch millis, with `-1L` doubling as "absent" | Epoch millis in a `Long`, absent means absent | +| Assigning `null` | Removes the key for the non-temporal types; writes a `-1L` sentinel for `Date`/`Instant`/`LocalDateTime`/`LocalDate`/`LocalTime` | Always removes the key | +| `Double` | Raw IEEE-754 bits in a `Long` — `SharedPreferences` has no double primitive, so never read that key back with `getLong` | Native `doublePreferencesKey` | +| `ByteArray` | Base64 (`NO_WRAP`) in a `String`; undecodable data reads back as null | Native `byteArrayPreferencesKey` | +| `Date` / `Instant` | Epoch millis, with `-1L` doubling as "absent" | Epoch millis in a `Long`; absent means absent | +| `Set` | Defensive copy on both read and write, so neither side can be corrupted by later mutation | Immutable set from DataStore | -That last row matters if you store pre-epoch dates: `BasePrefsHelper` cannot distinguish `Date(-1L)` from null, `BaseDataStoreHelper` can. +Two consequences worth knowing: + +- **Pre-epoch timestamps.** `BasePrefsHelper` cannot distinguish `Date(-1L)` / `Instant.ofEpochMilli(-1)` from null; `BaseDataStoreHelper` can. If you store timestamps that could legitimately be 1ms before the epoch, use the DataStore helper. +- **`Set` tolerates deleted constants.** Stored names that no longer match a constant are dropped on read rather than throwing, on both helpers — so removing an enum value doesn't break existing installs. ## Migrating to 2.0 @@ -259,7 +264,9 @@ The convenience constructor deliberately keeps the store's own internal actor on ### 7. New, not breaking: the two helpers now support the same types -`BasePrefsHelper` gains `Double`, `BaseDataStoreHelper` gains `Date`. Nothing existing changes — these are additions, and both follow their helper's established conventions. See [Supported types](#supported-types) for the storage differences that remain between the two backends. +2.0 closes the gap where each helper supported types the other didn't, and adds several new ones. `BasePrefsHelper` gains `Double`, `Float`, `ByteArray`, `Set`, `Instant` and `Set`; `BaseDataStoreHelper` gains `Date`, `Float`, `ByteArray`, `Set`, `Instant` and `Set`. + +Nothing existing changes — these are additions, and each follows its helper's established conventions. See [Supported types](#supported-types) for the storage differences that remain between the two backends. ## R8 / ProGuard / Minification diff --git a/app/src/test/java/com/duck/app/BaseDataStoreHelperTest.kt b/app/src/test/java/com/duck/app/BaseDataStoreHelperTest.kt index 22cfa1d..69df9a0 100644 --- a/app/src/test/java/com/duck/app/BaseDataStoreHelperTest.kt +++ b/app/src/test/java/com/duck/app/BaseDataStoreHelperTest.kt @@ -13,6 +13,7 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import kotlin.time.Duration.Companion.milliseconds import org.junit.After +import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull @@ -891,6 +892,464 @@ class BaseDataStoreHelperTest { assertNull(awaitValue(predicate = { it == null }) { dataStoreHelper.delegateLocalTime }) } + // region Float + + @Test + fun testWriteAndReadFloat() = runBlocking { + dataStoreHelper.testWriteFloat("float_key", 3.14f) + assertEquals(3.14f, dataStoreHelper.testReadFloatValue("float_key") ?: 0f, 0.0001f) + } + + @Test + fun testFloatBoundaryValues() = runBlocking { + dataStoreHelper.testWriteFloat("max_float", Float.MAX_VALUE) + assertEquals(Float.MAX_VALUE, dataStoreHelper.testReadFloatValue("max_float")) + + dataStoreHelper.testWriteFloat("min_float", Float.MIN_VALUE) + assertEquals(Float.MIN_VALUE, dataStoreHelper.testReadFloatValue("min_float")) + + dataStoreHelper.testWriteFloat("negative_float", -42.5f) + assertEquals(-42.5f, dataStoreHelper.testReadFloatValue("negative_float") ?: 0f, 0.0001f) + } + + @Test + fun testFloatNaN() = runBlocking { + dataStoreHelper.testWriteFloat("nan_float", Float.NaN) + val value = dataStoreHelper.testReadFloatValue("nan_float") + assertNotNull(value) + assertTrue("Value should be NaN", value!!.isNaN()) + } + + @Test + fun testWriteNullFloatRemovesKey() = runBlocking { + dataStoreHelper.testWriteFloat("null_float_key", 1.5f) + assertNotNull(dataStoreHelper.testReadFloatValue("null_float_key")) + + dataStoreHelper.testWriteFloat("null_float_key", null) + assertNull(dataStoreHelper.testReadFloatValue("null_float_key")) + } + + @Test + fun testReadFloatFlow() = runBlocking { + dataStoreHelper.testWriteFloat("float_flow_key", 2.5f) + assertEquals(2.5f, dataStoreHelper.testReadFloatFlow("float_flow_key").first() ?: 0f, 0.0001f) + } + + @Test + fun testReadFloatValueWithDefault() = runBlocking { + assertEquals(9.9f, dataStoreHelper.testReadFloatValueWithDefault("no_such_float", 9.9f), 0.0001f) + assertEquals(9.9f, dataStoreHelper.testReadFloatFlowWithDefault("no_such_float", 9.9f).first(), 0.0001f) + } + + @Test + fun testWriteFloatAsync() = runBlocking { + dataStoreHelper.testWriteFloatAsync("float_async_key", 7.7f).join() + assertEquals(7.7f, dataStoreHelper.testReadFloatValue("float_async_key") ?: 0f, 0.0001f) + } + + @Test + fun testFloatPrefDelegateRoundTripsValue() = runBlocking { + dataStoreHelper.delegateFloat = 1.25f + val value = awaitValue(predicate = { it == 1.25f }) { dataStoreHelper.delegateFloat } + assertEquals(1.25f, value, 0.0001f) + } + + @Test + fun testFloatPrefDelegateReturnsDefaultWhenAbsent() = runBlocking { + dataStoreHelper.testClearPrefs() + assertEquals(-1f, dataStoreHelper.delegateFloat, 0.0001f) + } + + @Test + fun testNullableFloatPrefDelegateRemovesKeyOnNull() = runBlocking { + dataStoreHelper.delegateNullableFloat = 0.5f + assertEquals(0.5f, awaitValue(predicate = { it == 0.5f }) { dataStoreHelper.delegateNullableFloat } ?: 0f, 0.0001f) + + dataStoreHelper.delegateNullableFloat = null + assertNull(awaitValue(predicate = { it == null }) { dataStoreHelper.delegateNullableFloat }) + } + + @Test + fun testFloatPrefFlowEmitsDelegateWrites() = runBlocking { + dataStoreHelper.delegateFloat = 3.5f + awaitValue(predicate = { it == 3.5f }) { dataStoreHelper.delegateFloat } + assertEquals(3.5f, dataStoreHelper.delegateFloatFlow.first(), 0.0001f) + } + + // endregion + + // region StringSet + + @Test + fun testWriteAndReadStringSet() = runBlocking { + val values = setOf("alpha", "beta", "gamma") + dataStoreHelper.testWriteStringSet("string_set_key", values) + assertEquals(values, dataStoreHelper.testReadStringSetValue("string_set_key")) + } + + @Test + fun testStringSetEmptyRoundTripsAsEmptyNotNull() = runBlocking { + dataStoreHelper.testWriteStringSet("empty_set_key", emptySet()) + val value = dataStoreHelper.testReadStringSetValue("empty_set_key") + assertNotNull("Empty set must be stored distinctly from an absent key", value) + assertEquals(emptySet(), value) + assertNull(dataStoreHelper.testReadStringSetValue("absent_set_key")) + } + + @Test + fun testStringSetContainingEmptyString() = runBlocking { + val values = setOf("", "present") + dataStoreHelper.testWriteStringSet("set_with_empty_string", values) + assertEquals(values, dataStoreHelper.testReadStringSetValue("set_with_empty_string")) + } + + @Test + fun testWriteNullStringSetRemovesKey() = runBlocking { + dataStoreHelper.testWriteStringSet("null_set_key", setOf("a")) + assertNotNull(dataStoreHelper.testReadStringSetValue("null_set_key")) + + dataStoreHelper.testWriteStringSet("null_set_key", null) + assertNull(dataStoreHelper.testReadStringSetValue("null_set_key")) + } + + @Test + fun testReadStringSetFlow() = runBlocking { + val values = setOf("one", "two") + dataStoreHelper.testWriteStringSet("string_set_flow_key", values) + assertEquals(values, dataStoreHelper.testReadStringSetFlow("string_set_flow_key").first()) + } + + @Test + fun testReadStringSetValueWithDefault() = runBlocking { + val fallback = setOf("default") + assertEquals(fallback, dataStoreHelper.testReadStringSetValueWithDefault("no_such_set", fallback)) + assertEquals(fallback, dataStoreHelper.testReadStringSetFlowWithDefault("no_such_set", fallback).first()) + } + + @Test + fun testWriteStringSetAsync() = runBlocking { + val values = setOf("async_a", "async_b") + dataStoreHelper.testWriteStringSetAsync("string_set_async_key", values).join() + assertEquals(values, dataStoreHelper.testReadStringSetValue("string_set_async_key")) + } + + @Test + fun testStringSetPrefDelegateRoundTripsValue() = runBlocking { + val values = setOf("x", "y") + dataStoreHelper.delegateStringSet = values + val value = awaitValue(predicate = { it == values }) { dataStoreHelper.delegateStringSet } + assertEquals(values, value) + } + + @Test + fun testStringSetPrefDelegateReturnsDefaultWhenAbsent() = runBlocking { + dataStoreHelper.testClearPrefs() + assertEquals(emptySet(), dataStoreHelper.delegateStringSet) + } + + @Test + fun testNullableStringSetPrefDelegateRemovesKeyOnNull() = runBlocking { + val values = setOf("temp") + dataStoreHelper.delegateNullableStringSet = values + assertEquals(values, awaitValue(predicate = { it == values }) { dataStoreHelper.delegateNullableStringSet }) + + dataStoreHelper.delegateNullableStringSet = null + assertNull(awaitValue(predicate = { it == null }) { dataStoreHelper.delegateNullableStringSet }) + } + + @Test + fun testStringSetPrefFlowEmitsDelegateWrites() = runBlocking { + val values = setOf("flowed") + dataStoreHelper.delegateStringSet = values + awaitValue(predicate = { it == values }) { dataStoreHelper.delegateStringSet } + assertEquals(values, dataStoreHelper.delegateStringSetFlow.first()) + } + + // endregion + + // region ByteArray + + @Test + fun testWriteAndReadByteArray() = runBlocking { + val bytes = byteArrayOf(0, -1, 127, -128) + dataStoreHelper.testWriteByteArray("byte_array_key", bytes) + assertArrayEquals(bytes, dataStoreHelper.testReadByteArrayValue("byte_array_key")) + } + + @Test + fun testByteArrayEmptyRoundTrips() = runBlocking { + dataStoreHelper.testWriteByteArray("empty_bytes_key", byteArrayOf()) + assertArrayEquals(byteArrayOf(), dataStoreHelper.testReadByteArrayValue("empty_bytes_key")) + } + + @Test + fun testWriteNullByteArrayRemovesKey() = runBlocking { + dataStoreHelper.testWriteByteArray("null_bytes_key", byteArrayOf(1, 2, 3)) + assertNotNull(dataStoreHelper.testReadByteArrayValue("null_bytes_key")) + + dataStoreHelper.testWriteByteArray("null_bytes_key", null) + assertNull(dataStoreHelper.testReadByteArrayValue("null_bytes_key")) + } + + @Test + fun testReadByteArrayFlow() = runBlocking { + val bytes = byteArrayOf(10, 20, 30) + dataStoreHelper.testWriteByteArray("byte_array_flow_key", bytes) + assertArrayEquals(bytes, dataStoreHelper.testReadByteArrayFlow("byte_array_flow_key").first()) + } + + @Test + fun testReadByteArrayValueWithDefault() = runBlocking { + val fallback = byteArrayOf(9) + assertArrayEquals(fallback, dataStoreHelper.testReadByteArrayValueWithDefault("no_such_bytes", fallback)) + assertArrayEquals(fallback, dataStoreHelper.testReadByteArrayFlowWithDefault("no_such_bytes", fallback).first()) + } + + @Test + fun testWriteByteArrayAsync() = runBlocking { + val bytes = byteArrayOf(0, -1, 127, -128) + dataStoreHelper.testWriteByteArrayAsync("byte_array_async_key", bytes).join() + assertArrayEquals(bytes, dataStoreHelper.testReadByteArrayValue("byte_array_async_key")) + } + + @Test + fun testByteArrayPrefDelegateRoundTripsValue() = runBlocking { + // byteArrayPref is nullable-only (no defaulted overload). + val bytes = byteArrayOf(0, -1, 127, -128) + dataStoreHelper.delegateByteArray = bytes + val value = awaitValue(predicate = { it != null && it.contentEquals(bytes) }) { + dataStoreHelper.delegateByteArray + } + assertArrayEquals(bytes, value) + } + + @Test + fun testByteArrayPrefDelegateRemovesKeyOnNull() = runBlocking { + val bytes = byteArrayOf(1) + dataStoreHelper.delegateByteArray = bytes + assertNotNull(awaitValue(predicate = { it != null && it.contentEquals(bytes) }) { dataStoreHelper.delegateByteArray }) + + dataStoreHelper.delegateByteArray = null + assertNull(awaitValue(predicate = { it == null }) { dataStoreHelper.delegateByteArray }) + } + + @Test + fun testByteArrayPrefFlowEmitsDelegateWrites() = runBlocking { + val bytes = byteArrayOf(5, 6) + dataStoreHelper.delegateByteArray = bytes + awaitValue(predicate = { it != null && it.contentEquals(bytes) }) { dataStoreHelper.delegateByteArray } + assertArrayEquals(bytes, dataStoreHelper.delegateByteArrayFlow.first()) + } + + // endregion + + // region Instant + + @Test + fun testWriteAndReadInstant() = runBlocking { + val instant = java.time.Instant.ofEpochMilli(1_700_000_000_123L) + dataStoreHelper.testWriteInstant("instant_key", instant) + assertEquals(instant, dataStoreHelper.testReadInstantValue("instant_key")) + } + + /** + * Instant is stored as epoch millis, so it must survive the epoch itself and pre-epoch + * (negative millis) values — the case a naive -1L sentinel would corrupt. + */ + @Test + fun testInstantBoundaryValues() = runBlocking { + dataStoreHelper.testWriteInstant("epoch", java.time.Instant.EPOCH) + assertEquals(java.time.Instant.EPOCH, dataStoreHelper.testReadInstantValue("epoch")) + + val preEpoch = java.time.Instant.ofEpochMilli(-1L) + dataStoreHelper.testWriteInstant("pre_epoch", preEpoch) + assertEquals(preEpoch, dataStoreHelper.testReadInstantValue("pre_epoch")) + } + + @Test + fun testInstantMillisecondPrecisionRoundTrip() = runBlocking { + // Storage is epoch millis — sub-millisecond nanos are truncated by design. + val millisPrecise = java.time.Instant.ofEpochMilli(1_600_000_000_999L) + dataStoreHelper.testWriteInstant("ms_precision", millisPrecise) + assertEquals(millisPrecise, dataStoreHelper.testReadInstantValue("ms_precision")) + assertEquals(1_600_000_000_999L, dataStoreHelper.testReadInstantValue("ms_precision")!!.toEpochMilli()) + } + + @Test + fun testWriteNullInstantRemovesKey() = runBlocking { + dataStoreHelper.testWriteInstant("null_instant_key", java.time.Instant.EPOCH) + assertNotNull(dataStoreHelper.testReadInstantValue("null_instant_key")) + + dataStoreHelper.testWriteInstant("null_instant_key", null) + assertNull(dataStoreHelper.testReadInstantValue("null_instant_key")) + } + + @Test + fun testReadInstantFlow() = runBlocking { + val instant = java.time.Instant.ofEpochMilli(1_500_000_000_000L) + dataStoreHelper.testWriteInstant("instant_flow_key", instant) + assertEquals(instant, dataStoreHelper.testReadInstantFlow("instant_flow_key").first()) + } + + @Test + fun testReadInstantValueWithDefault() = runBlocking { + val fallback = java.time.Instant.ofEpochMilli(42L) + assertEquals(fallback, dataStoreHelper.testReadInstantValueWithDefault("no_such_instant", fallback)) + assertEquals(fallback, dataStoreHelper.testReadInstantFlowWithDefault("no_such_instant", fallback).first()) + } + + @Test + fun testWriteInstantAsync() = runBlocking { + val instant = java.time.Instant.ofEpochMilli(1_400_000_000_000L) + dataStoreHelper.testWriteInstantAsync("instant_async_key", instant).join() + assertEquals(instant, dataStoreHelper.testReadInstantValue("instant_async_key")) + } + + @Test + fun testInstantPrefDelegateRoundTripsValue() = runBlocking { + val instant = java.time.Instant.ofEpochMilli(99L) + dataStoreHelper.delegateInstant = instant + val value = awaitValue(predicate = { it == instant }) { dataStoreHelper.delegateInstant } + assertEquals(instant, value) + } + + @Test + fun testInstantPrefDelegateReturnsDefaultWhenAbsent() = runBlocking { + dataStoreHelper.testClearPrefs() + assertEquals(java.time.Instant.EPOCH, dataStoreHelper.delegateInstant) + } + + @Test + fun testNullableInstantPrefDelegateRemovesKeyOnNull() = runBlocking { + val instant = java.time.Instant.ofEpochMilli(1L) + dataStoreHelper.delegateNullableInstant = instant + assertEquals(instant, awaitValue(predicate = { it == instant }) { dataStoreHelper.delegateNullableInstant }) + + dataStoreHelper.delegateNullableInstant = null + assertNull(awaitValue(predicate = { it == null }) { dataStoreHelper.delegateNullableInstant }) + } + + @Test + fun testInstantPrefFlowEmitsDelegateWrites() = runBlocking { + val instant = java.time.Instant.ofEpochMilli(123L) + dataStoreHelper.delegateInstant = instant + awaitValue(predicate = { it == instant }) { dataStoreHelper.delegateInstant } + assertEquals(instant, dataStoreHelper.delegateInstantFlow.first()) + } + + // endregion + + // region EnumSet + + @Test + fun testWriteAndReadEnumSet() = runBlocking { + val values: Set> = setOf(TestEnum.VALUE_A, TestEnum.VALUE_C) + dataStoreHelper.testWriteEnumSet("enum_set_key", values) + assertEquals( + setOf(TestEnum.VALUE_A, TestEnum.VALUE_C), + dataStoreHelper.testReadEnumSetValue("enum_set_key"), + ) + } + + @Test + fun testEnumSetEmptyRoundTrips() = runBlocking { + dataStoreHelper.testWriteEnumSet("empty_enum_set", emptySet()) + assertEquals(emptySet(), dataStoreHelper.testReadEnumSetValue("empty_enum_set")) + } + + @Test + fun testWriteNullEnumSetRemovesKey() = runBlocking { + dataStoreHelper.testWriteEnumSet("null_enum_set", setOf(TestEnum.VALUE_A)) + assertEquals(setOf(TestEnum.VALUE_A), dataStoreHelper.testReadEnumSetValue("null_enum_set")) + + dataStoreHelper.testWriteEnumSet("null_enum_set", null) + // Key gone → default empty set (readEnumSetValue is non-nullable with default emptySet). + assertEquals(emptySet(), dataStoreHelper.testReadEnumSetValue("null_enum_set")) + // Confirm the underlying string-set key is actually gone, not just filtered. + assertNull(dataStoreHelper.testReadStringSetValue("null_enum_set")) + } + + @Test + fun testReadEnumSetFlow() = runBlocking { + dataStoreHelper.testWriteEnumSet("enum_set_flow_key", setOf(TestEnum.VALUE_B)) + assertEquals( + setOf(TestEnum.VALUE_B), + dataStoreHelper.testReadEnumSetFlow("enum_set_flow_key").first(), + ) + } + + @Test + fun testReadEnumSetValueWithDefault() = runBlocking { + val fallback = setOf(TestEnum.VALUE_A) + assertEquals(fallback, dataStoreHelper.testReadEnumSetValue("no_such_enum_set", fallback)) + assertEquals(fallback, dataStoreHelper.testReadEnumSetFlow("no_such_enum_set", fallback).first()) + } + + @Test + fun testWriteEnumSetAsync() = runBlocking { + dataStoreHelper.testWriteEnumSetAsync("enum_set_async_key", setOf(TestEnum.VALUE_C)).join() + assertEquals( + setOf(TestEnum.VALUE_C), + dataStoreHelper.testReadEnumSetValue("enum_set_async_key"), + ) + } + + /** + * Stored enum-set names that no longer match a constant must be dropped silently — removing an + * enum constant from the app must not throw when reading previously stored preference data. + */ + @Test + fun testEnumSetDropsUnknownNames() = runBlocking { + // Write a raw string set on the same key so we can inject a name that is not a TestEnum constant. + dataStoreHelper.testWriteStringSet( + "enum_set_unknown_names", + setOf(TestEnum.VALUE_A.name, "BOGUS_REMOVED_CONSTANT", TestEnum.VALUE_B.name), + ) + assertEquals( + setOf(TestEnum.VALUE_A, TestEnum.VALUE_B), + dataStoreHelper.testReadEnumSetValue("enum_set_unknown_names"), + ) + assertEquals( + setOf(TestEnum.VALUE_A, TestEnum.VALUE_B), + dataStoreHelper.testReadEnumSetFlow("enum_set_unknown_names").first(), + ) + } + + /** + * Exercises [enumSetPref] through a property delegate on the [TestDataStoreHelper] subclass. + * + * Why this exists: an `inline fun ` that returns an anonymous object calling + * `protected` members of [BaseDataStoreHelper] throws [IllegalAccessError] at runtime when the + * anonymous class is emitted inside a subclass in another module (losing JVM-level protected + * access). [enumSetPref] is implemented as a thin inline wrapper over a non-inline + * `@PublishedApi internal enumSetPrefInternal` specifically to avoid that. Calling only + * [readEnumSetValue] would not catch a regression that re-inlines the anonymous object into the + * subclass — this test must go through the delegate property. + */ + @Test + fun testEnumSetPrefDelegateRoundTripsValue() = runBlocking { + val values = setOf(TestEnum.VALUE_A, TestEnum.VALUE_C) + dataStoreHelper.delegateEnumSet = values + val value = awaitValue(predicate = { it == values }) { dataStoreHelper.delegateEnumSet } + assertEquals(values, value) + } + + @Test + fun testEnumSetPrefDelegateReturnsDefaultWhenAbsent() = runBlocking { + dataStoreHelper.testClearPrefs() + assertEquals(emptySet(), dataStoreHelper.delegateEnumSet) + } + + @Test + fun testEnumSetPrefFlowEmitsDelegateWrites() = runBlocking { + val values = setOf(TestEnum.VALUE_B) + dataStoreHelper.delegateEnumSet = values + awaitValue(predicate = { it == values }) { dataStoreHelper.delegateEnumSet } + assertEquals(values, dataStoreHelper.delegateEnumSetFlow.first()) + } + + // endregion + enum class TestEnum { VALUE_A, VALUE_B, VALUE_C } @@ -995,6 +1454,50 @@ class BaseDataStoreHelperTest { fun testReadLocalTimeFlowWithDefault(key: String, default: java.time.LocalTime) = readLocalTime(key, default) fun testWriteLocalTimeAsync(key: String, value: java.time.LocalTime?) = writeLocalTimeAsync(key, value) + // Float methods + suspend fun testWriteFloat(key: String, value: Float?) = writeFloat(key, value) + fun testReadFloatValue(key: String) = readFloatValue(key) + fun testReadFloatValueWithDefault(key: String, default: Float) = readFloatValue(key, default) + fun testReadFloatFlow(key: String) = readFloat(key) + fun testReadFloatFlowWithDefault(key: String, default: Float) = readFloat(key, default) + fun testWriteFloatAsync(key: String, value: Float?) = writeFloatAsync(key, value) + + // StringSet methods + suspend fun testWriteStringSet(key: String, value: Set?) = writeStringSet(key, value) + fun testReadStringSetValue(key: String) = readStringSetValue(key) + fun testReadStringSetValueWithDefault(key: String, default: Set) = readStringSetValue(key, default) + fun testReadStringSetFlow(key: String) = readStringSet(key) + fun testReadStringSetFlowWithDefault(key: String, default: Set) = readStringSet(key, default) + fun testWriteStringSetAsync(key: String, value: Set?) = writeStringSetAsync(key, value) + + // ByteArray methods + suspend fun testWriteByteArray(key: String, value: ByteArray?) = writeByteArray(key, value) + fun testReadByteArrayValue(key: String) = readByteArrayValue(key) + fun testReadByteArrayValueWithDefault(key: String, default: ByteArray) = readByteArrayValue(key, default) + fun testReadByteArrayFlow(key: String) = readByteArray(key) + fun testReadByteArrayFlowWithDefault(key: String, default: ByteArray) = readByteArray(key, default) + fun testWriteByteArrayAsync(key: String, value: ByteArray?) = writeByteArrayAsync(key, value) + + // Instant methods + suspend fun testWriteInstant(key: String, value: java.time.Instant?) = writeInstant(key, value) + fun testReadInstantValue(key: String) = readInstantValue(key) + fun testReadInstantValueWithDefault(key: String, default: java.time.Instant) = readInstantValue(key, default) + fun testReadInstantFlow(key: String) = readInstant(key) + fun testReadInstantFlowWithDefault(key: String, default: java.time.Instant) = readInstant(key, default) + fun testWriteInstantAsync(key: String, value: java.time.Instant?) = writeInstantAsync(key, value) + + // EnumSet methods + suspend fun testWriteEnumSet(key: String, value: Set>?) = writeEnumSet(key, value) + fun testWriteEnumSetAsync(key: String, value: Set>?) = writeEnumSetAsync(key, value) + inline fun > testReadEnumSetValue( + key: String, + default: Set = emptySet(), + ) = readEnumSetValue(key, default) + inline fun > testReadEnumSetFlow( + key: String, + default: Set = emptySet(), + ) = readEnumSet(key, default) + // Enum property-based access - Uses actual BaseDataStoreHelper enum methods // This pattern matches NormalDataStore and actually tests the base class methods! var testEnumProperty: TestEnum? @@ -1041,6 +1544,26 @@ class BaseDataStoreHelperTest { var delegateLocalDate by localDatePref(KEY_DELEGATE_LD) var delegateLocalTime by localTimePref(KEY_DELEGATE_LT) + var delegateFloat by floatPref(KEY_DELEGATE_FLOAT, defaultValue = -1f) + var delegateNullableFloat by floatPref(KEY_DELEGATE_NULLABLE_FLOAT) + val delegateFloatFlow = floatPrefFlow(KEY_DELEGATE_FLOAT, defaultValue = -1f) + + var delegateStringSet by stringSetPref(KEY_DELEGATE_STRING_SET, defaultValue = emptySet()) + var delegateNullableStringSet by stringSetPref(KEY_DELEGATE_NULLABLE_STRING_SET) + val delegateStringSetFlow = stringSetPrefFlow(KEY_DELEGATE_STRING_SET, defaultValue = emptySet()) + + // byteArrayPref is nullable-only (no defaulted overload). + var delegateByteArray by byteArrayPref(KEY_DELEGATE_BYTE_ARRAY) + val delegateByteArrayFlow = byteArrayPrefFlow(KEY_DELEGATE_BYTE_ARRAY) + + var delegateInstant by instantPref(KEY_DELEGATE_INSTANT, defaultValue = java.time.Instant.EPOCH) + var delegateNullableInstant by instantPref(KEY_DELEGATE_NULLABLE_INSTANT) + val delegateInstantFlow = instantPrefFlow(KEY_DELEGATE_INSTANT, defaultValue = java.time.Instant.EPOCH) + + // Critical IllegalAccessError regression surface — see testEnumSetPrefDelegateRoundTripsValue. + var delegateEnumSet by enumSetPref(KEY_DELEGATE_ENUM_SET, defaultValue = emptySet()) + val delegateEnumSetFlow = enumSetPrefFlow(KEY_DELEGATE_ENUM_SET, defaultValue = emptySet()) + companion object { private const val KEY_TEST_ENUM = "test_enum_key" private const val KEY_DELEGATE_INT = "delegate_int_key" @@ -1058,6 +1581,14 @@ class BaseDataStoreHelperTest { private const val KEY_DELEGATE_LDT = "delegate_ldt_key" private const val KEY_DELEGATE_LD = "delegate_ld_key" private const val KEY_DELEGATE_LT = "delegate_lt_key" + private const val KEY_DELEGATE_FLOAT = "delegate_float_key" + private const val KEY_DELEGATE_NULLABLE_FLOAT = "delegate_nullable_float_key" + private const val KEY_DELEGATE_STRING_SET = "delegate_string_set_key" + private const val KEY_DELEGATE_NULLABLE_STRING_SET = "delegate_nullable_string_set_key" + private const val KEY_DELEGATE_BYTE_ARRAY = "delegate_byte_array_key" + private const val KEY_DELEGATE_INSTANT = "delegate_instant_key" + private const val KEY_DELEGATE_NULLABLE_INSTANT = "delegate_nullable_instant_key" + private const val KEY_DELEGATE_ENUM_SET = "delegate_enum_set_key" } suspend fun testClearPrefs() = clearPrefs() diff --git a/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt b/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt new file mode 100644 index 0000000..93894c5 --- /dev/null +++ b/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt @@ -0,0 +1,228 @@ +package com.duck.app + +import android.content.Context +import android.content.SharedPreferences +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import com.duck.prefshelper.BasePrefsHelper +import org.junit.Assert.assertArrayEquals +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import java.time.Instant + +/** + * [BasePrefsHelperTest] covers the whole API against a Mockito-mocked [SharedPreferences], which is + * fast but proves only that the right platform calls are made. The types whose storage involves a + * real encoding — `Double` (raw IEEE-754 bits in a Long), `ByteArray` (Base64), `Set` + * (defensive copies) — need a genuine `SharedPreferences` to prove they actually round-trip. + * + * Robolectric, so it still runs on the JVM with no device. `android.util.Base64` in particular + * returns nothing useful without an Android runtime, so the `ByteArray` path cannot be tested at + * all in the mock-based suite. + */ +@RunWith(AndroidJUnit4::class) +class BasePrefsHelperRealPrefsTest { + + private lateinit var prefs: SharedPreferences + private lateinit var helper: RealPrefsHelper + + @Before + fun setUp() { + val context = InstrumentationRegistry.getInstrumentation().targetContext + prefs = context.getSharedPreferences("real_prefs_test", Context.MODE_PRIVATE) + prefs.edit().clear().commit() + helper = RealPrefsHelper(prefs) + } + + // region Double — raw IEEE-754 bit encoding + + /** + * The values a naive `putFloat`-based encoding would quietly mangle. Each must come back + * bit-identical through a real `SharedPreferences`. + */ + @Test + fun testDoubleRoundTripsThroughRealPreferences() { + val values = listOf( + 0.0, + -0.0, + 1.0, + -1.0, + 3.14159265358979, + 1.0 / 3.0, + Double.MAX_VALUE, + Double.MIN_VALUE, + Double.POSITIVE_INFINITY, + Double.NEGATIVE_INFINITY, + ) + for (value in values) { + helper.setDouble("d", value) + assertEquals("round-trip failed for $value", value, helper.getDouble("d"), 0.0) + } + + helper.setDouble("d", Double.NaN) + assertTrue(helper.getDouble("d").isNaN()) + } + + /** -0.0 and 0.0 are equal under `==` but have different bit patterns; the encoding must keep them apart. */ + @Test + fun testNegativeZeroIsDistinctFromPositiveZero() { + helper.setDouble("d", -0.0) + assertEquals(java.lang.Double.doubleToRawLongBits(-0.0), java.lang.Double.doubleToRawLongBits(helper.getDouble("d"))) + assertNotEquals(java.lang.Double.doubleToRawLongBits(0.0), java.lang.Double.doubleToRawLongBits(helper.getDouble("d"))) + } + + @Test + fun testDoubleReturnsDefaultWhenAbsent() { + assertEquals(9.5, helper.getDouble("never_written", 9.5), 0.0) + } + + // endregion + + // region ByteArray — Base64 encoding + + @Test + fun testByteArrayRoundTripsThroughRealPreferences() { + val bytes = byteArrayOf(0, -1, 127, -128, 42) + helper.setByteArray("b", bytes) + assertArrayEquals(bytes, helper.getByteArray("b")) + } + + @Test + fun testEmptyByteArrayRoundTrips() { + helper.setByteArray("b", ByteArray(0)) + assertArrayEquals(ByteArray(0), helper.getByteArray("b")) + } + + @Test + fun testByteArrayNullRemovesKey() { + helper.setByteArray("b", byteArrayOf(1, 2, 3)) + assertNotNull(helper.getByteArray("b")) + + helper.setByteArray("b", null) + assertNull(helper.getByteArray("b")) + assertTrue(!helper.contains("b")) + } + + @Test + fun testByteArrayAbsentKeyReturnsNull() { + assertNull(helper.getByteArray("never_written")) + } + + /** A key holding a non-Base64 string must return null rather than propagating an exception. */ + @Test + fun testByteArrayReturnsNullForUndecodableValue() { + helper.setString("b", "!!! not base64 !!!") + assertNull(helper.getByteArray("b")) + } + + /** The delegate must go through the same encoding as the direct accessors. */ + @Test + fun testByteArrayDelegateRoundTrips() { + val bytes = byteArrayOf(9, 8, 7) + helper.byteArrayValue = bytes + assertArrayEquals(bytes, helper.byteArrayValue) + } + + // endregion + + // region Set — defensive copies against a real backing store + + @Test + fun testStringSetRoundTrips() { + helper.setStringSet("s", setOf("alpha", "beta", "gamma")) + assertEquals(setOf("alpha", "beta", "gamma"), helper.getStringSet("s")) + } + + @Test + fun testEmptyStringSetIsDistinctFromAbsent() { + helper.setStringSet("s", emptySet()) + assertEquals(emptySet(), helper.getStringSet("s")) + assertTrue(helper.contains("s")) + // An absent key falls back to the supplied default; an empty stored set does not. + assertEquals(setOf("fallback"), helper.getStringSet("never_written", setOf("fallback"))) + } + + /** + * Mutating the set we returned must not corrupt what is stored — the real trap + * `SharedPreferences.getStringSet` sets for callers. + */ + @Test + fun testGetStringSetCopyIsSafeToMutate() { + helper.setStringSet("s", setOf("a", "b")) + + @Suppress("UNCHECKED_CAST") + (helper.getStringSet("s") as MutableSet).add("c") + + assertEquals(setOf("a", "b"), helper.getStringSet("s")) + } + + /** And mutating the set we were given after writing must not change what was stored. */ + @Test + fun testSetStringSetCopiesCallerSet() { + val caller = mutableSetOf("a") + helper.setStringSet("s", caller) + caller.add("added-after-write") + + assertEquals(setOf("a"), helper.getStringSet("s")) + } + + // endregion + + // region Instant and Set + + @Test + fun testInstantRoundTrips() { + val instant = Instant.ofEpochMilli(1_700_000_000_123L) + helper.setInstant("i", instant) + assertEquals(instant, helper.getInstant("i")) + } + + @Test + fun testInstantEpochRoundTrips() { + helper.setInstant("i", Instant.EPOCH) + assertEquals(Instant.EPOCH, helper.getInstant("i")) + } + + /** + * `BasePrefsHelper` uses -1L as its "absent" sentinel for temporal types, so an Instant at + * exactly -1ms is indistinguishable from null. Documented in the README; pinned here so the + * limitation is a decision rather than a surprise. + */ + @Test + fun testInstantAtSentinelMillisReadsBackAsNull() { + helper.setInstant("i", Instant.ofEpochMilli(-1L)) + assertNull(helper.getInstant("i")) + } + + @Test + fun testEnumSetRoundTrips() { + helper.enumSetValue = setOf(BasePrefsHelperTest.TestEnum.VALUE_A, BasePrefsHelperTest.TestEnum.VALUE_C) + assertEquals( + setOf(BasePrefsHelperTest.TestEnum.VALUE_A, BasePrefsHelperTest.TestEnum.VALUE_C), + helper.enumSetValue, + ) + } + + /** Names that no longer map to a constant are dropped, not thrown on. */ + @Test + fun testEnumSetDropsUnknownStoredNames() { + helper.setStringSet("enum_set_key", setOf("VALUE_B", "DELETED_CONSTANT")) + assertEquals(setOf(BasePrefsHelperTest.TestEnum.VALUE_B), helper.enumSetValue) + } + + // endregion + + private fun assertNotNull(value: Any?) = assertTrue(value != null) + + private class RealPrefsHelper( + override val sharedPreferences: SharedPreferences, + ) : BasePrefsHelper() { + var byteArrayValue by byteArrayPref("byte_array_key") + var enumSetValue by enumSetPref("enum_set_key") + } +} diff --git a/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt b/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt index 827a0aa..cf69070 100644 --- a/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt +++ b/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt @@ -130,6 +130,121 @@ class BasePrefsHelperTest { assertTrue(Double.fromBits(Double.NaN.toRawBits()).isNaN()) } + @Test + fun testSetFloat() { + prefsHelper.setFloat("key", 1.5f) + verify(mockEditor).putFloat("key", 1.5f) + verify(mockEditor).apply() + } + + @Test + fun testGetFloat() { + `when`(mockSharedPreferences.getFloat("key", 0f)).thenReturn(2.5f) + assertEquals(2.5f, prefsHelper.getFloat("key"), 0f) + } + + @Test + fun testSetStringSet() { + prefsHelper.setStringSet("key", setOf("a", "b")) + verify(mockEditor).putStringSet("key", linkedSetOf("a", "b")) + verify(mockEditor).apply() + } + + @Test + fun testGetStringSet() { + `when`(mockSharedPreferences.getStringSet("key", null)).thenReturn(linkedSetOf("a", "b")) + assertEquals(setOf("a", "b"), prefsHelper.getStringSet("key")) + } + + @Test + fun testGetStringSetReturnsDefaultWhenAbsent() { + `when`(mockSharedPreferences.getStringSet("key", null)).thenReturn(null) + assertEquals(setOf("fallback"), prefsHelper.getStringSet("key", setOf("fallback"))) + } + + /** + * [android.content.SharedPreferences.getStringSet] documents its result as one callers must not + * modify, with undefined behaviour if they do. The helper hands back a copy so callers can't + * fall into that; mutating what we return must not touch what the platform gave us. + */ + @Test + fun testGetStringSetReturnsDefensiveCopy() { + val backing = linkedSetOf("a", "b") + `when`(mockSharedPreferences.getStringSet("key", null)).thenReturn(backing) + + val returned = prefsHelper.getStringSet("key") as MutableSet + returned.add("c") + + assertEquals(setOf("a", "b"), backing) + } + + /** + * Counterpart to the read side: the platform keeps a reference to the set it is handed, so the + * helper must store a copy rather than the caller's instance. + */ + @Test + fun testSetStringSetStoresDefensiveCopy() { + val caller = mutableSetOf("a") + prefsHelper.setStringSet("key", caller) + caller.add("mutated-after-write") + + verify(mockEditor).putStringSet("key", linkedSetOf("a")) + } + + @Test + fun testSetInstant() { + prefsHelper.setInstant("key", java.time.Instant.ofEpochMilli(1_700_000_000_000L)) + verify(mockEditor).putLong("key", 1_700_000_000_000L) + verify(mockEditor).apply() + } + + @Test + fun testSetInstantNullStoresSentinel() { + prefsHelper.setInstant("key", null) + verify(mockEditor).putLong("key", -1L) + } + + @Test + fun testGetInstant() { + `when`(mockSharedPreferences.getLong("key", -1L)).thenReturn(1_700_000_000_000L) + assertEquals(java.time.Instant.ofEpochMilli(1_700_000_000_000L), prefsHelper.getInstant("key")) + } + + @Test + fun testGetInstantReturnsNullForSentinel() { + `when`(mockSharedPreferences.getLong("key", -1L)).thenReturn(-1L) + assertNull(prefsHelper.getInstant("key")) + } + + @Test + fun testSetEnumSet() { + prefsHelper.setEnumSet("key", setOf(TestEnum.VALUE_A, TestEnum.VALUE_B)) + verify(mockEditor).putStringSet("key", linkedSetOf("VALUE_A", "VALUE_B")) + } + + @Test + fun testGetEnumSet() { + `when`(mockSharedPreferences.getStringSet("key", null)).thenReturn(linkedSetOf("VALUE_A", "VALUE_C")) + assertEquals(setOf(TestEnum.VALUE_A, TestEnum.VALUE_C), prefsHelper.getEnumSet("key")) + } + + /** + * Removing an enum constant must not break reads of data stored before the removal — unknown + * names are dropped rather than throwing. + */ + @Test + fun testGetEnumSetDropsUnknownNames() { + `when`(mockSharedPreferences.getStringSet("key", null)) + .thenReturn(linkedSetOf("VALUE_A", "REMOVED_IN_A_LATER_VERSION")) + assertEquals(setOf(TestEnum.VALUE_A), prefsHelper.getEnumSet("key")) + } + + @Test + fun testGetEnumSetReturnsDefaultWhenAbsent() { + `when`(mockSharedPreferences.getStringSet("key", null)).thenReturn(null) + assertEquals(setOf(TestEnum.VALUE_B), prefsHelper.getEnumSet("key", setOf(TestEnum.VALUE_B))) + } + @Test fun testGetBoolean() { `when`(mockSharedPreferences.getBoolean("key", false)).thenReturn(true) @@ -544,6 +659,97 @@ class BasePrefsHelperTest { verify(mockEditor).apply() } + // Float delegate + @Test + fun testFloatPrefDelegateGet() { + `when`(mockSharedPreferences.getFloat("float_key", 0.25f)).thenReturn(7.5f) + assertEquals(7.5f, prefsHelper.floatValue, 0f) + } + + @Test + fun testFloatPrefDelegateSet() { + prefsHelper.floatValue = 3.5f + verify(mockEditor).putFloat("float_key", 3.5f) + } + + @Test + fun testNullableFloatPrefReturnsNullWhenAbsent() { + `when`(mockSharedPreferences.contains("nullable_float_key")).thenReturn(false) + assertNull(prefsHelper.nullableFloat) + } + + @Test + fun testNullableFloatPrefRemovesKeyOnNullAssignment() { + prefsHelper.nullableFloat = null + verify(mockEditor).remove("nullable_float_key") + } + + // String set delegate + @Test + fun testStringSetPrefDelegateGet() { + `when`(mockSharedPreferences.getStringSet("string_set_key", null)).thenReturn(linkedSetOf("x", "y")) + assertEquals(setOf("x", "y"), prefsHelper.stringSetValue) + } + + @Test + fun testStringSetPrefDelegateFallsBackToDefault() { + `when`(mockSharedPreferences.getStringSet("string_set_key", null)).thenReturn(null) + assertEquals(setOf("default"), prefsHelper.stringSetValue) + } + + @Test + fun testStringSetPrefDelegateSet() { + prefsHelper.stringSetValue = setOf("p", "q") + verify(mockEditor).putStringSet("string_set_key", linkedSetOf("p", "q")) + } + + @Test + fun testNullableStringSetPrefReturnsNullWhenAbsent() { + `when`(mockSharedPreferences.contains("nullable_string_set_key")).thenReturn(false) + assertNull(prefsHelper.nullableStringSet) + } + + @Test + fun testNullableStringSetPrefRemovesKeyOnNullAssignment() { + prefsHelper.nullableStringSet = null + verify(mockEditor).remove("nullable_string_set_key") + } + + // Instant delegate + @Test + fun testInstantPrefDelegateGet() { + `when`(mockSharedPreferences.getLong("instant_key", -1L)).thenReturn(1_234_567L) + assertEquals(java.time.Instant.ofEpochMilli(1_234_567L), prefsHelper.instantValue) + } + + @Test + fun testInstantPrefDelegateSet() { + prefsHelper.instantValue = java.time.Instant.ofEpochMilli(99L) + verify(mockEditor).putLong("instant_key", 99L) + } + + // Enum set delegate + @Test + fun testEnumSetPrefDelegateGet() { + `when`(mockSharedPreferences.getStringSet("enum_set_key", null)) + .thenReturn(linkedSetOf("VALUE_B", "VALUE_C")) + assertEquals(setOf(TestEnum.VALUE_B, TestEnum.VALUE_C), prefsHelper.enumSetValue) + } + + @Test + fun testEnumSetPrefDelegateSet() { + prefsHelper.enumSetValue = setOf(TestEnum.VALUE_A) + verify(mockEditor).putStringSet("enum_set_key", linkedSetOf("VALUE_A")) + } + + // ByteArray delegate (encoding itself is covered against a real SharedPreferences in + // BasePrefsHelperRealPrefsTest — android.util.Base64 is not available to these plain-JVM mocks) + @Test + fun testByteArrayPrefRemovesKeyOnNullAssignment() { + prefsHelper.byteArrayValue = null + verify(mockEditor).remove("byte_array_key") + } + // Boolean delegate @Test fun testBooleanPrefDelegateGet() { @@ -689,6 +895,13 @@ class BasePrefsHelperTest { var nullableLong by longPref("nullable_long_key") var doubleValue by doublePref("double_key", defaultValue = 1.5) var nullableDouble by doublePref("nullable_double_key") + var floatValue by floatPref("float_key", defaultValue = 0.25f) + var nullableFloat by floatPref("nullable_float_key") + var stringSetValue by stringSetPref("string_set_key", defaultValue = setOf("default")) + var nullableStringSet by stringSetPref("nullable_string_set_key") + var byteArrayValue by byteArrayPref("byte_array_key") + var instantValue by instantPref("instant_key") + var enumSetValue by enumSetPref("enum_set_key") var boolValue by booleanPref("bool_key", defaultValue = true) var nullableBool by booleanPref("nullable_bool_key") var dateValue by datePref("date_delegate_key") From 504b790da49553d8bab6ac6b77b0ceeefae1523e Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 12:59:19 +0100 Subject: [PATCH 12/17] Align null/absent semantics across both helpers, and add prefs versioning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Null now means the same thing everywhere: assigning null removes the key, an absent key reads back as null. BasePrefsHelper's -1L temporal sentinel is gone. The sentinel was not just an inconsistency, it was a live defect. Epoch day -1 is 1969-12-31, so a LocalDate of that day was silently unstorable and read back as null. Same for Date/Instant at 1ms before the epoch. LocalTime was unaffected in practice, since -1 was never in its valid 0..86399 range — but it would now throw rather than misread, so getLocalTime treats out-of-range values as null. This changes how existing data reads, which is the sharp edge. A key left at -1L by 1.x is no longer "no value", it is 1969-12-31. migrateLegacyTemporalSentinels(vararg keys) sweeps those keys once and is safe to re-run; documented as migration item 8, and the "silent changes" banner now lists it as the only one that touches stored data. Also adds migrateIfNeeded(currentVersion) { from -> }, a small schema-version stamp so migrations that are NOT safe to repeat ("stored seconds are millis now") can be written directly. The subtle part is bootstrapping: an unstamped file is a pre-versioning install if it holds anything, or a fresh install if empty. Doing this now matters — once 2.0 ships without a stamp, 1.x and 2.0 installs would both lack the key and need different treatment, so the ambiguity only gets worse with time. Downgrades never rewind the stamp. BaseDataStoreHelper deliberately gets no equivalent. DataStore's own DataMigration already tracks whether it has run and applies atomically before the first read, which beats stamping a version afterwards; the convenience constructor now takes a migrations list instead. Noted in CLAUDE.md so nobody adds a parallel scheme for symmetry's sake. Tests 298 -> 311. The existing temporal tests encoded the old semantics and were updated; two of them turned out to be asserting that -1L means "not set", so they were rewritten rather than mechanically converted — one is now a regression guard proving 1969-12-31 round-trips. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 8 +- .../duck/prefshelper/BaseDataStoreHelper.kt | 18 ++ .../com/duck/prefshelper/BasePrefsHelper.kt | 201 +++++++++++++++--- README.md | 70 +++++- .../duck/app/BasePrefsHelperRealPrefsTest.kt | 164 +++++++++++++- .../java/com/duck/app/BasePrefsHelperTest.kt | 67 ++++-- 6 files changed, 464 insertions(+), 64 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 515769d..067e81d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,9 +63,15 @@ Storage conventions still differ by backend, and deliberately so: - **`Double` on `BasePrefsHelper`** is stored as raw IEEE-754 bits via `putLong`/`Double.fromBits`, because `SharedPreferences` has no double primitive. Reading that key with `getLong` returns the bit pattern, not the number. - **`ByteArray` on `BasePrefsHelper`** is Base64 (`NO_WRAP`) in a String, since SharedPreferences has no binary type. Undecodable data logs and returns null rather than throwing. - **`Set` on `BasePrefsHelper`** copies on both read and write. `SharedPreferences.getStringSet` documents its result as one callers must not modify, and the platform keeps a reference to the set it is handed — both directions are trapped, so both are copied. -- **`Date`/`Instant` on `BaseDataStoreHelper`** are epoch millis in a `longPreferencesKey` and null removes the key — unlike `BasePrefsHelper`, whose temporal types use the `-1L` sentinel and therefore cannot represent `-1L` millis distinctly from null. +- **Null semantics are identical on both helpers** as of 2.0: assigning null removes the key, an absent key reads as null. The `-1L` temporal sentinel `BasePrefsHelper` used before 2.0 is gone — it made `LocalDate` 1969-12-31 (epoch day -1) unstorable. `migrateLegacyTemporalSentinels(vararg keys)` sweeps stale sentinels left by 1.x; `getLocalTime` also treats out-of-range values as null, since `LocalTime.ofSecondOfDay(-1)` throws. - **`Set`** is stored as a set of `Enum.name` on both. Unknown names are dropped on read so deleting an enum constant doesn't break existing installs. +## Migrations + +`BasePrefsHelper.migrateIfNeeded(currentVersion) { from -> }` runs a block at most once per version, stamped under `KEY_HELPER_VERSION` in the consumer's own prefs file. An unstamped file is treated as `VERSION_LEGACY` (1) if it holds anything, or as a fresh install if empty — that's how pre-versioning installs are told from new ones. Downgrades don't rewind the stamp. The stamp is written with `commit`, so the first call does a small synchronous write. + +`BaseDataStoreHelper` has **no** equivalent, deliberately: DataStore's own `DataMigration` already tracks whether it has run and is applied atomically before the first read, which is strictly better than stamping a version afterwards. The convenience constructor takes a `migrations` list that is passed straight to `PreferenceDataStoreFactory`. Don't add a parallel version-key scheme here. + ## Gotchas - **Inline reified factories that access `protected` members**: an `inline fun ` that returns an anonymous object (e.g. a `ReadWriteProperty`) calling `protected` methods on `BaseDataStoreHelper` throws `IllegalAccessError` at runtime — the anonymous class is emitted inside the *subclass* at inline time, losing JVM-level protected access. Fix: split into a thin `inline` + `reified` wrapper that forwards to a non-inline `@PublishedApi internal` helper which owns the anonymous object. See `enumPref` / `enumPrefInternal` and `enumSetPref` / `enumSetPrefInternal` in `BaseDataStoreHelper.kt`. `BasePrefsHelper` is unaffected because its get/set accessors are `public`. diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt index d6e96b9..cdf3666 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt @@ -4,6 +4,7 @@ import android.content.Context import android.os.Build import android.util.Log import androidx.annotation.RequiresApi +import androidx.datastore.core.DataMigration import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.Preferences import androidx.datastore.preferences.core.booleanPreferencesKey @@ -83,6 +84,20 @@ abstract class BaseDataStoreHelper( * [readValueBlocking] against its own store. Use the primary constructor if you genuinely want * to control where the store schedules its work. * + * One-off data migrations go through [migrations], DataStore's own mechanism. Unlike + * `BasePrefsHelper.migrateIfNeeded`, there is deliberately no version-key scheme here: a + * [DataMigration] already answers "have I run?" via `shouldMigrate`, and DataStore applies it + * atomically before the first read rather than racing a stamp afterwards. Adding a second + * mechanism on top would be strictly worse. + * + * ```kotlin + * class AppPrefs(context: Context) : BaseDataStoreHelper( + * context, + * "app_prefs", + * migrations = listOf(SecondsToMillisMigration()), + * ) + * ``` + * * @param context Any [Context]; the application context is used internally * @param preferenceName Name of the preferences data store file * @param dispatcher The [CoroutineDispatcher] `suspend` functions run on. Does **not** affect the @@ -90,14 +105,17 @@ abstract class BaseDataStoreHelper( * @param scope The [CoroutineScope] the `*Async` writes are launched in. Note this does not own * the store's internal actor either, so a `CoroutineExceptionHandler` here sees failures from the * `*Async` writes, not from DataStore itself. + * @param migrations Migrations run once, before the first read */ constructor( context: Context, preferenceName: String, dispatcher: CoroutineDispatcher = Dispatchers.IO, scope: CoroutineScope = CoroutineScope(dispatcher + SupervisorJob()), + migrations: List> = emptyList(), ) : this( dataStore = PreferenceDataStoreFactory.create( + migrations = migrations, scope = CoroutineScope(Dispatchers.IO + SupervisorJob()), produceFile = { context.applicationContext.preferencesDataStoreFile(preferenceName) }, ), diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt index e1491fd..b4c847b 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt @@ -23,8 +23,13 @@ import kotlin.reflect.KProperty /** * A base class for SharedPreferences helpers * - * Provides common functionality for storing and retrieving preferences of various types - * including String, Int, Long, Date, Boolean, LocalDateTime, LocalDate, LocalTime, and Enum. + * Provides common functionality for storing and retrieving preferences of various types: + * String, Int, Long, Float, Double, Boolean, ByteArray, Set, Date, Instant, LocalDateTime, + * LocalDate, LocalTime, Enum and Set — the same set as [BaseDataStoreHelper]. + * + * Assigning null to any nullable preference removes the key, and an absent key reads back as null. + * Pre-2.0 versions wrote a -1L sentinel for the temporal types instead; see + * [migrateLegacyTemporalSentinels] if you are upgrading an install that ran one. * * Reads and writes are synchronous; only [clearPrefs] does blocking work, and it hops to * [dispatcher] to do it. The dispatcher carries no [kotlinx.coroutines.Job], so cancelling the @@ -54,6 +59,125 @@ abstract class BasePrefsHelper( return sharedPreferences.contains(key) } + /** + * Run one-off migrations, at most once per version. + * + * Modelled on a database schema version, deliberately kept small. [migrate] is invoked only when + * the stored version is behind [currentVersion], and the new version is stamped immediately + * afterwards, so a non-idempotent migration ("stored seconds are now millis, multiply by 1000") + * is safe to write. + * + * Call it from your subclass's `init` block, before anything reads a preference: + * + * ```kotlin + * class UserPrefs(context: Context) : BasePrefsHelper() { + * override val sharedPreferences = + * context.getSharedPreferences("user", Context.MODE_PRIVATE) + * + * init { + * migrateIfNeeded(currentVersion = 2) { from -> + * if (from < 2) migrateLegacyTemporalSentinels(KEY_DOB, KEY_LAST_SEEN) + * } + * } + * } + * ``` + * + * **Bootstrapping.** Installs that predate versioning have no stored version, and so does a + * brand-new install — they are told apart by whether the file holds anything at all. An empty + * file is a fresh install: nothing runs and the current version is stamped. A non-empty file is + * treated as version [VERSION_LEGACY]. + * + * **Downgrades are left alone.** If the stored version is *ahead* of [currentVersion] — an app + * downgrade — nothing runs and the stored version is not rewound, so a later upgrade still sees + * the true high-water mark. + * + * Two caveats worth knowing. The version is stamped with `commit`, so this does a small + * synchronous write on the calling thread the first time it runs; that is the price of not + * re-running a migration after a crash. And the window between [migrate] returning and the stamp + * landing is not atomic — a crash in between re-runs the migration, so prefer migrations that + * tolerate that where you reasonably can. + * + * @param currentVersion The version this code expects; must be at least [VERSION_LEGACY] + * @param migrate Invoked with the stored version when it is behind [currentVersion] + * @return true if [migrate] was invoked + */ + fun migrateIfNeeded(currentVersion: Int, migrate: (fromVersion: Int) -> Unit): Boolean { + require(currentVersion >= VERSION_LEGACY) { + "currentVersion must be >= $VERSION_LEGACY, was $currentVersion" + } + val stored: Int? = + if (sharedPreferences.contains(KEY_HELPER_VERSION)) { + sharedPreferences.getInt(KEY_HELPER_VERSION, VERSION_LEGACY) + } else { + null + } + // No stamp: an empty file is a fresh install, anything else predates versioning. + val from = stored ?: if (sharedPreferences.all.isEmpty()) currentVersion else VERSION_LEGACY + + if (from < currentVersion) { + migrate(from) + sharedPreferences.edit(commit = true) { putInt(KEY_HELPER_VERSION, currentVersion) } + return true + } + if (stored == null) { + // Fresh install, or one already at currentVersion but never stamped — record the + // baseline so the next upgrade has something to compare against. + sharedPreferences.edit(commit = true) { putInt(KEY_HELPER_VERSION, from) } + } + return false + } + + /** + * Clear pre-2.0 `-1L` sentinels left behind by the temporal setters. + * + * Before 2.0, assigning null to a [Date], [Instant], [LocalDateTime], [LocalDate] or [LocalTime] + * preference wrote `-1L` rather than removing the key. 2.0 removes the key instead, which means + * a value written by an older version now reads back as a real timestamp — `-1` epoch days is + * `1969-12-31`, not null. + * + * Call this once, before the first read, with the temporal keys this helper owns. Any listed key + * currently holding `-1L` is removed, so it reads as null exactly as it did before the upgrade. + * Keys that are absent, hold a different value, or hold a non-`Long` are left untouched, so + * calling it repeatedly is harmless. + * + * ```kotlin + * class UserPrefs(context: Context) : BasePrefsHelper() { + * override val sharedPreferences = context.getSharedPreferences("user", Context.MODE_PRIVATE) + * + * init { + * migrateLegacyTemporalSentinels(KEY_DOB, KEY_LAST_SEEN) + * } + * } + * ``` + * + * Only needed for installs that ran a pre-2.0 version. New installs can skip it, and it is safe + * to leave in place indefinitely. + * + * Note the flip side: a genuine `-1L` written by 2.0 — `Date(-1)`, or `LocalDate` of + * `1969-12-31` — would also be removed. Don't pass keys whose real value could legitimately be + * `-1L`, and drop the call once your install base has turned over. + * + * @param keys The temporal preference keys to sweep + * @return The keys that were actually removed + */ + fun migrateLegacyTemporalSentinels(vararg keys: String): List { + val stale = keys.filter { key -> + try { + sharedPreferences.contains(key) && sharedPreferences.getLong(key, 0L) == -1L + } catch (e: ClassCastException) { + // Key holds something other than a Long — not ours to migrate. + Log.w("BasePrefsHelper", "Skipping non-Long key \"$key\" during sentinel migration", e) + false + } + } + if (stale.isNotEmpty()) { + sharedPreferences.edit { + stale.forEach { remove(it) } + } + } + return stale + } + /** * Set a [String] preference * @@ -206,7 +330,7 @@ abstract class BasePrefsHelper( */ fun setDate(key: String, value: Date?) { sharedPreferences.edit { - putLong(key, value?.time ?: -1L) + if (value == null) remove(key) else putLong(key, value.time) } } @@ -214,17 +338,17 @@ abstract class BasePrefsHelper( * Get a [Date] preference * * @param key The key to get the value for - * @return The date stored under the key, or null if the key does not exist or the value is null + * @return The date stored under the key, or null if the key does not exist */ fun getDate(key: String): Date? { - val time = sharedPreferences.getLong(key, -1L) - return if(time == -1L) null else Date(time) + if (!sharedPreferences.contains(key)) return null + return Date(sharedPreferences.getLong(key, 0L)) } /** * Set an [Instant] preference * - * Stored as epoch milliseconds, using the same -1L sentinel as the other temporal types here. + * Stored as epoch milliseconds. Assigning null removes the key. * * @param key The key to store the value under * @param value The value to store @@ -232,7 +356,7 @@ abstract class BasePrefsHelper( @RequiresApi(Build.VERSION_CODES.O) fun setInstant(key: String, value: Instant?) { sharedPreferences.edit { - putLong(key, value?.toEpochMilli() ?: -1L) + if (value == null) remove(key) else putLong(key, value.toEpochMilli()) } } @@ -240,12 +364,12 @@ abstract class BasePrefsHelper( * Get an [Instant] preference * * @param key The key to get the value for - * @return The Instant stored under the key, or null if the key does not exist or the value is null + * @return The Instant stored under the key, or null if the key does not exist */ @RequiresApi(Build.VERSION_CODES.O) fun getInstant(key: String): Instant? { - val millis = sharedPreferences.getLong(key, -1L) - return if (millis == -1L) null else Instant.ofEpochMilli(millis) + if (!sharedPreferences.contains(key)) return null + return Instant.ofEpochMilli(sharedPreferences.getLong(key, 0L)) } /** @@ -311,7 +435,7 @@ abstract class BasePrefsHelper( @RequiresApi(Build.VERSION_CODES.O) fun setLocalDateTime(key: String, value: LocalDateTime?) { sharedPreferences.edit { - putLong(key, value?.toEpochSecond(ZoneOffset.UTC) ?: -1L) + if (value == null) remove(key) else putLong(key, value.toEpochSecond(ZoneOffset.UTC)) } } @@ -319,12 +443,12 @@ abstract class BasePrefsHelper( * Get a [LocalDateTime] preference * * @param key The key to get the value for - * @return The LocalDateTime stored under the key, or null if the key does not exist or the value is null + * @return The LocalDateTime stored under the key, or null if the key does not exist */ @RequiresApi(Build.VERSION_CODES.O) fun getLocalDateTime(key: String): LocalDateTime? { - val time = sharedPreferences.getLong(key, -1L) - return if(time == -1L) null else LocalDateTime.ofEpochSecond(time, 0, ZoneOffset.UTC) + if (!sharedPreferences.contains(key)) return null + return LocalDateTime.ofEpochSecond(sharedPreferences.getLong(key, 0L), 0, ZoneOffset.UTC) } /** @@ -336,7 +460,7 @@ abstract class BasePrefsHelper( @RequiresApi(Build.VERSION_CODES.O) fun setLocalDate(key: String, value: LocalDate?) { sharedPreferences.edit { - putLong(key, value?.toEpochDay() ?: -1L) + if (value == null) remove(key) else putLong(key, value.toEpochDay()) } } @@ -344,12 +468,12 @@ abstract class BasePrefsHelper( * Get a [LocalDate] preference * * @param key The key to get the value for - * @return The LocalDate stored under the key, or null if the key does not exist or the value is null + * @return The LocalDate stored under the key, or null if the key does not exist */ @RequiresApi(Build.VERSION_CODES.O) fun getLocalDate(key: String): LocalDate? { - val time = sharedPreferences.getLong(key, -1L) - return if(time == -1L) null else LocalDate.ofEpochDay(time) + if (!sharedPreferences.contains(key)) return null + return LocalDate.ofEpochDay(sharedPreferences.getLong(key, 0L)) } /** @@ -361,7 +485,7 @@ abstract class BasePrefsHelper( @RequiresApi(Build.VERSION_CODES.O) fun setLocalTime(key: String, value: LocalTime?) { sharedPreferences.edit { - putLong(key, value?.toSecondOfDay()?.toLong() ?: -1L) + if (value == null) remove(key) else putLong(key, value.toSecondOfDay().toLong()) } } @@ -369,12 +493,16 @@ abstract class BasePrefsHelper( * Get a [LocalTime] preference * * @param key The key to get the value for - * @return The LocalTime stored under the key, or null if the key does not exist or the value is null + * @return The LocalTime stored under the key, or null if the key does not exist or the stored + * value is outside [LocalTime]'s valid 0..86399 second range (which includes the pre-2.0 -1L + * sentinel, so stale data reads as null rather than throwing) */ @RequiresApi(Build.VERSION_CODES.O) fun getLocalTime(key: String): LocalTime? { - val time = sharedPreferences.getLong(key, -1L) - return if(time == -1L) null else LocalTime.ofSecondOfDay(time) + if (!sharedPreferences.contains(key)) return null + val seconds = sharedPreferences.getLong(key, 0L) + if (seconds !in 0L..86_399L) return null + return LocalTime.ofSecondOfDay(seconds) } /** @@ -636,7 +764,7 @@ abstract class BasePrefsHelper( /** * Create a property delegate for a nullable [Instant] preference. * - * Assigning null stores the -1L sentinel (matching [setInstant]/[getInstant]). + * Returns null when the key is absent. Assigning null removes the key (matching [setInstant]/[getInstant]). */ @RequiresApi(Build.VERSION_CODES.O) protected fun instantPref(key: String): ReadWriteProperty = @@ -648,7 +776,7 @@ abstract class BasePrefsHelper( /** * Create a property delegate for a nullable [Date] preference. * - * Assigning null stores the -1L sentinel (matching [setDate]/[getDate]). + * Returns null when the key is absent. Assigning null removes the key (matching [setDate]/[getDate]). */ protected fun datePref(key: String): ReadWriteProperty = object : ReadWriteProperty { @@ -659,7 +787,7 @@ abstract class BasePrefsHelper( /** * Create a property delegate for a nullable [LocalDateTime] preference. * - * Assigning null stores the -1L sentinel (matching [setLocalDateTime]/[getLocalDateTime]). + * Returns null when the key is absent. Assigning null removes the key (matching [setLocalDateTime]/[getLocalDateTime]). */ @RequiresApi(Build.VERSION_CODES.O) protected fun localDateTimePref(key: String): ReadWriteProperty = @@ -671,7 +799,7 @@ abstract class BasePrefsHelper( /** * Create a property delegate for a nullable [LocalDate] preference. * - * Assigning null stores the -1L sentinel (matching [setLocalDate]/[getLocalDate]). + * Returns null when the key is absent. Assigning null removes the key (matching [setLocalDate]/[getLocalDate]). */ @RequiresApi(Build.VERSION_CODES.O) protected fun localDatePref(key: String): ReadWriteProperty = @@ -683,7 +811,7 @@ abstract class BasePrefsHelper( /** * Create a property delegate for a nullable [LocalTime] preference. * - * Assigning null stores the -1L sentinel (matching [setLocalTime]/[getLocalTime]). + * Returns null when the key is absent. Assigning null removes the key (matching [setLocalTime]/[getLocalTime]). */ @RequiresApi(Build.VERSION_CODES.O) protected fun localTimePref(key: String): ReadWriteProperty = @@ -749,4 +877,21 @@ abstract class BasePrefsHelper( override fun setValue(thisRef: Any?, property: KProperty<*>, value: Set) = setEnumSet(key, value) } } + + companion object { + /** + * Key [migrateIfNeeded] stamps the schema version under. + * + * It lives in the same file as your own preferences and will show up in + * [SharedPreferences.getAll], so skip it if you enumerate keys. Double-underscored to stay + * clear of ordinary key names. + */ + const val KEY_HELPER_VERSION: String = "__prefs_helper_version" + + /** + * The version assumed for a non-empty file that has never been stamped — i.e. any install + * created before [migrateIfNeeded] existed. + */ + const val VERSION_LEGACY: Int = 1 + } } \ No newline at end of file diff --git a/README.md b/README.md index fec56af..7751545 100644 --- a/README.md +++ b/README.md @@ -172,20 +172,48 @@ class PrefsHelper(context: Context) { Each type exposes a non-nullable delegate `*Pref(key, defaultValue)` and a nullable delegate `*Pref(key)`. `BaseDataStoreHelper` additionally exposes matching `*PrefFlow` accessors for reactive reads. The `java.time` types require API 26 (`@RequiresApi(O)`), as they did before. -The types match, but the two backends store and clear some of them differently — inherent to `SharedPreferences` versus `DataStore`, not an oversight: +**Null means the same thing on both helpers as of 2.0:** assigning `null` removes the key, and an absent key reads back as `null`. There is no sentinel value any more — see [migration item 8](#8-the--1l-temporal-sentinel-is-gone) if you are upgrading. + +What still differs is only the underlying encoding, which is inherent to `SharedPreferences` versus `DataStore`: | | `BasePrefsHelper` | `BaseDataStoreHelper` | |---|---|---| -| Assigning `null` | Removes the key for the non-temporal types; writes a `-1L` sentinel for `Date`/`Instant`/`LocalDateTime`/`LocalDate`/`LocalTime` | Always removes the key | | `Double` | Raw IEEE-754 bits in a `Long` — `SharedPreferences` has no double primitive, so never read that key back with `getLong` | Native `doublePreferencesKey` | | `ByteArray` | Base64 (`NO_WRAP`) in a `String`; undecodable data reads back as null | Native `byteArrayPreferencesKey` | -| `Date` / `Instant` | Epoch millis, with `-1L` doubling as "absent" | Epoch millis in a `Long`; absent means absent | +| `Date` / `Instant` | Epoch millis in a `Long` | Epoch millis in a `Long` | | `Set` | Defensive copy on both read and write, so neither side can be corrupted by later mutation | Immutable set from DataStore | +| Migrations | [`migrateIfNeeded`](#versioned-migrations) — a stored version stamp | DataStore's own `DataMigration` list, passed to the constructor | + +**`Set` tolerates deleted constants** on both helpers: stored names that no longer match a constant are dropped on read rather than throwing, so removing an enum value doesn't break existing installs. + +### Versioned migrations + +`BasePrefsHelper.migrateIfNeeded` runs a block at most once per version, so a migration that *isn't* safe to repeat — "the stored value was seconds, it's millis now" — can be written without hand-rolling a guard: -Two consequences worth knowing: +```kotlin +class UserPrefs(context: Context) : BasePrefsHelper() { + override val sharedPreferences = + context.getSharedPreferences("user", Context.MODE_PRIVATE) -- **Pre-epoch timestamps.** `BasePrefsHelper` cannot distinguish `Date(-1L)` / `Instant.ofEpochMilli(-1)` from null; `BaseDataStoreHelper` can. If you store timestamps that could legitimately be 1ms before the epoch, use the DataStore helper. -- **`Set` tolerates deleted constants.** Stored names that no longer match a constant are dropped on read rather than throwing, on both helpers — so removing an enum value doesn't break existing installs. + init { + migrateIfNeeded(currentVersion = 2) { from -> + if (from < 2) migrateLegacyTemporalSentinels(KEY_DOB, KEY_LAST_SEEN) + } + } +} +``` + +An install with no stamp is treated as version 1 if the file already holds preferences, or as a fresh install if it's empty — so upgrades and first runs are told apart without you doing anything. Downgrades never rewind the stamp. The version lives under `BasePrefsHelper.KEY_HELPER_VERSION` in your own prefs file, so skip that key if you enumerate `getAll()`. + +`BaseDataStoreHelper` deliberately has **no** equivalent. DataStore already ships `DataMigration`, which answers "have I run?" itself and is applied atomically before the first read rather than racing a stamp written afterwards. Pass migrations to the constructor and use the platform mechanism: + +```kotlin +class AppPrefs(context: Context) : BaseDataStoreHelper( + context, + "app_prefs", + migrations = listOf(SecondsToMillisMigration()), +) +``` ## Migrating to 2.0 @@ -193,7 +221,11 @@ Two consequences worth knowing: **Most subclasses need no change at all.** If you extend `BasePrefsHelper()` or `BaseDataStoreHelper(context, "name")` without passing a coroutine context of your own, you are already on the new defaults. -**Two items on this list change behaviour without the compiler telling you: items 2 and 5.** The rest are removed or retyped symbols, so they fail the build and you'll find them immediately. Item 2 is the one that can corrupt state — read it properly. Item 5 only bites a specific threading arrangement. +**Three items change behaviour without the compiler telling you: items 2, 5 and 8.** The rest are removed or retyped symbols, so they fail the build and you'll find them immediately. + +- **Item 8** is the only one that changes how *existing stored data* is read. If you use temporal preferences on `BasePrefsHelper`, start there. +- **Item 2** can leave state half-written when a caller is cancelled. +- **Item 5** only bites one specific threading arrangement. ### 1. `coroutineContext` → `dispatcher` (+ optional `scope`) @@ -268,6 +300,30 @@ The convenience constructor deliberately keeps the store's own internal actor on Nothing existing changes — these are additions, and each follows its helper's established conventions. See [Supported types](#supported-types) for the storage differences that remain between the two backends. +### 8. The `-1L` temporal sentinel is gone + +**This one touches stored data. Read it if you use `Date`, `Instant`, `LocalDateTime`, `LocalDate` or `LocalTime` on `BasePrefsHelper`.** + +Before 2.0, assigning `null` to one of those wrote `-1L` rather than removing the key, and any stored `-1L` read back as `null`. 2.0 removes the key instead, matching `BaseDataStoreHelper`. + +That fixes a real defect: epoch day `-1` is **1969-12-31**, so a `LocalDate` of that day was silently unstorable and read back as `null`. Same for `Date`/`Instant` at 1ms before the epoch. + +But it also means **data written by 1.x now reads differently**. A key left at `-1L` by an older version is no longer "no value" — it is 1969-12-31. Sweep it once, at startup, before anything reads: + +```kotlin +init { + migrateIfNeeded(currentVersion = 2) { from -> + if (from < 2) migrateLegacyTemporalSentinels(KEY_DOB, KEY_LAST_SEEN) + } +} +``` + +`migrateLegacyTemporalSentinels` removes each listed key that currently holds `-1L`, so it reads as `null` exactly as before. It ignores absent keys, other values and non-`Long` keys, so it's safe to run repeatedly — the `migrateIfNeeded` wrapper is belt-and-braces, and worth adopting for the migrations that follow. Drop the call once your install base has turned over. + +Don't pass keys whose genuine value could be `-1L`, or you'll delete real data. + +`LocalTime` needs no migration: its valid range is 0..86399, so `-1` was never a legal value. Out-of-range stored values now read as `null` instead of throwing. + ## R8 / ProGuard / Minification PrefsHelper is fully compatible with R8 (including full mode) and requires **no consumer ProGuard rules** (the shipped `consumer-rules.pro` is intentionally empty). Preference keys are always explicit string arguments you pass to the `*Pref(...)` delegates — they are **never** derived from Kotlin property names via reflection. R8 is free to rename, merge, and repackage your `BasePrefsHelper`/`BaseDataStoreHelper` subclasses and their properties without changing any persisted key. Enum values are stored by `Enum.name` (preserved by R8's default Android rules), so enum prefs survive obfuscation. diff --git a/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt b/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt index 93894c5..90fa9ac 100644 --- a/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt +++ b/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt @@ -189,14 +189,168 @@ class BasePrefsHelperRealPrefsTest { } /** - * `BasePrefsHelper` uses -1L as its "absent" sentinel for temporal types, so an Instant at - * exactly -1ms is indistinguishable from null. Documented in the README; pinned here so the - * limitation is a decision rather than a surprise. + * Before 2.0 the temporal setters wrote `-1L` to mean null, which made these values unstorable. + * They round-trip now; this is the regression guard for that fix. + * + * `LocalDate` is the one that mattered in practice: epoch day -1 is 1969-12-31, an ordinary + * date that silently read back as null. */ @Test - fun testInstantAtSentinelMillisReadsBackAsNull() { + fun testFormerSentinelValuesNowRoundTrip() { helper.setInstant("i", Instant.ofEpochMilli(-1L)) - assertNull(helper.getInstant("i")) + assertEquals(Instant.ofEpochMilli(-1L), helper.getInstant("i")) + + helper.setDate("d", java.util.Date(-1L)) + assertEquals(java.util.Date(-1L), helper.getDate("d")) + + val lastDayOf1969 = java.time.LocalDate.of(1969, 12, 31) + assertEquals(-1L, lastDayOf1969.toEpochDay()) + helper.setLocalDate("ld", lastDayOf1969) + assertEquals(lastDayOf1969, helper.getLocalDate("ld")) + + val secondBeforeEpoch = java.time.LocalDateTime.of(1969, 12, 31, 23, 59, 59) + helper.setLocalDateTime("ldt", secondBeforeEpoch) + assertEquals(secondBeforeEpoch, helper.getLocalDateTime("ldt")) + } + + /** Assigning null removes the key outright now, matching `BaseDataStoreHelper`. */ + @Test + fun testNullTemporalRemovesKey() { + helper.setDate("d", java.util.Date(1_000L)) + assertTrue(helper.contains("d")) + + helper.setDate("d", null) + assertTrue(!helper.contains("d")) + assertNull(helper.getDate("d")) + } + + /** + * `LocalTime.ofSecondOfDay` throws outside 0..86399, so a stale pre-2.0 `-1L` would crash rather + * than misread. Out-of-range values read as null instead. + */ + @Test + fun testOutOfRangeLocalTimeReadsAsNullRatherThanThrowing() { + helper.setLong("lt", -1L) + assertNull(helper.getLocalTime("lt")) + + helper.setLong("lt", 999_999L) + assertNull(helper.getLocalTime("lt")) + } + + // endregion + + // region Legacy sentinel migration + + @Test + fun testMigrateLegacyTemporalSentinelsRemovesOnlySentinelKeys() { + helper.setLong("stale", -1L) + helper.setLong("real", 1_700_000_000_000L) + + val removed = helper.migrateLegacyTemporalSentinels("stale", "real", "never_written") + + assertEquals(listOf("stale"), removed) + assertTrue(!helper.contains("stale")) + assertEquals(1_700_000_000_000L, helper.getLong("real")) + } + + /** Safe to leave in place: a second run finds nothing and changes nothing. */ + @Test + fun testMigrateLegacyTemporalSentinelsIsIdempotent() { + helper.setLong("stale", -1L) + assertEquals(listOf("stale"), helper.migrateLegacyTemporalSentinels("stale")) + assertEquals(emptyList(), helper.migrateLegacyTemporalSentinels("stale")) + } + + /** A key holding something other than a Long must be skipped, not crash the migration. */ + @Test + fun testMigrateLegacyTemporalSentinelsSkipsNonLongKeys() { + helper.setString("text", "not a long") + assertEquals(emptyList(), helper.migrateLegacyTemporalSentinels("text")) + assertEquals("not a long", helper.getString("text")) + } + + /** End to end: the upgrade path a 1.x install actually takes. */ + @Test + fun testLegacyNullDateStillReadsAsNullAfterMigration() { + // What a pre-2.0 install left on disk for "no date". + helper.setLong("dob", -1L) + // Without the sweep this would surface as 1969-12-31. + assertEquals(java.util.Date(-1L), helper.getDate("dob")) + + helper.migrateLegacyTemporalSentinels("dob") + assertNull(helper.getDate("dob")) + } + + // endregion + + // region Version-stamped migrations + + @Test + fun testMigrateIfNeededSkipsFreshInstall() { + var ran = false + val invoked = helper.migrateIfNeeded(currentVersion = 2) { ran = true } + + assertTrue(!invoked) + assertTrue(!ran) + // Baseline stamped so the next upgrade has something to compare against. + assertEquals(2, prefs.getInt(BasePrefsHelper.KEY_HELPER_VERSION, -1)) + } + + @Test + fun testMigrateIfNeededTreatsUnstampedNonEmptyPrefsAsLegacy() { + helper.setString("pre_existing", "written by 1.x") + + var seenFrom = -99 + val invoked = helper.migrateIfNeeded(currentVersion = 2) { from -> seenFrom = from } + + assertTrue(invoked) + assertEquals(BasePrefsHelper.VERSION_LEGACY, seenFrom) + assertEquals(2, prefs.getInt(BasePrefsHelper.KEY_HELPER_VERSION, -1)) + } + + /** The whole point: a non-idempotent migration must not run twice. */ + @Test + fun testMigrateIfNeededRunsOnlyOnce() { + helper.setString("pre_existing", "written by 1.x") + + var runs = 0 + helper.migrateIfNeeded(currentVersion = 2) { runs++ } + helper.migrateIfNeeded(currentVersion = 2) { runs++ } + helper.migrateIfNeeded(currentVersion = 2) { runs++ } + + assertEquals(1, runs) + } + + @Test + fun testMigrateIfNeededRunsAgainForAHigherVersion() { + helper.setString("pre_existing", "written by 1.x") + + val seen = mutableListOf() + helper.migrateIfNeeded(currentVersion = 2) { seen += it } + helper.migrateIfNeeded(currentVersion = 3) { seen += it } + + assertEquals(listOf(BasePrefsHelper.VERSION_LEGACY, 2), seen) + assertEquals(3, prefs.getInt(BasePrefsHelper.KEY_HELPER_VERSION, -1)) + } + + /** An app downgrade must not rewind the stamp, or the migration would re-run on re-upgrade. */ + @Test + fun testMigrateIfNeededDoesNotRewindOnDowngrade() { + helper.setString("pre_existing", "written by 1.x") + helper.migrateIfNeeded(currentVersion = 5) { } + assertEquals(5, prefs.getInt(BasePrefsHelper.KEY_HELPER_VERSION, -1)) + + var ran = false + val invoked = helper.migrateIfNeeded(currentVersion = 3) { ran = true } + + assertTrue(!invoked) + assertTrue(!ran) + assertEquals(5, prefs.getInt(BasePrefsHelper.KEY_HELPER_VERSION, -1)) + } + + @Test(expected = IllegalArgumentException::class) + fun testMigrateIfNeededRejectsVersionBelowLegacy() { + helper.migrateIfNeeded(currentVersion = 0) { } } @Test diff --git a/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt b/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt index cf69070..2995b93 100644 --- a/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt +++ b/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt @@ -199,20 +199,21 @@ class BasePrefsHelperTest { } @Test - fun testSetInstantNullStoresSentinel() { + fun testSetInstantNullRemovesKey() { prefsHelper.setInstant("key", null) - verify(mockEditor).putLong("key", -1L) + verify(mockEditor).remove("key") } @Test fun testGetInstant() { - `when`(mockSharedPreferences.getLong("key", -1L)).thenReturn(1_700_000_000_000L) + `when`(mockSharedPreferences.contains("key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("key", 0L)).thenReturn(1_700_000_000_000L) assertEquals(java.time.Instant.ofEpochMilli(1_700_000_000_000L), prefsHelper.getInstant("key")) } @Test - fun testGetInstantReturnsNullForSentinel() { - `when`(mockSharedPreferences.getLong("key", -1L)).thenReturn(-1L) + fun testGetInstantReturnsNullWhenAbsent() { + `when`(mockSharedPreferences.contains("key")).thenReturn(false) assertNull(prefsHelper.getInstant("key")) } @@ -296,13 +297,14 @@ class BasePrefsHelperTest { @Test fun testSetNullDate() { prefsHelper.setDate("date_key", null) - verify(mockEditor).putLong("date_key", -1L) + verify(mockEditor).remove("date_key") verify(mockEditor).apply() } @Test fun testGetDate() { - `when`(mockSharedPreferences.getLong("date_key", -1L)).thenReturn(1234567890000L) + `when`(mockSharedPreferences.contains("date_key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("date_key", 0L)).thenReturn(1234567890000L) val date = prefsHelper.getDate("date_key") assertNotNull(date) assertEquals(1234567890000L, date?.time) @@ -310,7 +312,7 @@ class BasePrefsHelperTest { @Test fun testGetDateReturnsNullWhenNotSet() { - `when`(mockSharedPreferences.getLong("date_key", -1L)).thenReturn(-1L) + `when`(mockSharedPreferences.contains("date_key")).thenReturn(false) assertNull(prefsHelper.getDate("date_key")) } @@ -325,7 +327,7 @@ class BasePrefsHelperTest { @Test fun testSetNullLocalDateTime() { prefsHelper.setLocalDateTime("datetime_key", null) - verify(mockEditor).putLong("datetime_key", -1L) + verify(mockEditor).remove("datetime_key") verify(mockEditor).apply() } @@ -333,7 +335,8 @@ class BasePrefsHelperTest { fun testGetLocalDateTime() { val dateTime = LocalDateTime.of(2023, 11, 25, 10, 30, 45) val epochSecond = dateTime.toEpochSecond(ZoneOffset.UTC) - `when`(mockSharedPreferences.getLong("datetime_key", -1L)).thenReturn(epochSecond) + `when`(mockSharedPreferences.contains("datetime_key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("datetime_key", 0L)).thenReturn(epochSecond) val result = prefsHelper.getLocalDateTime("datetime_key") assertNotNull(result) assertEquals(dateTime, result) @@ -341,7 +344,7 @@ class BasePrefsHelperTest { @Test fun testGetLocalDateTimeReturnsNullWhenNotSet() { - `when`(mockSharedPreferences.getLong("datetime_key", -1L)).thenReturn(-1L) + `when`(mockSharedPreferences.contains("datetime_key")).thenReturn(false) assertNull(prefsHelper.getLocalDateTime("datetime_key")) } @@ -356,14 +359,15 @@ class BasePrefsHelperTest { @Test fun testSetNullLocalDate() { prefsHelper.setLocalDate("date_key", null) - verify(mockEditor).putLong("date_key", -1L) + verify(mockEditor).remove("date_key") verify(mockEditor).apply() } @Test fun testGetLocalDate() { val date = LocalDate.of(2023, 11, 25) - `when`(mockSharedPreferences.getLong("date_key", -1L)).thenReturn(date.toEpochDay()) + `when`(mockSharedPreferences.contains("date_key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("date_key", 0L)).thenReturn(date.toEpochDay()) val result = prefsHelper.getLocalDate("date_key") assertNotNull(result) assertEquals(date, result) @@ -371,10 +375,21 @@ class BasePrefsHelperTest { @Test fun testGetLocalDateReturnsNullWhenNotSet() { - `when`(mockSharedPreferences.getLong("date_key", -1L)).thenReturn(-1L) + `when`(mockSharedPreferences.contains("date_key")).thenReturn(false) assertNull(prefsHelper.getLocalDate("date_key")) } + /** + * Epoch day -1 is 1969-12-31, which the pre-2.0 sentinel made unstorable. It is an ordinary + * value now, so it must NOT be mistaken for "not set". + */ + @Test + fun testGetLocalDateReadsEpochDayMinusOneAsARealDate() { + `when`(mockSharedPreferences.contains("date_key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("date_key", 0L)).thenReturn(-1L) + assertEquals(LocalDate.of(1969, 12, 31), prefsHelper.getLocalDate("date_key")) + } + @Test fun testSetLocalTime() { val time = LocalTime.of(14, 30, 45) @@ -386,14 +401,15 @@ class BasePrefsHelperTest { @Test fun testSetNullLocalTime() { prefsHelper.setLocalTime("time_key", null) - verify(mockEditor).putLong("time_key", -1L) + verify(mockEditor).remove("time_key") verify(mockEditor).apply() } @Test fun testGetLocalTime() { val time = LocalTime.of(14, 30, 45) - `when`(mockSharedPreferences.getLong("time_key", -1L)).thenReturn(time.toSecondOfDay().toLong()) + `when`(mockSharedPreferences.contains("time_key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("time_key", 0L)).thenReturn(time.toSecondOfDay().toLong()) val result = prefsHelper.getLocalTime("time_key") assertNotNull(result) assertEquals(time, result) @@ -401,7 +417,7 @@ class BasePrefsHelperTest { @Test fun testGetLocalTimeReturnsNullWhenNotSet() { - `when`(mockSharedPreferences.getLong("time_key", -1L)).thenReturn(-1L) + `when`(mockSharedPreferences.contains("time_key")).thenReturn(false) assertNull(prefsHelper.getLocalTime("time_key")) } @@ -718,7 +734,8 @@ class BasePrefsHelperTest { // Instant delegate @Test fun testInstantPrefDelegateGet() { - `when`(mockSharedPreferences.getLong("instant_key", -1L)).thenReturn(1_234_567L) + `when`(mockSharedPreferences.contains("instant_key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("instant_key", 0L)).thenReturn(1_234_567L) assertEquals(java.time.Instant.ofEpochMilli(1_234_567L), prefsHelper.instantValue) } @@ -787,7 +804,8 @@ class BasePrefsHelperTest { // Date delegate @Test fun testDatePrefDelegateGet() { - `when`(mockSharedPreferences.getLong("date_delegate_key", -1L)).thenReturn(1234567890000L) + `when`(mockSharedPreferences.contains("date_delegate_key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("date_delegate_key", 0L)).thenReturn(1234567890000L) assertEquals(Date(1234567890000L), prefsHelper.dateValue) } @@ -800,7 +818,7 @@ class BasePrefsHelperTest { @Test fun testDatePrefDelegateReturnsNullWhenNotSet() { - `when`(mockSharedPreferences.getLong("date_delegate_key", -1L)).thenReturn(-1L) + `when`(mockSharedPreferences.contains("date_delegate_key")).thenReturn(false) assertNull(prefsHelper.dateValue) } @@ -808,7 +826,8 @@ class BasePrefsHelperTest { @Test fun testLocalDateTimePrefDelegateGet() { val dateTime = LocalDateTime.of(2023, 11, 25, 10, 30, 45) - `when`(mockSharedPreferences.getLong("ldt_delegate_key", -1L)) + `when`(mockSharedPreferences.contains("ldt_delegate_key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("ldt_delegate_key", 0L)) .thenReturn(dateTime.toEpochSecond(ZoneOffset.UTC)) assertEquals(dateTime, prefsHelper.localDateTimeValue) } @@ -825,7 +844,8 @@ class BasePrefsHelperTest { @Test fun testLocalDatePrefDelegateGet() { val date = LocalDate.of(2023, 11, 25) - `when`(mockSharedPreferences.getLong("ld_delegate_key", -1L)).thenReturn(date.toEpochDay()) + `when`(mockSharedPreferences.contains("ld_delegate_key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("ld_delegate_key", 0L)).thenReturn(date.toEpochDay()) assertEquals(date, prefsHelper.localDateValue) } @@ -841,7 +861,8 @@ class BasePrefsHelperTest { @Test fun testLocalTimePrefDelegateGet() { val time = LocalTime.of(14, 30, 45) - `when`(mockSharedPreferences.getLong("lt_delegate_key", -1L)).thenReturn(time.toSecondOfDay().toLong()) + `when`(mockSharedPreferences.contains("lt_delegate_key")).thenReturn(true) + `when`(mockSharedPreferences.getLong("lt_delegate_key", 0L)).thenReturn(time.toSecondOfDay().toLong()) assertEquals(time, prefsHelper.localTimeValue) } From 3996e1119dfdcceb5fe4a1de87c57f58e25cd71b Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 13:25:30 +0100 Subject: [PATCH 13/17] Replace the sample's getInstance() singleton with a Koin reference setup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The getInstance() double-checked singleton on NormalDataStore was not vestigial — DataStore still permits only one live instance per file, and MainActivity built PrefsHelper in onCreate, so without it a rotation would have constructed a second store and thrown. It was hand-rolled DI. Registering the helper as a Koin `single` gives the identical guarantee with none of the code, so getInstance() is gone. NormalPrefs, DevicePrefs and NormalDataStore are now injected into PrefsHelper. That also lets the sample demonstrate the two things 2.0 added that had no runnable example: - An application-lifetime scope carrying a CoroutineExceptionHandler, injected into NormalDataStore. This is the whole point of making `scope` injectable: without a handler, a failed fire-and-forget write reaches the thread's default handler and takes the process down. - migrateIfNeeded, in NormalPrefs, sweeping the pre-2.0 -1L sentinel. Side benefit: MainActivity was passing itself as the Context into long-lived helpers. They now get the application context. The library remains DI-agnostic — Koin is a :app dependency only. New PrefsModuleTest resolves the graph for real, because reference code that only compiles isn't proven. Mutation-checked: changing the NormalDataStore registration to `factory` fails exactly testDataStoreHelperIsASingleton, so the test genuinely pins the rule it claims to. Robolectric instantiates the manifest Application for every test, which would call startKoin repeatedly and throw once the JVM is reused, so robolectric.properties points it at the stock Application. No test uses the sample's graph — they all declare their own subclasses. README gains a "One instance per DataStore file" section covering both paths: DI registration for containers, and a correct double-checked singleton with applicationContext for apps without one. Tests 311 -> 316. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 44 +++++++++ app/build.gradle.kts | 6 ++ app/src/main/AndroidManifest.xml | 1 + .../main/java/com/duck/app/MainActivity.kt | 8 +- .../main/java/com/duck/app/PrefsHelperApp.kt | 26 ++++++ .../com/duck/app/data/prefs/PrefsHelper.kt | 69 ++++++++++---- .../main/java/com/duck/app/di/PrefsModule.kt | 55 +++++++++++ .../java/com/duck/app/di/PrefsModuleTest.kt | 92 +++++++++++++++++++ app/src/test/resources/robolectric.properties | 6 ++ gradle/libs.versions.toml | 3 + 10 files changed, 293 insertions(+), 17 deletions(-) create mode 100644 app/src/main/java/com/duck/app/PrefsHelperApp.kt create mode 100644 app/src/main/java/com/duck/app/di/PrefsModule.kt create mode 100644 app/src/test/java/com/duck/app/di/PrefsModuleTest.kt diff --git a/README.md b/README.md index 7751545..9d2b497 100644 --- a/README.md +++ b/README.md @@ -67,6 +67,50 @@ class AppPrefs(context: Context) : BaseDataStoreHelper(context, "app_prefs") { } ``` +### One instance per DataStore file — required + +**`BaseDataStoreHelper` subclasses must be singletons.** DataStore permits only one live instance per file per process; construct a second one over the same file and it throws: + +``` +IllegalStateException: There are multiple DataStores active for the same file +``` + +This is a DataStore rule, not one this library adds, and it is easy to trip by accident — building a helper in `Activity.onCreate` is enough, because a configuration change constructs another one while the first is still alive. + +`BasePrefsHelper` has no such constraint: `Context.getSharedPreferences` already returns a process-wide shared instance, so constructing several is wasteful but harmless. + +**With a DI container**, register it as a singleton and you're done — this is the whole enforcement mechanism: + +```kotlin +// Koin +single { AppPrefs(androidContext(), get(named("appScope"))) } // correct +factory { AppPrefs(androidContext(), get(named("appScope"))) } // throws on the second injection +``` + +```kotlin +// Hilt +@Provides @Singleton +fun provideAppPrefs(@ApplicationContext context: Context): AppPrefs = AppPrefs(context) +``` + +**Without a DI container**, you have to enforce it yourself. A Kotlin `object` works if you don't need a `Context` at construction; otherwise use a double-checked singleton and always pass the *application* context, never an Activity: + +```kotlin +class AppPrefs private constructor(context: Context) : BaseDataStoreHelper(context, "app_prefs") { + + companion object { + @Volatile private var INSTANCE: AppPrefs? = null + + fun getInstance(context: Context): AppPrefs = + INSTANCE ?: synchronized(this) { + INSTANCE ?: AppPrefs(context.applicationContext).also { INSTANCE = it } + } + } +} +``` + +The sample app in `app/` shows the DI version, and its `PrefsHelper.kt` notes what the `getInstance()` it used to have was actually buying. + ### Coroutines: `dispatcher` and `scope` Both base classes take a `dispatcher`; `BaseDataStoreHelper` also takes a `scope`. They do two separate jobs and neither defaults will surprise you: diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 82de954..7b6a073 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -87,6 +87,11 @@ dependencies { // implementation(libs.androidx.dataStore) implementation(project(":PrefsHelper")) + // Sample only — the library itself is DI-agnostic and depends on no DI framework. + // Used to demonstrate injecting an application-lifetime scope and guaranteeing the + // one-instance-per-DataStore-file rule structurally instead of via a hand-rolled singleton. + implementation(libs.koin.android) + // Aggregate the library's coverage into this module's Kover report, // since the tests that exercise PrefsHelper live here in :app. kover(project(":PrefsHelper")) @@ -100,6 +105,7 @@ dependencies { testImplementation(libs.androidx.test.ext.junit) testImplementation(libs.robolectric) testImplementation(libs.kotlinx.coroutines.test) + testImplementation(libs.koin.test) debugImplementation(libs.androidx.compose.ui.tooling) } diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 99a1d4d..bd24b1e 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -2,6 +2,7 @@ + if (fromVersion < 2) { + migrateLegacyTemporalSentinels(KEY_LAST_SEEN) + } + } + } + var exampleValue by stringPref(KEY_EXAMPLE, defaultValue = "") + var lastSeen by datePref(KEY_LAST_SEEN) companion object { const val KEY_EXAMPLE = "example_key" + const val KEY_LAST_SEEN = "last_seen" + + /** Bump alongside a new branch in the [migrateIfNeeded] block above. */ + private const val PREFS_VERSION = 2 } } @@ -45,21 +75,28 @@ class DevicePrefs(context: Context) : BasePrefsHelper() { } } -class NormalDataStore private constructor(context: Context) : BaseDataStoreHelper(context, "normal_dataStore") { +/** + * No `getInstance()` here any more. + * + * DataStore permits only one live instance per file per process, and that rule has not gone away — + * what changed is who enforces it. Registering this as a Koin `single` gives the same guarantee the + * old double-checked `getInstance()` did, with none of the code. Apps that don't use a DI container + * still have to enforce it themselves; see the README. + * + * [appScope] is the second thing worth noticing: fire-and-forget writes (the `*Async` methods, and + * every delegate setter) launch in it, so injecting an application-lifetime scope carrying a + * `CoroutineExceptionHandler` is what stops a failed background write from reaching the default + * handler and taking the process with it. + */ +class NormalDataStore( + context: Context, + appScope: CoroutineScope, +) : BaseDataStoreHelper(context, "normal_dataStore", scope = appScope) { var exampleValue by stringPref(KEY_EXAMPLE, defaultValue = "") val exampleValueFlow = stringPrefFlow(KEY_EXAMPLE) companion object { const val KEY_EXAMPLE = "example_key" - - @Volatile - private var INSTANCE: NormalDataStore? = null - fun getInstance(context: Context) = - INSTANCE ?: synchronized(this) { - INSTANCE ?: NormalDataStore(context).also { - INSTANCE = it - } - } } } diff --git a/app/src/main/java/com/duck/app/di/PrefsModule.kt b/app/src/main/java/com/duck/app/di/PrefsModule.kt new file mode 100644 index 0000000..f1e39a0 --- /dev/null +++ b/app/src/main/java/com/duck/app/di/PrefsModule.kt @@ -0,0 +1,55 @@ +package com.duck.app.di + +import android.util.Log +import com.duck.app.data.prefs.DevicePrefs +import com.duck.app.data.prefs.NormalDataStore +import com.duck.app.data.prefs.NormalPrefs +import com.duck.app.data.prefs.PrefsHelper +import kotlinx.coroutines.CoroutineExceptionHandler +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import org.koin.android.ext.koin.androidContext +import org.koin.core.module.dsl.singleOf +import org.koin.core.qualifier.named +import org.koin.dsl.module + +/** + * Qualifier for the application-lifetime [CoroutineScope] that owns fire-and-forget prefs writes. + */ +val APP_SCOPE = named("appScope") + +/** + * Reference wiring for PrefsHelper. Two things here are the point of the example: + * + * 1. **[NormalDataStore] is a `single`.** DataStore allows only one live instance per file per + * process. Declaring it as a `single` is what enforces that — it replaces the hand-rolled + * `getInstance()` double-checked singleton this sample used before 2.0. Register a DataStore + * helper as a `factory` and the second instantiation will throw. + * + * 2. **The scope is injected, with a handler attached.** `*Async` writes and every delegate setter + * launch in the helper's `scope`. The default is a bare per-instance `SupervisorJob`, which has + * nowhere to report a failure — an exception from a background write reaches the thread's + * default handler and takes the process down. Passing a scope that carries a + * [CoroutineExceptionHandler] is the fix, and is the reason 2.0 made the scope injectable. + * + * `BasePrefsHelper` subclasses have neither constraint — SharedPreferences is already + * process-shared and does no background work — so they are `single` only to avoid re-reading the + * file, not out of necessity. + */ +val prefsModule = module { + single(APP_SCOPE) { + CoroutineScope( + SupervisorJob() + Dispatchers.IO + CoroutineExceptionHandler { _, throwable -> + // A real app would route this to Crashlytics/Sentry rather than logcat. + Log.e("PrefsHelper", "Fire-and-forget preference write failed", throwable) + }, + ) + } + + single { NormalPrefs(androidContext()) } + single { DevicePrefs(androidContext()) } + single { NormalDataStore(androidContext(), get(APP_SCOPE)) } + + singleOf(::PrefsHelper) +} diff --git a/app/src/test/java/com/duck/app/di/PrefsModuleTest.kt b/app/src/test/java/com/duck/app/di/PrefsModuleTest.kt new file mode 100644 index 0000000..f20b854 --- /dev/null +++ b/app/src/test/java/com/duck/app/di/PrefsModuleTest.kt @@ -0,0 +1,92 @@ +package com.duck.app.di + +import android.content.Context +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import com.duck.app.data.prefs.DevicePrefs +import com.duck.app.data.prefs.NormalDataStore +import com.duck.app.data.prefs.NormalPrefs +import com.duck.app.data.prefs.PrefsHelper +import kotlinx.coroutines.CoroutineScope +import org.junit.After +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertSame +import org.junit.Test +import org.junit.runner.RunWith +import org.koin.android.ext.koin.androidContext +import org.koin.core.context.startKoin +import org.koin.core.context.stopKoin +import org.koin.test.KoinTest +import org.koin.test.get + +/** + * The sample's DI graph is reference code people copy, so "it compiles" isn't enough — a missing + * binding or a wrong qualifier only shows up when the app launches. This resolves the graph for + * real and pins the property that actually matters. + */ +@RunWith(AndroidJUnit4::class) +class PrefsModuleTest : KoinTest { + + @After + fun tearDown() { + stopKoin() + } + + private fun start() { + startKoin { + androidContext(InstrumentationRegistry.getInstrumentation().targetContext) + modules(prefsModule) + } + } + + @Test + fun testEveryDefinitionResolves() { + start() + + assertNotNull(get(APP_SCOPE)) + assertNotNull(get()) + assertNotNull(get()) + assertNotNull(get()) + assertNotNull(get()) + } + + /** + * The point of registering [NormalDataStore] as a `single`. DataStore allows one live instance + * per file per process; a `factory` here would construct a second and throw. Without this the + * module could silently regress to a factory and nothing would notice until runtime. + */ + @Test + fun testDataStoreHelperIsASingleton() { + start() + + assertSame(get(), get()) + } + + /** The façade must be handed the same instances the container holds, not fresh ones. */ + @Test + fun testFacadeSharesTheContainerInstances() { + start() + + assertSame(get(), get()) + assertSame(get(), get()) + assertSame(get(), get()) + } + + /** The injected scope is what gives fire-and-forget writes somewhere to report failures. */ + @Test + fun testAppScopeIsSharedAndActive() { + start() + + val scope = get(APP_SCOPE) + assertSame(scope, get(APP_SCOPE)) + assertNotNull(scope.coroutineContext[kotlinx.coroutines.CoroutineExceptionHandler]) + } + + /** `androidContext()` must be wired, or every helper needing a Context fails to resolve. */ + @Test + fun testAndroidContextIsAvailable() { + start() + + assertNotNull(get()) + } +} diff --git a/app/src/test/resources/robolectric.properties b/app/src/test/resources/robolectric.properties index 512cfae..a5cf0b6 100644 --- a/app/src/test/resources/robolectric.properties +++ b/app/src/test/resources/robolectric.properties @@ -2,3 +2,9 @@ # Robolectric android-all image yet; 36 (Android 16) is the newest Robolectric # 4.16 ships and is closest to compileSdk/targetSdk. Bump when 37 is published. sdk=36 + +# Use the stock Application rather than the sample's PrefsHelperApp. Robolectric instantiates the +# manifest's Application for every test, which would call startKoin() repeatedly and fail with +# KoinAppAlreadyStartedException once the JVM is reused. No test touches the sample's DI graph — +# they all declare their own helper subclasses — so there is nothing to lose by skipping it. +application=android.app.Application diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 9f26415..df58fef 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -7,6 +7,7 @@ dokka = "2.2.0" kover = "0.9.9" dataStore = "1.2.1" coroutines = "1.11.0" +koin = "4.1.1" mockitoCore = "5.23.0" mockitoKotlin = "6.3.0" robolectric = "4.16.1" @@ -36,6 +37,8 @@ junit = { module = "junit:junit", version.ref = "junit" } androidx-dataStore = { group = "androidx.datastore", name = "datastore-preferences", version.ref = "dataStore" } kotlinx-coroutines-test = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-test", version.ref = "coroutines" } +koin-android = { module = "io.insert-koin:koin-android", version.ref = "koin" } +koin-test = { module = "io.insert-koin:koin-test", version.ref = "koin" } android-documentation-plugin = { module = "org.jetbrains.dokka:android-documentation-plugin", version.ref = "dokka" } From c0ed8c62a77b636419f060df26a203084e0bd602 Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 14:16:20 +0100 Subject: [PATCH 14/17] Fix empty Set being mistaken for an absent key on BasePrefsHelper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getEnumSet and the enumSetPref delegate branched on names.isEmpty(), which conflates two different states: the key being absent, and the key holding an explicitly stored empty set. Only the first should yield the default. The consequence was that an empty enum set could not be stored at all whenever the default was non-empty — writing emptySet() and reading it back returned the default. It also broke the parity claimed a commit earlier, since BaseDataStoreHelper reads via `readStringSetValue(key) ?: default` and therefore distinguishes null from empty correctly. Both call sites now check contains(key) first, matching how every other nullable accessor on this class decides absence. The gap is a little embarrassing: BasePrefsHelperRealPrefsTest already had testEmptyStringSetIsDistinctFromAbsent for Set and the same reasoning simply wasn't applied to Set. There are now equivalents on both helpers, including one on the DataStore side purely as a parity guard so the two cannot drift again. Also corrects a KDoc on BaseDataStoreHelper.writeDate that still described BasePrefsHelper as writing a -1L sentinel — true until the previous commit removed it. Found by Grok in review. Tests 316 -> 318. Co-Authored-By: Claude Opus 5 (1M context) --- .../duck/prefshelper/BaseDataStoreHelper.kt | 4 +-- .../com/duck/prefshelper/BasePrefsHelper.kt | 6 ++-- .../com/duck/app/BaseDataStoreHelperTest.kt | 20 +++++++++++ .../duck/app/BasePrefsHelperRealPrefsTest.kt | 34 +++++++++++++++++++ .../java/com/duck/app/BasePrefsHelperTest.kt | 3 ++ 5 files changed, 63 insertions(+), 4 deletions(-) diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt index cdf3666..2d0881e 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BaseDataStoreHelper.kt @@ -632,8 +632,8 @@ abstract class BaseDataStoreHelper( /** * Add [Date] to the data store * - * Stored as epoch milliseconds in a [Long]. If value is null, the key will be removed — note - * this differs from `BasePrefsHelper`, which writes a -1L sentinel for temporal types. + * Stored as epoch milliseconds in a [Long]. If value is null, the key will be removed, matching + * `BasePrefsHelper` — which used to write a -1L sentinel here, but stopped doing so in 2.0. * * @param key The key to store the value under * @param value The value to store diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt index b4c847b..386c7ee 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt @@ -537,8 +537,9 @@ abstract class BasePrefsHelper( * @param default The value to return if the key does not exist, defaults to an empty set */ inline fun > getEnumSet(key: String, default: Set = emptySet()): Set { + // Absent and "stored, but empty" are different answers: only the former is the default. + if (!contains(key)) return default val names = getStringSet(key, emptySet()) - if (names.isEmpty()) return default return try { val constants = T::class.java.enumConstants ?: return default names.mapNotNullTo(LinkedHashSet()) { name -> constants.firstOrNull { it.name == name } } @@ -870,8 +871,9 @@ abstract class BasePrefsHelper( val constants = T::class.java.enumConstants return object : ReadWriteProperty> { override fun getValue(thisRef: Any?, property: KProperty<*>): Set { + // Absent and "stored, but empty" are different answers — see [getEnumSet]. + if (!contains(key)) return defaultValue val names = getStringSet(key, emptySet()) - if (names.isEmpty()) return defaultValue return names.mapNotNullTo(LinkedHashSet()) { name -> constants?.firstOrNull { it.name == name } } } override fun setValue(thisRef: Any?, property: KProperty<*>, value: Set) = setEnumSet(key, value) diff --git a/app/src/test/java/com/duck/app/BaseDataStoreHelperTest.kt b/app/src/test/java/com/duck/app/BaseDataStoreHelperTest.kt index 69df9a0..d59cc31 100644 --- a/app/src/test/java/com/duck/app/BaseDataStoreHelperTest.kt +++ b/app/src/test/java/com/duck/app/BaseDataStoreHelperTest.kt @@ -1269,6 +1269,26 @@ class BaseDataStoreHelperTest { assertNull(dataStoreHelper.testReadStringSetValue("null_enum_set")) } + /** + * Parity guard against `BasePrefsHelper`: a stored empty set is not the same as an absent key, + * so it must win over a non-empty default. `BasePrefsHelper.getEnumSet` originally got this + * wrong by testing `isEmpty()` instead of key presence. + */ + @Test + fun testEmptyEnumSetBeatsNonEmptyDefault() = runBlocking { + val default = setOf(TestEnum.VALUE_A) + + // Absent -> default. + assertEquals(default, dataStoreHelper.testReadEnumSetValue("absent_enum_set", default)) + + // Stored empty -> empty, not the default. + dataStoreHelper.testWriteEnumSet("stored_empty_enum_set", emptySet()) + assertEquals( + emptySet(), + dataStoreHelper.testReadEnumSetValue("stored_empty_enum_set", default), + ) + } + @Test fun testReadEnumSetFlow() = runBlocking { dataStoreHelper.testWriteEnumSet("enum_set_flow_key", setOf(TestEnum.VALUE_B)) diff --git a/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt b/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt index 90fa9ac..476c51f 100644 --- a/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt +++ b/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt @@ -362,6 +362,33 @@ class BasePrefsHelperRealPrefsTest { ) } + /** + * A stored empty set must not be confused with an absent key — otherwise an empty selection is + * unstorable whenever the default is non-empty, and this helper disagrees with + * `BaseDataStoreHelper`, which distinguishes the two. + */ + @Test + fun testEmptyEnumSetIsDistinctFromAbsent() { + val withDefault = EnumSetDefaultHelper(prefs) + + // Absent -> the default. + assertEquals(setOf(BasePrefsHelperTest.TestEnum.VALUE_A), withDefault.features) + + // Explicitly stored empty -> empty, NOT the default. + withDefault.features = emptySet() + assertEquals(emptySet(), withDefault.features) + + // Same distinction on the direct accessor. + assertEquals( + emptySet(), + helper.getEnumSet("features", setOf(BasePrefsHelperTest.TestEnum.VALUE_A)), + ) + assertEquals( + setOf(BasePrefsHelperTest.TestEnum.VALUE_A), + helper.getEnumSet("never_written_enum_set", setOf(BasePrefsHelperTest.TestEnum.VALUE_A)), + ) + } + /** Names that no longer map to a constant are dropped, not thrown on. */ @Test fun testEnumSetDropsUnknownStoredNames() { @@ -379,4 +406,11 @@ class BasePrefsHelperRealPrefsTest { var byteArrayValue by byteArrayPref("byte_array_key") var enumSetValue by enumSetPref("enum_set_key") } + + /** A delegate with a non-empty default, to expose empty-vs-absent confusion. */ + private class EnumSetDefaultHelper( + override val sharedPreferences: SharedPreferences, + ) : BasePrefsHelper() { + var features by enumSetPref("features", defaultValue = setOf(BasePrefsHelperTest.TestEnum.VALUE_A)) + } } diff --git a/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt b/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt index 2995b93..65cb6ee 100644 --- a/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt +++ b/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt @@ -225,6 +225,7 @@ class BasePrefsHelperTest { @Test fun testGetEnumSet() { + `when`(mockSharedPreferences.contains("key")).thenReturn(true) `when`(mockSharedPreferences.getStringSet("key", null)).thenReturn(linkedSetOf("VALUE_A", "VALUE_C")) assertEquals(setOf(TestEnum.VALUE_A, TestEnum.VALUE_C), prefsHelper.getEnumSet("key")) } @@ -235,6 +236,7 @@ class BasePrefsHelperTest { */ @Test fun testGetEnumSetDropsUnknownNames() { + `when`(mockSharedPreferences.contains("key")).thenReturn(true) `when`(mockSharedPreferences.getStringSet("key", null)) .thenReturn(linkedSetOf("VALUE_A", "REMOVED_IN_A_LATER_VERSION")) assertEquals(setOf(TestEnum.VALUE_A), prefsHelper.getEnumSet("key")) @@ -748,6 +750,7 @@ class BasePrefsHelperTest { // Enum set delegate @Test fun testEnumSetPrefDelegateGet() { + `when`(mockSharedPreferences.contains("enum_set_key")).thenReturn(true) `when`(mockSharedPreferences.getStringSet("enum_set_key", null)) .thenReturn(linkedSetOf("VALUE_B", "VALUE_C")) assertEquals(setOf(TestEnum.VALUE_B, TestEnum.VALUE_C), prefsHelper.enumSetValue) From 52e57fbf85fd538303c018dd1964e789a21ae5b0 Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 14:51:21 +0100 Subject: [PATCH 15/17] Fix clearPrefs wiping the migration stamp, and align Enum null semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from Grok's third review, both real. clearPrefs() cleared KEY_HELPER_VERSION along with user data (score 88). That is a data-corruption path the sample's own shape sets up: stamp at v2, logout calls clearPrefs, the helper is a DI singleton so init never re-runs, the app writes a preference, and the file is now non-empty and unstamped. The next cold start reads that as a pre-versioning install and re-runs every migration. Harmless for the idempotent sentinel sweep, corrupting for exactly the non-idempotent migrations migrateIfNeeded exists to protect. Worse, the README recommends clearPrefs on logout in the same guide that introduces versioned migrations, so a consumer following both pieces of advice would hit it. The stamp is schema metadata, not user state, so clearPrefs now preserves it. Editor.clear() is applied before puts in the same edit regardless of call order, so the restore is atomic rather than a second commit. Both the unit case and the full logout-then-relaunch sequence are covered, and both were confirmed failing before the fix. setEnum(null) wrote "" instead of removing the key (score 82). Reads coped, but contains(key) stayed true here and false on BaseDataStoreHelper — directly contradicting 2.0's "null means the same thing on both helpers" claim. Now removes the key. Docs corrected where they overclaimed: - CLAUDE.md still said temporal types "preserve the existing -1L sentinel" in one bullet while another said it was gone. - README claimed every type has both delegate overloads; it does not, and the two helpers differ. Replaced with the actual matrix. - Migration item 8 listed Instant among types 1.x wrote sentinels for. Instant is new in 2.0 and never had one. - Made "list every temporal key" explicit about why the sweep is per-key and what a forgotten key looks like, and resolved the contradictory advice about whether to leave the call in place. PrefsModuleTest's facade assertion was half-theatre — it proved PrefsHelper was a single, not that the container's instances were the ones injected. It now writes through the facade and reads from the separately-resolved sub-helper. Tests 318 -> 321. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 2 +- .../com/duck/prefshelper/BasePrefsHelper.kt | 44 ++++++++++--- README.md | 21 +++++-- .../duck/app/BasePrefsHelperRealPrefsTest.kt | 61 +++++++++++++++++++ .../java/com/duck/app/BasePrefsHelperTest.kt | 6 +- .../java/com/duck/app/di/PrefsModuleTest.kt | 28 +++++++-- 6 files changed, 142 insertions(+), 20 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 067e81d..6b132a9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -44,7 +44,7 @@ The library provides two abstract base classes that consumers extend: - Synchronous API for reads, async writes via `edit {}` - Subclasses must provide `sharedPreferences` instance - Takes a `dispatcher: CoroutineDispatcher = Dispatchers.IO` (no `Job`), used by `clearPrefs()`. Because the dispatcher carries no Job, `clearPrefs()` is cancelled when its caller is cancelled — this is deliberate, see the 2.0 migration note in the README. -- Preferred usage is the `*Pref` property delegate factories (e.g. `var flag by booleanPref(KEY, defaultValue = false)`). Each type has a non-nullable overload `(key, defaultValue)` and a nullable overload `(key)`. Temporal types (`Date`, `LocalDateTime`, `LocalDate`, `LocalTime`) are nullable-only and preserve the existing -1L sentinel. +- Preferred usage is the `*Pref` property delegate factories (e.g. `var flag by booleanPref(KEY, defaultValue = false)`). Most types have a non-nullable overload `(key, defaultValue)` and a nullable overload `(key)`; the temporal types (`Date`, `Instant`, `LocalDateTime`, `LocalDate`, `LocalTime`) and `ByteArray` are nullable-only here, and `enumSetPref` is non-null-with-default only. The `-1L` sentinel those temporal types used is **gone as of 2.0** — null removes the key, see the Migrations section below. ### BaseDataStoreHelper diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt index 386c7ee..38e9856 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt @@ -46,10 +46,28 @@ abstract class BasePrefsHelper( protected abstract val sharedPreferences: SharedPreferences /** - * Clear all preferences + * Clear all preferences. + * + * The [KEY_HELPER_VERSION] stamp written by [migrateIfNeeded] is deliberately preserved: it is + * schema metadata, not user state. Clearing it would leave the file non-empty and unstamped as + * soon as anything was written afterwards, which [migrateIfNeeded] reads as a pre-versioning + * install — re-running every migration on the next cold start. Harmless for an idempotent + * migration, corrupting for exactly the non-idempotent ones the version stamp exists to protect. + * + * `Editor.clear()` is applied before any put in the same edit regardless of call order, so the + * stamp is restored atomically rather than in a second commit. */ open suspend fun clearPrefs(): Unit = withContext(dispatcher) { - sharedPreferences.edit(commit = true) { clear() } + val version = + if (sharedPreferences.contains(KEY_HELPER_VERSION)) { + sharedPreferences.getInt(KEY_HELPER_VERSION, VERSION_LEGACY) + } else { + null + } + sharedPreferences.edit(commit = true) { + clear() + if (version != null) putInt(KEY_HELPER_VERSION, version) + } } /** @@ -150,12 +168,17 @@ abstract class BasePrefsHelper( * } * ``` * - * Only needed for installs that ran a pre-2.0 version. New installs can skip it, and it is safe - * to leave in place indefinitely. + * Only needed for installs that ran a pre-2.0 version. + * + * **Keep the call inside a [migrateIfNeeded] branch permanently** rather than deleting it once + * your install base turns over. Guarded that way it runs once per install and never sees data + * written by 2.0. Called bare from `init`, it re-examines those keys on every construction + * forever — and a genuine `-1L` written by 2.0 (a `Date(-1)`, or a [LocalDate] of `1969-12-31`) + * would then be deleted as though it were a stale sentinel. * - * Note the flip side: a genuine `-1L` written by 2.0 — `Date(-1)`, or `LocalDate` of - * `1969-12-31` — would also be removed. Don't pass keys whose real value could legitimately be - * `-1L`, and drop the call once your install base has turned over. + * List every temporal key the helper owns: the sweep is per-key on purpose, since scanning the + * whole file for `-1L` would destroy unrelated `Long` preferences that legitimately hold `-1`. + * A key you forget stays broken silently, surfacing as a 1969 date rather than an error. * * @param keys The temporal preference keys to sweep * @return The keys that were actually removed @@ -512,7 +535,12 @@ abstract class BasePrefsHelper( * @param value The value to store */ fun setEnum(key: String, value: Enum<*>?) { - setString(key, value?.name ?: "") + // Removes rather than writing "" so null means the same thing here as everywhere else, and + // as it does on BaseDataStoreHelper. Pre-2.0 this left an empty string behind, which read + // back as null but made contains(key) disagree between the two helpers. + sharedPreferences.edit { + if (value == null) remove(key) else putString(key, value.name) + } } /** diff --git a/README.md b/README.md index 9d2b497..d3988ff 100644 --- a/README.md +++ b/README.md @@ -214,7 +214,16 @@ class PrefsHelper(context: Context) { `String`, `Int`, `Long`, `Float`, `Double`, `Boolean`, `ByteArray`, `Set`, `Date`, `Instant`, `LocalDateTime`, `LocalDate`, `LocalTime`, `Enum<*>`, and `Set`. -Each type exposes a non-nullable delegate `*Pref(key, defaultValue)` and a nullable delegate `*Pref(key)`. `BaseDataStoreHelper` additionally exposes matching `*PrefFlow` accessors for reactive reads. The `java.time` types require API 26 (`@RequiresApi(O)`), as they did before. +Most types expose a non-nullable delegate `*Pref(key, defaultValue)` and a nullable delegate `*Pref(key)`. `BaseDataStoreHelper` additionally exposes matching `*PrefFlow` accessors for reactive reads. The `java.time` types require API 26 (`@RequiresApi(O)`), as they did before. + +The overload sets aren't identical across the two helpers — a historical shape rather than a rule: + +| | `BasePrefsHelper` | `BaseDataStoreHelper` | +|---|---|---| +| `Date`, `Instant`, `LocalDateTime`, `LocalDate`, `LocalTime` | nullable only | nullable **and** non-null-with-default | +| `ByteArray` | nullable only | nullable only | +| `Set` | non-null-with-default only | non-null-with-default only | +| everything else | both | both | **Null means the same thing on both helpers as of 2.0:** assigning `null` removes the key, and an absent key reads back as `null`. There is no sentinel value any more — see [migration item 8](#8-the--1l-temporal-sentinel-is-gone) if you are upgrading. @@ -346,7 +355,7 @@ Nothing existing changes — these are additions, and each follows its helper's ### 8. The `-1L` temporal sentinel is gone -**This one touches stored data. Read it if you use `Date`, `Instant`, `LocalDateTime`, `LocalDate` or `LocalTime` on `BasePrefsHelper`.** +**This one touches stored data. Read it if you use `Date`, `LocalDateTime`, `LocalDate` or `LocalTime` on `BasePrefsHelper`.** (`Instant` is new in 2.0, so no 1.x install ever wrote a sentinel for it.) Before 2.0, assigning `null` to one of those wrote `-1L` rather than removing the key, and any stored `-1L` read back as `null`. 2.0 removes the key instead, matching `BaseDataStoreHelper`. @@ -362,9 +371,13 @@ init { } ``` -`migrateLegacyTemporalSentinels` removes each listed key that currently holds `-1L`, so it reads as `null` exactly as before. It ignores absent keys, other values and non-`Long` keys, so it's safe to run repeatedly — the `migrateIfNeeded` wrapper is belt-and-braces, and worth adopting for the migrations that follow. Drop the call once your install base has turned over. +`migrateLegacyTemporalSentinels` removes each listed key that currently holds `-1L`, so it reads as `null` exactly as before. It ignores absent keys, other values and non-`Long` keys, so it's safe to run repeatedly. + +**List every temporal key the helper owns, including ones you rarely read.** The sweep is opt-in per key by design — it deliberately does *not* scan the file for `-1L` longs, because that would delete genuine `Long` preferences that happen to hold `-1`. The cost of that choice is that a key you forget stays broken, silently, and surfaces as a `1969-12-31` date rather than an error. Grep your subclass for every temporal delegate before writing the list. + +Conversely, don't pass keys whose genuine value could be `-1L` — a real `Date(-1)` or `LocalDate` of 1969-12-31 written by 2.0 would be deleted. -Don't pass keys whose genuine value could be `-1L`, or you'll delete real data. +Keep the call inside the `migrateIfNeeded(from < 2)` branch permanently rather than deleting it later: it then runs once per install and never touches data written by 2.0. A bare `init { migrateLegacyTemporalSentinels(…) }` is the form that stays dangerous, because it re-examines those keys on every construction forever. `LocalTime` needs no migration: its valid range is 0..86399, so `-1` was never a legal value. Out-of-range stored values now read as `null` instead of throwing. diff --git a/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt b/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt index 476c51f..e9b20c1 100644 --- a/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt +++ b/app/src/test/java/com/duck/app/BasePrefsHelperRealPrefsTest.kt @@ -5,6 +5,7 @@ import android.content.SharedPreferences import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import com.duck.prefshelper.BasePrefsHelper +import kotlinx.coroutines.runBlocking import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals import org.junit.Assert.assertNotEquals @@ -353,6 +354,51 @@ class BasePrefsHelperRealPrefsTest { helper.migrateIfNeeded(currentVersion = 0) { } } + /** + * The version stamp is schema metadata, not user session state, so `clearPrefs()` must not take + * it with them. + * + * Without this, the sample's own shape is a data-corruption path: stamp at v2 → logout calls + * clearPrefs → the helper is a DI singleton so `init` never re-runs → the app writes a + * preference → the file is now non-empty *and* unstamped → next cold start reads that as + * VERSION_LEGACY and re-runs every migration. Fine for the idempotent sentinel sweep, corrupting + * for the non-idempotent migrations `migrateIfNeeded` exists to make safe. + */ + @Test + fun testClearPrefsPreservesTheVersionStamp() = runBlocking { + helper.setString("pre_existing", "written by 1.x") + helper.migrateIfNeeded(currentVersion = 2) { } + assertEquals(2, prefs.getInt(BasePrefsHelper.KEY_HELPER_VERSION, -1)) + + helper.clearPrefs() + + assertEquals( + "clearPrefs must not reset the migration high-water mark", + 2, + prefs.getInt(BasePrefsHelper.KEY_HELPER_VERSION, -1), + ) + // User data is still gone — only the stamp survives. + assertNull(helper.getString("pre_existing", "").takeIf { it.isNotEmpty() }) + } + + /** The full logout-then-relaunch sequence, end to end. */ + @Test + fun testMigrationDoesNotRerunAfterClearPrefs() = runBlocking { + helper.setString("pre_existing", "written by 1.x") + + var runs = 0 + helper.migrateIfNeeded(currentVersion = 2) { runs++ } + assertEquals(1, runs) + + helper.clearPrefs() + helper.setString("written_after_logout", "x") + + // A cold start reconstructs the helper over the same file. + RealPrefsHelper(prefs).migrateIfNeeded(currentVersion = 2) { runs++ } + + assertEquals("migration must not re-run after clearPrefs", 1, runs) + } + @Test fun testEnumSetRoundTrips() { helper.enumSetValue = setOf(BasePrefsHelperTest.TestEnum.VALUE_A, BasePrefsHelperTest.TestEnum.VALUE_C) @@ -389,6 +435,21 @@ class BasePrefsHelperRealPrefsTest { ) } + /** + * Null must remove the key, not leave an empty string behind. Reads coped either way, but + * `contains(key)` did not — it stayed true on this helper and false on `BaseDataStoreHelper`, + * contradicting 2.0's "null means the same thing on both" claim. + */ + @Test + fun testNullEnumRemovesKey() { + helper.setEnum("e", BasePrefsHelperTest.TestEnum.VALUE_B) + assertTrue(helper.contains("e")) + + helper.setEnum("e", null) + assertTrue("null enum must remove the key, not store \"\"", !helper.contains("e")) + assertNull(helper.getEnum("e")) + } + /** Names that no longer map to a constant are dropped, not thrown on. */ @Test fun testEnumSetDropsUnknownStoredNames() { diff --git a/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt b/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt index 65cb6ee..1ba9963 100644 --- a/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt +++ b/app/src/test/java/com/duck/app/BasePrefsHelperTest.kt @@ -432,8 +432,10 @@ class BasePrefsHelperTest { @Test fun testSetNullEnum() { + // 2.0: removes the key rather than writing "", so null means the same thing here as on + // BaseDataStoreHelper and contains(key) agrees between the two. prefsHelper.setEnum("enum_key", null) - verify(mockEditor).putString("enum_key", "") + verify(mockEditor).remove("enum_key") verify(mockEditor).apply() } @@ -899,7 +901,7 @@ class BasePrefsHelperTest { @Test fun testNullableEnumPrefDelegateSetNull() { prefsHelper.nullableEnumValue = null - verify(mockEditor).putString("nullable_enum_key", "") + verify(mockEditor).remove("nullable_enum_key") verify(mockEditor).apply() } diff --git a/app/src/test/java/com/duck/app/di/PrefsModuleTest.kt b/app/src/test/java/com/duck/app/di/PrefsModuleTest.kt index f20b854..a4a58d4 100644 --- a/app/src/test/java/com/duck/app/di/PrefsModuleTest.kt +++ b/app/src/test/java/com/duck/app/di/PrefsModuleTest.kt @@ -9,6 +9,7 @@ import com.duck.app.data.prefs.NormalPrefs import com.duck.app.data.prefs.PrefsHelper import kotlinx.coroutines.CoroutineScope import org.junit.After +import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Assert.assertSame import org.junit.Test @@ -62,14 +63,31 @@ class PrefsModuleTest : KoinTest { assertSame(get(), get()) } - /** The façade must be handed the same instances the container holds, not fresh ones. */ + /** + * The façade must be wired to the container's instances, not fresh ones. + * + * Asserting `get() === get()` would only prove `PrefsHelper` is itself + * a `single` — it says nothing about what got injected into it. This writes through the façade + * and reads from the separately-resolved sub-helper, which fails if they are different objects + * over different files. + */ @Test - fun testFacadeSharesTheContainerInstances() { + fun testFacadeIsWiredToTheContainerInstances() { start() - assertSame(get(), get()) - assertSame(get(), get()) - assertSame(get(), get()) + val facade = get() + val normalPrefs = get() + val dataStore = get() + + facade.exampleNormalValue = "written-through-facade" + assertEquals("written-through-facade", normalPrefs.exampleValue) + + normalPrefs.exampleValue = "written-through-sub-helper" + assertEquals("written-through-sub-helper", facade.exampleNormalValue) + + // The DataStore helper is the one that must not be duplicated at all. + assertSame(dataStore, get()) + assertSame(facade, get()) } /** The injected scope is what gives fire-and-forget writes somewhere to report failures. */ From 0c7996cce15476eaec13ccb892d76124f7bc2090 Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 15:26:29 +0100 Subject: [PATCH 16/17] Verify the R8 claim instead of asserting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The README promised consumers need no ProGuard rules, but nothing in the repo exercised R8 — the sample built with isMinifyEnabled = false, unit tests never run R8, and Robolectric runs unminified code. The claim was plausible and untested. Now: - The sample's release build is minified. `:app:assembleRelease` produces no missing_rules.txt, which is R8 stating nothing needed keeping. That alone is the build-time half of the claim. - app/src/androidTest/R8SurvivalTest.kt round-trips enum, Set, Double, ByteArray, Set and Date preferences, plus the migration machinery, against the minified APK on a real device. 7/7 passing, with the sample obfuscated to NormalPrefs -> wn, Theme -> sw, Feature -> hh while Theme.DARK still persisted and reloaded as the literal "DARK" — the property enum prefs actually depend on. Manual by design: `./gradlew :app:connectedAndroidTest -PminifiedTests`. Not in CI, which has no device. Without the flag the tests run against unminified debug and prove nothing, so the first test asks the runtime whether its own class was renamed and fails loudly if not, rather than inferring from the build type. The sample gained Theme/Feature enum preferences so the obfuscated code under test is production-shaped rather than test-only classes. Two sets of keep rules were needed, both harness-only and both kept out of proguard-rules.pro on purpose, since that file staying empty is the whole signal: - proguard-rules-androidtest.pro disables shrinking of the test APK. - proguard-rules-instrumentation.pro is applied to the app ONLY under -PminifiedTests. AGP does not duplicate classes already on the app's classpath into the test APK, so anything the test uses and the app does not gets stripped from both. Uses -keepclassmembers rather than -keep so classes are still renamed and the obfuscation guard stays honest. JVM suite unaffected: 321 tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 12 +- README.md | 16 ++ app/build.gradle.kts | 37 +++- app/proguard-rules-androidtest.pro | 33 ++++ app/proguard-rules-instrumentation.pro | 35 ++++ .../java/com/duck/app/R8SurvivalTest.kt | 170 ++++++++++++++++++ .../com/duck/app/data/prefs/PrefsHelper.kt | 26 +++ gradle/libs.versions.toml | 3 + 8 files changed, 330 insertions(+), 2 deletions(-) create mode 100644 app/proguard-rules-androidtest.pro create mode 100644 app/proguard-rules-instrumentation.pro create mode 100644 app/src/androidTest/java/com/duck/app/R8SurvivalTest.kt diff --git a/CLAUDE.md b/CLAUDE.md index 6b132a9..17fffeb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,6 +27,14 @@ LocalDateTime, LocalDate, LocalTime, and Enums. ./gradlew :app:koverHtmlReportDebug # HTML -> app/build/reports/kover/htmlDebug/index.html ./gradlew :app:koverVerifyDebug # fails below the coverage floor (minBound in app/build.gradle.kts) +# R8 verification. The sample's release build is minified on purpose, so this proves the +# library needs no consumer keep rules (no missing_rules.txt = nothing needed keeping). +./gradlew :app:assembleRelease + +# Behavioural R8 check — MANUAL, needs a connected device, deliberately not in CI. +# Points instrumented tests at the minified release build instead of debug. +./gradlew :app:connectedAndroidTest -PminifiedTests + # Generate documentation (Dokka 2.x V2 task; the old `dokkaHtml` V1 task no longer exists) ./gradlew :PrefsHelper:dokkaGenerateHtml @@ -78,7 +86,9 @@ Storage conventions still differ by backend, and deliberately so: This only reproduces when the delegate is used **from a subclass in another module**, which is why `BaseDataStoreHelperTest`'s `TestDataStoreHelper` (in `:app`) declares `delegate`-prefixed properties for both enum delegates. A test that calls `readEnumSetValue` directly will not catch a regression here. -- **Tests are JVM-only (Robolectric), not instrumented**: all three test classes live in `app/src/test` (`BasePrefsHelperTest`, `BaseDataStoreHelperTest`, and `BaseDataStoreHelperInjectionTest`). `BaseDataStoreHelperTest` uses a real `DataStore` but runs on the JVM via Robolectric (`@RunWith(AndroidJUnit4::class)` delegates to `RobolectricTestRunner` off-device). This is deliberate: **Kover cannot instrument on-device tests**, so DataStore coverage would read ~0% if the tests were instrumented — running them under Robolectric makes the `koverVerifyDebug` floor meaningful across both helpers. Robolectric's SDK is pinned to 36 in `app/src/test/resources/robolectric.properties` because `targetSdk = 37` has no Robolectric image yet. +- **R8 verification is manual and needs a device**: `app/src/androidTest/R8SurvivalTest.kt` is the only test that can check the README's "no consumer ProGuard rules" claim, since unit tests never run R8 and Robolectric runs unminified code. It only means anything with `-PminifiedTests`, which flips `testBuildType` to the minified release build; its first test asserts at runtime that classes really were renamed so a debug run can't pass vacuously. Harness keeps live in `proguard-rules-instrumentation.pro`, applied **only** under that flag — keep them out of `proguard-rules.pro`, or a plain `assembleRelease` stops being evidence that consumers need no rules. + +- **Tests are JVM-only (Robolectric), not instrumented**: all *unit* test classes live in `app/src/test` (`BasePrefsHelperTest`, `BaseDataStoreHelperTest`, and `BaseDataStoreHelperInjectionTest`). `BaseDataStoreHelperTest` uses a real `DataStore` but runs on the JVM via Robolectric (`@RunWith(AndroidJUnit4::class)` delegates to `RobolectricTestRunner` off-device). This is deliberate: **Kover cannot instrument on-device tests**, so DataStore coverage would read ~0% if the tests were instrumented — running them under Robolectric makes the `koverVerifyDebug` floor meaningful across both helpers. Robolectric's SDK is pinned to 36 in `app/src/test/resources/robolectric.properties` because `targetSdk = 37` has no Robolectric image yet. ## Project Structure diff --git a/README.md b/README.md index d3988ff..13a2ae9 100644 --- a/README.md +++ b/README.md @@ -386,3 +386,19 @@ Keep the call inside the `migrateIfNeeded(from < 2)` branch permanently rather t PrefsHelper is fully compatible with R8 (including full mode) and requires **no consumer ProGuard rules** (the shipped `consumer-rules.pro` is intentionally empty). Preference keys are always explicit string arguments you pass to the `*Pref(...)` delegates — they are **never** derived from Kotlin property names via reflection. R8 is free to rename, merge, and repackage your `BasePrefsHelper`/`BaseDataStoreHelper` subclasses and their properties without changing any persisted key. Enum values are stored by `Enum.name` (preserved by R8's default Android rules), so enum prefs survive obfuscation. The one thing to keep in mind lives in *consumer* code, not the library: pass real string literals as keys. Since keys are explicit strings here, the classic prefs-library footgun (key derived from a renamed identifier) doesn't apply. + +### How that claim is verified + +It isn't taken on trust. The sample app builds with `isMinifyEnabled = true`, so `./gradlew :app:assembleRelease` exercises the library through R8 — including its one reflective path, `Enum.name` matched against `enumConstants` — and produces **no `missing_rules.txt`**, which is R8's way of saying nothing needed keeping. + +Build-time success only proves it links, though, so there is also an on-device test. `app/src/androidTest/.../R8SurvivalTest.kt` runs against the minified APK and round-trips enum, `Set`, `Double`, `ByteArray`, `Set` and `Date` preferences, plus the migration machinery: + +```bash +./gradlew :app:connectedAndroidTest -PminifiedTests +``` + +**Manual, and deliberately not in CI** — it needs a connected device. Without `-PminifiedTests` the instrumented tests run against the unminified debug build and prove nothing, so the suite's first test asserts at runtime that its own classes were actually renamed and fails loudly if not. + +Last verified on a real device with the sample's classes obfuscated to `NormalPrefs -> wn`, `Theme -> sw`, `Feature -> hh` — while `Theme.DARK` still persisted and reloaded as the string `"DARK"`. That is the property enum preferences depend on. + +Two caveats worth stating. Running instrumented tests against a minified app needs harness keep rules of its own (`proguard-rules-instrumentation.pro`, applied only under `-PminifiedTests`) because R8 strips test-only entry points and any helper method the sample itself never calls. None of that is required by consumers, which is exactly why it lives in a separate conditional file rather than in `proguard-rules.pro`. And `gradle.properties` sets `android.r8.strictFullModeForKeepRules=false`; the verification above holds with that setting as configured. diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 7b6a073..bef2588 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -19,15 +19,45 @@ configure { versionName = "1.0" testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" + + // Test-APK-only rules. When instrumented tests target the minified release build, AGP + // minifies the test APK too, and Espresso's transitive Error Prone annotations reference + // javax.lang.model.*, which does not exist on Android. Kept out of proguard-rules.pro on + // purpose: the app's own release build needs no keep rules, and that is the claim under test. + testProguardFiles("proguard-rules-androidtest.pro") } buildTypes { release { - isMinifyEnabled = false + // On deliberately: the library README promises consumers need no ProGuard rules of + // their own, and this is the only place in the repo that puts R8 anywhere near the + // library's reflective paths (Enum.name / enumConstants). If this build starts needing + // keep rules, that promise is broken. + isMinifyEnabled = true proguardFiles(getDefaultProguardFile("proguard-android-optimize.txt"), "proguard-rules.pro") + // Debug-signed so the minified build can actually be installed for the manual + // instrumented run below. This is a sample app that is never published. + signingConfig = signingConfigs.getByName("debug") + + // Harness-only keeps, added ONLY for the instrumented-test run. Kept conditional so + // that a plain `assembleRelease` — the shipped configuration — still proves the + // library needs no keep rules of its own. + if (providers.gradleProperty("minifiedTests").isPresent) { + proguardFile("proguard-rules-instrumentation.pro") + } } } + // Instrumented tests normally run against debug, which is not minified and so cannot say + // anything about R8. Opt in with -PminifiedTests to point them at the release build instead: + // + // ./gradlew :app:connectedAndroidTest -PminifiedTests + // + // Deliberately not the default and deliberately not in CI — it needs a device, and CI has none. + if (providers.gradleProperty("minifiedTests").isPresent) { + testBuildType = "release" + } + compileOptions { sourceCompatibility = JavaVersion.VERSION_11 targetCompatibility = JavaVersion.VERSION_11 @@ -107,5 +137,10 @@ dependencies { testImplementation(libs.kotlinx.coroutines.test) testImplementation(libs.koin.test) + // Instrumented tests exist solely to verify the library survives R8. See testBuildType above. + androidTestImplementation(libs.androidx.test.ext.junit) + androidTestImplementation(libs.androidx.test.espresso.core) + androidTestImplementation(libs.androidx.tracing) + debugImplementation(libs.androidx.compose.ui.tooling) } diff --git a/app/proguard-rules-androidtest.pro b/app/proguard-rules-androidtest.pro new file mode 100644 index 0000000..af4c40d --- /dev/null +++ b/app/proguard-rules-androidtest.pro @@ -0,0 +1,33 @@ +# R8 rules for the androidTest APK only — NOT for the app, and NOT anything consumers need. +# +# Applied via `testProguardFiles`, so it affects only the instrumented-test APK built when +# `-PminifiedTests` points instrumented tests at the minified release build. +# +# Espresso pulls in Guava, which pulls in Error Prone annotations, which reference +# javax.lang.model.* — a compile-time-only API that does not exist on Android. R8 warns about the +# dangling reference while minifying the test APK. The app's own release build needs no such rule: +# this is test tooling, not PrefsHelper. +# +# Do not migrate these into proguard-rules.pro. Keeping them separate is what preserves the signal +# that the library itself requires no keep rules. +# Leave the test APK alone entirely. The app under test stays fully minified and obfuscated — +# that is the thing being verified — but shrinking the *test* APK only strips the JUnit runner's +# reflective entry points and proves nothing. These two lines scope R8 to the app. +-dontshrink +-dontobfuscate + +-dontwarn javax.lang.model.element.Modifier + +# The instrumentation runner, its JUnit plumbing and the test classes themselves are all reached +# reflectively by the framework, so R8 sees them as unused and strips them. Nothing here is a +# statement about PrefsHelper — it is what any project pays to run instrumented tests against a +# minified build. +-keep class androidx.test.** { *; } +-keep class androidx.tracing.** { *; } +-keep class org.junit.** { *; } +-keep class junit.** { *; } +-keep class org.hamcrest.** { *; } +-keep @org.junit.runner.RunWith public class * { *; } +-keep public class com.duck.app.R8SurvivalTest { *; } +-dontwarn androidx.test.** +-dontwarn org.junit.** diff --git a/app/proguard-rules-instrumentation.pro b/app/proguard-rules-instrumentation.pro new file mode 100644 index 0000000..d8b43f1 --- /dev/null +++ b/app/proguard-rules-instrumentation.pro @@ -0,0 +1,35 @@ +# Applied to the APP's release build ONLY when running instrumented tests (-PminifiedTests). +# Never part of a shipped release, and nothing here is required by PrefsHelper. +# +# Why it has to live on the app side rather than in testProguardFiles: AGP does not duplicate +# classes into the test APK that are already on the app's compile classpath. androidx.tracing +# arrives transitively via AndroidX, so it is excluded from the test APK — and then R8 strips it +# from the app APK because the app itself never calls it. AndroidJUnitRunner.onCreate then dies +# with NoClassDefFoundError before a single test runs. +# +# Keeping this in a separate file, applied conditionally, is what preserves the actual signal: +# `./gradlew :app:assembleRelease` — the shipped configuration — still needs no keep rules at all. +# Each of these is a symbol the *test* code needs but the app does not, so R8 removes it from the +# app APK and AGP never duplicates it into the test APK. Rather than discovering them one crash at +# a time, keep the libraries wholesale. +# +# Crucially this does NOT weaken what the test verifies: `com.duck.**` — the sample's helpers, the +# enums, and PrefsHelper itself — is still shrunk and obfuscated, which is why +# testIsActuallyRunningMinified asserts on the runtime class name rather than trusting the setup. +-keep class kotlin.** { *; } +-keep class kotlinx.** { *; } +-keep class androidx.** { *; } +-keep class org.jetbrains.** { *; } +-keep class org.koin.** { *; } +-dontwarn androidx.test.** +-dontwarn kotlin.** + +# The instrumented test calls helper methods the sample app itself never calls (clearPrefs, +# setByteArray, migrateIfNeeded…). R8 rightly removes them as unused, and the test APK — compiled +# against the unminified API — then dies with NoSuchMethodError. This is an artefact of testing an +# app that is smaller than its test surface, not a library problem. +# +# `-keepclassmembers`, not `-keep`: the CLASSES are still renamed (NormalPrefs -> lj), so +# testIsActuallyRunningMinified still detects obfuscation, and the enum constants under test are +# untouched by this rule. +-keepclassmembers class com.duck.app.data.prefs.** { *; } diff --git a/app/src/androidTest/java/com/duck/app/R8SurvivalTest.kt b/app/src/androidTest/java/com/duck/app/R8SurvivalTest.kt new file mode 100644 index 0000000..5ea59d1 --- /dev/null +++ b/app/src/androidTest/java/com/duck/app/R8SurvivalTest.kt @@ -0,0 +1,170 @@ +package com.duck.app + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import com.duck.app.data.prefs.Feature +import com.duck.app.data.prefs.NormalDataStore +import com.duck.app.data.prefs.NormalPrefs +import com.duck.app.data.prefs.Theme +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.koin.core.context.GlobalContext +import java.util.Date + +/** + * Verifies the library survives R8, which is the one claim the README makes that no JVM test can + * check. Unit tests never run R8 and Robolectric runs unminified code, so this is the only place + * the obfuscated behaviour is observable. + * + * **Manual only — not in CI, which has no device:** + * + * ``` + * ./gradlew :app:connectedAndroidTest -PminifiedTests + * ``` + * + * Without `-PminifiedTests` this runs against the debug build and proves nothing about R8; it will + * still pass, so read the build type before trusting a green result. [testIsActuallyRunningMinified] + * fails loudly if the app under test was not minified. + * + * The classes exercised here (`NormalPrefs`, `NormalDataStore`, `Theme`, `Feature`) live in the + * app's **main** source set precisely so R8 processes them. A helper declared in this test source + * set would not be representative. + */ +@RunWith(AndroidJUnit4::class) +class R8SurvivalTest { + + private lateinit var prefs: NormalPrefs + private lateinit var dataStore: NormalDataStore + + @Before + fun setUp() { + val koin = requireNotNull(GlobalContext.getOrNull()) { + "Koin not started — PrefsHelperApp should have run" + } + prefs = koin.get() + dataStore = koin.get() + runBlocking { + prefs.clearPrefs() + dataStore.clearPrefs() + } + } + + /** + * Guard against a false pass. + * + * Asks the runtime directly whether the class was renamed, rather than inferring it from the + * build type — if R8 did not actually obfuscate, every other test here is vacuous and would + * still go green. + */ + @Test + fun testIsActuallyRunningMinified() { + val runtimeName = NormalPrefs::class.java.name + assertTrue( + "Classes are not obfuscated (NormalPrefs is still '$runtimeName'), so this run proves " + + "nothing about R8. Re-run with:\n" + + " ./gradlew :app:connectedAndroidTest -PminifiedTests", + runtimeName != "com.duck.app.data.prefs.NormalPrefs", + ) + } + + /** + * The core claim. Enum preferences are stored by [Enum.name] and resolved through + * `enumConstants`; if R8 renamed either the constants or the class in a way that broke that, + * this round-trip returns the default instead of the stored value. + */ + @Test + fun testEnumPrefSurvivesObfuscation() { + prefs.theme = Theme.DARK + assertEquals(Theme.DARK, prefs.theme) + + prefs.theme = Theme.LIGHT + assertEquals(Theme.LIGHT, prefs.theme) + } + + /** The name written must be the source-level constant name, not an obfuscated one. */ + @Test + fun testEnumIsStoredUnderItsSourceName() { + prefs.theme = Theme.DARK + assertEquals("DARK", prefs.getString(NormalPrefs.KEY_THEME)) + } + + @Test + fun testEnumSetPrefSurvivesObfuscation() { + prefs.enabledFeatures = setOf(Feature.OFFLINE_MODE, Feature.ANALYTICS) + assertEquals(setOf(Feature.OFFLINE_MODE, Feature.ANALYTICS), prefs.enabledFeatures) + + assertEquals( + setOf("OFFLINE_MODE", "ANALYTICS"), + prefs.getStringSet(NormalPrefs.KEY_FEATURES), + ) + } + + /** + * `enumSetPref` on DataStore goes through the inline / `@PublishedApi internal` split that + * exists to dodge `IllegalAccessError`. Minification is another way that could break, so it is + * worth exercising here and not only on the JVM. + */ + @Test + fun testDataStoreEnumPathsSurviveObfuscation() { + dataStore.theme = Theme.DARK + dataStore.enabledFeatures = setOf(Feature.BETA_SEARCH) + + awaitValue(Theme.DARK) { dataStore.theme } + awaitValue(setOf(Feature.BETA_SEARCH)) { dataStore.enabledFeatures } + } + + /** Keys are string literals, so obfuscation must not disturb any of the encodings either. */ + @Test + fun testEncodingsSurviveObfuscation() { + prefs.setDouble("d", 3.14159265358979) + assertEquals(3.14159265358979, prefs.getDouble("d"), 0.0) + + prefs.setByteArray("b", byteArrayOf(0, -1, 127, -128)) + assertTrue(byteArrayOf(0, -1, 127, -128).contentEquals(prefs.getByteArray("b"))) + + prefs.setStringSet("s", setOf("alpha", "beta")) + assertEquals(setOf("alpha", "beta"), prefs.getStringSet("s")) + + val date = Date(1_700_000_000_000L) + prefs.setDate("dt", date) + assertEquals(date, prefs.getDate("dt")) + + prefs.setDate("dt", null) + assertNull(prefs.getDate("dt")) + } + + /** The version stamp and its key must also survive; the key is a literal, not a class name. */ + @Test + fun testMigrationMachinerySurvivesObfuscation() { + prefs.setString("something", "x") + + var ranFrom = -1 + val migrated = prefs.migrateIfNeeded(currentVersion = 99) { from -> ranFrom = from } + + assertTrue(migrated) + assertTrue(ranFrom >= 1) + // Second call must be a no-op — proves the stamp was written and read back. + assertTrue(!prefs.migrateIfNeeded(currentVersion = 99) { }) + } + + private fun awaitValue(expected: T, timeoutMs: Long = 5_000, read: () -> T) { + val deadline = System.currentTimeMillis() + timeoutMs + var actual = read() + while (actual != expected && System.currentTimeMillis() < deadline) { + Thread.sleep(25) + actual = read() + } + assertEquals(expected, actual) + } + + private companion object { + init { + InstrumentationRegistry.getInstrumentation() + } + } +} diff --git a/app/src/main/java/com/duck/app/data/prefs/PrefsHelper.kt b/app/src/main/java/com/duck/app/data/prefs/PrefsHelper.kt index e53be32..f153590 100644 --- a/app/src/main/java/com/duck/app/data/prefs/PrefsHelper.kt +++ b/app/src/main/java/com/duck/app/data/prefs/PrefsHelper.kt @@ -26,9 +26,15 @@ class PrefsHelper( var lastSeen by normalPrefs::lastSeen + var theme by normalPrefs::theme + var enabledFeatures by normalPrefs::enabledFeatures + var exampleDataStoreValue by normalDataStore::exampleValue val exampleDataStoreValueFlow = normalDataStore.exampleValueFlow + var dataStoreTheme by normalDataStore::theme + var dataStoreFeatures by normalDataStore::enabledFeatures + suspend fun clearPrefs() { normalPrefs.clearPrefs() normalDataStore.clearPrefs() @@ -55,15 +61,29 @@ class NormalPrefs(context: Context) : BasePrefsHelper() { var exampleValue by stringPref(KEY_EXAMPLE, defaultValue = "") var lastSeen by datePref(KEY_LAST_SEEN) + /** + * Enum-backed preferences are the library's only reflective path — values are matched by + * [Enum.name] against `enumConstants`. Kept in the sample so the R8 instrumented test has + * production-shaped, minified code to exercise rather than test-only classes. + */ + var theme by enumPref(KEY_THEME, Theme.SYSTEM) + var enabledFeatures by enumSetPref(KEY_FEATURES) + companion object { const val KEY_EXAMPLE = "example_key" const val KEY_LAST_SEEN = "last_seen" + const val KEY_THEME = "theme" + const val KEY_FEATURES = "enabled_features" /** Bump alongside a new branch in the [migrateIfNeeded] block above. */ private const val PREFS_VERSION = 2 } } +enum class Theme { LIGHT, DARK, SYSTEM } + +enum class Feature { OFFLINE_MODE, BETA_SEARCH, ANALYTICS } + class DevicePrefs(context: Context) : BasePrefsHelper() { override val sharedPreferences: SharedPreferences = context.getSharedPreferences("device_prefs", Context.MODE_PRIVATE) @@ -96,7 +116,13 @@ class NormalDataStore( var exampleValue by stringPref(KEY_EXAMPLE, defaultValue = "") val exampleValueFlow = stringPrefFlow(KEY_EXAMPLE) + /** Same reflective path as [NormalPrefs.theme], on the DataStore side. */ + var theme by enumPref(KEY_THEME, Theme.SYSTEM) + var enabledFeatures by enumSetPref(KEY_FEATURES) + companion object { const val KEY_EXAMPLE = "example_key" + const val KEY_THEME = "theme" + const val KEY_FEATURES = "enabled_features" } } diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index df58fef..a5bd9e2 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -39,6 +39,9 @@ androidx-dataStore = { group = "androidx.datastore", name = "datastore-preferenc kotlinx-coroutines-test = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-test", version.ref = "coroutines" } koin-android = { module = "io.insert-koin:koin-android", version.ref = "koin" } koin-test = { module = "io.insert-koin:koin-test", version.ref = "koin" } +# Needed only by the minified instrumented-test APK: AndroidJUnitRunner touches androidx.tracing.Trace +# at startup, and it is not pulled in transitively here. +androidx-tracing = { module = "androidx.tracing:tracing", version = "1.2.0" } android-documentation-plugin = { module = "org.jetbrains.dokka:android-documentation-plugin", version.ref = "dokka" } From 90ef67a6d7f618c553a8b5ff1498f9623b3bf9fc Mon Sep 17 00:00:00 2001 From: Bradley Duck Date: Fri, 31 Jul 2026 15:39:09 +0100 Subject: [PATCH 17/17] Polish from Grok's fourth review, and settle the strict-full-mode question MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four residual items from the re-review, none of which blocked. - R8SurvivalTest KDoc contradicted itself: said a debug run "will still pass" and also that the minify guard fails loudly. The guard does fail, so a debug run reports failure. Reworded. - Nullable enumPref KDoc still said assigning null "clears the stored value"; it removes the key as of 2.0. Corrected, with the pre-2.0 behaviour noted since that is what makes contains() differ. - Dropped a pointless companion-object init in R8SurvivalTest that called InstrumentationRegistry for no effect, and its now-unused import. The open question was whether android.r8.strictFullModeForKeepRules=false was propping up the clean R8 result. It is not: with it flipped to true, :app:assembleRelease still produces no missing_rules.txt and the on-device suite still passes 7/7 against the minified build. gradle.properties keeps its original value — changing a project-wide R8 setting is not this change's business — but the README now states the claim holds either way rather than hedging with "as configured". 321 JVM tests, 0 failures. Release build clean. Co-Authored-By: Claude Opus 5 (1M context) --- .../java/com/duck/prefshelper/BasePrefsHelper.kt | 4 +++- README.md | 4 +++- .../java/com/duck/app/R8SurvivalTest.kt | 14 ++++---------- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt index 38e9856..7a62bbe 100644 --- a/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt +++ b/PrefsHelper/src/main/java/com/duck/prefshelper/BasePrefsHelper.kt @@ -870,7 +870,9 @@ abstract class BasePrefsHelper( /** * Create a property delegate for a nullable [Enum] preference. * - * Returns null when the key is absent or cannot be parsed. Assigning null clears the stored value. + * Returns null when the key is absent or cannot be parsed. Assigning null removes the key, as of + * 2.0 — earlier versions left an empty string behind, which read back as null but kept + * [contains] returning true. */ protected inline fun > enumPref(key: String): ReadWriteProperty { val constants = T::class.java.enumConstants diff --git a/README.md b/README.md index 13a2ae9..7f4cd18 100644 --- a/README.md +++ b/README.md @@ -401,4 +401,6 @@ Build-time success only proves it links, though, so there is also an on-device t Last verified on a real device with the sample's classes obfuscated to `NormalPrefs -> wn`, `Theme -> sw`, `Feature -> hh` — while `Theme.DARK` still persisted and reloaded as the string `"DARK"`. That is the property enum preferences depend on. -Two caveats worth stating. Running instrumented tests against a minified app needs harness keep rules of its own (`proguard-rules-instrumentation.pro`, applied only under `-PminifiedTests`) because R8 strips test-only entry points and any helper method the sample itself never calls. None of that is required by consumers, which is exactly why it lives in a separate conditional file rather than in `proguard-rules.pro`. And `gradle.properties` sets `android.r8.strictFullModeForKeepRules=false`; the verification above holds with that setting as configured. +One caveat worth stating: running instrumented tests against a minified app needs harness keep rules of its own (`proguard-rules-instrumentation.pro`, applied only under `-PminifiedTests`) because R8 strips test-only entry points and any helper method the sample itself never calls. None of that is required by consumers, which is exactly why it lives in a separate conditional file rather than in `proguard-rules.pro`. + +`gradle.properties` sets `android.r8.strictFullModeForKeepRules=false`, so the obvious question is whether the clean result depends on it. It doesn't — the release build and the full on-device suite were both re-run with it flipped to `true`, producing no missing rules and 7/7 passing. The repo keeps the original setting; the claim holds either way. diff --git a/app/src/androidTest/java/com/duck/app/R8SurvivalTest.kt b/app/src/androidTest/java/com/duck/app/R8SurvivalTest.kt index 5ea59d1..1c63ae8 100644 --- a/app/src/androidTest/java/com/duck/app/R8SurvivalTest.kt +++ b/app/src/androidTest/java/com/duck/app/R8SurvivalTest.kt @@ -1,7 +1,6 @@ package com.duck.app import androidx.test.ext.junit.runners.AndroidJUnit4 -import androidx.test.platform.app.InstrumentationRegistry import com.duck.app.data.prefs.Feature import com.duck.app.data.prefs.NormalDataStore import com.duck.app.data.prefs.NormalPrefs @@ -27,9 +26,10 @@ import java.util.Date * ./gradlew :app:connectedAndroidTest -PminifiedTests * ``` * - * Without `-PminifiedTests` this runs against the debug build and proves nothing about R8; it will - * still pass, so read the build type before trusting a green result. [testIsActuallyRunningMinified] - * fails loudly if the app under test was not minified. + * Without `-PminifiedTests` this runs against the debug build, where every round-trip below would + * pass while proving nothing about R8. [testIsActuallyRunningMinified] exists to stop that being a + * silent false green: it asks the runtime whether its own class was renamed and fails the run if + * not, so a debug invocation reports failure rather than success. * * The classes exercised here (`NormalPrefs`, `NormalDataStore`, `Theme`, `Feature`) live in the * app's **main** source set precisely so R8 processes them. A helper declared in this test source @@ -161,10 +161,4 @@ class R8SurvivalTest { } assertEquals(expected, actual) } - - private companion object { - init { - InstrumentationRegistry.getInstrumentation() - } - } }