diff --git a/modules/archive/__init__.py b/modules/archive/__init__.py index 70099d44..78eaa984 100644 --- a/modules/archive/__init__.py +++ b/modules/archive/__init__.py @@ -13,7 +13,7 @@ from sqlalchemy.orm import Session from wrolpi.cmd import SINGLE_FILE_BIN, CHROMIUM, FIREFOX, DENO_BIN, SINGLE_FILE_DENO_SCRIPT from wrolpi.collections import Collection from wrolpi.common import logger, register_modeler, register_refresh_cleanup, limit_concurrent, split_lines_by_length, \ - slow_logger, get_title_from_html, TRACE_LEVEL + slow_logger, get_title_from_html, strip_surrogates, TRACE_LEVEL from wrolpi.db import get_db_session from wrolpi.downloader import Downloader, Download, DownloadContext, DownloadResult from wrolpi.errors import UnrecoverableDownloadError @@ -306,10 +306,13 @@ def model_archive(session: Session, file_group: FileGroup) -> Archive: title = get_title(readability_json_path) if not title: title = get_title_from_html(lib.read_singlefile_html(singlefile_path)) + # Scraped titles can contain unpaired surrogates which sqlite3 refuses to store; a poisoned + # title fails the modeler's batch commit (it killed archive modeling on 10.0.0.9). + title = strip_surrogates(title) contents = None if readability_txt_path: - contents = get_article(readability_txt_path) + contents = strip_surrogates(get_article(readability_txt_path)) # Check if an Archive already exists for this FileGroup archive = session.query(Archive).filter_by(file_group_id=file_group_id).one_or_none() @@ -405,7 +408,17 @@ async def archive_modeler(progress_callback: Callable[[int], None] = None): if PYTEST: raise - session.commit() + try: + session.commit() + except Exception as e: + # One poisoned row must not kill the modeler: skip this batch for the rest of this + # run and keep modeling. The batch stays unindexed, so the next refresh retries it. + session.rollback() + logger.error(f'archive_modeler failed to commit batch, skipping {len(results_list)} FileGroups', + exc_info=e) + invalid_archives.update(fg.id for fg, _ in results_list) + if PYTEST: + raise # Report batch progress batch_count = len(results_list) diff --git a/modules/archive/models.py b/modules/archive/models.py index 068a9902..475df06f 100644 --- a/modules/archive/models.py +++ b/modules/archive/models.py @@ -9,7 +9,8 @@ from sqlalchemy.orm import relationship, Session from wrolpi import dates from wrolpi.collections import Collection -from wrolpi.common import ModelHelper, Base, logger, get_title_from_html, get_wrolpi_config, get_media_directory +from wrolpi.common import ModelHelper, Base, logger, get_title_from_html, get_wrolpi_config, get_media_directory, \ + strip_surrogates from wrolpi.dates import now from wrolpi.errors import UnknownArchive from wrolpi.files.models import FileGroup @@ -283,8 +284,9 @@ class Archive(Base, ModelHelper): try: with readability_json_path.open() as fh: json_contents = json.load(fh) - url = json_contents.get('url') - title = json_contents.get('title') + # Scraped strings can contain unpaired surrogates which sqlite3 refuses to store. + url = strip_surrogates(json_contents.get('url')) + title = strip_surrogates(json_contents.get('title')) except Exception as e: raise InvalidArchive() from e @@ -346,7 +348,7 @@ class Archive(Base, ModelHelper): # Local import to avoid circular import within archive module from modules.archive import lib if self.singlefile_path and not self.file_group.title: - self.file_group.title = get_title_from_html(lib.read_singlefile_html(self.singlefile_path)) + self.file_group.title = strip_surrogates(get_title_from_html(lib.read_singlefile_html(self.singlefile_path))) def apply_metadata(self): """Read and apply (and more) data from the Singlefile HTML.""" @@ -356,11 +358,11 @@ class Archive(Base, ModelHelper): metadata = lib.parse_article_html_metadata(contents) if metadata.author: - self.file_group.author = metadata.author + self.file_group.author = strip_surrogates(metadata.author) if metadata.title: - self.file_group.title = metadata.title + self.file_group.title = strip_surrogates(metadata.title) if metadata.description: - self.file_group.b_text = metadata.description + self.file_group.b_text = strip_surrogates(metadata.description) if metadata.published_datetime: self.file_group.published_datetime = metadata.published_datetime if metadata.modified_datetime: diff --git a/modules/archive/test/test_archive_modeler_loop.py b/modules/archive/test/test_archive_modeler_loop.py index 99184b71..79cedb2b 100644 --- a/modules/archive/test/test_archive_modeler_loop.py +++ b/modules/archive/test/test_archive_modeler_loop.py @@ -105,3 +105,43 @@ async def test_archive_modeler_processes_more_than_20_files(async_client, test_s ).count() assert still_unindexed == 0, \ f"All HTML FileGroups should be indexed, but {still_unindexed} remain unindexed" + + +@pytest.mark.asyncio +async def test_archive_modeler_surrogate_title(async_client, test_session, test_directory, + make_files_structure): + """A scraped title containing an unpaired UTF-16 surrogate must not kill the modeler. + + Regression test from 10.0.0.9: one archive's readability title contained half of an emoji + ('\\ud83c'); sqlite3 refused the batch commit with "UnicodeEncodeError: surrogates not + allowed", the whole archive_modeler run died, and every archive after it was left + unmodeled (their empty domain collections were then deleted by cleanup).""" + domain = 'example.com' + archive_dir = test_directory / 'archives' / domain + archive_dir.mkdir(parents=True) + + files_structure = {} + for i in range(3): + timestamp = archive_strftime(datetime.datetime(2000, 1, 1 + i, 0, 0, 0).astimezone(pytz.UTC)) + url = f'https://{domain}/article-{i}' + files_structure[str(archive_dir / f'{timestamp}_Article {i}.html')] = \ + create_singlefile_content(url, f'Article {i}') + if i == 1: + # The readability json's title wins over the HTML title; give it a lone surrogate. + files_structure[str(archive_dir / f'{timestamp}_Article {i}.readability.json')] = \ + '{"title": "Broken emoji \\ud83c title", "url": "%s"}' % url + + files = make_files_structure(files_structure) + for file_path in files: + if pathlib.Path(file_path).suffix == '.html': + FileGroup.from_paths(test_session, *[f for f in files + if str(f).startswith(str(file_path)[:-5])]) + test_session.commit() + + await archive_modeler() + + archives = test_session.query(Archive).all() + assert len(archives) == 3 + titles = sorted(a.file_group.title for a in archives) + # The surrogate was stripped, the rest of the title survived. + assert titles == ['Article 0', 'Article 2', 'Broken emoji title'] diff --git a/wrolpi/common.py b/wrolpi/common.py index eefed8f5..c74e7cbb 100644 --- a/wrolpi/common.py +++ b/wrolpi/common.py @@ -2678,6 +2678,15 @@ WHITESPACE_SPLITTER = re.compile(r'\s+') TAB = re.compile(r'\t+') +def strip_surrogates(text: Optional[str]) -> Optional[str]: + """Remove unpaired UTF-16 surrogate characters (e.g. broken emoji halves from scraped HTML). + + sqlite3 refuses to store them: "UnicodeEncodeError: surrogates not allowed".""" + if text is None: + return None + return text.encode('utf-8', errors='ignore').decode('utf-8') + + def split_lines_by_length(text: str, max_line_length: int = 38) -> str: """ Break up any long lines along word boundaries. diff --git a/wrolpi/files/lib.py b/wrolpi/files/lib.py index 00704811..ee2f06b6 100644 --- a/wrolpi/files/lib.py +++ b/wrolpi/files/lib.py @@ -28,7 +28,7 @@ from wrolpi.common import get_media_directory, wrol_mode_check, logger, \ partition, \ get_files_and_directories, chunks_by_stem, walk, \ get_wrolpi_config, \ - unique_by_predicate, get_paths_in_media_directory, TRACE_LEVEL, get_relative_to_media_directory + unique_by_predicate, get_paths_in_media_directory, TRACE_LEVEL, get_relative_to_media_directory, strip_surrogates from wrolpi.dates import now, from_timestamp, months_selector_to_where, date_range_to_where from wrolpi.db import get_db_session, get_db_curs, values_clause from wrolpi.downloader import download_manager, Download @@ -929,7 +929,7 @@ async def apply_indexers(progress_callback: Callable[[int, int], None] = None): ' c_text = :c_text, d_text = :d_text WHERE id = :id') def _strip_surrogates(value): - return value.encode('utf-8', errors='replace').decode('utf-8') if isinstance(value, str) else value + return strip_surrogates(value) if isinstance(value, str) else value while True: # Read the next batch as plain columns and let the read transaction end. Indexers can run