Inferring FromServices optionality #39804
Conversation
ghost
added
the
area-mvc
| // ServicesModelBinder does not have any state. Re-use the same instance for binding. | ||
|
|
||
| private readonly ServicesModelBinder _modelBinder = new ServicesModelBinder(); | ||
| private readonly NullabilityInfoContext _nullabilityContext = new(); |
There was a problem hiding this comment.
We have some issues with thread safety and unit tests when we implemented this in minimal APIs. This isn't thread safe so I wonder if it should be part of the ModelMetadata instead.
cc @pranavkm
There was a problem hiding this comment.
@davidfowl good to know. My initial design was move to modelmetada, since the same will be need around and that is the reason I did not published the PR.
The problem is, today there is a IsRequired property there already that does not take exactly the nullabiliy context in account. So, I am not sure what to do about that.
There was a problem hiding this comment.
Actually, let me do a deep look at the IsRequired because I feel it does what we need already.
There was a problem hiding this comment.
I think IsRequired works here and also take in account the [Required] attribute.
The only scenario that will cause a big change to the current behavior is when Reference type parameters without a default value in an oblivious nullability context, because today they will throw an exception and after the change they will, based on the IsRequired logic, very similar to the RequestDelegateFactory, bind to null. I was trying to avoid this behavioral change, but any thoughts about that?
There was a problem hiding this comment.
Eg.:
#nullable disable
public IActionResult Action([FromServices] IPersonService param1);
#nullable restore
There was a problem hiding this comment.
We shouldn't change the semantics of IsRequired if the context is oblivious. Is that something we can infer from the context?
There was a problem hiding this comment.
Do you mean infer for the Service as required to keep the same behavior for the scenario? If so, let me double check but maybe is possible to infer based on something like this:
!ModelType.IsValueType && nullabilityInfo.ReadState == NullabilityState.Unknown
There was a problem hiding this comment.
@pranavkm I have updated the PR with something I don't like much but that covers the scenario without change the IsRequired.
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| internal bool IsOptionalParameter(ParameterInfo parameterInfo) => | ||
| parameterInfo.HasDefaultValue || |
There was a problem hiding this comment.
Can you use ParameterDefaultValue?
There was a problem hiding this comment.
I didn't get what you mean with that.
src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinderProvider.cs
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinderProvider.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinderProvider.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/test/ModelBinding/Binders/ServicesModelBinderProviderTest.cs
Show resolved
Hide resolved
src/Mvc/Mvc.Core/test/ModelBinding/TestModelBinderProviderContext.cs
Outdated
Show resolved
Hide resolved
…rovider.cs Co-authored-by: Pranav K <prkrishn@hotmail.com>
…ext.cs Co-authored-by: Pranav K <prkrishn@hotmail.com>
…ProviderTest.cs Co-authored-by: Pranav K <prkrishn@hotmail.com>
…ext.cs Co-authored-by: Pranav K <prkrishn@hotmail.com>
src/Mvc/Mvc.Core/test/ModelBinding/Binders/ServicesModelBinderProviderTest.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/test/ModelBinding/Binders/ServicesModelBinderProviderTest.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Pranav K <prkrishn@hotmail.com>
Fixes #39757