From f186165f80ec0dfba3b209b3b7556f0e649924db Mon Sep 17 00:00:00 2001 From: lutovich Date: Mon, 22 May 2017 15:56:43 +0200 Subject: [PATCH 1/2] Multiple bookmarks Previously it was possible to only supply a single bookmark when creating a new session. However there is a use-case to be able to supply multiple bookmarks when multiple worker threads execute write queries and then reader should be able to observe all those writes. To achieve this driver now exposed session creation methods that take an iterable of bookmarks. It was chosen to add additional `Driver#session()` overloads with `Iterable` instead of extending single-bookmark methods with varargs because: * this maintains backwards compatibility for pre-existing methods * addition of varargs would make API look strange because call or `Driver#session()` and call of `Driver#session(String... bookmarks)` without params is essentially the same thing * invocation of vararg methods with single `null` argument can be confusing * varargs are more suitable for manual params listing Driver will now send: ``` { bookmark: "max", bookmarks: ["one", "two", "max"] } ``` instead of simple: ``` { bookmark: "max" } ``` this is done to maintain backwards compatibility with databases that only support single bookmark. It forces driver to parse and compare bookmarks which violates the fact that bookmarks are opaque. This is done only to maintain backwards compatibility and should not be copied. Code doing this will eventually be removed. --- .../org/neo4j/driver/internal/Bookmark.java | 154 ++++++++++++++++++ .../driver/internal/BookmarkCollector.java | 2 +- .../driver/internal/ExplicitTransaction.java | 34 ++-- .../neo4j/driver/internal/InternalDriver.java | 20 ++- .../internal/InternalStatementResult.java | 2 +- .../neo4j/driver/internal/NetworkSession.java | 13 +- .../neo4j/driver/internal/SessionFactory.java | 2 +- .../driver/internal/SessionFactoryImpl.java | 32 ++-- .../internal/net/SocketResponseHandler.java | 5 +- .../neo4j/driver/internal/spi/Collector.java | 7 +- .../internal/summary/SummaryBuilder.java | 3 +- .../main/java/org/neo4j/driver/v1/Driver.java | 32 +++- .../java/org/neo4j/driver/v1/Session.java | 4 +- .../neo4j/driver/internal/BookmarkTest.java | 139 ++++++++++++++++ .../driver/internal/DirectDriverTest.java | 34 +++- .../internal/ExplicitTransactionTest.java | 41 ++++- .../driver/internal/NetworkSessionTest.java | 68 +++++--- .../internal/RoutingDriverBoltKitTest.java | 29 ++++ .../driver/internal/RoutingDriverTest.java | 9 +- .../test/resources/multiple_bookmarks.script | 16 ++ .../resources/read_tx_with_bookmarks.script | 2 +- .../write_read_tx_with_bookmarks.script | 4 +- .../resources/write_tx_with_bookmarks.script | 2 +- 23 files changed, 563 insertions(+), 91 deletions(-) create mode 100644 driver/src/main/java/org/neo4j/driver/internal/Bookmark.java create mode 100644 driver/src/test/java/org/neo4j/driver/internal/BookmarkTest.java create mode 100644 driver/src/test/resources/multiple_bookmarks.script diff --git a/driver/src/main/java/org/neo4j/driver/internal/Bookmark.java b/driver/src/main/java/org/neo4j/driver/internal/Bookmark.java new file mode 100644 index 0000000000..af5d71e5ff --- /dev/null +++ b/driver/src/main/java/org/neo4j/driver/internal/Bookmark.java @@ -0,0 +1,154 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.driver.internal; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Iterator; +import java.util.Map; + +import org.neo4j.driver.v1.Value; + +import static java.util.Collections.emptyMap; +import static java.util.Collections.singleton; +import static org.neo4j.driver.v1.Values.value; + +public final class Bookmark +{ + private static final String BOOKMARK_KEY = "bookmark"; + private static final String BOOKMARKS_KEY = "bookmarks"; + private static final String BOOKMARK_PREFIX = "neo4j:bookmark:v1:tx"; + + private static final long UNKNOWN_BOOKMARK_VALUE = -1; + + private static final Bookmark EMPTY = new Bookmark( Collections.emptySet() ); + + private final Iterable values; + private final String maxValue; + + private Bookmark( Iterable values ) + { + this.values = values; + this.maxValue = maxBookmark( values ); + } + + public static Bookmark empty() + { + return EMPTY; + } + + public static Bookmark from( String value ) + { + if ( value == null ) + { + return empty(); + } + return from( singleton( value ) ); + } + + public static Bookmark from( Iterable values ) + { + if ( values == null ) + { + return empty(); + } + return new Bookmark( values ); + } + + public boolean isEmpty() + { + return maxValue == null; + } + + public String asString() + { + return maxValue; + } + + public Map asParameters() + { + if ( isEmpty() ) + { + return emptyMap(); + } + + // Driver sends {bookmark: "max", bookmarks: ["one", "two", "max"]} instead of simple + // {bookmarks: ["one", "two", "max"]} for backwards compatibility reasons. Old servers can only accept single + // bookmark that is why driver has to parse and compare given list of bookmarks. This functionality will + // eventually be removed. + Map parameters = new HashMap<>( 4 ); + parameters.put( BOOKMARK_KEY, value( maxValue ) ); + parameters.put( BOOKMARKS_KEY, value( values ) ); + return parameters; + } + + @Override + public String toString() + { + return "Bookmark{values=" + values + "}"; + } + + private static String maxBookmark( Iterable bookmarks ) + { + if ( bookmarks == null ) + { + return null; + } + + Iterator iterator = bookmarks.iterator(); + + if ( !iterator.hasNext() ) + { + return null; + } + + String maxBookmark = iterator.next(); + long maxValue = bookmarkValue( maxBookmark ); + + while ( iterator.hasNext() ) + { + String bookmark = iterator.next(); + long value = bookmarkValue( bookmark ); + + if ( value > maxValue ) + { + maxBookmark = bookmark; + maxValue = value; + } + } + + return maxBookmark; + } + + private static long bookmarkValue( String value ) + { + if ( value.startsWith( BOOKMARK_PREFIX ) ) + { + try + { + return Long.parseLong( value.substring( BOOKMARK_PREFIX.length() ) ); + } + catch ( NumberFormatException e ) + { + return UNKNOWN_BOOKMARK_VALUE; + } + } + return UNKNOWN_BOOKMARK_VALUE; + } +} diff --git a/driver/src/main/java/org/neo4j/driver/internal/BookmarkCollector.java b/driver/src/main/java/org/neo4j/driver/internal/BookmarkCollector.java index adb0aa2f48..ae8529f247 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/BookmarkCollector.java +++ b/driver/src/main/java/org/neo4j/driver/internal/BookmarkCollector.java @@ -30,7 +30,7 @@ class BookmarkCollector extends NoOperationCollector } @Override - public void bookmark( String bookmark ) + public void bookmark( Bookmark bookmark ) { transaction.setBookmark( bookmark ); } diff --git a/driver/src/main/java/org/neo4j/driver/internal/ExplicitTransaction.java b/driver/src/main/java/org/neo4j/driver/internal/ExplicitTransaction.java index 12f33c3cce..ec01691821 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/ExplicitTransaction.java +++ b/driver/src/main/java/org/neo4j/driver/internal/ExplicitTransaction.java @@ -34,8 +34,6 @@ import org.neo4j.driver.v1.exceptions.Neo4jException; import org.neo4j.driver.v1.types.TypeSystem; -import static java.util.Collections.emptyMap; -import static java.util.Collections.singletonMap; import static org.neo4j.driver.v1.Values.ofValue; import static org.neo4j.driver.v1.Values.value; @@ -68,19 +66,19 @@ private enum State private final SessionResourcesHandler resourcesHandler; private final Connection conn; - private String bookmark = null; + private Bookmark bookmark = Bookmark.empty(); private State state = State.ACTIVE; public ExplicitTransaction( Connection conn, SessionResourcesHandler resourcesHandler ) { - this( conn, resourcesHandler, null ); + this( conn, resourcesHandler, Bookmark.empty() ); } - ExplicitTransaction( Connection conn, SessionResourcesHandler resourcesHandler, String bookmark ) + ExplicitTransaction( Connection conn, SessionResourcesHandler resourcesHandler, Bookmark initialBookmark ) { this.conn = conn; this.resourcesHandler = resourcesHandler; - runBeginStatement( conn, bookmark ); + runBeginStatement( conn, initialBookmark ); } @Override @@ -238,32 +236,28 @@ public synchronized void markToClose() state = State.FAILED; } - public String bookmark() + public Bookmark bookmark() { return bookmark; } - void setBookmark( String bookmark ) + void setBookmark( Bookmark bookmark ) { - this.bookmark = bookmark; + if ( bookmark != null && !bookmark.isEmpty() ) + { + this.bookmark = bookmark; + } } - private static void runBeginStatement( Connection connection, String bookmark ) + private static void runBeginStatement( Connection connection, Bookmark bookmark ) { - Map parameters; - if ( bookmark != null ) - { - parameters = singletonMap( "bookmark", value( bookmark ) ); - } - else - { - parameters = emptyMap(); - } + Bookmark initialBookmark = bookmark == null ? Bookmark.empty() : bookmark; + Map parameters = initialBookmark.asParameters(); connection.run( "BEGIN", parameters, Collector.NO_OP ); connection.pullAll( Collector.NO_OP ); - if ( bookmark != null ) + if ( !initialBookmark.isEmpty() ) { connection.sync(); } diff --git a/driver/src/main/java/org/neo4j/driver/internal/InternalDriver.java b/driver/src/main/java/org/neo4j/driver/internal/InternalDriver.java index 094ff8ec19..97474bd529 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/InternalDriver.java +++ b/driver/src/main/java/org/neo4j/driver/internal/InternalDriver.java @@ -18,6 +18,7 @@ */ package org.neo4j.driver.internal; +import java.util.Collection; import java.util.concurrent.atomic.AtomicBoolean; import org.neo4j.driver.internal.security.SecurityPlan; @@ -62,7 +63,7 @@ public final Session session() @Override public final Session session( AccessMode mode ) { - return session( mode, null ); + return newSession( mode, Bookmark.empty() ); } @Override @@ -73,6 +74,23 @@ public final Session session( String bookmark ) @Override public final Session session( AccessMode mode, String bookmark ) + { + return newSession( mode, Bookmark.from( bookmark ) ); + } + + @Override + public Session session( Iterable bookmarks ) + { + return session( AccessMode.WRITE, bookmarks ); + } + + @Override + public Session session( AccessMode mode, Iterable bookmarks ) + { + return newSession( mode, Bookmark.from( bookmarks ) ); + } + + private Session newSession( AccessMode mode, Bookmark bookmark ) { assertOpen(); Session session = sessionFactory.newInstance( mode, bookmark ); diff --git a/driver/src/main/java/org/neo4j/driver/internal/InternalStatementResult.java b/driver/src/main/java/org/neo4j/driver/internal/InternalStatementResult.java index c6701209bc..23f2c785d5 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/InternalStatementResult.java +++ b/driver/src/main/java/org/neo4j/driver/internal/InternalStatementResult.java @@ -138,7 +138,7 @@ public void notifications( List notifications ) } @Override - public void bookmark( String bookmark ) + public void bookmark( Bookmark bookmark ) { if ( transaction != null ) { diff --git a/driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java b/driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java index 43607c1b49..0e38bb6c7d 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java +++ b/driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java @@ -53,7 +53,7 @@ public class NetworkSession implements Session, SessionResourcesHandler private final RetryLogic retryLogic; protected final Logger logger; - private String bookmark; + private Bookmark bookmark = Bookmark.empty(); private PooledConnection currentConnection; private ExplicitTransaction currentTransaction; @@ -179,7 +179,7 @@ public synchronized Transaction beginTransaction() @Override public synchronized Transaction beginTransaction( String bookmark ) { - setBookmark( bookmark ); + setBookmark( Bookmark.from( bookmark ) ); return beginTransaction(); } @@ -195,12 +195,9 @@ public T writeTransaction( TransactionWork work ) return transaction( AccessMode.WRITE, work ); } - // Internal method for setting the bookmark explicitly, mainly for testing. - // This method does not prevent setting the bookmark to null since that - // is a valid requirement for some test scenarios. - void setBookmark( String bookmark ) + void setBookmark( Bookmark bookmark ) { - if( bookmark != null ) + if ( bookmark != null && !bookmark.isEmpty() ) { this.bookmark = bookmark; } @@ -209,7 +206,7 @@ void setBookmark( String bookmark ) @Override public String lastBookmark() { - return bookmark; + return bookmark == null ? null : bookmark.asString(); } @Override diff --git a/driver/src/main/java/org/neo4j/driver/internal/SessionFactory.java b/driver/src/main/java/org/neo4j/driver/internal/SessionFactory.java index c4b2e6c6b5..fefc799536 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/SessionFactory.java +++ b/driver/src/main/java/org/neo4j/driver/internal/SessionFactory.java @@ -23,5 +23,5 @@ public interface SessionFactory extends AutoCloseable { - Session newInstance( AccessMode mode, String bookmark ); + Session newInstance( AccessMode mode, Bookmark bookmark ); } diff --git a/driver/src/main/java/org/neo4j/driver/internal/SessionFactoryImpl.java b/driver/src/main/java/org/neo4j/driver/internal/SessionFactoryImpl.java index 431d133b4f..6d633019ec 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/SessionFactoryImpl.java +++ b/driver/src/main/java/org/neo4j/driver/internal/SessionFactoryImpl.java @@ -27,10 +27,10 @@ public class SessionFactoryImpl implements SessionFactory { - protected final ConnectionProvider connectionProvider; - protected final RetryLogic retryLogic; - protected final Logging logging; - protected final boolean leakedSessionsLoggingEnabled; + private final ConnectionProvider connectionProvider; + private final RetryLogic retryLogic; + private final Logging logging; + private final boolean leakedSessionsLoggingEnabled; SessionFactoryImpl( ConnectionProvider connectionProvider, RetryLogic retryLogic, Config config ) { @@ -41,23 +41,23 @@ public class SessionFactoryImpl implements SessionFactory } @Override - public Session newInstance( AccessMode mode, String bookmark ) + public final Session newInstance( AccessMode mode, Bookmark bookmark ) { - NetworkSession session; - if ( leakedSessionsLoggingEnabled ) - { - session = new LeakLoggingNetworkSession( connectionProvider, mode, retryLogic, logging ); - } - else - { - session = new NetworkSession( connectionProvider, mode, retryLogic, logging ); - } + NetworkSession session = createSession( connectionProvider, retryLogic, mode, logging ); session.setBookmark( bookmark ); return session; } + protected NetworkSession createSession( ConnectionProvider connectionProvider, RetryLogic retryLogic, + AccessMode mode, Logging logging ) + { + return leakedSessionsLoggingEnabled ? + new LeakLoggingNetworkSession( connectionProvider, mode, retryLogic, logging ) : + new NetworkSession( connectionProvider, mode, retryLogic, logging ); + } + @Override - public void close() throws Exception + public final void close() throws Exception { connectionProvider.close(); } @@ -69,7 +69,7 @@ public void close() throws Exception * * @return the connection provider used by this factory. */ - public ConnectionProvider getConnectionProvider() + public final ConnectionProvider getConnectionProvider() { return connectionProvider; } diff --git a/driver/src/main/java/org/neo4j/driver/internal/net/SocketResponseHandler.java b/driver/src/main/java/org/neo4j/driver/internal/net/SocketResponseHandler.java index 2ca4e0f089..b7dda67019 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/net/SocketResponseHandler.java +++ b/driver/src/main/java/org/neo4j/driver/internal/net/SocketResponseHandler.java @@ -22,7 +22,7 @@ import java.util.Queue; import java.util.concurrent.ConcurrentLinkedQueue; -import org.neo4j.driver.v1.exceptions.AuthenticationException; +import org.neo4j.driver.internal.Bookmark; import org.neo4j.driver.internal.messaging.MessageHandler; import org.neo4j.driver.internal.spi.Collector; import org.neo4j.driver.internal.summary.InternalNotification; @@ -30,6 +30,7 @@ import org.neo4j.driver.internal.summary.InternalProfiledPlan; import org.neo4j.driver.internal.summary.InternalSummaryCounters; import org.neo4j.driver.v1.Value; +import org.neo4j.driver.v1.exceptions.AuthenticationException; import org.neo4j.driver.v1.exceptions.ClientException; import org.neo4j.driver.v1.exceptions.DatabaseException; import org.neo4j.driver.v1.exceptions.Neo4jException; @@ -206,7 +207,7 @@ private void collectBookmark( Collector collector, Value bookmark ) { if ( bookmark != null ) { - collector.bookmark( bookmark.asString() ); + collector.bookmark( Bookmark.from( bookmark.asString() ) ); } } diff --git a/driver/src/main/java/org/neo4j/driver/internal/spi/Collector.java b/driver/src/main/java/org/neo4j/driver/internal/spi/Collector.java index 4cdfbcbd39..dd056f4d05 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/spi/Collector.java +++ b/driver/src/main/java/org/neo4j/driver/internal/spi/Collector.java @@ -20,6 +20,7 @@ import java.util.List; +import org.neo4j.driver.internal.Bookmark; import org.neo4j.driver.v1.Value; import org.neo4j.driver.v1.exceptions.ClientException; import org.neo4j.driver.v1.exceptions.Neo4jException; @@ -130,7 +131,9 @@ public void profile( ProfiledPlan plan ) {} public void notifications( List notifications ) {} @Override - public void bookmark( String bookmark ) {} + public void bookmark( Bookmark bookmark ) + { + } @Override public void done() {} @@ -179,7 +182,7 @@ public void serverVersion( String server ){} void notifications( List notifications ); - void bookmark( String bookmark ); + void bookmark( Bookmark bookmark ); void done(); diff --git a/driver/src/main/java/org/neo4j/driver/internal/summary/SummaryBuilder.java b/driver/src/main/java/org/neo4j/driver/internal/summary/SummaryBuilder.java index dfcfb6e054..cd1b4dc577 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/summary/SummaryBuilder.java +++ b/driver/src/main/java/org/neo4j/driver/internal/summary/SummaryBuilder.java @@ -22,6 +22,7 @@ import java.util.List; import java.util.concurrent.TimeUnit; +import org.neo4j.driver.internal.Bookmark; import org.neo4j.driver.internal.spi.Collector; import org.neo4j.driver.v1.Statement; import org.neo4j.driver.v1.Value; @@ -131,7 +132,7 @@ public void notifications( List notifications ) } @Override - public void bookmark( String bookmark ) + public void bookmark( Bookmark bookmark ) { // intentionally empty } diff --git a/driver/src/main/java/org/neo4j/driver/v1/Driver.java b/driver/src/main/java/org/neo4j/driver/v1/Driver.java index ef9f5e1d39..679126dd90 100644 --- a/driver/src/main/java/org/neo4j/driver/v1/Driver.java +++ b/driver/src/main/java/org/neo4j/driver/v1/Driver.java @@ -77,7 +77,7 @@ public interface Driver extends AutoCloseable * Alias to {@code session(mode, null)}. * * @param mode the type of access required by units of work in this session, - * e.g. {@link AccessMode#READ read access}. + * e.g. {@link AccessMode#READ read access} or {@link AccessMode#WRITE write access}. * @return a new {@link Session} object. */ Session session( AccessMode mode ); @@ -101,13 +101,41 @@ public interface Driver extends AutoCloseable * transaction referenced by the supplied bookmark. * * @param mode the type of access required by units of work in this session, - * e.g. {@link AccessMode#READ read access}. + * e.g. {@link AccessMode#READ read access} or {@link AccessMode#WRITE write access}. * @param bookmark the initial reference to some previous transaction. A {@code null} value is permitted, and * indicates that the bookmark does not exist or is unknown. * @return a new {@link Session} object. */ Session session( AccessMode mode, String bookmark ); + /** + * Create a new {@link AccessMode#WRITE write} {@link Session} with specified initial bookmarks. + * First transaction in the created session will ensure that server hosting is at least as up-to-date as the + * latest transaction referenced by the supplied iterable of bookmarks. + *

+ * Alias to {@code session(AccessMode.WRITE, bookmarks)}. + * + * @param bookmarks initial references to some previous transactions. Both {@code null} value and empty iterable + * are permitted, and indicate that the bookmarks do not exist or are unknown. + * @return a new {@link Session} object. + */ + Session session( Iterable bookmarks ); + + /** + * Create a new {@link AccessMode#WRITE write} {@link Session} with specified initial bookmarks. + * First transaction in the created session will ensure that server hosting is at least as up-to-date as the + * latest transaction referenced by the supplied iterable of bookmarks. + *

+ * Alias to {@code session(AccessMode.WRITE, bookmarks)}. + * + * @param mode the type of access required by units of work in this session, + * e.g. {@link AccessMode#READ read access} or {@link AccessMode#WRITE write access}. + * @param bookmarks initial references to some previous transactions. Both {@code null} value and empty iterable + * are permitted, and indicate that the bookmarks do not exist or are unknown. + * @return a new {@link Session} object. + */ + Session session( AccessMode mode, Iterable bookmarks ); + /** * Close all the resources assigned to this driver, including any open connections. */ diff --git a/driver/src/main/java/org/neo4j/driver/v1/Session.java b/driver/src/main/java/org/neo4j/driver/v1/Session.java index 63f6f0986a..2c4e3e1609 100644 --- a/driver/src/main/java/org/neo4j/driver/v1/Session.java +++ b/driver/src/main/java/org/neo4j/driver/v1/Session.java @@ -18,6 +18,8 @@ */ package org.neo4j.driver.v1; +import java.util.Collection; + import org.neo4j.driver.v1.util.Resource; /** @@ -75,7 +77,7 @@ public interface Session extends Resource, StatementRunner * * @param bookmark a reference to a previous transaction * @return a new {@link Transaction} - * @deprecated This method is deprecated in favour of {@link Driver#session(String)} that accepts an initial + * @deprecated This method is deprecated in favour of {@link Driver#session(Collection)} that accepts an initial * bookmark. Session will ensure that all nested transactions are chained with bookmarks to guarantee * causal consistency. This method will be removed in the next major release. */ diff --git a/driver/src/test/java/org/neo4j/driver/internal/BookmarkTest.java b/driver/src/test/java/org/neo4j/driver/internal/BookmarkTest.java new file mode 100644 index 0000000000..12b1d166da --- /dev/null +++ b/driver/src/test/java/org/neo4j/driver/internal/BookmarkTest.java @@ -0,0 +1,139 @@ +/* + * Copyright (c) 2002-2017 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.driver.internal; + +import org.junit.Test; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import org.neo4j.driver.v1.Value; + +import static java.util.Arrays.asList; +import static java.util.Collections.emptyMap; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.neo4j.driver.v1.Values.value; + +public class BookmarkTest +{ + @Test + public void isEmptyForEmptyBookmark() + { + Bookmark bookmark = Bookmark.empty(); + assertTrue( bookmark.isEmpty() ); + } + + @Test + public void asStringForEmptyBookmark() + { + Bookmark bookmark = Bookmark.empty(); + assertNull( bookmark.asString() ); + } + + @Test + public void asParametersForEmptyBookmark() + { + Bookmark bookmark = Bookmark.empty(); + assertEquals( emptyMap(), bookmark.asParameters() ); + } + + @Test + public void isEmptyForNonEmptyBookmark() + { + Bookmark bookmark = Bookmark.from( "SomeBookmark" ); + assertFalse( bookmark.isEmpty() ); + } + + @Test + public void asStringForNonEmptyBookmark() + { + Bookmark bookmark = Bookmark.from( "SomeBookmark" ); + assertEquals( "SomeBookmark", bookmark.asString() ); + } + + @Test + public void asParametersForNonEmptyBookmark() + { + Bookmark bookmark = Bookmark.from( "SomeBookmark" ); + verifyParameters( bookmark, "SomeBookmark", "SomeBookmark" ); + } + + @Test + public void bookmarkFromString() + { + Bookmark bookmark = Bookmark.from( "Cat" ); + assertEquals( "Cat", bookmark.asString() ); + verifyParameters( bookmark, "Cat", "Cat" ); + } + + @Test + public void bookmarkFromNullString() + { + Bookmark bookmark = Bookmark.from( (String) null ); + assertTrue( bookmark.isEmpty() ); + } + + @Test + public void bookmarkFromIterable() + { + Bookmark bookmark = Bookmark.from( asList( + "neo4j:bookmark:v1:tx42", "neo4j:bookmark:v1:tx10", "neo4j:bookmark:v1:tx12" ) ); + assertEquals( "neo4j:bookmark:v1:tx42", bookmark.asString() ); + verifyParameters( bookmark, + "neo4j:bookmark:v1:tx42", + "neo4j:bookmark:v1:tx42", "neo4j:bookmark:v1:tx10", "neo4j:bookmark:v1:tx12" ); + } + + @Test + public void bookmarkFromNullIterable() + { + Bookmark bookmark = Bookmark.from( (Iterable) null ); + assertTrue( bookmark.isEmpty() ); + } + + @Test + public void bookmarkFromEmptyIterable() + { + Bookmark bookmark = Bookmark.from( Collections.emptyList() ); + assertTrue( bookmark.isEmpty() ); + } + + @Test + public void asParametersForBookmarkWithInvalidValue() + { + Bookmark bookmark = Bookmark.from( asList( + "neo4j:bookmark:v1:tx1", "neo4j:bookmark:v1:txcat", "neo4j:bookmark:v1:tx3" ) ); + assertEquals( "neo4j:bookmark:v1:tx3", bookmark.asString() ); + verifyParameters( bookmark, + "neo4j:bookmark:v1:tx3", + "neo4j:bookmark:v1:tx1", "neo4j:bookmark:v1:txcat", "neo4j:bookmark:v1:tx3" ); + } + + private static void verifyParameters( Bookmark bookmark, String bookmarkValue, String... bookmarkValues ) + { + Map expectedParameters = new HashMap<>(); + expectedParameters.put( "bookmark", value( bookmarkValue ) ); + expectedParameters.put( "bookmarks", value( bookmarkValues ) ); + assertEquals( expectedParameters, bookmark.asParameters() ); + } +} diff --git a/driver/src/test/java/org/neo4j/driver/internal/DirectDriverTest.java b/driver/src/test/java/org/neo4j/driver/internal/DirectDriverTest.java index 90dcf93eed..7984d66e83 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/DirectDriverTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/DirectDriverTest.java @@ -23,6 +23,8 @@ import org.junit.Test; import java.net.URI; +import java.util.Arrays; +import java.util.List; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.util.ServerVersion; @@ -30,11 +32,13 @@ import org.neo4j.driver.v1.GraphDatabase; import org.neo4j.driver.v1.Record; import org.neo4j.driver.v1.Session; +import org.neo4j.driver.v1.Transaction; import org.neo4j.driver.v1.util.StubServer; import org.neo4j.driver.v1.util.TestNeo4j; import static org.hamcrest.Matchers.is; import static org.hamcrest.core.IsEqual.equalTo; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; import static org.junit.Assume.assumeTrue; @@ -98,9 +102,9 @@ public void shouldRejectInvalidAddress() try { driver = GraphDatabase.driver( uri, neo4j.authToken() ); - fail("Expecting error for wrong uri"); + fail( "Expecting error for wrong uri" ); } - catch( IllegalArgumentException e ) + catch ( IllegalArgumentException e ) { assertThat( e.getMessage(), equalTo( "Invalid URI format `*`" ) ); } @@ -145,6 +149,32 @@ public void shouldBeAbleRunCypher() throws Exception assertThat( server.exitStatus(), equalTo( 0 ) ); } + @Test + public void shouldSendMultipleBookmarks() throws Exception + { + StubServer server = StubServer.start( "multiple_bookmarks.script", 9001 ); + + List bookmarks = Arrays.asList( "neo4j:bookmark:v1:tx5", "neo4j:bookmark:v1:tx29", + "neo4j:bookmark:v1:tx94", "neo4j:bookmark:v1:tx56", "neo4j:bookmark:v1:tx16", + "neo4j:bookmark:v1:tx68" ); + + try ( Driver driver = GraphDatabase.driver( "bolt://localhost:9001", INSECURE_CONFIG ); + Session session = driver.session( bookmarks ) ) + { + try ( Transaction tx = session.beginTransaction() ) + { + tx.run( "CREATE (n {name:'Bob'})" ); + tx.success(); + } + + assertEquals( "neo4j:bookmark:v1:tx95", session.lastBookmark() ); + } + finally + { + assertEquals( 0, server.exitStatus() ); + } + } + /** * Check if running test neo4j instance supports {@value org.neo4j.driver.v1.util.Neo4jSettings#LISTEN_ADDR} * configuration option. Only 3.1+ versions support it. diff --git a/driver/src/test/java/org/neo4j/driver/internal/ExplicitTransactionTest.java b/driver/src/test/java/org/neo4j/driver/internal/ExplicitTransactionTest.java index 465fbc4097..055db1a446 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/ExplicitTransactionTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/ExplicitTransactionTest.java @@ -29,6 +29,7 @@ import org.neo4j.driver.v1.Transaction; import org.neo4j.driver.v1.Value; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; @@ -39,7 +40,6 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; -import static org.neo4j.driver.v1.Values.value; public class ExplicitTransactionTest { @@ -135,12 +135,12 @@ public void shouldOnlyQueueMessagesWhenNoBookmarkGiven() @Test public void shouldSyncWhenBookmarkGiven() { - String bookmark = "hi, I'm bookmark"; + Bookmark bookmark = Bookmark.from( "hi, I'm bookmark" ); Connection connection = mock( Connection.class ); new ExplicitTransaction( connection, mock( SessionResourcesHandler.class ), bookmark ); - Map expectedParams = Collections.singletonMap( "bookmark", value( bookmark ) ); + Map expectedParams = bookmark.asParameters(); InOrder inOrder = inOrder( connection ); inOrder.verify( connection ).run( "BEGIN", expectedParams, Collector.NO_OP ); @@ -219,6 +219,41 @@ public void shouldBeClosedWhenMarkedToCloseAndClosed() assertFalse( tx.isOpen() ); } + @Test + public void shouldHaveEmptyBookmarkInitially() + { + ExplicitTransaction tx = new ExplicitTransaction( openConnectionMock(), mock( SessionResourcesHandler.class ) ); + assertTrue( tx.bookmark().isEmpty() ); + } + + @Test + public void shouldNotKeepInitialBookmark() + { + ExplicitTransaction tx = new ExplicitTransaction( openConnectionMock(), mock( SessionResourcesHandler.class ), + Bookmark.from( "Dog" ) ); + assertTrue( tx.bookmark().isEmpty() ); + } + + @Test + public void shouldNotOverwriteBookmarkWithNull() + { + ExplicitTransaction tx = new ExplicitTransaction( openConnectionMock(), mock( SessionResourcesHandler.class ) ); + tx.setBookmark( Bookmark.from( "Cat" ) ); + assertEquals( "Cat", tx.bookmark().asString() ); + tx.setBookmark( null ); + assertEquals( "Cat", tx.bookmark().asString() ); + } + + @Test + public void shouldNotOverwriteBookmarkWithEmptyBookmark() + { + ExplicitTransaction tx = new ExplicitTransaction( openConnectionMock(), mock( SessionResourcesHandler.class ) ); + tx.setBookmark( Bookmark.from( "Cat" ) ); + assertEquals( "Cat", tx.bookmark().asString() ); + tx.setBookmark( Bookmark.empty() ); + assertEquals( "Cat", tx.bookmark().asString() ); + } + private static Connection openConnectionMock() { Connection connection = mock( Connection.class ); diff --git a/driver/src/test/java/org/neo4j/driver/internal/NetworkSessionTest.java b/driver/src/test/java/org/neo4j/driver/internal/NetworkSessionTest.java index 6d5f2a1a12..26c37346f0 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/NetworkSessionTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/NetworkSessionTest.java @@ -44,7 +44,6 @@ import org.neo4j.driver.v1.exceptions.ServiceUnavailableException; import org.neo4j.driver.v1.exceptions.SessionExpiredException; -import static java.util.Collections.singletonMap; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; @@ -72,7 +71,6 @@ import static org.neo4j.driver.internal.spi.Collector.NO_OP; import static org.neo4j.driver.v1.AccessMode.READ; import static org.neo4j.driver.v1.AccessMode.WRITE; -import static org.neo4j.driver.v1.Values.value; public class NetworkSessionTest { @@ -362,7 +360,7 @@ public void updatesBookmarkWhenTxIsClosed() NetworkSession session = newSession( connectionProvider, READ ); Transaction tx = session.beginTransaction(); - setBookmark( tx, "TheBookmark" ); + setBookmark( tx, Bookmark.from( "TheBookmark" ) ); assertNull( session.lastBookmark() ); @@ -519,7 +517,7 @@ public void closesConnectionWhenResultIsBuffered() @Test public void bookmarkIsPropagatedFromSession() { - String bookmark = "Bookmark"; + Bookmark bookmark = Bookmark.from( "Bookmark" ); ConnectionProvider connectionProvider = mock( ConnectionProvider.class ); PooledConnection connection = mock( PooledConnection.class ); @@ -535,7 +533,7 @@ public void bookmarkIsPropagatedFromSession() @Test public void bookmarkIsPropagatedInBeginTransaction() { - String bookmark = "Bookmark"; + Bookmark bookmark = Bookmark.from( "Bookmark" ); ConnectionProvider connectionProvider = mock( ConnectionProvider.class ); PooledConnection connection = mock( PooledConnection.class ); @@ -552,8 +550,8 @@ public void bookmarkIsPropagatedInBeginTransaction() @Test public void bookmarkIsPropagatedBetweenTransactions() { - String bookmark1 = "Bookmark1"; - String bookmark2 = "Bookmark2"; + Bookmark bookmark1 = Bookmark.from( "Bookmark1" ); + Bookmark bookmark2 = Bookmark.from( "Bookmark2" ); ConnectionProvider connectionProvider = mock( ConnectionProvider.class ); PooledConnection connection = mock( PooledConnection.class ); @@ -565,16 +563,16 @@ public void bookmarkIsPropagatedBetweenTransactions() setBookmark( tx, bookmark1 ); } - assertEquals( bookmark1, session.lastBookmark() ); + assertEquals( bookmark1.asString(), session.lastBookmark() ); try ( Transaction tx = session.beginTransaction() ) { verifyBeginTx( connection, bookmark1 ); - assertNull( getBookmark( tx ) ); + assertTrue( getBookmark( tx ).isEmpty() ); setBookmark( tx, bookmark2 ); } - assertEquals( bookmark2, session.lastBookmark() ); + assertEquals( bookmark2.asString(), session.lastBookmark() ); } @Test @@ -596,7 +594,7 @@ public void setLastBookmark() { NetworkSession session = newSession( mock( ConnectionProvider.class ), WRITE ); - session.setBookmark( "TheBookmark" ); + session.setBookmark( Bookmark.from( "TheBookmark" ) ); assertEquals( "TheBookmark", session.lastBookmark() ); } @@ -608,7 +606,7 @@ public void testPassingNoBookmarkShouldRetainBookmark() PooledConnection connection = openConnectionMock(); when( connectionProvider.acquireConnection( READ ) ).thenReturn( connection ); NetworkSession session = newSession( connectionProvider, READ ); - session.setBookmark( "X" ); + session.setBookmark( Bookmark.from( "X" ) ); session.beginTransaction(); assertThat( session.lastBookmark(), equalTo( "X" ) ); } @@ -621,7 +619,7 @@ public void testPassingNullBookmarkShouldRetainBookmark() PooledConnection connection = openConnectionMock(); when( connectionProvider.acquireConnection( READ ) ).thenReturn( connection ); NetworkSession session = newSession( connectionProvider, READ ); - session.setBookmark( "X" ); + session.setBookmark( Bookmark.from( "X" ) ); session.beginTransaction( null ); assertThat( session.lastBookmark(), equalTo( "X" ) ); } @@ -735,6 +733,7 @@ public void writeTxRetriedUntilFailureWhenTxCloseThrows() } @Test + @SuppressWarnings( "deprecation" ) public void transactionShouldBeOpenAfterSessionReset() { ConnectionProvider connectionProvider = mock( ConnectionProvider.class ); @@ -750,6 +749,7 @@ public void transactionShouldBeOpenAfterSessionReset() } @Test + @SuppressWarnings( "deprecation" ) public void transactionShouldBeClosedAfterSessionResetAndClose() { ConnectionProvider connectionProvider = mock( ConnectionProvider.class ); @@ -767,6 +767,31 @@ public void transactionShouldBeClosedAfterSessionResetAndClose() assertFalse( tx.isOpen() ); } + @Test + public void shouldHaveNullLastBookmarkInitially() + { + NetworkSession session = newSession( mock( ConnectionProvider.class ), READ ); + assertNull( session.lastBookmark() ); + } + + @Test + public void shouldNotOverwriteBookmarkWithNull() + { + NetworkSession session = newSession( mock( ConnectionProvider.class ), READ, Bookmark.from( "Cat" ) ); + assertEquals( "Cat", session.lastBookmark() ); + session.setBookmark( null ); + assertEquals( "Cat", session.lastBookmark() ); + } + + @Test + public void shouldNotOverwriteBookmarkWithEmptyBookmark() + { + NetworkSession session = newSession( mock( ConnectionProvider.class ), READ, Bookmark.from( "Cat" ) ); + assertEquals( "Cat", session.lastBookmark() ); + session.setBookmark( Bookmark.empty() ); + assertEquals( "Cat", session.lastBookmark() ); + } + private static void testConnectionAcquisition( AccessMode sessionMode, AccessMode transactionMode ) { ConnectionProvider connectionProvider = mock( ConnectionProvider.class ); @@ -968,21 +993,22 @@ else if ( mode == WRITE ) private static NetworkSession newSession( ConnectionProvider connectionProvider, AccessMode mode ) { - return newSession( connectionProvider, mode, null ); + return newSession( connectionProvider, mode, Bookmark.empty() ); } private static NetworkSession newSession( ConnectionProvider connectionProvider, RetryLogic retryLogic ) { - return newSession( connectionProvider, WRITE, retryLogic, null ); + return newSession( connectionProvider, WRITE, retryLogic, Bookmark.empty() ); } - private static NetworkSession newSession( ConnectionProvider connectionProvider, AccessMode mode, String bookmark ) + private static NetworkSession newSession( ConnectionProvider connectionProvider, AccessMode mode, + Bookmark bookmark ) { return newSession( connectionProvider, mode, new FixedRetryLogic( 0 ), bookmark ); } private static NetworkSession newSession( ConnectionProvider connectionProvider, AccessMode mode, - RetryLogic retryLogic, String bookmark ) + RetryLogic retryLogic, Bookmark bookmark ) { NetworkSession session = new NetworkSession( connectionProvider, mode, retryLogic, DEV_NULL_LOGGING ); session.setBookmark( bookmark ); @@ -1028,9 +1054,9 @@ private static void verifyBeginTx( PooledConnection connectionMock, Verification verifyRun( connectionMock, "BEGIN", mode ); } - private static void verifyBeginTx( PooledConnection connectionMock, String bookmark ) + private static void verifyBeginTx( PooledConnection connectionMock, Bookmark bookmark ) { - verify( connectionMock ).run( "BEGIN", singletonMap( "bookmark", value( bookmark ) ), NO_OP ); + verify( connectionMock ).run( "BEGIN", bookmark.asParameters(), NO_OP ); } private static void verifyCommitTx( PooledConnection connectionMock, VerificationMode mode ) @@ -1053,12 +1079,12 @@ private static Map anyParams() return anyMapOf( String.class, Value.class ); } - private static String getBookmark( Transaction tx ) + private static Bookmark getBookmark( Transaction tx ) { return ((ExplicitTransaction) tx).bookmark(); } - private static void setBookmark( Transaction tx, String bookmark ) + private static void setBookmark( Transaction tx, Bookmark bookmark ) { ((ExplicitTransaction) tx).setBookmark( bookmark ); } diff --git a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverBoltKitTest.java b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverBoltKitTest.java index d893fbeac3..8c6ea1f5a3 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverBoltKitTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverBoltKitTest.java @@ -25,6 +25,7 @@ import java.io.IOException; import java.net.URI; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; import java.util.logging.Level; @@ -1007,6 +1008,34 @@ public void shouldTreatRoutingTableWithSingleRouterAsValid() throws Exception } } + @Test + public void shouldSendMultipleBookmarks() throws Exception + { + StubServer router = StubServer.start( "acquire_endpoints.script", 9001 ); + StubServer writer = StubServer.start( "multiple_bookmarks.script", 9007 ); + + List bookmarks = Arrays.asList( "neo4j:bookmark:v1:tx5", "neo4j:bookmark:v1:tx29", + "neo4j:bookmark:v1:tx94", "neo4j:bookmark:v1:tx56", "neo4j:bookmark:v1:tx16", + "neo4j:bookmark:v1:tx68" ); + + try ( Driver driver = GraphDatabase.driver( "bolt+routing://localhost:9001", config ); + Session session = driver.session( bookmarks ) ) + { + try ( Transaction tx = session.beginTransaction() ) + { + tx.run( "CREATE (n {name:'Bob'})" ); + tx.success(); + } + + assertEquals( "neo4j:bookmark:v1:tx95", session.lastBookmark() ); + } + finally + { + assertEquals( 0, router.exitStatus() ); + assertEquals( 0, writer.exitStatus() ); + } + } + private static Driver newDriverWithSleeplessClock( String uriString ) { DriverFactory driverFactory = new DriverFactoryWithClock( new SleeplessClock() ); diff --git a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java index 97403a5115..ec857b03e0 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java @@ -33,6 +33,7 @@ import org.neo4j.driver.internal.cluster.RoutingSettings; import org.neo4j.driver.internal.net.BoltServerAddress; import org.neo4j.driver.internal.retry.FixedRetryLogic; +import org.neo4j.driver.internal.retry.RetryLogic; import org.neo4j.driver.internal.spi.Collector; import org.neo4j.driver.internal.spi.ConnectionPool; import org.neo4j.driver.internal.spi.ConnectionProvider; @@ -45,7 +46,6 @@ import org.neo4j.driver.v1.EventLogger; import org.neo4j.driver.v1.GraphDatabase; import org.neo4j.driver.v1.Logging; -import org.neo4j.driver.v1.Session; import org.neo4j.driver.v1.Value; import org.neo4j.driver.v1.exceptions.ClientException; import org.neo4j.driver.v1.exceptions.ProtocolException; @@ -440,11 +440,10 @@ private static class NetworkSessionWithAddressFactory extends SessionFactoryImpl } @Override - public Session newInstance( AccessMode mode, String bookmark ) + protected NetworkSession createSession( ConnectionProvider connectionProvider, RetryLogic retryLogic, + AccessMode mode, Logging logging ) { - NetworkSessionWithAddress session = new NetworkSessionWithAddress( connectionProvider, mode, logging ); - session.setBookmark( bookmark ); - return session; + return new NetworkSessionWithAddress( connectionProvider, mode, logging ); } } diff --git a/driver/src/test/resources/multiple_bookmarks.script b/driver/src/test/resources/multiple_bookmarks.script new file mode 100644 index 0000000000..5e7344103c --- /dev/null +++ b/driver/src/test/resources/multiple_bookmarks.script @@ -0,0 +1,16 @@ +!: AUTO INIT +!: AUTO RESET +!: AUTO PULL_ALL + +C: RUN "BEGIN" {"bookmark": "neo4j:bookmark:v1:tx94", "bookmarks": ["neo4j:bookmark:v1:tx5", "neo4j:bookmark:v1:tx29", "neo4j:bookmark:v1:tx94", "neo4j:bookmark:v1:tx56", "neo4j:bookmark:v1:tx16", "neo4j:bookmark:v1:tx68"]} + PULL_ALL +S: SUCCESS {} + SUCCESS {} +C: RUN "CREATE (n {name:'Bob'})" {} + PULL_ALL +S: SUCCESS {} + SUCCESS {"bookmark": "neo4j:bookmark:v1:tx95"} +C: RUN "COMMIT" {} + PULL_ALL +S: SUCCESS {} + SUCCESS {} diff --git a/driver/src/test/resources/read_tx_with_bookmarks.script b/driver/src/test/resources/read_tx_with_bookmarks.script index a48ee65f9a..47e9244506 100644 --- a/driver/src/test/resources/read_tx_with_bookmarks.script +++ b/driver/src/test/resources/read_tx_with_bookmarks.script @@ -2,7 +2,7 @@ !: AUTO RESET !: AUTO PULL_ALL -C: RUN "BEGIN" {"bookmark": "OldBookmark"} +C: RUN "BEGIN" {"bookmark": "OldBookmark", "bookmarks": ["OldBookmark"]} PULL_ALL S: SUCCESS {} SUCCESS {} diff --git a/driver/src/test/resources/write_read_tx_with_bookmarks.script b/driver/src/test/resources/write_read_tx_with_bookmarks.script index cb689a1627..f01feff289 100644 --- a/driver/src/test/resources/write_read_tx_with_bookmarks.script +++ b/driver/src/test/resources/write_read_tx_with_bookmarks.script @@ -2,7 +2,7 @@ !: AUTO RESET !: AUTO PULL_ALL -C: RUN "BEGIN" {"bookmark": "BookmarkA"} +C: RUN "BEGIN" {"bookmark": "BookmarkA", "bookmarks": ["BookmarkA"]} PULL_ALL S: SUCCESS {} SUCCESS {} @@ -14,7 +14,7 @@ C: RUN "COMMIT" {} PULL_ALL S: SUCCESS {} SUCCESS {} -C: RUN "BEGIN" {"bookmark": "BookmarkB"} +C: RUN "BEGIN" {"bookmark": "BookmarkB", "bookmarks": ["BookmarkB"]} PULL_ALL S: SUCCESS {} SUCCESS {} diff --git a/driver/src/test/resources/write_tx_with_bookmarks.script b/driver/src/test/resources/write_tx_with_bookmarks.script index 08247a9e2a..4413266b16 100644 --- a/driver/src/test/resources/write_tx_with_bookmarks.script +++ b/driver/src/test/resources/write_tx_with_bookmarks.script @@ -2,7 +2,7 @@ !: AUTO RESET !: AUTO PULL_ALL -C: RUN "BEGIN" {"bookmark": "OldBookmark"} +C: RUN "BEGIN" {"bookmark": "OldBookmark", "bookmarks": ["OldBookmark"]} PULL_ALL S: SUCCESS {} SUCCESS {} From 9bcd4b2ac8d0274035b76d0240505654b0ad612d Mon Sep 17 00:00:00 2001 From: lutovich Date: Wed, 24 May 2017 11:09:19 +0200 Subject: [PATCH 2/2] Address review comments * improve method naming in `Bookmark` * handle `null` bookmark in given `Iterable` --- .../org/neo4j/driver/internal/Bookmark.java | 6 +- .../driver/internal/ExplicitTransaction.java | 2 +- .../neo4j/driver/internal/NetworkSession.java | 2 +- .../neo4j/driver/internal/BookmarkTest.java | 56 ++++++++++++++----- .../internal/ExplicitTransactionTest.java | 10 ++-- .../driver/internal/NetworkSessionTest.java | 6 +- 6 files changed, 54 insertions(+), 28 deletions(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/Bookmark.java b/driver/src/main/java/org/neo4j/driver/internal/Bookmark.java index af5d71e5ff..a7729f15dc 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/Bookmark.java +++ b/driver/src/main/java/org/neo4j/driver/internal/Bookmark.java @@ -76,12 +76,12 @@ public boolean isEmpty() return maxValue == null; } - public String asString() + public String maxBookmarkAsString() { return maxValue; } - public Map asParameters() + public Map asBeginTransactionParameters() { if ( isEmpty() ) { @@ -138,7 +138,7 @@ private static String maxBookmark( Iterable bookmarks ) private static long bookmarkValue( String value ) { - if ( value.startsWith( BOOKMARK_PREFIX ) ) + if ( value != null && value.startsWith( BOOKMARK_PREFIX ) ) { try { diff --git a/driver/src/main/java/org/neo4j/driver/internal/ExplicitTransaction.java b/driver/src/main/java/org/neo4j/driver/internal/ExplicitTransaction.java index ec01691821..4701ec2314 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/ExplicitTransaction.java +++ b/driver/src/main/java/org/neo4j/driver/internal/ExplicitTransaction.java @@ -252,7 +252,7 @@ void setBookmark( Bookmark bookmark ) private static void runBeginStatement( Connection connection, Bookmark bookmark ) { Bookmark initialBookmark = bookmark == null ? Bookmark.empty() : bookmark; - Map parameters = initialBookmark.asParameters(); + Map parameters = initialBookmark.asBeginTransactionParameters(); connection.run( "BEGIN", parameters, Collector.NO_OP ); connection.pullAll( Collector.NO_OP ); diff --git a/driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java b/driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java index 0e38bb6c7d..8f2c78fe7d 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java +++ b/driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java @@ -206,7 +206,7 @@ void setBookmark( Bookmark bookmark ) @Override public String lastBookmark() { - return bookmark == null ? null : bookmark.asString(); + return bookmark == null ? null : bookmark.maxBookmarkAsString(); } @Override diff --git a/driver/src/test/java/org/neo4j/driver/internal/BookmarkTest.java b/driver/src/test/java/org/neo4j/driver/internal/BookmarkTest.java index 12b1d166da..c5ab4fcf53 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/BookmarkTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/BookmarkTest.java @@ -22,6 +22,7 @@ import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.neo4j.driver.v1.Value; @@ -44,17 +45,17 @@ public void isEmptyForEmptyBookmark() } @Test - public void asStringForEmptyBookmark() + public void maxBookmarkAsStringForEmptyBookmark() { Bookmark bookmark = Bookmark.empty(); - assertNull( bookmark.asString() ); + assertNull( bookmark.maxBookmarkAsString() ); } @Test - public void asParametersForEmptyBookmark() + public void asBeginTransactionParametersForEmptyBookmark() { Bookmark bookmark = Bookmark.empty(); - assertEquals( emptyMap(), bookmark.asParameters() ); + assertEquals( emptyMap(), bookmark.asBeginTransactionParameters() ); } @Test @@ -65,14 +66,14 @@ public void isEmptyForNonEmptyBookmark() } @Test - public void asStringForNonEmptyBookmark() + public void maxBookmarkAsStringForNonEmptyBookmark() { Bookmark bookmark = Bookmark.from( "SomeBookmark" ); - assertEquals( "SomeBookmark", bookmark.asString() ); + assertEquals( "SomeBookmark", bookmark.maxBookmarkAsString() ); } @Test - public void asParametersForNonEmptyBookmark() + public void asBeginTransactionParametersForNonEmptyBookmark() { Bookmark bookmark = Bookmark.from( "SomeBookmark" ); verifyParameters( bookmark, "SomeBookmark", "SomeBookmark" ); @@ -82,7 +83,7 @@ public void asParametersForNonEmptyBookmark() public void bookmarkFromString() { Bookmark bookmark = Bookmark.from( "Cat" ); - assertEquals( "Cat", bookmark.asString() ); + assertEquals( "Cat", bookmark.maxBookmarkAsString() ); verifyParameters( bookmark, "Cat", "Cat" ); } @@ -98,7 +99,7 @@ public void bookmarkFromIterable() { Bookmark bookmark = Bookmark.from( asList( "neo4j:bookmark:v1:tx42", "neo4j:bookmark:v1:tx10", "neo4j:bookmark:v1:tx12" ) ); - assertEquals( "neo4j:bookmark:v1:tx42", bookmark.asString() ); + assertEquals( "neo4j:bookmark:v1:tx42", bookmark.maxBookmarkAsString() ); verifyParameters( bookmark, "neo4j:bookmark:v1:tx42", "neo4j:bookmark:v1:tx42", "neo4j:bookmark:v1:tx10", "neo4j:bookmark:v1:tx12" ); @@ -119,21 +120,46 @@ public void bookmarkFromEmptyIterable() } @Test - public void asParametersForBookmarkWithInvalidValue() + public void asBeginTransactionParametersForBookmarkWithInvalidValue() { Bookmark bookmark = Bookmark.from( asList( "neo4j:bookmark:v1:tx1", "neo4j:bookmark:v1:txcat", "neo4j:bookmark:v1:tx3" ) ); - assertEquals( "neo4j:bookmark:v1:tx3", bookmark.asString() ); + assertEquals( "neo4j:bookmark:v1:tx3", bookmark.maxBookmarkAsString() ); verifyParameters( bookmark, "neo4j:bookmark:v1:tx3", "neo4j:bookmark:v1:tx1", "neo4j:bookmark:v1:txcat", "neo4j:bookmark:v1:tx3" ); } - private static void verifyParameters( Bookmark bookmark, String bookmarkValue, String... bookmarkValues ) + @Test + public void asBeginTransactionParametersForBookmarkWithEmptyStringValue() + { + Bookmark bookmark = Bookmark.from( asList( "neo4j:bookmark:v1:tx9", "", "neo4j:bookmark:v1:tx3" ) ); + assertEquals( "neo4j:bookmark:v1:tx9", bookmark.maxBookmarkAsString() ); + verifyParameters( bookmark, + "neo4j:bookmark:v1:tx9", + "neo4j:bookmark:v1:tx9", "", "neo4j:bookmark:v1:tx3" ); + } + + @Test + public void asBeginTransactionParametersForBookmarkWithNullValue() + { + Bookmark bookmark = Bookmark.from( asList( "neo4j:bookmark:v1:tx41", null, "neo4j:bookmark:v1:tx42" ) ); + assertEquals( "neo4j:bookmark:v1:tx42", bookmark.maxBookmarkAsString() ); + verifyParameters( bookmark, + "neo4j:bookmark:v1:tx42", + asList( "neo4j:bookmark:v1:tx41", null, "neo4j:bookmark:v1:tx42" ) ); + } + + private static void verifyParameters( Bookmark bookmark, String expectedMaxValue, String... expectedValues ) + { + verifyParameters( bookmark, expectedMaxValue, asList( expectedValues ) ); + } + + private static void verifyParameters( Bookmark bookmark, String expectedMaxValue, List expectedValues ) { Map expectedParameters = new HashMap<>(); - expectedParameters.put( "bookmark", value( bookmarkValue ) ); - expectedParameters.put( "bookmarks", value( bookmarkValues ) ); - assertEquals( expectedParameters, bookmark.asParameters() ); + expectedParameters.put( "bookmark", value( expectedMaxValue ) ); + expectedParameters.put( "bookmarks", value( expectedValues ) ); + assertEquals( expectedParameters, bookmark.asBeginTransactionParameters() ); } } diff --git a/driver/src/test/java/org/neo4j/driver/internal/ExplicitTransactionTest.java b/driver/src/test/java/org/neo4j/driver/internal/ExplicitTransactionTest.java index 055db1a446..ba41de32c5 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/ExplicitTransactionTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/ExplicitTransactionTest.java @@ -140,7 +140,7 @@ public void shouldSyncWhenBookmarkGiven() new ExplicitTransaction( connection, mock( SessionResourcesHandler.class ), bookmark ); - Map expectedParams = bookmark.asParameters(); + Map expectedParams = bookmark.asBeginTransactionParameters(); InOrder inOrder = inOrder( connection ); inOrder.verify( connection ).run( "BEGIN", expectedParams, Collector.NO_OP ); @@ -239,9 +239,9 @@ public void shouldNotOverwriteBookmarkWithNull() { ExplicitTransaction tx = new ExplicitTransaction( openConnectionMock(), mock( SessionResourcesHandler.class ) ); tx.setBookmark( Bookmark.from( "Cat" ) ); - assertEquals( "Cat", tx.bookmark().asString() ); + assertEquals( "Cat", tx.bookmark().maxBookmarkAsString() ); tx.setBookmark( null ); - assertEquals( "Cat", tx.bookmark().asString() ); + assertEquals( "Cat", tx.bookmark().maxBookmarkAsString() ); } @Test @@ -249,9 +249,9 @@ public void shouldNotOverwriteBookmarkWithEmptyBookmark() { ExplicitTransaction tx = new ExplicitTransaction( openConnectionMock(), mock( SessionResourcesHandler.class ) ); tx.setBookmark( Bookmark.from( "Cat" ) ); - assertEquals( "Cat", tx.bookmark().asString() ); + assertEquals( "Cat", tx.bookmark().maxBookmarkAsString() ); tx.setBookmark( Bookmark.empty() ); - assertEquals( "Cat", tx.bookmark().asString() ); + assertEquals( "Cat", tx.bookmark().maxBookmarkAsString() ); } private static Connection openConnectionMock() diff --git a/driver/src/test/java/org/neo4j/driver/internal/NetworkSessionTest.java b/driver/src/test/java/org/neo4j/driver/internal/NetworkSessionTest.java index 26c37346f0..2cc85b6579 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/NetworkSessionTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/NetworkSessionTest.java @@ -563,7 +563,7 @@ public void bookmarkIsPropagatedBetweenTransactions() setBookmark( tx, bookmark1 ); } - assertEquals( bookmark1.asString(), session.lastBookmark() ); + assertEquals( bookmark1.maxBookmarkAsString(), session.lastBookmark() ); try ( Transaction tx = session.beginTransaction() ) { @@ -572,7 +572,7 @@ public void bookmarkIsPropagatedBetweenTransactions() setBookmark( tx, bookmark2 ); } - assertEquals( bookmark2.asString(), session.lastBookmark() ); + assertEquals( bookmark2.maxBookmarkAsString(), session.lastBookmark() ); } @Test @@ -1056,7 +1056,7 @@ private static void verifyBeginTx( PooledConnection connectionMock, Verification private static void verifyBeginTx( PooledConnection connectionMock, Bookmark bookmark ) { - verify( connectionMock ).run( "BEGIN", bookmark.asParameters(), NO_OP ); + verify( connectionMock ).run( "BEGIN", bookmark.asBeginTransactionParameters(), NO_OP ); } private static void verifyCommitTx( PooledConnection connectionMock, VerificationMode mode )