From 60ba42ce6bde42796a03f6f52e7e9b99a08f6276 Mon Sep 17 00:00:00 2001 From: lutovich Date: Tue, 31 Jul 2018 16:16:18 +0200 Subject: [PATCH] Improve handling of malformed BEGIN and RUN metadata Make metadata parsing errors result in a FAILURE message. Previously, they forced Bolt server to close the connection. Such errors can happen when bookmark format is invalid, transaction timeout or metadata can't be parsed or if of illegal type. --- .../org/neo4j/bolt/v1/runtime/ReadyState.java | 3 +- .../bolt/v1/runtime/bookmarking/Bookmark.java | 19 +- .../bookmarking/BookmarkFormatException.java | 41 ++++ .../v3/messaging/request/BeginMessage.java | 5 +- .../request/MessageMetadataParser.java | 18 +- .../bolt/v3/messaging/request/RunMessage.java | 3 +- .../v1/runtime/bookmarking/BookmarkTest.java | 205 ++++++------------ .../request/MessageMetadataParserTest.java | 66 ++++++ .../integration/BoltV3TransportIT.java | 108 ++++++++- 9 files changed, 289 insertions(+), 179 deletions(-) create mode 100644 community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/bookmarking/BookmarkFormatException.java create mode 100644 community/bolt/src/test/java/org/neo4j/bolt/v3/messaging/request/MessageMetadataParserTest.java diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/ReadyState.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/ReadyState.java index 2a0cc974b8d4..95fe1f54cf0f 100644 --- a/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/ReadyState.java +++ b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/ReadyState.java @@ -30,7 +30,6 @@ import org.neo4j.bolt.v1.messaging.request.RunMessage; import org.neo4j.bolt.v1.runtime.bookmarking.Bookmark; import org.neo4j.graphdb.security.AuthorizationExpiredException; -import org.neo4j.internal.kernel.api.exceptions.KernelException; import org.neo4j.values.storable.Values; import static org.neo4j.bolt.v1.runtime.RunMessageChecker.isBegin; @@ -118,7 +117,7 @@ private BoltStateMachineState processRunMessage( RunMessage message, StateMachin } } - private static StatementMetadata processRunMessage( RunMessage message, StatementProcessor statementProcessor ) throws KernelException + private static StatementMetadata processRunMessage( RunMessage message, StatementProcessor statementProcessor ) throws Exception { if ( isBegin( message ) ) { diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/bookmarking/Bookmark.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/bookmarking/Bookmark.java index 97125b65e64e..34cfa424f7e8 100644 --- a/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/bookmarking/Bookmark.java +++ b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/bookmarking/Bookmark.java @@ -22,8 +22,6 @@ import java.util.Objects; import org.neo4j.bolt.runtime.BoltResponseHandler; -import org.neo4j.internal.kernel.api.exceptions.KernelException; -import org.neo4j.kernel.api.exceptions.Status; import org.neo4j.values.AnyValue; import org.neo4j.values.storable.TextValue; import org.neo4j.values.storable.Values; @@ -37,7 +35,7 @@ public class Bookmark { private static final String BOOKMARK_KEY = "bookmark"; private static final String BOOKMARKS_KEY = "bookmarks"; - private static final String BOOKMARK_TX_PREFIX = "neo4j:bookmark:v1:tx"; + static final String BOOKMARK_TX_PREFIX = "neo4j:bookmark:v1:tx"; private final long txId; @@ -160,19 +158,4 @@ public void attachTo( BoltResponseHandler state ) { state.onMetadata( BOOKMARK_KEY, stringValue( toString() ) ); } - - static class BookmarkFormatException extends KernelException - { - BookmarkFormatException( String bookmarkString, NumberFormatException e ) - { - super( Status.Transaction.InvalidBookmark, e, "Supplied bookmark [%s] does not conform to pattern %s; " + - "unable to parse transaction id", bookmarkString, BOOKMARK_TX_PREFIX ); - } - - BookmarkFormatException( Object bookmarkObject ) - { - super( Status.Transaction.InvalidBookmark, "Supplied bookmark [%s] does not conform to pattern %s", - bookmarkObject, BOOKMARK_TX_PREFIX ); - } - } } diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/bookmarking/BookmarkFormatException.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/bookmarking/BookmarkFormatException.java new file mode 100644 index 000000000000..c25f8eb2282e --- /dev/null +++ b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/bookmarking/BookmarkFormatException.java @@ -0,0 +1,41 @@ +/* + * Copyright (c) 2002-2018 "Neo4j," + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.bolt.v1.runtime.bookmarking; + +import org.neo4j.bolt.messaging.BoltIOException; +import org.neo4j.kernel.api.exceptions.Status; + +import static org.neo4j.bolt.v1.runtime.bookmarking.Bookmark.BOOKMARK_TX_PREFIX; + +class BookmarkFormatException extends BoltIOException +{ + BookmarkFormatException( String bookmarkString, NumberFormatException cause ) + { + super( Status.Transaction.InvalidBookmark, + String.format( "Supplied bookmark [%s] does not conform to pattern %s; unable to parse transaction id", bookmarkString, BOOKMARK_TX_PREFIX ), + cause ); + } + + BookmarkFormatException( Object bookmarkObject ) + { + super( Status.Transaction.InvalidBookmark, + String.format( "Supplied bookmark [%s] does not conform to pattern %s", bookmarkObject, BOOKMARK_TX_PREFIX ) ); + } +} diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v3/messaging/request/BeginMessage.java b/community/bolt/src/main/java/org/neo4j/bolt/v3/messaging/request/BeginMessage.java index 32e0d114bc51..c6ba73819d65 100644 --- a/community/bolt/src/main/java/org/neo4j/bolt/v3/messaging/request/BeginMessage.java +++ b/community/bolt/src/main/java/org/neo4j/bolt/v3/messaging/request/BeginMessage.java @@ -30,7 +30,6 @@ import org.neo4j.values.virtual.VirtualValues; import static java.util.Objects.requireNonNull; -import static org.neo4j.bolt.v3.messaging.request.MessageMetadataParser.parseBookmark; import static org.neo4j.bolt.v3.messaging.request.MessageMetadataParser.parseTransactionMetadata; import static org.neo4j.bolt.v3.messaging.request.MessageMetadataParser.parseTransactionTimeout; @@ -51,7 +50,7 @@ public BeginMessage() throws BoltIOException public BeginMessage( MapValue meta ) throws BoltIOException { this.meta = requireNonNull( meta ); - this.bookmark = parseBookmark( meta ); + this.bookmark = Bookmark.fromParamsOrNull( meta ); this.txTimeout = parseTransactionTimeout( meta ); this.txMetadata = parseTransactionMetadata( meta ); } @@ -84,7 +83,7 @@ public boolean equals( Object o ) return false; } BeginMessage that = (BeginMessage) o; - return Objects.equals( meta, that.meta ) && Objects.equals( meta, that.meta ); + return Objects.equals( meta, that.meta ); } @Override diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v3/messaging/request/MessageMetadataParser.java b/community/bolt/src/main/java/org/neo4j/bolt/v3/messaging/request/MessageMetadataParser.java index 19c241d35716..4666b586f0f8 100644 --- a/community/bolt/src/main/java/org/neo4j/bolt/v3/messaging/request/MessageMetadataParser.java +++ b/community/bolt/src/main/java/org/neo4j/bolt/v3/messaging/request/MessageMetadataParser.java @@ -24,11 +24,9 @@ import java.util.Map; import org.neo4j.bolt.messaging.BoltIOException; -import org.neo4j.bolt.v1.runtime.bookmarking.Bookmark; import org.neo4j.graphdb.Node; import org.neo4j.graphdb.Relationship; import org.neo4j.graphdb.spatial.Point; -import org.neo4j.internal.kernel.api.exceptions.KernelException; import org.neo4j.kernel.api.exceptions.Status; import org.neo4j.kernel.impl.util.BaseToObjectValueWriter; import org.neo4j.values.AnyValue; @@ -40,21 +38,13 @@ /** * The parsing methods in this class returns null if the specified key is not found in the input message metadata map. */ -class MessageMetadataParser +final class MessageMetadataParser { private static final String TX_TIMEOUT_KEY = "tx_timeout"; private static final String TX_META_DATA_KEY = "tx_metadata"; - static Bookmark parseBookmark( MapValue meta ) throws BoltIOException + private MessageMetadataParser() { - try - { - return Bookmark.fromParamsOrNull( meta ); - } - catch ( KernelException e ) - { - throw new BoltIOException( Status.Request.InvalidFormat, e.getMessage(), e ); - } } static Duration parseTransactionTimeout( MapValue meta ) throws BoltIOException @@ -70,7 +60,7 @@ else if ( anyValue instanceof LongValue ) } else { - throw new BoltIOException( Status.Request.InvalidFormat, "Expecting transaction timeout value to be a Long value, but got: " + anyValue ); + throw new BoltIOException( Status.Request.Invalid, "Expecting transaction timeout value to be a Long value, but got: " + anyValue ); } } @@ -91,7 +81,7 @@ else if ( anyValue instanceof MapValue ) } else { - throw new BoltIOException( Status.Request.InvalidFormat, "Expecting transaction metadata value to be a Map value, but got: " + anyValue ); + throw new BoltIOException( Status.Request.Invalid, "Expecting transaction metadata value to be a Map value, but got: " + anyValue ); } } diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v3/messaging/request/RunMessage.java b/community/bolt/src/main/java/org/neo4j/bolt/v3/messaging/request/RunMessage.java index 314603751b52..e6bff84d8c67 100644 --- a/community/bolt/src/main/java/org/neo4j/bolt/v3/messaging/request/RunMessage.java +++ b/community/bolt/src/main/java/org/neo4j/bolt/v3/messaging/request/RunMessage.java @@ -30,7 +30,6 @@ import org.neo4j.values.virtual.VirtualValues; import static java.util.Objects.requireNonNull; -import static org.neo4j.bolt.v3.messaging.request.MessageMetadataParser.parseBookmark; import static org.neo4j.bolt.v3.messaging.request.MessageMetadataParser.parseTransactionMetadata; import static org.neo4j.bolt.v3.messaging.request.MessageMetadataParser.parseTransactionTimeout; @@ -62,7 +61,7 @@ public RunMessage( String statement, MapValue params, MapValue meta ) throws Bol this.params = requireNonNull( params ); this.meta = requireNonNull( meta ); - this.bookmark = parseBookmark( meta ); + this.bookmark = Bookmark.fromParamsOrNull( meta ); this.txTimeout = parseTransactionTimeout( meta ); this.txMetadata = parseTransactionMetadata( meta ); } diff --git a/community/bolt/src/test/java/org/neo4j/bolt/v1/runtime/bookmarking/BookmarkTest.java b/community/bolt/src/test/java/org/neo4j/bolt/v1/runtime/bookmarking/BookmarkTest.java index 6c9c5b61bc4c..7bd3a7c36110 100644 --- a/community/bolt/src/test/java/org/neo4j/bolt/v1/runtime/bookmarking/BookmarkTest.java +++ b/community/bolt/src/test/java/org/neo4j/bolt/v1/runtime/bookmarking/BookmarkTest.java @@ -19,7 +19,7 @@ */ package org.neo4j.bolt.v1.runtime.bookmarking; -import org.junit.Test; +import org.junit.jupiter.api.Test; import org.neo4j.kernel.impl.util.ValueUtils; import org.neo4j.values.AnyValue; @@ -29,21 +29,15 @@ import static java.util.Arrays.asList; import static java.util.Collections.emptyList; -import static org.hamcrest.Matchers.instanceOf; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.fail; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; -public class BookmarkTest +class BookmarkTest { - private MapValue singletonMap( String key, Object value ) - { - return VirtualValues.map( new String[]{key}, new AnyValue[]{ValueUtils.of( value )} ); - } - @Test - public void shouldFormatAndParseSingleBookmarkContainingTransactionId() throws Exception + void shouldFormatAndParseSingleBookmarkContainingTransactionId() throws Exception { // given long txId = 1234; @@ -57,7 +51,7 @@ public void shouldFormatAndParseSingleBookmarkContainingTransactionId() throws E } @Test - public void shouldFormatAndParseMultipleBookmarksContainingTransactionId() throws Exception + void shouldFormatAndParseMultipleBookmarksContainingTransactionId() throws Exception { // given long txId1 = 1234; @@ -74,7 +68,7 @@ public void shouldFormatAndParseMultipleBookmarksContainingTransactionId() throw } @Test - public void shouldParseAndFormatSingleBookmarkContainingTransactionId() throws Exception + void shouldParseAndFormatSingleBookmarkContainingTransactionId() throws Exception { // given String expected = "neo4j:bookmark:v1:tx1234"; @@ -88,7 +82,7 @@ public void shouldParseAndFormatSingleBookmarkContainingTransactionId() throws E } @Test - public void shouldParseAndFormatMultipleBookmarkContainingTransactionId() throws Exception + void shouldParseAndFormatMultipleBookmarkContainingTransactionId() throws Exception { // given String txId1 = "neo4j:bookmark:v1:tx1234"; @@ -103,118 +97,76 @@ public void shouldParseAndFormatMultipleBookmarkContainingTransactionId() throws } @Test - public void shouldFailWhenParsingBadlyFormattedSingleBookmark() + void shouldFailWhenParsingBadlyFormattedSingleBookmark() { - // given String bookmarkString = "neo4q:markbook:v9:xt998"; - // when - try - { - Bookmark.fromParamsOrNull( singletonMap( "bookmark", bookmarkString ) ); - fail( "should have thrown exception" ); - } - catch ( Bookmark.BookmarkFormatException e ) - { - // I've been expecting you, Mr Bond. - } + BookmarkFormatException e = assertThrows( BookmarkFormatException.class, + () -> Bookmark.fromParamsOrNull( singletonMap( "bookmark", bookmarkString ) ) ); + + assertTrue( e.causesFailureMessage() ); } @Test - public void shouldFailWhenParsingBadlyFormattedMultipleBookmarks() + void shouldFailWhenParsingBadlyFormattedMultipleBookmarks() { - // given String bookmarkString = "neo4j:bookmark:v1:tx998"; String wrongBookmarkString = "neo4q:markbook:v9:xt998"; - // when - try - { - Bookmark.fromParamsOrNull( singletonMap( "bookmarks", asList( bookmarkString, wrongBookmarkString ) ) ); - fail( "should have thrown exception" ); - } - catch ( Bookmark.BookmarkFormatException e ) - { - // I've been expecting you, Mr Bond. - } + BookmarkFormatException e = assertThrows( BookmarkFormatException.class, + () -> Bookmark.fromParamsOrNull( singletonMap( "bookmarks", asList( bookmarkString, wrongBookmarkString ) ) ) ); + + assertTrue( e.causesFailureMessage() ); } @Test - public void shouldFailWhenNoNumberFollowsThePrefixInSingleBookmark() + void shouldFailWhenNoNumberFollowsThePrefixInSingleBookmark() { - // given String bookmarkString = "neo4j:bookmark:v1:tx"; - // when - try - { - Bookmark.fromParamsOrNull( singletonMap( "bookmark", bookmarkString ) ); - fail( "should have thrown exception" ); - } - catch ( Bookmark.BookmarkFormatException e ) - { - // I've been expecting you, Mr Bond. - } + BookmarkFormatException e = assertThrows( BookmarkFormatException.class, + () -> Bookmark.fromParamsOrNull( singletonMap( "bookmark", bookmarkString ) ) ); + + assertTrue( e.causesFailureMessage() ); } @Test - public void shouldFailWhenNoNumberFollowsThePrefixInMultipleBookmarks() + void shouldFailWhenNoNumberFollowsThePrefixInMultipleBookmarks() { - // given String bookmarkString = "neo4j:bookmark:v1:tx10"; String wrongBookmarkString = "neo4j:bookmark:v1:tx"; - // when - try - { - Bookmark.fromParamsOrNull( singletonMap( "bookmarks", asList( bookmarkString, wrongBookmarkString ) ) ); - fail( "should have thrown exception" ); - } - catch ( Bookmark.BookmarkFormatException e ) - { - // I've been expecting you, Mr Bond. - } + BookmarkFormatException e = assertThrows( BookmarkFormatException.class, + () -> Bookmark.fromParamsOrNull( singletonMap( "bookmarks", asList( bookmarkString, wrongBookmarkString ) ) ) ); + + assertTrue( e.causesFailureMessage() ); } @Test - public void shouldFailWhenSingleBookmarkHasExtraneousTrailingCharacters() + void shouldFailWhenSingleBookmarkHasExtraneousTrailingCharacters() { - // given String bookmarkString = "neo4j:bookmark:v1:tx1234supercalifragilisticexpialidocious"; - // when - try - { - Bookmark.fromParamsOrNull( singletonMap( "bookmark", bookmarkString ) ); - fail( "should have thrown exception" ); - } - catch ( Bookmark.BookmarkFormatException e ) - { - // I've been expecting you, Mr Bond. - } + BookmarkFormatException e = assertThrows( BookmarkFormatException.class, + () -> Bookmark.fromParamsOrNull( singletonMap( "bookmark", bookmarkString ) ) ); + + assertTrue( e.causesFailureMessage() ); } @Test - public void shouldFailWhenMultipleBookmarksHaveExtraneousTrailingCharacters() + void shouldFailWhenMultipleBookmarksHaveExtraneousTrailingCharacters() { - // given String bookmarkString = "neo4j:bookmark:v1:tx1234"; String wrongBookmarkString = "neo4j:bookmark:v1:tx1234supercalifragilisticexpialidocious"; - // when - try - { - Bookmark.fromParamsOrNull( singletonMap( "bookmarks", asList( bookmarkString, wrongBookmarkString ) ) ); - fail( "should have thrown exception" ); - } - catch ( Bookmark.BookmarkFormatException e ) - { - // I've been expecting you, Mr Bond. - } + BookmarkFormatException e = assertThrows( BookmarkFormatException.class, + () -> Bookmark.fromParamsOrNull( singletonMap( "bookmarks", asList( bookmarkString, wrongBookmarkString ) ) ) ); + + assertTrue( e.causesFailureMessage() ); } @Test - public void shouldUseMultipleBookmarksWhenGivenBothSingleAndMultiple() throws Exception + void shouldUseMultipleBookmarksWhenGivenBothSingleAndMultiple() throws Exception { MapValue params = params( "neo4j:bookmark:v1:tx42", @@ -226,7 +178,7 @@ public void shouldUseMultipleBookmarksWhenGivenBothSingleAndMultiple() throws Ex } @Test - public void shouldUseMultipleBookmarksWhenGivenOnlyMultiple() throws Exception + void shouldUseMultipleBookmarksWhenGivenOnlyMultiple() throws Exception { MapValue params = params( null, asList( "neo4j:bookmark:v1:tx85", "neo4j:bookmark:v1:tx47", "neo4j:bookmark:v1:tx15", "neo4j:bookmark:v1:tx6" ) ); @@ -237,7 +189,7 @@ public void shouldUseMultipleBookmarksWhenGivenOnlyMultiple() throws Exception } @Test - public void shouldUseSingleBookmarkWhenGivenOnlySingle() throws Exception + void shouldUseSingleBookmarkWhenGivenOnlySingle() throws Exception { MapValue params = params( "neo4j:bookmark:v1:tx82", null ); @@ -247,7 +199,7 @@ public void shouldUseSingleBookmarkWhenGivenOnlySingle() throws Exception } @Test - public void shouldUseSingleBookmarkWhenGivenBothSingleAndNullAsMultiple() throws Exception + void shouldUseSingleBookmarkWhenGivenBothSingleAndNullAsMultiple() throws Exception { MapValue params = params( "neo4j:bookmark:v1:tx58", null ); @@ -257,7 +209,7 @@ public void shouldUseSingleBookmarkWhenGivenBothSingleAndNullAsMultiple() throws } @Test - public void shouldUseSingleBookmarkWhenGivenBothSingleAndEmptyListAsMultiple() throws Exception + void shouldUseSingleBookmarkWhenGivenBothSingleAndEmptyListAsMultiple() throws Exception { MapValue params = params( "neo4j:bookmark:v1:tx67", emptyList() ); @@ -267,88 +219,64 @@ public void shouldUseSingleBookmarkWhenGivenBothSingleAndEmptyListAsMultiple() t } @Test - public void shouldThrowWhenMultipleBookmarksIsNotAList() + void shouldThrowWhenMultipleBookmarksIsNotAList() { MapValue params = params( "neo4j:bookmark:v1:tx67", new String[]{"neo4j:bookmark:v1:tx68"} ); - try - { - Bookmark.fromParamsOrNull( params ); - fail( "Exception expected" ); - } - catch ( Exception e ) - { - assertThat( e, instanceOf( Bookmark.BookmarkFormatException.class ) ); - } + BookmarkFormatException e = assertThrows( BookmarkFormatException.class, () -> Bookmark.fromParamsOrNull( params ) ); + + assertTrue( e.causesFailureMessage() ); } @Test - public void shouldThrowWhenMultipleBookmarksIsNotAListOfStrings() + void shouldThrowWhenMultipleBookmarksIsNotAListOfStrings() { MapValue params = params( "neo4j:bookmark:v1:tx67", asList( new String[]{"neo4j:bookmark:v1:tx50"}, new Object[]{"neo4j:bookmark:v1:tx89"} ) ); - try - { - Bookmark.fromParamsOrNull( params ); - fail( "Exception expected" ); - } - catch ( Exception e ) - { - assertThat( e, instanceOf( Bookmark.BookmarkFormatException.class ) ); - } + BookmarkFormatException e = assertThrows( BookmarkFormatException.class, () -> Bookmark.fromParamsOrNull( params ) ); + + assertTrue( e.causesFailureMessage() ); } @Test - public void shouldThrowWhenOneOfMultipleBookmarksIsMalformed() + void shouldThrowWhenOneOfMultipleBookmarksIsMalformed() { MapValue params = params( "neo4j:bookmark:v1:tx67", asList( "neo4j:bookmark:v1:tx99", "neo4j:bookmark:v1:tx12", "neo4j:bookmark:www:tx99" ) ); - try - { - Bookmark.fromParamsOrNull( params ); - fail( "Exception expected" ); - } - catch ( Exception e ) - { - assertThat( e, instanceOf( Bookmark.BookmarkFormatException.class ) ); - } + BookmarkFormatException e = assertThrows( BookmarkFormatException.class, () -> Bookmark.fromParamsOrNull( params ) ); + + assertTrue( e.causesFailureMessage() ); } @Test - public void shouldThrowWhenSingleBookmarkIsMalformed() + void shouldThrowWhenSingleBookmarkIsMalformed() { MapValue params = params( "neo4j:strange-bookmark:v1:tx6", null ); - try - { - Bookmark.fromParamsOrNull( params ); - fail( "Exception expected" ); - } - catch ( Exception e ) - { - assertThat( e, instanceOf( Bookmark.BookmarkFormatException.class ) ); - } + BookmarkFormatException e = assertThrows( BookmarkFormatException.class, () -> Bookmark.fromParamsOrNull( params ) ); + + assertTrue( e.causesFailureMessage() ); } @Test - public void shouldReturnNullWhenNoBookmarks() throws Exception + void shouldReturnNullWhenNoBookmarks() throws Exception { assertNull( Bookmark.fromParamsOrNull( VirtualValues.EMPTY_MAP ) ); } @Test - public void shouldReturnNullWhenGivenEmptyListForMultipleBookmarks() throws Exception + void shouldReturnNullWhenGivenEmptyListForMultipleBookmarks() throws Exception { MapValue params = params( null, emptyList() ); assertNull( Bookmark.fromParamsOrNull( params ) ); } @Test - public void shouldSkipNullsInMultipleBookmarks() throws Exception + void shouldSkipNullsInMultipleBookmarks() throws Exception { MapValue params = params( null, asList( "neo4j:bookmark:v1:tx3", "neo4j:bookmark:v1:tx5", null, "neo4j:bookmark:v1:tx17" ) ); @@ -368,4 +296,9 @@ private static MapValue params( String bookmark, Object bookmarks ) builder.add( "bookmarks", ValueUtils.of( bookmarks ) ); return builder.build(); } + + private static MapValue singletonMap( String key, Object value ) + { + return VirtualValues.map( new String[]{key}, new AnyValue[]{ValueUtils.of( value )} ); + } } diff --git a/community/bolt/src/test/java/org/neo4j/bolt/v3/messaging/request/MessageMetadataParserTest.java b/community/bolt/src/test/java/org/neo4j/bolt/v3/messaging/request/MessageMetadataParserTest.java new file mode 100644 index 000000000000..a4bb742a0bfa --- /dev/null +++ b/community/bolt/src/test/java/org/neo4j/bolt/v3/messaging/request/MessageMetadataParserTest.java @@ -0,0 +1,66 @@ +/* + * Copyright (c) 2002-2018 "Neo4j," + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.bolt.v3.messaging.request; + +import org.junit.jupiter.api.Test; + +import org.neo4j.bolt.messaging.BoltIOException; + +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.neo4j.bolt.v3.messaging.request.MessageMetadataParser.parseTransactionMetadata; +import static org.neo4j.bolt.v3.messaging.request.MessageMetadataParser.parseTransactionTimeout; +import static org.neo4j.helpers.collection.MapUtil.map; +import static org.neo4j.kernel.impl.util.ValueUtils.asMapValue; +import static org.neo4j.values.virtual.VirtualValues.emptyMap; + +class MessageMetadataParserTest +{ + @Test + void shouldAllowNoTransactionTimeout() throws Exception + { + assertNull( parseTransactionTimeout( emptyMap() ) ); + } + + @Test + void shouldAllowNoTransactionMetadata() throws Exception + { + assertNull( parseTransactionMetadata( emptyMap() ) ); + } + + @Test + void shouldThrowForIncorrectTransactionTimeout() + { + BoltIOException e = assertThrows( BoltIOException.class, + () -> parseTransactionTimeout( asMapValue( map( "tx_timeout", "15 minutes" ) ) ) ); + + assertTrue( e.causesFailureMessage() ); + } + + @Test + void shouldThrowForIncorrectTransactionMetadata() + { + BoltIOException e = assertThrows( BoltIOException.class, + () -> parseTransactionMetadata( asMapValue( map( "tx_metadata", "{key1: 'value1', key2: 'value2'}" ) ) ) ); + + assertTrue( e.causesFailureMessage() ); + } +} diff --git a/community/community-it/bolt-it/src/test/java/org/neo4j/bolt/v3/runtime/integration/BoltV3TransportIT.java b/community/community-it/bolt-it/src/test/java/org/neo4j/bolt/v3/runtime/integration/BoltV3TransportIT.java index e8cd49ebe98e..f76a06d7b62f 100644 --- a/community/community-it/bolt-it/src/test/java/org/neo4j/bolt/v3/runtime/integration/BoltV3TransportIT.java +++ b/community/community-it/bolt-it/src/test/java/org/neo4j/bolt/v3/runtime/integration/BoltV3TransportIT.java @@ -29,14 +29,17 @@ import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameter; +import java.io.IOException; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; +import org.neo4j.bolt.messaging.Neo4jPack; import org.neo4j.bolt.v1.messaging.request.DiscardAllMessage; import org.neo4j.bolt.v1.messaging.request.PullAllMessage; import org.neo4j.bolt.v1.messaging.request.ResetMessage; +import org.neo4j.bolt.v1.packstream.PackedOutputArray; import org.neo4j.bolt.v1.transport.integration.Neo4jWithSocket; import org.neo4j.bolt.v1.transport.integration.TransportTestUtil; import org.neo4j.bolt.v1.transport.socket.client.SecureSocketConnection; @@ -52,7 +55,6 @@ import org.neo4j.kernel.api.KernelTransactionHandle; import org.neo4j.kernel.api.exceptions.Status; import org.neo4j.kernel.impl.api.KernelTransactions; -import org.neo4j.kernel.impl.util.ValueUtils; import org.neo4j.kernel.internal.GraphDatabaseAPI; import org.neo4j.values.virtual.MapValue; @@ -78,8 +80,10 @@ import static org.neo4j.bolt.v3.messaging.request.RollbackMessage.ROLLBACK_MESSAGE; import static org.neo4j.graphdb.factory.GraphDatabaseSettings.auth_enabled; import static org.neo4j.helpers.collection.MapUtil.map; +import static org.neo4j.kernel.impl.util.ValueUtils.asMapValue; import static org.neo4j.values.storable.Values.longValue; import static org.neo4j.values.storable.Values.stringValue; +import static org.neo4j.values.virtual.VirtualValues.EMPTY_MAP; @RunWith( Parameterized.class ) public class BoltV3TransportIT @@ -386,7 +390,7 @@ public void shouldFailNicelyOnNullKeysInMap() throws Throwable // When negotiateBoltV3(); connection.send( util.chunk( - new RunMessage( "RETURN {p}", ValueUtils.asMapValue( params ) ), + new RunMessage( "RETURN {p}", asMapValue( params ) ), PullAllMessage.INSTANCE ) ); // Then @@ -428,7 +432,7 @@ public void shouldSetTxMetadata() throws Throwable negotiateBoltV3(); Map txMetadata = map( "who-is-your-boss", "Molly-mostly-white" ); Map msgMetadata = map( "tx_metadata", txMetadata ); - MapValue meta = ValueUtils.asMapValue( msgMetadata ); + MapValue meta = asMapValue( msgMetadata ); connection.send( util.chunk( new BeginMessage( meta ), @@ -453,13 +457,109 @@ public void shouldSetTxMetadata() throws Throwable connection.send( util.chunk( ROLLBACK_MESSAGE ) ); } + @Test + public void shouldSendFailureMessageForBeginWithInvalidBookmark() throws Exception + { + negotiateBoltV3(); + String bookmarkString = "Not a good bookmark for BEGIN"; + Map metadata = map( "bookmarks", singletonList( bookmarkString ) ); + + connection.send( util.chunk( 32, beginMessage( metadata ) ) ); + + assertThat( connection, util.eventuallyReceives( msgFailure( Status.Transaction.InvalidBookmark, bookmarkString ) ) ); + } + + @Test + public void shouldSendFailureMessageForBeginWithInvalidTransactionTimeout() throws Exception + { + negotiateBoltV3(); + String txTimeout = "Tx timeout can't be a string for BEGIN"; + Map metadata = map( "tx_timeout", txTimeout ); + + connection.send( util.chunk( 32, beginMessage( metadata ) ) ); + + assertThat( connection, util.eventuallyReceives( msgFailure( Status.Request.Invalid, txTimeout ) ) ); + } + + @Test + public void shouldSendFailureMessageForBeginWithInvalidTransactionMetadata() throws Exception + { + negotiateBoltV3(); + String txMetadata = "Tx metadata can't be a string for BEGIN"; + Map metadata = map( "tx_metadata", txMetadata ); + + connection.send( util.chunk( 32, beginMessage( metadata ) ) ); + + assertThat( connection, util.eventuallyReceives( msgFailure( Status.Request.Invalid, txMetadata ) ) ); + } + + @Test + public void shouldSendFailureMessageForRunWithInvalidBookmark() throws Exception + { + negotiateBoltV3(); + String bookmarkString = "Not a good bookmark for RUN"; + Map metadata = map( "bookmarks", singletonList( bookmarkString ) ); + + connection.send( util.chunk( 32, runMessage( metadata ) ) ); + + assertThat( connection, util.eventuallyReceives( msgFailure( Status.Transaction.InvalidBookmark, bookmarkString ) ) ); + } + + @Test + public void shouldSendFailureMessageForRunWithInvalidTransactionTimeout() throws Exception + { + negotiateBoltV3(); + String txTimeout = "Tx timeout can't be a string for RUN"; + Map metadata = map( "tx_timeout", txTimeout ); + + connection.send( util.chunk( 32, runMessage( metadata ) ) ); + + assertThat( connection, util.eventuallyReceives( msgFailure( Status.Request.Invalid, txTimeout ) ) ); + } + + @Test + public void shouldSendFailureMessageForRunWithInvalidTransactionMetadata() throws Exception + { + negotiateBoltV3(); + String txMetadata = "Tx metadata can't be a string for RUN"; + Map metadata = map( "tx_metadata", txMetadata ); + + connection.send( util.chunk( 32, runMessage( metadata ) ) ); + + assertThat( connection, util.eventuallyReceives( msgFailure( Status.Request.Invalid, txMetadata ) ) ); + } + private void negotiateBoltV3() throws Exception { connection.connect( address ) .send( util.acceptedVersions( 3, 0, 0, 0 ) ) - .send( util.chunk( new HelloMessage( MapUtil.map( "user_agent", USER_AGENT ) ) ) ); + .send( util.chunk( new HelloMessage( map( "user_agent", USER_AGENT ) ) ) ); assertThat( connection, eventuallyReceives( new byte[]{0, 0, 0, 3} ) ); assertThat( connection, util.eventuallyReceives( msgSuccess() ) ); } + + private byte[] beginMessage( Map metadata ) throws IOException + { + PackedOutputArray out = new PackedOutputArray(); + Neo4jPack.Packer packer = util.getNeo4jPack().newPacker( out ); + + packer.packStructHeader( 1, BeginMessage.SIGNATURE ); + packer.pack( asMapValue( metadata ) ); + + return out.bytes(); + } + + private byte[] runMessage( Map metadata ) throws IOException + { + PackedOutputArray out = new PackedOutputArray(); + Neo4jPack.Packer packer = util.getNeo4jPack().newPacker( out ); + + packer.packStructHeader( 3, RunMessage.SIGNATURE ); + packer.pack( "RETURN 1" ); + packer.pack( EMPTY_MAP ); + packer.pack( asMapValue( metadata ) ); + + return out.bytes(); + } }