Skip to content

Enable Option to Use Content Negotiation for ProblemDetails #45159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
commonsensesoftware opened this issue Nov 17, 2022 · 12 comments
Open

Enable Option to Use Content Negotiation for ProblemDetails #45159

commonsensesoftware opened this issue Nov 17, 2022 · 12 comments
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Milestone

Comments

@commonsensesoftware
Copy link

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.

@commonsensesoftware commonsensesoftware added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 17, 2022
@brunolins16 brunolins16 added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Nov 18, 2022
@rafikiassumani-msft rafikiassumani-msft added this to the 8.0-preview1 milestone Nov 23, 2022
@rafikiassumani-msft rafikiassumani-msft added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Nov 23, 2022
@ghost
Copy link

ghost commented Nov 23, 2022

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
Copy link
Member

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.

@halter73 halter73 added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Nov 28, 2022
@commonsensesoftware
Copy link
Author

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.

  1. 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¢.

  1. 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
Copy link
Member

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
Copy link
Author

@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
Copy link
Member

@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
Copy link
Author

@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
Copy link

ghost commented Apr 11, 2023

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
Copy link
Member

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
Copy link
Author

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

@captainsafia captainsafia added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jun 21, 2023
@captainsafia captainsafia added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 21, 2023
@kramnos
Copy link

kramnos commented Aug 10, 2023

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
Copy link
Author

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

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@captainsafia captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
No open projects
Status: Needs design
Development

No branches or pull requests

8 participants