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 create_tables() to SQLAlchemyOAuthStateStore #1365

Conversation

raphaelhuefner
Copy link
Contributor

Summary

Adds a create_tables() method to slack_sdk.oauth.state_store.sqlalchemy.SQLAlchemyOAuthStateStore, copied from slack_sdk.oauth.installation_store.sqlalchemy.SQLAlchemyInstallationStore.

It was / is missing when I tried using SQLAlchemyOAuthStateStore from the Python Slack SDK version 3.21.3.

I had to revert to the work-around of calling SQLAlchemyOAuthStateStore.metadata.create_all(SQLAlchemyOAuthStateStore.engine) directly.

Since sqlalchemy.Metadata.create_all() is idempotent, maybe a call to create_tables() could get added to SQLAlchemyOAuthStateStore. __init__() and SQLAlchemyInstallationStore.__init__()? Possibly behind a boolean flag parameter which would make it more obvious to 1st time users like me ;-) ?

LMK if you'd want me to add that to this PR.

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

  • 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-src (Documents, have you run ./scripts/docs.sh?)
  • /docs-src-v2 (Documents, have you run ./scripts/docs-v2.sh?)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

Requirements (place an x in each [ ])

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

@salesforce-cla
Copy link

salesforce-cla bot commented May 8, 2023

Thanks for the contribution! Before we can merge this, we need @raphaelhuefner to sign the Salesforce Inc. Contributor License Agreement.

@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #1365 (19cf2ce) into main (8815d21) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

❗ Current head 19cf2ce differs from pull request most recent head f23aa8f. Consider uploading reports for the commit f23aa8f to get more accurate results

@@            Coverage Diff             @@
##             main    #1365      +/-   ##
==========================================
- Coverage   85.58%   85.57%   -0.01%     
==========================================
  Files         111      111              
  Lines       11557    11559       +2     
==========================================
+ Hits         9891     9892       +1     
- Misses       1666     1667       +1     
Impacted Files Coverage Δ
slack_sdk/webhook/async_client.py 92.23% <ø> (ø)
slack_sdk/webhook/client.py 93.91% <ø> (ø)
slack_sdk/oauth/state_store/sqlalchemy/__init__.py 88.46% <50.00%> (-1.54%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@WilliamBergamin
Copy link
Contributor

WilliamBergamin commented May 8, 2023

Hi @raphaelhuefner thanks for your contribution 💯

This change does bring up some concerns that other users may be experiencing a similar issue!
I've been looking at our examples for sqlalchemy in the bolt-python library and it seem like using

installation_store.metadata.create_all(engine)
oauth_state_store.metadata.create_all(engine)

is the intended behavior, @seratch do you know what the intended behavior of SQLAlchemyOAuthStateStore is?

My main concern is that SQLAlchemyOAuthStateStore does not implement or extend MetaData it currently uses delegation to reference the object instead

@seratch seratch added this to the 3.22.0 milestone May 8, 2023
@seratch seratch self-assigned this May 8, 2023
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.

@raphaelhuefner Thanks for the PR! This looks good to me 👍

@WilliamBergamin The behavior indeed may not be intuitive but after initializing the metadata property, the internal code in the same constructor associates with the table definition. So, the changes in this PR should work well. The code in the example app that directly calls metadata's methods can be updated if we want to do so but to me, it sounds optional. As the metadata property is not private (meaning we don't name it with _ prefix like common naming convention), accessing it is totally fine too.

@seratch seratch merged commit 7eec685 into slackapi:main May 8, 2023
8 checks passed
@raphaelhuefner raphaelhuefner deleted the add-create_tables-to-SQLAlchemyOAuthStateStore branch May 10, 2023 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants