Make WordPress Core

Opened 14 months ago

Closed 12 months ago

Last modified 8 months ago

#55593 closed enhancement (fixed)

Prime caches for post parents in post rest api controller

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.1 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests has-dev-note
Focuses: rest-api, performance Cc:

Description

In the Post REST controller, for every post with post status of inherit and a parent post set, a lookup for the parent is done in the check_read_permission method. For a response with 100 posts, this could result in 100 queries to the posts table. Instead of looking up one by one, simple use _prime_post_caches and prime caches for all parents in one request.

Related #55592.

Change History (32)

This ticket was mentioned in Slack in #hosting-community by javier. View the logs.


13 months ago

This ticket was mentioned in Slack in #hosting-community by javier. View the logs.


13 months ago

This ticket was mentioned in Slack in #hosting-community by jadonn. View the logs.


13 months ago

#5 @spacedmonkey
13 months ago

  • Milestone changed from Future Release to 6.1
  • Version set to 4.7

This ticket was mentioned in Slack in #hosting-community by javier. View the logs.


13 months ago

This ticket was mentioned in Slack in #hosting-community by jadonn. View the logs.


13 months ago

#8 @furi3r
13 months ago

Hello Jonny, I've added unit test to cover this change, also investigated other possible scenarios where we should need to prime parent ids, but it seems this was the right and only place I could find.

#9 @furi3r
13 months ago

  • Keywords has-unit-tests added
  • Resolution set to invalid
  • Status changed from new to closed

#10 @furi3r
13 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

#12 @spacedmonkey
12 months ago

  • Owner set to spacedmonkey
  • Status changed from reopened to assigned

#13 @spacedmonkey
12 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 53506:

REST API: Prime caches for post parents in post REST API controller.

Prime caches for all post parents in the post REST API controller using the _prime_post_caches function. Post parent objects are required as part of the check_read_permission method’s permission check in post REST API controller.

Props spacedmonkey, furi3r, peterwilsoncc, mitogh, madpixels.
Fixes #55593.

spacedmonkey commented on PR #2612:


12 months ago
#14

Committed.

#15 @SergeyBiryukov
12 months ago

In 53507:

REST API: Some documentation and test improvements for update_post_parent_caches():

  • Make the function description more specific, for consistency with update_post_author_caches().
  • Add missing @covers tags for the unit test.

Follow-up to [53506].

See #55593.

#16 follow-up: @SergeyBiryukov
12 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

It looks like there might be some kind of a race condition in the test added in [53506]:

1) WP_Test_REST_Posts_Controller::test_get_items_parent_ids_primed
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 1880
-    1 => 1881
+    0 => 2003
 )

/var/www/tests/phpunit/includes/abstract-testcase.php:820
/var/www/tests/phpunit/tests/rest-api/rest-posts-controller.php:1828
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

See these PHP 7.4 jobs for example:
https://github.com/WordPress/wordpress-develop/runs/6898050638?check_suite_focus=true#step:20:267
https://github.com/WordPress/wordpress-develop/runs/6900501418?check_suite_focus=true#step:20:267

Last edited 12 months ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in Slack in #core by sergey. View the logs.


12 months ago

#18 @SergeyBiryukov
12 months ago

In 53509:

Tests: Some improvements for REST API cache priming tests:

  • Give the test methods more specific names and move them closer together.
  • Correct the @covers tags.

Follow-up to [53499], [53504], [53506], [53507], [53508].

See #55593, #55652.

#19 in reply to: ↑ 16 @SergeyBiryukov
12 months ago

Replying to SergeyBiryukov:

It looks like there might be some kind of a race condition in the test added in [53506]:

1) WP_Test_REST_Posts_Controller::test_get_items_parent_ids_primed
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 1880
-    1 => 1881
+    0 => 2003
 )

/var/www/tests/phpunit/includes/abstract-testcase.php:820
/var/www/tests/phpunit/tests/rest-api/rest-posts-controller.php:1828
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

See these PHP 7.4 jobs for example:
https://github.com/WordPress/wordpress-develop/runs/6898050638?check_suite_focus=true#step:20:267
https://github.com/WordPress/wordpress-develop/runs/6900501418?check_suite_focus=true#step:20:267

Looks like it's PHP 7.4 with memcached specifically that fails, not the regular PHP 7.4 job.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


12 months ago

#21 @SergeyBiryukov
12 months ago

In 53511:

Tests: Temporarily disable the failing REST API test for update_post_parent_caches().

This needs more investigation to address the test failure with persistent object cache.

Follow-up to [53506].

Props hellofromTonya.
See #55593.

#22 @spacedmonkey
12 months ago

@SergeyBiryukov I have put together a fix for the broken test. See #2815.

This ticket was mentioned in PR #2815 on WordPress/wordpress-develop by spacedmonkey.


12 months ago
#23

Fix broken test and uncomment it out.

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

SergeyBiryukov commented on PR #2815:


12 months ago
#24

This works for me, thanks!

  • Could we use the same approach in the update_post_thumbnail_cache() test above? That one also fails for me locally, though not on GitHub for some reason.
  • I think $primed_posts_found or $primed_ids_found would be more appropriate than $primed_query_found, as we're not searching for a query here.
  • I was wondering if we could use something like this instead of the whole MockAction approach.
    // Verify that all $parent_ids are cached.
    $this->assertEmpty( array_diff( $parent_ids, _get_non_cached_ids( $parent_ids, 'posts' ) ) );
    
    Seems like a more straightforward way to check.

SergeyBiryukov commented on PR #2815:


12 months ago
#25

I was wondering if we could use something like this instead of the whole MockAction approach:

// Verify that all $parent_ids are cached.
$this->assertEmpty( array_diff( $parent_ids, _get_non_cached_ids( $parent_ids, 'posts' ) ) );

Seems like a more straightforward way to check.

Sorry, I think I meant something like this:

$cached_ids = array_keys( array_filter( wp_cache_get_multiple( $parent_ids, 'posts' ) ) );
$this->assertSameSets( $parent_ids, $cached_ids );

But that doesn't seem to work in my testing.

spacedmonkey commented on PR #2815:


12 months ago
#26

@SergeyBiryukov I worked out the root cause of the issue and pushed a fix.

SergeyBiryukov commented on PR #2815:


12 months ago
#27

Ah, that also works. Looks good 👍

#28 @spacedmonkey
12 months ago

In 53512:

Tests: Re-enable failing REST API test for update_post_parent_caches().

Ensure that that attachment objects are not primed in cache so that test passes when object caching is enabled.

Follow-up to [53506].

Props SergeyBiryukov.
See #55593.

spacedmonkey commented on PR #2815:


12 months ago
#29

Committed.

#30 @spacedmonkey
12 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

#31 @desrosj
9 months ago

  • Keywords needs-dev-note added

There are a lot of caching related changes in 6.1 and a dev note should be written collecting them all. Marking this for inclusion.

Note: See TracTickets for help on using tickets.