From 855d065049ff2e72579fa06ba0aa60648250089e Mon Sep 17 00:00:00 2001 From: nagisml Date: Fri, 2 Oct 2026 21:28:38 +0200 Subject: [PATCH] GSAK import: busy progress, one distance recalc per target, index for NULL check - Switch the progress bar to busy mode while distances are recalculated after a GSAK import (it sat at 100% with no feedback). - When several GSAK databases go into the same target, only the last job recalculates; earlier jobs clear the stored dist_calc_* values, so the existing up-to-date check still catches a skipped/failed last job. - Migration 25: partial index ix_caches_distance_missing (distance IS NULL), so distances_up_to_date()'s NULL check no longer scans the whole table (~1 s -> <1 ms at 180k caches). Follow-up to #956. --- src/opensak/db/database.py | 19 +++++- src/opensak/gui/dialogs/gsak_import_dialog.py | 40 ++++++++++-- tests/unit-tests/test_gsak_import_dialog.py | 61 ++++++++++++++++++- tests/unit-tests/test_migrations.py | 13 ++++ 4 files changed, 125 insertions(+), 8 deletions(-) diff --git a/src/opensak/db/database.py b/src/opensak/db/database.py index 06fe535a..b5a55170 100644 --- a/src/opensak/db/database.py +++ b/src/opensak/db/database.py @@ -68,7 +68,7 @@ def _enable_wal_and_fk(dbapi_connection, _connection_record) -> None: # bumped to the highest migration number whenever a new migration is added # below — _run_migrations() skips the whole block when the database already # reports this version, so a stale constant means new migrations never run. -SCHEMA_VERSION = 24 +SCHEMA_VERSION = 25 def init_db(db_path: Path | None = None) -> Engine: @@ -985,6 +985,19 @@ def _run_migrations(engine: Engine) -> None: conn.commit() print(f"Migration: tilføjede caches.{', caches.'.join(added_23)}") + # ── Migration 25: partial index on caches missing a distance ───────── + logger.debug("[migrations] entering migration 25 (+%.2fs)", time.monotonic() - _mig_t0) + # distances_up_to_date() looks for a cache without a distance on + # every startup and database switch. On an up-to-date database that + # is a full table scan (~1 s at 180k caches); this index holds only + # the rows still missing one, so the lookup is instant. Rows leave it + # as recalculate_distances() fills them, so its upkeep is negligible. + conn.execute(text( + "CREATE INDEX IF NOT EXISTS ix_caches_distance_missing " + "ON caches (id) WHERE distance IS NULL" + )) + conn.commit() + # ── Stamp the schema version so the next launch skips the probes ───── # PRAGMA does not accept bind parameters; SCHEMA_VERSION is a trusted # int constant, so inlining it is safe. @@ -1302,8 +1315,8 @@ def distances_up_to_date(lat: float, lon: float, db_path: Path | None = None) -> if abs(fresh_distance - stored_distance) > _DISTANCE_EPSILON_KM: return False - # Stops at the first hit; only a database that is up to date pays for - # the full scan. + # Served by the partial index ix_caches_distance_missing (migration 25), + # so this stays cheap on large databases. with get_session() as session: missing = session.execute( text( diff --git a/src/opensak/gui/dialogs/gsak_import_dialog.py b/src/opensak/gui/dialogs/gsak_import_dialog.py index b5de6384..5cfab3b2 100644 --- a/src/opensak/gui/dialogs/gsak_import_dialog.py +++ b/src/opensak/gui/dialogs/gsak_import_dialog.py @@ -89,11 +89,14 @@ class GsakImportWorker(QThread): # from inside run() itself). def __init__(self, db3_path: Path, target_db_path: Path | None = None, - replace: bool = False): + replace: bool = False, update_distances: bool = True): super().__init__() self.db3_path = db3_path self.target_db_path = target_db_path # None → use currently active DB self.replace = replace # empty the target DB first + # False when a later job of the same run imports into the same + # target: that one recalculates, once for all of them. + self.update_distances = update_distances def run(self) -> None: from opensak.db.database import get_session, init_db @@ -120,7 +123,13 @@ 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) + db_path = self.target_db_path or original_path + if self.update_distances: + # Busy mode — the import's own progress sits at 100% meanwhile. + self.progress.emit(0, 0) + self._update_distances(db_path) + else: + self._invalidate_distances(db_path) except Exception: import traceback self.error.emit(traceback.format_exc()) @@ -150,6 +159,26 @@ def _update_distances(db_path: Path | None) -> None: logger.warning("GSAK import: could not update distances for %s", db_path, exc_info=True) + @staticmethod + def _invalidate_distances(db_path: Path | None) -> None: + """Forget the centre *db_path*'s distances were calculated for, so + distances_up_to_date() reports them stale. The target's last job + then recalculates; should it never run (skipped, failed, cancelled), + the check after the import or on switching to that database does. + """ + if db_path is None: + return + try: + from opensak.db import db_settings + db_settings.write_file(db_path, { + "dist_calc_lat": None, + "dist_calc_lon": None, + "dist_calc_method": None, + }) + except Exception: + logger.warning("GSAK import: could not invalidate distances for %s", + db_path, exc_info=True) + class GsakExtractWorker(QThread): """Unpacks the selected members of a GSAK backup .zip in the background. @@ -727,8 +756,11 @@ def _import_current_job(self) -> None: self._progress.setRange(0, 0) self._append_log(tr("gsak_import_running", name=f"{job.gsak_name} → {job.target_name}")) - worker = GsakImportWorker(job.db3_path, target_db_path=job.target_path, - replace=job.replace) + later = self._jobs[self._job_index + 1:] + worker = GsakImportWorker( + job.db3_path, target_db_path=job.target_path, replace=job.replace, + update_distances=all(j.target_name != job.target_name for j in later), + ) worker.progress.connect(self._on_progress) worker.cleared.connect( lambda count, name=job.target_name: diff --git a/tests/unit-tests/test_gsak_import_dialog.py b/tests/unit-tests/test_gsak_import_dialog.py index 1fc1f2b8..fcabe24a 100644 --- a/tests/unit-tests/test_gsak_import_dialog.py +++ b/tests/unit-tests/test_gsak_import_dialog.py @@ -70,7 +70,8 @@ def fake_import(path, session, progress_cb=None): seen = [] w.progress.connect(lambda done, total: seen.append((done, total))) w.run() - assert seen == [(5, 10)] + # (0, 0): busy mode while the distances are recalculated + assert seen == [(5, 10), (0, 0)] def test_run_exception_emits_error(self, monkeypatch): self._patch_common(monkeypatch) @@ -127,6 +128,22 @@ def test_run_updates_distances_of_target(self, monkeypatch): GsakImportWorker(Path("/gsak.db3")).run() # no target → the active DB assert updated == [Path("/other.db"), Path("/active.db")] + def test_run_without_update_distances_invalidates_them(self, monkeypatch): + updated = self._patch_common(monkeypatch) + invalidated = [] + monkeypatch.setattr(GsakImportWorker, "_invalidate_distances", + staticmethod(invalidated.append)) + 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) + w = GsakImportWorker(Path("/gsak.db3"), target_db_path=Path("/other.db"), + update_distances=False) + seen = [] + w.progress.connect(lambda done, total: seen.append((done, total))) + w.run() + assert updated == [] and invalidated == [Path("/other.db")] + assert seen == [] + def test_run_failed_import_does_not_update_distances(self, monkeypatch): updated = self._patch_common(monkeypatch) monkeypatch.setattr( @@ -450,6 +467,48 @@ def test_distances_are_calculated_for_each_target_database( finally: manager.ensure_active_initialised() + def test_distances_are_calculated_once_per_target( + self, dlg, manager, tmp_path, qtbot, monkeypatch): + """Several GSAK databases into one target: only its last job + recalculates; the earlier ones mark the distances stale instead.""" + from opensak.db import db_settings + flags = [] + real_init = GsakImportWorker.__init__ + + def spy(self, *a, **k): + real_init(self, *a, **k) + flags.append(self.update_distances) + + monkeypatch.setattr(GsakImportWorker, "__init__", spy) + dlg.set_path(_backup(tmp_path, {"A": "GC1AAA", "B": "GC2BBB", "C": "GC3CCC"}, + with_settings=False)) + _target(dlg, 1).setCurrentText("A") + _run(dlg, qtbot) + + assert flags == [False, True, True] + path = _db_by_name(manager, "A").path + assert _codes(path) == ["GC1AAA", "GC2BBB"] + conn = sqlite3.connect(path) + try: + assert conn.execute( + "SELECT COUNT(*) FROM caches WHERE distance IS NULL").fetchone()[0] == 0 + finally: + conn.close() + assert db_settings.read_file(path)["dist_calc_lat"] is not None + + def test_invalidated_distances_are_reported_stale(self, manager, tmp_path): + """What a target's earlier jobs leave behind if its last job never + runs: the check after the import / on switching recalculates.""" + from opensak.db import db_settings + from opensak.db.database import distances_up_to_date + path = manager.new_database("Stale").path + db_settings.write_file(path, {"dist_calc_lat": 55.0, "dist_calc_lon": 12.0, + "dist_calc_method": "haversine"}) + GsakImportWorker._invalidate_distances(path) + stored = db_settings.read_file(path) + assert stored["dist_calc_lat"] is None and stored["dist_calc_method"] is None + assert db_settings.peek_value(path, "dist_calc_lat") is None + def test_only_ticked_databases_are_imported(self, dlg, manager, tmp_path, qtbot): dlg.set_path(_backup(tmp_path, {"A": "GC1AAA", "B": "GC2BBB"}, with_settings=False)) dlg._table.item(1, gdlg.COL_GSAK).setCheckState(Qt.CheckState.Unchecked) diff --git a/tests/unit-tests/test_migrations.py b/tests/unit-tests/test_migrations.py index 0631d97b..81c9aa4e 100644 --- a/tests/unit-tests/test_migrations.py +++ b/tests/unit-tests/test_migrations.py @@ -451,3 +451,16 @@ def test_found_log_count_backfill_skipped_without_username_configured(tmp_path): assert "found_log_count" in cache_cols assert found_log_count == 0 + + +def test_missing_distance_lookup_uses_partial_index(tmp_path): + # Migration 25: distances_up_to_date()'s NULL check must not scan the table. + init_db(db_path=tmp_path / "idx.db") + with get_engine().connect() as c: + plan = " ".join( + str(row[-1]) for row in c.execute(text( + "EXPLAIN QUERY PLAN SELECT 1 FROM caches WHERE distance IS NULL " + "AND latitude IS NOT NULL AND longitude IS NOT NULL LIMIT 1" + )) + ) + assert "ix_caches_distance_missing" in plan, plan