From e38c0b5e155e587ac172b010c98b19518841da25 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Mon, 17 Aug 2026 10:29:58 +0200 Subject: [PATCH 1/2] fix: Do not mask unrelated import errors when guarding optional dependencies --- src/apify_client/_utils/try_import.py | 22 ++++++++++--- src/apify_client/http_compressors/__init__.py | 7 +++-- tests/unit/test_http_compressors.py | 2 +- tests/unit/test_utils.py | 31 +++++++++++++++++++ 4 files changed, 54 insertions(+), 8 deletions(-) diff --git a/src/apify_client/_utils/try_import.py b/src/apify_client/_utils/try_import.py index 450e63cf..3d663534 100644 --- a/src/apify_client/_utils/try_import.py +++ b/src/apify_client/_utils/try_import.py @@ -11,17 +11,29 @@ from typing import Any +@dataclass +class ImportState: + """Describe whether an optional import succeeded.""" + + available: bool = True + + @contextmanager -def try_import(module_name: str, *symbol_names: str) -> Iterator[None]: +def try_import(module_name: str, *symbol_names: str, dependency_name: str) -> Iterator[ImportState]: """Context manager to attempt importing symbols into a module. - If an `ImportError` is raised during the import, the symbol will be replaced with a `FailedImport` object. + If the named optional dependency is missing, the symbols are replaced with `FailedImport` objects. Import errors + caused by the importing module itself or by another dependency are propagated instead of being masked. """ + state = ImportState() try: - yield - except ImportError as e: + yield state + except ModuleNotFoundError as exc: + if exc.name != dependency_name: + raise + state.available = False for symbol_name in symbol_names: - setattr(sys.modules[module_name], symbol_name, FailedImport(e.args[0])) + setattr(sys.modules[module_name], symbol_name, FailedImport(exc.args[0])) def install_import_hook(module_name: str) -> None: diff --git a/src/apify_client/http_compressors/__init__.py b/src/apify_client/http_compressors/__init__.py index bd0edf5d..b4fdc800 100644 --- a/src/apify_client/http_compressors/__init__.py +++ b/src/apify_client/http_compressors/__init__.py @@ -9,7 +9,10 @@ # `brotli` is an optional extra, so it's wrapped in try_import. Accessing `BrotliHttpCompressor` # without the extra installed raises a clear ImportError instead of failing at package import time. -with _try_import(__name__, 'BrotliHttpCompressor'): +with _try_import(__name__, 'BrotliHttpCompressor', dependency_name='brotli') as _brotli_import: from apify_client.http_compressors._brotli import BrotliHttpCompressor -__all__ = ['BrotliHttpCompressor', 'GzipHttpCompressor', 'HttpCompressor'] +if _brotli_import.available: + __all__ = ['BrotliHttpCompressor', 'GzipHttpCompressor', 'HttpCompressor'] +else: + __all__ = ['GzipHttpCompressor', 'HttpCompressor'] diff --git a/tests/unit/test_http_compressors.py b/tests/unit/test_http_compressors.py index d5643ee9..7e97e51e 100644 --- a/tests/unit/test_http_compressors.py +++ b/tests/unit/test_http_compressors.py @@ -35,7 +35,7 @@ def _brotli_unavailable() -> Iterator[None]: class _Blocker: def find_spec(self, name: str, *_args: object) -> None: if name == 'brotli' or name.startswith('brotli.'): - raise ModuleNotFoundError(f"No module named '{name}'") + raise ModuleNotFoundError(f"No module named '{name}'", name='brotli') def _affected(name: str) -> bool: return name == 'brotli' or name.startswith(('brotli.', 'apify_client.http_compressors')) diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 61bbb3af..aff75fc0 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -3,9 +3,11 @@ import gzip import io import json +import sys from base64 import b64decode from datetime import timedelta from http import HTTPStatus +from types import ModuleType from typing import TYPE_CHECKING, Any from unittest.mock import Mock @@ -23,6 +25,7 @@ response_to_list, to_safe_id, ) +from apify_client._utils.try_import import FailedImport, try_import from apify_client.errors import ApifyApiError, InvalidResponseBodyError if TYPE_CHECKING: @@ -32,6 +35,34 @@ _GZIPPED_DATA = gzip.compress(b'buffer data') +def test_try_import_only_handles_the_named_missing_dependency() -> None: + """Optional-import handling must not hide a broken transitive dependency or module bug.""" + module_name = 'test_optional_import_module' + sys.modules[module_name] = ModuleType(module_name) + try: + with ( + pytest.raises(ModuleNotFoundError, match='broken-transitive'), + try_import(module_name, 'OptionalSymbol', dependency_name='optional-package'), + ): + raise ModuleNotFoundError('broken-transitive', name='broken-transitive') + assert not hasattr(sys.modules[module_name], 'OptionalSymbol') + finally: + del sys.modules[module_name] + + +def test_try_import_records_the_named_missing_dependency() -> None: + """A genuinely missing optional dependency is converted to a failed-import placeholder.""" + module_name = 'test_missing_optional_import_module' + sys.modules[module_name] = ModuleType(module_name) + try: + with try_import(module_name, 'OptionalSymbol', dependency_name='optional-package') as state: + raise ModuleNotFoundError("No module named 'optional-package'", name='optional-package') + assert state.available is False + assert isinstance(sys.modules[module_name].OptionalSymbol, FailedImport) + finally: + del sys.modules[module_name] + + def test_to_safe_id() -> None: assert to_safe_id('abc') == 'abc' assert to_safe_id('abc/def') == 'abc~def' From df8a08d48e48ca6a9e8db2ed7f3ad2b177635db0 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Mon, 17 Aug 2026 10:31:58 +0200 Subject: [PATCH 2/2] fix: Keep HttpResponse isinstance checks from consuming streamed responses --- pyproject.toml | 2 + src/apify_client/http_clients/_base.py | 7 +- tests/integration/test_dataset.py | 5 +- tests/integration/test_key_value_store.py | 13 ++- tests/unit/conftest.py | 14 +++ tests/unit/test_client_streaming.py | 129 ++++++++++++++++++++++ uv.lock | 2 + 7 files changed, 165 insertions(+), 7 deletions(-) create mode 100644 tests/unit/test_client_streaming.py diff --git a/pyproject.toml b/pyproject.toml index 30ed58bc..0019741f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -28,6 +28,8 @@ dependencies = [ "impit~=0.13.0", "more_itertools>=10.0.0", "pydantic[email]>=2.11.0", + # 4.6.0 is the first release whose runtime protocol checks look attributes up statically. + "typing-extensions>=4.6.0", ] [project.optional-dependencies] diff --git a/src/apify_client/http_clients/_base.py b/src/apify_client/http_clients/_base.py index 110b6abf..5a4491f8 100644 --- a/src/apify_client/http_clients/_base.py +++ b/src/apify_client/http_clients/_base.py @@ -7,9 +7,14 @@ from abc import ABC, abstractmethod from datetime import UTC, datetime, timedelta from importlib import metadata -from typing import TYPE_CHECKING, Any, Protocol, runtime_checkable +from typing import TYPE_CHECKING, Any from urllib.parse import urlencode +# `Protocol` comes from `typing_extensions`, not `typing`, because its runtime `isinstance` check looks attributes +# up statically. The `typing` implementation on Python 3.11 calls `hasattr`, which evaluates properties. On an +# unread streaming response, that either raises or silently buffers the whole body. +from typing_extensions import Protocol, runtime_checkable + from apify_client._consts import ( DEFAULT_MAX_RETRIES, DEFAULT_MIN_DELAY_BETWEEN_RETRIES, diff --git a/tests/integration/test_dataset.py b/tests/integration/test_dataset.py index 1bd9429b..333c7229 100644 --- a/tests/integration/test_dataset.py +++ b/tests/integration/test_dataset.py @@ -21,6 +21,7 @@ from apify_client._models import Dataset, DatasetListItem, DatasetStatistics, ListOfDatasets from apify_client._resource_clients.dataset import DatasetItemsPage from apify_client.errors import ApifyApiError +from apify_client.http_clients import HttpResponse if TYPE_CHECKING: from apify_client import ApifyClient, ApifyClientAsync @@ -727,7 +728,7 @@ async def get_items() -> DatasetItemsPage: if is_async: assert isinstance(stream_ctx, AbstractAsyncContextManager) async with stream_ctx as response: - assert isinstance(response, impit.Response) + assert isinstance(response, HttpResponse) assert response.status_code == 200 content = await response.aread() items = json.loads(content) @@ -736,7 +737,7 @@ async def get_items() -> DatasetItemsPage: else: assert isinstance(stream_ctx, AbstractContextManager) with stream_ctx as response: - assert isinstance(response, impit.Response) + assert isinstance(response, HttpResponse) assert response.status_code == 200 content = response.read() items = json.loads(content) diff --git a/tests/integration/test_key_value_store.py b/tests/integration/test_key_value_store.py index fdfedc72..5d1d9238 100644 --- a/tests/integration/test_key_value_store.py +++ b/tests/integration/test_key_value_store.py @@ -21,6 +21,7 @@ ) from apify_client._models import KeyValueStore, KeyValueStoreKey, ListOfKeys, ListOfKeyValueStores from apify_client.errors import ApifyApiError +from apify_client.http_clients import HttpResponse if TYPE_CHECKING: from apify_client import ApifyClient, ApifyClientAsync @@ -198,14 +199,14 @@ async def test_stream_record_signature( signature=test_kvs_of_another_user.keys_signature[key], ) as stream: # ty: ignore[invalid-context-manager] assert isinstance(stream, dict) - value = json.loads(stream['value'].content.decode('utf-8')) + value = json.loads((await stream['value'].aread()).decode('utf-8')) else: with kvs.stream_record( key, signature=test_kvs_of_another_user.keys_signature[key], ) as stream: # ty: ignore[invalid-context-manager] assert isinstance(stream, dict) - value = json.loads(stream['value'].content.decode('utf-8')) + value = json.loads(stream['value'].read().decode('utf-8')) assert test_kvs_of_another_user.expected_content[key] == value @@ -726,11 +727,15 @@ async def added_record_exists() -> bool: if is_async: async with store_client.stream_record('stream-key') as stream: # ty: ignore[invalid-context-manager] assert isinstance(stream, dict) - value = json.loads(stream['value'].content.decode('utf-8')) + response = stream['value'] + assert isinstance(response, HttpResponse) + value = json.loads((await response.aread()).decode('utf-8')) else: with store_client.stream_record('stream-key') as stream: # ty: ignore[invalid-context-manager] assert isinstance(stream, dict) - value = json.loads(stream['value'].content.decode('utf-8')) + response = stream['value'] + assert isinstance(response, HttpResponse) + value = json.loads(response.read().decode('utf-8')) assert value == {'data': 'streamed'} finally: diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 8ee3d0a8..b732cc1c 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -6,6 +6,8 @@ import pytest from pytest_httpserver import HTTPServer +from apify_client.http_clients import HttpClient, HttpClientAsync, ImpitHttpClient, ImpitHttpClientAsync + if TYPE_CHECKING: from collections.abc import Iterable @@ -28,3 +30,15 @@ def httpserver(make_httpserver: HTTPServer) -> Iterable[HTTPServer]: server = make_httpserver yield server server.clear() + + +@pytest.fixture(params=[pytest.param(ImpitHttpClient, id='impit')]) +def http_client_class(request: pytest.FixtureRequest) -> type[HttpClient]: + """Return each built-in synchronous HTTP client class.""" + return request.param + + +@pytest.fixture(params=[pytest.param(ImpitHttpClientAsync, id='impit')]) +def http_client_async_class(request: pytest.FixtureRequest) -> type[HttpClientAsync]: + """Return each built-in asynchronous HTTP client class.""" + return request.param diff --git a/tests/unit/test_client_streaming.py b/tests/unit/test_client_streaming.py new file mode 100644 index 00000000..d369455e --- /dev/null +++ b/tests/unit/test_client_streaming.py @@ -0,0 +1,129 @@ +from __future__ import annotations + +from typing import TYPE_CHECKING + +from apify_client import ApifyClient, ApifyClientAsync +from apify_client.http_clients import HttpResponse + +if TYPE_CHECKING: + from typing import Any + + from pytest_httpserver import HTTPServer + + from apify_client.http_clients import HttpClient, HttpClientAsync + + +DATASET_ID = 'test-dataset-id' +KVS_ID = 'test-kvs-id' +RECORD_KEY = 'test-record-key' +STREAM_CONTENT = b'[{"id": 1}]' + + +def test_dataset_stream_items_sync( + httpserver: HTTPServer, + http_client_class: type[HttpClient], +) -> None: + """Dataset streams expose a transport-independent response that can be read synchronously.""" + httpserver.expect_request(f'/v2/datasets/{DATASET_ID}/items').respond_with_data(STREAM_CONTENT) + api_url = httpserver.url_for('/').removesuffix('/') + client = ApifyClient.with_custom_http_client( + api_url=api_url, + http_client=http_client_class(), + ) + + with client.dataset(DATASET_ID).stream_items(item_format='json') as response: + assert isinstance(response, HttpResponse) + assert response.read() == STREAM_CONTENT + + +async def test_dataset_stream_items_async( + httpserver: HTTPServer, + http_client_async_class: type[HttpClientAsync], +) -> None: + """Dataset streams expose a transport-independent response that can be read asynchronously.""" + httpserver.expect_request(f'/v2/datasets/{DATASET_ID}/items').respond_with_data(STREAM_CONTENT) + api_url = httpserver.url_for('/').removesuffix('/') + client = ApifyClientAsync.with_custom_http_client( + api_url=api_url, + http_client=http_client_async_class(), + ) + + async with client.dataset(DATASET_ID).stream_items(item_format='json') as response: + assert isinstance(response, HttpResponse) + assert await response.aread() == STREAM_CONTENT + + +def test_key_value_store_stream_record_sync( + httpserver: HTTPServer, + http_client_class: type[HttpClient], +) -> None: + """KVS streams require reading the generic response before consuming its content.""" + httpserver.expect_request(f'/v2/key-value-stores/{KVS_ID}/records/{RECORD_KEY}').respond_with_data(STREAM_CONTENT) + api_url = httpserver.url_for('/').removesuffix('/') + client = ApifyClient.with_custom_http_client( + api_url=api_url, + http_client=http_client_class(), + ) + + with client.key_value_store(KVS_ID).stream_record(RECORD_KEY) as record: + assert isinstance(record, dict) + response = record['value'] + assert isinstance(response, HttpResponse) + assert response.read() == STREAM_CONTENT + + +async def test_key_value_store_stream_record_async( + httpserver: HTTPServer, + http_client_async_class: type[HttpClientAsync], +) -> None: + """KVS streams require asynchronously reading the generic response before consuming its content.""" + httpserver.expect_request(f'/v2/key-value-stores/{KVS_ID}/records/{RECORD_KEY}').respond_with_data(STREAM_CONTENT) + api_url = httpserver.url_for('/').removesuffix('/') + client = ApifyClientAsync.with_custom_http_client( + api_url=api_url, + http_client=http_client_async_class(), + ) + + async with client.key_value_store(KVS_ID).stream_record(RECORD_KEY) as record: + assert isinstance(record, dict) + response = record['value'] + assert isinstance(response, HttpResponse) + assert await response.aread() == STREAM_CONTENT + + +def test_protocol_check_leaves_stream_unread_sync( + httpserver: HTTPServer, + http_client_class: type[HttpClient], +) -> None: + """Checking a streaming response against the protocol inspects it without pulling the body off the wire.""" + httpserver.expect_request(f'/v2/datasets/{DATASET_ID}/items').respond_with_data(STREAM_CONTENT) + api_url = httpserver.url_for('/').removesuffix('/') + client = ApifyClient.with_custom_http_client( + api_url=api_url, + http_client=http_client_class(), + ) + + with client.dataset(DATASET_ID).stream_items(item_format='json') as response: + assert isinstance(response, HttpResponse) + # `is_stream_consumed` is transport state, not part of the protocol, but the built-in client exposes it. + raw: Any = response + assert raw.is_stream_consumed is False + + +async def test_protocol_check_leaves_stream_unread_async( + httpserver: HTTPServer, + http_client_async_class: type[HttpClientAsync], +) -> None: + """Checking a streaming response against the protocol inspects it without pulling the body off the wire.""" + httpserver.expect_request(f'/v2/datasets/{DATASET_ID}/items').respond_with_data(STREAM_CONTENT) + api_url = httpserver.url_for('/').removesuffix('/') + client = ApifyClientAsync.with_custom_http_client( + api_url=api_url, + http_client=http_client_async_class(), + ) + + async with client.dataset(DATASET_ID).stream_items(item_format='json') as response: + assert isinstance(response, HttpResponse) + # `is_stream_consumed` is transport state, not part of the protocol, but the built-in client exposes it. + raw: Any = response + assert raw.is_stream_consumed is False diff --git a/uv.lock b/uv.lock index 6e15b717..51cff51f 100644 --- a/uv.lock +++ b/uv.lock @@ -47,6 +47,7 @@ dependencies = [ { name = "impit" }, { name = "more-itertools" }, { name = "pydantic", extra = ["email"] }, + { name = "typing-extensions" }, ] [package.optional-dependencies] @@ -82,6 +83,7 @@ requires-dist = [ { name = "impit", specifier = "~=0.13.0" }, { name = "more-itertools", specifier = ">=10.0.0" }, { name = "pydantic", extras = ["email"], specifier = ">=2.11.0" }, + { name = "typing-extensions", specifier = ">=4.6.0" }, ] provides-extras = ["brotli"]