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

Use JSON Property Name attributes when creating ModelState Validation errors #39010

Closed
brunolins16 opened this issue Dec 13, 2021 · 11 comments · Fixed by #39008
Closed

Use JSON Property Name attributes when creating ModelState Validation errors #39010

brunolins16 opened this issue Dec 13, 2021 · 11 comments · Fixed by #39008
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-web-frameworks Docs This issue tracks updating documentation feature-model-binding
Milestone

Comments

@brunolins16
Copy link
Member

brunolins16 commented Dec 13, 2021

Background and Motivation

Currently, when the model validation produces the ModelErrorDictionary it will use the property name as error's key, as described in the example bellow, however in scenarios as SPA applications it can be difficult to handle the response since most of the time the property name is an implementation detail and not exactly what is expose through the JSON data.

public class Outer
{
    [Range(1, 100)]
    [JsonPropertyName("bar")]
    public int Num { get; set; }
    
    [Required]
    public InnerClass SubClass { get; set; }

    public class InnerClass
    {
       [Range(1,10)]
       public int Test1 { get; set; }
    }
}
{
  "errors": {
    "Num": [
      "The field Num must be between 1 and 100."
    ],
    "SubClass.Test1": [
      "The field Test1 must be between 1 and 100."
    ]
  }
}

Also, even when the JSON attributes are not used to define the property name the current implementation does not follow the JSON Naming Policy, that have been a complain since 2016 (#5590).

The proposal is to add two new Metadata Providers, JsonMetadataProvider and NewtonsoftJsonMetadataProvider, the last will be added when the AddNewtonsoftJson is called while the first one could be added by the customer using the MvcOptions. These new providers will be responsible for read from Newtonsoft.Json.JsonPropertyAttribute or System.Text.Json.SerializationJsonPropertyNameAttribute to set the new property ValidationMetadata.ValidationModelName that will be used during the Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationEntry creation.

When the new providers are included, the same example provided will produce the following result:

{
  "errors": {
    "bar": [
      "The field bar must be between 1 and 100."
    ],
    "subClass.test1": [
      "The field test1 must be between 1 and 100."
    ]
  }
}

Related:

Proposed API

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
   
+ /// <summary>
+ /// An implementation of <see cref="IDisplayMetadataProvider"/> and <see cref="IValidationMetadataProvider"/> for
+ /// the System.Text.Json.Serialization attribute classes.
+ /// </summary>
+ public class JsonMetadataProvider: IDisplayMetadataProvider, IValidationMetadataProvider
+ {
+     /// <summary>
+     /// Creates a new <see cref="JsonMetadataProvider"/> with the default <see cref="CamelCaseNamingStrategy"/>
+     /// </summary>
+     public JsonMetadataProvider()

+     /// <summary>
+     /// Creates a new <see cref="JsonMetadataProvider"/> with an optional <see cref="JsonNamingPolicy"/>
+     /// </summary>
+     /// <param name="namingPolicy">The <see cref="JsonNamingPolicy"/> to be used to configure the metadata provider.</param>
+     public JsonMetadataProvider(JsonNamingPolicy namingPolicy)

+     public void CreateDisplayMetadata(DisplayMetadataProviderContext context)

+     public void CreateValidationMetadata(ValidationMetadataProviderContext context)

+ }
namespace Microsoft.AspNetCore.Mvc.NewtonsoftJson;
   
+ /// <summary>
+ /// An implementation of <see cref="IDisplayMetadataProvider"/> and <see cref="IValidationMetadataProvider"/> for
+ /// the Newtonsoft.Json attribute classes.
+ /// </summary>
+ public class NewtonsoftJsonMetadataProvider : IDisplayMetadataProvider, IValidationMetadataProvider
+ {
+     /// <summary>
+     /// Creates a new <see cref="JsonMetadataProvider"/> with the default <see cref="CamelCaseNamingStrategy"/>
+     /// </summary>
+     public NewtonsoftJsonMetadataProvider()

+     /// <summary>
+     /// Initializes a new instance of <see cref="NewtonsoftJsonMetadataProvider"/> with an optional <see cref="NamingStrategy"/>
+     /// </summary>
+     /// <param name="namingStrategy">The <see cref="NamingStrategy"/> to be used to configure the metadata provider.</param>
+     public NewtonsoftJsonMetadataProvider(NamingStrategy namingStrategy)

+     public void CreateDisplayMetadata(DisplayMetadataProviderContext context)

+     public void CreateValidationMetadata(ValidationMetadataProviderContext context)

+ }
namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata
{
   public class ValidationMetadata
   {
+     public string? ValidationModelName { get; set; }
   }
}

Usage Examples

services..AddControllers(options => options.ModelMetadataDetailsProviders.Add(new JsonMetadataProvider()))

or

services.Configure<MvcOptions>(options =>
       options.ModelMetadataDetailsProviders.Add(new JsonMetadataProvider(JsonNamingPolicy.CamelCase)));

Is also possible remove the default provider, if needed:

services.Configure<MvcOptions>(options => {
               options.ModelMetadataDetailsProviders.RemoveType<NewtonsoftJsonMetadataProvider>();            
});
@brunolins16 brunolins16 added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates api-suggestion Early API idea and discussion, it is NOT ready for implementation area-web-frameworks and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Dec 13, 2021
@rafikiassumani-msft rafikiassumani-msft modified the milestones: .NET 7.0, .NET 7 Planning Dec 21, 2021
@ghost
Copy link

ghost commented Dec 21, 2021

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@rafikiassumani-msft rafikiassumani-msft added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Dec 21, 2021
@ghost
Copy link

ghost commented Dec 21, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@oliverjanik
Copy link

While this overlooked and downplayed bug is being looked at, what workarounds exist?

@pranavkm
Copy link
Contributor

pranavkm commented Jan 3, 2022

Let's seal these types and name them to be more explicit about what they are associated with (STJ vs NewtonsoftJson and that they have to do with validation)

- public class JsonMetadataProvider
+ public sealed class SystemTextJsonValidationMetadataProvider

-  public class NewtonsoftJsonMetadataProvider : IDisplayMetadataProvider, IValidationMetadataProvider
+ public sealed class NewtonsoftJsonValidationMetadataProvider : IDisplayMetadataProvider, IValidationMetadataProvider

@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jan 3, 2022
@brunolins16 brunolins16 self-assigned this Jan 3, 2022
@brunolins16 brunolins16 added this to To do in Web Frameworks (MVC) via automation Jan 3, 2022
@brunolins16 brunolins16 moved this from To do to In progress in Web Frameworks (MVC) Jan 3, 2022
@brunolins16
Copy link
Member Author

Let's seal these types and name them to be more explicit about what they are associated with (STJ vs NewtonsoftJson and that they have to do with validation)

- public class JsonMetadataProvider
+ public sealed class SystemTextJsonValidationMetadataProvider

-  public class NewtonsoftJsonMetadataProvider : IDisplayMetadataProvider, IValidationMetadataProvider
+ public sealed class NewtonsoftJsonValidationMetadataProvider : IDisplayMetadataProvider, IValidationMetadataProvider

@pranavkm just to include we discussed that will not add the providers as default. So, they will need to be included during the configuration using:

STJ

services..AddControllers(options => options.ModelMetadataDetailsProviders.Add(new SystemTextJsonValidationMetadataProvider()))

or

services.Configure<MvcOptions>(options =>
       options.ModelMetadataDetailsProviders.Add(new SystemTextJsonValidationMetadataProvider(JsonNamingPolicy.CamelCase)));

NewtonsoftJson

services..AddControllers(options => options.ModelMetadataDetailsProviders.Add(new NewtonsoftJsonValidationMetadataProvider ()))

or

services.Configure<MvcOptions>(options =>
       options.ModelMetadataDetailsProviders.Add(new NewtonsoftJsonValidationMetadataProvider (new CamelCaseNamingStrategy())));

Web Frameworks (MVC) automation moved this from In progress to Done Jan 7, 2022
@BrennanConroy BrennanConroy added the Docs This issue tracks updating documentation label Feb 3, 2022
@RehanSaeed
Copy link
Contributor

Any reason why this is not enabled by default?

@brunolins16
Copy link
Member Author

brunolins16 commented Feb 23, 2022

@RehanSaeed, the new providers will set the new ValidationModelName that is used during the ModelState error creation, and DisplayName that is used during the error creation but also in other parts of the framework, that means, in case, you have a Model, with Json attributes, that is been used for both Views and APIs (not recommended but possible) with the new providers added, the metadata will change for both cases. So, we decided to give the control when this behavior is applicable.

@niemyjski
Copy link

niemyjski commented Mar 4, 2022

@brunolins16 will there be a way to configure this without AddControllers? I'm using api controllers with ModelState driven Problem Details

Also, can this be backported to 6.x?

@brunolins16
Copy link
Member Author

@niemyjski can you share a sample app, so, I can provide what you need to do.

Right now we do not have plans to backport.

@niemyjski
Copy link

@brunolins16
Copy link
Member Author

@niemyjski you can add the provider here:

https://github.com/exceptionless/Exceptionless/blob/940a04c28c74cad34221113a865a979510c2d432/src/Exceptionless.Web/Startup.cs#L48

Example:

        services.AddControllers(o => {
            o.Filters.Add<ApiExceptionFilter>();
+           o.ModelMetadataDetailsProviders.Add(new SystemTextJsonValidationMetadataProvider());
            o.ModelBinderProviders.Insert(0, new CustomAttributesModelBinderProvider());
            o.InputFormatters.Insert(0, new RawRequestBodyFormatter());
        }).AddNewtonsoftJson(o => {
            o.SerializerSettings.DefaultValueHandling = DefaultValueHandling.Include;
            o.SerializerSettings.NullValueHandling = NullValueHandling.Include;
            o.SerializerSettings.Formatting = Formatting.Indented;
            o.SerializerSettings.ContractResolver = Core.Bootstrapper.GetJsonContractResolver(); // TODO: See if we can resolve this from the di.
        });

@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-web-frameworks Docs This issue tracks updating documentation feature-model-binding
Development

Successfully merging a pull request may close this issue.

7 participants