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

ILocatorService.Locate using 'withKey' doesn't work for wrapped types. #185

Closed
eglauko opened this issue Aug 8, 2018 · 11 comments
Closed
Labels

Comments

@eglauko
Copy link
Contributor

eglauko commented Aug 8, 2018

I have a class named OptionalWrapperStrategy (for Optional) extending BaseWrapperStrategy, and implementing IMissingExportStrategyProvider, IMissingDependencyExpressionProvider.

I can inject Optional with keys in properties, construtor parameters, and methods parameters.

If i try locate Optional using ILocatorService.Locate works fine, but if i try use 'withKey' an exception is thrown, and the methods of OptionalWrapperStrategy are not called.

@ipjohnson
Copy link
Owner

Could you include your optional wrapper implementation and I can take a look this evening?

@eglauko
Copy link
Contributor Author

eglauko commented Aug 8, 2018

Optional.zip

@ipjohnson ipjohnson added the bug label Aug 9, 2018
@ipjohnson
Copy link
Owner

I've been able to replicate this problem in my environment so I can fix it. The one thing I will say is that this resolution won't be fast. Grace only caches delegates for non keyed exports, which wouldn't be a problem because it would be cached at the export itself but it's a wrapped/keyed export which won't be cached.

I can fix the problem for version 6.4.1 but I won't be fast. I think caching will need to be a different issue and I'd want to release version 6.5.0 for it as it's a major feature.

@eglauko
Copy link
Contributor Author

eglauko commented Aug 9, 2018

Let me ask something: The IMissingDependencyExpressionProvider is used when i call the locate method?

My wrapper implements also the IMissingDependencyExpressionProvider, and I'm configuring this way:

var container = new DependencyInjectionContainer(GraceDynamicMethod.Configuration());
container.Configure(c =>
{
    var optionalStrategy = new OptionalWrapperStrategy(c.OwningScope);
    c.AddActivationStrategy(optionalStrategy);
    c.AddMissingDependencyExpressionProvider(optionalStrategy);
});

I realized that the IMissingDependencyExpressionProvider methods were not called from locate. Maybe using this strategy would solve the problem.

@ipjohnson
Copy link
Owner

@eglauko you are correct the IMissingDependencyExpressionProvider methods weren't being called because you were registering it as a strategy all that said I think you've stumbled on a bug that is going to be hard to fix without making a change to the IWrapperOrExportActivationStrategy interface.

After doing some digging the problem is going to affect all Wrapped/Keyed Locate requests. Essentially the GetActivationStrategyDelegate method needs to have key added to it but this will be a breaking change that I don't want to introduce in the 6.X series.

Now to the bad news I don't think I'm going to be able to start Grace 7 till next year because of a number of different family and work related obligations. This will most definitely make the list as well as caching keyed delegates but I just can't start the work now.

@ipjohnson
Copy link
Owner

@eglauko I think this solution will work for the moment and for 7 I'll remove the interface and add the object key parameter to IWrapperOrExportActivationStrategy

@eglauko
Copy link
Contributor Author

eglauko commented Aug 15, 2018

Yes, I understood what you wanted to do and I created a solution to solve this problem without creating any impact on the current version.

I thought you could create another cache in the BaseExportLocatorScope class, similar to the ActivationDelegates cache, for delegates with type and key. And in the locate method of the InjectionScope class you could create and search for these keyed delegates.

@eglauko
Copy link
Contributor Author

eglauko commented Aug 15, 2018

Could you release a preview version with this PR?

@ipjohnson
Copy link
Owner

I can do that this evening.

@eglauko
Copy link
Contributor Author

eglauko commented Aug 16, 2018

100% of my tests passed.
Super cool.

@ipjohnson
Copy link
Owner

I'm pushing out a release this evening for this. Glad it worked out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants