Skip to content

Commit 2d129ad

Browse files
vdusekclaude
andauthored
fix: use load sentinel and injected config in AliasResolver (#874)
## Summary `AliasResolver` had two consistency issues: - **Empty-dict sentinel.** `if not cls._alias_map` conflated an empty dict with 'not yet loaded', so every alias lookup re-hit the default KVS with `get_record` until the first alias was persisted. - **Mixed configuration sources.** `is_at_home` was read from the global `Configuration` while the KVS client was built from `self._configuration`, so a caller-injected config could silently skip writes or target the wrong KVS. ## Fix - Add a `_alias_map_loaded` sentinel so an empty KVS response counts as 'loaded'. - Use the injected configuration consistently for both `is_at_home` gating and KVS client construction. - Add regression tests for both paths. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent ed6eb80 commit 2d129ad

3 files changed

Lines changed: 45 additions & 3 deletions

File tree

src/apify/storage_clients/_apify/_alias_resolving.py

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
from apify_client import ApifyClientAsync
1010

1111
from ._utils import hash_api_base_url_and_token
12-
from apify._configuration import Configuration
1312

1413
if TYPE_CHECKING:
1514
from collections.abc import Callable
@@ -24,6 +23,8 @@
2423
RequestQueueCollectionClientAsync,
2524
)
2625

26+
from apify._configuration import Configuration
27+
2728
logger = getLogger(__name__)
2829

2930

@@ -128,6 +129,9 @@ class AliasResolver:
128129
_alias_map: ClassVar[dict[str, str]] = {}
129130
"""Map containing pre-existing alias storages and their ids. Global for all instances."""
130131

132+
_alias_map_loaded: ClassVar[bool] = False
133+
"""Tracks whether `_alias_map` was fetched from the default KVS — an empty map is a valid loaded state."""
134+
131135
_alias_init_lock: Lock | None = None
132136
"""Lock for creating alias storages. Only one alias storage can be created at the time. Global for all instances."""
133137

@@ -181,11 +185,12 @@ async def _get_alias_map(cls, configuration: Configuration) -> dict[str, str]:
181185
Returns:
182186
Map of aliases and storage ids.
183187
"""
184-
if not cls._alias_map and Configuration.get_global_configuration().is_at_home:
188+
if not cls._alias_map_loaded and configuration.is_at_home:
185189
default_kvs_client = await cls._get_default_kvs_client(configuration)
186190

187191
record = await default_kvs_client.get_record(cls._ALIAS_MAPPING_KEY)
188192
cls._alias_map = record.get('value', {}) if record else {}
193+
cls._alias_map_loaded = True
189194

190195
return cls._alias_map
191196

@@ -215,7 +220,7 @@ async def store_mapping(self, storage_id: str) -> None:
215220
alias_map = await self._get_alias_map(self._configuration)
216221
alias_map[self._storage_key] = storage_id
217222

218-
if not Configuration.get_global_configuration().is_at_home:
223+
if not self._configuration.is_at_home:
219224
logging.getLogger(__name__).debug(
220225
'_AliasResolver storage limited retention is only supported on Apify platform. Storage is not exported.'
221226
)

tests/unit/conftest.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ def _prepare_test_env() -> None:
7575

7676
# Reset the AliasResolver class state.
7777
AliasResolver._alias_map = {}
78+
AliasResolver._alias_map_loaded = False
7879
AliasResolver._alias_init_lock = None
7980

8081
# Verify that the test environment was set up correctly.

tests/unit/storage_clients/test_alias_resolver.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
from __future__ import annotations
22

3+
from unittest.mock import AsyncMock, patch
4+
35
from apify._configuration import Configuration
46
from apify.storage_clients._apify._alias_resolving import AliasResolver
57

@@ -78,6 +80,40 @@ async def test_get_alias_map_returns_in_memory_map() -> None:
7880
assert result == {}
7981

8082

83+
async def test_get_alias_map_loads_from_kvs_only_once_when_empty() -> None:
84+
"""An empty KVS response must not trigger repeat fetches on subsequent calls."""
85+
config = Configuration(is_at_home=True, token='test-token', default_key_value_store_id='default-kvs-id')
86+
87+
fake_kvs_client = AsyncMock()
88+
fake_kvs_client.get_record = AsyncMock(return_value=None)
89+
90+
with patch.object(AliasResolver, '_get_default_kvs_client', return_value=fake_kvs_client):
91+
await AliasResolver._get_alias_map(config)
92+
await AliasResolver._get_alias_map(config)
93+
await AliasResolver._get_alias_map(config)
94+
95+
assert fake_kvs_client.get_record.await_count == 1
96+
assert AliasResolver._alias_map == {}
97+
98+
99+
async def test_store_mapping_uses_injected_configuration_is_at_home() -> None:
100+
"""`store_mapping` gates on the injected configuration's `is_at_home`, not the global one."""
101+
# Global `is_at_home` defaults to False; injected config says True — the KVS write must still happen.
102+
config = Configuration(is_at_home=True, token='test-token', default_key_value_store_id='default-kvs-id')
103+
resolver = AliasResolver(storage_type='Dataset', alias='test-alias', configuration=config)
104+
105+
fake_kvs_client = AsyncMock()
106+
fake_kvs_client.get_record = AsyncMock(return_value=None)
107+
fake_kvs_client.set_record = AsyncMock(return_value=None)
108+
fake_kvs_client.get = AsyncMock(return_value={'id': 'default-kvs-id'})
109+
110+
with patch.object(AliasResolver, '_get_default_kvs_client', return_value=fake_kvs_client):
111+
await resolver.store_mapping(storage_id='new-id-789')
112+
113+
fake_kvs_client.set_record.assert_awaited_once()
114+
assert AliasResolver._alias_map[resolver._storage_key] == 'new-id-789'
115+
116+
81117
async def test_configuration_storages_alias_resolving() -> None:
82118
"""Test that `AliasResolver` correctly resolves ids of storages registered in Configuration."""
83119

0 commit comments

Comments
 (0)