mirror of
https://github.com/calibrain/shelfmark.git
synced 2026-09-24 19:30:30 +01:00
fix(sources): restore Direct Download search errors and language matches (#1339)
Fixes two regressions from the provider-driven refactor (#1337). First, the composite search caught RuntimeError, TypeError, ValueError and request errors from each provider and returned an empty list, so a failed search looked like one with no hits. It now raises the first provider failure when no provider returned releases. Second, the shared parser re-matched every row's language locally, dropping rows Anna's Archive had already matched with &lang= (free-text cells like 'English, French' or 'unknown'). parse_search_items gains a filter_languages option, which AA turns off, so AA's own language-from-path filter is again the only local one.
This commit is contained in:
@@ -768,6 +768,9 @@ def search_books(query: str, filters: SearchFilters) -> list[BrowseRecord]:
|
||||
provider_id="annas_archive",
|
||||
item_selector="tr",
|
||||
extract_item=_extract_aa_search_result,
|
||||
# AA already applied &lang= server-side; only the path-language pass below
|
||||
# (which skips &lang=) needs a local language filter.
|
||||
filter_languages=False,
|
||||
)
|
||||
|
||||
if path_language_enabled and requested_langs:
|
||||
|
||||
@@ -188,13 +188,19 @@ def parse_search_items(
|
||||
*,
|
||||
provider_id: str,
|
||||
extract_item: Callable[[Tag], ParsedSearchResult | None],
|
||||
filter_languages: bool = True,
|
||||
) -> list[BrowseRecord]:
|
||||
"""Normalize provider-specific HTML elements into Direct Download records.
|
||||
|
||||
Providers only describe how fields are extracted from their DOM. Language and
|
||||
format filtering, stable IDs, and BrowseRecord construction stay shared.
|
||||
|
||||
Pass ``filter_languages=False`` when the site already filtered by language: its
|
||||
language cells are free text, and re-matching them locally drops rows it matched.
|
||||
"""
|
||||
requested_languages = normalize_requested_languages(filters.lang) if filters else set()
|
||||
requested_languages = (
|
||||
normalize_requested_languages(filters.lang) if filters and filter_languages else set()
|
||||
)
|
||||
requested_formats = (
|
||||
{value.casefold() for value in (filters.format or get_supported_formats())}
|
||||
if filters
|
||||
@@ -253,6 +259,7 @@ def parse_search_page(
|
||||
provider_id: str,
|
||||
item_selector: str,
|
||||
extract_item: Callable[[Tag], ParsedSearchResult | None],
|
||||
filter_languages: bool = True,
|
||||
) -> list[BrowseRecord]:
|
||||
"""Parse a result page using provider-specific selectors and extraction."""
|
||||
root = BeautifulSoup(page, "html.parser") if isinstance(page, str) else page
|
||||
@@ -261,4 +268,5 @@ def parse_search_page(
|
||||
filters,
|
||||
provider_id=provider_id,
|
||||
extract_item=extract_item,
|
||||
filter_languages=filter_languages,
|
||||
)
|
||||
|
||||
@@ -173,7 +173,7 @@ class DirectDownloadSource(ReleaseSource):
|
||||
raise DirectDownloadUnavailableError(unavailable_reason)
|
||||
|
||||
releases: list[Release] = []
|
||||
unavailable_errors: list[SourceUnavailableError] = []
|
||||
failures: list[Exception] = []
|
||||
for provider in registry.enabled_providers(self._providers):
|
||||
try:
|
||||
records = provider.search(
|
||||
@@ -183,7 +183,7 @@ class DirectDownloadSource(ReleaseSource):
|
||||
content_type=content_type,
|
||||
)
|
||||
except SourceUnavailableError as exc:
|
||||
unavailable_errors.append(exc)
|
||||
failures.append(exc)
|
||||
continue
|
||||
except (
|
||||
RuntimeError,
|
||||
@@ -192,11 +192,14 @@ class DirectDownloadSource(ReleaseSource):
|
||||
requests.exceptions.RequestException,
|
||||
) as exc:
|
||||
logger.warning("%s search failed: %s", provider.display_name, exc)
|
||||
failures.append(exc)
|
||||
continue
|
||||
releases.extend(_browse_record_to_release(record) for record in records)
|
||||
|
||||
if unavailable_errors and not releases:
|
||||
raise unavailable_errors[0]
|
||||
# A provider failure is only quiet when another provider answered. Otherwise the
|
||||
# caller has to see it, or a failed search reads as a search with no hits.
|
||||
if failures and not releases:
|
||||
raise failures[0]
|
||||
return releases
|
||||
|
||||
def is_available(self) -> bool:
|
||||
|
||||
@@ -350,3 +350,38 @@ def test_book_matches_requested_languages_logic():
|
||||
assert aa._book_matches_requested_languages(None, set()) is True
|
||||
assert aa._book_matches_requested_languages("en", {"fr"}) is False
|
||||
assert aa._book_matches_requested_languages("fr", {"fr"}) is True
|
||||
|
||||
|
||||
def test_search_books_keeps_server_language_matches_when_path_language_disabled(monkeypatch):
|
||||
monkeypatch.setattr(aa, "_is_language_from_path_enabled", lambda: False)
|
||||
monkeypatch.setattr(aa.network, "get_aa_base_url", lambda: "https://mirror.example")
|
||||
monkeypatch.setattr(aa.network, "AAMirrorSelector", lambda: object())
|
||||
|
||||
captured_url: dict[str, str] = {}
|
||||
|
||||
def _row(record_id: str, language: str) -> str:
|
||||
return f"""
|
||||
<tr>
|
||||
<td><a href="/md5/{record_id}"><img src="c.jpg"></a></td>
|
||||
<td><span>Book {record_id}</span></td><td><span>Author</span></td>
|
||||
<td><span>Publisher</span></td><td><span>2025</span></td>
|
||||
<td><span>-</span></td><td><span>-</span></td>
|
||||
<td><span>{language}</span></td>
|
||||
<td><span>fiction</span></td><td><span>pdf</span></td>
|
||||
<td><span>2 mb</span></td>
|
||||
</tr>
|
||||
"""
|
||||
|
||||
def _fake_html_get_page(url: str, selector, **_kwargs):
|
||||
del selector
|
||||
captured_url["url"] = url
|
||||
rows = _row("rec-multi", "English, French") + _row("rec-unknown", "unknown")
|
||||
return f"<table>{rows}</table>"
|
||||
|
||||
monkeypatch.setattr(aa.downloader, "html_get_page", _fake_html_get_page)
|
||||
|
||||
records = aa.search_books("demo", SearchFilters(lang=["en"], format=["pdf"]))
|
||||
|
||||
# AA already narrowed by &lang=; its free-text language cells must not be re-matched.
|
||||
assert "&lang=en" in captured_url["url"]
|
||||
assert [record.id for record in records] == ["rec-multi", "rec-unknown"]
|
||||
|
||||
@@ -183,3 +183,28 @@ def test_provider_failure_is_suppressed_when_another_provider_succeeds(monkeypat
|
||||
releases = source.search(SimpleNamespace(title="Example"), SimpleNamespace())
|
||||
|
||||
assert [release.source_id for release in releases] == ["working:1"]
|
||||
|
||||
|
||||
def test_provider_search_error_surfaces_when_no_provider_answers(monkeypatch):
|
||||
from shelfmark.release_sources.direct_download.source import DirectDownloadSource
|
||||
|
||||
class FailingProvider:
|
||||
id = "failing"
|
||||
display_name = "Failing"
|
||||
|
||||
def is_enabled(self):
|
||||
return True
|
||||
|
||||
def search(self, *_args, **_kwargs):
|
||||
raise RuntimeError("No books found. Please try another query.")
|
||||
|
||||
monkeypatch.setattr(
|
||||
registry.config,
|
||||
"get",
|
||||
lambda key, default=None: True if key == "DIRECT_DOWNLOAD_ENABLED" else default,
|
||||
)
|
||||
source = DirectDownloadSource()
|
||||
source._providers = (FailingProvider(),)
|
||||
|
||||
with pytest.raises(RuntimeError, match="No books found"):
|
||||
source.search(SimpleNamespace(title="Example"), SimpleNamespace())
|
||||
|
||||
Reference in New Issue
Block a user