Some parts of the API cause ambiguity when components implement multiple interfaces #348

Open
dotnetjunkie opened this Issue Dec 15, 2016 · 12 comments

Projects

None yet

4 participants

@dotnetjunkie
Collaborator
dotnetjunkie commented Dec 15, 2016 edited

There are multiple design flaws in v3 that can cause ambiguity when components implement multiple interfaces.

The following parts of the API are affected:

  1. PredicateContext.Consumer.ServiceType
  2. ExpressionBuilding.RegisteredServiceType
  3. The Type argument in IPropertySelectionBehavior.SelectProperty(Type, PropertyInfo).
  4. InstanceInitializationData.Context.Producer

1. Conditional registrations can cause ambiguity when Consumer.ServiceType is used

There is a design flaw in v3 concerning RegisterConditional that we overlooked while designing v3.

The predicate supplied to a RegisterConditional method does not allow looking up to the consumer's consumer, because this would easily lead to ambiguity problems, when types are registered using a lifestyle other than Transient.

This ambiguity problem however still exists when a conditional registration is selected based on the service type of its consumer, while:

  • That consumer has a lifestyle other than transient, and
  • That consumer is registered using two different service types.

While using the following code structure:

public interface IX { }
public interface IY { }
public interface ILogger { }
public class XLogger : ILogger { }
public class YLogger : ILogger { }

public class A : IX, IY
{
    public readonly ILogger Logger;
    public A(ILogger logger) { this.Logger = logger; }
}

The following unit test demonstrates the problem:

var container = new Container();

container.RegisterConditional(typeof(ILogger), typeof(XLogger), Lifestyle.Singleton, 
    c => c.Consumer.ServiceType == typeof(IX));
container.RegisterConditional(typeof(ILogger), typeof(YLogger), Lifestyle.Singleton, 
    c => c.Consumer.ServiceType == typeof(IY));

var reg = Lifestyle.Singleton.CreateRegistration<A>(container);

container.AddRegistration(typeof(IX), reg);
container.AddRegistration(typeof(IY), reg);

container.GetInstance<IX>();
var y = (A)container.GetInstance<IY>();

Assert.IsInstanceOf<YLogger>(instance.Logger);

The previous test fails because the returned instance of IY does not contain the YLogger but the XLogger. This is caused by the GetInstance<IX>() call. That call causes A to be created (using the context of IX) and that instance is cached. During the time that IY is requested, the instance of A is already created and will be returned immediately.

The previous case is something that can't safely be expressed; it is technically impossible to do this, and instead of giving the user the illusion that he can differentiate based on the ServiceType, Simple Injector should prevent the user falling into this trap.

There are two possible solutions to this problem:

  • Remove the Consumer.ServiceType property from the PredicateContext, since this data is causing ambiguity. Downside is that quite a lot of users are using this property, so this is a severe breaking change.
  • Try to detect ambiguity and throw an exception when this happens. It is unclear however whether this is possible, but if possible, this won't be an easy task.

Not only is the detection of invalid use of ServiceType hard to implement, it still doesn't guide users into the right path. This means that Removing the ServiceType property is the best solution.

2. ExpressionBuilding.RegisteredServiceType

For ExpressionBuilding.RegisteredServiceType partly the same problems exist. ExpressionBuilding is called during the process of building the expression tree inside Registration, but since Registration can be reused by multiple InstanceProducer instances (that control the actual service type), ambiguity emerges when both InstanceProducer instances have a different service type.

3. The Type argument in IPropertySelectionBehavior.SelectProperty(Type, PropertyInfo).

For IPropertySelectionBehavior.SelectProperty the problem is identical to ExpressionBuilding.RegisteredServiceType. SelectProperty is called during the process of building the expression tree inside Registration, but since Registration can be reused by multiple InstanceProducer instances (that control the actual service type), ambiguity emerges when both InstanceProducer instances have a different service type.

4. InstanceInitializationData.Context.Producer

Similar to the previous problems, during the building of the expression tree of a Registration, the registration wraps the expression with the available initializers, registered through Container.RegisterInitializer (if any). In case there is an applicable initializer, the Registration class will call ensure that that initializer is called where it passes in an InstanceInitializationData instance. An InstanceInitializationData wraps both the created instance and an InitializationContext class. The InitializationContext contains both the Registration and the InstanceProducer. Obviously, this InstanceProducer again is the problem, because the Registration can belong to multiple InstanceProducer instances, while the expression tree will be built just once. This again causes ambiguity. The producer that gets built first wins.

To solve this problem, the InstanceInitializationData should not wrap an InitializationContext instance, but simply the Registration class.

@dotnetjunkie dotnetjunkie added this to the Backlog milestone Dec 15, 2016
@dotnetjunkie
Collaborator

Dear Simple Injector users, would you please tell us whether you use RegisterConditional in combination with PredicateContext.Consumer.ServiceType, and if so, could you show an example of how you are using this?

@qujck
Collaborator
qujck commented Dec 15, 2016

I haven't used it

@janhartmann

No, only with context => !context.Handled.

E.g. container.RegisterConditional(typeof(IValidator<>), typeof(ValidateNothingDecorator<>), Lifestyle.Singleton, context => !context.Handled);

@dotnetjunkie
Collaborator

@Fresa uses is as follows:

container.RegisterConditional(
    typeof(IConnection),
    Lifestyle.Singleton.CreateRegistration(() => new CarDatabaseConnection(), container),
    c => c.Consumer != null && c.Consumer.ServiceType == typeof(IDatabase<Car>));

container.Register(typeof(IDatabase<>), typeof(InMemoryDatabase<>), Lifestyle.Singleton);
@janhartmann
janhartmann commented Dec 16, 2016 edited

I just went over a few repositories on GitHub and found https://github.com/maca88/PowerArhitecture/blob/master/Source/PowerArhitecture.DataAccess/Extensions/SimpleInjectorExtensions.cs#L121 from @maca88:

container.RegisterConditional(typeof(ISession), registration, context =>
{
    var defRepoType = context.Consumer?.ServiceType?.GetGenericType(typeof(IRepository<,>));
    ...
    var modelType = defRepoType.GetGenericArguments()[0];
}
@dotnetjunkie
Collaborator
dotnetjunkie commented Dec 16, 2016 edited

@janhartmann, that's an interesting example, because it shows how the ServiceType is used to get its generic arguments from. This is much harder to do with only the ImplementationType, so this would probably mean that if we remove ServiceType, we should make it easier to extract the service type from the implementation type. For instance:

if (context.Consumer.ImplementationType.IsClosedTypeOf(typeof(IRepository<,>)) {
    Type defRepoType =
        context.Consumer.ImplementationType.GetClosedTypesOf(typeof(IRepository<,>)).Single();
    ...
    Type modelType = defRepoType.GetGenericArguments()[0];
}

Note the IsClosedTypeOf and GetClosedTypesOf extension methods. They currently don't exist.

@TheBigRic
TheBigRic commented Dec 16, 2016 edited

I guess this is almost the same as @janhartmann but nevertheless:

 public static void RegisterFallBackAuthorizedRepository(
     Container container, Type[] knownTypes, Lifestyle scope)
{
    container.RegisterConditional(typeof(IAuthorizedRepository<>), 
        typeof(AuthorizedRepository<>), scope,
        context =>
        {
            if (!context.Handled)
            {
                var type = context.ServiceType.GetGenericArguments().Single();
                if (!knownTypes.Contains(type))
                {
                    throw new NotSupportedException("some message");
                }
            }
            return !context.Handled;
        });
}

What I do with this code is preventing somebody in my team to use the default implementation of an AuthorizedRepo, which does no authorization at all. We have this implementations for models which include data available for all users in the system.

So it prevents somebody to use the fallback directly and hopefully the user will rethink what he was doing and may add a type to the knowntypes list or must create an implementation which will actually check if the current user has the correct permissions.

Although I think I also could get the generic type argument also via the implementationtype in this case.

@dotnetjunkie dotnetjunkie modified the milestone: v4, Backlog Dec 18, 2016
@dotnetjunkie
Collaborator

I moved the creation of these common extension methods on Type to #358.

@dotnetjunkie
Collaborator
dotnetjunkie commented Dec 23, 2016 edited

Using the IsClosedTypeOf and GetClosedTypesOf methods, the previously posted examples can be rewritten to the following:

@Fresa's example:

container.RegisterConditional(
    typeof(IConnection),
    Lifestyle.Singleton.CreateRegistration(() => new CarDatabaseConnection(), container),
    c => typeof(IDatabase<Car>).IsAssignableFrom(c.Consumer.ImplementationType));

NOTE: Examples updated to use GetClosedTypeOf extension method.

@maca88's example:

container.RegisterConditional(typeof(ISession), registration, context =>
{
    var defRepoType =
        context.Consumer.ImplementationType.GetClosedTypeOf(typeof(IRepository<,>);
    ...
    var modelType = defRepoType.GetGenericArguments()[0];
}

@TheBigRic's example:

var serviceType =
    context.ImplementationType.GetClosedTypeOf(typeof(IAuthorizedRepository<>));
var type = serviceType.GetGenericArguments().Single();
@dotnetjunkie
Collaborator

As GetClosedTypesOf(Type).Single() seems to be a very common use case, it might be good to add an Type GetClosedTypeOf(Type) method that returns a single type and throws an expressive exception in case there are none or multiple?

@TheBigRic

Seems like nice solutions!

@dotnetjunkie dotnetjunkie referenced this issue in simpleinjector/Documentation Dec 25, 2016
Open

v4 documentation changes #50

@dotnetjunkie
Collaborator

There's a similar API that can cause the same problems as with PredicateContext.Consumer.ServiceType, namely: ExpressionBuildingEventArgs.RegisteredServiceType. ExpressionBuildingEventArgs is used when hooking onto the ExpressionBuilding event. This event gets raised inside the Registration class and since a registration class can be reused by multiple InstanceProducer instances, it is impossible to know the exact RegisteredServiceType at that point in time.

@dotnetjunkie dotnetjunkie added a commit that referenced this issue Dec 27, 2016
@dotnetjunkie dotnetjunkie PredicateContext.ServiceType removed.
Related to #348
5392b6c
@dotnetjunkie dotnetjunkie added a commit that referenced this issue Dec 27, 2016
@dotnetjunkie dotnetjunkie Invalid commit fixed.
Related to #348
c752267
@dotnetjunkie dotnetjunkie changed the title from Conditional registrations can cause ambiguity when Consumer.ServiceType is used to Some parts of the API cause ambiguity when components implement multiple interfaces Dec 27, 2016
@dotnetjunkie dotnetjunkie self-assigned this Dec 28, 2016
@dotnetjunkie dotnetjunkie added a commit that referenced this issue Dec 30, 2016
@dotnetjunkie dotnetjunkie RegisterInitializer(Action, Predicate) changed,
Container.RegisterInitializer(Action<InstanceInitializationData>,
Predicate<InitializationContext>) replaced with
RegisterInitializer(Action<InstanceInitializationData>,
Predicate<InitializerContext>) method. Related to #348
a1aeb80
@dotnetjunkie dotnetjunkie added a commit that referenced this issue Dec 30, 2016
@dotnetjunkie dotnetjunkie changes.txt updated.
related to #348
63c1b5e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment