Skip to content

Commit 7fb2b2d

Browse files
committed
fix(notion_datasource): address review feedback from gemini-code-assist
- Widen the except clause in _build_page_entry to also catch requests.exceptions.RequestException, so a single HTTP error during parent resolution skips that item instead of aborting the whole enumeration (preserves the behaviour established by PR #2891). - Coalesce concurrent parent-block lookups via a thread-safe in-flight tracker (threading.Event per block_id), so multiple workers asking for the same ancestor share one HTTP request instead of racing past the cache miss and amplifying 429 backoff. - Restore API-side filtering in notion_page_search and notion_database_search by routing them through a new _search_filtered helper. They previously fetched everything and filtered in memory, which doubled the data transferred when callers used them independently.
1 parent e1757e8 commit 7fb2b2d

1 file changed

Lines changed: 75 additions & 18 deletions

File tree

datasources/notion_datasource/datasources/utils/notion_client.py

Lines changed: 75 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,11 @@ def __init__(self, integration_token: str):
4848
}
4949
# Memoize parent_id lookups so the threaded resolution in
5050
# get_authorized_pages doesn't re-fetch the same ancestor block twice.
51+
# `_parent_inflight` tracks lookups currently being resolved so that
52+
# concurrent workers asking for the same block coalesce onto a single
53+
# HTTP request instead of all racing past the cache miss.
5154
self._parent_cache: dict[str, str] = {}
55+
self._parent_inflight: dict[str, threading.Event] = {}
5256
self._parent_cache_lock = threading.Lock()
5357

5458
def _make_request(
@@ -523,7 +527,10 @@ def _build_page_entry(self, item: dict[str, Any]) -> OnlineDocumentPage | None:
523527
parent_id = parent[parent_type]
524528
else:
525529
parent_id = "root"
526-
except ValueError:
530+
except (ValueError, requests.exceptions.RequestException):
531+
# One unresolvable item (HTTP error, malformed parent, etc.) must not
532+
# abort the whole enumeration — preserve the original "skip and
533+
# continue" behavior established by PR #2891.
527534
return None
528535

529536
return OnlineDocumentPage(
@@ -547,35 +554,85 @@ def _resolve_worker_count() -> int:
547554
return max(1, min(value, _MAX_PARENT_RESOLVE_WORKERS))
548555

549556
def notion_page_search(self, access_token: str):
550-
return [item for item in self._search_all() if item.get("object") == "page"]
557+
return self._search_filtered("page")
551558

552559
def notion_database_search(self, access_token: str):
553-
return [item for item in self._search_all() if item.get("object") == "database"]
560+
return self._search_filtered("database")
561+
562+
def _search_filtered(self, object_type: str) -> list[dict[str, Any]]:
563+
"""Fetch only pages or only databases using the API-side object filter.
564+
565+
Used by the public ``notion_page_search`` / ``notion_database_search``
566+
wrappers when callers ask for just one type. ``get_authorized_pages``
567+
deliberately uses ``_search_all`` instead to collapse both kinds into a
568+
single pass.
569+
"""
570+
results: list[dict[str, Any]] = []
571+
next_cursor: str | None = None
572+
has_more = True
573+
while has_more:
574+
payload: dict[str, Any] = {
575+
"filter": {"value": object_type, "property": "object"},
576+
}
577+
if next_cursor:
578+
payload["start_cursor"] = next_cursor
579+
data = self._make_request("post", "/search", json_data=payload) or {}
580+
results.extend(data.get("results", []))
581+
has_more = data.get("has_more", False)
582+
next_cursor = data.get("next_cursor")
583+
return results
554584

555585
def notion_block_parent_page_id(self, access_token: str, block_id: str):
556586
return self._resolve_block_parent_page_id(block_id)
557587

558588
def _resolve_block_parent_page_id(self, block_id: str) -> str:
559589
clean_id = block_id.replace("-", "")
560-
cached = self._parent_cache.get(clean_id)
561-
if cached is not None:
562-
return cached
563590

564-
data = self._make_request("get", f"/blocks/{clean_id}", allow_status=(404,))
565-
if data is None:
566-
result = "root"
567-
else:
568-
parent = data.get("parent", {})
569-
parent_type = parent.get("type")
570-
if parent_type == "block_id":
571-
result = self._resolve_block_parent_page_id(parent[parent_type])
572-
elif parent_type == "workspace":
591+
# Coordinate cache and in-flight lookups under the same lock so two
592+
# workers can never both miss the cache for the same block and race to
593+
# fetch it — under Notion's ~3 req/s limit those redundant requests
594+
# would just amplify 429 backoff.
595+
while True:
596+
with self._parent_cache_lock:
597+
cached = self._parent_cache.get(clean_id)
598+
if cached is not None:
599+
return cached
600+
event = self._parent_inflight.get(clean_id)
601+
if event is None:
602+
# This thread is now responsible for fetching this id;
603+
# other threads asking for the same id will wait on `event`.
604+
event = threading.Event()
605+
self._parent_inflight[clean_id] = event
606+
break
607+
# Another worker is already fetching; wait for it to finish, then
608+
# re-check the cache on the next iteration.
609+
event.wait()
610+
611+
try:
612+
data = self._make_request("get", f"/blocks/{clean_id}", allow_status=(404,))
613+
if data is None:
573614
result = "root"
574-
elif parent_type and parent_type in parent:
575-
result = parent[parent_type]
576615
else:
577-
result = "root"
616+
parent = data.get("parent", {})
617+
parent_type = parent.get("type")
618+
if parent_type == "block_id":
619+
result = self._resolve_block_parent_page_id(parent[parent_type])
620+
elif parent_type == "workspace":
621+
result = "root"
622+
elif parent_type and parent_type in parent:
623+
result = parent[parent_type]
624+
else:
625+
result = "root"
626+
except Exception:
627+
# Release waiters so they don't block forever; they will re-enter
628+
# the lock above, see no cache entry, and retry on their own.
629+
with self._parent_cache_lock:
630+
self._parent_inflight.pop(clean_id, None)
631+
event.set()
632+
raise
578633

579634
with self._parent_cache_lock:
580635
self._parent_cache[clean_id] = result
636+
self._parent_inflight.pop(clean_id, None)
637+
event.set()
581638
return result

0 commit comments

Comments
 (0)