Refactor handlers to be separate from core logic by four43 · Pull Request #170 · Kludex/mangum

four43 · 2021-03-20T13:59:58Z

Okay here is a BIG one. This refactors out all of the logic for each event source into their own classes. This helps encapsulate all the messy logic for each event source into one place. It also makes it easier to add new event sources as we choose. I namespaced the handlers with "AWS" with thoughts that this could actually apply to other serverless implementations over on Azure, GCP, etc. if needed.

I wanted to get this in front of you sooner rather than later so you could take a look. It needs a couple more lines of coverage and I'm going to go back and look through some of the issues to make sure I have them covered in tests and they're still working (regression is no good)

It might be easier to look at the repo at my commit, not just the changes.

Let me know what you think, big changes like this are scary but it feels a lot cleaner.

four43 · 2021-03-20T14:53:56Z

(I'm trying to keep this squashed as best I can, don't mind the force-pushes to my branch)
And mypy ಠ_ಠ picky picky mypy lol

dltacube · 2021-03-20T15:37:01Z

This looks incredibly organized! Happy to put it through the wringer once it's ready.

four43 · 2021-03-20T15:51:44Z

Yay they passed!

Please give it a good look over @dltacube! I'm sure there are some weird fringe cases for each of these handlers that isn't spelled out super well in the AWS docs. It would be good to get some E2E tests going for stuff like this, some more real world examples.

Let me know what you think, thanks for taking a look.

koxudaxi · 2021-03-20T16:03:13Z

@four43
Thank you for creating this issue!!
First, I have a question.
Does your PR treat AWS WebSocket?
#132

My question is related to @jordaneremieff 's comment #163 (comment)

I namespaced the handlers with "AWS" with thoughts that this could actually apply to other serverless implementations over on Azure, GCP, etc. if needed.

I think it's a good first step.
However, We should confirm that the design can support Azure/GCP events.
@jordaneremieff said these requests are not the same structure as AWS. #163 (comment)

four43 · 2021-03-20T16:06:21Z

I don't think it will handle web sockets right now. I'm very curious about that though. I didn't have an environment set up to test real world concrete examples of it.

I've only used AWS to any real extent. It would be cool to see those other providers. I'd imagine they have a similar event handling pattern, probably different event and context formats however.

Since both of those were open issues, I don't see those as blockers.

Thoughts?

Thanks for your feedback!

koxudaxi · 2021-03-20T16:25:35Z

I use AWS WebSockets with the old version Mangum.
Currently, the feature is dropped.
I hope the feature is in Mangum or Other libraries.

Since both of those were open issues, I don't see those as blockers.

I think these features are not blockers in your PR directly.
But, We must decide whether Mangum should support these features or out of scope.

@jordaneremieff
What did you think about it?
The theme is very important for the community
We should decide 🤔

dltacube · 2021-03-20T16:34:00Z

Does this PR have any impact on bringing back websocket support?

jordaneremieff · 2021-03-20T17:07:41Z

I had a skim, this looks really good. I’ll give it a proper review tomorrow. Great work!

Also, I’ll follow up more about WebSockets/other providers after review, but I think this shouldn’t prevent either from being implemented later. The entrypoint for WebSockets probably could fit well as a handler here since it follows a similar event structure, but other provider events would be trickier. That said, supporting other providers is dropped from the roadmap (at least for now/without more investigation), but supporting WebSockets is something that has just been stalled.

jordaneremieff

Looks good! I tested against an HTTP API Gateway deployment without issues. Just a few comments, but otherwise I don't see anything that would prevent a merge.

jordaneremieff · 2021-03-21T04:52:45Z

@@ -0,0 +1,2 @@
+[flake8]
+max-line-length = 120


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.

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?

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 ¯_(ツ)_/¯

88 is the default for python black: https://pypi.org/project/black/

It is the default for black as dltacube mentioned, 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.

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.

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.

jordaneremieff · 2021-03-21T04:59:20Z

+
+
+@dataclass
+class Scope:


How about making this Event instead of Scope and ScopeDict renamed back to Scope?

Then as_dict would become get_scope (or maybe a new scope property?), and it could be accessed like handler.event.scope or handler.event.get_scope().

Alternatively, maybe Request instead of Event makes more sense here.

I'd like to leave this one scope solely because that's what the ASGI folks call it: https://asgi.readthedocs.io/en/latest/specs/www.html#http-connection-scope

Similarly Event is what AWS calls all of these different lambda triggers. Keeping with existing conventions.

I'll move that as_dict() call till later though, might keep things more structured until the very last possible moment

This should be changed from Scope to something else because it actually encompasses more than just the Scope in the class definition - the ScopeDict itself is would be more consistent with the other types (e.g. we don't have ReceiveCallback). So rather than Event or Scope it seems Request would be most appropriate because it contains both the lambda event information and the ASGI request information that ultimately becomes the scope later.

jordaneremieff · 2021-03-21T05:02:10Z

+
+
+@dataclass
+class Response:


Maybe a new datastructures.py file that combines the classes in response.py and scope.py in a single file.

I re-worked the init files a bit to cleanup imports

four43 · 2021-03-21T22:06:00Z

There we go I hit some of those cleanup point @jordaneremieff - thanks for looking it over.

jordaneremieff · 2021-03-21T23:23:05Z

@@ -1 +1,5 @@
+from .response import Response


Please move Response and Scope to a single file. The reason for doing so isn't for the imports, rather it is easier to maintain a single file with related classes like this.

Since I was able to clean up adapter so much, they might fit nicely in adapter... but now circular references.

four43 · 2021-03-22T00:04:13Z

Alright that should do it. That's gonna be enough bike shedding for me though. Feel free to merge and fiddle with more if you want. I think it's doing the gist of what I wanted it to do.

jordaneremieff · 2021-03-22T00:20:43Z


 @app.route("/")
-def endpoint(request: framework.requests.Request) -> dict:
+def endpoint(request: Request) -> dict:


This would be unchanged because the request in the framework is different than the request used in Mangum itself, though I can change this later after merging.

four43 · 2021-03-22T00:25:04Z

Lol hey no worries man. It’s your project. You get to pick the line length!

On Sun, Mar 21, 2021 at 7:23 PM Jordan Eremieff ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In .flake8 <#170 (comment)>: > @@ -0,0 +1,2 @@ +[flake8] +max-line-length = 120 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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#170 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD6HDVWGOB42M6SSBQXETTTE2EXJANCNFSM4ZQOVWMQ> .

-- -Seth

jordaneremieff · 2021-03-22T00:25:12Z

Nicely done @four43, I'll merge this is now. :)

four43 · 2021-03-22T00:26:44Z

Thanks man! Onto the next fun stuff. Sounds like web sockets. I’ll see if I can get a setup for that in Terraform. I’m curious how that works with lambdas that don’t run all the time.

On Sun, Mar 21, 2021 at 7:25 PM Jordan Eremieff ***@***.***> wrote: Nicely done @four43 <https://github.com/four43>, I'll merge this is now. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#170 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD6HDUXKBWZGWBEXRMGJN3TE2E7JANCNFSM4ZQOVWMQ> .

-- -Seth

tuukkamustonen · 2021-03-22T14:12:35Z

@four43 Does this drop log_level parameter from Mangum constructor? I don't see it used anymore, but the docstring is still there? Should the docs reflect that?

four43 · 2021-03-22T14:41:30Z

@tuukkamustonen - YES! Sorry about that. You'd just use the normal way you configure python loggers.

logging.getLogger('mangum').setLevel(...)

It seemed redundant to provide that functionality in this library. Good call on the docs. We should update those.

tuukkamustonen · 2021-03-22T15:29:06Z

It seemed redundant to provide that functionality in this library.

I agree about that. See also #156 for some discussion about the topic.

tuukkamustonen · 2021-03-22T15:31:38Z

@four43 Something else that might bug me is that kwargs are passed around as non-validated dict. When user passes a key, that's not detected or supported by Mangum / chosen handler, wouldn't better approach be to raise an error? kwargs are just dropped by default now (https://github.com/jordaneremieff/mangum/blob/main/mangum/handlers/abstract_handler.py#L17).

* Refactor handlers to be separate from core logic * Cleanup code for black, flake8, pypy * Cleanup imports * Move flake8 config to existing setup.cfg * Updated code style to adhere to 88 char limit

Necessary after Kludex#170 added a `tests/__init__.py` file that lead `find_packages` to treat `tests`as a package dir, which will clobber `mangum` users' ability to import from a repo-local `tests` directory.

Necessary after #170 added a `tests/__init__.py` file that lead `find_packages` to treat `tests`as a package dir, which will clobber `mangum` users' ability to import from a repo-local `tests` directory.

* Refactor handlers to be separate from core logic * Cleanup code for black, flake8, pypy * Cleanup imports * Move flake8 config to existing setup.cfg * Updated code style to adhere to 88 char limit

Necessary after Kludex#170 added a `tests/__init__.py` file that lead `find_packages` to treat `tests`as a package dir, which will clobber `mangum` users' ability to import from a repo-local `tests` directory.

four43 force-pushed the refactor-split-handlers branch from e549e4b to 9402d60 Compare March 20, 2021 14:03

This was referenced Mar 20, 2021

How do we return a binary (.zip) file as a response from FastAPI through AWS Lambda and API Gateway? #161

Closed

issue in reading .woff/.ttf/.woff2 (font extension) files #160

Closed

Refactor tests parameters and fixtures #106

Closed

four43 force-pushed the refactor-split-handlers branch from 9402d60 to a0c3e14 Compare March 20, 2021 14:46

Refactor handlers to be separate from core logic

7a4134e

four43 force-pushed the refactor-split-handlers branch from 57c3ce0 to 7a4134e Compare March 20, 2021 14:53

Cleanup code for black, flake8, pypy

03c3b26

jordaneremieff reviewed Mar 21, 2021

View reviewed changes

four43 added 2 commits March 21, 2021 17:04

Cleanup imports

91727f8

Move flake8 config to existing setup.cfg

d9f759f

jordaneremieff reviewed Mar 21, 2021

View reviewed changes

Updated code style to adhere to 88 char limit

38022ef

jordaneremieff reviewed Mar 22, 2021

View reviewed changes

jordaneremieff merged commit ddf531b into Kludex:main Mar 22, 2021

tuukkamustonen mentioned this pull request Mar 22, 2021

[Feature Request] Support for custom loggers #156

Closed

four43 deleted the refactor-split-handlers branch March 22, 2021 15:07

four43 mentioned this pull request Mar 22, 2021

Validate kwargs more strictly #172

Closed

jordaneremieff mentioned this pull request Mar 28, 2021

AWS WebSocket support #132

Closed

jurasofish mentioned this pull request Apr 17, 2021

Double url-encoding of query string with ALB #178

Closed

bradsbrown mentioned this pull request Aug 10, 2021

Exclude tests from find_packages #200

Merged

matthieudesprez mentioned this pull request Sep 21, 2021

mypy - Argument "api_gateway_base_path" to "Mangum" has incompatible type "str"; expected "Dict[str, Any]" #201

Closed

Uh oh!

Conversation

four43 commented Mar 20, 2021

Uh oh!

four43 commented Mar 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dltacube commented Mar 20, 2021

Uh oh!

four43 commented Mar 20, 2021

Uh oh!

koxudaxi commented Mar 20, 2021

Uh oh!

four43 commented Mar 20, 2021

Uh oh!

koxudaxi commented Mar 20, 2021

Uh oh!

dltacube commented Mar 20, 2021

Uh oh!

jordaneremieff commented Mar 20, 2021

Uh oh!

jordaneremieff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

four43 commented Mar 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

four43 Mar 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

four43 commented Mar 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

four43 commented Mar 22, 2021 via email

Uh oh!

jordaneremieff commented Mar 22, 2021

Uh oh!

four43 commented Mar 22, 2021 via email

Uh oh!

tuukkamustonen commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

four43 commented Mar 22, 2021

Uh oh!

tuukkamustonen commented Mar 22, 2021

Uh oh!

tuukkamustonen commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

four43 commented Mar 20, 2021 •

edited

Loading

four43 Mar 21, 2021 •

edited

Loading

tuukkamustonen commented Mar 22, 2021 •

edited

Loading

tuukkamustonen commented Mar 22, 2021 •

edited

Loading