From 4b2554e347aa72f73f43b6b57f38b588fa8f3c99 Mon Sep 17 00:00:00 2001 From: nagisml Date: Thu, 1 Oct 2026 20:48:50 +0200 Subject: [PATCH 1/2] Fix proposal for #957 --- src/opensak/db/database.py | 127 ++++++++++++++---- src/opensak/email/service.py | 32 ++--- src/opensak/gui/dialogs/gsak_import_dialog.py | 18 +-- src/opensak/gui/dialogs/import_dialog.py | 108 +++++++-------- src/opensak/gui/dialogs/move_caches_dialog.py | 22 +-- src/opensak/importer/__init__.py | 13 +- tests/unit-tests/test_db.py | 59 ++++++++ tests/unit-tests/test_gsak_import_dialog.py | 17 ++- tests/unit-tests/test_import_dialog.py | 16 ++- tests/unit-tests/test_pq_email_service.py | 4 +- 10 files changed, 272 insertions(+), 144 deletions(-) diff --git a/src/opensak/db/database.py b/src/opensak/db/database.py index 06fe535a..c8b5cd91 100644 --- a/src/opensak/db/database.py +++ b/src/opensak/db/database.py @@ -103,39 +103,67 @@ def init_db(db_path: Path | None = None) -> Engine: from opensak.config import get_db_path db_path = get_db_path() - new_engine = _make_engine(db_path) - new_session = sessionmaker( - bind=new_engine, + t0 = time.monotonic() + # Opening (create_all + migrations) happens before touching the globals — + # if the file is not a valid SQLite database _open_engine() raises and the + # current engine is unaffected. + new_engine = _open_engine(db_path) + + # Only swap the global pointers after everything above succeeded. The + # replaced engine is disposed so repeated init_db() calls don't leave one + # connection pool per call behind. Sessions still holding a connection + # from it keep working; the connection is closed when they return it. + old_engine = _engine + _engine = new_engine + _SessionLocal = _session_factory(new_engine) + if old_engine is not None and old_engine is not new_engine: + old_engine.dispose() + logger.info("init_db: ready (+%.2fs total)", time.monotonic() - t0) + return _engine + + +def _session_factory(engine: Engine) -> sessionmaker: + """Session factory with the settings every OpenSAK session uses.""" + return sessionmaker( + bind=engine, autoflush=False, autocommit=False, expire_on_commit=False, # keep objects usable after session closes ) - # Issue #549: if opening this database is about to migrate its schema, - # save a one-time copy of it first. This must happen before create_all(), - # which already adds any new tables to an old database. Best-effort: - # backup_before_migration() never raises and never blocks the migration. - if db_path not in _migrated_paths: - backup_before_migration(db_path, SCHEMA_VERSION) - - # Create all tables that don't exist yet (safe to call multiple times). - # Do this before touching the globals — if the file is not a valid SQLite - # database create_all() raises here and the current engine is unaffected. - logger.info("init_db: opening %s", db_path) - t0 = time.monotonic() - Base.metadata.create_all(new_engine) - logger.info("init_db: create_all() done (+%.2fs)", time.monotonic() - t0) - # Kør schema-migrationer for eksisterende databaser (kun én gang per DB-sti) - if db_path not in _migrated_paths: - _run_migrations(new_engine) - _migrated_paths.add(db_path) +def _open_engine(db_path: Path) -> Engine: + """ + Create an engine for *db_path* and bring its schema up to date + (create_all + migrations), without touching the module globals. - # Only swap the global pointers after everything above succeeded. - _engine = new_engine - _SessionLocal = new_session - logger.info("init_db: ready (+%.2fs total)", time.monotonic() - t0) - return _engine + Shared by init_db() (the app's active database) and session_for() + (a private engine for a background job on another database). + """ + new_engine = _make_engine(db_path) + try: + # Issue #549: if opening this database is about to migrate its schema, + # save a one-time copy of it first. This must happen before + # create_all(), which already adds any new tables to an old database. + # Best-effort: backup_before_migration() never raises and never + # blocks the migration. + if db_path not in _migrated_paths: + backup_before_migration(db_path, SCHEMA_VERSION) + + # Create all tables that don't exist yet (safe to call multiple times). + logger.info("open_db: opening %s", db_path) + t0 = time.monotonic() + Base.metadata.create_all(new_engine) + logger.info("open_db: create_all() done (+%.2fs)", time.monotonic() - t0) + + # Kør schema-migrationer for eksisterende databaser (kun én gang per DB-sti) + if db_path not in _migrated_paths: + _run_migrations(new_engine) + _migrated_paths.add(db_path) + except Exception: + new_engine.dispose() + raise + return new_engine # ── Schema migrationer ──────────────────────────────────────────────────────── @@ -1061,8 +1089,51 @@ def get_session() -> Generator[Session, None, None]: session.close() -def make_session(): - """Return a bare Session — caller handles commit/rollback/close.""" +@contextmanager +def session_for(db_path: Path) -> Generator[Session, None, None]: + """ + Like get_session(), but on a private engine for *db_path* — the active + database (init_db()'s globals) is left untouched. + + Use this for background jobs that work on a database other than the + active one (import into another DB, move/copy caches, PQ e-mail import). + Swapping the globals with init_db() from a worker thread instead would + let anything on the GUI thread that calls get_session() meanwhile + (timers, queued signals) read from or write to the wrong database. + + The schema is brought up to date first, exactly as init_db() would, and + the engine is disposed when the block exits. + + Example + ------- + with session_for(other_db) as session: + session.add(some_object) + """ + engine = _open_engine(db_path) + try: + session: Session = _session_factory(engine)() + try: + yield session + session.commit() + except Exception: + session.rollback() + raise + finally: + session.close() + finally: + engine.dispose() + + +def make_session(engine: Engine | None = None): + """ + Return a bare Session — caller handles commit/rollback/close. + + Bound to the active database, or to *engine* when given (e.g. the bind + of a session_for() session, so a helper opens its own session on the + same database as the session it was handed). + """ + if engine is not None: + return _session_factory(engine)() if _SessionLocal is None: raise RuntimeError("Database not initialised — call init_db() first.") return _SessionLocal() diff --git a/src/opensak/email/service.py b/src/opensak/email/service.py index aafd3489..aeeec7db 100644 --- a/src/opensak/email/service.py +++ b/src/opensak/email/service.py @@ -75,32 +75,24 @@ def _match_database(pq_name: str) -> "DatabaseInfo | None": def _import_zip_bytes(zip_bytes: bytes, target_db_path: Path | None) -> "ImportResult": """ Importér `zip_bytes` i den angivne database (eller den aktuelt - aktive, hvis `target_db_path` er None), og gendan bagefter altid - den oprindelige aktive database. + aktive, hvis `target_db_path` er None). - Samme mønster som ImportWorker i import_dialog.py — se dens - docstring for hvorfor: init_db() skifter en global session-factory, - så den skal altid gendannes i en `finally`-blok, uanset resultatet. + En anden database end den aktive får sin egen session via + session_for() — den globale engine (init_db()) røres ikke, så + GUI-tråden kan ikke ramme den forkerte database imens. """ - from opensak.db.database import get_session, init_db + from opensak.db.database import get_session, session_for from opensak.db.manager import get_db_manager from opensak.importer import import_zip - manager = get_db_manager() - original_path = manager.active_path - switched = target_db_path is not None and target_db_path != original_path - if switched: - init_db(db_path=target_db_path) + active_path = get_db_manager().active_path + other_db = target_db_path is not None and target_db_path != active_path - try: - with tempfile.TemporaryDirectory() as tmpdir: - zip_path = Path(tmpdir) / "pq_email.zip" - zip_path.write_bytes(zip_bytes) - with get_session() as session: - return import_zip(zip_path, session) - finally: - if switched and original_path is not None: - init_db(db_path=original_path) + with tempfile.TemporaryDirectory() as tmpdir: + zip_path = Path(tmpdir) / "pq_email.zip" + zip_path.write_bytes(zip_bytes) + with (session_for(target_db_path) if other_db else get_session()) as session: + return import_zip(zip_path, session) def scan_and_import( diff --git a/src/opensak/gui/dialogs/gsak_import_dialog.py b/src/opensak/gui/dialogs/gsak_import_dialog.py index b5de6384..55560ef0 100644 --- a/src/opensak/gui/dialogs/gsak_import_dialog.py +++ b/src/opensak/gui/dialogs/gsak_import_dialog.py @@ -96,23 +96,22 @@ def __init__(self, db3_path: Path, target_db_path: Path | None = None, self.replace = replace # empty the target DB first def run(self) -> None: - from opensak.db.database import get_session, init_db + from opensak.db.database import get_session, session_for from opensak.db.manager import get_db_manager from opensak.importer.gsak_importer import ( clear_opensak_cache_data, import_gsak_db, ) - manager = get_db_manager() - original_path = manager.active_path - switched = ( + # Another database than the active one gets a private session — + # never swap the app-wide engine from this thread (see session_for). + active_path = get_db_manager().active_path + other_db = ( self.target_db_path is not None - and self.target_db_path != original_path + and self.target_db_path != active_path ) - if switched: - init_db(db_path=self.target_db_path) try: - with get_session() as session: + with (session_for(self.target_db_path) if other_db else get_session()) as session: if self.replace: self.cleared.emit(clear_opensak_cache_data(session)) result = import_gsak_db( @@ -124,9 +123,6 @@ def run(self) -> None: except Exception: import traceback self.error.emit(traceback.format_exc()) - finally: - if switched and original_path is not None: - init_db(db_path=original_path) @staticmethod def _update_distances(db_path: Path | None) -> None: diff --git a/src/opensak/gui/dialogs/import_dialog.py b/src/opensak/gui/dialogs/import_dialog.py index 880cbc8e..e689bf5e 100644 --- a/src/opensak/gui/dialogs/import_dialog.py +++ b/src/opensak/gui/dialogs/import_dialog.py @@ -40,70 +40,66 @@ def __init__(self, paths: list[Path], target_db_path: Path | None = None): self.target_db_path = target_db_path # None → use currently active DB def run(self) -> None: - from opensak.db.database import get_session, init_db + from opensak.db.database import get_session, session_for from opensak.db.manager import get_db_manager from opensak.importer import import_gpx, import_zip, _count_wpts, _is_companion_gpx from opensak.utils.utils import get_import_type, ImportType - # Switch to target DB if different from active - manager = get_db_manager() - original_path = manager.active_path - switched = ( + # Another database than the active one gets a private session — + # never swap the app-wide engine from this thread (see session_for). + active_path = get_db_manager().active_path + other_db = ( self.target_db_path is not None - and self.target_db_path != original_path + and self.target_db_path != active_path ) - if switched: - init_db(db_path=self.target_db_path) - try: - for i, path in enumerate(self.paths): - self.file_started.emit(i, path.name) - try: - import_type: ImportType = get_import_type(path) - - if import_type == ImportType.GPX: - try: - self.total.emit(_count_wpts(path)) - except Exception: - self.total.emit(-1) - # Find a companion file by inspecting GPX content, - # not by assuming a specific filename convention. - wpts_path = None - try: - wpts_path = next( - ( - f for f in sorted(path.parent.glob("*.gpx")) - if f != path and _is_companion_gpx(f) - ), - None, - ) - except Exception: - pass - with get_session() as session: - result = import_gpx( - path, session, - wpts_path=wpts_path, - progress_cb=self.progress.emit, - ) - else: + def _session(): + return session_for(self.target_db_path) if other_db else get_session() + + for i, path in enumerate(self.paths): + self.file_started.emit(i, path.name) + try: + import_type: ImportType = get_import_type(path) + + if import_type == ImportType.GPX: + try: + self.total.emit(_count_wpts(path)) + except Exception: self.total.emit(-1) - with get_session() as session: - result = import_zip( - path, session, - progress_cb=self.progress.emit, - ) - - self.file_finished.emit(i, result) - - except ValueError as e: - self.file_error.emit(i, str(e)) - except Exception: - import traceback - self.file_error.emit(i, traceback.format_exc()) - finally: - # Always restore the original active DB - if switched and original_path is not None: - init_db(db_path=original_path) + # Find a companion file by inspecting GPX content, + # not by assuming a specific filename convention. + wpts_path = None + try: + wpts_path = next( + ( + f for f in sorted(path.parent.glob("*.gpx")) + if f != path and _is_companion_gpx(f) + ), + None, + ) + except Exception: + pass + with _session() as session: + result = import_gpx( + path, session, + wpts_path=wpts_path, + progress_cb=self.progress.emit, + ) + else: + self.total.emit(-1) + with _session() as session: + result = import_zip( + path, session, + progress_cb=self.progress.emit, + ) + + self.file_finished.emit(i, result) + + except ValueError as e: + self.file_error.emit(i, str(e)) + except Exception: + import traceback + self.file_error.emit(i, traceback.format_exc()) # No explicit completion signal — QThread.finished fires automatically # once this method returns, and the dialog reacts to that instead. diff --git a/src/opensak/gui/dialogs/move_caches_dialog.py b/src/opensak/gui/dialogs/move_caches_dialog.py index be06897a..73799f60 100644 --- a/src/opensak/gui/dialogs/move_caches_dialog.py +++ b/src/opensak/gui/dialogs/move_caches_dialog.py @@ -44,17 +44,19 @@ def __init__( self.copy_only = copy_only def run(self) -> None: - from opensak.db.database import init_db, get_session + from opensak.db.database import session_for from opensak.db.models import ( Cache, Log, Attribute, Trackable, Waypoint, UserNote, ) from sqlalchemy.orm import joinedload, selectinload + # Both databases get private sessions (session_for) — the app-wide + # engine is never swapped from this thread, so the GUI thread keeps + # reading the active database while the move runs. try: # ── 1. Load full caches from source DB ──────────────────────── - init_db(db_path=self.source_db_path) cache_snapshots = [] - with get_session() as session: + with session_for(self.source_db_path) as session: caches = ( session.query(Cache) .options( @@ -77,15 +79,13 @@ def run(self) -> None: return # ── 2. Insert into target DB ────────────────────────────────── - init_db(db_path=self.target_db_path) - with get_session() as session: + with session_for(self.target_db_path) as session: for snap in cache_snapshots: _insert_snapshot(session, snap) # ── 3. Delete from source DB (move only) ────────────────── if not self.copy_only: - init_db(db_path=self.source_db_path) - with get_session() as session: + with session_for(self.source_db_path) as session: cache_ids = [ row[0] for row in session.query(Cache.id) @@ -112,17 +112,9 @@ def run(self) -> None: Cache.id.in_(cache_ids) ).delete(synchronize_session=False) - # ── 4. Restore source DB as active ──────────────────────────── - init_db(db_path=self.source_db_path) - self.finished.emit(len(cache_snapshots)) except Exception as exc: - # Always try to restore the source DB - try: - init_db(db_path=self.source_db_path) - except Exception: - pass self.error.emit(str(exc)) diff --git a/src/opensak/importer/__init__.py b/src/opensak/importer/__init__.py index 8c914c9c..ff276107 100644 --- a/src/opensak/importer/__init__.py +++ b/src/opensak/importer/__init__.py @@ -978,6 +978,12 @@ def _parse_loc_waypoint(wpt_el) -> Optional[dict]: # ── DB upsert ───────────────────────────────────────────────────────────────── +def _bind_of(session) -> "Engine | None": + """Engine of *session* so an internal session opens on the same database; + None (= the active database) when no real Session was passed.""" + return session.get_bind() if isinstance(session, Session) else None + + def _load_existing_gc_map(session: Session) -> dict[str, int]: """Return ``{gc_code: cache.id}`` for every cache already in the database. @@ -1725,13 +1731,14 @@ def import_gpx( Import a single GPX file into the database using streaming for high performance. Uses etree.iterparse to handle files of any size without RAM exhaustion. - The session parameter is kept for compatibility but a new session is managed internally. + A new session is managed internally; when *session* is given it is opened + on the same database (its bind), otherwise on the active database. """ from opensak.db.database import make_session result = ImportResult() source = gpx_path.name - db_session = make_session() + db_session = make_session(_bind_of(session)) extra_wpts: list = [] processed_count = 0 @@ -1873,7 +1880,7 @@ def import_zip(zip_path: Path, session: Session | None = None, progress_cb=None) overall_result.errors.append(f"Parse error: {str(e)}") # Step 2: Write all parsed cache data sequentially (single session). - db_session = make_session() + db_session = make_session(_bind_of(session)) existing_ids = _load_existing_gc_map(db_session) _enter_bulk_import_pragmas(db_session) try: diff --git a/tests/unit-tests/test_db.py b/tests/unit-tests/test_db.py index 3100079c..5d6cae57 100644 --- a/tests/unit-tests/test_db.py +++ b/tests/unit-tests/test_db.py @@ -13,6 +13,7 @@ get_engine, make_session, dispose_engine, + session_for, db_health_check, reload_caches_full, ) @@ -263,6 +264,64 @@ def test_disposes_when_path_matches(self, tmp_path): assert database._engine is None +class TestInitDbDisposesReplacedEngine: + def test_old_engine_disposed_on_switch(self, tmp_path, monkeypatch): + init_db(db_path=tmp_path / "a.db") + old = database._engine + disposed = [] + monkeypatch.setattr(old, "dispose", lambda *a, **k: disposed.append(old)) + init_db(db_path=tmp_path / "b.db") + assert database._engine is not old + assert disposed == [old] + + def test_failed_open_keeps_current_engine(self, tmp_path): + init_db(db_path=tmp_path / "ok.db") + current = database._engine + bad = tmp_path / "bad.db" + bad.write_bytes(b"not a sqlite database" * 100) + with pytest.raises(Exception): + init_db(db_path=bad) + assert database._engine is current + + +class TestSessionFor: + def test_does_not_touch_active_engine(self, tmp_path): + init_db(db_path=tmp_path / "active.db") + engine, factory = database._engine, database._SessionLocal + with session_for(tmp_path / "other.db") as s: + s.add(Cache(gc_code="GCOTHER", name="Other", cache_type="Traditional Cache", latitude=1.0, longitude=2.0)) + assert database._engine is engine + assert database._SessionLocal is factory + with get_session() as s: + assert s.query(Cache).filter_by(gc_code="GCOTHER").first() is None + + def test_commits_to_target_db(self, tmp_path): + target = tmp_path / "target.db" + with session_for(target) as s: + s.add(Cache(gc_code="GCTARGET", name="Target", cache_type="Traditional Cache", latitude=1.0, longitude=2.0)) + with session_for(target) as s: + assert s.query(Cache).filter_by(gc_code="GCTARGET").count() == 1 + + def test_rolls_back_on_error(self, tmp_path): + target = tmp_path / "rb.db" + with pytest.raises(RuntimeError): + with session_for(target) as s: + s.add(Cache(gc_code="GCROLL", name="Roll", cache_type="Traditional Cache", latitude=1.0, longitude=2.0)) + s.flush() + raise RuntimeError("boom") + with session_for(target) as s: + assert s.query(Cache).filter_by(gc_code="GCROLL").first() is None + + def test_make_session_binds_to_given_engine(self, tmp_path): + init_db(db_path=tmp_path / "active.db") + with session_for(tmp_path / "other.db") as s: + inner = make_session(s.get_bind()) + try: + assert inner.get_bind() is s.get_bind() + finally: + inner.close() + + class TestModelReprs: def test_waypoint_repr(self): assert "Waypoint" in repr(Waypoint(prefix="PK", cache_id=1)) diff --git a/tests/unit-tests/test_gsak_import_dialog.py b/tests/unit-tests/test_gsak_import_dialog.py index 1fc1f2b8..daf4f942 100644 --- a/tests/unit-tests/test_gsak_import_dialog.py +++ b/tests/unit-tests/test_gsak_import_dialog.py @@ -84,22 +84,31 @@ def test_run_exception_emits_error(self, monkeypatch): w.run() assert errs and "boom" in errs[0] - def test_run_switches_and_restores_db(self, monkeypatch): + def test_run_other_db_uses_private_session(self, monkeypatch): + # Another target DB gets its own session; the active (global) engine + # is never swapped from the worker thread. self._patch_common(monkeypatch, active_path=Path("/active.db")) monkeypatch.setattr("opensak.importer.gsak_importer.import_gsak_db", lambda path, session, progress_cb=None: _result()) - inits = [] + inits, opened = [], [] monkeypatch.setattr("opensak.db.database.init_db", lambda **k: inits.append(k.get("db_path"))) + monkeypatch.setattr("opensak.db.database.get_session", + lambda: pytest.fail("active DB session used for another DB")) + monkeypatch.setattr("opensak.db.database.session_for", + lambda p: opened.append(p) or _fake_session()) w = GsakImportWorker(Path("/gsak.db3"), target_db_path=Path("/other.db")) w.run() - assert inits == [Path("/other.db"), Path("/active.db")] + assert opened == [Path("/other.db")] + assert inits == [] - def test_run_no_switch_when_target_is_active(self, monkeypatch): + def test_run_active_target_uses_active_session(self, monkeypatch): self._patch_common(monkeypatch, active_path=Path("/active.db")) monkeypatch.setattr("opensak.importer.gsak_importer.import_gsak_db", lambda path, session, progress_cb=None: _result()) inits = [] monkeypatch.setattr("opensak.db.database.init_db", lambda **k: inits.append(k.get("db_path"))) + monkeypatch.setattr("opensak.db.database.session_for", + lambda p: pytest.fail("private session opened for the active DB")) w = GsakImportWorker(Path("/gsak.db3"), target_db_path=Path("/active.db")) w.run() assert inits == [] diff --git a/tests/unit-tests/test_import_dialog.py b/tests/unit-tests/test_import_dialog.py index 39d93d48..2587eb18 100644 --- a/tests/unit-tests/test_import_dialog.py +++ b/tests/unit-tests/test_import_dialog.py @@ -78,15 +78,21 @@ def test_run_generic_exception_traceback(self, monkeypatch): w.run() assert errs and "boom" in errs[0] - def test_run_switches_and_restores_db(self, monkeypatch): + def test_run_other_db_uses_private_session(self, monkeypatch): + # Another target DB gets its own session; the active (global) engine + # is never swapped from the worker thread. ImportType = self._patch_common(monkeypatch, active_path=Path("/active.db")) monkeypatch.setattr("opensak.utils.utils.get_import_type", lambda p: ImportType.GPX) - inits = [] + inits, opened = [], [] monkeypatch.setattr("opensak.db.database.init_db", lambda **k: inits.append(k.get("db_path"))) - w = ImportWorker([Path("/a.gpx")], target_db_path=Path("/other.db")) + monkeypatch.setattr("opensak.db.database.get_session", + lambda: pytest.fail("active DB session used for another DB")) + monkeypatch.setattr("opensak.db.database.session_for", + lambda p: opened.append(p) or _fake_session()) + w = ImportWorker([Path("/a.gpx"), Path("/b.gpx")], target_db_path=Path("/other.db")) w.run() - # switched to target, then restored original - assert inits == [Path("/other.db"), Path("/active.db")] + assert opened == [Path("/other.db"), Path("/other.db")] + assert inits == [] def test_run_gpx_passes_companion_wpts_path(self, monkeypatch, tmp_path): # Companion detected by content inspection, not filename. diff --git a/tests/unit-tests/test_pq_email_service.py b/tests/unit-tests/test_pq_email_service.py index 7d1c8141..b4e5c191 100644 --- a/tests/unit-tests/test_pq_email_service.py +++ b/tests/unit-tests/test_pq_email_service.py @@ -88,7 +88,7 @@ def _setup_common_mocks(self, monkeypatch, import_result=None, dbs=None): manager.active_path = manager.active.path monkeypatch.setattr("opensak.db.manager.get_db_manager", lambda: manager) monkeypatch.setattr("opensak.db.database.get_session", _fake_get_session) - monkeypatch.setattr("opensak.db.database.init_db", lambda db_path=None: None) + monkeypatch.setattr("opensak.db.database.session_for", lambda db_path: _fake_get_session()) monkeypatch.setattr( "opensak.importer.import_zip", lambda *a, **kw: import_result if import_result is not None else FakeImportResult(created=1), @@ -244,7 +244,7 @@ def _setup(self, monkeypatch, import_result=None): manager.active_path = north.path monkeypatch.setattr("opensak.db.manager.get_db_manager", lambda: manager) monkeypatch.setattr("opensak.db.database.get_session", _fake_get_session) - monkeypatch.setattr("opensak.db.database.init_db", lambda db_path=None: None) + monkeypatch.setattr("opensak.db.database.session_for", lambda db_path: _fake_get_session()) monkeypatch.setattr( "opensak.importer.import_zip", lambda *a, **kw: import_result or FakeImportResult(created=1), From abe86deb786f0ae7f78ec29a66a9dc8db2fac4a4 Mon Sep 17 00:00:00 2001 From: nagisml Date: Fri, 2 Oct 2026 20:48:28 +0200 Subject: [PATCH 2/2] Adapt #957 session_for refactor to distances-in-worker (#956) After rebasing onto beta, GsakImportWorker still referenced the removed original_path, and recalculate_distances()/distances_up_to_date() assumed the global engine had been switched to db_path. With session_for() it no longer is, so a non-active target's distances would have been computed from and written to the active database. - database: _session_on(db_path) picks get_session() for the active DB and session_for() otherwise; the distance helpers use it. - gsak_import_dialog: update distances of target_db_path or the active DB. - Narrow Optional paths for session_for() and fix the Engine annotation in importer (mypy clean). - test: patch session_for instead of init_db. Co-Authored-By: Claude Opus 5.5 --- src/opensak/db/database.py | 36 ++++++++++++------- src/opensak/email/service.py | 4 ++- src/opensak/gui/dialogs/gsak_import_dialog.py | 14 +++++--- src/opensak/importer/__init__.py | 7 ++-- tests/unit-tests/test_gsak_import_dialog.py | 2 +- 5 files changed, 42 insertions(+), 21 deletions(-) diff --git a/src/opensak/db/database.py b/src/opensak/db/database.py index c8b5cd91..805ef79d 100644 --- a/src/opensak/db/database.py +++ b/src/opensak/db/database.py @@ -20,7 +20,7 @@ from typing import Generator from sqlalchemy import create_engine, event, text -from sqlalchemy.engine import Engine +from sqlalchemy.engine import Connection, Engine from sqlalchemy.orm import Session, sessionmaker from opensak.backup.premigration import backup_before_migration @@ -122,7 +122,7 @@ def init_db(db_path: Path | None = None) -> Engine: return _engine -def _session_factory(engine: Engine) -> sessionmaker: +def _session_factory(engine: Engine | Connection) -> sessionmaker: """Session factory with the settings every OpenSAK session uses.""" return sessionmaker( bind=engine, @@ -1124,7 +1124,19 @@ def session_for(db_path: Path) -> Generator[Session, None, None]: engine.dispose() -def make_session(engine: Engine | None = None): +def _session_on(db_path: Path | None): + """get_session() when *db_path* is None or the database the active + engine already points at, otherwise session_for(db_path) — so a helper + given another database's path never reads or writes the active one.""" + if db_path is None or ( + _engine is not None and _engine.url.database + and Path(_engine.url.database).resolve() == Path(db_path).resolve() + ): + return get_session() + return session_for(db_path) + + +def make_session(engine: Engine | Connection | None = None): """ Return a bare Session — caller handles commit/rollback/close. @@ -1197,12 +1209,12 @@ def recalculate_distances(lat: float, lon: float, db_path: Path | None = None) - refresh). Uses distance_km_batch() which dispatches to Haversine or Vincenty depending on the user's distance_method setting. - *db_path* is the database the open engine points at, when that may not - be the active one — e.g. GsakImportWorker, which switches the engine to - each target database in turn. The centre used is then stored in that - file's own settings (see distances_up_to_date()); without it it goes to - the active database's settings, which while the engine is switched - would be the wrong database. + *db_path* is the database to work on when that may not be the active + one — e.g. GsakImportWorker, which imports into each target database in + turn. Its caches are then read and written through a private session + (see session_for()) and the centre used is stored in that file's own + settings (see distances_up_to_date()); without it both go to the active + database. Returns the number of caches updated. """ @@ -1234,7 +1246,7 @@ def _scalar(la2: float, lo2: float) -> float: return (math.degrees(math.atan2(x2, y2)) + 360) % 360 return [_scalar(la, lo) for la, lo in zip(lats, lons)] - with get_session() as session: + with _session_on(db_path) as session: rows = session.execute( text("SELECT id, latitude, longitude FROM caches WHERE latitude IS NOT NULL AND longitude IS NOT NULL") ).fetchall() @@ -1326,7 +1338,7 @@ def distances_up_to_date(lat: float, lon: float, db_path: Path | None = None) -> # whether it currently has a persisted distance. Checked first (and # unconditionally) so an empty/coordinate-less database is always # considered up to date — there is nothing to recalculate either way. - with get_session() as session: + with _session_on(db_path) as session: row = session.execute( text( "SELECT latitude, longitude, distance FROM caches " @@ -1375,7 +1387,7 @@ def distances_up_to_date(lat: float, lon: float, db_path: Path | None = None) -> # Stops at the first hit; only a database that is up to date pays for # the full scan. - with get_session() as session: + with _session_on(db_path) as session: missing = session.execute( text( "SELECT 1 FROM caches WHERE distance IS NULL " diff --git a/src/opensak/email/service.py b/src/opensak/email/service.py index aeeec7db..a511b498 100644 --- a/src/opensak/email/service.py +++ b/src/opensak/email/service.py @@ -91,7 +91,9 @@ def _import_zip_bytes(zip_bytes: bytes, target_db_path: Path | None) -> "ImportR with tempfile.TemporaryDirectory() as tmpdir: zip_path = Path(tmpdir) / "pq_email.zip" zip_path.write_bytes(zip_bytes) - with (session_for(target_db_path) if other_db else get_session()) as session: + with (session_for(target_db_path) + if other_db and target_db_path is not None + else get_session()) as session: return import_zip(zip_path, session) diff --git a/src/opensak/gui/dialogs/gsak_import_dialog.py b/src/opensak/gui/dialogs/gsak_import_dialog.py index 55560ef0..55d376e3 100644 --- a/src/opensak/gui/dialogs/gsak_import_dialog.py +++ b/src/opensak/gui/dialogs/gsak_import_dialog.py @@ -105,13 +105,16 @@ def run(self) -> None: # Another database than the active one gets a private session — # never swap the app-wide engine from this thread (see session_for). active_path = get_db_manager().active_path + target_path = self.target_db_path or active_path other_db = ( self.target_db_path is not None and self.target_db_path != active_path ) try: - with (session_for(self.target_db_path) if other_db else get_session()) as session: + with (session_for(self.target_db_path) + if other_db and self.target_db_path is not None + else get_session()) as session: if self.replace: self.cleared.emit(clear_opensak_cache_data(session)) result = import_gsak_db( @@ -119,7 +122,7 @@ def run(self) -> None: progress_cb=lambda done, total: self.progress.emit(done, total), ) self.result_ready.emit(result) - self._update_distances(self.target_db_path or original_path) + self._update_distances(target_path) except Exception: import traceback self.error.emit(traceback.format_exc()) @@ -129,9 +132,10 @@ def _update_distances(db_path: Path | None) -> None: """Bring the imported database's distances up to date while still in the background, so neither the refresh after the import nor a later switch to that database has to recalculate them on the GUI thread - (both still check, as a safety net). Runs while the engine points at - *db_path*, so its home point and the centre used are read from and - stored in that file, not in the active database's settings. A failure + (both still check, as a safety net). Works on *db_path* through its own + session when that isn't the active database, and its home point and + the centre used are read from and stored in that file, not in the + active database's settings. A failure here only means that check recalculates later — the import itself succeeded. """ diff --git a/src/opensak/importer/__init__.py b/src/opensak/importer/__init__.py index ff276107..05bc00f0 100644 --- a/src/opensak/importer/__init__.py +++ b/src/opensak/importer/__init__.py @@ -15,7 +15,7 @@ import zipfile from datetime import datetime, timezone from pathlib import Path -from typing import Optional +from typing import TYPE_CHECKING, Optional from lxml import etree from sqlalchemy.orm import Session @@ -23,6 +23,9 @@ from opensak.db.models import Attribute, Cache, Log, Trackable, UserNote, Waypoint +if TYPE_CHECKING: + from sqlalchemy.engine import Connection, Engine + # ── XML namespace map used by Groundspeak Pocket Queries ───────────────────── # Primary namespace map — uses /1/0/1 (newer PQ files) @@ -978,7 +981,7 @@ def _parse_loc_waypoint(wpt_el) -> Optional[dict]: # ── DB upsert ───────────────────────────────────────────────────────────────── -def _bind_of(session) -> "Engine | None": +def _bind_of(session) -> "Engine | Connection | None": """Engine of *session* so an internal session opens on the same database; None (= the active database) when no real Session was passed.""" return session.get_bind() if isinstance(session, Session) else None diff --git a/tests/unit-tests/test_gsak_import_dialog.py b/tests/unit-tests/test_gsak_import_dialog.py index daf4f942..6191c1bd 100644 --- a/tests/unit-tests/test_gsak_import_dialog.py +++ b/tests/unit-tests/test_gsak_import_dialog.py @@ -131,7 +131,7 @@ def test_run_updates_distances_of_target(self, monkeypatch): updated = self._patch_common(monkeypatch, active_path=Path("/active.db")) monkeypatch.setattr("opensak.importer.gsak_importer.import_gsak_db", lambda path, session, progress_cb=None: _result()) - monkeypatch.setattr("opensak.db.database.init_db", lambda **k: None) + monkeypatch.setattr("opensak.db.database.session_for", lambda p: _fake_session()) GsakImportWorker(Path("/gsak.db3"), target_db_path=Path("/other.db")).run() GsakImportWorker(Path("/gsak.db3")).run() # no target → the active DB assert updated == [Path("/other.db"), Path("/active.db")]