diff --git a/cytotable/convert.py b/cytotable/convert.py index 8662f16e..454f78a4 100644 --- a/cytotable/convert.py +++ b/cytotable/convert.py @@ -53,7 +53,11 @@ def _get_table_columns_and_types( import duckdb - from cytotable.utils import _duckdb_reader, _sqlite_mixed_type_query_to_parquet + from cytotable.utils import ( + _duckdb_reader, + _raise_if_sqlite_readonly_error, + _sqlite_mixed_type_query_to_parquet, + ) source_path = source["source_path"] source_type = str(source_path.suffix).lower() @@ -113,6 +117,8 @@ def _get_table_columns_and_types( ) except duckdb.Error as e: + _raise_if_sqlite_readonly_error(e, source_path=source_path) + # if we see a mismatched type error # run a more nuanced query through sqlite # to handle the mixed types @@ -320,6 +326,12 @@ def _get_table_keyset_pagination_sets( Returns: Union[List[Optional[Tuple[Union[int, float], Union[int, float]]]], List[None], None]: List of keys to use for reading the data later on. + + Raises: + duckdb.Error: + Re-raised (or converted to SQLiteReadOnlyException) when the + underlying duckdb query fails for a reason other than mixed-type + or invalid-input errors on a sqlite source. """ import logging @@ -329,7 +341,11 @@ def _get_table_keyset_pagination_sets( import duckdb from cytotable.exceptions import NoInputDataException - from cytotable.utils import _duckdb_reader, _generate_pagesets + from cytotable.utils import ( + _duckdb_reader, + _generate_pagesets, + _raise_if_sqlite_readonly_error, + ) logger = logging.getLogger(__name__) @@ -373,12 +389,21 @@ def _get_table_keyset_pagination_sets( duckdb.InvalidInputException, NoInputDataException, ) as invalid_input_exc: + if isinstance(invalid_input_exc, duckdb.InvalidInputException): + _raise_if_sqlite_readonly_error( + invalid_input_exc, source_path=source_path + ) + logger.warning( msg=f"Skipping file due to input file errors: {str(invalid_input_exc)}" ) return None + except duckdb.Error as e: + _raise_if_sqlite_readonly_error(e, source_path=source_path) + raise + elif sql_stmt is not None: with _duckdb_reader() as ddb_reader: sql_query = f"SELECT {page_key} FROM ({sql_stmt}) ORDER BY {page_key}" @@ -436,6 +461,7 @@ def _source_pageset_to_parquet( from cytotable.utils import ( _duckdb_reader, _extract_npz_to_parquet, + _raise_if_sqlite_readonly_error, _sqlite_mixed_type_query_to_parquet, _write_parquet_table_with_metadata, ) @@ -536,6 +562,8 @@ def _source_pageset_to_parquet( # Include exception handling to read mixed-type data # using sqlite3 and special utility function. except duckdb.Error as e: + _raise_if_sqlite_readonly_error(e, source_path=source["source_path"]) + # if we see a mismatched type error # run a more nuanced query through sqlite # to handle the mixed types diff --git a/cytotable/exceptions.py b/cytotable/exceptions.py index ec6bb37d..e36c05e7 100644 --- a/cytotable/exceptions.py +++ b/cytotable/exceptions.py @@ -26,3 +26,12 @@ class SchemaException(CytoTableException): """ Exception for schema challenges. """ + + +class SQLiteReadOnlyException(CytoTableException): + """ + Exception for when a SQLite source cannot be opened due to it + being in WAL journal mode without write access to its directory + (SQLite requires the ability to create '-wal'/'-shm' companion + files even for read-only queries against a WAL-mode database). + """ diff --git a/cytotable/sources.py b/cytotable/sources.py index 5053cc9b..40dd810a 100644 --- a/cytotable/sources.py +++ b/cytotable/sources.py @@ -70,15 +70,24 @@ def _get_source_filepaths( since at least one is required to identify source files. NoInputDataException: Raised when no input files are found at ``path``. + duckdb.Error: + Re-raised (or converted to SQLiteReadOnlyException) when querying + a sqlite source's table list fails for a reason other than the + known WAL/readonly failure mode. """ import os import pathlib + import duckdb from cloudpathlib import AnyPath, CloudPath from cytotable.exceptions import DatatypeException, NoInputDataException - from cytotable.utils import _cache_cloudpath_to_local, _duckdb_reader + from cytotable.utils import ( + _cache_cloudpath_to_local, + _duckdb_reader, + _raise_if_sqlite_readonly_error, + ) if (targets is None or targets == []) and source_datatype is None: raise DatatypeException( @@ -136,6 +145,27 @@ def _get_source_filepaths( for element in sources: # check that the path is of sqlite type if element["source_path"].suffix.lower() == ".sqlite": + # perform a query to find the table names from the sqlite file + try: + table_names = ( + ddb_reader.execute( + """ + /* perform query on sqlite_master table for metadata on tables */ + SELECT name as table_name + FROM sqlite_scan(?, 'sqlite_master') + WHERE type='table' + """, + parameters=[str(element["source_path"])], + ) + .fetch_arrow_table()["table_name"] + .to_pylist() + ) + except duckdb.Error as e: + _raise_if_sqlite_readonly_error( + e, source_path=element["source_path"] + ) + raise + # creates individual entries for each table expanded_sources += [ { @@ -144,18 +174,7 @@ def _get_source_filepaths( ), "table_name": table_name, } - # perform a query to find the table names from the sqlite file - for table_name in ddb_reader.execute( - """ - /* perform query on sqlite_master table for metadata on tables */ - SELECT name as table_name - FROM sqlite_scan(?, 'sqlite_master') - WHERE type='table' - """, - parameters=[str(element["source_path"])], - ) - .fetch_arrow_table()["table_name"] - .to_pylist() + for table_name in table_names # make sure the table names match with compartment + metadata names if targets is not None and any(target.lower() in table_name.lower() for target in targets) diff --git a/cytotable/utils.py b/cytotable/utils.py index 79e38c0f..31102570 100644 --- a/cytotable/utils.py +++ b/cytotable/utils.py @@ -212,6 +212,63 @@ def _duckdb_reader() -> duckdb.DuckDBPyConnection: ) +def _raise_if_sqlite_readonly_error( + duckdb_exc: duckdb.Error, source_path: Union[str, pathlib.Path] +) -> None: + """ + Detects the "attempt to write a readonly database" error which + DuckDB's sqlite_scanner raises when a SQLite source is in WAL + journal mode and lacks write access to its directory (SQLite + requires the ability to create '-wal'/'-shm' companion files even + for read-only queries against a WAL-mode database, so this is not + avoidable via read-only connection settings). When detected, this + raises a more actionable SQLiteReadOnlyException with a suggested + fix; otherwise it returns without raising, leaving the original + exception for the caller to handle. + + Args: + duckdb_exc (duckdb.Error): + The duckdb error raised while scanning a SQLite source. + source_path (Union[str, pathlib.Path]): + Path to the SQLite source which triggered the error. + + Raises: + SQLiteReadOnlyException: + When the underlying error matches the known WAL/readonly + failure mode. + """ + + import shlex + + from cytotable.exceptions import SQLiteReadOnlyException + + if "attempt to write a readonly database" not in str(duckdb_exc).lower(): + return + + source_path_arg = shlex.quote(str(source_path)) + + raise SQLiteReadOnlyException( + f"Unable to read SQLite source '{source_path}' because it appears to be in " + "WAL journal mode without write access to its directory. SQLite requires " + "the ability to create '-wal'/'-shm' companion files even for read-only " + "queries against a WAL-mode database, so this cannot be resolved through " + "read-only connection settings alone. This commonly happens when a .sqlite " + "file is copied without its '-wal'/'-shm' companion files, or is accessed " + "from read-only storage.\n\n" + "To fix this, run the following once on a system where you have write " + "access to the file, then retry:\n\n" + f" sqlite3 {source_path_arg} 'PRAGMA journal_mode=DELETE;'\n\n" + "This checkpoints the database out of WAL mode permanently. This is a safe " + "change to make: DELETE is SQLite's original and still most widely used " + "journal mode, it does not alter or remove any data in the database, and it " + "does not require the database to be closed or otherwise unavailable " + "beforehand. The only trade-off is that DELETE mode does not allow readers " + "and writers to access the database simultaneously (unlike WAL mode), which " + "is not a concern for a database that is being read from as a static " + "source, as is the case here." + ) from duckdb_exc + + def _sqlite_mixed_type_query_to_parquet( source_path: str, table_name: str, diff --git a/tests/test_convert.py b/tests/test_convert.py index a314e7ea..27d3159d 100644 --- a/tests/test_convert.py +++ b/tests/test_convert.py @@ -10,6 +10,9 @@ import itertools import os import pathlib +import shutil +import sqlite3 +from contextlib import closing from importlib.util import find_spec from shutil import copy from typing import Any, Dict, List, Tuple, cast @@ -28,6 +31,7 @@ from cytotable.convert import ( _concat_join_sources, _concat_source_group, + _get_table_keyset_pagination_sets, _infer_source_group_common_schema, _join_source_pageset, _prepare_join_sql, @@ -35,7 +39,7 @@ _run_export_workflow, convert, ) -from cytotable.exceptions import CytoTableException +from cytotable.exceptions import CytoTableException, SQLiteReadOnlyException from cytotable.presets import config from cytotable.sources import _infer_source_datatype from cytotable.utils import ( @@ -862,6 +866,46 @@ def test_infer_source_datatype( _infer_source_datatype(sources=data) +def test_get_table_keyset_pagination_sets_with_wal_mode_readonly_sqlite( + load_parsl_default: None, fx_tempdir: str +): + """ + Tests that pagination raises SQLiteReadOnlyException for a read-only + WAL-mode sqlite source missing its '-wal'/'-shm' companion files. + """ + + tmp_dir_path = pathlib.Path(fx_tempdir) + + source_dir = tmp_dir_path / "pagination_wal_source" + source_dir.mkdir() + source_path = source_dir / "example.sqlite" + with closing(sqlite3.connect(source_path)) as conn: + with conn: + conn.execute("PRAGMA journal_mode=WAL;") + conn.execute("CREATE TABLE Image (ImageNumber INTEGER);") + conn.execute("INSERT INTO Image VALUES (1);") + conn.commit() + + readonly_dir = tmp_dir_path / "pagination_readonly" + readonly_dir.mkdir() + readonly_path = readonly_dir / "example.sqlite" + shutil.copy(source_path, readonly_path) + + os.chmod(readonly_path, 0o444) + os.chmod(readonly_dir, 0o555) + + try: + with pytest.raises(SQLiteReadOnlyException, match="journal_mode=DELETE"): + _get_table_keyset_pagination_sets( + chunk_size=1000, + page_key="ImageNumber", + source={"table_name": "Image", "source_path": readonly_path}, + ).result() + finally: + os.chmod(readonly_dir, 0o755) + os.chmod(readonly_path, 0o644) + + def test_run_export_workflow( load_parsl_default: None, fx_tempdir: str, diff --git a/tests/test_sources.py b/tests/test_sources.py index ea369824..cb8d5d55 100644 --- a/tests/test_sources.py +++ b/tests/test_sources.py @@ -2,9 +2,16 @@ Testing for cytotable/sources.py """ +import os import pathlib +import shutil +import sqlite3 import tempfile +from contextlib import closing +import pytest + +from cytotable.exceptions import SQLiteReadOnlyException from cytotable.sources import _file_is_more_than_one_line, _get_source_filepaths @@ -67,3 +74,66 @@ def test_get_source_filepaths_with_npz(): "test_file.npz" in str(file["source_path"]) for file in result[next(iter(result))] ) + + +def test_get_source_filepaths_with_wal_mode_readonly_sqlite(): + """ + Tests that _get_source_filepaths raises a helpful SQLiteReadOnlyException + (instead of an opaque duckdb.Error) when a source .sqlite file is left in + WAL journal mode and is missing its '-wal'/'-shm' companion files while + the file and its directory are read-only. This reproduces the "attempt to + write a readonly database" error which SQLite raises even for read-only + queries against such a file. + + This is a meaningful regression check, not a vacuous one: without the + os.chmod calls below to make the file/directory read-only, no exception + is raised at all (SQLite is able to create the missing '-wal'/'-shm' + files on the fly), and if _raise_if_sqlite_readonly_error stops + converting the underlying duckdb.Error, this test fails because a plain + duckdb.Error (not a SQLiteReadOnlyException) propagates instead. + + Note this test is skipped when running as root, since root can bypass + the read-only file permission bits set below, which would prevent the + "attempt to write a readonly database" error from being reproduced. + """ + if hasattr(os, "geteuid") and os.geteuid() == 0: + pytest.skip( + "root bypasses file permission bits, so read-only setup below" + " would not reproduce the target error." + ) + + with tempfile.TemporaryDirectory() as tmp_dir: + tmp_dir_path = pathlib.Path(tmp_dir) + + # create a WAL-mode sqlite source with a single table + source_dir = tmp_dir_path / "source" + source_dir.mkdir() + source_path = source_dir / "example.sqlite" + with closing(sqlite3.connect(source_path)) as conn: + with conn: + conn.execute("PRAGMA journal_mode=WAL;") + conn.execute("CREATE TABLE Image (ImageNumber INTEGER);") + conn.execute("INSERT INTO Image VALUES (1);") + conn.commit() + + # copy only the main db file (omitting -wal/-shm companions), + # simulating a copy/sync which dropped the companion files + readonly_dir = tmp_dir_path / "readonly" + readonly_dir.mkdir() + readonly_path = readonly_dir / "example.sqlite" + shutil.copy(source_path, readonly_path) + + # make the file and its directory read-only + os.chmod(readonly_path, 0o444) + os.chmod(readonly_dir, 0o555) + + try: + with pytest.raises(SQLiteReadOnlyException, match="journal_mode=DELETE"): + _get_source_filepaths( + path=readonly_dir, + targets=["image"], + ) + finally: + # restore write permissions so TemporaryDirectory cleanup succeeds + os.chmod(readonly_dir, 0o755) + os.chmod(readonly_path, 0o644)