Skip to content

Inferring FromServices optionality #39804

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.

Sign up for GitHub

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 10 commits into from
Feb 2, 2022
Merged

Inferring FromServices optionality #39804

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8c701ab
Inferring FromServices optionality
brunolins16 Jan 27, 2022
4a66bcc
Using IsRequired
brunolins16 Jan 28, 2022
5f0c160
Adding integration tests
brunolins16 Jan 28, 2022
d1c4680
Update src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinderP…
brunolins16 Jan 28, 2022
3e298f3
Update src/Mvc/Mvc.Core/test/ModelBinding/TestModelBinderProviderCont…
brunolins16 Jan 28, 2022
7231185
Update src/Mvc/Mvc.Core/test/ModelBinding/Binders/ServicesModelBinder…
brunolins16 Jan 28, 2022
3eadb0e
Update src/Mvc/Mvc.Core/test/ModelBinding/TestModelBinderProviderCont…
brunolins16 Jan 28, 2022
70b369f
Merge branch 'main' into brunolins16/issues/39757
brunolins16 Jan 28, 2022
14b53d7
Fixing unit test
brunolins16 Feb 1, 2022
69a9fd8
Apply suggestions from code review
brunolins16 Feb 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;
/// </summary>
public class ServicesModelBinder : IModelBinder
{
internal bool IsOptionalParameter { get; set; }

/// <inheritdoc />
public Task BindModelAsync(ModelBindingContext bindingContext)
{
Expand All @@ -23,9 +25,14 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
}

var requestServices = bindingContext.HttpContext.RequestServices;
var model = requestServices.GetRequiredService(bindingContext.ModelType);
var model = IsOptionalParameter ?
requestServices.GetService(bindingContext.ModelType) :
requestServices.GetRequiredService(bindingContext.ModelType);

bindingContext.ValidationState.Add(model, new ValidationStateEntry() { SuppressValidation = true });
if (model != null)
{
bindingContext.ValidationState.Add(model, new ValidationStateEntry() { SuppressValidation = true });
}

bindingContext.Result = ModelBindingResult.Success(model);
return Task.CompletedTask;
Expand Down
14 changes: 10 additions & 4 deletions src/Mvc/Mvc.Core/src/ModelBinding/Binders/ServicesModelBinderProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#nullable enable

using System.Reflection;

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;

Expand All @@ -11,9 +12,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;
/// </summary>
public class ServicesModelBinderProvider : IModelBinderProvider
{
// ServicesModelBinder does not have any state. Re-use the same instance for binding.

private readonly ServicesModelBinder _modelBinder = new ServicesModelBinder();
private readonly NullabilityInfoContext _nullabilityContext = new();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, let me do a deep look at the IsRequired because I feel it does what we need already.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Eg.:

#nullable disable
        public IActionResult Action([FromServices] IPersonService param1);
#nullable restore

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't change the semantics of IsRequired if the context is oblivious. Is that something we can infer from the context?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@pranavkm I have updated the PR with something I don't like much but that covers the scenario without change the IsRequired.


/// <inheritdoc />
public IModelBinder? GetBinder(ModelBinderProviderContext context)
Expand All @@ -26,9 +25,16 @@ public class ServicesModelBinderProvider : IModelBinderProvider
if (context.BindingInfo.BindingSource != null &&
context.BindingInfo.BindingSource.CanAcceptDataFrom(BindingSource.Services))
{
return _modelBinder;
return new ServicesModelBinder()
{
IsOptionalParameter = IsOptionalParameter(context.Metadata.Identity.ParameterInfo!)
};
}

return null;
}

internal bool IsOptionalParameter(ParameterInfo parameterInfo) =>
parameterInfo.HasDefaultValue ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use ParameterDefaultValue?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't get what you mean with that.

_nullabilityContext.Create(parameterInfo).ReadState == NullabilityState.Nullable;
}
56 changes: 55 additions & 1 deletion src/Mvc/Mvc.Core/test/ModelBinding/Binders/ServicesModelBinderProviderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Reflection;

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;

public class ServicesModelBinderProviderTest
Expand Down Expand Up @@ -51,7 +53,59 @@ public void Create_WhenBindingSourceIsFromServices_ReturnsBinder()
Assert.IsType<ServicesModelBinder>(result);
}

[Theory]
[MemberData(nameof(ParameterInfoData))]
public void Create_WhenBindingSourceIsNullableFromServices_ReturnsBinder(ParameterInfo parameterInfo, bool isOptional)
{
// Arrange
var provider = new ServicesModelBinderProvider();

var context = new TestModelBinderProviderContext(parameterInfo);

// Act
var result = provider.GetBinder(context);

// Assert
var binder = Assert.IsType<ServicesModelBinder>(result);
Assert.Equal(isOptional, binder.IsOptionalParameter);
}

private class IPersonService
{
}

public static TheoryData<ParameterInfo, bool> ParameterInfoData()
{
return new TheoryData<ParameterInfo, bool>()
{
{ ParameterInfos.NullableParameterInfo, true },
{ ParameterInfos.DefaultValueParameterInfo, true },
{ ParameterInfos.NonNullableParameterInfo, false },
};
}

private class ParameterInfos
{
public void TestMethod([FromServices] IPersonService param1, [FromServices] IPersonService param2 = null)
{ }

#nullable enable
public void TestMethod2([FromServices] IPersonService? param2)
{ }
#nullable restore

public static ParameterInfo NullableParameterInfo
= typeof(ParameterInfos)
.GetMethod(nameof(ParameterInfos.TestMethod2))
.GetParameters()[0];
public static ParameterInfo NonNullableParameterInfo
= typeof(ParameterInfos)
.GetMethod(nameof(ParameterInfos.TestMethod))
.GetParameters()[0];
public static ParameterInfo DefaultValueParameterInfo
= typeof(ParameterInfos)
.GetMethod(nameof(ParameterInfos.TestMethod))
.GetParameters()[1];

}
}
18 changes: 17 additions & 1 deletion src/Mvc/Mvc.Core/test/ModelBinding/TestModelBinderProviderContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
// Licensed to the .NET Foundation under one or more agreements.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using Microsoft.Extensions.Options;
using System.Reflection;

namespace Microsoft.AspNetCore.Mvc.ModelBinding;

Expand Down Expand Up @@ -38,6 +39,21 @@ public TestModelBinderProviderContext(Type modelType, BindingInfo bindingInfo)
(Services, MvcOptions) = GetServicesAndOptions();
}

public TestModelBinderProviderContext(ParameterInfo parameterInfo)
{
Metadata = CachedMetadataProvider.GetMetadataForParameter(parameterInfo);
MetadataProvider = CachedMetadataProvider;
_bindingInfo = new BindingInfo
{
BinderModelName = Metadata.BinderModelName,
BinderType = Metadata.BinderType,
BindingSource = Metadata.BindingSource,
PropertyFilterProvider = Metadata.PropertyFilterProvider,
};

(Services, MvcOptions) = GetServicesAndOptions();
}

public override BindingInfo BindingInfo => _bindingInfo;

public override ModelMetadata Metadata { get; }
Expand Down