False positive validation error when using service setup with certain conditions

[Metadorius]

Same background as in #576, registering a logger factory with two resolution routes, but now another issue (I bandaid fixed one in #576 for now so ignore it in context of this issue):

var container = new Container()
    .WithMefAttributedModel();

var serilogLogger = new LoggerConfiguration()
    /* ... */
    .CreateLogger();

var loggerFactory = new SerilogLoggerFactory(serilogLogger);

container.RegisterInstance<ILoggerFactory>(loggerFactory);

container.Register(
    Made.Of(_ => ServiceInfo.Of<ILoggerFactory>(),
        f => f.CreateLogger(/*categoryName:*/ null!)),
    setup: Setup.With(condition: r => r.Parent.ImplementationType == null));

container.Register(
    Made.Of(_ => ServiceInfo.Of<ILoggerFactory>(),
        f => f.CreateLogger(Arg.Index<Type>(0)),
        r => r.Parent.ImplementationType),
    setup: Setup.With(condition: r => r.Parent.ImplementationType != null));

Upon calling container.ValidateAndThrow() I am met with:

DryIoc.ContainerException: code: Error.UnableToResolveFromRegisteredServices;
message: Unable to resolve resolution root Microsoft.Extensions.Logging.ILogger {ServiceKey=DefaultKey(1)}
  from container without scope
 with Rules with {TrackingDisposableTransients, UsedForValidation} and without {ThrowOnRegisteringDisposableTransient, ImplicitCheckForReuseMatchingScope, EagerCachingSingletonForFasterAccess} with TotalDependencyCountInLambdaToSplitBigObjectGraph=2147483647
 with DefaultReuse=Singleton {Lifespan=1000}
 with Made={FactoryMethod=<custom>, PropertiesAndFields=<custom>, ParameterSelector=<custom>}
  with normal and dynamic registrations:
  (DefaultKey(0), {FactoryID=677, HasCondition})  (DefaultKey(1), {FactoryID=678, HasCondition})
   at DryIoc.Throw.It(Int32 error, Object arg0, Object arg1, Object arg2, Object arg3) in /_/src/DryIoc/Container.cs:line 14538
   at DryIoc.Container.TryThrowUnableToResolve(Request request) in /_/src/DryIoc/Container.cs:line 927
   at DryIoc.Container.DryIoc.IContainer.ResolveFactory(Request request) in /_/src/DryIoc/Container.cs:line 911
   at DryIoc.ContainerTools.Validate(IContainer container, ServiceInfo[] roots) in /_/src/DryIoc/Container.cs:line 4460

It seems that the validation routine creates a resolution request with null .Parent.ImplementationType which makes the request to resolve the second registration of ILogger fail because of unfulfilled condition, because if you remove the second registration's condition it is able to resolve it just fine.

[dadhi]

@Metadorius Interesting, will check. Thanks for the code.

[dadhi]

@Metadorius This happens to be an interesting problem.

We have two logger registrations, and when calling the Validate without the roots parameter,
it will try to resolve all available registrations from the root.

The resolution of the logger with condition r => r.Parent.ImplementationType == null will succeed.
But the resolution of the logger with the opposite condition will not.

The problem is that Validate does not know about the context, that the conditions are complementary to each other and resolving the first as a root is fine, but the second is not.

So in a way, Validate output is expected - you ask to validate all the roots and one of them is invalid because it is not supposed to be the root -> so "you are the problem, not me!" :-)

You may, and probably should, narrow the roots by passing the specific list of the service types, or service info or the registration condition, like this ValidateAndThrow(typeof(ILogger)). In this case, Validate will try to resolve what you give him, so it will resolve ILogger service once, and it will succeed as expected using the correct registration.

I will add this advice to the validation error message.

[Metadorius]

Hmm, yeah an interesting issue indeed.

You may, and probably should, narrow the roots by passing the specific list of the service types, or service info or the registration condition, like this ValidateAndThrow(typeof(ILogger)). In this case, Validate will try to resolve what you give him, so it will resolve ILogger service once, and it will succeed as expected using the correct registration.

I see, will check. I wonder if it's possible to exclude certain registrations from validation though, because I think right now I need to either have a big whitelist or make some validation condition (or maybe supply a "blacklist" of sorts).

[Metadorius]

Something like:

container.Register(
    Made.Of(_ => ServiceInfo.Of<ILoggerFactory>(),
        f => f.CreateLogger(Arg.Index<Type>(0)),
        r => r.Parent.ImplementationType),
    setup: Setup.With(condition: r => r.Parent.ImplementationType != null,
        excludeFromValidation: true));

[Metadorius]

On another thought, the "validation roots whitelist" approach (if I correctly understood it) would also not work good with MEF attributed model which I use, because I don't know the service types beforehand and would need to somehow retrieve them.

[dadhi]

@Metadorius You may exclude via metadata or vice versa, include via metadata.
I think, this will work for the MEF model too. I even have the AsResolutionRootAttribute in the DryIocAttributes for this matter.

[Metadorius]

Is this the thing?

            container.Register(
                Made.Of(_ => ServiceInfo.Of</*ILoggerFactory*/_SerilogLoggerFactory>(),
                    f => f.CreateLogger(Arg.Index<Type>(0)),
                    r => r.Parent.ImplementationType),
                setup: Setup.With(condition: r => r.Parent.ImplementationType != null,
                    asResolutionRoot: false));

Because when I use it - it still throws the same exception. I also don't see exclusion of AsResolutionRoot == true services in the Validate, only see open generics by default.

[dadhi]

Is this the thing?

Nope, it is not working for the Validate, which maybe confusing. Will look into it.
Meanwhile, I have pushed the example with the metadata.

[Metadorius]

Thanks much, checked it out!

What I thought of:

        /// <summary>Excluding open-generic registrations and non-root registrations, cause you need to provide type arguments to actually create these types.</summary>
        public static bool DefaultValidateCondition(ServiceRegistrationInfo reg) => !reg.ServiceType.IsOpenGeneric() && reg.AsResolutionRoot;

        /// <summary>Helps to find potential problems in service registration setup. Method tries to resolve the specified registrations, collects exceptions, 
        /// and returns them to user. Does not create any actual service objects. You must specify <paramref name="condition"/> to define your resolution roots,
        /// otherwise container will try to resolve all registrations, which usually is not realistic case to validate.</summary>
        public static KeyValuePair<ServiceInfo, ContainerException>[] Validate(this IContainer container, Func<ServiceRegistrationInfo, bool> condition = null)
        {
            var conditionWithDefault = condition == null
                ? (Func<ServiceRegistrationInfo, bool>)DefaultValidateCondition
                : (r => condition(r) && DefaultValidateCondition(r));

            var roots = container.GetServiceRegistrations().Where(conditionWithDefault).Select(r => r.ToServiceInfo()).ToArray();
            if (roots.Length == 0)
                Throw.It(Error.FoundNoRootsToValidate, container);

            return container.Validate(roots);
        }

(can PR that if you want)

Reiterated on my understanding of the issue at hand, I now understand what's resolution root better, and it seems to me that it makes more sense this way, considering usually you don't have many resolution roots (tho for my application where I resolve the module pages manually this isn't the case, but generally it is) so I didn't change the default. Also the parameterless call to Validate doesn't seem to make much sense, and I think having people manually specify resolution roots makes more sense in the general scenario (may also adjust the error message in this case). Though I wonder if it's already possible to flip this switch of "everything/nothing is root by default" already, vaguely remember somehting like this.

Edit: actually not sure, maybe it's better to have everything be resolution root by default.

[dadhi]

It breaks the default behavior. I would rather filter all registrations based on the IsResolutionRoot, then if nothing found fallback to the current behavior.

[Metadorius]

Hmm you're right, not sure how to deal with it best. Either way I think that's a topic for another issue, the error explanation amendment now gives a push in the correct direction so I think we can close this issue. Thanks for the great work :)