Skip to content

Commit 5e574a3

Browse files
vdusekclaude
andcommitted
fix!: Raise ValueError when http_client conflicts with config params
Instead of silently ignoring max_retries, min_delay_between_retries, timeout, and headers when a custom http_client is provided, raise a ValueError to make the conflict explicit. Uses @overload to express the two mutually exclusive signatures for static type checkers. Also fix docstring check/fix scripts to handle @overload stubs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 46693a8 commit 5e574a3

File tree

4 files changed

+177
-31
lines changed

4 files changed

+177
-31
lines changed

scripts/check_async_docstrings.py

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,19 @@
3333
# Find corresponding sync method in the sync class
3434
sync_method = sync_class.find('DefNode', name=async_method.name)
3535

36-
# Skip methods with @ignore_docs decorator
37-
if len(async_method.decorators) and str(async_method.decorators[0].value) == 'ignore_docs':
36+
# Skip methods with @ignore_docs or @overload decorator
37+
if len(async_method.decorators) and str(async_method.decorators[0].value) in ('ignore_docs', 'overload'):
3838
continue
3939

40+
# When there are overloads, find() may return a stub instead of the implementation.
41+
# Find the actual implementation by skipping overload-decorated methods.
42+
if sync_method and any(str(d.value) == 'overload' for d in sync_method.decorators):
43+
candidates = sync_class.find_all('DefNode', name=async_method.name)
44+
sync_method = next(
45+
(m for m in candidates if not any(str(d.value) == 'overload' for d in m.decorators)),
46+
None,
47+
)
48+
4049
# If the sync method has a docstring, check if it matches the async dostring
4150
if sync_method and isinstance(sync_method.value[0].value, str):
4251
sync_docstring = sync_method.value[0].value

scripts/fix_async_docstrings.py

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,25 @@
3030
# Find corresponding sync method in the sync class
3131
sync_method = sync_class.find('DefNode', name=async_method.name)
3232

33-
# Skip methods with @ignore_docs decorator
34-
if len(async_method.decorators) and str(async_method.decorators[0].value) == 'ignore_docs':
33+
# Skip methods with @ignore_docs or @overload decorator
34+
if len(async_method.decorators) and str(async_method.decorators[0].value) in ('ignore_docs', 'overload'):
3535
continue
3636

3737
# Skip methods that don't exist in the sync class
3838
if sync_method is None:
3939
continue
4040

41+
# When there are overloads, find() may return a stub instead of the implementation.
42+
# Find the actual implementation by skipping overload-decorated methods.
43+
if any(str(d.value) == 'overload' for d in sync_method.decorators):
44+
candidates = sync_class.find_all('DefNode', name=async_method.name)
45+
sync_method = next(
46+
(m for m in candidates if not any(str(d.value) == 'overload' for d in m.decorators)),
47+
None,
48+
)
49+
if sync_method is None:
50+
continue
51+
4152
# If the sync method has a docstring, copy it to the async method (with adjustments)
4253
if isinstance(sync_method.value[0].value, str):
4354
sync_docstring = sync_method.value[0].value

src/apify_client/_apify_client.py

Lines changed: 89 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
import warnings
44
from functools import cached_property
5-
from typing import TYPE_CHECKING, ClassVar
5+
from typing import TYPE_CHECKING, ClassVar, overload
66

77
from apify_client._client_registry import ClientRegistry, ClientRegistryAsync
88
from apify_client._consts import (
@@ -113,6 +113,29 @@ class ApifyClient:
113113

114114
_OVERRIDABLE_DEFAULT_HEADERS: ClassVar[set[str]] = {'Accept', 'Authorization', 'Accept-Encoding', 'User-Agent'}
115115

116+
@overload
117+
def __init__(
118+
self,
119+
token: str | None = None,
120+
*,
121+
api_url: str = DEFAULT_API_URL,
122+
api_public_url: str | None = DEFAULT_API_URL,
123+
max_retries: int = DEFAULT_MAX_RETRIES,
124+
min_delay_between_retries: timedelta = DEFAULT_MIN_DELAY_BETWEEN_RETRIES,
125+
timeout: timedelta = DEFAULT_TIMEOUT,
126+
headers: dict[str, str] | None = None,
127+
) -> None: ...
128+
129+
@overload
130+
def __init__(
131+
self,
132+
token: str | None = None,
133+
*,
134+
api_url: str = DEFAULT_API_URL,
135+
api_public_url: str | None = DEFAULT_API_URL,
136+
http_client: HttpClient,
137+
) -> None: ...
138+
116139
def __init__(
117140
self,
118141
token: str | None = None,
@@ -144,13 +167,33 @@ def __init__(
144167
headers: Additional HTTP headers to include in all API requests. Only used when `http_client` is not
145168
provided.
146169
http_client: A custom HTTP client instance extending `HttpClient`. When provided, the `max_retries`,
147-
`min_delay_between_retries`, `timeout`, and `headers` parameters are ignored, as the custom
170+
`min_delay_between_retries`, `timeout`, and `headers` parameters must not be set, as the custom
148171
client is responsible for its own configuration.
172+
173+
Raises:
174+
ValueError: If `http_client` is provided together with `max_retries`, `min_delay_between_retries`,
175+
`timeout`, or `headers`.
149176
"""
150177
# We need to do this because of mocking in tests and default mutable arguments.
151178
api_url = DEFAULT_API_URL if api_url is None else api_url
152179
api_public_url = DEFAULT_API_URL if api_public_url is None else api_public_url
153180

181+
if http_client is not None:
182+
conflicting = []
183+
if max_retries != DEFAULT_MAX_RETRIES:
184+
conflicting.append('max_retries')
185+
if min_delay_between_retries != DEFAULT_MIN_DELAY_BETWEEN_RETRIES:
186+
conflicting.append('min_delay_between_retries')
187+
if timeout != DEFAULT_TIMEOUT:
188+
conflicting.append('timeout')
189+
if headers is not None:
190+
conflicting.append('headers')
191+
if conflicting:
192+
raise ValueError(
193+
f'Cannot pass {", ".join(conflicting)} together with http_client. When using a custom '
194+
'HTTP client, configure these options directly on the client instance.'
195+
)
196+
154197
if headers and not http_client:
155198
self._check_custom_headers(headers)
156199

@@ -427,6 +470,29 @@ async def main() -> None:
427470

428471
_OVERRIDABLE_DEFAULT_HEADERS: ClassVar[set[str]] = {'Accept', 'Authorization', 'Accept-Encoding', 'User-Agent'}
429472

473+
@overload
474+
def __init__(
475+
self,
476+
token: str | None = None,
477+
*,
478+
api_url: str = DEFAULT_API_URL,
479+
api_public_url: str | None = DEFAULT_API_URL,
480+
max_retries: int = DEFAULT_MAX_RETRIES,
481+
min_delay_between_retries: timedelta = DEFAULT_MIN_DELAY_BETWEEN_RETRIES,
482+
timeout: timedelta = DEFAULT_TIMEOUT,
483+
headers: dict[str, str] | None = None,
484+
) -> None: ...
485+
486+
@overload
487+
def __init__(
488+
self,
489+
token: str | None = None,
490+
*,
491+
api_url: str = DEFAULT_API_URL,
492+
api_public_url: str | None = DEFAULT_API_URL,
493+
http_client: HttpClientAsync,
494+
) -> None: ...
495+
430496
def __init__(
431497
self,
432498
token: str | None = None,
@@ -458,13 +524,33 @@ def __init__(
458524
headers: Additional HTTP headers to include in all API requests. Only used when `http_client` is not
459525
provided.
460526
http_client: A custom HTTP client instance extending `HttpClientAsync`. When provided, the `max_retries`,
461-
`min_delay_between_retries`, `timeout`, and `headers` parameters are ignored, as the custom
527+
`min_delay_between_retries`, `timeout`, and `headers` parameters must not be set, as the custom
462528
client is responsible for its own configuration.
529+
530+
Raises:
531+
ValueError: If `http_client` is provided together with `max_retries`, `min_delay_between_retries`,
532+
`timeout`, or `headers`.
463533
"""
464534
# We need to do this because of mocking in tests and default mutable arguments.
465535
api_url = DEFAULT_API_URL if api_url is None else api_url
466536
api_public_url = DEFAULT_API_URL if api_public_url is None else api_public_url
467537

538+
if http_client is not None:
539+
conflicting = []
540+
if max_retries != DEFAULT_MAX_RETRIES:
541+
conflicting.append('max_retries')
542+
if min_delay_between_retries != DEFAULT_MIN_DELAY_BETWEEN_RETRIES:
543+
conflicting.append('min_delay_between_retries')
544+
if timeout != DEFAULT_TIMEOUT:
545+
conflicting.append('timeout')
546+
if headers is not None:
547+
conflicting.append('headers')
548+
if conflicting:
549+
raise ValueError(
550+
f'Cannot pass {", ".join(conflicting)} together with http_client. When using a custom '
551+
'HTTP client, configure these options directly on the client instance.'
552+
)
553+
468554
if headers and not http_client:
469555
self._check_custom_headers(headers)
470556

tests/unit/test_pluggable_http_client.py

Lines changed: 64 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
from __future__ import annotations
22

3-
import warnings
43
from dataclasses import dataclass, field
54
from datetime import timedelta
65
from typing import TYPE_CHECKING, Any
@@ -15,7 +14,6 @@
1514
HttpClientAsync,
1615
HttpResponse,
1716
)
18-
from apify_client._consts import DEFAULT_MAX_RETRIES, DEFAULT_TIMEOUT
1917
from apify_client._http_clients import ImpitHttpClient, ImpitHttpClientAsync
2018
from apify_client.errors import ApifyApiError
2119

@@ -227,40 +225,44 @@ def test_apify_client_custom_http_client_receives_requests() -> None:
227225
assert result == {'data': {'id': 'test123'}}
228226

229227

230-
def test_apify_client_custom_http_client_ignores_other_params() -> None:
231-
"""Test that timeout/retries/headers params don't affect custom http_client."""
228+
def test_apify_client_custom_http_client_rejects_conflicting_params() -> None:
229+
"""Test that passing config params together with http_client raises ValueError."""
232230
fake_client = FakeHttpClient()
233-
client = ApifyClient(
234-
token='test_token',
235-
http_client=fake_client,
236-
timeout=timedelta(seconds=999),
237-
max_retries=99,
238-
headers={'X-Custom': 'should-be-ignored'},
239-
)
240-
241-
# The custom client should be used as-is
242-
assert client.http_client is fake_client
243231

244-
# Verify the custom client retained its own defaults (params were not forwarded)
245-
assert fake_client._timeout == DEFAULT_TIMEOUT
246-
assert fake_client._max_retries == DEFAULT_MAX_RETRIES
232+
with pytest.raises(ValueError, match=r'Cannot pass .* together with http_client'):
233+
ApifyClient( # ty: ignore[no-matching-overload]
234+
token='test_token',
235+
http_client=fake_client,
236+
timeout=timedelta(seconds=999),
237+
max_retries=99,
238+
headers={'X-Custom': 'should-not-be-allowed'},
239+
)
247240

248241

249-
def test_apify_client_custom_http_client_no_header_warning() -> None:
250-
"""Test that no header warning is raised when custom http_client is provided."""
242+
def test_apify_client_custom_http_client_rejects_headers() -> None:
243+
"""Test that passing headers together with http_client raises ValueError."""
251244
fake_client = FakeHttpClient()
252245

253-
# This should NOT raise a UserWarning even though we pass overriding headers,
254-
# because headers are ignored when http_client is provided.
255-
with warnings.catch_warnings():
256-
warnings.simplefilter('error')
257-
ApifyClient(
246+
with pytest.raises(ValueError, match=r'headers.*http_client'):
247+
ApifyClient( # ty: ignore[no-matching-overload]
258248
token='test_token',
259249
http_client=fake_client,
260250
headers={'User-Agent': 'Custom/1.0', 'Authorization': 'Bearer custom'},
261251
)
262252

263253

254+
def test_apify_client_custom_http_client_accepts_only_url_params() -> None:
255+
"""Test that http_client can be combined with token, api_url, and api_public_url."""
256+
fake_client = FakeHttpClient()
257+
client = ApifyClient(
258+
token='test_token',
259+
api_url='https://custom.api.example.com',
260+
api_public_url='https://public.api.example.com',
261+
http_client=fake_client,
262+
)
263+
assert client.http_client is fake_client
264+
265+
264266
# -- ApifyClientAsync with custom http_client --
265267

266268

@@ -294,6 +296,44 @@ async def test_apify_client_async_custom_http_client_receives_requests() -> None
294296
assert result == {'data': {'id': 'test123'}}
295297

296298

299+
async def test_apify_client_async_custom_http_client_rejects_conflicting_params() -> None:
300+
"""Test that passing config params together with http_client raises ValueError for async client."""
301+
fake_client = FakeHttpClientAsync()
302+
303+
with pytest.raises(ValueError, match=r'Cannot pass .* together with http_client'):
304+
ApifyClientAsync( # ty: ignore[no-matching-overload]
305+
token='test_token',
306+
http_client=fake_client,
307+
timeout=timedelta(seconds=999),
308+
max_retries=99,
309+
headers={'X-Custom': 'should-not-be-allowed'},
310+
)
311+
312+
313+
async def test_apify_client_async_custom_http_client_rejects_headers() -> None:
314+
"""Test that passing headers together with http_client raises ValueError for async client."""
315+
fake_client = FakeHttpClientAsync()
316+
317+
with pytest.raises(ValueError, match=r'headers.*http_client'):
318+
ApifyClientAsync( # ty: ignore[no-matching-overload]
319+
token='test_token',
320+
http_client=fake_client,
321+
headers={'User-Agent': 'Custom/1.0'},
322+
)
323+
324+
325+
async def test_apify_client_async_custom_http_client_accepts_only_url_params() -> None:
326+
"""Test that async http_client can be combined with token, api_url, and api_public_url."""
327+
fake_client = FakeHttpClientAsync()
328+
client = ApifyClientAsync(
329+
token='test_token',
330+
api_url='https://custom.api.example.com',
331+
api_public_url='https://public.api.example.com',
332+
http_client=fake_client,
333+
)
334+
assert client.http_client is fake_client
335+
336+
297337
# -- Public exports --
298338

299339

0 commit comments

Comments
 (0)