-
-
Notifications
You must be signed in to change notification settings - Fork 297
Fix external access setting persistence #1730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
52ac822
b66f049
e736b69
4af1adc
eae3d1f
42615c9
c1af6d3
7d0cb06
114a110
51baf68
d50a2c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,8 @@ struct StoredConnection: Codable { | |
|
|
||
| let safeModeLevel: String | ||
|
|
||
| let externalAccess: String | ||
|
|
||
| let aiPolicy: String? | ||
|
|
||
| // AI rules text included in the system prompt for this connection | ||
|
|
@@ -115,6 +117,8 @@ struct StoredConnection: Codable { | |
|
|
||
| self.safeModeLevel = connection.safeModeLevel.rawValue | ||
|
|
||
| self.externalAccess = connection.externalAccess.rawValue | ||
|
|
||
| self.aiPolicy = connection.aiPolicy?.rawValue | ||
| self.aiRules = connection.aiRules | ||
| self.aiAlwaysAllowedTools = connection.aiAlwaysAllowedTools.isEmpty | ||
|
|
@@ -163,6 +167,7 @@ struct StoredConnection: Codable { | |
| case sslMode, sslCaCertificatePath, sslClientCertificatePath, sslClientKeyPath | ||
| case color, tagId, groupId, sshProfileId | ||
| case safeModeLevel | ||
| case externalAccess | ||
| case isReadOnly // Legacy key for migration reading only | ||
| case aiPolicy | ||
| case aiRules | ||
|
|
@@ -207,6 +212,7 @@ struct StoredConnection: Codable { | |
| try container.encodeIfPresent(groupId, forKey: .groupId) | ||
| try container.encodeIfPresent(sshProfileId, forKey: .sshProfileId) | ||
| try container.encode(safeModeLevel, forKey: .safeModeLevel) | ||
| try container.encode(externalAccess, forKey: .externalAccess) | ||
| try container.encodeIfPresent(aiPolicy, forKey: .aiPolicy) | ||
| try container.encodeIfPresent(aiRules, forKey: .aiRules) | ||
| try container.encodeIfPresent(aiAlwaysAllowedTools, forKey: .aiAlwaysAllowedTools) | ||
|
|
@@ -272,6 +278,9 @@ struct StoredConnection: Codable { | |
| let wasReadOnly = try container.decodeIfPresent(Bool.self, forKey: .isReadOnly) ?? false | ||
| safeModeLevel = wasReadOnly ? SafeModeLevel.readOnly.rawValue : SafeModeLevel.silent.rawValue | ||
| } | ||
| externalAccess = try container.decodeIfPresent( | ||
| String.self, forKey: .externalAccess | ||
| ) ?? ExternalAccessLevel.readOnly.rawValue | ||
|
Comment on lines
+281
to
+283
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fresh evidence in this revision is that the decoder only reads the new top-level Useful? React with 👍 / 👎. |
||
| aiPolicy = try container.decodeIfPresent(String.self, forKey: .aiPolicy) | ||
| aiRules = try container.decodeIfPresent(String.self, forKey: .aiRules) | ||
| aiAlwaysAllowedTools = try container.decodeIfPresent([String].self, forKey: .aiAlwaysAllowedTools) | ||
|
|
@@ -388,6 +397,7 @@ struct StoredConnection: Codable { | |
| aiPolicy: parsedAIPolicy, | ||
| aiRules: aiRules, | ||
| aiAlwaysAllowedTools: Set(aiAlwaysAllowedTools ?? []), | ||
| externalAccess: ExternalAccessLevel(rawValue: externalAccess) ?? .readOnly, | ||
| redisDatabase: redisDatabase, | ||
| startupCommands: startupCommands, | ||
| sortOrder: sortOrder, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| // | ||
| // ConnectionStorageExternalAccessTests.swift | ||
| // TableProTests | ||
| // | ||
|
|
||
| import Foundation | ||
| import TableProPluginKit | ||
| @testable import TablePro | ||
| import Testing | ||
|
|
||
| @Suite("ConnectionStorage External Access") | ||
| @MainActor | ||
| struct ConnectionStorageExternalAccessTests { | ||
| private let storage: ConnectionStorage | ||
|
|
||
| init() { | ||
| let unique = UUID().uuidString | ||
| let fileURL = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("tablepro-tests") | ||
| .appendingPathComponent("connections_\(unique).json") | ||
| try? FileManager.default.createDirectory( | ||
| at: fileURL.deletingLastPathComponent(), | ||
| withIntermediateDirectories: true | ||
| ) | ||
| let defaultsName = "com.TablePro.tests.ConnectionStorage.ExternalAccess.\(unique)" | ||
| let syncName = "com.TablePro.tests.Sync.ExternalAccess.\(unique)" | ||
| guard let defaults = UserDefaults(suiteName: defaultsName), | ||
| let syncDefaults = UserDefaults(suiteName: syncName) else { | ||
| fatalError("UserDefaults suite creation failed in test setup") | ||
| } | ||
| let metadata = SyncMetadataStorage(userDefaults: syncDefaults) | ||
| let tracker = SyncChangeTracker(metadataStorage: metadata) | ||
| self.storage = ConnectionStorage( | ||
| fileURL: fileURL, | ||
| userDefaults: defaults, | ||
| syncTracker: tracker | ||
| ) | ||
| } | ||
|
|
||
| @Test("round-trip preserves external access", arguments: ExternalAccessLevel.allCases) | ||
| func roundTripExternalAccess(_ level: ExternalAccessLevel) { | ||
| let id = UUID() | ||
| let connection = DatabaseConnection( | ||
| id: id, | ||
| name: "Test", | ||
| type: .postgresql, | ||
| externalAccess: level | ||
| ) | ||
|
|
||
| storage.addConnection(connection) | ||
| defer { storage.deleteConnection(connection) } | ||
|
|
||
| let loaded = storage.loadConnections().first { $0.id == id } | ||
| #expect(loaded?.externalAccess == level) | ||
| } | ||
|
|
||
| @Test("external access survives mutate-and-update cycle") | ||
| func updateExternalAccess() { | ||
| let id = UUID() | ||
| let connection = DatabaseConnection( | ||
| id: id, | ||
| name: "Test", | ||
| type: .mysql, | ||
| externalAccess: .readOnly | ||
| ) | ||
|
|
||
| storage.addConnection(connection) | ||
| defer { storage.deleteConnection(connection) } | ||
|
|
||
| var updated = connection | ||
| updated.externalAccess = .readWrite | ||
| storage.updateConnection(updated) | ||
|
|
||
| let loaded = storage.loadConnections().first { $0.id == id } | ||
| #expect(loaded?.externalAccess == .readWrite) | ||
| } | ||
|
|
||
| @Test("legacy records without externalAccess default to readOnly") | ||
| func legacyDecodeDefaultsToReadOnly() throws { | ||
| let stored = try JSONDecoder().decode( | ||
| StoredConnection.self, | ||
| from: Data(Self.legacyJSONWithoutExternalAccess.utf8) | ||
| ) | ||
| #expect(stored.toConnection().externalAccess == .readOnly) | ||
| } | ||
|
|
||
| private static let legacyJSONWithoutExternalAccess = """ | ||
| { | ||
| "id": "11111111-2222-3333-4444-555555555555", | ||
| "name": "Legacy", | ||
| "host": "localhost", | ||
| "port": 3306, | ||
| "database": "test", | ||
| "username": "root", | ||
| "type": "MySQL", | ||
| "sshEnabled": false, | ||
| "sshHost": "", | ||
| "sshUsername": "", | ||
| "sshAuthMethod": "password", | ||
| "sshPrivateKeyPath": "", | ||
| "sshAgentSocketPath": "", | ||
| "sslMode": "disabled", | ||
| "sslCaCertificatePath": "", | ||
| "sslClientCertificatePath": "", | ||
| "sslClientKeyPath": "", | ||
| "color": "None", | ||
| "safeModeLevel": "silent", | ||
| "sortOrder": 0, | ||
| "localOnly": false, | ||
| "isSample": false, | ||
| "isFavorite": false, | ||
| "totpMode": "none", | ||
| "totpAlgorithm": "sha1", | ||
| "totpDigits": 6, | ||
| "totpPeriod": 30 | ||
| } | ||
| """ | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| // | ||
| // AdvancedPaneViewModelTests.swift | ||
| // TableProTests | ||
| // | ||
|
|
||
| import Foundation | ||
| import TableProPluginKit | ||
| @testable import TablePro | ||
| import Testing | ||
|
|
||
| @Suite("Advanced pane external access") | ||
| @MainActor | ||
| struct AdvancedPaneViewModelTests { | ||
| @Test("Loads external access from the connection") | ||
| func loadsExternalAccessFromConnection() { | ||
| let connection = DatabaseConnection(name: "Test", externalAccess: .readWrite) | ||
| let viewModel = AdvancedPaneViewModel() | ||
|
|
||
| viewModel.load(from: connection) | ||
|
|
||
| #expect(viewModel.externalAccess == .readWrite) | ||
| } | ||
|
|
||
| @Test("Does not leak external access into plugin additional fields") | ||
| func doesNotWriteExternalAccessIntoAdditionalFields() { | ||
| let viewModel = AdvancedPaneViewModel() | ||
| viewModel.externalAccess = .readWrite | ||
|
|
||
| var fields: [String: String] = [:] | ||
| viewModel.write(into: &fields) | ||
|
|
||
| #expect(fields["externalAccess"] == nil) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With iCloud sync enabled, this only writes the new setting to the local
StoredConnectionJSON.ConnectionStorage.add/updatemarks dirty connections andSyncCoordinator.uploadPendingChangesserializes them throughSyncRecordMapper.toCKRecord, but that mapper still writes fields likesafeModeLeveland never writes or readsexternalAccess, so a connection set to Blocked or Read & Write on one Mac arrives on another as the default Read Only. Please add the field to the CloudKit record mapping alongside this local persistence.Useful? React with 👍 / 👎.