-
-
Notifications
You must be signed in to change notification settings - Fork 160
Deprecation of IsRequiredAttribute #847
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.
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
Conversation
|
The goal should be to intercept whether Some quick unpolished prototyped code that demonstrates what needs to be done: public void ConfigureMvc()
{
_mvcBuilder.AddMvcOptions(options =>
{
options.EnableEndpointRouting = true;
options.Filters.AddService<IAsyncJsonApiExceptionFilter>();
options.Filters.AddService<IAsyncQueryStringActionFilter>();
options.Filters.AddService<IAsyncConvertEmptyActionResultFilter>();
var innerProvider = options.ModelValidatorProviders.First();
options.ModelValidatorProviders.Clear();
options.ModelValidatorProviders.Add(new CustomModelValidatorProvider(innerProvider));
ConfigureMvcOptions?.Invoke(options);
});
if (_options.ValidateModelState)
{
_mvcBuilder.AddDataAnnotations();
}
}
public class CustomModelValidatorProvider : IModelValidatorProvider
{
private readonly IModelValidatorProvider _innerProvider;
public CustomModelValidatorProvider(IModelValidatorProvider innerProvider)
{
_innerProvider = innerProvider;
}
// https://github.com/dotnet/aspnetcore/blob/v3.1.4/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsModelValidatorProvider.cs
public void CreateValidators(ModelValidatorProviderContext context)
{
var meta = context.ModelMetadata;
var kind = context.ModelMetadata.MetadataKind;
foreach (ValidatorItem result in context.Results)
{
if (result.ValidatorMetadata is RequiredAttribute requiredAttribute)
{
result.Validator = new CustomRequiredValidator(requiredAttribute);
}
}
_innerProvider.CreateValidators(context);
}
}
public class CustomRequiredValidator : IModelValidator
{
private readonly RequiredAttribute _requiredAttribute;
public CustomRequiredValidator(RequiredAttribute requiredAttribute)
{
_requiredAttribute = requiredAttribute;
}
// https://github.com/dotnet/aspnetcore/blob/c565386a3ed135560bc2e9017aa54a950b4e35dd/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsModelValidator.cs
public IEnumerable<ModelValidationResult> Validate(ModelValidationContext validationContext)
{
var metadata = validationContext.ModelMetadata;
var memberName = metadata.Name;
var container = validationContext.Container;
var context = new ValidationContext(
instance: container ?? validationContext.Model ?? new object(),
serviceProvider: validationContext.ActionContext?.HttpContext?.RequestServices,
items: null)
{
DisplayName = metadata.GetDisplayName(),
MemberName = memberName
};
// TODO: Choose if we want to run or skip required-field-validation, based on request.
var result = _requiredAttribute.GetValidationResult(validationContext.Model, context);
if (result != null)
{
return new[]
{
new ModelValidationResult(memberName, result.ErrorMessage)
};
}
return Array.Empty<ModelValidationResult>();
}
}Let me know if you need me to dive in more or that you'll take it from here. |
|
After extensive discussions and a proper deep dive with @bart-degreed, I came up with an alternative solution. In the Unfortunately, this does not completely do the trick... Apart from the position in the object model graph, we also need access to We can work around this by having a custom |
bart-degreed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will discuss the rest offline.
- Only inject when ModelState active - Added support for ID validation
|
Pushed changes that simplify injection. |
Closes #834.
Deprecates
IsRequiredattribute and allows for the same functionality by using the aspnetcore built-inRequiredattribute instead.First approach
Property validators attributes like
Required,MinLengthetc are handled by DataAnnotationsModelValidatorProvider. My initial approach was to replace this provider with an adjusted provider to cover for what is needed in #671. I expected it would be possible to just add new a provider to MvcOptions.ModelValidatorProviders and remove the built-in one.This turns out not to be possible because
DataAnnotationsModelValidatorProviderinaccessible. If even if it would have been possible to replace this validator with our own, it would have been problematic because the class is bothinternalandsealed, which means we would end up copying the code which is a pain to maintain.DataAnnotationsModelValidatorProvideris inaccessible because it is not exposed to the developer throughMvcOptions.ModelValidatorProviders. Instead, it is added to the list of validator after the developer and JADNC have configuredMvcOptions(right here). It happens in theapp.UseEndpoints(endpoints => endpoints.MapControllers());call in phase 2 ofStartup.cs, and pretty much directly after it is consumed and disposed without a way for us to intercept that.### Final approachI ended up adding a customJsonApiModelValidatorProviderto the validator pipeline. Unlike the other built-inIModelValidatorProviders, this one does not create any validator objects. Instead it is only used to access theModelValidatorProviderContextwhich is shared among all validation providers. This object contains metadata about which validation attributes are to be executed. By doing a minor adjustment on that object, I trickDataAnnotationsModelValidatorProviderinto executing our ownJsonApiRequiredAttributewhen the validation for the built-inRequiredAttributeis triggered. This way, a developer doesn't have to use our custom[IsRequired]any longer.A point of attention here is the use of reflection to update a property that does not have a setter. I'm aware its a smell to mess with an internal backing field, but I think the risk of bad maintainability is relatively low because the targeted property is part of the public API and therefore the backing field is not likely to change.That being said, since interceptingDataAnnotationsModelValidatorProvideris not possible, we can't get around tampering with some internal code anyway, and I believe this one comes with the lowest risk. I also think its worth it if it means people will no longer have to use our ownIsRequiredattribute.Update: See comments in thread for final approach.