Enable Option to Use Content Negotiation for ProblemDetails

[commonsensesoftware]

Background and Motivation

The implementation of DefaultProblemDetailsWriter strictly enforces that ProblemDetails are only written if Accept is present and can be content negotiated. This is contrary to the HTTP specification. RFC 7231 §5.3.2 semantics for the Accept header states:

...disregard the header field by treating the response as if it is not subject to content negotiation.

The current behavior is very sensible as a default. It requires a client to understand they can get a ProblemDetails response; however, it is very common for API authors to not honor content negotiation for errors.

DefaultProblemDetailsWriter is internal and sealed with functionality that cannot be easily extended or customized. Ultimately, an API author simply wants to decide if content negotiation should take place for ProblemDetails and that shouldn't require customization when trivial configuration will suffice.

Proposed API

The proposed API would be to extend ProblemDetailsOptions to include a property to determine whether content negotiation should be used. The default value will be true, which retains the current behavior. If a developer sets the value to false, the expectation is that content negotiation is skipped.

public class ProblemDetailsOptions
{
    /// <summary>
    /// The operation that customizes the current <see cref="Mvc.ProblemDetails"/> instance.
    /// </summary>
    public Action<ProblemDetailsContext>? CustomizeProblemDetails { get; set; }

+   /// <summary>
+   /// Gets or sets a value indicating whether to use HTTP content negotiation.
+   /// </summary>
+   /// <value>True if the content negotiation is used; otherwise, false. The default value is <c>true</c>.</value>
+   public bool UseContentNegotiation { get; set; } = true;
}

Usage Examples

The default configuration where content negotiation is used. This is the same behavior as today.

builder.Services.AddProblemDetails();

A new configuration option which can instruct IProblemDetailsWriter implementations not to honor content negotiation.

builder.Services.AddProblemDetails(options => options.UseContentNegotiation = false);

Risks

Nothing apparent. The default configuration and behavior would be retained.

Additional Information

In accordance with RFC 7231 §5.3.2, if Accept is unspecified or is empty, then the value of ProblemDetailsOptions.UseContentNegotiation should be ignored and implied to be false. This behavior is being tracked in #45051.

[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. Where is this options used? When writing problem details via the new service for MVC, the error handler middleware, status page result, the developer exception page.
2. Is there another name for UseContentNegotiation that can default for false? IgnoreAcceptHeader?
3. Can we change the default and make a breaking change announcement? The developer exception page will try to render to HTML first, so it shouldn't break that. @commonsensesoftware Do you think it would be too breaking to change the default here?

We'll hold off on approving this until we're confident what we want the default to be. Ideally, we'll have whatever we name the option false by default.

[commonsensesoftware]

Great questions.

1. Where is this option used?

This should be useful in any scenario where ProblemDetails or ProblemDetailsOptions would be used to make a decision, which includes all of the scenarios you outlined.

I was specifically thinking in DefaultProblemDetailsWriter like this:

public DefaultProblemDetailsWriter(IOptions<ProblemDetailsOptions> options)
{
    _options = options.Value;
}

public bool CanWrite(ProblemDetailsContext context)
{
+    if (_options.SkipContentNegotiation)
+    {
+        // always return true because we aren't honoring Accept
+        return true;
+    }
...
}

However, this could just as easily be used for the ProblemDetailsFactory too.

2. Is there another name that can default to false?

• SkipContentNegotiation
• DisableContentNegotation

were a few other variants I considered. Those can be used to default to false.

I did consider IgnoreAcceptHeader, but content negotiation can be more complex that just the Accept header. I was concerned the term would be misleading, even though the alternative is a bit verbose.

Consider the following request:

POST /order HTTP/3
Host: localhost
Content-Type: application/json
Content-Length: 420

{"customer": "John Doe", "product": "Contoso soap", "quantity": 42}

If this results in a client error, say validation, and ProblemDetails were to be returned, then the current behavior would not yield a body because there is no Accept. A server could rationalize that application/problem+json is a reasonable choice because Content-Type was application/json. This would be a form of content negotiation without Accept.

UseContentNegotiation or SkipContentNegotiation would express the same intent, albeit a bit longer, but without restricting it specifically to the Accept header. I have a mild concern that if the option is too specific, then it could lead to additional options or changes in the future. I don't think we'd want to add IgnoreAcceptHeader and then potentially IgnoreContenTypeHeader later.

I'm open to other names, but that's my 2¢.

3. Can we change the default and make a breaking change announcement?

Arguably, the behavior is already broken in .NET 7; at least, using the IProblemDetailsService. I agree and think the behavior to restrict matching a ProblemDetails response to Accept (when specified) is correct, but it was unexpected. I don't immediately see a need to change the default behavior for AddProblemDetails in .NET 8.0+. Anyone that adapts to the .NET 7.0 behavior will likely be upset if things flip-flop. This new option would be congruent with how things currently behave.

As you point out, this option could be used in other places and could be tricky with the current, expected behaviors. Another alternative could be to use:

public bool? SkipContentNegotiation { get; set; }

This would be expected to have the following behaviors:

• false - do not skip content negotiation
• true - skip/ignore content negotiation
• null - the default; since it is unspecified, the option consumer decides which would allow retaining backward compatibility

An enumeration might be more intention revealing here:

public enum ProblemDetailsContentNegotiation
{
    Default = 0, // whatever the existing, default behavior is; backward compatible
    Strict  = 1, // could also be Required, Enforce
    Skip    = 2, // could also be None, Optional
}

public ProblemDetailsContentNegotiation ContentNegotiation { get; set; }

[brunolins16]

I have been thinking about this proposal and one thing that, since the initial proposal, annoyed me is that we are adding an option to ProblemDetailsOptions that will not be always honored.

Let me try explaining what I mean about "will not be always honored". We are adding the new flag SkipContentNegotiation, or IgnoreAccept, as a user my expectation is that once it is true all problem details generation, using the IProblemDetailsService will skip the Accept header validation, however, it will only be true for our DefaultProblemDetailsWriter and we cannot guarantee that a custom IProblemDetailsWriter will follow this flag or not.

I was thinking about naming it something like AlwaysFallbackToDefaultWriter, the name is bad 😂 but the idea is to be something that clear indicates a Problem Details will be always generated, using the default writer, when no other writer is able to handle it. Basically, when true we will never call CanWrite for DefaultProblemDetailsWriter and write directly.

Eg.:

// Try to write using all registered writers (not including the defaullt writer)
// sequentially and stop at the first one that
// `canWrite.
for (var i = 0; i < _writers.Length; i++)
{
    var selectedWriter = _writers[i];
    if (selectedWriter.CanWrite(context))
    {
        return selectedWriter.WriteAsync(context);
    }
}

if (_options.AlwaysFallbackToDefaultWriter || _defaultWriter.CanWrite(context))
{
    return _defaultWriter.WriteAsync(context);
}

@commonsensesoftware Thoughts?

[commonsensesoftware]

@brunolins16 ignoring options is always a possibility. Extenders don't have to honor the JsonOptions either. They might elect not to or don't realize they need to nor how (e.g. via DI). This falls into the category of caveat emptor IMHO. There is a certain amount of knowledge expected when you extend or customize things. The most rudimentary of testing should equally reveal if the correct behavior has been implemented.

I think what we are collectively saying and agreeing to, in somewhat different ways, is that content negotiation validation in conjunction with the proposed option needs to be lifted out of the internal space. There are two ways I can currently think of that this could be done.

Option 1

Refactor the validation logic for each type of media type into an interface that would have a concrete implementation for 2 default rules:

• Accept
• Content-Type

The ProblemDetailsOptions could then add these rules in its default initialization. This would provide full access to the rules and logic in a reusable way as well as allow extenders to remove the rules or add, new custom implementions.

While this would work, I concede that it's probably over-engineered, which is why I haven't bothered showcasing what the code might look like (but I can 😉). These are likely the only two content negotiation scenarios that will ever exist.

Option 2

Create a new base class that has the common logic baked into. Something like:

public abstract class ProblemDetailsWriterBase : IProblemDetailsWriter
{
    private static readonly MediaTypeHeaderValue _jsonMediaType = new("application/json");
    private static readonly MediaTypeHeaderValue _problemDetailsJsonMediaType = new("application/problem+json");
    private readonly IOptions<ProblemDetailsOptions> _options;

    protected ProblemDetailsWriterBase(IOptions<ProblemDetailsOptions> options) => _options = options;

    protected ProblemDetailsOptions Options => _options.Value;

    bool IProblemDetailsWriter.CanWrite(ProblemDetailsContext context) =>
        CanNegotiateContent(context) && CanWrite(context);

    public abstract ValueTask WriteAsync(ProblemDetailsContext context);

    protected virtual bool CanWrite(ProblemDetailsContext context) => true;

    protected virtual bool CanNegotiateContent(ProblemDetailsContext context)
    {
        if (Options.SkipContentNegotiation)
        {
            return true;
        }
        
        var headers = context.HttpContext.Request.Headers;
        var accept = headers.Accept.GetList<MediaTypeHeaderValue>();

        if (IsAcceptable(accept))
        {
            return true;
        }

        // if Accept isn't specified, infer from Content-Type if possible
        if (accept.Count == 0 &&
            headers.Get<MediaTypeHeaderValue>(HeaderNames.ContentType) is MediaTypeHeaderValue contentType)
        {
            return IsSupportedMediaType(contentType);
        }

        return false;
    }

    protected bool IsAcceptable(IReadOnlyList<MediaTypeHeaderValue> accept)
    {
        // Based on https://www.rfc-editor.org/rfc/rfc7231#section-5.3.2 a request
        // without the Accept header implies that the user agent
        // will accept any media type in response
        if (accept.Count == 0)
        {
            return true;
        }

        for (var i = 0; i < accept.Count; i++)
        {
            var value = accept[i];

            if (_jsonMediaType.IsSubsetOf(value) ||
                _problemDetailsJsonMediaType.IsSubsetOf(value))
            {
                return true;
            }
        }

        return false;
    }

    protected bool IsSupportedMediaType(MediaTypeHeaderValue contentType) =>
        jsonMediaType.IsSubsetOf( contentType )
        || problemDetailsJsonMediaType.IsSubsetOf( contentType );
}

Using this approach, all of the logic in DefaultProblemDetailsWriter.CanWrite is moved to the base class. Extenders can (and likely will) start with the ProblemDetailsWriterBase, which will enforce whether content negotiation is valid before checking any additional validation. The door is still open to implement IProblemDetailsWriter directly, but then we circle back to caveat emptor and, as the extending developer, you're responsible for doing the right thing.

[brunolins16]

@commonsensesoftware appreciate your thoughts and option 2 is not bad but I really believe we could try simplifying instead over engineering it. We can have the DefaultProblemDetailsWriter public and probably change some methods to virtual, however, I think we should try to keep the design simply and just allow an always skip/validation behavior or maybe a delegate that could be customized.

[commonsensesoftware]

@brunolins16 I agree - no need to over-engineer things. Rolling up things into the DefaultProblemDetailsWriter and making it extensible would be great. I suspect it would probably look like the example provided.

Just as things can be over-engineered, they can also be over-configured. Having some kind of delegate is unnecessary IMO. I suspect most people will not change whatever the default setting settles on. For those that do change it, they will likely only want all validation or none - as originally described. Anyone that needs or wants to configure things further should be expected to understand the 🐰 🕳️ they are going down. This is where keeping it simple and enforcing specific behaviors are at odds. Expecting a developer to know and honor the option when they down this path is not unreasonable to me. Similarly extending the out-of-the-box functionality should be possible without having to reimplement everything (as is currently the case).

Ultimately, that's why the original proposal is a single, simple option setting that you can configure. Honoring the setting should be the onus of the developers that extend the functionality as is the case with every other setting.

[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.

[adityamandaleeka]

Looks like the next step here is to revise the proposed API and decide how to proceed. Moving this back to .NET 8 planning; the team is fairly slammed at the moment so we don't expect to have cycles for this in the near term.

[commonsensesoftware]

@adityamandaleeka The primary work is settling on the the API design, which seems largely to be naming. The effort to execute is trivial IMHO and I'm happy to put the PR for it once some decisions are locked. If there is something more I can do to help move things along, just let me know.

[source-studio]

We're experiencing an issue that might be related to this and also #45051.

We've enabled versioning on our Minimal API using the MediaTypeApiVersionReader approach, which stipulates the inclusion of a version parameter within the Accept header, such as v=1.0. A typical Accept header would therefore look something like: application/json; v=1.0.

However, since this is a more specific media type than application/json, the call to _jsonMediaType.IsSubsetOf(value) inRfc7231ProblemDetailsWriter returns false, which means the problem details response is never written. I actually believe this was the original cause of #45051, as it describes the exact scenario we're facing.

I guess it might be necessary to permit more specific media types by allowing for this in the conditional checks, for example:

if (_jsonMediaType.IsSubsetOf(acceptHeaderValue) ||
    acceptHeaderValue.IsSubsetOf(_jsonMediaType) ||
    _problemDetailsJsonMediaType.IsSubsetOf(acceptHeaderValue) ||
    acceptHeaderValue.IsSubsetOf(_problemDetailsJsonMediaType))
{
    return true;
}

This is unfortunately blocking us from using both Minimal API versioning with a media type header in conjunction with problem details responses, so we'll have to fall back to another version reader implementation until this is addressed.

[commonsensesoftware]

@source-studio I have confirmed that this is, in fact, a 🐞 . The current adapter logic is backward. It should be:

if ( acceptHeaderValue.IsSubsetOf( jsonMediaType ) ||
     acceptHeaderValue.IsSubsetOf( problemDetailsJsonMediaType ) )
{
    return true;
}

The naming is a feels off IMHO. application/json;v=1.0 feels like a superset of application/json as opposed to a subset. Upon reading the documentation more closely, it is clear that it's backward. I was able to repro the scenario and verify things should be inverted.

Please file a new bug in the API Versioning repo. I'll have it patched for you ASAP.