Polymorphic binder: validation not working when custom model bound is wrapped in another class

zoka-cz commented 3 years ago

Bug description

When changed the official sample PolymorphicModelBinding to:

Validate CPUIndexand ScreenSize (e.g. with Required attribute)
Wrap the Device class

Adapting the other code, to corespond with the new model structure and running the sample, the model is bound correctly, but not validated.

To Reproduce

https://github.com/zoka-cz/AspNetCore.Docs/tree/master/aspnetcore/mvc/advanced/custom-model-binding/3.0sample/PolymorphicModelBinding Just few changes to the official sample on my fork.

build and run
click the "Add device" link
Keep default values (Laptop kind, empty CPUIndex, empty ScreenSize)
Click "Add" button
Observe, that the device was correctly added, while there is [Required] attribute on the CPUIndex property, and it should gives you the validation error.
Original sample where the Device class is not wrapped, with [Required] attributes added to the mentioned properties is validated correctly.

Exceptions (if any)

No exception.

Further technical details

ASP.NET Core version 3.0.1
The IDE (VS / VS Code/ VS4Mac) you're running on, and it's version: Microsoft Visual Studio Community 2019 Version 16.8.1

ghost commented 3 years ago

Thanks for contacting us. We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

zoka-cz commented 3 years ago

I have dived into the problem once more and found few potential issues, which could lead to solution.

the official sample and either the documentation contains the error. When ValidationStateEntry is add, it is add with wrong key. I believe the key should be newBindingContext.Result.Model and not only newBindingContext.Result as is written.
I believe that ValidationVisitor.Visit causes the problem. It has inside following code:

   else if (metadata.HasValidators == false &&
         ModelState.GetFieldValidationState(key) != ModelValidationState.Invalid)

When run, and evaluated for DeviceWrapper it says it has no validators. It is incorrect,as it doesn't take into account the correct metadata for Laptop/SmartPhone type, but Device metadata (which really doesn't contain any validators).

This finding let me to the workaround: I have to ensure, that Device property of DeviceWrapper returns it has some Validators. Either by letting Device implement IValidatableObject or by decorating Device property of DeviceWrapper with some dummy validation attribute. Then the submodel is validated as expected.

But I believe there is an error in the above mentioned condition.

ghost commented 3 years ago

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.

DrakeLambert commented 3 years ago

I have a similar issue. I'm building an endpoint that accepts polymorphic data. Validations aren't being run on the derived types of my declared type.

Here are three key things that have been complicating any kind of workaround I've seen on similar issues:

My endpoint accepts JSON, not form values
The polymorpic type is not a top-level parameter. It is nested inside a complex object parameter
The object graph supports an infinite level of nesting

I have tried implementing a custom model binder like the one here. I haven't been able to crack getting this to work given the above two complications.

I can see the framework does attempt to recalculate model data base on actual types before validation, but it only checks if the top-level type is different before re-running the metadata generation. If I'm interpreting the source wrong, let me know.

More info on my scenario:

public class TopLevelRequestModel
{
    [Required]
    public string? Name { get; set; }

    public DeclaredNestedRequestModel? Nested { get; }
}

public abstract class DeclaredNestedRequestModel
{
    [Required]
    public string? ModelType { get; set; }
}

public class ActualNestedRequestModel : DeclaredNestedRequestModel
{
    [Required]
    public int? Age { get; set; }

    public DeclaredNestedRequestModel? Nested { get; set; }
}

[ApiController]
public class Controller : ControllerBase
{
    [HttpPost]
    public ActionResult Post([FromBody] TopLevelRequestModel request)
    {
        // request.Nested.Age could be null because it wasn't validated

        // ...

        return Ok();
    }
}

maarten-kieft commented 3 years ago

This finding let me to the workaround: I have to ensure, that Device property of DeviceWrapper returns it has some Validators. Either by letting Device implement IValidatableObject or by decorating Device property of DeviceWrapper with some dummy validation attribute. Then the submodel is validated as expected.

But I believe there is an error in the above mentioned condition.

Thanks for sharing. I did the same and my derived class does now fire the validate from the IValidatableObject interface, however my attributes are still not validated. Am I missing something here or?

zoka-cz commented 3 years ago

Thanks for sharing. I did the same and my derived class does now fire the validate from the IValidatableObject interface, however my attributes are still not validated. Am I missing something here or?

You do not provide too much info on that. But please check my original stackoverflow question and my answer, which is more detailed than the one in this thread. Hope it helps.

DrakeLambert commented 3 years ago

Update to my comment above:

I am now able to do polymorphic model validation with the help of Fluent Validation. I still had to add in some extra spice to get Fluent Validation to validate runtime types instead of declared types.

I had to create a special IValidationRule that would inspect the runtime type, and apply the appropriate validator.

maarten-kieft commented 3 years ago

@DrakeLambert Do you still use the existing attributes as well or just fluent validation>?

DrakeLambert commented 3 years ago

@maarten-kieft I unfortunately had to migrate fully to fluent validators, and I'm no longer using data annotations.

YuKitsune commented 2 years ago

Looks like this is still an issue, I'm running into it in .NET 6.0.300

Underfakerrip commented 1 year ago

Updated to .NET 7 and still not working. Minimal example with regex validation rule (for example not allowing '<'):

 public class WeatherForecast
    {
        public DateTime Date { get; set; }

        public int TemperatureC { get; set; }

        public int TemperatureF => 32 + (int)(TemperatureC / 0.5556);
        [RegularExpression("(^(?!\\s)[A-Za-zäÄöÖüÜßẞ\\d!?.,\\-_()/%€$@&\"#*+=:;\\\\\\s–\\[\\]]*$)")]
        public string? Summary { get; set; }
        public List<Summary> Summaries { get; set; }
    }

    public class Summary
    {
        public List<DummyBase> DummiesBase { get; set; }
        public List<Dummy> Dummies { get; set; }
    }

    public class Dummy : DummyBase
    {
        [RegularExpression("(^(?!\\s)[A-Za-zäÄöÖüÜßẞ\\d!?.,\\-_()/%€$@&\"#*+=:;\\\\\\s–\\[\\]]*$)")]
        [Required]
        [StringLength(255)]
        public string MyProperty1 { get; set; }
        [RegularExpression("(^(?!\\s)[A-Za-zäÄöÖüÜßẞ\\d!?.,\\-_()/%€$@&\"#*+=:;\\\\\\s–\\[\\]]*$)")]
        [Required]
        [StringLength(255)]
        public string MyProperty2 { get; set; }
    }

    [JsonConverter(typeof(JsonSubtypes), nameof(DummyBase.FieldType))]
    [JsonSubtypes.KnownSubType(typeof(Dummy), 0)]
    public class DummyBase
    {
        public int FieldType { get; set; }
        public int Id { get; set; }
    }

Controller:

        [HttpPost]
        [Route("{id}")]
        public void Add(int id, WeatherForecast cast)
        {
            Console.WriteLine(TryValidateModel(cast));
        }

Settings:


var builder = WebApplication.CreateBuilder(args);

builder.Services.AddControllers().AddNewtonsoftJson();

builder.Services.AddSwaggerGen(c =>
{
    c.UseOneOfForPolymorphism();
    c.UseAllOfForInheritance();
});

var app = builder.Build();

// Configure the HTTP request pipeline.
if (app.Environment.IsDevelopment())
{
    app.UseSwagger();
    app.UseSwaggerUI();
}

app.UseHttpsRedirection();

app.UseAuthorization();

app.MapControllers();

app.Run();

Now calling with:


{
  "date": "2022-11-18T10:47:14.887Z",
  "temperatureC": 0,
  "summary": "string",
  "summaries": [
    {
      "dummiesBase": [
        {
          "fieldType": 0,
          "id": 0,
          "myProperty1": "str<ing",
          "myProperty2": "string"
        }
      ],
      "dummies": [
        {
          "fieldType": 0,
          "id": 0,
          "myProperty1": "string",
          "myProperty2": "string"
        }
      ]
    }
  ]
}

This results in OK result

Calling:

{
  "date": "2022-11-18T10:47:14.887Z",
  "temperatureC": 0,
  "summary": "string",
  "summaries": [
    {
      "dummiesBase": [
        {
          "fieldType": 0,
          "id": 0,
          "myProperty1": "string",
          "myProperty2": "string"
        }
      ],
      "dummies": [
        {
          "fieldType": 0,
          "id": 0,
          "myProperty1": "str<ing",
          "myProperty2": "string"
        }
      ]
    }
  ]
}

This results in bad request though it is the same Model but not being validated in case of polymorphic binding

aldrashan commented 10 months ago

if (newBindingContext.Result.IsModelSet)
{
    var validationResults = new List<ValidationResult>();
    var validationContext = new ValidationContext(newBindingContext.Result.Model!);
    if (!Validator.TryValidateObject(newBindingContext.Result.Model!, validationContext, validationResults, true))
    {
        foreach (var result in validationResults)
        {
            if (result.MemberNames == null || !result.MemberNames.Any())
            {
                bindingContext.ModelState.AddModelError(string.Empty, result.ErrorMessage!);
            }
            else
            {
                foreach (var name in result.MemberNames)
                {
                    bindingContext.ModelState.AddModelError(ModelNames.CreatePropertyModelName(bindingContext.ModelName, name), result.ErrorMessage!);
                }
            }
        }
    }
    // Setting the ValidationState ensures properties on derived types are correctly 
    bindingContext.ValidationState[newBindingContext.Result.Model!] = new ValidationStateEntry
    {
        Metadata = newBindingContext.ModelMetadata,
    };
}

I modified the last part in the example. Not sure if this is the way to do it, but it appears this was an issue in 2012 as well and I copied it from there.

dotnet / aspnetcore