Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions .github/workflows/ci-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,63 @@ jobs:
token: ${{ secrets.CODECOV_TOKEN }}
verbose: true

databases:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This new job spins up a postgrrrresql db that can then be used to run our unit tests against, it should prevent this type of issue from coming up again 🚀

I tried adding mysql as well but it was failing, I'll it to my TODOs as a follow up PR

# 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
needs:
- lint
- typecheck
- unittest
- databases
if: ${{ !success() && github.ref == 'refs/heads/main' && github.event_name != 'workflow_dispatch' }}
steps:
- name: Send notifications of failing tests
Expand Down
5 changes: 5 additions & 0 deletions requirements/databases.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Database drivers for CI testing

# PostgreSQL drivers
psycopg2-binary>=2.9,<3
asyncpg>=0.27,<1
11 changes: 11 additions & 0 deletions slack_sdk/oauth/installation_store/sqlalchemy/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
33 changes: 33 additions & 0 deletions slack_sdk/oauth/sqlalchemy_utils/__init__.py
Original file line number Diff line number Diff line change
@@ -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)
Comment on lines +31 to +32
Copy link
Member

Choose a reason for hiding this comment

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

🪬 question(non-blocking): Should we check that tzinfo is UTC before removing it? Or converting this first? I haven't explored SQLAlchemy enough but timezones confuse me sincere...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So when the tzinfo is not provided SQLAlchemy sets the type as "datetime timezone unaware" and uses utc as the default

If we set tzinfo=utc then SQLAlchemy sets the type as "datetime timezone aware" which creates a breaking change with POSTGRESQL

return dt
11 changes: 7 additions & 4 deletions slack_sdk/oauth/state_store/sqlalchemy/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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(),
Expand All @@ -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}")
Expand Down Expand Up @@ -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(),
Expand All @@ -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}")
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,25 @@
import os
import unittest
from tests.helpers import async_test
from sqlalchemy.ext.asyncio import AsyncEngine, create_async_engine

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)
Expand Down Expand Up @@ -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)
33 changes: 32 additions & 1 deletion tests/slack_sdk/oauth/installation_store/test_sqlalchemy.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import os
import unittest

import sqlalchemy
Expand All @@ -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)

Expand Down Expand Up @@ -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)
24 changes: 23 additions & 1 deletion tests/slack_sdk/oauth/state_store/test_async_sqlalchemy.py
Original file line number Diff line number Diff line change
@@ -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)
Expand 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)
23 changes: 22 additions & 1 deletion tests/slack_sdk/oauth/state_store/test_sqlalchemy.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import os
import time
import unittest

Expand All @@ -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)

Expand All @@ -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)