-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Comments
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsDescriptionI 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
Expected behaviorBoth tests should result in a 400 since field "field" is not provided. Actual behaviorOnly 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 WorkaroundsNo response Configuration.Net 8 Other informationNo response
|
|
@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? |
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; }
} |
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? |
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. |
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; }
} |
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 @tarekgh should we keep this open for potential support in the main validation library? |
Tagging subscribers to this area: @dotnet/area-system-componentmodel-dataannotations Issue DetailsDescriptionI 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
Expected behaviorBoth tests should result in a 400 since field "field" is not provided. Actual behaviorOnly 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 WorkaroundsNo response Configuration.Net 8 Other informationNo response
|
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 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."
]
}, |
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:
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. |
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 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? |
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? |
Correct, ASP.NET Core does its own object traversal for validation. |
@danroth27 How does this work with AOT? CC @eerhardt |
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 |
Thanks for the clarification, can Blazor take advantage of |
Adding @javiercn and @captainsafia for their thoughts on this. |
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 |
@eiriktsarpalis Following your suggestion we can keep the |
It's because |
Description
I have a polymorphic class like:
and 2 methods
This request to "test-property" results in 200 (which is incorrect since the field "field" is not specified):
while a request to "test-request" results in 400 ("The Field field is required." which is correct):
Reproduction Steps
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
The text was updated successfully, but these errors were encountered: