Skip to content

Support resolving keyed services from DI in RDF and RDG #50095

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

Merged
merged 3 commits into from
Aug 17, 2023

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Aug 15, 2023

Description

This PR adds support for generating the appropriate code gen, for both run-time and compile-time generation strategies, to resolve keyed services from dependency injection in minimal APIs.

Fixes #49633

Customer Impact

Support for keyed services was added to the runtime in .NET 8 Preview 7. This change allows customers to leverage keyed services in their minimal APIs and contributes to a more complete user experience.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

Low risk, because the change is additive and doesn't affect pre-existing codepaths around parameter binding

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@ghost ghost added the area-runtime label Aug 15, 2023
@captainsafia captainsafia added feature-rdg area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-rdf and removed area-runtime labels Aug 15, 2023
@captainsafia captainsafia added the Servicing-consider Shiproom approval is required for the issue label Aug 16, 2023
@ghost
Copy link

ghost commented Aug 16, 2023

Hi @captainsafia. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@captainsafia
Copy link
Member Author

Approved via email.

@captainsafia captainsafia added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Aug 17, 2023
@ghost
Copy link

ghost commented Aug 17, 2023

Hi @captainsafia. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we ask for [FromKeyedServices] to add a AttributeTargets.Property target for [AsParameters] support? Should we define our own attribute for this purpose since [FromServices] does support properties and it's an unfortunate gap?

@captainsafia
Copy link
Member Author

@halter73 Good question! I'd be curious to hear @benjaminpetit thoughts on the scenario as well.

IMO, the bigger issue with the lack of AttributeUsage.Property is the fact that this wouldn't play nice with MVC's implicit surrogate model for arguments.

@benjaminpetit Is adding another AttributeUsage considered an API change here that we could do for .NET 8?

FWIW, I don't believe we'll need to do any reaction to it in ASP.NET once it's enabled since we already support recursing into the type so it's relatively cheap.

@benjaminpetit
Copy link
Member

@benjaminpetit Is adding another AttributeUsage considered an API change here that we could do for .NET 8?

No idea. I am not sure it's the way to go, since it would be specific for this use case (unless I am missing something).

Maybe defining your own attribute might be better, even if it might be confusing with all these attributes...

@captainsafia
Copy link
Member Author

@benjaminpetit Spoke with Ben offline.

Personally, I'd prefer that we add the AttributeUsage.Property in the attribute in the runtime over creating a new attribute in ASP.NET Core. I want to avoid reproducing the problems we currently have with multiple variants of similar attributes throughout the stack.

For .NET 8, I'm more comfortable with an attribute that is consistent throughout all implementations but limited in functionality than inconsistent attributes with different behaviors.

@wtgodbe wtgodbe merged commit ff097c6 into release/8.0-rc1 Aug 17, 2023
@wtgodbe wtgodbe deleted the safia/minapi-keyed-di branch August 17, 2023 20:07
@ghost ghost added this to the 8.0-rc1 milestone Aug 17, 2023
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Aug 17, 2023
@JamesNK
Copy link
Member

JamesNK commented Aug 17, 2023

FromKeyedService should support properties.

That would also allow DI containers that support property setters to use it.

@captainsafia captainsafia mentioned this pull request Aug 17, 2023
10 tasks
wtgodbe pushed a commit that referenced this pull request Aug 17, 2023
* Update dependencies from https://github.com/dotnet/arcade build 20230815.4 (#50113)

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Build.Tasks.Installers , Microsoft.DotNet.Build.Tasks.Templating , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.RemoteExecutor
 From Version 8.0.0-beta.23411.1 -> To Version 8.0.0-beta.23415.4

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* [release/8.0-rc1] [Blazor] Add APIs for "enhanced refresh" (#50124)

* Add NavigationManager.Refresh() + tests

* PR feedback

* Add `forceReload` parameter

---------

Co-authored-by: Mackinnon Buck <[email protected]>

* Support resolving keyed services from DI in RDF and RDG (#50095)

* Support resolving keyed services from DI in RDF and RDG

* Address feedback from peer review

* Support keyed services with different keys but same arg name

---------

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Mackinnon Buck <[email protected]>
Co-authored-by: Safia Abdalla <[email protected]>
@BrennanConroy BrennanConroy added the blog-candidate Consider mentioning this in the release blog post label Aug 21, 2023
@ghost
Copy link

ghost commented Aug 21, 2023

@captainsafia, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc blog-candidate Consider mentioning this in the release blog post feature-rdf feature-rdg Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants