Query: Post IDs cached for search and other 'LIKE' queries are unreachable

[dlh]

[53941] added caching for the post ID database query in WP_Query. The cache key for the post IDs is determined by hashing (among other details) the $request property of the query, which contains the generated SQL.

At the time that this cache key is generated, the SQL in $request still contains escaped placeholders from wpdb::add_placeholder_escape(). For example:

	SELECT SQL_CALC_FOUND_ROWS  wp_posts.*
	FROM wp_posts
	WHERE 1=1  AND (((wp_posts.post_title LIKE '{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}hello{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}') OR (wp_posts.post_excerpt LIKE '{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}hello{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}') OR (wp_posts.post_content LIKE '{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}hello{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}')))  AND ((wp_posts.post_type = 'post' AND (wp_posts.post_status = 'publish'
OR wp_posts.post_status = 'private')) OR (wp_posts.post_type = 'page' AND (wp_posts.post_status = 'publish'
OR wp_posts.post_status = 'private')) OR (wp_posts.post_type = 'attachment' AND (wp_posts.post_status = 'publish'
OR wp_posts.post_status = 'private')))

	ORDER BY wp_posts.post_title LIKE '{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}hello{6bf47a1ba3fa507a5e63620d433b08dd0a8c3664854a451088b2fd023cb1d0f0}' DESC, wp_posts.post_date DESC
	LIMIT 0, 10

wpdb generates this placeholder once per request using uniqid(), making it unlikely that the placeholder will be the same across multiple requests.

As a result, the same search will generate different cache keys on each request, since the SQL will differ along with the placeholder. The IDs will be cached, but that cache will probably never be found.

From what I can tell, this issue affects search queries (with 's') and meta queries that use 'compare' => 'LIKE' or 'compare_key' => 'LIKE' (or NOT LIKE). Presumably, any other queries that include placeholders and use wpdb::prepare() would also be affected.

I'm not quite sure what the best course for the issue would be. My first thought was to expand, in WP_Query,

$id_query_is_cacheable = ! str_contains( strtoupper( $orderby ), ' RAND(' );

to include

&& ! str_contains( $this->request, $wpdb->placeholder_escape() )

However, calling wpdb::placeholder_escape() has a side-effect of adding a filter to query, and that might not be desirable.

Perhaps a naive check like ! str_contains( $this->request, " LIKE '{" ) would do the trick, but it's possible that that would catch unrelated queries.

[peterwilsoncc]

Thanks David. As caching was added during the current release cycle, I've moved it on to the milestone for investigation.

The placeholder value is stored in a static variable so I am not sure how to test for this.

The issue is limited to sites with a persistent cache, potentially stuffing them with keys they can never access again.

I like the options you provide: protentially the best way forward is to skip LIKE queries for now and revisit in 6.2.

cc @spacedmonkey

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

[spacedmonkey]

Thanks for the flag @dlh

The solution is extremely simply. I have created a PR with unit tests ​here.

I personally think that this small fix could make it way into 6.1 RC2 or 6.1.1. I don't think can or should wait until 6.2.

Thoughts @desrosj, @peterwilsoncc ?

[dlh]

It looks like there's a second scenario where placeholders can interfere with the cache key: During search queries, the prepared search terms are included with the search_orderby_title query variable, e.g.

'search_orderby_title' => [
	'wp_posts.post_title LIKE \'{8b8e7d5ae92dd4a249779814885fb4bdb2051b6bef993769f8b4352e46de15ad}hello{8b8e7d5ae92dd4a249779814885fb4bdb2051b6bef993769f8b4352e46de15ad}\'',
	'wp_posts.post_title LIKE \'{8b8e7d5ae92dd4a249779814885fb4bdb2051b6bef993769f8b4352e46de15ad}world{8b8e7d5ae92dd4a249779814885fb4bdb2051b6bef993769f8b4352e46de15ad}\'',
]

so even if placeholders are removed from $request, they'll still be in $cache_args.

It seems pretty safe to me to unset $cache_args['search_orderby_title'] along with suppress_filters, fields, etc. Or, wpdb::remove_placeholder_escape() could be run against these additional strings like it is in the PR.

[chaion07]

@dlh thank you for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback we are adding the keyword needs-testing to ensure this keyword is only removed when the fix has been verified.

Cheers!

Props to @costdev & @mukesh27

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

[spacedmonkey]

The first PR, isn't specially tidy. I have worked on another approach. See ​#3462. This adds a filter, which, I think might be useful for developers.

[peterwilsoncc]

Although it's not major, it would be good to get ​PR#3462 in to 6.1. This will avoid filling persistent caches with unreachable keys & potentially having reachable keys dropped as a result.

[spacedmonkey]

@peterwilsoncc @desrosj Are you happy for me to commit then?

Closing in favour of ​https://github.com/WordPress/wordpress-develop/pull/3462

[peterwilsoncc]

Marking for commit pending sign-off by a second committer.

[davidbaumwald]

Looks good to me!

[spacedmonkey]

In 54634:

Query: Remove placeholder from query cache key.

Remove escape placeholder from query cache key, as placeholders are a based on a unique id on every request. This means that it is impossible for a cache to be reused, making queries that use escape placeholders such as LIKE searches, unable to be cached.

Props dhl, spacedmonkey, peterwilsoncc, desrosj, chaion07, davidbaumwald, mukesh27.
Fixes #56802.

Committed ​https://github.com/WordPress/wordpress-develop/commit/7b176e6deaeb54249f7096c8ef8415d3edb46e39

[jorbin]

Reopening in order to encourage discussion of the filter added. See this long thread ​https://wordpress.slack.com/archives/C02KGN5K076/p1666106492041839

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

[audrasjb]

As per today's bugscrub,

We have a patch but it still needs review and dev feedback.

Adding high priority, given we're 3 days before RC3 :)

[peterwilsoncc]

Updating the keywords to account for the follow up PR.

[audrasjb]

As per today's bug scrub: let's keep it in the milestone as the proposed PR is under active review from various committers. This should be backported in the next few hours.

[peterwilsoncc]

@desrosj has given me the OK via Slack to put the ​linked follow up PR in to trunk. I shall do so shortly.

Marking for dev-feedback for backport consideration.

[peterwilsoncc]

In 54685:

Query: Move cache key generation to its own method.

Introduce WP_Query::generate_cache_key() for generating the cache key used by the main database query.

This removes the need for a filter to test that cache keys do not include the WPDB placeholder causing unreachable cache keys. The tests now call WP_Query::generate_cache_key() directly.

The filter wp_query_cache_key is removed as a hard deprecation. The filter was not included in a stable release.

Follow up to [54634].

Props spacedmonkey, jorbin, azaozz, hellofromtonya, mukesh27, peterwilsoncc, desrosj, audrasjb, adamsilverstein, flixos90, davidbaumwald, joedolson, sergeybiryukov.
Fixes #56802.

[peterwilsoncc]

Reopening for backport to the 6.1 branch pending a second committers approval.

Merged in https://core.trac.wordpress.org/changeset/54685 / 8b3314ad2520432f44519edf5f1be6a3cb224fe3

[chaion07]

Thanks @dlh for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback received we are expecting a second reviewer or a core committer to have a look at the ticket so that we can land it in 6.1.

Cheers!

Props to @kebbet & @robinwpdeveloper

[kirasong]

Thanks so much for everyone's work here!
Love the teamwork + communication in finding a better solution.

+1 for landing this in the 6.1 branch.

[davidbaumwald]

In 54692:

Query: Move cache key generation to its own method.

Introduce WP_Query::generate_cache_key() for generating the cache key used by the main database query.

This removes the need for a filter to test that cache keys do not include the WPDB placeholder causing unreachable cache keys. The tests now call WP_Query::generate_cache_key() directly.

The filter wp_query_cache_key is removed as a hard deprecation. The filter was not included in a stable release.

Follow up to [54685].

Props spacedmonkey, jorbin, azaozz, hellofromtonya, mukesh27, peterwilsoncc, desrosj, audrasjb, adamsilverstein, flixos90, davidbaumwald, joedolson, sergeybiryukov.
Reviewed by mikeschroder.
Merges [54685] to the 6.1 branch.
Fixes #56802.

[davidbaumwald]

Resetting the keywords after the 6.1 merge.

[AlanP57]

I'm experiencing a similar issue and have just tested a like query with RC5. The result is an SQL error and the output from the prepare function is
like '%'my search text'%'. There is an extra set of quote marks around the search text.

For more info, see ​https://wordpress.org/support/topic/prepare-function-removes-percent-signs-from-like-sql-statement/.

[spacedmonkey]

@AlanP57 Can you provide more detail. I can't replicate this. Is there some code I can look at? Is there a reason why you are doing a raw SQL query over using WP_Query. There are going to other issues if you do this...

[AlanP57]

In some cases, I prefer not to use WP_Query when displaying data. This is the code from the plugin:

<?php
      $sql = $wpdb->prepare("(select $wpdb->posts.ID, post_title, {$wpdb->prefix}mgmlp_folders.folder_id, pm.meta_value as attached_file, 'a' as item_type 
from $wpdb->posts
LEFT JOIN {$wpdb->prefix}mgmlp_folders ON($wpdb->posts.ID = {$wpdb->prefix}mgmlp_folders.post_id)
LEFT JOIN $wpdb->postmeta AS pm ON (pm.post_id = $wpdb->posts.ID)
LEFT JOIN $wpdb->users AS us ON ($wpdb->posts.post_author = us.ID) 
where post_type = 'mgmlp_media_folder' and pm.meta_key = '_wp_attached_file' and SUBSTRING_INDEX(pm.meta_value, '/', -1) like '%%%s%%')
union all
(select $wpdb->posts.ID, post_title, {$wpdb->prefix}mgmlp_folders.folder_id, pm.meta_value as attached_file, 'b' as item_type
from $wpdb->posts 
LEFT JOIN {$wpdb->prefix}mgmlp_folders ON( $wpdb->posts.ID = {$wpdb->prefix}mgmlp_folders.post_id) 
LEFT JOIN $wpdb->postmeta AS pm ON (pm.post_id = $wpdb->posts.ID) 
LEFT JOIN $wpdb->users AS us ON ($wpdb->posts.post_author = us.ID) 
where post_type = 'attachment' and pm.meta_key = '_wp_attached_file' and SUBSTRING_INDEX(pm.meta_value, '/', -1) like '%%%s%%') order by attached_file", $search_string, $search_string);

And here is an example of the SQL:
(select wp_posts.ID, post_title, wp_mgmlp_folders.folder_id, pm.meta_value as attached_file, 'a' as item_type
from wp_posts
LEFT JOIN wp_mgmlp_folders ON(wp_posts.ID = wp_mgmlp_folders.post_id)
LEFT JOIN wp_postmeta AS pm ON (pm.post_id = wp_posts.ID)
LEFT JOIN wp_users AS us ON (wp_posts.post_author = us.ID)
where post_type = 'mgmlp_media_folder' and pm.meta_key = '_wp_attached_file' and SUBSTRING_INDEX(pm.meta_value, '/', -1) like '{48bf6209debff2ee81208ffaee83c0ccfe32af6953d915a72a2fd46f1d0be2e1}'my search text'{48bf6209debff2ee81208ffaee83c0ccfe32af6953d915a72a2fd46f1d0be2e1}')
union all
(select wp_posts.ID, post_title, wp_mgmlp_folders.folder_id, pm.meta_value as attached_file, 'b' as item_type
from wp_posts
LEFT JOIN wp_mgmlp_folders ON( wp_posts.ID = wp_mgmlp_folders.post_id)
LEFT JOIN wp_postmeta AS pm ON (pm.post_id = wp_posts.ID)
LEFT JOIN wp_users AS us ON (wp_posts.post_author = us.ID)
where post_type = 'attachment' and pm.meta_key = '_wp_attached_file' and SUBSTRING_INDEX(pm.meta_value, '/', -1) like '{48bf6209debff2ee81208ffaee83c0ccfe32af6953d915a72a2fd46f1d0be2e1}'my search text'{48bf6209debff2ee81208ffaee83c0ccfe32af6953d915a72a2fd46f1d0be2e1}') order by attached_file

This use to work in versions before 6.1.

[desrosj]

I hope you don't mind, @AlanP57, but I've gone and created #56933 on your behalf. I detailed some reasons why in a comment on that ticket.

I'm going to close this out as fixed again. If you were a participant on this ticket, please see how you can help push #56933 along and rule out the changesets related to this bug (especially with 6.1 due out in a ~24 hours).