Fix bug in search reindex command (#689)

Author: DavidMStraub

gramps_webapi/__main__.py

@@ -30,12 +30,12 @@
 import warnings
 import webbrowser
 from threading import Thread
-
 import click
 import waitress  # type: ignore
 
 from .api.search import get_search_indexer, get_semantic_search_indexer
 from .api.tasks import send_email_confirm_email, send_email_reset_password
+from .types import ProgressCallback
 from .api.util import close_db, get_db_manager, list_trees
 from .app import create_app
 from .auth import add_user, delete_user, fill_tree, user_db
@@ -259,17 +259,22 @@ def search(ctx, tree, semantic):
             ctx.obj["search_indexer"] = get_search_indexer(tree=tree)
 
 
-def progress_callback_count(
-    app, current: int, total: int, prev: int | None = None
-) -> None:
-    if total == 0:
-        return
-    pct = int(100 * current / total)
-    if prev is None:
-        prev = current - 1
-    pct_prev = int(100 * prev / total)
-    if current == 0 or pct != pct_prev:
-        app.logger.info(f"Progress: {pct}%")
+def progress_callback_count_factory(app) -> ProgressCallback:
+    """Create a progress callback function that logs progress to the app logger."""
+
+    def progress_callback_count(
+        current: int, total: int, prev: int | None = None
+    ) -> None:
+        if total == 0:
+            return
+        pct = int(100 * current / total)
+        if prev is None:
+            prev = current - 1
+        pct_prev = int(100 * prev / total)
+        if current == 0 or pct != pct_prev:
+            app.logger.info(f"Progress: {pct}%")
+
+    return progress_callback_count
 
 
 @search.command("index-full")
@@ -284,9 +289,10 @@ def index_full(ctx):
 
     t0 = time.time()
     try:
-        indexer.reindex_full(db, progress_cb=progress_callback_count)
-    except:
+        indexer.reindex_full(db, progress_cb=progress_callback_count_factory(app))
+    except Exception as e:
         app.logger.exception("Error during indexing")
+        raise click.ClickException(f"Indexing failed: {e}")
     finally:
         close_db(db)
     app.logger.info(f"Done building search index in {time.time() - t0:.0f} seconds.")
@@ -302,9 +308,12 @@ def index_incremental(ctx):
     db = db_manager.get_db().db
 
     try:
-        indexer.reindex_incremental(db, progress_cb=progress_callback_count)
-    except Exception:
+        indexer.reindex_incremental(
+            db, progress_cb=progress_callback_count_factory(app)
+        )
+    except Exception as e:
         app.logger.exception("Error during indexing")
+        raise click.ClickException(f"Indexing failed: {e}")
     finally:
         close_db(db)
     app.logger.info("Done updating search index.")

gramps_webapi/api/search/indexer.py

@@ -21,11 +21,12 @@
 
 from __future__ import annotations
 
-from typing import Any, Callable, Dict, List, Optional, Set
+from typing import Any, Callable, Dict, List, Set
 
 import sifts
 from gramps.gen.db.base import DbReadBase
 
+from ...types import ProgressCallback
 from .text import iter_obj_strings, obj_strings_from_handle
 from ..util import get_total_number_of_objects, get_object_timestamps
 
@@ -39,7 +40,7 @@ class SearchIndexerBase:
     def __init__(
         self,
         tree: str,
-        db_url: Optional[str] = None,
+        db_url: str | None = None,
         embedding_function: Callable | None = None,
         use_fts: bool = True,
         use_semantic_text: bool = False,
@@ -126,7 +127,7 @@ def _add_objects(self, obj_dicts: List[Dict[str, Any]]):
         self.index_public.add(contents=contents, ids=ids, metadatas=metadatas)
 
     def reindex_full(
-        self, db_handle: DbReadBase, progress_cb: Optional[Callable] = None
+        self, db_handle: DbReadBase, progress_cb: ProgressCallback | None = None
     ):
         """Reindex the whole database."""
         total = get_total_number_of_objects(db_handle)
@@ -213,7 +214,7 @@ def add_or_update_object(
             self._add_objects([obj_dict])
 
     def reindex_incremental(
-        self, db_handle: DbReadBase, progress_cb: Optional[Callable] = None
+        self, db_handle: DbReadBase, progress_cb: ProgressCallback | None = None
     ):
         """Update the index incrementally."""
         update_info = self._get_update_info(db_handle)
@@ -287,10 +288,10 @@ def search(
         page: int,
         pagesize: int,
         include_private: bool = True,
-        sort: Optional[List[str]] = None,
-        object_types: Optional[List[str]] = None,
-        change_op: Optional[str] = None,
-        change_value: Optional[float] = None,
+        sort: List[str] | None = None,
+        object_types: List[str] | None = None,
+        change_op: str | None = None,
+        change_value: float | None = None,
         include_content: bool = False,
     ):
         """Search the index.
@@ -338,7 +339,7 @@ class SearchIndexer(SearchIndexerBase):
     def __init__(
         self,
         tree: str,
-        db_url: Optional[str] = None,
+        db_url: str | None = None,
     ):
         """Initialize the indexer."""
         super().__init__(
@@ -355,7 +356,7 @@ class SemanticSearchIndexer(SearchIndexerBase):
     def __init__(
         self,
         tree: str,
-        db_url: Optional[str] = None,
+        db_url: str | None = None,
         embedding_function: Callable | None = None,
     ):
         """Initialize the indexer."""

gramps_webapi/types.py

@@ -22,7 +22,7 @@
 from __future__ import annotations
 
 from pathlib import Path
-from typing import Any, NewType, Union
+from typing import Any, NewType, Protocol, Union
 
 import flask.typing
 
@@ -32,3 +32,10 @@
 TransactionJson = list[dict[str, Any]]
 ResponseReturnValue = flask.typing.ResponseReturnValue
 MatchSegment = dict[str, Union[float, int, str, None]]
+
+
+class ProgressCallback(Protocol):
+    """Protocol for progress callback functions."""
+
+    def __call__(self, current: int, total: int, prev: int | None = None) -> None:
+        """Call the progress callback with current progress, total items, and optional previous value."""

tests/test_cli.py

@@ -154,6 +154,10 @@ def test_search_reindex_incremental(self):
             ],
         )
         assert result.exit_code == 0
+        # The log messages go to the logger, not stdout, so we can't check for them in result.output
+        # But we can ensure there's no error output and the command succeeded
+        assert "Error" not in result.output
+        assert "Exception" not in result.output
 
     def test_search_reindex_full(self):
         tree = WebDbManager(name=self.name).dirname
@@ -169,6 +173,10 @@ def test_search_reindex_full(self):
             ],
         )
         assert result.exit_code == 0
+        # The log messages go to the logger, not stdout, so we can't check for them in result.output
+        # But we can ensure there's no error output and the command succeeded
+        assert "Error" not in result.output
+        assert "Exception" not in result.output
 
     def test_search_reindex_incremental_notree(self):
         tree = WebDbManager(name=self.name).dirname
@@ -182,6 +190,10 @@ def test_search_reindex_incremental_notree(self):
             ],
         )
         assert result.exit_code == 0
+        # The log messages go to the logger, not stdout, so we can't check for them in result.output
+        # But we can ensure there's no error output and the command succeeded
+        assert "Error" not in result.output
+        assert "Exception" not in result.output
 
     def test_search_reindex_full_notree(self):
         tree = WebDbManager(name=self.name).dirname
@@ -195,3 +207,30 @@ def test_search_reindex_full_notree(self):
             ],
         )
         assert result.exit_code == 0
+        # The log messages go to the logger, not stdout, so we can't check for them in result.output
+        # But we can ensure there's no error output and the command succeeded
+        assert "Error" not in result.output
+        assert "Exception" not in result.output
+
+    def test_search_reindex_with_broken_progress_callback(self):
+        """Test that would catch the original bug where progress_callback_count was undefined."""
+        tree = WebDbManager(name=self.name).dirname
+        
+        # Temporarily break the progress callback by patching it to raise an error
+        with patch('gramps_webapi.__main__.progress_callback_count_factory') as mock_factory:
+            mock_factory.side_effect = NameError("name 'progress_callback_count' is not defined")
+            
+            result = self.runner.invoke(
+                cli,
+                [
+                    "--config",
+                    self.config_file.name,
+                    "search",
+                    "--tree",
+                    tree,
+                    "index-full",
+                ],
+            )
+            # With the improved error handling, this should now fail with a non-zero exit code
+            assert result.exit_code != 0
+            assert "Indexing failed" in result.output