ProblemDetails serialization to Xml uses incorrect casing

kingmotley commented 5 years ago

Describe the bug

ValidationProblemDetails/ProblemDetails serialization to Xml uses incorrect casing

To Reproduce

Steps to reproduce the behavior: using ASP.NET Core 2.2 return a ValidationProblemDetails and you get something similar to the following:

<?xml version="1.0" encoding="utf-8"?>
<ValidationProblemDetails xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
    <Type>https://asp.net/core</Type>
    <Title>One or more validation errors occurred.</Title>
    <Status>400</Status>
    <Detail>Please refer to the errors property for additional details.</Detail>
    <Instance>/api/v1/SimpleOrders</Instance>
</ValidationProblemDetails>

Expected behavior

Conform to the example https://tools.ietf.org/html/rfc7807#page-15

<?xml version="1.0" encoding="utf-8"?>
<problem xmlns="urn:ietf:rfc:7807">
    <type>https://asp.net/core</Type>
    <title>One or more validation errors occurred.</Title>
    <status>400</Status>
    <detail>Please refer to the errors property for additional details.</Detail>
    <instance>/api/v1/SimpleOrders</Instance>
</problem>

Additional context

Xml is a case-sensitive format, so casing here is important. Additionally, the errors node is not serialized by default which leaves a poor user experience for anyone expecting Web API to work for both JSON and XML formats.

pranavkm commented 5 years ago

Is your CompatiblityVersion set to 2.2? We made some changes to the xml serialization to make the serialization closer to the spec. See https://docs.microsoft.com/en-us/aspnet/core/mvc/compatibility-version?view=aspnetcore-2.2 for documentation on specifying the compat version.

kingmotley commented 5 years ago

@pranavkm Yes, it is already set to 2.2.

            services.AddMvc(config =>
            {
                config.ReturnHttpNotAcceptable = true;
                config.SuppressBindingUndefinedValueToEnumType = true;

                // Remove the text/plaintext response format type
                config.OutputFormatters.RemoveType<StringOutputFormatter>();

                config.EnableEndpointRouting = true;

                // Add XML Content Negotiation
                config.RespectBrowserAcceptHeader = true;
                config.InputFormatters.Add(new XmlSerializerInputFormatter(config));
                config.OutputFormatters.Add(new XmlSerializerOutputFormatter(new XmlWriterSettings { Indent = true, NamespaceHandling = NamespaceHandling.OmitDuplicates }));

            }).SetCompatibilityVersion(CompatibilityVersion.Version_2_2);

pranavkm commented 5 years ago

You have to DI in the formatters rather than add the formatters manually for it to be configured correctly. Something along the lines of

services
   .AddMvc(config => { ...})
   .AddXmlSerializerFormatters()
   .SetCompatibilityVersion(CompatibilityVersion.Version_2_2);

services.Configure<MvcOptions>(options =>
{
   var outputFormatter = options.OutputFormatters.OfType<XmlSerializerOutputFormatter>().First();
   outputFormatter.WriterSettings.Indent = true;
   outputFormatter.WriterSettings.NamespaceHandling = NamespaceHandling.OmitDuplicates;

});

kingmotley commented 5 years ago

Ouch. OK, that does look better. Now I get responses like this for xml:

<problem xmlns="urn:ietf:rfc:7807">
    <detail>Please refer to the errors property for additional details.</detail>
    <instance>https://localhost:44379/api/v1/SimpleOrders</instance>
    <status>400</status>
    <title>One or more validation errors occurred.</title>
    <MVC-Errors>
        <Recipient.Zip>Zip '6009' is not valid for country 'UNITED STATES'.</Recipient.Zip>
        <Recipient.LastName>The fields FirstName and LastName must be strings with a combined maximum length of '39'.</Recipient.LastName>
        <Recipient.FirstName>The fields FirstName and LastName must be strings with a combined maximum length of '39'. This is the second error message for FirstName</Recipient.FirstName>
    </MVC-Errors>
</problem>

and this for json:

{
    "errors": {
        "Recipient.Zip": [
            "Zip '6009' is not valid for country 'UNITED STATES'."
        ],
        "Recipient.LastName": [
            "The fields FirstName and LastName must be strings with a combined maximum length of '39'."
        ],
        "Recipient.FirstName": [
            "The fields FirstName and LastName must be strings with a combined maximum length of '39'.",
            "This is the second error message for FirstName"
        ]
    },
    "title": "One or more validation errors occurred.",
    "status": 400,
    "detail": "Please refer to the errors property for additional details.",
    "instance": "https://localhost:44379/api/v1/SimpleOrders"
}

Now I just have to get swagger to agree that is what it is going to send back. If I put [ProducesResponseType((int)HttpStatusCode.BadRequest)] on my action, then swagger says this is the response:

<?xml version="1.0" encoding="UTF-8"?>
<ProblemDetails>
    <type>string</type>
    <title>string</title>
    <status>0</status>
    <detail>string</detail>
    <instance>string</instance>
    <additionalProp>Unknown Type: object</additionalProp>
</ProblemDetails>

and I put [ProducesResponseType(typeof(ValidationProblemDetails), (int)HttpStatusCode.BadRequest)] then it says this is the response:

<?xml version="1.0" encoding="UTF-8"?>
<ValidationProblemDetails>
    <errors>
        <additionalProp>Unknown Type: array</additionalProp>
    </errors>
    <type>string</type>
    <title>string</title>
    <status>0</status>
    <detail>string</detail>
    <instance>string</instance>
    <additionalProp>Unknown Type: object</additionalProp>
</ValidationProblemDetails>

kingmotley commented 5 years ago

It does seem odd that in XML, the messages are joined together by a space so that it returns a single string like this <Recipient.FirstName>The fields FirstName and LastName must be strings with a combined maximum length of '39'. This is the second error message for FirstName</Recipient.FirstName>, but with JSON, the messages are returned as an array of string like this "Recipient.FirstName": [ "The fields FirstName and LastName must be strings with a combined maximum length of '39'.", "This is the second error message for FirstName" ].

Makes creating a unified model rather difficult. Pick one and stick with it for both formats. Either return a string with all the messages combined, or return an array of strings, but don't change it based on the serializer.

pranavkm commented 5 years ago

The RFC has a very specific format for representing arrays in XML - https://tools.ietf.org/html/rfc7807#appendix-A - something that skipped due to time constraints when initially implementing this feature. Error values would need to be represented using this format since it's an array of strings:

<MVC-Errors>
    <Recipient.Zip>
        <i>Zip '6009' is not valid for country 'UNITED STATES'.</i>
    </Recipient.Zip>
    <Recipient.LastName>
        <i>The fields FirstName and LastName must be strings with a combined maximum length of '39'.</i>
    </Recipient.LastName>
    <Recipient.FirstName>
        <i>The fields FirstName and LastName must be strings with a combined maximum length of '39'.</i>
        <i>This is the second error message for FirstName</i>
    </Recipient.FirstName>
</MVC-Errors>

We'd be happy to accept a PR if this is something you would be interested in improving. Here are the relevant types:

I'm closing this issue since the initial question was resolved.

dotnet / aspnetcore