Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Add [ValidateNever] and IPropertyValidationFilter #5676

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Diagnostics;
using System.Reflection;
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;
using Microsoft.Extensions.Internal;

namespace Microsoft.AspNetCore.Mvc.ModelBinding
Expand Down Expand Up @@ -295,6 +296,13 @@ public string PropertyName
/// </summary>
public abstract string TemplateHint { get; }

/// <summary>
/// Gets an <see cref="IPropertyValidationFilter"/> implementation that indicates whether this model should be
/// validated. If <c>null</c>, properties with this <see cref="ModelMetadata"/> are validated.
/// </summary>
/// <value>Defaults to <c>null</c>.</value>
public virtual IPropertyValidationFilter PropertyValidationFilter => null;

/// <summary>
/// Gets a value that indicates whether properties or elements of the model should be validated.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
{
/// <summary>
/// Contract for attributes that determine whether associated properties should be validated. When the attribute is
/// applied to a property, the validation system calls <see cref="ShouldValidateEntry"/> to determine whether to
/// validate that property. When applied to a type, the validation system calls <see cref="ShouldValidateEntry"/>
/// for each property that type defines to determine whether to validate it.
/// </summary>
public interface IPropertyValidationFilter
{
/// <summary>
/// Gets an indication whether the <paramref name="entry"/> should be validated.
/// </summary>
/// <param name="entry"><see cref="ValidationEntry"/> to check.</param>
/// <param name="parentEntry"><see cref="ValidationEntry"/> containing <paramref name="entry"/>.</param>
/// <returns><c>true</c> if <paramref name="entry"/> should be validated; <c>false</c> otherwise.</returns>
bool ShouldValidateEntry(ValidationEntry entry, ValidationEntry parentEntry);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
/// </summary>
public struct ValidationEntry
{
private object _model;
private Func<object> _modelAccessor;

/// <summary>
/// Creates a new <see cref="ValidationEntry"/>.
/// </summary>
Expand All @@ -30,7 +33,37 @@ public ValidationEntry(ModelMetadata metadata, string key, object model)

Metadata = metadata;
Key = key;
Model = model;
_model = model;
_modelAccessor = null;
}

/// <summary>
/// Creates a new <see cref="ValidationEntry"/>.
/// </summary>
/// <param name="metadata">The <see cref="ModelMetadata"/> associated with the <see cref="Model"/>.</param>
/// <param name="key">The model prefix associated with the <see cref="Model"/>.</param>
/// <param name="modelAccessor">A delegate that will return the <see cref="Model"/>.</param>
public ValidationEntry(ModelMetadata metadata, string key, Func<object> modelAccessor)
{
if (metadata == null)
{
throw new ArgumentNullException(nameof(metadata));
}

if (key == null)
{
throw new ArgumentNullException(nameof(key));
}

if (modelAccessor == null)
{
throw new ArgumentNullException(nameof(modelAccessor));
}

Metadata = metadata;
Key = key;
_model = null;
_modelAccessor = modelAccessor;
}

/// <summary>
Expand All @@ -46,6 +79,18 @@ public ValidationEntry(ModelMetadata metadata, string key, object model)
/// <summary>
/// The model object.
/// </summary>
public object Model { get; }
public object Model
{
get
{
if (_modelAccessor != null)
{
_model = _modelAccessor();
_modelAccessor = null;
}

return _model;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -84,36 +84,20 @@ public bool MoveNext()
var propertyName = property.BinderModelName ?? property.PropertyName;
var key = ModelNames.CreatePropertyModelName(_key, propertyName);

object model;

// Our property accessors don't work on Mono 4.0.4 - see https://github.com/aspnet/External/issues/44
// This is a workaround for what the PropertyGetter does in the background.
if (IsMono)
if (_model == null)
{
// Performance: Never create a delegate when container is null.
_entry = new ValidationEntry(property, key, model: null);
}
else if (IsMono)
{
if (_model == null)
{
model = null;
}
else
{
var propertyInfo = _model.GetType().GetRuntimeProperty(property.PropertyName);
try
{
model = propertyInfo.GetValue(_model);
}
catch (TargetInvocationException ex)
{
throw ex.InnerException;
}
}
_entry = new ValidationEntry(property, key, () => GetModelOnMono(_model, property.PropertyName));
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Lost a comment somewhere: See ValidateNeverProperty_IsSkippedWithoutAccessingModel(). That and similar tests fail with NREs before the IShouldValidate implementation is called.

Copy link
Member

Choose a reason for hiding this comment

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

They fail on mono or on CLR?

Copy link
Member Author

Choose a reason for hiding this comment

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

The Model accesses fail everywhere.

But, I now suspect you were asking specifically about the Mono workaround. We have repeatedly chosen to leave all similar workarounds alone. I wasn't going to spend the time checking whether the problem has been fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Ok dope

}
else
{
model = property.PropertyGetter(_model);
_entry = new ValidationEntry(property, key, () => GetModel(_model, property));
}

_entry = new ValidationEntry(property, key, model);

return true;
}

Expand All @@ -125,6 +109,26 @@ public void Reset()
{
throw new NotImplementedException();
}

private static object GetModel(object container, ModelMetadata property)
{
return property.PropertyGetter(container);
}

// Our property accessors don't work on Mono 4.0.4 - see https://github.com/aspnet/External/issues/44
// This is a workaround for what the PropertyGetter does in the background.
private static object GetModelOnMono(object container, string propertyName)
{
var propertyInfo = container.GetType().GetRuntimeProperty(propertyName);
try
{
return propertyInfo.GetValue(container);
}
catch (TargetInvocationException ex)
{
throw ex.InnerException;
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Linq;
using System.Reflection;
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;

Expand Down Expand Up @@ -34,6 +36,24 @@ public void CreateValidationMetadata(ValidationMetadataProviderContext context)
}
}
}

// IPropertyValidationFilter attributes on a type affect properties in that type, not properties that have
// that type. Thus, we ignore context.TypeAttributes for properties and not check at all for types.
if (context.Key.MetadataKind == ModelMetadataKind.Property)
{
var validationFilter = context.PropertyAttributes.OfType<IPropertyValidationFilter>().FirstOrDefault();
if (validationFilter == null)
{
// No IPropertyValidationFilter attributes on the property.
// Check if container has such an attribute.
validationFilter = context.Key.ContainerType.GetTypeInfo()
.GetCustomAttributes(inherit: true)
.OfType<IPropertyValidationFilter>()
.FirstOrDefault();
}

context.ValidationMetadata.PropertyValidationFilter = validationFilter;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
{
/// <summary>
/// Indicates that a property should be excluded from model binding. When applied to a property, the model binding
/// system excludes that property. When applied to a type, the model binding system excludes all properties of that
/// type.
/// system excludes that property. When applied to a type, the model binding system excludes all properties that
/// type defines.
/// </summary>
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Property, AllowMultiple = false, Inherited = true)]
public sealed class BindNeverAttribute : BindingBehaviorAttribute
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
/// <summary>
/// Indicates that a property is required for model binding. When applied to a property, the model binding system
/// requires a value for that property. When applied to a type, the model binding system requires values for all
/// properties of that type.
/// properties that type defines.
/// </summary>
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Property, AllowMultiple = false, Inherited = true)]
public sealed class BindRequiredAttribute : BindingBehaviorAttribute
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;
using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata
{
Expand Down Expand Up @@ -531,6 +532,15 @@ public override string TemplateHint
}
}

/// <inheritdoc />
public override IPropertyValidationFilter PropertyValidationFilter
{
get
{
return ValidationMetadata.PropertyValidationFilter;
}
}

/// <inheritdoc />
public override bool ValidateChildren
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public class ExcludeBindingMetadataProvider : IBindingMetadataProvider
/// Creates a new <see cref="ExcludeBindingMetadataProvider"/> for the given <paramref name="type"/>.
/// </summary>
/// <param name="type">
/// The <see cref="Type"/>. All properties of this <see cref="Type"/> will have
/// The <see cref="Type"/>. All properties with this <see cref="Type"/> will have
/// <see cref="ModelMetadata.IsBindingAllowed"/> set to <c>false</c>.
/// </param>
public ExcludeBindingMetadataProvider(Type type)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Collections.Generic;
using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata
{
Expand All @@ -18,9 +19,15 @@ public class ValidationMetadata
/// </summary>
public bool? IsRequired { get; set; }

/// <summary>
/// Gets or sets an <see cref="IPropertyValidationFilter"/> implementation that indicates whether this model
/// should be validated. See <see cref="ModelMetadata.PropertyValidationFilter"/>.
/// </summary>
public IPropertyValidationFilter PropertyValidationFilter { get; set; }

/// <summary>
/// Gets or sets a value that indicates whether children of the model should be validated. If <c>null</c>
/// then <see cref="ModelMetadata.ValidateChildren"/> will be <c>true</c> if either of
/// then <see cref="ModelMetadata.ValidateChildren"/> will be <c>true</c> if either of
/// <see cref="ModelMetadata.IsComplexType"/> or <see cref="ModelMetadata.IsEnumerableType"/> is <c>true</c>;
/// <c>false</c> otherwise.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
{
/// <summary>
/// <see cref="IPropertyValidationFilter"/> implementation that unconditionally indicates a property should be
/// excluded from validation. When applied to a property, the validation system excludes that property. When
/// applied to a type, the validation system excludes all properties within that type.
/// </summary>
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Property, AllowMultiple = false, Inherited = true)]
public sealed class ValidateNeverAttribute : Attribute, IPropertyValidationFilter
{
/// <inheritdoc />
public bool ShouldValidateEntry(ValidationEntry entry, ValidationEntry parentEntry)
{
return false;
}
}
}
Loading