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.

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

Feature request: introduce ALBRouter, etc. to set the type of current_event in the REST API Router #1781

Closed
1 of 2 tasks
kmkhr opened this issue Dec 13, 2022 · 11 comments · Fixed by #1824
Closed
1 of 2 tasks
Assignees
Labels
typing Static typing definition related issues (mypy, pyright, etc.)

Comments

@kmkhr
Copy link
Contributor

kmkhr commented Dec 13, 2022

Use case

In #761, APIGatewayRestResolver, APIGatewayHttpResolver, ALBResolver, and LambdaFunctionUrlResolver were created to access all properties from current_event.
I would like to access all properties from current_event in router as well.

Solution/User Experience

router = ALBRouter()

@router.get("/todos")
def get_todos():
    router.current_event.request_context.elb_target_group_arn

Alternative solutions

No response

Acknowledgment

@kmkhr kmkhr added feature-request feature request triage Pending triage from maintainers labels Dec 13, 2022
@heitorlessa
Copy link
Contributor

hey @kt-hr, did you run into any issue when accessing current_event from a Router instance?

It should work, please let us know otherwise with a snippet we can reproduce - Test, Docs Example, API reference

image

@heitorlessa heitorlessa added need-more-information Pending information to continue and removed triage Pending triage from maintainers labels Dec 13, 2022
@kmkhr
Copy link
Contributor Author

kmkhr commented Dec 14, 2022

hi @heitorlessa , thank you for your confirmation. Sorry, let me correct the content.

I mistakenly thought I could not access properties because the IDE was showing an error.

The type of current_event is BaseProxyEvent, so when I want to access properties such as elb_target_group_arn, the IDE completion does not work.
Although the completion will work by informing the IDE of the type with cast, we think it would be easier to understand if it were ALBRouter, etc.

@kmkhr kmkhr changed the title Feature request: allow REST API router to access all properties Feature request: introduce ALBRouter, etc. to set the type of current_event in the REST API Router Dec 14, 2022
@heitorlessa heitorlessa added typing Static typing definition related issues (mypy, pyright, etc.) and removed feature-request feature request labels Dec 15, 2022
@heitorlessa
Copy link
Contributor

Ahhh that makes more sense now - thanks for clarifying and updating the title. Let's do it - it should be fairly trivial as you can simply subclass, override the type like we did for each explicit resolver class (ALBResolver), and call out in the docs that each corresponding resolver has a Router for accurate static typing.

Feel free to create a PR and we can review in January after we're back from winter holidays.

Thank you!

@rubenfonseca rubenfonseca self-assigned this Jan 6, 2023
@rubenfonseca
Copy link
Contributor

Hi @kt-hr just so I can understand this problem better, why are you not using ALBResolver in your code in the first place? Why are you using the bare Router when you know you're processing ALB events?

@kmkhr
Copy link
Contributor Author

kmkhr commented Jan 10, 2023

Hi, @rubenfonseca thank you for checking.

In my use case, I want to access properties such as router.current_event.request_context.authorizer.principal_id when I split it into separate modules as in the Split routes with Router example.
Cast as typed_event = cast(APIGatewayProxyEvent, router.current_event) to enable IDE completion.

_router in APIGatewayRestResolver APIGatewayHttpResolver ALBResolver is List[Route], but _router in Router is Dict[tuple, Callable], so I think they are incompatible.

@rubenfonseca rubenfonseca removed the need-more-information Pending information to continue label Jan 10, 2023
@rubenfonseca
Copy link
Contributor

Great, I now get the problem! Will work on this today :)

@rubenfonseca
Copy link
Contributor

@kt-hr I opened a PR to fix the UX above, please feel free to leave your comment before we merge!

@kmkhr
Copy link
Contributor Author

kmkhr commented Jan 10, 2023

@rubenfonseca thank you very much. Very helpful.

I have nothing to add to that.

@github-actions
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

@github-actions github-actions bot added the pending-release Fix or implementation already in dev waiting to be released label Jan 11, 2023
@rubenfonseca rubenfonseca reopened this Jan 12, 2023
@github-actions
Copy link
Contributor

This is now released under 2.6.0 version!

@github-actions github-actions bot removed the pending-release Fix or implementation already in dev waiting to be released label Jan 12, 2023
@github-actions github-actions bot added the pending-release Fix or implementation already in dev waiting to be released label Feb 9, 2023
@github-actions
Copy link
Contributor

This is now released under 2.8.0 version!

@github-actions github-actions bot removed the pending-release Fix or implementation already in dev waiting to be released label Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typing Static typing definition related issues (mypy, pyright, etc.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants