From 7b494753eb3632fed525e904c9e1a55b92f9f4c5 Mon Sep 17 00:00:00 2001 From: Cody Fincher Date: Fri, 10 Oct 2025 19:31:04 +0000 Subject: [PATCH] fix(migrations): eliminate duplicate SQL file loading Fixes #118 ## Summary Removes unnecessary `clear_cache()` calls that were causing SQL files to be loaded and parsed 3 times during a single migration operation. ## Root Cause SQLFileLoader.clear_cache() was being called before every migration operation, destroying CoreSQLFileLoader's internal cache (_files, _queries, _query_to_file). This forced files to be re-parsed multiple times. ## Changes - Removed clear_cache() from SQLFileLoader.get_up_sql() - Removed clear_cache() from SQLFileLoader.get_down_sql() - Removed clear_cache() from SQLFileLoader.validate_migration_file() - Removed clear_cache() from SyncMigrationRunner.load_migration() - Removed clear_cache() from AsyncMigrationRunner.load_migration() ## Impact - Files now loaded once per migration operation (down from 3x) - All 126 tests passing ## Testing - Added test_sql_loader_caches_files to verify cache persistence - Verified all existing migration tests pass - No functional changes or breaking changes --- sqlspec/migrations/loaders.py | 3 -- sqlspec/migrations/runner.py | 2 - .../test_migrations/test_migration_runner.py | 38 +++++++++++++++++++ 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/sqlspec/migrations/loaders.py b/sqlspec/migrations/loaders.py index d3a9f652..1114038d 100644 --- a/sqlspec/migrations/loaders.py +++ b/sqlspec/migrations/loaders.py @@ -93,7 +93,6 @@ async def get_up_sql(self, path: Path) -> list[str]: Raises: MigrationLoadError: If migration file is invalid or missing up query. """ - self.sql_loader.clear_cache() self.sql_loader.load_sql(path) version = self._extract_version(path.name) @@ -115,7 +114,6 @@ async def get_down_sql(self, path: Path) -> list[str]: Returns: List containing single SQL statement for downgrade, or empty list. """ - self.sql_loader.clear_cache() self.sql_loader.load_sql(path) version = self._extract_version(path.name) @@ -141,7 +139,6 @@ def validate_migration_file(self, path: Path) -> None: msg = f"Invalid migration filename: {path.name}" raise MigrationLoadError(msg) - self.sql_loader.clear_cache() self.sql_loader.load_sql(path) up_query = f"migrate-{version}-up" if not self.sql_loader.has_query(up_query): diff --git a/sqlspec/migrations/runner.py b/sqlspec/migrations/runner.py index 5711206c..5e075f2e 100644 --- a/sqlspec/migrations/runner.py +++ b/sqlspec/migrations/runner.py @@ -228,7 +228,6 @@ def load_migration(self, file_path: Path, version: "str | None" = None) -> "dict if file_path.suffix == ".sql": version = metadata["version"] up_query, down_query = f"migrate-{version}-up", f"migrate-{version}-down" - self.loader.clear_cache() self.loader.load_sql(file_path) has_upgrade, has_downgrade = self.loader.has_query(up_query), self.loader.has_query(down_query) else: @@ -394,7 +393,6 @@ async def load_migration(self, file_path: Path, version: "str | None" = None) -> if file_path.suffix == ".sql": version = metadata["version"] up_query, down_query = f"migrate-{version}-up", f"migrate-{version}-down" - self.loader.clear_cache() await async_(self.loader.load_sql)(file_path) has_upgrade, has_downgrade = self.loader.has_query(up_query), self.loader.has_query(down_query) else: diff --git a/tests/unit/test_migrations/test_migration_runner.py b/tests/unit/test_migrations/test_migration_runner.py index a0370f4d..e9cd97fe 100644 --- a/tests/unit/test_migrations/test_migration_runner.py +++ b/tests/unit/test_migrations/test_migration_runner.py @@ -538,3 +538,41 @@ def test_many_migration_files_performance() -> None: for i, (version, _) in enumerate(files): expected_version = f"{i + 1:04d}" assert version == expected_version + + +def test_sql_loader_caches_files() -> None: + """Test that SQL migration files leverage CoreSQLFileLoader caching. + + Verifies fix for bug #118 - duplicate SQL loading during migrations. + The SQLFileLoader should NOT call clear_cache() before operations, + allowing CoreSQLFileLoader's internal caching to work properly. + """ + import asyncio + + from sqlspec.migrations.loaders import SQLFileLoader + + with tempfile.TemporaryDirectory() as temp_dir: + migrations_path = Path(temp_dir) + + migration_file = migrations_path / "0001_test_migration.sql" + migration_content = """ +-- name: migrate-0001-up +CREATE TABLE test (id INTEGER PRIMARY KEY); + +-- name: migrate-0001-down +DROP TABLE test; +""" + migration_file.write_text(migration_content) + + sql_loader = SQLFileLoader() + + async def test_operations() -> None: + await sql_loader.get_up_sql(migration_file) + path_str = str(migration_file) + assert path_str in sql_loader.sql_loader._files + assert sql_loader.sql_loader.has_query("migrate-0001-up") + assert sql_loader.sql_loader.has_query("migrate-0001-down") + await sql_loader.get_down_sql(migration_file) + assert path_str in sql_loader.sql_loader._files + + asyncio.run(test_operations())