refactor: openApi made lazy.

[dave42w]

Makes openAPI lazy (only created if enabled, all methods only called if enabled).

Fixes a potential issue in init where the default argument creates a shared object at parse time rather than a separate one for each instance. As only one instance, not normally an issue but not good practice.

One FIXME is an edge case in init_openapi where neither if clause evaluates to True for openapi_file_path (should we log an error or what?)

Description

This PR is progressing towards the larger refactor to simplify the processing of add_route. See #1059

Summary

This PR does make it so that

1. the openApi instance variable is only created if needed
2. the openApi is created if needed (and not passed as argument) at Robyn.init not at parse time
3. all the calls to add_openapi_path_obj are made in app.start so after all the routes have been fully sorted (only if openApi is enabled)

Please look carefully at the TODO We should cover this edge case

        if self.openapi_file_path:
            self.openapi.override_openapi(Path(self.directory_path).joinpath(self.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?

PR Checklist

Please ensure that:

• The PR contains a descriptive title
• The PR contains a descriptive summary of the changes
• You build and test your changes before submitting a PR.
• You have added relevant documentation {No API changes}
• You have added relevant tests. We prefer integration tests wherever possible

Pre-Commit Instructions:

• Ensure that you have run the pre-commit hooks on your PR.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
robyn	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 19, 2024 4:15pm

[codspeed-hq]

CodSpeed Performance Report

Merging #1066 will degrade performances by 47.63%

_{Comparing dave42w:lazy-openapi (f9bf080) with main (7495342)}

Summary

❌ 5 regressions
✅ 141 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	main	dave42w:lazy-openapi	Change
❌	test_add_request_header	341.9 µs	652.8 µs	-47.63%
❌	test_add_response_header	342 µs	652.5 µs	-47.59%
❌	test_allow_cors	455.3 µs	756.7 µs	-39.83%
❌	test_lifecycle_handlers	373.5 µs	682.9 µs	-45.32%
❌	test_custom_openapi_spec	466.3 µs	765.7 µs	-39.11%

[dave42w]

Second Commit 642eac9

This should reduce the performance problem as we no longer add openapi routes in the decorators. Instead we store the openapi_name and openapi_tags (plus auth_rerquired because it will be needed) in the Route.
So we can build all the openapi routes together in app.start (if they are needed)

All tests are passing. This will complete the openapi part of the add_routes refactor.

[dave42w]

@sansyrox if you are happy with this direction then this PR is ready

Once this is in I'll start another PR for the add_router next step (which will keep routes local to the router/subrouter until we start the server).

[sansyrox]

Hey @dave42w 👋

Could you explain this modification a bit?

[dave42w]

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.

[sansyrox]

Hey @dave42w 👋

Thank you for the PR 😄

I have some questions regarding the implementations

[dave42w]

Hey @dave42w 👋

Thank you for the PR 😄

I have some questions regarding the implementations

Hi @sansyrox

I hope my answers are clear, useful and correct :-)

[dave42w]

Hi @sansyrox
Please can we merge this. It is passing all the tests (with 5 speed regressions all initialisation related and so not affecting runtime performance). Once it is merged. I can work on the add_route stuff which should significantly improve the code base.

Thanks

[sansyrox]

One question @dave42w 👋

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

[dave42w]

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.

[sansyrox]

@dave42w , what do we mean by this comment?

[dave42w]

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

[sansyrox]

Hey @dave42w 👋

I've gone through the changes and all looks good. I just have a few comments and suggestions 😄

Merry shipmas btw 😉 🎄

[dave42w]

Hey @dave42w 👋

I've gone through the changes and all looks good. I just have a few comments and suggestions 😄

Merry shipmas btw 😉 🎄

Thanks. I've replied to all the comments. Hopefully, this is just a stepping stone to the bigger router refactor where the real benefits lie (much simpler functions without side effects; support for nesting subrouters; support for building routes/routers in more flexible ways)