fix(Core): Fix plugin configuration validation (#543)

Previous validation logic threw an error if it encountered a configuration stanza with no corresponding plugin registered for that key. However, the current CLI emits some plugin keys in unexpected situations, breaking this validation. Remediation would require a customer to edit the config file, which is possibly dangerous, and definitely inconvenient. This change: - Replaces the throwing error with a logged warning when the config encounters an unmatched plugin key - Changes the `Plugin.configure` signature to accept an optional `Any?` argument, instead of a non-optional. This will let plugins decide if they need to throw or not. - Fixes a data race in hub test code - Fix AWSCognitoAuthPlugin and AWSS3StoragePlugin to throw a PluginError instead of a category-specific error if they encounter an error during configuration fixes #540
aws-amplify · Jun 16, 2020 · d526c61 · d526c61
1 parent 2b6972b
commit d526c61
Show file tree

Hide file tree

Showing 56 changed files with 600 additions and 134 deletions.
diff --git a/Amplify.xcodeproj/project.pbxproj b/Amplify.xcodeproj/project.pbxproj
@@ -2834,9 +2834,9 @@
 		FAC2354C227A056600424678 /* CategoryTests */ = {
 			isa = PBXGroup;
 			children = (
-				B4F3E9F624314E1300F23296 /* Auth */,
 				FAC2355D227A056600424678 /* Analytics */,
 				FAC23555227A056600424678 /* API */,
+				B4F3E9F624314E1300F23296 /* Auth */,
 				FAD3937B23820CE200463F5E /* DataStore */,
 				FAC23584227A442000424678 /* Hub */,
 				FAC23559227A056600424678 /* Logging */,
@@ -2956,9 +2956,9 @@
 			isa = PBXGroup;
 			children = (
 				FA607FE1233D131B00DFEA24 /* AmplifyOperationHubTests.swift */,
-				FACF52052329652600646E10 /* DefaultPluginTests */,
 				FAC23585227A443200424678 /* HubCategoryConfigurationTests.swift */,
 				FAC23587227A446C00424678 /* HubClientAPITests.swift */,
+				FACF52052329652600646E10 /* DefaultPluginTests */,
 			);
 			path = Hub;
 			sourceTree = "<group>";

diff --git a/Amplify/Categories/API/Internal/APICategory+CategoryConfigurable.swift b/Amplify/Categories/API/Internal/APICategory+CategoryConfigurable.swift
@@ -16,10 +16,7 @@ extension AmplifyAPICategory: CategoryConfigurable {
             throw error
         }
 
-        for (pluginKey, pluginConfiguration) in configuration.plugins {
-            let plugin = try getPlugin(for: pluginKey)
-            try plugin.configure(using: pluginConfiguration)
-        }
+        try Amplify.configure(plugins: Array(plugins.values), using: configuration)
 
         isConfigured = true
     }

diff --git a/Amplify/Categories/Analytics/Internal/AnalyticsCategory+CategoryConfigurable.swift b/Amplify/Categories/Analytics/Internal/AnalyticsCategory+CategoryConfigurable.swift
@@ -16,10 +16,7 @@ extension AnalyticsCategory: CategoryConfigurable {
             throw error
         }
 
-        for (pluginKey, pluginConfiguration) in configuration.plugins {
-            let plugin = try getPlugin(for: pluginKey)
-            try plugin.configure(using: pluginConfiguration)
-        }
+        try Amplify.configure(plugins: Array(plugins.values), using: configuration)
 
         isConfigured = true
     }

diff --git a/Amplify/Categories/Auth/Internal/AuthCategory+CategoryConfigurable.swift b/Amplify/Categories/Auth/Internal/AuthCategory+CategoryConfigurable.swift
@@ -18,10 +18,7 @@ extension AuthCategory: CategoryConfigurable {
             throw error
         }
 
-        for (pluginKey, pluginConfiguration) in configuration.plugins {
-            let plugin = try getPlugin(for: pluginKey)
-            try plugin.configure(using: pluginConfiguration)
-        }
+        try Amplify.configure(plugins: Array(plugins.values), using: configuration)
 
         isConfigured = true
     }

diff --git a/Amplify/Categories/DataStore/Internal/DataStoreCategory+Configurable.swift b/Amplify/Categories/DataStore/Internal/DataStoreCategory+Configurable.swift
@@ -24,10 +24,7 @@ extension DataStoreCategory: CategoryConfigurable {
             throw error
         }
 
-        for (pluginKey, pluginConfiguration) in configuration.plugins {
-            let plugin = try getPlugin(for: pluginKey)
-            try plugin.configure(using: pluginConfiguration)
-        }
+        try Amplify.configure(plugins: Array(plugins.values), using: configuration)
 
         isConfigured = true
     }

diff --git a/Amplify/Categories/Hub/Internal/HubCategory+CategoryConfigurable.swift b/Amplify/Categories/Hub/Internal/HubCategory+CategoryConfigurable.swift
@@ -18,10 +18,7 @@ extension HubCategory: CategoryConfigurable {
             throw error
         }
 
-        for (pluginKey, pluginConfiguration) in configuration.plugins {
-            let plugin = try getPlugin(for: pluginKey)
-            try plugin.configure(using: pluginConfiguration)
-        }
+        try Amplify.configure(plugins: Array(plugins.values), using: configuration)
 
         configurationState = .configured
     }

diff --git a/Amplify/Categories/Logging/Internal/LoggingCategory+CategoryConfigurable.swift b/Amplify/Categories/Logging/Internal/LoggingCategory+CategoryConfigurable.swift
@@ -26,18 +26,7 @@ extension LoggingCategory: CategoryConfigurable {
                 throw error
             }
 
-            guard let pluginConfiguration = configuration.plugins[plugin.key] else {
-                throw LoggingError.configuration(
-                    "No configuration found for added plugin `\(plugin.key)`",
-                    """
-                    Either fix the configuration file to specify the plugin's key value of '\(plugin.key)',
-                    or add a plugin with one of the keys specified in the configuration:
-                    \(configuration.plugins.keys.joined(separator: ", "))
-                    """
-                )
-            }
-
-            try plugin.configure(using: pluginConfiguration)
+            try plugin.configure(using: configuration.plugins[plugin.key])
             self.plugin = plugin
             configurationState = .configured
         }

diff --git a/Amplify/Categories/Predictions/Internal/PredictionsCategory+CategoryConfigurable.swift b/Amplify/Categories/Predictions/Internal/PredictionsCategory+CategoryConfigurable.swift
@@ -16,10 +16,7 @@ extension PredictionsCategory: CategoryConfigurable {
             throw error
         }
 
-        for (pluginKey, pluginConfiguration) in configuration.plugins {
-            let plugin = try getPlugin(for: pluginKey)
-            try plugin.configure(using: pluginConfiguration)
-        }
+        try Amplify.configure(plugins: Array(plugins.values), using: configuration)
 
         isConfigured = true
     }

diff --git a/Amplify/Categories/Storage/Internal/StorageCategory+CategoryConfigurable.swift b/Amplify/Categories/Storage/Internal/StorageCategory+CategoryConfigurable.swift
@@ -16,10 +16,7 @@ extension StorageCategory: CategoryConfigurable {
             throw error
         }
 
-        for (pluginKey, pluginConfiguration) in configuration.plugins {
-            let plugin = try getPlugin(for: pluginKey)
-            try plugin.configure(using: pluginConfiguration)
-        }
+        try Amplify.configure(plugins: Array(plugins.values), using: configuration)
 
         isConfigured = true
     }

diff --git a/Amplify/Core/Configuration/AmplifyConfiguration.swift b/Amplify/Core/Configuration/AmplifyConfiguration.swift
@@ -145,4 +145,21 @@ extension Amplify {
 
         try configurable.configure(using: configuration)
     }
+
+    /// Configures a list of plugins with the specified CategoryConfiguration. If any configurations do not match the
+    /// specified plugins, emits a log warning.
+    static func configure(plugins: [Plugin], using configuration: CategoryConfiguration) throws {
+        var pluginConfigurations = configuration.plugins
+
+        for plugin in plugins {
+            let pluginConfiguration = pluginConfigurations[plugin.key]
+            try plugin.configure(using: pluginConfiguration)
+            pluginConfigurations.removeValue(forKey: plugin.key)
+        }
+
+        for unusedPluginKey in pluginConfigurations.keys {
+            log.warn("No plugin found for configuration key `\(unusedPluginKey)`. Add a plugin for that key.")
+        }
+    }
+
 }
diff --git a/Amplify/Core/Plugin/Plugin.swift b/Amplify/Core/Plugin/Plugin.swift
@@ -20,7 +20,7 @@ public protocol Plugin: CategoryTypeable, Resettable {
     /// handling potential conflicts with globally-specified options.
     /// - Throws:
     ///   - PluginError.pluginConfigurationError: If the plugin encounters an error during configuration
-    func configure(using configuration: Any) throws
+    func configure(using configuration: Any?) throws
 }
 
 /// Convenience typealias to clarify when Strings are being used as plugin keys

diff --git a/Amplify/DefaultPlugins/AWSHubPlugin/AWSHubPlugin.swift b/Amplify/DefaultPlugins/AWSHubPlugin/AWSHubPlugin.swift
@@ -34,8 +34,8 @@ final public class AWSHubPlugin: HubCategoryPlugin {
     }
 
     /// For protocol conformance only--this plugin has no applicable configurations
-    public func configure(using configuration: Any) throws {
-        return
+    public func configure(using configuration: Any?) throws {
+        // Do nothing
     }
 
     /// Removes listeners and empties the message queue

diff --git a/Amplify/DefaultPlugins/AWSUnifiedLoggingPlugin/AWSUnifiedLoggingPlugin.swift b/Amplify/DefaultPlugins/AWSUnifiedLoggingPlugin/AWSUnifiedLoggingPlugin.swift
@@ -45,8 +45,8 @@ final public class AWSUnifiedLoggingPlugin: LoggingCategoryPlugin {
     }
 
     /// For protocol conformance only--this plugin has no applicable configurations
-    public func configure(using configuration: Any) throws {
-        return
+    public func configure(using configuration: Any?) throws {
+        // Do nothing
     }
 
     /// Removes listeners and empties the message queue

diff --git a/AmplifyPlugins/API/APICategoryPlugin.xcodeproj/project.pbxproj b/AmplifyPlugins/API/APICategoryPlugin.xcodeproj/project.pbxproj
@@ -922,6 +922,7 @@
 		B4DFA5BE237A611D0013E17B /* AWSAPICategoryPluginTests */ = {
 			isa = PBXGroup;
 			children = (
+				B4DFA5DD237A611D0013E17B /* Info.plist */,
 				B4DFA5DE237A611D0013E17B /* AWSAPICategoryPlugin+ConfigureTests.swift */,
 				B4DFA5CC237A611D0013E17B /* AWSAPICategoryPlugin+GraphQLBehaviorTests.swift */,
 				B4DFA5C9237A611D0013E17B /* AWSAPICategoryPlugin+InterceptorBehaviorTests.swift */,
@@ -932,7 +933,6 @@
 				B4DFA5CB237A611D0013E17B /* AWSAPICategoryPluginTestBase.swift */,
 				FA9554E1238B6C1200D42A43 /* Concurrency */,
 				B4DFA5C6237A611D0013E17B /* Configuration */,
-				B4DFA5DD237A611D0013E17B /* Info.plist */,
 				B4DFA5D9237A611D0013E17B /* Interceptor */,
 				B4DFA5BF237A611D0013E17B /* Mocks */,
 				B4DFA5C3237A611D0013E17B /* Operation */,

diff --git a/AmplifyPlugins/API/AWSAPICategoryPlugin/AWSAPIPlugin+Configure.swift b/AmplifyPlugins/API/AWSAPICategoryPlugin/AWSAPIPlugin+Configure.swift
@@ -17,16 +17,16 @@ public extension AWSAPIPlugin {
     /// - Parameter configuration: The configuration specified for this plugin
     /// - Throws:
     ///   - PluginError.pluginConfigurationError: If one of the required configuration values is invalid or empty
-    func configure(using configuration: Any) throws {
+    func configure(using configuration: Any?) throws {
 
         guard let jsonValue = configuration as? JSONValue else {
             throw PluginError.pluginConfigurationError(
                 "Could not cast incoming configuration to JSONValue",
                 """
-                The specified configuration is not convertible to a JSONValue. Review the configuration and ensure it \
-                contains the expected values, and does not use any types that aren't convertible to a corresponding \
-                JSONValue:
-                \(configuration)
+                The specified configuration is either nil, or not convertible to a JSONValue. Review the configuration \
+                and ensure it contains the expected values, and does not use any types that aren't convertible to a \
+                corresponding JSONValue:
+                \(String(describing: configuration))
                 """
             )
         }

diff --git a/.../API/AWSAPICategoryPluginTests/Configuration/AWSAPICategoryPluginConfigurationTests.swift b/.../API/AWSAPICategoryPluginTests/Configuration/AWSAPICategoryPluginConfigurationTests.swift
@@ -7,14 +7,26 @@
 
 import XCTest
 
+@testable import Amplify
+@testable import AWSAPICategoryPlugin
+
 class AWSAPICategoryPluginConfigurationTests: XCTestCase {
-    // TODO: Fix incomplete implementation
 
-    //    func testConfigureSuccess() throws {
-    //        XCTFail("Not yet implemented.")
-    //    }
-    //
-    //    func testConfigureThrowsErrorForMissingX() throws {
-    //        XCTFail("Not yet implemented.")
-    //    }
+    func testThrowsOnMissingConfig() throws {
+        let plugin = AWSAPIPlugin()
+        try Amplify.add(plugin: plugin)
+
+        let categoryConfig = APICategoryConfiguration(plugins: ["NonExistentPlugin": true])
+        let amplifyConfig = AmplifyConfiguration(api: categoryConfig)
+        do {
+            try Amplify.configure(amplifyConfig)
+            XCTFail("Should have thrown a pluginConfigurationError if not supplied with a plugin-specific config.")
+        } catch {
+            guard case PluginError.pluginConfigurationError = error else {
+                XCTFail("Should have thrown a pluginConfigurationError if not supplied with a plugin-specific config.")
+                return
+            }
+        }
+    }
+
 }
diff --git a/...fyPlugins/Analytics/AWSPinpointAnalyticsPlugin/AWSPinpointAnalyticsPlugin+Configure.swift b/...fyPlugins/Analytics/AWSPinpointAnalyticsPlugin/AWSPinpointAnalyticsPlugin+Configure.swift
@@ -18,7 +18,7 @@ extension AWSPinpointAnalyticsPlugin {
     /// - Parameter configuration: The configuration specified for this plugin
     /// - Throws:
     ///   - PluginError.pluginConfigurationError: If one of the configuration values is invalid or empty
-    public func configure(using configuration: Any) throws {
+    public func configure(using configuration: Any?) throws {
         guard let config = configuration as? JSONValue else {
             throw PluginError.pluginConfigurationError(
                 AnalyticsPluginErrorConstant.decodeConfigurationError.errorDescription,

diff --git a/.../Analytics/AWSPinpointAnalyticsPluginTests/AWSPinpointAnalyticsPluginConfigureTests.swift b/.../Analytics/AWSPinpointAnalyticsPluginTests/AWSPinpointAnalyticsPluginConfigureTests.swift
@@ -58,4 +58,5 @@ class AWSPinpointAnalyticsPluginConfigureTests: AWSPinpointAnalyticsPluginTestBa
             XCTFail("Failed to configure analytics plugin")
         }
     }
+
 }
diff --git a/...ointAnalyticsPluginTests/Configuration/AWSPinpointAnalyticsPluginConfigurationTests.swift b/...ointAnalyticsPluginTests/Configuration/AWSPinpointAnalyticsPluginConfigurationTests.swift
@@ -417,4 +417,22 @@ class AWSPinpointAnalyticsPluginConfigurationTests: XCTestCase {
             XCTAssertEqual(errorDescription, AnalyticsPluginErrorConstant.invalidRegion.errorDescription)
         }
     }
+
+    func testThrowsOnMissingConfig() throws {
+        let plugin = AWSPinpointAnalyticsPlugin()
+        try Amplify.add(plugin: plugin)
+
+        let categoryConfig = AnalyticsCategoryConfiguration(plugins: ["NonExistentPlugin": true])
+        let amplifyConfig = AmplifyConfiguration(analytics: categoryConfig)
+        do {
+            try Amplify.configure(amplifyConfig)
+            XCTFail("Should have thrown a pluginConfigurationError if not supplied with a plugin-specific config.")
+        } catch {
+            guard case PluginError.pluginConfigurationError = error else {
+                XCTFail("Should have thrown a pluginConfigurationError if not supplied with a plugin-specific config.")
+                return
+            }
+        }
+    }
+
 }
diff --git a/AmplifyPlugins/Auth/AWSCognitoAuthPlugin.xcodeproj/project.pbxproj b/AmplifyPlugins/Auth/AWSCognitoAuthPlugin.xcodeproj/project.pbxproj
@@ -114,6 +114,7 @@
 		B4F3EA4F243A782700F23296 /* ViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = B4F3EA4C243A782700F23296 /* ViewController.swift */; };
 		B4F3EA50243A782700F23296 /* AppDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = B4F3EA4D243A782700F23296 /* AppDelegate.swift */; };
 		B4F3EA51243A782700F23296 /* SceneDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = B4F3EA4E243A782700F23296 /* SceneDelegate.swift */; };
+		FA6B0EA8249443C90062AA59 /* AWSCognitoAuthPluginConfigTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = FA6B0EA7249443C90062AA59 /* AWSCognitoAuthPluginConfigTests.swift */; };
 		FECB988C412E46FD5961894A /* Pods_AWSCognitoAuthPlugin_AWSCognitoAuthPluginTests.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 1674B6AE81501F6E278CE00B /* Pods_AWSCognitoAuthPlugin_AWSCognitoAuthPluginTests.framework */; };
 /* End PBXBuildFile section */
 
@@ -278,6 +279,7 @@
 		C49A4C812B0F973F5536DCC8 /* Pods-AWSAuthPlugin.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-AWSAuthPlugin.release.xcconfig"; path = "Target Support Files/Pods-AWSAuthPlugin/Pods-AWSAuthPlugin.release.xcconfig"; sourceTree = "<group>"; };
 		C5E50D8021B9740CB511898D /* Pods-AWSAuthPlugin.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-AWSAuthPlugin.debug.xcconfig"; path = "Target Support Files/Pods-AWSAuthPlugin/Pods-AWSAuthPlugin.debug.xcconfig"; sourceTree = "<group>"; };
 		E9289652B314AA0AA1F31BC8 /* Pods-HostApp.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-HostApp.release.xcconfig"; path = "Target Support Files/Pods-HostApp/Pods-HostApp.release.xcconfig"; sourceTree = "<group>"; };
+		FA6B0EA7249443C90062AA59 /* AWSCognitoAuthPluginConfigTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AWSCognitoAuthPluginConfigTests.swift; sourceTree = "<group>"; };
 /* End PBXFileReference section */
 
 /* Begin PBXFrameworksBuildPhase section */
@@ -606,6 +608,7 @@
 			isa = PBXGroup;
 			children = (
 				B41D0FE02475A3A10049D08D /* Info.plist */,
+				FA6B0EA7249443C90062AA59 /* AWSCognitoAuthPluginConfigTests.swift */,
 				B41D0FDE2475A3A10049D08D /* HubEventTests */,
 				B41D0FDC2475A3A10049D08D /* Utils */,
 			);
@@ -1320,6 +1323,7 @@
 			isa = PBXSourcesBuildPhase;
 			buildActionMask = 2147483647;
 			files = (
+				FA6B0EA8249443C90062AA59 /* AWSCognitoAuthPluginConfigTests.swift in Sources */,
 				B41D0FE32475A3A10049D08D /* AuthHubEventHandlerTests.swift in Sources */,
 				B41D0FE22475A3A10049D08D /* AuthUserAttributeKeyTests.swift in Sources */,
 			);

diff --git a/AmplifyPlugins/Auth/AWSCognitoAuthPlugin/AWSCognitoAuthPlugin+Configure.swift b/AmplifyPlugins/Auth/AWSCognitoAuthPlugin/AWSCognitoAuthPlugin+Configure.swift
@@ -17,10 +17,12 @@ extension AWSCognitoAuthPlugin {
     /// - Parameter configuration: The configuration specified for this plugin
     /// - Throws:
     ///   - PluginError.pluginConfigurationError: If one of the configuration values is invalid or empty
-    public func configure(using configuration: Any) throws {
+    public func configure(using configuration: Any?) throws {
         guard let jsonValueConfiguration = configuration as? JSONValue else {
-            throw AuthError.configuration(AuthPluginErrorConstants.decodeConfigurationError.errorDescription,
-                                          AuthPluginErrorConstants.decodeConfigurationError.recoverySuggestion)
+            throw PluginError.pluginConfigurationError(
+                AuthPluginErrorConstants.decodeConfigurationError.errorDescription,
+                AuthPluginErrorConstants.decodeConfigurationError.recoverySuggestion
+            )
         }
         do {
             // Convert the JSONValue to [String: Any] dictionary to be used by AWSMobileClient

diff --git a/AmplifyPlugins/Auth/AWSCognitoAuthPluginTests/AWSCognitoAuthPluginConfigTests.swift b/AmplifyPlugins/Auth/AWSCognitoAuthPluginTests/AWSCognitoAuthPluginConfigTests.swift
@@ -0,0 +1,31 @@
+//
+// Copyright 2018-2020 Amazon.com,
+// Inc. or its affiliates. All Rights Reserved.
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+
+import XCTest
+import Amplify
+import AWSCognitoAuthPlugin
+
+class AWSCognitoAuthPluginConfigTests: XCTestCase {
+
+    func testThrowsOnMissingConfig() throws {
+        let plugin = AWSCognitoAuthPlugin()
+        try Amplify.add(plugin: plugin)
+
+        let categoryConfig = AuthCategoryConfiguration(plugins: ["NonExistentPlugin": true])
+        let amplifyConfig = AmplifyConfiguration(auth: categoryConfig)
+        do {
+            try Amplify.configure(amplifyConfig)
+            XCTFail("Should have thrown a pluginConfigurationError if not supplied with a plugin-specific config.")
+        } catch {
+            guard case PluginError.pluginConfigurationError = error else {
+                XCTFail("Should have thrown a pluginConfigurationError if not supplied with a plugin-specific config.")
+                return
+            }
+        }
+    }
+
+}
diff --git a/AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/AWSDataStorePlugin.swift b/AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/AWSDataStorePlugin.swift
@@ -78,7 +78,7 @@ final public class AWSDataStorePlugin: DataStoreCategoryPlugin {
     /// By the time this method gets called, DataStore will already have invoked
     /// `AmplifyModelRegistration.registerModels`, so we can inspect those models to derive isSyncEnabled, and pass
     /// them to `StorageEngine.setUp(models:)`
-    public func configure(using amplifyConfiguration: Any) throws {
+    public func configure(using amplifyConfiguration: Any?) throws {
         modelRegistration.registerModels(registry: ModelRegistry.self)
         resolveSyncEnabled()
         try resolveStorageEngine(dataStoreConfiguration: dataStoreConfiguration)
-Original file line number
+Diff line change
@@ Expand Up @@
                 XCTFail("Failed to configure analytics plugin")
             }
         }
     }