Skip to content

Commit d5ac724

Browse files
camilasancamilasan
committed
fix(macos): skip empty ocId rows while enumerating a folder.
Skip empty ocId rows while enumerating a folder, and clear any left from before. Also skip empty identifiers in the deletion batch. Assisted-by: Claude Code:claude-opus-4-8 Signed-off-by: Camila Ayres <hello@camilasan.com>
1 parent 9d88543 commit d5ac724

5 files changed

Lines changed: 152 additions & 5 deletions

File tree

‎shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -470,7 +470,24 @@ public final class FilesDatabaseManager: Sendable {
470470
}
471471
}
472472

473+
// This path writes directly to Realm, bypassing addItemMetadata's guard. Drop any
474+
// empty-ocId row here so it never becomes an item with an empty identifier. See #10701.
475+
let droppedCount = metadatasToCreate.filter { $0.ocId.isEmpty }.count
476+
+ metadatasToUpdate.filter { $0.ocId.isEmpty }.count
477+
if droppedCount > 0 {
478+
logger.error("Dropping \(droppedCount) metadata row(s) with empty ocId during depth-1 ingestion.", [.url: serverUrl])
479+
}
480+
metadatasToCreate = metadatasToCreate.filter { !$0.ocId.isEmpty }
481+
metadatasToUpdate = metadatasToUpdate.filter { !$0.ocId.isEmpty }
482+
473483
try database.write {
484+
// Self-heal a database poisoned by an earlier build: an empty-ocId row is keyed by ""
485+
// and is always invalid, so remove it while we are enumerating.
486+
let poisoned = database.objects(RealmItemMetadata.self).where { $0.ocId == "" }
487+
if !poisoned.isEmpty {
488+
database.delete(poisoned)
489+
}
490+
474491
// Evict any logical-address duplicates before persisting fresh
475492
// payloads, so an ocId rotation (or rename whose target collides
476493
// with a third row) does not leave two non-deleted siblings at
@@ -482,8 +499,9 @@ public final class FilesDatabaseManager: Sendable {
482499
evictLogicalDuplicates(of: metadata, in: database)
483500
}
484501

485-
// Do not delete the metadatas that have been deleted
486-
database.add(metadatasToDelete.map { RealmItemMetadata(value: $0) }, update: .modified)
502+
// Do not delete the metadatas that have been deleted. Skip empty-ocId markers so the
503+
// self-heal above is not immediately undone.
504+
database.add(metadatasToDelete.filter { !$0.ocId.isEmpty }.map { RealmItemMetadata(value: $0) }, update: .modified)
487505
database.add(metadatasToUpdate.map { RealmItemMetadata(value: $0) }, update: .modified)
488506
database.add(metadatasToCreate.map { RealmItemMetadata(value: $0) }, update: .all)
489507
}

‎shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+ObserverReporting.swift‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,11 @@ extension Enumerator {
132132
remoteInterface: RemoteInterface,
133133
dbManager: FilesDatabaseManager
134134
) {
135-
let deletedFileProviderItemIdentifiers = deleted.map { NSFileProviderItemIdentifier($0.ocId) }
135+
// Never report an empty identifier to the framework: a corrupt empty-ocId row must not reach
136+
// didDeleteItems. See #10701.
137+
let deletedFileProviderItemIdentifiers = deleted.compactMap {
138+
$0.ocId.isEmpty ? nil : NSFileProviderItemIdentifier($0.ocId)
139+
}
136140

137141
// Per-item trace so a debug archive can reconstruct exactly which items each batch carried.
138142
for metadata in updated {

‎shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item+Create.swift‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,13 @@ public extension Item {
101101
return (nil, NSFileProviderError(.cannotSynchronize))
102102
}
103103

104+
// The server response must carry an ocId. Returning an item with an empty identifier would
105+
// crash the framework. See #10701.
106+
guard !directory.ocId.isEmpty else {
107+
logger.error("Refusing to return created folder with empty ocId.", [.url: remotePath])
108+
return (nil, NSFileProviderError(.cannotSynchronize))
109+
}
110+
104111
directory.downloaded = true
105112
directory.keepDownloaded = parentKeepDownloaded
106113
dbManager.addItemMetadata(directory)
@@ -161,7 +168,8 @@ public extension Item {
161168
progressHandler: { $0.copyCurrentStateToProgress(progress) }
162169
)
163170

164-
guard error == .success, let ocId else {
171+
// Reject an empty ocId too: an item with an empty identifier crashes the framework. See #10701.
172+
guard error == .success, let ocId, !ocId.isEmpty else {
165173
logger.error(
166174
"""
167175
Could not upload item with filename: \(itemTemplate.filename),

‎shell_integration/MacOSX/NextcloudFileProviderKit/Tests/Interface/MockRemoteInterface.swift‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -606,6 +606,10 @@ public class MockRemoteInterface: RemoteInterface, @unchecked Sendable {
606606
/// Use this to simulate server-side upload rejections (e.g. 404 path gone, 507 quota).
607607
public var uploadError: NKError?
608608

609+
/// When set, the next created folder uses this as its identifier (including "") instead of a
610+
/// random one. Simulates a server response lacking an ocId. See #10701.
611+
public var createFolderIdentifierOverride: String?
612+
609613
/// Records the `If-Match` header the most recent upload call carried (nil if none).
610614
/// Lets tests assert the optimistic-concurrency precondition was sent, and with
611615
/// which etag. Captured before any injected `uploadError` short-circuit.
@@ -762,7 +766,7 @@ public class MockRemoteInterface: RemoteInterface, @unchecked Sendable {
762766
}
763767

764768
let item = MockRemoteItem(
765-
identifier: randomIdentifier(),
769+
identifier: createFolderIdentifierOverride ?? randomIdentifier(),
766770
name: itemName,
767771
remotePath: remotePath,
768772
directory: true,

‎shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/MissingIdentifierCrashGuardTests.swift‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,4 +91,117 @@ final class MissingIdentifierCrashGuardTests: NextcloudFileProviderKitTestCase {
9191
XCTAssertEqual(items.count, 1)
9292
XCTAssertEqual(items.first?.itemIdentifier.rawValue, "valid-id")
9393
}
94+
95+
// Producer: depth-1 PROPFIND ingestion writes directly to Realm, bypassing addItemMetadata. An
96+
// empty-ocId row here is the most likely source of the poison. It must be dropped on write.
97+
func testDepthOneIngestionDropsEmptyOcIdRows() {
98+
let folderPath = Self.account.davFilesUrl + "/folder"
99+
100+
var target = SendableItemMetadata(ocId: "target-dir", fileName: "folder", account: Self.account)
101+
target.directory = true
102+
target.serverUrl = Self.account.davFilesUrl
103+
var valid = SendableItemMetadata(ocId: "child-valid", fileName: "good.txt", account: Self.account)
104+
valid.serverUrl = folderPath
105+
var poison = SendableItemMetadata(ocId: "", fileName: "bad.txt", account: Self.account)
106+
poison.serverUrl = folderPath
107+
108+
let changeSet = dbManager.depth1ReadUpdateItemMetadatas(
109+
account: Self.account.ncKitAccount,
110+
serverUrl: folderPath,
111+
updatedMetadatas: [target, valid, poison],
112+
keepExistingDownloadState: false
113+
)
114+
115+
XCTAssertNil(dbManager.itemMetadata(ocId: ""))
116+
XCTAssertNotNil(dbManager.itemMetadata(ocId: "child-valid"))
117+
XCTAssertFalse((changeSet?.created ?? []).contains { $0.ocId.isEmpty })
118+
}
119+
120+
// Self-heal: a database already poisoned by a previous build must shed its empty-ocId row the
121+
// next time the containing folder is enumerated, not keep it forever.
122+
func testDepthOneIngestionPurgesExistingEmptyOcIdRow() {
123+
let folderPath = Self.account.davFilesUrl + "/folder"
124+
125+
var poison = SendableItemMetadata(ocId: "", fileName: "bad.txt", account: Self.account)
126+
poison.serverUrl = folderPath
127+
insertRaw(poison)
128+
XCTAssertNotNil(dbManager.itemMetadata(ocId: ""))
129+
130+
var target = SendableItemMetadata(ocId: "target-dir", fileName: "folder", account: Self.account)
131+
target.directory = true
132+
target.serverUrl = Self.account.davFilesUrl
133+
134+
_ = dbManager.depth1ReadUpdateItemMetadatas(
135+
account: Self.account.ncKitAccount,
136+
serverUrl: folderPath,
137+
updatedMetadatas: [target],
138+
keepExistingDownloadState: false
139+
)
140+
141+
XCTAssertNil(dbManager.itemMetadata(ocId: ""))
142+
}
143+
144+
// Deletion path: an empty-ocId row that disappears remotely must not be reported to
145+
// didDeleteItems as an empty identifier.
146+
func testChangeBatchSkipsEmptyOcIdDeletions() throws {
147+
let remoteInterface = MockRemoteInterface(account: Self.account, rootItem: rootItem)
148+
let enumerator = try Enumerator(
149+
enumeratedItemIdentifier: .workingSet,
150+
account: Self.account,
151+
remoteInterface: remoteInterface,
152+
dbManager: dbManager,
153+
log: FileProviderLogMock()
154+
)
155+
let observer = MockChangeObserver(enumerator: enumerator)
156+
157+
let valid = SendableItemMetadata(ocId: "del-valid", fileName: "a.txt", account: Self.account)
158+
let poison = SendableItemMetadata(ocId: "", fileName: "b.txt", account: Self.account)
159+
160+
enumerator.completeChangesBatch(
161+
observer,
162+
updated: [],
163+
deleted: [valid, poison],
164+
anchor: Enumerator.syncAnchor(at: Date(timeIntervalSince1970: 1)),
165+
moreComing: false,
166+
account: Self.account,
167+
remoteInterface: remoteInterface,
168+
dbManager: dbManager
169+
)
170+
171+
XCTAssertEqual(observer.deletedItemIdentifiers.map(\.rawValue), ["del-valid"])
172+
}
173+
174+
// Create callback: when the server yields an item without an identifier, create must return an
175+
// error rather than hand the framework an item with an empty identifier.
176+
func testCreateFolderReturnsErrorWhenServerOcIdEmpty() async throws {
177+
let remoteInterface = MockRemoteInterface(account: Self.account, rootItem: rootItem)
178+
remoteInterface.createFolderIdentifierOverride = ""
179+
180+
var folderMeta = SendableItemMetadata(ocId: "template-id", fileName: "folder", account: Self.account)
181+
folderMeta.directory = true
182+
folderMeta.classFile = NKTypeClassFile.directory.rawValue
183+
folderMeta.serverUrl = Self.account.davFilesUrl
184+
185+
let template = Item(
186+
metadata: folderMeta,
187+
parentItemIdentifier: .rootContainer,
188+
account: Self.account,
189+
remoteInterface: remoteInterface,
190+
dbManager: dbManager
191+
)
192+
193+
let (created, error) = await Item.create(
194+
basedOn: template,
195+
contents: nil,
196+
account: Self.account,
197+
remoteInterface: remoteInterface,
198+
progress: Progress(),
199+
dbManager: dbManager,
200+
log: FileProviderLogMock()
201+
)
202+
203+
XCTAssertNil(created)
204+
XCTAssertNotNil(error)
205+
XCTAssertNil(dbManager.itemMetadata(ocId: ""))
206+
}
94207
}

0 commit comments

Comments
 (0)