Removing custom ProblemDetails Converters (#46492)

* Removing custom ProblemDetails Converters * Apply suggestions from code review Co-authored-by: Safia Abdalla <safia@safia.rocks> --------- Co-authored-by: Safia Abdalla <safia@safia.rocks>
dotnet · Feb 15, 2023 · 330d246 · 330d246
1 parent 62c712c
commit 330d246
Show file tree

Hide file tree

Showing 20 changed files with 70 additions and 363 deletions.
diff --git a/src/Http/Http.Abstractions/src/Microsoft.AspNetCore.Http.Abstractions.csproj b/src/Http/Http.Abstractions/src/Microsoft.AspNetCore.Http.Abstractions.csproj
@@ -26,8 +26,6 @@ Microsoft.AspNetCore.Http.HttpResponse</Description>
     <Compile Include="$(SharedSourceRoot)PropertyHelper\**\*.cs" />
     <Compile Include="$(SharedSourceRoot)\UrlDecoder\UrlDecoder.cs" Link="UrlDecoder.cs" />
     <Compile Include="$(SharedSourceRoot)ValueTaskExtensions\**\*.cs" />
-    <Compile Include="$(SharedSourceRoot)ProblemDetails\HttpValidationProblemDetailsJsonConverter.cs" LinkBase="ProblemDetails\Converters" />
-    <Compile Include="$(SharedSourceRoot)ProblemDetails\ProblemDetailsJsonConverter.cs" LinkBase="ProblemDetails\Converters" />
     <Compile Include="$(SharedSourceRoot)Reroute.cs" />
   </ItemGroup>
 

diff --git a/src/Http/Http.Abstractions/src/ProblemDetails/HttpValidationProblemDetails.cs b/src/Http/Http.Abstractions/src/ProblemDetails/HttpValidationProblemDetails.cs
@@ -1,15 +1,13 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System.Text.Json.Serialization;
 using Microsoft.AspNetCore.Mvc;
 
 namespace Microsoft.AspNetCore.Http;
 
 /// <summary>
 /// A <see cref="ProblemDetails"/> for validation errors.
 /// </summary>
-[JsonConverter(typeof(HttpValidationProblemDetailsJsonConverter))]
 public class HttpValidationProblemDetails : ProblemDetails
 {
     /// <summary>
@@ -38,5 +36,5 @@ private HttpValidationProblemDetails(Dictionary<string, string[]> errors)
     /// <summary>
     /// Gets the validation errors associated with this instance of <see cref="HttpValidationProblemDetails"/>.
     /// </summary>
-    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);
 }
diff --git a/src/Http/Http.Abstractions/src/ProblemDetails/ProblemDetails.cs b/src/Http/Http.Abstractions/src/ProblemDetails/ProblemDetails.cs
@@ -2,14 +2,12 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Text.Json.Serialization;
-using Microsoft.AspNetCore.Http;
 
 namespace Microsoft.AspNetCore.Mvc;
 
 /// <summary>
 /// A machine-readable format for specifying errors in HTTP API responses based on <see href="https://tools.ietf.org/html/rfc7807"/>.
 /// </summary>
-[JsonConverter(typeof(ProblemDetailsJsonConverter))]
 public class ProblemDetails
 {
     /// <summary>
@@ -18,33 +16,38 @@ public class ProblemDetails
     /// (e.g., using HTML [W3C.REC-html5-20141028]). When this member is not present, its value is assumed to be
     /// "about:blank".
     /// </summary>
-    [JsonPropertyName("type")]
+    [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
+    [JsonPropertyOrder(-5)]
     public string? Type { get; set; }
 
     /// <summary>
     /// A short, human-readable summary of the problem type. It SHOULD NOT change from occurrence to occurrence
     /// of the problem, except for purposes of localization(e.g., using proactive content negotiation;
     /// see[RFC7231], Section 3.4).
     /// </summary>
-    [JsonPropertyName("title")]
+    [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
+    [JsonPropertyOrder(-4)]
     public string? Title { get; set; }
 
     /// <summary>
     /// The HTTP status code([RFC7231], Section 6) generated by the origin server for this occurrence of the problem.
     /// </summary>
-    [JsonPropertyName("status")]
+    [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
+    [JsonPropertyOrder(-3)]
     public int? Status { get; set; }
 
     /// <summary>
     /// A human-readable explanation specific to this occurrence of the problem.
     /// </summary>
-    [JsonPropertyName("detail")]
+    [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
+    [JsonPropertyOrder(-2)]
     public string? Detail { get; set; }
 
     /// <summary>
     /// A URI reference that identifies the specific occurrence of the problem. It may or may not yield further information if dereferenced.
     /// </summary>
-    [JsonPropertyName("instance")]
+    [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
+    [JsonPropertyOrder(-1)]
     public string? Instance { get; set; }
 
     /// <summary>
@@ -59,5 +62,5 @@ public class ProblemDetails
     /// In particular, complex types or collection types may not round-trip to the original type when using the built-in JSON or XML formatters.
     /// </remarks>
     [JsonExtensionData]
-    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);
 }
diff --git a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt
@@ -1,4 +1,6 @@
 #nullable enable
 Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult
 Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult.ExecuteAsync(Microsoft.AspNetCore.Http.HttpContext! httpContext) -> System.Threading.Tasks.Task!
+Microsoft.AspNetCore.Http.HttpValidationProblemDetails.Errors.set -> void
+Microsoft.AspNetCore.Mvc.ProblemDetails.Extensions.set -> void
 static Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult.Instance.get -> Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult!
diff --git a/src/Http/Http.Abstractions/test/HttpValidationProblemDetailsJsonConverterTest.cs b/src/Http/Http.Abstractions/test/HttpValidationProblemDetailsJsonConverterTest.cs
@@ -14,7 +14,6 @@ public class HttpValidationProblemDetailsJsonConverterTest
     [Fact]
     public void Write_Works()
     {
-        var converter = new HttpValidationProblemDetailsJsonConverter();
         var problemDetails = new HttpValidationProblemDetails();
 
         problemDetails.Type = "https://tools.ietf.org/html/rfc9110#section-15.5.5";
@@ -28,7 +27,7 @@ public void Write_Works()
 
         var ms = new MemoryStream();
         var writer = new Utf8JsonWriter(ms);
-        converter.Write(writer, problemDetails, JsonSerializerOptions);
+        JsonSerializer.Serialize(writer, problemDetails, JsonSerializerOptions);
         writer.Flush();
 
         ms.Seek(0, SeekOrigin.Begin);
@@ -57,13 +56,13 @@ public void Read_Works()
         var traceId = "|37dd3dd5-4a9619f953c40a16.";
         var json = $"{{\"type\":\"{type}\",\"title\":\"{title}\",\"status\":{status},\"detail\":\"{detail}\", \"instance\":\"{instance}\",\"traceId\":\"{traceId}\"," +
             "\"errors\":{\"key0\":[\"error0\"],\"key1\":[\"error1\",\"error2\"]}}";
-        var converter = new HttpValidationProblemDetailsJsonConverter();
         var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json));
         reader.Read();
 
         // Act
-        var problemDetails = converter.Read(ref reader, typeof(HttpValidationProblemDetails), JsonSerializerOptions);
+        var problemDetails = JsonSerializer.Deserialize<HttpValidationProblemDetails>(ref reader, JsonSerializerOptions);
 
+        Assert.NotNull(problemDetails);
         Assert.Equal(type, problemDetails.Type);
         Assert.Equal(title, problemDetails.Title);
         Assert.Equal(status, problemDetails.Status);
@@ -100,13 +99,13 @@ public void Read_WithSomeMissingValues_Works()
         var traceId = "|37dd3dd5-4a9619f953c40a16.";
         var json = $"{{\"type\":\"{type}\",\"title\":\"{title}\",\"status\":{status},\"traceId\":\"{traceId}\"," +
             "\"errors\":{\"key0\":[\"error0\"],\"key1\":[\"error1\",\"error2\"]}}";
-        var converter = new HttpValidationProblemDetailsJsonConverter();
         var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json));
         reader.Read();
 
         // Act
-        var problemDetails = converter.Read(ref reader, typeof(HttpValidationProblemDetails), JsonSerializerOptions);
+        var problemDetails = JsonSerializer.Deserialize<HttpValidationProblemDetails>(ref reader, JsonSerializerOptions);
 
+        Assert.NotNull(problemDetails);
         Assert.Equal(type, problemDetails.Type);
         Assert.Equal(title, problemDetails.Title);
         Assert.Equal(status, problemDetails.Status);

diff --git a/src/Http/Http.Abstractions/test/ProblemDetailsJsonConverterTest.cs b/src/Http/Http.Abstractions/test/ProblemDetailsJsonConverterTest.cs
@@ -18,13 +18,12 @@ public void Read_ThrowsIfJsonIsIncomplete()
     {
         // Arrange
         var json = "{";
-        var converter = new ProblemDetailsJsonConverter();
 
         // Act & Assert
         var ex = Record.Exception(() =>
         {
             var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json));
-            converter.Read(ref reader, typeof(ProblemDetails), JsonSerializerOptions);
+            JsonSerializer.Deserialize(ref reader, typeof(ProblemDetails), JsonSerializerOptions);;
         });
         Assert.IsAssignableFrom<JsonException>(ex);
     }
@@ -40,14 +39,14 @@ public void Read_Works()
         var instance = "http://example.com/products/14";
         var traceId = "|37dd3dd5-4a9619f953c40a16.";
         var json = $"{{\"type\":\"{type}\",\"title\":\"{title}\",\"status\":{status},\"detail\":\"{detail}\", \"instance\":\"{instance}\",\"traceId\":\"{traceId}\"}}";
-        var converter = new ProblemDetailsJsonConverter();
         var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json));
         reader.Read();
 
         // Act
-        var problemDetails = converter.Read(ref reader, typeof(ProblemDetails), JsonSerializerOptions);
+        var problemDetails = JsonSerializer.Deserialize<ProblemDetails>(ref reader, JsonSerializerOptions);
 
         //Assert
+        Assert.NotNull(problemDetails);
         Assert.Equal(type, problemDetails.Type);
         Assert.Equal(title, problemDetails.Title);
         Assert.Equal(status, problemDetails.Status);
@@ -135,14 +134,14 @@ public void Read_WithSomeMissingValues_Works()
         var status = 404;
         var traceId = "|37dd3dd5-4a9619f953c40a16.";
         var json = $"{{\"type\":\"{type}\",\"title\":\"{title}\",\"status\":{status},\"traceId\":\"{traceId}\"}}";
-        var converter = new ProblemDetailsJsonConverter();
         var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json));
         reader.Read();
 
         // Act
-        var problemDetails = converter.Read(ref reader, typeof(ProblemDetails), JsonSerializerOptions);
+        var problemDetails = JsonSerializer.Deserialize<ProblemDetails>(ref reader, JsonSerializerOptions);
 
         // Assert
+        Assert.NotNull(problemDetails);
         Assert.Equal(type, problemDetails.Type);
         Assert.Equal(title, problemDetails.Title);
         Assert.Equal(status, problemDetails.Status);
@@ -174,13 +173,12 @@ public void Write_Works()
                 }
         };
         var expected = $"{{\"type\":\"{JsonEncodedText.Encode(value.Type)}\",\"title\":\"{value.Title}\",\"status\":{value.Status},\"detail\":\"{value.Detail}\",\"instance\":\"{JsonEncodedText.Encode(value.Instance)}\",\"traceId\":\"{traceId}\",\"some-data\":[\"value1\",\"value2\"]}}";
-        var converter = new ProblemDetailsJsonConverter();
         var stream = new MemoryStream();
 
         // Act
         using (var writer = new Utf8JsonWriter(stream))
         {
-            converter.Write(writer, value, JsonSerializerOptions);
+            JsonSerializer.Serialize(writer, value, JsonSerializerOptions);
         }
 
         // Assert
@@ -199,13 +197,12 @@ public void Write_WithSomeMissingContent_Works()
             Status = 404,
         };
         var expected = $"{{\"type\":\"{JsonEncodedText.Encode(value.Type)}\",\"title\":\"{value.Title}\",\"status\":{value.Status}}}";
-        var converter = new ProblemDetailsJsonConverter();
         var stream = new MemoryStream();
 
         // Act
         using (var writer = new Utf8JsonWriter(stream))
         {
-            converter.Write(writer, value, JsonSerializerOptions);
+            JsonSerializer.Serialize(writer, value, JsonSerializerOptions);
         }
 
         // Assert
@@ -231,13 +228,12 @@ public void Write_WithNullExtensionValue_Works()
                 }
         };
         var expected = $"{{\"type\":\"{JsonEncodedText.Encode(value.Type)}\",\"title\":\"{value.Title}\",\"status\":{value.Status},\"detail\":\"{value.Detail}\",\"instance\":\"{JsonEncodedText.Encode(value.Instance)}\",\"traceId\":null,\"some-data\":[\"value1\",\"value2\"]}}";
-        var converter = new ProblemDetailsJsonConverter();
         var stream = new MemoryStream();
 
         // Act
         using (var writer = new Utf8JsonWriter(stream))
         {
-            converter.Write(writer, value, JsonSerializerOptions);
+            JsonSerializer.Serialize(writer, value, JsonSerializerOptions);
         }
 
         // Assert

diff --git a/src/Http/Http.Extensions/src/ProblemDetailsJsonContext.cs b/src/Http/Http.Extensions/src/ProblemDetailsJsonContext.cs
@@ -9,8 +9,6 @@ namespace Microsoft.AspNetCore.Http;
 
 [JsonSerializable(typeof(ProblemDetails))]
 [JsonSerializable(typeof(HttpValidationProblemDetails))]
-// ExtensionData
-[JsonSerializable(typeof(IDictionary<string, object?>))]
 // Additional values are specified on JsonSerializerContext to support some values for extensions.
 // For example, the DeveloperExceptionMiddleware serializes its complex type to JsonElement, which problem details then needs to serialize.
 [JsonSerializable(typeof(JsonElement))]

diff --git a/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt b/src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt
@@ -1,3 +1,5 @@
 #nullable enable
 static Microsoft.AspNetCore.Http.HttpRequestJsonExtensions.ReadFromJsonAsync(this Microsoft.AspNetCore.Http.HttpRequest! request, System.Text.Json.Serialization.Metadata.JsonTypeInfo! jsonTypeInfo, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.ValueTask<object?>
-static Microsoft.AspNetCore.Http.HttpResponseJsonExtensions.WriteAsJsonAsync(this Microsoft.AspNetCore.Http.HttpResponse! response, object? value, System.Text.Json.Serialization.Metadata.JsonTypeInfo! jsonTypeInfo, string? contentType = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.Task!
+static Microsoft.AspNetCore.Http.HttpResponseJsonExtensions.WriteAsJsonAsync(this Microsoft.AspNetCore.Http.HttpResponse! response, object? value, System.Text.Json.Serialization.Metadata.JsonTypeInfo! jsonTypeInfo, string? contentType = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.Task!
+Microsoft.AspNetCore.Mvc.ProblemDetails.Extensions.set -> void (forwarded, contained in Microsoft.AspNetCore.Http.Abstractions)
+Microsoft.AspNetCore.Http.HttpValidationProblemDetails.Errors.set -> void (forwarded, contained in Microsoft.AspNetCore.Http.Abstractions)
diff --git a/src/Http/Http.Extensions/test/ProblemDetailsDefaultWriterTest.cs b/src/Http/Http.Extensions/test/ProblemDetailsDefaultWriterTest.cs
@@ -17,6 +17,8 @@ namespace Microsoft.AspNetCore.Http.Extensions.Tests;
 
 public partial class DefaultProblemDetailsWriterTest
 {
+    private static readonly JsonSerializerOptions SerializerOptions = JsonOptions.DefaultSerializerOptions;
+
     [Fact]
     public async Task WriteAsync_Works()
     {
@@ -43,7 +45,7 @@ public async Task WriteAsync_Works()
 
         //Assert
         stream.Position = 0;
-        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream);
+        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream, SerializerOptions);
         Assert.NotNull(problemDetails);
         Assert.Equal(expectedProblem.Status, problemDetails.Status);
         Assert.Equal(expectedProblem.Type, problemDetails.Type);
@@ -81,7 +83,7 @@ public async Task WriteAsync_Works_WithJsonContext()
 
         //Assert
         stream.Position = 0;
-        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream);
+        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream, SerializerOptions);
         Assert.NotNull(problemDetails);
         Assert.Equal(expectedProblem.Status, problemDetails.Status);
         Assert.Equal(expectedProblem.Type, problemDetails.Type);
@@ -119,7 +121,7 @@ public async Task WriteAsync_Works_WithMultipleJsonContext()
 
         //Assert
         stream.Position = 0;
-        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream);
+        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream, SerializerOptions);
         Assert.NotNull(problemDetails);
         Assert.Equal(expectedProblem.Status, problemDetails.Status);
         Assert.Equal(expectedProblem.Type, problemDetails.Type);
@@ -156,7 +158,7 @@ public async Task WriteAsync_Works_WithHttpValidationProblemDetails()
 
         //Assert
         stream.Position = 0;
-        var problemDetails = await JsonSerializer.DeserializeAsync<HttpValidationProblemDetails>(stream);
+        var problemDetails = await JsonSerializer.DeserializeAsync<HttpValidationProblemDetails>(stream, SerializerOptions);
         Assert.NotNull(problemDetails);
         Assert.Equal(expectedProblem.Status, problemDetails.Status);
         Assert.Equal(expectedProblem.Type, problemDetails.Type);
@@ -197,7 +199,7 @@ public async Task WriteAsync_Works_WithHttpValidationProblemDetails_AndJsonConte
 
         //Assert
         stream.Position = 0;
-        var problemDetails = await JsonSerializer.DeserializeAsync<HttpValidationProblemDetails>(stream);
+        var problemDetails = await JsonSerializer.DeserializeAsync<HttpValidationProblemDetails>(stream, SerializerOptions);
         Assert.NotNull(problemDetails);
         Assert.Equal(expectedProblem.Status, problemDetails.Status);
         Assert.Equal(expectedProblem.Type, problemDetails.Type);
@@ -352,7 +354,7 @@ public async Task WriteAsync_AddExtensions()
 
         //Assert
         stream.Position = 0;
-        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream);
+        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream, SerializerOptions);
         Assert.NotNull(problemDetails);
         Assert.Collection(problemDetails.Extensions,
             (extension) =>
@@ -393,7 +395,7 @@ public async Task WriteAsync_AddExtensions_WithJsonContext()
         //Assert
         stream.Position = 0;
 
-        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream);
+        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream, options.SerializerOptions);
         Assert.NotNull(problemDetails);
 
         Assert.Collection(problemDetails.Extensions,
@@ -420,7 +422,7 @@ public async Task WriteAsync_Applies_Defaults()
 
         //Assert
         stream.Position = 0;
-        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream);
+        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream, SerializerOptions);
         Assert.NotNull(problemDetails);
         Assert.Equal(StatusCodes.Status500InternalServerError, problemDetails.Status);
         Assert.Equal("https://tools.ietf.org/html/rfc9110#section-15.6.1", problemDetails.Type);
@@ -453,7 +455,7 @@ await writer.WriteAsync(new ProblemDetailsContext()
 
         //Assert
         stream.Position = 0;
-        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream);
+        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream, SerializerOptions);
         Assert.NotNull(problemDetails);
         Assert.Equal(StatusCodes.Status406NotAcceptable, problemDetails.Status);
         Assert.Equal("https://tools.ietf.org/html/rfc9110#section-15.5.1", problemDetails.Type);
@@ -484,7 +486,7 @@ await writer.WriteAsync(new ProblemDetailsContext()
 
         //Assert
         stream.Position = 0;
-        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream);
+        var problemDetails = await JsonSerializer.DeserializeAsync<ProblemDetails>(stream, SerializerOptions);
         Assert.NotNull(problemDetails);
         Assert.Equal(statusCode, problemDetails.Status);
         Assert.Equal(type, problemDetails.Type);

diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
@@ -6019,7 +6019,7 @@ string HelloName(string name)
         await requestDelegate(httpContext);
 
         // Assert
-        var decodedResponseBody = JsonSerializer.Deserialize<Mvc.ProblemDetails>(responseBodyStream.ToArray());
+        var decodedResponseBody = JsonSerializer.Deserialize<Mvc.ProblemDetails>(responseBodyStream.ToArray(), JsonOptions.DefaultSerializerOptions);
         Assert.Equal(400, httpContext.Response.StatusCode);
         Assert.Equal("New response", decodedResponseBody!.Detail);
     }