mirror of
https://github.com/Second-Hand-Friends/kleinanzeigen-bot.git
synced 2026-07-09 12:41:05 +02:00
fix: avoid TypeError abort when web_request gets a non-dict response (#1162)
## ℹ️ Description When publishing a batch of ads, a single transient page-load failure could crash the **entire** run with an unhandled `TypeError`. During the pre-publish delete step, `delete_flow.delete_ad` calls `web_request`, which runs a `fetch(...)` via `web_execute`. When the page context is torn down mid-navigation (the same condition that surfaces as *"Page did not finish loading within 30.0 seconds"*), `web_execute` returns a CDP `ExceptionDetails` object (or `None`) instead of the expected response dict. `web_request` then does `response["statusCode"]`, raising: ``` TypeError: [ExceptionDetails] object is not subscriptable File ".../utils/web_scraping_mixin.py", line 1537, in web_request response["statusCode"] in valid_response_codes, ``` This `TypeError` is **not** caught by the publish retry loop (which handles `TimeoutError` / `ProtocolException`), so it propagates all the way up and aborts the whole batch — every remaining ad goes unpublished. Observed live while publishing ~25 ads: 9 published successfully, two hit transient load timeouts, and the next `delete_ad` request crashed the process. - Link to the related issue(s): Issue # ## 📋 Changes Summary - `web_request`: guard the response — if it is not a `dict` containing `statusCode`, raise a `ProtocolException` (already imported) instead of blindly subscripting it. - `ProtocolException` is already handled by the publish retry/skip logic, so a single transient failure now retries and then skips that one ad, letting the rest of the batch continue. - Add a parametrized regression test in `test_web_scraping_mixin.py` covering `None` / non-dict / missing-`statusCode` responses. ### ⚙️ Type of Change - [x] 🐞 Bug fix (non-breaking change which fixes an issue) - [ ] ✨ New feature (adds new functionality without breaking existing usage) - [ ] 💥 Breaking change (changes that might break existing user setups, scripts, or configurations) ## ✅ Checklist - [x] I have reviewed my changes to ensure they meet the project's standards. - [x] I have tested my changes and ensured the affected tests pass (`pytest tests/unit/test_web_scraping_mixin.py -k web_request` → 7 passed). - [ ] I have formatted the code (`pdm run format`). - [ ] I have verified that linting passes (`pdm run lint`). - [ ] I have updated documentation where necessary (n/a). By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Enhanced validation for network operation responses with improved error messaging for unexpected situations, preventing hidden failures and providing clearer diagnostics when issues occur. * **Tests** * Extended test coverage to comprehensively validate error handling across various edge cases and unexpected response scenarios in network operations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
committed by
GitHub
parent
0c75eea071
commit
c6843a49c0
@@ -1533,6 +1533,14 @@ class WebScrapingMixin: # noqa: PLR0904
|
|||||||
""")
|
""")
|
||||||
if isinstance(valid_response_codes, int):
|
if isinstance(valid_response_codes, int):
|
||||||
valid_response_codes = [valid_response_codes]
|
valid_response_codes = [valid_response_codes]
|
||||||
|
# web_execute may return a CDP ExceptionDetails (or None) instead of the
|
||||||
|
# expected dict when the page context is torn down mid-navigation. Treat
|
||||||
|
# that as a transient protocol failure so the publish retry loop skips the
|
||||||
|
# ad instead of crashing the whole batch with a TypeError.
|
||||||
|
if not isinstance(response, dict) or "statusCode" not in response:
|
||||||
|
raise ProtocolException(
|
||||||
|
f"Unexpected response for HTTP {method} to {url}: {response!r}"
|
||||||
|
)
|
||||||
ensure(
|
ensure(
|
||||||
response["statusCode"] in valid_response_codes,
|
response["statusCode"] in valid_response_codes,
|
||||||
f'Invalid response "{response["statusCode"]} {response["statusMessage"]}" received for HTTP {method} to {url}',
|
f'Invalid response "{response["statusCode"]} {response["statusMessage"]}" received for HTTP {method} to {url}',
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ from unittest.mock import ANY, AsyncMock, MagicMock, Mock, mock_open, patch
|
|||||||
import nodriver
|
import nodriver
|
||||||
import psutil
|
import psutil
|
||||||
import pytest
|
import pytest
|
||||||
|
from nodriver.core.connection import ProtocolException
|
||||||
from nodriver.core.element import Element
|
from nodriver.core.element import Element
|
||||||
from nodriver.core.tab import Tab as Page
|
from nodriver.core.tab import Tab as Page
|
||||||
|
|
||||||
@@ -448,6 +449,19 @@ class TestWebScrapingErrorHandling:
|
|||||||
with pytest.raises(Exception, match = "Network error"):
|
with pytest.raises(Exception, match = "Network error"):
|
||||||
await web_scraper.web_request("https://example.com")
|
await web_scraper.web_request("https://example.com")
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
@pytest.mark.parametrize("bad_response", [None, "boom", 42, ["not", "a", "dict"], {"foo": "bar"}])
|
||||||
|
async def test_web_request_non_dict_response_raises_protocol_exception(
|
||||||
|
self, web_scraper:WebScrapingMixin, mock_page:TrulyAwaitableMockPage, bad_response:object,
|
||||||
|
) -> None:
|
||||||
|
"""A non-dict / missing-statusCode response (e.g. CDP ExceptionDetails when the page
|
||||||
|
context is torn down mid-navigation) must raise a catchable ProtocolException rather
|
||||||
|
than a TypeError that would abort an entire publish batch."""
|
||||||
|
mock_page.evaluate.return_value = bad_response
|
||||||
|
|
||||||
|
with pytest.raises(ProtocolException):
|
||||||
|
await web_scraper.web_request("https://example.com")
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_web_check_element_not_found(self, web_scraper:WebScrapingMixin, mock_page:TrulyAwaitableMockPage) -> None:
|
async def test_web_check_element_not_found(self, web_scraper:WebScrapingMixin, mock_page:TrulyAwaitableMockPage) -> None:
|
||||||
"""Test element not found error in web_check."""
|
"""Test element not found error in web_check."""
|
||||||
|
|||||||
Reference in New Issue
Block a user