ikyriak / IdempotentAPI

A .NET library that handles the HTTP write operations (POST and PATCH) that can affect only once for the given request data and idempotency-key by using an ASP.NET Core attribute (filter).
MIT License
270 stars 39 forks source link

406 Not Accepted on cached response with Content-Type: application/json; charset=utf-8 #78

Open angularsen opened 4 months ago

angularsen commented 4 months ago

Moved to a seaparate issue from https://github.com/ikyriak/IdempotentAPI/issues/74#issuecomment-2180577800

I am getting 406 Not Accepted and 0 bytes written for our cached API responses when using IdempotentAPI, due to the cached response having Content-Type: application/json; charset=utf-8.

Root cause

The problem is caching responses with header Content-Type: application/json; charset=utf-8, which our API normally returns for JSON.

ASP.NET Core fails to find a compatible output formatter, despite NewtonsoftJsonOutputFormatter being configured:

  1. DefaultOutputFormatterSelector.SelectFormatterUsingSortedAcceptHeadersAndContentTypes fails to identify a compatible formatter from the 4 default formatters:
    Microsoft.AspNetCore.Mvc.Formatters.HttpNoContentOutputFormatter
    Microsoft.AspNetCore.Mvc.Formatters.StringOutputFormatter
    Microsoft.AspNetCore.Mvc.Formatters.StreamOutputFormatter, Microsoft.AspNetCore.Mvc.Formatters.NewtonsoftJsonOutputFormatter

  2. NewtonsoftJsonOutputFormatter.CanWriteResult returns false, because MediaTypeHeaderValue.IsSubsetOf() and MediaTypeHeaderValue.MatchesParameters() fail to recognize that Accept: application/json is compatible with Content-Type: application/json; charset=utf-8

Not sure if this is a bug or by design in aspnetcore, can't find any related issues.

Minimal repro

This controller-based API returns 406 Not Accepted by simulating the way IdempotentAPI sets the Content-Type for cached responses, but not actually using IdempotentAPI here.

Full source code: Repro-IdempotentAPI-issue-74.zip

Call it like this:

using Microsoft.AspNetCore.Mvc;

namespace WebApplication2.Controllers;

[ApiController]
[Route("[controller]")]
public class FooController : ControllerBase
{
    [HttpGet("pass")]
    public IActionResult Pass()
    {
        return new ObjectResult(new
        {
            Message = """
                      This will succeed due to supported response content type 'application/json'.
                      """
        })
        {
            ContentTypes = ["application/json"],
            StatusCode = 200
        };
    }

    [HttpGet("fail")]
    public IActionResult Fail()
    {
        return new ObjectResult(new
        {
            Message = """
                      This will fail with 406 Not Accepted due to unsupported response content type 'application/json; charset=utf-8':

                      Error message: 
                      No output formatter was found for content types 'application/json; charset=utf-8, application/json; charset=utf-8' to write the response.
                      """
        })
        {
            ContentTypes = ["application/json; charset=utf-8"],
            StatusCode = 200
        };
    }
}

Workaround - Don't cache Content-Type response header or remove charset suffix

After some debugging and removing most other MVC filters and middlewares, I isolated a fix/workaround to Idempotency.cs, by

                // Include cached headers (if does not exist) at the response:
                Dictionary<string, List<string>> headerKeyValues = (Dictionary<string, List<string>>)cacheData["Response.Headers"];
                if (headerKeyValues != null)
                {
                    foreach (KeyValuePair<string, List<string>> headerKeyValue in headerKeyValues)
                    {
                        if (!context.HttpContext.Response.Headers.ContainsKey(headerKeyValue.Key))
                        {
                            // Workaround: 406 Not Acceptable and 0 bytes written if charset is defined,
                            // DefaultOutputFormatterSelector.SelectFormatterUsingSortedAcceptHeadersAndContentTypes fails to match NewtonsoftJsonOutputFormatter on
                            // response header "Content-Type: application/json; charset=utf-8".
                            // This happens in OutputFormatter.CanWriteResult => MediaType.IsSubsetOf(MediaType other) => ReadOnlyMediaTypeHeaderValue.ContainsAllParameters() returning false,
                            // because of the explicitly set charset in the response.
                            // The problem happens regardless of "Accept" request header, even "Accept: application/json; charset=utf-8" or wildcard "Accept: */*".
                            // context.HttpContext.Response.Headers.Add(headerKeyValue.Key, headerKeyValue.Value.ToArray());
                            context.HttpContext.Response.Headers.Add(headerKeyValue.Key, headerKeyValue.Value.Select(val => val.Replace("application/json; charset=utf-8", "application/json")).ToArray());
                        }
                    }
                }

We could also skip setting Content-Length, since this is automatically calculated, but this brings me to a more philosophical question about how this idempotency is implemented. I think caching and response-writing should happen earlier in the pipeline (closer to start of request/end of response) to include any changes made by other middleware. Will add a separate comment on this.

Details from debugging

First request, not cached, 200 OK and 736 bytes

image

Second request, cached, 406 Not Accepted and 0 bytes resulting in 500 Internal Server Error

image

With verbose logging, showing how it fails to resolve JSON output formatter: image

Debugging the second request looks OK at first glance

200 OK is set with JSON content type and 736 byte length. Pretty similar to first request.

image

image

angularsen commented 4 months ago

Maybe you have already considered this, but my initial thought was, would it not be better to move the caching and cached response writing earlier in the pipeline as a middleware instead of as an MVC action filter?

A few advantages:

  1. Simpler, no need to deal with various MVC result types and Minimal API types. Request hash is based on http request headers and body bytes. Cached response matches the actual headers and body returned to the client.
  2. More consistent, if deploying changes to middleware or MVC filters, any previously cached responses won't change and will still be returned as-is.
  3. More performant, it kan short-circuit the request pipeline on cache hit, no need to go through all middleware up to MVC.
  4. It would avoid this particular problem, no need match any output formatter for MVC action result.
  5. You can cache any type of response (text/csv, binary files, ++)
  6. No need to configure JSON serializers, per #74

Challenges:

  1. I'm not yet sure how to selectively enable idempotency for certain MVC endpoints only, like using attributes, without actually running MVC. One option could be to still run the MVC filter as before, but: a. Instead of deserializing the original DTO, it just reads the cached response body bytes and writes that to the response body, maybe by returning ContentResult to bypass any output formatters b. If cache miss, it flags the request, for example Request.HttpContext.Items["IdempotentAPI:CacheMiss"] = true c. A middleware registered early in the pipeline then reacts to this flag and caches the final response headers and body (bytes)

Just a brain dump and no prototyping of this, but just wanted to hear what you think of this approach and if you have considered something similar already?

angularsen commented 4 months ago

Created an issue over at aspnetcore, to see if this is a bug or by design: https://github.com/dotnet/aspnetcore/issues/56357

ikyriak commented 3 months ago

Hello @angularsen,

Thank you for the detailed error report. I will investigate it and let you know.

ikyriak commented 3 months ago

Until now, I have not received any issues that have made me rethink its architecture and keep its simplicity.

I want to perform a refactor and cleanup to integrate the new functionalities better because I have tried to avoid breaking changes.

I will take your proposal into account. Thank you :pray:

Maybe you have already considered this, but my initial thought was, would it not be better to move the caching and cached response writing earlier in the pipeline as a middleware instead of as an MVC action filter?

A few advantages:

1. Simpler, no need to deal with various MVC result types and Minimal API types. Request hash is based on http request headers and body bytes. Cached response matches the actual headers and body returned to the client.

2. More consistent, if deploying changes to middleware or MVC filters, any previously cached responses won't change and will still be returned as-is.

3. More performant, it kan short-circuit the request pipeline on cache hit, no need to go through all middleware up to MVC.

4. It would avoid this particular problem, no need match any output formatter for MVC action result.

5. You can cache any type of response (text/csv, binary files, ++)

6. No need to configure JSON serializers, per [Feature: Configurable JSON serializers #74](https://github.com/ikyriak/IdempotentAPI/issues/74)

Challenges:

1. I'm not yet sure how to selectively enable idempotency for certain MVC endpoints only, like using attributes, without actually running MVC. One option could be to still run the MVC filter as before, but:
   a. Instead of deserializing the original DTO, it just reads the cached response body bytes and writes that to the response body, maybe by returning `ContentResult` to bypass any output formatters
   b. If cache miss, it flags the request, for example `Request.HttpContext.Items["IdempotentAPI:CacheMiss"] = true`
   c. A middleware registered early in the pipeline then reacts to this flag and caches the final response headers and body (bytes)

Just a brain dump and no prototyping of this, but just wanted to hear what you think of this approach and if you have considered something similar already?

ikyriak commented 3 months ago

To overcome this issue, we are restricting the request and response formats as follows (Our Example, .NET Documentation).

[ApiController]
[Route("[controller]")]
[Consumes("application/json")]
[Produces("application/json")]
[Idempotent(Enabled = true)]
public class FooController : ControllerBase
{
    // ...
}

I have adjusted the Repro-IdempotentAPI-issue-74 project to use the IdempotentAPI. You can test the POST command with the follows as follows (Repro-IdempotentAPI-issue-74-2.zip):

curl --location --request POST 'http://localhost:5176/foo/apostrequest' \
--header 'IdempotencyKey: atestkey001' \
--data ''