sparckles / Robyn

Robyn is a Super Fast Async Python Web Framework with a Rust runtime.
https://robyn.tech/
BSD 2-Clause "Simplified" License
4.46k stars 231 forks source link

processpool.py spawn_process is only calling server stubs #1029

Closed dave42w closed 1 week ago

dave42w commented 1 week ago

Bug Description

Three bugs.

  1. Type checking has caught a problem.
  2. all the methods in Server called by spawn_process (eg add_route) are empty in the Server class (all they do is pass)
  3. I can't see any tests for any of the processpool methods

Steps to Reproduce

Type checking has uncovered reusing local scope variables for a different type.

    for route in routes:
        route_type, endpoint, function, is_const = route
        server.add_route(route_type, endpoint, function, is_const)
...
    for route_type, endpoint, function in route_middlewares:
        server.add_middleware_route(route_type, endpoint, function)

route_type is an HttpMethod in the route for loop In the route_middlewares route_type is supposed to be a MiddlewareType

A quick fix is

    for route in routes:
        route_type, endpoint, function, is_const = route
        server.add_route(route_type, endpoint, function, is_const)
...
    for mid_route_type, endpoint, function in route_middlewares:
        server.add_middleware_route(mid_route_type, endpoint, function)

However, there are bigger problems. All these server.add_... methods that spawn_process calls are defined as only pass in the server class. They do nothing.

I can't see any tests that check the Server class has all the routes. So what's going on? How is the system working?

From a style perspective, I can't see why we are taking the Route and RouteMiddleware objects apart anyway. Why don't we just pass the list of routes to the server so it can make a copy?

Also, why are we keeping so many stale copies of Routes? If I dynamically add a Route to the Robyn object, will that route be available everywhere?

Your operating system

Linux

Your Python version (python --version)

3.12

Your Robyn version

main branch

Additional Info

No response

sansyrox commented 1 week ago

Hey @dave42w 👋

However, there are bigger problems. All these server.add_... methods that spawn_process calls are defined as only pass in the server class. They do nothing.

*.pyi files are for type definitions only. These are defined in the src folder and exactly src/server.rs file.

And how are these two code samples different?

for route in routes:

route_type, endpoint, function, is_const = route server.add_route(route_type, endpoint, function, is_const) ... for route_type, endpoint, function in route_middlewares: server.add_middleware_route(route_type, endpoint, function) route_type is an HttpMethod in the route for loop In the route_middlewares route_type is supposed to be a MiddlewareType

A quick fix is

for route in routes:
    route_type, endpoint, function, is_const = route
    server.add_route(route_type, endpoint, function, is_const)

... for mid_route_type, endpoint, function in route_middlewares: server.add_middleware_route(mid_route_type, endpoint, function)

dave42w commented 1 week ago

Ok, so I don't understand the relationship between the rust src in the src directory and the python source in the robyn directory.

I've been trying a small refactor to tidy robyn/__init.py by moving these lines from Robyn.add_route to be a function next to HttpMethod(enum) in robyn/robyn.pyi

from this in Robyn.add_route

        if isinstance(route_type, str):
            http_methods = {
                "GET": HttpMethod.GET,
                "POST": HttpMethod.POST,
                "PUT": HttpMethod.PUT,
                "DELETE": HttpMethod.DELETE,
                "PATCH": HttpMethod.PATCH,
                "HEAD": HttpMethod.HEAD,
                "OPTIONS": HttpMethod.OPTIONS,
            }
            route_type = http_methods[route_type]

to this in robyn.pyi

def get_http_method(method: Union[HttpMethod, str]) -> HttpMethod:
    if isinstance(method, str):
        http_methods = {
            "GET": HttpMethod.GET,
            "POST": HttpMethod.POST,
            "PUT": HttpMethod.PUT,
            "DELETE": HttpMethod.DELETE,
            "PATCH": HttpMethod.PATCH,
            "HEAD": HttpMethod.HEAD,
            "OPTIONS": HttpMethod.OPTIONS,
        }
        return http_methods[method]

    return method

I'm getting inconsistent errors when running the test server with not finding get_http_method.

Is this because of this rust src issue? I can't see Robyn defined in the rust code?

sansyrox commented 1 week ago

Hey @dave42w 👋

*.pyi files are not supposed to have any business logic. Just type stubs

dave42w commented 1 week ago

Hey @dave42w 👋

*.pyi files are not supposed to have any business logic. Just type stubs

Is there a good learning resource to explain how the python code gets into robyn.pyi?

There looks to me to be more than just type stubs?

sansyrox commented 1 week ago

Is there a good learning resource to explain how the python code gets into robyn.pyi?

You need to add the type definition manually. Once you add something in src/lib.rs , it is available to import from the robyn module. However, to make sure mypy and python linters detect the code, you need to add the type hints manually in robyn.pyi files.

dave42w commented 1 week ago

You need to add the type definition manually. Once you add something in src/lib.rs , it is available to import from the robyn module. However, to make sure mypy and python linters detect the code, you need to add the type hints manually in robyn.pyi files.

Ok, this makes more sense now. It explains why I was confused by all the pass statements in robyn.pyi.

Can we add a docstring comment to the top of robyn.pyi to explain this?

Thanks

dave42w commented 1 week ago

I'm going to close this add add a new issue for just the missing tests