WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 4 months ago

#45605 closed defect (bug) (fixed)

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

Reported by: dlh Owned by: dlh
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.0
Component: REST API Keywords: has-patch has-unit-tests
Focuses: Cc:
PR Number:

Description

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.

Attachments (5)

45605.diff (1.6 KB) - added by dlh 12 months ago.
45605.2.diff (4.8 KB) - added by dlh 10 months ago.
45605.3.diff (1.6 KB) - added by dlh 6 months ago.
45605.4.diff (4.8 KB) - added by dlh 6 months ago.
45605.5.diff (4.3 KB) - added by dlh 5 months ago.

Download all attachments as: .zip

Change History (28)

@dlh
12 months ago

#1 @TimothyBlynJacobs
12 months ago

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.

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


12 months ago

#3 @SergeyBiryukov
12 months ago

  • Milestone changed from Awaiting Review to 5.1

#4 follow-up: @RogerTheriault
11 months ago

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.

#5 in reply to: ↑ 4 @dlh
11 months ago

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?

#6 follow-ups: @TimothyBlynJacobs
11 months ago

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

#7 in reply to: ↑ 6 @RogerTheriault
11 months ago

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. k/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.

Version 1, edited 11 months ago by RogerTheriault (previous) (next) (diff)

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


11 months ago

#9 in reply to: ↑ 6 @danielbachhuber
11 months ago

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.

#10 @pento
11 months ago

  • Keywords needs-refresh added
  • Milestone changed from 5.1 to Future Release

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


10 months ago

@dlh
10 months ago

#12 @dlh
10 months ago

  • Keywords needs-refresh removed

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.

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


9 months ago

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


9 months ago

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


7 months ago

#16 @kadamwhite
7 months ago

  • Milestone changed from Future Release to 5.3
  • Owner set to dlh
  • Status changed from new to assigned

@dlh
6 months ago

@dlh
6 months ago

#17 @dlh
6 months ago

  • Keywords has-unit-tests added

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.

#18 @TimothyBlynJacobs
5 months ago

.3 looks good to me!

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

@dlh
5 months ago

#19 @dlh
5 months ago

@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.

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


5 months ago

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


4 months ago

#22 @kadamwhite
4 months ago

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.

#23 @kadamwhite
4 months ago

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

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.

Note: See TracTickets for help on using tickets.