Fix last read date deletion crash

Zotero/Assets/en.lproj/Localizable.strings

@@ -418,6 +418,7 @@
 "errors.sync_toolbar.conflict_retry_limit" = "Remote sync in progress. Please try again in a few minutes.";
 "errors.sync_toolbar.group_permissions" = "You don’t have permission to edit groups.";
 "errors.sync_toolbar.internet_connection" = "Unable to connect to the network. Please try again.";
+"errors.sync_toolbar.unexpected_my_library_last_read_deletions" = "Sync received unexpected last read settings deletions for My Library.";
 "errors.sync_toolbar.personal_quota_reached" = "You have reached your Zotero Storage quota. Some files were not uploaded. Other Zotero data will continue to sync.\n\nSee your zotero.org account settings for additional storage options.";
 "errors.sync_toolbar.group_quota_reached" = "The group “%@” has reached its Zotero Storage quota. Some files were not uploaded. Other Zotero data will continue to sync.\n\nThe group owner can increase the group’s storage capacity from their storage settings on zotero.org.";
 "errors.sync_toolbar.insufficient_space" = "You have insufficient space on your WebDAV server. Some files were not uploaded. Other Zotero data will continue to sync.";

Zotero/Controllers/Database/Requests/MarkObjectsAsSyncedDbRequest.swift

@@ -51,9 +51,9 @@ struct MarkSettingsAsSyncedDbRequest: DbRequest {
     func process(in database: Realm) throws {
         for setting in settings {
             let object: UpdatableObject&Syncable
-            if setting.uid.starts(with: "lastRead"), let lastRead = database.objects(RLastReadDate.self).uniqueObject(key: setting.key, libraryId: setting.libraryId) {
+            if setting.uid.starts(with: "lastRead_"), let lastRead = database.objects(RLastReadDate.self).uniqueObject(key: setting.key, libraryId: setting.libraryId) {
                 object = lastRead
-            } else if setting.uid.starts(with: "lastPageIndex"), let pageIndex = database.objects(RPageIndex.self).uniqueObject(key: setting.key, libraryId: setting.libraryId) {
+            } else if setting.uid.starts(with: "lastPageIndex_"), let pageIndex = database.objects(RPageIndex.self).uniqueObject(key: setting.key, libraryId: setting.libraryId) {
                 object = pageIndex
             } else {
                 DDLogError("MarkSettingsAsSyncedDbRequest: could not find setting for \(setting.uid)")

Zotero/Controllers/Database/Requests/PerformDeletionsDbRequest.swift

@@ -143,22 +143,32 @@ struct PerformPageIndexDeletionsDbRequest: DbRequest {
 }
 
 struct PerformLastReadDeletionsDbRequest: DbRequest {
+    enum Error: Swift.Error {
+        case myLibraryNotSupported
+    }
+
     let libraryId: LibraryIdentifier
     let keys: [String]
 
     var needsWrite: Bool { return true }
 
     func process(in database: Realm) throws {
-        let objects = database.objects(RLastReadDate.self).filter(.keys(keys, in: libraryId))
-        for object in objects {
-            guard !object.isInvalidated else { continue }
-            if object.isChanged {
-                // If remotely deleted lastRead is changed locally, we want to keep the lastRead, so we mark that
-                // this lastRead is new and it will be reinserted by sync
-                object.markAsChanged(in: database)
-            } else {
-                object.willRemove(in: database)
-                database.delete(object)
+        switch libraryId {
+        case .custom(.myLibrary):
+            throw Error.myLibraryNotSupported
+
+        case .group:
+            let objects = database.objects(RLastReadDate.self).filter(.keys(keys, in: libraryId))
+            for object in objects {
+                guard !object.isInvalidated else { continue }
+                if object.isChanged {
+                    // If remotely deleted lastRead is changed locally, we want to keep the lastRead, so we mark that
+                    // this lastRead is new and it will be reinserted by sync
+                    object.markAsChanged(in: database)
+                } else {
+                    object.willRemove(in: database)
+                    database.delete(object)
+                }
             }
         }
     }

Zotero/Controllers/Sync/SyncActions/PerformDeletionsSyncAction.swift

@@ -8,10 +8,11 @@
 
 import Foundation
 
+import CocoaLumberjackSwift
 import RxSwift
 
 struct PerformDeletionsSyncAction: SyncAction {
-    typealias Result = [(String, String)]
+    typealias Result = (conflicts: [(String, String)], unexpectedMyLibraryLastReadDeletions: [String])
 
     private static let batchSize = 500
     let libraryId: LibraryIdentifier
@@ -24,7 +25,7 @@ struct PerformDeletionsSyncAction: SyncAction {
     unowned let dbStorage: DbStorage
     let queue: DispatchQueue
 
-    var result: Single<[(String, String)]> {
+    var result: Single<Result> {
         return Single.create { subscriber -> Disposable in
             do {
                 let hasCollections = try dbStorage.perform(request: CountObjectsDbRequest<RCollection>(), on: queue) > 0
@@ -57,7 +58,7 @@ struct PerformDeletionsSyncAction: SyncAction {
                     }
                 }
 
-                let pageIndices = settings.filter({ $0.hasPrefix("lastPageIndex") })
+                let pageIndices = settings.filter({ $0.hasPrefix("lastPageIndex_") })
                 let hasPageIndices = try dbStorage.perform(request: CountObjectsDbRequest<RPageIndex>(), on: queue) > 0
                 if hasPageIndices {
                     try batch(values: pageIndices, batchSize: Self.batchSize) { uids in
@@ -72,7 +73,8 @@ struct PerformDeletionsSyncAction: SyncAction {
                     }
                 }
 
-                let lastRead = settings.filter({ $0.hasPrefix("lastRead") })
+                var unexpectedMyLibraryLastReadDeletions: [String] = []
+                let lastRead = settings.filter({ $0.hasPrefix("lastRead_") })
                 let hasLastRead = try dbStorage.perform(request: CountObjectsDbRequest<RLastReadDate>(), on: queue) > 0
                 if hasLastRead {
                     try batch(values: lastRead, batchSize: Self.batchSize) { uids in
@@ -82,12 +84,25 @@ struct PerformDeletionsSyncAction: SyncAction {
                             groupedIndices[libraryId, default: []].append(key)
                         }
                         for (libraryId, keys) in groupedIndices {
-                            try dbStorage.perform(request: PerformLastReadDeletionsDbRequest(libraryId: libraryId, keys: keys), on: queue)
+                            do {
+                                try dbStorage.perform(request: PerformLastReadDeletionsDbRequest(libraryId: libraryId, keys: keys), on: queue)
+                            } catch let error {
+                                switch error {
+                                case PerformLastReadDeletionsDbRequest.Error.myLibraryNotSupported:
+                                    unexpectedMyLibraryLastReadDeletions.append(contentsOf: keys)
+
+                                default:
+                                    throw error
+                                }
+                            }
                         }
                     }
                 }
+                if !unexpectedMyLibraryLastReadDeletions.isEmpty {
+                    DDLogWarn("PerformDeletionsSyncAction: Received unexpected My Library lastRead deletions - \(unexpectedMyLibraryLastReadDeletions)")
+                }
 
-                subscriber(.success(conflicts))
+                subscriber(.success((conflicts: conflicts, unexpectedMyLibraryLastReadDeletions: unexpectedMyLibraryLastReadDeletions)))
             } catch let error {
                 subscriber(.failure(error))
             }

Zotero/Controllers/Sync/SyncActions/SubmitDeletionSyncAction.swift

@@ -66,7 +66,7 @@ struct SubmitDeletionSyncAction: SyncAction {
                         var groupedSettings: [LibraryIdentifier: [String]] = [:]
                         for uid in keys {
                             // only lastRead is deletable
-                            guard uid.starts(with: "lastRead"), let (key, libraryId) = try? SettingKeyParser.parse(key: uid) else { continue }
+                            guard uid.starts(with: "lastRead_"), let (key, libraryId) = try? SettingKeyParser.parse(key: uid) else { continue }
                             groupedSettings[libraryId, default: []].append(key)
                         }
                         for (libraryId, keys) in groupedSettings {

Zotero/Controllers/Sync/SyncController.swift

@@ -470,7 +470,8 @@ final class SyncController: SynchronizationController {
                  .webDavDeletionFailed,
                  .webDavVerification,
                  .webDavDownload,
-                 .webDavUpload:
+                 .webDavUpload,
+                 .unexpectedMyLibraryLastReadDeletions:
                 reportErrors.append(error)
             }
         }
@@ -1394,9 +1395,9 @@ final class SyncController: SynchronizationController {
             queue: self.workQueue
         )
         action.result.subscribe(on: self.workScheduler)
-                     .subscribe(onSuccess: { [weak self] conflicts in
+                     .subscribe(onSuccess: { [weak self] result in
                          self?.accessQueue.async(flags: .barrier) { [weak self] in
-                             self?.finishDeletionsSync(result: .success(conflicts), items: items, libraryId: libraryId)
+                             self?.finishDeletionsSync(result: .success(result), items: items, libraryId: libraryId)
                          }
                      }, onFailure: { [weak self] error in
                          self?.accessQueue.async(flags: .barrier) { [weak self] in
@@ -1406,9 +1407,14 @@ final class SyncController: SynchronizationController {
                      .disposed(by: self.disposeBag)
     }
 
-    private func finishDeletionsSync(result: Result<[(String, String)], Error>, items: [String]?, libraryId: LibraryIdentifier, version: Int? = nil) {
+    private func finishDeletionsSync(result: Result<PerformDeletionsSyncAction.Result, Error>, items: [String]?, libraryId: LibraryIdentifier, version: Int? = nil) {
         switch result {
-        case .success(let conflicts):
+        case .success(let result):
+            if !result.unexpectedMyLibraryLastReadDeletions.isEmpty {
+                nonFatalErrors.append(.unexpectedMyLibraryLastReadDeletions(keys: result.unexpectedMyLibraryLastReadDeletions))
+            }
+
+            let conflicts = result.conflicts
             if !conflicts.isEmpty {
                 self.resolve(conflict: .removedItemsHaveLocalChanges(keys: conflicts, libraryId: libraryId))
             } else {

Zotero/Models/Sync/SyncErrors.swift

@@ -79,6 +79,7 @@ enum SyncError {
         case webDavDownload(WebDavError.Download)
         case webDavUpload(WebDavError.Upload)
         case preconditionFailed(LibraryIdentifier)
+        case unexpectedMyLibraryLastReadDeletions(keys: [String])
 
         var isVersionMismatch: Bool {
             switch self {

Zotero/Scenes/General/Views/SyncToolbarController.swift

@@ -266,6 +266,9 @@ final class SyncToolbarController {
 
             case .preconditionFailed(let libraryId):
                 return (L10n.Errors.SyncToolbar.conflictRetryLimit, SyncError.ErrorData(itemKeys: nil, libraryId: libraryId))
+
+            case .unexpectedMyLibraryLastReadDeletions(let keys):
+                return (L10n.Errors.SyncToolbar.unexpectedMyLibraryLastReadDeletions, SyncError.ErrorData(itemKeys: keys, libraryId: .custom(.myLibrary)))
             }
         }
 

ZoteroTests/SyncActionsSpec.swift

@@ -740,6 +740,48 @@ final class SyncActionsSpec: QuickSpec {
                     })
                 }
             }
+
+            context("remote settings deletions") {
+                it("doesn't treat lastReadAloudPosition deletions as lastRead deletions") {
+                    try! realm.write {
+                        let lastRead = RLastReadDate()
+                        lastRead.key = "EXISTINGKEY"
+                        lastRead.date = Date(timeIntervalSince1970: 1234)
+                        lastRead.groupKey = 1
+                        realm.add(lastRead)
+                    }
+
+                    waitUntil(timeout: .seconds(60), action: { doneAction in
+                        PerformDeletionsSyncAction(
+                            libraryId: .custom(.myLibrary),
+                            collections: [],
+                            items: [],
+                            searches: [],
+                            tags: [],
+                            settings: ["lastReadAloudPosition_u_R2NCC4YU"],
+                            conflictMode: .resolveConflicts,
+                            dbStorage: dbStorage,
+                            queue: .main
+                        )
+                        .result
+                        .subscribe(onSuccess: { result in
+                            expect(result.conflicts).to(beEmpty())
+                            expect(result.unexpectedMyLibraryLastReadDeletions).to(beEmpty())
+
+                            realm.refresh()
+                            let lastReadDates = realm.objects(RLastReadDate.self)
+                            expect(lastReadDates.count).to(equal(1))
+                            expect(lastReadDates.first?.key).to(equal("EXISTINGKEY"))
+
+                            doneAction()
+                        }, onFailure: { error in
+                            fail("PerformDeletionsSyncAction failed with \(error)")
+                            doneAction()
+                        })
+                        .disposed(by: disposeBag)
+                    })
+                }
+            }
         }
     }
 }

ZoteroTests/UpdatableObjectSpec.swift

@@ -114,6 +114,35 @@ final class UpdatableObjectSpec: QuickSpec {
                     expect(refinedBy["end"] as? Int).to(equal(1705))
                 }
             }
+
+            context("when applying My Library last-read deletions") {
+                it("throws and leaves the item's last-read value unchanged") {
+                    let key = "AAAAAAAA"
+                    let date = Date(timeIntervalSince1970: 1234)
+
+                    try! realm.write {
+                        let item = RItem()
+                        item.key = key
+                        item.rawType = ItemTypes.attachment
+                        item.customLibraryKey = .myLibrary
+                        item.dateAdded = Date()
+                        item.dateModified = item.dateAdded
+                        item.lastRead = date
+                        item.updateEffectiveLastRead()
+                        realm.add(item)
+                    }
+
+                    expect {
+                        try realm.write {
+                            try PerformLastReadDeletionsDbRequest(libraryId: .custom(.myLibrary), keys: [key]).process(in: realm)
+                        }
+                    }.to(throwError())
+
+                    let item = realm.objects(RItem.self).first!
+                    expect(item.lastRead).to(equal(date))
+                    expect(item.effectiveLastRead).to(equal(date))
+                }
+            }
         }
     }
 }