From a874ea72c49502d2eaabd3b1f2e368a47e4c3a1d Mon Sep 17 00:00:00 2001 From: Rouven Bauer Date: Tue, 25 Jan 2022 14:21:43 +0100 Subject: [PATCH 1/4] Replace `Session.last_bookmark` with `Session.last_bookmarks` Renamed the attribute and made it return 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. --- CHANGELOG.md | 2 ++ neo4j/_async/work/session.py | 31 ++++++++++++++----- neo4j/_sync/work/session.py | 31 ++++++++++++++----- testkitbackend/_async/requests.py | 5 +-- testkitbackend/_sync/requests.py | 5 +-- .../examples/test_pass_bookmarks_example.py | 4 +-- tests/unit/async_/work/test_session.py | 13 +++++--- tests/unit/sync/work/test_session.py | 13 +++++--- 8 files changed, 70 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d7dd37b3..ee53ad6fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,8 @@ 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). ## Version 4.4 diff --git a/neo4j/_async/work/session.py b/neo4j/_async/work/session.py index f1ba97da2..dcf090fe3 100644 --- a/neo4j/_async/work/session.py +++ b/neo4j/_async/work/session.py @@ -110,7 +110,7 @@ async def _connect(self, 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 +222,28 @@ async def run(self, query, parameters=None, **kwargs): return self._auto_result - 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. + async def last_bookmarks(self): + """Return most recent bookmarks of the session. - :returns: :class:`neo4j.Bookmark` object + 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. + + 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-transaction (Session.run) this will trigger a + ``consume`` for the current result. + + :returns: list of bookmarks + :rtype: list[str] """ # The set of bookmarks to be passed into the next transaction. @@ -237,9 +254,7 @@ 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] - return None + return list(self._bookmarks) async def _transaction_closed_handler(self): if self._transaction: diff --git a/neo4j/_sync/work/session.py b/neo4j/_sync/work/session.py index dbdf94828..9840b4038 100644 --- a/neo4j/_sync/work/session.py +++ b/neo4j/_sync/work/session.py @@ -110,7 +110,7 @@ def _connect(self, access_mode): def _collect_bookmark(self, bookmark): if bookmark: - self._bookmarks = [bookmark] + self._bookmarks = bookmark, def _result_closed(self): if self._auto_result: @@ -222,11 +222,28 @@ def run(self, query, parameters=None, **kwargs): return self._auto_result - 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. + def last_bookmarks(self): + """Return most recent bookmarks of the session. - :returns: :class:`neo4j.Bookmark` object + 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. + + 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-transaction (Session.run) this will trigger a + ``consume`` for the current result. + + :returns: list of bookmarks + :rtype: list[str] """ # The set of bookmarks to be passed into the next transaction. @@ -237,9 +254,7 @@ def last_bookmark(self): self._collect_bookmark(self._transaction._bookmark) self._transaction = None - if len(self._bookmarks): - return self._bookmarks[len(self._bookmarks)-1] - return None + return list(self._bookmarks) def _transaction_closed_handler(self): if self._transaction: diff --git a/testkitbackend/_async/requests.py b/testkitbackend/_async/requests.py index 26f159e10..0b9dfd453 100644 --- a/testkitbackend/_async/requests.py +++ b/testkitbackend/_async/requests.py @@ -323,10 +323,7 @@ async def RetryableNegative(backend, data): async def SessionLastBookmarks(backend, data): key = data["sessionId"] session = backend.sessions[key].session - bookmark = await session.last_bookmark() - bookmarks = [] - if bookmark: - bookmarks.append(bookmark) + bookmarks = await session.last_bookmarks() await backend.send_response("Bookmarks", {"bookmarks": bookmarks}) diff --git a/testkitbackend/_sync/requests.py b/testkitbackend/_sync/requests.py index cfdfb3751..ce14570ad 100644 --- a/testkitbackend/_sync/requests.py +++ b/testkitbackend/_sync/requests.py @@ -323,10 +323,7 @@ def RetryableNegative(backend, data): def SessionLastBookmarks(backend, data): key = data["sessionId"] session = backend.sessions[key].session - bookmark = session.last_bookmark() - bookmarks = [] - if bookmark: - bookmarks.append(bookmark) + bookmarks = session.last_bookmarks() backend.send_response("Bookmarks", {"bookmarks": bookmarks}) diff --git a/tests/integration/examples/test_pass_bookmarks_example.py b/tests/integration/examples/test_pass_bookmarks_example.py index 51cc33d9b..a582d8025 100644 --- a/tests/integration/examples/test_pass_bookmarks_example.py +++ b/tests/integration/examples/test_pass_bookmarks_example.py @@ -76,13 +76,13 @@ def main(self): with self.driver.session() as session_a: session_a.write_transaction(self.create_person, "Alice") session_a.write_transaction(self.employ, "Alice", "Wayne Enterprises") - saved_bookmarks.append(session_a.last_bookmark()) + saved_bookmarks.extend(session_a.last_bookmarks()) # Create the second person and employment relationship. with self.driver.session() as session_b: session_b.write_transaction(self.create_person, "Bob") session_b.write_transaction(self.employ, "Bob", "LexCorp") - saved_bookmarks.append(session_b.last_bookmark()) + saved_bookmarks.extend(session_b.last_bookmarks()) # Create a friendship between the two people created above. with self.driver.session(bookmarks=saved_bookmarks) as session_c: diff --git a/tests/unit/async_/work/test_session.py b/tests/unit/async_/work/test_session.py index cfba57dd0..fd93270cd 100644 --- a/tests/unit/async_/work/test_session.py +++ b/tests/unit/async_/work/test_session.py @@ -169,16 +169,21 @@ async def test_closes_connection_after_tx_commit(pool, test_run_args): assert session._connection is None -@pytest.mark.parametrize("bookmarks", (None, [], ["abc"], ["foo", "bar"])) +@pytest.mark.parametrize( + "bookmarks", + (None, [], ["abc"], ["foo", "bar"], {"a", "b"}, ("1", "two")) +) @mark_async_test async def test_session_returns_bookmark_directly(pool, bookmarks): async with AsyncSession( pool, SessionConfig(bookmarks=bookmarks) ) as session: - if bookmarks: - assert await session.last_bookmark() == bookmarks[-1] + if bookmarks is None: + assert await session.last_bookmarks() == [] + elif isinstance(bookmarks, set): + assert sorted(await session.last_bookmarks()) == sorted(bookmarks) else: - assert await session.last_bookmark() is None + assert await session.last_bookmarks() == list(bookmarks) @pytest.mark.parametrize(("query", "error_type"), ( diff --git a/tests/unit/sync/work/test_session.py b/tests/unit/sync/work/test_session.py index a985fc6b9..d7b63be20 100644 --- a/tests/unit/sync/work/test_session.py +++ b/tests/unit/sync/work/test_session.py @@ -169,16 +169,21 @@ def test_closes_connection_after_tx_commit(pool, test_run_args): assert session._connection is None -@pytest.mark.parametrize("bookmarks", (None, [], ["abc"], ["foo", "bar"])) +@pytest.mark.parametrize( + "bookmarks", + (None, [], ["abc"], ["foo", "bar"], {"a", "b"}, ("1", "two")) +) @mark_sync_test def test_session_returns_bookmark_directly(pool, bookmarks): with Session( pool, SessionConfig(bookmarks=bookmarks) ) as session: - if bookmarks: - assert session.last_bookmark() == bookmarks[-1] + if bookmarks is None: + assert session.last_bookmarks() == [] + elif isinstance(bookmarks, set): + assert sorted(session.last_bookmarks()) == sorted(bookmarks) else: - assert session.last_bookmark() is None + assert session.last_bookmarks() == list(bookmarks) @pytest.mark.parametrize(("query", "error_type"), ( From ac74fdf4061e2db79e48a36bcf296c3f6caefaf5 Mon Sep 17 00:00:00 2001 From: Rouven Bauer Date: Fri, 28 Jan 2022 14:42:42 +0100 Subject: [PATCH 2/4] 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. --- CHANGELOG.md | 9 +- docs/source/api.rst | 11 +- docs/source/async_api.rst | 2 + neo4j/__init__.py | 2 + neo4j/_async/work/session.py | 69 ++++++++++- neo4j/_sync/work/session.py | 69 ++++++++++- neo4j/api.py | 78 ++++++++++++- neo4j/conf.py | 3 +- testkitbackend/_async/requests.py | 8 +- testkitbackend/_sync/requests.py | 8 +- .../examples/test_pass_bookmarks_example.py | 11 +- tests/unit/async_/work/test_session.py | 20 ++-- tests/unit/common/test_api.py | 110 +++++++++++++++--- tests/unit/sync/work/test_session.py | 20 ++-- 14 files changed, 361 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee53ad6fb..0aa94c420 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/source/api.rst b/docs/source/api.rst index 64659a2db..c146f35cb 100644 --- a/docs/source/api.rst +++ b/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: diff --git a/docs/source/async_api.rst b/docs/source/async_api.rst index 29d1a35f5..9e2ae8645 100644 --- a/docs/source/async_api.rst +++ b/docs/source/async_api.rst @@ -299,6 +299,8 @@ AsyncSession .. automethod:: run + .. automethod:: last_bookmarks + .. automethod:: last_bookmark .. automethod:: begin_transaction diff --git a/neo4j/__init__.py b/neo4j/__init__.py index 4fe2efd5a..46a39f66e 100644 --- a/neo4j/__init__.py +++ b/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, diff --git a/neo4j/_async/work/session.py b/neo4j/_async/work/session.py index dcf090fe3..8ff2c5821 100644 --- a/neo4j/_async/work/session.py +++ b/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: diff --git a/neo4j/_sync/work/session.py b/neo4j/_sync/work/session.py index 9840b4038..d05f7596e 100644 --- a/neo4j/_sync/work/session.py +++ b/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: diff --git a/neo4j/api.py b/neo4j/api.py index ae9aabec2..0c920311d 100644 --- a/neo4j/api.py +++ b/neo4j/api.py @@ -24,10 +24,7 @@ urlparse, ) -from .exceptions import ( - ConfigurationError, - DriverError, -) +from .exceptions import ConfigurationError from .meta import deprecated @@ -167,9 +164,14 @@ def custom_auth(principal, credentials, realm, scheme, **parameters): class Bookmark: """A Bookmark object contains an immutable list of bookmark string values. + .. deprecated:: 5.0 + `Bookmark` will be removed in version 6.0. + Use :class:`Bookmarks` instead. + :param values: ASCII string values """ + @deprecated("Use the `Bookmarks`` class instead.") def __init__(self, *values): if values: bookmarks = [] @@ -202,6 +204,74 @@ def values(self): return self._values +class Bookmarks: + """Container for an immutable set of bookmark string values. + + Bookmarks are used to causally chain session. + See :meth:`Session.last_bookmarks` or :meth:`AsyncSession.last_bookmarks` + for more information. + + Use addition to combine multiple Bookmarks objects:: + + bookmarks3 = bookmarks1 + bookmarks2 + """ + + def __init__(self): + self._raw_values = frozenset() + + def __repr__(self): + """ + :return: repr string with sorted values + """ + return "".format( + ", ".join(map(repr, sorted(self._raw_values))) + ) + + def __bool__(self): + return bool(self._raw_values) + + def __add__(self, other): + if isinstance(other, Bookmarks): + if not other: + return self + ret = self.__class__() + ret._raw_values = self._raw_values | other._raw_values + return ret + return NotImplemented + + @property + def raw_values(self): + """The raw bookmark values. + + You should not need to access them unless you want to serialize + bookmarks. + + :return: immutable list of bookmark string values + :rtype: frozenset[str] + """ + return self._raw_values + + @classmethod + def from_raw_values(cls, values): + """Create a Bookmarks object from a list of raw bookmark string values. + + You should not need to use this method unless you want to deserialize + bookmarks. + + :param values: ASCII string values (raw bookmarks) + :type values: Iterable[str] + """ + obj = cls() + bookmarks = [] + for value in values: + if not isinstance(value, str): + raise TypeError("Raw bookmark values must be str. " + "Found {}".format(type(value))) + bookmarks.append(value) + obj._raw_values = frozenset(bookmarks) + return obj + + class ServerInfo: """ Represents a package of information relating to a Neo4j server. """ diff --git a/neo4j/conf.py b/neo4j/conf.py index 058b45d81..b5c0cfd34 100644 --- a/neo4j/conf.py +++ b/neo4j/conf.py @@ -310,11 +310,10 @@ class SessionConfig(WorkspaceConfig): """ #: Bookmarks - bookmarks = () + bookmarks = None #: Default AccessMode default_access_mode = WRITE_ACCESS - # access_mode = DeprecatedAlias("default_access_mode") class TransactionConfig(Config): diff --git a/testkitbackend/_async/requests.py b/testkitbackend/_async/requests.py index 0b9dfd453..7da6f7872 100644 --- a/testkitbackend/_async/requests.py +++ b/testkitbackend/_async/requests.py @@ -227,9 +227,12 @@ async def NewSession(backend, data): access_mode = neo4j.WRITE_ACCESS else: raise ValueError("Unknown access mode:" + access_mode) + bookmarks = None + if "bookmarks" in data and data["bookmarks"]: + bookmarks = neo4j.Bookmarks.from_raw_values(data["bookmarks"]) config = { "default_access_mode": access_mode, - "bookmarks": data["bookmarks"], + "bookmarks": bookmarks, "database": data["database"], "fetch_size": data.get("fetchSize", None), "impersonated_user": data.get("impersonatedUser", None), @@ -324,7 +327,8 @@ async def SessionLastBookmarks(backend, data): key = data["sessionId"] session = backend.sessions[key].session bookmarks = await session.last_bookmarks() - await backend.send_response("Bookmarks", {"bookmarks": bookmarks}) + await backend.send_response("Bookmarks", + {"bookmarks": list(bookmarks.raw_values)}) async def TransactionRun(backend, data): diff --git a/testkitbackend/_sync/requests.py b/testkitbackend/_sync/requests.py index ce14570ad..a58abd51b 100644 --- a/testkitbackend/_sync/requests.py +++ b/testkitbackend/_sync/requests.py @@ -227,9 +227,12 @@ def NewSession(backend, data): access_mode = neo4j.WRITE_ACCESS else: raise ValueError("Unknown access mode:" + access_mode) + bookmarks = None + if "bookmarks" in data and data["bookmarks"]: + bookmarks = neo4j.Bookmarks.from_raw_values(data["bookmarks"]) config = { "default_access_mode": access_mode, - "bookmarks": data["bookmarks"], + "bookmarks": bookmarks, "database": data["database"], "fetch_size": data.get("fetchSize", None), "impersonated_user": data.get("impersonatedUser", None), @@ -324,7 +327,8 @@ def SessionLastBookmarks(backend, data): key = data["sessionId"] session = backend.sessions[key].session bookmarks = session.last_bookmarks() - backend.send_response("Bookmarks", {"bookmarks": bookmarks}) + backend.send_response("Bookmarks", + {"bookmarks": list(bookmarks.raw_values)}) def TransactionRun(backend, data): diff --git a/tests/integration/examples/test_pass_bookmarks_example.py b/tests/integration/examples/test_pass_bookmarks_example.py index a582d8025..545805d30 100644 --- a/tests/integration/examples/test_pass_bookmarks_example.py +++ b/tests/integration/examples/test_pass_bookmarks_example.py @@ -24,7 +24,10 @@ # isort: off # tag::pass-bookmarks-import[] -from neo4j import GraphDatabase +from neo4j import ( + Bookmarks, + GraphDatabase, +) # end::pass-bookmarks-import[] # isort: on @@ -70,19 +73,19 @@ def print_friendships(cls, tx): print("{} knows {}".format(record["a.name"], record["b.name"])) def main(self): - saved_bookmarks = [] # To collect the session bookmarks + saved_bookmarks = Bookmarks() # To collect the session bookmarks # Create the first person and employment relationship. with self.driver.session() as session_a: session_a.write_transaction(self.create_person, "Alice") session_a.write_transaction(self.employ, "Alice", "Wayne Enterprises") - saved_bookmarks.extend(session_a.last_bookmarks()) + saved_bookmarks += session_a.last_bookmarks() # Create the second person and employment relationship. with self.driver.session() as session_b: session_b.write_transaction(self.create_person, "Bob") session_b.write_transaction(self.employ, "Bob", "LexCorp") - saved_bookmarks.extend(session_b.last_bookmarks()) + saved_bookmarks += session_a.last_bookmarks() # Create a friendship between the two people created above. with self.driver.session(bookmarks=saved_bookmarks) as session_c: diff --git a/tests/unit/async_/work/test_session.py b/tests/unit/async_/work/test_session.py index fd93270cd..7130c30dc 100644 --- a/tests/unit/async_/work/test_session.py +++ b/tests/unit/async_/work/test_session.py @@ -23,6 +23,7 @@ from neo4j import ( AsyncSession, AsyncTransaction, + Bookmarks, SessionConfig, unit_of_work, ) @@ -170,20 +171,25 @@ async def test_closes_connection_after_tx_commit(pool, test_run_args): @pytest.mark.parametrize( - "bookmarks", + "bookmark_values", (None, [], ["abc"], ["foo", "bar"], {"a", "b"}, ("1", "two")) ) @mark_async_test -async def test_session_returns_bookmark_directly(pool, bookmarks): +async def test_session_returns_bookmarks_directly(pool, bookmark_values): + if bookmark_values is not None: + bookmarks = Bookmarks.from_raw_values(bookmark_values) + else: + bookmarks = Bookmarks() async with AsyncSession( pool, SessionConfig(bookmarks=bookmarks) ) as session: - if bookmarks is None: - assert await session.last_bookmarks() == [] - elif isinstance(bookmarks, set): - assert sorted(await session.last_bookmarks()) == sorted(bookmarks) + ret_bookmarks = (await session.last_bookmarks()) + assert isinstance(ret_bookmarks, Bookmarks) + ret_bookmarks = ret_bookmarks.raw_values + if bookmark_values is None: + assert ret_bookmarks == frozenset() else: - assert await session.last_bookmarks() == list(bookmarks) + assert ret_bookmarks == frozenset(bookmark_values) @pytest.mark.parametrize(("query", "error_type"), ( diff --git a/tests/unit/common/test_api.py b/tests/unit/common/test_api.py index 78bf76395..ceb427aac 100644 --- a/tests/unit/common/test_api.py +++ b/tests/unit/common/test_api.py @@ -16,6 +16,8 @@ # limitations under the License. +from copy import deepcopy +import itertools from uuid import uuid4 import pytest @@ -148,8 +150,14 @@ def test_value_dehydration_should_disallow_object(test_input, expected): dehydrated_value(test_input) +def test_bookmark_is_deprecated(): + with pytest.deprecated_call(): + neo4j.Bookmark() + + def test_bookmark_initialization_with_no_values(): - bookmark = neo4j.api.Bookmark() + with pytest.deprecated_call(): + bookmark = neo4j.Bookmark() assert bookmark.values == frozenset() assert bool(bookmark) is False assert repr(bookmark) == "" @@ -166,7 +174,8 @@ def test_bookmark_initialization_with_no_values(): ] ) def test_bookmark_initialization_with_values_none(test_input, expected_values, expected_bool, expected_repr): - bookmark = neo4j.api.Bookmark(*test_input) + with pytest.deprecated_call(): + bookmark = neo4j.Bookmark(*test_input) assert bookmark.values == expected_values assert bool(bookmark) is expected_bool assert repr(bookmark) == expected_repr @@ -183,7 +192,8 @@ def test_bookmark_initialization_with_values_none(test_input, expected_values, e ] ) def test_bookmark_initialization_with_values_empty_string(test_input, expected_values, expected_bool, expected_repr): - bookmark = neo4j.api.Bookmark(*test_input) + with pytest.deprecated_call(): + bookmark = neo4j.Bookmark(*test_input) assert bookmark.values == expected_values assert bool(bookmark) is expected_bool assert repr(bookmark) == expected_repr @@ -198,7 +208,8 @@ def test_bookmark_initialization_with_values_empty_string(test_input, expected_v ] ) def test_bookmark_initialization_with_valid_strings(test_input, expected_values, expected_bool, expected_repr): - bookmark = neo4j.api.Bookmark(*test_input) + with pytest.deprecated_call(): + bookmark = neo4j.Bookmark(*test_input) assert bookmark.values == expected_values assert bool(bookmark) is expected_bool assert repr(bookmark) == expected_repr @@ -213,8 +224,79 @@ def test_bookmark_initialization_with_valid_strings(test_input, expected_values, ] ) def test_bookmark_initialization_with_invalid_strings(test_input, expected): - with pytest.raises(expected) as e: - bookmark = neo4j.api.Bookmark(*test_input) + with pytest.raises(expected): + neo4j.Bookmark(*test_input) + + +@pytest.mark.parametrize("test_as_generator", [True, False]) +@pytest.mark.parametrize("values", ( + ("bookmark1", "bookmark2", "bookmark3"), + {"bookmark1", "bookmark2", "bookmark3"}, + frozenset(("bookmark1", "bookmark2", "bookmark3")), + ["bookmark1", "bookmark2", "bookmark3"], + ("bookmark1", "bookmark2", "bookmark1"), + ("bookmark1", ""), + ("bookmark1",), + (), + (not_ascii,), +)) +def test_bookmarks_raw_values(test_as_generator, values): + expected = frozenset(values) + if test_as_generator: + values = (v for v in values) + received = neo4j.Bookmarks().from_raw_values(values).raw_values + assert isinstance(received, frozenset) + assert received == expected + + +@pytest.mark.parametrize(("values", "exc_type"), ( + (("bookmark1", None), TypeError), + ((neo4j.Bookmarks(),), TypeError), + (neo4j.Bookmarks(), TypeError), + ((None,), TypeError), + (None, TypeError), + ((False,), TypeError), + (((),), TypeError), + (([],), TypeError), + ((dict(),), TypeError), + ((set(),), TypeError), + ((frozenset(),), TypeError), + ((["bookmark1", "bookmark2"],), TypeError), +)) +def test_bookmarks_invalid_raw_values(values, exc_type): + with pytest.raises(exc_type): + neo4j.Bookmarks().from_raw_values(values) + + +@pytest.mark.parametrize(("values", "expected_repr"), ( + (("bm1", "bm2"), ""), + (("bm2", "bm1"), ""), + (("bm42",), ""), + ((), ""), +)) +def test_bookmarks_repr(values, expected_repr): + bookmarks = neo4j.Bookmarks().from_raw_values(values) + assert repr(bookmarks) == expected_repr + + +@pytest.mark.parametrize(("values1", "values2"), ( + (values + for values in itertools.combinations_with_replacement( + ( + ("bookmark1",), + ("bookmark1", "bookmark2"), + ("bookmark3",), + (), + ), + 2 + )) +)) +def test_bookmarks_combination(values1, values2): + bookmarks1 = neo4j.Bookmarks().from_raw_values(values1) + bookmarks2 = neo4j.Bookmarks().from_raw_values(values2) + bookmarks3 = bookmarks1 + bookmarks2 + assert bookmarks3.raw_values == (bookmarks2 + bookmarks1).raw_values + assert bookmarks3.raw_values == frozenset(values1) | frozenset(values2) @pytest.mark.parametrize( @@ -231,7 +313,7 @@ def test_bookmark_initialization_with_invalid_strings(test_input, expected): ] ) def test_version_initialization(test_input, expected_str, expected_repr): - version = neo4j.api.Version(*test_input) + version = neo4j.Version(*test_input) assert str(version) == expected_str assert repr(version) == expected_repr @@ -247,7 +329,7 @@ def test_version_initialization(test_input, expected_str, expected_repr): ] ) def test_version_from_bytes_with_valid_bolt_version_handshake(test_input, expected_str, expected_repr): - version = neo4j.api.Version.from_bytes(test_input) + version = neo4j.Version.from_bytes(test_input) assert str(version) == expected_str assert repr(version) == expected_repr @@ -264,7 +346,7 @@ def test_version_from_bytes_with_valid_bolt_version_handshake(test_input, expect ) def test_version_from_bytes_with_not_valid_bolt_version_handshake(test_input, expected): with pytest.raises(expected): - version = neo4j.api.Version.from_bytes(test_input) + version = neo4j.Version.from_bytes(test_input) @pytest.mark.parametrize( @@ -280,7 +362,7 @@ def test_version_from_bytes_with_not_valid_bolt_version_handshake(test_input, ex ] ) def test_version_to_bytes_with_valid_bolt_version(test_input, expected): - version = neo4j.api.Version(*test_input) + version = neo4j.Version(*test_input) assert version.to_bytes() == expected @@ -289,9 +371,9 @@ def test_serverinfo_initialization(): from neo4j.addressing import Address address = Address(("bolt://localhost", 7687)) - version = neo4j.api.Version(3, 0) + version = neo4j.Version(3, 0) - server_info = neo4j.api.ServerInfo(address, version) + server_info = neo4j.ServerInfo(address, version) assert server_info.address is address assert server_info.protocol_version is version assert server_info.agent is None @@ -312,9 +394,9 @@ def test_serverinfo_with_metadata(test_input, expected_agent, from neo4j.addressing import Address address = Address(("bolt://localhost", 7687)) - version = neo4j.api.Version(*protocol_version) + version = neo4j.Version(*protocol_version) - server_info = neo4j.api.ServerInfo(address, version) + server_info = neo4j.ServerInfo(address, version) server_info.update(test_input) diff --git a/tests/unit/sync/work/test_session.py b/tests/unit/sync/work/test_session.py index d7b63be20..a9a9e0a43 100644 --- a/tests/unit/sync/work/test_session.py +++ b/tests/unit/sync/work/test_session.py @@ -21,6 +21,7 @@ import pytest from neo4j import ( + Bookmarks, Session, SessionConfig, Transaction, @@ -170,20 +171,25 @@ def test_closes_connection_after_tx_commit(pool, test_run_args): @pytest.mark.parametrize( - "bookmarks", + "bookmark_values", (None, [], ["abc"], ["foo", "bar"], {"a", "b"}, ("1", "two")) ) @mark_sync_test -def test_session_returns_bookmark_directly(pool, bookmarks): +def test_session_returns_bookmarks_directly(pool, bookmark_values): + if bookmark_values is not None: + bookmarks = Bookmarks.from_raw_values(bookmark_values) + else: + bookmarks = Bookmarks() with Session( pool, SessionConfig(bookmarks=bookmarks) ) as session: - if bookmarks is None: - assert session.last_bookmarks() == [] - elif isinstance(bookmarks, set): - assert sorted(session.last_bookmarks()) == sorted(bookmarks) + ret_bookmarks = (session.last_bookmarks()) + assert isinstance(ret_bookmarks, Bookmarks) + ret_bookmarks = ret_bookmarks.raw_values + if bookmark_values is None: + assert ret_bookmarks == frozenset() else: - assert session.last_bookmarks() == list(bookmarks) + assert ret_bookmarks == frozenset(bookmark_values) @pytest.mark.parametrize(("query", "error_type"), ( From 1bad6342329ce3971675638956d247f4c72ee041 Mon Sep 17 00:00:00 2001 From: Rouven Bauer Date: Tue, 1 Feb 2022 11:22:13 +0100 Subject: [PATCH 3/4] Update docs --- docs/source/api.rst | 11 ++++++-- neo4j/_async_compat/network/__init__.py | 4 +-- .../{bolt_socket.py => _bolt_socket.py} | 2 +- .../network/{util.py => _util.py} | 0 neo4j/_async_compat/util.py | 6 +++++ neo4j/meta.py | 25 +++++++++++++------ 6 files changed, 36 insertions(+), 12 deletions(-) rename neo4j/_async_compat/network/{bolt_socket.py => _bolt_socket.py} (99%) rename neo4j/_async_compat/network/{util.py => _util.py} (100%) diff --git a/docs/source/api.rst b/docs/source/api.rst index c146f35cb..be219ff26 100644 --- a/docs/source/api.rst +++ b/docs/source/api.rst @@ -483,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. -:Default: ``()`` +.. 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: ``None`` .. _database-ref: diff --git a/neo4j/_async_compat/network/__init__.py b/neo4j/_async_compat/network/__init__.py index c2611569c..a66d86338 100644 --- a/neo4j/_async_compat/network/__init__.py +++ b/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, ) diff --git a/neo4j/_async_compat/network/bolt_socket.py b/neo4j/_async_compat/network/_bolt_socket.py similarity index 99% rename from neo4j/_async_compat/network/bolt_socket.py rename to neo4j/_async_compat/network/_bolt_socket.py index 1af0793a5..7759e5dbe 100644 --- a/neo4j/_async_compat/network/bolt_socket.py +++ b/neo4j/_async_compat/network/_bolt_socket.py @@ -46,7 +46,7 @@ DriverError, ServiceUnavailable, ) -from .util import ( +from ._util import ( AsyncNetworkUtil, NetworkUtil, ) diff --git a/neo4j/_async_compat/network/util.py b/neo4j/_async_compat/network/_util.py similarity index 100% rename from neo4j/_async_compat/network/util.py rename to neo4j/_async_compat/network/_util.py diff --git a/neo4j/_async_compat/util.py b/neo4j/_async_compat/util.py index 891475d13..b00d010bd 100644 --- a/neo4j/_async_compat/util.py +++ b/neo4j/_async_compat/util.py @@ -21,6 +21,12 @@ from ..meta import experimental +__all__ = [ + "AsyncUtil", + "Util", +] + + class AsyncUtil: @staticmethod async def iter(it): diff --git a/neo4j/meta.py b/neo4j/meta.py index 37d8f8d0e..e3e827fe6 100644 --- a/neo4j/meta.py +++ b/neo4j/meta.py @@ -16,6 +16,7 @@ # limitations under the License. +import asyncio from functools import wraps @@ -52,13 +53,23 @@ def foo(x): pass """ - def f__(f): - @wraps(f) - def f_(*args, **kwargs): - deprecation_warn(message) - return f(*args, **kwargs) - return f_ - return f__ + def decorator(f): + if asyncio.iscoroutinefunction(f): + @wraps(f) + async def inner(*args, **kwargs): + deprecation_warn(message) + return await f(*args, **kwargs) + + return inner + else: + @wraps(f) + def inner(*args, **kwargs): + deprecation_warn(message) + return f(*args, **kwargs) + + return inner + + return decorator class ExperimentalWarning(Warning): From 14613a332da572d0570bbd08b0ce82bd7debe126 Mon Sep 17 00:00:00 2001 From: Rouven Bauer Date: Tue, 1 Feb 2022 12:16:01 +0100 Subject: [PATCH 4/4] Add unit tests for deprecated bookmarks APIs --- neo4j/_async/work/session.py | 11 ++++++---- neo4j/_sync/work/session.py | 11 ++++++---- neo4j/meta.py | 8 +++---- tests/unit/async_/work/test_session.py | 30 ++++++++++++++++++++++++++ tests/unit/sync/work/test_session.py | 30 ++++++++++++++++++++++++++ 5 files changed, 78 insertions(+), 12 deletions(-) diff --git a/neo4j/_async/work/session.py b/neo4j/_async/work/session.py index 8ff2c5821..5f608e574 100644 --- a/neo4j/_async/work/session.py +++ b/neo4j/_async/work/session.py @@ -109,14 +109,17 @@ async def __aexit__(self, exception_type, exception_value, traceback): 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.") + 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).") diff --git a/neo4j/_sync/work/session.py b/neo4j/_sync/work/session.py index d05f7596e..6ef573fb5 100644 --- a/neo4j/_sync/work/session.py +++ b/neo4j/_sync/work/session.py @@ -109,14 +109,17 @@ def __exit__(self, exception_type, exception_value, traceback): 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.") + 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).") diff --git a/neo4j/meta.py b/neo4j/meta.py index e3e827fe6..6c06c6a8b 100644 --- a/neo4j/meta.py +++ b/neo4j/meta.py @@ -38,9 +38,9 @@ def get_user_agent(): return template.format(*fields) -def deprecation_warn(message): +def deprecation_warn(message, stack_level=2): from warnings import warn - warn(message, category=DeprecationWarning, stacklevel=2) + warn(message, category=DeprecationWarning, stacklevel=stack_level) def deprecated(message): @@ -57,14 +57,14 @@ def decorator(f): if asyncio.iscoroutinefunction(f): @wraps(f) async def inner(*args, **kwargs): - deprecation_warn(message) + deprecation_warn(message, stack_level=3) return await f(*args, **kwargs) return inner else: @wraps(f) def inner(*args, **kwargs): - deprecation_warn(message) + deprecation_warn(message, stack_level=3) return f(*args, **kwargs) return inner diff --git a/tests/unit/async_/work/test_session.py b/tests/unit/async_/work/test_session.py index 7130c30dc..37f8a22b2 100644 --- a/tests/unit/async_/work/test_session.py +++ b/tests/unit/async_/work/test_session.py @@ -192,6 +192,36 @@ async def test_session_returns_bookmarks_directly(pool, bookmark_values): assert ret_bookmarks == frozenset(bookmark_values) +@pytest.mark.parametrize( + "bookmarks", + (None, [], ["abc"], ["foo", "bar"], ("1", "two")) +) +@mark_async_test +async def test_session_last_bookmark_is_deprecated(pool, bookmarks): + async with AsyncSession(pool, SessionConfig( + bookmarks=bookmarks + )) as session: + with pytest.warns(DeprecationWarning): + if bookmarks: + assert (await session.last_bookmark()) == bookmarks[-1] + else: + assert (await session.last_bookmark()) is None + + +@pytest.mark.parametrize( + "bookmarks", + (("foo",), ("foo", "bar"), (), ["foo", "bar"], {"a", "b"}) +) +@mark_async_test +async def test_session_bookmarks_as_iterable_is_deprecated(pool, bookmarks): + with pytest.warns(DeprecationWarning): + async with AsyncSession(pool, SessionConfig( + bookmarks=bookmarks + )) as session: + ret_bookmarks = (await session.last_bookmarks()).raw_values + assert ret_bookmarks == frozenset(bookmarks) + + @pytest.mark.parametrize(("query", "error_type"), ( (None, ValueError), (1234, TypeError), diff --git a/tests/unit/sync/work/test_session.py b/tests/unit/sync/work/test_session.py index a9a9e0a43..727f3330f 100644 --- a/tests/unit/sync/work/test_session.py +++ b/tests/unit/sync/work/test_session.py @@ -192,6 +192,36 @@ def test_session_returns_bookmarks_directly(pool, bookmark_values): assert ret_bookmarks == frozenset(bookmark_values) +@pytest.mark.parametrize( + "bookmarks", + (None, [], ["abc"], ["foo", "bar"], ("1", "two")) +) +@mark_sync_test +def test_session_last_bookmark_is_deprecated(pool, bookmarks): + with Session(pool, SessionConfig( + bookmarks=bookmarks + )) as session: + with pytest.warns(DeprecationWarning): + if bookmarks: + assert (session.last_bookmark()) == bookmarks[-1] + else: + assert (session.last_bookmark()) is None + + +@pytest.mark.parametrize( + "bookmarks", + (("foo",), ("foo", "bar"), (), ["foo", "bar"], {"a", "b"}) +) +@mark_sync_test +def test_session_bookmarks_as_iterable_is_deprecated(pool, bookmarks): + with pytest.warns(DeprecationWarning): + with Session(pool, SessionConfig( + bookmarks=bookmarks + )) as session: + ret_bookmarks = (session.last_bookmarks()).raw_values + assert ret_bookmarks == frozenset(bookmarks) + + @pytest.mark.parametrize(("query", "error_type"), ( (None, ValueError), (1234, TypeError),