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

Please provide a method like ValidateConfiguration() for tests #328

Open
jnm2 opened this issue Sep 11, 2017 · 51 comments
Open

Please provide a method like ValidateConfiguration() for tests #328

jnm2 opened this issue Sep 11, 2017 · 51 comments

Comments

@jnm2
Copy link
Contributor

jnm2 commented Sep 11, 2017

There's nothing in the documentation about validating a container. I want a guarantee under test that for all the dependencies the container knows about, you would not receive a configuration error.

This is the best thing I can find, courtesy of @mikehadlow:

private static void CheckForPotentiallyMisconfiguredComponents(IWindsorContainer container)
{
    var host = (IDiagnosticsHost)container.Kernel.GetSubSystem(SubSystemConstants.DiagnosticsKey);
    var diagnostics = host.GetDiagnostic<IPotentiallyMisconfiguredComponentsDiagnostic>();

    var handlers = diagnostics.Inspect();

    if (handlers.Any())
    {
        var message = new StringBuilder();
        var inspector = new DependencyInspector(message);

        foreach (IExposeDependencyInfo handler in handlers)
        {
            handler.ObtainDependencyDetails(inspector);
        }

        throw new MisconfiguredComponentException(message.ToString());
    }
}

There's nothing obvious about this code. If it is indeed the code you're recommend, please ship the code as a ValidateConfiguration().

I'm surprised that such a test isn't recommended practice for every user. This is something I'd expect to find conspicuously linked from both the readme and the FAQ. This is the closest thing I could find. It talks about testing various aspects of certain registrations but does not mention validating registrations to check specifically that all dependencies can be satisfied: https://github.com/castleproject/Windsor/blob/master/docs/mvc-tutorial-part-3a-testing-your-first-installer.md

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 11, 2017

And, the above code is no good if you have typed factories. So I'm hunting the correct code now.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 11, 2017

This seems to be close to what I need. Usage:

[Test]
public static void Default_configuration_is_valid()
{
    // See https://github.com/castleproject/Windsor/issues/328

    using (var container = Program.InitializeContainer())
    {
        if (TryGetUnresolvableDependenciesMessage(container.Kernel, out var message))
            Assert.Fail(message);
    }
}

Implementation:

private static bool TryGetUnresolvableDependenciesMessage(IKernel kernel, out string message)
{
    var actuallyUnresolvable = new List<(DependencyModel dependency, IHandler handler)>();
    var visitedHandlers = new HashSet<IHandler>();

    var naming = (INamingSubSystem)kernel.GetSubSystem(SubSystemConstants.NamingKey);
    foreach (var handler in naming.GetAllHandlers())
        FindActuallyUnresolvableDependencies(handler);

    foreach (var handler in naming.GetAllHandlers())
        FindActuallyUnresolvableDependencies(handler);

    void FindActuallyUnresolvableDependencies(IHandler handler)
    {
        if (handler.CurrentState == HandlerState.Valid || !visitedHandlers.Add(handler)) return;

        foreach (var dependency in handler.ComponentModel.Dependencies)
        {
            if (dependency.IsOptional || kernel.Resolver.CanResolve(CreationContext.CreateEmpty(), handler, handler.ComponentModel, dependency)) continue;

            var dependencyHandler = kernel.GetHandler(dependency.TargetItemType);
            if (dependencyHandler != null)
                FindActuallyUnresolvableDependencies(dependencyHandler);
            else
                actuallyUnresolvable.Add((dependency, handler));
        }
    }

    if (actuallyUnresolvable.Count == 0)
    {
        message = null;
        return false;
    }

    var sb = new StringBuilder("Unresolvable dependencies:");

    foreach (var dependentsByDependency in actuallyUnresolvable
        .GroupBy(_ => (type: _.dependency.TargetItemType, name: _.dependency.ReferencedComponentName)))
    {
        sb.AppendLine();
        sb.AppendLine();
        sb.Append(dependentsByDependency.Key.type);
        if (dependentsByDependency.Key.name != null)
            sb.Append(" named \"").Append(dependentsByDependency.Key.name);
        sb.Append(":");

        foreach (var linksByDependentNames in dependentsByDependency.GroupBy(_ => _.handler.ComponentModel.Name))
        {
            sb.AppendLine();
            sb.Append(" - ");

            var isFirst = true;
            foreach (var link in linksByDependentNames)
            {
                if (isFirst) isFirst = false;
                else sb.Append(", ");
                sb.Append(link.dependency.DependencyKey);
            }

            sb.Append(" for ").Append(linksByDependentNames.Key);
        }
    }

    message = sb.ToString();
    return true;
}

@ghost
Copy link

ghost commented Sep 13, 2017

@jnm2 - You practically wrote the code for me 👍

As soon as we merge #329 this will pass in appveyor and hopefully make into the next release.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 13, 2017

Sweet. Before you get too far, I'm running into some more issues with validation and typed factories.

I want this to be considered a totally valid configuration:

class ServiceWithRuntimeArg
{
    public ServiceWithRuntimeArg(int arg) { }
}

class ConsumingService
{
    public ConsumingService(Func<int, ServiceWithRuntimeArg> factory) { }
}

And this to be invalid:

class ServiceWithRuntimeArg
{
    public ServiceWithRuntimeArg(int arg) { }
}

class ConsumingService
{
    public ConsumingService(Func<ServiceWithRuntimeArg> factory) { }
}

But I'm having all kinds of problems examining the kernel to enumerate all explicit and implicit typed factories. I'm close to getting working code but it involves IKernelInternal.LoadHandlerByType and copying an uncomfortable amount of internal implementation details.

I want to share the code and the tests I'm using and hear your thoughts. It's almost passing. I suspect that either I'm missing an existing easy way to do something, or that you'll want to perhaps add API so that I don't have to rely on replicating internal Windsor logic.

@ghost
Copy link

ghost commented Sep 13, 2017

I will add some more test cases once I have finished debugging another issue. The internals are quite overwhelming at first.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 13, 2017

Do you agree with the outcomes in these test cases?

public sealed class Dependency_on_service_with_unknown_arg : ContainerFixture
{
    private sealed class ServiceWithUnknownArg
    {
        public ServiceWithUnknownArg(int arg)
        {
        }
    }

    [Test]
    public void Should_be_invalid()
    {
        Assert.That(Kernel, HasInvalidComponent<ServiceWithUnknownArg>());
    }
}

public sealed class Dependency_on_service_with_unknown_arg_with_explicit_factory : ContainerFixture
{
    private sealed class ServiceWithUnknownArg
    {
        public ServiceWithUnknownArg(int arg)
        {
        }
    }

    public Dependency_on_service_with_unknown_arg_with_explicit_factory()
    {
        Kernel.Register(Component.For<Func<int, ServiceWithUnknownArg>>().AsFactory());
    }

    [Test]
    public void Should_be_valid()
    {
        Assert.That(Kernel, HasValidComponent<ServiceWithUnknownArg>());
    }
}

public sealed class Dependency_on_factory_of_service_with_unknown_arg_implicit : ContainerFixture
{
    private sealed class ServiceWithUnknownArg
    {
        public ServiceWithUnknownArg(int arg)
        {
        }
    }

    private sealed class ConsumingService
    {
        public ConsumingService(Func<int, ServiceWithUnknownArg> factory) { }
    }

    [Test]
    public void Service_with_unknown_arg_should_be_valid()
    {
        Assert.That(Kernel, HasValidComponent<ServiceWithUnknownArg>());
    }

    [Test]
    public void Consuming_service_should_be_valid()
    {
        Assert.That(Kernel, HasValidComponent<ConsumingService>());
    }
}

public sealed class Dependency_on_factory_of_service_with_unknown_arg_explicit : ContainerFixture
{
    private sealed class ServiceWithUnknownArg
    {
        public ServiceWithUnknownArg(int arg)
        {
        }
    }

    private sealed class ConsumingService
    {
        public ConsumingService(Func<int, ServiceWithUnknownArg> factory)
        {
        }
    }

    public Dependency_on_factory_of_service_with_unknown_arg_explicit()
    {
        Kernel.Register(Component.For<Func<int, ServiceWithUnknownArg>>().AsFactory());
    }

    [Test]
    public void Service_with_unknown_arg_should_be_valid()
    {
        Assert.That(Kernel, HasValidComponent<ServiceWithUnknownArg>());
    }

    [Test]
    public void Consuming_service_should_be_valid()
    {
        Assert.That(Kernel, HasValidComponent<ConsumingService>());
    }
}

public sealed class Dependency_on_factory_of_service_missing_unknown_arg_with_implicit_factory : ContainerFixture
{
    private sealed class ServiceWithUnknownArg
    {
        public ServiceWithUnknownArg(int arg)
        {
        }
    }

    private sealed class ConsumingService
    {
        public ConsumingService(Func<ServiceWithUnknownArg> factory) { }
    }

    [Test]
    public void Service_with_unknown_arg_should_be_invalid()
    {
        Assert.That(Kernel, HasInvalidComponent<ServiceWithUnknownArg>());
    }

    [Test]
    public void Consuming_service_with_improper_factory_should_be_invalid()
    {
        Assert.That(Kernel, HasInvalidComponent<ConsumingService>());
    }
}

public sealed class Dependency_on_factory_of_service_missing_unknown_arg_with_explicit_factory : ContainerFixture
{
    private sealed class ServiceWithUnknownArg
    {
        public ServiceWithUnknownArg(int arg)
        {
        }
    }

    private sealed class ConsumingService
    {
        public ConsumingService(Func<ServiceWithUnknownArg> factory)
        {
        }
    }

    public Dependency_on_factory_of_service_missing_unknown_arg_with_explicit_factory()
    {
        Kernel.Register(Component.For<Func<int, ServiceWithUnknownArg>>().AsFactory());
    }

    [Test]
    public void Service_with_unknown_arg_should_be_valid()
    {
        Assert.That(Kernel, HasValidComponent<ServiceWithUnknownArg>());
    }

    [Test]
    public void Consuming_service_with_improper_factory_should_be_invalid()
    {
        Assert.That(Kernel, HasInvalidComponent<ConsumingService>());
    }
}

Can provide the implementation code too if you need it but I'm more interested in making sure we agree on what constitutes validity.

@ghost
Copy link

ghost commented Sep 13, 2017

What do you usually test with? I am not sure NUnit is happy with constructors, instead it likes you to define [SetUp] and [OneTimeSetup]

Like so:

// Looks correct, no registration
public sealed class Dependency_on_service_with_unknown_arg : ContainerFixture
{
    private sealed class ServiceWithUnknownArg
    {
        public ServiceWithUnknownArg(int arg)
        {
        }
    }

    [Test]
    public void Should_be_invalid()
    {
        Assert.That(Kernel, HasInvalidComponent<ServiceWithUnknownArg>());
    }
}

// Instead of using constructor can we use a [SetUp]? Then this should be good.
public sealed class Dependency_on_service_with_unknown_arg_with_explicit_factory : ContainerFixture
{
    private sealed class ServiceWithUnknownArg
    {
        public ServiceWithUnknownArg(int arg)
        {
        }
    }

	[SetUp]
    public void SetUp()
    {
        Kernel.Register(Component.For<Func<int, ServiceWithUnknownArg>>().AsFactory());
    }

    [Test]
    public void Should_be_valid()
    {
        Assert.That(Kernel, HasValidComponent<ServiceWithUnknownArg>());
    }
}

// Instead of using constructor can we use a [SetUp]? Like so.
public sealed class Dependency_on_factory_of_service_with_unknown_arg_implicit : ContainerFixture
{
    private sealed class ServiceWithUnknownArg
    {
        public ServiceWithUnknownArg(int arg)
        {
        }
    }

    private sealed class ConsumingService
    {
        public ConsumingService(Func<int, ServiceWithUnknownArg> factory) { }
    }
	
	[SetUp]
    public void SetUp()
    {
        Kernel.Register(Component.For<Func<int, ServiceWithUnknownArg>>().AsFactory());
        Kernel.Register(Component.For<ConsumingService>().ImplementedBy<ConsumingService>);
    }


    [Test]
    public void Service_with_unknown_arg_should_be_valid()
    {
        Assert.That(Kernel, HasValidComponent<ServiceWithUnknownArg>());
    }

    [Test]
    public void Consuming_service_should_be_valid()
    {
        Assert.That(Kernel, HasValidComponent<ConsumingService>());
    }
}

public sealed class Dependency_on_factory_of_service_with_unknown_arg_explicit : ContainerFixture
{
    private sealed class ServiceWithUnknownArg
    {
        public ServiceWithUnknownArg(int arg)
        {
        }
    }

    private sealed class ConsumingService
    {
        public ConsumingService(Func<int, ServiceWithUnknownArg> factory)
        {
        }
    }
	
	[SetUp] // And again
    public void SetUp()
    {
        Kernel.Register(Component.For<Func<int, ServiceWithUnknownArg>>().AsFactory());
        Kernel.Register(Component.For<ConsumingService>().ImplementedBy<ConsumingService>);
    }


    [Test]
    public void Service_with_unknown_arg_should_be_valid()
    {
        Assert.That(Kernel, HasValidComponent<ServiceWithUnknownArg>());
    }

    [Test]
    public void Consuming_service_should_be_valid()
    {
        Assert.That(Kernel, HasValidComponent<ConsumingService>());
    }
}

Try altering your test cases and get back to me.

@ghost
Copy link

ghost commented Sep 13, 2017

I did not change all your tests.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 13, 2017

From what I know of NUnit it'll be fine, but I'd want [OneTimeSetUp] if I was to change it.

Here's the base class:

public abstract class ContainerFixture : IDisposable
{
    private readonly IWindsorContainer container;

    protected IKernel Kernel => container.Kernel;

    protected ContainerFixture()
    {
        container = new WindsorContainer();
        container.AddFacility<TypedFactoryFacility>();

        foreach (var nestedType in GetType().GetNestedTypes(BindingFlags.Public | BindingFlags.NonPublic))
            container.Register(Component.For(nestedType));
    }

    protected Constraint HasInvalidComponent<TComponent>() => new ComponentValidityConstraint(typeof(TComponent), isValid: false);
    protected Constraint HasValidComponent<TComponent>() => new ComponentValidityConstraint(typeof(TComponent), isValid: true);

    private sealed class ComponentValidityConstraint : Constraint
    {
        private readonly Type componentType;
        private readonly bool isValid;

        public ComponentValidityConstraint(Type componentType, bool isValid)
        {
            this.componentType = componentType;
            this.isValid = isValid;
        }

        public override ConstraintResult ApplyTo<TActual>(TActual actual)
        {
            if (!((object)actual is IKernel kernel)) throw new ArgumentException("Expected an IKernel", nameof(actual));

            return new ConstraintResult(this, actual,
                isValid != kernel.GetUnresolvableDependencies().Any(_ => _.handler.Supports(componentType)));
        }
    }

    public void Dispose()
    {
        container.Dispose();
    }
}

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 13, 2017

I'll share the work-in-progress extension method on IKernel GetUnresolvableDependencies if you like.

@ghost
Copy link

ghost commented Sep 13, 2017

Yes, if you can point me to a repository I will plumb this into Windsor and debug them for you.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 13, 2017

That's quite awesome of you. Let me move them to a public repo.
Meantime though, do you agree with the logic each time I chose to assert that a class was invalid or valid?
Those asserts were written prior to any implementation and I'm pretty sure I stand by them but I want to discuss if you don't think the same way as me.

@ghost
Copy link

ghost commented Sep 13, 2017

I need to see it in code, I accidentally misread your first test. Just browsing over it very quickly there is nothing jumping up at me. Once I have it in a Windsor branch I definitively tell you more.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 13, 2017

Okay, here it is: https://github.com/jnm2/WindsorConfigValidation

Three tests still fail.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 13, 2017

And obligatory “excuse the mess” while I’m in the middle of sorting out what’s going on in there. Happy to explain what made me write a certain part.

@ghost
Copy link

ghost commented Sep 13, 2017

And obligatory “excuse the mess” while I’m in the middle of sorting out what’s going on in there. Happy to explain what made me write a certain part.

Ha ha, no worries! I have baked your code in and can see the 3 failing tests. Going to sign off for tonight and pick this up first thing tomorrow.

@ghost
Copy link

ghost commented Sep 14, 2017

Hi

I had a further look at this and have managed to bake you extension and the tests into a branch of windsor. I raised a PR back my forked master to give you a tidy diff of what I did so far. You can see it here

The first thing to note is that I changed your extension from using the IKernel to using the IWindsorContainer. The reason I did this was because I found some already coded goodness in the diagnostics namespace there. I managed to reduce your extension method to this:

public static IReadOnlyCollection<(DependencyModel dependency, IHandler handler)> GetUnresolvableDependencies(this IWindsorContainer container)
{
	var unresolvables = new List<(DependencyModel dependency, IHandler handler)>();
	var allHandlers = container.Kernel.GetAssignableHandlers(typeof(object)).ToList();
	var waitingHandlers = allHandlers.FindAll(handler => handler.CurrentState == HandlerState.WaitingDependency);
	foreach (var waitingHandler in waitingHandlers)
	{
		foreach (var dependency in waitingHandler.MissingDependencies)
		{
			unresolvables.Add((dependency, waitingHandler));
		}
	}
	return unresolvables.AsReadOnly();
}

I did have to make a change to the IHandler interface to expose MissingDependencies from the AbstractHandler. The only thing that implements the IHandler directly is the ParentHandlerWrapper which is only used in parent/child container scenarios. Not sure if you want to support this.

The DependencyModelInspector gives away a few clues about how the different resolution types would work but I am still not sure how this would play into the testing here.

This then starts causing tests you would expect to pass to fail, I think this is because we might need to make the tests smarter about what the IWindsorContainer.Resolve method would expect to resolve correctly. For example:

public sealed class Dependency_on_service_with_unknown_arg_with_explicit_factory : ContainerFixture
{
	private sealed class ServiceWithUnknownArg
	{
		public ServiceWithUnknownArg(int arg)
		{
		}
	}
	public Dependency_on_service_with_unknown_arg_with_explicit_factory()
	{
		Container.Register(Component.For<Func<int, ServiceWithUnknownArg>>().AsFactory());
	}
	[Test] 
	public void Should_be_valid()
	{
		Assert.That(Container, HasValidComponent<ServiceWithUnknownArg>());
	}
}

This test fails because it is expecting an Int32 value. So perhaps this should be marked as Invalid.

image

If you continue your effort's by forking this branch I am sure you eventually end up with what you are looking for and be able to raise a PR to master.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 14, 2017

Okay, cool. Now here's my need which is driving the rationale of marking ServiceWithUnknownArg as valid in the test you mentioned. There are almost three levels of validity for a service:

  1. This service can be resolved directly with no additional arguments.
    • Valid for container.Resolve()
    • Valid as a direct dependency of another service
    • Valid if produced via typed factories
  2. This service requires ever-changing runtime arguments
    • Valid if not resolved or depended upon directly but only via typed factories whose signatures require those arguments
  3. The service requires arguments which aren't registered, and there either are no typed factories through which to supply that argument or a typed factory does exist which fails to supply that argument
    • Invalid

Half my services are in category 2. If these tests don't make the useful distinction between categories 2 and 3, I will always have dozens and dozens of failures. I need category 2 to be considered valid or I won't be able to pick out actual misconfiguration above all the noise. I know this because I've been there and tried it.

My application only uses a single .Resolve<MainApp>() and all dependencies are constructor injected for the rest of the entire tree. So the only service that actually needs to be in category 1 is MainApp. Everything else may be category 2 and be perfectly valid.

What I need to detect as failures are services that I intended to be in category 2 but accidentally went to category 3 due to requiring a runtime argument,

  • when the service is used as a direct (non-factory) dependency of another service,
  • or, when there is no explicitly registered or implicitly discoverable typed factory for the service
    (Explicitly registered means using .AsFactory(), implicit means that the delegate or interface appears as a dependency of some other service)
  • or finally, when there is an explicitly registered or implicitly discoverable typed factory that does produce the service in question but does not require the service's runtime argument.
    Of course, the typed factory does not need to require arguments that the container knows how to resolve, only runtime arguments which can only be known at the call site which uses the typed factory.

Notice that to validate category 2, it is actually counter-productive to think in terms of whether you could resolve the service directly from the container with no additional arguments. That's not what category 2 is for, and category 2 is an unavoidable characteristic of much of the application.

Does that make sense?

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 14, 2017

If that makes sense, then I'm not interested in testing whether each service can be resolved but actually I'm interested in following the three criteria bullets at the end of my last comment.

That means I need to be able to get the IWindsorContainer to tell me about:

  • All explicitly registered typed factories (which appear at registration time with .AsFactory())
  • All implicitly discoverable typed factories (which only appear as constructor arguments of other services)
  • For each typed factory, which services it produces and what arguments it requires.

There doesn't seem to be an API to discover any of these three things which is what most of the stuff you removed was straining to achieve. Namely, this code tries to index the arguments which the factory requires by the type of service the factory returns:

https://github.com/jnm2/WindsorConfigValidation/blob/4262387cf59f637fce95db25cba5f07d65446af4/src/WindsorConfigValidation/KernelUtils.cs#L28-L76

And this code tries to figure out if the dependency which can't be otherwise resolved is required by all explicit and implicit typed factories, using that indexed information:

https://github.com/jnm2/WindsorConfigValidation/blob/4262387cf59f637fce95db25cba5f07d65446af4/src/WindsorConfigValidation/KernelUtils.cs#L104-L120

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 14, 2017

Sorry, I'm not seeing the branch you pushed here or at fir3pho3nixx/Windsor. Once you point it out to me I'll fork the branch and see how far I get in achieving these goals, if you don't see a problem with my rationale above.

@ghost
Copy link

ghost commented Sep 14, 2017

Was checking to see if you can do this with GraphNodes on the IKernel by scanning Dependencies that have a dependency key of "factory" but it almost feels like much of a muchness. It also does not encapsulate any state about whether it can be resolved or not. So it feels like you are heading in the right direction.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 14, 2017

Oh, found it from the PR: https://github.com/fir3pho3nixx/Windsor/tree/validate-configuration
Missed the scrollbar in the dropdown, lol!

@ghost
Copy link

ghost commented Sep 14, 2017

Forget my branch, sorry I was not more helpful!

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 14, 2017

Well, if I'm already headed the right direction, my hope is that you and I will be able to work out a nice API to interrogate the typed factory configuration once we figure out what the code is that makes the all the tests pass.

Having the tests in the branch helps because I can edit Windsor code too now. 😈

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 14, 2017

fir3pho3nixx is a bit intense for a git remote name; I think I'll call it fire. 😄

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 14, 2017

What do you think of the new APIs in https://github.com/jnm2/Windsor/commits/validate-configuration?

namespace Castle.Facilities.TypedFactory
{
    public static class TypedFactoryAnalysis
    {
        public static IReadOnlyCollection<TypedFactoryInfo> GetAllTypedFactories(this IWindsorContainer container);
    }
    public sealed class TypedFactoryInfo
    {
        public IReadOnlyCollection<ComponentModel> Dependents { get; }
        public Type FactoryType { get; }
        public bool IsExplicitlyRegistered { get; }
        public IReadOnlyCollection<TypedFactoryResolveMethod> ResolveMethods { get; }
        public TypedFactoryInfo(Type factoryType, bool isExplicitlyRegistered, IReadOnlyCollection<ComponentModel> dependents, IReadOnlyCollection<TypedFactoryResolveMethod> resolveMethods);
        public override string ToString();
    }
    public sealed class TypedFactoryResolveMethod
    {
        public Type ComponentType { get; }
        public DependencyModelCollection Dependencies { get; }
        public MethodInfo Method { get; }
        public override string ToString();
    }
}

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 14, 2017

Next I'm going to consume that API in ConfigurationValidationExtensions.

@ghost
Copy link

ghost commented Sep 14, 2017

Looks good from the outside, the VisitModelHierarchy is still a bit of a busy bee :)

Like the TypeFactoryInfo ... nice touch :)

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 14, 2017

@Fir3pho3nixx Yes it is too busy– per the TODOs I'm hoping to see what code it duplicates in the internals of the typed factory runtime resolve logic, and move that logic out into a new internal class. Then I'll be able to call that class instead of large chunks of what's currently in VisitModelHierarchy and we can simplify from there.

Thanks. I separated TypedFactoryInfo from TypedFactoryResolveMethod because you don't want breaking changes down the road if someone starts wanting to see release methods or some other info.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 15, 2017

Thinking out loud.

Definitions

Runtime parameter: a parameter which Castle Windsor cannot satisfy. Can only be satisfied through typed factories requiring the parameter in the resolve method.

Validation

A runtime parameter is valid IFF there is at least one explicit or implicit typed factory resolve method which requires the parameter.

An explicit or implicit typed factory is valid IFF each resolve method on the factory includes as method parameters all runtime parameters to the type it returns.

A service is valid IFF it contains no invalid runtime parameter AND it has no direct dependency to another VALID service which has a runtime parameter.

(I don't think it makes sense to consider a service invalid if it depends on an invalid typed factory unless the typed factory's signature is being declared by the service's usage of the type, for example the Func<...> family. But for custom delegates and interfaces, the dependent service's usage of the type is often not coupled to the signature so it seems unnecessary to also consider the dependent service invalid just because the typed factory is invalid. Even if we did consider it invalid due to the typed factory, we wouldn't include that in a diagnostic message because that information is already in the typed factory's diagnostic message.)

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 15, 2017

Diagnostic messages

This section will be useful to figure out what data needs to be aggregated in order to report diagnostics.

Invalid runtime parameters

If a runtime parameter is invalid AND there are no typed factories which return the service type, a message should be produced containing information similar to:

In order for {service type} to depend on {parameter type} {parameter name}, either the
dependency must be registered in the container, or a typed factory must be explicitly
registered or implicitly used which requires a {parameter type} {parameter name} argument
in order to resolve each instance.

If there are factories, separate messages will show up for the invalid ones.

Invalid typed factories

If a typed factory is invalid, a message should be produced containing information similar to:

In order for typed factory {typed factory type} to return {service type}, either these
dependencies must be registered in the container, or the typed factory must require these
runtime parameters in order to resolve each instance:
 - {parameter 1 type} {parameter 1 name}
 - {...}

The typed factory is {registered explicitly and also} implicitly used as a dependency of:
 - {dependent service 1 type}
 - {...}

Invalid services

If a service is invalid for any reason OTHER than having an invalid runtime parameter (which we already thoroughly cover in its own message), a message should be produced containing information similar to:

In order for {dependent service type} to depend on {dependency service type}, either these
dependencies must be registered in the container, or {dependent service type} must replace
the direct dependency with a dependency on a typed factory returning {dependency service
type} which requires these runtime parameters in order to resolve each instance:
 - {parameter 1 type} {parameter 1 name}
 - {...}

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 15, 2017

So what should the intermediate representation of the validation look like for consumption by people's unit tests?

I can imagine wanting a result back per service, whether valid or invalid, to drive a TestCaseSource. On the other hand I'm not sure that provides a huge benefit unless you're overwhelmed with error messages and want to read one at a time; even so, not sure it's worth anything fancy.

It maybe doesn't have to be any fancier than:

public static class ConfigurationValidationExtensions
{
    public static IReadOnlyCollection<string> GetValidationErrors(this IWindsorContainer container);
}

Though that's not very future-proof. We might want to return something service-centric or dependency-centric rather than message-centric. Something which you can examine in a language-independent way.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 15, 2017

@Fir3pho3nixx What's your impression of this API?

namespace Castle.Windsor.Diagnostics
{
    public static class ConfigurationValidationExtensions
    {
        public static IReadOnlyCollection<ServiceValidationInfo> GetValidationInfo(this IWindsorContainer container);
    }
    public sealed class ServiceValidationInfo
    {
        public IReadOnlyCollection<ServiceValidationInfo> DirectDependenciesLackingRuntimeParameters { get; }
        public IReadOnlyCollection<TypedFactoryInfo> ReturnedByTypedFactories { get; }
        public IReadOnlyCollection<DependencyModel> RuntimeParameters { get; }
        public Type ServiceType { get; }
        public IReadOnlyCollection<TypedFactoryInfo> TypedFactoriesLackingParameters { get; }

        public ServiceValidationInfo(
            Type serviceType,
            IReadOnlyCollection<DependencyModel> runtimeParameters,
            IReadOnlyCollection<TypedFactoryInfo> returnedByTypedFactories,
            IReadOnlyCollection<TypedFactoryInfo> typedFactoriesLackingParameters,
            IReadOnlyCollection<ServiceValidationInfo> directDependenciesLackingRuntimeParameters);

        public IEnumerable<string> GetErrorMessages();
        public override string ToString();
    }
}

https://github.com/jnm2/Windsor/blob/validate-configuration/src/Castle.Windsor/Windsor/Diagnostics/ServiceValidationInfo.cs

@ghost
Copy link

ghost commented Sep 15, 2017

Definitions

Runtime parameter: a parameter which Castle Windsor cannot satisfy. Can only be satisfied through typed factories requiring the parameter in the resolve method.

This is based on abstract facilities. What have you considered UsingFactoryMethod in ComponentRegistration.cs?

Validation

A runtime parameter is valid IFF there is at least one explicit or implicit typed factory resolve method which requires the parameter.

I am very sure there are more paths into this, have you considered IContributeComponentModelConstruction?

An explicit or implicit typed factory is valid IFF each resolve method on the factory includes as method parameters all runtime parameters to the type it returns.

A service is valid IFF it contains no invalid runtime parameter AND it has no direct dependency to another VALID service which has a runtime parameter.

I understand what you are trying to achieve here but I think we need @jonorossi's help. If I add a custom ISubDependencyResolver which would potentially break this. I am sure there are way more abstractions lying around which might break this, I just dont know them all ...

I am going to bail out here for now and wait for your response. I really appreciate what you are trying to do here BTW! :)

@ghost
Copy link

ghost commented Sep 15, 2017

@Fir3pho3nixx What's your impression of this API?

I like it. Although I think we should try and adhere to the open/closed principal. Sealing ServiceValidationInfo makes it vulnerable to modification. Also TypedFactoryInfo might morph into an abstract once we hear back from @jonorossi. I can already think of 2. I am sure there are way more.

Your immediate goal is testing resolvable determinism in the container. I can't speak for all the conditions, I think @jonorossi can help massively with this. In the meantime look at the abstractions I mentioned and let's talk it out. Happy to help investigate this.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 15, 2017

Great, I was hoping you'd bring stuff like that to my attention. 😃

On UsingFactoryMethod, I do use that in projects. That's a case where Windsor can satisfy the dependency, and therefore the dependency is not a runtime parameter as I defined it. I may need a more specific name than "runtime parameter" to capture what I'm talking about. It's any parameter which is unresolvable without either passing additional arguments to Resolve, or passing them to a typed factory.

Not sure what you mean by "This is based on abstract facilities," can you clarify?

I'll look at IContributeComponentModelConstruction.

With that and with ISubDependencyResolver, I expect that you and @jonorossi will have to help me see where this whole thing fits in the wider landscape. Maybe it should only be a typed-factory-specific diagnostic, or maybe with your guidance we can keep it flexible enough to extend to other scenarios.
You understand my goal of verifying determinism via automated tests, so however you think is best to arrive there is fine with me.

Sealing ServiceValidationInfo makes it vulnerable to modification.

I can see your point if we are going to flesh this out and typed factory validation ends up as only part of that.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 15, 2017

So I'll wait over here until @jonorossi reviews.

@ghost
Copy link

ghost commented Sep 15, 2017

Not sure what you mean by "This is based on abstract facilities," can you clarify?

Sure, abstract facilities encapsulate IConfiguration and IKernel.

IConfiguration is mostly immutable. This represents the intent of the consumer configuration, things like "I am a public consumer, this is my input".

IKernel is based on the MicroKernel implementation. So IContributeComponentModelConstruction is something that contributes to building up component models that feed into the graph node configuration of the container. This answers the question of "How does my configuration map to a dependency resolution graph and what do I have to build the objects inside it?". It does not give anything away about whether this is possible or not. This purely looks like input state management to me with mutable abstractions.

The ISubDependencyResolver is a way of overriding resolve logic. This represents the overriding intent from the initial state configuration and answers questions like "I see what you want and I am going to stick my hand up and resolve this for you". In effect this could override/extend the MicroKernel resolution process.

So the lifecycle of the container is (based on my naive point of view):

Facilities -> { <- These encapsulate Config/MicroKernel }
    Configuration -> { <- User Input }
    Kernel -> { <- Microkernel }
        IContributeComponentModelConstruction { <- Implements Config }
    ** !! Resolve !! **
        ISubDependencyResolver { Overrides resolution }

I am trying to work out where best we put this, and how we deal with side effects, this container is mega super extensible. If you do it at a facility level, then we have to rip the extension out of MicoKernel.Diagnostics and place it in the typed factory facility. Although resolution determinism would be akin to solving the P vs NP problem in terms of complexity.

@ghost
Copy link

ghost commented Sep 16, 2017

At the moment you are trying a TypeFactory inside out approach. Would you be willing to try DIP where we try and brute force resolution from the outside in?

@ghost
Copy link

ghost commented Sep 16, 2017

This makes your concerns valid and adds a utility which is awesmo for testing. I am thinking Castle.Facilities.Testing. Thoughts?

@ghost
Copy link

ghost commented Sep 17, 2017

@jnm2 - Please weigh in here. We are thinking of deprecating the FactoryFacility. #339

@jonorossi
Copy link
Member

I haven't weighted in until now because I was interested to see how far you got because this would be cool, but I really don't think this will ever be sanely possible with so many extensibility points, with Windsor things are never deterministic, which is why the current diagnostics says "potentially misconfigured" for the few things it checks. If you threw an ISubDependencyResolver in your container (even something basic like ArrayResolver) it could provide any sort of dependency you wouldn't know about unless you ran that code.

The docs have an overview page for Windsor's extension points more of which can affect this functionality:
https://github.com/castleproject/Windsor/blob/master/docs/extension-points.md

The docs also call out that the diagnostics are not 100% accurate, sometimes listing false-positives or not listing false-negatives:
https://github.com/castleproject/Windsor/blob/master/docs/debugger-views.md#potentially-misconfigured-components

I think the only sane way to test your configuration is with integration tests which runs the actual code using any required external configuration, otherwise you'd be increasing Windsor's complexity with questionable benefit since it wouldn't ever be accurate.

@jnm2 what is the ultimate use case for this, are you expecting your unit tests to always report the right result or that you have to hard code a whole bunch of things to ignore?

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 20, 2017

@Fir3pho3nixx

Would you be willing to try DIP where we try and brute force resolution from the outside in?

Yes. More on that below.

We are thinking of deprecating the FactoryFacility

I don't believe I've used Castle.FactorySupportFacility; I've used UsingFactoryMethod. I absolutely can't manage without the TypedFactory built-in facility though. So you aren't thinking of deprecating anything relevant to this particular thread?

@jonorossi

what is the ultimate use case for this, are you expecting your unit tests to always report the right result or that you have to hard code a whole bunch of things to ignore?

I do not expect to hardcode anything to ignore, ultimately. I can manually look at the whole graph and figure out if there is an unavoidably erroneous configuration; in that sense, everything is deterministic. If not everything, at least most things.

@jonorossi + @Fir3pho3nixx

If I can mentally apply certain rules to discover absolute failures, unit tests can too. In the worst case scenario, Castle Windsor can't accept any of my diagnostics (deterministic as they are) because they are too specific to my needs. In this scenario, I still ask that you expose any API necessary for me to make my own determinations of validity. For example, something such as TypedFactoryInfo. The focus for Castle Windsor is then not validation but simply analysis; reflection, in a general sense.

Determinism is a favorable property. I'm not convinced that it wouldn't ever be accurate. What if there was an iterative approach where each facility and sub resolver is responsible, if they so choose, to provide validation which parallels the resolution they provide?
Say a facility participates in resolution but doesn't (yet) participate in validation because no one needs it yet. I will have to choose between either not using that other facility until the validation logic is contributed, or manually whitelisting all dependencies which rely on that facility's resolution logic, or just temporarily disabling deterministic tests and rely on integration tests.

@jnm2
Copy link
Contributor Author

jnm2 commented Sep 20, 2017

I don't even think it would be that challenging to design the validation system. It would contribute to static analysis with the same flow that dependency resolution currently uses.

What stood out to me that I dislike about the current implementation of the typed factory facility is that it's basically all implementation detail. Right now there's no representation of the configured model as such; it's merely a forensic trail which can't be reverse engineered with full fidelity. That's not like Windsor in general. If that were to change, I think we would find static analysis to be quite as tractable as it is in plain Windsor.

@jonorossi
Copy link
Member

In this scenario, I still ask that you expose any API necessary for me to make my own determinations of validity. For example, something such as TypedFactoryInfo. The focus for Castle Windsor is then not validation but simply analysis; reflection, in a general sense.

Sounds like a good idea, I see no reason Windsor shouldn't provide access to a model for the typed factory facility.

If there was an iterative approach where each facility and sub resolver is responsible, if they so choose, to provide validation which parallels the resolution they provide?

I guess you could probably use ISubDependencyResolver.CanResolve, obviously I haven't looked through other extension points to know what else is possible.

What stood out to me that I dislike about the current implementation of the typed factory facility is that it's basically all implementation detail. Right now there's no representation of the configured model as such; it's merely a forensic trail which can't be reverse engineered with full fidelity. That's not like Windsor in general. If that were to change, I think we would find static analysis to be quite as tractable as it is in plain Windsor.

This sounds like the next step, getting the information you need visible in Windsor's public API, that'll allow you to continue working on fleshing out what you can do for different extension points without hacking Windsor.

@ghost
Copy link

ghost commented Sep 22, 2017

I don't believe I've used Castle.FactorySupportFacility; I've used UsingFactoryMethod. I absolutely can't manage without the TypedFactory built-in facility though. So you aren't thinking of deprecating anything relevant to this particular thread?

Most definitely not. Sorry about the false negatives.

Determinism is a favorable property

I agree, and I think I have just cracked open generics in the MicroKernel DefaultDependencyResolver resolve process. Please see: #343

I think some of this stuff should be made common between the CanResolve and Resolve logic in the DefaultDependencyResolver. This is problematic though. Resolve carries with it a CreationContext which allows it to infer generic parameters to open generic types for handlers. You simply don't have this context during static CanResolve time for debugger views. We need some kind of re-entrant way of discovering resolvable types in these debugger views from what we have learn't here.

@ghost
Copy link

ghost commented Oct 9, 2017

@jnm2 - I have just implemented 3 facilities which are sitting in a PR, I must say it really jumped out at me that we probably need a facility for testing. The tests in Windsor are rather yukky and need some love. Instead of trying to bake this into the MicroKernel, can we start a facility up which I can add handy testing utilities to start reducing the boilerplate in the test library?

@jnm2
Copy link
Contributor Author

jnm2 commented Oct 9, 2017

That sounds like a good idea, so long as the test facility would be of interest beyond the Castle Windsor codebase and so long as we aren't compromising the possibility of having all facilities participate in static analysis describing how they resolve things. Would they become coupled to the test facility?

@ghost
Copy link

ghost commented Oct 9, 2017

@jnm2 - Yes, this would start a learning exercise which would start tackling the bigger problem of resolution determinism(we start with TypedFactories first). We need to start somewhere. Propagating this to all facilities via extensions is definitely on the table.

Also I was looking at this problem and it is mental. I made the test pass here which felt hacky. It did expose a thought though, the MicroKernel internals need to start giving out more information. This is the biggest problem right now.

There is this massive wall between registration and resolve. And In some cases resolve will do it anyway(it just evolved that way). I think we need to make registration smarter (#343).

We need a change the IHandler interface which exposes a readonly property that describes Missing Dependencies.

Then we need to start looking at how we classify these missing dependencies. An example would be an open generic registration (you know IFoo<T> where T is not registered). Another would be a lazy loaded closed generic.

I think then we will get closer to the answer. We test the facility out by cleaning up the tests and then once we feel this has achieved it's goal, we can make it mainstream.

Sounds simple but there is a lot of work here.

@ghost
Copy link

ghost commented Oct 9, 2017

If you are OK with this, I would like to submit a PR for the IHandler change to start. I would like to see how this helps you're efforts.

@jnm2
Copy link
Contributor Author

jnm2 commented Oct 9, 2017

Absolutely, that sounds good. I'm catching up on my backlog, so don't let me hold you back.

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