From 4b14ebf22e15f5e6efabd8746027827686f2b4d4 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 19 Feb 2026 15:10:36 +0200 Subject: [PATCH 1/7] fix: add composite index for pr_creators DISTINCT ON query timeout The contributors endpoint timed out (>60s) due to missing index for DISTINCT ON (repository, pr_number) ORDER BY created_at pattern. Adds partial composite index on webhooks(repository, pr_number, created_at) WHERE pr_number IS NOT NULL. Closes #68 --- .../20260219_0001_add_pr_creators_index.py | 46 +++++ backend/models.py | 7 + tests/test_models.py | 159 ++++++++++++++++++ 3 files changed, 212 insertions(+) create mode 100644 backend/migrations/versions/20260219_0001_add_pr_creators_index.py create mode 100644 tests/test_models.py diff --git a/backend/migrations/versions/20260219_0001_add_pr_creators_index.py b/backend/migrations/versions/20260219_0001_add_pr_creators_index.py new file mode 100644 index 0000000..fe6fc2e --- /dev/null +++ b/backend/migrations/versions/20260219_0001_add_pr_creators_index.py @@ -0,0 +1,46 @@ +"""Add composite index for pr_creators DISTINCT ON query. + +Revision ID: f5g6h7i8j9k0 +Revises: e4f5g6h7i8j9 +Create Date: 2026-02-19 00:01:00.000000 + +Adds a partial composite index to optimize the pr_creators CTE query pattern: + DISTINCT ON (repository, pr_number) ... ORDER BY repository, pr_number, created_at ASC + +This query was timing out (>60s) on the contributors endpoint due to full table sort. +The index enables PostgreSQL to satisfy DISTINCT ON via index scan instead of sort. +""" + +from alembic import op +from sqlalchemy import text + +# revision identifiers, used by Alembic. +revision = "f5g6h7i8j9k0" # pragma: allowlist secret +down_revision = "e4f5g6h7i8j9" # pragma: allowlist secret +branch_labels = None +depends_on = None + + +def upgrade() -> None: + """Create composite index for DISTINCT ON (repository, pr_number) queries. + + Uses CONCURRENTLY to avoid locking the table during index creation. + Must run outside a transaction (autocommit mode). + """ + conn = op.get_bind() + # CONCURRENTLY requires autocommit (no transaction) + with conn.execution_options(isolation_level="AUTOCOMMIT"): + conn.execute( + text( + """ + CREATE INDEX CONCURRENTLY IF NOT EXISTS ix_webhooks_repo_pr_number_created_at + ON webhooks (repository, pr_number, created_at ASC) + WHERE pr_number IS NOT NULL + """ + ) + ) + + +def downgrade() -> None: + """Drop the composite index.""" + op.drop_index("ix_webhooks_repo_pr_number_created_at", table_name="webhooks") diff --git a/backend/models.py b/backend/models.py index de72170..da75095 100644 --- a/backend/models.py +++ b/backend/models.py @@ -82,6 +82,13 @@ class Webhook(Base): __table_args__ = ( Index("ix_webhooks_repository_created_at", "repository", "created_at"), Index("ix_webhooks_repository_event_type", "repository", "event_type"), + Index( + "ix_webhooks_repo_pr_number_created_at", + "repository", + "pr_number", + "created_at", + postgresql_where=text("pr_number IS NOT NULL"), + ), ) id: Mapped[UUID] = mapped_column( diff --git a/tests/test_models.py b/tests/test_models.py new file mode 100644 index 0000000..5ca5243 --- /dev/null +++ b/tests/test_models.py @@ -0,0 +1,159 @@ +"""Tests for SQLAlchemy models and Alembic migrations. + +Tests model definitions including: +- Table arguments and index definitions +- Migration revision chain integrity +- Migration upgrade/downgrade operations +""" + +import importlib +import types +from unittest.mock import MagicMock, patch + +from sqlalchemy import Index + +from backend.models import Webhook + +INDEX_NAME = "ix_webhooks_repo_pr_number_created_at" + + +class TestWebhookCompositeIndex: + """Tests for the composite index on the Webhook model.""" + + def test_table_args_contains_composite_index(self) -> None: + """Test that Webhook.__table_args__ includes the composite index.""" + index_names = [arg.name for arg in Webhook.__table_args__ if isinstance(arg, Index)] + assert INDEX_NAME in index_names + + def test_composite_index_columns(self) -> None: + """Test that the composite index covers repository, pr_number, and created_at.""" + target_index = None + for arg in Webhook.__table_args__: + if isinstance(arg, Index) and arg.name == INDEX_NAME: + target_index = arg + break + + assert target_index is not None, f"Index {INDEX_NAME} not found in __table_args__" + + column_names = [col.name for col in target_index.columns] + assert column_names == ["repository", "pr_number", "created_at"] + + def test_composite_index_has_partial_where_clause(self) -> None: + """Test that the composite index has a WHERE pr_number IS NOT NULL clause.""" + target_index = None + for arg in Webhook.__table_args__: + if isinstance(arg, Index) and arg.name == INDEX_NAME: + target_index = arg + break + + assert target_index is not None, f"Index {INDEX_NAME} not found in __table_args__" + + dialect_options = target_index.dialect_options.get("postgresql", {}) + where_clause = dialect_options.get("where") + assert where_clause is not None, "Index missing postgresql_where clause" + assert "pr_number IS NOT NULL" in str(where_clause.text) + + def test_webhook_has_three_composite_indexes(self) -> None: + """Test that Webhook.__table_args__ has exactly three composite indexes.""" + indexes = [arg for arg in Webhook.__table_args__ if isinstance(arg, Index)] + assert len(indexes) == 3 + + def test_all_composite_index_names(self) -> None: + """Test all composite index names in Webhook.__table_args__.""" + expected_names = { + "ix_webhooks_repository_created_at", + "ix_webhooks_repository_event_type", + INDEX_NAME, + } + actual_names = {arg.name for arg in Webhook.__table_args__ if isinstance(arg, Index)} + assert actual_names == expected_names + + +class TestPrCreatorsIndexMigration: + """Tests for the 20260219_0001_add_pr_creators_index migration.""" + + @staticmethod + def _load_migration() -> types.ModuleType: + """Load the migration module.""" + return importlib.import_module("backend.migrations.versions.20260219_0001_add_pr_creators_index") + + def test_revision_id(self) -> None: + """Test that the migration has the expected revision ID.""" + migration = self._load_migration() + assert migration.revision == "f5g6h7i8j9k0" + + def test_down_revision_matches_previous_migration(self) -> None: + """Test that down_revision points to the previous migration.""" + migration = self._load_migration() + assert migration.down_revision == "e4f5g6h7i8j9" + + def test_revision_chain_links_to_remove_cross_team_columns(self) -> None: + """Test that the migration chains after the remove_cross_team_columns migration.""" + previous_migration = importlib.import_module( + "backend.migrations.versions.20251210_0001_remove_cross_team_columns" + ) + current_migration = self._load_migration() + assert current_migration.down_revision == previous_migration.revision + + def test_branch_labels_is_none(self) -> None: + """Test that branch_labels is None (linear migration chain).""" + migration = self._load_migration() + assert migration.branch_labels is None + + def test_depends_on_is_none(self) -> None: + """Test that depends_on is None (no cross-branch dependencies).""" + migration = self._load_migration() + assert migration.depends_on is None + + def test_upgrade_creates_correct_index(self) -> None: + """Test that upgrade creates the ix_webhooks_repo_pr_number_created_at index.""" + migration = self._load_migration() + + mock_bind = MagicMock() + + with patch.object(migration.op, "get_bind", return_value=mock_bind): + migration.upgrade() + + mock_bind.execution_options.assert_called_once_with(isolation_level="AUTOCOMMIT") + executed_sql = mock_bind.execute.call_args[0][0] + sql_text = str(executed_sql.text).strip() + assert INDEX_NAME in sql_text + assert "CONCURRENTLY" in sql_text + assert "IF NOT EXISTS" in sql_text + assert "pr_number IS NOT NULL" in sql_text + + def test_upgrade_targets_webhooks_table(self) -> None: + """Test that upgrade creates the index on the webhooks table.""" + migration = self._load_migration() + + mock_bind = MagicMock() + + with patch.object(migration.op, "get_bind", return_value=mock_bind): + migration.upgrade() + + executed_sql = str(mock_bind.execute.call_args[0][0].text).strip() + assert "ON webhooks" in executed_sql + + def test_upgrade_index_column_order(self) -> None: + """Test that the upgrade SQL specifies columns in the correct order.""" + migration = self._load_migration() + + mock_bind = MagicMock() + + with patch.object(migration.op, "get_bind", return_value=mock_bind): + migration.upgrade() + + executed_sql = str(mock_bind.execute.call_args[0][0].text).strip() + assert "repository, pr_number, created_at ASC" in executed_sql + + def test_downgrade_drops_correct_index(self) -> None: + """Test that downgrade drops the ix_webhooks_repo_pr_number_created_at index.""" + migration = self._load_migration() + + with patch.object(migration.op, "drop_index") as mock_drop: + migration.downgrade() + + mock_drop.assert_called_once_with( + INDEX_NAME, + table_name="webhooks", + ) From b7513b97d0a3de814cb0e3a5b5c36e9b7a47127f Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 19 Feb 2026 15:37:50 +0200 Subject: [PATCH 2/7] fix: address CodeRabbit review comments - Use DROP INDEX CONCURRENTLY in migration downgrade for consistency - Extract _find_index helper to reduce test duplication - Update downgrade test for CONCURRENTLY pattern --- .../20260219_0001_add_pr_creators_index.py | 17 +++++++-- tests/test_models.py | 35 +++++++++---------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/backend/migrations/versions/20260219_0001_add_pr_creators_index.py b/backend/migrations/versions/20260219_0001_add_pr_creators_index.py index fe6fc2e..fd9ccb5 100644 --- a/backend/migrations/versions/20260219_0001_add_pr_creators_index.py +++ b/backend/migrations/versions/20260219_0001_add_pr_creators_index.py @@ -42,5 +42,18 @@ def upgrade() -> None: def downgrade() -> None: - """Drop the composite index.""" - op.drop_index("ix_webhooks_repo_pr_number_created_at", table_name="webhooks") + """Drop the composite index. + + Uses CONCURRENTLY to avoid locking the table during index removal. + Must run outside a transaction (autocommit mode). + """ + conn = op.get_bind() + # CONCURRENTLY requires autocommit (no transaction) + with conn.execution_options(isolation_level="AUTOCOMMIT"): + conn.execute( + text( + """ + DROP INDEX CONCURRENTLY IF EXISTS ix_webhooks_repo_pr_number_created_at + """ + ) + ) diff --git a/tests/test_models.py b/tests/test_models.py index 5ca5243..816c54a 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -20,6 +20,13 @@ class TestWebhookCompositeIndex: """Tests for the composite index on the Webhook model.""" + @staticmethod + def _find_index(name: str) -> Index: + for arg in Webhook.__table_args__: + if isinstance(arg, Index) and arg.name == name: + return arg + raise AssertionError(f"Index {name} not found in __table_args__") + def test_table_args_contains_composite_index(self) -> None: """Test that Webhook.__table_args__ includes the composite index.""" index_names = [arg.name for arg in Webhook.__table_args__ if isinstance(arg, Index)] @@ -27,26 +34,14 @@ def test_table_args_contains_composite_index(self) -> None: def test_composite_index_columns(self) -> None: """Test that the composite index covers repository, pr_number, and created_at.""" - target_index = None - for arg in Webhook.__table_args__: - if isinstance(arg, Index) and arg.name == INDEX_NAME: - target_index = arg - break - - assert target_index is not None, f"Index {INDEX_NAME} not found in __table_args__" + target_index = self._find_index(INDEX_NAME) column_names = [col.name for col in target_index.columns] assert column_names == ["repository", "pr_number", "created_at"] def test_composite_index_has_partial_where_clause(self) -> None: """Test that the composite index has a WHERE pr_number IS NOT NULL clause.""" - target_index = None - for arg in Webhook.__table_args__: - if isinstance(arg, Index) and arg.name == INDEX_NAME: - target_index = arg - break - - assert target_index is not None, f"Index {INDEX_NAME} not found in __table_args__" + target_index = self._find_index(INDEX_NAME) dialect_options = target_index.dialect_options.get("postgresql", {}) where_clause = dialect_options.get("where") @@ -150,10 +145,12 @@ def test_downgrade_drops_correct_index(self) -> None: """Test that downgrade drops the ix_webhooks_repo_pr_number_created_at index.""" migration = self._load_migration() - with patch.object(migration.op, "drop_index") as mock_drop: + mock_bind = MagicMock() + + with patch.object(migration.op, "get_bind", return_value=mock_bind): migration.downgrade() - mock_drop.assert_called_once_with( - INDEX_NAME, - table_name="webhooks", - ) + mock_bind.execution_options.assert_called_once_with(isolation_level="AUTOCOMMIT") + executed_sql = str(mock_bind.execute.call_args[0][0].text).strip() + assert "DROP INDEX CONCURRENTLY" in executed_sql + assert INDEX_NAME in executed_sql From 30c1c4d5f5e1edaf8e64053c91fef85957ebd991 Mon Sep 17 00:00:00 2001 From: rnetser Date: Mon, 23 Feb 2026 12:33:29 +0200 Subject: [PATCH 3/7] fix: use explicit COMMIT instead of execution_options for CONCURRENTLY DDL Alembic's env.py wraps migrations in context.begin_transaction(). SQLAlchemy 2.0 rejects changing isolation_level on a connection with an active transaction, causing InvalidRequestError in production. Fix by committing Alembic's active transaction before running CREATE/DROP INDEX CONCURRENTLY, which cannot run inside a transaction. --- .../20260219_0001_add_pr_creators_index.py | 42 ++++++++++--------- tests/test_models.py | 28 ++++++++----- 2 files changed, 39 insertions(+), 31 deletions(-) diff --git a/backend/migrations/versions/20260219_0001_add_pr_creators_index.py b/backend/migrations/versions/20260219_0001_add_pr_creators_index.py index fd9ccb5..a52f12e 100644 --- a/backend/migrations/versions/20260219_0001_add_pr_creators_index.py +++ b/backend/migrations/versions/20260219_0001_add_pr_creators_index.py @@ -25,35 +25,37 @@ def upgrade() -> None: """Create composite index for DISTINCT ON (repository, pr_number) queries. Uses CONCURRENTLY to avoid locking the table during index creation. - Must run outside a transaction (autocommit mode). + CONCURRENTLY cannot run inside a transaction, so the active Alembic + transaction is committed first. """ conn = op.get_bind() - # CONCURRENTLY requires autocommit (no transaction) - with conn.execution_options(isolation_level="AUTOCOMMIT"): - conn.execute( - text( - """ - CREATE INDEX CONCURRENTLY IF NOT EXISTS ix_webhooks_repo_pr_number_created_at - ON webhooks (repository, pr_number, created_at ASC) - WHERE pr_number IS NOT NULL - """ - ) + # CONCURRENTLY cannot run inside a transaction — commit Alembic's active transaction + conn.execute(text("COMMIT")) + conn.execute( + text( + """ + CREATE INDEX CONCURRENTLY IF NOT EXISTS ix_webhooks_repo_pr_number_created_at + ON webhooks (repository, pr_number, created_at ASC) + WHERE pr_number IS NOT NULL + """ ) + ) def downgrade() -> None: """Drop the composite index. Uses CONCURRENTLY to avoid locking the table during index removal. - Must run outside a transaction (autocommit mode). + CONCURRENTLY cannot run inside a transaction, so the active Alembic + transaction is committed first. """ conn = op.get_bind() - # CONCURRENTLY requires autocommit (no transaction) - with conn.execution_options(isolation_level="AUTOCOMMIT"): - conn.execute( - text( - """ - DROP INDEX CONCURRENTLY IF EXISTS ix_webhooks_repo_pr_number_created_at - """ - ) + # CONCURRENTLY cannot run inside a transaction — commit Alembic's active transaction + conn.execute(text("COMMIT")) + conn.execute( + text( + """ + DROP INDEX CONCURRENTLY IF EXISTS ix_webhooks_repo_pr_number_created_at + """ ) + ) diff --git a/tests/test_models.py b/tests/test_models.py index 816c54a..9975afd 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -109,13 +109,14 @@ def test_upgrade_creates_correct_index(self) -> None: with patch.object(migration.op, "get_bind", return_value=mock_bind): migration.upgrade() - mock_bind.execution_options.assert_called_once_with(isolation_level="AUTOCOMMIT") - executed_sql = mock_bind.execute.call_args[0][0] - sql_text = str(executed_sql.text).strip() - assert INDEX_NAME in sql_text - assert "CONCURRENTLY" in sql_text - assert "IF NOT EXISTS" in sql_text - assert "pr_number IS NOT NULL" in sql_text + assert mock_bind.execute.call_count == 2 + commit_sql = str(mock_bind.execute.call_args_list[0][0][0].text).strip() + assert commit_sql == "COMMIT" + executed_sql = str(mock_bind.execute.call_args_list[1][0][0].text).strip() + assert INDEX_NAME in executed_sql + assert "CONCURRENTLY" in executed_sql + assert "IF NOT EXISTS" in executed_sql + assert "pr_number IS NOT NULL" in executed_sql def test_upgrade_targets_webhooks_table(self) -> None: """Test that upgrade creates the index on the webhooks table.""" @@ -126,7 +127,8 @@ def test_upgrade_targets_webhooks_table(self) -> None: with patch.object(migration.op, "get_bind", return_value=mock_bind): migration.upgrade() - executed_sql = str(mock_bind.execute.call_args[0][0].text).strip() + assert mock_bind.execute.call_count == 2 + executed_sql = str(mock_bind.execute.call_args_list[1][0][0].text).strip() assert "ON webhooks" in executed_sql def test_upgrade_index_column_order(self) -> None: @@ -138,7 +140,8 @@ def test_upgrade_index_column_order(self) -> None: with patch.object(migration.op, "get_bind", return_value=mock_bind): migration.upgrade() - executed_sql = str(mock_bind.execute.call_args[0][0].text).strip() + assert mock_bind.execute.call_count == 2 + executed_sql = str(mock_bind.execute.call_args_list[1][0][0].text).strip() assert "repository, pr_number, created_at ASC" in executed_sql def test_downgrade_drops_correct_index(self) -> None: @@ -150,7 +153,10 @@ def test_downgrade_drops_correct_index(self) -> None: with patch.object(migration.op, "get_bind", return_value=mock_bind): migration.downgrade() - mock_bind.execution_options.assert_called_once_with(isolation_level="AUTOCOMMIT") - executed_sql = str(mock_bind.execute.call_args[0][0].text).strip() + assert mock_bind.execute.call_count == 2 + commit_sql = str(mock_bind.execute.call_args_list[0][0][0].text).strip() + assert commit_sql == "COMMIT" + executed_sql = str(mock_bind.execute.call_args_list[1][0][0].text).strip() assert "DROP INDEX CONCURRENTLY" in executed_sql assert INDEX_NAME in executed_sql + assert "IF EXISTS" in executed_sql From 49b25df7a8a3de57d0cc3045192ae0f0dbc0f421 Mon Sep 17 00:00:00 2001 From: rnetser Date: Mon, 23 Feb 2026 12:45:14 +0200 Subject: [PATCH 4/7] refactor: extract shared helpers for migration upgrade/downgrade tests Extract _run_upgrade_and_get_sql() and _run_downgrade_and_get_sql() helpers to eliminate duplicated mock setup, COMMIT verification, and DDL extraction across four migration tests. --- tests/test_models.py | 56 ++++++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 33 deletions(-) diff --git a/tests/test_models.py b/tests/test_models.py index 9975afd..4ff6ca6 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -100,10 +100,9 @@ def test_depends_on_is_none(self) -> None: migration = self._load_migration() assert migration.depends_on is None - def test_upgrade_creates_correct_index(self) -> None: - """Test that upgrade creates the ix_webhooks_repo_pr_number_created_at index.""" + def _run_upgrade_and_get_sql(self) -> str: + """Run the upgrade migration and return the DDL SQL string.""" migration = self._load_migration() - mock_bind = MagicMock() with patch.object(migration.op, "get_bind", return_value=mock_bind): @@ -112,51 +111,42 @@ def test_upgrade_creates_correct_index(self) -> None: assert mock_bind.execute.call_count == 2 commit_sql = str(mock_bind.execute.call_args_list[0][0][0].text).strip() assert commit_sql == "COMMIT" - executed_sql = str(mock_bind.execute.call_args_list[1][0][0].text).strip() - assert INDEX_NAME in executed_sql - assert "CONCURRENTLY" in executed_sql - assert "IF NOT EXISTS" in executed_sql - assert "pr_number IS NOT NULL" in executed_sql + return str(mock_bind.execute.call_args_list[1][0][0].text).strip() - def test_upgrade_targets_webhooks_table(self) -> None: - """Test that upgrade creates the index on the webhooks table.""" + def _run_downgrade_and_get_sql(self) -> str: + """Run the downgrade migration and return the DDL SQL string.""" migration = self._load_migration() - mock_bind = MagicMock() with patch.object(migration.op, "get_bind", return_value=mock_bind): - migration.upgrade() + migration.downgrade() assert mock_bind.execute.call_count == 2 - executed_sql = str(mock_bind.execute.call_args_list[1][0][0].text).strip() + commit_sql = str(mock_bind.execute.call_args_list[0][0][0].text).strip() + assert commit_sql == "COMMIT" + return str(mock_bind.execute.call_args_list[1][0][0].text).strip() + + def test_upgrade_creates_correct_index(self) -> None: + """Test that upgrade creates the ix_webhooks_repo_pr_number_created_at index.""" + executed_sql = self._run_upgrade_and_get_sql() + assert INDEX_NAME in executed_sql + assert "CONCURRENTLY" in executed_sql + assert "IF NOT EXISTS" in executed_sql + assert "pr_number IS NOT NULL" in executed_sql + + def test_upgrade_targets_webhooks_table(self) -> None: + """Test that upgrade creates the index on the webhooks table.""" + executed_sql = self._run_upgrade_and_get_sql() assert "ON webhooks" in executed_sql def test_upgrade_index_column_order(self) -> None: """Test that the upgrade SQL specifies columns in the correct order.""" - migration = self._load_migration() - - mock_bind = MagicMock() - - with patch.object(migration.op, "get_bind", return_value=mock_bind): - migration.upgrade() - - assert mock_bind.execute.call_count == 2 - executed_sql = str(mock_bind.execute.call_args_list[1][0][0].text).strip() + executed_sql = self._run_upgrade_and_get_sql() assert "repository, pr_number, created_at ASC" in executed_sql def test_downgrade_drops_correct_index(self) -> None: """Test that downgrade drops the ix_webhooks_repo_pr_number_created_at index.""" - migration = self._load_migration() - - mock_bind = MagicMock() - - with patch.object(migration.op, "get_bind", return_value=mock_bind): - migration.downgrade() - - assert mock_bind.execute.call_count == 2 - commit_sql = str(mock_bind.execute.call_args_list[0][0][0].text).strip() - assert commit_sql == "COMMIT" - executed_sql = str(mock_bind.execute.call_args_list[1][0][0].text).strip() + executed_sql = self._run_downgrade_and_get_sql() assert "DROP INDEX CONCURRENTLY" in executed_sql assert INDEX_NAME in executed_sql assert "IF EXISTS" in executed_sql From e775d1f7d0c22e23c5e56169696b7cf26896d86e Mon Sep 17 00:00:00 2001 From: rnetser Date: Mon, 23 Feb 2026 12:52:31 +0200 Subject: [PATCH 5/7] fix: shorten AssertionError message to satisfy ruff TRY003 --- tests/test_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_models.py b/tests/test_models.py index 4ff6ca6..4635675 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -25,7 +25,7 @@ def _find_index(name: str) -> Index: for arg in Webhook.__table_args__: if isinstance(arg, Index) and arg.name == name: return arg - raise AssertionError(f"Index {name} not found in __table_args__") + raise AssertionError(f"Index {name!r} not found") def test_table_args_contains_composite_index(self) -> None: """Test that Webhook.__table_args__ includes the composite index.""" From da0fcd31610a188b3b89ff605f3a6a1b65e08f44 Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 24 Feb 2026 17:31:23 +0200 Subject: [PATCH 6/7] fix: address CodeRabbit review comments - Split test file into per-module files (test_models.py + test_migration_add_pr_creators_index.py) - Remove brittle index count test - Unify duplicate upgrade/downgrade test helpers - Add DROP INDEX before CREATE to handle invalid indexes from failed CONCURRENTLY builds - Add BEGIN after CONCURRENTLY DDL to restore Alembic transaction context - Use Literal type for migration action parameter - Use more robust alembic.op.get_bind patching --- .../20260219_0001_add_pr_creators_index.py | 12 ++ tests/test_migration_add_pr_creators_index.py | 108 ++++++++++++++++++ tests/test_models.py | 101 +--------------- 3 files changed, 121 insertions(+), 100 deletions(-) create mode 100644 tests/test_migration_add_pr_creators_index.py diff --git a/backend/migrations/versions/20260219_0001_add_pr_creators_index.py b/backend/migrations/versions/20260219_0001_add_pr_creators_index.py index a52f12e..8483119 100644 --- a/backend/migrations/versions/20260219_0001_add_pr_creators_index.py +++ b/backend/migrations/versions/20260219_0001_add_pr_creators_index.py @@ -31,6 +31,14 @@ def upgrade() -> None: conn = op.get_bind() # CONCURRENTLY cannot run inside a transaction — commit Alembic's active transaction conn.execute(text("COMMIT")) + # Drop any existing invalid index left by a prior failed CONCURRENTLY build + conn.execute( + text( + """ + DROP INDEX CONCURRENTLY IF EXISTS ix_webhooks_repo_pr_number_created_at + """ + ) + ) conn.execute( text( """ @@ -40,6 +48,8 @@ def upgrade() -> None: """ ) ) + # Re-open a transaction so Alembic can update alembic_version + conn.execute(text("BEGIN")) def downgrade() -> None: @@ -59,3 +69,5 @@ def downgrade() -> None: """ ) ) + # Re-open a transaction so Alembic can update alembic_version + conn.execute(text("BEGIN")) diff --git a/tests/test_migration_add_pr_creators_index.py b/tests/test_migration_add_pr_creators_index.py new file mode 100644 index 0000000..6c54357 --- /dev/null +++ b/tests/test_migration_add_pr_creators_index.py @@ -0,0 +1,108 @@ +"""Tests for the 20260219_0001_add_pr_creators_index migration. + +Tests migration revision chain integrity and upgrade/downgrade operations +for the pr_creators DISTINCT ON composite index. +""" + +import importlib +import types +from typing import Literal +from unittest.mock import MagicMock, patch + +INDEX_NAME = "ix_webhooks_repo_pr_number_created_at" + + +class TestPrCreatorsIndexMigration: + """Tests for the 20260219_0001_add_pr_creators_index migration.""" + + @staticmethod + def _load_migration() -> types.ModuleType: + """Load the migration module.""" + return importlib.import_module("backend.migrations.versions.20260219_0001_add_pr_creators_index") + + def _run_migration_and_get_sql( + self, action: Literal["upgrade", "downgrade"], expected_call_count: int, sql_index: int + ) -> str: + """Run a migration action and return the DDL SQL string. + + Args: + action: Migration action to run ("upgrade" or "downgrade"). + expected_call_count: Expected number of execute calls. + sql_index: Index of the DDL SQL call in the call args list. + + Returns: + The DDL SQL string at the specified index. + """ + migration = self._load_migration() + mock_bind = MagicMock() + + with patch("alembic.op.get_bind", return_value=mock_bind): + getattr(migration, action)() + + assert mock_bind.execute.call_count == expected_call_count + commit_sql = str(mock_bind.execute.call_args_list[0][0][0].text).strip() + assert commit_sql == "COMMIT" + begin_sql = str(mock_bind.execute.call_args_list[-1][0][0].text).strip() + assert begin_sql == "BEGIN" + return str(mock_bind.execute.call_args_list[sql_index][0][0].text).strip() + + def _run_upgrade_and_get_sql(self) -> str: + """Run the upgrade migration and return the DDL SQL string.""" + return self._run_migration_and_get_sql("upgrade", expected_call_count=4, sql_index=2) + + def _run_downgrade_and_get_sql(self) -> str: + """Run the downgrade migration and return the DDL SQL string.""" + return self._run_migration_and_get_sql("downgrade", expected_call_count=3, sql_index=1) + + def test_revision_id(self) -> None: + """Test that the migration has the expected revision ID.""" + migration = self._load_migration() + assert migration.revision == "f5g6h7i8j9k0" + + def test_down_revision_matches_previous_migration(self) -> None: + """Test that down_revision points to the previous migration.""" + migration = self._load_migration() + assert migration.down_revision == "e4f5g6h7i8j9" + + def test_revision_chain_links_to_remove_cross_team_columns(self) -> None: + """Test that the migration chains after the remove_cross_team_columns migration.""" + previous_migration = importlib.import_module( + "backend.migrations.versions.20251210_0001_remove_cross_team_columns" + ) + current_migration = self._load_migration() + assert current_migration.down_revision == previous_migration.revision + + def test_branch_labels_is_none(self) -> None: + """Test that branch_labels is None (linear migration chain).""" + migration = self._load_migration() + assert migration.branch_labels is None + + def test_depends_on_is_none(self) -> None: + """Test that depends_on is None (no cross-branch dependencies).""" + migration = self._load_migration() + assert migration.depends_on is None + + def test_upgrade_creates_correct_index(self) -> None: + """Test that upgrade creates the ix_webhooks_repo_pr_number_created_at index.""" + executed_sql = self._run_upgrade_and_get_sql() + assert INDEX_NAME in executed_sql + assert "CONCURRENTLY" in executed_sql + assert "IF NOT EXISTS" in executed_sql + assert "pr_number IS NOT NULL" in executed_sql + + def test_upgrade_targets_webhooks_table(self) -> None: + """Test that upgrade creates the index on the webhooks table.""" + executed_sql = self._run_upgrade_and_get_sql() + assert "ON webhooks" in executed_sql + + def test_upgrade_index_column_order(self) -> None: + """Test that the upgrade SQL specifies columns in the correct order.""" + executed_sql = self._run_upgrade_and_get_sql() + assert "repository, pr_number, created_at ASC" in executed_sql + + def test_downgrade_drops_correct_index(self) -> None: + """Test that downgrade drops the ix_webhooks_repo_pr_number_created_at index.""" + executed_sql = self._run_downgrade_and_get_sql() + assert "DROP INDEX CONCURRENTLY" in executed_sql + assert INDEX_NAME in executed_sql + assert "IF EXISTS" in executed_sql diff --git a/tests/test_models.py b/tests/test_models.py index 4635675..cefd9f1 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -1,15 +1,9 @@ -"""Tests for SQLAlchemy models and Alembic migrations. +"""Tests for the Webhook SQLAlchemy model. Tests model definitions including: - Table arguments and index definitions -- Migration revision chain integrity -- Migration upgrade/downgrade operations """ -import importlib -import types -from unittest.mock import MagicMock, patch - from sqlalchemy import Index from backend.models import Webhook @@ -48,11 +42,6 @@ def test_composite_index_has_partial_where_clause(self) -> None: assert where_clause is not None, "Index missing postgresql_where clause" assert "pr_number IS NOT NULL" in str(where_clause.text) - def test_webhook_has_three_composite_indexes(self) -> None: - """Test that Webhook.__table_args__ has exactly three composite indexes.""" - indexes = [arg for arg in Webhook.__table_args__ if isinstance(arg, Index)] - assert len(indexes) == 3 - def test_all_composite_index_names(self) -> None: """Test all composite index names in Webhook.__table_args__.""" expected_names = { @@ -62,91 +51,3 @@ def test_all_composite_index_names(self) -> None: } actual_names = {arg.name for arg in Webhook.__table_args__ if isinstance(arg, Index)} assert actual_names == expected_names - - -class TestPrCreatorsIndexMigration: - """Tests for the 20260219_0001_add_pr_creators_index migration.""" - - @staticmethod - def _load_migration() -> types.ModuleType: - """Load the migration module.""" - return importlib.import_module("backend.migrations.versions.20260219_0001_add_pr_creators_index") - - def test_revision_id(self) -> None: - """Test that the migration has the expected revision ID.""" - migration = self._load_migration() - assert migration.revision == "f5g6h7i8j9k0" - - def test_down_revision_matches_previous_migration(self) -> None: - """Test that down_revision points to the previous migration.""" - migration = self._load_migration() - assert migration.down_revision == "e4f5g6h7i8j9" - - def test_revision_chain_links_to_remove_cross_team_columns(self) -> None: - """Test that the migration chains after the remove_cross_team_columns migration.""" - previous_migration = importlib.import_module( - "backend.migrations.versions.20251210_0001_remove_cross_team_columns" - ) - current_migration = self._load_migration() - assert current_migration.down_revision == previous_migration.revision - - def test_branch_labels_is_none(self) -> None: - """Test that branch_labels is None (linear migration chain).""" - migration = self._load_migration() - assert migration.branch_labels is None - - def test_depends_on_is_none(self) -> None: - """Test that depends_on is None (no cross-branch dependencies).""" - migration = self._load_migration() - assert migration.depends_on is None - - def _run_upgrade_and_get_sql(self) -> str: - """Run the upgrade migration and return the DDL SQL string.""" - migration = self._load_migration() - mock_bind = MagicMock() - - with patch.object(migration.op, "get_bind", return_value=mock_bind): - migration.upgrade() - - assert mock_bind.execute.call_count == 2 - commit_sql = str(mock_bind.execute.call_args_list[0][0][0].text).strip() - assert commit_sql == "COMMIT" - return str(mock_bind.execute.call_args_list[1][0][0].text).strip() - - def _run_downgrade_and_get_sql(self) -> str: - """Run the downgrade migration and return the DDL SQL string.""" - migration = self._load_migration() - mock_bind = MagicMock() - - with patch.object(migration.op, "get_bind", return_value=mock_bind): - migration.downgrade() - - assert mock_bind.execute.call_count == 2 - commit_sql = str(mock_bind.execute.call_args_list[0][0][0].text).strip() - assert commit_sql == "COMMIT" - return str(mock_bind.execute.call_args_list[1][0][0].text).strip() - - def test_upgrade_creates_correct_index(self) -> None: - """Test that upgrade creates the ix_webhooks_repo_pr_number_created_at index.""" - executed_sql = self._run_upgrade_and_get_sql() - assert INDEX_NAME in executed_sql - assert "CONCURRENTLY" in executed_sql - assert "IF NOT EXISTS" in executed_sql - assert "pr_number IS NOT NULL" in executed_sql - - def test_upgrade_targets_webhooks_table(self) -> None: - """Test that upgrade creates the index on the webhooks table.""" - executed_sql = self._run_upgrade_and_get_sql() - assert "ON webhooks" in executed_sql - - def test_upgrade_index_column_order(self) -> None: - """Test that the upgrade SQL specifies columns in the correct order.""" - executed_sql = self._run_upgrade_and_get_sql() - assert "repository, pr_number, created_at ASC" in executed_sql - - def test_downgrade_drops_correct_index(self) -> None: - """Test that downgrade drops the ix_webhooks_repo_pr_number_created_at index.""" - executed_sql = self._run_downgrade_and_get_sql() - assert "DROP INDEX CONCURRENTLY" in executed_sql - assert INDEX_NAME in executed_sql - assert "IF EXISTS" in executed_sql From c68bad95dda608522c1a1478adf84cc0a851e3df Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 24 Feb 2026 18:15:17 +0200 Subject: [PATCH 7/7] fix: add test for upgrade defensive DROP INDEX cleanup --- tests/test_migration_add_pr_creators_index.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/test_migration_add_pr_creators_index.py b/tests/test_migration_add_pr_creators_index.py index 6c54357..07aa9ae 100644 --- a/tests/test_migration_add_pr_creators_index.py +++ b/tests/test_migration_add_pr_creators_index.py @@ -100,6 +100,13 @@ def test_upgrade_index_column_order(self) -> None: executed_sql = self._run_upgrade_and_get_sql() assert "repository, pr_number, created_at ASC" in executed_sql + def test_upgrade_drops_invalid_index_before_create(self) -> None: + """Test that upgrade drops any existing invalid index before creating.""" + drop_sql = self._run_migration_and_get_sql("upgrade", expected_call_count=4, sql_index=1) + assert "DROP INDEX CONCURRENTLY" in drop_sql + assert "IF EXISTS" in drop_sql + assert INDEX_NAME in drop_sql + def test_downgrade_drops_correct_index(self) -> None: """Test that downgrade drops the ix_webhooks_repo_pr_number_created_at index.""" executed_sql = self._run_downgrade_and_get_sql()