Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: openApi made lazy. #1066

Merged
merged 8 commits into from
Dec 31, 2024
Merged

refactor: openApi made lazy. #1066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0dccef3
refactor: openApi made lazy. This is a staging post so there are extr…
dave42w Dec 1, 2024
642eac9
refactor: openApi made lazy.
dave42w Dec 2, 2024
2ae5e7f
refactor: try to solve the speed issue by improving handling moving f…
dave42w Dec 2, 2024
f9ebbee
refactor: slightly optimised lower_http_method I've tested with 100,0…
dave42w Dec 2, 2024
b1dae0b
refactor: remove extra Robyn instance variable to avoid extra side ef…
dave42w Dec 2, 2024
cf11725
Merge branch 'sparckles:main' into lazy-openapi
dave42w Dec 3, 2024
2d261e0
Merge branch 'sparckles:main' into lazy-openapi
dave42w Dec 10, 2024
f9bf080
Merge branch 'sparckles:main' into lazy-openapi
dave42w Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 45 additions & 32 deletions robyn/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def __init__(
file_object: str,
config: Config = Config(),
openapi_file_path: Optional[str] = None,
openapi: OpenAPI = OpenAPI(),
openapi: Optional[OpenAPI] = None,
dependencies: DependencyMap = DependencyMap(),
) -> None:
directory_path = os.path.dirname(os.path.abspath(file_object))
Expand All @@ -53,10 +53,7 @@ def __init__(
self.dependencies = dependencies
self.openapi = openapi

if openapi_file_path:
openapi.override_openapi(Path(self.directory_path).joinpath(openapi_file_path))
elif Path(self.directory_path).joinpath("openapi.json").exists():
openapi.override_openapi(Path(self.directory_path).joinpath("openapi.json"))
self.init_openapi(openapi_file_path)

if not bool(os.environ.get("ROBYN_CLI", False)):
# the env variables are already set when are running through the cli
Expand All @@ -82,6 +79,20 @@ def __init__(
self.event_handlers: dict = {}
self.exception_handler: Optional[Callable] = None
self.authentication_handler: Optional[AuthenticationHandler] = None
self.included_routers: List[Router] = []

def init_openapi(self, openapi_file_path: Optional[str]) -> None:
if self.config.disable_openapi:
return

if self.openapi is None:
self.openapi = OpenAPI()
dave42w marked this conversation as resolved.
Show resolved Hide resolved

if openapi_file_path:
self.openapi.override_openapi(Path(self.directory_path).joinpath(openapi_file_path))
elif Path(self.directory_path).joinpath("openapi.json").exists():
self.openapi.override_openapi(Path(self.directory_path).joinpath("openapi.json"))
# TODO! what about when the elif fails?

def _handle_dev_mode(self):
cli_dev_mode = self.config.dev # --dev
Expand All @@ -102,6 +113,8 @@ def add_route(
handler: Callable,
is_const: bool = False,
auth_required: bool = False,
openapi_name: str = "",
openapi_tags: Union[List[str], None] = None,
):
"""
Connect a URI to a handler
Expand All @@ -117,6 +130,8 @@ def add_route(
"""
injected_dependencies = self.dependencies.get_dependency_map(self)

list_openapi_tags: List[str] = openapi_tags if openapi_tags else []

if auth_required:
self.middleware_router.add_auth_middleware(endpoint)(handler)

Expand All @@ -137,6 +152,9 @@ def add_route(
endpoint=endpoint,
handler=handler,
is_const=is_const,
auth_required=auth_required,
openapi_name=openapi_name,
openapi_tags=list_openapi_tags,
exception_handler=self.exception_handler,
injected_dependencies=injected_dependencies,
)
Expand Down Expand Up @@ -240,6 +258,15 @@ def is_port_in_use(self, port: int) -> bool:
raise Exception(f"Invalid port number: {port}")

def _add_openapi_routes(self, auth_required: bool = False):
if self.config.disable_openapi:
return

if self.openapi is None:
logger.error("No openAPI")
return

self.router.prepare_routes_openapi(self.openapi, self.included_routers)

self.add_route(
route_type=HttpMethod.GET,
endpoint="/openapi.json",
Expand Down Expand Up @@ -369,9 +396,7 @@ def get(
"""

def inner(handler):
self.openapi.add_openapi_path_obj("get", endpoint, openapi_name, openapi_tags, handler)

return self.add_route(HttpMethod.GET, endpoint, handler, const, auth_required)
Comment on lines -372 to -374
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @dave42w 👋

Could you explain this modification a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

My goal is that we do the minimum processing of routes while in the app setup stage. ie between app = Robyn(__file__) and app.start(...)

My logic is that things can happen in quite a dynamic way in this section of the code, developers might do things in an odd order (include a subrouter and then add more routes to it). Or include a subrouter in a subrouter and then include the subrouter and all it's subsubrouters. Or add middleware before the routes or in the middle of them. This could happen because we bring in a subrouter via a module and there it includes routes and middleware. Or want different middleware for the subrouter from accounting compared to sales etc.

Therefore if we call add_openapi_path_obj in add_route the path might not yet be fixed. So it takes more logic (and potentially doing things multiple times) to handle these variations.

When we have calls to openapi scattered around the code then it gets expensive in runtime and maintenance if in every case we want to only make the call if openapi is enabled. It gets slow to maintain if we have 10 calls to add_openapi_path_obj and need to change it.

Of course if we are going to collect together all the calls to add_openapi_path_obj and do them in one place as the server starts then we need to have the handler, openapi_name and openapi_tags available during app.start(). This is why I have added them to the Route class.

I'm pretty sure that when I refactor add_route it will be a lot cleaner if we create an incomplete Route instance much earlier and then pass the single object around the system rather than 7 arguments. Deep in the system we do the work to complete function_info to finish the Route instance. That will also reduce the need for parsing the arguments all over the place (maybe Route needs to be a dataclass to validate itself). All that is for a future PR.

return self.add_route(HttpMethod.GET, endpoint, handler, const, auth_required, openapi_name, openapi_tags)

return inner

Expand All @@ -392,9 +417,7 @@ def post(
"""

def inner(handler):
self.openapi.add_openapi_path_obj("post", endpoint, openapi_name, openapi_tags, handler)

return self.add_route(HttpMethod.POST, endpoint, handler, auth_required=auth_required)
return self.add_route(HttpMethod.POST, endpoint, handler, auth_required=auth_required, openapi_name=openapi_name, openapi_tags=openapi_tags)

return inner

Expand All @@ -415,9 +438,7 @@ def put(
"""

def inner(handler):
self.openapi.add_openapi_path_obj("put", endpoint, openapi_name, openapi_tags, handler)

return self.add_route(HttpMethod.PUT, endpoint, handler, auth_required=auth_required)
return self.add_route(HttpMethod.PUT, endpoint, handler, auth_required=auth_required, openapi_name=openapi_name, openapi_tags=openapi_tags)

return inner

Expand All @@ -438,9 +459,7 @@ def delete(
"""

def inner(handler):
self.openapi.add_openapi_path_obj("delete", endpoint, openapi_name, openapi_tags, handler)

return self.add_route(HttpMethod.DELETE, endpoint, handler, auth_required=auth_required)
return self.add_route(HttpMethod.DELETE, endpoint, handler, auth_required=auth_required, openapi_name=openapi_name, openapi_tags=openapi_tags)

return inner

Expand All @@ -461,9 +480,7 @@ def patch(
"""

def inner(handler):
self.openapi.add_openapi_path_obj("patch", endpoint, openapi_name, openapi_tags, handler)

return self.add_route(HttpMethod.PATCH, endpoint, handler, auth_required=auth_required)
return self.add_route(HttpMethod.PATCH, endpoint, handler, auth_required=auth_required, openapi_name=openapi_name, openapi_tags=openapi_tags)

return inner

Expand All @@ -484,9 +501,7 @@ def head(
"""

def inner(handler):
self.openapi.add_openapi_path_obj("head", endpoint, openapi_name, openapi_tags, handler)

return self.add_route(HttpMethod.HEAD, endpoint, handler, auth_required=auth_required)
return self.add_route(HttpMethod.HEAD, endpoint, handler, auth_required=auth_required, openapi_name=openapi_name, openapi_tags=openapi_tags)

return inner

Expand All @@ -507,9 +522,7 @@ def options(
"""

def inner(handler):
self.openapi.add_openapi_path_obj("options", endpoint, openapi_name, openapi_tags, handler)

return self.add_route(HttpMethod.OPTIONS, endpoint, handler, auth_required=auth_required)
return self.add_route(HttpMethod.OPTIONS, endpoint, handler, auth_required=auth_required, openapi_name=openapi_name, openapi_tags=openapi_tags)

return inner

Expand All @@ -530,8 +543,7 @@ def connect(
"""

def inner(handler):
self.openapi.add_openapi_path_obj("connect", endpoint, openapi_name, openapi_tags, handler)
return self.add_route(HttpMethod.CONNECT, endpoint, handler, auth_required=auth_required)
return self.add_route(HttpMethod.CONNECT, endpoint, handler, auth_required=auth_required, openapi_name=openapi_name, openapi_tags=openapi_tags)

return inner

Expand All @@ -552,9 +564,7 @@ def trace(
"""

def inner(handler):
self.openapi.add_openapi_path_obj("trace", endpoint, openapi_name, openapi_tags, handler)

return self.add_route(HttpMethod.TRACE, endpoint, handler, auth_required=auth_required)
return self.add_route(HttpMethod.TRACE, endpoint, handler, auth_required=auth_required, openapi_name=openapi_name, openapi_tags=openapi_tags)

return inner

Expand All @@ -564,11 +574,14 @@ def include_router(self, router):

:param router Robyn: the router object to include the routes from
"""
self.included_routers.append(router)

self.router.routes.extend(router.router.routes)
self.middleware_router.global_middlewares.extend(router.middleware_router.global_middlewares)
self.middleware_router.route_middlewares.extend(router.middleware_router.route_middlewares)

self.openapi.add_subrouter_paths(router.openapi)
if not self.config.disable_openapi and self.openapi is not None:
self.openapi.add_subrouter_paths(self.openapi)

# extend the websocket routes
prefix = router.prefix
Expand Down
2 changes: 1 addition & 1 deletion robyn/processpool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ def spawn_process(
server.set_response_headers_exclude_paths(excluded_response_headers_paths)

for route in routes:
route_type, endpoint, function, is_const = route
route_type, endpoint, function, is_const, auth_required, openapi_name, openapi_tags = route
Copy link
Member

@sansyrox sansyrox Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question @dave42w 👋

Where are the auth_required, openapi_name , etc used here? Could we do a better job in unpacking if not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's preparation for the full route refactor. Then the authentication route processing happens in server start rather than in add_route.
I wanted to reduce the times I need to touch the code and so put this in before it's needed.

server.add_route(route_type, endpoint, function, is_const)

for middleware_type, middleware_function in global_middlewares:
Expand Down
24 changes: 22 additions & 2 deletions robyn/router.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from robyn.authentication import AuthenticationHandler, AuthenticationNotConfiguredError
from robyn.dependency_injection import DependencyMap
from robyn.jsonify import jsonify
from robyn.openapi import OpenAPI
from robyn.responses import FileResponse
from robyn.robyn import FunctionInfo, Headers, HttpMethod, Identity, MiddlewareType, QueryParams, Request, Response, Url
from robyn.types import Body, Files, FormData, IPAddress, Method, PathParams
Expand All @@ -19,11 +20,18 @@
_logger = logging.getLogger(__name__)


def lower_http_method(method: HttpMethod):
return (str(method))[11:].lower()


class Route(NamedTuple):
route_type: HttpMethod
route: str
function: FunctionInfo
is_const: bool
auth_required: bool
openapi_name: str
openapi_tags: List[str]
sansyrox marked this conversation as resolved.
Show resolved Hide resolved


class RouteMiddleware(NamedTuple):
Expand Down Expand Up @@ -108,6 +116,9 @@ def add_route( # type: ignore
endpoint: str,
handler: Callable,
is_const: bool,
auth_required: bool,
openapi_name: str,
openapi_tags: List[str],
dave42w marked this conversation as resolved.
Show resolved Hide resolved
exception_handler: Optional[Callable],
injected_dependencies: dict,
) -> Union[Callable, CoroutineType]:
Expand Down Expand Up @@ -226,7 +237,7 @@ def inner_handler(*args, **kwargs):
params,
new_injected_dependencies,
)
self.routes.append(Route(route_type, endpoint, function, is_const))
self.routes.append(Route(route_type, endpoint, function, is_const, auth_required, openapi_name, openapi_tags))
return async_inner_handler
else:
function = FunctionInfo(
Expand All @@ -236,9 +247,18 @@ def inner_handler(*args, **kwargs):
params,
new_injected_dependencies,
)
self.routes.append(Route(route_type, endpoint, function, is_const))
self.routes.append(Route(route_type, endpoint, function, is_const, auth_required, openapi_name, openapi_tags))
return inner_handler

def prepare_routes_openapi(self, openapi: OpenAPI, included_routers: List) -> None:
for route in self.routes:
openapi.add_openapi_path_obj(lower_http_method(route.route_type), route.route, route.openapi_name, route.openapi_tags, route.function.handler)

# TODO! after include_routes does not immediatelly merge all the routes
# for router in included_routers:
# for route in router:
# openapi.add_openapi_path_obj(lower_http_method(route.route_type), route.route, route.openapi_name, route.openapi_tags, route.function.handler)

Comment on lines +256 to +261
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dave42w , what do we mean by this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, when we include_routes all the routes from a subrouter get copied into the Robyn router immediately. This means we shouldn't add extra routes after calling include_routes.
The full routes refactor will move all evaluation and processing of routes and routers to when the server starts. So add_route is just adding route objects to the current router. Also include_routes just adds a router to the parent without copying any routes. This massively simplifies these processes and means that the ordering of adding routes and routers does not matter.
The server start then builds the complete route table, handling nesting of routers, open_api registration, authentication, middleware after the application developer has registered everything in any order.
The rust processes will get the same routes as they do now but we do all the route processing once in one place.
The comment is a placeholder to remind me not to forget to make sure that openapi still handles things properly (it might not be this code depending on the order that seems best for handling both include _routes and add_openapi_path_obj).

def get_routes(self) -> List[Route]:
return self.routes

Expand Down
Loading