Merge pull request #213 from cashapp/bradfol/duplicate-behavior

Implement DuplicateRegistrationDetector for whole graph Containers

README.md

@@ -63,7 +63,48 @@ The following commands are available:
 
 #### Default Setting
 
-By default Knit generates named getters for its type safety. 
+By default Knit generates named getters for its type safety.
+
+## Duplicate Registration Detection
+
+As an app scales, so do the number of DI registrations. It is important to detect duplicate registrations because by
+default Swinject will overwrite early registrations with any duplicates.
+While this capability is useful in test environments, at runtime it almost always indicates a programming/configuration
+error, which could lead to bugs.
+
+Knit includes two capabilities to detect duplicate registrations for additional DI graph safety:
+
+1. Compile-time detection within a single module.
+1. Runtime detection across an entire container's graph.
+
+### Compile-time Detection
+
+When Knit parses the assembly files within a single module it will automatically detect any duplicates
+found during parsing and immediately report an error with information about the duplicate.
+This is always on and automatic.
+
+### Runtime Detection
+
+Knit only parses the assemblies in a single module at a time so it is not able to detect duplicates across modules
+during parsing/compile time. 
+Instead Knit provides a `DuplicateRegistrationDetector` class, which is a Swinject Behavior.
+`DuplicateRegistrationDetector` allows for runtime detection of all registrations made to a single Container (DI graph),
+regardless of which modules those registrations came from. 
+This adds safety from duplicate registrations to your whole DI graph.
+
+An instance of DuplicateRegistrationDetector should be added to the Container to make use of this feature. Configuration steps:
+
+1. Create an instance of `DuplicateRegistrationDetector`.
+1. Provide that instance to your Container (ScopedModuleAssembler/ModuleAssembler/Assembler also allow behaviors to be
+    passed into their initializers).
+1. Perform the registration phase of your Container setup. If you are using ScopedModuleAssembler/ModuleAssembler then the registration phase will be complete after the initialer returns.
+1. Check the `detectedKeys` property of your `DuplicateRegistrationDetector` instance for duplicates.
+
+Note that there are also helpers on `DuplicateRegistrationDetector.Key` and `Array<DuplicateRegistrationDetector.Key>`
+to help with creating reports/error messages that are easier to read.
+
+`DuplicateRegistrationDetector` also provides a `duplicateWasDetected` closure hook if you would like to be informed of each
+duplicate at the moment that duplicate is registered.
 
 ---
 

Sources/Knit/DuplicateRegistrationDetector.swift

@@ -0,0 +1,119 @@
+//
+// Copyright © Block, Inc. All rights reserved.
+//
+
+import Swinject
+
+public final class DuplicateRegistrationDetector {
+
+    /// If a duplicate registration is detected, the `Key` describing that registration will be provided to this closure.
+    /// The closure can be called multiple times, once for each duplicate found.
+    public var duplicateWasDetected: ((Key) -> Void)?
+
+    /// An array of all the keys for duplicate registrations detected by this Behavior.
+    /// The array is updated before the `duplicateWasDetected` closure is invoked so will always contain all detected duplicates up to that point.
+    public private(set) var detectedKeys = [Key]()
+
+    /// Describes a single registration key.
+    /// If a duplicate is detected this key will be provided to the `duplicateWasDetected` closure.
+    public struct Key {
+        public let serviceType: Any.Type
+        public let argumentsType: Any.Type
+        public let name: String?
+    }
+
+    /// Set of all registration Keys that have been detected so far.
+    /// Useful to check that the count of this set is non-zero to confirm the Detector was set up correctly.
+    public private(set) var existingRegistrations = Set<Key>()
+
+    public init(
+        duplicateWasDetected: ((Key) -> Void)? = nil
+    ) {
+        self.duplicateWasDetected = duplicateWasDetected
+    }
+
+}
+
+// MARK: -
+
+extension DuplicateRegistrationDetector: Behavior {
+
+    public func container<Type, Service>(
+        _ container: Container,
+        didRegisterType type: Type.Type,
+        toService entry: ServiceEntry<Service>,
+        withName name: String?
+    ) {
+        // Make sure to use `didRegisterType type: Type.Type` rather than `Service`
+        let key = Key(
+            serviceType: type,
+            argumentsType: entry.argumentsType,
+            name: name
+        )
+
+        let preInsertCount = existingRegistrations.count
+        existingRegistrations.insert(key)
+
+        if preInsertCount == existingRegistrations.count {
+            // The registration count did not increment, so the current service entry was a duplicate of an existing entry
+            detectedKeys.append(key)
+            duplicateWasDetected?(key)
+        }
+    }
+
+}
+
+// MARK: -
+
+extension DuplicateRegistrationDetector.Key: Hashable, Equatable {
+
+    public func hash(into hasher: inout Hasher) {
+        ObjectIdentifier(serviceType).hash(into: &hasher)
+        ObjectIdentifier(argumentsType).hash(into: &hasher)
+        name?.hash(into: &hasher)
+    }
+
+    public static func == (lhs: Self, rhs: Self) -> Bool {
+        return lhs.serviceType == rhs.serviceType
+            && lhs.argumentsType == rhs.argumentsType
+            && lhs.name == rhs.name
+    }
+
+}
+
+// MARK: -
+
+extension DuplicateRegistrationDetector.Key: CustomStringConvertible {
+
+    // Provide a more structured string description of the key, useful for logging error messages
+    public var description: String {
+        """
+        Duplicate Registration Key
+        Service type: \(serviceType)
+        Arguments type: \(argumentsType)
+        Name: \(name ?? "`nil`")
+        """
+    }
+
+
+}
+
+// MARK: -
+
+extension Array where Element == DuplicateRegistrationDetector.Key {
+
+    public var duplicatesDescription: String {
+        guard count > 0 else {
+            return "No duplicates in array"
+        }
+
+        let keyDescriptions = reduce("") { partialResult, key in
+            partialResult + "\(key)\n\n"
+        }
+        return """
+            Duplicate registrations were detected (count: \(count):
+            \(keyDescriptions)
+            """
+    }
+
+}

Sources/Knit/Module/ModuleAssembler.swift

@@ -72,6 +72,7 @@ public final class ModuleAssembler {
         overrideBehavior: OverrideBehavior = .defaultOverridesWhenTesting,
         assemblyValidation: ((any ModuleAssembly.Type) throws -> Void)? = nil,
         errorFormatter: ModuleAssemblerErrorFormatter = DefaultModuleAssemblerErrorFormatter(),
+        behaviors: [Behavior] = [],
         postAssemble: ((Container) -> Void)? = nil
     ) throws {
         self.builder = try DependencyBuilder(
@@ -84,7 +85,10 @@ public final class ModuleAssembler {
         )
 
         self.parent = parent
-        self._container = Container(parent: parent?._container)
+        self._container = Container(
+            parent: parent?._container,
+            behaviors: behaviors
+        )
         self.serviceCollector = .init(parent: parent?.serviceCollector)
         self._container.addBehavior(serviceCollector)
         let abstractRegistrations = self._container.registerAbstractContainer()

Sources/Knit/Module/ScopedModuleAssembler.swift

@@ -25,6 +25,7 @@ public final class ScopedModuleAssembler<ScopedResolver> {
         _ modules: [any ModuleAssembly],
         overrideBehavior: OverrideBehavior = .defaultOverridesWhenTesting,
         errorFormatter: ModuleAssemblerErrorFormatter = DefaultModuleAssemblerErrorFormatter(),
+        behaviors: [Behavior] = [],
         postAssemble: ((Container) -> Void)? = nil,
         file: StaticString = #fileID,
         line: UInt = #line
@@ -34,6 +35,7 @@ public final class ScopedModuleAssembler<ScopedResolver> {
                 parent: parent,
                 _modules: modules,
                 overrideBehavior: overrideBehavior,
+                behaviors: behaviors,
                 postAssemble: postAssemble
             )
         } catch {
@@ -53,6 +55,7 @@ public final class ScopedModuleAssembler<ScopedResolver> {
         _modules modules: [any ModuleAssembly],
         overrideBehavior: OverrideBehavior = .defaultOverridesWhenTesting,
         errorFormatter: ModuleAssemblerErrorFormatter = DefaultModuleAssemblerErrorFormatter(),
+        behaviors: [Behavior] = [],
         postAssemble: ((Container) -> Void)? = nil
     ) throws {
         // For provided modules, fail early if they are scoped incorrectly
@@ -80,6 +83,7 @@ public final class ScopedModuleAssembler<ScopedResolver> {
                 }
             },
             errorFormatter: errorFormatter,
+            behaviors: behaviors,
             postAssemble: postAssemble
         )
     }

Sources/Swinject/ServiceEntry.swift

@@ -18,8 +18,8 @@ internal protocol ServiceEntryProtocol: AnyObject {
 /// As a returned instance from ``Container/register(_:name:factory:)-8gy9r``, some configurations can be added.
 public final class ServiceEntry<Service>: ServiceEntryProtocol {
     fileprivate var initCompletedActions: [(Resolver, Service) -> Void] = []
-    internal let serviceType: Any.Type
-    internal let argumentsType: Any.Type
+    public let serviceType: Any.Type
+    public let argumentsType: Any.Type
 
     internal let factory: FunctionType
     internal weak var container: Container?