Deprecate Session.last_bookmark instead of removing it

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

Author: robsdedude

CHANGELOG.md

@@ -54,8 +54,13 @@
   was silently ignored.
 - `Result.single` now raises `ResultNotSingleError` if not exactly one result is
   available.
-- `Session.last_bookmark` was renamed to `Session.last_bookmarks`.
-  It returns a list of strings (previously string or None).
+- 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
@@ -1366,9 +1368,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,6 +108,18 @@ async def __aexit__(self, exception_type, exception_value, traceback):
             self._state_failed = True
         await self.close()
 
+    def _prepare_bookmarks(self, bookmarks):
+        if not bookmarks:
+            return ()
+        if isinstance(bookmarks, Bookmarks):
+            return tuple(bookmarks.raw_values)
+        if hasattr(bookmarks, "__iter__"):
+            deprecation_warn("Passing an iterable to `bookmarks` is "
+                             "deprecated. Please use a Bookmarks instance.")
+            return tuple(bookmarks)
+        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
@@ -222,6 +239,40 @@ 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-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.
+
+        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
+
+        if self._bookmarks:
+            return self._bookmarks[-1]
+        return None
+
     async def last_bookmarks(self):
         """Return most recent bookmarks of the session.
 
@@ -231,6 +282,12 @@ async def last_bookmarks(self):
         ``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.
@@ -239,11 +296,11 @@ async def last_bookmarks(self):
         or creation, or the last bookmark the session received after committing
         a transaction to the server.
 
-        Note: For auto-transaction (Session.run) this will trigger a
-        ``consume`` for the current result.
+        Note: For auto-transactions (:meth:`Session.run`), this will trigger
+        :meth:`Result.consume` for the current result.
 
-        :returns: list of bookmarks
-        :rtype: list[str]
+        :returns: the session's last known bookmarks
+        :rtype: Bookmarks
         """
         # The set of bookmarks to be passed into the next transaction.
 
@@ -254,7 +311,7 @@ async def last_bookmarks(self):
             self._collect_bookmark(self._transaction._bookmark)
             self._transaction = None
 
-        return list(self._bookmarks)
+        return Bookmarks.from_raw_values(self._bookmarks)
 
     async def _transaction_closed_handler(self):
         if self._transaction:

neo4j/_sync/work/session.py

@@ -23,6 +23,7 @@
 
 from ..._async_compat import 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 Result
 from .transaction import Transaction
@@ -85,7 +90,7 @@ class Session(Workspace):
     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,6 +108,18 @@ def __exit__(self, exception_type, exception_value, traceback):
             self._state_failed = True
         self.close()
 
+    def _prepare_bookmarks(self, bookmarks):
+        if not bookmarks:
+            return ()
+        if isinstance(bookmarks, Bookmarks):
+            return tuple(bookmarks.raw_values)
+        if hasattr(bookmarks, "__iter__"):
+            deprecation_warn("Passing an iterable to `bookmarks` is "
+                             "deprecated. Please use a Bookmarks instance.")
+            return tuple(bookmarks)
+        raise TypeError("Bookmarks must be an instance of Bookmarks or an "
+                        "iterable of raw bookmarks (deprecated).")
+
     def _connect(self, access_mode):
         if access_mode is None:
             access_mode = self._config.default_access_mode
@@ -222,6 +239,40 @@ 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."
+    )
+    def last_bookmark(self):
+        """Return the bookmark received following the last completed transaction.
+
+        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.
+
+        if self._auto_result:
+            self._auto_result.consume()
+
+        if self._transaction and self._transaction._closed:
+            self._collect_bookmark(self._transaction._bookmark)
+            self._transaction = None
+
+        if self._bookmarks:
+            return self._bookmarks[-1]
+        return None
+
     def last_bookmarks(self):
         """Return most recent bookmarks of the session.
 
@@ -231,6 +282,12 @@ def last_bookmarks(self):
         ``session2 = driver.session(bookmarks=session1.last_bookmarks())`` to
         achieve this.
 
+        Combine the bookmarks of multiple sessions like so::
+
+            bookmarks1 = session1.last_bookmarks()
+            bookmarks2 = 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.
@@ -239,11 +296,11 @@ def last_bookmarks(self):
         or creation, or the last bookmark the session received after committing
         a transaction to the server.
 
-        Note: For auto-transaction (Session.run) this will trigger a
-        ``consume`` for the current result.
+        Note: For auto-transactions (:meth:`Session.run`), this will trigger
+        :meth:`Result.consume` for the current result.
 
-        :returns: list of bookmarks
-        :rtype: list[str]
+        :returns: the session's last known bookmarks
+        :rtype: Bookmarks
         """
         # The set of bookmarks to be passed into the next transaction.
 
@@ -254,7 +311,7 @@ def last_bookmarks(self):
             self._collect_bookmark(self._transaction._bookmark)
             self._transaction = None
 
-        return list(self._bookmarks)
+        return Bookmarks.from_raw_values(self._bookmarks)
 
     def _transaction_closed_handler(self):
         if self._transaction: