Support Keyed Services in MVC#50145
Support Keyed Services in MVC#50145adityamandaleeka merged 10 commits intodotnet:release/8.0-rc1from
Conversation
benjaminpetit
added
the
area-mvc
| !context.Metadata.ModelType.IsValueType && | ||
| context.Metadata.NullabilityState == NullabilityState.Unknown); | ||
|
|
||
| var attribute = context.Metadata.Identity.ParameterInfo?.GetCustomAttribute<FromKeyedServicesAttribute>(); |
There was a problem hiding this comment.
It would be great to get the key when constructing the BindingInfo, but it will require adding some properties in several places.
|
OK, please mark servicing-consider when ready and email tactics for approval for rc1. |
|
I opened an issue to track the api change needed here: #49634 |
| [HttpGet("GetBoth")] | ||
| public ActionResult<string> GetBoth( | ||
| [FromKeyedServices("ok_service")] ICustomService s1, | ||
| [FromKeyedServices("not_ok_service")] ICustomService s2) |
There was a problem hiding this comment.
Do we have any tests for optional and required services when the service is missing?
| // Keyed services | ||
| if (attributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.GetType()))) | ||
| { | ||
| if (attributes.OfType<FromKeyedServicesAttribute>().FirstOrDefault() is not null) |
There was a problem hiding this comment.
Why not combine these into a single conditional?
And why not support both and treat it the same as if the [FromServices] wasn't there? Regardless, we should add a test with both [FromServices] and [FromKeyedServices].
There was a problem hiding this comment.
And why not support both and treat it the same as if the [FromServices] wasn't there? Regardless, we should add a test with both [FromServices] and [FromKeyedServices].
This came up as a topic of discussion in the minimal API PR (see here). Overall, I think throwing an exception that informs users that the two attributes are mutually exclusive is a clearer experience then figuring out what sort of implicit behavior we have here.
|
Build is failing because
But I didn't changed |
| @@ -1 +1,3 @@ | |||
| #nullable enable | |||
| Microsoft.AspNetCore.Mvc.ModelBinding.BindingInfo.ServiceKey.get -> object? | |||
| Microsoft.AspNetCore.Mvc.ModelBinding.BindingInfo.ServiceKey.set -> void | |||
There was a problem hiding this comment.
@benjaminpetit here's the modification to the API file that the build is complaining about. Normally we don't change these files in release branches, but since this is the RC1 branch it's fine. Ping me or a manager when this is approved & CI failures are down to just this, and we can force-merge it for you (note I'll be OOF for a week after about 3:30 PST today)
|
|
Approved over email. Looks like the remaining error is about an extra using directive. Once that's fixed I guess we can merge it @benjaminpetit |
|
Hi @benjaminpetit. 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. |
benjaminpetit
force-pushed
the
feature/keyed-di-rebase
branch
from
dd131fc to
b85e637
Compare
benjaminpetit
force-pushed
the
feature/keyed-di-rebase
branch
from
b85e637 to
397abeb
Compare
|
Ready to be merged, the only failure is because I added an API. |
ghost
added this to the
BrennanConroy
added
the
blog-candidate
|
@benjaminpetit, 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! |
7 participants
Description
This PR adds support to resolve keyed services from dependency injection in MVC.
Fixes #49634
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 MVC and contributes to a more complete user experience.
Regression?
Risk
Low risk, because the change is additive and doesn't affect pre-existing codepaths around parameter binding.
Verification
Packaging changes reviewed?