Derive cloudPath from parentID chain instead of denormalized column - #459
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR removes Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
CryptomatorFileProvider/Middleware/OnlineItemNameCollisionHandler.swift (1)
48-68: 💤 Low valueConsider whether collision handling is applicable to all task types.
The collision handler creates updated tasks for all CloudTask types, including
DownloadTaskandItemEnumerationTask. However,CloudProviderError.itemAlreadyExiststypically occurs during write operations (upload, folder creation, reparent), not read operations. If a download or enumeration task reaches this handler, it may indicate an unexpected code path.The current implementation is defensive and won't break, but you may want to add logging or an assertion for these unexpected cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CryptomatorFileProvider/Middleware/OnlineItemNameCollisionHandler.swift` around lines 48 - 68, cloudPathCollisionUpdate currently treats all CloudTask subclasses the same, but collision errors are expected only for write operations; update cloudPathCollisionUpdate(for:) to log or assert when it receives unexpected read-only types (DownloadTask, ItemEnumerationTask) instead of silently treating them as normal: inside cloudPathCollisionUpdate(for:) after computing collisionFreeCloudPath and before returning the updated task, add a warning log via your logger or a precondition/assert for the cases matching DownloadTask and ItemEnumerationTask (referencing the DownloadTask and ItemEnumerationTask type checks in the switch) so unexpected paths are visible during debugging while still returning or throwing as appropriate.CryptomatorFileProviderTests/DB/UploadTaskManagerTests.swift (1)
23-31: ⚡ Quick winAdd an explicit unsaved-metadata guard test for
createNewTaskRecord(for:).Given the new contract, this suite should lock in that passing metadata without a persisted
idthrowsDBManagerError.nonSavedItemMetadata(instead of silently creating invalid task rows).Proposed test addition
+ func testCreateNewTaskRecordForUnsavedMetadataThrowsNonSavedItemMetadata() throws { + let unsavedMetadata = ItemMetadata( + name: "Unsaved", + type: .file, + size: nil, + parentID: NSFileProviderItemIdentifier.rootContainerDatabaseValue, + lastModifiedDate: nil, + statusCode: .isUploaded, + isPlaceholderItem: false + ) + XCTAssertThrowsError(try manager.createNewTaskRecord(for: unsavedMetadata)) { error in + guard case DBManagerError.nonSavedItemMetadata = error else { + XCTFail("Throws the wrong error: \(error)") + return + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CryptomatorFileProviderTests/DB/UploadTaskManagerTests.swift` around lines 23 - 31, Add a unit test that verifies UploadTaskDBManager.createNewTaskRecord(for:) throws DBManagerError.nonSavedItemMetadata when given ItemMetadata that has not been persisted; specifically, instantiate an ItemMetadata (do not call ItemMetadataDBManager.cacheMetadata or otherwise persist it), call manager.createNewTaskRecord(for: unsavedItem) and assert it throws DBManagerError.nonSavedItemMetadata, referencing UploadTaskDBManager.createNewTaskRecord(for:) and DBManagerError.nonSavedItemMetadata so the new contract is locked in.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CryptomatorFileProvider/DB/DatabaseHelper.swift`:
- Around line 212-237: Before rebuilding itemMetadata in
migrator.registerMigration("v5"), detect and handle case-only sibling collisions
by querying itemMetadata grouped by parentID and lower(name) (e.g., SELECT
parentID, lower(name) AS lname, GROUP_CONCAT(id) ids, COUNT(*) cnt FROM
itemMetadata GROUP BY parentID, lname HAVING cnt > 1); if any rows are returned
either abort the migration with a clear MigrationError including the offending
ids (so upgrade can be fixed manually) or deterministically resolve them (choose
one row to keep, e.g., MIN(id) or newest lastEnumeratedAt, and delete the other
ids) before proceeding to CREATE/INSERT/DROP/RENAME; implement this check/repair
inside the same migrator.registerMigration("v5") block referencing itemMetadata
and cloudPath so childOfFolder/cacheMetadata will not later pick the wrong row.
In `@CryptomatorFileProvider/DB/ItemMetadataDBManager.swift`:
- Around line 143-160: The method getAllCachedMetadata currently force-unwraps
parent.id when building the recursive SQL arguments; replace that with a guarded
check: validate parent.id with guard let id = parent.id else { throw ... } and
throw the same non-saved-item error pattern used by the task DB managers (i.e.
the existing non-saved-item error used elsewhere) instead of crashing, then use
the guarded id variable in the SQL arguments and keep the rest of the logic
unchanged.
- Around line 225-240: The ancestor CTE currently hardcodes the root row id as
`1` which breaks if the root database value differs; in
`resolveCloudPath(for:database:)` update the SQL to use the `rootID` variable
instead of `1` (e.g. replace `WHERE a.id != 1` with a parameterized check
against `rootID`) and add `rootID` to the `arguments` array passed to
`Row.fetchAll(...)` so the query uses the derived
`NSFileProviderItemIdentifier.rootContainerDatabaseValue` consistently.
In
`@CryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterTestCase.swift`:
- Around line 57-58: The test currently hides failures by defaulting to
CloudPath("/") when metadataManagerMock.getCloudPath(for: id) fails; change the
fixture to fail fast instead: make the constructor for the UploadTask test
fixture require a valid cloud path and propagate/throw the error from
metadataManagerMock.getCloudPath(for: id) (or assert/force-unwrap in the test)
rather than creating CloudPath("/"), so any broken metadata chain is surfaced;
update the call site that constructs UploadTask(taskRecord:..., itemMetadata:
metadata, cloudPath: ..., onURLSessionTaskCreation: ...) to use the real
CloudPath from metadataManagerMock.getCloudPath(for: id) and fail the test if
that lookup does not succeed.
---
Nitpick comments:
In `@CryptomatorFileProvider/Middleware/OnlineItemNameCollisionHandler.swift`:
- Around line 48-68: cloudPathCollisionUpdate currently treats all CloudTask
subclasses the same, but collision errors are expected only for write
operations; update cloudPathCollisionUpdate(for:) to log or assert when it
receives unexpected read-only types (DownloadTask, ItemEnumerationTask) instead
of silently treating them as normal: inside cloudPathCollisionUpdate(for:) after
computing collisionFreeCloudPath and before returning the updated task, add a
warning log via your logger or a precondition/assert for the cases matching
DownloadTask and ItemEnumerationTask (referencing the DownloadTask and
ItemEnumerationTask type checks in the switch) so unexpected paths are visible
during debugging while still returning or throwing as appropriate.
In `@CryptomatorFileProviderTests/DB/UploadTaskManagerTests.swift`:
- Around line 23-31: Add a unit test that verifies
UploadTaskDBManager.createNewTaskRecord(for:) throws
DBManagerError.nonSavedItemMetadata when given ItemMetadata that has not been
persisted; specifically, instantiate an ItemMetadata (do not call
ItemMetadataDBManager.cacheMetadata or otherwise persist it), call
manager.createNewTaskRecord(for: unsavedItem) and assert it throws
DBManagerError.nonSavedItemMetadata, referencing
UploadTaskDBManager.createNewTaskRecord(for:) and
DBManagerError.nonSavedItemMetadata so the new contract is locked in.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de082e5f-9fd5-48ee-8797-656631676a65
📒 Files selected for processing (61)
CryptomatorFileProvider/CloudTask/CloudTask.swiftCryptomatorFileProvider/CloudTask/DeletionTask.swiftCryptomatorFileProvider/CloudTask/DownloadTask.swiftCryptomatorFileProvider/CloudTask/FolderCreationTask.swiftCryptomatorFileProvider/CloudTask/ItemEnumerationTask.swiftCryptomatorFileProvider/CloudTask/ReparentTask.swiftCryptomatorFileProvider/CloudTask/UploadTask.swiftCryptomatorFileProvider/DB/DatabaseHelper.swiftCryptomatorFileProvider/DB/DeletionTaskDBManager.swiftCryptomatorFileProvider/DB/DownloadTaskDBManager.swiftCryptomatorFileProvider/DB/ItemEnumerationTaskDBManager.swiftCryptomatorFileProvider/DB/ItemMetadata.swiftCryptomatorFileProvider/DB/ItemMetadataDBManager.swiftCryptomatorFileProvider/DB/ReparentTaskDBManager.swiftCryptomatorFileProvider/DB/UploadTaskDBManager.swiftCryptomatorFileProvider/FileProviderAdapter.swiftCryptomatorFileProvider/FileProviderAdapterError.swiftCryptomatorFileProvider/FileProviderAdapterManager.swiftCryptomatorFileProvider/FileProviderItem.swiftCryptomatorFileProvider/Middleware/OnlineItemNameCollisionHandler.swiftCryptomatorFileProvider/Middleware/TaskExecutor/DeletionTaskExecutor.swiftCryptomatorFileProvider/Middleware/TaskExecutor/DownloadTaskExecutor.swiftCryptomatorFileProvider/Middleware/TaskExecutor/FolderCreationTaskExecutor.swiftCryptomatorFileProvider/Middleware/TaskExecutor/ItemEnumerationTaskExecutor.swiftCryptomatorFileProvider/Middleware/TaskExecutor/UploadTaskExecutor.swiftCryptomatorFileProviderTests/DB/CachedFileManagerTests.swiftCryptomatorFileProviderTests/DB/DeletionTaskManagerTests.swiftCryptomatorFileProviderTests/DB/DownloadTaskManagerTests.swiftCryptomatorFileProviderTests/DB/ItemEnumerationTaskManagerTests.swiftCryptomatorFileProviderTests/DB/MaintenanceManagerTests.swiftCryptomatorFileProviderTests/DB/MetadataManagerTests.swiftCryptomatorFileProviderTests/DB/ReparentTaskManagerTests.swiftCryptomatorFileProviderTests/DB/UploadTaskManagerTests.swiftCryptomatorFileProviderTests/FileImportingServiceSourceTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterCreateDirectoryTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterDeleteItemTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterEnumerateItemTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterGetItemIdentifierTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterGetItemTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterImportDocumentTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterMoveItemTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterRecoverUploadsTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterSetFavoriteRankTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterSetTagDataTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterStartProvidingItemTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterTestCase.swiftCryptomatorFileProviderTests/FileProviderEnumeratorTests.swiftCryptomatorFileProviderTests/FileProviderItemTests.swiftCryptomatorFileProviderTests/FileProviderNotificatorTests.swiftCryptomatorFileProviderTests/Middleware/ErrorMapperTests.swiftCryptomatorFileProviderTests/Middleware/OnlineItemNameCollisionHandlerTests.swiftCryptomatorFileProviderTests/Middleware/TaskExecutor/CloudTaskExecutorTestCase.swiftCryptomatorFileProviderTests/Middleware/TaskExecutor/DeletionTaskExecutorTests.swiftCryptomatorFileProviderTests/Middleware/TaskExecutor/DownloadTaskExecutorTests.swiftCryptomatorFileProviderTests/Middleware/TaskExecutor/FolderCreationTaskExecutorTests.swiftCryptomatorFileProviderTests/Middleware/TaskExecutor/ItemEnumerationTaskTests.swiftCryptomatorFileProviderTests/Middleware/TaskExecutor/ReparentTaskExecutorTests.swiftCryptomatorFileProviderTests/Middleware/TaskExecutor/UploadTaskExecutorTests.swiftCryptomatorFileProviderTests/PermissionProviderImplTests.swiftCryptomatorFileProviderTests/ServiceSource/CacheManagingServiceSourceTests.swiftCryptomatorFileProviderTests/WorkingSetObserverTests.swift
phil1995
left a comment
phil1995
left a comment
There was a problem hiding this comment.
Had a quick look at the code. I personally like the idea more of deriving the cloudPath from the relationship graph.
I wonder if we can still run into wrong / stale paths if we have a move task queued before another task for the same item as we store the snapshot at task construction. Need to disclaim that I have not tested this in the Files app yet if it's even possible to do this or the item is in a state where it's not possible to interact with it anyway (I guess if we rely on that assumption than it needs to be tested with every supported major iOS version).
| private let itemMetadataManager: ItemMetadataManager | ||
|
|
||
| init(database: DatabaseWriter) { | ||
| init(database: DatabaseWriter, itemMetadataManager: ItemMetadataManager) { |
There was a problem hiding this comment.
Maybe it makes sense to provide ItemMetadataManager via swift-dependencies?
There was a problem hiding this comment.
I looked into this and I'm not sure it's the right fit. Unlike our stateless services like permissionProvider, ItemMetadataManager is built per-vault from that vault's database in createAdapterCacheItem and the same instance is shared across the task managers, the adapter, and the working set observer, so there's no real global liveValue to register. Modeling that with swift-dependencies would mean an unimplemented live value plus a withDependencies scope around the whole per-vault graph construction (something we don't do anywhere yet), and switching only UploadTaskDBManager while DownloadTaskDBManager and ItemEnumerationTaskDBManager keep constructor injection would make it less consistent rather than more. Did you have a specific scoping approach in mind, or were you thinking of migrating all the DB managers?
…-task lock ordering
…tches order-independently
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CryptomatorFileProvider/DB/ItemMetadataDBManager.swift (1)
300-319: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider index maps to avoid the O(n*m) sibling scans.
Both passes scan
cachedChildrenlinearly for every reported item, andfirstClaimableByCasescans it again. For a folder with many children the cost grows as reported items times cached rows, with canonical-equivalence string comparison on each step.You can keep the exact semantics by keying dictionaries on
precomposedStringWithCanonicalMapping, because SwiftStringequality already treats canonically equivalent spellings as equal. The case-claim map can key on the lowercased canonical form and hold the lowest-id eligible row, which preserves the current "first by id" choice fromchildrenOfFolder. Both maps must be updated insideoverwriteand after each insert so the in-pass merge behavior stays intact.This is optional. It does not change behavior, so defer it if folder sizes stay small in practice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CryptomatorFileProvider/DB/ItemMetadataDBManager.swift` around lines 300 - 319, Optionally optimize the sibling matching in the metadata update flow by replacing repeated cachedChildren scans with canonical-name and lowercased-canonical-name maps. Preserve exact matching semantics and first-by-id case claiming, and update both maps whenever overwrite is called or a new metadata row is inserted so later items in the same pass still merge correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@CryptomatorFileProvider/DB/ItemMetadataDBManager.swift`:
- Around line 300-319: Optionally optimize the sibling matching in the metadata
update flow by replacing repeated cachedChildren scans with canonical-name and
lowercased-canonical-name maps. Preserve exact matching semantics and
first-by-id case claiming, and update both maps whenever overwrite is called or
a new metadata row is inserted so later items in the same pass still merge
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fff49756-2b7f-4775-bec7-14b57514c078
📒 Files selected for processing (2)
CryptomatorFileProvider/DB/ItemMetadataDBManager.swiftCryptomatorFileProviderTests/DB/MetadataManagerTests.swift
2 participants
Closes #450. Alternative to #457.
The stale-descendant bug exists because
cloudPathis denormalized per row:moveItemLocallyupdates the moved folder but leaves descendants pointing at the old path. #457 fixes this by rewriting descendants on move and adding a repair migration. This PR removes the source of the inconsistency instead: drop the column, derive on demand by walking theparentIDchain.What changed
itemMetadatawithout thecloudPathcolumn; addparentIDindex.ItemMetadataManager.getCloudPath(for id:)walks theparentIDchain via recursive CTE.getCachedMetadata(for: CloudPath)does a per-component descent, preferring an exactly matching child and falling back to case-insensitive matching (Swift.lowercased(), full Unicode rather than SQLite's ASCII-onlyLOWER()).getAllCachedMetadata(inside:)uses a recursive descendant CTE. Siblings are ordered byid, so the fallback resolves to a stable row.CloudTasknow carries acloudPathresolved when the task is created. Executors readtask.cloudPathinstead ofitemMetadata.cloudPath. Survives concurrent local renames of the same row.cacheMetadata([ItemMetadata])groups by parent and reconciles each folder in two passes against a single sibling fetch. Every exact name match claims its row first; only leftovers may claim a row case-insensitively, and only one still flaggedisMaybeOutdatedand unclaimed. Placeholders never claim by case, so a newly created item cannot adopt the identity of an existing row.ItemMetadataManagerto resolve the path at construction. Four task-DB-manager functions touched by this refactor now guarditem.idwiththrow DBManagerError.nonSavedItemMetadata(matching the existingReparentTaskDBManager.createTaskRecordprecedent). Three of those functions had pre-existingitem.id!that the guard now covers too; mild scope creep in the name of consistency within the changed surface.UploadTaskDBManager.createNewTaskRecordandItemMetadataDBManager.getAllCachedMetadata(inside:)drop theirid!force-unwraps for the same guard.getCloudPath(for:)/getCachedMetadata(for:)since the derived path lives off the row now. Direct DB-layer tests covergetCloudPathresolution plus itsitemNotFoundandunresolvableParentChainpaths, the unsaved-folder guard, deterministic case-only-sibling resolution (forced viaPRAGMA reverse_unordered_selects), and the reconciliation rules above.Name matching
Sibling identity is case-sensitive, because
Cryptor.encryptFileName(_:dirId:encoding:)derives a different ciphertext name per case:Foo.txtandfoo.txtare two distinct cloud items and get two rows. It precomposes to NFC before encrypting, so canonically equivalent NFC/NFD spellings share one ciphertext name and resolve to a single row. Path lookup stays case-insensitive, since Shortcuts and the local-collision pre-flight both pass user-supplied casing, but an exact match wins over a case-folded one.One known limit: reconciliation is per batch, and the File Provider extension pages every listing at 500 items, so any folder above that is reconciled in several batches. If a case-variant pair straddles a page boundary, the two rows can swap identities; both files stay visible either way. Which page an entry lands on is effectively unpredictable from its cleartext name, since paging and ordering happen on the ciphertext name.
Trade-offs vs #457
cloudPath:fromItemMetadata(...)calls.getItemIdentifierenumeration fallback structure stays the same, just changes the final comparison fromcloudPath ==to a re-lookup against the populated cache.Out of scope (worth a separate look)
DownloadTaskDBManager.init(deleted fromItemEnumerationTaskRecordinstead ofDownloadTaskRecord) is fixed inline, with a regression test. Wasn't related to Investigation: Hardcoded CloudPath #450 but the one-line fix was hard to leave in place once seen.ItemMetadata.idis still optional; only the four new DB-manager sites adopt theguard letpattern. Project-wide cleanup is a separate refactor.childrenOfFoldermaterializes a folder's rows to match names in Swift, which stock SQLite can't do for canonical equivalence. Fine at realistic folder sizes; persisted NFC name keys plus a(parentID, name)index would restore indexed lookups.