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

.net 8.0 support #36

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

.net 8.0 support #36

wants to merge 7 commits into from

Conversation

jods4
Copy link

@jods4 jods4 commented Nov 23, 2023

I have bootstrapped the work to support keyed services in .net 8.0.
Please note that this is incomplete as there are new features that I'm not sure how to best add to Grace:

  • ServiceDescriptor has new members (IsKeyedService and co.), to register keyed services.
  • GraceServiceProvider should implement IKeyedServiceProvider, an extension of IServiceProvider.
    It looks like .net still always injects IServiceProvider but then checks if IKeyedServiceProvider is implemented when keyed services are requested. Extension methods GetKeyedService are defined on IServiceProvider based on this principle.
  • ServiceProviderIsServiceImpl should implement extended interface IServiceProviderIsKeyedService.
    Skimming .net source code, it looks like this is downcasted from IServiceProviderIsService but IServiceProviderIsKeyedService is also considered a legit exported type, so I exported this interface explicitly, too.
  • FromKeyedServiceAttribute I feel like this BCL attribute needs to be understood by Grace, as it indicates the imported key when resolving services that were registered through BCL ServiceCollection apis.
    Not sure what the best code is here: I guess in GraceRegistration.Register() we need to look at ctor parameters to identify which ones have [FromKeyedServices] and configure the strategy accordingly?
    Also wondering how this interacts with auto-registration: can any type not going through GraceRegistration.Register() use [FromKeyedServices]? That feels more robust but then I'm not sure how to teach core Grace about this attribute in a decoupled fashion.
  • KeyedService.AnyKey This is a well-known singleton, that when used during registration (i.e. container.AddKeyedService<T>(KeyedService.AnyKey)) indicates that this registration should match all keys during resolution. I think Grace has no such feature at the moment.
    My understanding of this new feature is that when locating one instance, matching keys have priority. Then if no such key exist then AnyKey registrations are considered. When locating all instances (e.g. IEnumerable<T>), then both matching keys and AnyKey must be returned.
    It might seem weird to resolve a registration regardless of its key, but when you combine this with the next feature ServiceKeyAttribute, it opens up interesting use-cases as the ctor can be aware of the key being created.
    Note that AnyKey must not be conflated with null. .net considers null as a non-keyed registration. Also when doing a non-keyed Locate<T>(), AnyKey registrations must NOT be included.
  • ServiceKeyAttribute This is another BCL attribute that Grace needs to know about: it can be used on a string ctor parameter and the parameter must receive the service key being created. This works nicely in combination with AnyKey.

@jods4
Copy link
Author

jods4 commented Nov 24, 2023

I gave some thoughts to the missing features:

AnyKey would be easy to adapt, if Grace had a similar Grace.AnyKey singleton for similar purposes.
We just need to substitute during registration: key == KeyedService.AnyKey ? Grace.AnyKey : key.

[FromKeyedService] and [ServiceKey] are more annoying because they can't be modified to implement Grace interfaces (for instance, FromKeyedServiceAttribute should implement IImportAttribute but that's out of our control).
I think Grace needs a way to register attribute adaptors.

So instead of just looking for specific interfaces, like it does now:

// In ConstructorExpressionCreator.ProcessImportAttributes()
var importAttribute = (IImportAttribute)parameter.GetCustomAttributes()?.FirstOrDefault(a => a is IImportAttribute);
if (importAttribute != null)
{
  var info = importAttribute.ProvideImportInfo(parameter.ParameterType, parameter.Name);
  // ...
}

It would have an additional layer that consider registered adapters:

// In Grace 
private Dictionary<Type, Func<Attribute, ParameterInfo, ImportAttributeInfo>> adaptors = new();

public void RegisterIImportAttribute<A>(Func<Attribute, ParameterInfo, ImportAttributeInfo> adaptor)
  where A: Attribute
  => adaptors.Add(typeof(A), adaptor);
  
public ImportAttributeInfo? GetImportAttributeInfo(ParameterInfo parameter)
{
  foreach (var attr in parameter.GetCustomAttributes())
  {
    if (attr is IImportAttribute i) return i.ProvideImportInfo(parameter.ParameterType, parameter.Name);
    if (adaptors.TryGet(attr.GetType(), out var adaptor)) return adaptor(attr, parameter);
  }
  return null;
}
  
// In Grace.DependecyInjection.Extensions
RegisterIImportAttribute<FromKeyedServiceAttribute>((attr, _) => 
  new ImportAttributeInfo
  { 
    ImportKey = ((FromKeyedServiceAttribute)attr).Key, 
    IsRequired = true,
  };

@ipjohnson
Copy link
Owner

@jods4 thank you so much for taking a look at this, between work and family life I haven't been able to do any OSS development this year. My hope is in the coming year things calm down at work and I'm able to find some time for Grace and my other projects.

I think your idea of registering adapters makes a lot of sense. I can't work on it for the next two weeks but I'm hoping to have some time Christmas week (I have a presentation to put together as well as an on call shift coming up).

@jods4
Copy link
Author

jods4 commented Nov 26, 2023

@ipjohnson No worry, I'll try to give a hand. Registering attribute adapters is something I should be able to PR.

Quick question: how do you usually proceed when a development in one library (this one) depends on a new release of Grace core?

@jods4
Copy link
Author

jods4 commented Nov 28, 2023

More thoughts on missing Grace features for .net 8:

AnyKey I'm thinking of exposing a static ImportKey.Any that works similarly to .net 8:

  • For keyed Locate, locating ImportKey.Any is used as a fallback;
  • For keyed LocateAll, the result is a concat of original key and ImportKey.Any;
  • For CanLocate it's an or between original key and ImportKey.Any;
  • not clear yet for me whether there's something to do during the configuration process when gluing all services definitions together. Probably yes, the strategy needs to figure out the exact key to use for a specific dependency import, or at least if it's possible.

Service Key I'm thinking of encoding this with another well-known key: ImportKey.Key. Importing a string keyed with this magic key would be fulfilled by the the key of service being injected.
This has the benefit of requiring no change to Grace internal structures. It can be used simply with [Import(Key = ImportKey.Key)] but I'm thinking of adding a convenience [ImportKey].
Because this is just a keyed import, .net ServiceKeyAttribute can be registered with the same adapter infra. discussed above for FromKeyedServiceAttribute.
Implementation-wise, I was thinking maybe a new special strategy can be created for that. I have to dig into the source code more.

@ipjohnson
Copy link
Owner

how do you usually proceed when a development in one library (this one) depends on a new release of Grace core?
I will frequently build the package and store it locally, then configure a nuget feed based off the local directory.

@jods4
Copy link
Author

jods4 commented Jan 5, 2024

I have completed the integration of MS DI thanks to the new features in ipjohnson/Grace#314 and I added the new Keyed specification tests from MS.
It's not building in CI as it depends on a local build of Grace (8.0.0-RC838) that is not published yet.

EDIT
Results updated after ipjohnson/Grace@be118fc: 4 tests fixed
Fix for keyed export factory accepting the injection key as 2nd parameter: 2 tests fixed
Fix to support IKeyedServiceProvider in scopes: 3 tests fixed
Fix to support IEnumerable of keyed: 3 tests fixed
Fix for CanLocate keyed collections: 2 tests fixed
Fix to throw on bad Any key conversion: 2 tests fixed

I built it locally and ran the tests.
All "old" tests are still green. There are 6 failures out of the 30 new keyed tests.
Many are expected because they are LocateAll / IEnumerable tests, which we know don't work for keyed services in Grace as of now. I have yet to examine the cause of failure for the other tests.

The source code of MS keyed DI tests is here:
https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.DependencyInjection.Specification.Tests/src/KeyedDependencyInjectionSpecificationTests.cs

One thing I have already noticed that will require important changes in Grace:
Multiple registrations for the same keyed service must be supported.

@jods4
Copy link
Author

jods4 commented Feb 7, 2024

Great milestone: with the recent fixes, all MS DI tests are now passing (when this package is built against the unreleased .net 8.0 branch of Grace). 🎉

image

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

Successfully merging this pull request may close these issues.

2 participants