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

Unexpected Behavior with Generic Parameters #11

Closed
danechambers opened this issue Jan 11, 2022 · 9 comments
Closed

Unexpected Behavior with Generic Parameters #11

danechambers opened this issue Jan 11, 2022 · 9 comments

Comments

@danechambers
Copy link

danechambers commented Jan 11, 2022

I'm not sure if this is working as designed or not, but it is something that caught me off guard. Please feel free to correct any misconceptions I may have about how the library should work.

Describe the Bug

When a method on the aggregate services interface accepts a generic parameter, a new instance of the parameter is created (or is attempted to) when invoking the method rather than using the parameter that is given to it.

Steps to Reproduce

I added a new pass-through interface and implementation

    public interface IPassThroughOpenGeneric<T>
    {
        IOpenGeneric<T> OpenGeneric { get; }
    }

    public class OpenGenericPassThroughImpl<T> : IPassThroughOpenGeneric<T>
    {
        public OpenGenericPassThroughImpl(IOpenGeneric<T> openGeneric)
        {
            OpenGeneric = openGeneric;
        }

        public IOpenGeneric<T> OpenGeneric { get; }
    }

and added this method to IOpenGenericAggregate

        IPassThroughOpenGeneric<T> PassThroughOpenGeneric<T>(IOpenGeneric<T> openGeneric);

In AggregateServiceGenericsFixture, I added this registration

    builder.RegisterGeneric(typeof(OpenGenericPassThroughImpl<>))
        .As(typeof(IPassThroughOpenGeneric<>));

then this test

        [Fact]
        public void Method_UnpackOpenGeneric()
        {
            var aggregateService = _container.Resolve<IOpenGenericAggregate>();

            var generic = aggregateService.GetOpenGeneric<It.IsAnyType>();

// -----works as expected-----
            var passThroughFunc = _container.Resolve<Func<IOpenGeneric<It.IsAnyType>, IPassThroughOpenGeneric<It.IsAnyType>>>();
            var genericFromPassThroughFunc = passThroughFunc(generic);
//----------
            Assert.Same(generic, genericFromPassThroughFunc.OpenGeneric);
        }

However, if I use the aggregate service instead, it does not work.

        [Fact]
        public void Method_UnpackOpenGeneric()
        {
            var aggregateService = _container.Resolve<IOpenGenericAggregate>();

            var generic = aggregateService.GetOpenGeneric<It.IsAnyType>();

// -----does not work-----
            var genericPassThrough = aggregateService.PassThroughOpenGeneric(generic);
//----------
            Assert.Same(generic, genericPassThrough.OpenGeneric);
        }

Expected Behavior

I am under the impression that the aggregate service should behave as the Func<> does. Here is my example if you like to see my full implementation:
GenericParameterExample.zip

Dependency Versions

Autofac: v6.0.0

@tillig
Copy link
Member

tillig commented Jan 11, 2022

It would be helpful to have all the relevant code (eg, IOpenGenericAggregate and the code to build the container) posted if possible. I recognize the zip file is there, but that's sort of hard to work with on, say, a mobile device. Posting the code visibly can help you get an answer faster.

@danechambers
Copy link
Author

@tillig
Copy link
Member

tillig commented Jan 11, 2022

Oh, man, I'm sorry I probably wasn't speaking clearly. When I said "having it posted" I meant "right here in the issue" so when folks come in and read this...

and added this method to IOpenGenericAggregate

...the code for IOpenGenericAggregate is actually right here in the issue, fully self-contained. I apologize for being sorta pedantic on this. I realize it's probably painful. There's method to the madness!

  • I (personally) have limited time to address all the questions and, honestly, there's a touch of OSS maintainer burnout, so I get to answering most questions on my phone while I'm watching TV or [sorta?] spending time with the family.
  • Seeing all the code in one spot, when that's possible, can sometimes make it clear what's going on without having to dive into debugging territory.
  • Later when we close this issue out, we will still have all the code right here in the issue so if folks are searching for it or if we want to know why we made a decision, it's right there, searchable/indexed and all spelled out. Remote repos work while we're fixing it, but they mysteriously disappear after the issue gets resolved; zip files are great but not indexed for search and hard to work with in mobile.

It may be that one of the other maintainers might be able to dive in more interactively. It might be a day or three before I can look at it. I was just hoping to potentially see something obvious without going spelunking.

@danechambers
Copy link
Author

That makes complete sense. I totally get the burnout too. Thank you for the context, I really do appreciate your time.
If you think this is something worth looking into, I can help dive into the issue too. 🙂

I've posted the code for IOpenGenericAggregate below.

namespace Autofac.Extras.AggregateService.Test
{
    public interface IOpenGenericAggregate
    {
        IOpenGeneric<T> GetOpenGeneric<T>();

        IOpenGeneric<string> GetResolvedGeneric();

        IPassThroughOpenGeneric<T> PassThroughOpenGeneric<T>(IOpenGeneric<T> openGeneric);
    }
}

Here is the full fixture as well

namespace Autofac.Extras.AggregateService.Test
{
    public class AggregateServiceGenericsFixture
    {
        private readonly IContainer _container;

        public AggregateServiceGenericsFixture()
        {
            var builder = new ContainerBuilder();
            builder.RegisterAggregateService<IOpenGenericAggregate>();
            builder.RegisterGeneric(typeof(OpenGenericImpl<>))
                .As(typeof(IOpenGeneric<>));
            builder.RegisterGeneric(typeof(OpenGenericPassThroughImpl<>))
                .As(typeof(IPassThroughOpenGeneric<>));

            _container = builder.Build();
        }

        /// <summary>
        /// Attempts to resolve an open generic by a method call.
        /// </summary>
        [Fact]
        public void Method_ResolveOpenGeneric()
        {
            var aggregateService = _container.Resolve<IOpenGenericAggregate>();

            var generic = aggregateService.GetOpenGeneric<object>();
            Assert.NotNull(generic);

            var ungeneric = aggregateService.GetResolvedGeneric();
            Assert.NotNull(ungeneric);
            Assert.NotSame(generic, ungeneric);
        }

        [Fact]
        public void Method_UnpackOpenGeneric()
        {
            var aggregateService = _container.Resolve<IOpenGenericAggregate>();

            var generic = aggregateService.GetOpenGeneric<It.IsAnyType>();

            // works
            var passThroughFunc = _container.Resolve<Func<IOpenGeneric<It.IsAnyType>, IPassThroughOpenGeneric<It.IsAnyType>>>();
            var genericFromPassThroughFunc = passThroughFunc(generic);
            Assert.Same(generic, genericFromPassThroughFunc.OpenGeneric);

            // doesn't work
            var genericPassThrough = aggregateService.PassThroughOpenGeneric(generic);
            Assert.Same(generic, genericPassThrough.OpenGeneric);
        }
    }
}

Anything else I can do, please let me know.

@tillig
Copy link
Member

tillig commented Jan 11, 2022

That helps, thanks!

Things I'm thinking about as I see it now (just brainstorming)...

It.IsAnyType is a Moq thing. Moq uses Castle.DynamicProxy under the covers just like AggregateService does for its stuff. I don't know what's underlying It.IsAnyType - does this test fail if you use a real type and not a Moq placeholder?

If you make a closed generic version of IOpenGeneric<T>, like public class ClosedGeneric : IOpenGeneric<string> and use that as the parameter value for the PassThroughOpenGeneric<T>(xxx) method, does it work? Probably not, but curious if it makes a difference.

If something is amiss, it's probably going to be in this bit where the dynamic proxy is building up the list of methods and figuring out the backing implementations. I don't see that we have unit tests that cover methods that take generics as parameters so that's likely where I'd start looking.

@tillig
Copy link
Member

tillig commented Jan 14, 2022

The problem is here, where the method handler basically says, "Add a parameter of type IOpenGeneric<T> to the list, with the value of this closed generic IOpenGeneric<SomeType>. Autofac looks for a typed parameter with the type IOpenGeneric<SomeType>, doesn't see it, and skips on past.

The existing mechanism is simple and it works, but it's not as rich as the full-on Func<X, Y> factory generator in core Autofac.

I'm not sure what the best route to fixing this is.

My gut says the most robust solution would be to do something similar to what the GeneratedFactoryRegistrationSource does and use the factory generator to build the Func<X, Y> that underlies the method. The downside is that, with an open generic, it could be expensive because you'll have to do that for every call, unless you also introduce a caching mechanism to store every combination of IOpenGeneric<T> to generated factory (which core Autofac does for generated factories when it caches registrations for things). But that would be the most robust, and it'd support all the stuff core Autofac Func<X, Y> does.

Possibly simpler but looking more complicated, would be to build the delegate Func<X, Y> through reflection and then resolve that function and execute it. Which is to say, build up the type Func<IOpenGeneric<T>, IPassThroughGeneric<T>> and resolve that, then execute that factory with the parameters provided. It would mean less complexity tying to the factory generator and it should technically still support all the core Autofac features.

I'll see if I can get that second idea working.

@tillig tillig closed this as completed in a889559 Jan 15, 2022
tillig added a commit that referenced this issue Jan 15, 2022
Resolve #11: Use Func<X, Y> to handle methods with parameters.
@tillig
Copy link
Member

tillig commented Jan 15, 2022

I won't have time until next week to cut a release with the fix, but it'll happen.

@danechambers
Copy link
Author

Thank you again for your time on this. I definitely appreciate this project (in addition to autofac core) and all the time/effort that everybody puts into it. Looking forward to the release 🙂.

@tillig
Copy link
Member

tillig commented Jan 18, 2022

v6.1.2 published.

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

No branches or pull requests

2 participants