-
-
Notifications
You must be signed in to change notification settings - Fork 130
Refactor handlers to be separate from core logic #170
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
Merged
Merged
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
7a4134e
Refactor handlers to be separate from core logic
four43 03c3b26
Cleanup code for black, flake8, pypy
four43 91727f8
Cleanup imports
four43 d9f759f
Move flake8 config to existing setup.cfg
four43 38022ef
Updated code style to adhere to 88 char limit
four43 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| [flake8] | ||
| max-line-length = 120 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| from .abstract_handler import AbstractHandler | ||
| from .aws_alb import AwsAlb | ||
| from .aws_api_gateway import AwsApiGateway | ||
| from .aws_cf_lambda_at_edge import AwsCfLambdaAtEdge | ||
| from .aws_http_gateway import AwsHttpGateway | ||
|
|
||
| __all__ = [ | ||
| "AbstractHandler", | ||
| "AwsAlb", | ||
| "AwsApiGateway", | ||
| "AwsCfLambdaAtEdge", | ||
| "AwsHttpGateway", | ||
| ] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| import base64 | ||
| from abc import ABCMeta, abstractmethod | ||
| from typing import Dict, Any, TYPE_CHECKING, Tuple, List | ||
|
|
||
| from mangum.response import Response | ||
| from mangum.scope import Scope | ||
|
|
||
| if TYPE_CHECKING: # pragma: no cover | ||
| from awslambdaric.lambda_context import LambdaContext | ||
|
|
||
|
|
||
| class AbstractHandler(metaclass=ABCMeta): | ||
| def __init__( | ||
| self, | ||
| trigger_event: Dict[str, Any], | ||
| trigger_context: "LambdaContext", | ||
| **kwargs: Dict[str, Any] | ||
| ): | ||
| self.trigger_event = trigger_event | ||
| self.trigger_context = trigger_context | ||
|
|
||
| @property | ||
| @abstractmethod | ||
| def scope(self) -> Scope: | ||
| """ | ||
| Parse an ASGI scope from the request event | ||
| """ | ||
|
|
||
| @property | ||
| @abstractmethod | ||
| def body(self) -> bytes: | ||
| """ | ||
| Get the raw body from the request event | ||
| """ | ||
|
|
||
| @abstractmethod | ||
| def transform_response(self, response: Response) -> Dict[str, Any]: | ||
| """ | ||
| After running our application, transform the response to the correct format for this handler | ||
| """ | ||
|
|
||
| @staticmethod | ||
| def from_trigger( | ||
| trigger_event: Dict[str, Any], | ||
| trigger_context: "LambdaContext", | ||
| **kwargs: Dict[str, Any] | ||
| ) -> "AbstractHandler": | ||
| """ | ||
| A factory method that determines which handler to use. All this code should probably stay in one place to make | ||
| sure we are able to uniquely find each handler correctly. | ||
| """ | ||
|
|
||
| # These should be ordered from most specific to least for best accuracy | ||
| if ( | ||
| "requestContext" in trigger_event | ||
| and "elb" in trigger_event["requestContext"] | ||
| ): | ||
| from . import AwsAlb | ||
|
|
||
| return AwsAlb(trigger_event, trigger_context, **kwargs) | ||
|
|
||
| if ( | ||
| "Records" in trigger_event | ||
| and len(trigger_event["Records"]) > 0 | ||
| and "cf" in trigger_event["Records"][0] | ||
| ): | ||
| from . import AwsCfLambdaAtEdge | ||
|
|
||
| return AwsCfLambdaAtEdge(trigger_event, trigger_context, **kwargs) | ||
|
|
||
| if "version" in trigger_event and "requestContext" in trigger_event: | ||
| from . import AwsHttpGateway | ||
|
|
||
| return AwsHttpGateway(trigger_event, trigger_context, **kwargs) | ||
|
|
||
| if "resource" in trigger_event: | ||
| from . import AwsApiGateway | ||
|
|
||
| return AwsApiGateway(trigger_event, trigger_context, **kwargs) # type: ignore | ||
|
|
||
| raise TypeError("Unable to determine handler from trigger event") | ||
|
|
||
| @staticmethod | ||
| def _handle_multi_value_headers( | ||
| response_headers: List[List[bytes]], | ||
| ) -> Tuple[Dict[str, str], Dict[str, List[str]]]: | ||
| headers: Dict[str, str] = {} | ||
| multi_value_headers: Dict[str, List[str]] = {} | ||
| for key, value in response_headers: | ||
| lower_key = key.decode().lower() | ||
| if lower_key in multi_value_headers: | ||
| multi_value_headers[lower_key].append(value.decode()) | ||
| elif lower_key in headers: | ||
| # Move existing to multi_value_headers and append current | ||
| multi_value_headers[lower_key] = [ | ||
| headers[lower_key], | ||
| value.decode(), | ||
| ] | ||
| del headers[lower_key] | ||
| else: | ||
| headers[lower_key] = value.decode() | ||
| return headers, multi_value_headers | ||
|
|
||
| @staticmethod | ||
| def _handle_base64_response_body( | ||
| body: bytes, headers: Dict[str, str] | ||
| ) -> Tuple[str, bool]: | ||
| """ | ||
| To ease debugging for our users, try and return strings where we can, otherwise to ensure maximum | ||
| compatibility with binary data, base64 encode it. | ||
| """ | ||
| is_base64_encoded = False | ||
| output_body = "" | ||
| if body != b"": | ||
| from ..adapter import DEFAULT_TEXT_MIME_TYPES | ||
|
|
||
| for text_mime_type in DEFAULT_TEXT_MIME_TYPES: | ||
| if text_mime_type in headers.get("content-type", ""): | ||
| try: | ||
| output_body = body.decode() | ||
| except UnicodeDecodeError: | ||
| # Can't decode it, base64 it and be done | ||
| output_body = base64.b64encode(body).decode() | ||
| is_base64_encoded = True | ||
| break | ||
| else: | ||
| # Not text, base64 encode | ||
| output_body = base64.b64encode(body).decode() | ||
| is_base64_encoded = True | ||
|
|
||
| return output_body, is_base64_encoded |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flake8 configuration is taken care of in
setup.cfg, so I don't think we need this. In any case, the max-line-length should be 88.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah there it is! I was wondering where you got 88 from? It seems old convention was 80 and modern convention is 120 to 150. Why 88?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it at 120 😈 I am curious though, I never know if there is an official source for stuff like this. Conventions and multiple files at once browsing is good. I can comfortably fit 2 120 files on my 2K monitor ¯_(ツ)_/¯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
88 is the default for python black: https://pypi.org/project/black/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the default for black as
dltacubementioned, and it is also the max line length for this project. I personally find beyond this uncomfortable to read/review, and I think when lines start getting too long it generally indicates something should be refactored (though this is the less important reason).Please change it back to 88.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really it's only longer URLs that span the 88 characters. It's brutal to cut everything down so small, lots of weird multiline strings result. I changed it, but it's uglier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I know it may seem nitpicky or pedantic, but it really does make it much easier for me to review contributions with this line length.