diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index ec7f2ca..8f870c5 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -22,6 +22,10 @@ jobs: else TAG=${GITHUB_REF#refs/heads/} fi + # Docker tags allow only [A-Za-z0-9_.-] (max 128, no leading . or -), + # so a branch like fix/foo would fail the build with "invalid + # reference format". + TAG=$(printf '%s' "$TAG" | sed -e 's/[^A-Za-z0-9_.-]/-/g' -e 's/^[.-]*//' | cut -c1-128) echo "tag=$TAG" >> $GITHUB_OUTPUT # Mirror what publish_to_pypi.yaml does so the docker image and the diff --git a/src/test/test_neo4j_failure_no_silent_empty.py b/src/test/test_neo4j_failure_no_silent_empty.py new file mode 100644 index 0000000..f3049d9 --- /dev/null +++ b/src/test/test_neo4j_failure_no_silent_empty.py @@ -0,0 +1,145 @@ +"""A failed Neo4j main query must never read as "no rows". + +Backend-free: ``vc`` is monkeypatched, so these run in CI without credentials. + +Why: from 2026-10-12 the TransgeneExpressionHere main query for mushroom body +timed out on pdb. ``commit_list`` reports that by returning ``False``, which +became an empty table, so the function returned ``{'count': 2339, 'rows': []}`` +— and the Solr result cache's ``count >= 0`` rule would have stored it. +""" +import types + +import pytest + +from vfbquery import cached_functions as cf +from vfbquery import solr_result_cache as src +from vfbquery import vfb_queries as vq + + +def _count_reply(n): + return [{"columns": ["total_count"], "data": [{"row": [n]}]}] + + +class _NC: + """Stand-in for ``vc.nc`` replaying one reply per ``commit_list`` call.""" + + def __init__(self, *replies): + self._replies = list(replies) + + def commit_list(self, statements): + return self._replies.pop(0) + + +def _wire(monkeypatch, *replies): + monkeypatch.setattr(vq, "vc", types.SimpleNamespace( + nc=_NC(*replies), + vfb=types.SimpleNamespace(oc=types.SimpleNamespace( + get_subclasses=lambda **kw: [])), + )) + + +def test_transgene_expression_raises_when_main_query_fails(monkeypatch): + _wire(monkeypatch, _count_reply(2339), False) + with pytest.raises(vq.Neo4jQueryError, match="TransgeneExpressionHere main query"): + cf._original_get_transgene_expression_here( + "FBbt_00003748", return_dataframe=False, limit=10) + + +def test_transgene_expression_raises_when_count_query_fails(monkeypatch): + _wire(monkeypatch, False) + with pytest.raises(vq.Neo4jQueryError, match="TransgeneExpressionHere count query"): + cf._original_get_transgene_expression_here( + "FBbt_00003748", return_dataframe=False, limit=10) + + +def test_expression_overlaps_raises_when_main_query_fails(monkeypatch): + _wire(monkeypatch, _count_reply(79), False) + with pytest.raises(vq.Neo4jQueryError, match="ExpressionOverlapsHere main query"): + cf._original_get_expression_overlaps_here( + "VFBexp_FBtp0001321", return_dataframe=False, limit=5) + + +def test_genuinely_empty_result_is_still_returned(monkeypatch): + empty = [{"columns": [], "data": []}] + _wire(monkeypatch, _count_reply(0), empty) + result = cf._original_get_transgene_expression_here( + "FBbt_00000000", return_dataframe=False, limit=10) + assert result["count"] == 0 + assert result["rows"] == [] + + +@pytest.mark.parametrize("result, missing", [ + ({"headers": {}, "rows": [], "count": 2339}, True), + ({"headers": {}, "rows": [], "count": 0}, False), + ({"headers": {}, "rows": [{"id": "x"}], "count": 2339}, False), + ({"headers": {}, "rows": [], "count": -1}, False), + ({"Id": "FBbt_00003748", "Name": "mushroom body"}, False), + ([], False), +]) +def test_result_rows_missing(result, missing): + assert src.result_rows_missing(result) is missing + + +def test_result_rows_missing_accepts_numpy_count(): + np = pytest.importorskip("numpy") + assert src.result_rows_missing({"rows": [], "count": np.int64(5)}) is True + + +# --------------------------------------------------------------------------- +# read path: entries stored before the write-path guard existed +# --------------------------------------------------------------------------- + +class _Solr: + """Answers cache lookups from a dict and records delete posts.""" + + def __init__(self, docs): + self.docs = docs + self.posts = [] + + def get(self, url, params=None, timeout=None): + doc_id = (params or {}).get("q", "").split("id:", 1)[-1] + payload = self.docs.get(doc_id) + docs = [{"cache_data": payload}] if payload is not None else [] + return types.SimpleNamespace(status_code=200, + json=lambda: {"response": {"docs": docs}}) + + def post(self, url, data=None, headers=None, params=None, timeout=None): + self.posts.append(data) + return types.SimpleNamespace(status_code=200, text="", json=lambda: {}) + + +def _cache_with(monkeypatch, result): + import json + for var in ("VFBQUERY_CACHE_NAMESPACE", "VFBQUERY_CACHE_NAMESPACE_FALLBACK", + "VFBQUERY_CACHE_READONLY", "VFBQUERY_CACHE_ENABLED"): + monkeypatch.delenv(var, raising=False) + cache = src.SolrResultCache() + monkeypatch.setattr(cache, "_solr_available", lambda: True) + payload = src._encode_cache_field(json.dumps( + cache._create_cache_metadata(result, limit=-1))) + solr = _Solr({"vfb_query_transgene_expression_here_T": payload}) + monkeypatch.setattr(src.requests, "get", solr.get) + monkeypatch.setattr(src.requests, "post", solr.post) + return cache, solr + + +def test_cached_result_with_missing_rows_is_a_miss_and_purged(monkeypatch): + # Stored 2026-10-15 for mushroom body by the pre-fix code, served to every + # PR check until it expired. + cache, solr = _cache_with(monkeypatch, {"headers": {}, "rows": [], "count": 2339}) + assert cache.get_cached_result("transgene_expression_here", "T", limit=-1) is None + assert any("vfb_query_transgene_expression_here_T" in p for p in solr.posts) + + +def test_cached_result_with_missing_rows_is_not_purged_when_read_only(monkeypatch): + cache, solr = _cache_with(monkeypatch, {"headers": {}, "rows": [], "count": 2339}) + monkeypatch.setenv("VFBQUERY_CACHE_READONLY", "true") + assert cache.get_cached_result("transgene_expression_here", "T", limit=-1) is None + assert solr.posts == [] + + +def test_cached_result_with_rows_is_served(monkeypatch): + good = {"headers": {}, "rows": [{"id": "VFBexp_1"}], "count": 2339} + cache, solr = _cache_with(monkeypatch, good) + assert cache.get_cached_result("transgene_expression_here", "T", limit=-1) == good + assert solr.posts == [] diff --git a/src/vfbquery/solr_result_cache.py b/src/vfbquery/solr_result_cache.py index 2fd85a3..e0caf7d 100644 --- a/src/vfbquery/solr_result_cache.py +++ b/src/vfbquery/solr_result_cache.py @@ -166,6 +166,25 @@ def result_is_partial(result) -> bool: return isinstance(result, dict) and bool(result.get(PARTIAL_RESULT_KEY)) +def result_rows_missing(result) -> bool: + """True for a dict result that reports rows (count > 0) but carries none. + + That is the shape of a query whose count query succeeded and whose main + query failed: Neo4j's ``commit_list`` returns ``False``, which becomes an + empty table. ``count >= 0`` alone would store it and serve "no rows" for + the cache TTL (TransgeneExpressionHere on mushroom body, 2026-10). + """ + if not isinstance(result, dict): + return False + rows = result.get('rows') + if not isinstance(rows, list) or rows: + return False + try: + return int(result.get('count', 0)) > 0 + except (TypeError, ValueError): + return False + + def preview_is_resolved(query: Dict[str, Any]) -> bool: """True when a query's preview holds a final answer. @@ -592,7 +611,18 @@ def _get_cached_result_in(self, namespace: str, allow_purge: bool, if cached_limit != -1: logger.debug(f"Cache miss: Unexpected cached result with limit={cached_limit}, expected -1") return None - + + # Entries stored before result_rows_missing() guarded the write + # path can still hold count > 0 with no rows (a failed main query). + # Treat them as a miss and purge them so the next writable call + # recomputes and overwrites them, rather than serving "no rows" + # until the entry expires. + if result_rows_missing(cached_data.get("result")): + logger.warning( + f"Cached result for {query_type}({term_id}) has count > 0 but no rows; clearing it") + purge(cache_doc_id) + return None + # If requesting unlimited results, return the full cached result if requested_limit == -1: result = cached_data["result"] @@ -1440,6 +1470,10 @@ def _call(*call_args, **call_kwargs): full_is_valid = False logger.warning( f"Not caching partial result for {query_type}({term_id})") + if full_is_valid and result_rows_missing(full_result): + full_is_valid = False + logger.warning( + f"Not caching result with count > 0 but no rows for {query_type}({term_id})") elif isinstance(full_result, (list, str)): full_is_valid = len(full_result) > 0 else: @@ -1526,6 +1560,10 @@ def _call(*call_args, **call_kwargs): result_is_valid = False logger.warning( f"Not caching partial result for {query_type}({term_id})") + if result_is_valid and result_rows_missing(result): + result_is_valid = False + logger.warning( + f"Not caching result with count > 0 but no rows for {query_type}({term_id})") elif isinstance(result, (list, str)): result_is_valid = len(result) > 0 else: diff --git a/src/vfbquery/vfb_queries.py b/src/vfbquery/vfb_queries.py index ee36982..1c8d659 100644 --- a/src/vfbquery/vfb_queries.py +++ b/src/vfbquery/vfb_queries.py @@ -144,6 +144,26 @@ class ConnectivityBackendError(RuntimeError): """ +class Neo4jQueryError(RuntimeError): + """A Neo4j query errored or timed out instead of returning a result. + + ``commit_list`` signals failure by returning ``False``, and + ``get_dict_cursor`` turns that into ``[]``, so a timed-out main query used + to come back as ``{'count': N, 'rows': []}``. That is a valid result to the + Solr cache (``count >= 0``) and reads as "no data" to every caller + (TransgeneExpressionHere on mushroom body, 2026-10). Same rationale as + :class:`ConnectivityBackendError`: raise rather than report zero rows. + """ + + +def _commit_or_raise(query, description): + """``vc.nc.commit_list([query])``, raising :class:`Neo4jQueryError` on failure.""" + results = vc.nc.commit_list([query]) + if not isinstance(results, list): + raise Neo4jQueryError(f"Neo4j query failed: {description}") + return results + + #: Result key set by :func:`get_downstream_class_connectivity` / #: :func:`get_upstream_class_connectivity` when the aggregation ran but some #: queried instances contributed nothing (their per-instance connectivity @@ -3913,7 +3933,7 @@ def get_expression_overlaps_here(expression_pattern_short_form: str, return_data RETURN COUNT(DISTINCT anat) AS total_count """ - count_results = vc.nc.commit_list([count_query]) + count_results = _commit_or_raise(count_query, f"ExpressionOverlapsHere count query for {expression_pattern_short_form}") count_df = pd.DataFrame.from_records(get_dict_cursor()(count_results)) total_count = count_df['total_count'][0] if not count_df.empty else 0 @@ -3961,16 +3981,26 @@ def get_expression_overlaps_here(expression_pattern_short_form: str, return_data }} CALL {{ WITH anat - OPTIONAL MATCH (anat)<-[:has_source|SUBCLASSOF|INSTANCEOF*]-(i:Individual)<-[:depicts]-(channel:Individual)-[irw:in_register_with]->(template:Individual)-[:depicts]->(template_anat:Individual) - OPTIONAL MATCH (channel)-[:is_specified_output_of]->(technique:Class) - // Do NOT filter `i IS NOT NULL` here: this is a CALL subquery, so an - // input anat for which it yields no rows is dropped entirely. Anatomy - // classes rarely have this image path, so the filter emptied the whole + // apoc.path.subgraphNodes instead of a `[...*]` var-length + // pattern — see get_transgene_expression_here. The `*` form timed + // out for every anat in the AnatomyExpressedIn tests (2026-10); + // this returns in well under a second even for GABAergic neuron + // (~110k reachable nodes), since LIMIT 5 stops the walk early. + // + // Do NOT let an anat without images drop out of the result: anatomy + // classes rarely have this image path, so doing so emptied the whole // table (count 79 -> 0 rows) and desynced it from count_query. The - // all-null row is stripped to '' by the REPLACE below, so the anat - // still returns a row (empty thumbnail/template/technique). - WITH anat, i, template_anat, technique, irw LIMIT 5 - WITH anat, collect({{i: i, template_anat: template_anat, technique: technique, irw: irw}}) AS imgs + // inner collect() always yields one row (imgs = [] when there is no + // image), so the anat keeps its row with an empty + // thumbnail/template/technique. + CALL {{ + WITH anat + CALL apoc.path.subgraphNodes(anat, {{relationshipFilter: '(template:Individual)-[:depicts]->(template_anat:Individual) + OPTIONAL MATCH (channel)-[:is_specified_output_of]->(technique:Class) + WITH i, template_anat, technique, irw LIMIT 5 + RETURN collect({{i: i, template_anat: template_anat, technique: technique, irw: irw}}) AS imgs + }} WITH anat, imgs, head(imgs) AS rep RETURN rep.template_anat AS template_anat, @@ -3995,7 +4025,7 @@ def get_expression_overlaps_here(expression_pattern_short_form: str, return_data thumbnail """ - results = vc.nc.commit_list([main_query]) + results = _commit_or_raise(main_query, f"ExpressionOverlapsHere main query for {expression_pattern_short_form}") df = pd.DataFrame.from_records(get_dict_cursor()(results)) if not df.empty: @@ -7453,7 +7483,7 @@ def get_transgene_expression_here(anatomy_short_form: str, return_dataframe=True WHERE anat.short_form IN {anat_short_forms!r} RETURN COUNT(DISTINCT ep) AS total_count """ - count_results = vc.nc.commit_list([count_query]) + count_results = _commit_or_raise(count_query, f"TransgeneExpressionHere count query for {anatomy_short_form}") count_df = pd.DataFrame.from_records(get_dict_cursor()(count_results)) total_count = count_df['total_count'][0] if not count_df.empty else 0 @@ -7538,19 +7568,29 @@ def get_transgene_expression_here(anatomy_short_form: str, return_dataframe=True }} CALL {{ WITH ep - OPTIONAL MATCH (ep)<-[:has_source|SUBCLASSOF|INSTANCEOF*]-(i:Individual)<-[:depicts]-(channel:Individual)-[irw:in_register_with]->(:Template)-[:depicts]->(templ:Template) - OPTIONAL MATCH (channel)-[:is_specified_output_of]->(technique:Class) - // Do NOT filter `i IS NOT NULL` here: this is a CALL subquery, so an - // input ep for which it yields no rows is dropped from the result - // entirely. Filtering image-less EPs desynced the table from - // count_query (which has no image filter) and silently hid every - // expression pattern with no aligned image — e.g. splits targeting a - // neuron that carry no registered image. When there is no image the - // OPTIONAL MATCH yields one all-null row; the null placeholders are - // stripped to '' by the REPLACE below, so the ep still returns a row - // (empty thumbnail/template/technique) and stays in the results. - WITH ep, i, templ, technique, irw LIMIT 5 - WITH ep, collect({{i: i, templ: templ, technique: technique, irw: irw}}) AS imgs + // Walk the has_source/SUBCLASSOF/INSTANCEOF closure with + // apoc.path.subgraphNodes rather than a `[...*]` var-length + // pattern. Both reach the same image Individuals, but the planner + // expanded `*` from the Template end and enumerated every path, + // taking ~37s for 10 mushroom-body EPs and timing out at + // limit=-1 (2026-10). subgraphNodes is anchored at ep and visits + // each node once, with no depth cap. + // + // Do NOT let an image-less ep drop out of the result: filtering + // them desynced the table from count_query (which has no image + // filter) and silently hid e.g. splits targeting a neuron that + // carry no registered image. The collect() below is an aggregate + // with no grouping key, so this subquery always yields one row + // (imgs = [] when there is no image) and the ep keeps its row + // with an empty thumbnail/template/technique. + CALL {{ + WITH ep + CALL apoc.path.subgraphNodes(ep, {{relationshipFilter: '(:Template)-[:depicts]->(templ:Template) + OPTIONAL MATCH (channel)-[:is_specified_output_of]->(technique:Class) + WITH i, templ, technique, irw LIMIT 5 + RETURN collect({{i: i, templ: templ, technique: technique, irw: irw}}) AS imgs + }} WITH ep, imgs, head(imgs) AS rep RETURN rep.templ AS templ, @@ -7575,7 +7615,7 @@ def get_transgene_expression_here(anatomy_short_form: str, return_dataframe=True thumbnail """ - results = vc.nc.commit_list([main_query]) + results = _commit_or_raise(main_query, f"TransgeneExpressionHere main query for {anatomy_short_form}") df = pd.DataFrame.from_records(get_dict_cursor()(results)) if not df.empty: df = encode_markdown_links(df, ['name', 'expressed_in', 'template', 'thumbnail'])