Skip to content

Conversation

@sassiwalid
Copy link
Owner

This PR adds circular dependency detection to our dependency injection framework. Here are the main changes:

Dependency Graph Architecture:

  1. Added a dependencyGraph that maintains a map of dependencies between services

  2. Key is an ObjectIdentifier of the service type

  3. Value is a Set of ObjectIdentifier representing its dependencies

Cycle Detection:

  1. Implemented a DFS (Depth-First Search) algorithm to detect cycles
  2. Uses two sets: visited for already explored nodes and stack for the current path
  3. If a node already in the stack is encountered, a circular dependency is detected

API Changes:

  • Extended the register method with an optional dependencies parameter

  • Dependencies must be explicitly declared to enable

@sassiwalid sassiwalid force-pushed the feat/dependencyGraph branch from ddc89e7 to 8528129 Compare January 6, 2025 22:19
@sassiwalid sassiwalid requested a review from hmlongco January 11, 2025 15:46
@hmlongco
Copy link
Collaborator

hmlongco commented Jan 11, 2025

As near as I can tell, it looks like you're planning on using arguments to pass the dependencies needed for construction of an object.

What I haven't seen is a basic example that shows, say, your APIService requiring an instance of the NetworkManager to do its job. How is that resolved, since the outside world doesn't want to know that APIService uses NetworkManager under the hood?

Also, since the circular dependency check only appears to check arguments, what happens should APIService just use the @dependency property wrapper to obtain a NetworkManager?

One other note is that you're executing detectCircularDependencies on registration. First issue is that registration happens at startup and that wants to be as fast as possible. Second is what happens if a dependent type hasn't been registered yet?

README example shows:

func registerDependencies(in container: DependencyContainer) {

    await container.register(
    type: APIService.self,
    instance: { APIService() },
    scope: .singleton
    )

    await container.register(
    type: NetworkManager.self,
    instance: { NetworkManager() },
    scope: .transient
    )

  }

}

If APIService had a network manager as a dependency, how can the circular check succeed since NetworkManager hasn't been registered yet? There's an order-of-registration problem.

The circular check needs to happen on resolution, not registration.

@sassiwalid
Copy link
Owner Author

Hello @hmlongco yes I think I should move the circular check on resolve instead of register.

@hmlongco
Copy link
Collaborator

"Also, since the circular dependency check only appears to check arguments, what happens should a service just use the dependency property wrapper?"

class ServiceA {
    @Dependency(assembler) var service: ServiceB
}

class ServiceB {
    @Dependency(assembler) var service: ServiceA
}

Simply checking the args won't do the trick.

@sassiwalid
Copy link
Owner Author

"Also, since the circular dependency check only appears to check arguments, what happens should a service just use the dependency property wrapper?"

class ServiceA {
    @Dependency(assembler) var service: ServiceB
}

class ServiceB {
    @Dependency(assembler) var service: ServiceA
}

Simply checking the args won't do the trick.

Ok thank you Michael I will try to check how I can detect circular dependency even using property wrapper @dependency

@sassiwalid
Copy link
Owner Author

Hello @hmlongco this is a summary of the changes that I did it to check circular dependency injection even with @dependency property wrapper

Summary of Changes:

Core Dependency Resolution:

  • Split resolution into two steps: initial resolution and dependency injection

  • Added tracking of resolved instances to prevent infinite loops

  • Implemented dependency resolution through Injectable protocol

Test Improvements:

  • Added complex circular dependency test case (A → B → D → A)

  • Verified detection with multiple dependency paths

Key Bug Fixes:

  • Fixed DFS to check all dependencies, not just the first one

@sassiwalid
Copy link
Owner Author

Hello @hmlongco maybe the changes that I made didn't correspond that you want me to resolve for dependency wrapper case ?

@sassiwalid sassiwalid merged commit 4108f24 into main Jan 26, 2025
1 check passed
@sassiwalid sassiwalid deleted the feat/dependencyGraph branch January 28, 2025 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants