From d6f8c37914be795bc2c24ade8c211746baa6aa54 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 9 May 2026 22:33:48 +0700 Subject: [PATCH] fix(sync): propagate decode errors instead of silently reverting to defaults --- CHANGELOG.md | 1 + TablePro/Core/Sync/SyncCoordinator.swift | 16 +++++- TablePro/Core/Sync/SyncRecordMapper.swift | 65 +++++++++++++++++------ 3 files changed, 64 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c610ec9c..d7f36a069 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - When saving the connections file fails (disk full, sandbox denied, encoding error), TablePro now aborts the dependent steps instead of continuing as if the save had succeeded. Previously a failed delete could remove the keychain password and queue a CloudKit tombstone for a record that was still on disk; on next sync the connection would be nuked from iCloud as well. Now: delete, add, update, duplicate, batch-delete, sync-pull, group-cleanup, and the plugin secure field migration all check the persistence result and skip the downstream side effects on failure. The connection form surfaces a localized error and keeps the form open instead of dismissing. +- iCloud sync no longer silently substitutes empty defaults when an SSH config, SSL config, jump-host list, or driver-specific field stored in a synced record fails to decode. A device on an older app build that pulled a record written by a newer build would previously end up with a connection missing its SSH/SSL config, then push that empty config back to iCloud and overwrite the authoritative copy. Decode failures now skip the record entirely and log which field failed; the cloud copy stays intact until the device is updated. - Result-grid cells on rows marked for deletion keep their dropdown / date / JSON / blob chevron visible at reduced opacity instead of hiding it, so the cell type is still legible while clearly inactive. Click on the dimmed chevron is a no-op; FK arrow navigation is unchanged. Matches the macOS HIG "disabled appearance" guideline. - Foreign key navigation from a table with unsaved edits opens the referenced table in a new window tab to preserve the edit buffer. Closing that new tab no longer wipes the original tab's data grid. Previously the new tab's teardown broadcast a connection-scoped event that other coordinators on the same connection received, causing them to release their cell data. - Tables sidebar refreshes automatically after a successful SQL import; the refresh notification now fires after the success sheet's dismissal animation, so the main window is key when the observer runs (#1114) diff --git a/TablePro/Core/Sync/SyncCoordinator.swift b/TablePro/Core/Sync/SyncCoordinator.swift index bd9ca4071..a40ac1ad9 100644 --- a/TablePro/Core/Sync/SyncCoordinator.swift +++ b/TablePro/Core/Sync/SyncCoordinator.swift @@ -473,7 +473,13 @@ final class SyncCoordinator { @discardableResult private func applyRemoteConnection(_ record: CKRecord, tombstoneIds: Set) -> Bool { - guard let remoteConnection = SyncRecordMapper.toConnection(record) else { return false } + let remoteConnection: DatabaseConnection + do { + remoteConnection = try SyncRecordMapper.toConnection(record) + } catch { + Self.logger.error("Skipping remote connection \(record.recordID.recordName, privacy: .public): \(error.localizedDescription, privacy: .public)") + return false + } if tombstoneIds.contains(remoteConnection.id.uuidString) { return false @@ -544,7 +550,13 @@ final class SyncCoordinator { } private func applyRemoteSSHProfile(_ record: CKRecord, tombstoneIds: Set) { - guard let remoteProfile = SyncRecordMapper.toSSHProfile(record) else { return } + let remoteProfile: SSHProfile + do { + remoteProfile = try SyncRecordMapper.toSSHProfile(record) + } catch { + Self.logger.error("Skipping remote SSH profile \(record.recordID.recordName, privacy: .public): \(error.localizedDescription, privacy: .public)") + return + } if tombstoneIds.contains(remoteProfile.id.uuidString) { return } var profiles = SSHProfileStorage.shared.loadProfiles() diff --git a/TablePro/Core/Sync/SyncRecordMapper.swift b/TablePro/Core/Sync/SyncRecordMapper.swift index 9028383d1..660af3250 100644 --- a/TablePro/Core/Sync/SyncRecordMapper.swift +++ b/TablePro/Core/Sync/SyncRecordMapper.swift @@ -20,6 +20,20 @@ enum SyncRecordType: String, CaseIterable { case sshProfile = "SSHProfile" } +enum SyncDecodeError: Error, LocalizedError { + case missingRequiredField(String) + case decodeFailure(field: String, underlying: Error) + + var errorDescription: String? { + switch self { + case .missingRequiredField(let field): + return "Sync record missing required field: \(field)" + case .decodeFailure(let field, let underlying): + return "Sync record decode failed for \(field): \(underlying.localizedDescription)" + } + } +} + /// Pure-function mapper between local models and CKRecord struct SyncRecordMapper { private static let logger = Logger(subsystem: "com.TablePro", category: "SyncRecordMapper") @@ -119,14 +133,17 @@ struct SyncRecordMapper { return record } - static func toConnection(_ record: CKRecord) -> DatabaseConnection? { + static func toConnection(_ record: CKRecord) throws -> DatabaseConnection { guard let connectionIdString = record["connectionId"] as? String, - let connectionId = UUID(uuidString: connectionIdString), - let name = record["name"] as? String, - let typeRawValue = record["type"] as? String + let connectionId = UUID(uuidString: connectionIdString) else { - logger.warning("Failed to decode connection from CKRecord: missing required fields") - return nil + throw SyncDecodeError.missingRequiredField("connectionId") + } + guard let name = record["name"] as? String else { + throw SyncDecodeError.missingRequiredField("name") + } + guard let typeRawValue = record["type"] as? String else { + throw SyncDecodeError.missingRequiredField("type") } let host = record["host"] as? String ?? "localhost" @@ -145,22 +162,33 @@ struct SyncRecordMapper { let sortOrder = (record["sortOrder"] as? Int64).map { Int($0) } ?? 0 let sshProfileId = (record["sshProfileId"] as? String).flatMap { UUID(uuidString: $0) } - // Decode complex structs and expand portable ~/… paths to device-local form. var sshConfig = SSHConfiguration() if let sshData = record["sshConfigJson"] as? Data { - sshConfig = (try? decoder.decode(SSHConfiguration.self, from: sshData)) ?? SSHConfiguration() + do { + sshConfig = try decoder.decode(SSHConfiguration.self, from: sshData) + } catch { + throw SyncDecodeError.decodeFailure(field: "sshConfigJson", underlying: error) + } Self.expandPaths(&sshConfig) } var sslConfig = SSLConfiguration() if let sslData = record["sslConfigJson"] as? Data { - sslConfig = (try? decoder.decode(SSLConfiguration.self, from: sslData)) ?? SSLConfiguration() + do { + sslConfig = try decoder.decode(SSLConfiguration.self, from: sslData) + } catch { + throw SyncDecodeError.decodeFailure(field: "sslConfigJson", underlying: error) + } Self.expandPaths(&sslConfig) } var additionalFields: [String: String]? if let fieldsData = record["additionalFieldsJson"] as? Data { - additionalFields = try? decoder.decode([String: String].self, from: fieldsData) + do { + additionalFields = try decoder.decode([String: String].self, from: fieldsData) + } catch { + throw SyncDecodeError.decodeFailure(field: "additionalFieldsJson", underlying: error) + } } return DatabaseConnection( @@ -327,13 +355,14 @@ struct SyncRecordMapper { return record } - static func toSSHProfile(_ record: CKRecord) -> SSHProfile? { + static func toSSHProfile(_ record: CKRecord) throws -> SSHProfile { guard let profileIdString = record["profileId"] as? String, - let profileId = UUID(uuidString: profileIdString), - let name = record["name"] as? String + let profileId = UUID(uuidString: profileIdString) else { - logger.warning("Failed to decode SSH profile from CKRecord: missing required fields") - return nil + throw SyncDecodeError.missingRequiredField("profileId") + } + guard let name = record["name"] as? String else { + throw SyncDecodeError.missingRequiredField("name") } let host = record["host"] as? String ?? "" @@ -349,7 +378,11 @@ struct SyncRecordMapper { var jumpHosts: [SSHJumpHost] = [] if let jumpHostsData = record["jumpHostsJson"] as? Data { - jumpHosts = (try? decoder.decode([SSHJumpHost].self, from: jumpHostsData)) ?? [] + do { + jumpHosts = try decoder.decode([SSHJumpHost].self, from: jumpHostsData) + } catch { + throw SyncDecodeError.decodeFailure(field: "jumpHostsJson", underlying: error) + } Self.expandPaths(&jumpHosts) }