-
Notifications
You must be signed in to change notification settings - Fork 90
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
refactor: migrate away from MEF to Dependency Injection #412
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JamieMagee
commented
Jan 26, 2023
JamieMagee
force-pushed
the
users/jamagee/dependency-injection
branch
from
January 27, 2023 16:52
6af2dd6
to
c87abe8
Compare
This comment was marked as resolved.
This comment was marked as resolved.
JamieMagee
force-pushed
the
users/jamagee/dependency-injection
branch
from
January 30, 2023 05:00
df76a9f
to
0291e0f
Compare
Currently, Managed Extensibility Framework (MEF) does property injection. In migrating to .NET's own Dependency Injection framework `Microsoft.Extensions.DependencyInjection` we need to migrate to constructor injection. This change adds 2 constructors for every class that has properties injected into it: - A parameterless constructor (for MEF) - A constructor with parameters for all the required dependencies (for .NET DI) This is an intermediate step that allows MEF to work, while work continues on the migration to MEF. See #400 for more information
This is the equivalent of `DetectorTestUtilityCreator` and `DetectorTestUtility`, but for .NET DI. See #400
…lder` This change migrates all of the component detector tests to us `DetectorTestUtilityBuilder`, based on .NET DI, instead of `DetectorTestUtility`, based on MEF. This also adds a new base class for all component detector tests called `BaseDetectorTest` which constructs the `DetectorTestUtilityBuilder` in its constructor.
No longer required after all component detector tests have been migrated to use `DetectorTestUtilityBuilder` instead
This change uses the constructor that has parameters for all required properties of a service, instead of using object initializers (which actually use the parameterless constructor)
This change adds all of the services that will be managed by .NET's dependency injection framework to the `AddComponentDetection` method. This allows you easily configure all the services that Component Detection requires ```csharp var serviceProvider = new ServiceCollection() .AddComponentDetection() .BuildServiceProvider(); var orchestrator = serviceProvider.GetRequiredService<Orchestrator>(); var result = await orchestrator.LoadAsync(args); ```
This is one of the final changes in migrating away from Managed Extensibility Framework (MEF) and to .NET's Dependency Injection (DI). These changes include: - Deleting classes that are no longer required for handling property injection: - `IDetectorDependencies` and `DetectorDependencies` which held references to all common services - `InjectionParameters` which had `IDetectorDependencies` injected, and contained static references to all common services - `IDetectorRegistryService` and `DetectorRegistryService` which loaded additional classes from DLLs - This functionality designed to be useful before Component Detection was open sourced. Now that it is, detectors can be added more easily. This functionality was not used internally. - This means that `IComponentGovernanceOwnedDetectors` can also be deleted - Updating injected types - `BcdeScanExecutionService` and `DetectorListingCommandService` had `IDetectorRegistryService` injected. Instead, they now have `IEnumerable<IComponentDetector>` injected directly. - `Init` method for `FileWritingService` and `Logger` moved to their respective interfaces, `IFileWritingService` and `ILogger` - `Orchestrator` had the concrete types `FileWritingService` and `Logger` injected. However, it's best practice to inject interfaces instead. In making this change, some methods that were available in the concrete type had to be added to the interface - `VerbosityMode` needed to be moved from `Common` to `Contracts` to facilitate this
As `TelemetryRelay` is a static class, it cannot be instantiated using Dependency Injection. Instead, we inject an `IServiceProvider` into `Orchestrator` and call `Init` on `TelemetryRelay` with the required `IEnumerable<ITelemetryService>`
The parameter `IEnumerable<Lazy<IGraphTranslationService, GraphTranslationServiceMetadata>>` was designed to allow loading of alternative graph translation services, dynamically, from DLLs. That functionality never materialized, and MEF is being removed, so I am simplifying that constructor parameter to be simply `IGraphTranslationService`. If alternate graph translation services are required in the future, the constructor can be updated to take a `IDictionary<IGraphTranslationService, GraphTranslationServiceMetadata>>`. This can be added to the `IServiceCollection` as follows: ```csharp services.AddSingleton<DefaultGraphTranslationService>(); services.AddSingleton<FancyNewGraphTranslationService>(); services.AddSingleton<IDictionary<IGraphTranslationService, GraphTranslationServiceMetadata>>(sp => new Dictionary<IGraphTranslationService, GraphTranslationServiceMetadata>> { { sp.GetRequiredService<DefaultGraphTranslationService>(), new GraphTranslationServiceMetadata { Priority = 1 } }, { sp.GetRequiredService<FancyNewGraphTranslationService>(), new GraphTranslationServiceMetadata { Priority = 0 } }, }); ```
This includes: - Removal of `[Export]` and `[Import]` attributes - Conversion of public properties to private readonly fields - Removal of parameterless constructors
…e managed by dependency injection
This can likely be done better or cleaner, but it's sufficient for now
JamieMagee
force-pushed
the
users/jamagee/dependency-injection
branch
from
February 6, 2023 03:48
0291e0f
to
48dc640
Compare
cobya
approved these changes
Feb 8, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed offline and changes look good.
JamieMagee
added
version:major
breaking change
Breaking change, requires major version bump
labels
Feb 14, 2023
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a quite a large PR, that breaks down all the steps required to close #400. I strongly recommend reviewing this commit-by-commit and reading the commit message.
1. refactor: add constructors to injected classes
Currently, Managed Extensibility Framework (MEF) does property injection. In migrating to .NET's own Dependency Injection framework
Microsoft.Extensions.DependencyInjection
we need to migrate to constructor injection. This change adds 2 constructors for every class that has properties injected into it:This is an intermediate step that allows MEF to work, while work continues on the migration to MEF.
2. test: add DetectorTestUtilityBuilder
This is the equivalent of
DetectorTestUtilityCreator
andDetectorTestUtility
, but for .NET DI.3. test: migrate component detector tests to use `DetectorTestUtilityBuilder`
This change migrates all of the component detector tests to us
DetectorTestUtilityBuilder
, based on .NET DI, instead ofDetectorTestUtility
, based on MEF.This also adds a new base class for all component detector tests called
BaseDetectorTest
which constructs theDetectorTestUtilityBuilder
in its constructor.4. test: delete DetectorTestUtility and DetectorTestUtilityCreator
No longer required after all component detector tests have been migrated to use
DetectorTestUtilityBuilder
instead5. test: use dependency injection constructor for service tests
This change uses the constructor that has parameters for all required properties of a service, instead of using object initializers (which actually use the parameterless constructor)
6. feat: add all services to AddComponentDetection
This change adds all of the services that will be managed by .NET's dependency injection framework to the
AddComponentDetection
method. This allows you easily configure all the services that Component Detection requires7. refactor: use AddComponentDetection to setup services
This is one of the final changes in migrating away from Managed Extensibility Framework (MEF) and to .NET's Dependency Injection (DI).
These changes include:
IDetectorDependencies
andDetectorDependencies
which held references to all common servicesInjectionParameters
which hadIDetectorDependencies
injected, and contained static references to all common servicesIDetectorRegistryService
andDetectorRegistryService
which loaded additional classes from DLLsIComponentGovernanceOwnedDetectors
can also be deletedBcdeScanExecutionService
andDetectorListingCommandService
hadIDetectorRegistryService
injected. Instead, they now haveIEnumerable<IComponentDetector>
injected directly.Init
method forFileWritingService
andLogger
moved to their respective interfaces,IFileWritingService
andILogger
Orchestrator
had the concrete typesFileWritingService
andLogger
injected. However, it's best practice to inject interfaces instead. In making this change, some methods that were available in the concrete type had to be added to the interfaceVerbosityMode
needed to be moved fromCommon
toContracts
to facilitate this8. refactor: init TelemetryRelay during orchestrator setup
As
TelemetryRelay
is a static class, it cannot be instantiated using Dependency Injection. Instead, we inject anIServiceProvider
intoOrchestrator
and callInit
onTelemetryRelay
with the requiredIEnumerable<ITelemetryService>
9. refactor: only one GraphTranslationService
The parameter
IEnumerable<Lazy<IGraphTranslationService, GraphTranslationServiceMetadata>>
was designed to allow loading of alternative graph translation services, dynamically, from DLLs. That functionality never materialized, and MEF is being removed, so I am simplifying that constructor parameter to be simplyIGraphTranslationService
.If alternate graph translation services are required in the future, the constructor can be updated to take a
IDictionary<IGraphTranslationService, GraphTranslationServiceMetadata>>
. This can be added to theIServiceCollection
as follows:10. refactor: remove System.Composition usage
This includes:
[Export]
and[Import]
attributes11. refactor: migrate YarnLockFileFactory and YarnLockFileParser to be managed by dependency injection
12. test: final test fixes
13. refactor: remove System.CompositionModel dependencies
🎉
14. refactor: fix misc analyzer warnings