Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#48530 closed enhancement (fixed)

Match REST API route by namespace before performing regex checks

Reported by: timothyblynjacobs's profile TimothyBlynJacobs Owned by: kadamwhite's profile kadamwhite
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: has-patch has-unit-tests has-dev-note
Focuses: performance Cc:


Right now, in order to match a request to a specific route handler, we iterate over every route and perform a regex check.

Whenever a route is registered, you must specify a namespace. If we first checked the namespaces for a match, we'd be able to eliminate whole groups of routes from the regex check.

Attachments (4)

48530.diff (2.7 KB) - added by TimothyBlynJacobs 5 years ago.
48530.2.diff (2.7 KB) - added by TimothyBlynJacobs 4 years ago.
48530.3.diff (1.7 KB) - added by david.binda 4 years ago.
48530.4.diff (2.1 KB) - added by TimothyBlynJacobs 4 years ago.

Download all attachments as: .zip

Change History (22)

#1 @kadamwhite
5 years ago

  • Milestone changed from Awaiting Review to 5.4

#2 @TimothyBlynJacobs
5 years ago

  • Keywords has-patch has-unit-tests added

Here is a first pass at doing this filtering. I opted to add a filter argument to WP_REST_Server::get_routes() because the transformation process it applies results in losing the namespace registration route argument being lost so we can't easily filter the list after.

Right now, the list is filtered before apply_filters( 'rest_endpoints' ), that feels somewhat better to me since whatever is happening in that filter would be able to work on less items. We could filter the list after the apply_filters( 'rest_endpoints' ) too. I guess that would make sense if we were worried that someone might be caching the results of their filter call?

I did a simple performance test. Please double check my workings, it could be completely bogus :)

Registering 10 namespaces with 15 routes each.

function test_dispatch_performance() {
        $namespaces = 10;
        $routes     = 15;

        for ( $i = 0; $i < $namespaces; $i ++ ) {
                for ( $j = 0; $j < $routes; $j ++ ) {
                        register_rest_route( 'my-namespace-' . $i, '/my-' . $j . '-route/(?P<id>[\d]+)' , array(
                                'methods'  => 'GET',
                                'callback' => '__return_empty_array',
                        ) );

        $start = microtime( true );
        rest_get_server()->dispatch( new WP_REST_Request( 'GET', sprintf( '/my-namespace-%d/my-%d-route/5', 0, 0 ) ) );
        var_dump( microtime( true ) - $start );

        $start = microtime( true );
        rest_get_server()->dispatch( new WP_REST_Request( 'GET', sprintf( '/my-namespace-%d/my-%d-route/5', $namespaces / 2, $routes / 2 ) ) );
        var_dump( microtime( true ) - $start );

        $start = microtime( true );
        rest_get_server()->dispatch( new WP_REST_Request( 'GET', sprintf( '/my-namespace-%d/my-%d-route/5', $namespaces - 1, $routes - 1 ) ) );
        var_dump( microtime( true ) - $start );
// Before patch

// After patch

It looks about 2.5x faster? While the absolute numbers are small, this can be run hundreds of times during a request for a collection response with embedding enabled.

I'd like to try and figure out a real world test that would work. But my initial tests using a bunch of plugins with REST API routes showed way too much variance between API requests to get meaningful data out of it. If anyone has any ideas that'd be awesome.

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

4 years ago

#4 @davidbaumwald
4 years ago

@TimothyBlynJacobs How is this looking for 5.4 with Beta 1 coming up in a few days?

#5 @TimothyBlynJacobs
4 years ago

Since #48838 this is less relevant, but I think would still be useful. Going to take a look at our open 5.4 tickets over the next few days.

I think the biggest open question to me is whether the added filtering support to get_routes makes sense. Maybe you have thoughts on that @kadamwhite?

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

4 years ago

#7 @TimothyBlynJacobs
4 years ago

Attached a new patch that makes the namespace parameter more reusable by letting it accepting a list of filters for wp_list_filter().

#8 @kadamwhite
4 years ago

@TimothyBlynJacobs and I discussed in Slack (DM thread, unfortunately) that because namespace is the only key-value pair on the namespace arrays that would be worth matching against, the string namespace parameter feels preferable.

This ticket was mentioned in PR #154 on WordPress/wordpress-develop by TimothyBJacobs.

4 years ago

#10 @kadamwhite
4 years ago

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

In 47260:

REST API: Match REST API routes on namespace before performing regex checks.

Rule out groups of API endpoints by simple namespace string comparison to reduce the number of regex checks necessary when matching a route.

Props TimothyBlynJacobs.
Fixes #48530.

#11 @david.binda
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

While testing the related patch, I have run into an issue with clashing namespaces resulting in routes not being found. This might be an edge case, but still feels like a backward compatibility breaking change.

In case there is a namespace extending a shorter one, it's routes are not being recognised.

For instance, routes in a namespace test-ns/v1 would not be found, if there is a test-ns namespace also registered. While it might not be the best use-case to have such namespaces, it definitely worked prior r47260

I'm attaching a patch with a unit test showing the issue (which is passing prior r47260, but not afterwards).

Feels like we could still preserve some of the performance improvements by not breaking the foreach cycle if a match on a namespace is found, but rather continue the cycle, while merging found namespaces together (see attached diff).

4 years ago

#12 @TimothyBlynJacobs
4 years ago

This is a great catch, thanks @davidbinda! We should definitely fix this.

I made a small change to your patch so we don't call array_merge in a loop ( info ).

As well as more accurately testing that it is a namespace match by adding a trailing slash in the comparison.

This ticket was mentioned in PR #156 on WordPress/wordpress-develop by TimothyBJacobs.

4 years ago

#14 @dlh
4 years ago

I'd like to also suggest that this ticket could use a dev note, which I'd be happy to draft. The wp_rest_server_class filter allows developers to use a custom extension to WP_REST_Server, and classes that have overloaded get_routes() (as we do on some of our projects) will start to see a PHP warning because the signature is no longer compatible.

#15 @TimothyBlynJacobs
4 years ago

Updated my PR with some more test cases.

@dlh Agreed. If you have time to draft that that'd be greatly appreciated! I think we should also mention in that post the signature change to embed_links().

#16 @dlh
4 years ago

  • Keywords needs-dev-note added

Sounds good. I'll try to have it in the first half of this week.

#17 @kadamwhite
4 years ago

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

In 47351:

REST API: Fix namespace shadowing issue in route matching logic.

Following [47260] a namespace such as "test-ns" prevents any namespace such as "test-ns/v1" from being found when matching routes.
While not best practice, this was an unintentional back-compat break; this patch restores the original behavior.

Props david.binda, TimothyBlynJacobs.
Fixes #48530.

#18 @desrosj
4 years ago

  • Keywords has-dev-note added; needs-dev-note removed

This was detailed in the following dev note:

Note: See TracTickets for help on using tickets.