JasperFx / lamar

Fast Inversion of Control Tool and Successor to StructureMap
https://jasperfx.github.io/lamar
MIT License
563 stars 118 forks source link

Inconsistent Error Message When Requesting Concrete Class #264

Closed RyanThomas73 closed 3 years ago

RyanThomas73 commented 3 years ago

When requesting a concrete type from the container that has no interfaces, the scanner does not have a convention for registering the concrete types as themselves. If the concrete class was in an assembly that was scanned the container will try to resolve the concrete type; but if it cannot find a constructor with parameters it knows how to supply, an exception is thrown stating that it doesn't know about the concrete type.

This exception message is very misleading because it makes it appear as if the scanner did not find / did not attempt to register the concrete type. The behavior should be to thrown an exception message that matches what happens when a registered interface is requested, indicating which constructor parameters are unknown.

Example Test:

    [TestClass]
    public class LamarBehaviorTests
    {
        [TestMethod]
        public void LamarContainer_returns_the_ClassWithNoInterface()
        {
            var serviceCollection = new TestRegistry();
            using var serviceProvider = new Lamar.Container(serviceCollection);

            Exception firstException;
            try
            {
                var someInterface = serviceProvider.GetRequiredService<ISomeInterface>();
            }
            catch (Exception exc) 
            {
                firstException = exc;
            }

            Exception secondException;
            try
            {
                var classWithNoInterface = serviceProvider.GetRequiredService<ClassWithNoInterface>();
            }
            catch (Exception exc) 
            {
                secondException = exc;
            }

            // The exception message from the 2nd case should be the similar to the first case
            secondException.Message.Should.BeSimilarTo(firstException.Message);
        }
    }

    public interface IInterfaceWithNoImplementation
    {

    }

    public class ClassWithNoInterface
    {
        public ClassWithNoInterface(IInterfaceWithNoImplementation unknown) 
        {

        }
    }

    public interface ISomeInterface
    {
    }

    public class ClassWithInterface : ISomeInterface
    {
        public ClassWithInterface(IInterfaceWithNoImplementation unknown) 
        {

        }
    }

    public class TestRegistry : Lamar.ServiceRegistry
    {

        public TestRegistry()
        {
            var defaultLifetime = Microsoft.Extensions.DependencyInjection.ServiceLifetime.Scoped;

            Scan(scan =>
            {
                scan.AssemblyContainingType<LamarBehaviorTests>();

                scan.WithDefaultConventions(defaultLifetime);
            });
        }
    }
jeremydmiller commented 3 years ago

@RyanThomas73 This behavior is independent of the type scanning. If you request a concrete type that is not currently registered, Lamar (and StructureMap) will attempt to create a registration for it on the fly, depending upon whether or not Lamar can build all the dependencies of the concrete type's dependencies.

I'm going to respectively disagree with you on the exception message. The existing exception message came about through other user feedback and explains why Lamar can't resolve this type.

So yes, the exception messages are inconsistent because it's two different scenarios.

RyanThomas73 commented 3 years ago

@jeremydmiller While it is true that the behavior is different for the two different scenarios, the value provided by the exception message in the Auto-Wiring / on the fly registration scenario is considerably less than the value provided by the exception message in the type scanning scenario.

As far as I know, Lamar does not have an OOTB convention to support adding registrations for scanned concrete type and doing so might incur unnecessary overhead/bloat for the registrations. Thus, the consumer is forced to use the on-the-fly registration for OOTB support.

In the case of the first exception message the consumer gets an exception that identifies the actual problem (no plugin type registered for IInterfaceWithNoImplementation)

In the 2nd case the actual problem is the same but the consumer get much less valuable information and thus more leg work is required to determine why ClassWithNoInterface isn't registered. Does it not automatically register concrete types that are scanned? Is ClassWithNoInterface not included in the scanned types? Is ClassWithNoInterface included in the scanned types but it failed to create a registration on the fly because some other dependency is unavailable in the build plan? Having an exception message that narrows down the possibility so that you get closer to the actual problem the way the first exception message does adds considerable value.

jeremydmiller commented 3 years ago

@RyanThomas73,

"Lamar does not have an OOTB convention to support adding registrations for scanned concrete type and doing so might incur unnecessary overhead/bloat for the registrations." -- it doesn't have anything out of the box, no, but the type discovery is very extensible and you have plenty of facilities for filtering out types to avoid over-registering. TBH, I use very little type scanning in my own projects beyond simple OOTB conventions. I've found the auto-discovery of concrete types to be very helpful over the years personally.

If you really feel strongly enough about this, I'll happily accept a PR that makes the exceptions be consistent, but I'd want the message to be a union of the information presented in the two exceptions today.

I take pull requests.