From e38c0b5e155e587ac172b010c98b19518841da25 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Mon, 17 Aug 2026 10:29:58 +0200 Subject: [PATCH] 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'