Tech debt: Refactor api_gateway.py file by splitting its code into multiple files.

leandrodamascena commented 6 months ago

Why is this needed?

Refactor api_gateway.py by splitting its "monolithic" structure with many classes and methods into multiple files for better code organization and maintainability.

Which area does this relate to?

Event Handler - REST API

Suggestion

No response

Acknowledgment

[X] This request meets Powertools for AWS Lambda (Python) Tenets
[ ] Should this be considered in other Powertools for AWS Lambda languages? i.e. Java, TypeScript, and .NET

leandrodamascena commented 6 months ago

Hello @heitorlessa, before start working on this issue, I would like to hear your opinion here. The api_gateway.py file is quite complicated to understand now because we have added a lot of things like Middleware, OpenAPI and others, so my suggestion is to split this file into multiple files and keep the export at a high level for the existing items. I'm not sure about some circular dependencies problems, but I think we can make it work.

(aws-lambda-powertools-py3.10) ➜  event_handler git:(split-api-gw-code) ✗ tree
.
├── __init__.py
├── api_gateway.py --------> **Remove this file??**  
├── appsync.py
├── constants.py --------> **Move ProxyEventType and other constants here**  
├── content_types.py
├── cors.py ------> **Move CORSConfig here**
├── exceptions.py
├── middlewares
│   ├── __init__.py
│   ├── base.py --------> **Move MiddlewareFrame here**  
│   ├── openapi_validation.py
│   └── schema_validation.py
├── openapi
│   ├── __init__.py
│   ├── compat.py
│   ├── constants.py
│   ├── dependant.py
│   ├── encoders.py
│   ├── exceptions.py
│   ├── models.py
│   ├── params.py
│   ├── swagger_ui
│   │   ├── __init__.py
│   │   ├── html.py
│   │   ├── oauth2.py
│   │   ├── swagger-ui-bundle.min.js
│   │   └── swagger-ui.min.css
│   └── types.py
├── resolvers
│   ├── api_gateway_http.py ------> **Move APIGatewayHttpResolver here**
│   ├── api_gateway_rest.py ------> **Move APIGatewayRestResolver here**
│   ├── application_load_balancer.py ------> **Move ALBResolver here**
│   ├── base.py ------> **Move ApiGatewayResolver here**
│   ├── bedrock_agent.py ------> **Move BedrockAgentResolver here**
│   ├── lambda_function_url.py ------> **Move LambdaFunctionUrlResolver here**
│   └── vpc_lattice.py ------> **Move VPCLatticeV2Resolver and VPCLatticeResolver here**
├── response.py ------> **Move ResponseBuilder and Response here**
├── router.py ------> **Move Router and Route here**
├── types.py
└── util.py

The __init__.py file could be something like this:

from aws_lambda_powertools.event_handler.appsync import AppSyncResolver
from aws_lambda_powertools.event_handler.resolvers.api_gateway_http import APIGatewayHttpResolver
from aws_lambda_powertools.event_handler.resolvers.api_gateway_rest import APIGatewayRestResolver
from aws_lambda_powertools.event_handler.resolvers.application_load_balancer import ALBResolver
from aws_lambda_powertools.event_handler.resolvers.bedrock_agent import BedrockAgentResolver
from aws_lambda_powertools.event_handler.resolvers.lambda_function_url import (
    LambdaFunctionUrlResolver,
)
from aws_lambda_powertools.event_handler.resolvers.vpc_lattice import VPCLatticeResolver, VPCLatticeV2Resolver

__all__ = [
    "AppSyncResolver",
    "APIGatewayRestResolver",
    "APIGatewayHttpResolver",
    "ALBResolver",
    "ApiGatewayResolver",
    "BedrockAgentResolver",
    "CORSConfig",
    "LambdaFunctionUrlResolver",
    "Response",
    "VPCLatticeResolver",
    "VPCLatticeV2Resolver",
]

What do you think about?

heitorlessa commented 6 months ago

Whenever you have time, could you share the list of imports that will break for customers using absolute imports with these?

It’ll be useful for import hooks (prevent breaking), and to have a better idea whether we go one step further towards modularisation

On Mon, 13 May 2024 at 22:46, Leandro Damascena @.***> wrote:

Hello @heitorlessa https://github.com/heitorlessa, before start working on this issue, I would like to hear your opinion here. The api_gateway.py file is quite complicated to understand now because we have added a lot of things like Middleware, OpenAPI and others, so my suggestion is to split this file into multiple files and keep the export at a high level for the existing items. I'm not sure about some circular dependencies problems, but I think we can make it work.

(aws-lambda-powertools-py3.10) ➜ event_handler git:(split-api-gw-code) ✗ tree. ├── init.py ├── api_gateway.py --------> Remove this file?? ├── appsync.py ├── constants.py --------> Move ProxyEventType and other constants here ├── content_types.py ├── cors.py ------> Move CORSConfig here ├── exceptions.py ├── middlewares │ ├── init.py │ ├── base.py --------> Move MiddlewareFrame here │ ├── openapi_validation.py │ └── schema_validation.py ├── openapi │ ├── init.py │ ├── compat.py │ ├── constants.py │ ├── dependant.py │ ├── encoders.py │ ├── exceptions.py │ ├── models.py │ ├── params.py │ ├── swagger_ui │ │ ├── init.py │ │ ├── html.py │ │ ├── oauth2.py │ │ ├── swagger-ui-bundle.min.js │ │ └── swagger-ui.min.css │ └── types.py ├── resolvers │ ├── api_gateway_http.py ------> Move APIGatewayHttpResolver here │ ├── api_gateway_rest.py ------> Move APIGatewayRestResolver here │ ├── application_load_balancer.py ------> Move ALBResolver here │ ├── base.py ------> Move ApiGatewayResolver here │ ├── bedrock_agent.py ------> Move BedrockAgentResolver here │ ├── lambda_function_url.py ------> Move LambdaFunctionUrlResolver here │ └── vpc_lattice.py ------> Move VPCLatticeV2Resolver and VPCLatticeResolver here ├── response.py ------> Move ResponseBuilder and Response here ├── router.py ------> Move Router and Route here ├── types.py └── util.py

The init.py file could be something like this:

from aws_lambda_powertools.event_handler.appsync import AppSyncResolverfrom aws_lambda_powertools.event_handler.resolvers.api_gateway_http import APIGatewayHttpResolverfrom aws_lambda_powertools.event_handler.resolvers.api_gateway_rest import APIGatewayRestResolverfrom aws_lambda_powertools.event_handler.resolvers.application_load_balancer import ALBResolverfrom aws_lambda_powertools.event_handler.resolvers.bedrock_agent import BedrockAgentResolverfrom aws_lambda_powertools.event_handler.resolvers.lambda_function_url import ( LambdaFunctionUrlResolver, )from aws_lambda_powertools.event_handler.resolvers.vpc_lattice import VPCLatticeResolver, VPCLatticeV2Resolver all = [ "AppSyncResolver", "APIGatewayRestResolver", "APIGatewayHttpResolver", "ALBResolver", "ApiGatewayResolver", "BedrockAgentResolver", "CORSConfig", "LambdaFunctionUrlResolver", "Response", "VPCLatticeResolver", "VPCLatticeV2Resolver", ]

What do you think about?

— Reply to this email directly, view it on GitHub https://github.com/aws-powertools/powertools-lambda-python/issues/4194#issuecomment-2108855998, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZPQBARNFAU6Z27YF2MTNTZCEYB3AVCNFSM6AAAAABGWVE24OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBYHA2TKOJZHA . You are receiving this because you were mentioned.Message ID: @.*** com>

leandrodamascena commented 6 months ago

Whenever you have time, could you share the list of imports that will break for customers using absolute imports with these? It’ll be useful for import hooks (prevent breaking), and to have a better idea whether we go one step further towards modularisation

The short answer for this is: everything importing from api_gateway will break.

from aws_lambda_powertools.event_handler.api_gateway import APIGatewayRestResolver
from aws_lambda_powertools.event_handler.api_gateway import Router
from aws_lambda_powertools.event_handler.api_gateway import .......

We can think about using import hooks to avoid any break in runtime. I just don't know if this break IDE hints/autocompletion.

heitorlessa commented 5 months ago

Assuming import hooks works, I'd be fine with the change :) I'll have more thoughts as the PR gets implemented, for now, what's top of mind for me is:

appsync.py should be moved to resolvers, possibly broken too as there's some notable pieces that shouldn't be a single file
we're missing a responses subpackage to compartmentalise content_types.py, response.py, and future-proof anything that can reduce boilerplate in building known response formats

In the future, we might expand to modularization, where most of this will be under a core, and resolvers will be separate subpackages entirely. That's one of the reasons import hooks are crucial - both for backwards compatibility and future-proof a new era of highly specialized Event Handlers e.g., CWLogs, EventBridge, etc.

leandrodamascena commented 3 months ago

I'm closing this issue because it doesn't make sense to implement it now. Moving files around without any optimization, maintenance, or refactoring gains will only introduce unnecessary breaking changes to our customers. Working with import hooks is kind complicated now without any real gains here.

It's better to redo this module completely in v4 than to just move files around now.

github-actions[bot] commented 3 months ago

⚠️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.

aws-powertools / powertools-lambda-python