Decouple local item identity from server ocId; combine with Realm → GRDB migration
- Dominant language
- C++
- Stars
- 3.9k
- Forks
- 1k
- Avg merge
- 1d 17h
- Merged PRs (30d)
- 123
Description
The macOS file provider extension treats Nextcloud's server-side `ocId` as a stable, unique lifetime identifier and uses it as both the local database primary key and the value of `NSFileProviderItemIdentifier`. That assumption is false in two distinct ways, the most recent of which manifests as an infinite move loop when a file appears at multiple paths under a shared mount.
This issue proposes decoupling local identity from server identifiers, combined with the already-scheduled Realm → GRDB migration in #9046.
## Problem
`ocId` is not a stable lifetime identifier:
1. **`ocId` changes on trash/restore.** The server issues a different `ocId` after a restore. `Item+Trash.swift:82` and `:135` work around this today by forcibly rewriting the server-returned `ocId` back to the old value before persisting, hiding the change from the rest of the package.
2. **`ocId` is not unique across logical paths — by design.** A file shared with the user multiple times can be mounted at multiple paths. Every mount point reports the same `ocId` because it references the same file content; only the path differs. The web front-end resolves this by treating path as the primary identity key. The same is true of `fileId`. **No server-issued identifier is unique per item-in-the-local-file-provider-domain.**
Because the current schema enforces `ocId` as the primary key, the second mount overwrites the first on upsert. The framework then sees a move and reconciliation produces an apparent infinite move loop. Reproduced with `/Talk/20250825_174132.png` and `/Talk/2025/20250825_174132.png`.
The existing dedup infrastructure (`FilesDatabaseManager+Deduplication.swift`, `658dbe8325`) handles the *adjacent* case of one logical path holding two `ocId`s. It does not cover the multi-mount case — the second row never makes it into the database.
## Why localized patches don't work
Roughly 22 source files across `Database/`, `Item/`, `Extension/`, `Enumeration/`, and `Metadata/` hard-code the "ocId = local identity" assumption:
- Realm primary key is `RealmItemMetadata.ocId`.
- `Item.itemIdentifier` returns `NSFileProviderItemIdentifier(metadata.ocId)`.
- ~17 sites construct `NSFileProviderItemIdentifier` directly from `ocId`.
- 40+ lookups by `ocId`.
- Chunked uploads embed `itemIdentifier.rawValue` (i.e. `ocId`) in the server-side chunk-store folder name.
No targeted patch resolves both classes of violation without changing the foundation.
## Proposed approach
Combine the identity-model fix with the planned Realm → GRDB migration (#9046) into a single user-facing release. The GRDB cutover provides a sanctioned re-enumeration event and a clean-slate schema. The identity-model fix lands on top of the new persistence layer.
Design:
- New local identifier `localId` (UUID), assigned by us at first sight of a row. Stable for as long as the row exists. Becomes the value behind `NSFileProviderItemIdentifier`.
- Logical-path uniqueness enforced at the database level via `UNIQUE (serverUrl, fileName)`.
- `ocId` and `fileId` demoted to plain indexed reference fields. No uniqueness constraint. Allowed to change (trash/restore), allowed to be shared across rows (multi-mount).
- Reconciliation of server responses to local rows uses path as the primary match key.
### Side cleanup: drop the `account` namespace
The current schema namespaces every entity by `account` because the package historically used one shared database across multiple accounts. The database is now dedicated to a single account, so the `account` column is obsolete heritage. The new GRDB schema drops `account` entirely. Every path key in this plan is `(serverUrl, fileName)` — no account component. Same applies to lookups, indexes, and `where` clauses currently filtering by account.
## Plan
- [ ] Design GRDB schema: `localId TEXT PRIMARY KEY`, `UNIQUE (serverUrl, fileName)`, `ocId` and `fileId` as plain indexed fields. No `account` column.
- [ ] Reimplement `FilesDatabaseManager` against GRDB with one-to-one API parity. Land as a separate PR ahead of identity-model changes.
- [ ] Re-route identifier construction throughout the package: `Item.itemIdentifier`, ~17 `NSFileProviderItemIdentifier(metadata.ocId)` call sites, parent-identifier derivation.
- [ ] Implement path-based reconciliation in PROPFIND ingestion. Cover every move-vs-multi-mount case:
- Single path → single path (no-op).
- Single path → moved path (server move).
- Two paths with same `ocId`/`fileId` → two paths (multi-mount stable).
- Two paths → one path (un-share; the disappearing row is soft-deleted).
- One path → two paths (newly multi-mounted; new row added).
- [ ] Remove the forced-`ocId`-rewrite at `Item+Trash.swift:82` and `:135`.
- [ ] Retire `FilesDatabaseManager+Deduplication.swift`. The `UNIQUE (serverUrl, fileName)` constraint replaces it.
- [ ] Revisit `chunkUploadId` derivation so the new `localId` doesn't leak into server-side chunk-store folder names unnecessarily.
- [ ] Domain reset on first launch of the combined version: signal the framework, verify re-enumeration completes cleanly.
- [ ] Port persistence tests to GRDB. Add fixtures for trash/restore identifier stability, multi-mount shares, and the five move-vs-multi-mount reconciliation cases.
- [ ] Manual verification on a real Nextcloud server, including multi-mount shares and the reproduction case from the original bug report.
## Files affected
- `Metadata/RealmItemMetadata.swift` — retired; replaced by the GRDB row type.
- `Metadata/SendableItemMetadata.swift` — add `localId`, drop `account`.
- `Database/FilesDatabaseManager.swift` — reimplemented against GRDB. All `itemMetadata(...)` lookups, parent-identifier methods.
- `Database/FilesDatabaseManager+Deduplication.swift` — removed.
- `Database/FilesDatabaseManager+Directories.swift` — path lookups no longer keyed on account.
- `Item/Item.swift` — `itemIdentifier` derives from `localId`.
- `Item/Item+Trash.swift` — remove forced-ocId-rewrite.
- `Item/Item+Create.swift` — assign `localId` on creation; revisit `chunkUploadId`.
- `Item/Item+Modify.swift` — move/rename keyed by `localId`.
- All other `Item+*.swift` files — `ocId` → `localId` for local lookups; retain `ocId` only as a server reference.
- `Enumeration/Enumerator.swift` and the rest of `Enumeration/` — identifier construction from `localId`.
- `Extensions/NKFile+Extensions.swift` — reconciliation probe attaches existing `localId` if a path match exists.
- `Tests/NextcloudFileProviderKitTests/` — fixtures and persistence tests updated for GRDB; new cases added.
## Risks
| Severity | Risk | Mitigation |
|---|---|---|
| High | Path-reconciliation mis-classifies a multi-mount situation as a move, or vice versa | Explicit fixtures for every move-vs-multi-mount case; manual verification with multi-share installation. |
| High | GRDB cutover and identity-model changes interact at runtime in non-obvious ways | Stage as two reviewable diffs in one release: GRDB cutover with API parity first, identity model on top. |
| Medium | `chunkUploadId` embedding leaks `localId` server-side | Confirm acceptability or hash/prefix the value before sending. |
| Medium | Forward-only release; partial rollback not possible | Internal preview / TestFlight against a representative dataset including multi-mount installations before public release. |
| Low | In-flight uploads at the cutover surface as failures | Same precedent as the macOS sandbox reset; user retries. |
## Time estimate
**Roughly 6–8 calendar months** for one engineer on a normal European working schedule, new to GRDB, with heavy reviewer load. Central estimate: **~7 months**.
### How this was computed
The basis is ~8–10 weeks of ideal focused-engineering effort, sized by counting the work: ~22 files to touch, one persistence-engine rewrite at one-to-one API parity, one piece of non-trivial new logic (path-reconciliation with five move-vs-multi-mount cases), removal of two band-aids, plus tests.
Effort breakdown (focused-engineering weeks, before adjustments):
| Effort | Work |
|---|---|
| 3 weeks | GRDB schema and `FilesDatabaseManager` reimplementation with API parity; persistence tests. |
| 2 weeks | Re-keying ~22 files to `localId`; path-based reconciliation in PROPFIND ingestion. |
| 1 week | Removing the trash-restore hack and retiring the dedup paths. |
| 1 week | Full-domain-reset flow on first launch. |
| 1.5 weeks | End-to-end manual verification on a real Nextcloud server. |
| 0.5 week | Buffer. |
Calendar adjustments from focused-effort weeks:
| Factor | Adjustment | Why |
|---|---|---|
| Productive focus per workday | ×1.7 | Real-world focused engineering is 4–5h per workday, not 8h. |
| Vacation, holidays, sick leave | ×1.12 | ~10–12% of a quarter at typical European rates. |
| Cone-of-uncertainty (new to GRDB) | ×1.5 | Learning curve on a novel persistence engine. |
| Heavy reviewer load | +~4 weeks | Calendar slip across the planned two-PR stack waiting for reviews. |
Arithmetic: `8 × 1.7 × 1.12 × 1.5 + 4 ≈ 27 weeks` (lower) to `10 × 1.7 × 1.12 × 1.5 + 4 ≈ 33 weeks` (upper). Rounded to **6–8 calendar months**, with **~7 months** as the central estimate.
### Caveat
This is a from-the-outside estimate. Before committing to a delivery date, run a **1–2 week time-boxed scoping spike** on the riskiest piece — either the path-reconciliation logic with the move-vs-multi-mount fixtures, or a GRDB prototype of `FilesDatabaseManager` covering the most-used five methods. The spike will calibrate the rest of the estimate far better than this can.
## Why not a full rewrite
The broken part (identity) is structurally separable from the working parts (enumerator, sync engine, chunked upload, locking, trash semantics, XPC interface). A full rewrite would cost 3–6+ months and re-introduce regressions in code that currently works — notably the sync-anchor invalidation logic (`5eedd14aee`) and the locking, chunked-upload, and trash semantics. The combined refactor described here *is* the lean version of "rewrite": it replaces the persistence engine (already planned in #9046) and rebuilds the identity model on top, without restarting from zero.
## Acceptance criteria
- [ ] Original move-loop reproduction (two paths sharing the same `ocId`) resolves with both items reachable in Finder at their respective paths.
- [ ] Multi-mount-share fixture produces two distinct local rows with stable `NSFileProviderItemIdentifier` values that survive relaunches.
- [ ] Trash/restore preserves the `NSFileProviderItemIdentifier` rawValue byte-for-byte before and after.
- [ ] First launch of the combined release on an existing installation produces a clean re-enumeration: no orphan rows, no crashes, no duplicate items in Finder once the enumeration settles.
- [ ] The `/Talk/20250825_174132.png` + `/Talk/2025/20250825_174132.png` reproduction case lands as an integration test fixture and passes.
- [ ] Existing tests in `FilesDatabaseManagerTests.swift`, `ItemDeleteTests.swift`, `ItemModifyTests.swift` ported to GRDB and green, plus new tests for the move-vs-multi-mount cases.
- [ ] Manual verification on a live server covering: trash, restore, move, rename, multi-mount share, un-share one mount, chunked-upload-in-flight across the upgrade boundary.
Related: #9046
Contributor guide
Assessment
This issue has not been assessed yet.