fix: address PR review — fix partial registration, wire up dead code, rewrite tests

datlechin · datlechin · commit 96375dfd1e98 · 2026-03-13T16:57:20.000+07:00
diff --git a/TablePro/Core/Plugins/PluginManager.swift b/TablePro/Core/Plugins/PluginManager.swift
@@ -62,6 +62,7 @@ final class PluginManager {
     private static let logger = Logger(subsystem: "com.TablePro", category: "PluginManager")
 
     private var pendingPluginURLs: [(url: URL, source: PluginSource)] = []
+    private var validatedConnectionFieldPlugins: Set<String> = []
 
     private init() {}
 
@@ -242,8 +243,8 @@ final class PluginManager {
     // MARK: - Capability Registration
 
     private func registerCapabilities(_ instance: any TableProPlugin, pluginId: String) {
-        pluginInstances[pluginId] = instance
         let declared = Set(type(of: instance).capabilities)
+        var registeredAny = false
 
         if let driver = instance as? any DriverPlugin {
             if !declared.contains(.databaseDriver) {
@@ -252,15 +253,17 @@ final class PluginManager {
             do {
                 try validateDriverDescriptor(type(of: driver), pluginId: pluginId)
             } catch {
-                Self.logger.error("Plugin '\(pluginId)' rejected: \(error.localizedDescription)")
-                return
+                Self.logger.error("Plugin '\(pluginId)' driver rejected: \(error.localizedDescription)")
             }
-            let typeId = type(of: driver).databaseTypeId
-            driverPlugins[typeId] = driver
-            for additionalId in type(of: driver).additionalDatabaseTypeIds {
-                driverPlugins[additionalId] = driver
+            if !driverPlugins.keys.contains(type(of: driver).databaseTypeId) {
+                let typeId = type(of: driver).databaseTypeId
+                driverPlugins[typeId] = driver
+                for additionalId in type(of: driver).additionalDatabaseTypeIds {
+                    driverPlugins[additionalId] = driver
+                }
+                Self.logger.debug("Registered driver plugin '\(pluginId)' for database type '\(typeId)'")
+                registeredAny = true
             }
-            Self.logger.debug("Registered driver plugin '\(pluginId)' for database type '\(typeId)'")
         }
 
         if let exportPlugin = instance as? any ExportFormatPlugin {
@@ -270,6 +273,7 @@ final class PluginManager {
             let formatId = type(of: exportPlugin).formatId
             exportPlugins[formatId] = exportPlugin
             Self.logger.debug("Registered export plugin '\(pluginId)' for format '\(formatId)'")
+            registeredAny = true
         }
 
         if let importPlugin = instance as? any ImportFormatPlugin {
@@ -279,6 +283,11 @@ final class PluginManager {
             let formatId = type(of: importPlugin).formatId
             importPlugins[formatId] = importPlugin
             Self.logger.debug("Registered import plugin '\(pluginId)' for format '\(formatId)'")
+            registeredAny = true
+        }
+
+        if registeredAny {
+            pluginInstances[pluginId] = instance
         }
     }
 
@@ -303,7 +312,7 @@ final class PluginManager {
 
     /// Reject-level validation: runs synchronously before registration.
     /// Checks only properties already accessed during the loading flow.
-    private func validateDriverDescriptor(_ driverType: any DriverPlugin.Type, pluginId: String) throws {
+    func validateDriverDescriptor(_ driverType: any DriverPlugin.Type, pluginId: String) throws {
         guard !driverType.databaseTypeId.trimmingCharacters(in: .whitespaces).isEmpty else {
             throw PluginError.invalidDescriptor(pluginId: pluginId, reason: "databaseTypeId is empty")
         }
@@ -322,6 +331,9 @@ final class PluginManager {
         }
 
         let allAdditionalIds = driverType.additionalDatabaseTypeIds
+        // Warn-only (not reject): redundant but harmless — the primary ID is already registered,
+        // so the duplicate entry in additionalIds just overwrites with the same value.
+        // Cross-plugin duplicates are rejected above because they indicate a real conflict.
         if allAdditionalIds.contains(typeId) {
             Self.logger.warning("Plugin '\(pluginId)': additionalDatabaseTypeIds contains the primary databaseTypeId '\(typeId)'")
         }
@@ -337,9 +349,10 @@ final class PluginManager {
         }
     }
 
-    /// Warn-level connection field validation. Called lazily when fields are accessed,
-    /// not during plugin loading (protocol witness tables may be unstable for dynamically loaded bundles).
-    private func validateConnectionFields(_ fields: [ConnectionField], pluginId: String) {
+    /// Warn-level connection field validation. Called lazily on first access via
+    /// `additionalConnectionFields(for:)`, not during plugin loading (protocol witness
+    /// tables may be unstable for dynamically loaded bundles during the loading path).
+    func validateConnectionFields(_ fields: [ConnectionField], pluginId: String) {
         var seenIds = Set<String>()
         for field in fields {
             if field.id.trimmingCharacters(in: .whitespaces).isEmpty {
@@ -445,7 +458,13 @@ final class PluginManager {
     func additionalConnectionFields(for databaseType: DatabaseType) -> [ConnectionField] {
         loadPendingPlugins()
         guard let plugin = driverPlugins[databaseType.pluginTypeId] else { return [] }
-        return Swift.type(of: plugin).additionalConnectionFields
+        let fields = Swift.type(of: plugin).additionalConnectionFields
+        let pluginId = databaseType.pluginTypeId
+        if !validatedConnectionFieldPlugins.contains(pluginId) {
+            validatedConnectionFieldPlugins.insert(pluginId)
+            validateConnectionFields(fields, pluginId: pluginId)
+        }
+        return fields
     }
 
     // MARK: - Plugin Property Lookups
diff --git a/TableProTests/Core/Plugins/PluginValidationTests.swift b/TableProTests/Core/Plugins/PluginValidationTests.swift
@@ -8,6 +8,115 @@ import TableProPluginKit
 import Testing
 @testable import TablePro
 
+// MARK: - Mock DriverPlugin for Testing
+
+private final class MockDriverPlugin: NSObject, TableProPlugin, DriverPlugin {
+    static var pluginName = "MockDriver"
+    static var pluginVersion = "1.0.0"
+    static var pluginDescription = "Test plugin"
+    static var capabilities: [PluginCapability] = [.databaseDriver]
+    static var dependencies: [String] = []
+
+    static var databaseTypeId = "mock-db"
+    static var databaseDisplayName = "Mock Database"
+    static var iconName = "cylinder.fill"
+    static var defaultPort = 9999
+
+    func createDriver(config: DriverConnectionConfig) -> any PluginDatabaseDriver {
+        fatalError("Not used in tests")
+    }
+
+    required override init() {
+        super.init()
+    }
+
+    static func reset(
+        typeId: String = "mock-db",
+        displayName: String = "Mock Database",
+        additionalIds: [String] = []
+    ) {
+        databaseTypeId = typeId
+        databaseDisplayName = displayName
+        additionalDatabaseTypeIds = additionalIds
+    }
+
+    static var additionalDatabaseTypeIds: [String] = []
+}
+
+// MARK: - validateDriverDescriptor Tests
+
+@Suite("PluginManager.validateDriverDescriptor")
+struct ValidateDriverDescriptorTests {
+
+    @Test("rejects empty databaseTypeId")
+    @MainActor func rejectsEmptyTypeId() {
+        MockDriverPlugin.reset(typeId: "")
+        let pm = PluginManager.shared
+        #expect(throws: PluginError.self) {
+            try pm.validateDriverDescriptor(MockDriverPlugin.self, pluginId: "test")
+        }
+    }
+
+    @Test("rejects whitespace-only databaseTypeId")
+    @MainActor func rejectsWhitespaceTypeId() {
+        MockDriverPlugin.reset(typeId: "   ")
+        let pm = PluginManager.shared
+        #expect(throws: PluginError.self) {
+            try pm.validateDriverDescriptor(MockDriverPlugin.self, pluginId: "test")
+        }
+    }
+
+    @Test("rejects empty databaseDisplayName")
+    @MainActor func rejectsEmptyDisplayName() {
+        MockDriverPlugin.reset(typeId: "valid-id", displayName: "")
+        let pm = PluginManager.shared
+        #expect(throws: PluginError.self) {
+            try pm.validateDriverDescriptor(MockDriverPlugin.self, pluginId: "test")
+        }
+    }
+
+    @Test("rejects whitespace-only databaseDisplayName")
+    @MainActor func rejectsWhitespaceDisplayName() {
+        MockDriverPlugin.reset(typeId: "valid-id", displayName: "   ")
+        let pm = PluginManager.shared
+        #expect(throws: PluginError.self) {
+            try pm.validateDriverDescriptor(MockDriverPlugin.self, pluginId: "test")
+        }
+    }
+
+    @Test("accepts valid descriptor with no conflicts")
+    @MainActor func acceptsValidDescriptor() throws {
+        MockDriverPlugin.reset(typeId: "unique-test-db-type", displayName: "Unique Test DB")
+        let pm = PluginManager.shared
+        try pm.validateDriverDescriptor(MockDriverPlugin.self, pluginId: "test")
+    }
+
+    @Test("rejects duplicate primary type ID already registered")
+    @MainActor func rejectsDuplicatePrimaryTypeId() {
+        // "MySQL" is registered by the built-in MySQL plugin
+        MockDriverPlugin.reset(typeId: "MySQL", displayName: "Fake MySQL")
+        let pm = PluginManager.shared
+        #expect(throws: PluginError.self) {
+            try pm.validateDriverDescriptor(MockDriverPlugin.self, pluginId: "test")
+        }
+    }
+
+    @Test("rejects duplicate additional type ID already registered")
+    @MainActor func rejectsDuplicateAdditionalTypeId() {
+        MockDriverPlugin.reset(
+            typeId: "unique-test-db-type-2",
+            displayName: "Test DB",
+            additionalIds: ["MySQL"]
+        )
+        let pm = PluginManager.shared
+        #expect(throws: PluginError.self) {
+            try pm.validateDriverDescriptor(MockDriverPlugin.self, pluginId: "test")
+        }
+    }
+}
+
+// MARK: - PluginError.invalidDescriptor Formatting
+
 @Suite("PluginError.invalidDescriptor")
 struct PluginErrorInvalidDescriptorTests {
 
@@ -33,90 +142,61 @@ struct PluginErrorInvalidDescriptorTests {
         #expect(description.contains("mysql"))
         #expect(description.contains("MySQL"))
     }
-
-    @Test("error description for empty display name")
-    func emptyDisplayNameDescription() {
-        let error = PluginError.invalidDescriptor(
-            pluginId: "com.example.test",
-            reason: "databaseDisplayName is empty"
-        )
-        let description = error.localizedDescription
-        #expect(description.contains("databaseDisplayName is empty"))
-    }
-
-    @Test("error description for additional type ID conflict")
-    func additionalTypeIdConflict() {
-        let error = PluginError.invalidDescriptor(
-            pluginId: "com.example.multi",
-            reason: "additionalDatabaseTypeId 'redshift' is already registered by 'PostgreSQL'"
-        )
-        let description = error.localizedDescription
-        #expect(description.contains("additionalDatabaseTypeId"))
-        #expect(description.contains("redshift"))
-        #expect(description.contains("PostgreSQL"))
-    }
 }
 
-@Suite("ConnectionField Validation Logic")
-struct ConnectionFieldValidationLogicTests {
+// MARK: - validateConnectionFields Tests
+
+@Suite("PluginManager.validateConnectionFields")
+struct ValidateConnectionFieldsTests {
 
-    @Test("duplicate field IDs are detectable")
-    func duplicateFieldIds() {
+    @Test("duplicate field IDs are detected")
+    @MainActor func duplicateFieldIds() {
         let fields = [
             ConnectionField(id: "encoding", label: "Encoding"),
             ConnectionField(id: "timeout", label: "Timeout"),
             ConnectionField(id: "encoding", label: "Character Encoding")
         ]
-        var seenIds = Set<String>()
-        var duplicates: [String] = []
-        for field in fields {
-            if !seenIds.insert(field.id).inserted {
-                duplicates.append(field.id)
-            }
-        }
-        #expect(duplicates == ["encoding"])
+        // Should not crash — warns via logger
+        PluginManager.shared.validateConnectionFields(fields, pluginId: "test")
     }
 
-    @Test("empty field ID is detectable")
-    func emptyFieldId() {
-        let field = ConnectionField(id: "", label: "Something")
-        #expect(field.id.trimmingCharacters(in: .whitespaces).isEmpty)
+    @Test("empty field ID is detected without crash")
+    @MainActor func emptyFieldId() {
+        let fields = [ConnectionField(id: "", label: "Something")]
+        PluginManager.shared.validateConnectionFields(fields, pluginId: "test")
     }
 
-    @Test("empty field label is detectable")
-    func emptyFieldLabel() {
-        let field = ConnectionField(id: "test", label: "")
-        #expect(field.label.trimmingCharacters(in: .whitespaces).isEmpty)
+    @Test("empty field label is detected without crash")
+    @MainActor func emptyFieldLabel() {
+        let fields = [ConnectionField(id: "test", label: "")]
+        PluginManager.shared.validateConnectionFields(fields, pluginId: "test")
     }
 
-    @Test("dropdown with empty options is detectable")
-    func emptyDropdownOptions() {
-        let field = ConnectionField(
-            id: "encoding",
-            label: "Encoding",
-            fieldType: .dropdown(options: [])
-        )
-        if case .dropdown(let options) = field.fieldType {
-            #expect(options.isEmpty)
-        } else {
-            Issue.record("Expected dropdown field type")
-        }
+    @Test("dropdown with empty options is detected without crash")
+    @MainActor func emptyDropdownOptions() {
+        let fields = [
+            ConnectionField(
+                id: "encoding",
+                label: "Encoding",
+                fieldType: .dropdown(options: [])
+            )
+        ]
+        PluginManager.shared.validateConnectionFields(fields, pluginId: "test")
     }
 
-    @Test("dropdown with options is valid")
-    func validDropdown() {
-        let field = ConnectionField(
-            id: "encoding",
-            label: "Encoding",
-            fieldType: .dropdown(options: [
-                ConnectionField.DropdownOption(value: "utf8", label: "UTF-8"),
-                ConnectionField.DropdownOption(value: "latin1", label: "Latin-1")
-            ])
-        )
-        if case .dropdown(let options) = field.fieldType {
-            #expect(options.count == 2)
-        } else {
-            Issue.record("Expected dropdown field type")
-        }
+    @Test("valid fields pass without issue")
+    @MainActor func validFields() {
+        let fields = [
+            ConnectionField(id: "encoding", label: "Encoding"),
+            ConnectionField(
+                id: "mode",
+                label: "Mode",
+                fieldType: .dropdown(options: [
+                    ConnectionField.DropdownOption(value: "fast", label: "Fast"),
+                    ConnectionField.DropdownOption(value: "safe", label: "Safe")
+                ])
+            )
+        ]
+        PluginManager.shared.validateConnectionFields(fields, pluginId: "test")
     }
 }
