Remove (Http)(Validation)ProblemDetails converters

[brunolins16]

Hi, I think this may be related, but the casing for ProblemDetails is also inconsistent. ProblemDetails itself defaults to camel case, but as you can see above when describing the "Summary" field, it's capitalized. Shouldn't that be lower case, assuming @TanvirArjel is using the default naming policy?

I've also noticed ProblemDetails does not obey JsonOptions settings, e.g., setting

services.Configure<JsonOptions>(opt =>
{
    opt.JsonSerializerOptions.PropertyNamingPolicy = null; // don't convert to camel case
});

should give a response like

{
    "Type": "https://tools.ietf.org/html/rfc7231#section-6.5.4",
    "Title": "Not Found",
    "Status": 404,
    "Detail": "Account 1 not found.",
    "TraceId": "00-50f63c5d7921484e4797d82687f43033-915b95474bf55ff2-00"
}

but it does not, at least from what I can tell. Maybe this is by design since the spec (https://www.rfc-editor.org/rfc/rfc7807) explicitly lists the properties lower/regular camel case, but it also doesn't seem to explicitly call out camelcase must be used. I think anyone who is using problem details should be able to use whatever casing their API uses already rather than being forced to either a) have inconsistent casing returned or b) use camel case.

Originally posted by @jchoca in #43261 (comment)

Repro:: https://github.com/jchoca/ProblemDetailsJsonSettings

[brunolins16]

Background and Motivation

After investigation, based on the JsonConverter's code, listed below, was decided to keep camel case for all properties:

aspnetcore/src/Shared/ProblemDetails/ProblemDetailsJsonConverter.cs

Lines 13 to 17 in b02a2f1

	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");

While it is almost every time the expected behavior, as stated in the issue, the developers should be able to have the chance to configure the JsonOptions and this configuration reflects in the serialized ProblemDetails.

Also, the introduction of the converters (#12216), 3 years ago, was decided to work around the missing IgnoreNullValues feature (introduced here dotnet/runtime#34049).

In this proposal, I am suggesting the removal of the internal S.T.J ProblemDetails converters, replacing them with [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)], that could also contribute to avoid issues with JSON Source Generation (#43236)

This should be just an implementation change, however, while is technically possible to serialize/deserialize a property with private setter with S.T.J we will get the SYSLIB1038: System.Text.Json source generator encountered a property annotated with [JsonInclude] but with inaccessible accessors warning/error when using S.T.J source generation.

Proposed API

namespace Microsoft.AspNetCore.Mvc;

-[JsonConverter(typeof(ValidationProblemDetailsJsonConverter))]
public class ValidationProblemDetails : HttpValidationProblemDetails
{
-    public new IDictionary<string, string[]> Errors => base.Errors;
+    public new IDictionary<string, string[]> Errors { get {  return base.Errors; } set { base.Errors = value; }  }
}

-[JsonConverter(typeof(ProblemDetailsJsonConverter))]
public class ProblemDetails
{
-   [JsonPropertyName("type")]
+   [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    public string? Type { get; set; }

-   [JsonPropertyName("title")]
+   [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    public string? Title { get; set; }

-   [JsonPropertyName("status")]
+   [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    public int? Status { get; set; }

-   [JsonPropertyName("detail")]
+   [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    public string? Detail { get; set; }

-   [JsonPropertyName("instance")]
+   [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    public string? Instance { get; set; }

-   public IDictionary<string, object?> Extensions { get; } = new Dictionary<string, object?>(StringComparer.Ordinal);
+   public IDictionary<string, object?> Extensions { get; set; } = new Dictionary<string, object?>(StringComparer.Ordinal);
}

namespace Microsoft.AspNetCore.Http;

-[JsonConverter(typeof(HttpValidationProblemDetailsJsonConverter))]
public class HttpValidationProblemDetails : ProblemDetails
{
-   public IDictionary<string, string[]> Errors { get; } = new Dictionary<string, string[]>(StringComparer.Ordinal);
+   public IDictionary<string, string[]> Errors { get; set; } = new Dictionary<string, string[]>(StringComparer.Ordinal);
}

Usage Examples

As a side effect of making the properties Extensions and Errors settable:

var problem = new HttpValidationProblemDetails();

problem.Extensions = new Dictionary<string, object>()
{
    ["ext"] = "ext-value"
};

problem.Errors = new Dictionary<string, string[]> 
{ 
    ["key0"] = new string[] { "error" }
};

Not part of the API change itself but since we are removing the Converters users will be able to include all ProblemDetails types as part of JsonSerializerContext.

[JsonSerializable(typeof(ProblemDetails))]
[JsonSerializable(typeof(ValidationProblemDetails))]
[JsonSerializable(typeof(HttpValidationProblemDetails))]
public partial class TestContext : JsonSerializerContext
{
}

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 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.

[michael-budnik]

Just came across this.
I'm currently having a problem where all the built-in properties are lowercase due to JsonPropertyName attributes on ProblemDetails.
That is fine as it matches what I want.
But the problem is with Extensions property, which is an IDictionary.

There is a separate json option JsonSerializerOptions.DictionaryKeyPolicy for handling dictionary keys but I don't think I can use it here.

Due to special way Extensions work (by adding dictionary entries as properties to root problem details) I'd like them to be camel cased.
I don't want to change it globally in app obviously, but can't seem to find a way to do it just for Extensions.

I think I could just derive from ProblemDetails for each of different type of errors I'm returning but what's the point of Extensions then?

My current not ideal workaround:

Type = "https://www.rfc-editor.org/rfc/rfc7231#section-6.5.4",
Status = 404,
Instance = instanceId.ToString(),
Detail = er.Message,
Title = "Entity not found",
Extensions =
{
    { JsonNamingPolicy.CamelCase.ConvertName(nameof(er.ErrorCode)), er.ErrorCode },
    { JsonNamingPolicy.CamelCase.ConvertName(nameof(er.EntityName)), er.EntityName },
    { JsonNamingPolicy.CamelCase.ConvertName(nameof(er.EntityId)), er.EntityId }
}

[khellang]

IMO, the right thing to do for JsonExtensionData would be to follow PropertyNamingPolicy as the fact that it's implemented as a .NET dictionary is just an implementation detail. From a JSON POV it's just a bag of properties. Unfortunately that issue has been shut down due to round-tripping concerns; dotnet/runtime#31167

[ghost]

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.

[halter73]

API Review notes:

1. Is removing the [JsonPropertyName("type")] etc... attributes safe? What if someone was manually serializing without web default previously. Is this an improvement or a regression?
2. Why do we need the setter? It's required to source gen the S.T.J stuff without the converter. This seems like a problem with S.T.J source generation. Why doesn't it support serializing read-only types?

@brunolins16 Can you double check if the setters are still absolutely necessary? If so, why?

[brunolins16]

@halter73 I have tested and still the same warning when I followed what is documented here Non-public property accessors, the usage of non-public property accessors is not supported unless when using the JsonInclude attribute.

warning SYSLIB1038: The member 'ProblemDetails.MyProperty' has been annotated with the JsonIncludeAttribute but is not visible to the source generator.

I can see the red squiggles, or an explicitly exception thrown (probably the setter is not even used) in the source gen'd code, however, it successfully compiles, and the deserialization works.

Maybe someone from @dotnet/area-system-text-json can help us to understand if it is absolutely necessary.

Sample: https://github.com/brunolins16/problemdetails-sourcegen

[eiriktsarpalis]

Your example happens to work because you're prepopulating the dictionary on the property. If I change the POCO to

public class ProblemDetails
{
    [JsonExtensionData]
    [JsonInclude]
    public IDictionary<string, object>? MyProperty { get; } = null;
}

Then your app will immediately hit the exception produced by the source generator.

The function of JsonIgnoreAttribute is to instruct the serializer to bypass accessibility modifiers and try to use private or internal property setters, if available. It predates the source generator and is generally speaking problematic when used in conjunction with it. I would recommend removing it altogether and exposing a property setter that the serializer can readily use.

[brunolins16]

. I would recommend removing it altogether and exposing a property setter that the serializer can readily use.

@halter73 I believe we need to follow with what I proposed originally and add the public setter as recommended.

[ghost]

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.

[halter73]

API Review Notes:

• We're happy with the attribute changes to remove the custom JsonConverter for the ProblemDetails-derived classes.
• We should handle internal JSON converters with source gen better. There's now an issue at JsonConverterAttribute not working with source-gen when converter is internal type runtime#81703.
• Can we file an issue against S.T.J for allowing get-only [JsonInclude] properties for serialization only?

Let's get rid of these JsonConverters! We can revisit removing the setters if we get an S.T.J update that doesn't require it, but it doesn't seem too bad either way.

API Approved!

namespace Microsoft.AspNetCore.Mvc;

-[JsonConverter(typeof(ValidationProblemDetailsJsonConverter))]
public class ValidationProblemDetails : HttpValidationProblemDetails
{
-    public new IDictionary<string, string[]> Errors => base.Errors;
+    public new IDictionary<string, string[]> Errors { get {  return base.Errors; } set { base.Errors = value; }  }
}

-[JsonConverter(typeof(ProblemDetailsJsonConverter))]
public class ProblemDetails
{
-   [JsonPropertyName("type")]
+   [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    public string? Type { get; set; }

-   [JsonPropertyName("title")]
+   [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    public string? Title { get; set; }

-   [JsonPropertyName("status")]
+   [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    public int? Status { get; set; }

-   [JsonPropertyName("detail")]
+   [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    public string? Detail { get; set; }

-   [JsonPropertyName("instance")]
+   [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
    public string? Instance { get; set; }

-   public IDictionary<string, object?> Extensions { get; } = new Dictionary<string, object?>(StringComparer.Ordinal);
+   public IDictionary<string, object?> Extensions { get; set; } = new Dictionary<string, object?>(StringComparer.Ordinal);
}

namespace Microsoft.AspNetCore.Http;

-[JsonConverter(typeof(HttpValidationProblemDetailsJsonConverter))]
public class HttpValidationProblemDetails : ProblemDetails
{
-   public IDictionary<string, string[]> Errors { get; } = new Dictionary<string, string[]>(StringComparer.Ordinal);
+   public IDictionary<string, string[]> Errors { get; set; } = new Dictionary<string, string[]>(StringComparer.Ordinal);
}