feat: add support nested routes prefix and multiple auth handler #889

yomaaf · 2024-07-10T17:55:50Z

Description

This PR fixes #865 #708

This PR adds the ability to nested route and ability multiple auth handler.
The example already added to integration tests

vercel · 2024-07-10T17:55:53Z

@yomaaf is attempting to deploy a commit to the sparckles Team on Vercel.

A member of the Team first needs to authorize it.

yomaaf · 2024-07-11T04:19:41Z

Hey @sansyrox please check this out. I made some adjustment to enable nested routes

sansyrox · 2024-07-11T14:16:02Z

Hey @yomaaf 👋

Thank you for the PR 😄 I will have a look soon. @VishnuSanal , could you have an initial look?

VishnuSanal

lgtm

yomaaf · 2024-07-12T10:19:02Z

Please let me know if this code need some adjustment @sansyrox @VishnuSanal, because I'm also working on #708 issue based on this code. I found out your framework and want to use it on production. But need this nested function and multiple authentication. I read the comment on #708 issue and have different approach. Will PR soon.

sansyrox · 2024-07-12T19:35:00Z

Hey @yomaaf 👋

Thanks for the PR. I am with limited access till Sunday. I will do a thorough review by Sunday 😄

sansyrox · 2024-07-12T19:36:48Z

robyn/__init__.py

+    def include_router(self, router, added_routes=None):
        """
        The method to include the routes from another router

        :param router Robyn: the router object to include the routes from


why do we need an added_route param here? Could you please the usage in the docstring?

Please also add the typing here.

I'm sorry I want to comment in this conversation but reply on another conversation instead. My mistake.
#889 (comment)

sansyrox · 2024-07-12T19:38:34Z

robyn/__init__.py

+        self.sub_router = []
+        self.temp_routes = []  # Temporary storage for routes


Could you explain the idea behind these memebers?

Hi I'm sorry there is some issue before when register route. I already fix this. Please check again. I also add some feature to handle multiple auth handler like I said in comment before.

I also add some pytest to ensure the function is working

The idea is we save all the nested subrouter and route before the main robyn call include router. Like what I explain in this comment #889 (comment)

robyn/__init__.py

sansyrox · 2024-07-12T19:39:45Z

robyn/__init__.py

+    def get(self, endpoint: str, const: bool = False, auth_required: bool = False):
+        def inner(handler):
+            self.add_temp_route(HttpMethod.GET, endpoint, handler, const, auth_required)


Also, why did you have to refactor the original implementation here?

^^ I am trying to understand you thoughts here.

The original implementation of the get, post, etc., decorators calls super(), which registers the route when the decorator is used. However, when nested subrouters are used, the decorator has already been called, so the route retains the old prefix instead of adopting the nested prefix.

The original implementation of the get, post, etc., decorators calls super(), which registers the route when the decorator is used. However, when nested subrouters are used, the decorator has already been called, so the route retains the old prefix instead of adopting the nested prefix.

Could you please add it in comment/docstring in the codebase?

Of course. I'll add it.

sansyrox · 2024-07-12T19:40:31Z

Hey @yomaaf ,

I have a few questions.

* add multiple auth handler * add basic authorization getter * add route params auth handler name

codspeed-hq · 2024-07-13T06:47:58Z

CodSpeed Performance Report

Merging #889 will not alter performance

_{Comparing yomaaf:feature/nested-route (4d2e77d) with main (5e7bcbe)}

Summary

✅ 110 untouched benchmarks

🆕 20 new benchmarks

Benchmarks breakdown

	Benchmark	`main`	`yomaaf:feature/nested-route`	Change
🆕	`test_invalid_authentication_header_basic[async]`	N/A	4.7 ms	N/A
🆕	`test_invalid_authentication_header_basic[sync]`	N/A	4.7 ms	N/A
🆕	`test_invalid_authentication_header_bearer_2[async]`	N/A	4.7 ms	N/A
🆕	`test_invalid_authentication_header_bearer_2[sync]`	N/A	4.7 ms	N/A
🆕	`test_invalid_authentication_no_token_basic[async]`	N/A	4.7 ms	N/A
🆕	`test_invalid_authentication_no_token_basic[sync]`	N/A	4.7 ms	N/A
🆕	`test_invalid_authentication_no_token_bearer_2[async]`	N/A	4.7 ms	N/A
🆕	`test_invalid_authentication_no_token_bearer_2[sync]`	N/A	4.7 ms	N/A
🆕	`test_invalid_authentication_token_basic[async]`	N/A	4.7 ms	N/A
🆕	`test_invalid_authentication_token_basic[sync]`	N/A	4.7 ms	N/A
🆕	`test_invalid_authentication_token_bearer_2[async]`	N/A	4.7 ms	N/A
🆕	`test_invalid_authentication_token_bearer_2[sync]`	N/A	4.7 ms	N/A
🆕	`test_nested_router_invalid_authentication_header`	N/A	4.9 ms	N/A
🆕	`test_nested_router_invalid_authentication_no_token`	N/A	4.9 ms	N/A
🆕	`test_nested_router_invalid_authentication_token`	N/A	4.9 ms	N/A
🆕	`test_nested_router_valid_authentication`	N/A	5 ms	N/A
🆕	`test_valid_authentication_basic[async]`	N/A	4.8 ms	N/A
🆕	`test_valid_authentication_basic[sync]`	N/A	4.8 ms	N/A
🆕	`test_valid_authentication_bearer_2[async]`	N/A	4.8 ms	N/A
🆕	`test_valid_authentication_bearer_2[sync]`	N/A	4.8 ms	N/A

sansyrox · 2024-07-13T12:44:52Z

Hey @yomaaf 👋

Thanks for the PR but this PR is doing a lot of things 😅

Could you split the PR in two parts

Add support for nested routes in one
Add multiple auth handlers in the other

I am recommending the above as it will take a lot of time to get it landed 😄

yomaaf · 2024-07-14T03:06:28Z

Hi, @sansyrox.

I will do that. I'll separate the PR, but maybe not today. Will do in a few days because I need to catch up on another project. In the meantime, I'll create my own branch in my repository and use pip install from it till the PRs land. I'm going to push it as soon as possible. Thanks

feat: add support nested routes prefix

94a89be

yomaaf marked this pull request as draft July 11, 2024 04:15

yomaaf marked this pull request as ready for review July 11, 2024 04:15

fix: nested route when decoration called before include_router

f3c7339

fix: call include router from different file

257948a

VishnuSanal approved these changes Jul 12, 2024

View reviewed changes

sansyrox reviewed Jul 12, 2024

View reviewed changes

robyn/__init__.py Show resolved Hide resolved

sansyrox reviewed Jul 12, 2024

View reviewed changes

feat: some new feature

323bca1

* add multiple auth handler * add basic authorization getter * add route params auth handler name

yomaaf changed the title ~~feat: add support nested routes prefix~~ feat: add support nested routes prefix and multiple auth handler Jul 13, 2024

feat: add test case for auth handler subrouter

4d2e77d

yomaaf closed this Jul 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support nested routes prefix and multiple auth handler #889

feat: add support nested routes prefix and multiple auth handler #889

yomaaf commented Jul 10, 2024 •

edited

Loading

vercel bot commented Jul 10, 2024

yomaaf commented Jul 11, 2024

sansyrox commented Jul 11, 2024

VishnuSanal left a comment

yomaaf commented Jul 12, 2024

sansyrox commented Jul 12, 2024

sansyrox Jul 12, 2024

sansyrox Jul 12, 2024

yomaaf Jul 13, 2024

sansyrox Jul 12, 2024

yomaaf Jul 13, 2024

yomaaf Jul 13, 2024 •

edited

Loading

yomaaf Jul 13, 2024

sansyrox Jul 12, 2024

yomaaf Jul 13, 2024

sansyrox Jul 13, 2024

yomaaf Jul 14, 2024

sansyrox commented Jul 12, 2024

codspeed-hq bot commented Jul 13, 2024 •

edited

Loading

sansyrox commented Jul 13, 2024

yomaaf commented Jul 14, 2024

		self.sub_router = []
		self.temp_routes = [] # Temporary storage for routes

feat: add support nested routes prefix and multiple auth handler #889

feat: add support nested routes prefix and multiple auth handler #889

Conversation

yomaaf commented Jul 10, 2024 • edited Loading

vercel bot commented Jul 10, 2024

yomaaf commented Jul 11, 2024

sansyrox commented Jul 11, 2024

VishnuSanal left a comment

Choose a reason for hiding this comment

yomaaf commented Jul 12, 2024

sansyrox commented Jul 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yomaaf Jul 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sansyrox commented Jul 12, 2024

codspeed-hq bot commented Jul 13, 2024 • edited Loading

Merging #889 will not alter performance

Summary

Benchmarks breakdown

sansyrox commented Jul 13, 2024

yomaaf commented Jul 14, 2024

yomaaf commented Jul 10, 2024 •

edited

Loading

yomaaf Jul 13, 2024 •

edited

Loading

codspeed-hq bot commented Jul 13, 2024 •

edited

Loading