REST API: Avoid DB queries from generating sample permalink when possible

[dlh]

The results of get_sample_permalink() were added to the REST API as part of #45017. Although the permalink_template and generated_slug fields are used only in the edit context, the sample permalink is always generated, which leads to at least one direct database request via wp_unique_post_slug() for each post.

The attached patch would add a check for whether the permalink_template or generated_slug fields have been requested before generating the sample permalink.

However, the attached patch is still not great because permalink_template and generated_slug will be considered requested fields by default.

One potential workaround for this default behavior would be to deliberately check the request context as well as the requested fields before generating the permalink.

However, that seems duplicative of the work that \WP_REST_Controller::filter_response_by_context() is meant to do, so I'm not sure it would be the best approach.

[TimothyBlynJacobs]

As an initial impression, I would be +1 to accounting for the context in get_fields_for_response(). I think any BC breaks due to selective including of fields at the controller level would have already happened when implementing get_fields_for_response() in the first place.

[RogerTheriault]

I came across this ticket while investigating performance issues relating to many database requests from wp_unique_post_slug() in situations where a published attachment had an unfortunately common title such as '1' and a REST request was made that includes the embeds ( e.g. /wp-json/wp/v2/posts/1234567?_embed=true )

The patch doesn't seem to reduce those.

I may have a minor addition to suggest, but could use a sanity check...

Is the third parameter for get_sample_permalink() necessary here? The default is null yet an empty string is being passed. It causes the post's current post_name to be ignored and as a result, the do loop in wp_unique_post_slug() iterates wastefully - at least for the cases we've tested. (In fact, very wastefully in some cases... many uncached queries where there are a lot of uploads with the same short title)

My suggestion would be to change that line to
$sample_permalink = get_sample_permalink( $post->ID, $post->post_title );

If that should go in a separate ticket, I can open one.

[dlh]

Replying to RogerTheriault:

I came across this ticket while investigating performance issues relating to many database requests from wp_unique_post_slug() in situations where a published attachment had an unfortunately common title such as '1' and a REST request was made that includes the embeds ( e.g. /wp-json/wp/v2/posts/1234567?_embed=true )

The patch doesn't seem to reduce those.

Correct, the patch currently addresses only cases where the _fields parameter is passed. To address requests without _fields (like for embeds), the request context would also need to be checked.

I'm happy to modify the patch to check context if there's a consensus that this is an exceptional case making it OK to do so. @TimothyBlynJacobs, can I ask what your impression is now, with some time having passed?

[TimothyBlynJacobs]

I think adding something like this to WP_REST_Controller::get_fields_for_response() makes sense.

if ( ! empty( $request['context'] ) ) {
        foreach ( $fields as $i => $field ) {
                if ( isset( $schema['properties'][ $field ]['context'] ) && ! in_array( $request['context'], $schema['properties'][ $field ]['context'] ) ) {
                        unset( $fields[ $i ] );
                }
        }
}

That would essentially extend the _fields performance increases to all requests. After the _fields performance enhancement, code would've had to been updated to be aware that any field may no longer be in the response when filtering on rest_prepare_post. If the code hadn't been updated, it would be more likely to experience an issue with that change. Thoughts @kadamwhite @danielbachhuber?

If that is deemed too risky, I think checking the context of the request manually is fine in this scenario.

Is the third parameter for get_sample_permalink() necessary here?...

This was introduced here: ​https://github.com/WordPress/gutenberg/commit/3d93ac6a2f60fa98a599196785f25a68b9997c12

[RogerTheriault]

Replying to TimothyBlynJacobs:

I think adding something like this to WP_REST_Controller::get_fields_for_response() makes sense.

I tested that with the requests I was concerned about, e.g. /wp-json/wp/v2/posts?_embed=true

Happy to say the get_sample_permalink() code was not being executed for those requests.

Is the third parameter for get_sample_permalink() necessary here?...

This was introduced here: ​https://github.com/WordPress/gutenberg/commit/3d93ac6a2f60fa98a599196785f25a68b9997c12

Thanks. It seems like that may have been overly broad.

I think this fix will help, hopefully it has no adverse effect on other requests.

[danielbachhuber]

Replying to TimothyBlynJacobs:

I think adding something like this to WP_REST_Controller::get_fields_for_response() makes sense.

I'm amenable to this.

That would essentially extend the _fields performance increases to all requests. After the _fields performance enhancement, code would've had to been updated to be aware that any field may no longer be in the response when filtering on rest_prepare_post. If the code hadn't been updated, it would be more likely to experience an issue with that change.

I agree. The _fields performance change had a minor back-compat break for rest_prepare_post. Given there doesn't seem to be any negative fallout, it seems reasonable to implement @TimothyBlynJacobs' approach.

Notably, we need to make sure that any fields without a defined context are still included by default.

[dlh]

45605.2.diff​ adds the context check to WP_REST_Controller::get_fields_for_response() and related tests. It also removes a call to trim() that (I think) is unnecessary now that wp_parse_list() is in use.

This new patch combines with 45605.diff​ to address the original issue described in this ticket. The patches are related in the context of the ticket but seemed conceptually separable, so I've kept them separate, but I can combine them into one patch if desired.

[dlh]

I've refreshed the two patches, and they should be ready for review. As with before, I've kept the patches separate as they can be seen as addressing different concerns.

In 45605.3.diff​: Generate a sample permalink only when a field that depends on it is requested.

In 45605.4.diff​: In WP_REST_Controller::get_fields_for_response(), exclude fields that specify a different context than the request context.

[TimothyBlynJacobs]

.3 looks good to me!

For .4, why was $requested_fields = array_map( 'trim', $requested_fields ); removed?

[dlh]

@TimothyBlynJacobs I got it into my head that it was unnecessary because wp_parse_list() used PREG_SPLIT_NO_EMPTY, but, I see now, that was plain crazy. Restored in 45605.5.diff​.

[kadamwhite]

In 45705:

REST API: Generate sample permalink only when a dependent field is requested.

The sample permalink will now only be generated if the derivative permalink_template or generated_slug fields are to be included in the response, preventing an unnecessary database request for each post (via wp_unique_post_slug()) when those fields are not requested.

Props dlh.
See #45605.

[kadamwhite]

In 45706:

REST API: Skip processing fields which are not present in the selected context.

In WP_REST_Controller::get_fields_for_response(), exclude fields which are not registered to appear in the request's context.

In conjunction with r45705 this prevents the unnecessary computation of the sample permalink when making a request that is not context=edit.

Props dlh.
Fixes #45605.