Cache REST API endpoints

[iandunn]

This is still a work in progress, but a good chunk of it is there.

I'd love to get some feedback from @donnchawp and @kraftbj. Do you think this is heading in the right direction? Are there any major parts missing? Any other thoughts?

One thing I haven't looked at yet is support for mod_rewrite, so this is just for legacy and PHP modes.

[iandunn]

The main things this is doing right now are:

1. wpsc_rest_eof_tags() - This adds regex patterns for JSON objects and collections to the end-of-file tags, so that cache files will be generated for JSON "pages".

2. wpsc_rest_toggle_slash_check() - This works around the $wp_cache_slash_check logic in wp_cache_serve_cache_file(), so that the cache files will be served for both {url} and {url}/.

3. wpsc_rest_remove_comments() - This removes the HTML comments that normally get appended to the cache buffer -- e.g., Dynamic page generated in 0.578 seconds. -- because those break JSON parsing.

[iandunn]

There are two main problems left to solve:

1. Adding support for mod_rewrite, so we don't have to hit PHP
2. Ensuring the correct headers are sent, including for JSONP callbacks, and being future proof against any modifications to the current security headers the API sends.

I did some searching to see what other plugins have done in this space, but I only came up with 2 small ones, and they're both specifically targeting the REST API. None of the major general-purpose plugins have done this yet.

Those two plugins both use transients, so they're not really an effective solution for the type of user who would use a general-purpose caching plugin. Using transients is definitely better than nothing, but unless you have an object cache setup, you're still hitting MySQL. Even if you do have an object cache, you're still hitting PHP and loading all of WP.

The approach here should be much faster than that, even if we don't come up with a way to support mod_rewrite.

[iandunn]

I rebased the branch into a handful of commits, to make reviewing easier, and to give it a cleaner merge history. I haven't found any problems in my tests, so I think it's ready for review.

@kraftbj Do you mind taking a look and giving me any feedback you have? I can see three options for moving forward:

1. Merge the entire pull request and leave the REST plugin on by default.
2. Merge the entire pull request, but change the REST plugin to be off by default. After it's gotten some real-world testing, we can switch it to be on by default.
3. Only merge the new filters, but not the REST plugin. Then, I can run the plugin on WordCamp.org for awhile just to get some real-world testing, and then we can go back to options 1 or 2.

[iandunn]

@kraftbj, any thoughts on the above?

[kraftbj]

Apologies for the delay. I'll take a look tomorrow.

[kraftbj]

I'd like to see if there's a way to determine what we need to clear in a more specific manner. Nuking everything is more conservative but afraid about scaling on really large sites.

[iandunn]

Yeah, that's a good point to bring up. I don't have any ideas for how to map content to API endpoints right now (other than just a hardcoded list for known endpoints like /posts).

One thing that could help avoid scaling issues, would be to keep track of when we flush the cache, and don't do it more often then once every 5 minutes, or something like that. We'll need to do more thinking/testing/etc there, though

[rmccue]

You can check rest_base on the post types, but otherwise there's no generic mapping.

[kraftbj]

I really like it and reads wonderfully. I need to go back through to make sure there's a way to confirm you're being served a cached file; my initial tests didn't have a header or content added to the json, etc to confirm that it's working, clearing, etc.

Going to ask Guardians for a second set of eyes too.

[iandunn]

it reads wonderfully

Thanks! That warms my heart :D

make sure there's a way to confirm you're being served a cached file

Ah, I should have documented what I did. Added that in ca8a37a.

[rmccue]

Note that null, "hello", and 21345 are all completely valid JSON responses as well, but core wouldn't generate that by default.

[iandunn]

Thanks, I'll add a @todo note about that for future iterations.

[iandunn]

I enabled this on WordCamp.org production so that we can get some real-world testing, and so far things look good. I'll keep an eye on it, though, and push commits for any bugs.

[iandunn]

I noticed a problem on production, but it turned out to be #97, so I'll send a PR for that.

If WP_ALLOW_MULTISITE doesn't exist, then legacy files are never served during phase 1, but they'll be generated in phase 2.

[axelson]

Sorry for the noise but I just want to say that I'm really looking forward to this feature. Especially since the latest version of wordpress includes the rest api by default.

[donnchawp]

I think legacy cache files will the best way of storing REST API calls, especially as now I've merged in code that stores those files in the supercache directories. See #177. They'll store header fields as well as page contents.

I think we'll be able to figure out a way to map garbage collection of the "main" supercache directory with the REST API directories that will be created. After all, we'll have post_ids of any posts.

I also added wpsc_delete_files and wpsc_rebuild_files functions to somewhat replace prune_super_cache in some situations so those could be entry points for doing the deleting of cached REST API calls.

[donnchawp]

I think something like the gc_cache action could be used to delete the REST API cache files, but I might need to add a gc_cache_id action - that would pass a post_id rather than a path..

[iandunn]

I rebased the PR against master to merge the latest updates. I also squashed some of the commits and force-pushed the new ones, so that they'll be cleaner when they're merged.

In hindsight, those were both probably bad ideas, since any comments on the original commits were lost, and anyone who has a local checkout will need to git fetch && git reset origin/cache-rest-api --hard, in order to pull down the new commits.

[iandunn]

@donnchawp, unfortunately I don't think I'll have much more time to devote to this anytime soon, so what do you think about merging the current PR as v1, and then doing the changes in your comments in future iterations?

If it'd help, we could keep the plugin off by default, in order to get more testing before it's turned on by default.

[donnchawp]

I'll test it when I have time. We'll need some sort of garbage collection in there but I don't think it'll be hard to do.

I agree, we should try and get a version 1 out there as soon as possible, with the changes to where legacy cache files are stored we could almost call it a version 2.0 :)

[iandunn]

I ran into a problem with this branch today, and it's already documented above, but I'd completely forgotten about it, so it's probably worth repeating.

Right now the HTTP headers aren't saved and served during cached responses, which can cause bugs in plugins. For example, here's an API response that returns a WP_Error

Cache miss:

> curl -i https://wp.dev/wp-json/test/v1/foo
HTTP/1.1 500 Internal Server Error
Server: nginx
Date: Mon, 19 Jun 2017 22:15:22 GMT
Content-Type: application/json; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
X-Powered-By: PHP/5.5.9-1ubuntu4.20
Vary: Cookie
X-Robots-Tag: noindex
Link: <https://wp.dev/wp-json/>; rel="https://api.w.org/"
X-Content-Type-Options: nosniff
Access-Control-Expose-Headers: X-WP-Total, X-WP-TotalPages
Access-Control-Allow-Headers: Authorization, Content-Type
Allow: GET

{"code":"errorunique","message":"this is a test","data":null}

Cache hit

> curl -i https://wp.dev/wp-json/test/v1/foo
HTTP/1.1 200 OK
Server: nginx
Date: Mon, 19 Jun 2017 22:14:50 GMT
Content-Type: application/json
Transfer-Encoding: chunked
Connection: keep-alive
X-Powered-By: PHP/5.5.9-1ubuntu4.20
Vary: Accept-Encoding, Cookie
Cache-Control: max-age=3, must-revalidate
WP-Super-Cache: Served supercache file from PHP

{"code":"errorunique","message":"this is a test","data":null}

If there's some code that's checking the status code of the response, it'll correctly get 500 for the cache miss, but 200 for the cache hit. During cache hits, it'll treat the WP_Error as if it were valid data, leading to all kinds of bugs.

I think this is something that'll need to be fixed before this branch can be merged. It sounds like @donnchawp already has plans for that.

[iandunn]

Another blocker for merging that I just realized, is that we need to make sure that API requests that require some form of authentication are never cached.

For normal requests, Super Cache relies on WP's cookie auth, which can also be used with the REST API, but the API allows several additional methods -- e.g., OAuth, JSON web tokens, etc -- especially for server-to-server requests.

At first glance, it seems like checking for the Authorization header would cover all standards cases. I'm not 100% confident about that, though.

There'll still be situations where plugin authors pass tokens in arbitrary $_GET parameters, though. I'm not sure if we should make custom endpoints opt-in to avoid problems there, or just say they're doing it wrong. Maybe there's a better way that I haven't thought of, too.

To err on the side of caution, it might be good to also avoid caching requests that contain an X-WP-Nonce header, or $_REQUEST['_wpnonce']. That won't catch arbitrary parameters names, though.

cc @donnchawp

[beaulebens]

Another blocker for merging that I just realized, is that we need to make sure that API requests that require some form of authentication are never cached.

Could we maybe checked for an Authorization header, and also for a current user (which would imply that some authorization happened, and if either are there, then don't cache?

[rmccue]

If there's a current user by the time the response is generated, then some form of authentication has worked. Authentication plugins for the REST API don't do anything special, they just hook into the determine_current_user filter in core.

The Authorization header will cover a bunch of use cases, but OAuth 1 and 2 can both pass request parameters elsewhere (oauth_token and access_token respectively), so it's not a certainty.

[iandunn]

If there's a current user by the time the response is generated, then some form of authentication has worked. Authentication plugins for the REST API don't do anything special, they just hook into the determine_current_user filter in core.

Are you thinking that it's enough to just check for a current user, rather than checking for both the current user AND the Authorization header? I don't think that's what you meant, but given the context I want to be extra careful that there isn't any miscommunication.

I think checking for Authorization (and other known parameters, like you mentioned) will still be required, because there will be plugins that rely only on the Authorization header, and don't touch the current user.

[donnchawp]

As I said previously, I think we should use legacy cache files to store REST API calls. That may go some way towards taking care of authentication, but serving cache files to authenticated REST API clients might be problematic.

The plugin needs to support an authentication key that's plugged into the code that generates wp_cache_key. That code fires way before WordPress authentication happens however.

[donnchawp]

https://github.com/airesvsg/wp-rest-api-cache/blob/master/class-wp-rest-cache.php does something interesting. It uses rest_pre_dispatch to intercept REST API requests, and then "runs" the request if it's not cached, caches it in a transient option before serving it. It doesn't do any granular maintenance of cache files.
It uses $request_uri, $server and $request for the cache key which should take care of authentication..

[therealgilles]

Quick question: a bit off topic, but what happens if a page does a GET AJAX (JSON) request to the backend? shouldn't JSON support be extended to that case too?

[ivptr]

Any update on this? All mobile apps are using REST API to access the content and this should be cached, too.

[donnchawp]

No update at all. There are other plugins that will cache REST API requests. Someone asked about it on the forum recently. A quick search of the plugins found a few. Have you tried any of them?
https://wordpress.org/support/topic/rest-api-caching/

[iandunn]

Closing since there are other options, and this PR would take a lot of work to become useful.