Improve performance of X-WP-TotalPages queries

[spacedmonkey]

In the REST API, there is a header field called X-WP-TotalPages. This field is used to understand how many total pages there are to a REST API call.

This does a call to WP_Query / WP_Comment_Query / WP_User_Query and gets the total of number of results for a query. Here are some examples found in core.

• ​class-wp-rest-posts-controller.php
• ​class-wp-rest-global-styles-revisions-controller.php
• ​class-wp-rest-users-controller.php
• ​class-wp-rest-comments-controller.php
• ​class-wp-rest-global-styles-revisions-controller.php

These queries could be improved, by passing different arguments, to the query classes.

For WP_Query requesting only fields 'ids', limiting the query to 1 result, and disable priming of term / meta caches.
For WP_Comment_Query, passing count parameter and disable comment meta priming.
For WP_User_Query, questing only fields 'ids', limiting the query to 1 result and disable priming of user caches.

This change should be simple, as total counts should be covered by existing unit tests.

Trac ticket: https://core.trac.wordpress.org/ticket/62801

[johnbillion]

Which condition does the $total_posts < 1 logic handle? ie. why is the found_posts property not populated in the main query?

[spacedmonkey]

@johnbillion I don't have any context on this one. Maybe @TimothyBlynJacobs @kadamwhite or @rachelbaker would know why this was done.

[joehoyle]

Could it be that an out of bounds query means found_posts is 0? I.e. if I hit ?paged=100 when there is only 10 pages, I guess found_posts even with the FOUND_ROWS() returns 0?

[adamsilverstein]

Thanks for the PR @Sukhendu2002 - is this ready for review (I see it is draft), or do you have any questions? cc: @spacedmonkey

[kadamwhite]

I believe @joehoyle is correct, if you have (say) 12 items and you did page=3, you'd get 0 results back but it's not clear if you've just paged beyond the maximum or if there's no items at all.

[kadamwhite]

If it's considered ready for review by @sukhendu2002 I'm +1 on the patch, it appears to achieve @spacedmonkey's desired improvements

[sukhendu2002]

Replying to adamsilverstein:

Thanks for the PR @Sukhendu2002 - is this ready for review (I see it is draft), or do you have any questions? cc: @spacedmonkey

Sorry for the delay! I've now marked the PR as ready for review. Let me know if you have any feedback.

Hey @spacedmonkey, Thanks for the review! I have also applied the change in the WP_REST_Revisions_Controller class. Take a look when you get a chance. Thanks!

[spacedmonkey]

This looks good. To late to commit in WP 6.8. Adding to WP 6.9 and assigning to me.

[jorbin]

I think some automated tests would be very dandy here. Especially in light of the call out for what happens when going beyond the maximum page.

[spacedmonkey]

@sukhendu2002 Would it be possible for you to add unit tests to the PR you provided? It is okay if not, just need to know.

[spacedmonkey]

Looking at the patch again, I am going to disagree with @jorbin here. There is no functional change here, so there is nothing to test.

[jorbin]

@spacedmonkey A specific that I think should be covered by test is that caches are not being primed. This will prevent accidentally reintroducing priming if there are changes made in the query classes.

[spacedmonkey]

@jorbin Any recommendation on the best way to do that?

[westonruter]

Do a call to wp_cache_get() to see if the object cache has been populated?

[westonruter]

@sukhendu2002 Could you update your PR with the above feedback to add some tests?

[kadamwhite]

@westonruter @jorbin I'm struggling myself to imagine how we'd effectively test for this, given that the relevant logic is 50 or more lines into a long function that does also fetch other content... is there any other guidance you could provide to help @sukhendu2002 (or myself, or others) go down the right path on testing?

[westonruter]

@kadamwhite I'll defer to your judgement as to whether this warrants additional tests.

[spacedmonkey]

We have two compontent maintainers here saying this is nearly impossible to test.

IMO, the only way the test would break, if someone tweaks the logic in these methods. Would it not be better to have a comment in the code to say not to change it.

[peterwilsoncc]

I tend to agree, there isn't a great way to test this. It could possibly be done using the posts_clauses_request filter but it would be flaky at best.

It occurs to me that if someone has used the "rest_{$this->post_type}_query" to add the argument 'no_found_rows' => true the counting query will run on page two onwards without achieving it's desired result, so it can be bypassed altogether in that case. Alternatively, the argument could be prevented for rest requests.

Same looks to apply for the comment endpoint and possibly others.

[spacedmonkey]

The only test I can think make sense, might be use to filter like parse_query and get $wp_query->query_vars and check the query args. But this change not only changes parameters to WP_Query, but also WP_User_Query and WP_Comment_Query.

Again, i really don't see the point of these tests. The only way these would change, if someone changes the code, a comment would cover that.

[peterwilsoncc]

In 61002:

REST API: Improve performance of X-WP-Total/X-WP-TotalPages queries.

Improve the performance of fallback queries to determine the total number of objects available on various REST endpoints for out of bounds queries.

The database queries are modified to return the minimum amount of data required for determining the count and bypass priming of meta a term caches where appropriate.

Props adamsilverstein, joehoyle, johnbillion, jorbin, kadamwhite, spacedmonkey, sukhendu2002, westonruter.
Fixes #62801.