refactor(apigateway): Add BaseRouter and duplicate route check

[michaelbrewer]

Issue #, if available:

Updates related to #645

Description of changes:

• router: Add abstract class BaseRouter which includes the get,delete, post etc..
• router: Add check for duplicate routes and print a warning
• router: General code cleanup and reducing the code
• logger: Add behavior check for custom_formatter and correlation ids

Notes on layouts

This is an example project layout that we can use for the documentation section for this feature. In other cases it could be src/hello_world/handler.py (which hello_world is a descriptive name of the lambda, useful when there are multiple together in the same src folder.

.
├── Makefile
├── Pipfile
├── Pipfile.lock
├── README.md
├── events
│   └── wallet_event.json
├── src
│   ├── __init__.py
│   ├── requirements.txt
│   └── app
│       ├── __init__.py       # this file makes "app" a "Python package"
│       ├── main.py           # Main lambda handler
│       └── routers
│           ├── __init__.py
│           ├── items.py      # "items" submodule, e.g. import app.routers.items
│           └── users.py      # "users" submodule, e.g. import app.routers.users
├── template.yaml
└── tests
    ├── __init__.py
    ├── conftest.py
    └── unit
        ├── __init__.py
        └── test_app_main.py

Example source for the SAM template.yml, note the handler is app.main.lambda_handler and not just app.lambda_handler.

AWSTemplateFormatVersion: '2010-09-09'
Transform: AWS::Serverless-2016-10-31
Description: >
    app

Globals:
    Api:
        EndpointConfiguration: REGIONAL
        TracingEnabled: true
        Cors:
            # AllowOrigin: "'https://example.com'"
            AllowOrigin: "'*'"  # Dev only
            AllowHeaders: "'Content-Type,Authorization,X-Amz-Date'"
            MaxAge: "'300'"
        BinaryMediaTypes:
          - '*~1*'
    Function:
        Timeout: 10
        MemorySize: 512
        Runtime: python3.8
        Tracing: Active
        AutoPublishAlias: live
        DeploymentPreference:
            Type: Linear10PercentEvery1Minute 
        Environment:
            Variables:
                LOG_LEVEL: INFO
                POWERTOOLS_LOGGER_SAMPLE_RATE: 0.1
                POWERTOOLS_LOGGER_LOG_EVENT: true
                POWERTOOLS_METRICS_NAMESPACE: MyServerlessApplication
                POWERTOOLS_SERVICE_NAME: app

Resources:
    AppFunction:
        Type: AWS::Serverless::Function
        Properties:
            Handler: app.main.lambda_handler
            CodeUri: src
            Description: App function
            Events:
                ItemsPath:
                    Type: Api
                    Properties:
                        Path: /items
                        Method: GET
                UserPath:
                    Type: Api
                    Properties:
                        Path: /users/{name}
                        Method: GET
            Environment:
                Variables:
                    PARAM1: VALUE
            Tags:
                LambdaPowertools: python
Outputs:
    AppApigwURL:
      Description: "API Gateway endpoint URL for Prod environment for App Function"
      Value: !Sub "https://${ServerlessRestApi}.execute-api.${AWS::Region}.amazonaws.com/Prod/app"

    AppFunction:
      Description: "App Lambda Function ARN"
      Value: !GetAtt AppFunction.Arn

Source for the lamdba handler: src/app/main.py. Other naming examples would be handler.py, app.py, index.py, endpoint.py. Note how the imports for the routers are relative.

from aws_lambda_powertools import Logger, Tracer
from aws_lambda_powertools.event_handler.api_gateway import ApiGatewayResolver
from aws_lambda_powertools.logging import correlation_paths

from .routers import users, items

tracer = Tracer()
logger = Logger()
app = ApiGatewayResolver()
app.include_router(users.router)
app.include_router(items.router, prefix="/items")

@logger.inject_lambda_context(correlation_id_path=correlation_paths.API_GATEWAY_REST)
@tracer.capture_lambda_handler
def lambda_handler(event, context):
    return app.resolve(event, context)

Source for an example router: src/app/routers/items.py

from aws_lambda_powertools import Logger, Tracer
from aws_lambda_powertools.event_handler.api_gateway import Router

tracer = Tracer()
logger = Logger(child=True)
router = Router()

@router.get('/hello')
@tracer.capture_method
def get_hello():
    return {
        "message": ['/hello', 'GET'],
        "query_string": router.current_event.query_string_parameters,
    }

@router.route('/world', ('GET','POST'))
@tracer.capture_method
def multi_world():
    return {
        "message": ['/world', 'GET/POST'],
        "query_string": router.current_event.query_string_parameters,
    }

Source for how the unit testing could work using pytest: src/tests/unit/test_app_main.py

import json

from src.app import main


def test_lambda_handler(apigw_event, lambda_context):
    ret = main.lambda_handler(apigw_event, lambda_context)
    expected = json.dumps({"message": "hello universe"}, separators=(",", ":"))

    assert ret["statusCode"] == 200
    assert ret["body"] == expected

Checklist

• Meet tenets criteria
• Update tests
• Update docs
• PR title follows conventional commit semantics

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[codecov-commenter]

Codecov Report

Merging #757 (4e9a46f) into develop (905cbad) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #757      +/-   ##
===========================================
+ Coverage    99.93%   99.97%   +0.04%     
===========================================
  Files          116      116              
  Lines         4953     4939      -14     
  Branches       276      275       -1     
===========================================
- Hits          4950     4938      -12     
+ Misses           1        0       -1     
+ Partials         2        1       -1     

Impacted Files	Coverage Δ
...mbda_powertools/utilities/validation/exceptions.py	100.00% <ø> (ø)
aws_lambda_powertools/event_handler/api_gateway.py	100.00% <100.00%> (ø)
aws_lambda_powertools/logging/logger.py	100.00% <0.00%> (+1.56%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 905cbad...4e9a46f. Read the comment docs.

[michaelbrewer]

@BVMiko @heitorlessa - added some of the feedback on #645 . Next i can look at some of the documentation work

[heitorlessa]

That's a great cleanup, two thoughts:

1/ Can we make BaseRouter an actual class to merge the routing logic between both ApiGatewayResolver and Router?

e.g., Router registers & decorates a route, then re-registers & re-decorates again, and do extra checks on cors and such, then finally gets called by the parent. We could do it later too, as we can launch as-is, have a maintenance issue, and tackle them later as a chore item.

2/ Let's support multiple methods for a given route in the parent so it's aligned and makes it easier to merge logic in the Router later too.

Minors as GH is giving me a hard time annotating in the right LOC:

• Router.api -> Router.routes
• method: Union[str, Tuple[str], List[str]], -> method: Union[str, Liststr]],
  • I'd typically use Sequence[str] or Iterable[str] but it's been deprecated in 3.9; a list of strings suffices
• Docstrings for include_router as I'll surely forget the logic a few weeks ahead
• include_router has the prefix logic we accidentally had a bug on "/" before, perhaps 1/ can help in case we find another one in the future.

Lemme know your thoughts on 1, as I think we had chatted about 2 already. Happy with everything else

[michaelbrewer]

@heitorlessa - I am going to switch to the docs next. This can merge if we are good at this point.

Or I can contribute to add docs to this PR too.

[heitorlessa]

I’ll merge tomorrow or Friday - let’s start the docs in a separate PR as there will be plenty of conversations

…

On Wed, 13 Oct 2021 at 16:45, Michael Brewer ***@***.***> wrote: @heitorlessa <https://github.com/heitorlessa> - I am going to switch to the docs next. This can merge if we are good at this point. Or I can contribute to add docs to this PR too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#757 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZPQBGRSNL73GI56WNYM2DUGWLSHANCNFSM5FWNDPVA> .

[michaelbrewer]

@heitorlessa - i will start working on the docs PR next

[heitorlessa]

I find this clever yet puzzling that I can't shake the feeling this is wrong but it's the quirk of a dynamic language allowing such a thing.

https://github.com/awslabs/aws-lambda-powertools-python/pull/757/files#diff-bdb4b43087f89fdf398381f3d88048f6182562ca67e8907259d5c56d658465c1R497-R498

Every other solution I could think of like property setter, subclass hook to confirm they have set instance variables... would eventually become a N+1 problem as we'd need to keep track of router instances, loop + inject event and lambda context.

Unless we go back to the idea of having a global config (here be dragons...)... I'll have a think for the next hour before merging just in case something comes up. We can always fix it later without impacting customers.

[heitorlessa]

@cakepietoast had a good idea on using a RequestState class and a route_state pointer so we can separate the concerns while not having a N+1 problem per se.

Ignore the dataclass as this wouldn't work in 3.6, we can do at the constructor level w/ a normal class

class BaseRouter:
    def __init__(self):
        self.router_state = RouterState()

    @property
    def current_event(self):
        return self.router_state.current_event

    @property
    def lambda_context(self):
        return self.router_state.lambda_context


@dataclass
class RouterState:
    current_event: str = None
    lambda_context: str = None


class ApiGatewayResolver(BaseRouter):
    def resolve(self, event, context) -> Dict[str, Any]:
        self.router_state.current_event = event
        self.router_state.lambda_context = context

    def include_router(self, router):
        router.router_state = self.router_state


class Router(BaseRouter):
    pass


router1 = Router()
router2 = Router()  # just pretend this is in another module
app = ApiGatewayResolver()
app.include_router(router1)
app.include_router(router2)

[heitorlessa]

I'll merge as-is, refactor in a separate PR so you don't have to. Thanks again for making it leaner @michaelbrewer !