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

feat: #1276 add Asyncio SQLAlchemy support #1633

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

galuszkak
Copy link

Summary

Add support for Asyncio SQLAlchemy Installation and StateStore

Testing

Category

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.signature (Request Signature Verifier)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.models (UI component builders)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.rtm_v2 (RTM client)
  • /docs (Documents)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

Requirements

  • 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 python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes.

@seratch seratch added enhancement M-T: A feature request for new functionality Version: 3x oauth area:async labels Jan 14, 2025
@seratch seratch self-assigned this Jan 14, 2025
@seratch seratch added this to the 3.x milestone Jan 14, 2025
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.

Left a few comments; Also, it seems the tests fail even for supported Python runtime versions. Could you check the issue?

slack_sdk/oauth/installation_store/sqlalchemy/__init__.py Outdated Show resolved Hide resolved
slack_sdk/oauth/installation_store/sqlalchemy/__init__.py Outdated Show resolved Hide resolved
tests/slack_sdk/oauth/state_store/test_async_sqlalchemy.py Outdated Show resolved Hide resolved
@galuszkak galuszkak force-pushed the feature/async-sqlalchemy branch from 4be23e7 to bdcea76 Compare January 14, 2025 16:26
@galuszkak
Copy link
Author

@seratch will look into that probably today. The problem is that my tests are passing, it just something happens later in tests that they are braking - I think it's somehow related to retrieving asyncio loop by std library.

@galuszkak galuszkak force-pushed the feature/async-sqlalchemy branch from bdcea76 to 69822e9 Compare January 14, 2025 16:41
@galuszkak
Copy link
Author

I've identified the source of the test failures @seratch. The original async_test helper created a single event loop and set it globally, which was the core of the problem. unittest.IsolatedAsyncioTestCase uses an asyncio runner that closes the event loop in its finally block. This meant that tests after the first were trying to use an already closed loop. By changing async_test to generate a new loop for each test, the problem is solved, and all tests are passing in the latest Python versions. This fix works because, while the decorator creates a new loop, the runner's finally block was closing the previously set loop, leading to the RuntimeError: There is no current event loop in thread 'MainThread'.

I assume I have to still rework this to make it work with Python 3.6 and 3.7 so in the end I will revert that - but it was nice to find root cause of this.

@galuszkak
Copy link
Author

I've tested locally now with support from Python 3.6/3.7

Testing with Python 3.7 I got

855 passed, 5 skipped, 23 warnings in 239.12s (0:03:59)

On my M1 Macbook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:async cla:signed enhancement M-T: A feature request for new functionality oauth Version: 3x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants