diff --git a/Package.resolved b/Package.resolved index 29413dc..bc80f7a 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,5 +1,5 @@ { - "originHash" : "80b25f78b0b17934ec8338366df30b99e1b361299a3ebc0c9671bdb9b520b5b6", + "originHash" : "e7be7ae2f468d0465b934f5d628592d6882882d2b0d232d6fa1cce6407427a36", "pins" : [ { "identity" : "swift-argument-parser", @@ -22,10 +22,10 @@ { "identity" : "swinject", "kind" : "remoteSourceControl", - "location" : "https://github.com/Swinject/Swinject.git", + "location" : "https://github.com/bradfol/Swinject.git", "state" : { - "revision" : "be9dbcc7b86811bc131539a20c6f9c2d3e56919f", - "version" : "2.9.1" + "branch" : "service-entry", + "revision" : "c4441192155a5e9a8206117132d708b1362dda3b" } }, { diff --git a/Package.swift b/Package.swift index bc3e7f9..aebdd83 100644 --- a/Package.swift +++ b/Package.swift @@ -15,7 +15,7 @@ let package = Package( .executable(name: "knit-cli", targets: ["KnitCommand"]), ], dependencies: [ - .package(url: "https://github.com/Swinject/Swinject.git", from: "2.9.1"), + .package(url: "https://github.com/bradfol/Swinject.git", branch: "service-entry"), .package(url: "https://github.com/Swinject/SwinjectAutoregistration.git", from: "2.9.1"), .package(url: "https://github.com/apple/swift-syntax.git", from: "510.0.2"), .package(url: "https://github.com/apple/swift-argument-parser", from: "1.4.0"), diff --git a/README.md b/README.md index 55b4e59..42a1787 100644 --- a/README.md +++ b/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` +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. --- diff --git a/Sources/Knit/DuplicateRegistrationDetector.swift b/Sources/Knit/DuplicateRegistrationDetector.swift new file mode 100644 index 0000000..1879122 --- /dev/null +++ b/Sources/Knit/DuplicateRegistrationDetector.swift @@ -0,0 +1,117 @@ +// +// 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? + } + + var existingRegistrations = Set() + + public init( + duplicateWasDetected: ((Key) -> Void)? = nil + ) { + self.duplicateWasDetected = duplicateWasDetected + } + +} + +// MARK: - + +extension DuplicateRegistrationDetector: Behavior { + + public func container( + _ container: Container, + didRegisterType type: Type.Type, + toService entry: ServiceEntry, + 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) + """ + } + +} diff --git a/Sources/Knit/Module/ModuleAssembler.swift b/Sources/Knit/Module/ModuleAssembler.swift index 6266036..31e28a7 100644 --- a/Sources/Knit/Module/ModuleAssembler.swift +++ b/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() diff --git a/Sources/Knit/Module/ScopedModuleAssembler.swift b/Sources/Knit/Module/ScopedModuleAssembler.swift index 4044048..dbf8dfd 100644 --- a/Sources/Knit/Module/ScopedModuleAssembler.swift +++ b/Sources/Knit/Module/ScopedModuleAssembler.swift @@ -25,6 +25,7 @@ public final class ScopedModuleAssembler { _ 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 { parent: parent, _modules: modules, overrideBehavior: overrideBehavior, + behaviors: behaviors, postAssemble: postAssemble ) } catch { @@ -53,6 +55,7 @@ public final class ScopedModuleAssembler { _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 { } }, errorFormatter: errorFormatter, + behaviors: behaviors, postAssemble: postAssemble ) } diff --git a/Tests/KnitTests/DuplicateRegistrationDetectorTests.swift b/Tests/KnitTests/DuplicateRegistrationDetectorTests.swift new file mode 100644 index 0000000..5c14b4a --- /dev/null +++ b/Tests/KnitTests/DuplicateRegistrationDetectorTests.swift @@ -0,0 +1,187 @@ +// +// Copyright © Block, Inc. All rights reserved. +// + +@testable import Knit +import XCTest + +final class DuplicateRegistrationDetectorTests: XCTestCase { + + func testBasicDetection() throws { + var reportedDuplicates = [DuplicateRegistrationDetector.Key]() + let duplicateRegistrationDetector = DuplicateRegistrationDetector(duplicateWasDetected: { key in + reportedDuplicates.append(key) + }) + let container = Container( + behaviors: [duplicateRegistrationDetector] + ) + + XCTAssertEqual(reportedDuplicates.count, 0) + container.register(String.self, factory: { _ in "one" }) + XCTAssertEqual(reportedDuplicates.count, 0) + + container.register(String.self, factory: { _ in "two" }) + XCTAssertEqual(reportedDuplicates.count, 1) + let firstReport = try XCTUnwrap(reportedDuplicates.first) + XCTAssert(firstReport.serviceType == String.self) + XCTAssert(firstReport.argumentsType == (Resolver).self) + XCTAssertEqual(firstReport.name, nil) + + container.register(String.self, factory: { _ in "three" }) + XCTAssertEqual(reportedDuplicates.count, 2) + } + + func testNames() throws { + var reportedDuplicates = [DuplicateRegistrationDetector.Key]() + let duplicateRegistrationDetector = DuplicateRegistrationDetector(duplicateWasDetected: { key in + reportedDuplicates.append(key) + }) + let container = Container( + behaviors: [duplicateRegistrationDetector] + ) + + XCTAssertEqual(reportedDuplicates.count, 0) + container.register(String.self, factory: { _ in "no name" }) + container.register(String.self, name: "nameOne", factory: { _ in "one" }) + container.register(String.self, name: "nameTwo", factory: { _ in "two" }) + XCTAssertEqual(reportedDuplicates.count, 0) + + container.register(String.self, name: "nameOne", factory: { _ in "one duplicate" }) + XCTAssertEqual(reportedDuplicates.count, 1) + let firstReport = try XCTUnwrap(reportedDuplicates.first) + XCTAssert(firstReport.serviceType == String.self) + XCTAssert(firstReport.argumentsType == (Resolver).self) + XCTAssertEqual(firstReport.name, "nameOne") + } + + func testArguments() throws { + var reportedDuplicates = [DuplicateRegistrationDetector.Key]() + let duplicateRegistrationDetector = DuplicateRegistrationDetector(duplicateWasDetected: { key in + reportedDuplicates.append(key) + }) + let container = Container( + behaviors: [duplicateRegistrationDetector] + ) + + XCTAssertEqual(reportedDuplicates.count, 0) + container.register(String.self, factory: { _ in "no arguments" }) + container.register(String.self, factory: { (_, _: Bool) in "Bool arg" }) + container.register(String.self, factory: { (_, _: Int) in "Int arg" }) + XCTAssertEqual(reportedDuplicates.count, 0) + + container.register(String.self, factory: { (_, _: Int) in "Int arg duplicate" }) + XCTAssertEqual(reportedDuplicates.count, 1) + let firstReport = try XCTUnwrap(reportedDuplicates.first) + XCTAssert(firstReport.serviceType == String.self) + XCTAssert(firstReport.argumentsType == (Resolver, Int).self) + XCTAssertEqual(firstReport.name, nil) + } + + func testNoDuplicates() throws { + var reportedDuplicates = [DuplicateRegistrationDetector.Key]() + let duplicateRegistrationDetector = DuplicateRegistrationDetector(duplicateWasDetected: { key in + reportedDuplicates.append(key) + }) + let container = Container( + behaviors: [duplicateRegistrationDetector] + ) + + container.register(String.self, factory: { _ in "" }) + container.register(Bool.self, factory: { _ in true }) + container.register(Int.self, factory: { _ in 1 }) + XCTAssertEqual(reportedDuplicates.count, 0) + } + + func testParentContainerNotDuplicate() throws { + // A parent container is allowed to contain the same registration key as a child container + // This is not a duplicate but a "shadow" registration + + var reportedDuplicates = [DuplicateRegistrationDetector.Key]() + + let parentDuplicateRegistrationDetector = DuplicateRegistrationDetector(duplicateWasDetected: { key in + reportedDuplicates.append(key) + }) + let parentContainer = Container(behaviors: [parentDuplicateRegistrationDetector]) + + let childDuplicateRegistrationDetector = DuplicateRegistrationDetector(duplicateWasDetected: { key in + reportedDuplicates.append(key) + }) + let childContainer = Container(parent: parentContainer, behaviors: [childDuplicateRegistrationDetector]) + + parentContainer.register(String.self, factory: { _ in "parent" }) + childContainer.register(String.self, factory: { _ in "child" }) + + XCTAssertEqual(reportedDuplicates.count, 0) + } + + func testTypeForwarding() throws { + // A forwarded type (`.implements()`) should not cause a duplicate registration + let duplicateRegistrationDetector = DuplicateRegistrationDetector() + let container = Container(behaviors: [duplicateRegistrationDetector]) + + XCTAssertEqual(duplicateRegistrationDetector.detectedKeys.count, 0) + container.register(String.self, factory: { _ in "string"} ) + .implements((any StringProtocol).self) + XCTAssertEqual(duplicateRegistrationDetector.detectedKeys.count, 0) + + // Registering `Substring` does not cause a duplicate + let substringEntry = container.register(Substring.self, factory: { _ in "substring"} ) + XCTAssertEqual(duplicateRegistrationDetector.detectedKeys.count, 0) + // However forwarding to the same type twice still results in a duplicate + substringEntry.implements((any StringProtocol).self) + XCTAssertEqual(duplicateRegistrationDetector.detectedKeys.count, 1) + } + + func testCustomStringDescription() throws { + assertCustomStringDescription(key: DuplicateRegistrationDetector.Key( + serviceType: String.self, + argumentsType: ((Resolver)).self, + name: nil + ), expectedDescription: + """ + Duplicate Registration Key + Service type: String + Arguments type: Resolver + Name: `nil` + """ + ) + + assertCustomStringDescription(key: DuplicateRegistrationDetector.Key( + serviceType: Int.self, + argumentsType: (Resolver, Bool).self, + name: nil + ), expectedDescription: + """ + Duplicate Registration Key + Service type: Int + Arguments type: (Resolver, Bool) + Name: `nil` + """ + ) + + assertCustomStringDescription(key: DuplicateRegistrationDetector.Key( + serviceType: String.self, + argumentsType: ((Resolver)).self, + name: "namedRegistration" + ), expectedDescription: + """ + Duplicate Registration Key + Service type: String + Arguments type: Resolver + Name: namedRegistration + """ + ) + } + +} + +// MARK: - + +private func assertCustomStringDescription( + key: DuplicateRegistrationDetector.Key, + expectedDescription: String, + file: StaticString = #filePath, + line: UInt = #line +) { + XCTAssertEqual("\(key)", expectedDescription, file: file, line: line) +} diff --git a/Tests/KnitTests/ScopedModuleAssemblerTests.swift b/Tests/KnitTests/ScopedModuleAssemblerTests.swift index 58a8cf8..051e966 100644 --- a/Tests/KnitTests/ScopedModuleAssemblerTests.swift +++ b/Tests/KnitTests/ScopedModuleAssemblerTests.swift @@ -4,6 +4,7 @@ import Foundation @testable import Knit +@testable import Swinject import XCTest final class ScopedModuleAssemblerTests: XCTestCase { @@ -76,6 +77,30 @@ final class ScopedModuleAssemblerTests: XCTestCase { ) } + @MainActor + func test_integration_initializerBehaviorsPassedToInternalContainer() throws { + // This is a bit of an integration test to ensure that behaviors passed to the ScopedModuleAssembler initializer + // are passed through correctly to the backing Container instance. + + let testBehavior = TestBehavior() + let scopedModuleAssembler = ScopedModuleAssembler( + [], + behaviors: [testBehavior] + ) + let container = scopedModuleAssembler._container + // ModuleAssembler automatically adds behaviors for ServiceCollector and AbstractRegistrationContainer + // so first filter those out + let foundBehaviors = container.behaviors.filter { behavior in + let behaviorType = type(of: behavior) + return behaviorType != ServiceCollector.self && + behaviorType != Container.AbstractRegistrationContainer.self + } + // There should only be one behavior left + XCTAssertEqual(foundBehaviors.count, 1) + let containerBehavior = try XCTUnwrap(foundBehaviors.first as? AnyObject) + XCTAssert(containerBehavior === testBehavior) + } + } private struct Assembly1: AutoInitModuleAssembly { @@ -96,3 +121,16 @@ private struct Assembly3: AutoInitModuleAssembly { static var dependencies: [any ModuleAssembly.Type] { [Assembly1.self] } func assemble(container: Container) { } } + +private final class TestBehavior: Behavior { + + func container( + _ container: Container, + didRegisterType type: Type.Type, + toService entry: Swinject.ServiceEntry, + withName name: String? + ) { + // No op + } + +}