Review comments

Author: Pijukatel

src/apify/storage_clients/_apify/_request_queue_client.py

@@ -69,7 +69,12 @@ def __init__(
         """Fetch lock to minimize race conditions when communicating with API."""
 
     async def _get_metadata(self) -> RequestQueueMetadata:
-        """Try to get cached metadata first. If multiple clients, fuse with global metadata."""
+        """Try to get cached metadata first. If multiple clients, fuse with global metadata.
+
+        This method is used internally to avoid unnecessary API call unless needed (multiple clients).
+        Local estimation of metadata is without delay, unlike metadata from API. In situation where there is only one
+        client, it is the better choice.
+        """
         if self._metadata.had_multiple_clients:
             return await self.get_metadata()
         # Get local estimation (will not include changes done bo another client)

tests/integration/conftest.py

@@ -98,11 +98,6 @@ def apify_token() -> str:
     return api_token
 
 
-@pytest.fixture(autouse=True)
-def set_token(apify_token: str, monkeypatch: pytest.MonkeyPatch) -> None:
-    monkeypatch.setenv(ApifyEnvVars.TOKEN, apify_token)
-
-
 @pytest.fixture
 def apify_client_async(apify_token: str) -> ApifyClientAsync:
     """Create an instance of the ApifyClientAsync.
@@ -117,9 +112,10 @@ def apify_client_async(apify_token: str) -> ApifyClientAsync:
 
 
 @pytest.fixture
-async def request_queue_force_cloud() -> AsyncGenerator[RequestQueue]:
+async def request_queue_force_cloud(apify_token: str, monkeypatch: pytest.MonkeyPatch) -> AsyncGenerator[RequestQueue]:
     """Create an instance of the Apify request queue on the platform and drop it when the test is finished."""
     request_queue_name = generate_unique_resource_name('request_queue')
+    monkeypatch.setenv(ApifyEnvVars.TOKEN, apify_token)
 
     async with Actor:
         rq = await Actor.open_request_queue(name=request_queue_name, force_cloud=True)

tests/integration/test_actor_request_queue.py

@@ -318,86 +318,6 @@ def return_unprocessed_requests(requests: list[dict], *_: Any, **__: Any) -> dic
     assert (stats_after['writeCount'] - stats_before['writeCount']) == 1
 
 
-async def test_request_queue_enhanced_metadata(
-    request_queue_force_cloud: RequestQueue,
-    apify_client_async: ApifyClientAsync,
-) -> None:
-    """Test metadata tracking.
-
-    Multiple clients scenarios are not guaranteed to give correct results without delay. But at least multiple clients,
-    single producer, should be reliable on the producer side."""
-
-    for i in range(1, 10):
-        await request_queue_force_cloud.add_request(Request.from_url(f'http://example.com/{i}'))
-        # Reliable information as the API response is enhanced with local metadata estimation.
-        assert (await request_queue_force_cloud.get_metadata()).total_request_count == i
-
-    # Accessed with client created explicitly with `client_key=None` should appear as distinct client
-    api_client = apify_client_async.request_queue(request_queue_id=request_queue_force_cloud.id, client_key=None)
-    await api_client.list_head()
-
-    # The presence of another non-producing client should not affect the metadata
-    for i in range(10, 20):
-        await request_queue_force_cloud.add_request(Request.from_url(f'http://example.com/{i}'))
-        # Reliable information as the API response is enhanced with local metadata estimation.
-        assert (await request_queue_force_cloud.get_metadata()).total_request_count == i
-
-
-async def test_request_queue_metadata_another_client(
-    request_queue_force_cloud: RequestQueue,
-    apify_client_async: ApifyClientAsync,
-) -> None:
-    """Test metadata tracking. The delayed metadata should be reliable even when changed by another client."""
-    api_client = apify_client_async.request_queue(request_queue_id=request_queue_force_cloud.id, client_key=None)
-    await api_client.add_request(Request.from_url('http://example.com/1').model_dump(by_alias=True, exclude={'id'}))
-
-    # Wait to be sure that the API has updated the global metadata
-    await asyncio.sleep(10)
-
-    assert (await request_queue_force_cloud.get_metadata()).total_request_count == 1
-
-
-async def test_request_queue_had_multiple_clients_local(
-    request_queue_force_cloud: RequestQueue,
-    apify_client_async: ApifyClientAsync,
-) -> None:
-    """Test that `RequestQueue` correctly detects multiple clients.
-
-    Clients created with different `client_key` should appear as distinct clients."""
-    await request_queue_force_cloud.fetch_next_request()
-
-    # Accessed with client created explicitly with `client_key=None` should appear as distinct client
-    api_client = apify_client_async.request_queue(request_queue_id=request_queue_force_cloud.id, client_key=None)
-    await api_client.list_head()
-
-    # Check that it is correctly in the RequestQueueClient metadata
-    assert (await request_queue_force_cloud.get_metadata()).had_multiple_clients is True
-
-    # Check that it is correctly in the API
-    api_response = await api_client.get()
-    assert api_response
-    assert api_response['hadMultipleClients'] is True
-
-
-async def test_request_queue_not_had_multiple_clients_local(
-    request_queue_force_cloud: RequestQueue, apify_client_async: ApifyClientAsync
-) -> None:
-    """Test that same `RequestQueue` created from Actor does not act as multiple clients."""
-
-    # Two calls to API to create situation where different `client_key` can set `had_multiple_clients` to True
-    await request_queue_force_cloud.fetch_next_request()
-    await request_queue_force_cloud.fetch_next_request()
-
-    # Check that it is correctly in the RequestQueueClient metadata
-    assert (await request_queue_force_cloud.get_metadata()).had_multiple_clients is False
-
-    # Check that it is correctly in the API
-    api_client = apify_client_async.request_queue(request_queue_id=request_queue_force_cloud.id)
-    api_response = await api_client.get()
-    assert api_response
-    assert api_response['hadMultipleClients'] is False
-
-
 async def test_request_queue_had_multiple_clients_platform(
     make_actor: MakeActorFunction,
     run_actor: RunActorFunction,

tests/integration/test_request_queue.py

@@ -1,12 +1,18 @@
 from __future__ import annotations
 
+import asyncio
 from typing import TYPE_CHECKING
 
 import pytest
 
+from crawlee import Request
+
 from apify import Actor
 
 if TYPE_CHECKING:
+    from apify_client import ApifyClientAsync
+    from crawlee.storages import RequestQueue
+
     from .conftest import MakeActorFunction, RunActorFunction
 
 
@@ -1195,3 +1201,83 @@ async def consumer() -> int:
     actor = await make_actor(label='rq-performance-pattern-test', main_func=main)
     run_result = await run_actor(actor)
     assert run_result.status == 'SUCCEEDED'
+
+
+async def test_request_queue_enhanced_metadata(
+    request_queue_force_cloud: RequestQueue,
+    apify_client_async: ApifyClientAsync,
+) -> None:
+    """Test metadata tracking.
+
+    Multiple clients scenarios are not guaranteed to give correct results without delay. But at least multiple clients,
+    single producer, should be reliable on the producer side."""
+
+    for i in range(1, 10):
+        await request_queue_force_cloud.add_request(Request.from_url(f'http://example.com/{i}'))
+        # Reliable information as the API response is enhanced with local metadata estimation.
+        assert (await request_queue_force_cloud.get_metadata()).total_request_count == i
+
+    # Accessed with client created explicitly with `client_key=None` should appear as distinct client
+    api_client = apify_client_async.request_queue(request_queue_id=request_queue_force_cloud.id, client_key=None)
+    await api_client.list_head()
+
+    # The presence of another non-producing client should not affect the metadata
+    for i in range(10, 20):
+        await request_queue_force_cloud.add_request(Request.from_url(f'http://example.com/{i}'))
+        # Reliable information as the API response is enhanced with local metadata estimation.
+        assert (await request_queue_force_cloud.get_metadata()).total_request_count == i
+
+
+async def test_request_queue_metadata_another_client(
+    request_queue_force_cloud: RequestQueue,
+    apify_client_async: ApifyClientAsync,
+) -> None:
+    """Test metadata tracking. The delayed metadata should be reliable even when changed by another client."""
+    api_client = apify_client_async.request_queue(request_queue_id=request_queue_force_cloud.id, client_key=None)
+    await api_client.add_request(Request.from_url('http://example.com/1').model_dump(by_alias=True, exclude={'id'}))
+
+    # Wait to be sure that the API has updated the global metadata
+    await asyncio.sleep(10)
+
+    assert (await request_queue_force_cloud.get_metadata()).total_request_count == 1
+
+
+async def test_request_queue_had_multiple_clients(
+    request_queue_force_cloud: RequestQueue,
+    apify_client_async: ApifyClientAsync,
+) -> None:
+    """Test that `RequestQueue` correctly detects multiple clients.
+
+    Clients created with different `client_key` should appear as distinct clients."""
+    await request_queue_force_cloud.fetch_next_request()
+
+    # Accessed with client created explicitly with `client_key=None` should appear as distinct client
+    api_client = apify_client_async.request_queue(request_queue_id=request_queue_force_cloud.id, client_key=None)
+    await api_client.list_head()
+
+    # Check that it is correctly in the RequestQueueClient metadata
+    assert (await request_queue_force_cloud.get_metadata()).had_multiple_clients is True
+
+    # Check that it is correctly in the API
+    api_response = await api_client.get()
+    assert api_response
+    assert api_response['hadMultipleClients'] is True
+
+
+async def test_request_queue_not_had_multiple_clients(
+    request_queue_force_cloud: RequestQueue, apify_client_async: ApifyClientAsync
+) -> None:
+    """Test that same `RequestQueue` created from Actor does not act as multiple clients."""
+
+    # Two calls to API to create situation where different `client_key` can set `had_multiple_clients` to True
+    await request_queue_force_cloud.fetch_next_request()
+    await request_queue_force_cloud.fetch_next_request()
+
+    # Check that it is correctly in the RequestQueueClient metadata
+    assert (await request_queue_force_cloud.get_metadata()).had_multiple_clients is False
+
+    # Check that it is correctly in the API
+    api_client = apify_client_async.request_queue(request_queue_id=request_queue_force_cloud.id)
+    api_response = await api_client.get()
+    assert api_response
+    assert api_response['hadMultipleClients'] is False