-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix: Robust error handling for async database operations in graph storage #2356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,6 +133,7 @@ async def has_node(self, node_id: str) -> bool: | |
| async with self._driver.session( | ||
| database=self._DATABASE, default_access_mode="READ" | ||
| ) as session: | ||
| result = None | ||
| try: | ||
| workspace_label = self._get_workspace_label() | ||
| query = f"MATCH (n:`{workspace_label}` {{entity_id: $entity_id}}) RETURN count(n) > 0 AS node_exists" | ||
|
|
@@ -146,7 +147,10 @@ async def has_node(self, node_id: str) -> bool: | |
| logger.error( | ||
| f"[{self.workspace}] Error checking node existence for {node_id}: {str(e)}" | ||
| ) | ||
| await result.consume() # Ensure the result is consumed even on error | ||
| if result is not None: | ||
| await ( | ||
| result.consume() | ||
| ) # Ensure the result is consumed even on error | ||
| raise | ||
|
|
||
| async def has_edge(self, source_node_id: str, target_node_id: str) -> bool: | ||
|
|
@@ -170,6 +174,7 @@ async def has_edge(self, source_node_id: str, target_node_id: str) -> bool: | |
| async with self._driver.session( | ||
| database=self._DATABASE, default_access_mode="READ" | ||
| ) as session: | ||
| result = None | ||
| try: | ||
| workspace_label = self._get_workspace_label() | ||
| query = ( | ||
|
|
@@ -190,7 +195,10 @@ async def has_edge(self, source_node_id: str, target_node_id: str) -> bool: | |
| logger.error( | ||
| f"[{self.workspace}] Error checking edge existence between {source_node_id} and {target_node_id}: {str(e)}" | ||
| ) | ||
| await result.consume() # Ensure the result is consumed even on error | ||
| if result is not None: | ||
| await ( | ||
| result.consume() | ||
| ) # Ensure the result is consumed even on error | ||
| raise | ||
|
|
||
| async def get_node(self, node_id: str) -> dict[str, str] | None: | ||
|
|
@@ -312,6 +320,7 @@ async def get_all_labels(self) -> list[str]: | |
| async with self._driver.session( | ||
| database=self._DATABASE, default_access_mode="READ" | ||
| ) as session: | ||
| result = None | ||
| try: | ||
| workspace_label = self._get_workspace_label() | ||
| query = f""" | ||
|
|
@@ -328,7 +337,10 @@ async def get_all_labels(self) -> list[str]: | |
| return labels | ||
| except Exception as e: | ||
| logger.error(f"[{self.workspace}] Error getting all labels: {str(e)}") | ||
| await result.consume() # Ensure the result is consumed even on error | ||
| if result is not None: | ||
| await ( | ||
| result.consume() | ||
| ) # Ensure the result is consumed even on error | ||
| raise | ||
|
|
||
| async def get_node_edges(self, source_node_id: str) -> list[tuple[str, str]] | None: | ||
|
|
@@ -352,6 +364,7 @@ async def get_node_edges(self, source_node_id: str) -> list[tuple[str, str]] | N | |
| async with self._driver.session( | ||
| database=self._DATABASE, default_access_mode="READ" | ||
| ) as session: | ||
| results = None | ||
| try: | ||
| workspace_label = self._get_workspace_label() | ||
| query = f"""MATCH (n:`{workspace_label}` {{entity_id: $entity_id}}) | ||
|
|
@@ -389,7 +402,10 @@ async def get_node_edges(self, source_node_id: str) -> list[tuple[str, str]] | N | |
| logger.error( | ||
| f"[{self.workspace}] Error getting edges for node {source_node_id}: {str(e)}" | ||
| ) | ||
| await results.consume() # Ensure results are consumed even on error | ||
| if results is not None: | ||
| await ( | ||
| results.consume() | ||
| ) # Ensure results are consumed even on error | ||
| raise | ||
| except Exception as e: | ||
| logger.error( | ||
|
|
@@ -419,6 +435,7 @@ async def get_edge( | |
| async with self._driver.session( | ||
| database=self._DATABASE, default_access_mode="READ" | ||
| ) as session: | ||
| result = None | ||
| try: | ||
| workspace_label = self._get_workspace_label() | ||
| query = f""" | ||
|
|
@@ -451,7 +468,10 @@ async def get_edge( | |
| logger.error( | ||
| f"[{self.workspace}] Error getting edge between {source_node_id} and {target_node_id}: {str(e)}" | ||
| ) | ||
| await result.consume() # Ensure the result is consumed even on error | ||
| if result is not None: | ||
| await ( | ||
| result.consume() | ||
| ) # Ensure the result is consumed even on error | ||
| raise | ||
|
|
||
| async def upsert_node(self, node_id: str, node_data: dict[str, str]) -> None: | ||
|
|
@@ -1030,11 +1050,12 @@ async def get_popular_labels(self, limit: int = 300) -> list[str]: | |
| "Memgraph driver is not initialized. Call 'await initialize()' first." | ||
| ) | ||
|
|
||
| try: | ||
| workspace_label = self._get_workspace_label() | ||
| async with self._driver.session( | ||
| database=self._DATABASE, default_access_mode="READ" | ||
| ) as session: | ||
| workspace_label = self._get_workspace_label() | ||
| async with self._driver.session( | ||
| database=self._DATABASE, default_access_mode="READ" | ||
| ) as session: | ||
| result = None | ||
| try: | ||
| query = f""" | ||
| MATCH (n:`{workspace_label}`) | ||
| WHERE n.entity_id IS NOT NULL | ||
|
|
@@ -1054,9 +1075,13 @@ async def get_popular_labels(self, limit: int = 300) -> list[str]: | |
| f"[{self.workspace}] Retrieved {len(labels)} popular labels (limit: {limit})" | ||
| ) | ||
| return labels | ||
| except Exception as e: | ||
| logger.error(f"[{self.workspace}] Error getting popular labels: {str(e)}") | ||
| return [] | ||
| except Exception as e: | ||
| logger.error( | ||
| f"[{self.workspace}] Error getting popular labels: {str(e)}" | ||
| ) | ||
| if result is not None: | ||
| await result.consume() | ||
| return [] | ||
|
||
|
|
||
| async def search_labels(self, query: str, limit: int = 50) -> list[str]: | ||
| """Search labels with fuzzy matching | ||
|
|
@@ -1078,11 +1103,12 @@ async def search_labels(self, query: str, limit: int = 50) -> list[str]: | |
| if not query_lower: | ||
| return [] | ||
|
|
||
| try: | ||
| workspace_label = self._get_workspace_label() | ||
| async with self._driver.session( | ||
| database=self._DATABASE, default_access_mode="READ" | ||
| ) as session: | ||
| workspace_label = self._get_workspace_label() | ||
| async with self._driver.session( | ||
| database=self._DATABASE, default_access_mode="READ" | ||
| ) as session: | ||
| result = None | ||
| try: | ||
|
||
| cypher_query = f""" | ||
| MATCH (n:`{workspace_label}`) | ||
| WHERE n.entity_id IS NOT NULL | ||
|
|
@@ -1109,6 +1135,8 @@ async def search_labels(self, query: str, limit: int = 50) -> list[str]: | |
| f"[{self.workspace}] Search query '{query}' returned {len(labels)} results (limit: {limit})" | ||
| ) | ||
| return labels | ||
| except Exception as e: | ||
| logger.error(f"[{self.workspace}] Error searching labels: {str(e)}") | ||
| return [] | ||
| except Exception as e: | ||
| logger.error(f"[{self.workspace}] Error searching labels: {str(e)}") | ||
| if result is not None: | ||
| await result.consume() | ||
|
||
| return [] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_popular_labels()previously wrapped_get_workspace_label()and the session acquisition in a try/except that logged failures and returned an empty list. The new implementation performs those steps outside the try block, so a driver connection error or_get_workspace_label()failure now propagates as an unhandled exception and will bubble up to callers instead of returning[]as before. This is a behavioral regression and contradicts the commitβs intent of a purely defensive change.Useful? React with πΒ / π.