docs: decide to build our own store and delete the vendored provider
The vendored dmfs provider was kept on the grounds that it hands us the sync bookkeeping for free. The phase-1 sync audit measured that bookkeeping and found most of it broken, absent, or unusable: _DIRTY not set on delete, no home for a per-collection sync token, read-only collections inexpressible, ACCOUNT_TYPE write-once so enabling sync is a full migration, and cleanUpLists able to delete a user's lists after a backup restore. Sixteen findings are provider-imposed rather than platform- or protocol-imposed. Costing the alternative showed the swap is far smaller than assumed. TasksDataSource is already a 14-method, domain-shaped interface; exactly one file above the data layer references TasksContract. The work is a second implementation behind an interface built for it, not a rewrite. Against ~5 weeks to build, owning the store removes 2.5-4 weeks from the sync plan, and 8,200 of the vendored 14,555 lines are things we would never write - 23 migrations from a 2013 schema, 798 lines of full-text search the app has zero call sites for, and 1,581 lines of a type-safe layer over ContentValues that Room deletes. External mode (OpenTasks, tasks.org) is unaffected and keeps every file that describes somebody else's schema. STORAGE-DECISION.md is the reasoning; OWN-STORE.md is the architecture and the six-phase plan. :provider stays in-tree until phase 5 so recurrence parity can be tested against it before it goes. Also corrected here: the provider's JVM test count (51 -> 56, measured from the test-results XML) and a fourth site of the debunked "switching sync on is never a migration" claim, in StorageMode.kt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
246
docs/STORAGE-DECISION.md
Normal file
246
docs/STORAGE-DECISION.md
Normal file
@@ -0,0 +1,246 @@
|
||||
# Storage: keep the vendored provider, or build our own?
|
||||
|
||||
**Status:** **decided — build our own.** See [`OWN-STORE.md`](OWN-STORE.md) for
|
||||
the architecture and plan; this document is the reasoning that got there.
|
||||
|
||||
The decision went further than the recommendation below: `:provider` is not kept
|
||||
alongside a Room store, it is **deleted**. External mode (OpenTasks, tasks.org)
|
||||
stays. The staged sequencing survives in a different form — the provider remains
|
||||
in-tree until `OWN-STORE.md` phase 5 so recurrence parity can be tested against
|
||||
it, then goes.
|
||||
|
||||
This reopens a question `SYNC.md` marked settled. It is reopened on purpose: the
|
||||
argument that settled it was *"the provider hands us the sync bookkeeping for
|
||||
free"*, and the phase-1 audit found most of that bookkeeping broken, absent, or
|
||||
unusable for our purposes. A conclusion is only as good as its premise.
|
||||
|
||||
---
|
||||
|
||||
## The three options
|
||||
|
||||
| | What it means | Store | Exported provider |
|
||||
|---|---|---|---|
|
||||
| **A** | Keep `:provider` as-is | dmfs `TaskProvider` | yes, ours today |
|
||||
| **B** | Room, same `TaskContract` shape | our Room DB | dropped, or a later facade |
|
||||
| **C** | Room, clean domain schema, `TaskContract` only as an export format | our Room DB | no |
|
||||
|
||||
External mode (talking to OpenTasks / tasks.org) is orthogonal and survives all
|
||||
three. It is the reason nothing below gets deleted.
|
||||
|
||||
---
|
||||
|
||||
## What the swap actually touches — measured, not estimated
|
||||
|
||||
### The seam is already there, and it is clean
|
||||
|
||||
`TasksDataSource` (`data/tasks/TasksDataSource.kt`, 58 lines) is a **14-method,
|
||||
domain-shaped interface**. It takes and returns `Task`, `TaskList`, `TaskForm` —
|
||||
no `Cursor`, no `Uri`, no `ContentValues`.
|
||||
|
||||
Above it, **17 files** import from `data.tasks`. What they import:
|
||||
|
||||
```
|
||||
8 × TasksRepository 4 × TasksDataSource 3 × ProviderResolver
|
||||
4 × recoveringFromProviderFailure 2 × ProviderStatus
|
||||
1 each: StorageMode, StorageModeHolder, ProviderEnvironment, TaskQuery, …
|
||||
```
|
||||
|
||||
**Exactly one file outside the data package touches `TasksContract` at all** —
|
||||
`domain/Models.kt`, and only for four status integers, one priority constant and
|
||||
one account-type string. Ten lines. Nothing else above the data layer knows a
|
||||
ContentProvider exists.
|
||||
|
||||
> The whole UI, all five milestones of it, is untouched by a storage swap.
|
||||
> That is not luck — `AndroidTasksDataSource`'s own KDoc says the seam exists so
|
||||
> that *"swapping the provider never reaches above this file."* It holds.
|
||||
|
||||
### Nothing gets deleted
|
||||
|
||||
| File | Lines | Under a Room store |
|
||||
|---|---|---|
|
||||
| `AndroidTasksDataSource.kt` | 220 | **kept** — External mode still needs it |
|
||||
| `TasksContract.kt` | 178 | **kept** — External mode speaks it |
|
||||
| `TasksRepositoryImpl.kt` | 151 | unchanged |
|
||||
| `ProviderResolver.kt` | 143 | unchanged |
|
||||
| `TaskWriteMapper.kt` | 118 | **kept** for External |
|
||||
| `TaskMapper.kt` | 102 | **kept** for External |
|
||||
| `TasksDataSource.kt` | 58 | unchanged — it is the interface |
|
||||
| `TasksRepository.kt` | 53 | unchanged |
|
||||
| `ProviderEnvironment.kt` | 51 | unchanged |
|
||||
| `StorageModeHolder.kt` | 47 | unchanged |
|
||||
| `ColumnReader.kt` | 40 | **kept** for External |
|
||||
| `ProviderFlow.kt` | 31 | unchanged |
|
||||
| `StorageMode.kt` | 30 | one new constant |
|
||||
| `TaskProjections.kt` | 24 | **kept** for External |
|
||||
| `Failures.kt` | 16 | unchanged |
|
||||
| | **1,262** | **0 removed** |
|
||||
|
||||
The work is **additive**: a second `TasksDataSource` implementation, a third
|
||||
`StorageMode`, and one `@Binds` becoming a dispatcher. `DataModule.kt` has a
|
||||
single binding to change.
|
||||
|
||||
This reframes the question. It is not *rewrite vs. keep*. It is **write a second
|
||||
backend behind an interface that exists for exactly this purpose, and run both
|
||||
until one wins.**
|
||||
|
||||
---
|
||||
|
||||
## What the new backend has to do
|
||||
|
||||
Room entities and DAOs for the ~50 columns the app actually uses across four
|
||||
tables are mechanical. The real work is the behaviour the provider's processors
|
||||
perform. Measured against `:provider`'s Java:
|
||||
|
||||
| Behaviour | Provider | Notes |
|
||||
|---|---:|---|
|
||||
| Instance expansion | ~1,070 | `Instantiating` + `instancedata` + iterables. **The hard one.** |
|
||||
| Recurring-instance edit | 337 | `Detaching` — this *is* the recurrence-model decision |
|
||||
| Completion coherence | 210 | `AutoCompleting`: status ↔ percent ↔ completed ↔ is_closed |
|
||||
| Validation | 601 | three processors, mostly defending a *public* API |
|
||||
| Parent / child | 269 | we use `parent_id` only |
|
||||
| Alarm property rows | 133 | one Room entity |
|
||||
| | **~2,620** | |
|
||||
|
||||
And what we would **not** write, of the 14,555 vendored lines:
|
||||
|
||||
| Not needed | Lines | Why |
|
||||
|---|---:|---|
|
||||
| `TaskDatabaseHelper` | 895 | 23 migrations from a 2013 schema. We start at v1. |
|
||||
| `FTSDatabaseHelper` + ngrams | 798 | **the app never searches the provider** — verified, zero call sites |
|
||||
| `model/adapters` | 1,581 | a type-safe layer over `ContentValues`. Room entities delete the problem. |
|
||||
| `model` | 1,811 | cursor ↔ entity adaptation. Room's job. |
|
||||
| `TaskProvider` + `SQLiteContentProvider` | 1,772 | URI matching, permissions, batch ops — for a public API |
|
||||
| `CategoryHandler` + `RelationHandler` | 553 | unused |
|
||||
| `utils` (most) | ~800 | dmfs jems idiom → Kotlin stdlib |
|
||||
| **≈ 8,200 lines we would simply not have** | | |
|
||||
|
||||
Two things make instance expansion less frightening than its line count:
|
||||
|
||||
1. **We need client-side recurrence expansion regardless.** Server-side
|
||||
`CALDAV:expand` on `VTODO` is broken on every server we target (`SYNC.md`),
|
||||
so `lib-recur` is in the build either way.
|
||||
2. **We would use the same eight `lib-recur` classes the provider does** —
|
||||
`RecurrenceRule`, `RecurrenceSet`, `RecurrenceSetIterator`, `RecurrenceList`,
|
||||
`RecurrenceRuleAdapter`, `DateTime`, `Duration`,
|
||||
`InvalidRecurrenceRuleException`. The algorithm is in the library, not in the
|
||||
provider.
|
||||
3. And the provider's expansion **materialises only one upcoming occurrence
|
||||
anyway** — it is not the complete implementation its size suggests.
|
||||
|
||||
---
|
||||
|
||||
## The cost, both directions
|
||||
|
||||
### Building it
|
||||
|
||||
| | |
|
||||
|---|---:|
|
||||
| Schema, entities, DAOs | 1 wk |
|
||||
| Instance expansion on `lib-recur`, with a real test suite | 1.5–2 wk |
|
||||
| Completion / parent / validation semantics | 1 wk |
|
||||
| Recurring-edit model — *shared cost, phase 1 either way* | (0.5–1 wk) |
|
||||
| Migrating existing users' local data out of the provider | 0.5 wk |
|
||||
| Tests to parity with the current 93 + 56 | 1 wk |
|
||||
| **Net additional** | **4.5–6 wk** |
|
||||
|
||||
### What it removes from the sync plan
|
||||
|
||||
Roughly sixteen of the phase-1 audit's storage findings are **provider-imposed**
|
||||
— they exist only because we run dmfs's implementation, and vanish when we own
|
||||
the store:
|
||||
|
||||
- `_DIRTY` not set on delete, and defaulting to `1`
|
||||
- `TaskLists._DIRTY` as a monotonic counter, not a flag
|
||||
- the instances URI ignoring `CALLER_IS_SYNCADAPTER`
|
||||
- no home for a per-collection sync token, href, ETag or CTag — all four squat
|
||||
into generic `SYNC1`–`SYNC8` slots
|
||||
- **read-only collections cannot be represented at all** (`ACCESS_LEVEL` inert)
|
||||
- sync-adapter delete ignoring the account parameters it forces you to supply
|
||||
- `Moving` leaving a dual-UID collision
|
||||
- `ACCOUNT_TYPE` write-once → enabling sync is a full data migration
|
||||
- Auto Backup restore arming `cleanUpLists` → silent task loss
|
||||
- `Detaching` deciding the recurring-completion model for us
|
||||
- the `lib-recur` version trap (0.16.0 removed `RecurrenceSet`) — we pin because
|
||||
the provider does, not because we want to
|
||||
|
||||
Conservatively that is **2.5–4 weeks** off phases 0, 3 and 4 of the 11.5–15 week
|
||||
sync plan, plus a class of bug that is currently *unfixable without patching
|
||||
vendored Java*.
|
||||
|
||||
### Net
|
||||
|
||||
**≈ +1 to +3.5 weeks**, for a store we control, in exchange for two real losses.
|
||||
|
||||
---
|
||||
|
||||
## The honest case for keeping it (Option A)
|
||||
|
||||
Not nothing, and it should not be waved away:
|
||||
|
||||
- **It works, and it has 56 passing JVM tests** over recurrence, reparenting,
|
||||
instances and observers. A Room reimplementation is *new code with new bugs*,
|
||||
in the layer that holds the user's only copy of their data. That risk is real
|
||||
and it points at A.
|
||||
- **Tombstones actually work.** Soft delete for account rows, hard delete for
|
||||
sync adapters, hidden from normal queries, undelete refused. Of all the sync
|
||||
bookkeeping, this is the piece that held up under audit.
|
||||
- **The exported provider under our own authority** — third-party apps can read
|
||||
Agendula's tasks, and asking DAVx5 to sync us stays possible.
|
||||
- Eleven local modification sites, all marked `AGENDULA CHANGE`, all documented
|
||||
in `provider/PROVENANCE.md`. The fork is under control today.
|
||||
|
||||
And the case against keeping it:
|
||||
|
||||
- **14,555 lines of Java — 1.66× the entire app** (8,783 lines of Kotlin). We
|
||||
carry, build, lint, translate and ship all of it to use maybe a third.
|
||||
- Upstream is effectively dormant; every future `targetSdk` bump and every
|
||||
Android SQLite behaviour change lands on us, in someone else's code, in a
|
||||
language the rest of the app does not use.
|
||||
- It makes behavioural decisions on our behalf (`Detaching`, `AutoCompleting`)
|
||||
that we then have to reverse-engineer before we can honour them over CalDAV.
|
||||
|
||||
---
|
||||
|
||||
## Recommendation
|
||||
|
||||
**Option B — build our own store on Room, keeping the `TaskContract` *shape* as
|
||||
the internal model — and keep `:provider` in-tree while we do.**
|
||||
|
||||
Three reasons, in order of weight:
|
||||
|
||||
1. **The seam already exists and the work is additive.** Nothing is deleted,
|
||||
nothing above `data/tasks` changes, and both backends can ship side by side
|
||||
behind `StorageMode`. The "big rewrite" this decision was originally weighed
|
||||
against does not exist.
|
||||
2. **The premise that settled it is gone.** The provider was kept for sync
|
||||
bookkeeping we have since measured as broken. Sixteen findings deep, keeping
|
||||
it is now a *cost* to the sync plan, not a saving.
|
||||
3. **The window is now.** After phase 1 the mapper and engine are written against
|
||||
whichever store won, and this stops being a two-file change.
|
||||
|
||||
Keeping the `TaskContract` *shape* rather than going domain-native (Option C) is
|
||||
deliberate: it is a proven schema for exactly this problem, other engines
|
||||
understand it, and it keeps a future exported facade cheap — without obliging us
|
||||
to run a 2015 Java implementation of it.
|
||||
|
||||
### Sequencing that keeps the risk low
|
||||
|
||||
1. Add `StorageMode.OWN` and a Room `TasksDataSource`. Both backends live.
|
||||
2. Ship it behind a setting; the vendored provider stays the default.
|
||||
3. Run the sync engine against Room only.
|
||||
4. Once Room has real production mileage, decide whether the exported
|
||||
ContentProvider is worth re-implementing as a thin facade (~1–1.5 wk) or
|
||||
whether External mode already covers everyone who wanted it.
|
||||
|
||||
Step 4 is a genuinely open question and does not need answering now. That is the
|
||||
point of sequencing it last.
|
||||
|
||||
---
|
||||
|
||||
## Open
|
||||
|
||||
- **Is an exported provider worth keeping at all?** It matters only if third
|
||||
parties should read our tasks, or if we want DAVx5 to sync our store. External
|
||||
mode arguably already serves the second. Undecided.
|
||||
- **The Room estimate is mine, not measured.** Instance expansion is the item
|
||||
that could overrun; everything else is well-bounded.
|
||||
Reference in New Issue
Block a user