Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Object graph visualization using VisualizeObjectGraph produces incomplete result for IEnumerable<> dependecies #759

Open
pavel-pargachev opened this issue Oct 5, 2019 · 5 comments
Labels
bug
Milestone

Comments

@pavel-pargachev
Copy link

@pavel-pargachev pavel-pargachev commented Oct 5, 2019

VisualizeObjectGraph does not include component types that are part of IEnumerable<> dependency.

How To Reproduce

Consider the following registrations:

var container = new Container();

container.Collection.Register(typeof(IValidator<>), typeof(IValidator<>).Assembly);
container.Register(typeof(IValidator<>), typeof(CompositeValidator<>));
container.Register<MyService>();

container.Verify();

var actual = container.GetRegistration(typeof(MyService)).VisualizeObjectGraph();

Console.WriteLine(actual);

Using types:

class MyService { public MyService(IValidator<MyModel> myModelValidator) { } }

interface IValidator<T> { }

class Composite<T> : IValidator<T> { public Composite(IEnumerable<IValidator<T>> v) { } }

class Validator1 : IValidator<MyModel> { public Validator1(Dependency d) { } }
class Validator2 : IValidator<MyModel> { public Validator2(Dependency d) { } }

class Dependency { }

class MyModel { }

Prints to the console

MyService( // Transient
    Composite<MyModel>( // Transient
        IEnumerable<IValidator<MyModel>>( // Singleton
            Dependency(), // Transient
            Dependency()))) // Transient

Expected to print

MyService( // Transient
    Composite<MyModel>( // Transient
        IEnumerable<IValidator<MyModel>>( // Singleton
            Validator1( // Transient
                Dependency()), // Transient
            Validator2( // Transient
                Dependency())))) // Transient

Additional context

  • SimpleInjector 4.7.1
  • .NET Core 3.0 console app
@dotnetjunkie dotnetjunkie added bug task and removed question labels Oct 5, 2019
@dotnetjunkie dotnetjunkie added this to the v4.8 milestone Oct 5, 2019
@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

@dotnetjunkie dotnetjunkie commented Oct 5, 2019

Thank you for sending this high-quality repro. I can confirm that this is a bug. I unfortunately can't give you a workaround for this issue, so I hope this isn't a blocking issue for you.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

@dotnetjunkie dotnetjunkie commented Oct 5, 2019

bug-759 branch created.

@dotnetjunkie dotnetjunkie modified the milestones: v4.8, v4.9 Nov 17, 2019
@dotnetjunkie dotnetjunkie removed the task label Dec 5, 2019
@dotnetjunkie dotnetjunkie modified the milestones: v4.9, v4.10 Jan 5, 2020
@kwlin

This comment has been minimized.

Copy link
Contributor

@kwlin kwlin commented Jan 9, 2020

I think the issue is in this code:

var dependencies = producer
.GetRelationships()
.Select(relationship => relationship.Dependency)
.ToList();

When producer is of a ServiceType IEnumerable<> it returns all dependencies within the IEnumerable<> type, instead it should iterate through the IEnumerable<> to get the producer of an item.

@dotnetjunkie: Should I check if the ServiceType is an IEnumerable<>, then iterate through the Relationships; Per Relationship find/get the InstanceProducer? Or is there another way to get the InstanceProducers within an IEnumerable<> ServiceType?

@pavel-pargachev: Thanks for the great reproduction steps/unittests.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

@dotnetjunkie dotnetjunkie commented Jan 10, 2020

@kwlin, checking for just IEnumerable<T> is probably not enough, because there is a range of collection types that is supported.

I'm also wondering whether this code is the correct place to fix this. Perhaps the problem lies more deeply inside the InstanceProducer or its Registration instance. This bug might possibly also have effect on other parts of the system, for instance when users analyzes the relationships of a registration themselves, they would also see this same (incorrectly) flattened graph.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

@dotnetjunkie dotnetjunkie commented Jan 12, 2020

I've been digging a bit deeper, and I think the core of the problem lies here:

KnownRelationship[] IContainerControlledCollection.GetRelationships() => (
from producer in this.producers.Select(p => p.Value)
from relationship in producer.GetRelationships()
select relationship)
.Distinct()
.ToArray();

As @kwlin's code snippet shows, InstanceProducer instances can return a list of KnownRelationship instances through their GetRelationship() method. KnownRelationship describes the relationship and is used to do verification and visualizing the object graph.

A KnownRelationship describes the relationship between the component and its dependencies. In the case of the MyService component, for instance, it contains one KnownRelationship and its points back to MyService as consumer and IValidator<MyModel> as dependency.

This model breaks in case of working with collections, where Simple Injector's internal ContainerControlledCollection<T> returns KnownRelationship instances where the consumer is either Validator1 or Validator2 and the dependency the validators' dependencies.

This is clearly wrong, because the relationship is between the collection and the validators. So the ContainerControlledCollection<T> is removing itself from the equation. This leads to scewed results.

Okay, so now we know what the problem is. But fixing it proves to be a bit harder than I initially thought, because:

  • A KnownRelationship wraps an InjectionConsumerInfo. This gives information about the consuming end of the relationship. But an InjectionConsumerInfo is described using either a (constructor) ParameterInfo or a PropertyInfo. The InjectionConsumerInfo, therefore, not only describes the type of the consumer, but also the parameter or property in which the dependency is injected into. But now the question becomes: where is the dependency (either Validator1 or Validator2) injected into? This is something that probably has to be made up, because in reality the are not injected (instead, they are created by the ContainerControlledCollection<T>, but that's an implementation detail).
  • Letting the ContainerControlledCollection<T> (or the ConrolledCollectionHelper.ContainerControlledCollectionRegistration) produce 'the right' KnownRelationship instances, however, causes a new problem; because of the addition of the validators, the diagnostic are tripped and causes lifestyle mismatches. I haven't investigated much further on what causes these mismatches and how they should be fixed. This likely requires a change in the diagnostic analyzer, but I have to investigate this.

I'll keep you posted with my progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.