dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.37k stars 9.99k forks source link

Microsoft.AspNetCore.Mvc.ProblemDetails no longer serializes to JSON in RFC 7807 compliant casing in .NET 8 #53639

Closed js8080 closed 5 months ago

js8080 commented 9 months ago

Is there an existing issue for this?

Describe the bug

In .NET 7, Microsoft.AspNetCore.Mvc.ProblemDetails always properly serialized to JSON using camelCase properties which complies with RFC 7807. However, in .NET 8, the properties are serialized using whatever JsonSerializerOptions happen to be in use, which may result in incorrect casing of the property names.

One can set the overall JSON Serializer Options Naming Policy for their entire Web API to force camelCase but that may not be appropriate for ALL models being returned in one's controllers. The RFC defines the casing for this and it should be respected.

This was changed in https://github.com/dotnet/aspnetcore/pull/46492

Expected Behavior

RFC 7807 defines the JSON field names as camelCase and Microsoft.AspNetCore.Mvc.ProblemDetails is clearly intended to be used for returning errors and must therefore always use the case defined in the RFC regardless of other JsonSerializerOptions set. Alternatively, some straightforward mechanism must be made available to allow this class to be serialized in an RFC-compliant manner while the overall JsonSerializer used for Web API Controllers can be set however developers desire.

Steps To Reproduce

If one calls services.AddControllers() and sets their JSON Serializer Options PropertyNamingPolicy to null (meaning leave property names unchanged) like so:

services.AddControllers()
    .AddJsonOptions(options =>
    {
        options.JsonSerializerOptions.PropertyNamingPolicy = null;
    });

And then in their controller returns Problem(...) like so:

return Problem(detail: "Error Detail", instance: "/Instance", statusCode: StatusCodes.Status500InternalServerError, title: "Error Titlle", type: "Some Type");

The resulting JSON response will contain TitleCase properties like this:

{"Type":"Some Type","Title":"Error Title","Status":500,"Detail":"Error Detail","Instance":"/Instance"}

When the JSON representation should be:

{"type":"Some Type","title":"Error Title","status":500,"detail":"Error Detail","instance":"/Instance"}

Exceptions (if any)

No response

.NET Version

8.0.100

Anything else?

$ dotnet --info .NET SDK: Version: 8.0.100 Commit: 57efcf1350 Workload version: 8.0.100-manifests.6c33ef20

Runtime Environment: OS Name: ubuntu OS Version: 22.04 OS Platform: Linux RID: linux-x64 Base Path: /usr/share/dotnet/sdk/8.0.100/

.NET workloads installed: Workload version: 8.0.100-manifests.6c33ef20 There are no installed workloads to display.

Host: Version: 8.0.0 Architecture: x64 Commit: 5535e31a71

.NET SDKs installed: 6.0.417 [/usr/share/dotnet/sdk] 7.0.405 [/usr/share/dotnet/sdk] 8.0.100 [/usr/share/dotnet/sdk]

.NET runtimes installed: Microsoft.AspNetCore.App 6.0.25 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 7.0.15 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 8.0.0 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App] Microsoft.NETCore.App 6.0.25 [/usr/share/dotnet/shared/Microsoft.NETCore.App] Microsoft.NETCore.App 7.0.15 [/usr/share/dotnet/shared/Microsoft.NETCore.App] Microsoft.NETCore.App 8.0.0 [/usr/share/dotnet/shared/Microsoft.NETCore.App]

Other architectures found: None

Environment variables: DOTNET_ROOT [/usr/share/dotnet/]

global.json file: Not found

Learn more: https://aka.ms/dotnet/info

Download .NET: https://aka.ms/dotnet/download

EklipZgit commented 8 months ago

Actively breaking our enterprise in multiple places after .NET8 upgrade. Microsoft did not invent and does not own ProblemDetails, this object is an RFC object and always has been, all the way down to the way the extension properties are organized.

The breaking change was incorrectly documented as only a deserialization change https://learn.microsoft.com/en-us/dotnet/core/compatibility/aspnet-core/8.0/problemdetails-custom-converters

It completely fails to call out that anywhere you serialize problem details, you will no longer be serializing the RFC compliant version of the object, breaking all downstream consumers of your errors.

It also fails to call out the fact that STJ will no longer deserialize the W3C RFC spec like it did before. It says "to get the correct behavior" but fails to mention that the "correct behavior" is the ONLY valid behavior for serializing and deserializing problem details. The 'workaround' (for deserialization) also involves changing the deserialization mode for your whole application / api client that receives problem details just to deserialize an w3c RFC object correctly, which is not a good workaround, since problem details may be included as part of other responses that shouldn't have their serialization mode modified. Note that I still have not fully even found a workaround for the undocumented serialization issue.

This seems like a huge miss, whoever made this change must not have realized they were dealing with RFC objects?

I will point out that ValidationProblemDetails is also broken and also part of the same RFC in need of the same fix, as the original issue above does not mention that ValidationProblemDetails is impacted too, I believe the additional Errors property that has is required to be "errors" in the json as well. I note this because ValidationProblemDetails unlike ProblemDetails did not explicitly declare a json property hint for that Errors property, so that may have been handled purely by the now-removed converters.

captainsafia commented 8 months ago

@EklipZgit Thanks for bring this to my attention!

You're indeed correct that this code change makes our ProblemDetails implementation no longer complaint with the RFC, which is particular about the casing of the property keys using in problem details messages.

When we issued the breaking change announcement for this, we were focused on removing the dependency on custom converters in our implementation that primarily existed to work around limitations in the base STJ types.

I'm looking to fix this and will bring this to servicing for .NET 8 to avoid the breaks.

EklipZgit commented 8 months ago

Thanks. In the meantime, I've figured out a workaround that preserves the original behavior (both in the OpenApi swashbuckle spec and the actual serialization) for users of System.Text.Json, not Newtonsoft

TEMPORARY workaround for those impacted:

1: Add the converters back to your own project:

Because we cannot add [JsonPropertyName("type|title|status|...")] hints to the properties on the framework type from outside the aspnetcore assembly, we need to work around the issue by adding back in the removed converters. I suspect .NET 8 can just add back in the [JsonPropertyName("type|title|status|...")] hints and not need the custom converters anymore, but WE have no way to do that from outside the dll, so our workaround needs the custom converters back for now:

// Temporary workaround for https://github.com/dotnet/aspnetcore/issues/53639, 
//  converter code pulled from those deleted in this PR: https://github.com/dotnet/aspnetcore/pull/46492

using System.Diagnostics.CodeAnalysis;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Text.Json.Serialization.Metadata;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;

namespace {YourNameSpace};

#nullable enable

internal sealed class ProblemDetailsJsonConverter : JsonConverter<ProblemDetails>
{
    private static readonly new 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.TokenType != JsonTokenType.StartObject)
        {
            throw new JsonException("Unexpected end when reading JSON.");
        }

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

        if (reader.TokenType != JsonTokenType.EndObject)
        {
            throw new JsonException("Unexpected end when reading JSON.");
        }

        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, JsonTypeInfo extensionDataTypeInfo)
    {
        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, extensionDataTypeInfo);
        }
    }

    internal static bool TryReadStringProperty(ref Utf8JsonReader reader, JsonEncodedText propertyName, [NotNullWhen(true)] 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(kvp.Key);

            if (kvp.Value is null)
            {
                writer.WriteNullValue();
            }
            else
            {
                JsonSerializer.Serialize(writer, kvp.Value, options.GetTypeInfo(kvp.Value.GetType()));
            }
        }
    }
}

internal sealed class HttpValidationProblemDetailsJsonConverter : JsonConverter<HttpValidationProblemDetails>
{
    private static readonly JsonEncodedText Errors = JsonEncodedText.Encode("errors");

    public override HttpValidationProblemDetails Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        var problemDetails = new HttpValidationProblemDetails();
        return ReadProblemDetails(ref reader, options, problemDetails);
    }

    public static HttpValidationProblemDetails ReadProblemDetails(ref Utf8JsonReader reader, JsonSerializerOptions options, HttpValidationProblemDetails problemDetails)
    {
        if (reader.TokenType != JsonTokenType.StartObject)
        {
            throw new JsonException("Unexpected end when reading JSON.");
        }

        var objectTypeInfo = options.GetTypeInfo(typeof(object));
        while (reader.Read() && reader.TokenType != JsonTokenType.EndObject)
        {
            if (reader.ValueTextEquals(Errors.EncodedUtf8Bytes))
            {
                ReadErrors(ref reader, problemDetails.Errors);
            }
            else
            {
                ProblemDetailsJsonConverter.ReadValue(ref reader, problemDetails, objectTypeInfo);
            }
        }

        if (reader.TokenType != JsonTokenType.EndObject)
        {
            throw new JsonException("Unexpected end when reading JSON.");
        }

        return problemDetails;

        static void ReadErrors(ref Utf8JsonReader reader, IDictionary<string, string[]> errors)
        {
            if (!reader.Read())
            {
                throw new JsonException("Unexpected end when reading JSON.");
            }

            switch (reader.TokenType)
            {
                case JsonTokenType.StartObject:
                    while (reader.Read() && reader.TokenType != JsonTokenType.EndObject)
                    {
                        var name = reader.GetString()!;

                        if (!reader.Read())
                        {
                            throw new JsonException("Unexpected end when reading JSON.");
                        }

                        if (reader.TokenType == JsonTokenType.Null)
                        {
                            errors[name] = null!;
                        }
                        else
                        {
                            var values = new List<string>();
                            while (reader.Read() && reader.TokenType != JsonTokenType.EndArray)
                            {
                                values.Add(reader.GetString()!);
                            }
                            errors[name] = values.ToArray();
                        }
                    }
                    break;
                case JsonTokenType.Null:
                    return;
                default:
                    throw new JsonException($"Unexpected token when reading errors: {reader.TokenType}");
            }
        }
    }

    public override void Write(Utf8JsonWriter writer, HttpValidationProblemDetails value, JsonSerializerOptions options)
    {
        WriteProblemDetails(writer, value, options);
    }

    public static void WriteProblemDetails(Utf8JsonWriter writer, HttpValidationProblemDetails value, JsonSerializerOptions options)
    {
        writer.WriteStartObject();
        ProblemDetailsJsonConverter.WriteProblemDetails(writer, value, options);

        writer.WritePropertyName(Errors);
        WriteErrors(writer, value, options);

        writer.WriteEndObject();

        static void WriteErrors(Utf8JsonWriter writer, HttpValidationProblemDetails value, JsonSerializerOptions options)
        {
            writer.WriteStartObject();
            foreach (var kvp in value.Errors)
            {
                var name = kvp.Key;
                var errors = kvp.Value;

                writer.WritePropertyName(options.DictionaryKeyPolicy?.ConvertName(name) ?? name);
                if (errors is null)
                {
                    writer.WriteNullValue();
                }
                else
                {
                    writer.WriteStartArray();
                    foreach (var error in errors)
                    {
                        writer.WriteStringValue(error);
                    }
                    writer.WriteEndArray();
                }
            }
            writer.WriteEndObject();
        }
    }
}
  1. Register them in Startup.cs:
        services
            .AddControllers()
            .AddJsonOptions(
                options =>
                {
                    //... any other json options you already have / need
                    options.JsonSerializerOptions.Converters.Add(new ProblemDetailsJsonConverter());
                    options.JsonSerializerOptions.Converters.Add(new HttpValidationProblemDetailsJsonConverter());
                });

Now we've fixed the serialization / deserialization issue for System.Text.Json. HOWEVER, our OpenApi spec (assuming you use Swashbuckle or one of the OpenApi libraries to publish one) probably still incorrectly declares the uppercase property names, violating the RFC for any api-contract consumers that you have downstream. We still need to fix the OpenApi schema, because it doesn't know we're overriding the declared property names with our custom converter.

If you don't publish API schemas / Swagger, then you can skip the rest of the workaround.

So next:

  1. Add a Swashbuckle OpenApi schema filter to properly declare in your OpenApi documents, the correct casing that our serializer is now going to output:
    
    using System.Collections.Generic;
    using System.Linq;
    using Microsoft.AspNetCore.Mvc;
    using Microsoft.OpenApi.Models;
    using Swashbuckle.AspNetCore.SwaggerGen;

namespace {YourNameSpace};

public class ProblemDetailsWorkaroundSchemaFilter : ISchemaFilter { public static readonly string[] Replaceable = [ "Type", "Title", "Status", "Detail", "Instance", "Errors" ];

public void Apply(OpenApiSchema schema, SchemaFilterContext context)
{
    if (context.Type.IsAssignableTo(typeof(ProblemDetails)))
    {
        var newProps = new Dictionary<string, OpenApiSchema>();

        foreach ((var key, var propSchema) in schema.Properties)
        {
            if (Replaceable.Contains(key))
            {
                // Lowercase the property name. They are all all one word, so we don't have to deal with substringing to lowercase just the first letter.

                newProps[key.ToLower()] = propSchema;
            }
            else
            {
                // Preserve any custom properties people have added in their own classes derived from ProblemDetails.
                newProps[key] = propSchema;
            }
        }

        schema.Properties = newProps;
    }
}

}


4. Register that schema filter in Startup.cs:
```CS
        services.AddSwaggerGen(c =>
        {
            //.. Any other swagger gen stuff you may already have
            c.SchemaFilter<ProblemDetailsWorkaroundSchemaFilter>();
        });

This seems to be working for us so far, I'll edit this if I find anything else necessary to work around the issue in the interim while the aspnetcore team works to fix the type itself.

captainsafia commented 5 months ago

Closing as this has been fixed in .NET 9 and backported to .NET 8.