diff --git a/.github/workflows/ci-build.yml b/.github/workflows/ci-build.yml index c705fbb7d..2cdb5b9d5 100644 --- a/.github/workflows/ci-build.yml +++ b/.github/workflows/ci-build.yml @@ -114,6 +114,55 @@ jobs: token: ${{ secrets.CODECOV_TOKEN }} verbose: true + databases: + # TODO: Add MySQL and other database testing when possible + name: Database Unit Tests + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + services: + postgres: + image: postgres@sha256:e4842c8a99ca99339e1693e6fe5fe62c7becb31991f066f989047dfb2fbf47af # 16 + env: + POSTGRES_USER: test_user + POSTGRES_PASSWORD: password + POSTGRES_DB: test + ports: + - 5432:5432 + options: >- + --health-cmd pg_isready + --health-interval 10s + --health-timeout 5s + --health-retries 5 + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + - name: Set up Python ${{ env.LATEST_SUPPORTED_PY }} + uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + with: + python-version: ${{ env.LATEST_SUPPORTED_PY }} + cache: pip + - name: Install dependencies + run: | + pip install -U pip + pip install -r requirements/testing.txt + pip install -r requirements/optional.txt + pip install -r requirements/databases.txt + - name: Run sync tests (PostgreSQL) + env: + TEST_DATABASE_URL: postgresql://test_user:password@localhost/test + run: | + PYTHONPATH=$PWD:$PYTHONPATH pytest -s tests/slack_sdk/oauth/installation_store/test_sqlalchemy.py + PYTHONPATH=$PWD:$PYTHONPATH pytest -s tests/slack_sdk/oauth/state_store/test_sqlalchemy.py + - name: Run async tests (PostgreSQL) + env: + ASYNC_TEST_DATABASE_URL: postgresql+asyncpg://test_user:password@localhost/test + run: | + PYTHONPATH=$PWD:$PYTHONPATH pytest -s tests/slack_sdk/oauth/installation_store/test_async_sqlalchemy.py + PYTHONPATH=$PWD:$PYTHONPATH pytest -s tests/slack_sdk/oauth/state_store/test_async_sqlalchemy.py + notifications: name: Regression notifications runs-on: ubuntu-latest @@ -121,6 +170,7 @@ jobs: - lint - typecheck - unittest + - databases if: ${{ !success() && github.ref == 'refs/heads/main' && github.event_name != 'workflow_dispatch' }} steps: - name: Send notifications of failing tests diff --git a/requirements/databases.txt b/requirements/databases.txt new file mode 100644 index 000000000..a7cd29b5f --- /dev/null +++ b/requirements/databases.txt @@ -0,0 +1,5 @@ +# Database drivers for CI testing + +# PostgreSQL drivers +psycopg2-binary>=2.9,<3 +asyncpg>=0.27,<1 diff --git a/slack_sdk/oauth/installation_store/sqlalchemy/__init__.py b/slack_sdk/oauth/installation_store/sqlalchemy/__init__.py index f629deead..942602b36 100644 --- a/slack_sdk/oauth/installation_store/sqlalchemy/__init__.py +++ b/slack_sdk/oauth/installation_store/sqlalchemy/__init__.py @@ -23,6 +23,7 @@ from slack_sdk.oauth.installation_store.async_installation_store import ( AsyncInstallationStore, ) +from slack_sdk.oauth.sqlalchemy_utils import normalize_datetime_for_db class SQLAlchemyInstallationStore(InstallationStore): @@ -140,6 +141,9 @@ def save(self, installation: Installation): with self.engine.begin() as conn: i = installation.to_dict() i["client_id"] = self.client_id + i["installed_at"] = normalize_datetime_for_db(i.get("installed_at")) + i["bot_token_expires_at"] = normalize_datetime_for_db(i.get("bot_token_expires_at")) + i["user_token_expires_at"] = normalize_datetime_for_db(i.get("user_token_expires_at")) i_column = self.installations.c installations_rows = conn.execute( @@ -171,6 +175,8 @@ def save_bot(self, bot: Bot): # bots b = bot.to_dict() b["client_id"] = self.client_id + b["installed_at"] = normalize_datetime_for_db(b.get("installed_at")) + b["bot_token_expires_at"] = normalize_datetime_for_db(b.get("bot_token_expires_at")) b_column = self.bots.c bots_rows = conn.execute( @@ -419,6 +425,9 @@ async def async_save(self, installation: Installation): async with self.engine.begin() as conn: i = installation.to_dict() i["client_id"] = self.client_id + i["installed_at"] = normalize_datetime_for_db(i.get("installed_at")) + i["bot_token_expires_at"] = normalize_datetime_for_db(i.get("bot_token_expires_at")) + i["user_token_expires_at"] = normalize_datetime_for_db(i.get("user_token_expires_at")) i_column = self.installations.c installations_rows = await conn.execute( @@ -450,6 +459,8 @@ async def async_save_bot(self, bot: Bot): # bots b = bot.to_dict() b["client_id"] = self.client_id + b["installed_at"] = normalize_datetime_for_db(b.get("installed_at")) + b["bot_token_expires_at"] = normalize_datetime_for_db(b.get("bot_token_expires_at")) b_column = self.bots.c bots_rows = await conn.execute( diff --git a/slack_sdk/oauth/sqlalchemy_utils/__init__.py b/slack_sdk/oauth/sqlalchemy_utils/__init__.py new file mode 100644 index 000000000..a0692bda3 --- /dev/null +++ b/slack_sdk/oauth/sqlalchemy_utils/__init__.py @@ -0,0 +1,33 @@ +from datetime import datetime +from typing import Optional + + +# TODO: Remove this function in next major release (v4.0.0) after updating all +# DateTime columns to DateTime(timezone=True). See issue #1832 for context. +def normalize_datetime_for_db(dt: Optional[datetime]) -> Optional[datetime]: + """ + Normalize timezone-aware datetime to naive UTC datetime for database storage. + + Ensures compatibility with existing databases using TIMESTAMP WITHOUT TIME ZONE. + SQLAlchemy DateTime columns without timezone=True create naive timestamp columns + in databases like PostgreSQL. This function strips timezone information from + timezone-aware datetimes (which are already in UTC) to enable safe comparisons. + + Args: + dt: A timezone-aware or naive datetime object, or None + + Returns: + A naive datetime in UTC, or None if input is None + + Example: + >>> from datetime import datetime, timezone + >>> aware_dt = datetime(2024, 1, 1, 12, 0, 0, tzinfo=timezone.utc) + >>> naive_dt = normalize_datetime_for_db(aware_dt) + >>> naive_dt.tzinfo is None + True + """ + if dt is None: + return None + if dt.tzinfo is not None: + return dt.replace(tzinfo=None) + return dt diff --git a/slack_sdk/oauth/state_store/sqlalchemy/__init__.py b/slack_sdk/oauth/state_store/sqlalchemy/__init__.py index 8bb3ec1ff..3898c5b32 100644 --- a/slack_sdk/oauth/state_store/sqlalchemy/__init__.py +++ b/slack_sdk/oauth/state_store/sqlalchemy/__init__.py @@ -10,6 +10,7 @@ from sqlalchemy import Table, Column, Integer, String, DateTime, and_, MetaData from sqlalchemy.engine import Engine from sqlalchemy.ext.asyncio import AsyncEngine +from slack_sdk.oauth.sqlalchemy_utils import normalize_datetime_for_db class SQLAlchemyOAuthStateStore(OAuthStateStore): @@ -55,7 +56,7 @@ def logger(self) -> Logger: def issue(self, *args, **kwargs) -> str: state: str = str(uuid4()) - now = datetime.fromtimestamp(time.time() + self.expiration_seconds, tz=timezone.utc) + now = normalize_datetime_for_db(datetime.fromtimestamp(time.time() + self.expiration_seconds, tz=timezone.utc)) with self.engine.begin() as conn: conn.execute( self.oauth_states.insert(), @@ -65,9 +66,10 @@ def issue(self, *args, **kwargs) -> str: def consume(self, state: str) -> bool: try: + now = normalize_datetime_for_db(datetime.now(tz=timezone.utc)) with self.engine.begin() as conn: c = self.oauth_states.c - query = self.oauth_states.select().where(and_(c.state == state, c.expire_at > datetime.now(tz=timezone.utc))) + query = self.oauth_states.select().where(and_(c.state == state, c.expire_at > now)) result = conn.execute(query) for row in result.mappings(): self.logger.debug(f"consume's query result: {row}") @@ -124,7 +126,7 @@ def logger(self) -> Logger: async def async_issue(self, *args, **kwargs) -> str: state: str = str(uuid4()) - now = datetime.fromtimestamp(time.time() + self.expiration_seconds, tz=timezone.utc) + now = normalize_datetime_for_db(datetime.fromtimestamp(time.time() + self.expiration_seconds, tz=timezone.utc)) async with self.engine.begin() as conn: await conn.execute( self.oauth_states.insert(), @@ -134,9 +136,10 @@ async def async_issue(self, *args, **kwargs) -> str: async def async_consume(self, state: str) -> bool: try: + now = normalize_datetime_for_db(datetime.now(tz=timezone.utc)) async with self.engine.begin() as conn: c = self.oauth_states.c - query = self.oauth_states.select().where(and_(c.state == state, c.expire_at > datetime.now(tz=timezone.utc))) + query = self.oauth_states.select().where(and_(c.state == state, c.expire_at > now)) result = await conn.execute(query) for row in result.mappings(): self.logger.debug(f"consume's query result: {row}") diff --git a/tests/slack_sdk/oauth/installation_store/test_async_sqlalchemy.py b/tests/slack_sdk/oauth/installation_store/test_async_sqlalchemy.py index 35aa79623..6370ffb77 100644 --- a/tests/slack_sdk/oauth/installation_store/test_async_sqlalchemy.py +++ b/tests/slack_sdk/oauth/installation_store/test_async_sqlalchemy.py @@ -1,3 +1,4 @@ +import os import unittest from tests.helpers import async_test from sqlalchemy.ext.asyncio import AsyncEngine, create_async_engine @@ -5,13 +6,20 @@ from slack_sdk.oauth.installation_store import Installation from slack_sdk.oauth.installation_store.sqlalchemy import AsyncSQLAlchemyInstallationStore +database_url = os.environ.get("ASYNC_TEST_DATABASE_URL", "sqlite+aiosqlite:///:memory:") + + +def setUpModule(): + """Emit database configuration for CI visibility across builds.""" + print(f"\n[InstallationStore/AsyncSQLAlchemy] Database: {database_url}") + class TestAsyncSQLAlchemy(unittest.TestCase): engine: AsyncEngine @async_test async def setUp(self): - self.engine = create_async_engine("sqlite+aiosqlite:///:memory:") + self.engine = create_async_engine(database_url) self.store = AsyncSQLAlchemyInstallationStore(client_id="111.222", engine=self.engine) async with self.engine.begin() as conn: await conn.run_sync(self.store.metadata.create_all) @@ -296,3 +304,27 @@ async def test_issue_1441_mixing_user_and_bot_installations(self): self.assertIsNone(installation) installation = await store.async_find_installation(enterprise_id=None, team_id="T111") self.assertIsNone(installation) + + @async_test + async def test_timezone_aware_datetime_compatibility(self): + installation = Installation( + app_id="A111", + enterprise_id="E111", + team_id="T111", + user_id="U111", + bot_id="B111", + bot_token="xoxb-111", + bot_scopes=["chat:write"], + bot_user_id="U222", + ) + + # First save + await self.store.async_save(installation) + found = await self.store.async_find_installation(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(found) + self.assertEqual(found.app_id, "A111") + + # Second save (update) - tests WHERE clause with installed_at comparison + await self.store.async_save(installation) + found = await self.store.async_find_installation(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(found) diff --git a/tests/slack_sdk/oauth/installation_store/test_sqlalchemy.py b/tests/slack_sdk/oauth/installation_store/test_sqlalchemy.py index 4d827f70b..75568e94c 100644 --- a/tests/slack_sdk/oauth/installation_store/test_sqlalchemy.py +++ b/tests/slack_sdk/oauth/installation_store/test_sqlalchemy.py @@ -1,3 +1,4 @@ +import os import unittest import sqlalchemy @@ -6,12 +7,19 @@ from slack_sdk.oauth.installation_store import Installation from slack_sdk.oauth.installation_store.sqlalchemy import SQLAlchemyInstallationStore +database_url = os.environ.get("TEST_DATABASE_URL", "sqlite:///:memory:") + + +def setUpModule(): + """Emit database configuration for CI visibility across builds.""" + print(f"\n[InstallationStore/SQLAlchemy] Database: {database_url}") + class TestSQLAlchemy(unittest.TestCase): engine: Engine def setUp(self): - self.engine = sqlalchemy.create_engine("sqlite:///:memory:") + self.engine = sqlalchemy.create_engine(database_url) self.store = SQLAlchemyInstallationStore(client_id="111.222", engine=self.engine) self.store.metadata.create_all(self.engine) @@ -289,3 +297,26 @@ def test_issue_1441_mixing_user_and_bot_installations(self): self.assertIsNone(installation) installation = store.find_installation(enterprise_id=None, team_id="T111") self.assertIsNone(installation) + + def test_timezone_aware_datetime_compatibility(self): + installation = Installation( + app_id="A111", + enterprise_id="E111", + team_id="T111", + user_id="U111", + bot_id="B111", + bot_token="xoxb-111", + bot_scopes=["chat:write"], + bot_user_id="U222", + ) + + # First save + self.store.save(installation) + found = self.store.find_installation(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(found) + self.assertEqual(found.app_id, "A111") + + # Second save (update) - tests WHERE clause with installed_at comparison + self.store.save(installation) + found = self.store.find_installation(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(found) diff --git a/tests/slack_sdk/oauth/state_store/test_async_sqlalchemy.py b/tests/slack_sdk/oauth/state_store/test_async_sqlalchemy.py index 87886c6ee..74bfcfe6e 100644 --- a/tests/slack_sdk/oauth/state_store/test_async_sqlalchemy.py +++ b/tests/slack_sdk/oauth/state_store/test_async_sqlalchemy.py @@ -1,17 +1,25 @@ import asyncio +import os import unittest from tests.helpers import async_test from sqlalchemy.ext.asyncio import AsyncEngine, create_async_engine from slack_sdk.oauth.state_store.sqlalchemy import AsyncSQLAlchemyOAuthStateStore +database_url = os.environ.get("ASYNC_TEST_DATABASE_URL", "sqlite+aiosqlite:///:memory:") + + +def setUpModule(): + """Emit database configuration for CI visibility across builds.""" + print(f"\n[StateStore/AsyncSQLAlchemy] Database: {database_url}") + class TestSQLAlchemy(unittest.TestCase): engine: AsyncEngine @async_test async def setUp(self): - self.engine = create_async_engine("sqlite+aiosqlite:///:memory:") + self.engine = create_async_engine(database_url) self.store = AsyncSQLAlchemyOAuthStateStore(engine=self.engine, expiration_seconds=2) async with self.engine.begin() as conn: await conn.run_sync(self.store.metadata.create_all) @@ -36,3 +44,17 @@ async def test_expiration(self): await asyncio.sleep(3) result = await self.store.async_consume(state) self.assertFalse(result) + + @async_test + async def test_timezone_aware_datetime_compatibility(self): + # Issue a state (tests INSERT with timezone-aware datetime) + state = await self.store.async_issue() + self.assertIsNotNone(state) + + # Consume it immediately (tests WHERE clause comparison with timezone-aware datetime) + result = await self.store.async_consume(state) + self.assertTrue(result) + + # Second consume should fail (state already consumed) + result = await self.store.async_consume(state) + self.assertFalse(result) diff --git a/tests/slack_sdk/oauth/state_store/test_sqlalchemy.py b/tests/slack_sdk/oauth/state_store/test_sqlalchemy.py index 441400d60..1a2940a81 100644 --- a/tests/slack_sdk/oauth/state_store/test_sqlalchemy.py +++ b/tests/slack_sdk/oauth/state_store/test_sqlalchemy.py @@ -1,3 +1,4 @@ +import os import time import unittest @@ -6,12 +7,19 @@ from slack_sdk.oauth.state_store.sqlalchemy import SQLAlchemyOAuthStateStore +database_url = os.environ.get("TEST_DATABASE_URL", "sqlite:///:memory:") + + +def setUpModule(): + """Emit database configuration for CI visibility across builds.""" + print(f"\n[StateStore/SQLAlchemy] Database: {database_url}") + class TestSQLAlchemy(unittest.TestCase): engine: Engine def setUp(self): - self.engine = sqlalchemy.create_engine("sqlite:///:memory:") + self.engine = sqlalchemy.create_engine(database_url) self.store = SQLAlchemyOAuthStateStore(engine=self.engine, expiration_seconds=2) self.store.metadata.create_all(self.engine) @@ -31,3 +39,16 @@ def test_expiration(self): time.sleep(3) result = self.store.consume(state) self.assertFalse(result) + + def test_timezone_aware_datetime_compatibility(self): + # Issue a state (tests INSERT with timezone-aware datetime) + state = self.store.issue() + self.assertIsNotNone(state) + + # Consume it immediately (tests WHERE clause comparison with timezone-aware datetime) + result = self.store.consume(state) + self.assertTrue(result) + + # Second consume should fail (state already consumed) + result = self.store.consume(state) + self.assertFalse(result)