Skip to content
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

Add Falcon (ASGI) adapter #614

Merged
merged 10 commits into from
Mar 15, 2022

Conversation

sarayourfriend
Copy link
Contributor

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
Copy link

CLAassistant commented Mar 15, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
slack_bolt/adapter/falcon/__init__.py Outdated Show resolved Hide resolved
slack_bolt/adapter/falcon/async_resource.py Outdated Show resolved Hide resolved
slack_bolt/adapter/falcon/async_resource.py Outdated Show resolved Hide resolved
tests/adapter_tests_async/test_async_falcon.py Outdated Show resolved Hide resolved
@seratch seratch self-assigned this Mar 15, 2022
@seratch seratch added this to the 1.12.0 milestone Mar 15, 2022
@sarayourfriend
Copy link
Contributor Author

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

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update a few comments in the example apps?

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
Co-authored-by: Kazuhiro Sera <seratch@gmail.com>
Co-authored-by: Kazuhiro Sera <seratch@gmail.com>
@seratch
Copy link
Member

seratch commented Mar 15, 2022

@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
Copy link
Contributor Author

sarayourfriend commented Mar 15, 2022

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

# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do the same with fastapi examples

@seratch
Copy link
Member

seratch commented Mar 15, 2022

@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
Copy link
Contributor Author

@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
Copy link
Member

seratch commented Mar 15, 2022

@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
Copy link
Contributor Author

sarayourfriend commented Mar 15, 2022

@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
Copy link
Member

seratch commented Mar 15, 2022

@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
Copy link
Contributor Author

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

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Co-authored-by: Kazuhiro Sera <seratch@gmail.com>
Co-authored-by: Kazuhiro Sera <seratch@gmail.com>
@codecov
Copy link

codecov bot commented Mar 15, 2022

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

Impacted file tree graph

@@            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.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@seratch seratch merged commit 911e21d into slackapi:main Mar 15, 2022
@seratch seratch changed the title Add ASGI Falcon App support Add Falcon (ASGI) adapter Mar 17, 2022
@seratch seratch mentioned this pull request Mar 20, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants