From 934cf96020db5487a83e3ff7b4e415f001c7a92d Mon Sep 17 00:00:00 2001 From: Dokujaa Date: Mon, 14 Jul 2025 22:17:59 -0400 Subject: [PATCH 1/2] Fix cache invalidation bug for Forge key scope updates - Fix cache key format mismatch in invalidate_forge_scope_cache() - Ensure cache invalidation uses same key format as cache setting - Add comprehensive unit tests for cache invalidation behavior Fixes #8 --- app/core/async_cache.py | 19 ++++ app/core/cache.py | 19 ++++ tests/unit_tests/test_cache_invalidation.py | 117 ++++++++++++++++++++ 3 files changed, 155 insertions(+) create mode 100644 tests/unit_tests/test_cache_invalidation.py diff --git a/app/core/async_cache.py b/app/core/async_cache.py index 43a88f0..d856e7c 100644 --- a/app/core/async_cache.py +++ b/app/core/async_cache.py @@ -183,6 +183,25 @@ async def invalidate_user_cache_async(api_key: str) -> None: await async_user_cache.delete(f"user:{api_key}") +async def invalidate_forge_scope_cache_async(api_key: str) -> None: + """Invalidate forge scope cache for a specific API key asynchronously""" + if not api_key: + return + + # The cache key format uses the API key WITHOUT the "forge-" prefix + # to match how it's set in get_user_by_api_key() + cache_key = api_key + if cache_key.startswith("forge-"): + cache_key = cache_key[6:] # Remove "forge-" prefix to match cache setting format + + await async_provider_service_cache.delete(f"forge_scope:{cache_key}") + + if DEBUG_CACHE: + # Mask the API key for logging + masked_key = cache_key[:8] + "..." if len(cache_key) > 8 else cache_key + logger.debug(f"Cache: Invalidated forge scope cache for API key: {masked_key} (async)") + + async def invalidate_user_cache_by_id_async(user_id: int) -> None: """Invalidate all cache entries for a specific user ID asynchronously""" if not user_id: diff --git a/app/core/cache.py b/app/core/cache.py index b02913f..4c1f8d5 100644 --- a/app/core/cache.py +++ b/app/core/cache.py @@ -170,6 +170,25 @@ def invalidate_user_cache(api_key: str) -> None: user_cache.delete(f"user:{api_key}") +def invalidate_forge_scope_cache(api_key: str) -> None: + """Invalidate forge scope cache for a specific API key""" + if not api_key: + return + + # The cache key format uses the API key WITHOUT the "forge-" prefix + # to match how it's set in get_user_by_api_key() + cache_key = api_key + if cache_key.startswith("forge-"): + cache_key = cache_key[6:] # Remove "forge-" prefix to match cache setting format + + provider_service_cache.delete(f"forge_scope:{cache_key}") + + if DEBUG_CACHE: + # Mask the API key for logging + masked_key = cache_key[:8] + "..." if len(cache_key) > 8 else cache_key + logger.debug(f"Cache: Invalidated forge scope cache for API key: {masked_key}") + + # Provider service functions def get_cached_provider_service(user_id: int) -> Any: """Get a provider service from cache by user ID""" diff --git a/tests/unit_tests/test_cache_invalidation.py b/tests/unit_tests/test_cache_invalidation.py new file mode 100644 index 0000000..56bf20a --- /dev/null +++ b/tests/unit_tests/test_cache_invalidation.py @@ -0,0 +1,117 @@ +""" +Unit tests for cache invalidation behavior when Forge API key scope is updated. + +Tests the fix for issue #8: Newly added provider not reflected in allowed provider list for Forge key +""" + +import pytest +from unittest.mock import MagicMock, patch +from app.core.cache import invalidate_forge_scope_cache +from app.core.async_cache import invalidate_forge_scope_cache_async +import asyncio + + +class TestForgeKeyCacheInvalidation: + """Test cache invalidation for Forge API keys""" + + def test_invalidate_forge_scope_cache_with_prefix(self): + """Test that cache invalidation works correctly with forge- prefix""" + # Mock the provider_service_cache + with patch('app.core.cache.provider_service_cache') as mock_cache: + # Test with full API key (including forge- prefix) + full_api_key = "forge-abc123def456" + + invalidate_forge_scope_cache(full_api_key) + + # Should strip the "forge-" prefix when creating cache key + expected_cache_key = "forge_scope:abc123def456" + mock_cache.delete.assert_called_once_with(expected_cache_key) + + def test_invalidate_forge_scope_cache_without_prefix(self): + """Test that cache invalidation works correctly without forge- prefix""" + # Mock the provider_service_cache + with patch('app.core.cache.provider_service_cache') as mock_cache: + # Test with stripped API key (without forge- prefix) + stripped_api_key = "abc123def456" + + invalidate_forge_scope_cache(stripped_api_key) + + # Should use the key as-is when creating cache key + expected_cache_key = "forge_scope:abc123def456" + mock_cache.delete.assert_called_once_with(expected_cache_key) + + def test_invalidate_forge_scope_cache_empty_key(self): + """Test that cache invalidation handles empty keys gracefully""" + # Mock the provider_service_cache + with patch('app.core.cache.provider_service_cache') as mock_cache: + # Test with empty API key + invalidate_forge_scope_cache("") + + # Should not call delete for empty keys + mock_cache.delete.assert_not_called() + + def test_invalidate_forge_scope_cache_none_key(self): + """Test that cache invalidation handles None keys gracefully""" + # Mock the provider_service_cache + with patch('app.core.cache.provider_service_cache') as mock_cache: + # Test with None API key + invalidate_forge_scope_cache(None) + + # Should not call delete for None keys + mock_cache.delete.assert_not_called() + + @pytest.mark.asyncio + async def test_invalidate_forge_scope_cache_async_with_prefix(self): + """Test that async cache invalidation works correctly with forge- prefix""" + # Mock the async_provider_service_cache + with patch('app.core.async_cache.async_provider_service_cache') as mock_cache: + from unittest.mock import AsyncMock + mock_cache.delete = AsyncMock() + + # Test with full API key (including forge- prefix) + full_api_key = "forge-abc123def456" + + await invalidate_forge_scope_cache_async(full_api_key) + + # Should strip the "forge-" prefix when creating cache key + expected_cache_key = "forge_scope:abc123def456" + mock_cache.delete.assert_called_once_with(expected_cache_key) + + @pytest.mark.asyncio + async def test_invalidate_forge_scope_cache_async_without_prefix(self): + """Test that async cache invalidation works correctly without forge- prefix""" + # Mock the async_provider_service_cache + with patch('app.core.async_cache.async_provider_service_cache') as mock_cache: + from unittest.mock import AsyncMock + mock_cache.delete = AsyncMock() + + # Test with stripped API key (without forge- prefix) + stripped_api_key = "abc123def456" + + await invalidate_forge_scope_cache_async(stripped_api_key) + + # Should use the key as-is when creating cache key + expected_cache_key = "forge_scope:abc123def456" + mock_cache.delete.assert_called_once_with(expected_cache_key) + + def test_cache_key_format_consistency(self): + """Test that cache invalidation uses the same key format as cache setting""" + # This test verifies the fix for issue #8 + # The bug was that cache was set with stripped key but invalidated with full key + + with patch('app.core.cache.provider_service_cache') as mock_cache: + # Simulate the DB key format (with forge- prefix) + db_api_key = "forge-d8fc7c26e350771b28fe94b7" + + # When we invalidate using the DB key + invalidate_forge_scope_cache(db_api_key) + + # It should create the same cache key format used by get_user_by_api_key + # which strips the forge- prefix: api_key = api_key_from_header[6:] + stripped_key = db_api_key[6:] # Remove "forge-" prefix + expected_cache_key = f"forge_scope:{stripped_key}" + + mock_cache.delete.assert_called_once_with(expected_cache_key) + + # Verify the exact cache key format + assert expected_cache_key == "forge_scope:d8fc7c26e350771b28fe94b7" \ No newline at end of file From 9fd50a4b02ff59090e3fa477801bebe0968822cd Mon Sep 17 00:00:00 2001 From: Dokujaa Date: Mon, 14 Jul 2025 22:25:04 -0400 Subject: [PATCH 2/2] Fix cache invalidation bug for Forge key scope updates - Fix cache key format mismatch in invalidate_forge_scope_cache() - Apply same fix to async version - Add comprehensive unit tests with 7 test cases - Follow project code style guidelines Fixes #8 --- app/api/routes/api_keys.py | 8 +++++++- app/core/async_cache.py | 6 +++++- app/core/cache.py | 6 +++++- tests/unit_tests/test_cache_invalidation.py | 10 +++++----- 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/app/api/routes/api_keys.py b/app/api/routes/api_keys.py index 8be9d6a..b6b8c9b 100644 --- a/app/api/routes/api_keys.py +++ b/app/api/routes/api_keys.py @@ -13,7 +13,7 @@ ForgeApiKeyResponse, ForgeApiKeyUpdate, ) -from app.core.cache import invalidate_provider_service_cache, invalidate_user_cache +from app.core.cache import invalidate_provider_service_cache, invalidate_user_cache, invalidate_forge_scope_cache from app.core.database import get_db from app.core.security import generate_forge_api_key from app.models.forge_api_key import ForgeApiKey @@ -137,6 +137,10 @@ def _update_api_key_internal( db.commit() db.refresh(db_api_key) + # Invalidate forge scope cache if the scope was updated + if api_key_update.allowed_provider_key_ids is not None: + invalidate_forge_scope_cache(db_api_key.key) + response_data = db_api_key.__dict__.copy() response_data["allowed_provider_key_ids"] = [ pk.id for pk in db_api_key.allowed_provider_keys @@ -174,6 +178,7 @@ def _delete_api_key_internal( db.commit() invalidate_user_cache(key_to_invalidate) + invalidate_forge_scope_cache(key_to_invalidate) invalidate_provider_service_cache(current_user.id) return ForgeApiKeyResponse(**response_data) @@ -195,6 +200,7 @@ def _regenerate_api_key_internal( # Invalidate caches for the old key old_key = db_api_key.key invalidate_user_cache(old_key) + invalidate_forge_scope_cache(old_key) invalidate_provider_service_cache(current_user.id) # Generate and set new key diff --git a/app/core/async_cache.py b/app/core/async_cache.py index d856e7c..c08e222 100644 --- a/app/core/async_cache.py +++ b/app/core/async_cache.py @@ -184,7 +184,11 @@ async def invalidate_user_cache_async(api_key: str) -> None: async def invalidate_forge_scope_cache_async(api_key: str) -> None: - """Invalidate forge scope cache for a specific API key asynchronously""" + """Invalidate forge scope cache for a specific API key asynchronously. + + Args: + api_key (str): The API key to invalidate cache for. Can include or exclude 'forge-' prefix. + """ if not api_key: return diff --git a/app/core/cache.py b/app/core/cache.py index 4c1f8d5..01e4d15 100644 --- a/app/core/cache.py +++ b/app/core/cache.py @@ -171,7 +171,11 @@ def invalidate_user_cache(api_key: str) -> None: def invalidate_forge_scope_cache(api_key: str) -> None: - """Invalidate forge scope cache for a specific API key""" + """Invalidate forge scope cache for a specific API key. + + Args: + api_key (str): The API key to invalidate cache for. Can include or exclude 'forge-' prefix. + """ if not api_key: return diff --git a/tests/unit_tests/test_cache_invalidation.py b/tests/unit_tests/test_cache_invalidation.py index 56bf20a..6b6ac59 100644 --- a/tests/unit_tests/test_cache_invalidation.py +++ b/tests/unit_tests/test_cache_invalidation.py @@ -4,11 +4,13 @@ Tests the fix for issue #8: Newly added provider not reflected in allowed provider list for Forge key """ +import asyncio +from unittest.mock import AsyncMock, MagicMock, patch + import pytest -from unittest.mock import MagicMock, patch -from app.core.cache import invalidate_forge_scope_cache + from app.core.async_cache import invalidate_forge_scope_cache_async -import asyncio +from app.core.cache import invalidate_forge_scope_cache class TestForgeKeyCacheInvalidation: @@ -65,7 +67,6 @@ async def test_invalidate_forge_scope_cache_async_with_prefix(self): """Test that async cache invalidation works correctly with forge- prefix""" # Mock the async_provider_service_cache with patch('app.core.async_cache.async_provider_service_cache') as mock_cache: - from unittest.mock import AsyncMock mock_cache.delete = AsyncMock() # Test with full API key (including forge- prefix) @@ -82,7 +83,6 @@ async def test_invalidate_forge_scope_cache_async_without_prefix(self): """Test that async cache invalidation works correctly without forge- prefix""" # Mock the async_provider_service_cache with patch('app.core.async_cache.async_provider_service_cache') as mock_cache: - from unittest.mock import AsyncMock mock_cache.delete = AsyncMock() # Test with stripped API key (without forge- prefix)