Bookmark manager no longer tracks per database (#859)

* The experimental bookmark manager feature was changed to no longer track
  bookmarks per database.
  This effectively changes the signature of almost all bookmark
  manager related methods:
  - `neo4j.BookmarkManger` and `neo4j.AsyncBookmarkManger` abstract base
    classes:
    - ``update_bookmarks`` has no longer a ``database`` argument.
    - ``get_bookmarks`` has no longer a ``database`` argument.
    - The ``get_all_bookmarks`` method was removed.
    - The ``forget`` method was removed.
  - `neo4j.GraphDatabase.bookmark_manager` and
    `neo4j.AsyncGraphDatabase.bookmark_manager` factory methods:
    - ``initial_bookmarks`` is no longer a mapping from database name
      to bookmarks but plain bookmarks.
    - ``bookmarks_supplier`` no longer receives the database name as
      an argument.
    - ``bookmarks_consumer`` no longer receives the database name as
      an argument.

* Fix logging the wrong variable

Author: robsdedude

CHANGELOG.md

@@ -1,12 +1,32 @@
 # Neo4j Driver Change Log (breaking/major changes only)
 
+See also https://github.com/neo4j/neo4j-python-driver/wiki for more details.
+
 ## Version 5.3
 - Python 3.11 support added
 - Removed undocumented, unused `neo4j.data.map_type`
 - Query strings are now typed `LiteralString` instead of `str` to help mitigate
   accidental Cypher injections. There are rare use-cases where a computed
   string is necessary. Please use `# type: ignore`, or `typing.cast` to
   suppress the type checking in those cases.
+- The experimental bookmark manager feature was changed to no longer track
+  bookmarks per database.  
+  This effectively changes the signature of almost all bookmark
+  manager related methods:
+  - `neo4j.BookmarkManger` and `neo4j.AsyncBookmarkManger` abstract base
+    classes:
+    - ``update_bookmarks`` has no longer a ``database`` argument.
+    - ``get_bookmarks`` has no longer a ``database`` argument.
+    - The ``get_all_bookmarks`` method was removed.
+    - The ``forget`` method was removed.
+  - `neo4j.GraphDatabase.bookmark_manager` and
+    `neo4j.AsyncGraphDatabase.bookmark_manager` factory methods:
+    - ``initial_bookmarks`` is no longer a mapping from database name
+      to bookmarks but plain bookmarks.
+    - ``bookmarks_supplier`` no longer receives the database name as
+      an argument.
+    - ``bookmarks_consumer`` no longer receives the database name as
+      an argument.  
 
 
 ## Version 5.2

src/neo4j/_async/bookmark_manager.py

@@ -19,7 +19,6 @@
 from __future__ import annotations
 
 import typing as t
-from collections import defaultdict
 
 from .._async_compat.concurrency import AsyncCooperativeLock
 from .._async_compat.util import AsyncUtil
@@ -29,9 +28,8 @@
 )
 
 
-TBmSupplier = t.Callable[[t.Optional[str]],
-                         t.Union[Bookmarks, t.Awaitable[Bookmarks]]]
-TBmConsumer = t.Callable[[str, Bookmarks], t.Union[None, t.Awaitable[None]]]
+TBmSupplier = t.Callable[[], t.Union[Bookmarks, t.Awaitable[Bookmarks]]]
+TBmConsumer = t.Callable[[Bookmarks], t.Union[None, t.Awaitable[None]]]
 
 
 def _bookmarks_to_set(
@@ -45,62 +43,41 @@ def _bookmarks_to_set(
 class AsyncNeo4jBookmarkManager(AsyncBookmarkManager):
     def __init__(
         self,
-        initial_bookmarks: t.Optional[t.Mapping[str, t.Union[Bookmarks,
-                                                t.Iterable[str]]]] = None,
+        initial_bookmarks: t.Union[None, Bookmarks, t.Iterable[str]] = None,
         bookmarks_supplier: t.Optional[TBmSupplier] = None,
         bookmarks_consumer: t.Optional[TBmConsumer] = None
     ) -> None:
         super().__init__()
         self._bookmarks_supplier = bookmarks_supplier
         self._bookmarks_consumer = bookmarks_consumer
-        if initial_bookmarks is None:
-            initial_bookmarks = {}
-        self._bookmarks = defaultdict(
-            set, ((k, _bookmarks_to_set(v))
-                  for k, v in initial_bookmarks.items())
-        )
+        if not initial_bookmarks:
+            self._bookmarks = set()
+        else:
+            self._bookmarks = set(getattr(
+                initial_bookmarks, "raw_values",
+                t.cast(t.Iterable[str], initial_bookmarks)
+            ))
         self._lock = AsyncCooperativeLock()
 
     async def update_bookmarks(
-        self, database: str, previous_bookmarks: t.Collection[str],
+        self, previous_bookmarks: t.Collection[str],
         new_bookmarks: t.Collection[str]
     ) -> None:
         if not new_bookmarks:
             return
         with self._lock:
-            curr_bms = self._bookmarks[database]
-            curr_bms.difference_update(previous_bookmarks)
-            curr_bms.update(new_bookmarks)
+            self._bookmarks.difference_update(previous_bookmarks)
+            self._bookmarks.update(new_bookmarks)
             if self._bookmarks_consumer:
-                curr_bms_snapshot = Bookmarks.from_raw_values(curr_bms)
+                curr_bms_snapshot = Bookmarks.from_raw_values(self._bookmarks)
         if self._bookmarks_consumer:
-            await AsyncUtil.callback(
-                self._bookmarks_consumer, database, curr_bms_snapshot
-            )
+            await AsyncUtil.callback(self._bookmarks_consumer,
+                                     curr_bms_snapshot)
 
-    async def get_bookmarks(self, database: str) -> t.Set[str]:
+    async def get_bookmarks(self) -> t.Set[str]:
         with self._lock:
-            bms = set(self._bookmarks[database])
+            bms = set(self._bookmarks)
         if self._bookmarks_supplier:
-            extra_bms = await AsyncUtil.callback(
-                self._bookmarks_supplier, database
-            )
+            extra_bms = await AsyncUtil.callback(self._bookmarks_supplier)
             bms.update(extra_bms.raw_values)
         return bms
-
-    async def get_all_bookmarks(self) -> t.Set[str]:
-        bms: t.Set[str] = set()
-        with self._lock:
-            for database in self._bookmarks.keys():
-                bms.update(self._bookmarks[database])
-        if self._bookmarks_supplier:
-            extra_bms = await AsyncUtil.callback(
-                self._bookmarks_supplier, None
-            )
-            bms.update(extra_bms.raw_values)
-        return bms
-
-    async def forget(self, databases: t.Iterable[str]) -> None:
-        with self._lock:
-            for database in databases:
-                self._bookmarks.pop(database, None)

src/neo4j/_async/driver.py

@@ -220,8 +220,7 @@ def driver(cls, uri, *, auth=None, **config) -> AsyncDriver:
     )
     def bookmark_manager(
         cls,
-        initial_bookmarks: t.Optional[t.Mapping[str, t.Union[Bookmarks,
-                                                t.Iterable[str]]]] = None,
+        initial_bookmarks: t.Union[None, Bookmarks, t.Iterable[str]] = None,
         bookmarks_supplier: t.Optional[_TBmSupplier] = None,
         bookmarks_consumer: t.Optional[_TBmConsumer] = None
     ) -> AsyncBookmarkManager:
@@ -261,31 +260,40 @@ def bookmark_manager(
 
         :param initial_bookmarks:
             The initial set of bookmarks. The returned bookmark manager will
-            use this to initialize its internal bookmarks per database.
-            If present, this parameter must be a mapping of database names
-            to :class:`.Bookmarks` or an iterable of raw bookmark values (str).
+            use this to initialize its internal bookmarks.
         :param bookmarks_supplier:
             Function which will be called every time the default bookmark
             manager's method :meth:`.AsyncBookmarkManager.get_bookmarks`
-            or :meth:`.AsyncBookmarkManager.get_all_bookmarks` gets called.
-            The function will be passed the name of the database (``str``) if
-            ``.get_bookmarks`` is called or ``None`` if ``.get_all_bookmarks``
-            is called. The function must return a :class:`.Bookmarks` object.
-            The result of ``bookmarks_supplier`` will then be concatenated with
-            the internal set of bookmarks and used to configure the session in
-            creation.
+            gets called.
+            The function takes no arguments and must return a
+            :class:`.Bookmarks` object. The result of ``bookmarks_supplier``
+            will then be concatenated with the internal set of bookmarks and
+            used to configure the session in creation. It will, however, not
+            update the internal set of bookmarks.
         :param bookmarks_consumer:
             Function which will be called whenever the set of bookmarks
             handled by the bookmark manager gets updated with the new
-            internal bookmark set. It will receive the name of the database
-            and the new set of bookmarks.
+            internal bookmark set. It will receive the new set of bookmarks
+            as a :class:`.Bookmarks` object and return :const:`None`.
 
         :returns: A default implementation of :class:`AsyncBookmarkManager`.
 
         **This is experimental.** (See :ref:`filter-warnings-ref`)
         It might be changed or removed any time even without prior notice.
 
         .. versionadded:: 5.0
+
+        .. versionchanged:: 5.3
+            The bookmark manager no longer tracks bookmarks per database.
+            This effectively changes the signature of almost all bookmark
+            manager related methods:
+
+            * ``initial_bookmarks`` is no longer a mapping from database name
+              to bookmarks but plain bookmarks.
+            * ``bookmarks_supplier`` no longer receives the database name as
+              an argument.
+            * ``bookmarks_consumer`` no longer receives the database name as
+              an argument.
         """
         return AsyncNeo4jBookmarkManager(
             initial_bookmarks=initial_bookmarks,

src/neo4j/_async/io/_pool.py

@@ -704,11 +704,11 @@ async def ensure_routing_table_is_fresh(
                 # Remove unused databases in the routing table
                 # Remove the routing table after a timeout = TTL + 30s
                 log.debug("[#0000]  _: <POOL> routing aged?, database=%s",
-                          database)
+                          database_)
                 routing_table = self.routing_tables[database_]
                 if routing_table.should_be_purged_from_memory():
                     log.debug("[#0000]  _: <POOL> dropping routing table for "
-                              "database=%s", database)
+                              "database=%s", database_)
                     del self.routing_tables[database_]
 
             routing_table = await self.get_or_create_routing_table(database)

src/neo4j/_async/work/session.py

@@ -144,8 +144,7 @@ def _handle_cancellation(self, message="General"):
 
     async def _result_closed(self):
         if self._auto_result:
-            await self._update_bookmark(self._auto_result._database,
-                                        self._auto_result._bookmark)
+            await self._update_bookmark(self._auto_result._bookmark)
             self._auto_result = None
             await self._disconnect()
 
@@ -177,7 +176,6 @@ async def close(self) -> None:
                     try:
                         await self._auto_result.consume()
                         await self._update_bookmark(
-                            self._auto_result._database,
                             self._auto_result._bookmark
                         )
                     except Exception as error:
@@ -286,7 +284,7 @@ async def run(
             cx, self._config.fetch_size, self._result_closed,
             self._result_error
         )
-        bookmarks = await self._get_all_bookmarks()
+        bookmarks = await self._get_bookmarks()
         parameters = dict(parameters or {}, **kwargs)
         await self._auto_result._run(
             query, parameters, self._config.database,
@@ -322,8 +320,7 @@ async def last_bookmark(self) -> t.Optional[str]:
             await self._auto_result.consume()
 
         if self._transaction and self._transaction._closed():
-            await self._update_bookmark(self._transaction._database,
-                                        self._transaction._bookmark)
+            await self._update_bookmark(self._transaction._bookmark)
             self._transaction = None
 
         if self._bookmarks:
@@ -364,16 +361,14 @@ async def last_bookmarks(self) -> Bookmarks:
             await self._auto_result.consume()
 
         if self._transaction and self._transaction._closed():
-            await self._update_bookmark(self._transaction._database,
-                                        self._transaction._bookmark)
+            await self._update_bookmark(self._transaction._bookmark)
             self._transaction = None
 
         return Bookmarks.from_raw_values(self._bookmarks)
 
     async def _transaction_closed_handler(self):
         if self._transaction:
-            await self._update_bookmark(self._transaction._database,
-                                        self._transaction._bookmark)
+            await self._update_bookmark(self._transaction._bookmark)
             self._transaction = None
             await self._disconnect()
 
@@ -397,7 +392,7 @@ async def _open_transaction(
             self._transaction_error_handler,
             self._transaction_cancel_handler
         )
-        bookmarks = await self._get_all_bookmarks()
+        bookmarks = await self._get_bookmarks()
         await self._transaction._begin(
             self._config.database, self._config.impersonated_user,
             bookmarks, access_mode, metadata, timeout

src/neo4j/_async/work/workspace.py

@@ -102,63 +102,33 @@ def _initialize_bookmarks(self, bookmarks):
                             "iterable of raw bookmarks (deprecated).")
         self._initial_bookmarks = self._bookmarks = prepared_bookmarks
 
-    async def _get_bookmarks(self, database):
+    async def _get_bookmarks(self,):
         if self._bookmark_manager is None:
             return self._bookmarks
 
-        # For 4.3- support: the server will not send the resolved home
-        # database back. To avoid confusion between `None` as in "all
-        # database" and `None` as in "home database" we re-write the
-        # home database to `""`, which otherwise is an invalid database
-        # name. It will not work properly either way, as the home database
-        # can change (server config change or client side user change).
-        if database is None:
-            database = ""
         self._last_from_bookmark_manager = tuple({
-            *await AsyncUtil.callback(
-                self._bookmark_manager.get_bookmarks, database
-            ),
+            *await AsyncUtil.callback(self._bookmark_manager.get_bookmarks),
             *self._initial_bookmarks
         })
         return self._last_from_bookmark_manager
 
-    async def _get_all_bookmarks(self):
-        if self._bookmark_manager is None:
-            return self._bookmarks
-
-        self._last_from_bookmark_manager = tuple({
-            *await AsyncUtil.callback(
-                self._bookmark_manager.get_all_bookmarks,
-            ),
-            *self._initial_bookmarks
-        })
-        return self._last_from_bookmark_manager
-
-    async def _update_bookmarks(self, database, new_bookmarks):
+    async def _update_bookmarks(self, new_bookmarks):
         if not new_bookmarks:
             return
         self._initial_bookmarks = ()
         self._bookmarks = new_bookmarks
         if self._bookmark_manager is None:
             return
         previous_bookmarks = self._last_from_bookmark_manager
-        # For 4.3- support: the server will not send the resolved home
-        # database back. To avoid confusion between `None` as in "all
-        # database" and `None` as in "home database" we re-write the home
-        # database to `""`, which otherwise is an invalid database name.
-        if database is None:
-            database = ""
         await AsyncUtil.callback(
             self._bookmark_manager.update_bookmarks,
-            database, previous_bookmarks, new_bookmarks
+            previous_bookmarks, new_bookmarks
         )
 
-    async def _update_bookmark(self, database, bookmark):
+    async def _update_bookmark(self, bookmark):
         if not bookmark:
             return
-        if not database:
-            database = self._config.database
-        await self._update_bookmarks(database, (bookmark,))
+        await self._update_bookmarks((bookmark,))
 
     async def _connect(self, access_mode, **acquire_kwargs):
         acquisition_timeout = self._config.connection_acquisition_timeout
@@ -183,15 +153,15 @@ async def _connect(self, access_mode, **acquire_kwargs):
                 await self._pool.update_routing_table(
                     database=self._config.database,
                     imp_user=self._config.impersonated_user,
-                    bookmarks=await self._get_bookmarks("system"),
+                    bookmarks=await self._get_bookmarks(),
                     acquisition_timeout=acquisition_timeout,
                     database_callback=self._set_cached_database
                 )
         acquire_kwargs_ = {
             "access_mode": access_mode,
             "timeout": acquisition_timeout,
             "database": self._config.database,
-            "bookmarks": await self._get_bookmarks("system"),
+            "bookmarks": await self._get_bookmarks(),
             "liveness_check_timeout": None,
         }
         acquire_kwargs_.update(acquire_kwargs)