ParameterCompatibleWithTypeConstraint returns true instead of false when resolving open generic service

[sebekz]

I have a simple case of an open generic type with type constraint, like:

public class MultiTenantHandler<TRequest, TResponse> : IResponseHandler<TRequest, TResponse>
    where TResponse : IQueryable<IMultitenant> {}

public class MultiTenantOptionalHandler<TRequest, TResponse> : IResponseHandler<TRequest, TResponse>
    where TResponse : IQueryable<IMultitenantOptional> {}

https://gist.github.com/sebekz/4c658a5c7551ba5a2b3fd81488ea3ee7 holds an console app sample for this particular case.

I have logged a StackOverflow question http://stackoverflow.com/questions/39291046/autofac-issues-when-resolving-generic-types-implementing-the-same-interface-nee which led to a hint of potential bug/issue in Autofac itself, therefore I am logging an issue here.

The problem is, that when two (or more) open generic types are registered in the container

builder.RegisterGeneric(typeof(MultiTenantHandler<,>))
    .As(typeof(IResponseHandler<,>))
    .SingleInstance();  
builder.RegisterGeneric(typeof(MultiTenantOptionalHandler<,>))
    .As(typeof(IResponseHandler<,>))
    .SingleInstance();

an attempt to resolve it will fail n-1 times and succeed afterwards, where n equals number of implementations of IResponseHandler<,>. So if we had one more class MultiTenantAnotherHandler with yet another where type constraint, it would fail to .Resolve() 2 times and succeed each time after that. For a full issue description, please refer to StackOverflow question mentioned above.

Further investigation has led to a call stack:

IsCompatibleWithGenericParameterConstraints()
  ParameterCompatibleWithTypeConstraint()
    IsAssignableFrom()

IsCompatibleWithGenericParameterConstraints method call ensures that the type parameters are compatible with the given type constraints. It uses the method ParameterCompatibleWithTypeConstraint to check for that and that metod uses IsAssignableFrom in its implementation. So far so good, since IsAssignableFrom works as expected, but in the case it returns false for all its checks ParameterCompatibleWithTypeConstraint checks if it can create a generic type with the base type of the constraint and the generic argument of the parameter.

Traverse.Across(parameter, p => p.GetTypeInfo().BaseType)
                .Concat(parameter.GetTypeInfo().ImplementedInterfaces)
                .Any(p => ParameterEqualsConstraint(p, constraint));

And Since this is always possible, the method returns true for all checks, which is incorrect, e.g.:

Type parameter = typeof(IQueryable<Game>); // implements IMultitenant
Type constraint = typeof(IQueryable<IMultitenant>);
// returns true
var results = Traverse.Across(parameter, p => p.GetTypeInfo().BaseType)
    .Concat(parameter.GetTypeInfo().ImplementedInterfaces)
    .Any(p => ParameterEqualsConstraint(p, constraint)); 

constraint = typeof(IQueryable<IMultitenantOptional>);
// returns true
results = Traverse.Across(parameter, p => p.GetTypeInfo().BaseType)
    .Concat(parameter.GetTypeInfo().ImplementedInterfaces)
    .Any(p => ParameterEqualsConstraint(p, constraint));

parameter = typeof(IQueryable<Trophy>); // implements IMultitenantOptional
constraint = typeof(IQueryable<IMultitenant>);
// returns true
results = Traverse.Across(parameter, p => p.GetTypeInfo().BaseType)
    .Concat(parameter.GetTypeInfo().ImplementedInterfaces)
    .Any(p => ParameterEqualsConstraint(p, constraint));

constraint = typeof(IQueryable<IMultitenantOptional>);
// returns true
results = Traverse.Across(parameter, p => p.GetTypeInfo().BaseType)
    .Concat(parameter.GetTypeInfo().ImplementedInterfaces)
    .Any(p => ParameterEqualsConstraint(p, constraint));

true is always returned because for IQueryable<Game> the call to Traverse.Across(parameter, p => p.GetTypeInfo().BaseType) returns one element of type IQueryable<Game>, and this resolves itself into an always true ParameterEqualsConstraint(typeof(IQueryable<Game>), typeof(IQueryable<T>)).

// returns true
var pec = ParameterEqualsConstraint(typeof(IQueryable<Game>), typeof(IQueryable<IMultitenant>));
// returns true
pec = ParameterEqualsConstraint(typeof(IQueryable<Game>), typeof(IQueryable<IMultitenantOptional>));

It's always true, because ParameterEqualsConstraint is always able to create an instance of IQueryable<Game> from IQueryable<T> generic type definition, for example.

// parameter is IQueryable<Game>
// constraint is IQueryable<IMultitenantOptional>
private static bool ParameterEqualsConstraint(Type parameter, Type constraint)
// initializes the variable as Game
var genericArguments = parameter.GetTypeInfo().GenericTypeArguments;
// initializes the variable as IQueryable<T>
var typeDefinition = constraint.GetGenericTypeDefinition();
// returns IQueryable<Game>
var genericType = typeDefinition.MakeGenericType(genericArguments);
// returns true
return genericType == parameter;

It looks like a bug, if there may be a valid reason for this I would appreciate an explanation and perhaps we could work on a solution which would satisfy both my particular use case (which I think is perfectly valid) as well.

[sebekz]

I got it working by replacing the ParameterEqualsConstraint method definition with

private static bool ParameterEqualsConstraint(Type parameter, Type constraint)
{
    var genericArguments = parameter.GetTypeInfo().GenericTypeArguments;
    if (genericArguments.Length > 0 && constraint.GetTypeInfo().IsGenericType)
    {
        var typeDefinition = constraint.GetGenericTypeDefinition();
        if (typeDefinition.GetTypeInfo().GenericTypeParameters.Length == genericArguments.Length)
        {
            try
            {
                var genericType = typeDefinition.MakeGenericType(genericArguments);
                var constraintArguments = constraint.GetTypeInfo().GenericTypeArguments;

                for (int i = 0; i < constraintArguments.Count(); i++)
                {
                    var constraintArgument = constraintArguments[i].GetTypeInfo();
                    if (!constraintArgument.IsGenericParameter && !constraintArgument.IsAssignableFrom(genericArguments[i].GetTypeInfo())) return false;
                }

                return genericType == parameter;
            }
            catch (Exception)
            {
                return false;
            }
        }
    }

    return false;
}

The IsGenericParameter check was required so that existing tests (e.g. CanResolveComponentWithNestedConstraintViaInterface) still succeeded. I added my own test for my specific use case (names could be generalized, but I left them as in my implementation so that I was sure I did not miss anything) and all tests pass, including existing Autofac repo ones.

public interface IMultitenant
{
}

public interface IMultitenantOptional
{
}

public interface IRequest<TResponse> { }

public interface IResponseHandler<in TRequest, TResponse>
{
}

public class Game : IMultitenant
{
}

public class Trophy : IMultitenantOptional
{
}

public class MultiTenantHandler<TRequest, TResponse> : IResponseHandler<TRequest, TResponse>
    where TResponse : IQueryable<IMultitenant>
{
}

public class MultiTenantOptionalHandler<TRequest, TResponse> : IResponseHandler<TRequest, TResponse>
    where TResponse : IQueryable<IMultitenantOptional>
{
}

[Fact]
public void CanResolveComponentUsingInterfaceWhenConstraintAndClassUseInterfaces()
{
    var builder = new ContainerBuilder();

    builder.RegisterGeneric(typeof(MultiTenantHandler<,>))
        .As(typeof(IResponseHandler<,>));

    builder.RegisterGeneric(typeof(MultiTenantOptionalHandler<,>))
        .As(typeof(IResponseHandler<,>));                

    var container = builder.Build();

    // These simple concrete implementations lookup fail as per issue #794
    Assert.True(container.IsRegistered<IResponseHandler<int, IQueryable<Game>>>());
    Assert.True(container.IsRegistered<IResponseHandler<int, IQueryable<Trophy>>>());

    // These a little bit more complex concrete implementations lookup fail as well as per issue #794
    Assert.True(container.IsRegistered<IResponseHandler<IRequest<IQueryable<Game>>, IQueryable<Game>>>());
    Assert.True(container.IsRegistered<IResponseHandler<IRequest<IQueryable<Trophy>>, IQueryable<Trophy>>>());

    // These should resolve, but per issue #794 they throw exceptions
    container.Resolve<IResponseHandler<IRequest<IQueryable<Game>>, IQueryable<Game>>>();
    container.Resolve<IResponseHandler<IRequest<IQueryable<Game>>, IQueryable<Trophy>>>();
}

That's the "easy" part. Now, I've got a funny feeling that this is not going to be that easy and I want to discuss this before posting a PR to the repo. Especially this [SuppressMessage("Microsoft.Design", "CA1031", Justification = "Implementing a real TryMakeGenericType is not worth the effort.")] entry in Autofac code makes me think, that it cannot be that easy... can it?

[tillig]

We really appreciate you taking the time to walk through this. I am excited to see a potential PR for it. Sorry to be slow in responding - you posted your SO question near the end of the day on a long holiday weekend for the US. 😉

I don't recall personally working on the generic type stuff there. Given some of it's been around for a while, it may be that some things have gotten easier. If you can make the existing tests continue to pass but also make your new tests pass, hey, that sounds great to me.

I admit I'm personally a bit swamped with some other projects that fell by the wayside during the .NET Core conversion that ate our time for the past year, give or take. If a PR comes in, I'd love to check it out and see about pulling it in; otherwise it may be a bit of time before we can take a crack at this ourselves.

[sebekz]

Thanks @tillig for reaching out. I will definitely test this much more, but so far it looks like I should be able to submit a PR soon. I can confirm that both the existing tests as well as my new tests pass with flying colors. It's all green with the changes in and working as expected in the real project scenario when using modified Autofac package.

So, long story short, please look out for a PR in a nearest future. Frankly, if not for the vague comment in the ParameterEqualsConstraint code, I would be much more happy with my fix. But that suppressed CA1031 with justification "Implementing a real TryMakeGenericType is not worth the effort." (there is a try with a generic Exception-based catch in ParameterEqualsConstraint code) makes me question the the whole ParameterEqualsConstraint implementation. Perhaps there is a better way to do this.

I will keep you posted.

[sebekz]

@tillig, I have just submitted a pull request #796 with what I think is a fix for the issue. Please let me know in case you need any additional explanations or changes made.

[tillig]

Pulled!

Truly, this is awesome. Getting help with these sometimes really hard to lock down issues is amazing. Thanks so much for the PR!

[tillig]

Fix included in v4.1.1, pushed to NuGet.