Skip to content

Avoid logging unsupported alternative for complex type binding #39353

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 1 commit into from
Jan 19, 2022
Merged

Avoid logging unsupported alternative for complex type binding #39353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexTypeModelBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,14 @@ protected virtual object CreateModel(ModelBindingContext bindingContext)
var modelType = bindingContext.ModelType;
if (modelType.IsAbstract || modelType.GetConstructor(Type.EmptyTypes) == null)
{
// If the model is not a top-level object, we can't examine the defined constructor
// to evaluate if the non-null property has been set so we do not provide this as a valid
// alternative.
if (!bindingContext.IsTopLevelObject)
{
throw new InvalidOperationException(Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_ForType(modelType.FullName));
}

var metadata = bindingContext.ModelMetadata;
switch (metadata.MetadataKind)
{
Expand Down
44 changes: 39 additions & 5 deletions src/Mvc/Mvc.Core/test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,11 +588,8 @@ public void CreateModel_ForStructModelType_AsProperty_ThrowsException()
string.Format(
CultureInfo.CurrentCulture,
"Could not create an instance of type '{0}'. Model bound complex types must not be abstract or " +
"value types and must have a parameterless constructor. Alternatively, set the '{1}' property to" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this message still valid / correct, isn't it? Should we be changing it for this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmmm. My understanding was we wanted to stop logging this recommendation for any complex type where we couldn't reliably examine the constructor to verify that they were initializing the property inside it?

Copy link
Contributor

Choose a reason for hiding this comment

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

The way I had thought this would work was that we would not show this message for top-level objects. We would still show this for arbitrary properties that we couldn't initialize because it is valid for users to initialize it and for us to bind properties on it. That said, this one is a struct so it might have unique characteristics.

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 this exception still makes sense. For a set of types like:

private class Location
    {
        public PointStruct Point { get; set; }
    }

    private readonly struct PointStruct
    {
        public PointStruct(double x, double y)
        {
            X = x;
            Y = y;
        }

        public double X { get; }
        public double Y { get; }
    }

We can't examine the Location constructor to check if Point is initialized so it doesn't make sense to log that behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I re-read the message and it works out alrigh - it says you cannot model bind a value type. The part about the ctor is entirely irrelevant here. This seems alright enough, let's get this in.

" a non-null value in the '{2}' constructor.",
typeof(PointStruct).FullName,
nameof(Location.Point),
typeof(Location).FullName),
"value types and must have a parameterless constructor.",
typeof(PointStruct).FullName),
exception.Message);
}

Expand Down Expand Up @@ -1002,6 +999,32 @@ public async Task BindModelAsync_Success()
Assert.True(bindingContext.ModelState.IsValid);
}

// Validates fix for https://github.com/dotnet/aspnetcore/issues/21916
[Fact]
public async Task BindModelAsync_PropertyInitializedInNonParameterlessConstructorConstructor()
{
// Arrange
var model = new ModelWithPropertyInitializedInConstructor("TestName");
var property = GetMetadataForProperty(model.GetType(), nameof(ModelWithPropertyInitializedInConstructor.NameContainer));
var nestedProperty = GetMetadataForProperty(typeof(ClassWithNoParameterlessConstructor), nameof(ClassWithNoParameterlessConstructor.Name));
var bindingContext = CreateContext(property);
bindingContext.IsTopLevelObject = false;
var valueProvider = new Mock<IValueProvider>(MockBehavior.Strict);
valueProvider
.Setup(provider => provider.ContainsPrefix("theModel.Name"))
.Returns(true);
bindingContext.ValueProvider = valueProvider.Object;
var binder = CreateBinder(bindingContext.ModelMetadata);

binder.Results[nestedProperty] = ModelBindingResult.Success(null);

// Act
var exception = await Assert.ThrowsAsync<InvalidOperationException>(async () => await binder.BindModelAsync(bindingContext));
// Assert
var unexpectedMessage = "Alternatively, set the 'NameContainer' property to a non-null value in the 'Microsoft.AspNetCore.Mvc.ModelBinding.Binders.ComplexTypeModelBinderTest+ModelWithPropertyInitializedInConstructor' constructor.";
Assert.DoesNotContain(exception.Message, unexpectedMessage);
}

[Fact]
public void SetProperty_PropertyHasDefaultValue_DefaultValueAttributeDoesNothing()
{
Expand Down Expand Up @@ -1302,6 +1325,17 @@ public ClassWithNoParameterlessConstructor(string name)
public string Name { get; set; }
}

private class ModelWithPropertyInitializedInConstructor
{
public ModelWithPropertyInitializedInConstructor(string name)
{
NameContainer = new ClassWithNoParameterlessConstructor(name);
}

[ValueBinderMetadataAttribute]
public ClassWithNoParameterlessConstructor NameContainer { get; set; }
}

private class BindingOptionalProperty
{
[BindingBehavior(BindingBehavior.Optional)]
Expand Down
7 changes: 2 additions & 5 deletions src/Mvc/test/Mvc.IntegrationTests/ActionParametersIntegrationTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,11 +478,8 @@ public async Task ActionParameter_UsingComplexTypeModelBinder_ModelPropertyTypeW
string.Format(
CultureInfo.CurrentCulture,
"Could not create an instance of type '{0}'. Model bound complex types must not be abstract or " +
"value types and must have a parameterless constructor. Alternatively, set the '{1}' property to" +
" a non-null value in the '{2}' constructor.",
typeof(ClassWithNoDefaultConstructor).FullName,
nameof(Class1.Property1),
typeof(Class1).FullName),
"value types and must have a parameterless constructor.",
typeof(ClassWithNoDefaultConstructor).FullName),
exception.Message);
}

Expand Down