fix: preventing false positives when validating circular references. …

[jescobar]

Thanks for the great job with this awesome DI crate for rust, I tested several DI crates and in my opinion this is the best.

While using the more-rs-di crate I found a false positive error when validating circular dependencies, I checked the issue and I sent a possible solution, it passes the unittests including the one for detecting circular dependencies, in the following text it is an small explanation, but if you have any question I can provide more details and explanation. Thanks a lot!

…This change allows that child dependencies rely on the same dependencies without being a circular reference. example: A, B, C and Z depend on M; A depends on B; Z depends on A and C. This is valid because there are no circular references, however, before this commit this example was interpreted as a circular reference.

[commonsensesoftware]

Thank you for the kind words. I appreciate taking the time track down the issue, report it, and propose a fix.

The visualization of your graph appears to be:

Z --→ A --→ B
|     │     |
|     ↓     |
├---→ M ←---┘
|     ↑
|     |
└---→ C

That looks correct to me.

Can we add a test case to cover this scenario? The test is the documentation and it would prevent a potential future regression.

This might be the case where the test setup needs to be flipped. That would probably include:

1. Temporarily revert this one line change
2. Add a test case (ex: dependency_visited_multiple_times_should_not_be_circular or something like that) which will fail
3. Reapply your change
4. The test should now pass

[commonsensesoftware]

I took the change since it was one line, but I can't repro the false positive. I've added the following setup:

struct A;
struct B;
struct C;
struct M;
struct Z;

let mut services = ServiceCollection::new();

services
    .add(transient_as_self::<M>().from(|_| ServiceRef::new(M {})))
    .add(
        transient_as_self::<A>()
            .depends_on(exactly_one::<B>())
            .depends_on(exactly_one::<M>())
            .from(|_| ServiceRef::new(A {})),
    )
    .add(
        transient_as_self::<B>()
            .depends_on(exactly_one::<M>())
            .from(|_| ServiceRef::new(B {})),
    )
    .add(
        transient_as_self::<C>()
            .depends_on(exactly_one::<M>())
            .from(|_| ServiceRef::new(C {})),
    )
    .add(
        transient_as_self::<Z>()
            .depends_on(exactly_one::<A>())
            .depends_on(exactly_one::<C>())
            .depends_on(exactly_one::<M>())
            .from(|_| ServiceRef::new(Z {})),
    );

let result = validate(&services);

result.is_ok() is true with or without the change. Do you have a setup that repros the condition?

[commonsensesoftware]

As I'm looking at this - again, I'm not sure visited is even necessary for checking circular references. It doesn't actually do anything in this context. The process simply flattens out all of the dependencies and if a dependency is found that matches the type that we started at, it is circular. This simplifies things quite a bit.

[jescobar]

Hi @commonsensesoftware , please excuse me for the late reply, I had a busy week, but, regarding this I have prepared the unittest, however, I found that in a complex scenario it could allow more circular references, so I'm working right now in a complete fix, I was thinking in using a graph because it should be easier to detect the circular references using it, I have already the Unittests but one it failing, so that's why I have not send any change yet because I'm working on this in my free time, but I hope to send a new PR on monday

[jescobar]

@commonsensesoftware I will add the unit test in a new commit but I just added a partial code here if you want to test verifications.rs

`#[test] fn dependency_visited_multiple_times_should_not_be_circular() { let mut services = ServiceCollection::new();

    services.add(
        singleton::<dyn ServiceM, ServiceMImpl>().from(|_sp| ServiceRef::new(ServiceMImpl)),
    );
    services.add(
        singleton::<dyn ServiceB, ServiceBImpl>()
            .depends_on(exactly_one::<dyn ServiceM>())
            .from(|sp| ServiceRef::new(ServiceBImpl::new(sp.get_required::<dyn ServiceM>()))),
    );
    services.add(
        singleton::<dyn ServiceC, ServiceCImpl>()
            .depends_on(exactly_one::<dyn ServiceM>())
            .from(|sp| ServiceRef::new(ServiceCImpl::new(sp.get_required::<dyn ServiceM>()))),
    );
    services.add(
        singleton::<dyn ServiceA, ServiceAImpl>()
            .depends_on(exactly_one::<dyn ServiceM>())
            .depends_on(exactly_one::<dyn ServiceB>())
            .from(|sp| {
                ServiceRef::new(ServiceAImpl::new(
                    sp.get_required::<dyn ServiceM>(),
                    sp.get_required::<dyn ServiceB>(),
                ))
            }),
    );
    services.add(
        singleton::<dyn ServiceY, ServiceYImpl>()
            .depends_on(exactly_one::<dyn ServiceM>())
            .depends_on(exactly_one::<dyn ServiceC>())
            .from(|sp| {
                ServiceRef::new(ServiceYImpl::new(
                    sp.get_required::<dyn ServiceM>(),
                    sp.get_required::<dyn ServiceC>(),
                ))
            }),
    );
    services.add(
        singleton::<dyn ServiceX, ServiceXImpl>()
            .depends_on(exactly_one::<dyn ServiceM>())
            .depends_on(exactly_one::<dyn ServiceY>())
            .from(|sp| {
                ServiceRef::new(ServiceXImpl::new(
                    sp.get_required::<dyn ServiceM>(),
                    sp.get_required::<dyn ServiceY>(),
                ))
            }),
    );
    services.add(
        singleton::<dyn ServiceZ, ServiceZImpl>()
            .depends_on(exactly_one::<dyn ServiceM>())
            .depends_on(exactly_one::<dyn ServiceA>())
            .depends_on(exactly_one::<dyn ServiceX>())
            .from(|sp| {
                ServiceRef::new(ServiceZImpl::new(
                    sp.get_required::<dyn ServiceM>(),
                    sp.get_required::<dyn ServiceA>(),
                    sp.get_required::<dyn ServiceX>(),
                ))
            }),
    );

    // act
    let result = validate(&services);
    assert_eq!(result.is_err(), false) //I will add the proper assert here;
}

#[test]
fn validate_should_report_circular_dependency_on_complex_dependency_tree() {
    let mut services = ServiceCollection::new();

    services.add(
        singleton::<dyn ServiceM, ServiceMImpl>().from(|_sp| ServiceRef::new(ServiceMImpl)),
    );
    services.add(
        singleton::<dyn ServiceB, ServiceBImpl>()
            .depends_on(exactly_one::<dyn ServiceM>())
            .from(|sp| ServiceRef::new(ServiceBImpl::new(sp.get_required::<dyn ServiceM>()))),
    );
    services.add(
        singleton::<dyn ServiceC, ServiceCWithCircleRefToXImpl>()
            .depends_on(exactly_one::<dyn ServiceM>())
            .depends_on(exactly_one::<dyn ServiceX>())
            .from(|sp| {
                ServiceRef::new(ServiceCWithCircleRefToXImpl::new(
                    sp.get_required::<dyn ServiceM>(),
                    sp.get_required::<dyn ServiceX>(),
                ))
            }),
    );
    services.add(
        singleton::<dyn ServiceA, ServiceAImpl>()
            .depends_on(exactly_one::<dyn ServiceM>())
            .depends_on(exactly_one::<dyn ServiceB>())
            .from(|sp| {
                ServiceRef::new(ServiceAImpl::new(
                    sp.get_required::<dyn ServiceM>(),
                    sp.get_required::<dyn ServiceB>(),
                ))
            }),
    );
    services.add(
        singleton::<dyn ServiceY, ServiceYImpl>()
            .depends_on(exactly_one::<dyn ServiceM>())
            .depends_on(exactly_one::<dyn ServiceC>())
            .from(|sp| {
                ServiceRef::new(ServiceYImpl::new(
                    sp.get_required::<dyn ServiceM>(),
                    sp.get_required::<dyn ServiceC>(),
                ))
            }),
    );
    services.add(
        singleton::<dyn ServiceX, ServiceXImpl>()
            .depends_on(exactly_one::<dyn ServiceM>())
            .depends_on(exactly_one::<dyn ServiceY>())
            .from(|sp| {
                ServiceRef::new(ServiceXImpl::new(
                    sp.get_required::<dyn ServiceM>(),
                    sp.get_required::<dyn ServiceY>(),
                ))
            }),
    );
    services.add(
        singleton::<dyn ServiceZ, ServiceZImpl>()
            .depends_on(exactly_one::<dyn ServiceM>())
            .depends_on(exactly_one::<dyn ServiceA>())
            .depends_on(exactly_one::<dyn ServiceX>())
            .from(|sp| {
                ServiceRef::new(ServiceZImpl::new(
                    sp.get_required::<dyn ServiceM>(),
                    sp.get_required::<dyn ServiceA>(),
                    sp.get_required::<dyn ServiceX>(),
                ))
            }),
    );

    // act
    let result = validate(&services);
    assert_eq!(result.is_err(), true) // I will add the proper assert here;
}`

[jescobar]

in test.rs `pub(crate) trait ServiceA {}

pub(crate) trait ServiceB {}

pub(crate) trait ServiceC {}

pub(crate) trait ServiceM {}

pub(crate) trait ServiceY {}

pub(crate) trait ServiceX {}

pub(crate) trait ServiceZ {}

pub(crate) struct ServiceAImpl { _service_m: ServiceRef, _service_b: ServiceRef, }

impl ServiceAImpl { pub(crate) fn new( _service_m: ServiceRef, _service_b: ServiceRef, ) -> Self { Self { _service_m, _service_b, } } }

impl ServiceA for ServiceAImpl {}

pub(crate) struct ServiceBImpl { _service_m: ServiceRef, }

impl ServiceBImpl { pub(crate) fn new(_service_m: ServiceRef) -> Self { Self { _service_m } } }

impl ServiceB for ServiceBImpl {}

pub(crate) struct ServiceCImpl { _service_m: ServiceRef, }

impl ServiceCImpl { pub(crate) fn new(_service_m: ServiceRef) -> Self { Self { _service_m } } } impl ServiceC for ServiceCImpl {}

pub(crate) struct ServiceCWithCircleRefToXImpl { _service_m: ServiceRef, _service_x: ServiceRef, }

impl ServiceCWithCircleRefToXImpl { pub(crate) fn new(_service_m: ServiceRef, _service_x: ServiceRef) -> Self { Self { _service_m, _service_x } } }

impl ServiceC for ServiceCWithCircleRefToXImpl {}

pub(crate) struct ServiceMImpl;

impl ServiceM for ServiceMImpl {}

pub(crate) struct ServiceYImpl { _service_m: ServiceRef, _service_c: ServiceRef, }

impl ServiceYImpl { pub(crate) fn new( _service_m: ServiceRef, _service_c: ServiceRef, ) -> Self { Self { _service_m, _service_c, } } }

impl ServiceY for ServiceYImpl {}

pub(crate) struct ServiceXImpl { _service_m: ServiceRef, _service_y: ServiceRef, }

impl ServiceX for ServiceXImpl {}

impl ServiceXImpl { pub(crate) fn new( _service_m: ServiceRef, _service_y: ServiceRef, ) -> Self { Self { _service_m, _service_y, } } }

pub(crate) struct ServiceZImpl { _service_m: ServiceRef, _service_a: ServiceRef, _service_x: ServiceRef, }

impl ServiceZImpl { pub(crate) fn new( _service_m: ServiceRef, _service_a: ServiceRef, _service_x: ServiceRef, ) -> Self { Self { _service_m, _service_a, _service_x, } } }

impl ServiceZ for ServiceZImpl {}`

[commonsensesoftware]

@jescobar Thanks for taking the time. No rush. I realize we're all busy, myself included. The peculiar thing is that visited is added to, but there are never any tests against it for circular dependencies whatsoever. I think that is correct and I just forgot to remove it. At an earlier point in the implementation, I thought it was needed, but it's not. It is needed elsewhere, but not for circular dependencies. The only thing the code did was clear the collection and then start adding to it. It was purely wasted cycles. That would explain why there no difference. I've started #8 which completely removes the use of visit and the results are the same.

What I'm most curious about is that you found or had some case of a circular reference with a false positive. I want to reproduce that and work backward. Right now, the improvement is "remove unnecessary code", but if there is something more, I want to capture and address it.