Make WordPress Core

Opened 19 months ago

Closed 18 months ago

Last modified 14 months ago

#55677 closed enhancement (fixed)

Improve Media REST API endpoint

Reported by: furi3r's profile furi3r 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 commit add-to-field-guide
Focuses: performance Cc:

Description

Duplicated queries when querying media_types without files.
await wp.apiFetch({ path: 'wp/v2/media?_envelope&media_type=text'});

Duplicated queries when querying non-existing mime_types.
await wp.apiFetch({ path: 'wp/v2/media?_envelope&mime_type=image/webp'});

Probably related.

Attachments (1)

Screen Shot 2022-05-04 at 3.44.47 PM.png (159.0 KB) - added by furi3r 19 months ago.
Duplicated queries when querying media_types witouth files.

Download all attachments as: .zip

Change History (15)

@furi3r
19 months ago

Duplicated queries when querying media_types witouth files.

#1 @gdetassigny
19 months ago

Thanks for the details @furi3r . I've figured out the issue for this and should have a patch available soon!

This ticket was mentioned in PR #2753 on WordPress/wordpress-develop by gabriel-detassigny.


19 months ago
#2

  • Keywords has-patch has-unit-tests added

## The issue

WP REST API has this logic to re-run the query without the page parameter if we didn't get any posts back from the main one. This is to handle the case where we requested an extra page of a collection that is empty. In that case, we want the API to return an empty collection, but still return the total number of items in that collection. Therefore that extra query gives that total back.

However, it currently doesn't check if we were on the first page already.
In that specific case, the second query would be the exact same as the first one and therefore wouldn't provide any additional value (the total of items would still be zero). This is why we sometimes ends up with a duplicated query.
By checking if we're already on the first page, we can avoid a duplicated query for empty collections.

Note that the trac ticket refers to the error happening on attachments endpoint, however any empty collection of posts will be affected (This is why some tests that were previously checking for two queries and had a "FIXME" comment, have been updated to one).

## How to test

  • Query Monitor plugin should be enabled to detect any duplicate queries.
  • Open the gutenberg editor
  • Open the JS console
  • Type await wp.apiFetch({ path: 'wp/v2/media?_envelope&media_type=text'}
  • The object returned should contain the number of duplicated queries. On trunc, there will be a duplicate which this patch will fix.

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

#3 @gdetassigny
19 months ago

I've added a patch to fix this.
Something worth noting : while this issue was reported about the attachments rest endpoint, this actually is a potential issue for any post type. It's probably more apparent on attachments because they're more likely to have a completely empty collection.

#4 @johnbillion
19 months ago

  • Milestone changed from Awaiting Review to 6.1

#5 @TimothyBlynJacobs
18 months ago

  • Keywords commit added
  • Version set to 4.7

TimothyBJacobs commented on PR #2753:


18 months ago
#6

Can we also interrogate the X-WP-TotalPages and X-WP-Total headers in our tests here to have assertions that they still match our expected results.

gabriel-detassigny commented on PR #2753:


18 months ago
#7

Thanks @TimothyBJacobs . I've made some changes now:

TimothyBJacobs commented on PR #2753:


18 months ago
#8

Thanks @gabriel-detassigny, this looks good to me!

#9 @spacedmonkey
18 months ago

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

#10 @spacedmonkey
18 months ago

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

In 53498:

REST API: Avoid duplicated query in post collections.

Avoid duplicated query when retrieving empty posts collections by adding a check if the page is more than 1.

Props furi3r, gdetassigny, TimothyBlynJacobs, spacedmonkey.
Fixes #55677.

This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.


14 months ago

#13 @milana_cap
14 months ago

  • Keywords add-to-field-guide added

#14 @spacedmonkey
14 months ago

Early draft of a dev note can be found here.

Note: See TracTickets for help on using tickets.