JSON API 2.x responses always result in a Page Cache MISS

@yobottehg, thanks for testing this out and for creating a detailed issue!

Thanks a lot for trying 2.0-RC1! 🙏 🎉

No caching possible.

JSON API 2.x is less forgiving towards code that's not handling cacheability correctly. Related: #2948666: Remove JSON API's use of $context['cacheable_metadata'], #2940342: Cacheability metadata on an entity fields' properties is lost, #2984964: JSON API + hook_node_grants() implementations: accessing /jsonapi/node/article as non-admin user results in a cacheability metadata leak, #3005826: Follow-up for #2984964: JSON API + hook_node_grants() implementations: count queries still result in cacheability metadata leak. Also: I'm very pleased that some people do pay attention to cacheability! 😄 Most people seem oblivious to this.

4 questions:

1. Does this site have any modules that implement hook_node_grants()?
2. Does this site have any modules that have some normalizers of their own?
3. Could you apply #2951814-41: Improve X-Drupal-Cache and X-Drupal-Dynamic-Cache headers, even for responses that are not cacheable and report back what the X-Drupal-Cache and X-Drupal-Dynamic-Cache response headers look like then?
4. Can you also reproduce this on "vanilla Drupal", with just the modules you mentioned, and zero other modules? I can't.

P.S.: you don't need #3007091: Performance issue in ConfigurableResourceType to make JSON API Extras install via composer, but #2994885: JSONAPI Version Requirements prevent installation via Composer. You probably meant that issue?

Also:

Performance before update:
- Uncached single entity: ~ 1.300ms
…
Performance after update:
- Uncached single entity: ~ 1.100ms

15% faster, nice :)

I cannot reproduce #7: I can get Dynamic-Page-Cache: HIT even with jsonapi_extras installed.

However, I can reproduce #9. The reason is that when looking up a Page Cache entry, the request format is always still html, since we're so early in the request/response processing system. But when a Page Cache entry gets written, the format is api_json. Related: #2983178: Using URL query parameters on JSON API routes disallowed by the JSON API spec now result in "400: Bad Request" responses + #2933062: [>=8.5.4] Spec Compliance: Return 400 for unrecognized/unsupported query parameters. Needs further investigation.

Thank you! 👌

Retitling per #10+#11, tightening scope to only the Page Cache aspect.

I've narrowed this down to #2987205: FormatSetter doesn't set the format to `api_json` when accessing just `/jsonapi`.

@Wim Leers comment in #10 was the crucial clue. We used to have some middleware that set the api_json format on the incoming route.

I used git bisect to find the commit where that was removed, then confirmed that page cache was always MISS after that commit, but not before.

I'm looking into why it was removed and how to restore it now.

This adds a regression test that reproduces the issue.

@Wim Leers was spot on with his diagnosis in #10. There are only two ways to make this work. Restore the ?_format=api_json route requirement or set it ourselves based on something else. I think we all agree that we do not want to bring back the _format query parameter. Blech. So that means the desired format needs to be detected. This is what we did before #2987205: FormatSetter doesn't set the format to `api_json` when accessing just `/jsonapi`. However, we removed that service because it was unreliable and used complicated/broken code to do the detection.

AFAICT, the only reliable mechanism we have to determine that the client intends to get a JSON API response is the Content-Type header. This header is required by the spec, so it should be on every incoming request.

Unfortunately, relying on Content-Type means that the format can't be detected when requesting JSON API in the browser. At first, I thought this meant it was a no-go. After thinking about it more, what's the big deal, really? The browser is an edge case is only used for quick debugging. Requiring the Content-Type header will work just fine for Postman, curl and anything that can send the Content-Type header. It fixes this cache bug and also formats 404s in JSON for all the "real" clients. Only the browser will get HTML 404s.

I think that's the best solution. It's simple and reliable and solves the 95% case. @Wim Leers?

@mortona2k helped me troubleshoot some things related to this issue in Slack.

WFM. This will cause some disruption. But not for anybody who's already using "proper" tools like Postman etc. I suggest a CR where we explicitly suggest this.

Then it also means we're finally fully spec-compliant.

…

OTOH (devil's advocate): is it really that bad to automatically make all /jsonapi requests have this format?

IMHO ship with this, unless there's too much backlash, then we can still add base path-based format setting later.

This will cause some disruption.
...
Then it also means we're finally fully spec-compliant.
…

OTOH (devil's advocate): is it really that bad to automatically make all /jsonapi requests have this format?

I think I could have been a little more clear. With this patch you can still get JSON:API responses without the Content-Type header, even in the browser as long as the URL matches a JSON:API route. So, /jsonapi/article/page/missing still works. But /jsonapi/misspelled/page/uuid will not unless you have the header.

So we're not technically fully spec compliant, but we're closer. And I think in most cases the browser will still work just fine.

What were accepting is just that the page cache won't work without the Content-Type header and missing routes without it will fall back to HTML. But with it, everything works better than it ever did!

What we could do, is make this more strict and actually have an error if the request doesn't have the header linking to documentation about using a "proper" tool.

I'm fine with that also.

+++ b/jsonapi.services.yml
@@ -141,6 +141,14 @@ services:
+      # Set priority to 201 so it happens right before the page cache
+      # middleware (priority 200) has the opportunity to respond.

FWIW, page cache middleware is not always available, e.g. you are using external page cache and you disabled the page_cache module. Instead of adding this middleware all the time we should only add it when the page_cache module is enabled. Correct me if I'm wrong, I think, external page cache doesn't care about this.

@jibran, I don't think we only want this when Page Cache is enabled.

I think it's good to have on all the time. If we don't have it and a user visits /jsonapi/node/misspelled/{uuid} then the 404 response will be formatted in HTML rather than JSON, for example. Not that big a deal, but it can't hurt to have this middleware can it?

With the years I have shifted perception on this. I think that content type negotiation based on path detection is not a good practice. Specially when that path is variable. Specially when we don't have a way to negotiate legit 404's and route level 403's.

This patch is my preferred approach.

I'm seeing many mentions to Content-Type where it should be Accept. To be 100% clear, we want to negotiate requests with an Accept header so the response contains the Content-Type header.

Additionally, unsafe requests with a body should contain a Content-Type header to tell the server how to deserialize the body. This has no effect on the Content-Type header of the response.

Ah, thanks @e0ipso! I misread the spec:

Clients MUST send all JSON:API data in request documents with the header Content-Type: application/vnd.api+json without any media type parameters.

Clients that include the JSON:API media type in their Accept header MUST specify the media type there at least once without any media type parameters.

What I missed was "Clients must send all data". I read it as "send all requests".

So, this patch needs work, because it's looking for the Content-Type header.

Also, this will be a BC break since the spec does not require all requests to have the Accept header.

I'm fine with that however.

This patch is my preferred approach.

Alright, even better! 👏

I'm seeing many mentions to Content-Type where it should be Accept.

I noticed that too, but I thought it was pretty clear what Gabe meant :)

Also, this will be a BC break since the spec does not require all requests to have the Accept header.

I'm fine with that however.

Me too. Every single example request at https://jsonapi.org/format/ explicitly has that request header too btw! So it's definitely a SHOULD-HAVE already.

Bam!

https://www.drupal.org/node/3013494

Moving to Drupal core's issue queue.

I'm working on https://www.drupal.org/project/drupal/issues/3122113