From c8d5765ea3cce0e1b8d544f906dea8fd98039c7e Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:05:08 -0600 Subject: [PATCH] Scrape: fix multiple suffixes and relative link resolution (#801) * Scrape: match any of the comma-separated suffixes The UI's File Suffixes field accepts a comma-separated list, but the downloader compared each link against the whole string, so a scrape with more than one suffix never matched anything. Co-Authored-By: Claude Opus 5.5 * Scrape: resolve relative links against the page's directory resolve_url appended a relative href to the page URL, so a link to "a.txt" on ".../dir/page.html" became ".../dir/page.html/a.txt" and the file download 404'd. Use urljoin, and resolve against the final URL after redirects so a directory URL without a trailing slash still works. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- wrolpi/files/test/test_scrape_downloader.py | 56 ++++++++++++++++++++- wrolpi/scrape_downloader.py | 38 ++++++-------- 2 files changed, 70 insertions(+), 24 deletions(-) diff --git a/wrolpi/files/test/test_scrape_downloader.py b/wrolpi/files/test/test_scrape_downloader.py index 33e48151..3c434009 100644 --- a/wrolpi/files/test/test_scrape_downloader.py +++ b/wrolpi/files/test/test_scrape_downloader.py @@ -11,6 +11,7 @@ from wrolpi.scrape_downloader import ( ExecutedScrape, PreparedScrape, ScrapeHTMLDownloader, + resolve_url, scrape_html_downloader, ) @@ -29,7 +30,8 @@ EXAMPLE_HTTP = ''' async def fake_fetch_html(self, url: str): - return EXAMPLE_HTTP + # Servers redirect a directory URL to its trailing-slash form. + return f'{url.rstrip("/")}/', EXAMPLE_HTTP async def fake_file_do_download(*a, **kwargs): @@ -275,6 +277,56 @@ async def test_fetch_html_decodes_declared_charset(test_directory, simple_file_s (test_directory / 'page.html').write_bytes(html.encode(encoding)) host, port = simple_file_server.server_address - content = await ScrapeHTMLDownloader.fetch_html(f'http://127.0.0.1:{port}/page.html') + _, content = await ScrapeHTMLDownloader.fetch_html(f'http://127.0.0.1:{port}/page.html') assert 'Zurück' in content + + +@pytest.mark.asyncio +async def test_execute_collects_multiple_suffixes(test_directory): + """The UI submits a comma-separated list of suffixes; a link matching any of them is collected.""" + prepared = PreparedScrape( + url='https://example.com/dir', + depth=1, + suffix='.pdf,.html', + max_pages=100, + destination=test_directory, + ) + + with mock.patch('wrolpi.scrape_downloader.ScrapeHTMLDownloader.fetch_html', fake_fetch_html): + executed = await scrape_html_downloader.execute_download(prepared, make_test_ctx()) + + assert sorted(executed.download_urls) == sorted([ + 'https://example.com/one.pdf', + 'https://example.com/two.pdf', + 'https://example.com/dir/three.pdf', + 'https://example.com/dir/other.html', + ]) + + +@pytest.mark.parametrize('page_url,href,expected', [ + # A link on a page resolves against the page's directory, not the page itself. + ('http://example.com/archive/archive.html', 'bc-a02.txt', 'http://example.com/archive/bc-a02.txt'), + ('http://example.com/archive/', 'bc-a02.txt', 'http://example.com/archive/bc-a02.txt'), + ('http://example.com/archive/archive.html', '../index.html', 'http://example.com/index.html'), + ('http://example.com/archive/archive.html', './a.txt', 'http://example.com/archive/a.txt'), + ('http://example.com/archive/archive.html', '/root.txt', 'http://example.com/root.txt'), + ('http://example.com/archive/archive.html', 'https://other.com/x.txt', 'https://other.com/x.txt'), + ('https://example.com/archive/archive.html', '//cdn.example.com/x.txt', 'https://cdn.example.com/x.txt'), + ('http://example.com/archive/archive.html', 'a.txt?v=1', 'http://example.com/archive/a.txt?v=1'), +]) +def test_resolve_url(page_url, href, expected): + assert resolve_url(page_url, href) == expected + + +@pytest.mark.asyncio +async def test_fetch_html_returns_redirected_url(test_directory, simple_file_server): + """Links in a directory listing resolve against the redirected URL (with trailing slash).""" + (test_directory / 'archive').mkdir() + (test_directory / 'archive' / 'a.txt').write_text('a') + host, port = simple_file_server.server_address + + final_url, content = await ScrapeHTMLDownloader.fetch_html(f'http://127.0.0.1:{port}/archive') + + assert final_url == f'http://127.0.0.1:{port}/archive/' + assert resolve_url(final_url, 'a.txt') == f'http://127.0.0.1:{port}/archive/a.txt' diff --git a/wrolpi/scrape_downloader.py b/wrolpi/scrape_downloader.py index 0c74b8d3..3558401d 100644 --- a/wrolpi/scrape_downloader.py +++ b/wrolpi/scrape_downloader.py @@ -1,8 +1,8 @@ import pathlib from copy import copy from dataclasses import dataclass -from typing import List -from urllib.parse import urlparse +from typing import List, Tuple +from urllib.parse import urljoin from bs4.dammit import UnicodeDammit from sqlalchemy.orm import Session @@ -14,20 +14,9 @@ from wrolpi.errors import UnrecoverableDownloadError logger = logger.getChild(__name__) -def resolve_url(parent_url: str, url: str): - parsed_domain = urlparse(parent_url) - - parsed = urlparse(url) - if parsed.scheme: - # URL is complete or external. - return url - elif parsed.path.startswith('/'): - # URL is absolute from domain. - return f'{parsed_domain.scheme}://{parsed_domain.netloc}{url}' - - # Assume URL is relative to the parent URL page. - parent_url = parent_url.rstrip('/') - return f'{parent_url}/{url}' +def resolve_url(page_url: str, url: str) -> str: + """Resolve a link the way a browser does: relative to the page's directory (or absolute/external).""" + return urljoin(page_url, url) @dataclass @@ -64,13 +53,16 @@ class ScrapeHTMLDownloader(Downloader): return '' @staticmethod - async def fetch_html(url: str) -> str: + async def fetch_html(url: str) -> Tuple[str, str]: """Fetch and decode a page. A charset in the Content-Type header wins, then the page's - charset; aiohttp alone ignores and assumes UTF-8.""" + charset; aiohttp alone ignores and assumes UTF-8. + + Returns the final URL after redirects (links resolve against it) and the page content.""" async with aiohttp_get(url, timeout=60 * 5) as response: body = await response.read() known_encodings = [response.charset] if response.charset else [] - return UnicodeDammit(body, known_definite_encodings=known_encodings, is_html=True).unicode_markup + final_url = str(response.url) + return final_url, UnicodeDammit(body, known_definite_encodings=known_encodings, is_html=True).unicode_markup def prepare_download(self, session: Session, download: Download) -> PreparedScrape: """Validate settings and ensure the destination directory exists. @@ -107,7 +99,9 @@ class ScrapeHTMLDownloader(Downloader): async def execute_download(self, prepared: PreparedScrape, ctx: DownloadContext, download: Download = None) -> ExecutedScrape: - """Crawl HTML pages up to depth, collecting links that match the configured suffix.""" + """Crawl HTML pages up to depth, collecting links that match any of the configured suffixes.""" + # The UI submits a comma-separated list of suffixes. + suffixes = tuple(i.strip() for i in prepared.suffix.split(',') if i.strip()) urls = [prepared.url] download_urls: List[str] = [] page_count = 0 @@ -122,7 +116,7 @@ class ScrapeHTMLDownloader(Downloader): break # Get the HTML of the URL. - content = await self.fetch_html(url) + url, content = await self.fetch_html(url) page_count += 1 if not content: logger.error(f'Failed to download url: {url}') @@ -143,7 +137,7 @@ class ScrapeHTMLDownloader(Downloader): logger.debug(f'Not a real anchor: {a}') continue child_url = resolve_url(url, href) - if child_url and child_url.lower().endswith(prepared.suffix): + if child_url and child_url.lower().endswith(suffixes): # Found a file that the User requested. logger.info(f'ScrapeHTMLDownloader will download {child_url}') download_urls.append(child_url)