Repository navigation
feat(crawlers): implement Request.skip_navigation to skip navigation and run the handler directly
#2338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v2
Are you sure you want to change the base?
feat(crawlers): implement Request.skip_navigation to skip navigation and run the handler directly
#2338
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |||||||||||||||||||||||||||||||||||||||
| from asyncio import CancelledError | ||||||||||||||||||||||||||||||||||||||||
| from collections.abc import AsyncGenerator, Awaitable, Callable, Iterable, Sequence | ||||||||||||||||||||||||||||||||||||||||
| from contextlib import AsyncExitStack | ||||||||||||||||||||||||||||||||||||||||
| from dataclasses import replace as dataclass_replace | ||||||||||||||||||||||||||||||||||||||||
| from datetime import timedelta | ||||||||||||||||||||||||||||||||||||||||
| from functools import partial | ||||||||||||||||||||||||||||||||||||||||
| from http import HTTPStatus | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -57,6 +58,7 @@ | |||||||||||||||||||||||||||||||||||||||
| ContextPipelineInterruptedError, | ||||||||||||||||||||||||||||||||||||||||
| HttpClientStatusCodeError, | ||||||||||||||||||||||||||||||||||||||||
| HttpStatusCodeError, | ||||||||||||||||||||||||||||||||||||||||
| NavigationSkippedError, | ||||||||||||||||||||||||||||||||||||||||
| PersistentRateLimitError, | ||||||||||||||||||||||||||||||||||||||||
| RequestCollisionError, | ||||||||||||||||||||||||||||||||||||||||
| RequestHandlerError, | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -115,6 +117,33 @@ | |||||||||||||||||||||||||||||||||||||||
| SkippedRequestCallback = Callable[[str, SkippedReason], Awaitable[None]] | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| class _NavigationSkippedRequestProxy: | ||||||||||||||||||||||||||||||||||||||||
| """Wraps a `Request` whose navigation was skipped due to `skip_navigation`. | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| All attribute access is forwarded to the wrapped request, except `loaded_url`, which raises a | ||||||||||||||||||||||||||||||||||||||||
| `NavigationSkippedError` — mirroring the JS SDK behavior for skipped navigations. | ||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| __slots__ = ('_raise_on_loaded_url', '_request') | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def __init__(self, request: Request, raise_on_loaded_url: Callable[[], str]) -> None: | ||||||||||||||||||||||||||||||||||||||||
| self._request = request | ||||||||||||||||||||||||||||||||||||||||
| self._raise_on_loaded_url = raise_on_loaded_url | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def __getattr__(self, name: str) -> Any: | ||||||||||||||||||||||||||||||||||||||||
| # Attribute writes (e.g. `request.state = ...` in the router) must land on the wrapped request, | ||||||||||||||||||||||||||||||||||||||||
| # so intercept only reads that the wrapped request cannot resolve itself. | ||||||||||||||||||||||||||||||||||||||||
| if name == 'loaded_url': | ||||||||||||||||||||||||||||||||||||||||
| return self._raise_on_loaded_url() | ||||||||||||||||||||||||||||||||||||||||
| return getattr(self._request, name) | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+133
to
+138
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def __setattr__(self, name: str, value: Any) -> None: | ||||||||||||||||||||||||||||||||||||||||
| if name in ('_request', '_raise_on_loaded_url'): | ||||||||||||||||||||||||||||||||||||||||
| super().__setattr__(name, value) | ||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||
| setattr(self._request, name, value) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| class _BasicCrawlerOptions(TypedDict): | ||||||||||||||||||||||||||||||||||||||||
| """Non-generic options the `BasicCrawler` constructor.""" | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1722,6 +1751,11 @@ async def _defer_throttled_request(self, request: Request, error: RequestThrottl | |||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| async def _run_request_handler(self, context: BasicCrawlingContext) -> None: | ||||||||||||||||||||||||||||||||||||||||
| context.request.state = RequestState.BEFORE_NAV | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| if context.request.skip_navigation: | ||||||||||||||||||||||||||||||||||||||||
| await self._run_request_handler_with_skipped_navigation(context) | ||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| await self._context_pipeline( | ||||||||||||||||||||||||||||||||||||||||
| context, | ||||||||||||||||||||||||||||||||||||||||
| lambda final_context: wait_for( | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1733,6 +1767,37 @@ async def _run_request_handler(self, context: BasicCrawlingContext) -> None: | |||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| async def _run_request_handler_with_skipped_navigation(self, context: BasicCrawlingContext) -> None: | ||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Important: Bypassing the pipeline means the handler gets a plain |
||||||||||||||||||||||||||||||||||||||||
| """Run the request handler without navigating, for requests with `skip_navigation` set. | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| The context pipeline is bypassed entirely: its middleware steps are responsible for the navigation, | ||||||||||||||||||||||||||||||||||||||||
| so the crawling context they would produce does not contain the navigation results. Navigation-only | ||||||||||||||||||||||||||||||||||||||||
| context members (`request.loaded_url`) raise a `NavigationSkippedError` instead of returning `None`. | ||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||
| self._logger.debug(f'Skipping navigation for request {context.request.url} (`skip_navigation` was used).') | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def _raise_on_loaded_url() -> str: | ||||||||||||||||||||||||||||||||||||||||
| raise NavigationSkippedError( | ||||||||||||||||||||||||||||||||||||||||
| 'The `request.loaded_url` property is not available - `skip_navigation` was used.' | ||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| guarded_request = cast( | ||||||||||||||||||||||||||||||||||||||||
| 'Request', | ||||||||||||||||||||||||||||||||||||||||
| _NavigationSkippedRequestProxy(context.request, _raise_on_loaded_url), | ||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||
| guarded_context = cast( | ||||||||||||||||||||||||||||||||||||||||
| 'TCrawlingContext', | ||||||||||||||||||||||||||||||||||||||||
| dataclass_replace(context, request=guarded_request), # ty: ignore[invalid-argument-type] | ||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: With the locked ty version this ignore is reported as unused, so the type-check job fails.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| await wait_for( | ||||||||||||||||||||||||||||||||||||||||
| lambda: self.router(guarded_context), | ||||||||||||||||||||||||||||||||||||||||
| timeout=self._request_handler_timeout, | ||||||||||||||||||||||||||||||||||||||||
| timeout_message=f'{self._request_handler_timeout_text}' | ||||||||||||||||||||||||||||||||||||||||
| f' {self._request_handler_timeout.total_seconds()} seconds', | ||||||||||||||||||||||||||||||||||||||||
| logger=self._logger, | ||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1793
to
+1799
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Important: The router runs outside
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def _raise_for_error_status_code(self, status_code: int) -> None: | ||||||||||||||||||||||||||||||||||||||||
| """Raise an exception if the given status code is considered an error. | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| 'ContextPipelineInterruptedError', | ||
| 'HttpClientStatusCodeError', | ||
| 'HttpStatusCodeError', | ||
| 'NavigationSkippedError', | ||
| 'PersistentRateLimitError', | ||
| 'ProxyError', | ||
| 'RequestCollisionError', | ||
|
|
@@ -35,6 +36,16 @@ class UserHandlerTimeoutError(UserDefinedErrorHandlerError): | |
| """Raised when a router fails due to user raised timeout. This is different from user-defined handler timing out.""" | ||
|
|
||
|
|
||
| @docs_group('Errors') | ||
| class NavigationSkippedError(Exception): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: In JS, |
||
| """Raised when a navigation-only crawling context member is accessed, but `skip_navigation` was used. | ||
|
|
||
| When a request has `skip_navigation` set to `True`, the crawler does not open the page and the crawling | ||
| context does not contain the results of the navigation (e.g. `http_response`, `$`, parsed content or | ||
| `request.loaded_url`). Accessing such properties raises this error at runtime. | ||
|
Comment on lines
+41
to
+45
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: Only |
||
| """ | ||
|
|
||
|
|
||
| @docs_group('Errors') | ||
| class SessionError(Exception): | ||
| """Errors of `SessionError` type will trigger a session rotation. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ | |
| from crawlee.configuration import Configuration | ||
| from crawlee.crawlers import BasicCrawler | ||
| from crawlee.errors import ( | ||
| NavigationSkippedError, | ||
| PersistentRateLimitError, | ||
| RequestCollisionError, | ||
| RequestThrottledError, | ||
|
|
@@ -3169,3 +3170,45 @@ async def test_stall_ignores_shared_concurrency() -> None: | |
| pytest.raises(PersistentRateLimitError), | ||
| ): | ||
| await cast('Any', crawler)._BasicCrawler__is_finished_function() | ||
|
|
||
|
|
||
| async def test_skip_navigation_runs_handler_without_navigation() -> None: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: |
||
| """A request with `skip_navigation` set is processed directly, without the navigation pipeline.""" | ||
| crawler = BasicCrawler( | ||
| configure_logging=False, | ||
| concurrency_settings=ConcurrencySettings(desired_concurrency=1, max_concurrency=1), | ||
| ) | ||
| processed: list[str] = [] | ||
| navigation_members: dict[str, str] = {} | ||
|
Comment on lines
+3181
to
+3182
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: The contributing guide prefers |
||
|
|
||
| @crawler.router.default_handler | ||
| async def handler(context: BasicCrawlingContext) -> None: | ||
| processed.append(context.request.unique_key) | ||
| try: | ||
| _ = context.request.loaded_url | ||
| except NavigationSkippedError: | ||
| navigation_members[context.request.unique_key] = 'raised' | ||
|
|
||
| plain_request = Request.from_url('https://plain.placeholder.com', unique_key='plain') | ||
| skipped_request = Request.from_url('https://skipped.placeholder.com', unique_key='skipped') | ||
| skipped_request.skip_navigation = True | ||
|
|
||
| stats = await crawler.run([plain_request, skipped_request]) | ||
|
|
||
| assert stats.requests_finished == 2 | ||
| assert processed == ['plain', 'skipped'] | ||
| # `loaded_url` only raises for the skip-navigation request; on the plain one it is just `None`. | ||
| assert navigation_members == {'skipped': 'raised'} | ||
|
|
||
|
|
||
| async def test_skip_navigation_request_serialization_roundtrip() -> None: | ||
| """`skip_navigation` survives serialization via the `skipNavigation` user data alias.""" | ||
| request = Request.from_url('https://placeholder.com', unique_key='roundtrip') | ||
| assert request.skip_navigation is False | ||
|
|
||
| request.skip_navigation = True | ||
| data = request.model_dump(by_alias=True) | ||
| assert data['userData']['__crawlee']['skipNavigation'] is True | ||
|
|
||
| restored = Request.model_validate(data) | ||
| assert restored.skip_navigation is True | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
raise_on_loaded_urlonly ever has one value, so__getattr__could raiseNavigationSkippedErrordirectly and the extra slot and callable can go. The docstring also has a non-ASCII em dash and a reference to the JS SDK, which the repo avoids in source.