Use casing for ProblemDetails that specified by RFC #8529

pranavkm · 2018-10-01T22:14:34Z

Use JsonProperty.MemberName to specify lowercase casing for ProblemDetails properties -
https://tools.ietf.org/html/rfc7807#section-3
Use XML NS and lowercase for Xml elements specified by RFC -
https://tools.ietf.org/html/rfc7807#appendix-A

dougbu

Looks good.

However, wasn't ProblemDetails released in 2.1? If so, might need a compat switch controlling the serialization ☹️

JamesNK · 2018-10-02T03:33:45Z

src/Microsoft.AspNetCore.Mvc.Core/ProblemDetails.cs

@@ -18,33 +18,33 @@ 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>
-        [JsonProperty(NullValueHandling = NullValueHandling.Ignore)]
+        [JsonProperty(NullValueHandling = NullValueHandling.Ignore, PropertyName = "type")]


nit: order PropertyName first

pranavkm

cc @glennc

We're tweaking ProblemDetails to be more inline with the RFC. Needed your opinion on this:

pranavkm · 2018-10-02T20:26:42Z

src/Microsoft.AspNetCore.Mvc.Core/ProblemDetails.cs

        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>
-        [JsonProperty(NullValueHandling = NullValueHandling.Ignore)]
+        [JsonProperty(NullValueHandling = NullValueHandling.Ignore, PropertyName = "title")]


This may be construed as a breaking change from 2.1. if your app returned ProblemDetails and used a Json.NET Contract resolver other than the default. The value specified in PropertyName will be used ignoring the naming strategy.

e.g. If you changed MVC to use DefaultNamingStrategy instead of CamelCase:
In 2.1: { "Title": "Some title" }
In 2.2: { "title": "Some title" }

If Json.NET is used on the client and the JSON is being deserialized then this isn't a problem. Json.NET will fallback to being case-insensitive when it can't find a match.

If JavaScript is used then this could break a user.

Discussed with @Eilon and we're OK with the breaking change here due to the rarity of the scenario, the fact that it fixes a bug, and the difficultly of providing a switch for the JSON case.

It would also be possible for someone to work around by defining their own version of ProblemDetails, and using the other features we provide to plug it in.

pranavkm · 2018-10-02T20:30:49Z

src/Microsoft.AspNetCore.Mvc.Formatters.Xml/ProblemDetailsWrapper.cs

@@ -12,9 +12,11 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml
    /// <summary>
    /// Wrapper class for <see cref="Mvc.ProblemDetails"/> to enable it to be serialized by the xml formatters.
    /// </summary>
-    [XmlRoot(nameof(ProblemDetails))]
+    [XmlRoot("problem", Namespace = Namespace)]


More of the same with Xml, although it does have more than casing changes:

2.1

<ProblemDetails> <Title>Some title</Title> <Status>403</Status> <Instance>Some instance</Instance> </ProblemDetails>

2.2

<problem xmlns="urn:ietf:rfc:7807"> <title>Some title</title> <status>403</status> <instance>Some instance</instance> </problem>

Tornhoof · 2018-10-04T07:11:10Z

Just wondering is there a reason why you're using the JSON.NET specific JsonPropertyAttribute instead of the more widely accepted (by formatters that is) DataMemberAttribute?

pranavkm · 2018-10-08T19:41:06Z

@Tornhoof the Json attribute was already there. More importantly, if it isn't JSON.NET, you almost have to write a converter of some sort to support the JsonExtensionData based property that was added in 2.2.

Tornhoof · 2018-10-08T19:49:10Z

You mean the IDictionary?
Serialization should work with pretty much every Formatter, they should all support IDictionary<K,V>.
Deserialization is a different story, but that's even in the comments.
As you now also changed the XML Part, you can probably get rid of most/all of the custom XML parts, If you would actually use Datamember as the Xml DataContractSerializer is designed to use that Attribute.

pranavkm · 2018-10-08T19:56:30Z

Serialization should work with pretty much every Formatter, they should all support IDictionary<K,V>.

The spec requires that the property be serialized at the root level e.g.

{ "title": "..",  "status" : 400, "keyFromDictionary": "valueFromDictionary" }

and not as a property of a key named additionalProperties. The latter's fairly easy to do. Maybe I'm holding it wrong, but I couldn't find a trivial way to do this with Json and both kinds of Xml serialization without having to customize it.

Tornhoof · 2018-10-08T20:18:12Z

Duh, I forgot about that. The easiest way for the more advanced formatters is to derive from DynamicObject and simply return the "fixed properties" via TryGetMember etc. This should work for derserialization too. Another solution is for ProblemDetails to implement IEnumerable<KeyValuePair<string,object>>, but this will most likely only work for serialization.

Anyway it gets complicated very fast now. As long as JSON.NET is still the preferred serializer and the stuff from corefxlabs is not yet production-ready, we shouldn't really overcomplicate the problem. I rest my case. Thanks for answering my questions.

* Use JsonProperty.MemberName to specify lowercase casing for ProblemDetails properties - https://tools.ietf.org/html/rfc7807#section-3 * Use XML NS and lowercase for Xml elements specified by RFC - https://tools.ietf.org/html/rfc7807#appendix-A Fixes #8501

pranavkm · 2018-10-11T17:53:07Z

🆙 📅

The casing change is behind a compat flag now.

rynowak · 2018-10-11T17:56:44Z

src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs

@@ -442,6 +479,10 @@ public int MaxModelValidationErrors
            }
        }

+        internal event Action<MvcOptions> OnAfterPostConfigure;
+
+        internal void InvokeOnAfterPostConfigure() => OnAfterPostConfigure?.Invoke(this);


Based on our offline conversation, I tried making an MvcXmlOptions type and putting the compat flag on it, setting up the xml formatters after the compat option was initialized. However, since nothing uses the options type, the configure never executes. Leaving the flag on MvcOptions seems like the least problematic way forward. However it also keeps this event around and makes you 😭.

pranavkm · 2018-10-11T19:59:14Z

src/Microsoft.AspNetCore.Mvc.Formatters.Json/ProblemDetailsConverter.cs

+
+namespace Microsoft.AspNetCore.Mvc.Formatters.Json
+{
+    internal class ProblemDetailsConverter : JsonConverter


@JamesNK could you review the converters?

pranavkm · 2018-10-12T18:39:41Z

...pNetCore.Mvc.Formatters.Xml.Test/Internal/MvcXmlDataContractSerializerMvcOptionsSetupTest.cs

-using Microsoft.Extensions.Logging.Abstractions;
-using Xunit;
-
-namespace Microsoft.AspNetCore.Mvc.Formatters.Xml.Internal


This type was renamed to https://github.com/aspnet/Mvc/pull/8529/files#diff-e7a152988018dcb2d8180cb94a414e95R8

pranavkm

🆙 📅 part duex

Added MvcXmlOptions with compat flag
Added tests etc

rynowak · 2018-10-12T19:38:04Z

src/Microsoft.AspNetCore.Mvc.Formatters.Xml/MvcXmlOptions.cs

+        /// higher then this setting will have the value <see langword="true"/> unless explicitly configured.
+        /// </para>
+        /// </remarks>
+        public bool AllowRfc7807CompliantProblemDetailsFormat


Please add a blurb here:

Mvc/src/Microsoft.AspNetCore.Mvc.Core/CompatibilityVersion.cs

Line 72 in b156dee

Version_2_2,

rynowak · 2018-10-12T20:53:40Z

src/Microsoft.AspNetCore.Mvc.Formatters.Xml/ProblemDetailsWrapperProviderFactory.cs

+            }
+
+            return null;
+        }


pranavkm assigned dougbu Oct 1, 2018

pranavkm requested a review from rynowak October 1, 2018 22:14

rynowak approved these changes Oct 2, 2018

View reviewed changes

dougbu approved these changes Oct 2, 2018

View reviewed changes

JamesNK approved these changes Oct 2, 2018

View reviewed changes

pranavkm commented Oct 2, 2018

View reviewed changes

pranavkm assigned glennc and unassigned dougbu Oct 2, 2018

pranavkm force-pushed the prkrishn/problemdetails-metadata branch 2 times, most recently from ceec2a0 to 4ce807c Compare October 11, 2018 17:50

rynowak reviewed Oct 11, 2018

View reviewed changes

pranavkm commented Oct 11, 2018

View reviewed changes

pranavkm force-pushed the prkrishn/problemdetails-metadata branch from 4ce807c to 857e664 Compare October 11, 2018 22:33

Use compat flag to drive XML ProblemDetails formatting

6d147b0

pranavkm force-pushed the prkrishn/problemdetails-metadata branch from 857e664 to 6d147b0 Compare October 12, 2018 18:37

pranavkm commented Oct 12, 2018

View reviewed changes

rynowak reviewed Oct 12, 2018

View reviewed changes

src/Microsoft.AspNetCore.Mvc.Formatters.Xml/ProblemDetailsWrapperProviderFactory.cs

}

return null;

}

Copy link

Member

rynowak Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great 👍

rynowak approved these changes Oct 12, 2018

View reviewed changes

pranavkm closed this Oct 12, 2018

pranavkm reopened this Oct 12, 2018

pranavkm merged commit a40c1f2 into release/2.2 Oct 12, 2018

pranavkm deleted the prkrishn/problemdetails-metadata branch October 12, 2018 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use casing for ProblemDetails that specified by RFC #8529

Use casing for ProblemDetails that specified by RFC #8529

pranavkm commented Oct 1, 2018

dougbu left a comment

JamesNK Oct 2, 2018

pranavkm left a comment

pranavkm Oct 2, 2018

JamesNK Oct 2, 2018

rynowak Oct 12, 2018

pranavkm Oct 2, 2018

Tornhoof commented Oct 4, 2018

pranavkm commented Oct 8, 2018

Tornhoof commented Oct 8, 2018

pranavkm commented Oct 8, 2018

Tornhoof commented Oct 8, 2018 •

edited

Loading

pranavkm commented Oct 11, 2018

rynowak Oct 11, 2018

pranavkm Oct 11, 2018

pranavkm Oct 11, 2018

pranavkm Oct 12, 2018

pranavkm left a comment •

edited

Loading

rynowak Oct 12, 2018

rynowak Oct 12, 2018

Use casing for ProblemDetails that specified by RFC #8529

Use casing for ProblemDetails that specified by RFC #8529

Conversation

pranavkm commented Oct 1, 2018

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2.1

2.2

Tornhoof commented Oct 4, 2018

pranavkm commented Oct 8, 2018

Tornhoof commented Oct 8, 2018

pranavkm commented Oct 8, 2018

Tornhoof commented Oct 8, 2018 • edited Loading

pranavkm commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tornhoof commented Oct 8, 2018 •

edited

Loading

pranavkm left a comment •

edited

Loading