Detect routes that collide during registration

[bagerard]

I've recently been hit by this problem, after changing the ordering in which controllers were registered, some routes weren't called.

This is because some routes using pattern matching were registered before other routes.
For instance route /users/{user_id} registered before /users/export

Of course this can be fixed by changing the ordering in which routes get registered but it is something that is probably bugging other users and that can be detected.

I was able to have a unit test that would detect this in our project (by parsing proutes output) but I was wondering if that is a feature that could be of interest to be baked in pyramid itself? I'd be happy to hack on this, please just point me to a starting point in the codebase

[stevepiercy]

I think that at least this would be a good recipe for the Community Cookbook if nothing else. The Testing section is pretty thin.

https://docs.pylonsproject.org/projects/pyramid_cookbook/en/latest/testing/index.html

Maybe a new page for Pyramid pytest recipes?

See also https://docs.pylonsproject.org/projects/pyramid/en/latest/designdefense.html#routes-need-relative-ordering and https://docs.pylonsproject.org/projects/pyramid/en/latest/narr/urldispatch.html#route-declaration-ordering

[mmerickel]

I think it would be hard to craft a patch that we would merge for this. There are so many corner cases where warnings are not desirable when you bring in other features of route configuration that can be used to solve conflicts - namely route predicates.

[bagerard]

Alright no problem, I understand and for sure I don't want to add code that would be difficult to maintain.

What I have is the following (to give a rough idea of how it looks, there are still things to improve in order to make it more generic). It is plug-and-play with the pyramid cookie cutter

test_routes_consistency.py

import re
from dataclasses import dataclass
from typing import List

import pytest
from pyramid.paster import bootstrap
from pyramid.scripts.proutes import get_route_data


@dataclass
class AppRoute:
    name: str
    pattern: str
    view_module: str
    request_methods: str


@pytest.fixture
def pyramid_app_routes(ini_file) -> List[AppRoute]:
    def _get_mapper(registry):
        from pyramid.config import Configurator

        config = Configurator(registry=registry)
        return config.get_routes_mapper()

    env = bootstrap(ini_file, options={})

    registry = env["registry"]
    mapper = _get_mapper(registry)
    raw_routes = mapper.get_routes(include_static=True)

    app_routes = []
    for raw_route in raw_routes:
        route_group = get_route_data(raw_route, registry)
        app_routes += [AppRoute(*route) for route in route_group]

    return app_routes


def test_pyramid_routes_swallowing_due_to_route_registration_ordering_issue(
    pyramid_app_routes,
):
    """The order in which routes are registered matters as explained here
    https://docs.pylonsproject.org/projects/pyramid/en/latest/designdefense.html#routes-need-relative-ordering

    This means that it is important to register specific route BEFORE generic ones

    For instance: it is important to register "/users/export" before "/users/{id}"

    This test aims at detecting generic route that would "swallow" other route, thus
    suggesting a route-ordering problem.
    """

    def route_to_regex_pattern(route):
        regex_matching_route = re.sub(r"{[a-zA-Z]*}", lambda x: "[a-z]+", route)
        return f"^{regex_matching_route}$"

    def route_match_pattern(route, pattern):
        return bool(re.match(pattern, route))

    def is_matching_route(route):
        return all(c in route for c in ("{", "}"))

    clashes = set()
    for idx, route in enumerate(pyramid_app_routes):

        if not is_matching_route(route.pattern):
            continue

        re_pattern = route_to_regex_pattern(route.pattern)

        next_routes = pyramid_app_routes[idx+1:]
        for next_route in next_routes:
            if route.pattern == next_route.pattern:
                continue

            if (
                route_match_pattern(next_route.pattern, pattern=re_pattern)
                and route.request_methods == next_route.request_methods
            ):
                clashes.add(
                    f"The route {route.pattern} ({route.view_module}) is swallowing {next_route.pattern} ({next_route.view_module})"
                )

        assert clashes == set()

Were you suggesting to add a bullet point below "Testing a POST request using cURL" in this page. If that's still what you want, I can open a PR and then we can go through formal code review there of course

[stevepiercy]

Were you suggesting to add a bullet point below "Testing a POST request using cURL" in this page. If that's still what you want, I can open a PR and then we can go through formal code review there of course

Yes, as well as a link to a new page that contains the recipe.

[huntcsg]

I wonder if this would be an interesting use of an add-on, for the wide number of simple apps that are not using more sophisticated features.

like pyramid_strict_routes that would do some configurable start up validation.