From 77b76e1be313a8475784563c3827ea4bfbab2f11 Mon Sep 17 00:00:00 2001 From: lutovich Date: Thu, 5 Jul 2018 19:39:07 +0200 Subject: [PATCH] Fix bookmark comparison Bookmarks need to be parsed and compared in order to send the most up-to-date bookmark to an old-ish Neo4j version. Previous versions do not support multiple bookmarks in metadata. They only support a single one. This commit fixes a problem with bookmark parsing for further comparison. Bookmark format consists of a fixed prefix and an int identifier. Previous parser assumed that they are colon-separated. --- neo4j/v1/api.py | 28 +++++++++++-------- test/stub/scripts/bookmark_chain.script | 8 +++--- .../bookmark_chain_with_autocommit.script | 10 +++---- test/stub/scripts/return_1_in_tx.script | 2 +- test/stub/scripts/return_1_in_tx_twice.script | 6 ++-- test/stub/scripts/return_1_twice_in_tx.script | 2 +- test/stub/scripts/return_2_in_tx.script | 4 +-- test/stub/test_bookmarking.py | 18 ++++++------ 8 files changed, 42 insertions(+), 36 deletions(-) diff --git a/neo4j/v1/api.py b/neo4j/v1/api.py index 247a5ef3..39fbd957 100644 --- a/neo4j/v1/api.py +++ b/neo4j/v1/api.py @@ -43,20 +43,26 @@ RETRY_DELAY_MULTIPLIER = 2.0 RETRY_DELAY_JITTER_FACTOR = 0.2 +BOOKMARK_PREFIX = "neo4j:bookmark:v1:tx" + def last_bookmark(b0, b1): - """ Return the latest of two bookmarks by looking for the maximum - integer value following the last colon in the bookmark string. + """ Return the latest of two bookmarks. """ - n = [None, None] - _, _, n[0] = b0.rpartition(":") - _, _, n[1] = b1.rpartition(":") - for i in range(2): - try: - n[i] = int(n[i]) - except ValueError: - raise ValueError("Invalid bookmark: {}".format(b0)) - return b0 if n[0] > n[1] else b1 + return b0 if _bookmark_value(b0) > _bookmark_value(b1) else b1 + + +def _bookmark_value(b): + """Return the int value of the given bookmark. + """ + if b is None or not b.startswith(BOOKMARK_PREFIX): + raise ValueError("Invalid bookmark: {}".format(b)) + + value_string = b[len(BOOKMARK_PREFIX):] + try: + return int(value_string) + except ValueError: + raise ValueError("Invalid bookmark: {}".format(b)) def retry_delay_generator(initial_delay, multiplier, jitter_factor): diff --git a/test/stub/scripts/bookmark_chain.script b/test/stub/scripts/bookmark_chain.script index cb96bbbc..17c09a76 100644 --- a/test/stub/scripts/bookmark_chain.script +++ b/test/stub/scripts/bookmark_chain.script @@ -1,20 +1,20 @@ !: AUTO INIT !: AUTO RESET -C: RUN "BEGIN" {"bookmark": "bookmark:1", "bookmarks": ["bookmark:0", "bookmark:1"]} +C: RUN "BEGIN" {"bookmark": "neo4j:bookmark:v1:tx1", "bookmarks": ["neo4j:bookmark:v1:tx0", "neo4j:bookmark:v1:tx1"]} PULL_ALL S: SUCCESS {} SUCCESS {} C: RUN "COMMIT" {} PULL_ALL -S: SUCCESS {"bookmark": "bookmark:2", "bookmarks": ["bookmark:2"]} +S: SUCCESS {"bookmark": "neo4j:bookmark:v1:tx2", "bookmarks": ["neo4j:bookmark:v1:tx2"]} SUCCESS {} -C: RUN "BEGIN" {"bookmark": "bookmark:2", "bookmarks": ["bookmark:2"]} +C: RUN "BEGIN" {"bookmark": "neo4j:bookmark:v1:tx2", "bookmarks": ["neo4j:bookmark:v1:tx2"]} PULL_ALL S: SUCCESS {} SUCCESS {} C: RUN "COMMIT" {} PULL_ALL -S: SUCCESS {"bookmark": "bookmark:3", "bookmarks": ["bookmark:3"]} +S: SUCCESS {"bookmark": "neo4j:bookmark:v1:tx3", "bookmarks": ["neo4j:bookmark:v1:tx3"]} SUCCESS {} diff --git a/test/stub/scripts/bookmark_chain_with_autocommit.script b/test/stub/scripts/bookmark_chain_with_autocommit.script index d06d3c7d..ed60831a 100644 --- a/test/stub/scripts/bookmark_chain_with_autocommit.script +++ b/test/stub/scripts/bookmark_chain_with_autocommit.script @@ -1,25 +1,25 @@ !: AUTO INIT !: AUTO RESET -C: RUN "BEGIN" {"bookmark": "bookmark:1", "bookmarks": ["bookmark:1"]} +C: RUN "BEGIN" {"bookmark": "neo4j:bookmark:v1:tx1", "bookmarks": ["neo4j:bookmark:v1:tx1"]} PULL_ALL S: SUCCESS {} SUCCESS {} C: RUN "COMMIT" {} PULL_ALL -S: SUCCESS {"bookmark": "bookmark:2", "bookmarks": ["bookmark:2"]} +S: SUCCESS {"bookmark": "neo4j:bookmark:v1:tx2", "bookmarks": ["neo4j:bookmark:v1:tx2"]} SUCCESS {} C: RUN "RETURN 1" {} PULL_ALL -S: SUCCESS {"bookmark": "bookmark:x", "bookmarks": ["bookmark:x"]} +S: SUCCESS {"bookmark": "neo4j:bookmark:v1:tx42", "bookmarks": ["neo4j:bookmark:v1:tx42"]} SUCCESS {} -C: RUN "BEGIN" {"bookmark": "bookmark:2", "bookmarks": ["bookmark:2"]} +C: RUN "BEGIN" {"bookmark": "neo4j:bookmark:v1:tx2", "bookmarks": ["neo4j:bookmark:v1:tx2"]} PULL_ALL S: SUCCESS {} SUCCESS {} C: RUN "COMMIT" {} PULL_ALL -S: SUCCESS {"bookmark": "bookmark:3", "bookmarks": ["bookmark:3"]} +S: SUCCESS {"bookmark": "neo4j:bookmark:v1:tx3", "bookmarks": ["neo4j:bookmark:v1:tx3"]} SUCCESS {} diff --git a/test/stub/scripts/return_1_in_tx.script b/test/stub/scripts/return_1_in_tx.script index 6af15609..664fcabc 100644 --- a/test/stub/scripts/return_1_in_tx.script +++ b/test/stub/scripts/return_1_in_tx.script @@ -14,5 +14,5 @@ S: SUCCESS {"fields": ["1"]} C: RUN "COMMIT" {} PULL_ALL -S: SUCCESS {"bookmark": "bookmark:1", "bookmarks": ["bookmark:1"]} +S: SUCCESS {"bookmark": "neo4j:bookmark:v1:tx1", "bookmarks": ["neo4j:bookmark:v1:tx1"]} SUCCESS {} diff --git a/test/stub/scripts/return_1_in_tx_twice.script b/test/stub/scripts/return_1_in_tx_twice.script index a19d66aa..766802d7 100644 --- a/test/stub/scripts/return_1_in_tx_twice.script +++ b/test/stub/scripts/return_1_in_tx_twice.script @@ -14,10 +14,10 @@ S: SUCCESS {"fields": ["1"]} C: RUN "COMMIT" {} PULL_ALL -S: SUCCESS {"bookmark": "bookmark:1", "bookmarks": ["bookmark:1"]} +S: SUCCESS {"bookmark": "neo4j:bookmark:v1:tx1", "bookmarks": ["neo4j:bookmark:v1:tx1"]} SUCCESS {} -C: RUN "BEGIN" {"bookmark": "bookmark:1", "bookmarks": ["bookmark:1"]} +C: RUN "BEGIN" {"bookmark": "neo4j:bookmark:v1:tx1", "bookmarks": ["neo4j:bookmark:v1:tx1"]} PULL_ALL S: SUCCESS {"fields": []} SUCCESS {} @@ -30,5 +30,5 @@ S: SUCCESS {"fields": ["1"]} C: RUN "COMMIT" {} PULL_ALL -S: SUCCESS {"bookmark": "bookmark:2", "bookmarks": ["bookmark:2"]} +S: SUCCESS {"bookmark": "neo4j:bookmark:v1:tx2", "bookmarks": ["neo4j:bookmark:v1:tx2"]} SUCCESS {} diff --git a/test/stub/scripts/return_1_twice_in_tx.script b/test/stub/scripts/return_1_twice_in_tx.script index c7f1b050..1084b64c 100644 --- a/test/stub/scripts/return_1_twice_in_tx.script +++ b/test/stub/scripts/return_1_twice_in_tx.script @@ -20,5 +20,5 @@ S: SUCCESS {"fields": ["x"]} C: RUN "COMMIT" {} PULL_ALL -S: SUCCESS {"bookmark": "bookmark:1", "bookmarks": ["bookmark:1"]} +S: SUCCESS {"bookmark": "neo4j:bookmark:v1:tx1", "bookmarks": ["neo4j:bookmark:v1:tx1"]} SUCCESS {} diff --git a/test/stub/scripts/return_2_in_tx.script b/test/stub/scripts/return_2_in_tx.script index d05c5706..dd838dfe 100644 --- a/test/stub/scripts/return_2_in_tx.script +++ b/test/stub/scripts/return_2_in_tx.script @@ -1,7 +1,7 @@ !: AUTO INIT !: AUTO RESET -C: RUN "BEGIN" {"bookmark": "bookmark:1", "bookmarks": ["bookmark:1"]} +C: RUN "BEGIN" {"bookmark": "neo4j:bookmark:v1:tx1", "bookmarks": ["neo4j:bookmark:v1:tx1"]} PULL_ALL S: SUCCESS {"fields": []} SUCCESS {} @@ -14,5 +14,5 @@ S: SUCCESS {"fields": ["2"]} C: RUN "COMMIT" {} PULL_ALL -S: SUCCESS {"bookmark": "bookmark:2", "bookmarks": ["bookmark:2"]} +S: SUCCESS {"bookmark": "neo4j:bookmark:v1:tx2", "bookmarks": ["neo4j:bookmark:v1:tx2"]} SUCCESS {} diff --git a/test/stub/test_bookmarking.py b/test/stub/test_bookmarking.py index 4c8e5871..5dc6a7b6 100644 --- a/test/stub/test_bookmarking.py +++ b/test/stub/test_bookmarking.py @@ -44,31 +44,31 @@ def test_should_be_able_to_set_multiple_bookmarks(self): with StubCluster({9001: "router.script"}): uri = "bolt+routing://localhost:9001" with GraphDatabase.driver(uri, auth=self.auth_token, encrypted=False) as driver: - with driver.session(bookmarks=[":1", ":2"]) as session: - assert session.last_bookmark() == ":2" + with driver.session(bookmarks=["neo4j:bookmark:v1:tx1", "neo4j:bookmark:v1:tx2"]) as session: + assert session.last_bookmark() == "neo4j:bookmark:v1:tx2" def test_should_automatically_chain_bookmarks(self): with StubCluster({9001: "router.script", 9004: "bookmark_chain.script"}): uri = "bolt+routing://localhost:9001" with GraphDatabase.driver(uri, auth=self.auth_token, encrypted=False) as driver: - with driver.session(access_mode=READ_ACCESS, bookmarks=["bookmark:0", "bookmark:1"]) as session: + with driver.session(access_mode=READ_ACCESS, bookmarks=["neo4j:bookmark:v1:tx0", "neo4j:bookmark:v1:tx1"]) as session: with session.begin_transaction(): pass - assert session.last_bookmark() == "bookmark:2" + assert session.last_bookmark() == "neo4j:bookmark:v1:tx2" with session.begin_transaction(): pass - assert session.last_bookmark() == "bookmark:3" + assert session.last_bookmark() == "neo4j:bookmark:v1:tx3" def test_autocommit_transaction_does_not_break_chain(self): with StubCluster({9001: "router.script", 9004: "bookmark_chain_with_autocommit.script"}): uri = "bolt+routing://localhost:9001" with GraphDatabase.driver(uri, auth=self.auth_token, encrypted=False) as driver: - with driver.session(access_mode=READ_ACCESS, bookmark="bookmark:1") as session: + with driver.session(access_mode=READ_ACCESS, bookmark="neo4j:bookmark:v1:tx1") as session: with session.begin_transaction(): pass - assert session.last_bookmark() == "bookmark:2" + assert session.last_bookmark() == "neo4j:bookmark:v1:tx2" session.run("RETURN 1").consume() - assert session.last_bookmark() == "bookmark:2" + assert session.last_bookmark() == "neo4j:bookmark:v1:tx2" with session.begin_transaction(): pass - assert session.last_bookmark() == "bookmark:3" + assert session.last_bookmark() == "neo4j:bookmark:v1:tx3"