Deprecate Session.last_bookmark for Session.last_bookmarks (#649)

Deprecate `Session.last_bookmark`.

Add `Session.last_bookmarks` which returns a list of strings instead of an
optional string.
Furthermore, it will return *all* initial bookmarks (if not updated by a  server
message since session creation) instead of only the last bookmark, which not
necessarily would be the latest bookmark.

Add `neo4j.Bookmarks` to replace (now deprecated) `neo4j.Bookmark` and use
if for managing bookmarks instead of raw bookmark strings.

Author: robsdedude

CHANGELOG.md

@@ -54,6 +54,13 @@
   was silently ignored.
 - `Result.single` now raises `ResultNotSingleError` if not exactly one result is
   available.
+- Bookmarks
+  - `Session.last_bookmark` was deprecated. Its behaviour is partially incorrect
+    and cannot be fixed without breaking its signature.  
+    Use `Session.last_bookmarks` instead.
+  - `neo4j.Bookmark` was deprecated.  
+    Use `neo4j.Bookmarks` instead.
+
 
 ## Version 4.4
 

docs/source/api.rst

@@ -447,6 +447,8 @@ Session
 
     .. automethod:: run
 
+    .. automethod:: last_bookmarks
+
     .. automethod:: last_bookmark
 
     .. automethod:: begin_transaction
@@ -481,9 +483,16 @@ To construct a :class:`neo4j.Session` use the :meth:`neo4j.Driver.session` metho
 
 ``bookmarks``
 -------------
-An iterable containing :class:`neo4j.Bookmark`
+Optional :class:`neo4j.Bookmarks`. Use this to causally chain sessions.
+See :meth:`Session.last_bookmarks` or :meth:`AsyncSession.last_bookmarks` for
+more information.
+
+.. deprecated:: 5.0
+    Alternatively, an iterable of strings can be passed. This usage is
+    deprecated and will be removed in a future release. Please use a
+    :class:`neo4j.Bookmarks` object instead.
 
-:Default: ``()``
+:Default: ``None``
 
 
 .. _database-ref:
@@ -1366,9 +1375,12 @@ This example shows how to suppress the :class:`neo4j.ExperimentalWarning` using
     warnings.filterwarnings("ignore", category=ExperimentalWarning)
 
 
-********
-Bookmark
-********
+*********
+Bookmarks
+*********
+
+.. autoclass:: neo4j.Bookmarks
+    :members:
 
 .. autoclass:: neo4j.Bookmark
     :members:

docs/source/async_api.rst

@@ -299,6 +299,8 @@ AsyncSession
 
     .. automethod:: run
 
+    .. automethod:: last_bookmarks
+
     .. automethod:: last_bookmark
 
     .. automethod:: begin_transaction

neo4j/__init__.py

@@ -32,6 +32,7 @@
     "bearer_auth",
     "BoltDriver",
     "Bookmark",
+    "Bookmarks",
     "Config",
     "custom_auth",
     "DEFAULT_DATABASE",
@@ -97,6 +98,7 @@
     basic_auth,
     bearer_auth,
     Bookmark,
+    Bookmarks,
     custom_auth,
     DEFAULT_DATABASE,
     kerberos_auth,

neo4j/_async/work/session.py

@@ -23,6 +23,7 @@
 
 from ..._async_compat import async_sleep
 from ...api import (
+    Bookmarks,
     READ_ACCESS,
     WRITE_ACCESS,
 )
@@ -37,6 +38,10 @@
     TransactionError,
     TransientError,
 )
+from ...meta import (
+    deprecated,
+    deprecation_warn,
+)
 from ...work import Query
 from .result import AsyncResult
 from .transaction import AsyncTransaction
@@ -85,7 +90,7 @@ class AsyncSession(AsyncWorkspace):
     def __init__(self, pool, session_config):
         super().__init__(pool, session_config)
         assert isinstance(session_config, SessionConfig)
-        self._bookmarks = tuple(session_config.bookmarks)
+        self._bookmarks = self._prepare_bookmarks(session_config.bookmarks)
 
     def __del__(self):
         if asyncio.iscoroutinefunction(self.close):
@@ -103,14 +108,29 @@ async def __aexit__(self, exception_type, exception_value, traceback):
             self._state_failed = True
         await self.close()
 
+    def _prepare_bookmarks(self, bookmarks):
+        if isinstance(bookmarks, Bookmarks):
+            return tuple(bookmarks.raw_values)
+        if hasattr(bookmarks, "__iter__"):
+            deprecation_warn(
+                "Passing an iterable as `bookmarks` to `Session` is "
+                "deprecated. Please use a `Bookmarks` instance.",
+                stack_level=5
+            )
+            return tuple(bookmarks)
+        if not bookmarks:
+            return ()
+        raise TypeError("Bookmarks must be an instance of Bookmarks or an "
+                        "iterable of raw bookmarks (deprecated).")
+
     async def _connect(self, access_mode):
         if access_mode is None:
             access_mode = self._config.default_access_mode
         await super()._connect(access_mode)
 
     def _collect_bookmark(self, bookmark):
         if bookmark:
-            self._bookmarks = [bookmark]
+            self._bookmarks = bookmark,
 
     async def _result_closed(self):
         if self._auto_result:
@@ -222,11 +242,26 @@ async def run(self, query, parameters=None, **kwargs):
 
         return self._auto_result
 
+    @deprecated(
+        "`last_bookmark` has been deprecated in favor of `last_bookmarks`. "
+        "This method can lead to unexpected behaviour."
+    )
     async def last_bookmark(self):
         """Return the bookmark received following the last completed transaction.
-        Note: For auto-transaction (Session.run) this will trigger an consume for the current result.
 
-        :returns: :class:`neo4j.Bookmark` object
+        Note: For auto-transactions (:meth:`Session.run`), this will trigger
+        :meth:`Result.consume` for the current result.
+
+        .. warning::
+            This method can lead to unexpected behaviour if the session has not
+            yet successfully completed a transaction.
+
+        .. deprecated:: 5.0
+            :meth:`last_bookmark` will be removed in version 6.0.
+            Use :meth:`last_bookmarks` instead.
+
+        :returns: last bookmark
+        :rtype: str or None
         """
         # The set of bookmarks to be passed into the next transaction.
 
@@ -237,10 +272,50 @@ async def last_bookmark(self):
             self._collect_bookmark(self._transaction._bookmark)
             self._transaction = None
 
-        if len(self._bookmarks):
-            return self._bookmarks[len(self._bookmarks)-1]
+        if self._bookmarks:
+            return self._bookmarks[-1]
         return None
 
+    async def last_bookmarks(self):
+        """Return most recent bookmarks of the session.
+
+        Bookmarks can be used to causally chain sessions. For example,
+        if a session (``session1``) wrote something, that another session
+        (``session2``) needs to read, use
+        ``session2 = driver.session(bookmarks=session1.last_bookmarks())`` to
+        achieve this.
+
+        Combine the bookmarks of multiple sessions like so::
+
+            bookmarks1 = await session1.last_bookmarks()
+            bookmarks2 = await session2.last_bookmarks()
+            session3 = driver.session(bookmarks=bookmarks1 + bookmarks2)
+
+        A session automatically manages bookmarks, so this method is rarely
+        needed. If you need causal consistency, try to run the relevant queries
+        in the same session.
+
+        "Most recent bookmarks" are either the bookmarks passed to the session
+        or creation, or the last bookmark the session received after committing
+        a transaction to the server.
+
+        Note: For auto-transactions (:meth:`Session.run`), this will trigger
+        :meth:`Result.consume` for the current result.
+
+        :returns: the session's last known bookmarks
+        :rtype: Bookmarks
+        """
+        # The set of bookmarks to be passed into the next transaction.
+
+        if self._auto_result:
+            await self._auto_result.consume()
+
+        if self._transaction and self._transaction._closed:
+            self._collect_bookmark(self._transaction._bookmark)
+            self._transaction = None
+
+        return Bookmarks.from_raw_values(self._bookmarks)
+
     async def _transaction_closed_handler(self):
         if self._transaction:
             self._collect_bookmark(self._transaction._bookmark)

neo4j/_async_compat/network/__init__.py

@@ -16,11 +16,11 @@
 # limitations under the License.
 
 
-from .bolt_socket import (
+from ._bolt_socket import (
     AsyncBoltSocket,
     BoltSocket,
 )
-from .util import (
+from ._util import (
     AsyncNetworkUtil,
     NetworkUtil,
 )

neo4j/_async_compat/network/bolt_socket.py renamed to neo4j/_async_compat/network/_bolt_socket.py

@@ -46,7 +46,7 @@
     DriverError,
     ServiceUnavailable,
 )
-from .util import (
+from ._util import (
     AsyncNetworkUtil,
     NetworkUtil,
 )

neo4j/_async_compat/network/util.py renamed to neo4j/_async_compat/network/_util.py

@@ -21,6 +21,12 @@
 from ..meta import experimental
 
 
+__all__ = [
+    "AsyncUtil",
+    "Util",
+]
+
+
 class AsyncUtil:
     @staticmethod
     async def iter(it):