BaseRouter and Router have different signatures for add_route

[dave42w]

Bug Description

According to mypy None of the subclasses of BaseRouter correctly implement the abstractmethod add_route.

class BaseRouter(ABC):
    @abstractmethod
    def add_route(*args) -> Union[Callable, CoroutineType, WebSocket]: ...

vs

class Router(BaseRouter):
...
    def add_route(
        self,
        route_type: HttpMethod,
        endpoint: str,
        handler: Callable,
        is_const: bool,
        exception_handler: Optional[Callable],
        injected_dependencies: dict,
    ) -> Union[Callable, CoroutineType]:

vs

class MiddlewareRouter(BaseRouter):
...
    def add_route(
        self,
        middleware_type: MiddlewareType,
        endpoint: str,
        handler: Callable,
        injected_dependencies: dict,
    ) -> Callable:

vs

class WebSocketRouter(BaseRouter):
...
    def add_route(self, endpoint: str, web_socket: WebSocket) -> None:
        self.routes[endpoint] = web_socket

The mypy errors are:

Signature of "add_route" incompatible with supertype "BaseRouter"
...
     Superclass:
         def add_route(*args: BaseRouter) -> Callable[..., Any] | CoroutineType[Any, Any, Any] | WebSocket
     Subclass:
         def add_route(self, route_type: HttpMethod, endpoint: str, handler: Callable[..., Any], is_const: bool, exception_handler: Callable[..., Any] | None, injected_dependencies: dict[Any, Any]) -> Callable[..., Any] | CoroutineType[Any, Any, Any]
...
     Superclass:
         def add_route(*args: BaseRouter) -> Callable[..., Any] | CoroutineType[Any, Any, Any] | WebSocket
     Subclass:
         def add_route(self, middleware_type: MiddlewareType, endpoint: str, handler: Callable[..., Any], injected_dependencies: dict[Any, Any]) -> Callable[..., Any]
...
     Superclass:
         def add_route(*args: BaseRouter) -> Callable[..., Any] | CoroutineType[Any, Any, Any] | WebSocket
     Subclass:
         def add_route(self, endpoint: str, web_socket: WebSocket) -> None

I can't find any use of BaseRouter in the codebase.

Options:

1. fix the signatures in the 3 subclasses to match BaseRouter
2. remove the abstractmethod add_route from BaseRouter (all tests still pass)
3. remove the whole BaseRouter abstract class and make the others just classes (all tests still pass)

Steps to Reproduce

1. run mypy 1.13 on the codebase

Your operating system

Linux

Your Python version (python --version)

3.12

Your Robyn version

main branch

Additional Info

No response

[sansyrox]

Hey @dave42w 👋

I just want to show that every router needs to have an add_route method. It can have variable args though and the base class is supposed to show that. How can we achieve that otherwise?

[dave42w]

Hey @dave42w 👋

I just want to show that every router needs to have an add_route method. It can have variable args though and the base class is supposed to show that. How can we achieve that otherwise?

I'm not sure :-) I'll have a think

[dave42w]

Hey @dave42w 👋

I just want to show that every router needs to have an add_route method. It can have variable args though and the base class is supposed to show that. How can we achieve that otherwise?

I think we could fix by creating two more ABCs. One for the parameters eg BaseRouteArg and one for the return type eg BaseRouteReturn

For each type of Router we need two classes eg

RouterRouteArg, RouterRouteReturn
MiddlewareRouteArg, MiddlewareRouteReturn
WebSocketRouteArg, WebSocketRouteReturn

That seems a bit clumsy, but if we create these as dataclasses, we can move argument validation into them. That would simplify the add_route methods, which are currently long with deep nesting.

So now we have

class BaseRouter(ABC):
    @abstractmethod
    def add_route(self, params: BaseRouterArg) -> BaseRouterReturn: ...

[dave42w]

If the constructor for RouterRouteArg has exactly the same arguments as the Router.add_route currently does then code changes should be minimal. eg In Robyn.add_route we construct a RouterRouteArg before passing it to add_route_response = self.router.add_route

[dave42w]

Hi @sansyrox

This passes mypy. It's slightly odd to me that the ABC class needs to be used for the argument but the specific subclass can be used for the return. That's why I've added the assert.

class BaseRouterArg(ABC):
    pass

class RouterArg(BaseRouterArg):
    def __init__(self, arg1: str) -> None:
        super().__init__()
        self.arg1: str = arg1

class MiddlewareRouterArg(BaseRouterArg):
    def __init__(self, arg2: int) -> None:
        super().__init__()
        self.arg2: int = arg2

class BaseRouterReturn(ABC):
    pass

class RouterReturn(BaseRouterReturn):
    def __init__(self, arg1: str) -> None:
        super().__init__()
        self.arg1: str = arg1

class MiddlewareRouterReturn(BaseRouterReturn):
    def __init__(self, arg2: int) -> None:
        super().__init__()
        self.arg2: int = arg2

class BaseRouter(ABC):
    @abstractmethod
    def add_route(self, bra: BaseRouterArg) -> BaseRouterReturn: ...

class Router(BaseRouter):
    def add_route(self, bra: BaseRouterArg) -> RouterReturn:
        assert isinstance(bra, RouterArg)
        return RouterReturn("test")

class MiddlewareRouter(BaseRouter):
    def add_route(self, bra: BaseRouterArg) -> MiddlewareRouterReturn:
        assert isinstance(bra, MiddlewareRouterArg)
        return MiddlewareRouterReturn(1)

Are you interested in a PR for this?

[dave42w]

Hi @sansyrox,

I've been looking at this add_route issue a bit more. I'm finding it confusing, trying to follow the logic.

Are there two separate stores of routes (Robyn object as they are declared, in Rust as the app is started)?

In the Robyn instance within the router objects:

     self.router = Router()
     self.middleware_router = MiddlewareRouter()
     self.web_socket_router = WebSocketRouter()

which (I think) are only populated by these methods

Robyn.add_route # no defined return but returns add_route_response
Robyn.add_web_socket(self, endpoint: str, ws: WebSocket) -> None: # does not call Robyn.add_route
Robyn._add_openapi_routes(self, auth_required: bool = False): # calls Robyn.add_route
MiddlewareRouter.add_middleware

then there are routing tables in rust which are created for each server instance from the python Robyn object?

This differs from the architecture drawing which implies that Rust and Python are using the same router. If I'm right then my understanding is

1. Once app.start is called, we can't change routes dynamically as they have already been copied to the rust data structures?

2. Ithe python code to add_routes and parse them is not performance critical as we load them as the code is declared and then rust code processes them into a separate high performance lookup for each server process to be use as requests come in.

I would like to understand this as it feels that router.py could be refactored to be simpler without changing either the app developer experience (which I like) or the performance (which I like). That would help us get things like nesting subrouters and hopefully it would also simplify authentication and authorisation.

What I am thinking is that we simplify so the Robyn class has one Router instance. That Router instance holds it's own routes and potentially multiple subrouters.

The subrouters also hold only their own routes and potentially multiple subrouters.

This simplifies the code (we don't need both Router and SubRouter) we simply create as many Routers as we want. Any of them can become a subrouter by adding them to another Router. We make one Router the "master" by adding it to the Robyn instance.

It will be slower to parse into a complete list of routes (probably using recursion) but that is only done once to create the full route table in rust.

I guess we could add a means to get all the worker processes to reload their route table if we want to be able to dynamically change it (for example we could implement a dynamic multi-tenancy application. When we add a new tenant we add a whole new set of routes for them by adding an extra subrouter to the main application Router and then reload all the routes).

Depending on how we want to do things we could allow different middleware for each router, so supporting different authentication/authorisations policies by router.

I'm not yet sure how openapi and web_sockets fit into this

Does this make any sense or have I misread the code?

[sansyrox]

Hey @dave42w 👋

I think I partly understand your concern. But

I guess we could add a means to get all the worker processes to reload their route table if we want to be able to dynamically change it (for example we could implement a dynamic multi-tenancy application. When we add a new tenant we add a whole new set of routes for them by adding an extra subrouter to the main application Router and then reload all the routes).

What do you mean by this

or example we could implement a dynamic multi-tenancy application. When we add a new tenant we add a whole new set of routes for them by adding an extra subrouter to the main application Router and then reload all the routes).

?

[sansyrox]

And there is only one Router instance, which holds all the routes. Why would you want to change that?

[dave42w]

Hi @sansyrox

And there is only one Router instance, which holds all the routes. Why would you want to change that?

Well before suggesting changes it is critical that I understand the existing code and design :-)

I understand that in the applications Robyn instance there are three Router instances

        self.router = Router()
        self.middleware_router = MiddlewareRouter()
        self.web_socket_router = WebSocketRouter()

What I'm unclear about is when the server is started and the worker threads start do these use the routes directly from the same 3 instance variables in the apps Robyn instance? As I looked at the rust code it seemed to me that this makes a copy of the routes eg in server.rs

        let router = self.router.clone();
        let const_router = self.const_router.clone();
        let middleware_router = self.middleware_router.clone();
        let web_socket_router = self.websocket_router.clone();

and

                    app = app
                        .app_data(web::Data::new(router.clone()))
                        .app_data(web::Data::new(const_router.clone()))
                        .app_data(web::Data::new(middleware_router.clone()))
                        .app_data(web::Data::new(global_request_headers.clone()))
                        .app_data(web::Data::new(global_response_headers.clone()))
                        .app_data(web::Data::new(excluded_response_headers_paths.clone()));

So it seems to me that we have two different add_route mechanisms.

1. The app developer uses @app.get("/crimes"), app.include_router(frontend)` etc (I confess I don't understand the need for and differences between subroutes and views) to build up the routes into the app Robyn instance.

2. The app.start(...) will get the rust code to clone the routes out of the Robyn instance and somehow it has it's own _add_route and more that I don't yet understand.

Am I correct?