dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.29k stars 4.74k forks source link

DataAnnotations validation not supported for properties of nested values. #98430

Open ilya-scale opened 9 months ago

ilya-scale commented 9 months ago

Description

I have a polymorphic class like:

public record Request
{
    public required Base Prop { get; init; } 
}

[JsonPolymorphic(TypeDiscriminatorPropertyName = "type")]
[JsonDerivedType(typeof(Derived), "derived")]
public abstract record Base
{
}

public record Derived : Base
{

    [Required, StringLength(1)]
    public string? Field { get; init; }
}

and 2 methods

[HttpPost("test-property")]
public void Test(Request request)
{

}

[HttpPost("test-request")]
public void Test(Base request)
{

}

This request to "test-property" results in 200 (which is incorrect since the field "field" is not specified):

{
    "prop": {
        "type": "derived"
    }
}

while a request to "test-request" results in 400 ("The Field field is required." which is correct):

{
    "type": "derived"
}

Reproduction Steps

  1. Use the action methods described in the description
  2. Run the described payloads on both methods

Expected behavior

Both tests should result in a 400 since field "field" is not provided.

Actual behavior

Only one of them results in 400. If the property is polymorphic, then the validation does not work (it is not only the Required attribute that is affected)

Regression?

No response

Known Workarounds

No response

Configuration

.Net 8 Mac OS ARM

Other information

No response

ghost commented 9 months ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

Issue Details
### Description I have a polymorphic class like: ```csharp public record Request { public required Base Prop { get; init; } } [JsonPolymorphic(TypeDiscriminatorPropertyName = "type")] [JsonDerivedType(typeof(Derived), "derived")] public abstract record Base { } public record Derived : Base { [Required, StringLength(1)] public string? Field { get; init; } } ``` and 2 methods ```csharp [HttpPost("test-property")] public void Test(Request request) { } [HttpPost("test-request")] public void Test(Base request) { } ``` This request to "test-property" results in 200 (which is incorrect since the field "field" is not specified): ```json { "prop": { "type": "derived" } } ``` while a request to "test-request" results in 400 ("The Field field is required." which is correct): ```json { "type": "derived" } ``` ### Reproduction Steps 1. Use the action methods described in the description 2. Run the described payloads on both methods ### Expected behavior Both tests should result in a 400 since field "field" is not provided. ### Actual behavior Only one of them results in 400. If the property is polymorphic, then the validation does not work (it is not only the Required attribute that is affected) ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration .Net 8 Mac OS ARM ### Other information _No response_
Author: ilya-scale
Assignees: -
Labels: `area-System.Text.Json`, `untriaged`
Milestone: -
eiriktsarpalis commented 9 months ago

Required is a validation attribute and not a System.Text.Json attribute (there is a JsonRequired attribute for that). Performing validation in aspnetcore is a manual step that is documented here.

ilya-scale commented 9 months ago

@eiriktsarpalis I am not sure why was this issue closed. It seems to me that it was just routed wrong. As far as I can see this has nothing to do with System.Text.Json at all. The deserialization happens properly, the validation also occurs, it just does not work on polymorphic classes fully.

Maybe the issue should just be routed to a different area?

eiriktsarpalis commented 9 months ago

Did you try following the aspnetcore documentation I shared earlier? Your example works as expected when attempting to validate it:

Base value = new Derived { Field = null };
var ctx = new ValidationContext(value);
Validator.ValidateObject(value, ctx, validateAllProperties: true); // The Field field is required.

value = new Derived { Field = "abc" };
ctx = new ValidationContext(value);
Validator.ValidateObject(value, ctx, validateAllProperties: true); // The field Field must be a string with a maximum length of 1.

public abstract record Base
{
}

public record Derived : Base
{
    [Required, StringLength(1)]
    public string? Field { get; init; }
}
ilya-scale commented 9 months ago

The validation in asp.net core happens automatically (not sure how, probably because I have inherited from the ApiController). I did not try to do the validation explicitly like you just posted.

Since you figured out that this explicit validation does work correctly, the implicit validation by Asp.Net core does not work the same way.

I suppose that it should work exactly the same way and validate all of the properties the same way the ValidationContext does?

eiriktsarpalis commented 9 months ago

I'll transfer to aspnetcore repo for further triage, but in the meantime it would help if you could share a self-contained and minimal reproduction.

ilya-scale commented 9 months ago

Here is the full example for the Program.cs

using System.ComponentModel.DataAnnotations;
using System.Text.Json.Serialization;
using Microsoft.AspNetCore.Mvc;

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddControllers();
var app = builder.Build();

app.MapControllers();

app.Run();

[ApiController]
public class TestController : ControllerBase
{
    [HttpPost("test-property")]
    public void Test(Request request) { }

    [HttpPost("test-request")]
    public void Test(Base request){ }
}

public record Request
{
    public required Base Prop { get; init; } 
}

[JsonPolymorphic(TypeDiscriminatorPropertyName = "type")]
[JsonDerivedType(typeof(Derived), "derived")]
public abstract record Base { }

public record Derived : Base
{

    [Required, StringLength(1)]
    public string? Field { get; init; }
}
eiriktsarpalis commented 9 months ago

I can reproduce. The issue is not related to polymorphism per se, it is a duplicate of https://github.com/dotnet/runtime/issues/36093. IOW validation doesn't support nested property validation. The original issue was closed because the ValidateObjectMembers attribute was added, however it seems like this is only honored by the options validator.

@tarekgh should we keep this open for potential support in the main validation library?

ghost commented 9 months ago

Tagging subscribers to this area: @dotnet/area-system-componentmodel-dataannotations See info in area-owners.md if you want to be subscribed.

Issue Details
### Description I have a polymorphic class like: ```csharp public record Request { public required Base Prop { get; init; } } [JsonPolymorphic(TypeDiscriminatorPropertyName = "type")] [JsonDerivedType(typeof(Derived), "derived")] public abstract record Base { } public record Derived : Base { [Required, StringLength(1)] public string? Field { get; init; } } ``` and 2 methods ```csharp [HttpPost("test-property")] public void Test(Request request) { } [HttpPost("test-request")] public void Test(Base request) { } ``` This request to "test-property" results in 200 (which is incorrect since the field "field" is not specified): ```json { "prop": { "type": "derived" } } ``` while a request to "test-request" results in 400 ("The Field field is required." which is correct): ```json { "type": "derived" } ``` ### Reproduction Steps 1. Use the action methods described in the description 2. Run the described payloads on both methods ### Expected behavior Both tests should result in a 400 since field "field" is not provided. ### Actual behavior Only one of them results in 400. If the property is polymorphic, then the validation does not work (it is not only the Required attribute that is affected) ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration .Net 8 Mac OS ARM ### Other information _No response_
Author: ilya-scale
Assignees: -
Labels: `area-System.ComponentModel.DataAnnotations`, `untriaged`, `needs-area-label`
Milestone: -
ilya-scale commented 9 months ago

I do believe this issue is related to polymorphism (the generic inheritance, not the System.Text.Json attributes). If in my example you chose to change the Prop property to derived, it will work properly:

public record Request
{
    public required Derived Prop { get; init; } 
}

Then the same request that gave 200 before gives this now:

"errors": {
    "Prop.Field": [
      "The Field field is required."
    ]
  },
eiriktsarpalis commented 9 months ago

Hmm, you are correct and I can reproduce this locally. My confusion stems from the fact that calling the validator directly

Request value = new() { Prop = new Derived { Field = null } };
var ctx = new ValidationContext(value);
Validator.ValidateObject(value, ctx, validateAllProperties: true); // Does not fail validation!

value = new() { Prop = new Derived { Field = "abc" } };
ctx = new ValidationContext(value);
Validator.ValidateObject(value, ctx, validateAllProperties: true); // Does not fail validation!

public record Request
{
    public required Derived Prop { get; init; }
}

public record Derived
{
    [Required, StringLength(1)]
    public string? Field { get; init; }
}

Exhibits behavior similar to https://github.com/dotnet/runtime/issues/36093, however deserializing the same shape in aspnetcore does result in validation errors occurring. This suggests to me that aspnetcore is using a separate validation engine, can anybody from @dotnet/aspnet-team confirm this?

Ultimately then we have identified two issues here:

  1. The issue as reported in https://github.com/dotnet/runtime/issues/36093 also applies to the Validator class and
  2. The validation being used by aspnetcore does traverse nested objects by default, however it fails to validate properties of polymorphic values (note that this could be by design).

I would suggest we keep this issue to track 1) and then perhaps open a new issue in aspnetcore with the repro that you shared.

ilya-scale commented 9 months ago

If I understood correctly, then ValidateObjectMembersAttribute is what has solved the issue in the 1. that you are referring to, perhaps it would work the same way with the Validator class, and then it appears to be by design and not an issue? (I would expect the full traversal by default though)

So perhaps we can do the other way around and use this issue to track 2., and if 1. appears to be an issue still, then we can open one more bug for it?

eiriktsarpalis commented 9 months ago

If I understood correctly, then ValidateObjectMembersAttribute is what has solved the issue in the 1.

No, ValidateObjectMembersAttribute is part of a separate component, the options validator, that uses the same validation attributes.

tarekgh commented 9 months ago

No, ValidateObjectMembersAttribute is part of a separate component, the options validator, that uses the same validation attributes.

Is there any problem for users to use the options validations for this scenario? is there any gap if users do that?

danroth27 commented 9 months ago

This suggests to me that aspnetcore is using a separate validation engine, can anybody from @dotnet/aspnet-team confirm this?

Correct, ASP.NET Core does its own object traversal for validation.

tarekgh commented 9 months ago

@danroth27 How does this work with AOT?

CC @eerhardt

danroth27 commented 9 months ago

@danroth27 How does this work with AOT?

We don't currently support Native AOT with our web UI frameworks like MVC, Razor Pages, and Blazor. Blazor does support AOT for WebAssembly, but we haven't yet shipped support for full object validation for Blazor: https://github.com/dotnet/aspnetcore/issues/28640

tarekgh commented 9 months ago

Thanks for the clarification, can Blazor take advantage of ValidateObjectMembersAttribute from options extensions library? or even the options validation source gen?

danroth27 commented 9 months ago

Thanks for the clarification, can Blazor take advantage of ValidateObjectMembersAttribute from options extensions library? or even the options validation source gen?

Adding @javiercn and @captainsafia for their thoughts on this.

captainsafia commented 9 months ago

Thanks for the clarification, can Blazor take advantage of ValidateObjectMembersAttribute from options extensions library? or even the options validation source gen?

The attributes and source generator are likely not reusable as-is in minimal APIs/Blazor since they need to conform to the semantics of those frameworks. We're looking at seeing what kind of code reuse is possible between the options validation generator and what we will add for minimal/Blazor via the work outlined in https://github.com/dotnet/aspnetcore/issues/46349

ilya-scale commented 9 months ago

@eiriktsarpalis Following your suggestion we can keep the Validator issue we identified in here and I have opened a new bug for the Asp.Net Core validation: dotnet/aspnetcore#54070. It got routed to System.Text.Json ironically enough since I wrote explicitly it should not be :) (I guess the bot just found out that substring and did it)

eiriktsarpalis commented 9 months ago

It got routed to System.Text.Json ironically enough since I wrote explicitly it should not be :) (I guess the bot just found out that substring and did it)

It's because dotnet/runtime doesn't contain aspnet components. That's fine, I transferred it to the relevant repo.