Skip to content
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

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

Open
ilya-scale opened this issue Feb 13, 2024 · 22 comments
Open

Comments

@ilya-scale
Copy link

ilya-scale commented Feb 13, 2024

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

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 13, 2024
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 13, 2024
@vcsjones vcsjones removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 13, 2024
@ghost
Copy link

ghost commented Feb 13, 2024

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:

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

Author: ilya-scale
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

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.

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Feb 14, 2024
@ilya-scale
Copy link
Author

@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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 14, 2024
@eiriktsarpalis eiriktsarpalis transferred this issue from dotnet/runtime Feb 14, 2024
@ilya-scale
Copy link
Author

ilya-scale commented Feb 14, 2024

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
Copy link
Member

eiriktsarpalis commented Feb 14, 2024

I can reproduce. The issue is not related to polymorphism per se, it is a duplicate of #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?

@eiriktsarpalis eiriktsarpalis transferred this issue from dotnet/aspnetcore Feb 14, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 14, 2024
@ghost
Copy link

ghost commented Feb 14, 2024

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:

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

Author: ilya-scale
Assignees: -
Labels:

area-System.ComponentModel.DataAnnotations, untriaged, needs-area-label

Milestone: -

@eiriktsarpalis eiriktsarpalis removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 14, 2024
@eiriktsarpalis eiriktsarpalis changed the title Polymorphic properties of request models are not being validated in asp.net core DataAnnotations validation not supported for properties of nested values. Feb 14, 2024
@tarekgh tarekgh added this to the Future milestone Feb 14, 2024
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Feb 14, 2024
@ilya-scale
Copy link
Author

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
Copy link
Member

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 #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 [Extensions.Options] ValidateDataAnnotation doesn't work when classes have properties that also have DataAnnotations on their members #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
Copy link
Author

ilya-scale commented Feb 15, 2024

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
Copy link
Member

eiriktsarpalis commented Feb 15, 2024

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
Copy link
Member

tarekgh commented Feb 15, 2024

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
Copy link
Member

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
Copy link
Member

tarekgh commented Feb 15, 2024

@danroth27 How does this work with AOT?

CC @eerhardt

@danroth27
Copy link
Member

@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: dotnet/aspnetcore#28640

@tarekgh
Copy link
Member

tarekgh commented Feb 15, 2024

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

@danroth27
Copy link
Member

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
Copy link
Member

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 dotnet/aspnetcore#46349

@ilya-scale
Copy link
Author

ilya-scale commented Feb 16, 2024

@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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants