slackapi / python-slack-sdk

Slack Developer Kit for Python
https://tools.slack.dev/python-slack-sdk/
MIT License
3.85k stars 837 forks source link

SQLAlchemy OAuth module: 'ResultProxy' object has no attribute 'mappings' #1337

Closed uroboro closed 1 year ago

uroboro commented 1 year ago

Summary

Some compatibility with SQLAlchemy v1 was broken when adding compatibility with SQLAlchemy v2 (in #1335 for https://github.com/slackapi/bolt-python/issues/822).

Overview

I've confirmed that the code works with both v1.4 and v2.0.

From the documentation on SQLAlchemy 1.4:

Changed in version 1.4: The CursorResult and LegacyCursorResult classes replace the previous ResultProxy interface. These classes are based on the Result calling API which provides an updated usage model and calling facade for SQLAlchemy Core and SQLAlchemy ORM.

CursorResult has a mappings() member that ResultProxy does not.

Lastly, setup.py declares compatibility with SQLAlchemy>=1,<3, this caught us off guard.

The code can be updated to support both CursorResult and ResultProxy as the return type of sqlalchemy.engine.Connection.execute with something like the following:

-   for row in result.mappings():
+   for row in (result.mappings() if hasattr(result, "mappings") else result):

for all the similar usages in the PR.

The Slack SDK version

slack-sdk==3.20.0

Python runtime version

3.9.5

Steps to reproduce:

  1. Install slack-sdk==3.20.0 and SQLAlchemy<1.4.0
  2. Deploy service
  3. Receive an event

Expected result:

Process the event.

Actual result:

Traceback (most recent call last):
  File ".../python3.9/site-packages/slack_bolt/app/app.py", line 477, in dispatch
    resp = middleware.process(req=req, resp=resp, next=middleware_next)
  File ".../python3.9/site-packages/slack_bolt/middleware/authorization/multi_teams_authorization.py", line 58, in process
    auth_result: Optional[AuthorizeResult] = self.authorize(
  File ".../python3.9/site-packages/slack_bolt/authorization/authorize.py", line 166, in __call__
    latest_installation: Optional[Installation] = self.installation_store.find_installation(
  File "../python3.9/site-packages/slack_sdk/oauth/installation_store/sqlalchemy/__init__.py", line 264, in find_installation
    for row in result.mappings():  # type: ignore
AttributeError: 'ResultProxy' object has no attribute 'mappings' 
seratch commented 1 year ago

Hi @uroboro, thanks for taking the time to report this issue with details. I've confirmed all the unit tests in this project pass with the changes works for both v1.4 and v2.0, but it seems that it was not enough to verify the backward compatibility.

I can work on resolving this breaking change early next week and release a new patch version. If you are happy to send a pull request fixing this, your contribution will be greatly apprecitated too!

seratch commented 1 year ago

I tried to resolve this issue but found that more changes are required for 1.3 or older version supports:

sqlalchemy.exc.ArgumentError: columns argument to select() must be a Python list or other iterable

Having if/else conditions based on the SQLAlchemy version runtime is not a complexity we want to have only for legacy versions. So, let us drop SQLAlchemy 1.3 version support since v3.20. We will make it clear in the setup.py starting in the next patch version.

To: existing users who have to keep using SQLAlchemy 1.3 or older, A workaround until you upgrade SQLAlchemy minor version is to fork the installation_store / state_store as of v3.19.5 in your app project. We'd appreciate it if you could understand this.

@uroboro Thank you very much again for reporting this issue with details, and I am sorry to come to this decision. I hope migrating to 1.4 or newer won't be hard for your apps.

uroboro commented 1 year ago

Thank you for the quick reply!

We were able to upgrade to 1.4 without major issues, mostly SQLAlchemy typing issues that mypy was not able to determine as it did before.