From 2a4dcb9be6642bacd29eef8cec01f2c07efd6f90 Mon Sep 17 00:00:00 2001 From: Richard Date: Sat, 20 Jun 2026 11:12:50 -0700 Subject: [PATCH 1/2] fix(ui): correct misleading cross-catalog description comment The shared ObjectsDatabase handle in object_details.py was documented as "only used when cross-catalog descriptions are enabled" -- but there is no such setting and _other_catalog_descriptions() always runs on the description view. Rewrite the comment to state that it always runs and to document the connection lifecycle (lives for the UI process, like the per-instance ObservationsDatabase, released at process exit). No behaviour change; the feature stays always-on (per-object query is a single indexed lookup, skipped entirely for virtual planets/comets/OBS objects). Post-merge cleanup from the review of PR #394 (issue #2). Co-Authored-By: Claude Opus 4.8 (1M context) --- python/PiFinder/ui/object_details.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/python/PiFinder/ui/object_details.py b/python/PiFinder/ui/object_details.py index 8641d5d1..784b995c 100644 --- a/python/PiFinder/ui/object_details.py +++ b/python/PiFinder/ui/object_details.py @@ -35,7 +35,10 @@ # Read-only handle to the catalog DB, opened once and shared across detail -# views (only used when cross-catalog descriptions are enabled). +# views. Used by _other_catalog_descriptions() to pull an object's listings in +# its *other* catalogs (this always runs on the description view). Like the +# per-instance ObservationsDatabase opened below, this read connection lives for +# the life of the UI process and is closed when that process exits. _objects_db = None From 073654145c7d54ac0368bdf3a802beb7ecf1ddcb Mon Sep 17 00:00:00 2001 From: Richard Date: Sat, 20 Jun 2026 11:12:59 -0700 Subject: [PATCH 2/2] fix(obslist): guard read_list resolution loop so a bad entry can't crash the UI read_list only wrapped the file read in try/except; the resolution loop (resolve_object / _build_name_index / resolve_by_name / _coordinate_object) was unprotected. A DB error, uninitialised catalogs, or a malformed entry propagated to UIObsList.key_right, which has no handler -- crashing the UI instead of showing the existing "Error loading" message. - Per-entry guard: one malformed entry is logged and skipped, and the rest of the list still loads (result stays "success", matched < parsed). - Systemic failure (every entry raises, e.g. catalog DB unavailable) is reported as the error dict instead of a silent "0 objects" success. - Outer safety net mirrors the file-read error-dict contract for any failure outside the per-entry guard; it counts len(list_catalog) so it can't re-raise on the same broken `entries`. Return shape is unchanged, so UIObsList.key_right works without modification. Adds unit tests for all three paths (per-entry skip, systemic error, outer net). Post-merge cleanup from the review of PR #394 (issue #4). Co-Authored-By: Claude Opus 4.8 (1M context) --- python/PiFinder/obslist.py | 98 ++++++++++++++++++++-------- python/tests/test_obslist_resolve.py | 90 +++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 27 deletions(-) diff --git a/python/PiFinder/obslist.py b/python/PiFinder/obslist.py index 2cfc9e0f..564bf0ae 100644 --- a/python/PiFinder/obslist.py +++ b/python/PiFinder/obslist.py @@ -292,35 +292,79 @@ def read_list(catalogs: Catalogs, name): list_catalog: list = [] name_index = None # built lazily, only if an entry needs name resolution - for i, entry in enumerate(obs_list.entries): - _object = None - - # Try catalog resolution with catalog_names (skylist multi-name support) - if entry.catalog_names: - _object = resolve_object(entry.catalog_names, catalogs) - elif entry.catalog_code and entry.sequence: - _object = resolve_object( - [f"{entry.catalog_code} {entry.sequence}"], catalogs - ) - - # No catalog match: try resolving by object name (handles lists that - # identify objects only by name, e.g. carbon stars "VY Andromedae"). - if not _object and entry.name: - if name_index is None: - name_index = _build_name_index(catalogs) - _object = resolve_by_name(entry.name, name_index) - - # Resolved objects are shared catalog instances: record this list's - # description under the list name instead of clobbering the catalog one. - if _object and entry.description: - _object.list_descriptions[obs_list.name] = entry.description + errors = 0 + last_error: Exception | None = None + try: + for i, entry in enumerate(obs_list.entries): + try: + _object = None + + # Try catalog resolution with catalog_names (skylist multi-name) + if entry.catalog_names: + _object = resolve_object(entry.catalog_names, catalogs) + elif entry.catalog_code and entry.sequence: + _object = resolve_object( + [f"{entry.catalog_code} {entry.sequence}"], catalogs + ) - # Fall back to coordinate-based object - if not _object and (entry.ra or entry.dec): - _object = _coordinate_object(entry, i) + # No catalog match: try resolving by object name (handles lists + # that identify objects only by name, e.g. "VY Andromedae"). + if not _object and entry.name: + if name_index is None: + name_index = _build_name_index(catalogs) + _object = resolve_by_name(entry.name, name_index) + + # Resolved objects are shared catalog instances: record this + # list's description under the list name rather than clobbering + # the catalog one. + if _object and entry.description: + _object.list_descriptions[obs_list.name] = entry.description + + # Fall back to coordinate-based object + if not _object and (entry.ra or entry.dec): + _object = _coordinate_object(entry, i) + + if _object: + list_catalog.append(_object) + except Exception as e: + # One malformed entry (bad coords, a lookup that errors) must not + # sink the whole list: log it, skip it, keep resolving the rest. + errors += 1 + last_error = e + logger.warning( + "Skipping observing list entry %d (%r): %s", + i, + getattr(entry, "name", "?"), + e, + ) + except Exception as e: + # Safety net for a failure outside the per-entry guard (e.g. the entries + # iterable itself raises). Mirror the file-read error-dict contract so + # UIObsList.key_right shows its message instead of crashing the UI. Use + # len(list_catalog), not len(obs_list.entries): if entries was the thing + # that raised, touching it again here would re-raise and defeat the net. + logger.critical("Failed to resolve observing list %s: %s", name, e) + return { + "result": "error", + "objects_parsed": len(list_catalog), + "message": str(e), + "catalog_objects": list_catalog, + } - if _object: - list_catalog.append(_object) + # Every entry raised: this is systemic (e.g. the catalog DB is unavailable), + # not a few bad rows. Report it as an error rather than a silent "0 objects". + if obs_list.entries and errors == len(obs_list.entries): + logger.critical( + "Failed to resolve any entries in observing list %s: %s", + name, + last_error, + ) + return { + "result": "error", + "objects_parsed": len(obs_list.entries), + "message": str(last_error), + "catalog_objects": list_catalog, + } return { "result": "success", diff --git a/python/tests/test_obslist_resolve.py b/python/tests/test_obslist_resolve.py index 00fd7d2a..dda30811 100644 --- a/python/tests/test_obslist_resolve.py +++ b/python/tests/test_obslist_resolve.py @@ -2,11 +2,19 @@ Tests for name-based observing-list resolution: constellation-genitive normalization and exact name lookup. These let lists that identify objects only by name (e.g. carbon stars "VY Andromedae") match the catalog. + +Also covers read_list()'s error handling: a single bad entry is skipped and the +rest still load, while a systemic resolution failure is returned as the error +dict (never propagated -- UIObsList.key_right has no handler and would crash). """ +from types import SimpleNamespace + import pytest +from PiFinder import obslist from PiFinder.obslist import _normalize_designation, resolve_by_name +from PiFinder.obslist_formats import ObsList, ObsListEntry @pytest.mark.unit @@ -47,3 +55,85 @@ def test_no_match(self): def test_empty_name(self): assert resolve_by_name("", {"x": 1}) is None + + +def _entry(name, code, seq, desc=""): + """Minimal catalog-resolvable entry (read_list reads catalog_code+sequence).""" + return ObsListEntry( + name=name, + ra=10.0, + dec=20.0, + obj_type="Gx", + catalog_code=code, + sequence=seq, + description=desc, + ) + + +@pytest.mark.unit +class TestReadListErrorHandling: + """read_list's resolution loop must not propagate exceptions to the UI.""" + + def test_per_entry_failure_skips_and_continues(self, monkeypatch): + # First entry's lookup raises, second resolves -- the list still loads. + obs_list = ObsList( + name="Mixed", + entries=[_entry("NGC 224", "NGC", 224), _entry("M 42", "M", 42)], + ) + monkeypatch.setattr(obslist, "formats_read_file", lambda path: obs_list) + + resolved = SimpleNamespace(list_descriptions={}) + + def fake_resolve(catalog_numbers, catalogs): + if "NGC 224" in catalog_numbers: + raise RuntimeError("bad row") + return resolved + + monkeypatch.setattr(obslist, "resolve_object", fake_resolve) + + result = obslist.read_list(catalogs=None, name="Mixed.skylist") + + assert result["result"] == "success" + assert result["objects_parsed"] == 2 + # The bad entry is dropped (not turned into a coordinate object); only + # the one that resolved survives. + assert result["catalog_objects"] == [resolved] + + def test_all_entries_fail_returns_error(self, monkeypatch): + # Every lookup raises (e.g. catalog DB unavailable): systemic, so report + # an error rather than a silent "0 objects" success. + obs_list = ObsList( + name="AllBad", + entries=[_entry("NGC 224", "NGC", 224), _entry("M 42", "M", 42)], + ) + monkeypatch.setattr(obslist, "formats_read_file", lambda path: obs_list) + + def fake_resolve(catalog_numbers, catalogs): + raise RuntimeError("catalog db gone") + + monkeypatch.setattr(obslist, "resolve_object", fake_resolve) + + result = obslist.read_list(catalogs=None, name="AllBad.skylist") + + assert result["result"] == "error" + assert result["objects_parsed"] == 2 + assert result["catalog_objects"] == [] + assert "catalog db gone" in result["message"] + + def test_catastrophic_iteration_failure_returns_error(self, monkeypatch): + # Failure outside the per-entry guard (the entries iterable itself + # raises) hits the outer safety net and returns the error dict. + class ExplodingList: + name = "Boom" + + @property + def entries(self): + raise RuntimeError("entries unavailable") + + monkeypatch.setattr(obslist, "formats_read_file", lambda path: ExplodingList()) + + result = obslist.read_list(catalogs=None, name="Boom.skylist") + + assert result["result"] == "error" + assert result["catalog_objects"] == [] + assert "entries unavailable" in result["message"]