Skip to content

Expose IoC to IPropertyValidationFilter.ShouldValidateEntry for more flexible ModelState validation #26580

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

Open
maurei opened this issue Oct 3, 2020 · 3 comments
Labels
affected-few This issue impacts only small number of customers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-model-binding severity-minor This label is used by an internal tool
Milestone

Comments

@maurei
Copy link

maurei commented Oct 3, 2020

The problem

In the JsonApiDotNetCore framework which I'm maintaining, there is a problem with ModelState validation when using the RequiredAttribute. The framework implements the json:api specification which allows for partial patching, and validation does not work well with that.

Consider the model Article with a to-one relationship to Author, with RequiredAttributes on all properties. Then, for example, in a PATCH /articles request , we require the following behaviour

  • Properties Article.* should ONLY be validated if they are targeted explicitly by the request body
  • Properties Article.Author.* should NEVER be validated because in the json:api spec it is supported to assign relationships up to 1 layer deep, but it is not allowed to simultaneously update properties of that relationship

On the other hand, for a POST /articles request, all Article.* properties should always be validated.

I have tried to implement this behaviour by using IPropertyValidationFilter.ShouldValidateEntry(ValidationEntry entry, ValidationEntry parentEntry) (this is my custom IModelMetadataProvider that does the trick).

Currently IPropertyValidationFilter.ShouldValidateEntry(ValidationEntry entry, ValidationEntry parentEntry) allows me to deduce where I am in the object model graph through the Entry.Key property. With this I can successfully ignore Article.Author.*. But I also need access to HttpContextAccessor to figure out if the request is PATCH/POST. This is currently not possible because the IoC is not passed along.

Additionally I also need access to IoC for other json:api specific metadata about the request scope. Eg. if the endpoint is a ../relationships/... endpoint, in which case validation should never occur, regardless if POST or PATCH.

Solution

I would love to see the IoC being exposed to IPropertyValidationFilter.

Proposed approach:

In IPropertyValidationFilter.cs:

public interface IPropertyValidationFilter
{
    bool ShouldValidateEntry(PropertyValidationFilterContext filterContext);
}

Then, PropertyValidationFilterContext would look something like:

public class PropertyValidationFilterContext
{
    private readonly IServiceProvider _serviceProvider;
    public ValidationEntry Entry { get; }
    public ValidationEntry ParentyEntry { get; }

    public PropertyValidationFilterContext(ValidationEntry entry, ValidationEntry parentyEntry, ActionContext actionContext)
    {
        Entry = entry;
        ParentyEntry = parentyEntry;
        _serviceProvider = actionContext?.HttpContext?.RequestServices;
    }

    public TService GetService<TService>() => _serviceProvider.GetService<TService>();
}

And for ValidationVisitor.VisitChildren(IValidationStrategy):

protected override bool VisitChildren(IValidationStrategy strategy)
{
    var isValid = true;
    var enumerator = strategy.GetChildren(Metadata, Key, Model);
    var parentEntry = new ValidationEntry(Metadata, Key, Model);

    while (enumerator.MoveNext())
    {
        var entry = enumerator.Current;
        var metadata = entry.Metadata;
        var key = entry.Key;

        if (metadata.PropertyValidationFilter?.ShouldValidateEntry(new PropertyValidationFilterContext(entry, parentEntry, Context)) == false)
        {
            SuppressValidation(key);
            continue;
        }

        isValid &= Visit(metadata, key, entry.Model);
    }

    return isValid;
}

Additional context

Currently I can work around this problem by using a custom ValidationVisitor that calls my IPropertyValidationFilter with a reference to IServiceProvider. This is a bit tedious because the only way to have my application use this one instead of the built-in visitor implementation requires me to register a custom ObjectModelValidator. For this implementation I need pretty much everything from DefaultObjectValidator but this type is internal, so I need to copy-paste its internals which I think is not a good thing to do.

If the idea is approved, I would love to make a PR for this myself.

Related issue in JADNC framework: json-api-dotnet/JsonApiDotNetCore#847

@maurei maurei changed the title Expose IoC to IPropertyValidationFilter.ShouldValidateEntry for improved ModelState validation Expose IoC to IPropertyValidationFilter.ShouldValidateEntry for more flexible ModelState validation Oct 3, 2020
@mkArtakMSFT mkArtakMSFT added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Oct 5, 2020
@maurei
Copy link
Author

maurei commented Oct 5, 2020

I realised that I can just register and inject HttpContextAccessor and instantiate my IPropertyValidationFilter with it. That allows me to get the context data I need.

@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Oct 6, 2020
@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone Oct 6, 2020
@ghost
Copy link

ghost commented Oct 6, 2020

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@SteveSandersonMS SteveSandersonMS added affected-few This issue impacts only small number of customers severity-minor This label is used by an internal tool labels Oct 7, 2020 — with ASP.NET Core Issue Ranking
@yonail
Copy link

yonail commented Jun 21, 2021

+1

I'm also facing the same issue, I need to access the IOptions from IoC to determine whether or not a property of the model needs to be validated.

@mkArtakMSFT mkArtakMSFT added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Oct 14, 2021
@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-few This issue impacts only small number of customers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-model-binding severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

6 participants