ipjohnson / Grace

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

Attribute [Export] is exported twice #237

Open jods4 opened 5 years ago

jods4 commented 5 years ago

Here's my config, I think everything except the last line is irrelevant to the issue at hand:

      var container = new DependencyInjectionContainer(c => c.AutoRegisterUnknown = false);
      container.Configure(c =>
      {        
        // Register one ILogger per container with automatic job context
        c.ExportFactory((IExportLocatorScope scope) => Log.ForContext("Job", scope.ScopeName))
         .Lifestyle.SingletonPerScope();

        c.ExportFactory(A).AsKeyed<IDbConnection>("a");
        c.ExportFactory(B).AsKeyed<IDbConnection>("b");

        c.AddInspector(new ConfigInspector());

        // [Export] from this assembly
        c.ExportAssemblyContaining<JobSchedulerService>().ExportAttributedTypes();
      });

ExportAttributedTypes does export all classes in my assembly that have [Export], or [ExportByInterfaces], etc.

But when I look at activation strategies, or simply at WhatDoIHave output, I notice that all [Export] exports are actually exported twice. They have the AsType: XY line listed twice for every type they are exported as.

ipjohnson commented 5 years ago

Interesting. I'll have to take a look this weekend.

jods4 commented 5 years ago

@ipjohnson I was reading the code trying to see if I can figure out why. This is a complex codebase so I might be completely off... but could it be:

  1. CreateExportStrategiesForTypes will process the types found in my assemblies. Notice that this method looks for IExportAttribute and will add those types to exportTypes right here: https://github.com/ipjohnson/Grace/blob/39d4afb781a4b3b1634d33c6fd6b1165aac633a7/src/Grace/DependencyInjection/Impl/ExportTypeSetConfiguration.cs#L357 Because we have types in exportTypes, the method will then call CreateExportStrategyForType, here: https://github.com/ipjohnson/Grace/blob/39d4afb781a4b3b1634d33c6fd6b1165aac633a7/src/Grace/DependencyInjection/Impl/ExportTypeSetConfiguration.cs#L373

  2. CreateExportStrategyForType does two unfortunate things. It exports everything in exportTypes, right here: https://github.com/ipjohnson/Grace/blob/39d4afb781a4b3b1634d33c6fd6b1165aac633a7/src/Grace/DependencyInjection/Impl/ExportTypeSetConfiguration.cs#L402 I think this might be our first AsType export Later it goes on to process attributes here: https://github.com/ipjohnson/Grace/blob/39d4afb781a4b3b1634d33c6fd6b1165aac633a7/src/Grace/DependencyInjection/Impl/ExportTypeSetConfiguration.cs#L421

  3. I'm skipping a few indirections and intermediate methods. The default IActivationStrategyAttributeProcessor will end up right here, inside ProcessClassAttributes: https://github.com/ipjohnson/Grace/blob/e6853c7fb3b23a27824b9b2e17ad860b9b4a403b/src/Grace/DependencyInjection/Impl/ActivationStrategyAttributeProcessor.cs#L126 I think this is where we are AsType exported for the second time

Maybe I found the reason why attributes are exported twice. I am not sure what the fix is, though.

Gut feeling is that exportTypes at step 1 should not conflate exported types and types that are of interest because they have attributes.

ipjohnson commented 5 years ago

That does look to be a problem but my concern would be changing some logic right now and introducing a new bug.

I've been thinking maybe the answer is to check and make sure that duplicate types aren't being added to the the export type list.

jods4 commented 5 years ago

It's more dirty but you could post-process the registrations and dedup identical exports... the result from user perspective would be the same.

jods4 commented 4 years ago

Friendly bump! 🧸 I'm starting a new project with more or less the same auto-discovery setup, wondering if you had a chance to think about this bug.

ipjohnson commented 4 years ago

I've added some duplicate checking for ExportAs at the strategy level. Could you try it on the nightly nuget feed https://ci.appveyor.com/nuget/grace-master ?

jods4 commented 4 years ago

I did a quick test and I can confirm:

Other than that my project seem to pass smoke tests.

ipjohnson commented 4 years ago

I'll push this to NuGet this weekend along with the attribute usage change we talked about

jods4 commented 1 year ago

Hi @ipjohnson !

This is old but I'd like to let you know that we use 7.2 in my projects and as reported in my previous comment exports are not duplicated anymore, so I think you can close this issue.

Thanks a lot and I wish you a happy new year!

PS: I noticed you're working on a big 8.0 release, are there release notes with the main changes somewhere?

ipjohnson commented 1 year ago

Hi @jods4

Version 8 is about removing deprecated code and deprecated targets, I'll put together list of changes this weekend.