Reopen 7439 - ModelState json serialization should be camel cased

[antonioortizpola]

Describe the bug

The issue 7439 was not completely addressed, the bot blocked the issue from commenting, but I think this main problem should be addressed:

As is, Net Core responds almost everything as camel case, except for model validations, which I think is odd:

{
  "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
  "title": "One or more validation errors occurred.",
  "status": 400,
  "traceId": "|706e0133-46db8fce16cb675d.",
  "errors": {
    "Email": [
      "The Email field is not a valid e-mail address."
    ],
    "Password": [
      "The field Password must be a string or array type with a minimum length of '8'."
    ]
  }
}

As you see, all the properties are camel case but the field names, a workaround is to use CustomProblemDetailsFactory from the comment in the same issue.

To Reproduce

Create a project and a controller with a model with validations.

Make sure the controller is annotated with [ApiController], so validation is handled automatically.

Example:

    [Authorize]
    [ApiController]
    [Route("Api/Address/[controller]")]
    public class ColonyController : Controller
    {
        
        [HttpPost]
        [Route("[action]")]
        public Task<int> Create([FromBody]ColonyToAddDto colonyToAddDto)
        {
            return Task.FromResult(0);
        }
    }

With the model

    public class ColonyToAddDto
    {
        [Required]
        [StringLength(300, MinimumLength = 3)]
        public string Name { get; set; }
        [Range(1, 99999)]
        public int PostalCode { get; set; }
        [Range(1, double.PositiveInfinity)]
        public int MunicipalityId { get; set; }
    }

Further technical details

• ASP.NET Core version 3.1

[pranavkm]

Thanks for the issue report. We'll consider this issue during our next planning. In the meanwhile, writing a custom factory as you have discovered would be the best workaround.

[mjenzen]

A different, perhaps more direct to the issue workaround (albeit, requiring more code copy/pasting) is to override the implementation of ValidationProblemDetailsJsonConverter.

The issue here seems to be that the Errors dictionary is manually being serialized within this converter and the property names are written directly to the writer without going though the JsonSerializer. This manual dictionary serialization happens in other JsonConverters within the Mvc Project as well. (ex. ProblemDetailsJsonConverter)

the core runtime still appears to be having issues with dictionary serialization casing captured here (dotnet/runtime#31849 and dotnet/runtime#33508), which prevents directly serializing the dictionary in this case, so my workaround is adapted from a comment in one of the above-mentioned issues.

Here is the relevant code chunk within the ValidationProblemDetailsJsonConverter.Write function:

            writer.WriteStartObject(Errors);
            foreach (var kvp in value.Errors)
            {
                writer.WritePropertyName(kvp.Key);
                JsonSerializer.Serialize(writer, kvp.Value, kvp.Value?.GetType() ?? typeof(object), options);
            }
            writer.WriteEndObject();

my change:

            writer.WriteStartObject(Errors);
            foreach (var kvp in value.Errors)
            {
                // Updated Line below
                writer.WritePropertyName(options?.DictionaryKeyPolicy?.ConvertName(kvp.Key) ?? kvp.Key);
                JsonSerializer.Serialize(writer, kvp.Value, kvp.Value?.GetType() ?? typeof(object), options);
            }
            writer.WriteEndObject();

As these JsonConverter classes are internal, I had to copy/paste them into my project to fix. Here are the relevant additions.

ProblemDetailsJsonConverter.cs

using Microsoft.AspNetCore.Mvc;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Threading.Tasks;

namespace Sample.Web.Mvc.Infrastructure
{
    public class ProblemDetailsJsonConverter : JsonConverter<ProblemDetails>
    {
        private static readonly JsonEncodedText Type = JsonEncodedText.Encode("type");
        private static readonly JsonEncodedText Title = JsonEncodedText.Encode("title");
        private static readonly JsonEncodedText Status = JsonEncodedText.Encode("status");
        private static readonly JsonEncodedText Detail = JsonEncodedText.Encode("detail");
        private static readonly JsonEncodedText Instance = JsonEncodedText.Encode("instance");

        public override ProblemDetails Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        {
            var problemDetails = new ProblemDetails();

            if (!reader.Read())
            {
                throw new JsonException();
            }

            while (reader.Read() && reader.TokenType != JsonTokenType.EndObject)
            {
                ReadValue(ref reader, problemDetails, options);
            }

            if (reader.TokenType != JsonTokenType.EndObject)
            {
                throw new JsonException();
            }

            return problemDetails;
        }

        public override void Write(Utf8JsonWriter writer, ProblemDetails value, JsonSerializerOptions options)
        {
            writer.WriteStartObject();
            WriteProblemDetails(writer, value, options);
            writer.WriteEndObject();
        }

        internal static void ReadValue(ref Utf8JsonReader reader, ProblemDetails value, JsonSerializerOptions options)
        {
            if (TryReadStringProperty(ref reader, Type, out var propertyValue))
            {
                value.Type = propertyValue;
            }
            else if (TryReadStringProperty(ref reader, Title, out propertyValue))
            {
                value.Title = propertyValue;
            }
            else if (TryReadStringProperty(ref reader, Detail, out propertyValue))
            {
                value.Detail = propertyValue;
            }
            else if (TryReadStringProperty(ref reader, Instance, out propertyValue))
            {
                value.Instance = propertyValue;
            }
            else if (reader.ValueTextEquals(Status.EncodedUtf8Bytes))
            {
                reader.Read();
                if (reader.TokenType == JsonTokenType.Null)
                {
                    // Nothing to do here.
                }
                else
                {
                    value.Status = reader.GetInt32();
                }
            }
            else
            {
                var key = reader.GetString();
                reader.Read();
                value.Extensions[key] = JsonSerializer.Deserialize(ref reader, typeof(object), options);
            }
        }

        internal static bool TryReadStringProperty(ref Utf8JsonReader reader, JsonEncodedText propertyName, out string value)
        {
            if (!reader.ValueTextEquals(propertyName.EncodedUtf8Bytes))
            {
                value = default;
                return false;
            }

            reader.Read();
            value = reader.GetString();
            return true;
        }

        internal static void WriteProblemDetails(Utf8JsonWriter writer, ProblemDetails value, JsonSerializerOptions options)
        {
            if (value.Type != null)
            {
                writer.WriteString(Type, value.Type);
            }

            if (value.Title != null)
            {
                writer.WriteString(Title, value.Title);
            }

            if (value.Status != null)
            {
                writer.WriteNumber(Status, value.Status.Value);
            }

            if (value.Detail != null)
            {
                writer.WriteString(Detail, value.Detail);
            }

            if (value.Instance != null)
            {
                writer.WriteString(Instance, value.Instance);
            }

            foreach (var kvp in value.Extensions)
            {
                writer.WritePropertyName(options?.DictionaryKeyPolicy?.ConvertName(kvp.Key) ?? kvp.Key);
                JsonSerializer.Serialize(writer, kvp.Value, kvp.Value?.GetType() ?? typeof(object), options);
            }
        }
    }
}

ValidationProblemDetailsJsonConverter.cs

using Microsoft.AspNetCore.Mvc;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Threading.Tasks;
using static Ostar.Web.Mvc.Infrastructure.ProblemDetailsJsonConverter;

namespace Sample.Web.Mvc.Infrastructure
{
    public class ValidationProblemDetailsJsonConverter : JsonConverter<ValidationProblemDetails>
    {
        private static readonly JsonEncodedText Errors = JsonEncodedText.Encode("errors");

        public override ValidationProblemDetails Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        {
            var problemDetails = new ValidationProblemDetails();

            if (!reader.Read())
            {
                throw new JsonException();
            }

            while (reader.Read() && reader.TokenType != JsonTokenType.EndObject)
            {
                if (reader.ValueTextEquals(Errors.EncodedUtf8Bytes))
                {
                    var errors = JsonSerializer.Deserialize<Dictionary<string, string[]>>(ref reader, options);
                    foreach (var item in errors)
                    {
                        problemDetails.Errors[item.Key] = item.Value;
                    }
                }
                else
                {
                    ReadValue(ref reader, problemDetails, options);
                }
            }

            if (reader.TokenType != JsonTokenType.EndObject)
            {
                throw new JsonException();
            }

            return problemDetails;
        }

        public override void Write(Utf8JsonWriter writer, ValidationProblemDetails value, JsonSerializerOptions options)
        {
            writer.WriteStartObject();
            WriteProblemDetails(writer, value, options);

            writer.WriteStartObject(Errors);
            foreach (var kvp in value.Errors)
            {
                writer.WritePropertyName(options?.DictionaryKeyPolicy?.ConvertName(kvp.Key) ?? kvp.Key);
                JsonSerializer.Serialize(writer, kvp.Value, kvp.Value?.GetType() ?? typeof(object), options);
            }
            writer.WriteEndObject();

            writer.WriteEndObject();
        }
    }
}

Finally register the converters within AddJsonOptions in Startup.cs

            services.AddControllersWithViews(config => config.Filters.Add<ContextManagerActionFilter>()).AddJsonOptions(opts =>
            {
                opts.JsonSerializerOptions.DictionaryKeyPolicy = JsonNamingPolicy.CamelCase;
                opts.JsonSerializerOptions.Converters.Add(new ValidationProblemDetailsJsonConverter());
            });

EDIT: Bugfix to fix property name having literal quotes around it :(

[antonioortizpola]

That is great @mjenzen, I am hitting some cases where the deserialization is wrong, this could be a step in the right direction. For my part I have already returned to newtonsoft, system.text does not seem ready for production environments yet

[bcronje]

@javiercn @pranavkm can I implore that you reconsider the affected-few and severity-nice-to-have labels and re-apply it as a bug? This issue is still present in .NET 5 and clearly does not honor the DictionaryKeyPolicy = JsonNamingPolicy.CamelCase policy. I think it is safe to say the majority of users consuming an API from javascript expects camelcase properties, so not sure why you would think it only affects a few?

See also the following two issues which has been closed by bot:

#7439
dotnet/runtime#31849

How can the below "Email" and "Password" keys not be considered a bug?

{
  "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
  "title": "One or more validation errors occurred.",
  "status": 400,
  "traceId": "|706e0133-46db8fce16cb675d.",
  "errors": {
    "Email": [
      "The Email field is not a valid e-mail address."
    ],
    "Password": [
      "The field Password must be a string or array type with a minimum length of '8'."
    ]
  }
}

[Snowirbix]

Yes please I just lost a few hours because of this bug.
The documentation states that DictionaryKeyPolicy is used to convert a IDictionary key's name to another format, such as camel-casing (serialization only)
Yet this ValidationProblemDetails does not respect this behaviour.

[pranavkm]

Thanks. Are you open to sending us a PR to change this? #17999 (comment) would be pretty close to what you might do in the in-box converters.

[mjenzen]

Thanks. Are you open to sending us a PR to change this? #17999 (comment) would be pretty close to what you might do in the in-box converters.

I could do that

[Palpie]

@mjenzen For nested validation errors like Foo.Bar this would convert to foo.Bar. When using camelcase you probably have sent something like foo.bar in your request. I would like to suggest this change to ValidationProblemDetailsJsonConverter.Write:

public override void Write(Utf8JsonWriter writer, ValidationProblemDetails value, JsonSerializerOptions options)
{
    writer.WriteStartObject();
    WriteProblemDetails(writer, value, options);

    writer.WriteStartObject(Errors);
    foreach (var kvp in value.Errors)
    {
        writer.WritePropertyName(ConvertName(kvp.Key, options));
        JsonSerializer.Serialize(writer, kvp.Value, kvp.Value?.GetType() ?? typeof(object), options);
    }

    writer.WriteEndObject();

    writer.WriteEndObject();
}

private static string ConvertName(string name, JsonSerializerOptions options) =>
    options?.DictionaryKeyPolicy != null
        ? string.Join(".", name.Split(".").Select(options.DictionaryKeyPolicy.ConvertName))
        : name;

[bcronje]

@mjenzen For nested validation errors like Foo.Bar this would convert to foo.Bar. When using camelcase you probably have sent something like foo.bar in your request. I would like to suggest this change to ValidationProblemDetailsJsonConverter.Write:

public override void Write(Utf8JsonWriter writer, ValidationProblemDetails value, JsonSerializerOptions options)
{
    writer.WriteStartObject();
    WriteProblemDetails(writer, value, options);

    writer.WriteStartObject(Errors);
    foreach (var kvp in value.Errors)
    {
        writer.WritePropertyName(ConvertName(kvp.Key, options));
        JsonSerializer.Serialize(writer, kvp.Value, kvp.Value?.GetType() ?? typeof(object), options);
    }

    writer.WriteEndObject();

    writer.WriteEndObject();
}

private static string ConvertName(string name, JsonSerializerOptions options) =>
    options?.DictionaryKeyPolicy != null
        ? string.Join(".", name.Split(".").Select(options.DictionaryKeyPolicy.ConvertName))
        : name;

This! It would be pointless to end up with foo.Bar if the expectation is camelCase. Whichever implementation is to go forward it would have to support nested fields as well e.g. foo.bar as per @Palpie proposal.

[GREsau]

I think the more general problem here is that model validation errors only include the C# property path, rather than the original input path - while it's most commonly noticed by the errors using PascalCase instead of camelCase, it's not limited to just casing:

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

    [JsonPropertyName("inner_value")]
    public Inner Inner { get; set; }
}

public class Inner
{
    [Range(1, 100)]
    [JsonPropertyName("baz")]
    public int Num { get; set; }
}

When sending { "bar": 0, "inner_value": { "baz": 0 } } to an action that takes an Outer, the 400 response contains:

{
  "errors": {
    "Num": [
      "The field Num must be between 1 and 100."
    ],
    "Inner.Num": [
      "The field Num must be between 1 and 100."
    ]
  }
}

Even though the client shouldn't know anything about "Num" or "Inner", since those are just implementation details of the API they're calling. Ideally, the response would be something more like:

{
  "errors": {
    "bar": [ // or "$.bar"
      "The field bar must be between 1 and 100."
    ],
    "inner_value.baz": [ // or "$.inner_value.baz"
      "The field baz must be between 1 and 100."
    ]
  }
}

[GREsau]

After digging into the modelstate validation internals, it seems that:

• the keys of the errors properties come from BindingMetadata.BinderModelName via:

  aspnetcore/src/Mvc/Mvc.Core/src/ModelBinding/Validation/DefaultComplexObjectValidationStrategy.cs

  Line 110 in c925f99

  var propertyName = property.BinderModelName ?? property.PropertyName;

• the property names in the validation error messages (at least the ones from DataAnnotations attributes) come from DisplayMetadata.DisplayName via:

  aspnetcore/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsModelValidator.cs

  Line 91 in c925f99

  DisplayName = metadata.GetDisplayName(),

With that in mind, I hacked together a quick fix that respects both [JsonPropertyName(...)] attributes and the configured JSON property naming policy (typically camelCase) here - https://gist.github.com/GREsau/add50755ac51500dc2ec78d2ace124e2. Bear in mind I mostly got it working through trial-and-error, so I'd caution against anyone blindly copy-pasting it into their production code 😄

[TanvirArjel]

1. The issue is a weird bug, not an enhancement.
2. The issue is not even fixed in .NET 6.0. :'(

[antonioortizpola]

I do not agree with the labels applied to this issue, as @TanvirArjel says:

• This is a bug, not an enhancement
• This does not affect a few, it affects anyone with a SPA wanting to use validation with the JSON standard camel-case.
• It is not a nice a to have, it is a bug that breaks apps and the docs, and needs time and research to apply workarounds for something really, really basic

[brunolins16]

@antonioortizpola I've just sent a PR based on @mjenzen and @Palpie suggestions to address the naming convention issue.

[brunolins16]

@antonioortizpola I have completed the PR #38853 just to support the usage of the JsonSerializerOptions.DictionaryKeyPolicy during the ModelStateErrors serialization, however, we still need work to do here to fully support the issue.

We are working on it, probably it will need an API change. As soon as we have an API suggestion or ready to review, I will let you know. Thanks.

[brunolins16]

@antonioortizpola I have submitted an API suggestion (#39010) that should cover the issue, including the scenario reported by @GREsau (#17999 (comment))

[oliverjanik]

Am I the only one here thinking this shouldn't follow DictionaryKeyPolicy ? The fact that "errors" in the response is backed by a dictionary is an implementation detail.

You can have your PropertyNamingPolicy = JsonNamingPolicy.CamelCase and still want dictionary keys untouched.

#39010 Looks like a much better solution.