Skip to content

Add support to 4.2 by implementing the extended handshake #798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 9, 2020
Merged

Add support to 4.2 by implementing the extended handshake #798

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
537c102
Add suport to 4.2 by implementign the extended handshake
bigmontz Dec 8, 2020
859a784
Apply suggestions from code review
bigmontz Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions driver/src/main/java/org/neo4j/driver/internal/async/connection/BoltProtocolUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ public final class BoltProtocolUtil

private static final ByteBuf HANDSHAKE_BUF = unreleasableBuffer( copyInt(
BOLT_MAGIC_PREAMBLE,
BoltProtocolV43.VERSION.toInt(),
BoltProtocolV42.VERSION.toInt(),
BoltProtocolV43.VERSION.toIntRange( BoltProtocolV42.VERSION ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just wondering if we should push the range option to the bottom for 4.3 -> 4.2. So the handshake is 4.3, 4.2, 4.1-> 4.0, 3. When 4.3 driver talks to 4.2 server it would skip over 4.2 and negotiate 4.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we will lose the multi database support on the version 4.0, since i will negotiate as 3.5.
Uses 4.1 to talk with a 4.2.x server will not represent any feature drop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good point. I guess we just have to be a little careful whilst we switch to the new handshake

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a comment on the protocol definition document, to fix the example to consider the version range in decreasing order.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea that we should go to BoltProtocolV43.VERSION.toIntRange(BoltProtocolVersionV40.VERSION)? That way we are saying all version 4 protocols are accepted by this driver and save 2 slots in the handshake.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be done in the protocol, but I think we should keep using the slots for the 4.0 and 4.1, just in case we connect to a non-update server which do not know how to interpret the extend handshake.

BoltProtocolV41.VERSION.toInt(),
BoltProtocolV4.VERSION.toInt(),
BoltProtocolV3.VERSION.toInt() ) ).asReadOnly();

private static final String HANDSHAKE_STRING = createHandshakeString();
Expand Down
15 changes: 15 additions & 0 deletions driver/src/main/java/org/neo4j/driver/internal/messaging/BoltProtocolVersion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,21 @@ public int toInt()
return shiftedMinor | majorVersion;
}

public int toIntRange(BoltProtocolVersion minVersion)
{
if(majorVersion != minVersion.majorVersion)
{
throw new IllegalArgumentException( "Versions should be from the same version" );
}
else if (minorVersion < minVersion.minorVersion)
{
throw new IllegalArgumentException("Max version should be newest than min version");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I know its less consistently applied in the driver but can you format the changes using the server checkstyles

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've did a format on the file, what's off in the current format?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the space before and after the bracket : throw new IllegalArgumentException( "Max version should be newest than min version" );

}
int range = minorVersion - minVersion.minorVersion;
int shiftedRange = range << 16;
return shiftedRange | toInt();
}

/**
* @return the version in format X.Y where X is the major version and Y is the minor version
*/
Expand Down
8 changes: 4 additions & 4 deletions driver/src/test/java/org/neo4j/driver/internal/async/connection/BoltProtocolUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@
import org.junit.jupiter.api.Test;

import org.neo4j.driver.internal.messaging.v3.BoltProtocolV3;
import org.neo4j.driver.internal.messaging.v4.BoltProtocolV4;
import org.neo4j.driver.internal.messaging.v41.BoltProtocolV41;
import org.neo4j.driver.internal.messaging.v42.BoltProtocolV42;
import org.neo4j.driver.internal.messaging.v43.BoltProtocolV43;

import static org.junit.jupiter.api.Assertions.assertEquals;
Expand All @@ -43,15 +43,15 @@ void shouldReturnHandshakeBuf()
{
assertByteBufContains(
handshakeBuf(),
BOLT_MAGIC_PREAMBLE, BoltProtocolV43.VERSION.toInt(), BoltProtocolV42.VERSION.toInt(),
BoltProtocolV41.VERSION.toInt(), BoltProtocolV3.VERSION.toInt()
BOLT_MAGIC_PREAMBLE, (1 << 16) | BoltProtocolV43.VERSION.toInt(), BoltProtocolV41.VERSION.toInt(),
BoltProtocolV4.VERSION.toInt(), BoltProtocolV3.VERSION.toInt()
);
}

@Test
void shouldReturnHandshakeString()
{
assertEquals( "[0x6060b017, 772, 516, 260, 3]", handshakeString() );
assertEquals( "[0x6060b017, 66308, 260, 4, 3]", handshakeString() );
}

@Test
Expand Down
35 changes: 35 additions & 0 deletions driver/src/test/java/org/neo4j/driver/internal/messaging/BoltProtocolVersionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.junit.jupiter.params.provider.CsvSource;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

class BoltProtocolVersionTest
{
Expand Down Expand Up @@ -50,6 +51,40 @@ void shouldCompareTo(int majorA, int minorA, int majorB, int minorB, int expecte

}

@ParameterizedTest( name = "V{0}.{1} toIntRange V{2}.{3}")
@CsvSource({
"1, 0, 1, 0, 0x000001",
"4, 3, 4, 2, 0x010304",
"4, 3, 4, 1, 0x020304",
"4, 3, 4, 0, 0x030304",
"100, 100, 100, 0, 0x646464",
"255, 255, 255, 0, 0xFFFFFF"
} )
void shouldOutputCorrectIntRange(int majorA, int minorA, int majorB, int minorB, int expectedResult)
{
BoltProtocolVersion versionA = new BoltProtocolVersion( majorA, minorA );
BoltProtocolVersion versionB = new BoltProtocolVersion( majorB, minorB );

assertEquals( expectedResult, versionA.toIntRange( versionB ) );
}

@ParameterizedTest( name = "V{0}.{1} toIntRange V{2}.{3}")
@CsvSource({
"1, 0, 2, 0",
"2, 0, 1, 0",
"4, 3, 4, 5",
"4, 6, 3, 7",
"3, 7, 4, 6",
"255, 255, 100, 0"
} )
void shouldThrowsIllegalArgumentExceptionForIncorrectIntRange(int majorA, int minorA, int majorB, int minorB)
{
BoltProtocolVersion versionA = new BoltProtocolVersion( majorA, minorA );
BoltProtocolVersion versionB = new BoltProtocolVersion( majorB, minorB );

assertThrows( IllegalArgumentException.class, () -> versionA.toIntRange( versionB ));
}

@Test
void shouldOutputCorrectLongFormatForMajorVersionOnly()
{
Expand Down