ipjohnson / Grace

Grace is a feature rich dependency injection container library
MIT License
337 stars 33 forks source link

Problem with Locate and wrappers #104

Closed darkcamper closed 6 years ago

darkcamper commented 7 years ago

Hi @ipjohnson, how are things going?

I've found a possible incosistency between locating directly a wrapper (such a Func<> that wraps a non exported abstract type), and locating a type that has a dependency on the same wrapper. The former works as expected, but the latter throws a LocateException.

I found this while trying a WrapperStrategy I wrote for the Option<> type) but it seems to happen also with the built-in wrappers (I've tested with Func)

I've uploaded a GIST containing an example of this.

Is there something I'm missing? Thanks!

ipjohnson commented 7 years ago

Hi @darkcamper

Things are good can't complain.

The example you have is kind of interesting both should fail and ultimately do (when the func is executed it throws that it can't find the abstract) but not at the same place.

What's the behavior you are expecting that both locate should fail (The abstract can never be instantiated)?

darkcamper commented 7 years ago

I would expect both would succeed initially, and then when the Func<> is called, it would fail.

The wrapper I'm currently writing (OptionWrapperStrategy) is a means of letting a constructor expose that "it's okay if you can't provide an instance of this dependency, it's really optional". For instance, I could declare on a repository class that it may use a Validator before persisting the entity, but if there is not a validator available nothing wrong will happen . Something like this:

class AClass
{
    public AClass(Option<AbstractValidator<Entity>> optionalDependency)
    {
          //Do something depending on optionalDependency having an internal 
          //AbstractValidator<Entity>instance or not.
    }
}

If "Locate" always fails without giving a chance to OptionWrapperStrategy to work its magic I've no idea how you could implement this functionality.

ipjohnson commented 7 years ago

Hmmm in the past I've handled optional parameters one of three ways.

1) When registering the export you mark the parameter as IsRequired(false) 2) If you have control over the constructor you can set the default value to null

class AClass
{
    public AClass(AbstractValidator<Entity> optionalDependency = null)
    {
          // default value null will be used if validator does not exist
    }
}

3) Take in an IEnumerable<AbstractValidator<Entity>> then iterate over the provided validators (I usually prefer this as it allows you to provide small fine grained validators instead of one large one)

I do agree it's not the most consistent behavior and it would probably be best to make it configurable if Func<T> is unknown it can still be returned. I'll see when I can get to it in the coming weeks as I have some work I agreed to do for BenchmarkDotNet and we've got a new born at home that takes up most of my time these days :)

darkcamper commented 7 years ago

Hehehe, newborns require indeed a lot of time (but it's time well spent!)

I agree with you that making the behavior of locating Wrapper, when T is unknown for the container, configurable would be the best solution. In the meanwhile I'll change my constructors to use null default values, instead of the Option type, as you suggested.

If you need some help testing the changes (when you find some spare time to implement them) just leave me a message. I'll be glad to help.

Thanks a lot!

ipjohnson commented 7 years ago

@darkcamper I thought about it a bit more and I think with a couple small tweaks to the code base a OptionStrategy would be pretty easy to implement.

I'm going to open a couple issue to cleanup some small things that will make it easier to implement (Option<T> works now but Option<int> does not).

darkcamper commented 7 years ago

@ipjohnson Sounds promising! I'll be following the issues you just opened

ipjohnson commented 7 years ago

@darkcamper I put together an example strategy and tests for Option<> here.

Do you want to try out the nightly nuget packages https://ci.appveyor.com/nuget/grace-master (says master but has dev as well)

darkcamper commented 7 years ago

@ipjohnson I tried the last nuget + your Option strategy example, and it works really well! My application is running again with Option<> dependencies :smile:

I know you're really short on time and I really appreciate the effort you put in to make these changes.

ipjohnson commented 7 years ago

I'm going to look at trying to roll out a release this weekend. Everything working well?

darkcamper commented 7 years ago

Yeah! I've being using the dev nuget package since you released it, and found no problems at all

ipjohnson commented 7 years ago

@darkcamper just heads up for 6.3.0 the signature for IMissingExportStrategyProvider has changed so you will need to update your Optional<T> solution.

Thinking about it a bit more you can just add your strategy directly to the container and skip the missing strategy provider

container.Configure(c => c.AddActivationStrategy(new OptionalStrategy(c.OwningScope)));
ipjohnson commented 6 years ago

I'm going to close out this issue