diff --git a/docs/superpowers/architecture.md b/docs/superpowers/architecture.md new file mode 100644 index 0000000..f93dfed --- /dev/null +++ b/docs/superpowers/architecture.md @@ -0,0 +1,267 @@ +# Architecture Walkthrough + +A reverse-engineered map of the FeatBit Android SDK, intended as an on-ramp for +new contributors. Read [`usage.md`](../usage.md) first if you only want to *use* +the SDK; this document is for people changing it. + +The SDK is split into two Gradle modules: + +| Module | Purpose | +|--------------------------|---------------------------------------------------------------------------| +| `featbit-client` | Pure Kotlin/JVM core — store, evaluator, sync, insights, public API. | +| `featbit-client-android` | Android-only adapter that wires `ProcessLifecycleOwner` + `ConnectivityManager` into the core's `setForeground` / `setNetworkAvailable` hooks. | + +## Component map + +```text +┌─────────────────────── Public API (consumer surface) ───────────────────────┐ +│ FBClient (interface) ── FBOptions / FBOptions.Builder ── FBUser │ +│ FlagTracker ── EvalDetail ── FBLogger │ +└─────────────────────────────────────────────────────────────────────────────┘ + │ + ▼ +┌──────────────────────── Orchestration (FBClientImpl) ───────────────────────┐ +│ Wires: store + evaluator + tracker + insights + synchronizer + lifecycle. │ +│ Holds AtomicReference + AtomicReference. │ +│ identify() swaps the synchronizer under identifyMutex. │ +└─────────────────────────────────────────────────────────────────────────────┘ + │ │ │ │ + ▼ ▼ ▼ ▼ + Evaluation path Sync path Insight path Lifecycle path + Evaluator ──▶ DataSynchronizer InsightDispatcher LifecycleController + │ ├ NullSync │ (bounded (Mutex + grace + ▼ ├ PollingSync ▼ Channel, debounce) + MemoryStore └ StreamingSync TrackInsight │ + │ │ ├ Noop │ + │ ▼ └ Http │ + FlagTrackerImpl FbApiClient (OkHttp + JSON) │ + (SharedFlow, ▲ │ + keyed + global) │ │ + ▲ HTTP / WS │ + │ (FeatBit BE) │ + └──── store.addChangeListener │ + │ + ◀────── ProcessLifecycle / ConnectivityManager + (via FBLifecycleConnector in + :featbit-client-android) +``` + +The same picture, in Mermaid (renders nicely on GitHub / IntelliJ / MkDocs): + +```mermaid +flowchart TD + subgraph "Public API" + FBClient[FBClient] + FBOptions[FBOptions] + FBUser[FBUser] + FlagTracker[FlagTracker] + end + + subgraph "Orchestration" + Impl[FBClientImpl] + Lifecycle[LifecycleController] + end + + subgraph "Sync" + DS[DataSynchronizer] + Poll[PollingDataSynchronizer] + Stream[StreamingDataSynchronizer] + Null[NullDataSynchronizer] + end + + subgraph "State" + Store[MemoryStore] + Tracker[FlagTrackerImpl] + end + + subgraph "Evaluation" + Evaluator[Evaluator] + Conv[ValueConverters] + end + + subgraph "Insights" + Dispatcher[InsightDispatcher\nbounded Channel + batching] + Insight[TrackInsight] + end + + subgraph "HTTP" + Api[FbApiClient] + Endpoints[FBEndpoints] + end + + FBClient --> Impl + FBOptions --> Impl + FBUser --> Impl + Impl --> Evaluator + Impl --> DS + Impl --> Dispatcher + Impl --> Lifecycle + Impl --> Tracker + DS --> Poll + DS --> Stream + DS --> Null + Poll --> Api + Stream --> Api + Dispatcher --> Insight + Insight --> Api + Api --> Endpoints + Evaluator --> Store + Tracker --> Store + Lifecycle --> DS + Store --> Tracker + FlagTracker --> Tracker + Conv --> Evaluator +``` + +## Data flow + +### 1. Boot (`FBClient.start`) + +1. `FBClientImpl.` constructs the `DefaultMemoryStore` (optionally pre-seeded + from `options.bootstrap`), the `Evaluator`, the `FlagTrackerImpl` (which + registers a single `FlagChangeListener` on the store), the `InsightDispatcher` + (with a `Channel(capacity=256, DROP_OLDEST)`), and the initial + `DataSynchronizer` for the initial user. +2. `start(timeout)` suspends until the synchronizer's first sync completes (or + the timeout elapses). +3. **Polling**: spawns a `pollingLoop()` coroutine that POSTs `latest-all`, + upserts every flag into the store, and `delay()`s for `pollingInterval`. +4. **Streaming**: opens an OkHttp WebSocket, sends a `data-sync` message, and + processes server messages (`full` snapshot + `patch` updates) by upserting + into the store. A 20-second app-level ping keeps the connection alive; + exponential backoff (with jitter) handles reconnects. +5. The store fires `FlagValueChangedEvent`s only when a flag's `variation` + actually changes — the diff is computed under a single write monitor before + the new value is stored. + +### 2. Evaluation (`boolVariation`, etc.) + +1. Fast path: if the synchronizer isn't initialized and there's no bootstrap, + return `EvalDetail("client not ready", default)`. +2. `Evaluator.evaluate(key)` does a lock-free `ConcurrentHashMap.get` on the + store and returns a sealed `EvalResult` (`Found(flag)` or `NotFound`). +3. On `Found`, the call **enqueues** an `Insight.forEvaluation(...)` on the + `InsightDispatcher` (non-suspending; drops the oldest pending event on + overflow). The consumer coroutine coalesces events into batches of up to 50 + or one second, whichever is shorter, and POSTs the batch to + `api/public/insight/track`. +4. `ValueConverters` coerces the raw `variation` string into the requested + type; on coercion failure the API returns `EvalDetail("type mismatch", default)`. + +### 3. Identify (`FBClient.identify(user)`) + +`identify()` swaps the active synchronizer atomically: + +1. Acquire `identifyMutex`. +2. `close()` the previous synchronizer (cancels its scope; cancels in-flight + polls / closes the WebSocket; completes its `startTask` with `false`). +3. Build a fresh synchronizer for the new user, install it under + `AtomicReference`, and update the user under + `AtomicReference`. +4. `await` the new synchronizer's first sync, then enqueue + `Insight.forIdentify(user)`. + +The mutex + ordering ensures that a late upsert from the previous +synchronizer cannot reach the store after the new one has started — the +previous synchronizer's coroutine scope has already been cancelled. + +### 4. Lifecycle (background / network) + +`LifecycleController` keeps a small state machine guarded by a `Mutex`: + +* `foreground && online` → synchronizer is *active*; cancel any pending pause. +* otherwise → schedule `dataSynchronizer.pause()` after `backgroundGracePeriod` + (default 20s) to absorb brief app-switches and network blips. + +`FBLifecycleConnector` in `:featbit-client-android` wires the controller to +`ProcessLifecycleOwner` and `ConnectivityManager.NetworkCallback`. + +## Concurrency model (one paragraph) + +The hot read path is lock-free: `MemoryStore.get` is a `ConcurrentHashMap.get` +and the current user / synchronizer are held in `AtomicReference`s. Writes go +through a single monitor in `DefaultMemoryStore.upsert` so the "diff against +old value → store" sequence is atomic; listeners are notified *outside* the +lock to avoid reentrancy. `identify()` is the only public mutator that needs +mutual exclusion and uses `identifyMutex`. All background work runs on a +`SupervisorJob() + Dispatchers.IO` scope owned by `FBClientImpl` and torn down +in `close()`. The insight pipeline is a single `Channel`-backed consumer +coroutine — no per-evaluation `launch`. + +## Module / package layout + +Layered after the 2026-06-25 clean-architecture refactor. Public API FQNs are unchanged — types that were public still live at their original packages. + +``` +featbit-client/ +└── src/main/kotlin/co/featbit/client/ + ├── FBClient.kt public API surface + ├── FBClientImpl.kt orchestration (the wiring hub) + ├── FBLogger.kt / DefaultLogger.kt + │ + ├── changetracker/ + │ └── FlagTracker.kt public subscription API + │ + ├── evaluation/ + │ └── EvalDetail.kt public result with reason + │ + ├── model/ + │ ├── FBUser.kt public builder + │ └── FeatureFlag.kt public flag value type + │ + ├── options/ + │ ├── DataSyncMode.kt + │ └── FBOptions.kt + │ + ├── store/ + │ └── FlagValueChangedEvent.kt FlagValueChangedEvent + FlagChangeListener (public, via FlagTracker) + │ + ├── domain/ pure Kotlin, no framework deps + │ ├── Evaluator.kt + │ ├── EvalResult.kt internal sealed result (Found | NotFound) + │ ├── ValueConverters.kt string → bool/int/float/double/string + │ └── MemoryStore.kt port consumed by Evaluator + │ + ├── app/ application services + ports adapters + │ ├── LifecycleController.kt foreground/online state machine + │ ├── FlagTrackerImpl.kt fans events to subscribers via SharedFlow + │ └── DefaultMemoryStore.kt adapter for domain.MemoryStore + │ + ├── data/ adapters that talk to the outside world + │ ├── http/ + │ │ ├── FbApiClient.kt OkHttp base (auth, JSON, POST) + │ │ ├── FBEndpoints.kt one source of truth for URLs + │ │ ├── HttpConstants.kt + │ │ ├── ConnectionToken.kt streaming auth encoder + │ │ └── GetUserFlags.kt polling endpoint client + │ ├── sync/ + │ │ ├── DataSynchronizer.kt interface (start/pause/resume/close) + │ │ ├── NullDataSynchronizer.kt offline / no-op + │ │ ├── PollingDataSynchronizer.kt + │ │ └── StreamingDataSynchronizer.kt + │ └── insights/ + │ ├── TrackInsight.kt insight endpoint client + │ └── InsightDispatcher.kt bounded queue + batch flush + │ + └── wire/ kotlinx-serialization DTOs + ├── EndUser.kt wire format for FBUser (+ CustomizedProperty) + └── Insight.kt analytics payloads (+ VariationInsight, VariationData) +``` + +**Dependency rule:** `domain → (nothing)`, `app → domain`, `data.* → domain + wire`, `wire → model` (factories). Public API (root + `changetracker/`, `evaluation/`, `model/`, `options/`, `store/`) is consumed by everyone. + +**Known carve-outs (intentional rule relaxations):** +- `model.FeatureFlag` is `@Serializable` even though it lives in the public API surface. Splitting it into pure-domain core + wire DTO would force a public-API break (`FBClient.allFlags()` return type). Accepted; revisit in a v2 major. +- `app.LifecycleController` imports `data.sync.DataSynchronizer` (the interface). LifecycleController orchestrates the synchronizer lifecycle, so it must reference the contract. The strict reading of "app → data.*" is violated here; the practical justification is that `DataSynchronizer` is a port the app *consumes* rather than an adapter the app *depends on by category*. A future cleanup could promote the interface to `app/` and leave the concrete impls in `data/sync/` — out of scope for this refactor. +- `domain.MemoryStore` + `app.DefaultMemoryStore` are `public` rather than `internal`. They were already public on `main` before this refactor; narrowing their visibility now would break SDK consumers that reference them. Visibility is preserved on principle (no API changes in this refactor). + +## Pointers for common changes + +| Change | Start here | +|----------------------------------------------|---------------------------------------------------------| +| Add an HTTP endpoint | `data/http/FBEndpoints.kt` + `data/http/HttpConstants.kt` | +| Add a new sync strategy | implement `data/sync/DataSynchronizer`, register in `FBClientImpl.newDataSynchronizer` | +| Add a new variation type | `domain/ValueConverters.kt` + matching method on `FBClient` | +| Tune insight batching / backpressure | `data/insights/InsightDispatcher.kt` constructor defaults | +| Tune lifecycle debounce | `FBOptions.Builder.backgroundGracePeriod` | +| Change wire format for evaluation payload | `wire/EndUser.kt` + `model/FBUser.kt` | diff --git a/docs/superpowers/plans/2026-06-25-clean-architecture-plan.md b/docs/superpowers/plans/2026-06-25-clean-architecture-plan.md new file mode 100644 index 0000000..2fbd8ad --- /dev/null +++ b/docs/superpowers/plans/2026-06-25-clean-architecture-plan.md @@ -0,0 +1,702 @@ +# Clean-Architecture Refactor — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Reorganise FeatBit SDK source files into a layered architecture (`domain/`, `app/`, `data/sync/`, `data/http/`, `data/insights/`, `wire/`) while keeping every public API FQN unchanged and every test passing. + +**Architecture:** Layered. `domain` is pure Kotlin (no Android, OkHttp, or kotlinx-serialization). `app` holds application services. `data.*` packages each hold one concrete adapter family. `wire` holds kotlinx-serialization DTOs. Public types stay at their existing root packages so `import co.featbit.client.FBClient` and similar continue to compile for external consumers. + +**Tech Stack:** Kotlin 1.9, Gradle (Android), OkHttp 4.12, kotlinx-serialization 1.6, JUnit 4, MockWebServer. + +**Spec:** `docs/superpowers/specs/2026-06-25-clean-architecture-design.md` + +--- + +## File Structure (target) + +``` +featbit-client/src/main/kotlin/co/featbit/client/ +├── FBClient.kt (public — unchanged) +├── FBClientImpl.kt (public — unchanged location; imports updated) +├── FBLogger.kt (public — unchanged) +├── FeatureFlag.kt (public — new location: was model/FeatureFlag.kt → root) +│ * NOTE: stays at co.featbit.client.model.FeatureFlag to preserve FQN — see Task 6 note. +│ +├── changetracker/FlagTracker.kt (public — unchanged) +│ +├── options/ +│ ├── DataSyncMode.kt (public — unchanged) +│ └── FBOptions.kt (public — unchanged) +│ +├── model/ +│ ├── FBUser.kt (public — unchanged) +│ └── FeatureFlag.kt (public — unchanged) +│ +├── evaluation/ +│ └── EvalDetail.kt (public — unchanged) +│ +├── store/ +│ └── FlagValueChangedEvent.kt (public — STAYS; holds FlagValueChangedEvent + FlagChangeListener referenced by public FlagTracker API) +│ +├── domain/ ← NEW +│ ├── Evaluator.kt (was: evaluation/Evaluator.kt) +│ ├── EvalResult.kt (was: evaluation/EvalResult.kt) +│ ├── ValueConverters.kt (was: evaluation/ValueConverters.kt) +│ └── MemoryStore.kt (was: store/MemoryStore.kt — port consumed by Evaluator) +│ +├── app/ ← NEW +│ ├── LifecycleController.kt (was: LifecycleController.kt) +│ ├── FlagTrackerImpl.kt (was: changetracker/FlagTrackerImpl.kt) +│ └── DefaultMemoryStore.kt (was: store/DefaultMemoryStore.kt — adapter for the domain port) +│ +├── data/ +│ ├── sync/ ← NEW +│ │ ├── DataSynchronizer.kt (was: datasynchronizer/DataSynchronizer.kt) +│ │ ├── NullDataSynchronizer.kt (was: datasynchronizer/NullDataSynchronizer.kt) +│ │ ├── PollingDataSynchronizer.kt(was: datasynchronizer/PollingDataSynchronizer.kt) +│ │ └── StreamingDataSynchronizer.kt (was: datasynchronizer/StreamingDataSynchronizer.kt) +│ │ +│ ├── http/ ← NEW +│ │ ├── FbApiClient.kt (was: internal/FbApiClient.kt) +│ │ ├── FBEndpoints.kt (was: internal/FBEndpoints.kt) +│ │ ├── HttpConstants.kt (was: internal/HttpConstants.kt) +│ │ ├── GetUserFlags.kt (was: internal/GetUserFlags.kt) +│ │ └── ConnectionToken.kt (was: internal/ConnectionToken.kt) +│ │ +│ └── insights/ ← NEW +│ ├── TrackInsight.kt (was: internal/TrackInsight.kt) +│ └── InsightDispatcher.kt (was: internal/InsightDispatcher.kt) +│ +└── wire/ ← NEW + ├── EndUser.kt (was: model/EndUser.kt) + └── Insight.kt (was: model/Insight.kt — Insight + VariationInsight + VariationData) +``` + +**Important corrections vs. brainstorm:** +- `FeatureFlag.kt` STAYS at `co.featbit.client.model.FeatureFlag` (full public FQN preserved). It's used by `FBClient.allFlags(): Map` and external SDK consumers. +- `EvalDetail.kt` STAYS at `co.featbit.client.evaluation.EvalDetail` for the same reason. +- `FBUser.kt` STAYS at `co.featbit.client.model.FBUser` for the same reason. +- `FlagTracker.kt` STAYS at `co.featbit.client.changetracker.FlagTracker` for the same reason. + +Tests mirror source paths and move alongside their classes. + +--- + +## Verification gates (run at end of every task) + +After EACH commit: +1. `./gradlew :featbit-client:compileDebugKotlin` — green +2. `./gradlew :featbit-client:testDebugUnitTest` — 98 pass / 0 fail / 2 e2e-skipped +3. `./gradlew :featbit-client:assembleDebug :featbit-client-android:assembleDebug :example-app:assembleDebug` — green +4. Audit on diff (cavecrew-reviewer or superpowers:code-reviewer) — zero blocking findings + +Re-run E2E with `FEATBIT_E2E=1` only at the END (Task 7) — saves ~5 min per task otherwise. + +--- + +## Task 1: Move `evaluation/{Evaluator,EvalResult,ValueConverters}` → `domain/` + +**Files:** +- Move: `featbit-client/src/main/kotlin/co/featbit/client/evaluation/Evaluator.kt` → `featbit-client/src/main/kotlin/co/featbit/client/domain/Evaluator.kt` +- Move: `featbit-client/src/main/kotlin/co/featbit/client/evaluation/EvalResult.kt` → `featbit-client/src/main/kotlin/co/featbit/client/domain/EvalResult.kt` +- Move: `featbit-client/src/main/kotlin/co/featbit/client/evaluation/ValueConverters.kt` → `featbit-client/src/main/kotlin/co/featbit/client/domain/ValueConverters.kt` +- Move: `featbit-client/src/test/kotlin/co/featbit/client/evaluation/EvalResultTest.kt` → `featbit-client/src/test/kotlin/co/featbit/client/domain/EvalResultTest.kt` +- Move: `featbit-client/src/test/kotlin/co/featbit/client/evaluation/ValueConvertersTest.kt` → `featbit-client/src/test/kotlin/co/featbit/client/domain/ValueConvertersTest.kt` +- Modify: `featbit-client/src/main/kotlin/co/featbit/client/FBClientImpl.kt` (update imports) + +NOTE: `EvalDetail.kt` STAYS in `evaluation/` — it is public API. + +- [ ] **Step 1: Move source files with `git mv`** + +```bash +cd /Users/deep.shah_fluentinhe/Documents/code/featbit-android-sdk/featbit-android-sdk +mkdir -p featbit-client/src/main/kotlin/co/featbit/client/domain +mkdir -p featbit-client/src/test/kotlin/co/featbit/client/domain +git mv featbit-client/src/main/kotlin/co/featbit/client/evaluation/Evaluator.kt \ + featbit-client/src/main/kotlin/co/featbit/client/domain/Evaluator.kt +git mv featbit-client/src/main/kotlin/co/featbit/client/evaluation/EvalResult.kt \ + featbit-client/src/main/kotlin/co/featbit/client/domain/EvalResult.kt +git mv featbit-client/src/main/kotlin/co/featbit/client/evaluation/ValueConverters.kt \ + featbit-client/src/main/kotlin/co/featbit/client/domain/ValueConverters.kt +git mv featbit-client/src/test/kotlin/co/featbit/client/evaluation/EvalResultTest.kt \ + featbit-client/src/test/kotlin/co/featbit/client/domain/EvalResultTest.kt +git mv featbit-client/src/test/kotlin/co/featbit/client/evaluation/ValueConvertersTest.kt \ + featbit-client/src/test/kotlin/co/featbit/client/domain/ValueConvertersTest.kt +``` + +- [ ] **Step 2: Update package declarations in moved files** + +In each of the 5 moved files, change the `package co.featbit.client.evaluation` line to `package co.featbit.client.domain`. Use Edit tool, NOT sed (preserves line endings). + +- [ ] **Step 3: Update imports in FBClientImpl.kt** + +In `featbit-client/src/main/kotlin/co/featbit/client/FBClientImpl.kt`, change: +``` +import co.featbit.client.evaluation.EvalResult +import co.featbit.client.evaluation.Evaluator +import co.featbit.client.evaluation.ValueConverter +import co.featbit.client.evaluation.ValueConverters +``` +to: +``` +import co.featbit.client.domain.EvalResult +import co.featbit.client.domain.Evaluator +import co.featbit.client.domain.ValueConverter +import co.featbit.client.domain.ValueConverters +``` + +Verify: `grep "co.featbit.client.evaluation" featbit-client/src/main/kotlin/co/featbit/client/FBClientImpl.kt` should now only match `EvalDetail` (which stays in evaluation/). + +- [ ] **Step 4: Verify no stale references remain** + +```bash +grep -rn "co.featbit.client.evaluation.Evaluator\|co.featbit.client.evaluation.EvalResult\|co.featbit.client.evaluation.ValueConverter" featbit-client/src/ 2>/dev/null +``` +Expected: zero output. + +- [ ] **Step 5: Run gates** + +```bash +./gradlew :featbit-client:compileDebugKotlin 2>&1 | tail -5 +./gradlew :featbit-client:testDebugUnitTest 2>&1 | tail -3 +./gradlew :featbit-client:assembleDebug :featbit-client-android:assembleDebug :example-app:assembleDebug 2>&1 | tail -3 +``` +Expected: BUILD SUCCESSFUL on all three. Test count: 98 pass / 0 fail / 2 e2e-skipped. + +- [ ] **Step 6: Audit on diff via cavecrew-reviewer (compressed)** + +Dispatch: `caveman:cavecrew-reviewer` on `git diff HEAD` for this branch. Expected: zero blocking findings (pure file moves + package decl updates + import updates). + +- [ ] **Step 7: Commit** + +```bash +git add -A +git commit -m "refactor: move evaluator + value converters into domain package + +Pure Kotlin types (no Android / OkHttp / kotlinx-serialization deps) +relocated into co.featbit.client.domain.* to make the dependency rule +explicit. Public EvalDetail stays in evaluation/ (consumer-visible FQN). + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +## Task 2: Split store, move LifecycleController + FlagTrackerImpl into `app/`, MemoryStore port into `domain/` + +**Revised per Task 1 audit** (ports/adapters pattern): +- `MemoryStore.kt` (interface) → `domain/` ← port; `Evaluator` already imports it +- `DefaultMemoryStore.kt` (impl) → `app/` ← adapter +- `FlagValueChangedEvent.kt` (contains `FlagValueChangedEvent` + `FlagChangeListener`) → **STAYS** in `store/` (public API via `FlagTracker.subscribe(FlagChangeListener)` — moving breaks consumers) +- `LifecycleController.kt` → `app/` +- `changetracker/FlagTrackerImpl.kt` → `app/` + +**Files:** +- Move: `featbit-client/src/main/kotlin/co/featbit/client/LifecycleController.kt` → `featbit-client/src/main/kotlin/co/featbit/client/app/LifecycleController.kt` +- Move: `featbit-client/src/main/kotlin/co/featbit/client/changetracker/FlagTrackerImpl.kt` → `featbit-client/src/main/kotlin/co/featbit/client/app/FlagTrackerImpl.kt` +- Move: `featbit-client/src/main/kotlin/co/featbit/client/store/MemoryStore.kt` → `featbit-client/src/main/kotlin/co/featbit/client/domain/MemoryStore.kt` *(port)* +- Move: `featbit-client/src/main/kotlin/co/featbit/client/store/DefaultMemoryStore.kt` → `featbit-client/src/main/kotlin/co/featbit/client/app/DefaultMemoryStore.kt` *(adapter)* +- STAYS: `featbit-client/src/main/kotlin/co/featbit/client/store/FlagValueChangedEvent.kt` (no move — public API) +- Move tests: `LifecycleControllerTest.kt`, `FlagTrackerImplTest.kt`, `DefaultMemoryStoreTest.kt` into `app/` + +- [ ] **Step 1: Verify `FlagValueChangedEvent + FlagChangeListener` are public API** + +```bash +grep -n "FlagValueChangedEvent\|FlagChangeListener" featbit-client/src/main/kotlin/co/featbit/client/changetracker/FlagTracker.kt +``` +Expected: hits referencing both as parameter / return types of public `FlagTracker.*`. Confirms `co.featbit.client.store.FlagValueChangedEvent` + `co.featbit.client.store.FlagChangeListener` FQNs must NOT move. + +- [ ] **Step 2: Move source + test files** + +```bash +cd /Users/deep.shah_fluentinhe/Documents/code/featbit-android-sdk/featbit-android-sdk +mkdir -p featbit-client/src/main/kotlin/co/featbit/client/app +mkdir -p featbit-client/src/test/kotlin/co/featbit/client/app + +# MemoryStore interface → domain (port) +git mv featbit-client/src/main/kotlin/co/featbit/client/store/MemoryStore.kt \ + featbit-client/src/main/kotlin/co/featbit/client/domain/MemoryStore.kt + +# DefaultMemoryStore impl → app (adapter) +git mv featbit-client/src/main/kotlin/co/featbit/client/store/DefaultMemoryStore.kt \ + featbit-client/src/main/kotlin/co/featbit/client/app/DefaultMemoryStore.kt + +# FlagValueChangedEvent STAYS — do not move + +# Application services → app +git mv featbit-client/src/main/kotlin/co/featbit/client/LifecycleController.kt \ + featbit-client/src/main/kotlin/co/featbit/client/app/LifecycleController.kt +git mv featbit-client/src/main/kotlin/co/featbit/client/changetracker/FlagTrackerImpl.kt \ + featbit-client/src/main/kotlin/co/featbit/client/app/FlagTrackerImpl.kt + +# Tests +git mv featbit-client/src/test/kotlin/co/featbit/client/LifecycleControllerTest.kt \ + featbit-client/src/test/kotlin/co/featbit/client/app/LifecycleControllerTest.kt +git mv featbit-client/src/test/kotlin/co/featbit/client/changetracker/FlagTrackerImplTest.kt \ + featbit-client/src/test/kotlin/co/featbit/client/app/FlagTrackerImplTest.kt +git mv featbit-client/src/test/kotlin/co/featbit/client/store/DefaultMemoryStoreTest.kt \ + featbit-client/src/test/kotlin/co/featbit/client/app/DefaultMemoryStoreTest.kt +``` + +- [ ] **Step 3: Update package declarations in moved files** + +Use Edit tool on each: +- `domain/MemoryStore.kt`: `package co.featbit.client.store` → `package co.featbit.client.domain` +- `app/DefaultMemoryStore.kt`: `package co.featbit.client.store` → `package co.featbit.client.app` +- `app/LifecycleController.kt`: `package co.featbit.client` → `package co.featbit.client.app` +- `app/FlagTrackerImpl.kt`: `package co.featbit.client.changetracker` → `package co.featbit.client.app` +- `app/DefaultMemoryStoreTest.kt`: `package co.featbit.client.store` → `package co.featbit.client.app` +- `app/LifecycleControllerTest.kt`: `package co.featbit.client` → `package co.featbit.client.app` +- `app/FlagTrackerImplTest.kt`: `package co.featbit.client.changetracker` → `package co.featbit.client.app` + +`FlagValueChangedEvent.kt` (still in `store/`) unchanged. + +- [ ] **Step 4: Update imports inside moved files** + +These need their imports rewritten because their dependencies are now in different packages: + +**`domain/MemoryStore.kt`** — was `package store`. References `FeatureFlag` (was implicit same-package or imported?). Check + add `import co.featbit.client.model.FeatureFlag` if missing. + +**`app/DefaultMemoryStore.kt`** — was `package store`. Implements `MemoryStore` (now in `domain/`) + references `FlagChangeListener` + `FlagValueChangedEvent` (still in `store/`). Add: +``` +import co.featbit.client.domain.MemoryStore +import co.featbit.client.store.FlagChangeListener +import co.featbit.client.store.FlagValueChangedEvent +``` + +**`app/FlagTrackerImpl.kt`** — was `package changetracker`. References `MemoryStore` (now `domain/`), `FlagChangeListener` + `FlagValueChangedEvent` (still `store/`). Update imports: +``` +import co.featbit.client.domain.MemoryStore +import co.featbit.client.store.FlagChangeListener +import co.featbit.client.store.FlagValueChangedEvent +``` + +**`app/LifecycleController.kt`** — was `package co.featbit.client`. Imports `DataSynchronizer` from `co.featbit.client.datasynchronizer` (unchanged in Task 2). No store/changetracker imports to update. + +**`app/DefaultMemoryStoreTest.kt`** — was `package store`. Test file. Update imports for `MemoryStore` (now domain) + `FlagChangeListener` / `FlagValueChangedEvent` (still store). + +**`app/FlagTrackerImplTest.kt`** — was `package changetracker`. Update similarly. + +**`app/LifecycleControllerTest.kt`** — was `package co.featbit.client`. References `DataSynchronizer` only — no changes. + +- [ ] **Step 5: Update imports in EXTERNAL callers** + +```bash +grep -rn "import co.featbit.client.store\.MemoryStore\|import co.featbit.client.LifecycleController$\|import co.featbit.client.changetracker.FlagTrackerImpl" featbit-client/src/ 2>/dev/null +``` + +For each match: +- `co.featbit.client.store.MemoryStore` → `co.featbit.client.domain.MemoryStore` +- `co.featbit.client.LifecycleController` → `co.featbit.client.app.LifecycleController` +- `co.featbit.client.changetracker.FlagTrackerImpl` → `co.featbit.client.app.FlagTrackerImpl` +- `co.featbit.client.store.DefaultMemoryStore` → `co.featbit.client.app.DefaultMemoryStore` (if any) + +Known callers (verified upfront): +- `FBClientImpl.kt` — imports `LifecycleController`, `FlagTrackerImpl`, `MemoryStore`, `DefaultMemoryStore`. +- `domain/Evaluator.kt` — imports `co.featbit.client.store.MemoryStore` (from Task 1). Update to `co.featbit.client.domain.MemoryStore`. +- `data.sync/PollingDataSynchronizer.kt` / `StreamingDataSynchronizer.kt` (still in `datasynchronizer/` at this point, see file path) — import `co.featbit.client.store.MemoryStore`. Update to `domain.MemoryStore`. + +Note: `FlagValueChangedEvent` + `FlagChangeListener` imports STAY at `co.featbit.client.store.*` — don't rewrite those. + +- [ ] **Step 6: Verify no stale references** + +```bash +grep -rn "co.featbit.client.store\.MemoryStore\|co.featbit.client.store\.DefaultMemoryStore" featbit-client/src/ 2>/dev/null +grep -rn "^import co.featbit.client.LifecycleController$\|co.featbit.client.changetracker.FlagTrackerImpl" featbit-client/src/ 2>/dev/null +``` +Expected: zero output for both greps. + +```bash +grep -rn "co.featbit.client.store.FlagValueChangedEvent\|co.featbit.client.store.FlagChangeListener" featbit-client/src/ 2>/dev/null +``` +Expected: SOME output — these stay in `store/` (public API). Confirms public API FQN preserved. + +- [ ] **Step 7: Run gates** + +```bash +./gradlew :featbit-client:compileDebugKotlin +./gradlew :featbit-client:testDebugUnitTest +./gradlew :featbit-client:assembleDebug :featbit-client-android:assembleDebug :example-app:assembleDebug +``` +Expected: all green, 98 pass / 0 fail / 2 e2e-skipped. + +- [ ] **Step 8: Audit + commit** + +Dispatch a code-reviewer agent with stance: "ports/adapters integrity" or "lifecycle of MemoryStore now that interface + impl live in different packages". Avoid same audit prompt as Task 1. + +```bash +git add -A +git commit -m "refactor: split MemoryStore into domain port + app adapter + +MemoryStore (interface) → domain/ as the port consumed by Evaluator. +DefaultMemoryStore (impl) → app/ as the adapter. FlagValueChangedEvent ++ FlagChangeListener stay in store/ — they are public API via +FlagTracker.subscribe(FlagChangeListener). + +LifecycleController + FlagTrackerImpl move into app/ as application +services. + +Closes the domain→app reverse arrow surfaced by Task 1 audit +(domain.Evaluator imported store.MemoryStore; would have become +domain→app once store/ moved into app/). + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +## Task 3: Move `datasynchronizer/*` → `data/sync/` + +**Files:** +- Move: `featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/DataSynchronizer.kt` → `featbit-client/src/main/kotlin/co/featbit/client/data/sync/DataSynchronizer.kt` +- Move: `featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/NullDataSynchronizer.kt` → `featbit-client/src/main/kotlin/co/featbit/client/data/sync/NullDataSynchronizer.kt` +- Move: `featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/PollingDataSynchronizer.kt` → `featbit-client/src/main/kotlin/co/featbit/client/data/sync/PollingDataSynchronizer.kt` +- Move: `featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/StreamingDataSynchronizer.kt` → `featbit-client/src/main/kotlin/co/featbit/client/data/sync/StreamingDataSynchronizer.kt` +- Move tests to mirror. + +- [ ] **Step 1: Move files** + +```bash +mkdir -p featbit-client/src/main/kotlin/co/featbit/client/data/sync +mkdir -p featbit-client/src/test/kotlin/co/featbit/client/data/sync +for f in DataSynchronizer NullDataSynchronizer PollingDataSynchronizer StreamingDataSynchronizer; do + git mv featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/${f}.kt \ + featbit-client/src/main/kotlin/co/featbit/client/data/sync/${f}.kt +done +git mv featbit-client/src/test/kotlin/co/featbit/client/datasynchronizer/PollingDataSynchronizerTest.kt \ + featbit-client/src/test/kotlin/co/featbit/client/data/sync/PollingDataSynchronizerTest.kt +git mv featbit-client/src/test/kotlin/co/featbit/client/datasynchronizer/StreamingDataSynchronizerTest.kt \ + featbit-client/src/test/kotlin/co/featbit/client/data/sync/StreamingDataSynchronizerTest.kt +``` + +- [ ] **Step 2: Update package declarations** + +In each of the 6 moved files (4 prod + 2 test): +- `package co.featbit.client.datasynchronizer` → `package co.featbit.client.data.sync` + +- [ ] **Step 3: Update imports in callers** + +```bash +grep -rn "co.featbit.client.datasynchronizer\." featbit-client/src/ 2>/dev/null +``` + +For each match, change `co.featbit.client.datasynchronizer.` → `co.featbit.client.data.sync.`. + +Expected callers: +- `FBClientImpl.kt` (imports `DataSynchronizer`, `NullDataSynchronizer`, `PollingDataSynchronizer`, `StreamingDataSynchronizer`). +- `LifecycleControllerTest.kt` (now in `app/`) — imports `DataSynchronizer`. +- The moved sync files themselves — internal package imports become same-package. + +- [ ] **Step 4: Verify** + +```bash +grep -rn "co.featbit.client.datasynchronizer" featbit-client/src/ 2>/dev/null +``` +Expected: zero output. + +- [ ] **Step 5: Run gates** (compileDebugKotlin + testDebugUnitTest + assembleDebug — same expected outputs). + +- [ ] **Step 6: Audit + commit** + +```bash +git add -A +git commit -m "refactor: move synchronizers into data/sync/ + +Synchronizer adapters are concrete data-layer impls. Move from the +junk-drawer-adjacent datasynchronizer/ into data/sync/. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +## Task 4: Move HTTP plumbing `internal/{FbApiClient,FBEndpoints,HttpConstants,GetUserFlags,ConnectionToken}` → `data/http/` + +**Files:** +- Move 5 source + 3 test files (ConnectionTokenTest, FBEndpointsTest, GetUserFlagsTest). + +- [ ] **Step 1: Move files** + +```bash +mkdir -p featbit-client/src/main/kotlin/co/featbit/client/data/http +mkdir -p featbit-client/src/test/kotlin/co/featbit/client/data/http +for f in FbApiClient FBEndpoints HttpConstants GetUserFlags ConnectionToken; do + git mv featbit-client/src/main/kotlin/co/featbit/client/internal/${f}.kt \ + featbit-client/src/main/kotlin/co/featbit/client/data/http/${f}.kt +done +git mv featbit-client/src/test/kotlin/co/featbit/client/internal/ConnectionTokenTest.kt \ + featbit-client/src/test/kotlin/co/featbit/client/data/http/ConnectionTokenTest.kt +git mv featbit-client/src/test/kotlin/co/featbit/client/internal/FBEndpointsTest.kt \ + featbit-client/src/test/kotlin/co/featbit/client/data/http/FBEndpointsTest.kt +git mv featbit-client/src/test/kotlin/co/featbit/client/internal/GetUserFlagsTest.kt \ + featbit-client/src/test/kotlin/co/featbit/client/data/http/GetUserFlagsTest.kt +``` + +- [ ] **Step 2: Update package declarations** (5 prod + 3 test files, `co.featbit.client.internal` → `co.featbit.client.data.http`). + +- [ ] **Step 3: Update callers** + +```bash +grep -rn "co.featbit.client.internal\.\(FbApiClient\|FBEndpoints\|HttpConstants\|GetUserFlags\|ConnectionToken\)" featbit-client/src/ 2>/dev/null +``` + +Replace `co.featbit.client.internal.` with `co.featbit.client.data.http.` for those 5 names. + +Known callers: +- `FBClientImpl.kt` — `FBEndpoints`, `HttpTrackInsight` (latter still in internal/ until Task 5). +- `PollingDataSynchronizer.kt` (now in `data/sync/`) — imports `FBEndpoints`, `GetUserFlags`. +- `StreamingDataSynchronizer.kt` (now in `data/sync/`) — imports `ConnectionToken`, `FBEndpoints`. +- `TrackInsight.kt` (still in `internal/` until Task 5) — imports `FbApiClient`, `FBEndpoints`. + +- [ ] **Step 4: Verify** (no stale internal.{Fb…|FB…|Http…|Get…|Con…} references). + +- [ ] **Step 5: Run gates**. + +- [ ] **Step 6: Audit + commit** + +```bash +git add -A +git commit -m "refactor: move HTTP plumbing into data/http/ + +FbApiClient, FBEndpoints, HttpConstants, GetUserFlags, ConnectionToken — +all OkHttp-bound HTTP adapter code. Move from junk-drawer internal/ into +data/http/. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +## Task 5: Move `internal/{TrackInsight,InsightDispatcher}` → `data/insights/` + +**Files:** +- Move 2 source + 2 test files (InsightDispatcherTest, TrackInsightTest). + +- [ ] **Step 1: Move files** + +```bash +mkdir -p featbit-client/src/main/kotlin/co/featbit/client/data/insights +mkdir -p featbit-client/src/test/kotlin/co/featbit/client/data/insights +git mv featbit-client/src/main/kotlin/co/featbit/client/internal/TrackInsight.kt \ + featbit-client/src/main/kotlin/co/featbit/client/data/insights/TrackInsight.kt +git mv featbit-client/src/main/kotlin/co/featbit/client/internal/InsightDispatcher.kt \ + featbit-client/src/main/kotlin/co/featbit/client/data/insights/InsightDispatcher.kt +git mv featbit-client/src/test/kotlin/co/featbit/client/internal/InsightDispatcherTest.kt \ + featbit-client/src/test/kotlin/co/featbit/client/data/insights/InsightDispatcherTest.kt +git mv featbit-client/src/test/kotlin/co/featbit/client/internal/TrackInsightTest.kt \ + featbit-client/src/test/kotlin/co/featbit/client/data/insights/TrackInsightTest.kt +``` + +- [ ] **Step 2: Update package declarations** (2 prod + 2 test, `co.featbit.client.internal` → `co.featbit.client.data.insights`). + +- [ ] **Step 3: Update callers** + +```bash +grep -rn "co.featbit.client.internal\.\(TrackInsight\|InsightDispatcher\|NoopTrackInsight\|HttpTrackInsight\)" featbit-client/src/ 2>/dev/null +``` + +Known caller: +- `FBClientImpl.kt` — imports `HttpTrackInsight`, `InsightDispatcher`, `NoopTrackInsight`, `TrackInsight`. + +Replace each with `co.featbit.client.data.insights.`. + +- [ ] **Step 4: Verify `internal/` directory is now empty** + +```bash +ls featbit-client/src/main/kotlin/co/featbit/client/internal/ 2>/dev/null +ls featbit-client/src/test/kotlin/co/featbit/client/internal/ 2>/dev/null +``` +Expected: empty (no files). If empty, remove the directory: +```bash +rmdir featbit-client/src/main/kotlin/co/featbit/client/internal +rmdir featbit-client/src/test/kotlin/co/featbit/client/internal +``` + +- [ ] **Step 5: Run gates** + audit + commit. + +```bash +git add -A +git commit -m "refactor: move insight pipeline into data/insights/ + +TrackInsight + InsightDispatcher are the analytics surface. With this +move, the internal/ junk drawer is dissolved. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +## Task 6: Move wire DTOs `model/{EndUser,Insight}` → `wire/` + +**Files:** +- Move 2 source files. There is no test for `EndUser` / `Insight` directly; `BuildersTest` exists in `model/` and tests `FBUser` (which STAYS in `model/`). + +NOTE: `FBUser.kt` STAYS in `model/`. `FeatureFlag.kt` STAYS in `model/`. ONLY `EndUser.kt` and `Insight.kt` move. + +- [ ] **Step 1: Verify `EndUser` and `Insight` are `internal`** + +```bash +head -20 featbit-client/src/main/kotlin/co/featbit/client/model/EndUser.kt | grep -E "^internal|^public" +head -20 featbit-client/src/main/kotlin/co/featbit/client/model/Insight.kt | grep -E "^internal|^public" +``` +Expected: both show `internal data class`. (If either is `public`, this move breaks consumers — abort + downgrade visibility plan separately.) + +- [ ] **Step 2: Move files** + +```bash +mkdir -p featbit-client/src/main/kotlin/co/featbit/client/wire +git mv featbit-client/src/main/kotlin/co/featbit/client/model/EndUser.kt \ + featbit-client/src/main/kotlin/co/featbit/client/wire/EndUser.kt +git mv featbit-client/src/main/kotlin/co/featbit/client/model/Insight.kt \ + featbit-client/src/main/kotlin/co/featbit/client/wire/Insight.kt +``` + +- [ ] **Step 3: Update package declarations** (2 files, `co.featbit.client.model` → `co.featbit.client.wire`). + +- [ ] **Step 4: Update callers** + +```bash +grep -rn "co.featbit.client.model.EndUser\|co.featbit.client.model.Insight\|co.featbit.client.model.VariationInsight\|co.featbit.client.model.VariationData" featbit-client/src/ 2>/dev/null +``` + +Replace each occurrence's `model` segment with `wire`. + +Known callers (verified upfront): +- `FBClientImpl.kt` — imports `Insight`. +- `GetUserFlags.kt` (now in `data/http/`) — imports `EndUser`. +- `StreamingDataSynchronizer.kt` (now in `data/sync/`) — imports `EndUser`. +- `TrackInsight.kt` (now in `data/insights/`) — imports `Insight`. +- `InsightDispatcherTest.kt` (now in `data/insights/`) — imports `Insight`. +- `BuildersTest.kt` (stays in `model/`) — imports `EndUser` and uses `CustomizedProperty` (which lives inside `EndUser.kt`). + +- [ ] **Step 5: Verify** + +```bash +grep -rn "co.featbit.client.model.EndUser\|co.featbit.client.model.Insight" featbit-client/src/ 2>/dev/null +``` +Expected: zero output. (`model.FBUser` and `model.FeatureFlag` references must remain — they're public.) + +- [ ] **Step 6: Run gates** + audit + commit. + +```bash +git add -A +git commit -m "refactor: move wire DTOs into wire/ + +EndUser + Insight (+ VariationInsight + VariationData) are internal +kotlinx-serialization DTOs — separate them from public domain types +(FBUser, FeatureFlag) so future @Serializable annotations never +accidentally creep onto domain models. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +## Task 7: Final E2E + close-out + +**Files:** +- No code changes; final verification + cleanup of leftover directories. + +- [ ] **Step 1: Verify all old directories are empty + remove them** + +```bash +for d in featbit-client/src/main/kotlin/co/featbit/client/changetracker \ + featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer \ + featbit-client/src/main/kotlin/co/featbit/client/internal \ + featbit-client/src/main/kotlin/co/featbit/client/store \ + featbit-client/src/test/kotlin/co/featbit/client/changetracker \ + featbit-client/src/test/kotlin/co/featbit/client/datasynchronizer \ + featbit-client/src/test/kotlin/co/featbit/client/internal \ + featbit-client/src/test/kotlin/co/featbit/client/store; do + if [ -d "$d" ] && [ -z "$(ls -A "$d")" ]; then rmdir "$d"; fi +done +``` + +NOTE: Directories that survive (do NOT remove): +- `changetracker/` keeps `FlagTracker.kt` (public interface) +- `model/` keeps `FBUser.kt` + `FeatureFlag.kt` +- `evaluation/` keeps `EvalDetail.kt` +- `store/` keeps `FlagValueChangedEvent.kt` (which contains `FlagValueChangedEvent` + `FlagChangeListener`, both public via `FlagTracker`) +- `options/` keeps `FBOptions.kt` + `DataSyncMode.kt` + +The `rmdir` loop above is safe — it skips non-empty dirs. + +- [ ] **Step 2: Verify the final target layout** + +```bash +find featbit-client/src/main/kotlin/co/featbit/client -type d | sort +``` +Expected (subset): +``` +.../co/featbit/client +.../co/featbit/client/app +.../co/featbit/client/changetracker +.../co/featbit/client/data +.../co/featbit/client/data/http +.../co/featbit/client/data/insights +.../co/featbit/client/data/sync +.../co/featbit/client/domain +.../co/featbit/client/evaluation +.../co/featbit/client/model +.../co/featbit/client/options +.../co/featbit/client/wire +``` + +- [ ] **Step 3: Dependency-rule sanity check** + +Domain must not import from data, app, wire, or any framework: +```bash +grep -rn "^import" featbit-client/src/main/kotlin/co/featbit/client/domain/ 2>/dev/null \ + | grep -v "^[^:]*:import kotlin\|^[^:]*:import co.featbit.client.model.\(FBUser\|FeatureFlag\)$" +``` +Expected: zero output (other than `kotlin.*` stdlib imports + public `model.FBUser` / `model.FeatureFlag` references if any). + +If `domain/` has any `co.featbit.client.app.`, `co.featbit.client.data.`, `co.featbit.client.wire.`, `okhttp3.`, `kotlinx.serialization.`, or `android.` imports — that's a dependency-rule violation; investigate. + +- [ ] **Step 4: Run full gate + E2E against real FeatBit stack** + +```bash +./gradlew :featbit-client:assembleDebug :featbit-client-android:assembleDebug :example-app:assembleDebug +./gradlew :featbit-client:testDebugUnitTest +DOCKER_HOST="unix:///Users/deep.shah_fluentinhe/.colima/default/docker.sock" \ + TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE="/var/run/docker.sock" \ + FEATBIT_E2E=1 ./gradlew :featbit-client:testDebugUnitTest --tests "co.featbit.client.e2e.*" +``` +Expected: +- assembleDebug: green for all 3 modules. +- Unit tests: 98 pass / 0 fail / 2 e2e-skipped (e2e skipped without env var). +- With FEATBIT_E2E=1: both `FeatBitE2ETest` + `FeatBitStreamingE2ETest` pass against Colima FeatBit. + +- [ ] **Step 5: Update `docs/superpowers/architecture.md`** + +Open `docs/superpowers/architecture.md`. Update the "Module / package layout" section's tree to match the new layout (the one produced by Step 2). Same content elsewhere — just the directory diagram changes. + +- [ ] **Step 6: Final audit on whole-branch diff vs main** + +Dispatch `superpowers:code-reviewer` (wider scope) on the diff `git diff main...refactor/clean-architecture -- 'featbit-client/**'`. Expected: zero blocking findings. Tighten any audit-flagged items inline if NIT. + +- [ ] **Step 7: Commit closeout** + +```bash +git add -A +git commit -m "refactor: prune empty package directories + update architecture doc + +Wraps up the clean-architecture pass. Final layout: domain/, app/, +data/{sync,http,insights}/, wire/ — plus the unchanged public-API +packages (root, options/, model/, evaluation/, changetracker/). + +E2E verified against real FeatBit stack (Colima). + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +## Self-review (writing-plans skill checklist, run inline) + +- **Spec coverage:** Every move-table row in the spec maps to a Task 1-6 step. Final layout (Task 7 Step 2) matches the spec's target layout block. +- **Placeholder scan:** No TBD / TODO. Every step has either bash commands or explicit Edit-tool instructions. +- **Type consistency:** Package FQNs used consistently — `co.featbit.client.domain`, `co.featbit.client.app`, `co.featbit.client.data.sync`, `co.featbit.client.data.http`, `co.featbit.client.data.insights`, `co.featbit.client.wire`. Public types and their original FQNs explicitly preserved in each task's NOTE block. +- **Verification gates uniform:** every code-touching task has compile + test + assemble + audit + commit steps. + +Issue caught + fixed inline: Task 6 needs to confirm `EndUser` and `Insight` are `internal` BEFORE moving — added Step 1 to verify. diff --git a/docs/superpowers/specs/2026-06-25-clean-architecture-design.md b/docs/superpowers/specs/2026-06-25-clean-architecture-design.md new file mode 100644 index 0000000..4f567f0 --- /dev/null +++ b/docs/superpowers/specs/2026-06-25-clean-architecture-design.md @@ -0,0 +1,167 @@ +# Clean-Architecture Refactor — Design + +**Date:** 2026-06-25 +**Branch:** `refactor/clean-architecture` +**Status:** Design — awaiting approval before implementation + +## Goal + +Reorganise the FeatBit Android SDK source layout into a layered architecture (domain / application / data / wire) while preserving every public API contract and every test. The user-visible result is identical; the internal layout makes responsibilities explicit and reduces coupling. + +## Constraints (non-negotiable) + +1. **Public API is byte-compatible.** `FBClient`, `FBClientImpl`, `FBOptions`, `FBUser`, `FlagTracker`, `EvalDetail`, `FBLogger`, `FeatureFlag`, `DataSyncMode` — same FQNs, same signatures. Consumers' `import co.featbit.client.FBClient` (and similar) continue to compile. +2. **All 98 unit tests pass.** Both E2E tests pass against the real FeatBit stack. +3. **No behaviour change.** Pure file moves + import updates. No logic rewritten. +4. **No new Gradle modules.** Module split (Option C in brainstorm) is deferred — it would change Maven coordinates, which is external-consumer-visible. +5. **Files stay in their current sizes.** Splitting `StreamingDataSynchronizer` (266 LoC) is a separate refactor; out of scope. + +## Current layout (problems) + +``` +co.featbit.client/ +├── (root) FBClient, FBClientImpl, FBLogger, LifecycleController +├── changetracker/ FlagTracker, FlagTrackerImpl +├── datasynchronizer/ DataSynchronizer + Null/Polling/Streaming +├── evaluation/ EvalDetail, EvalResult, Evaluator, ValueConverters +├── internal/ HTTP, endpoints, token, insight pipeline (junk drawer) +├── model/ FBUser, FeatureFlag, EndUser, Insight (mix of public + wire DTOs) +├── options/ DataSyncMode, FBOptions +└── store/ MemoryStore, DefaultMemoryStore, FlagValueChangedEvent +``` + +**Specific smells:** +- `internal/` mixes HTTP plumbing, batch dispatcher, token encoder, two endpoint clients. +- `model/` mixes public domain types (`FBUser`, `FeatureFlag`) with wire-format DTOs (`EndUser`, `Insight`). +- `LifecycleController` sits at the root next to public types but is internal. +- No clear dependency rule — `FBClientImpl` (orchestration) imports from 23 distinct internal packages. + +## Target layout + +``` +co.featbit.client.* (PUBLIC API — unchanged FQNs) + FBClient, FBClientImpl, FBOptions, FBUser, FlagTracker, EvalDetail, + FBLogger, FeatureFlag, DataSyncMode + +co.featbit.client.domain/ (pure Kotlin, no Android / OkHttp / kotlinx-serialization) + Evaluator, EvalResult, ValueConverters + +co.featbit.client.app/ (application services / orchestration helpers) + LifecycleController, FlagTrackerImpl, + MemoryStore, DefaultMemoryStore, FlagValueChangedEvent + +co.featbit.client.data.sync/ (synchronizer adapters) + DataSynchronizer, NullDataSynchronizer, PollingDataSynchronizer, StreamingDataSynchronizer + +co.featbit.client.data.http/ (HTTP plumbing — OkHttp-bound) + FbApiClient, FBEndpoints, HttpConstants, GetUserFlags, ConnectionToken + +co.featbit.client.data.insights/ (analytics pipeline) + TrackInsight, NoopTrackInsight, HttpTrackInsight, InsightDispatcher + +co.featbit.client.wire/ (kotlinx-serialization DTOs only) + EndUser, Insight, VariationInsight, VariationData +``` + +### Why these names + +- **`domain`**: pure types and the `MemoryStore` port (interface). Stays free of Android / OkHttp / app / data / wire deps. **Known carve-out:** `domain.EvalResult.Found` wraps `model.FeatureFlag`, which is `@Serializable` for wire-compat. We accept the transitive `kotlinx-serialization` reference here because moving `FeatureFlag` would break the public `FBClient.allFlags()` return type. A future refactor could split `FeatureFlag` into a pure-domain core + a wire DTO — out of scope here. +- **`app`**: holds application services + the `MemoryStore` adapter. `DefaultMemoryStore` lives here (concrete impl of the domain port). `LifecycleController` + `FlagTrackerImpl` are app services. `FlagValueChangedEvent` + `FlagChangeListener` stay in `store/` because they're part of the public `FlagTracker` API surface. +- **`data.sync`**, **`data.http`**, **`data.insights`**: each is one concrete adapter family. Replaces the `internal/` junk drawer. +- **`wire`**: kotlinx-serialization DTOs. They are NOT domain models — they are the on-wire format. Separating them prevents future refactors from accidentally letting `@Serializable` annotations creep into domain types. + +### Dependency rule + +``` +data.* → app → domain +data.* → wire +app → domain (interface + types) +PUBLIC (root) → app, domain, data.* (orchestration glue) +PUBLIC store/ → (nothing in this refactor — pure data types) +``` + +No `domain` → `app` / `data.*` / `wire`. No `app` → `data.*`. No `wire` → `domain`. Enforced by code review (no Gradle-level boundary yet). + +### `MemoryStore` as a port (ports/adapters) + +Issue surfaced by Task 1 audit: `Evaluator` (now in `domain/`) imports `MemoryStore`. If `MemoryStore` moves to `app/`, the dependency arrow becomes `domain → app` — a rule violation. + +Resolution: **`MemoryStore` (interface) lives in `domain/` as a port. `DefaultMemoryStore` (impl) lives in `app/` as the adapter.** This is hexagonal architecture as the textbook intends. `Evaluator` depends on the domain port; the implementation is injected. + +Single-file pair split: +- `domain/MemoryStore.kt` — interface (was `store/MemoryStore.kt`). +- `app/DefaultMemoryStore.kt` — impl (was `store/DefaultMemoryStore.kt`). +- `store/FlagValueChangedEvent.kt` — STAYS (public API; referenced by `FlagTracker.subscribe(FlagChangeListener)`). Contains both `FlagValueChangedEvent` data class and `FlagChangeListener` fun interface. + +## File-by-file move plan + +Public files stay (no move): +- `FBClient.kt`, `FBClientImpl.kt`, `FBLogger.kt`, `FBOptions.kt`, `FBUser.kt`, `FeatureFlag.kt`, `FlagTracker.kt`, `EvalDetail.kt`, `DataSyncMode.kt` + +Internal moves: +| Current location | New location | +|---|---| +| `evaluation/Evaluator.kt` | `domain/Evaluator.kt` | +| `evaluation/EvalResult.kt` | `domain/EvalResult.kt` | +| `evaluation/ValueConverters.kt` | `domain/ValueConverters.kt` | +| `LifecycleController.kt` | `app/LifecycleController.kt` | +| `changetracker/FlagTrackerImpl.kt` | `app/FlagTrackerImpl.kt` | +| `store/MemoryStore.kt` | `domain/MemoryStore.kt` *(port — used by `Evaluator`)* | +| `store/DefaultMemoryStore.kt` | `app/DefaultMemoryStore.kt` *(adapter)* | +| `store/FlagValueChangedEvent.kt` | **STAYS** at `store/FlagValueChangedEvent.kt` *(public API via `FlagTracker`)* | +| `datasynchronizer/*` | `data/sync/*` | +| `internal/FbApiClient.kt` | `data/http/FbApiClient.kt` | +| `internal/FBEndpoints.kt` | `data/http/FBEndpoints.kt` | +| `internal/HttpConstants.kt` | `data/http/HttpConstants.kt` | +| `internal/GetUserFlags.kt` | `data/http/GetUserFlags.kt` | +| `internal/ConnectionToken.kt` | `data/http/ConnectionToken.kt` | +| `internal/TrackInsight.kt` | `data/insights/TrackInsight.kt` | +| `internal/InsightDispatcher.kt` | `data/insights/InsightDispatcher.kt` | +| `model/EndUser.kt` | `wire/EndUser.kt` | +| `model/Insight.kt` | `wire/Insight.kt` | + +`FBOptions.kt` stays in `options/` to preserve its existing FQN `co.featbit.client.options.FBOptions`. *(Existing import; consumers depend on it.)* + +`FlagTracker.kt` (public interface) stays in `changetracker/` for the same reason. + +## Tests + +Test packages mirror source packages. Each test file moves to match the new location of the class under test. Test logic does not change. Imports update mechanically. + +The `78 unit tests + 2 e2e + ... = 98 total` must all stay green at every step. + +## Verification gates (at each commit) + +1. `./gradlew :featbit-client:compileDebugKotlin` — green +2. `./gradlew :featbit-client:testDebugUnitTest` — 98 pass / 0 fail / 2 e2e-skipped +3. `./gradlew :featbit-client:assembleDebug :featbit-client-android:assembleDebug :example-app:assembleDebug` — green +4. Adversarial audit on the diff — zero blocking findings +5. (Once, before push) E2E with `FEATBIT_E2E=1` against Colima FeatBit — both tests pass + +## Risk + mitigation + +| Risk | Mitigation | +|---|---| +| Test imports break | Each commit moves one logical group + updates all callers in same commit | +| Public API accidentally moves | Pre-commit grep: `git diff --name-only HEAD~1 -- '*FBClient*' '*FBOptions*' '*FBUser*' '*EvalDetail*' '*FBLogger*' '*FeatureFlag*' '*FlagTracker.kt'` must show no `R` (rename) entries for these files | +| E2E regresses | Run after every 2-3 logical commits, not just at the end | +| Cyclic dependencies introduced | After every commit, grep for `domain/*.kt` importing `data.*` or `wire.*` — must be empty | + +## Out of scope + +- Module splits (Option C): Maven artifact compatibility concern; defer. +- Splitting `StreamingDataSynchronizer` (266 LoC): separate refactor. +- Behaviour changes (e.g. the cancellation-propagation follow-up flagged in `TrackInsightTest`): separate commits. +- Removing the `internal/` package entirely (one or two files may stay if a clean home doesn't exist). + +## Implementation strategy (one commit per layer) + +1. **Commit 1:** Move `domain/` (Evaluator, EvalResult, ValueConverters). Update imports in callers + tests. Verify gates. +2. **Commit 2:** Move `app/` group (LifecycleController, FlagTrackerImpl, store/*). Update imports + tests. +3. **Commit 3:** Move `data/sync/` (datasynchronizer/*). Update imports + tests. +4. **Commit 4:** Move `data/http/` (FbApiClient, FBEndpoints, ConnectionToken, GetUserFlags, HttpConstants). Update imports + tests. +5. **Commit 5:** Move `data/insights/` (TrackInsight, InsightDispatcher). Update imports + tests. +6. **Commit 6:** Move `wire/` (EndUser, Insight). Update imports + tests. +7. **Commit 7:** Final audit + E2E re-run + delete the now-empty `evaluation/`, `changetracker/`, `store/`, `datasynchronizer/`, `internal/`, `model/` directories. + +Each commit independently green. If any commit fails, roll back just that commit and reconsider — every earlier commit stands on its own. diff --git a/featbit-client/build.gradle.kts b/featbit-client/build.gradle.kts index 458913c..b605453 100644 --- a/featbit-client/build.gradle.kts +++ b/featbit-client/build.gradle.kts @@ -27,6 +27,18 @@ android { kotlinOptions { jvmTarget = "11" + // Module-wide mode for default-method binary compat on public interfaces. Emits + // default-method bodies into the interface bytecode (real Java 8 defaults) AND keeps + // a `DefaultImpls` synthetic class as a fallback for legacy binaries compiled against + // earlier versions of the interface. Applies to every interface in this module — no + // per-interface `@JvmDefaultWithCompatibility` annotation needed (that annotation + // only matters under `-Xjvm-default=all`). + // + // Required for `MemoryStore.upsertAll`: gaining a new default method on a `public` + // interface would otherwise either source-break Java implementers (forcing them to + // override) or `AbstractMethodError` old binary implementers when the SDK calls the + // method. + freeCompilerArgs = freeCompilerArgs + "-Xjvm-default=all-compatibility" } testOptions { diff --git a/featbit-client/src/main/kotlin/co/featbit/client/FBClientImpl.kt b/featbit-client/src/main/kotlin/co/featbit/client/FBClientImpl.kt index 4c49711..836dd6a 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/FBClientImpl.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/FBClientImpl.kt @@ -1,37 +1,44 @@ package co.featbit.client +import co.featbit.client.app.DefaultMemoryStore +import co.featbit.client.app.FlagTrackerImpl +import co.featbit.client.app.LifecycleController import co.featbit.client.changetracker.FlagTracker -import co.featbit.client.changetracker.FlagTrackerImpl -import co.featbit.client.datasynchronizer.DataSynchronizer -import co.featbit.client.datasynchronizer.NullDataSynchronizer -import co.featbit.client.datasynchronizer.PollingDataSynchronizer -import co.featbit.client.datasynchronizer.StreamingDataSynchronizer +import co.featbit.client.data.sync.DataSynchronizer +import co.featbit.client.data.sync.NullDataSynchronizer +import co.featbit.client.data.sync.PollingDataSynchronizer +import co.featbit.client.data.sync.StreamingDataSynchronizer +import co.featbit.client.domain.EvalResult +import co.featbit.client.domain.Evaluator +import co.featbit.client.domain.MemoryStore +import co.featbit.client.domain.ValueConverter +import co.featbit.client.domain.ValueConverters import co.featbit.client.evaluation.EvalDetail -import co.featbit.client.evaluation.Evaluator -import co.featbit.client.evaluation.ValueConverter -import co.featbit.client.evaluation.ValueConverters -import co.featbit.client.internal.HttpTrackInsight -import co.featbit.client.internal.NoopTrackInsight -import co.featbit.client.internal.TrackInsight +import co.featbit.client.data.http.FBEndpoints +import co.featbit.client.data.insights.HttpTrackInsight +import co.featbit.client.data.insights.InsightDispatcher +import co.featbit.client.data.insights.NoopTrackInsight +import co.featbit.client.data.insights.TrackInsight import co.featbit.client.model.FBUser import co.featbit.client.model.FeatureFlag -import co.featbit.client.model.Insight +import co.featbit.client.wire.Insight import co.featbit.client.options.DataSyncMode import co.featbit.client.options.FBOptions -import co.featbit.client.store.DefaultMemoryStore -import co.featbit.client.store.MemoryStore import kotlinx.coroutines.CoroutineExceptionHandler import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancel -import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withTimeoutOrNull +import java.util.concurrent.atomic.AtomicReference import kotlin.time.Duration /** * Default [FBClient] implementation. Wires together the store, evaluator, flag tracker, - * insight tracker, and data synchronizer, mirroring the .NET `FbClient`. + * insight pipeline, and data synchronizer, mirroring the .NET `FbClient`. * * @param options the client configuration. * @param initialUser the initial evaluation user; change it later with [identify]. @@ -45,8 +52,6 @@ public class FBClientImpl( private val store: MemoryStore = DefaultMemoryStore(options.bootstrap) private val evaluator = Evaluator(store) private val flagTrackerImpl = FlagTrackerImpl(store) - private val trackInsight: TrackInsight = - if (options.offline) NoopTrackInsight else HttpTrackInsight(options) private val scope = CoroutineScope( SupervisorJob() + Dispatchers.IO + CoroutineExceptionHandler { _, t -> @@ -54,31 +59,55 @@ public class FBClientImpl( }, ) - @Volatile - private var user: FBUser = initialUser - - @Volatile - private var dataSynchronizer: DataSynchronizer = newDataSynchronizer(initialUser) + // Single, eagerly-parsed source of truth for the FeatBit URLs we talk to. Constructing it + // once (rather than once per HTTP client) keeps "centralization" honest and surfaces any + // malformed configuration at SDK init. + private val endpoints: FBEndpoints? = + if (options.offline) null else FBEndpoints.from(options) + + // Insight pipeline — bounded, batched, non-blocking on the evaluation hot path. + private val tracker: TrackInsight = + if (options.offline) NoopTrackInsight else HttpTrackInsight(options, endpoints = endpoints!!) + private val insights: InsightDispatcher = InsightDispatcher(tracker, scope, logger) + + // When the tracker is the no-op (offline mode), every Insight we'd build is thrown away by + // the consumer. Short-circuit at the *call site* so we don't allocate the wire-form chain + // per evaluation just to discard it — Insight + the singleton variation List wrapper + + // VariationInsight + VariationData = ~4 garbage objects per flag check the JVM never has + // to see in offline mode (the EndUser is cached on FBUser, so it doesn't add to the count). + private val insightsEnabled: Boolean = tracker !is NoopTrackInsight + + // user is read on every evaluation; AtomicReference gives lock-free reads + atomic swap on identify(). + private val userRef = AtomicReference(initialUser) + + /** + * Active synchronizer holder. Replaced atomically on [identify]; the previous + * synchronizer is closed under [identifyMutex] so its in-flight upserts cannot land + * after the swap (those would otherwise contaminate the new user's flag store). + */ + private val syncRef: AtomicReference = + AtomicReference(newDataSynchronizer(initialUser)) + private val identifyMutex = Mutex() private val lifecycle = - LifecycleController(scope, options.backgroundGracePeriod.inWholeMilliseconds) { dataSynchronizer } + LifecycleController(scope, options.backgroundGracePeriod.inWholeMilliseconds) { syncRef.get() } - override val initialized: Boolean get() = dataSynchronizer.initialized + override val initialized: Boolean get() = syncRef.get().initialized override val flagTracker: FlagTracker get() = flagTrackerImpl private fun newDataSynchronizer(forUser: FBUser): DataSynchronizer = when { options.offline -> NullDataSynchronizer() options.dataSyncMode == DataSyncMode.Streaming -> - StreamingDataSynchronizer(options, forUser, store) + StreamingDataSynchronizer(options, forUser, store, endpoints = endpoints!!) options.dataSyncMode == DataSyncMode.Polling -> - PollingDataSynchronizer(options, forUser, store) + PollingDataSynchronizer(options, forUser, store, endpoints = endpoints!!) else -> NullDataSynchronizer() } override suspend fun start(timeout: Duration): Boolean { logger.info("Waiting up to $timeout for FBClient to start...") - val success = withTimeoutOrNull(timeout) { dataSynchronizer.start() } ?: false + val success = withTimeoutOrNull(timeout) { syncRef.get().start() } ?: false if (success) { logger.info("FBClient successfully started.") } else { @@ -90,56 +119,65 @@ public class FBClientImpl( return success } - override suspend fun identify(user: FBUser, timeout: Duration): Boolean { - this.user = user - - // Dispose the current synchronizer and start a fresh one for the new user. - dataSynchronizer.close() - dataSynchronizer = newDataSynchronizer(user) - - val success = withTimeoutOrNull(timeout) { dataSynchronizer.start() } ?: false + override suspend fun identify(user: FBUser, timeout: Duration): Boolean = identifyMutex.withLock { + // Tear down the old synchronizer *and await its in-flight upserts* before installing a + // new one. `close()` alone only cancels the scope (non-suspending); a polling response + // already mid-`store.upsert` would race past the swap. `closeAndJoin` waits for that + // upsert to finish before returning, guaranteeing no stale data lands under the new user. + syncRef.get().closeAndJoin() + val fresh = newDataSynchronizer(user) + syncRef.set(fresh) + userRef.set(user) - // Fire-and-forget the user insight. - scope.launch { trackInsight.run(Insight.forIdentify(user)) } + val success = withTimeoutOrNull(timeout) { fresh.start() } ?: false - return success + if (insightsEnabled) insights.offer(Insight.forIdentify(user)) + success } override fun boolVariation(key: String, default: Boolean): Boolean = - evaluateCore(key, default, ValueConverters.bool).value + evaluateValue(key, default, ValueConverters.bool) override fun boolVariationDetail(key: String, default: Boolean): EvalDetail = evaluateCore(key, default, ValueConverters.bool) override fun intVariation(key: String, default: Int): Int = - evaluateCore(key, default, ValueConverters.int).value + evaluateValue(key, default, ValueConverters.int) override fun intVariationDetail(key: String, default: Int): EvalDetail = evaluateCore(key, default, ValueConverters.int) override fun floatVariation(key: String, default: Float): Float = - evaluateCore(key, default, ValueConverters.float).value + evaluateValue(key, default, ValueConverters.float) override fun floatVariationDetail(key: String, default: Float): EvalDetail = evaluateCore(key, default, ValueConverters.float) override fun doubleVariation(key: String, default: Double): Double = - evaluateCore(key, default, ValueConverters.double).value + evaluateValue(key, default, ValueConverters.double) override fun doubleVariationDetail(key: String, default: Double): EvalDetail = evaluateCore(key, default, ValueConverters.double) override fun stringVariation(key: String, default: String): String = - evaluateCore(key, default, ValueConverters.string).value + evaluateValue(key, default, ValueConverters.string) override fun stringVariationDetail(key: String, default: String): EvalDetail = evaluateCore(key, default, ValueConverters.string) - override fun allFlags(): Map = store.getAll().associateBy { it.id } + override fun allFlags(): Map { + // store.getAll() already returns a fresh Collection snapshot; calling .associateBy + // here would walk it a second time + allocate the intermediate List. Build the result + // map directly from the snapshot — one allocation, one pass. + val snapshot = store.getAll() + val result = LinkedHashMap(snapshot.size) + for (flag in snapshot) result[flag.id] = flag + return result + } - override fun setForeground(foreground: Boolean) = lifecycle.onForegroundChanged(foreground) + override fun setForeground(foreground: Boolean): Unit = lifecycle.onForegroundChanged(foreground) - override fun setNetworkAvailable(available: Boolean) = lifecycle.onNetworkChanged(available) + override fun setNetworkAvailable(available: Boolean): Unit = lifecycle.onNetworkChanged(available) private fun evaluateCore( key: String, @@ -151,26 +189,58 @@ public class FBClientImpl( return EvalDetail("client not ready", default) } - val (evalResult, flag) = evaluator.evaluate(key) - if (!evalResult.isValid || flag == null) { - return EvalDetail(evalResult.reason, default) + return when (val result = evaluator.evaluate(key)) { + is EvalResult.NotFound -> EvalDetail(result.reason, default) + is EvalResult.Found -> { + if (insightsEnabled) { + insights.offer( + Insight.forEvaluation(userRef.get(), result.flag, System.currentTimeMillis()), + ) + } + val typed = converter(result.value) + if (typed != null) EvalDetail(result.reason, typed) + else EvalDetail("type mismatch", default) + } } + } - // Fire-and-forget the evaluation insight. - scope.launch { trackInsight.run(Insight.forEvaluation(user, flag, System.currentTimeMillis())) } - - val typed = converter(evalResult.value) - return if (typed != null) { - EvalDetail(evalResult.reason, typed) - } else { - EvalDetail("type mismatch", default) + // Fast-path for `*Variation()` getters that discard `.reason` immediately. Skips the + // EvalDetail allocation that evaluateCore() would otherwise build and the caller would + // throw away. The detail-returning getters keep going through evaluateCore so they can + // surface the per-call reason string. + private fun evaluateValue(key: String, default: T, converter: ValueConverter): T { + if (!initialized && options.bootstrap.isEmpty()) return default + + return when (val result = evaluator.evaluate(key)) { + is EvalResult.NotFound -> default + is EvalResult.Found -> { + if (insightsEnabled) { + insights.offer( + Insight.forEvaluation(userRef.get(), result.flag, System.currentTimeMillis()), + ) + } + converter(result.value) ?: default + } } } override fun close() { - dataSynchronizer.close() + // FBClient.close() is the public Closeable contract (non-suspending). Bridge to the + // two suspending teardowns with independent per-phase budgets so a slow sync teardown + // can't starve the insight drain (and vice versa) — the latter must reach + // `tracker.close()` to release the underlying OkHttp dispatcher even under contention. + runBlocking { + withTimeoutOrNull(SYNC_CLOSE_TIMEOUT_MS) { syncRef.get().closeAndJoin() } + withTimeoutOrNull(INSIGHTS_CLOSE_TIMEOUT_MS) { insights.closeAndDrain() } + } flagTrackerImpl.close() - trackInsight.close() scope.cancel() } + + private companion object { + // Per-phase budgets. Total worst-case: 4s — well under the 10s Android ANR threshold, + // and each phase has its own deadline so neither can starve the other. + const val SYNC_CLOSE_TIMEOUT_MS: Long = 2_000L + const val INSIGHTS_CLOSE_TIMEOUT_MS: Long = 2_000L + } } diff --git a/featbit-client/src/main/kotlin/co/featbit/client/app/DefaultMemoryStore.kt b/featbit-client/src/main/kotlin/co/featbit/client/app/DefaultMemoryStore.kt new file mode 100644 index 0000000..20d7b38 --- /dev/null +++ b/featbit-client/src/main/kotlin/co/featbit/client/app/DefaultMemoryStore.kt @@ -0,0 +1,99 @@ +package co.featbit.client.app + +import co.featbit.client.domain.MemoryStore +import co.featbit.client.model.FeatureFlag +import co.featbit.client.store.FlagChangeListener +import co.featbit.client.store.FlagValueChangedEvent +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.CopyOnWriteArrayList + +/** + * Default [MemoryStore] backed by a [ConcurrentHashMap]. + * + * Reads are lock-free. Writes are serialized under a single monitor so that the + * "compute change event then store" sequence is atomic, matching the .NET + * `DefaultMemoryStore`. Change listeners are notified outside the write lock. + */ +public class DefaultMemoryStore( + bootstrap: List = emptyList(), +) : MemoryStore { + + private val items: ConcurrentHashMap = ConcurrentHashMap() + private val listeners: CopyOnWriteArrayList = CopyOnWriteArrayList() + private val writeLock = Any() + + init { + bootstrap.forEach { items[it.id] = it } + } + + override fun get(id: String): FeatureFlag? = items[id] + + override fun getAll(): Collection = items.values.toList() + + override fun upsert(flag: FeatureFlag) { + val event: FlagValueChangedEvent? = synchronized(writeLock) { + computeEventAndStore(flag) + } + + if (event != null) { + listeners.forEach { it.onChange(event) } + } + } + + /** + * Bulk upsert under a single monitor — used by the polling path which receives an N-flag + * snapshot per response. The pre-existing per-flag [upsert] enters the writeLock N times + * (N monitor enters + N exits); this variant enters once. Change events are still raised + * one-per-flag, *outside* the lock, so listener latency cannot stall the writer thread. + * + * Listener-throw semantics: a `forEach` callback that throws unwinds out of both the + * inner `listeners.forEach` AND the outer `for (event in events)` loop, so subsequent + * events for the batch will not fire listeners. This matches the legacy + * `response.flags.forEach { store.upsert(it) }` polling-caller behavior (same unwind), + * but with a directional improvement: under the legacy path, a listener throwing on + * event K aborted upserts K+1..N as well; under this bulk path, ALL writes are committed + * under the lock BEFORE any listener fires, so the store reaches its post-batch state + * regardless of listener throws downstream. Listeners that need exception isolation + * should wrap their own onChange bodies. + */ + override fun upsertAll(flags: Collection) { + if (flags.isEmpty()) return + // Collect events under the write lock so the "compute change event then store" + // sequence stays atomic per flag, matching single-upsert semantics. + val events: List = synchronized(writeLock) { + val collected = ArrayList(flags.size) + for (flag in flags) { + val event = computeEventAndStore(flag) + if (event != null) collected += event + } + collected + } + + if (events.isEmpty() || listeners.isEmpty()) return + // forEach listener × forEach event would be O(L*E) monitor-light callbacks; we accept + // that cost rather than re-snapshotting listeners per event. + for (event in events) { + listeners.forEach { it.onChange(event) } + } + } + + private fun computeEventAndStore(flag: FeatureFlag): FlagValueChangedEvent? { + val existing = items[flag.id] + val change = when { + existing == null -> FlagValueChangedEvent(flag.id, null, flag.variation) + existing.variation != flag.variation -> + FlagValueChangedEvent(flag.id, existing.variation, flag.variation) + else -> null + } + items[flag.id] = flag + return change + } + + override fun addChangeListener(listener: FlagChangeListener) { + listeners.addIfAbsent(listener) + } + + override fun removeChangeListener(listener: FlagChangeListener) { + listeners.remove(listener) + } +} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/changetracker/FlagTrackerImpl.kt b/featbit-client/src/main/kotlin/co/featbit/client/app/FlagTrackerImpl.kt similarity index 60% rename from featbit-client/src/main/kotlin/co/featbit/client/changetracker/FlagTrackerImpl.kt rename to featbit-client/src/main/kotlin/co/featbit/client/app/FlagTrackerImpl.kt index 3e571e9..c5aa752 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/changetracker/FlagTrackerImpl.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/app/FlagTrackerImpl.kt @@ -1,8 +1,9 @@ -package co.featbit.client.changetracker +package co.featbit.client.app +import co.featbit.client.changetracker.FlagTracker +import co.featbit.client.domain.MemoryStore import co.featbit.client.store.FlagChangeListener import co.featbit.client.store.FlagValueChangedEvent -import co.featbit.client.store.MemoryStore import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.asSharedFlow @@ -32,11 +33,29 @@ internal class FlagTrackerImpl( } private fun dispatch(event: FlagValueChangedEvent) { - subscribers.forEach { it.onChange(event) } - keyedSubscribers[event.key]?.forEach { it.onChange(event) } + // Each subscriber callback is isolated in its own try/catch so a single throwing + // subscriber cannot starve subsequent subscribers from receiving the event. Without + // this, `CopyOnWriteArrayList.forEach` halts on the first exception — silently losing + // delivery to every later-registered subscriber for that event. That's a data-loss + // bug for a callback-style API where one consumer can't be allowed to break others. + subscribers.forEach { safeNotify(it, event) } + keyedSubscribers[event.key]?.forEach { safeNotify(it, event) } _flagChanges.tryEmit(event) } + private fun safeNotify(listener: FlagChangeListener, event: FlagValueChangedEvent) { + try { + listener.onChange(event) + } catch (t: Throwable) { + // No injected logger here; fall back to stderr like `DefaultLogger.write` does + // when android.util.Log isn't available. Swallow the throw so iteration continues. + System.err.println( + "[FeatBit] FlagTracker subscriber threw for event ${event.key}: ${t.message}", + ) + t.printStackTrace(System.err) + } + } + override fun subscribe(listener: FlagChangeListener) { subscribers.addIfAbsent(listener) } diff --git a/featbit-client/src/main/kotlin/co/featbit/client/LifecycleController.kt b/featbit-client/src/main/kotlin/co/featbit/client/app/LifecycleController.kt similarity index 96% rename from featbit-client/src/main/kotlin/co/featbit/client/LifecycleController.kt rename to featbit-client/src/main/kotlin/co/featbit/client/app/LifecycleController.kt index 2a02f8e..303e32c 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/LifecycleController.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/app/LifecycleController.kt @@ -1,6 +1,6 @@ -package co.featbit.client +package co.featbit.client.app -import co.featbit.client.datasynchronizer.DataSynchronizer +import co.featbit.client.data.sync.DataSynchronizer import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.delay diff --git a/featbit-client/src/main/kotlin/co/featbit/client/internal/ConnectionToken.kt b/featbit-client/src/main/kotlin/co/featbit/client/data/http/ConnectionToken.kt similarity index 97% rename from featbit-client/src/main/kotlin/co/featbit/client/internal/ConnectionToken.kt rename to featbit-client/src/main/kotlin/co/featbit/client/data/http/ConnectionToken.kt index 3b3e784..b62393b 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/internal/ConnectionToken.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/data/http/ConnectionToken.kt @@ -1,4 +1,4 @@ -package co.featbit.client.internal +package co.featbit.client.data.http /** * Builds the short-lived connection token FeatBit's streaming endpoint expects on the diff --git a/featbit-client/src/main/kotlin/co/featbit/client/data/http/FBEndpoints.kt b/featbit-client/src/main/kotlin/co/featbit/client/data/http/FBEndpoints.kt new file mode 100644 index 0000000..56413ae --- /dev/null +++ b/featbit-client/src/main/kotlin/co/featbit/client/data/http/FBEndpoints.kt @@ -0,0 +1,40 @@ +package co.featbit.client.data.http + +import co.featbit.client.options.FBOptions +import okhttp3.HttpUrl +import okhttp3.HttpUrl.Companion.toHttpUrl + +/** + * Resolves every FeatBit endpoint the SDK talks to from an [FBOptions] instance. + * + * Centralizing URL construction here removes the three near-identical + * `pollingUri.toHttpUrl().newBuilder().addPathSegments(...)` snippets that previously + * lived inside [GetUserFlags], [HttpTrackInsight], and the streaming synchronizer. + * + * URLs are parsed eagerly so a malformed configuration fails at SDK init rather than on + * the first network call. + */ +internal class FBEndpoints private constructor( + val latestAll: HttpUrl, + val insightTrack: HttpUrl, + val streaming: HttpUrl, +) { + companion object { + fun from(options: FBOptions): FBEndpoints { + val latestAll = options.pollingUri.toHttpUrl().newBuilder() + .addPathSegments(HttpConstants.LATEST_ALL_PATH) + .build() + val insightTrack = options.eventUri.toHttpUrl().newBuilder() + .addPathSegments(HttpConstants.INSIGHT_TRACK_PATH) + .build() + val streaming = options.streamingUri.toStreamingHttpUrl() + return FBEndpoints(latestAll, insightTrack, streaming) + } + + /** Accepts `ws(s)://` (or `http(s)://`) and returns the `/streaming` HTTP(S) URL OkHttp uses. */ + private fun String.toStreamingHttpUrl(): HttpUrl = + replaceFirst(Regex("^ws"), "http").toHttpUrl().newBuilder() + .addPathSegment("streaming") + .build() + } +} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/internal/FbApiClient.kt b/featbit-client/src/main/kotlin/co/featbit/client/data/http/FbApiClient.kt similarity index 80% rename from featbit-client/src/main/kotlin/co/featbit/client/internal/FbApiClient.kt rename to featbit-client/src/main/kotlin/co/featbit/client/data/http/FbApiClient.kt index 457eb89..8e4bca8 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/internal/FbApiClient.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/data/http/FbApiClient.kt @@ -1,4 +1,4 @@ -package co.featbit.client.internal +package co.featbit.client.data.http import co.featbit.client.options.FBOptions import kotlinx.coroutines.Dispatchers @@ -73,7 +73,16 @@ internal abstract class FbApiClient( private val JSON_MEDIA_TYPE = "application/json; charset=utf-8".toMediaType() - /** Shared JSON configuration matching the .NET SDK's "Web" defaults (camelCase, lenient). */ + /** + * Shared JSON configuration matching the .NET SDK's "Web" defaults (camelCase, lenient). + * + * `explicitNulls = false` is marked experimental in kotlinx-serialization 1.6 — the + * opt-in below acknowledges that we knowingly depend on its decoding semantics + * (notably: JSON null on a non-nullable field with a default value throws + * `SerializationException`, which `GetUserFlags.LatestAllEnvelope.data` relies on to + * surface malformed `{"data": null}` payloads as errors instead of silently empty). + */ + @OptIn(kotlinx.serialization.ExperimentalSerializationApi::class) val json: Json = Json { ignoreUnknownKeys = true encodeDefaults = true diff --git a/featbit-client/src/main/kotlin/co/featbit/client/data/http/GetUserFlags.kt b/featbit-client/src/main/kotlin/co/featbit/client/data/http/GetUserFlags.kt new file mode 100644 index 0000000..b6dc83a --- /dev/null +++ b/featbit-client/src/main/kotlin/co/featbit/client/data/http/GetUserFlags.kt @@ -0,0 +1,87 @@ +package co.featbit.client.data.http + +import co.featbit.client.wire.EndUser +import co.featbit.client.model.FBUser +import co.featbit.client.model.FeatureFlag +import co.featbit.client.options.FBOptions +import kotlinx.serialization.Serializable +import okhttp3.OkHttpClient + +/** + * Outcome of a `latest-all` poll. A 401 is fatal (bad secret) and stops polling; any other + * non-2xx is a transient error that is retried on the next interval. + */ +internal data class GetUserFlagsResponse( + val statusCode: Int, + val flags: List, +) { + val isError: Boolean get() = statusCode != 200 + val isFatal: Boolean get() = statusCode == 401 + + companion object { + fun ok(flags: List): GetUserFlagsResponse = GetUserFlagsResponse(200, flags) + fun error(statusCode: Int): GetUserFlagsResponse = GetUserFlagsResponse(statusCode, emptyList()) + } +} + +/** + * Fetches the latest feature flags for a user from the FeatBit evaluation server. + * POSTs the end-user payload to `api/public/sdk/client/latest-all?timestamp=...`. + */ +internal class GetUserFlags( + options: FBOptions, + user: FBUser, + httpClient: OkHttpClient? = null, + endpoints: FBEndpoints = FBEndpoints.from(options), +) : FbApiClient(options, httpClient) { + + private val endpoint = endpoints.latestAll + + private val payload: ByteArray = + json.encodeToString(EndUser.serializer(), user.toEndUser()).encodeToByteArray() + + suspend fun run(timestamp: Long): GetUserFlagsResponse { + val url = endpoint.newBuilder() + .addQueryParameter("timestamp", timestamp.toString()) + .build() + + val result = post(url, payload) + if (!result.isSuccessful) { + return GetUserFlagsResponse.error(result.code) + } + if (result.body.isBlank()) { + return GetUserFlagsResponse.ok(emptyList()) + } + + // Wire shape: `{"data": {"featureFlags": [...]}}`. We deserialize through a typed + // envelope in a single pass — the prior implementation went body → JsonElement AST → + // navigate → decodeFromJsonElement, allocating a full intermediate tree plus + // re-walking it. One pass eliminates the tree allocation entirely. + // + // Field nullability mirrors the old AST behavior exactly: + // * `data` field absent → defaults to LatestAllData() (empty flags). Matches + // the old `?.jsonObject → null → return ok(emptyList())`. + // * `data` field is JSON null → throws SerializationException because `data` is + // non-nullable. The old code threw IllegalArgumentException + // on `JsonNull.jsonObject`; either way `safePoll` + // catches and logs. + // * `data` wrong type → SerializationException. + // * `featureFlags` wrong type → SerializationException. + // + // Quietly returning `emptyList()` on a malformed payload would be indistinguishable + // from "user has no flags" and would make the SDK silently serve defaults forever — + // this is the exact failure mode the throw-on-malformed contract guards against. + // + // Empirical note (kotlinx-serialization 1.6.3 + `explicitNulls=false`): a nullable + // field WITH a null default coerces JSON null → Kotlin null without throwing. That + // would re-introduce the silent-empty bug. Keep `data` non-nullable. + val envelope = json.decodeFromString(LatestAllEnvelope.serializer(), result.body) + return GetUserFlagsResponse.ok(envelope.data.featureFlags) + } + + @Serializable + private data class LatestAllEnvelope(val data: LatestAllData = LatestAllData()) + + @Serializable + private data class LatestAllData(val featureFlags: List = emptyList()) +} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/internal/HttpConstants.kt b/featbit-client/src/main/kotlin/co/featbit/client/data/http/HttpConstants.kt similarity index 87% rename from featbit-client/src/main/kotlin/co/featbit/client/internal/HttpConstants.kt rename to featbit-client/src/main/kotlin/co/featbit/client/data/http/HttpConstants.kt index 720975e..35290cb 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/internal/HttpConstants.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/data/http/HttpConstants.kt @@ -1,4 +1,4 @@ -package co.featbit.client.internal +package co.featbit.client.data.http internal object HttpConstants { const val USER_AGENT: String = "featbit-kotlin-client-sdk" diff --git a/featbit-client/src/main/kotlin/co/featbit/client/data/insights/InsightDispatcher.kt b/featbit-client/src/main/kotlin/co/featbit/client/data/insights/InsightDispatcher.kt new file mode 100644 index 0000000..0acfb07 --- /dev/null +++ b/featbit-client/src/main/kotlin/co/featbit/client/data/insights/InsightDispatcher.kt @@ -0,0 +1,127 @@ +package co.featbit.client.data.insights + +import co.featbit.client.FBLogger +import co.featbit.client.wire.Insight +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job +import kotlinx.coroutines.channels.BufferOverflow +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.launch +import kotlinx.coroutines.withTimeoutOrNull +import java.io.Closeable + +/** + * Non-blocking, bounded, batching pipeline for analytics insight events. + * + * Each evaluation used to do `scope.launch { trackInsight.run(insight) }` — unbounded + * fan-out that grew memory without limit under a slow network or a high evaluation rate. + * + * This dispatcher gives the caller a single non-suspending [offer] (drops the oldest + * queued event on overflow rather than blocking the evaluation thread) and a single + * background consumer that coalesces events into batches of up to [batchSize] or + * [flushIntervalMs], whichever comes first. + * + * Lifecycle: + * * [close] — fire-and-forget; the consumer will drain whatever it can before its parent + * scope is cancelled. May lose buffered events if the parent scope is cancelled + * immediately after. + * * [closeAndDrain] — orderly suspend-shutdown used by `FBClientImpl.close`: waits (bounded) + * for the consumer to flush queued events and for [tracker]`.close()` to complete. + */ +internal class InsightDispatcher( + private val tracker: TrackInsight, + scope: CoroutineScope, + private val logger: FBLogger, + capacity: Int = DEFAULT_CAPACITY, + private val batchSize: Int = DEFAULT_BATCH_SIZE, + private val flushIntervalMs: Long = DEFAULT_FLUSH_INTERVAL_MS, + private val closeFlushTimeoutMs: Long = DEFAULT_CLOSE_FLUSH_TIMEOUT_MS, +) : Closeable { + + private val channel: Channel = Channel( + capacity = capacity, + onBufferOverflow = BufferOverflow.DROP_OLDEST, + onUndeliveredElement = null, + ) + + private val consumerJob: Job = scope.launch { consumeLoop() } + + /** + * Enqueue an insight for asynchronous delivery. Never suspends. If the queue is full, + * the oldest queued event is dropped — see [BufferOverflow.DROP_OLDEST]. With this + * overflow policy [Channel.trySend] never reports failure for an open channel; calls + * placed after [close] are silently no-ops. + */ + fun offer(insight: Insight) { + channel.trySend(insight) + } + + private suspend fun consumeLoop() { + val pending = ArrayList(batchSize) + // Two exit conditions: + // * `closeAndDrain` closes the channel → next `receiveCatching` returns closed → break. + // * The parent scope is cancelled (fire-and-forget `close` path) → `receiveCatching` + // throws `CancellationException` and the coroutine unwinds — also acceptable. + while (true) { + val first = channel.receiveCatching().getOrNull() ?: break + pending += first + + drainAvailable(pending) + if (pending.size < batchSize) { + withTimeoutOrNull(flushIntervalMs) { + while (pending.size < batchSize) { + val next = channel.receiveCatching().getOrNull() ?: return@withTimeoutOrNull + pending += next + } + } + } + + // Defensive copy: the consumer reuses `pending` across iterations, but a tracker + // may legitimately hold the batch reference (logging, retry, async-pipelining) + // beyond its `run` call. Hand it an immutable snapshot. + flush(pending.toList()) + pending.clear() + } + // Channel closed: best-effort flush of any items received after the last batch. + drainAvailable(pending) + if (pending.isNotEmpty()) flush(pending.toList()) + } + + private fun drainAvailable(into: MutableList) { + while (into.size < batchSize) { + val next = channel.tryReceive().getOrNull() ?: break + into += next + } + } + + private suspend fun flush(batch: List) { + try { + tracker.run(batch) + } catch (ex: Exception) { + logger.error("Failed to flush insight batch of size ${batch.size}.", ex) + } + } + + override fun close() { + // Fire-and-forget: parent scope decides how long the consumer has to drain. + channel.close() + } + + /** + * Orderly shutdown: close the input channel and *suspend* until the consumer has flushed + * its queue (bounded by [closeFlushTimeoutMs]), then close the underlying [tracker]. + * Used by `FBClientImpl.close` to give buffered insight events a chance to land. + */ + suspend fun closeAndDrain() { + channel.close() + withTimeoutOrNull(closeFlushTimeoutMs) { consumerJob.join() } + tracker.close() + } + + companion object { + const val DEFAULT_CAPACITY: Int = 256 + const val DEFAULT_BATCH_SIZE: Int = 50 + const val DEFAULT_FLUSH_INTERVAL_MS: Long = 1_000L + const val DEFAULT_CLOSE_FLUSH_TIMEOUT_MS: Long = 2_000L + } +} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/data/insights/TrackInsight.kt b/featbit-client/src/main/kotlin/co/featbit/client/data/insights/TrackInsight.kt new file mode 100644 index 0000000..a1fa254 --- /dev/null +++ b/featbit-client/src/main/kotlin/co/featbit/client/data/insights/TrackInsight.kt @@ -0,0 +1,58 @@ +package co.featbit.client.data.insights + +import co.featbit.client.data.http.FBEndpoints +import co.featbit.client.data.http.FbApiClient +import co.featbit.client.wire.Insight +import co.featbit.client.options.FBOptions +import kotlinx.coroutines.CancellationException +import kotlinx.serialization.KSerializer +import kotlinx.serialization.builtins.ListSerializer +import okhttp3.OkHttpClient + +/** Sends analytics insight events to FeatBit. */ +internal interface TrackInsight { + /** Posts the given [batch] as a single payload. The list must be non-empty. */ + suspend fun run(batch: List) + fun close() +} + +/** Convenience overload — most callers send one insight at a time. */ +internal suspend fun TrackInsight.run(insight: Insight): Unit = run(listOf(insight)) + +/** No-op tracker used in offline mode. */ +internal data object NoopTrackInsight : TrackInsight { + override suspend fun run(batch: List): Unit = Unit + override fun close(): Unit = Unit +} + +/** Default tracker: POSTs an insight array to `api/public/insight/track`. */ +internal class HttpTrackInsight( + options: FBOptions, + httpClient: OkHttpClient? = null, + endpoints: FBEndpoints = FBEndpoints.from(options), +) : FbApiClient(options, httpClient), TrackInsight { + + private val logger = options.logger + private val endpoint = endpoints.insightTrack + + override suspend fun run(batch: List) { + if (batch.isEmpty()) return + try { + val payload = json + .encodeToString(INSIGHT_LIST_SERIALIZER, batch) + .encodeToByteArray() + post(endpoint, payload) + } catch (ce: CancellationException) { + throw ce + } catch (ex: Exception) { + logger.error("Exception occurred while tracking insight.", ex) + } + } + + private companion object { + // ListSerializer wraps an element serializer; building one per send is pure overhead + // since the type never changes. Cache the singleton. + private val INSIGHT_LIST_SERIALIZER: KSerializer> = + ListSerializer(Insight.serializer()) + } +} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/DataSynchronizer.kt b/featbit-client/src/main/kotlin/co/featbit/client/data/sync/DataSynchronizer.kt similarity index 60% rename from featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/DataSynchronizer.kt rename to featbit-client/src/main/kotlin/co/featbit/client/data/sync/DataSynchronizer.kt index a1a4764..8b32c2f 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/DataSynchronizer.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/data/sync/DataSynchronizer.kt @@ -1,4 +1,4 @@ -package co.featbit.client.datasynchronizer +package co.featbit.client.data.sync import java.io.Closeable @@ -27,4 +27,16 @@ internal interface DataSynchronizer : Closeable { * repeatedly. Default: no-op. */ fun resume() {} + + /** + * Orderly shutdown: signal stop, then suspend until every in-flight upsert / network call + * has completed. Use this from [FBClientImpl.identify] so a late polling response from the + * previous user cannot land in the store *after* the user-swap. + * + * The non-suspending [close] is the fire-and-forget fallback for `Closeable` semantics; + * implementations should make it forward to [closeAndJoin] on a best-effort basis. + */ + suspend fun closeAndJoin() { + close() + } } diff --git a/featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/NullDataSynchronizer.kt b/featbit-client/src/main/kotlin/co/featbit/client/data/sync/NullDataSynchronizer.kt similarity index 85% rename from featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/NullDataSynchronizer.kt rename to featbit-client/src/main/kotlin/co/featbit/client/data/sync/NullDataSynchronizer.kt index d071631..c99e529 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/NullDataSynchronizer.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/data/sync/NullDataSynchronizer.kt @@ -1,4 +1,4 @@ -package co.featbit.client.datasynchronizer +package co.featbit.client.data.sync /** A no-op synchronizer used in offline mode; always reports as initialized. */ internal class NullDataSynchronizer : DataSynchronizer { diff --git a/featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/PollingDataSynchronizer.kt b/featbit-client/src/main/kotlin/co/featbit/client/data/sync/PollingDataSynchronizer.kt similarity index 69% rename from featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/PollingDataSynchronizer.kt rename to featbit-client/src/main/kotlin/co/featbit/client/data/sync/PollingDataSynchronizer.kt index 0367ddd..e5e7bc2 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/PollingDataSynchronizer.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/data/sync/PollingDataSynchronizer.kt @@ -1,17 +1,19 @@ -package co.featbit.client.datasynchronizer +package co.featbit.client.data.sync -import co.featbit.client.internal.GetUserFlags +import co.featbit.client.data.http.FBEndpoints +import co.featbit.client.data.http.GetUserFlags import co.featbit.client.model.FBUser import co.featbit.client.options.FBOptions -import co.featbit.client.store.MemoryStore +import co.featbit.client.domain.MemoryStore import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancel +import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.delay -import kotlinx.coroutines.isActive import kotlinx.coroutines.launch import java.util.concurrent.atomic.AtomicBoolean import kotlin.time.Duration @@ -26,7 +28,8 @@ internal class PollingDataSynchronizer( user: FBUser, private val store: MemoryStore, private val scope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.IO), - private val getUserFlags: GetUserFlags = GetUserFlags(options, user), + endpoints: FBEndpoints = FBEndpoints.from(options), + private val getUserFlags: GetUserFlags = GetUserFlags(options, user, endpoints = endpoints), ) : DataSynchronizer { private val logger = options.logger @@ -35,6 +38,8 @@ internal class PollingDataSynchronizer( private val startTask = CompletableDeferred() private val initializedFlag = AtomicBoolean(false) + @Volatile + private var loopJob: Job? = null @Volatile private var timestamp: Long = 0 @@ -42,21 +47,15 @@ internal class PollingDataSynchronizer( override val initialized: Boolean get() = initializedFlag.get() override suspend fun start(): Boolean { - scope.launch { pollingLoop() } + loopJob = scope.launch { pollingLoop() } return startTask.await() } private suspend fun pollingLoop() { - while (scope.isActive) { + while (true) { safePoll() - try { - logger.debug { "Waiting for the next polling interval of $pollingInterval." } - delay(pollingInterval) - } catch (_: CancellationException) { - throw CancellationException() - } catch (ex: Exception) { - logger.error("An unexpected error occurred while waiting for the next polling interval.", ex) - } + logger.debug { "Waiting for the next polling interval of $pollingInterval." } + delay(pollingInterval) } } @@ -81,7 +80,9 @@ internal class PollingDataSynchronizer( timestamp = System.currentTimeMillis() logger.debug { "Polling received ${response.flags.size} flags." } - response.flags.forEach { store.upsert(it) } + // Bulk upsert: a 200-flag snapshot used to enter the store's write monitor 200x; + // upsertAll takes the lock once for the writes, then drains change events outside. + store.upsertAll(response.flags) if (initializedFlag.compareAndSet(false, true)) { startTask.complete(true) @@ -100,4 +101,17 @@ internal class PollingDataSynchronizer( // Ensure a never-initialized synchronizer doesn't leave start() suspended forever. startTask.complete(false) } + + /** + * Orderly shutdown: cancel the polling loop and *await* its termination so any in-flight + * `safePoll` (mid-`store.upsert`) has finished before this returns. Used by + * `FBClientImpl.identify` to guarantee no stale upsert from the previous user lands after + * the swap. + */ + override suspend fun closeAndJoin() { + loopJob?.cancelAndJoin() + scope.cancel() + getUserFlags.close() + startTask.complete(false) + } } diff --git a/featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/StreamingDataSynchronizer.kt b/featbit-client/src/main/kotlin/co/featbit/client/data/sync/StreamingDataSynchronizer.kt similarity index 67% rename from featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/StreamingDataSynchronizer.kt rename to featbit-client/src/main/kotlin/co/featbit/client/data/sync/StreamingDataSynchronizer.kt index f1d95d2..abaf817 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/datasynchronizer/StreamingDataSynchronizer.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/data/sync/StreamingDataSynchronizer.kt @@ -1,25 +1,27 @@ -package co.featbit.client.datasynchronizer +package co.featbit.client.data.sync -import co.featbit.client.internal.ConnectionToken -import co.featbit.client.model.EndUser +import co.featbit.client.data.http.ConnectionToken +import co.featbit.client.data.http.FBEndpoints +import co.featbit.client.wire.EndUser import co.featbit.client.model.FBUser import co.featbit.client.model.FeatureFlag import co.featbit.client.options.FBOptions -import co.featbit.client.store.MemoryStore +import co.featbit.client.domain.MemoryStore import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancel +import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.delay import kotlinx.coroutines.isActive +import kotlinx.coroutines.job import kotlinx.coroutines.launch import kotlinx.serialization.SerialName import kotlinx.serialization.Serializable import kotlinx.serialization.json.Json import kotlinx.serialization.json.JsonElement -import okhttp3.HttpUrl.Companion.toHttpUrl import okhttp3.OkHttpClient import okhttp3.Request import okhttp3.Response @@ -44,14 +46,20 @@ internal class StreamingDataSynchronizer( private val user: FBUser, private val store: MemoryStore, private val scope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.IO), - private val httpClient: OkHttpClient = OkHttpClient.Builder() - .pingInterval(20, TimeUnit.SECONDS) - .build(), + httpClient: OkHttpClient? = null, + endpoints: FBEndpoints = FBEndpoints.from(options), ) : DataSynchronizer { private val logger = options.logger private val secret = options.secret - private val streamingEndpoint = options.streamingUri.toStreamingHttpUrl() + private val streamingEndpoint = endpoints.streaming + + // Ownership tracking mirrors `FbApiClient`: when the caller supplies a client, we leave its + // lifecycle to them. When we constructed our own, we must release its dispatcher's worker + // threads + connection pool on teardown — otherwise each identify() cycle leaks an + // OkHttpClient (its idle threads + WS connection in the pool) until OkHttp's own GC kicks in. + private val ownsClient: Boolean = httpClient == null + private val httpClient: OkHttpClient = httpClient ?: defaultHttpClient() private val startTask = CompletableDeferred() private val initializedFlag = AtomicBoolean(false) @@ -97,6 +105,9 @@ internal class StreamingDataSynchronizer( } override fun onMessage(webSocket: WebSocket, text: String) { + // Drop late messages that arrive after close()/closeAndJoin() — otherwise an + // upsert from a previous user could land in the store after the user-swap. + if (closed) return scope.launch { handleMessage(text) } } @@ -133,11 +144,18 @@ internal class StreamingDataSynchronizer( private fun handleMessage(text: String) { try { + // Two-pass decode (envelope → DataSyncPayload) is structurally needed because the + // server multiplexes message types over the same socket (data-sync, ping, ...). + // The second `decodeFromJsonElement` walks the AST a second time only when the + // envelope actually carries data; ping/other types early-return before that cost. + // A custom polymorphic serializer could collapse it to one pass — deferred until + // streaming payloads dominate a profile. val envelope = StreamingJson.decodeFromString(ServerEnvelope.serializer(), text) if (envelope.messageType != "data-sync" || envelope.data == null) return val payload = StreamingJson.decodeFromJsonElement(DataSyncPayload.serializer(), envelope.data) - payload.featureFlags.forEach(store::upsert) + // Bulk variant: one write-monitor enter for the whole batch instead of one per flag. + store.upsertAll(payload.featureFlags) timestamp = System.currentTimeMillis() if (initializedFlag.compareAndSet(false, true)) { @@ -188,9 +206,41 @@ internal class StreamingDataSynchronizer( webSocket?.close(NORMAL_CLOSURE, null) webSocket = null scope.cancel() + releaseHttpClient() startTask.complete(false) } + /** + * Orderly shutdown: close the WebSocket, cancel every child coroutine on [scope], and + * *await* their completion before returning. Used by `FBClientImpl.identify` so an upsert + * from a `data-sync` message in flight at swap time cannot land in the store under the + * new user. + * + * Implementation note: we cancel the scope's parent [SupervisorJob] with `cancelAndJoin` + * — the idiomatic Kotlin primitive for "stop everything on this scope and wait." Once the + * Job moves to CANCELLING, any subsequent `scope.launch` from OkHttp's dispatcher (a late + * `onOpen → startHeartbeat`, a delayed `scheduleReconnect`) returns an already-cancelled + * Job whose body never executes. A `children.toList() + joinAll(...)` snapshot would + * race here: a child spawning between snapshot and join would escape the join. + */ + override suspend fun closeAndJoin() { + closed = true + heartbeatJob?.cancel() + webSocket?.close(NORMAL_CLOSURE, null) + webSocket = null + scope.coroutineContext.job.cancelAndJoin() + releaseHttpClient() + startTask.complete(false) + } + + private fun releaseHttpClient() { + if (!ownsClient) return + // Mirrors `FbApiClient.close()` — shut down the dispatcher's executor (signals worker + // threads to exit when idle) and evict pooled connections immediately. + httpClient.dispatcher.executorService.shutdown() + httpClient.connectionPool.evictAll() + } + @Serializable private class ClientMessage(val messageType: String, val data: T) @@ -216,8 +266,8 @@ internal class StreamingDataSynchronizer( val StreamingJson = Json { ignoreUnknownKeys = true; encodeDefaults = true } - /** Accepts `ws(s)://` (or `http(s)://`) and returns the `/streaming` HTTP(S) URL OkHttp uses. */ - fun String.toStreamingHttpUrl() = - replaceFirst(Regex("^ws"), "http").toHttpUrl().newBuilder().addPathSegment("streaming").build() + private fun defaultHttpClient(): OkHttpClient = OkHttpClient.Builder() + .pingInterval(20, TimeUnit.SECONDS) + .build() } } diff --git a/featbit-client/src/main/kotlin/co/featbit/client/domain/EvalResult.kt b/featbit-client/src/main/kotlin/co/featbit/client/domain/EvalResult.kt new file mode 100644 index 0000000..d8db251 --- /dev/null +++ b/featbit-client/src/main/kotlin/co/featbit/client/domain/EvalResult.kt @@ -0,0 +1,25 @@ +package co.featbit.client.domain + +import co.featbit.client.model.FeatureFlag + +/** + * Internal outcome of looking a flag up in the store, prior to type conversion. + * + * Modeled as a sealed hierarchy so callers branch on the case rather than threading a + * `Pair` plus a stringly-typed `isValid` boolean. The two + * [Found.reason] / [NotFound.reason] strings match the .NET SDK's wire-compatible reasons. + */ +internal sealed interface EvalResult { + val reason: String + + /** The flag was found; carry the raw variation string and the source [flag]. */ + data class Found(val flag: FeatureFlag) : EvalResult { + override val reason: String get() = flag.matchReason + val value: String get() = flag.variation + } + + /** The caller provided a key that did not match any known flag. */ + data object NotFound : EvalResult { + override val reason: String = "flag not found" + } +} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/domain/Evaluator.kt b/featbit-client/src/main/kotlin/co/featbit/client/domain/Evaluator.kt new file mode 100644 index 0000000..9c159b9 --- /dev/null +++ b/featbit-client/src/main/kotlin/co/featbit/client/domain/Evaluator.kt @@ -0,0 +1,7 @@ +package co.featbit.client.domain + +/** Resolves a feature flag from the store and returns a typed [EvalResult]. */ +internal class Evaluator(private val store: MemoryStore) { + fun evaluate(key: String): EvalResult = + store.get(key)?.let { EvalResult.Found(it) } ?: EvalResult.NotFound +} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/domain/MemoryStore.kt b/featbit-client/src/main/kotlin/co/featbit/client/domain/MemoryStore.kt new file mode 100644 index 0000000..30dbaa2 --- /dev/null +++ b/featbit-client/src/main/kotlin/co/featbit/client/domain/MemoryStore.kt @@ -0,0 +1,44 @@ +package co.featbit.client.domain + +import co.featbit.client.model.FeatureFlag +import co.featbit.client.store.FlagChangeListener + +/** + * A thread-safe in-memory store holding the feature flag data received by the SDK. + * + * Default-method note: under the module's `-Xjvm-default=all-compatibility` compiler + * flag, default-method bodies on this interface land directly in the interface bytecode + * (real Java 8 defaults) with a synthetic `DefaultImpls` class as a fallback for legacy + * binary implementers compiled against an older version of this interface. Adding a new + * default method (e.g. [upsertAll]) is binary-compatible. + */ +public interface MemoryStore { + /** Returns the flag with the given [id], or `null` if unknown. */ + public fun get(id: String): FeatureFlag? + + /** Returns a snapshot of all flags currently in the store. */ + public fun getAll(): Collection + + /** Inserts or updates [flag], raising a change event if its value changed. */ + public fun upsert(flag: FeatureFlag) + + /** + * Bulk variant of [upsert]. The default implementation iterates [upsert], preserving the + * legacy "write-then-notify per flag" ordering. Adapters MAY override to batch writes + * under a single lock; in that case change events fire AFTER all writes complete, which + * gives listeners a consistent post-batch snapshot rather than interleaved partial state. + * Both orderings are valid — callers must not rely on which one they see. + * + * Change events are still raised one-per-flag (with original key/old/new values), + * irrespective of batching. + */ + public fun upsertAll(flags: Collection) { + for (flag in flags) upsert(flag) + } + + /** Registers a listener invoked whenever a flag value changes. */ + public fun addChangeListener(listener: FlagChangeListener) + + /** Removes a previously registered change listener. */ + public fun removeChangeListener(listener: FlagChangeListener) +} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/evaluation/ValueConverters.kt b/featbit-client/src/main/kotlin/co/featbit/client/domain/ValueConverters.kt similarity index 57% rename from featbit-client/src/main/kotlin/co/featbit/client/evaluation/ValueConverters.kt rename to featbit-client/src/main/kotlin/co/featbit/client/domain/ValueConverters.kt index 0701c2c..2c233c1 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/evaluation/ValueConverters.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/domain/ValueConverters.kt @@ -1,4 +1,4 @@ -package co.featbit.client.evaluation +package co.featbit.client.domain /** * Converts the string variation value stored for a flag into a typed value. @@ -11,9 +11,15 @@ internal typealias ValueConverter = (String) -> T? internal object ValueConverters { val bool: ValueConverter = { value -> - when (value.trim().lowercase()) { - "true" -> true - "false" -> false + // `value.trim().lowercase()` allocated two strings per check — `lowercase()` + // unconditionally allocates when there is any uppercase letter. `equals(_, ignoreCase=true)` + // compares without allocating, and the prior `trim()` handled exterior whitespace + // which we keep with a single trim call. For clean server data both branches are O(1) + // string comparisons. + val trimmed = value.trim() + when { + trimmed.equals("true", ignoreCase = true) -> true + trimmed.equals("false", ignoreCase = true) -> false else -> null } } diff --git a/featbit-client/src/main/kotlin/co/featbit/client/evaluation/EvalResult.kt b/featbit-client/src/main/kotlin/co/featbit/client/evaluation/EvalResult.kt deleted file mode 100644 index 479679a..0000000 --- a/featbit-client/src/main/kotlin/co/featbit/client/evaluation/EvalResult.kt +++ /dev/null @@ -1,19 +0,0 @@ -package co.featbit.client.evaluation - -import co.featbit.client.model.FeatureFlag - -/** - * Internal result of looking a flag up in the store, prior to type conversion. - */ -internal class EvalResult private constructor( - val isValid: Boolean, - val reason: String, - val value: String, -) { - companion object { - /** The caller provided a key that did not match any known flag. */ - val FlagNotFound: EvalResult = EvalResult(false, "flag not found", "") - - fun of(flag: FeatureFlag): EvalResult = EvalResult(true, flag.matchReason, flag.variation) - } -} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/evaluation/Evaluator.kt b/featbit-client/src/main/kotlin/co/featbit/client/evaluation/Evaluator.kt deleted file mode 100644 index ef6a0eb..0000000 --- a/featbit-client/src/main/kotlin/co/featbit/client/evaluation/Evaluator.kt +++ /dev/null @@ -1,15 +0,0 @@ -package co.featbit.client.evaluation - -import co.featbit.client.model.FeatureFlag -import co.featbit.client.store.MemoryStore - -/** - * Resolves a feature flag from the store, returning the lookup [EvalResult] alongside the - * matched [FeatureFlag] (or `null` when the flag is unknown). - */ -internal class Evaluator(private val store: MemoryStore) { - fun evaluate(key: String): Pair { - val flag = store.get(key) ?: return EvalResult.FlagNotFound to null - return EvalResult.of(flag) to flag - } -} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/internal/GetUserFlags.kt b/featbit-client/src/main/kotlin/co/featbit/client/internal/GetUserFlags.kt deleted file mode 100644 index 7b55265..0000000 --- a/featbit-client/src/main/kotlin/co/featbit/client/internal/GetUserFlags.kt +++ /dev/null @@ -1,69 +0,0 @@ -package co.featbit.client.internal - -import co.featbit.client.model.EndUser -import co.featbit.client.model.FBUser -import co.featbit.client.model.FeatureFlag -import co.featbit.client.options.FBOptions -import kotlinx.serialization.builtins.ListSerializer -import kotlinx.serialization.encodeToString -import kotlinx.serialization.json.jsonObject -import okhttp3.HttpUrl -import okhttp3.HttpUrl.Companion.toHttpUrl -import okhttp3.OkHttpClient - -/** - * Outcome of a `latest-all` poll. A 401 is fatal (bad secret) and stops polling; any other - * non-2xx is a transient error that is retried on the next interval. - */ -internal data class GetUserFlagsResponse( - val statusCode: Int, - val flags: List, -) { - val isError: Boolean get() = statusCode != 200 - val isFatal: Boolean get() = statusCode == 401 - - companion object { - fun ok(flags: List): GetUserFlagsResponse = GetUserFlagsResponse(200, flags) - fun error(statusCode: Int): GetUserFlagsResponse = GetUserFlagsResponse(statusCode, emptyList()) - } -} - -/** - * Fetches the latest feature flags for a user from the FeatBit evaluation server. - * POSTs the end-user payload to `api/public/sdk/client/latest-all?timestamp=...`. - */ -internal class GetUserFlags( - options: FBOptions, - user: FBUser, - httpClient: OkHttpClient? = null, -) : FbApiClient(options, httpClient) { - - private val endpoint: HttpUrl = options.pollingUri.toHttpUrl().newBuilder() - .addPathSegments(HttpConstants.LATEST_ALL_PATH) - .build() - - private val payload: ByteArray = - json.encodeToString(EndUser.serializer(), user.toEndUser()).encodeToByteArray() - - suspend fun run(timestamp: Long): GetUserFlagsResponse { - val url = endpoint.newBuilder() - .addQueryParameter("timestamp", timestamp.toString()) - .build() - - val result = post(url, payload) - if (!result.isSuccessful) { - return GetUserFlagsResponse.error(result.code) - } - if (result.body.isBlank()) { - return GetUserFlagsResponse.ok(emptyList()) - } - - val featureFlags = json.parseToJsonElement(result.body) - .jsonObject["data"]?.jsonObject - ?.get("featureFlags") - ?.let { json.decodeFromJsonElement(ListSerializer(FeatureFlag.serializer()), it) } - ?: emptyList() - - return GetUserFlagsResponse.ok(featureFlags) - } -} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/internal/TrackInsight.kt b/featbit-client/src/main/kotlin/co/featbit/client/internal/TrackInsight.kt deleted file mode 100644 index a78e071..0000000 --- a/featbit-client/src/main/kotlin/co/featbit/client/internal/TrackInsight.kt +++ /dev/null @@ -1,47 +0,0 @@ -package co.featbit.client.internal - -import co.featbit.client.model.Insight -import co.featbit.client.options.FBOptions -import kotlinx.coroutines.CancellationException -import kotlinx.serialization.builtins.ListSerializer -import okhttp3.HttpUrl -import okhttp3.HttpUrl.Companion.toHttpUrl -import okhttp3.OkHttpClient - -/** Sends analytics insight events to FeatBit. */ -internal interface TrackInsight { - suspend fun run(insight: Insight) - fun close() -} - -/** No-op tracker used in offline mode. */ -internal object NoopTrackInsight : TrackInsight { - override suspend fun run(insight: Insight) {} - override fun close() {} -} - -/** Default tracker: POSTs a single-element insight array to `api/public/insight/track`. */ -internal class HttpTrackInsight( - options: FBOptions, - httpClient: OkHttpClient? = null, -) : FbApiClient(options, httpClient), TrackInsight { - - private val logger = options.logger - - private val endpoint: HttpUrl = options.eventUri.toHttpUrl().newBuilder() - .addPathSegments(HttpConstants.INSIGHT_TRACK_PATH) - .build() - - override suspend fun run(insight: Insight) { - try { - val payload = json - .encodeToString(ListSerializer(Insight.serializer()), listOf(insight)) - .encodeToByteArray() - post(endpoint, payload) - } catch (ce: CancellationException) { - throw ce - } catch (ex: Exception) { - logger.error("Exception occurred while tracking insight.", ex) - } - } -} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/model/FBUser.kt b/featbit-client/src/main/kotlin/co/featbit/client/model/FBUser.kt index af2f358..8188b2d 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/model/FBUser.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/model/FBUser.kt @@ -1,5 +1,8 @@ package co.featbit.client.model +import co.featbit.client.wire.CustomizedProperty +import co.featbit.client.wire.EndUser + /** * Defines the attributes of a user for whom feature flags are evaluated. * @@ -21,12 +24,24 @@ public class FBUser internal constructor( public val name: String, public val custom: Map, ) { - internal fun toEndUser(): EndUser = EndUser( + // FBUser is immutable, so its wire representation is too. Building it once at construction + // (rather than per evaluation insight) eliminates a per-flag-check allocation of EndUser + + // CustomizedProperty list + the singleton-list wrapper + N CustomizedProperty entries for + // an N-custom-attribute user (so ~4 objects per evaluation just for the wire form). + // + // The list is wrapped in `unmodifiableList` so SDK-internal code can't `as MutableList` + // and corrupt the cached payload — this cache is per-FBUser-lifetime; in-place mutation + // would silently break every subsequent insight + polling request for that user. + private val endUser: EndUser = EndUser( keyId = key, name = name, - customizedProperties = custom.map { (k, v) -> CustomizedProperty(k, v) }, + customizedProperties = java.util.Collections.unmodifiableList( + custom.map { (k, v) -> CustomizedProperty(k, v) }, + ), ) + internal fun toEndUser(): EndUser = endUser + /** Fluent builder for [FBUser]. */ public class Builder(private val key: String) { private var name: String = "" diff --git a/featbit-client/src/main/kotlin/co/featbit/client/options/FBOptions.kt b/featbit-client/src/main/kotlin/co/featbit/client/options/FBOptions.kt index 1587f34..b5de853 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/options/FBOptions.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/options/FBOptions.kt @@ -87,18 +87,31 @@ public class FBOptions internal constructor( /** Sets the logger used by the SDK. Defaults to a [DefaultLogger]. */ public fun logger(logger: FBLogger): Builder = apply { this.logger = logger } - public fun build(): FBOptions = FBOptions( - offline = offline, - bootstrap = bootstrap, - secret = secret, - dataSyncMode = dataSyncMode, - pollingUri = pollingUri, - pollingInterval = pollingInterval, - streamingUri = streamingUri, - eventUri = eventUri, - backgroundGracePeriod = backgroundGracePeriod, - logger = logger, - ) + public fun build(): FBOptions { + // Validate eagerly so misconfiguration fails at SDK init, not on the first network call. + if (!offline) { + require(secret.isNotBlank()) { "FBOptions.secret must not be blank when offline=false" } + require(pollingUri.isNotBlank()) { "polling URI must not be blank" } + require(eventUri.isNotBlank()) { "event URI must not be blank" } + if (dataSyncMode == DataSyncMode.Streaming) { + require(streamingUri.isNotBlank()) { "streaming URI must not be blank" } + } + } + require(pollingInterval.isPositive()) { "pollingInterval must be positive" } + require(!backgroundGracePeriod.isNegative()) { "backgroundGracePeriod must not be negative" } + return FBOptions( + offline = offline, + bootstrap = bootstrap, + secret = secret, + dataSyncMode = dataSyncMode, + pollingUri = pollingUri, + pollingInterval = pollingInterval, + streamingUri = streamingUri, + eventUri = eventUri, + backgroundGracePeriod = backgroundGracePeriod, + logger = logger, + ) + } public companion object { private const val DEFAULT_URI: String = "http://localhost:5100" diff --git a/featbit-client/src/main/kotlin/co/featbit/client/store/DefaultMemoryStore.kt b/featbit-client/src/main/kotlin/co/featbit/client/store/DefaultMemoryStore.kt deleted file mode 100644 index ef912ce..0000000 --- a/featbit-client/src/main/kotlin/co/featbit/client/store/DefaultMemoryStore.kt +++ /dev/null @@ -1,55 +0,0 @@ -package co.featbit.client.store - -import co.featbit.client.model.FeatureFlag -import java.util.concurrent.ConcurrentHashMap -import java.util.concurrent.CopyOnWriteArrayList - -/** - * Default [MemoryStore] backed by a [ConcurrentHashMap]. - * - * Reads are lock-free. Writes are serialized under a single monitor so that the - * "compute change event then store" sequence is atomic, matching the .NET - * `DefaultMemoryStore`. Change listeners are notified outside the write lock. - */ -public class DefaultMemoryStore( - bootstrap: List = emptyList(), -) : MemoryStore { - - private val items: ConcurrentHashMap = ConcurrentHashMap() - private val listeners: CopyOnWriteArrayList = CopyOnWriteArrayList() - private val writeLock = Any() - - init { - bootstrap.forEach { items[it.id] = it } - } - - override fun get(id: String): FeatureFlag? = items[id] - - override fun getAll(): Collection = items.values.toList() - - override fun upsert(flag: FeatureFlag) { - val event: FlagValueChangedEvent? = synchronized(writeLock) { - val existing = items[flag.id] - val change = when { - existing == null -> FlagValueChangedEvent(flag.id, null, flag.variation) - existing.variation != flag.variation -> - FlagValueChangedEvent(flag.id, existing.variation, flag.variation) - else -> null - } - items[flag.id] = flag - change - } - - if (event != null) { - listeners.forEach { it.onChange(event) } - } - } - - override fun addChangeListener(listener: FlagChangeListener) { - listeners.addIfAbsent(listener) - } - - override fun removeChangeListener(listener: FlagChangeListener) { - listeners.remove(listener) - } -} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/store/MemoryStore.kt b/featbit-client/src/main/kotlin/co/featbit/client/store/MemoryStore.kt deleted file mode 100644 index 8a402f7..0000000 --- a/featbit-client/src/main/kotlin/co/featbit/client/store/MemoryStore.kt +++ /dev/null @@ -1,23 +0,0 @@ -package co.featbit.client.store - -import co.featbit.client.model.FeatureFlag - -/** - * A thread-safe in-memory store holding the feature flag data received by the SDK. - */ -public interface MemoryStore { - /** Returns the flag with the given [id], or `null` if unknown. */ - public fun get(id: String): FeatureFlag? - - /** Returns a snapshot of all flags currently in the store. */ - public fun getAll(): Collection - - /** Inserts or updates [flag], raising a change event if its value changed. */ - public fun upsert(flag: FeatureFlag) - - /** Registers a listener invoked whenever a flag value changes. */ - public fun addChangeListener(listener: FlagChangeListener) - - /** Removes a previously registered change listener. */ - public fun removeChangeListener(listener: FlagChangeListener) -} diff --git a/featbit-client/src/main/kotlin/co/featbit/client/model/EndUser.kt b/featbit-client/src/main/kotlin/co/featbit/client/wire/EndUser.kt similarity index 93% rename from featbit-client/src/main/kotlin/co/featbit/client/model/EndUser.kt rename to featbit-client/src/main/kotlin/co/featbit/client/wire/EndUser.kt index de89345..0a2991e 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/model/EndUser.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/wire/EndUser.kt @@ -1,4 +1,4 @@ -package co.featbit.client.model +package co.featbit.client.wire import kotlinx.serialization.Serializable diff --git a/featbit-client/src/main/kotlin/co/featbit/client/model/Insight.kt b/featbit-client/src/main/kotlin/co/featbit/client/wire/Insight.kt similarity index 92% rename from featbit-client/src/main/kotlin/co/featbit/client/model/Insight.kt rename to featbit-client/src/main/kotlin/co/featbit/client/wire/Insight.kt index cebaf40..78d8946 100644 --- a/featbit-client/src/main/kotlin/co/featbit/client/model/Insight.kt +++ b/featbit-client/src/main/kotlin/co/featbit/client/wire/Insight.kt @@ -1,5 +1,7 @@ -package co.featbit.client.model +package co.featbit.client.wire +import co.featbit.client.model.FBUser +import co.featbit.client.model.FeatureFlag import kotlinx.serialization.Serializable /** diff --git a/featbit-client/src/test/kotlin/co/featbit/client/FBClientImplTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/FBClientImplTest.kt index e92ce80..879af43 100644 --- a/featbit-client/src/test/kotlin/co/featbit/client/FBClientImplTest.kt +++ b/featbit-client/src/test/kotlin/co/featbit/client/FBClientImplTest.kt @@ -4,10 +4,14 @@ import co.featbit.client.model.FBUser import co.featbit.client.model.FeatureFlag import co.featbit.client.options.FBOptions import kotlinx.coroutines.runBlocking +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Test +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.seconds class FBClientImplTest { @@ -77,6 +81,51 @@ class FBClientImplTest { client.close() } + /** + * Contract: the fast-path `*Variation()` getters honor the same not-ready guard as + * `evaluateCore`. With an uninitialized client and no bootstrap, every fast-path getter + * must return the caller's default — no flag lookup, no insight emission, no exception. + * + * This pins the `if (!initialized && options.bootstrap.isEmpty()) return default` line + * in `evaluateValue`. The combined "value and detail getters agree" test runs against an + * offline+bootstrap client (initialized=true), so it can't catch a mutation that drops + * this guard — this test does. + * + * Mutation that would fail this: + * * Removing the not-ready guard in evaluateValue — the call would fall through to + * `evaluator.evaluate(key)`, which would still return NotFound for an empty store + * (so default would be returned by accident). BUT: insight would also be skipped on + * NotFound, so the *observable* output stays "default" either way. The mutation is + * therefore semantically benign for the empty-store case. Acknowledge: this test + * can't catch the mutation as currently written without store inspection. What it + * DOES catch is a stronger mutation: the fast-path throwing or returning a + * non-default value when called pre-init. + * * Fast-path crashing on null userRef before guard checked — would throw rather + * than return default cleanly. + * + * Honest scope: the in-tree guard is mechanical and read by code review. This test + * pins the end-to-end "no throw, returns default" surface. + */ + @Test + fun `fast-path returns default when uninitialized without bootstrap`() { + // Non-offline, never started, no bootstrap — explicit pre-init state. + val options = FBOptions.Builder("secret").build() + val client = FBClientImpl(options, user) + try { + assertFalse("client must be in pre-init state", client.initialized) + + // Every fast-path getter must return the caller's default. + assertEquals(false, client.boolVariation("any", default = false)) + assertEquals(true, client.boolVariation("any", default = true)) + assertEquals(-1, client.intVariation("any", default = -1)) + assertEquals(0.5f, client.floatVariation("any", default = 0.5f)) + assertEquals(0.25, client.doubleVariation("any", default = 0.25), 0.0001) + assertEquals("fallback", client.stringVariation("any", default = "fallback")) + } finally { + client.close() + } + } + @Test fun `allFlags returns bootstrap snapshot`() { val client = offlineClientWith( @@ -88,4 +137,483 @@ class FBClientImplTest { assertEquals("1", all["a"]?.variation) client.close() } + + /** + * Contract: `*Variation()` (no-detail) and `*VariationDetail()` are sibling APIs that + * must agree on the returned value for every input. The non-detail variants go through + * the `evaluateValue` fast-path which skips the `EvalDetail` allocation; detail variants + * go through `evaluateCore`. Both code paths must produce the same value across: + * - found flag with valid conversion + * - found flag with type mismatch (falls back to default) + * - unknown flag (falls back to default) + * - bool / int / float / double / string converter coverage + * + * Mutation that would fail this: + * * The fast-path uses a different converter than the detail-path (e.g. swaps int for + * float) → typed value diverges between the two getters. + * * Fast-path skips the "type mismatch → default" branch (returns the converter's + * null instead of falling back) → mismatched-type cases would NPE or return wrong + * value. + * + * Note: this test runs with `initialized=true` (offline client with bootstrap), so it + * does NOT cover the fast-path's `!initialized && bootstrap.isEmpty()` guard. That + * branch is exercised separately by `fast-path returns default when uninitialized + * without bootstrap`. + */ + @Test + fun `value and detail getters agree across types and miss-paths`() = runBlocking { + val client = offlineClientWith( + FeatureFlag(id = "bool-flag", variation = "true"), + FeatureFlag(id = "int-flag", variation = "42"), + FeatureFlag(id = "float-flag", variation = "1.5"), + FeatureFlag(id = "double-flag", variation = "2.25"), + FeatureFlag(id = "string-flag", variation = "hello"), + FeatureFlag(id = "bad-bool", variation = "not-a-bool"), + FeatureFlag(id = "bad-int", variation = "not-an-int"), + ) + client.start() + try { + assertEquals(true, client.boolVariation("bool-flag")) + assertEquals(client.boolVariationDetail("bool-flag").value, client.boolVariation("bool-flag")) + + assertEquals(42, client.intVariation("int-flag", default = 0)) + assertEquals(client.intVariationDetail("int-flag", default = 0).value, client.intVariation("int-flag", default = 0)) + + assertEquals(1.5f, client.floatVariation("float-flag", default = 0f)) + assertEquals(client.floatVariationDetail("float-flag", default = 0f).value, client.floatVariation("float-flag", default = 0f)) + + assertEquals(2.25, client.doubleVariation("double-flag", default = 0.0), 0.0001) + assertEquals(client.doubleVariationDetail("double-flag", default = 0.0).value, client.doubleVariation("double-flag", default = 0.0), 0.0001) + + assertEquals("hello", client.stringVariation("string-flag")) + assertEquals(client.stringVariationDetail("string-flag").value, client.stringVariation("string-flag")) + + // Type mismatch — both must fall back to default. + assertEquals(false, client.boolVariation("bad-bool", default = false)) + assertEquals(client.boolVariationDetail("bad-bool", default = false).value, client.boolVariation("bad-bool", default = false)) + + assertEquals(-1, client.intVariation("bad-int", default = -1)) + assertEquals(client.intVariationDetail("bad-int", default = -1).value, client.intVariation("bad-int", default = -1)) + + // Unknown flag — both must fall back to default. + assertEquals("fallback", client.stringVariation("missing", default = "fallback")) + assertEquals(client.stringVariationDetail("missing", default = "fallback").value, client.stringVariation("missing", default = "fallback")) + } finally { + client.close() + } + } + + // --------------------------------------------------------------------------------------- + // identify / close contract tests (wider-scope audit H5) + // --------------------------------------------------------------------------------------- + + /** + * Contract: `identify(user)` must swap the synchronizer and re-fetch flags. After identify + * resolves, evaluation reflects the NEW user's payload, not the initial user's. + * + * Drives `FBClientImpl` in polling mode against `MockWebServer` — proves the swap end-to-end + * (closeAndJoin of previous sync, fresh sync starts, server is asked for new user's flags, + * store is updated, evaluation sees the update). + * + * Mutation that would fail this: + * * `identify` not setting `syncRef = fresh`. + * * `identify` re-using the old synchronizer (would never re-fetch). + * * `closeAndJoin` clearing the store mid-swap. + * * `identify` returning before the new sync's first response landed. + */ + @Test + fun `identify swaps synchronizer and evaluation reflects new user payload`() = runBlocking { + val server = MockWebServer() + server.start() + try { + // First user gets "alpha"; second user gets "beta". Polling interval is huge so the + // *only* time the SDK hits the server for each user is its initial fetch. + server.enqueue( + MockResponse().setResponseCode(200).setBody( + """{"data":{"featureFlags":[{"id":"f","variation":"alpha","matchReason":"fallthrough"}]}}""", + ), + ) + server.enqueue( + MockResponse().setResponseCode(200).setBody( + """{"data":{"featureFlags":[{"id":"f","variation":"beta","matchReason":"fallthrough"}]}}""", + ), + ) + + val options = FBOptions.Builder("secret") + .polling(server.url("/").toString(), interval = 10.seconds) + .event(server.url("/").toString()) + .build() + val client = FBClientImpl(options, FBUser.builder("user-A").build()) + + try { + assertTrue("initial start should succeed", client.start(timeout = 5.seconds)) + assertEquals( + "user-A evaluation must reflect server's 'alpha' payload", + "alpha", + client.stringVariation("f", default = "fallback"), + ) + + assertTrue( + "identify should succeed within timeout", + client.identify(FBUser.builder("user-B").build(), timeout = 5.seconds), + ) + + assertEquals( + "after identify, evaluation must reflect user-B's 'beta' payload", + "beta", + client.stringVariation("f", default = "fallback"), + ) + } finally { + client.close() + } + } finally { + server.shutdown() + } + } + + /** + * Contract: `close()` is idempotent. The public `Closeable.close` contract permits multiple + * calls; the SDK must not crash, hang, or throw on a second close. This pins the runBlocking + * + per-phase withTimeoutOrNull pattern's stability under double-invocation. + * + * For an offline client (NoopTrackInsight + NullDataSynchronizer + already-cancelled scope + * after the first close), the second close should be microsecond-scale. A 50ms threshold + * is generous enough to absorb GC pause / CI noise but tight enough to catch any mutation + * where the second close suspends on a phantom timeout or stuck channel.close(). + * + * Mutation that would fail this: + * * Removing the `withTimeoutOrNull` wrappers and letting a cancelled scope's + * `closeAndJoin`/`closeAndDrain` throw on the second call. + * * Any second-close path that suspends for >50ms (e.g. waiting on a timeout that + * never resolves because the scope is already cancelled). + */ + @Test + fun `close is idempotent`() { + val client = offlineClientWith(FeatureFlag(id = "f", variation = "v")) + client.close() + val startNs = System.nanoTime() + client.close() + val elapsedMs = (System.nanoTime() - startNs) / 1_000_000 + assertTrue( + "second close must return promptly (got ${elapsedMs}ms, expected < 50ms)", + elapsedMs < 50, + ) + } + + /** + * Contract: `close()` on an offline client (no network, no in-flight sync) returns promptly. + * Pins the lower bound of the close budget — proves the runBlocking{} bridge isn't + * waiting on a timeout that never fires when there's nothing to wait for. + * + * Mutation that would fail this: + * * `close()` blocking on a sleep, indefinite wait, or wrong condition. + * * `runBlocking { ... }` body waiting on a never-resolving deferred. + */ + @Test + fun `close completes promptly when offline`() { + val client = offlineClientWith(FeatureFlag(id = "f", variation = "v")) + val startNs = System.nanoTime() + client.close() + val elapsedMs = (System.nanoTime() - startNs) / 1_000_000 + assertTrue( + "offline close must return well under the per-phase 2s budgets (got ${elapsedMs}ms)", + elapsedMs < 500, + ) + } + + /** + * Contract: when the sync teardown can't make progress (e.g. polling sync is blocked in + * a non-responsive socket read), `close()` is bounded by `SYNC_CLOSE_TIMEOUT_MS = 2_000ms` + * — the per-phase budget set in `FBClientImpl.close()`. Without that budget, `close` would + * hang until OkHttp's 8s readTimeout fires (`FbApiClient.READ_TIMEOUT_SECONDS`). + * + * Setup: MockWebServer accepts the socket but never enqueues a response. PollingSync's + * first poll blocks in OkHttp's synchronous `socket.read`. `cancelAndJoin` can't preempt + * that read until readTimeout — so the outer `withTimeoutOrNull(2_000ms)` is the only + * thing keeping `close()` bounded. + * + * Mutation that would fail this: + * * Removing or extending `SYNC_CLOSE_TIMEOUT_MS` above ~3s. + * * Removing the outer `withTimeoutOrNull(SYNC_CLOSE_TIMEOUT_MS) { ... }` wrapper — + * close would hang until OkHttp's 8s readTimeout. + */ + @Test + fun `close is bounded when sync teardown is blocked on a non-responsive server`() = runBlocking { + val server = MockWebServer() + server.start() + try { + // No enqueued responses — the polling sync's first request will block in + // OkHttp's read until either cancellation cuts through or the 8s readTimeout fires. + val options = FBOptions.Builder("secret") + .polling(server.url("/").toString(), interval = 10.seconds) + .event(server.url("/").toString()) + .build() + val client = FBClientImpl(options, FBUser.builder("u1").build()) + + // Kick off start() with a short timeout so the polling sync issues its first request + // (which blocks in OkHttp's read since the server has no enqueued responses), then + // measure close(). start() itself returns false on the 200ms timeout; that's fine — + // we just need the polling loop to have begun a real HTTP call before we close. + client.start(timeout = 200.milliseconds) + + val startNs = System.nanoTime() + client.close() + val elapsedMs = (System.nanoTime() - startNs) / 1_000_000 + + // 2_500ms = SYNC_CLOSE_TIMEOUT_MS (2_000) + ~500ms CI slack (measured ~2.2s on + // local). A mutation that extended the constant past ~2.5s would fail here; a + // mutation that removed the outer withTimeoutOrNull would hit OkHttp's 8s + // readTimeout. The tight bound is intentional — looser thresholds let a 50%+ + // drift in the constant pass silently. + assertTrue( + "close must be bounded by SYNC_CLOSE_TIMEOUT_MS=2s + slack, not by OkHttp's " + + "8s readTimeout (got ${elapsedMs}ms)", + elapsedMs < 2_500, + ) + } finally { + server.shutdown() + } + } + + /** + * Contract: after `close()`, `initialized` continues to reflect the last-known state and + * evaluations return defaults / fall back via `client not ready` if the synchronizer was + * never initialized. The store contents survive — close does NOT wipe data. + * + * Mutation that would fail this: + * * `close()` clearing the store. + * * `close()` flipping `initialized` back to false in a way that breaks the bootstrap path. + */ + @Test + fun `close preserves store and bootstrap evaluations still work`() = runBlocking { + val client = offlineClientWith(FeatureFlag(id = "f", variation = "true")) + assertTrue(client.start()) + assertTrue("bootstrap flag evaluates before close", client.boolVariation("f")) + + client.close() + + // The store survived; bootstrap-backed evaluation still resolves. Note this is the + // contract for the offline+bootstrap path — a non-bootstrap client may behave + // differently post-close depending on whether the network sync ever completed. + assertTrue("bootstrap flag still evaluates after close", client.boolVariation("f")) + } + + /** + * Contract: `identify` on an offline client with bootstrap returns `true` (NullDataSync's + * `start()` is vacuously successful). User swap is recorded internally even when no + * network call happens. + * + * Mutation that would fail this: + * * `identify` swapping in a synchronizer that reports `initialized=false` for offline. + * * `NullDataSynchronizer.start()` returning `false`. + */ + @Test + fun `offline identify returns true without network`() = runBlocking { + val client = offlineClientWith(FeatureFlag(id = "f", variation = "true")) + assertTrue(client.start()) + assertTrue( + "offline identify must succeed (NullDataSynchronizer.start = true)", + client.identify(FBUser.builder("u2").build(), timeout = 1.seconds), + ) + assertTrue("client remains initialized after offline identify", client.initialized) + client.close() + } + + /** + * Contract: `start(timeout)` must return `false` (not throw) when the synchronizer fails to + * initialize within the budget. The current code uses `withTimeoutOrNull` which returns + * `null` -> coerced to `false`. This pins that behavior — a regression that threw or hung + * here would break user-facing start() semantics. + * + * Mutation that would fail this: + * * Replacing `withTimeoutOrNull` with `withTimeout` (would throw). + * * `withTimeoutOrNull` body suppressing the timeout (would hang). + */ + @Test + fun `start returns false when polling sync cannot initialize within timeout`() = runBlocking { + val server = MockWebServer() + server.start() + try { + // Never enqueue a response — the polling call will hang until the test's start() + // timeout, at which point start() should return false rather than throw. + val options = FBOptions.Builder("secret") + .polling(server.url("/").toString(), interval = 10.seconds) + .event(server.url("/").toString()) + .build() + val client = FBClientImpl(options, FBUser.builder("u1").build()) + try { + val startNs = System.nanoTime() + val started = client.start(timeout = 200.milliseconds) + val elapsedMs = (System.nanoTime() - startNs) / 1_000_000 + + assertFalse("start must return false on timeout, not throw", started) + assertFalse( + "initialized must remain false after failed start — catches mutations that " + + "flip the flag on timeout", + client.initialized, + ) + assertTrue( + "start respects the timeout (got ${elapsedMs}ms, expected ~200ms + slack)", + elapsedMs in 100..2_000, + ) + } finally { + client.close() + } + } finally { + server.shutdown() + } + } + + /** + * Contract: the fast-path `*Variation()` getters MUST emit one insight per `Found` + * evaluation when insights are enabled (online tracker). Symmetric to the detail-path — + * but the existing `value and detail getters agree` test uses an offline client where + * `insightsEnabled == false`, so neither path emits and the test can't catch a mutation + * that drops `insights.offer(...)` from `evaluateValue`'s Found branch. + * + * Driving an online client against MockWebServer exercises the full chain: + * polling sync → store.upsertAll(flags) → `boolVariation()` → evaluateValue Found → + * insights.offer → InsightDispatcher batch → HttpTrackInsight POST → MockWebServer. + * `client.close()` triggers `insights.closeAndDrain()` which flushes immediately + * regardless of the 1s batch interval. + * + * Mutation that would fail this: + * * Removing `insights.offer(...)` from `evaluateValue` — no insight POST is ever + * made, server records only the polling request(s). Assertion on a recorded insight + * request fails. + * * Replacing the fast-path's `Insight.forEvaluation` argument list (e.g. forgetting + * to pass `result.flag`) — payload shape diverges; serialization or downstream + * assertion fails. + * * Routing the fast-path through evaluateCore — would still emit, test passes; this + * is the wrong mutation to focus on (test catches absence of emission, not which + * code-path emitted). + */ + @Test + fun `fast-path boolVariation emits insight via online tracker`() = runBlocking { + val server = MockWebServer() + server.start() + try { + // Polling response: one flag we'll evaluate. + server.enqueue( + MockResponse().setResponseCode(200).setBody( + """{"data":{"featureFlags":[{"id":"f","variation":"true","matchReason":"fallthrough"}]}}""", + ), + ) + // Insight track response: 200 OK is enough; the tracker discards the body. + server.enqueue(MockResponse().setResponseCode(200)) + + val options = FBOptions.Builder("secret") + .polling(server.url("/").toString(), interval = 10.seconds) + .event(server.url("/").toString()) + .build() + val client = FBClientImpl(options, FBUser.builder("u1").name("bob").build()) + try { + assertTrue("polling start", client.start(timeout = 5.seconds)) + + // Fast-path call — no detail variant, value-only return. evaluateValue should + // emit one insight via the dispatcher. + assertEquals(true, client.boolVariation("f")) + } finally { + // Forces insight drain — closeAndDrain bypasses the 1s flush interval. + client.close() + } + + // Wait (bounded) for the insight POST to land at MockWebServer. `client.close()` + // returns once `closeAndDrain` joins the consumer + calls `tracker.close()`, but + // OkHttp's dispatcher executor only signals shutdown — in-flight calls finish on + // their own threads. A 1ms `takeRequest` after close races the on-the-wire POST. + // + // Strategy: drain requests with a per-poll budget that's large enough to absorb + // CI scheduler hiccups, then assert at least one insight POST appeared. The + // total wall-budget is bounded by `attempts × pollMs`. + val recordedPaths = mutableListOf() + var insightHits = 0 + val pollMs = 250L + val maxAttempts = 8 // 8 × 250ms = 2s total budget; CI noise tolerance. + for (attempt in 1..maxAttempts) { + val req = server.takeRequest(pollMs, java.util.concurrent.TimeUnit.MILLISECONDS) ?: break + val path = req.path ?: "" + recordedPaths += path + if (path.startsWith("/api/public/insight/")) { + insightHits++ + break // one insight is enough — drain stops here to keep wall-time tight + } + } + assertTrue( + "fast-path eval must emit at least one insight within 2s (paths: $recordedPaths)", + insightHits >= 1, + ) + } finally { + server.shutdown() + } + } + + /** + * Contract: the fast-path's `!initialized && options.bootstrap.isEmpty()` guard must + * return the caller's default WITHOUT emitting any insight. The Found branch is gated by + * `if (insightsEnabled)` so emission ONLY happens when the evaluator resolves a flag — + * the guard short-circuits before evaluator.evaluate is even called. + * + * This pairs with the prior `fast-path returns default when uninitialized without + * bootstrap` test (which only checks the return value); here we additionally pin that + * the insight pipeline is NOT exercised when the guard fires. + * + * Setup: online client (insightsEnabled = true), but never started — polling sync sits + * in pre-init with empty store. Hit fast-path getters; assert no insight POST landed. + * + * Mutation that would fail this: + * * Removing the `!initialized && options.bootstrap.isEmpty()` guard from + * evaluateValue — control would flow into the `when (evaluator.evaluate)` block. + * For an empty store this still falls into the NotFound branch (returns default), + * so the return value alone can't catch the mutation. But the Found branch now + * becomes reachable for any future code path that pre-populates the store under + * `!initialized` — the guard is the only place that prevents an insight from being + * emitted pre-init when bootstrap is empty. THIS test catches the symptom directly: + * no insight POST has been observed pre-init. + * + * Honest scope: with the current evaluator + empty store, the NotFound branch returns + * default and skips insight emission — same observable outcome as the guard firing. + * What this test really pins is "the fast-path emits zero insight requests pre-init" + * end-to-end. A mutation that drops the guard AND moves insight emission to the + * NotFound branch would still slide. The mechanical guard is read by code review. + */ + @Test + fun `fast-path emits no insight when uninitialized without bootstrap`() = runBlocking { + val server = MockWebServer() + server.start() + try { + // No polling response enqueued — the polling sync hangs on its first request. + // The client never reaches `initialized = true`, so all evaluations go down the + // pre-init fast-path guard. + val options = FBOptions.Builder("secret") + .polling(server.url("/").toString(), interval = 10.seconds) + .event(server.url("/").toString()) + .build() + val client = FBClientImpl(options, FBUser.builder("u1").build()) + + // Short start timeout — we don't wait for polling to succeed. + client.start(timeout = 100.milliseconds) + assertFalse("client must remain in pre-init", client.initialized) + + // Fast-path calls — all must return default and emit zero insights. + assertEquals(false, client.boolVariation("any", default = false)) + assertEquals(true, client.boolVariation("any", default = true)) + assertEquals("default", client.stringVariation("any", default = "default")) + + client.close() + + // Drain all requests the server saw; assert none were insight POSTs. + val recordedPaths = generateSequence { server.takeRequest(50, java.util.concurrent.TimeUnit.MILLISECONDS) } + .map { it.path ?: "" } + .toList() + val insightHits = recordedPaths.count { it.startsWith("/api/public/insight/") } + assertEquals( + "pre-init fast-path must not emit any insight (paths: $recordedPaths)", + 0, + insightHits, + ) + } finally { + server.shutdown() + } + } } diff --git a/featbit-client/src/test/kotlin/co/featbit/client/LifecycleControllerTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/LifecycleControllerTest.kt deleted file mode 100644 index dccda58..0000000 --- a/featbit-client/src/test/kotlin/co/featbit/client/LifecycleControllerTest.kt +++ /dev/null @@ -1,86 +0,0 @@ -package co.featbit.client - -import co.featbit.client.datasynchronizer.DataSynchronizer -import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.test.advanceTimeBy -import kotlinx.coroutines.test.runCurrent -import kotlinx.coroutines.test.runTest -import org.junit.Assert.assertEquals -import org.junit.Test -import java.util.concurrent.atomic.AtomicInteger - -@OptIn(ExperimentalCoroutinesApi::class) -class LifecycleControllerTest { - - private val grace = 1_000L - - private class FakeSynchronizer : DataSynchronizer { - val pauses = AtomicInteger() - val resumes = AtomicInteger() - override val initialized: Boolean = true - override suspend fun start(): Boolean = true - override fun pause() { pauses.incrementAndGet() } - override fun resume() { resumes.incrementAndGet() } - override fun close() {} - } - - @Test - fun `backgrounding pauses only after the grace period`() = runTest { - val sync = FakeSynchronizer() - val controller = LifecycleController(backgroundScope, grace) { sync } - - controller.onForegroundChanged(false) - runCurrent() - assertEquals("no pause before grace elapses", 0, sync.pauses.get()) - - advanceTimeBy(grace + 1) - runCurrent() - assertEquals("pause after grace", 1, sync.pauses.get()) - } - - @Test - fun `quick foreground return within grace does not pause`() = runTest { - val sync = FakeSynchronizer() - val controller = LifecycleController(backgroundScope, grace) { sync } - - controller.onForegroundChanged(false) - runCurrent() - advanceTimeBy(grace / 2) - controller.onForegroundChanged(true) - advanceTimeBy(grace + 1) - runCurrent() - - assertEquals("never paused", 0, sync.pauses.get()) - assertEquals("never paused so no resume needed", 0, sync.resumes.get()) - } - - @Test - fun `resume after a completed pause`() = runTest { - val sync = FakeSynchronizer() - val controller = LifecycleController(backgroundScope, grace) { sync } - - controller.onForegroundChanged(false) - advanceTimeBy(grace + 1) - runCurrent() - assertEquals(1, sync.pauses.get()) - - controller.onForegroundChanged(true) - runCurrent() - assertEquals("resume on foreground", 1, sync.resumes.get()) - } - - @Test - fun `losing network pauses, regaining it resumes`() = runTest { - val sync = FakeSynchronizer() - val controller = LifecycleController(backgroundScope, grace) { sync } - - controller.onNetworkChanged(false) - advanceTimeBy(grace + 1) - runCurrent() - assertEquals(1, sync.pauses.get()) - - controller.onNetworkChanged(true) - runCurrent() - assertEquals(1, sync.resumes.get()) - } -} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/app/DefaultMemoryStoreTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/app/DefaultMemoryStoreTest.kt new file mode 100644 index 0000000..372d927 --- /dev/null +++ b/featbit-client/src/test/kotlin/co/featbit/client/app/DefaultMemoryStoreTest.kt @@ -0,0 +1,480 @@ +package co.featbit.client.app + +import co.featbit.client.model.FeatureFlag +import co.featbit.client.store.FlagChangeListener +import co.featbit.client.store.FlagValueChangedEvent +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import java.util.concurrent.CopyOnWriteArrayList +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicInteger +import java.util.concurrent.atomic.AtomicReference + +class DefaultMemoryStoreTest { + + private fun flag(id: String, variation: String) = + FeatureFlag(id = id, variation = variation) + + @Test + fun `bootstrap is loaded`() { + val store = DefaultMemoryStore(listOf(flag("a", "1"), flag("b", "2"))) + assertEquals("1", store.get("a")?.variation) + assertEquals(2, store.getAll().size) + assertNull(store.get("missing")) + } + + @Test + fun `upsert of new flag fires event with null old value`() { + val store = DefaultMemoryStore() + val events = CopyOnWriteArrayList() + store.addChangeListener { events.add(it) } + + store.upsert(flag("a", "1")) + + assertEquals(1, events.size) + assertEquals(FlagValueChangedEvent("a", null, "1"), events[0]) + } + + @Test + fun `upsert with changed value fires event, unchanged does not`() { + val store = DefaultMemoryStore(listOf(flag("a", "1"))) + val events = CopyOnWriteArrayList() + store.addChangeListener { events.add(it) } + + store.upsert(flag("a", "1")) // unchanged + assertTrue(events.isEmpty()) + + store.upsert(flag("a", "2")) // changed + assertEquals(listOf(FlagValueChangedEvent("a", "1", "2")), events.toList()) + } + + @Test + fun `removed listener stops receiving events`() { + val store = DefaultMemoryStore() + val events = CopyOnWriteArrayList() + val listener = FlagChangeListener { events.add(it) } + store.addChangeListener(listener) + store.removeChangeListener(listener) + + store.upsert(flag("a", "1")) + assertTrue(events.isEmpty()) + } + + // --------------------------------------------------------------------------------------- + // concurrency tests (wider-scope audit N2) + // --------------------------------------------------------------------------------------- + + /** + * Contract: concurrent upserts of the same flag from many threads must (a) never throw + * any exception (ConcurrentModificationException, NPE, IllegalStateException), and (b) + * leave the store in a consistent final state with a value that some thread actually + * wrote. + * + * Setup: 16 threads × 250 upserts each = 4000 upserts. Each thread cycles values + * `"v0".."v3"`. + * + * Mutation discipline (honest scope): + * * Removing `CopyOnWriteArrayList` for listeners → CME on dispatch path under + * concurrent addChangeListener. This test (using a single listener) would not catch + * that — `add and remove listeners during dispatch` below pins that surface. + * * Removing `ConcurrentHashMap` for the items map → CME or torn read. This test WOULD + * catch a CME because all threads contend the same key. + * * Removing `synchronized(writeLock)` → race between diff and store. This test does + * NOT reliably catch that mutation: the race produces a wrong-but-plausible event + * count, not a thrown exception or a "garbage string" newValue. Catching the lock + * mutation would need a deterministic interleaving harness (custom lock with + * interposition), which is overkill for a unit test — the lock is mechanically a + * 1-line guard read by code review. + * + * Net: this test proves the thread-safe-collection invariants and "no garbage". The + * diff-under-lock invariant relies on review + the in-tree comment. + */ + @Test + fun `concurrent upserts are race-free and final state is consistent`() { + val store = DefaultMemoryStore() + val events = CopyOnWriteArrayList() + store.addChangeListener { events.add(it) } + + val threads = 16 + val perThread = 250 + val pool = Executors.newFixedThreadPool(threads) + val startGate = CountDownLatch(1) + val doneGate = CountDownLatch(threads) + val exceptions = AtomicReference(null) + + repeat(threads) { tid -> + pool.submit { + try { + startGate.await() // align all threads to hammer simultaneously + repeat(perThread) { i -> + val v = "v${(tid + i) % 4}" + store.upsert(flag("flagX", v)) + } + } catch (t: Throwable) { + exceptions.compareAndSet(null, t) + } finally { + doneGate.countDown() + } + } + } + + startGate.countDown() + assertTrue( + "all threads finished within 30s", + doneGate.await(30, TimeUnit.SECONDS), + ) + pool.shutdown() + + assertNull("no thread threw under concurrent upsert", exceptions.get()) + + // Final state: the store holds *some* value of "v0".."v3" for flagX. + val finalFlag = store.get("flagX") + assertTrue( + "final value is one of v0..v3 (got '${finalFlag?.variation}')", + finalFlag?.variation in setOf("v0", "v1", "v2", "v3"), + ) + + // Event count bounds: at least 1 (first insert always fires), at most 4000 (every + // upsert was a real transition — impossible in practice with only 4 distinct values + // and 16 racing threads, but pinned as an upper bound). + val n = events.size + assertTrue( + "received between 1 and $threads*$perThread events (got $n) — a tearing bug " + + "could push above 4000 by double-firing on the same transition", + n in 1..(threads * perThread), + ) + + // Strongest invariant: every recorded event's newValue is one of the actual values + // any thread might have written. Catches tearing that produces garbage strings. + events.forEach { ev -> + assertTrue( + "event newValue must be v0..v3 (got '${ev.newValue}')", + ev.newValue in setOf("v0", "v1", "v2", "v3"), + ) + assertEquals("event key is flagX", "flagX", ev.key) + } + } + + /** + * Contract: the `items[flag.id] = flag` write happens-before the listener callback fires. + * A listener can therefore re-enter `store.upsert(...)` with a different key, and can + * read the value just written for the parent key via `store.get(...)`. + * + * Honest scope: this test does NOT prove that notify happens *outside* the + * `synchronized(writeLock)` block. JVM monitors are reentrant — moving `listeners.forEach` + * inside the synchronized block would NOT deadlock the same-thread reentrant upsert; the + * parent's `items[flag.id] = flag` would have already happened-before the callback either + * way, so `store.get(event.key)` would still return the just-written value. The genuine + * "notify outside lock" guarantee matters when ANOTHER thread is contending; reproducing + * that deterministically requires a custom Lock with interposition, which is overkill for + * a single-line invariant. The mechanical guard is read by code review. + * + * Mutation that would fail this: + * * Not committing `items[flag.id] = flag` before notify — `store.get(event.key)` in + * the listener returns null or stale data. + * * Listener never fired — `reentrantSeen` stays null. + */ + @Test + fun `listener observes the just-written value (write happens-before notify)`() { + val store = DefaultMemoryStore() + val reentrantSeen = AtomicReference(null) + var reentrantUpserted = false + + store.addChangeListener { event -> + // First fire: re-enter with a *different* key. Capture what the store reports + // *during* the callback for the original key — must already reflect the write. + if (!reentrantUpserted) { + reentrantUpserted = true + reentrantSeen.set(store.get(event.key)?.variation) + store.upsert(flag("b", "from-listener")) + } + } + + store.upsert(flag("a", "from-test")) + + assertEquals( + "listener observed the just-written value of 'a' — proves the items[] = flag " + + "write happens-before the callback fires", + "from-test", + reentrantSeen.get(), + ) + assertEquals( + "reentrant upsert from inside the listener committed to the store", + "from-listener", + store.get("b")?.variation, + ) + } + + /** + * Contract: the listeners snapshot used by `forEach` (CopyOnWriteArrayList.iterator) is + * stable — adding a listener mid-iteration MUST NOT cause the new listener to fire for + * the in-flight event, and removing a listener mid-iteration MUST NOT throw + * ConcurrentModificationException. + * + * The first listener (which mutates the listener list during its own callback) and the + * subsequent listeners exercise both directions. + * + * Mutation that would fail this: + * * Replacing `CopyOnWriteArrayList` with a plain `ArrayList` — + * would throw ConcurrentModificationException on the add-during-iteration path. + */ + @Test + fun `add and remove listeners during dispatch do not corrupt iteration`() { + val store = DefaultMemoryStore() + val firedNames = CopyOnWriteArrayList() + + val late = FlagChangeListener { firedNames.add("late") } + val first = FlagChangeListener { + firedNames.add("first") + // Mutate the listener list mid-iteration. CopyOnWriteArrayList guarantees the + // active iterator sees the snapshot it captured at iteration start. + store.addChangeListener(late) + } + val second = FlagChangeListener { firedNames.add("second") } + + store.addChangeListener(first) + store.addChangeListener(second) + + // Explicit CME catch: a plain `ArrayList` would throw ConcurrentModificationException + // on the same-thread `addChangeListener(...)` call inside the iterator's forEach. The + // try/catch distinguishes that failure mode from any other assertion failure below, + // so a CI run failing here points the maintainer at exactly the regression class. + try { + store.upsert(flag("k", "v1")) + } catch (e: java.util.ConcurrentModificationException) { + org.junit.Assert.fail( + "listener list must use a snapshot iterator (CopyOnWriteArrayList); plain " + + "ArrayList would CME here: ${e.message}", + ) + } + + assertEquals( + "exactly first+second fired in iteration order; 'late' joined too late for this event", + listOf("first", "second"), + firedNames.toList(), + ) + + // Second upsert: 'late' is now in the snapshot, fires alongside first+second. + firedNames.clear() + store.upsert(flag("k", "v2")) + assertEquals( + "after add-during-dispatch, 'late' fires on subsequent events", + listOf("first", "second", "late"), + firedNames.toList(), + ) + } + + /** + * Contract: `addChangeListener` uses `listeners.addIfAbsent(listener)` so registering the + * same listener twice is a no-op. Prevents duplicate fan-out for callers that defensively + * re-register on lifecycle events. + * + * Mutation that would fail this: + * * Replacing `addIfAbsent` with `add` — the listener fires twice on the single upsert. + */ + @Test + fun `addChangeListener is idempotent — re-adding the same listener fires it only once`() { + val store = DefaultMemoryStore() + val count = AtomicInteger(0) + val listener = FlagChangeListener { count.incrementAndGet() } + + store.addChangeListener(listener) + store.addChangeListener(listener) // must be a no-op (addIfAbsent semantic) + + store.upsert(flag("a", "1")) + + assertEquals("listener fires exactly once despite two add calls", 1, count.get()) + } + + // --------------------------------------------------------------------------------------- + // upsertAll tests (perf-pass: one lock acquire per batch instead of N) + // --------------------------------------------------------------------------------------- + + /** + * Contract: `upsertAll(emptyCollection)` is a no-op. Polling can call this with an empty + * snapshot (server returns no flags); must not enter the write lock or fire any events. + * + * Mutation that would fail this: + * * Removing the `if (flags.isEmpty()) return` short-circuit — the impl would still + * enter the synchronized block (small but observable), but more importantly the + * listener-fire loop would still iterate (zero events, technically still a pass). + * This test pins the *event* side; lock-acquisition itself is not observable. + * * If a future refactor accidentally fires a spurious empty-event, this catches it. + */ + @Test + fun `upsertAll with empty collection fires no events`() { + val store = DefaultMemoryStore() + val events = CopyOnWriteArrayList() + store.addChangeListener { events.add(it) } + + store.upsertAll(emptyList()) + + assertTrue("empty bulk upsert fires zero events", events.isEmpty()) + } + + /** + * Contract: a bulk upsert of N new flags fires N change events, one per flag, each with + * `oldValue == null` and `newValue == flag.variation`. Output order = input order + * (preserved by ArrayList accumulation under the write lock). + * + * Mutation that would fail this: + * * Replacing the inner loop with a single-event emit (e.g., notifying once at end with + * just the last flag) — event count drops from N to 1. + * * Filtering out flags that "look unchanged" relative to a sibling in the same batch — + * would fire fewer events. + * * Re-ordering the accumulator (e.g., HashSet instead of ArrayList) — event order + * would scramble for some inputs. + */ + @Test + fun `upsertAll of new flags fires one event per flag in input order`() { + val store = DefaultMemoryStore() + val events = CopyOnWriteArrayList() + store.addChangeListener { events.add(it) } + + store.upsertAll(listOf(flag("a", "1"), flag("b", "2"), flag("c", "3"))) + + assertEquals( + "events fire one-per-flag with null oldValue, in input order", + listOf( + FlagValueChangedEvent("a", null, "1"), + FlagValueChangedEvent("b", null, "2"), + FlagValueChangedEvent("c", null, "3"), + ), + events.toList(), + ) + } + + /** + * Contract: in a bulk upsert mixing changed + unchanged flags, only the changed flags + * fire events. This mirrors the per-flag `upsert` semantic exactly — bulk must NOT + * fire an event for a flag whose `variation` matches the existing entry. + * + * Mutation that would fail this: + * * Bulk impl naively fires an event for every input (skipping the per-flag diff) — + * `c` would produce an unwanted event. + * * Diff comparing the wrong field (e.g., variationId instead of variation) — both `a` + * (unchanged variation) and `c` (unchanged variation) would either appear or + * disappear from events, depending on the bug. + */ + @Test + fun `upsertAll mixing changed and unchanged flags fires events only for changes`() { + val store = DefaultMemoryStore(listOf(flag("a", "1"), flag("c", "3"))) + val events = CopyOnWriteArrayList() + store.addChangeListener { events.add(it) } + + // a: unchanged. b: new. c: unchanged. d: new. + store.upsertAll(listOf(flag("a", "1"), flag("b", "2"), flag("c", "3"), flag("d", "4"))) + + assertEquals( + "only b (new) and d (new) fire — a and c are no-op for matching variation", + listOf( + FlagValueChangedEvent("b", null, "2"), + FlagValueChangedEvent("d", null, "4"), + ), + events.toList(), + ) + // Store reflects every entry — even ones that produced no event. + assertEquals("1", store.get("a")?.variation) + assertEquals("2", store.get("b")?.variation) + assertEquals("3", store.get("c")?.variation) + assertEquals("4", store.get("d")?.variation) + } + + /** + * Contract: when a bulk upsert overwrites existing entries with different variations, + * each event carries the correct (prior_variation, new_variation) pair. Pinning this + * separately from "new-flag events" because the diff path is harder to get right. + * + * Mutation that would fail this: + * * Computing `oldValue` from the freshly-stored map (after the write) instead of the + * pre-write read — every event would have `oldValue == newValue`. + * * Storing the whole input list pre-loop and reading old values from there — would + * skew on duplicate inputs. + */ + @Test + fun `upsertAll overwriting existing flags carries correct oldValue per event`() { + val store = DefaultMemoryStore(listOf(flag("a", "old-a"), flag("b", "old-b"))) + val events = CopyOnWriteArrayList() + store.addChangeListener { events.add(it) } + + store.upsertAll(listOf(flag("a", "new-a"), flag("b", "new-b"))) + + assertEquals( + listOf( + FlagValueChangedEvent("a", "old-a", "new-a"), + FlagValueChangedEvent("b", "old-b", "new-b"), + ), + events.toList(), + ) + } + + /** + * Contract: in `upsertAll`, listener callbacks fire AFTER all writes complete. A listener + * invoked during dispatch can call `store.get(otherKeyInTheSameBatch)` and observe the + * post-batch value, not a half-written intermediate state. This is the consistency + * guarantee documented in `MemoryStore.upsertAll`'s Kdoc. + * + * Mutation that would fail this: + * * Re-implementing `upsertAll` as `flags.forEach { upsert(it) }` (the *interface + * default*) — listeners fire interleaved with writes, so during the event for `a`, + * `b` is not yet visible in the store, and the captured snapshot would show `null` + * for `b`. + * + * Note: this is the documented behavioral DIFFERENCE between the override and the + * default, so future maintainers know which is which. + */ + @Test + fun `upsertAll fires listeners after all writes — listener sees consistent snapshot`() { + val store = DefaultMemoryStore() + val observedDuringDispatch = CopyOnWriteArrayList>() + store.addChangeListener { ev -> + // Capture, for each event, what the store reports for the *other* flag in the + // batch. With the override (write-then-notify), every event must observe every + // sibling already in place. + observedDuringDispatch.add(ev.key to store.get(otherKey(ev.key))?.variation) + } + + store.upsertAll(listOf(flag("a", "1"), flag("b", "2"))) + + // Both events must see both writes already committed. Note the snapshot map: for + // event-a, sibling "b" must already be in the store; for event-b, sibling "a" must + // be there too. + assertTrue( + "during event for 'a', sibling 'b' must already be visible (saw: $observedDuringDispatch)", + observedDuringDispatch.contains("a" to "2"), + ) + assertTrue( + "during event for 'b', sibling 'a' must already be visible (saw: $observedDuringDispatch)", + observedDuringDispatch.contains("b" to "1"), + ) + } + + private fun otherKey(key: String): String = when (key) { + "a" -> "b" + "b" -> "a" + else -> throw IllegalArgumentException("unexpected key $key") + } + + /** + * Contract: with zero registered listeners, `upsertAll` short-circuits the notify loop + * entirely. This pins the "no listeners → skip dispatch" optimization in the impl. + * + * Mutation that would fail this: + * * Removing the `listeners.isEmpty()` guard before the for-loop — would still produce + * correct (no) behavior because zero listeners = nothing to iterate. NOT directly + * observable. This test pins only that no exception fires; the optimization is + * mechanical and verified by code review. + */ + @Test + fun `upsertAll with no listeners does not throw`() { + val store = DefaultMemoryStore() + // Intentionally no addChangeListener — must not throw on the dispatch path. + store.upsertAll(listOf(flag("a", "1"), flag("b", "2"))) + assertEquals("1", store.get("a")?.variation) + assertEquals("2", store.get("b")?.variation) + } +} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/app/FlagTrackerImplTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/app/FlagTrackerImplTest.kt new file mode 100644 index 0000000..eef43d1 --- /dev/null +++ b/featbit-client/src/test/kotlin/co/featbit/client/app/FlagTrackerImplTest.kt @@ -0,0 +1,347 @@ +package co.featbit.client.app + +import app.cash.turbine.test +import co.featbit.client.model.FeatureFlag +import co.featbit.client.store.FlagValueChangedEvent +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertTrue +import org.junit.Test +import java.util.concurrent.CopyOnWriteArrayList +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean + +class FlagTrackerImplTest { + + private fun flag(id: String, variation: String) = FeatureFlag(id = id, variation = variation) + + @Test + fun `global subscriber receives all changes`() { + val store = DefaultMemoryStore() + val tracker = FlagTrackerImpl(store) + val received = CopyOnWriteArrayList() + tracker.subscribe { received.add(it) } + + store.upsert(flag("a", "1")) + store.upsert(flag("b", "2")) + + assertEquals(2, received.size) + } + + @Test + fun `keyed subscriber only receives its own key`() { + val store = DefaultMemoryStore() + val tracker = FlagTrackerImpl(store) + val received = CopyOnWriteArrayList() + tracker.subscribe("a") { received.add(it) } + + store.upsert(flag("a", "1")) + store.upsert(flag("b", "2")) + + assertEquals(1, received.size) + assertEquals("a", received[0].key) + } + + @Test + fun `unsubscribe stops delivery`() { + val store = DefaultMemoryStore() + val tracker = FlagTrackerImpl(store) + val received = CopyOnWriteArrayList() + val listener = co.featbit.client.store.FlagChangeListener { received.add(it) } + tracker.subscribe(listener) + tracker.unsubscribe(listener) + + store.upsert(flag("a", "1")) + assertTrue(received.isEmpty()) + } + + @Test + fun `flagChanges flow emits change events`() = runTest { + val store = DefaultMemoryStore() + val tracker = FlagTrackerImpl(store) + + tracker.flagChanges.test { + store.upsert(flag("a", "1")) + assertEquals(FlagValueChangedEvent("a", null, "1"), awaitItem()) + cancelAndIgnoreRemainingEvents() + } + } + + @Test + fun `close removes listener from store`() { + val store = DefaultMemoryStore() + val tracker = FlagTrackerImpl(store) + val received = CopyOnWriteArrayList() + tracker.subscribe { received.add(it) } + + tracker.close() + store.upsert(flag("a", "1")) + + assertTrue(received.isEmpty()) + } + + // --------------------------------------------------------------------------------------- + // capacity + fairness + exception tests (wider-scope audit H1) + // --------------------------------------------------------------------------------------- + + /** + * Contract: the SharedFlow uses `extraBufferCapacity = 64` (FlagTrackerImpl.kt:25). This + * buffer is for ACTIVE-BUT-SLOW collectors so the emitter doesn't suspend while the + * collector catches up. With a hung collector and replay=0, the emitter's `tryEmit` fills + * the buffer first, then DROPS subsequent events. Pins the audit's H1 finding — "a burst + * of upserts during a fresh data-sync snapshot can overflow silently." + * + * Setup: register a HUNG collector (suspends in the lambda so it can't drain), then push + * more events than capacity. The first 64 land in the buffer, the rest are dropped at + * `tryEmit`. Then release the collector and assert the bounded receive count. + * + * Honest scope: this test pins capacity + overflow-drops behavior. It does NOT change the + * production contract (no overflow signal to caller) — that's a follow-up design decision + * flagged in the audit. Future refactors that lift capacity or add an overflow callback + * will need to update this assertion. + * + * Mutation that would fail this: + * * Lowering `extraBufferCapacity` (e.g. to 16) — drop earlier, far fewer than 64 land. + * * Switching to `MutableSharedFlow(replay = N)` with a smaller buffer — same. + * * Changing `tryEmit` to suspending `emit` — emitter would suspend instead of dropping; + * test would observe more than 64 events after release (since none were dropped). + */ + @Test + fun `SharedFlow buffer caps at extraBufferCapacity under hung-collector burst`() { + val store = DefaultMemoryStore() + val tracker = FlagTrackerImpl(store) + + val subscribed = CountDownLatch(1) // collector has issued its first awaitItem + val firstReceived = CountDownLatch(1) // collector has drained the very first event + val releaseGate = CountDownLatch(1) // test releases the collector to drain the burst + val received = CopyOnWriteArrayList() + + // Hung collector on its own thread: signals when subscribed + when first event arrives, + // then blocks on releaseGate so subsequent emits must fit into extraBufferCapacity or + // be dropped. + val collectorThread = Thread { + kotlinx.coroutines.runBlocking { + tracker.flagChanges.collect { event -> + if (received.isEmpty()) { + // First event delivered. + received.add(event) + firstReceived.countDown() + releaseGate.await() + } else { + received.add(event) + } + } + } + } + collectorThread.isDaemon = true + collectorThread.start() + + // Wait for the thread to actually subscribe before pushing any events. We approximate + // "subscribed" by pushing a single warmup event in a tight loop until it lands. + val subscribeDeadline = System.currentTimeMillis() + 5_000 + var warmupAttempts = 0 + while (firstReceived.count > 0 && System.currentTimeMillis() < subscribeDeadline) { + store.upsert(flag("warmup", "v$warmupAttempts")) + warmupAttempts++ + Thread.sleep(10) + } + subscribed.countDown() + assertEquals( + "warmup event reached the collector within 5s (got $warmupAttempts attempts)", + 0L, + firstReceived.count, + ) + val baseline = received.size // = 1 + + // Push the burst. The collector is hung on `releaseGate`. Each emit either fills + // extraBufferCapacity (64) or drops at tryEmit. Suspending emit would NOT drop. + val burst = 200 + repeat(burst) { i -> store.upsert(flag("k$i", "v$i")) } + + // Release the collector and let it drain whatever the buffer held. + releaseGate.countDown() + Thread.sleep(500) // drain window + + val afterBurst = received.size - baseline + assertTrue( + "buffered emissions ≤ 64 (got $afterBurst) — extraBufferCapacity is the ceiling", + afterBurst <= 64, + ) + assertTrue( + "at least 1 buffered event survived (got $afterBurst) — buffer is not size 0", + afterBurst >= 1, + ) + + collectorThread.interrupt() + } + + /** + * Contract: a slow callback-style subscriber does not block other subscribers receiving + * the same event. `subscribers.forEach { it.onChange(event) }` is sequential per-event, + * so this pins what "slow" means: the SECOND subscriber (after the slow one) is delayed + * BUT still fires. + * + * Honest scope: this is not "concurrent fan-out" — production iterates synchronously. The + * test pins that the iteration completes even when one subscriber takes time. + * + * Mutation that would fail this: + * * `subscribers.forEach { ... }` rewritten to `subscribers.first().onChange(event)` — + * only the slow one fires, fast one never does. + * * Wrapping each `onChange` in a `launch` without awaiting — fast subscriber might fire + * before the slow one, but both eventually fire. Current sequential semantic is the + * contract. + */ + @Test + fun `slow subscriber does not prevent subsequent subscribers from firing`() { + val store = DefaultMemoryStore() + val tracker = FlagTrackerImpl(store) + + val slowEntered = CountDownLatch(1) + val slowFinished = CountDownLatch(1) + val fastFired = AtomicBoolean(false) + val slowFired = AtomicBoolean(false) + + // Order matters: slow subscribes first → slow runs first per forEach iteration. + tracker.subscribe { _ -> + slowEntered.countDown() + // Block briefly. Bounded so a regression doesn't hang the suite. + Thread.sleep(200) + slowFired.set(true) + slowFinished.countDown() + } + tracker.subscribe { _ -> fastFired.set(true) } + + // Trigger fan-out on the test thread (production also runs subscribers synchronously + // from the upsert thread, see DefaultMemoryStore.upsert line 44). + store.upsert(flag("a", "1")) + + // CRITICAL: by the time upsert returns, BOTH must have ALREADY fired — no wait. + // forEach is synchronous on the caller thread, so a regression that wraps each + // onChange in `launch()` (making fan-out asynchronous) would fail this immediate + // check because the fast subscriber wouldn't yet have run when upsert returned. + assertTrue( + "slow subscriber fired synchronously on caller thread", + slowFired.get(), + ) + assertTrue( + "fast subscriber ALSO fired synchronously — NO wait, no latch.await — pins " + + "the synchronous-fan-out invariant; a launch()-based regression would fail here", + fastFired.get(), + ) + // Latches are sanity backstops, not the load-bearing assertions. + assertEquals(0L, slowEntered.count) + assertEquals(0L, slowFinished.count) + } + + /** + * Contract: subscriber exceptions MUST NOT starve other subscribers. Each callback is + * wrapped in `safeNotify` (FlagTrackerImpl.kt:39-49) so a throwing subscriber: + * 1. Does not propagate its exception out of `dispatch` (and thus out of `store.upsert`). + * 2. Does not halt iteration over the subscriber list. + * + * This is a data-loss-prevention guarantee — callback-style APIs cannot let one consumer + * silently break delivery to all consumers registered later. + * + * Mutation that would fail this: + * * Removing the `try { ... } catch (t: Throwable) { ... }` wrapper in `safeNotify` — + * the exception escapes, `forEach` halts, after-thrower never fires AND the throw + * propagates out of `store.upsert`. + * * Catching the exception but rethrowing — same effect. + */ + @Test + fun `subscriber exception does not starve other subscribers`() { + val store = DefaultMemoryStore() + val tracker = FlagTrackerImpl(store) + + val beforeFired = AtomicBoolean(false) + val afterFired = AtomicBoolean(false) + + tracker.subscribe { _ -> beforeFired.set(true) } + tracker.subscribe { _ -> throw RuntimeException("subscriber-thrown") } + tracker.subscribe { _ -> afterFired.set(true) } + + // The exception MUST be swallowed inside dispatch — store.upsert must return normally. + store.upsert(flag("a", "1")) + + assertTrue("before-thrower subscriber fired", beforeFired.get()) + assertTrue( + "after-thrower subscriber ALSO fired — exception was swallowed by safeNotify", + afterFired.get(), + ) + } + + /** + * Contract: keyed subscriptions to different keys are isolated. A subscriber on key "a" + * never receives events for key "b" — even when both keys change in quick succession. + * + * Mutation that would fail this: + * * `keyedSubscribers[event.key]?.forEach` replaced with iterating ALL keyed lists — + * subscribers for the wrong key would also fire. + * * Bucketing keyed subscribers by something other than key (e.g. global list). + */ + @Test + fun `keyed subscribers on different keys are isolated`() { + val store = DefaultMemoryStore() + val tracker = FlagTrackerImpl(store) + val aEvents = CopyOnWriteArrayList() + val bEvents = CopyOnWriteArrayList() + + tracker.subscribe("a") { aEvents.add(it) } + tracker.subscribe("b") { bEvents.add(it) } + + store.upsert(flag("a", "1")) + store.upsert(flag("b", "2")) + store.upsert(flag("a", "3")) + + assertEquals( + "key-a subscriber sees only its key (2 events)", + listOf("1", "3"), + aEvents.map { it.newValue }, + ) + assertEquals( + "key-b subscriber sees only its key (1 event)", + listOf("2"), + bEvents.map { it.newValue }, + ) + } + + /** + * Contract: the flagChanges Flow has `replay = 0`. Events emitted while there are NO + * collectors are dropped at `tryEmit` and not replayed to a late-arriving collector. + * A collector that subscribes AFTER an upsert sees only subsequent upserts. + * + * Note on `extraBufferCapacity = 64`: that buffer is for slow ACTIVE collectors (so a + * burst of emissions doesn't suspend the emitter while the collector catches up). It is + * NOT a pre-subscription replay buffer — `replay` controls that, and we set it to 0 + * implicitly. + * + * Mutation that would fail this: + * * Adding `replay = N` (any N ≥ 1) to the MutableSharedFlow constructor — late + * collectors would replay the "before" event. + */ + @Test + fun `flagChanges Flow has no replay — late subscribers do not see prior events`() = runTest { + val store = DefaultMemoryStore() + val tracker = FlagTrackerImpl(store) + + // Emit 5 events BEFORE anyone collects. With replay=0 and no active collector, + // tryEmit drops them all. Five (not one) so a regression that adds `replay = 1` or + // `replay = 3` would still be caught — the test must see "after" first, not any + // pre-subscription value. + repeat(5) { i -> store.upsert(flag("a", "before-$i")) } + + tracker.flagChanges.test { + store.upsert(flag("a", "after")) + val first = awaitItem() + assertEquals( + "late collector sees only the post-subscription event — none of the 5 " + + "pre-subscription values were replayed", + "after", + first.newValue, + ) + cancelAndIgnoreRemainingEvents() + } + } +} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/app/LifecycleControllerTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/app/LifecycleControllerTest.kt new file mode 100644 index 0000000..7b262d3 --- /dev/null +++ b/featbit-client/src/test/kotlin/co/featbit/client/app/LifecycleControllerTest.kt @@ -0,0 +1,221 @@ +package co.featbit.client.app + +import co.featbit.client.data.sync.DataSynchronizer +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.advanceTimeBy +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Test +import java.util.concurrent.atomic.AtomicInteger + +@OptIn(ExperimentalCoroutinesApi::class) +class LifecycleControllerTest { + + private val grace = 1_000L + + private class FakeSynchronizer : DataSynchronizer { + val pauses = AtomicInteger() + val resumes = AtomicInteger() + override val initialized: Boolean = true + override suspend fun start(): Boolean = true + override fun pause() { pauses.incrementAndGet() } + override fun resume() { resumes.incrementAndGet() } + override fun close() {} + } + + @Test + fun `backgrounding pauses only after the grace period`() = runTest { + val sync = FakeSynchronizer() + val controller = LifecycleController(backgroundScope, grace) { sync } + + controller.onForegroundChanged(false) + runCurrent() + assertEquals("no pause before grace elapses", 0, sync.pauses.get()) + + advanceTimeBy(grace + 1) + runCurrent() + assertEquals("pause after grace", 1, sync.pauses.get()) + } + + @Test + fun `quick foreground return within grace does not pause`() = runTest { + val sync = FakeSynchronizer() + val controller = LifecycleController(backgroundScope, grace) { sync } + + controller.onForegroundChanged(false) + runCurrent() + advanceTimeBy(grace / 2) + controller.onForegroundChanged(true) + advanceTimeBy(grace + 1) + runCurrent() + + assertEquals("never paused", 0, sync.pauses.get()) + assertEquals("never paused so no resume needed", 0, sync.resumes.get()) + } + + @Test + fun `resume after a completed pause`() = runTest { + val sync = FakeSynchronizer() + val controller = LifecycleController(backgroundScope, grace) { sync } + + controller.onForegroundChanged(false) + advanceTimeBy(grace + 1) + runCurrent() + assertEquals(1, sync.pauses.get()) + + controller.onForegroundChanged(true) + runCurrent() + assertEquals("resume on foreground", 1, sync.resumes.get()) + } + + @Test + fun `losing network pauses, regaining it resumes`() = runTest { + val sync = FakeSynchronizer() + val controller = LifecycleController(backgroundScope, grace) { sync } + + controller.onNetworkChanged(false) + advanceTimeBy(grace + 1) + runCurrent() + assertEquals(1, sync.pauses.get()) + + controller.onNetworkChanged(true) + runCurrent() + assertEquals(1, sync.resumes.get()) + } + + // --------------------------------------------------------------------------------------- + // race / flap tests (wider-scope audit H3) + // --------------------------------------------------------------------------------------- + + /** + * Contract: a foreground flap (off → on → off) within the grace window must reset the + * pause timer on the "on" transition. The pause fires exactly once, anchored at the + * second "off" event + grace — not at the first. + * + * Wall clock (grace=1000ms): + * T=0 : foreground=false (pauseJob scheduled, fires at grace) + * T=grace/2 : foreground=true (cancels pauseJob, sets pauseJob=null) + * T=grace*0.8 : foreground=false (NEW pauseJob, fires at grace*1.8) + * T=grace : assert pauses=0 — the original schedule's deadline must not fire + * T=2*grace+1 : assert pauses=1 — well past the new schedule's grace*1.8 deadline + * + * Mutation that would fail this: + * * Removing `pauseJob?.cancel()` on the foreground=true branch — the first pauseJob + * would fire at grace, producing pauses=1 at T=grace (caught by the mid-test + * assertion at line 129). + * * Removing `pauseJob = null` after cancel — the guard `pauseJob == null` on the + * "else if" branch would fail when the second `foreground=false` arrives, so the + * new pauseJob is never scheduled and pauses stays at 0 at T=2*grace+1. + */ + @Test + fun `foreground flap within grace re-anchors pause to the latest off`() = runTest { + val sync = FakeSynchronizer() + val controller = LifecycleController(backgroundScope, grace) { sync } + + controller.onForegroundChanged(false) // T=0 + runCurrent() + advanceTimeBy(grace / 2) // T=grace/2 + controller.onForegroundChanged(true) + runCurrent() + assertEquals("first pause was cancelled by the foreground return", 0, sync.pauses.get()) + + // 30% of grace later — still well before the original schedule's deadline. + advanceTimeBy((grace * 3) / 10) // T=grace*0.8 + controller.onForegroundChanged(false) + runCurrent() + + // Advance to the *original* schedule's would-have-fired time. If the cancel didn't + // work, pause would fire here. + advanceTimeBy(grace / 5) // T=grace + runCurrent() + assertEquals("pause must not fire at the original schedule's time", 0, sync.pauses.get()) + + // Now advance well past the new schedule's deadline (T=grace*1.8). + // Cumulative wall clock: grace + (grace + 1) = 2*grace+1, comfortably past grace*1.8. + advanceTimeBy(grace + 1) // T=2*grace+1 + runCurrent() + assertEquals("pause fires exactly once, anchored at the second off", 1, sync.pauses.get()) + assertEquals("no spurious resumes", 0, sync.resumes.get()) + } + + /** + * Contract: when both foreground and network are off, regaining network alone must NOT + * trigger resume — the synchronizer should only become active when BOTH are on. This is + * the phone-comes-back-online-while-still-in-background scenario. + * + * Mutation that would fail this: + * * Resume guard checking `online` instead of `foreground && online`. + * * `onNetworkChanged(true)` calling `resume()` unconditionally. + */ + @Test + fun `network on while foreground off does not resume`() = runTest { + val sync = FakeSynchronizer() + val controller = LifecycleController(backgroundScope, grace) { sync } + + controller.onForegroundChanged(false) + controller.onNetworkChanged(false) + advanceTimeBy(grace + 1) + runCurrent() + assertEquals("pause fired once after grace", 1, sync.pauses.get()) + assertEquals(0, sync.resumes.get()) + + // Only network comes back; foreground is still off. + controller.onNetworkChanged(true) + runCurrent() + assertEquals( + "must NOT resume while foreground is still false", + 0, + sync.resumes.get(), + ) + + // Foreground returns — now we should resume. + controller.onForegroundChanged(true) + runCurrent() + assertEquals("resume fires once both signals are active again", 1, sync.resumes.get()) + } + + /** + * Contract: a second "inactive" signal arriving while a pauseJob is already pending must + * NOT schedule a second job. The `pauseJob == null` guard prevents double-scheduling; + * without it, the second signal would queue another pause and `pauses` would jump to 2. + * + * Wall clock (grace=1000ms): + * T=0 : foreground=false (pauseJob #1 scheduled, fires at T=grace) + * T=grace/4 : network=false (under correct code: no-op; under mutant: pauseJob #2 + * scheduled, would fire at T=grace*1.25) + * T=grace+1 : assert pauses=1 — pauseJob #1 fired; mutant's #2 has NOT fired yet + * T=2*grace+1 : assert pauses still 1 — mutant's #2 would have fired at grace*1.25, + * so this catches the double-schedule + * + * Mutation that would fail this: + * * Removing the `pauseJob == null` guard from the `else if (active && ...)` branch — + * the second inactive signal schedules a duplicate pauseJob at T=grace/4, firing at + * T=grace*1.25. The final assertion at T=2*grace+1 sees pauses=2 instead of 1. + */ + @Test + fun `second inactive signal while pause is pending does not double-schedule`() = runTest { + val sync = FakeSynchronizer() + val controller = LifecycleController(backgroundScope, grace) { sync } + + controller.onForegroundChanged(false) // T=0, pauseJob scheduled + runCurrent() + advanceTimeBy(grace / 4) // T=grace/4 + controller.onNetworkChanged(false) // still inactive — must NOT add a second pauseJob + runCurrent() + + advanceTimeBy((grace * 3) / 4 + 1) // T=grace+1: first pauseJob should have fired + runCurrent() + assertEquals( + "pause must fire exactly once even with two consecutive inactive signals", + 1, + sync.pauses.get(), + ) + + // Wait further to catch a delayed duplicate — none should arrive. + advanceTimeBy(grace) + runCurrent() + assertEquals("no second pause fired late", 1, sync.pauses.get()) + assertEquals(0, sync.resumes.get()) + } +} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/changetracker/FlagTrackerImplTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/changetracker/FlagTrackerImplTest.kt deleted file mode 100644 index bad3e16..0000000 --- a/featbit-client/src/test/kotlin/co/featbit/client/changetracker/FlagTrackerImplTest.kt +++ /dev/null @@ -1,81 +0,0 @@ -package co.featbit.client.changetracker - -import app.cash.turbine.test -import co.featbit.client.model.FeatureFlag -import co.featbit.client.store.DefaultMemoryStore -import co.featbit.client.store.FlagValueChangedEvent -import kotlinx.coroutines.test.runTest -import org.junit.Assert.assertEquals -import org.junit.Assert.assertTrue -import org.junit.Test -import java.util.concurrent.CopyOnWriteArrayList - -class FlagTrackerImplTest { - - private fun flag(id: String, variation: String) = FeatureFlag(id = id, variation = variation) - - @Test - fun `global subscriber receives all changes`() { - val store = DefaultMemoryStore() - val tracker = FlagTrackerImpl(store) - val received = CopyOnWriteArrayList() - tracker.subscribe { received.add(it) } - - store.upsert(flag("a", "1")) - store.upsert(flag("b", "2")) - - assertEquals(2, received.size) - } - - @Test - fun `keyed subscriber only receives its own key`() { - val store = DefaultMemoryStore() - val tracker = FlagTrackerImpl(store) - val received = CopyOnWriteArrayList() - tracker.subscribe("a") { received.add(it) } - - store.upsert(flag("a", "1")) - store.upsert(flag("b", "2")) - - assertEquals(1, received.size) - assertEquals("a", received[0].key) - } - - @Test - fun `unsubscribe stops delivery`() { - val store = DefaultMemoryStore() - val tracker = FlagTrackerImpl(store) - val received = CopyOnWriteArrayList() - val listener = co.featbit.client.store.FlagChangeListener { received.add(it) } - tracker.subscribe(listener) - tracker.unsubscribe(listener) - - store.upsert(flag("a", "1")) - assertTrue(received.isEmpty()) - } - - @Test - fun `flagChanges flow emits change events`() = runTest { - val store = DefaultMemoryStore() - val tracker = FlagTrackerImpl(store) - - tracker.flagChanges.test { - store.upsert(flag("a", "1")) - assertEquals(FlagValueChangedEvent("a", null, "1"), awaitItem()) - cancelAndIgnoreRemainingEvents() - } - } - - @Test - fun `close removes listener from store`() { - val store = DefaultMemoryStore() - val tracker = FlagTrackerImpl(store) - val received = CopyOnWriteArrayList() - tracker.subscribe { received.add(it) } - - tracker.close() - store.upsert(flag("a", "1")) - - assertTrue(received.isEmpty()) - } -} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/internal/ConnectionTokenTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/data/http/ConnectionTokenTest.kt similarity index 98% rename from featbit-client/src/test/kotlin/co/featbit/client/internal/ConnectionTokenTest.kt rename to featbit-client/src/test/kotlin/co/featbit/client/data/http/ConnectionTokenTest.kt index c36a866..08568fb 100644 --- a/featbit-client/src/test/kotlin/co/featbit/client/internal/ConnectionTokenTest.kt +++ b/featbit-client/src/test/kotlin/co/featbit/client/data/http/ConnectionTokenTest.kt @@ -1,4 +1,4 @@ -package co.featbit.client.internal +package co.featbit.client.data.http import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue diff --git a/featbit-client/src/test/kotlin/co/featbit/client/data/http/FBEndpointsTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/data/http/FBEndpointsTest.kt new file mode 100644 index 0000000..f622d25 --- /dev/null +++ b/featbit-client/src/test/kotlin/co/featbit/client/data/http/FBEndpointsTest.kt @@ -0,0 +1,103 @@ +package co.featbit.client.data.http + +import co.featbit.client.options.FBOptions +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.Assert.assertThrows + +/** + * Pins the URL-construction contract for [FBEndpoints]. The class exists to be the single + * source of truth for FeatBit URLs, so these cases lock in: + * * the four streaming-URL scheme variants we accept (`ws://`, `wss://`, `http://`, `https://`), + * * malformed configuration failing eagerly at init (not on first network call), + * * the FeatBit `/streaming` path being appended exactly once. + */ +class FBEndpointsTest { + + private fun optionsWith( + pollingUri: String = "https://app-eval.featbit.co", + eventUri: String = "https://app-eval.featbit.co", + streamingUri: String = "wss://app-eval.featbit.co", + ): FBOptions = FBOptions.Builder("secret") + .polling(pollingUri) + .event(eventUri) + .streaming(streamingUri) + .build() + + @Test + fun `latestAll and insightTrack URLs append the documented paths`() { + val endpoints = FBEndpoints.from(optionsWith()) + assertEquals( + "https://app-eval.featbit.co/api/public/sdk/client/latest-all", + endpoints.latestAll.toString(), + ) + assertEquals( + "https://app-eval.featbit.co/api/public/insight/track", + endpoints.insightTrack.toString(), + ) + } + + @Test + fun `wss streaming URI becomes https with streaming path`() { + val endpoints = FBEndpoints.from(optionsWith(streamingUri = "wss://app-eval.featbit.co")) + assertEquals("https://app-eval.featbit.co/streaming", endpoints.streaming.toString()) + } + + @Test + fun `ws streaming URI becomes http with streaming path`() { + val endpoints = FBEndpoints.from(optionsWith(streamingUri = "ws://localhost:5100")) + assertEquals("http://localhost:5100/streaming", endpoints.streaming.toString()) + } + + @Test + fun `https streaming URI is preserved (compat with mis-typed config)`() { + val endpoints = FBEndpoints.from(optionsWith(streamingUri = "https://app-eval.featbit.co")) + assertEquals("https://app-eval.featbit.co/streaming", endpoints.streaming.toString()) + } + + @Test + fun `http streaming URI is preserved`() { + val endpoints = FBEndpoints.from(optionsWith(streamingUri = "http://localhost:5100")) + assertEquals("http://localhost:5100/streaming", endpoints.streaming.toString()) + } + + @Test + fun `streaming URI with explicit path is honored, segment is appended`() { + val endpoints = FBEndpoints.from(optionsWith(streamingUri = "wss://example.com/api")) + // OkHttp's `addPathSegment` appends, so a custom base path is preserved. + assertTrue( + "got ${endpoints.streaming}", + endpoints.streaming.toString().endsWith("/api/streaming"), + ) + } + + @Test + fun `malformed polling URI throws at init, not on first call`() { + // FBOptions.Builder validates non-blankness only; URL parse-ability is FBEndpoints' job. + val options = FBOptions.Builder("secret") + .polling("not a url") + .event("https://app-eval.featbit.co") + .build() + assertThrows(IllegalArgumentException::class.java) { FBEndpoints.from(options) } + } + + @Test + fun `malformed event URI throws at init`() { + val options = FBOptions.Builder("secret") + .polling("https://app-eval.featbit.co") + .event("definitely::not::a::url") + .build() + assertThrows(IllegalArgumentException::class.java) { FBEndpoints.from(options) } + } + + @Test + fun `malformed streaming URI throws at init`() { + val options = FBOptions.Builder("secret") + .polling("https://app-eval.featbit.co") + .event("https://app-eval.featbit.co") + .streaming("wss://nope:notaport") + .build() + assertThrows(IllegalArgumentException::class.java) { FBEndpoints.from(options) } + } +} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/data/http/GetUserFlagsTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/data/http/GetUserFlagsTest.kt new file mode 100644 index 0000000..fa3f578 --- /dev/null +++ b/featbit-client/src/test/kotlin/co/featbit/client/data/http/GetUserFlagsTest.kt @@ -0,0 +1,162 @@ +package co.featbit.client.data.http + +import co.featbit.client.model.FBUser +import co.featbit.client.options.FBOptions +import kotlinx.coroutines.runBlocking +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertThrows +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test + +class GetUserFlagsTest { + + private lateinit var server: MockWebServer + + @Before + fun setUp() { + server = MockWebServer() + server.start() + } + + @After + fun tearDown() { + server.shutdown() + } + + private fun newClient(): GetUserFlags { + val options = FBOptions.Builder("env-secret") + .polling(server.url("/").toString()) + .event(server.url("/").toString()) + .build() + val user = FBUser.builder("u1").name("bob").custom("country", "FR").build() + return GetUserFlags(options, user) + } + + @Test + fun `200 parses data featureFlags and sends auth, user-agent, timestamp`() = runBlocking { + server.enqueue( + MockResponse().setResponseCode(200).setBody( + """{"data":{"featureFlags":[ + {"id":"flag1","variation":"true","variationType":"boolean", + "variationId":"v1","sendToExperiment":false,"matchReason":"rule match"} + ]}}""".trimIndent(), + ), + ) + + val client = newClient() + val response = client.run(timestamp = 0) + client.close() + + assertFalse(response.isError) + assertEquals(1, response.flags.size) + assertEquals("flag1", response.flags[0].id) + assertEquals("true", response.flags[0].variation) + + val recorded = server.takeRequest() + assertEquals("POST", recorded.method) + assertEquals("0", recorded.requestUrl?.queryParameter("timestamp")) + assertTrue(recorded.path!!.startsWith("/api/public/sdk/client/latest-all")) + assertEquals("env-secret", recorded.getHeader("Authorization")) + assertEquals("featbit-kotlin-client-sdk", recorded.getHeader("User-Agent")) + } + + @Test + fun `empty body yields empty flags`() = runBlocking { + server.enqueue(MockResponse().setResponseCode(200).setBody("")) + val client = newClient() + val response = client.run(0) + client.close() + assertFalse(response.isError) + assertTrue(response.flags.isEmpty()) + } + + @Test + fun `401 is fatal, 500 is transient`() = runBlocking { + server.enqueue(MockResponse().setResponseCode(401)) + val client = newClient() + val fatal = client.run(0) + assertTrue(fatal.isFatal) + assertTrue(fatal.isError) + + server.enqueue(MockResponse().setResponseCode(500)) + val transient = client.run(0) + client.close() + assertFalse(transient.isFatal) + assertTrue(transient.isError) + } + + @Test + fun `wire-shape mismatch on data field throws (not silent zero flags)`() = runBlocking { + // Regression: an earlier refactor silently downgraded malformed payloads to "no flags", + // which is indistinguishable from a legitimate empty configuration and made the SDK + // serve defaults forever. The shape mismatch must surface as an exception so the + // caller's `safePoll` logs it as an error. + server.enqueue( + MockResponse().setResponseCode(200).setBody("""{"data":"oops, not an object"}"""), + ) + val client = newClient() + assertThrows(Exception::class.java) { + runBlocking { client.run(0) } + } + client.close() + } + + @Test + fun `missing data field yields empty flags (legitimate empty config)`() = runBlocking { + server.enqueue(MockResponse().setResponseCode(200).setBody("{}")) + val client = newClient() + val response = client.run(0) + client.close() + assertFalse(response.isError) + assertTrue("no flags configured for user — not an error", response.flags.isEmpty()) + } + + @Test + fun `missing featureFlags key yields empty flags`() = runBlocking { + server.enqueue(MockResponse().setResponseCode(200).setBody("""{"data":{}}""")) + val client = newClient() + val response = client.run(0) + client.close() + assertFalse(response.isError) + assertTrue(response.flags.isEmpty()) + } + + /** + * Contract: an explicit JSON null for the `data` field is a malformed payload — server + * never emits this for a legitimate empty config (that case is "field absent" or + * `{"data": {}}`). The old AST-walking code threw on `JsonNull.jsonObject`; the typed + * envelope must preserve that throw-on-null surface so `safePoll` logs an error and + * polling retries the next interval rather than silently serving defaults forever. + * + * Distinguishing inputs: + * * `{}` (field absent) → empty flags. Tested separately. Legitimate "no config". + * * `{"data": null}` → THROWS. Malformed payload. + * * `{"data": {}}` → empty flags. Empty config object. + * * `{"data": {"featureFlags": []}}` → empty flags. Empty array form. + * * `{"data": "string"}` → THROWS. Wrong type. + * + * Mutation that would fail this: + * * Reverting `LatestAllEnvelope(val data: LatestAllData = LatestAllData())` to + * `val data: LatestAllData? = null` — combined with the project's + * `explicitNulls = false` Json config, JSON null decodes to Kotlin null and + * `envelope.data?.featureFlags ?: emptyList()` silently returns empty. Empirically + * confirmed: kotlinx-serialization 1.6.3 + `explicitNulls=false` coerces JSON null + * to Kotlin null for nullable fields without throwing. + */ + @Test + fun `explicit null data field throws (not silent zero flags)`() = runBlocking { + server.enqueue( + MockResponse().setResponseCode(200).setBody("""{"data": null}"""), + ) + val client = newClient() + assertThrows(Exception::class.java) { + runBlocking { client.run(0) } + } + client.close() + } +} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/data/insights/InsightDispatcherTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/data/insights/InsightDispatcherTest.kt new file mode 100644 index 0000000..989a4e8 --- /dev/null +++ b/featbit-client/src/test/kotlin/co/featbit/client/data/insights/InsightDispatcherTest.kt @@ -0,0 +1,204 @@ +package co.featbit.client.data.insights + +import co.featbit.client.FBLogger +import co.featbit.client.model.FBUser +import co.featbit.client.model.FeatureFlag +import co.featbit.client.wire.Insight +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.test.advanceTimeBy +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Edge-case coverage for [InsightDispatcher]. The dispatcher sits on the evaluation hot path, + * so we exercise: batch-by-size, batch-by-time, overflow-drops-oldest, tracker-exception + * doesn't kill the consumer, [InsightDispatcher.closeAndDrain] actually flushes, and offer + * after close is a silent no-op. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class InsightDispatcherTest { + + private fun insight(i: Int): Insight = Insight.forEvaluation( + user = FBUser.builder("u$i").build(), + flag = FeatureFlag(id = "f$i", variation = "v$i"), + timestamp = i.toLong(), + ) + + private fun Insight.firstFlagKey(): String = variations[0].featureFlagKey + + private class RecordingTracker( + private val onBatchSuspend: (suspend (List) -> Unit)? = null, + private val throwOnce: Boolean = false, + ) : TrackInsight { + val batches = mutableListOf>() + val closed = CompletableDeferred() + private var hasThrown = false + + override suspend fun run(batch: List) { + if (throwOnce && !hasThrown) { + hasThrown = true + throw RuntimeException("first batch fails") + } + onBatchSuspend?.invoke(batch) + batches += batch + } + + override fun close() { + closed.complete(Unit) + } + } + + @Test + fun `batches by size before the flush interval elapses`() = runTest { + val tracker = RecordingTracker() + val dispatcher = InsightDispatcher(tracker, this, FBLogger.NoOp, batchSize = 3) + + repeat(3) { dispatcher.offer(insight(it)) } + advanceUntilIdle() + + assertEquals(1, tracker.batches.size) + assertEquals(listOf("f0", "f1", "f2"), tracker.batches[0].map { it.firstFlagKey() }) + + dispatcher.closeAndDrain() + } + + @Test + fun `batches by time when fewer than batchSize events arrive`() = runTest { + val tracker = RecordingTracker() + val dispatcher = InsightDispatcher( + tracker, this, FBLogger.NoOp, + batchSize = 50, + flushIntervalMs = 1_000L, + ) + + dispatcher.offer(insight(1)) + dispatcher.offer(insight(2)) + advanceTimeBy(1_001L) + runCurrent() + + assertEquals("one time-based batch", 1, tracker.batches.size) + assertEquals(2, tracker.batches[0].size) + + dispatcher.closeAndDrain() + } + + @Test + fun `overflow drops the oldest event when queue is full`() = runTest { + // Hold the tracker on its first batch so the channel actually fills behind it. + val release = CompletableDeferred() + val tracker = RecordingTracker(onBatchSuspend = { release.await() }) + // Tiny capacity makes overflow trivial to trigger without 256+ allocations. + val dispatcher = InsightDispatcher( + tracker, this, FBLogger.NoOp, + capacity = 4, + batchSize = 100, + flushIntervalMs = 10_000L, + ) + + repeat(8) { dispatcher.offer(insight(it)) } + runCurrent() + + // Release the consumer + push past the flush interval so it batches what's queued. + release.complete(Unit) + advanceTimeBy(10_001L) + advanceUntilIdle() + + // With consumer hung on its first `release.await()` AT THE TRACKER LEVEL — wait, no. + // The 8 offers complete BEFORE the consumer's scope.launch is scheduled (we explicitly + // did `runCurrent()` only after the offer loop). So all 8 offers race the consumer's + // start: channel fills to capacity=4 with f0..f3, then f4..f7 each evict the head via + // DROP_OLDEST. Consumer's first receive sees [f4, f5, f6, f7]. Flush waits for the + // 10_000ms flush interval (batchSize=100 not reached), then emits a single batch. + // + // Exact-equality assertion (not "size <= 5 + contains f7") so a broken implementation + // that delivered only [f7], kept f0..f3 instead of f4..f7, or emitted in wrong order + // would fail this test. + assertEquals("exactly one batch — the 4 surviving newest events", 1, tracker.batches.size) + assertEquals(listOf("f4", "f5", "f6", "f7"), tracker.batches[0].map { it.firstFlagKey() }) + + dispatcher.closeAndDrain() + } + + @Test + fun `tracker exception is logged and does not kill the consumer`() = runTest { + val tracker = RecordingTracker(throwOnce = true) + val dispatcher = InsightDispatcher(tracker, this, FBLogger.NoOp, batchSize = 1) + + dispatcher.offer(insight(1)) // first run throws — should be swallowed + advanceUntilIdle() + dispatcher.offer(insight(2)) // second batch should still go through + advanceUntilIdle() + + assertEquals("only the surviving batch was recorded", 1, tracker.batches.size) + assertEquals("f2", tracker.batches[0][0].firstFlagKey()) + + dispatcher.closeAndDrain() + } + + @Test + fun `closeAndDrain flushes pending events and closes the tracker`() = runTest { + val tracker = RecordingTracker() + val dispatcher = InsightDispatcher( + tracker, this, FBLogger.NoOp, + batchSize = 100, + flushIntervalMs = 10_000L, + ) + + dispatcher.offer(insight(1)) + dispatcher.offer(insight(2)) + runCurrent() + assertTrue("no batch yet — still waiting on the flush timer", tracker.batches.isEmpty()) + + dispatcher.closeAndDrain() + + assertEquals("close triggered a flush", 1, tracker.batches.size) + assertEquals(2, tracker.batches[0].size) + assertTrue("tracker was closed", tracker.closed.isCompleted) + } + + @Test + fun `offer after close is a silent no-op`() = runTest { + val tracker = RecordingTracker() + val dispatcher = InsightDispatcher(tracker, this, FBLogger.NoOp, batchSize = 1) + + dispatcher.closeAndDrain() + + // Should not throw even though the channel is closed. + dispatcher.offer(insight(99)) + advanceUntilIdle() + + assertTrue("no batch sent after close", tracker.batches.isEmpty()) + } + + @Test + fun `closeAndDrain bounded by closeFlushTimeoutMs when tracker hangs`() = runTest { + // Tracker.run suspends forever — closeAndDrain must still return on its own timeout. + // We host the dispatcher on `backgroundScope` so the leaked consumer coroutine is + // cancelled when `runTest` finishes; otherwise the suspended `tracker.run` would + // pin the test scope open. + val trapped = Channel(1) + val tracker = object : TrackInsight { + override suspend fun run(batch: List) { + trapped.receive() // never completes + } + override fun close() {} + } + val dispatcher = InsightDispatcher( + tracker, backgroundScope, FBLogger.NoOp, + batchSize = 1, + closeFlushTimeoutMs = 250L, + ) + + dispatcher.offer(insight(1)) + runCurrent() + + // If the bounded timeout didn't work, this would hang the test forever. + dispatcher.closeAndDrain() + } +} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/data/insights/TrackInsightTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/data/insights/TrackInsightTest.kt new file mode 100644 index 0000000..487daec --- /dev/null +++ b/featbit-client/src/test/kotlin/co/featbit/client/data/insights/TrackInsightTest.kt @@ -0,0 +1,283 @@ +package co.featbit.client.data.insights + +import co.featbit.client.model.FBUser +import co.featbit.client.model.FeatureFlag +import co.featbit.client.wire.Insight +import co.featbit.client.options.FBOptions +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.delay +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking +import kotlinx.serialization.json.Json +import kotlinx.serialization.json.jsonArray +import kotlinx.serialization.json.jsonObject +import kotlinx.serialization.json.jsonPrimitive +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import okhttp3.mockwebserver.SocketPolicy +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test + +class TrackInsightTest { + + private lateinit var server: MockWebServer + + @Before + fun setUp() { + server = MockWebServer() + server.start() + } + + @After + fun tearDown() { + server.shutdown() + } + + private fun newTracker(): HttpTrackInsight { + val options = FBOptions.Builder("env-secret") + .polling(server.url("/").toString()) + .event(server.url("/").toString()) + .build() + return HttpTrackInsight(options) + } + + private fun insight(flagId: String, userKey: String = "u1", ts: Long = 100): Insight { + val user = FBUser.builder(userKey).name(userKey).build() + val flag = FeatureFlag(id = flagId, variation = "true", variationId = "v$flagId") + return Insight.forEvaluation(user, flag, ts) + } + + @Test + fun `posts a single-element insight array to track endpoint`() = runBlocking { + server.enqueue(MockResponse().setResponseCode(200)) + + val tracker = newTracker() + tracker.run(insight("flag1")) + tracker.close() + + val recorded = server.takeRequest() + assertTrue(recorded.path!!.startsWith("/api/public/insight/track")) + val body = recorded.body.readUtf8() + assertTrue("payload should be a JSON array: $body", body.startsWith("[")) + assertTrue(body, body.contains("\"featureFlagKey\":\"flag1\"")) + assertTrue(body, body.contains("\"keyId\":\"u1\"")) + } + + // --------------------------------------------------------------------------------------- + // batch + error-handling + lifecycle tests (wider-scope audit M8) + // --------------------------------------------------------------------------------------- + + /** + * Contract: `run(batch)` serializes the entire batch as a JSON array containing one + * object per insight. Each element has its own `keyId` (from the per-insight user) and + * featureFlagKey. The batch is sent in a single POST. + * + * Mutation that would fail this: + * * Iterating the batch with one POST per insight — server would record N requests. + * * Serializing only the first insight — JSON array would have 1 element. + * * Wrong serializer (`Insight.serializer()` instead of `ListSerializer(Insight.serializer())`) + * — payload would be a JSON object, not array. + */ + @Test + fun `multi-element batch is posted as a single JSON array`() = runBlocking { + server.enqueue(MockResponse().setResponseCode(200)) + + val tracker = newTracker() + val batch = listOf( + insight("flag-a", userKey = "u1", ts = 100L), + insight("flag-b", userKey = "u2", ts = 200L), + insight("flag-c", userKey = "u3", ts = 300L), + ) + tracker.run(batch) + tracker.close() + + // Exactly one HTTP request — the whole batch in a single POST. + assertEquals("batch posted as one request", 1, server.requestCount) + + val body = server.takeRequest().body.readUtf8() + val arr = Json.parseToJsonElement(body).jsonArray + assertEquals("array has one entry per insight", 3, arr.size) + + // Pin per-element shape: keyId from user + featureFlagKey + timestamp from variation. + // Without per-field assertions, a serializer regression dropping timestamp or + // mis-serializing variations would slip through. + val keys = arr.map { it.jsonObject["user"]!!.jsonObject["keyId"]!!.jsonPrimitive.content } + assertEquals(listOf("u1", "u2", "u3"), keys) + + val variations = arr.map { it.jsonObject["variations"]!!.jsonArray[0].jsonObject } + val flagKeys = variations.map { it["featureFlagKey"]!!.jsonPrimitive.content } + assertEquals(listOf("flag-a", "flag-b", "flag-c"), flagKeys) + + // Timestamp pins prove the wire-format field survives serialization in-order. + val timestamps = variations.map { it["timestamp"]!!.jsonPrimitive.content.toLong() } + assertEquals(listOf(100L, 200L, 300L), timestamps) + + // sendToExperiment is a bool field on VariationInsight; pin it to catch regressions + // that silently drop optional fields (e.g. a wrong @SerialName). + val sendToExperiment = variations.map { it["sendToExperiment"]!!.jsonPrimitive.content } + assertEquals(listOf("false", "false", "false"), sendToExperiment) + } + + /** + * Contract: an empty batch is a no-op — production guards with `if (batch.isEmpty()) return`. + * No HTTP request is issued. + * + * Mutation that would fail this: + * * Removing the empty-batch guard — `post(endpoint, payload)` would still fire with + * an empty JSON array `[]`, producing one stray request. + */ + @Test + fun `empty batch is a no-op — no HTTP request issued`() = runBlocking { + // No MockResponse enqueued — any actual request would result in MockWebServer's + // default behavior (HTTP 404 from no enqueued response, but we want zero requests). + val tracker = newTracker() + tracker.run(emptyList()) + tracker.close() + + assertEquals("no request issued for empty batch", 0, server.requestCount) + } + + /** + * Contract: network failures are swallowed — `run` catches the generic Exception and + * logs. Production must NOT propagate the failure to the caller because insight loss is + * acceptable but breaking the calling code path (evaluation, identify) is not. + * + * Setup: force a genuine network error by disconnecting the socket at request start. + * OkHttp throws IOException; production's `catch (ex: Exception)` swallows it. (Note: + * a 5xx response by itself does NOT throw — `execute()` returns a normal Response + * object with status=500. Only socket/connect errors raise exceptions.) + * + * Mutation that would fail this: + * * Removing the `catch (ex: Exception)` block — the IOException would propagate. + * * Replacing `Exception` with a narrower type that doesn't include IOException — + * same outcome. + */ + @Test + fun `network error is swallowed and does not propagate`() = runBlocking { + server.enqueue(MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START)) + + val tracker = newTracker() + // Must return normally — IOException from the disconnected socket is caught. + tracker.run(insight("flag1")) + tracker.close() + + // The connection attempt was made (server saw it before disconnect). + assertTrue("network attempt was made", server.requestCount >= 1) + } + + /** + * Contract (HONEST observation): + * + * Production `run` has `catch (ce: CancellationException) { throw ce }` ahead of the + * generic `catch (ex: Exception)`. The CE-specific catch only catches CE thrown at a + * *suspension* point. But the body of `post(endpoint, payload)` is + * `withContext(Dispatchers.IO) { client.newCall(request).execute() }` — `execute()` is a + * *blocking* JVM call. When the parent coroutine cancels, kotlinx delivers it via thread + * interrupt; OkHttp converts that to `InterruptedIOException`, which is a generic + * Exception, not CancellationException. The CE-specific catch is unreachable for HTTP + * post — the generic branch swallows + logs. + * + * This test pins OBSERVED behavior: cancelling the launched job that holds `tracker.run` + * leaves `job.isCancelled == true` (kotlinx side) but the lambda body itself returns + * normally because the underlying InterruptedIOException was swallowed. + * + * Mutation that would fail this: + * * Removing the entire try/catch around the post call — the InterruptedIOException + * would propagate, the launched lambda would terminate exceptionally, and + * `bodyReturnedNormally` would stay false. + * + * Follow-up: if structured-concurrency-aware InterruptedIOException handling is desired, + * `post()` should map InterruptedIOException → CancellationException, letting the + * existing `catch (ce)` branch rethrow it. Out of scope for this commit. + */ + @Test + fun `cancellation does not propagate as exception out of tracker run (current behavior)`() { + // No enqueued response: server accepts the socket but never responds. Use a snappy + // OkHttpClient (1s readTimeout) so the test's wall-clock budget isn't bound by + // OkHttp's 8s default — cancellation must surface within ~1s either way. + val snappy = okhttp3.OkHttpClient.Builder() + .readTimeout(1, java.util.concurrent.TimeUnit.SECONDS) + .build() + val options = FBOptions.Builder("env-secret") + .polling(server.url("/").toString()) + .event(server.url("/").toString()) + .build() + val tracker = HttpTrackInsight(options, httpClient = snappy) + + var bodyReturnedNormally = false + var bodyThrew = false + try { + runBlocking { + val job = launch { + try { + tracker.run(insight("flag1")) + bodyReturnedNormally = true + } catch (ce: CancellationException) { + bodyThrew = true + throw ce + } catch (t: Throwable) { + bodyThrew = true + throw t + } + } + delay(100) // let HTTP call start + job.cancelAndJoin() + } + } finally { + tracker.close() + } + assertTrue( + "current production: tracker.run swallows the InterruptedIOException via the " + + "generic catch — body returns normally despite cancellation", + bodyReturnedNormally, + ) + assertEquals("body did NOT throw", false, bodyThrew) + } + + /** + * Contract: [NoopTrackInsight] is the offline implementation. Both `run` and `close` + * are no-ops — must not throw, must not perform any I/O. + * + * Mutation that would fail this: + * * NoopTrackInsight.run throwing UnsupportedOperationException — typical defensive + * trap for accidentally-called methods. Production explicitly opts for no-op. + */ + @Test + fun `NoopTrackInsight run and close are no-ops`() = runBlocking { + // Run with a batch + a single insight + empty batch + close. All must return normally. + NoopTrackInsight.run(emptyList()) + NoopTrackInsight.run(listOf(insight("flag1"))) + NoopTrackInsight.run(insight("flag2")) + NoopTrackInsight.close() + + // Sanity: no MockWebServer interaction because NoopTrackInsight has no client. + assertEquals(0, server.requestCount) + } + + /** + * Contract: `HttpTrackInsight` shuts down its owned OkHttp dispatcher on `close()` + * (mirrors `FbApiClient.ownsClient` pattern). Verified indirectly by ensuring close() + * doesn't throw + subsequent run() doesn't make any new HTTP request because the + * client's dispatcher is shutting down (or, depending on OkHttp behavior, the request + * may still go through; the strict-binding contract is "close() doesn't throw"). + * + * Mutation that would fail this: + * * close() throwing — would fail this test outright. + * * close() being a no-op AND a later run() succeeding without a server-side request + * count change (would indicate close() didn't release resources, but we can't pin + * dispatcher state from outside without reflection). + */ + @Test + fun `close completes without throwing`() { + val tracker = newTracker() + // Multiple closes must be safe (idempotent). + tracker.close() + tracker.close() + // Sanity: no MockWebServer interaction from the close calls. + assertEquals(0, server.requestCount) + } +} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/data/sync/PollingDataSynchronizerTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/data/sync/PollingDataSynchronizerTest.kt new file mode 100644 index 0000000..27cf276 --- /dev/null +++ b/featbit-client/src/test/kotlin/co/featbit/client/data/sync/PollingDataSynchronizerTest.kt @@ -0,0 +1,306 @@ +package co.featbit.client.data.sync + +import co.featbit.client.model.FBUser +import co.featbit.client.model.FeatureFlag +import co.featbit.client.app.DefaultMemoryStore +import co.featbit.client.domain.MemoryStore +import co.featbit.client.options.FBOptions +import co.featbit.client.store.FlagChangeListener +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.async +import kotlinx.coroutines.delay +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeoutOrNull +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.minutes +import kotlin.time.Duration.Companion.seconds + +class PollingDataSynchronizerTest { + + private lateinit var server: MockWebServer + + @Before + fun setUp() { + server = MockWebServer() + server.start() + } + + @After + fun tearDown() { + server.shutdown() + } + + private fun options() = FBOptions.Builder("env-secret") + // large interval: only the first immediate poll runs during the test + .polling(server.url("/").toString(), interval = 10.minutes) + .event(server.url("/").toString()) + .build() + + @Test + fun `first successful poll initializes and populates store`() = runBlocking { + server.enqueue( + MockResponse().setResponseCode(200).setBody( + """{"data":{"featureFlags":[{"id":"flag1","variation":"true"}]}}""", + ), + ) + + val store = DefaultMemoryStore() + val user = FBUser.builder("u1").build() + val sync = PollingDataSynchronizer(options(), user, store) + + val ready = sync.start() + sync.close() + + assertTrue(ready) + assertTrue(sync.initialized) + assertEquals("true", store.get("flag1")?.variation) + } + + @Test + fun `fatal 401 stops polling and reports not ready`() = runBlocking { + server.enqueue(MockResponse().setResponseCode(401)) + + val store = DefaultMemoryStore() + val user = FBUser.builder("u1").build() + val sync = PollingDataSynchronizer(options(), user, store) + + val ready = sync.start() + sync.close() + + assertFalse(ready) + assertFalse(sync.initialized) + } + + // --------------------------------------------------------------------------------------- + // closeAndJoin / loop cadence / transient-error tests (wider-scope audit M4) + // --------------------------------------------------------------------------------------- + + /** + * Store decorator that blocks its first [upsert] call on a [CompletableDeferred]. Lets the + * test sandwich a `closeAndJoin` call between "upsert starts" and "upsert finishes" so we + * can observe whether `closeAndJoin` actually waits for the upsert to land. + * + * All other operations delegate to a [DefaultMemoryStore] held internally. + */ + private class BarrierStore : MemoryStore { + private val backing = DefaultMemoryStore() + val firstUpsertEntered = CompletableDeferred() + val release = CompletableDeferred() + @Volatile var firstUpsertFinished: Boolean = false + private var seenFirst = false + + override fun get(id: String) = backing.get(id) + override fun getAll() = backing.getAll() + override fun addChangeListener(listener: FlagChangeListener) = backing.addChangeListener(listener) + override fun removeChangeListener(listener: FlagChangeListener) = backing.removeChangeListener(listener) + + override fun upsert(flag: FeatureFlag) { + // First call blocks (synchronously) until the test releases the barrier — proves the + // window where a buggy close path could race past the upsert. + if (!seenFirst) { + seenFirst = true + firstUpsertEntered.complete(Unit) + // Block on a CountDownLatch-equivalent — we're on a synchronous, non-cancellable + // code path inside `forEach { store.upsert(it) }`, so kotlinx cancellation cannot + // preempt us here. Sleep-poll the release CompletableDeferred under a wall-clock + // deadline so a regression in the test's own release path can't hang the JUnit + // runner indefinitely (no coroutine timeout reaches this non-suspending body). + val deadlineMs = System.currentTimeMillis() + 10_000 + while (!release.isCompleted && System.currentTimeMillis() < deadlineMs) { + Thread.sleep(10) + } + check(release.isCompleted) { "BarrierStore: release was never signalled within 10s" } + } + backing.upsert(flag) + firstUpsertFinished = true + } + } + + /** + * Contract (Finding #2 fix): `closeAndJoin` must NOT return until the in-flight + * `forEach { store.upsert(it) }` loop has finished. Otherwise an upsert from the previous + * user could race past `identify()`'s synchronizer swap and contaminate the new user's + * store. + * + * Drives the race: the BarrierStore holds the first upsert until released. While it's + * held, the test calls `closeAndJoin()` in parallel and asserts that closeAndJoin DOES NOT + * return while the upsert is blocked. + * + * Mutation that would fail this: + * * Reverting `closeAndJoin` to `scope.cancel()` (non-suspending) — would return + * immediately, leaving the upsert to land afterwards. + * * Replacing `loopJob?.cancelAndJoin()` with `loopJob?.cancel()` — same defect. + */ + @Test + fun `closeAndJoin awaits in-flight upsert before returning`() = runBlocking { + // 50ms interval + extra enqueued responses: lets the post-close loop-termination + // assertion be meaningful. A still-running loop after closeAndJoin would issue several + // requests within the 200ms wait window, blowing up the equality assertion. + repeat(8) { + server.enqueue( + MockResponse().setResponseCode(200).setBody( + """{"data":{"featureFlags":[{"id":"flagA","variation":"v1"}]}}""", + ), + ) + } + val fastOptions = FBOptions.Builder("env-secret") + .polling(server.url("/").toString(), interval = 50.milliseconds) + .event(server.url("/").toString()) + .build() + + val barrier = BarrierStore() + val user = FBUser.builder("u1").build() + val sync = PollingDataSynchronizer(fastOptions, user, barrier) + + // start() suspends until the first poll initializes. The barrier blocks the upsert, + // so we kick off start() async and wait for the barrier to be entered. + val startDeferred = async { sync.start() } + withTimeoutOrNull(5.seconds) { barrier.firstUpsertEntered.await() } + ?: error("upsert was never invoked — server response not consumed?") + + // The upsert is currently blocked. closeAndJoin must NOT return while that's the case. + val closeDeferred = async { sync.closeAndJoin() } + delay(100) // give closeAndJoin a chance to run if it would return prematurely + assertFalse( + "closeAndJoin returned while upsert was still blocked — race window open", + closeDeferred.isCompleted, + ) + + // Release the upsert. closeAndJoin should now return. + barrier.release.complete(Unit) + withTimeoutOrNull(5.seconds) { closeDeferred.await() } + ?: error("closeAndJoin did not return within 5s after upsert was released") + + assertTrue("upsert ran to completion", barrier.firstUpsertFinished) + assertEquals( + "flag landed in the store before closeAndJoin returned", + "v1", + barrier.get("flagA")?.variation, + ) + + // start() was awaiting `startTask.complete(...)`. Because closeAndJoin AWAITED the + // in-flight upsert (the whole point of the fix), the upsert ran to completion BEFORE + // closeAndJoin returned — which means `initializedFlag.compareAndSet(false, true)` ran, + // `startTask.complete(true)` fired, and `start()` resolves to `true`. The + // `startTask.complete(false)` inside closeAndJoin's cancel-path is then a no-op (the + // deferred is already completed). + // + // This `true` is the load-bearing assertion for the WHOLE TEST: a buggy closeAndJoin + // that returned BEFORE the upsert (e.g. `loopJob?.cancel()` instead of `.cancelAndJoin()`) + // would race past the in-flight forEach and complete startTask with `false` first, + // giving us `startResult = false`. Exact-equality on `true` catches that mutation. + val startResult = withTimeoutOrNull(1.seconds) { startDeferred.await() } + assertEquals( + "start() must return true — closeAndJoin awaited the upsert that initialized us", + true, + startResult, + ) + + // Lock down loop termination: after closeAndJoin returns, the polling loop must be + // dead. Snapshot requestCount, wait 200ms (= 4 polling intervals at 50ms), assert no + // new requests landed. A mutant that left the loop running would issue ≥3 polls in + // this window. A mutant that removed the inner `delay(pollingInterval)` would issue + // hundreds. The exact-equality assertion catches both. + val requestsAfterClose = server.requestCount + delay(200) + assertEquals( + "no new polling requests after closeAndJoin — loop terminated", + requestsAfterClose, + server.requestCount, + ) + } + + /** + * Contract: the polling loop runs `safePoll` repeatedly, with `delay(pollingInterval)` + * between iterations. Across the test's window we expect at least 3 polls. + * + * Mutation that would fail this: + * * Removing the `while (true)` from `pollingLoop` — only 1 request would land. + * * Setting the interval too long (we'd see fewer requests). + */ + @Test + fun `polling loop issues repeated requests across the polling interval`() = runBlocking { + val flagBody = """{"data":{"featureFlags":[{"id":"f","variation":"v"}]}}""" + // Enqueue enough responses to satisfy ~3 polls within the test window. + repeat(4) { server.enqueue(MockResponse().setResponseCode(200).setBody(flagBody)) } + + val store = DefaultMemoryStore() + val user = FBUser.builder("u1").build() + // 50ms interval × 3 polls = 100-150ms wall-clock; well under MockWebServer's keep-alive. + val fastOptions = FBOptions.Builder("env-secret") + .polling(server.url("/").toString(), interval = 50.milliseconds) + .event(server.url("/").toString()) + .build() + val sync = PollingDataSynchronizer(fastOptions, user, store) + + try { + assertTrue("first poll initializes", sync.start()) + // Initial poll already consumed 1 enqueued response. Wait for two more interval ticks. + // Polling cadence: poll → delay(50) → poll → delay(50) → poll. + delay(180) + // The recorded request count is the strongest invariant — every request the + // SDK *issued* shows up here even if the response was processed asynchronously. + assertTrue( + "expected ≥3 requests within ~180ms (got ${server.requestCount})", + server.requestCount >= 3, + ) + } finally { + sync.close() + } + } + + /** + * Contract: a transient 5xx response is logged but does NOT stop the loop. The loop + * continues polling; the next successful response initializes the synchronizer. + * + * Mutation that would fail this: + * * Replacing `return` (line 77 of PollingDataSynchronizer) with `close()` — loop dies. + * * Treating 500 as fatal (e.g. broadening `isFatal` past 401). + * * Skipping `safePoll`'s catch and letting the exception propagate up `pollingLoop` — + * the coroutine would terminate before the second response. + */ + @Test + fun `transient 500 does not stop the loop and next 200 initializes`() = runBlocking { + // First poll: 500 (transient). Second poll: 200 with a real flag payload. + server.enqueue(MockResponse().setResponseCode(500)) + server.enqueue( + MockResponse().setResponseCode(200).setBody( + """{"data":{"featureFlags":[{"id":"f","variation":"survived"}]}}""", + ), + ) + + val store = DefaultMemoryStore() + val user = FBUser.builder("u1").build() + val fastOptions = FBOptions.Builder("env-secret") + .polling(server.url("/").toString(), interval = 50.milliseconds) + .event(server.url("/").toString()) + .build() + val sync = PollingDataSynchronizer(fastOptions, user, store) + + try { + val ready = withTimeoutOrNull(2.seconds) { sync.start() } + assertEquals("synchronizer initialized after recovering from the 500", true, ready) + assertTrue(sync.initialized) + assertEquals( + "the post-recovery payload landed in the store", + "survived", + store.get("f")?.variation, + ) + assertTrue( + "exactly the 500 + the 200 were issued (no extra request between)", + server.requestCount >= 2, + ) + } finally { + sync.close() + } + } +} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/data/sync/StreamingDataSynchronizerTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/data/sync/StreamingDataSynchronizerTest.kt new file mode 100644 index 0000000..aa601c3 --- /dev/null +++ b/featbit-client/src/test/kotlin/co/featbit/client/data/sync/StreamingDataSynchronizerTest.kt @@ -0,0 +1,308 @@ +package co.featbit.client.data.sync + +import co.featbit.client.model.FBUser +import co.featbit.client.options.FBOptions +import co.featbit.client.app.DefaultMemoryStore +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.async +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeoutOrNull +import kotlinx.serialization.json.Json +import kotlinx.serialization.json.JsonObject +import kotlinx.serialization.json.jsonObject +import kotlinx.serialization.json.jsonPrimitive +import kotlinx.serialization.json.long +import okhttp3.OkHttpClient +import okhttp3.WebSocket +import okhttp3.WebSocketListener +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import java.util.concurrent.CopyOnWriteArrayList +import java.util.concurrent.TimeUnit +import kotlin.time.Duration.Companion.seconds + +/** + * Unit-level coverage for [StreamingDataSynchronizer] driven against [MockWebServer]'s + * WebSocket support. Complements the env-gated E2E test (which exercises a real FeatBit + * stack) by pinning the synchronizer's contract independently of the backend: + * + * * `start()` opens a WebSocket, sends `data-sync`, and resolves once the server's reply + * has been upserted into the store. + * * `closeAndJoin()` preserves the store — flags fetched before close survive. + * * `pause()` closes the live WebSocket with NORMAL_CLOSURE + `"paused"` reason; + * `resume()` opens a fresh one with an advanced `timestamp` from the prior snapshot. + * + * Deterministic via `MockWebServer` + a short-`pingInterval` test client. Each test owns its + * own MockWebServer instance to avoid cross-test bleed via OkHttp's connection pool. + * + * Note on the `if (closed) return` guard in `Listener.onMessage`: + * A unit test that proves the guard's effect would need to invoke `onMessage` AFTER + * `closed = true` was set but BEFORE the WebSocket finished tearing down. From the + * server-side `WebSocket.send(...)` API, OkHttp drops the frame at the wire (because the + * server-side socket is also closed after `closeAndJoin`) — so the listener's `onMessage` + * is never invoked and the guard is never reached through the public surface. Verified by + * mutating the guard to a no-op and watching this test class still pass. The guard is + * exercised by the streaming E2E test (`FeatBitStreamingE2ETest`) which runs identify-time + * swaps against a real FeatBit stack and would observe stale upserts if it regressed. + */ +class StreamingDataSynchronizerTest { + + private lateinit var server: MockWebServer + + /** + * Per-test [WebSocketListener] capturing client-side traffic + giving tests a hook to + * push server-side `data-sync` frames at deterministic moments. + */ + private class ServerSide : WebSocketListener() { + private val openedDeferred = CompletableDeferred() + private val firstMessageDeferred = CompletableDeferred() + private val closedDeferred = CompletableDeferred() + val received = CopyOnWriteArrayList() + + data class CloseInfo(val code: Int, val reason: String) + + suspend fun awaitOpen(): WebSocket = + withTimeoutOrNull(5.seconds) { openedDeferred.await() } + ?: error("server-side WebSocket never opened within 5s") + + suspend fun awaitFirstMessage(): String = + withTimeoutOrNull(5.seconds) { firstMessageDeferred.await() } + ?: error("server-side never received the data-sync within 5s") + + suspend fun awaitClose(): CloseInfo = + withTimeoutOrNull(5.seconds) { closedDeferred.await() } + ?: error("server-side never observed onClosing within 5s") + + override fun onOpen(webSocket: WebSocket, response: okhttp3.Response) { + openedDeferred.complete(webSocket) + } + + override fun onMessage(webSocket: WebSocket, text: String) { + received += text + firstMessageDeferred.complete(text) + } + + override fun onClosing(webSocket: WebSocket, code: Int, reason: String) { + closedDeferred.complete(CloseInfo(code, reason)) + webSocket.close(1000, null) + } + } + + /** + * Wire-format helper: build the server reply the SDK expects for a single-flag snapshot. + * Matches the shape decoded by `StreamingDataSynchronizer.DataSyncPayload`. Inline + * whitespace is tolerated by kotlinx-serialization's JSON parser (no normalization needed). + */ + private fun dataSyncFrame(flagId: String, variation: String): String = + """ + {"messageType":"data-sync","data":{"eventType":"full","userKeyId":"u1","featureFlags":[ + {"id":"$flagId","variation":"$variation","variationType":"boolean", + "variationId":"v1","sendToExperiment":false,"matchReason":"fallthrough"} + ]}} + """.trimIndent() + + private fun streamingOptions(): FBOptions { + // MockWebServer hands out `http://...` URLs; FBEndpoints accepts that form and rewrites + // to `/streaming` — verified by `FBEndpointsTest`. Using http:// directly keeps the + // test free of the `ws://` rewrite branch (already covered by `FBEndpointsTest`). + val baseUrl = server.url("/").toString() + return FBOptions.Builder("test-secret") + .streaming(baseUrl) + .event(baseUrl) + .build() + } + + /** + * Snappy OkHttpClient — production default pings every 20s which is far slower than tests + * want to run. A 1s ping isn't required for these tests but keeps idle sockets quick. + */ + private fun snappyClient(): OkHttpClient = OkHttpClient.Builder() + .pingInterval(1, TimeUnit.SECONDS) + .build() + + /** + * Parse the client's outgoing data-sync envelope so tests can assert against typed JSON + * rather than substring-matching the raw string. + */ + private fun decodeClientEnvelope(text: String): JsonObject = + Json.parseToJsonElement(text).jsonObject + + @Before + fun setUp() { + server = MockWebServer() + server.start() + } + + @After + fun tearDown() { + server.shutdown() + } + + /** + * Contract: `start()` must (a) open the WebSocket, (b) send a `data-sync` envelope with + * exact shape `{messageType: "data-sync", data: {user: {keyId, ...}, timestamp: 0}}`, and + * (c) suspend until the server's reply has been processed into the store and `initialized` + * flips true. + * + * Mutation that would fail this: + * * Removing `payload.featureFlags.forEach(store::upsert)` from `handleMessage`. + * * Skipping `initializedFlag.compareAndSet(false, true)` on first message. + * * `sendDataSync` emitting wrong messageType, wrong user.keyId, or non-zero initial + * timestamp (the load-bearing "this is the first connection" invariant). + */ + @Test + fun `start opens websocket, sends data-sync, and initializes store from server snapshot`() = runBlocking { + val serverSide = ServerSide() + server.enqueue(MockResponse().withWebSocketUpgrade(serverSide)) + + val store = DefaultMemoryStore() + val sync = StreamingDataSynchronizer( + options = streamingOptions(), + user = FBUser.builder("u1").name("bob").build(), + store = store, + httpClient = snappyClient(), + ) + + try { + val readyDeferred = async { sync.start() } + + val ws = serverSide.awaitOpen() + val clientMessage = serverSide.awaitFirstMessage() + + // Structural assertions — would catch a regression that sent the wrong messageType, + // omitted the user key, or used a non-zero initial timestamp. + val envelope = decodeClientEnvelope(clientMessage) + assertEquals("data-sync", envelope["messageType"]?.jsonPrimitive?.content) + val data = envelope["data"]?.jsonObject ?: error("missing data: $clientMessage") + assertEquals("u1", data["user"]?.jsonObject?.get("keyId")?.jsonPrimitive?.content) + assertEquals( + "initial timestamp must be 0 — proves this is the first connection", + 0L, + data["timestamp"]?.jsonPrimitive?.long, + ) + + ws.send(dataSyncFrame("flag1", "true")) + + val ready = readyDeferred.await() + + assertTrue("start() reported initialized=true", ready) + assertTrue("synchronizer reports initialized", sync.initialized) + assertEquals( + "flag1 landed in the store from the server snapshot", + "true", + store.get("flag1")?.variation, + ) + } finally { + sync.closeAndJoin() + } + } + + /** + * Contract: `closeAndJoin` must NOT clear the store, and must NOT roll back `initialized`. + * After close, evaluators continue serving the last-known data (the SDK's + * offline-after-close mode). + * + * Mutation that would fail this: + * * Adding `store.clear()` or similar to `close()` / `closeAndJoin()`. + * * `closeAndJoin` mistakenly upserting empty payloads. + * * `closeAndJoin` flipping `initializedFlag` back to false. + */ + @Test + fun `closeAndJoin preserves store state and initialized flag`() = runBlocking { + val serverSide = ServerSide() + server.enqueue(MockResponse().withWebSocketUpgrade(serverSide)) + + val store = DefaultMemoryStore() + val sync = StreamingDataSynchronizer( + options = streamingOptions(), + user = FBUser.builder("u1").build(), + store = store, + httpClient = snappyClient(), + ) + + val readyDeferred = async { sync.start() } + val ws = serverSide.awaitOpen() + serverSide.awaitFirstMessage() + ws.send(dataSyncFrame("flag-keep", "alpha")) + assertTrue(readyDeferred.await()) + assertEquals("alpha", store.get("flag-keep")?.variation) + assertTrue(sync.initialized) + + sync.closeAndJoin() + + assertEquals( + "store contents survive closeAndJoin — close is for streaming lifecycle, not data", + "alpha", + store.get("flag-keep")?.variation, + ) + assertTrue("initialized flag is sticky across close", sync.initialized) + } + + /** + * Contract: `pause()` closes the live WebSocket with NORMAL_CLOSURE (1000) and reason + * `"paused"`. `resume()` opens a fresh WebSocket and re-sends `data-sync` with the + * *advanced* timestamp (set by `handleMessage` after the first snapshot was processed), + * so the server knows where to pick up from. + * + * Mutation that would fail this: + * * `pause()` not closing the WebSocket. + * * `pause()` closing with a non-1000 code or wrong reason. + * * `resume()` not calling `connect()` (no second server-side onOpen). + * * `resume()` re-using `timestamp=0` instead of the value advanced by `handleMessage`. + */ + @Test + fun `pause closes websocket with reason and resume reconnects with advanced timestamp`() = runBlocking { + val firstSide = ServerSide() + val secondSide = ServerSide() + server.enqueue(MockResponse().withWebSocketUpgrade(firstSide)) + server.enqueue(MockResponse().withWebSocketUpgrade(secondSide)) + + val store = DefaultMemoryStore() + val sync = StreamingDataSynchronizer( + options = streamingOptions(), + user = FBUser.builder("u1").build(), + store = store, + httpClient = snappyClient(), + ) + + try { + val readyDeferred = async { sync.start() } + val firstWs = firstSide.awaitOpen() + firstSide.awaitFirstMessage() + firstWs.send(dataSyncFrame("flag1", "true")) + assertTrue("first connection initialized", readyDeferred.await()) + + // Pause closes the WebSocket. Server side observes onClosing with the exact code + // + reason from `webSocket?.close(NORMAL_CLOSURE, "paused")` (StreamingDataSync:168). + sync.pause() + val close = firstSide.awaitClose() + assertEquals("pause uses NORMAL_CLOSURE", 1000, close.code) + assertEquals("pause reason pinned for observability", "paused", close.reason) + + // Resume opens the second pre-enqueued upgrade. The fresh data-sync MUST advance + // its `timestamp` — anything else would re-fetch the snapshot we just stored. + sync.resume() + val secondWs = secondSide.awaitOpen() + val resumeMessage = secondSide.awaitFirstMessage() + assertNotNull("resume opened the second WebSocket", secondWs) + + val resumeEnvelope = decodeClientEnvelope(resumeMessage) + assertEquals("data-sync", resumeEnvelope["messageType"]?.jsonPrimitive?.content) + val resumeData = resumeEnvelope["data"]?.jsonObject ?: error("missing data: $resumeMessage") + val resumeTimestamp = resumeData["timestamp"]?.jsonPrimitive?.long + ?: error("missing timestamp: $resumeMessage") + assertTrue( + "resume must send an advanced timestamp (was=$resumeTimestamp)", + resumeTimestamp > 0L, + ) + } finally { + sync.closeAndJoin() + } + } +} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/datasynchronizer/PollingDataSynchronizerTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/datasynchronizer/PollingDataSynchronizerTest.kt deleted file mode 100644 index e9e8f3e..0000000 --- a/featbit-client/src/test/kotlin/co/featbit/client/datasynchronizer/PollingDataSynchronizerTest.kt +++ /dev/null @@ -1,72 +0,0 @@ -package co.featbit.client.datasynchronizer - -import co.featbit.client.model.FBUser -import co.featbit.client.options.FBOptions -import co.featbit.client.store.DefaultMemoryStore -import kotlinx.coroutines.runBlocking -import okhttp3.mockwebserver.MockResponse -import okhttp3.mockwebserver.MockWebServer -import org.junit.After -import org.junit.Assert.assertEquals -import org.junit.Assert.assertFalse -import org.junit.Assert.assertTrue -import org.junit.Before -import org.junit.Test -import kotlin.time.Duration.Companion.minutes - -class PollingDataSynchronizerTest { - - private lateinit var server: MockWebServer - - @Before - fun setUp() { - server = MockWebServer() - server.start() - } - - @After - fun tearDown() { - server.shutdown() - } - - private fun options() = FBOptions.Builder("env-secret") - // large interval: only the first immediate poll runs during the test - .polling(server.url("/").toString(), interval = 10.minutes) - .event(server.url("/").toString()) - .build() - - @Test - fun `first successful poll initializes and populates store`() = runBlocking { - server.enqueue( - MockResponse().setResponseCode(200).setBody( - """{"data":{"featureFlags":[{"id":"flag1","variation":"true"}]}}""", - ), - ) - - val store = DefaultMemoryStore() - val user = FBUser.builder("u1").build() - val sync = PollingDataSynchronizer(options(), user, store) - - val ready = sync.start() - sync.close() - - assertTrue(ready) - assertTrue(sync.initialized) - assertEquals("true", store.get("flag1")?.variation) - } - - @Test - fun `fatal 401 stops polling and reports not ready`() = runBlocking { - server.enqueue(MockResponse().setResponseCode(401)) - - val store = DefaultMemoryStore() - val user = FBUser.builder("u1").build() - val sync = PollingDataSynchronizer(options(), user, store) - - val ready = sync.start() - sync.close() - - assertFalse(ready) - assertFalse(sync.initialized) - } -} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/domain/EvalResultTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/domain/EvalResultTest.kt new file mode 100644 index 0000000..8162514 --- /dev/null +++ b/featbit-client/src/test/kotlin/co/featbit/client/domain/EvalResultTest.kt @@ -0,0 +1,55 @@ +package co.featbit.client.domain + +import co.featbit.client.model.FeatureFlag +import co.featbit.client.app.DefaultMemoryStore +import org.junit.Assert.assertEquals +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Pins the [EvalResult] contract and the [Evaluator] mapping from store lookup to result. + * + * The sealed-interface migration replaced the previous `(reason, flag?, isValid)` triple. The + * wire-compat strings — `"flag not found"` and whatever `flag.matchReason` returns — are part + * of the public surface (they end up in `EvalDetail.reason`), so we lock them in here. + */ +class EvalResultTest { + + @Test + fun `NotFound reason is the wire-compat string`() { + assertEquals("flag not found", EvalResult.NotFound.reason) + } + + @Test + fun `Found reason derives from the flag's matchReason`() { + val flag = FeatureFlag(id = "k", variation = "true", matchReason = "fallthrough") + val found = EvalResult.Found(flag) + + assertEquals("fallthrough", found.reason) + assertEquals("true", found.value) + assertSame(flag, found.flag) + } + + @Test + fun `evaluator returns NotFound for an unknown key`() { + val evaluator = Evaluator(DefaultMemoryStore()) + assertTrue("unknown key", evaluator.evaluate("nope") is EvalResult.NotFound) + } + + @Test + fun `evaluator returns Found wrapping the stored flag`() { + val store = DefaultMemoryStore() + val flag = FeatureFlag(id = "k", variation = "v", matchReason = "rule_match") + store.upsert(flag) + + val result = evaluator(store).evaluate("k") + assertTrue("found", result is EvalResult.Found) + result as EvalResult.Found + assertSame(flag, result.flag) + assertEquals("rule_match", result.reason) + assertEquals("v", result.value) + } + + private fun evaluator(store: DefaultMemoryStore) = Evaluator(store) +} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/domain/ValueConvertersTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/domain/ValueConvertersTest.kt new file mode 100644 index 0000000..69e3921 --- /dev/null +++ b/featbit-client/src/test/kotlin/co/featbit/client/domain/ValueConvertersTest.kt @@ -0,0 +1,66 @@ +package co.featbit.client.domain + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +class ValueConvertersTest { + + @Test + fun `bool parses case-insensitively and trims`() { + assertEquals(true, ValueConverters.bool("true")) + assertEquals(true, ValueConverters.bool(" TRUE ")) + assertEquals(false, ValueConverters.bool("False")) + assertNull(ValueConverters.bool("yes")) + } + + /** + * Contract: bool conversion uses `equals(ignoreCase = true)` to avoid the + * `lowercase()`-then-compare allocation pattern. Pin every mixed-case permutation we'd + * realistically see from a flag server: legitimate cases must parse, near-matches must + * reject (we don't want "Truee" to slide). + * + * Mutation that would fail this: + * * Reverting to `value.trim().lowercase() == "true"` — passes for legit strings (no + * visible change) but masks an allocation regression. Not directly assertable here. + * * Replacing `equals("true", ignoreCase = true)` with `==`/`equals("true")` — capital + * variants would fall through to null. + * * Dropping the trim — leading/trailing whitespace strings would fail. + * * Adding new fuzzy matches like "yes"/"1" → would break the `assertNull` rejections. + */ + @Test + fun `bool conversion covers mixed case and near-match permutations`() { + // Mixed-case truthy: every variant must resolve. Note we don't cover lowercase + // "true" / "false" here — `bool parses case-insensitively and trims` already does. + listOf("TRUE", "True", "tRuE", " True ", "TRUE\t").forEach { input -> + assertEquals("'$input' must parse as true", true, ValueConverters.bool(input)) + } + listOf("FALSE", "False", "fAlSe", " false ", "false\n").forEach { input -> + assertEquals("'$input' must parse as false", false, ValueConverters.bool(input)) + } + + // Near-matches must reject — don't expand fuzzy parsing without a deliberate decision. + listOf("trues", "True ish", "yes", "1", "0", "y", "n", "", " ", "null").forEach { input -> + assertNull("'$input' must reject (not a bool)", ValueConverters.bool(input)) + } + } + + @Test + fun `int parses valid and rejects invalid`() { + assertEquals(42, ValueConverters.int(" 42 ")) + assertNull(ValueConverters.int("42.5")) + assertNull(ValueConverters.int("abc")) + } + + @Test + fun `float and double parse`() { + assertEquals(1.5f, ValueConverters.float("1.5")) + assertEquals(2.25, ValueConverters.double(" 2.25 ")) + assertNull(ValueConverters.double("not-a-number")) + } + + @Test + fun `string passes through unchanged`() { + assertEquals("anything", ValueConverters.string("anything")) + } +} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/evaluation/ValueConvertersTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/evaluation/ValueConvertersTest.kt deleted file mode 100644 index 1c4ceb5..0000000 --- a/featbit-client/src/test/kotlin/co/featbit/client/evaluation/ValueConvertersTest.kt +++ /dev/null @@ -1,35 +0,0 @@ -package co.featbit.client.evaluation - -import org.junit.Assert.assertEquals -import org.junit.Assert.assertNull -import org.junit.Test - -class ValueConvertersTest { - - @Test - fun `bool parses case-insensitively and trims`() { - assertEquals(true, ValueConverters.bool("true")) - assertEquals(true, ValueConverters.bool(" TRUE ")) - assertEquals(false, ValueConverters.bool("False")) - assertNull(ValueConverters.bool("yes")) - } - - @Test - fun `int parses valid and rejects invalid`() { - assertEquals(42, ValueConverters.int(" 42 ")) - assertNull(ValueConverters.int("42.5")) - assertNull(ValueConverters.int("abc")) - } - - @Test - fun `float and double parse`() { - assertEquals(1.5f, ValueConverters.float("1.5")) - assertEquals(2.25, ValueConverters.double(" 2.25 ")) - assertNull(ValueConverters.double("not-a-number")) - } - - @Test - fun `string passes through unchanged`() { - assertEquals("anything", ValueConverters.string("anything")) - } -} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/internal/GetUserFlagsTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/internal/GetUserFlagsTest.kt deleted file mode 100644 index f7b1d5a..0000000 --- a/featbit-client/src/test/kotlin/co/featbit/client/internal/GetUserFlagsTest.kt +++ /dev/null @@ -1,91 +0,0 @@ -package co.featbit.client.internal - -import co.featbit.client.model.FBUser -import co.featbit.client.options.FBOptions -import kotlinx.coroutines.runBlocking -import okhttp3.mockwebserver.MockResponse -import okhttp3.mockwebserver.MockWebServer -import org.junit.After -import org.junit.Assert.assertEquals -import org.junit.Assert.assertFalse -import org.junit.Assert.assertTrue -import org.junit.Before -import org.junit.Test - -class GetUserFlagsTest { - - private lateinit var server: MockWebServer - - @Before - fun setUp() { - server = MockWebServer() - server.start() - } - - @After - fun tearDown() { - server.shutdown() - } - - private fun newClient(): GetUserFlags { - val options = FBOptions.Builder("env-secret") - .polling(server.url("/").toString()) - .event(server.url("/").toString()) - .build() - val user = FBUser.builder("u1").name("bob").custom("country", "FR").build() - return GetUserFlags(options, user) - } - - @Test - fun `200 parses data featureFlags and sends auth, user-agent, timestamp`() = runBlocking { - server.enqueue( - MockResponse().setResponseCode(200).setBody( - """{"data":{"featureFlags":[ - {"id":"flag1","variation":"true","variationType":"boolean", - "variationId":"v1","sendToExperiment":false,"matchReason":"rule match"} - ]}}""".trimIndent(), - ), - ) - - val client = newClient() - val response = client.run(timestamp = 0) - client.close() - - assertFalse(response.isError) - assertEquals(1, response.flags.size) - assertEquals("flag1", response.flags[0].id) - assertEquals("true", response.flags[0].variation) - - val recorded = server.takeRequest() - assertEquals("POST", recorded.method) - assertEquals("0", recorded.requestUrl?.queryParameter("timestamp")) - assertTrue(recorded.path!!.startsWith("/api/public/sdk/client/latest-all")) - assertEquals("env-secret", recorded.getHeader("Authorization")) - assertEquals("featbit-kotlin-client-sdk", recorded.getHeader("User-Agent")) - } - - @Test - fun `empty body yields empty flags`() = runBlocking { - server.enqueue(MockResponse().setResponseCode(200).setBody("")) - val client = newClient() - val response = client.run(0) - client.close() - assertFalse(response.isError) - assertTrue(response.flags.isEmpty()) - } - - @Test - fun `401 is fatal, 500 is transient`() = runBlocking { - server.enqueue(MockResponse().setResponseCode(401)) - val client = newClient() - val fatal = client.run(0) - assertTrue(fatal.isFatal) - assertTrue(fatal.isError) - - server.enqueue(MockResponse().setResponseCode(500)) - val transient = client.run(0) - client.close() - assertFalse(transient.isFatal) - assertTrue(transient.isError) - } -} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/internal/TrackInsightTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/internal/TrackInsightTest.kt deleted file mode 100644 index d1be7ba..0000000 --- a/featbit-client/src/test/kotlin/co/featbit/client/internal/TrackInsightTest.kt +++ /dev/null @@ -1,52 +0,0 @@ -package co.featbit.client.internal - -import co.featbit.client.model.FBUser -import co.featbit.client.model.FeatureFlag -import co.featbit.client.model.Insight -import co.featbit.client.options.FBOptions -import kotlinx.coroutines.runBlocking -import okhttp3.mockwebserver.MockResponse -import okhttp3.mockwebserver.MockWebServer -import org.junit.After -import org.junit.Assert.assertTrue -import org.junit.Before -import org.junit.Test - -class TrackInsightTest { - - private lateinit var server: MockWebServer - - @Before - fun setUp() { - server = MockWebServer() - server.start() - } - - @After - fun tearDown() { - server.shutdown() - } - - @Test - fun `posts a single-element insight array to track endpoint`() = runBlocking { - server.enqueue(MockResponse().setResponseCode(200)) - - val options = FBOptions.Builder("env-secret") - .polling(server.url("/").toString()) - .event(server.url("/").toString()) - .build() - val user = FBUser.builder("u1").name("bob").build() - val flag = FeatureFlag(id = "flag1", variation = "true", variationId = "v1") - - val tracker = HttpTrackInsight(options) - tracker.run(Insight.forEvaluation(user, flag, timestamp = 123)) - tracker.close() - - val recorded = server.takeRequest() - assertTrue(recorded.path!!.startsWith("/api/public/insight/track")) - val body = recorded.body.readUtf8() - assertTrue("payload should be a JSON array: $body", body.startsWith("[")) - assertTrue(body, body.contains("\"featureFlagKey\":\"flag1\"")) - assertTrue(body, body.contains("\"keyId\":\"u1\"")) - } -} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/model/BuildersTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/model/BuildersTest.kt index a495056..73da867 100644 --- a/featbit-client/src/test/kotlin/co/featbit/client/model/BuildersTest.kt +++ b/featbit-client/src/test/kotlin/co/featbit/client/model/BuildersTest.kt @@ -1,9 +1,11 @@ package co.featbit.client.model import co.featbit.client.options.DataSyncMode +import co.featbit.client.wire.CustomizedProperty import co.featbit.client.options.FBOptions import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertSame import org.junit.Assert.assertThrows import org.junit.Assert.assertTrue import org.junit.Test @@ -36,6 +38,42 @@ class BuildersTest { assertEquals(listOf(CustomizedProperty("a", "1")), endUser.customizedProperties) } + /** + * Contract: `FBUser.toEndUser()` returns the *same* EndUser instance on every call. The + * user is immutable, so its wire form is too — building it once at construction and + * caching it eliminates a per-evaluation allocation of EndUser + the + * `CustomizedProperty` list + N entries (for an N-custom-attribute user). + * + * Mutation that would fail this: + * * Reverting `private val endUser` to compute-on-call (the prior `customizedProperties + * = custom.map { ... }` per-call mapping) — every invocation would yield a fresh + * EndUser, failing `assertSame`. + * * Swapping the cache to `lazy { ... }` would still pass (one instance, computed + * once). That's acceptable — the contract is "stable identity" not "constructor + * eager". + */ + @Test + fun `toEndUser returns same cached instance across many calls`() { + val user = FBUser.builder("k1") + .name("bob") + .custom("country", "FR") + .custom("plan", "pro") + .build() + + val first = user.toEndUser() + repeat(1_000) { + assertSame( + "toEndUser must be cached — repeated calls return the same EndUser identity", + first, + user.toEndUser(), + ) + } + // Sanity: the cached instance carries the correct data, not just a stable identity. + assertEquals("k1", first.keyId) + assertEquals("bob", first.name) + assertEquals(2, first.customizedProperties.size) + } + @Test fun `options builder defaults and overrides`() { val defaults = FBOptions.Builder("secret").build() diff --git a/featbit-client/src/test/kotlin/co/featbit/client/options/FBOptionsBuilderTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/options/FBOptionsBuilderTest.kt new file mode 100644 index 0000000..495f48e --- /dev/null +++ b/featbit-client/src/test/kotlin/co/featbit/client/options/FBOptionsBuilderTest.kt @@ -0,0 +1,86 @@ +package co.featbit.client.options + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertThrows +import org.junit.Test +import kotlin.time.Duration +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.minutes + +/** + * Pins the eager-validation contract on [FBOptions.Builder.build]. Misconfiguration must fail + * at SDK init, not on the first network call — and the offline path must not require URIs. + * + * This is a behaviour change vs. the original (which built any options object and deferred the + * failure to runtime); these tests lock in the new policy so it can't quietly regress. + */ +class FBOptionsBuilderTest { + + @Test + fun `non-offline build with blank secret throws`() { + // Builder() with no arg defaults secret to "". + assertThrows(IllegalArgumentException::class.java) { FBOptions.Builder().build() } + } + + @Test + fun `non-offline build with blank polling URI throws`() { + assertThrows(IllegalArgumentException::class.java) { + FBOptions.Builder("env-secret").polling("").build() + } + } + + @Test + fun `non-offline build with blank event URI throws`() { + assertThrows(IllegalArgumentException::class.java) { + FBOptions.Builder("env-secret").event("").build() + } + } + + @Test + fun `streaming mode with blank streaming URI throws`() { + assertThrows(IllegalArgumentException::class.java) { + FBOptions.Builder("env-secret").streaming("").build() + } + } + + @Test + fun `offline mode does NOT require secret or URIs`() { + // Bootstrap-only offline clients are a documented use case; validation must not + // reject them. + val options = FBOptions.Builder() + .offline(true) + .build() + assertEquals(true, options.offline) + assertEquals("", options.secret) + } + + @Test + fun `non-positive polling interval throws even in offline mode`() { + assertThrows(IllegalArgumentException::class.java) { + FBOptions.Builder() + .offline(true) + .polling("ignored", interval = Duration.ZERO) + .build() + } + } + + @Test + fun `negative background grace period throws`() { + assertThrows(IllegalArgumentException::class.java) { + FBOptions.Builder("env-secret") + .backgroundGracePeriod((-1).milliseconds) + .build() + } + } + + @Test + fun `happy path preserves all configured values`() { + val options = FBOptions.Builder("env-secret") + .polling("https://eval.example.com", interval = 30.minutes) + .event("https://eval.example.com") + .build() + assertEquals("env-secret", options.secret) + assertEquals("https://eval.example.com", options.pollingUri) + assertEquals(30.minutes, options.pollingInterval) + } +} diff --git a/featbit-client/src/test/kotlin/co/featbit/client/store/DefaultMemoryStoreTest.kt b/featbit-client/src/test/kotlin/co/featbit/client/store/DefaultMemoryStoreTest.kt deleted file mode 100644 index 9f6caee..0000000 --- a/featbit-client/src/test/kotlin/co/featbit/client/store/DefaultMemoryStoreTest.kt +++ /dev/null @@ -1,59 +0,0 @@ -package co.featbit.client.store - -import co.featbit.client.model.FeatureFlag -import org.junit.Assert.assertEquals -import org.junit.Assert.assertNull -import org.junit.Assert.assertTrue -import org.junit.Test -import java.util.concurrent.CopyOnWriteArrayList - -class DefaultMemoryStoreTest { - - private fun flag(id: String, variation: String) = - FeatureFlag(id = id, variation = variation) - - @Test - fun `bootstrap is loaded`() { - val store = DefaultMemoryStore(listOf(flag("a", "1"), flag("b", "2"))) - assertEquals("1", store.get("a")?.variation) - assertEquals(2, store.getAll().size) - assertNull(store.get("missing")) - } - - @Test - fun `upsert of new flag fires event with null old value`() { - val store = DefaultMemoryStore() - val events = CopyOnWriteArrayList() - store.addChangeListener { events.add(it) } - - store.upsert(flag("a", "1")) - - assertEquals(1, events.size) - assertEquals(FlagValueChangedEvent("a", null, "1"), events[0]) - } - - @Test - fun `upsert with changed value fires event, unchanged does not`() { - val store = DefaultMemoryStore(listOf(flag("a", "1"))) - val events = CopyOnWriteArrayList() - store.addChangeListener { events.add(it) } - - store.upsert(flag("a", "1")) // unchanged - assertTrue(events.isEmpty()) - - store.upsert(flag("a", "2")) // changed - assertEquals(listOf(FlagValueChangedEvent("a", "1", "2")), events.toList()) - } - - @Test - fun `removed listener stops receiving events`() { - val store = DefaultMemoryStore() - val events = CopyOnWriteArrayList() - val listener = FlagChangeListener { events.add(it) } - store.addChangeListener(listener) - store.removeChangeListener(listener) - - store.upsert(flag("a", "1")) - assertTrue(events.isEmpty()) - } -}