Add Falcon (ASGI) adapter #614

sarayourfriend · 2022-03-15T00:59:33Z

This PR adds an ASGI Falcon App adapter to complement the existing Falcon adapter for WSGI apps.

I wrote this code originally based on the existing adapter for https://git.sr.ht/~sara/openverse-slack-reaction.

The code I wrote there is copied almost directly here with some modifications to raise errors when an incompatible Falcon version is being used. You can see the original code here: https://git.sr.ht/~sara/openverse-slack-reaction/tree/main/item/falcon_adapter.py

It's deployed to production with https://openverse-slack.sarayourfriend.pictures.

Hope it's okay I didn't open an issue for this first, but I needed it for my own app one way or another and copying the file and adding tests was trivial so I didn't spend too much extra time on this compared to just writing a helpful issue describing the feature 🙂

Category (place an `x` in each of the `[ ]`)

slack_bolt.App and/or its core components
slack_bolt.async_app.AsyncApp and/or its core components
Adapters in slack_bolt.adapter
Document pages under /docs
Others

Requirements (place an `x` in each `[ ]`)

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've run ./scripts/install_all_and_run_tests.sh after making the changes.

CLAassistant · 2022-03-15T00:59:38Z

All committers have signed the CLA.

seratch

Thanks for the great addition! We are happy to have this enhancement but could you check my comments and update your branch?

slack_bolt/adapter/falcon/__init__.py

slack_bolt/adapter/falcon/async_resource.py

tests/adapter_tests_async/test_async_falcon.py

@seratch

Props to @seratch

Additionally only check it a single time

sarayourfriend · 2022-03-15T09:12:27Z

Thanks for the quick review @seratch! I've updated the PR with your requested changes.

seratch

Can you update a few comments in the example apps?

examples/falcon/async_app.py

examples/falcon/async_oauth_app.py

Co-authored-by: Kazuhiro Sera <seratch@gmail.com>

seratch · 2022-03-15T09:16:12Z

@sarayourfriend Thanks for quick updates on the feedback! My last request is to update the examples a bit. Once the builds pass, we can merge this PR and release a new version by the end of this week.

sarayourfriend · 2022-03-15T09:21:55Z

Hmm, it seems like there's an issue with the fact that there are tests that expect Falcon 2 instead of 3... is there some way to exclude the async Falcon tests when the falcon version is 2.x?

I suppose the module level skip could do it?

https://docs.pytest.org/en/latest/how-to/skipping.html#skipping-test-functions

seratch · 2022-03-15T09:50:49Z

examples/falcon/async_app.py

+# pip install -r requirements.txt
+# export SLACK_SIGNING_SECRET=***
+# export SLACK_BOT_TOKEN=xoxb-***
+# uvicorn --reload -h 0.0.0.0 -p 3000 async_app:api


@sarayourfriend Can you add unvicorn to requirements.txt in the same directory?

Sure thing. I'm not sure what version range to pin it to. The project is still <1. Should I just use uvicorn>=0.17.6 (the currently released version)?

You can do the same with fastapi examples

seratch · 2022-03-15T09:51:44Z

@sarayourfriend You can change this line to use >=3,<4 instead: https://github.com/slackapi/bolt-python/blob/v1.11.6/setup.py#L82
Changing the part does not affect users.

sarayourfriend · 2022-03-15T09:55:27Z

@seratch should I also remove the conditional in the existing Falcon adapter tests? I've never worked with setup.py before so just want to make sure I'm understanding correctly that it will run the tests only with Falcon 3.x?

It could make this function unnecessary: https://github.com/sarayourfriend/bolt-python/blob/78b64878b692039e39530888a012bf738ca74058/tests/adapter_tests/falcon/test_falcon.py#L23

seratch · 2022-03-15T10:03:36Z

@sarayourfriend Ah, sorry - never mind my above comment. I totally forget what I did in the past 🤦 No need to modify setup.py.

Can you modify https://github.com/slackapi/bolt-python/blob/v1.11.6/.github/workflows/tests.yml#L73 to have pip install "falcon>=3,<4" before the pytest execution? It should resolve the issue.

sarayourfriend · 2022-03-15T10:27:37Z

@seratch just wanted to make sure, because I don't see any other explicit dependency installations in that workflow... would it make sense to add the falcon>=3 range in the async dependency list as that's already being installed in the tests.yml

This is the patch I would apply if that makes sense, but I don't know if "conflicting" arguments like this would cause pip to fail (it seems to work okay locally FWIW):

diff --git a/setup.py b/setup.py
index 6543529..9c92d94 100755
--- a/setup.py
+++ b/setup.py
@@ -68,6 +68,8 @@ setuptools.setup(
             "aiohttp>=3,<4",
             # Socket Mode 3rd party implementation
             "websockets>=8,<10",
+            # Falcon introduced ASGI support in v3
+            "falcon>=3,<4",
         ],
         # pip install -e ".[adapter]"
         # NOTE: any of async ones requires pip install -e ".[async]" too
@@ -79,7 +81,7 @@ setuptools.setup(
             "click>=7,<8",  # for chalice
             "CherryPy>=18,<19",
             "Django>=3,<5",
-            "falcon>=3,<4",
+            "falcon>=2,<4",
             "fastapi>=0.70.0,<1",
             "Flask>=1,<3",
             "Werkzeug>=2,<3",

If this doesn't work for some reason or you still prefer the explicit installation in the tests.yml script then let me know and I'll make the appropriate change.

seratch · 2022-03-15T10:33:50Z

@sarayourfriend this line manually installs v2: https://github.com/slackapi/bolt-python/blob/v1.11.6/.github/workflows/tests.yml#L56 this is necessary for v2 tests but it causes errors in the async tests you added.

sarayourfriend · 2022-03-15T10:41:28Z

Ah, I see. Makes sense, thanks for explaining. Pushed the update!

seratch

@sarayourfriend I found that your unit test properly detects a bug in your code. Can you check my comments?

setup.py

slack_bolt/adapter/falcon/async_resource.py

Co-authored-by: Kazuhiro Sera <seratch@gmail.com>

codecov · 2022-03-15T12:33:30Z

Codecov Report

Merging #614 (efd3fb8) into main (837c773) will decrease coverage by 0.05%.
The diff coverage is 83.72%.

❗ Current head efd3fb8 differs from pull request most recent head 5a9cb0a. Consider uploading reports for the commit 5a9cb0a to get more accurate results

@@            Coverage Diff             @@
##             main     #614      +/-   ##
==========================================
- Coverage   91.52%   91.46%   -0.06%     
==========================================
  Files         169      170       +1     
  Lines        5723     5766      +43     
==========================================
+ Hits         5238     5274      +36     
- Misses        485      492       +7

Impacted Files	Coverage Δ
slack_bolt/adapter/falcon/__init__.py	`100.00% <ø> (ø)`
slack_bolt/adapter/falcon/async_resource.py	`83.72% <83.72%> (ø)`

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 837c773...5a9cb0a. Read the comment docs.

seratch

LGTM

sarayourfriend added 2 commits March 14, 2022 20:56

Add ASGI Falcon App support

667d89f

Add examples for ASGI Falcon adapter

7bd69cf

seratch requested changes Mar 15, 2022

View reviewed changes

seratch self-assigned this Mar 15, 2022

seratch added enhancement New feature or request area:async area:adapter labels Mar 15, 2022

seratch added this to the 1.12.0 milestone Mar 15, 2022

sarayourfriend added 2 commits March 15, 2022 05:08

Don't accidentally force async into non-async apps

937fddc

Props to @seratch

Move version check early in the execution

515345c

Additionally only check it a single time

seratch requested changes Mar 15, 2022

View reviewed changes

examples/falcon/async_app.py Outdated Show resolved Hide resolved

examples/falcon/async_oauth_app.py Outdated Show resolved Hide resolved

sarayourfriend and others added 2 commits March 15, 2022 05:14

Update examples/falcon/async_app.py

ae20975

Co-authored-by: Kazuhiro Sera <seratch@gmail.com>

Update examples/falcon/async_oauth_app.py

dc59647

Co-authored-by: Kazuhiro Sera <seratch@gmail.com>

seratch reviewed Mar 15, 2022

View reviewed changes

sarayourfriend added 2 commits March 15, 2022 06:40

Indicate uvicorn as a dependency of the asgi app

19fa7f5

Manually install falcon >=3 for async tests

95bc0ba

seratch requested changes Mar 15, 2022

View reviewed changes

setup.py Outdated Show resolved Hide resolved

slack_bolt/adapter/falcon/async_resource.py Show resolved Hide resolved

sarayourfriend and others added 2 commits March 15, 2022 08:26

Fix response text not being set

241a749

Co-authored-by: Kazuhiro Sera <seratch@gmail.com>

Revert unnecessary falcon version bump

5a9cb0a

Co-authored-by: Kazuhiro Sera <seratch@gmail.com>

seratch approved these changes Mar 15, 2022

View reviewed changes

seratch merged commit 911e21d into slackapi:main Mar 15, 2022

seratch changed the title ~~Add ASGI Falcon App support~~ Add Falcon (ASGI) adapter Mar 17, 2022

seratch mentioned this pull request Mar 20, 2022

Add Quart adapter (resolving #619) #620

Draft

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Falcon (ASGI) adapter #614

Add Falcon (ASGI) adapter #614

sarayourfriend commented Mar 15, 2022

CLAassistant commented Mar 15, 2022 •

edited

Loading

seratch left a comment

sarayourfriend commented Mar 15, 2022

seratch left a comment

seratch commented Mar 15, 2022

sarayourfriend commented Mar 15, 2022 •

edited

Loading

seratch Mar 15, 2022

sarayourfriend Mar 15, 2022

seratch Mar 15, 2022

seratch commented Mar 15, 2022

sarayourfriend commented Mar 15, 2022

seratch commented Mar 15, 2022

sarayourfriend commented Mar 15, 2022 •

edited

Loading

seratch commented Mar 15, 2022

sarayourfriend commented Mar 15, 2022

seratch left a comment

codecov bot commented Mar 15, 2022

seratch left a comment

Add Falcon (ASGI) adapter #614

Add Falcon (ASGI) adapter #614

Conversation

sarayourfriend commented Mar 15, 2022

Category (place an x in each of the [ ])

Requirements (place an x in each [ ])

CLAassistant commented Mar 15, 2022 • edited Loading

seratch left a comment

Choose a reason for hiding this comment

sarayourfriend commented Mar 15, 2022

seratch left a comment

Choose a reason for hiding this comment

seratch commented Mar 15, 2022

sarayourfriend commented Mar 15, 2022 • edited Loading

seratch Mar 15, 2022

Choose a reason for hiding this comment

sarayourfriend Mar 15, 2022

Choose a reason for hiding this comment

seratch Mar 15, 2022

Choose a reason for hiding this comment

seratch commented Mar 15, 2022

sarayourfriend commented Mar 15, 2022

seratch commented Mar 15, 2022

sarayourfriend commented Mar 15, 2022 • edited Loading

seratch commented Mar 15, 2022

sarayourfriend commented Mar 15, 2022

seratch left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 15, 2022

Codecov Report

seratch left a comment

Choose a reason for hiding this comment

Category (place an `x` in each of the `[ ]`)

Requirements (place an `x` in each `[ ]`)

CLAassistant commented Mar 15, 2022 •

edited

Loading

sarayourfriend commented Mar 15, 2022 •

edited

Loading

sarayourfriend commented Mar 15, 2022 •

edited

Loading