Match REST API route by namespace before performing regex checks

[TimothyBlynJacobs]

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.

[TimothyBlynJacobs]

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.

<?php
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
float(0.00058817863464355)
float(0.00049686431884766)
float(0.00046992301940918)

// After patch
float(0.00019693374633789)
float(0.0002131462097168)
float(0.00015592575073242)

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.

[davidbaumwald]

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

[TimothyBlynJacobs]

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?

[TimothyBlynJacobs]

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

[kadamwhite]

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

[kadamwhite]

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.

[david.binda]

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

[TimothyBlynJacobs]

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.

[dlh]

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.

[TimothyBlynJacobs]

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().

[dlh]

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

[kadamwhite]

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.

[desrosj]

This was detailed in the following dev note: ​https://make.wordpress.org/core/2020/02/29/rest-api-changes-in-5-4/