From 7c098649e29164b89adfd3839f248fc413be89a8 Mon Sep 17 00:00:00 2001 From: grant lodge <6323995+thelonelyvulpes@users.noreply.github.com> Date: Wed, 1 Jun 2022 15:29:47 +0100 Subject: [PATCH 1/3] remove null handling for nodes and relationships --- .../neo4j/driver/internal/InternalEntity.java | 15 +-------- .../neo4j/driver/internal/InternalNode.java | 7 ++-- .../driver/internal/InternalRelationship.java | 9 ++--- .../messaging/common/CommonValueUnpacker.java | 7 ++-- .../messaging/v5/ValueUnpackerV5.java | 33 +++++++------------ .../internal/packstream/PackStream.java | 21 ------------ .../driver/internal/InternalNodeTest.java | 12 +------ .../internal/InternalRelationshipTest.java | 33 ------------------- .../neo4j/driver/types/TypeSystemTest.java | 5 ++- 9 files changed, 24 insertions(+), 118 deletions(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/InternalEntity.java b/driver/src/main/java/org/neo4j/driver/internal/InternalEntity.java index 7779298d30..5b23cafec7 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/InternalEntity.java +++ b/driver/src/main/java/org/neo4j/driver/internal/InternalEntity.java @@ -36,18 +36,15 @@ public abstract class InternalEntity implements Entity, AsValue { private final long id; private final String elementId; private final Map properties; - private final boolean numericIdAvailable; - public InternalEntity(long id, String elementId, Map properties, boolean numericIdAvailable) { + public InternalEntity(long id, String elementId, Map properties) { this.id = id; this.elementId = elementId; this.properties = properties; - this.numericIdAvailable = numericIdAvailable; } @Override public long id() { - assertNumericIdAvailable(); return id; } @@ -125,14 +122,4 @@ public Iterable values() { public Iterable values(Function mapFunction) { return Iterables.map(properties.values(), mapFunction); } - - protected void assertNumericIdAvailable() { - if (!numericIdAvailable) { - throw new IllegalStateException(INVALID_ID_ERROR); - } - } - - public boolean isNumericIdAvailable() { - return numericIdAvailable; - } } diff --git a/driver/src/main/java/org/neo4j/driver/internal/InternalNode.java b/driver/src/main/java/org/neo4j/driver/internal/InternalNode.java index 1f77d2807c..db268659b4 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/InternalNode.java +++ b/driver/src/main/java/org/neo4j/driver/internal/InternalNode.java @@ -36,16 +36,15 @@ public InternalNode(long id) { } public InternalNode(long id, Collection labels, Map properties) { - this(id, String.valueOf(id), labels, properties, true); + this(id, String.valueOf(id), labels, properties); } public InternalNode( long id, String elementId, Collection labels, - Map properties, - boolean numericIdAvailable) { - super(id, elementId, properties, numericIdAvailable); + Map properties) { + super(id, elementId, properties); this.labels = labels; } diff --git a/driver/src/main/java/org/neo4j/driver/internal/InternalRelationship.java b/driver/src/main/java/org/neo4j/driver/internal/InternalRelationship.java index a250d90746..41aec3e3a8 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/InternalRelationship.java +++ b/driver/src/main/java/org/neo4j/driver/internal/InternalRelationship.java @@ -39,7 +39,7 @@ public InternalRelationship(long id, long start, long end, String type) { } public InternalRelationship(long id, long start, long end, String type, Map properties) { - this(id, String.valueOf(id), start, String.valueOf(start), end, String.valueOf(end), type, properties, true); + this(id, String.valueOf(id), start, String.valueOf(start), end, String.valueOf(end), type, properties); } public InternalRelationship( @@ -50,9 +50,8 @@ public InternalRelationship( long end, String endElementId, String type, - Map properties, - boolean numericIdAvailable) { - super(id, elementId, properties, numericIdAvailable); + Map properties) { + super(id, elementId, properties); this.start = start; this.startElementId = startElementId; this.end = end; @@ -77,7 +76,6 @@ public void setStartAndEnd(long start, String startElementId, long end, String e @Override public long startNodeId() { - assertNumericIdAvailable(); return start; } @@ -88,7 +86,6 @@ public String startNodeElementId() { @Override public long endNodeId() { - assertNumericIdAvailable(); return end; } diff --git a/driver/src/main/java/org/neo4j/driver/internal/messaging/common/CommonValueUnpacker.java b/driver/src/main/java/org/neo4j/driver/internal/messaging/common/CommonValueUnpacker.java index 4a722830b6..a165e9bd15 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/messaging/common/CommonValueUnpacker.java +++ b/driver/src/main/java/org/neo4j/driver/internal/messaging/common/CommonValueUnpacker.java @@ -226,8 +226,7 @@ protected Value unpackRelationship() throws IOException { endUrn, String.valueOf(endUrn), relType, - props, - true); + props); return new RelationshipValue(adapted); } @@ -246,7 +245,7 @@ protected InternalNode unpackNode() throws IOException { props.put(key, unpack()); } - return new InternalNode(urn, String.valueOf(urn), labels, props, true); + return new InternalNode(urn, String.valueOf(urn), labels, props); } protected Value unpackPath() throws IOException { @@ -268,7 +267,7 @@ protected Value unpackPath() throws IOException { String relType = unpacker.unpackString(); Map props = unpackMap(); uniqRels[i] = new InternalRelationship( - id, String.valueOf(id), -1, String.valueOf(-1), -1, String.valueOf(-1), relType, props, true); + id, String.valueOf(id), -1, String.valueOf(-1), -1, String.valueOf(-1), relType, props); } // Path sequence diff --git a/driver/src/main/java/org/neo4j/driver/internal/messaging/v5/ValueUnpackerV5.java b/driver/src/main/java/org/neo4j/driver/internal/messaging/v5/ValueUnpackerV5.java index d01349905a..53f5ee00c3 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/messaging/v5/ValueUnpackerV5.java +++ b/driver/src/main/java/org/neo4j/driver/internal/messaging/v5/ValueUnpackerV5.java @@ -57,7 +57,7 @@ protected int getRelationshipFields() { @Override protected InternalNode unpackNode() throws IOException { - Long urn = unpacker.unpackLongOrNull(); + long urn = unpacker.unpackLong(); int numLabels = (int) unpacker.unpackListHeader(); List labels = new ArrayList<>(numLabels); @@ -73,9 +73,7 @@ protected InternalNode unpackNode() throws IOException { String elementId = unpacker.unpackString(); - return urn == null - ? new InternalNode(-1, elementId, labels, props, false) - : new InternalNode(urn, elementId, labels, props, true); + return new InternalNode(urn, elementId, labels, props); } @Override @@ -94,15 +92,12 @@ protected Value unpackPath() throws IOException { ensureCorrectStructSize(TypeConstructor.RELATIONSHIP, 4, unpacker.unpackStructHeader()); ensureCorrectStructSignature( "UNBOUND_RELATIONSHIP", UNBOUND_RELATIONSHIP, unpacker.unpackStructSignature()); - Long id = unpacker.unpackLongOrNull(); + long id = unpacker.unpackLong(); String relType = unpacker.unpackString(); Map props = unpackMap(); String elementId = unpacker.unpackString(); - uniqRels[i] = id == null - ? new InternalRelationship( - -1, elementId, -1, String.valueOf(-1), -1, String.valueOf(-1), relType, props, false) - : new InternalRelationship( - id, elementId, -1, String.valueOf(-1), -1, String.valueOf(-1), relType, props, true); + uniqRels[i] = new InternalRelationship( + id, elementId, -1, String.valueOf(-1), -1, String.valueOf(-1), relType, props); } // Path sequence @@ -138,28 +133,22 @@ protected Value unpackPath() throws IOException { } private void setStartAndEnd(InternalRelationship rel, InternalNode start, InternalNode end) { - if (rel.isNumericIdAvailable() && start.isNumericIdAvailable() && end.isNumericIdAvailable()) { - rel.setStartAndEnd(start.id(), start.elementId(), end.id(), end.elementId()); - } else { - rel.setStartAndEnd(-1, start.elementId(), -1, end.elementId()); - } + rel.setStartAndEnd(start.id(), start.elementId(), end.id(), end.elementId()); } @Override protected Value unpackRelationship() throws IOException { - Long urn = unpacker.unpackLongOrNull(); - Long startUrn = unpacker.unpackLongOrNull(); - Long endUrn = unpacker.unpackLongOrNull(); + long urn = unpacker.unpackLong(); + long startUrn = unpacker.unpackLong(); + long endUrn = unpacker.unpackLong(); String relType = unpacker.unpackString(); Map props = unpackMap(); String elementId = unpacker.unpackString(); String startElementId = unpacker.unpackString(); String endElementId = unpacker.unpackString(); - InternalRelationship adapted = urn == null - ? new InternalRelationship(-1, elementId, -1, startElementId, -1, endElementId, relType, props, false) - : new InternalRelationship( - urn, elementId, startUrn, startElementId, endUrn, endElementId, relType, props, true); + InternalRelationship adapted = new InternalRelationship( + urn, elementId, startUrn, startElementId, endUrn, endElementId, relType, props); return new RelationshipValue(adapted); } } diff --git a/driver/src/main/java/org/neo4j/driver/internal/packstream/PackStream.java b/driver/src/main/java/org/neo4j/driver/internal/packstream/PackStream.java index ead9cab5b5..5af4f15490 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/packstream/PackStream.java +++ b/driver/src/main/java/org/neo4j/driver/internal/packstream/PackStream.java @@ -404,27 +404,6 @@ public long unpackMapHeader() throws IOException { } } - public Long unpackLongOrNull() throws IOException { - final byte markerByte = in.readByte(); - if (markerByte >= MINUS_2_TO_THE_4) { - return (long) markerByte; - } - switch (markerByte) { - case INT_8: - return (long) in.readByte(); - case INT_16: - return (long) in.readShort(); - case INT_32: - return (long) in.readInt(); - case INT_64: - return in.readLong(); - case NULL: - return null; - default: - throw new Unexpected("Expected an integer, but got: " + toHexString(markerByte)); - } - } - public long unpackLong() throws IOException { final byte markerByte = in.readByte(); if (markerByte >= MINUS_2_TO_THE_4) { diff --git a/driver/src/test/java/org/neo4j/driver/internal/InternalNodeTest.java b/driver/src/test/java/org/neo4j/driver/internal/InternalNodeTest.java index 64db23700e..0e6dc75923 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/InternalNodeTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/InternalNodeTest.java @@ -60,20 +60,10 @@ void accessUnknownKeyShouldBeNull() { assertThat(node.get("k3"), equalTo(NULL)); } - @Test - void shouldThrowOnIdWhenNumericIdUnavailable() { - // GIVEN - InternalNode node = new InternalNode(-1, "value", Collections.emptyList(), Collections.emptyMap(), false); - - // WHEN & THEN - IllegalStateException e = assertThrows(IllegalStateException.class, node::id); - assertEquals(InternalEntity.INVALID_ID_ERROR, e.getMessage()); - } - private InternalNode createNode() { Map props = new HashMap<>(); props.put("k1", value(1)); props.put("k2", value(2)); - return new InternalNode(42L, String.valueOf(42L), Collections.singletonList("L"), props, true); + return new InternalNode(42L, String.valueOf(42L), Collections.singletonList("L"), props); } } diff --git a/driver/src/test/java/org/neo4j/driver/internal/InternalRelationshipTest.java b/driver/src/test/java/org/neo4j/driver/internal/InternalRelationshipTest.java index a075e91f74..3c85c27e88 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/InternalRelationshipTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/InternalRelationshipTest.java @@ -60,39 +60,6 @@ void accessUnknownKeyShouldBeNull() { assertThat(relationship.get("k3"), equalTo(NULL)); } - @Test - void shouldThrowOnIdWhenNumericIdUnavailable() { - // GIVEN - InternalRelationship relationship = new InternalRelationship( - -1, "value", 1, String.valueOf(1), 2, String.valueOf(2), "T", Collections.emptyMap(), false); - - // WHEN & THEN - IllegalStateException e = assertThrows(IllegalStateException.class, relationship::id); - assertEquals(InternalEntity.INVALID_ID_ERROR, e.getMessage()); - } - - @Test - void shouldThrowOnStartNodeIdWhenNumericIdUnavailable() { - // GIVEN - InternalRelationship relationship = new InternalRelationship( - -1, "value", 1, String.valueOf(1), 2, String.valueOf(2), "T", Collections.emptyMap(), false); - - // WHEN & THEN - IllegalStateException e = assertThrows(IllegalStateException.class, relationship::startNodeId); - assertEquals(InternalEntity.INVALID_ID_ERROR, e.getMessage()); - } - - @Test - void shouldThrowOnEndNodeIdWhenNumericIdUnavailable() { - // GIVEN - InternalRelationship relationship = new InternalRelationship( - -1, "value", 1, String.valueOf(1), 2, String.valueOf(2), "T", Collections.emptyMap(), false); - - // WHEN & THEN - IllegalStateException e = assertThrows(IllegalStateException.class, relationship::endNodeId); - assertEquals(InternalEntity.INVALID_ID_ERROR, e.getMessage()); - } - private InternalRelationship createRelationship() { Map props = new HashMap<>(); props.put("k1", value(1)); diff --git a/driver/src/test/java/org/neo4j/driver/types/TypeSystemTest.java b/driver/src/test/java/org/neo4j/driver/types/TypeSystemTest.java index 6fe6d66eb1..dd776b558a 100644 --- a/driver/src/test/java/org/neo4j/driver/types/TypeSystemTest.java +++ b/driver/src/test/java/org/neo4j/driver/types/TypeSystemTest.java @@ -43,7 +43,7 @@ class TypeSystemTest { private final InternalNode node = - new InternalNode(42L, String.valueOf(42L), Collections.emptyList(), Collections.emptyMap(), true); + new InternalNode(42L, String.valueOf(42L), Collections.emptyList(), Collections.emptyMap()); private final InternalRelationship relationship = new InternalRelationship( 42L, String.valueOf(42L), @@ -52,8 +52,7 @@ class TypeSystemTest { 43L, String.valueOf(43L), "T", - Collections.emptyMap(), - true); + Collections.emptyMap()); private Value integerValue = value(13); private Value floatValue = value(13.1); From 035f718b40debd8d1c783b527966a204d1e60ea8 Mon Sep 17 00:00:00 2001 From: grant lodge <6323995+thelonelyvulpes@users.noreply.github.com> Date: Wed, 6 Jul 2022 09:35:38 +0100 Subject: [PATCH 2/3] Remove unused feature --- .../neo4j/org/testkit/backend/messages/requests/GetFeatures.java | 1 - 1 file changed, 1 deletion(-) diff --git a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/GetFeatures.java b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/GetFeatures.java index 5d00a740f7..6334b6e0c0 100644 --- a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/GetFeatures.java +++ b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/GetFeatures.java @@ -55,7 +55,6 @@ public class GetFeatures implements TestkitRequest { "Feature:API:Driver.IsEncrypted", "Feature:API:SSLConfig", "Detail:DefaultSecurityConfigValueEquality", - "Detail:ThrowOnMissingId", "Optimization:ImplicitDefaultArguments", "Feature:Bolt:Patch:UTC", "Feature:API:Type.Temporal")); From 4678f38ab324b01f420480ebae9a6841ce638dfa Mon Sep 17 00:00:00 2001 From: Rouven Bauer Date: Wed, 6 Jul 2022 13:29:50 +0200 Subject: [PATCH 3/3] Code style --- .../java/org/neo4j/driver/internal/InternalNode.java | 6 +----- .../driver/internal/messaging/v5/ValueUnpackerV5.java | 4 ++-- .../java/org/neo4j/driver/internal/InternalNodeTest.java | 2 -- .../neo4j/driver/internal/InternalRelationshipTest.java | 3 --- .../test/java/org/neo4j/driver/types/TypeSystemTest.java | 9 +-------- 5 files changed, 4 insertions(+), 20 deletions(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/InternalNode.java b/driver/src/main/java/org/neo4j/driver/internal/InternalNode.java index db268659b4..71195a504f 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/InternalNode.java +++ b/driver/src/main/java/org/neo4j/driver/internal/InternalNode.java @@ -39,11 +39,7 @@ public InternalNode(long id, Collection labels, Map prope this(id, String.valueOf(id), labels, properties); } - public InternalNode( - long id, - String elementId, - Collection labels, - Map properties) { + public InternalNode(long id, String elementId, Collection labels, Map properties) { super(id, elementId, properties); this.labels = labels; } diff --git a/driver/src/main/java/org/neo4j/driver/internal/messaging/v5/ValueUnpackerV5.java b/driver/src/main/java/org/neo4j/driver/internal/messaging/v5/ValueUnpackerV5.java index 393b87be15..2d920c8b5a 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/messaging/v5/ValueUnpackerV5.java +++ b/driver/src/main/java/org/neo4j/driver/internal/messaging/v5/ValueUnpackerV5.java @@ -97,7 +97,7 @@ protected Value unpackPath() throws IOException { Map props = unpackMap(); String elementId = unpacker.unpackString(); uniqRels[i] = new InternalRelationship( - id, elementId, -1, String.valueOf(-1), -1, String.valueOf(-1), relType, props); + id, elementId, -1, String.valueOf(-1), -1, String.valueOf(-1), relType, props); } // Path sequence @@ -148,7 +148,7 @@ protected Value unpackRelationship() throws IOException { String endElementId = unpacker.unpackString(); InternalRelationship adapted = new InternalRelationship( - urn, elementId, startUrn, startElementId, endUrn, endElementId, relType, props); + urn, elementId, startUrn, startElementId, endUrn, endElementId, relType, props); return new RelationshipValue(adapted); } } diff --git a/driver/src/test/java/org/neo4j/driver/internal/InternalNodeTest.java b/driver/src/test/java/org/neo4j/driver/internal/InternalNodeTest.java index 0e6dc75923..0cfc009264 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/InternalNodeTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/InternalNodeTest.java @@ -20,9 +20,7 @@ import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.junit.MatcherAssert.assertThat; -import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.neo4j.driver.Values.NULL; import static org.neo4j.driver.Values.value; diff --git a/driver/src/test/java/org/neo4j/driver/internal/InternalRelationshipTest.java b/driver/src/test/java/org/neo4j/driver/internal/InternalRelationshipTest.java index 3c85c27e88..5d156d2bb9 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/InternalRelationshipTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/InternalRelationshipTest.java @@ -20,13 +20,10 @@ import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.junit.MatcherAssert.assertThat; -import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.neo4j.driver.Values.NULL; import static org.neo4j.driver.Values.value; -import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.Map; diff --git a/driver/src/test/java/org/neo4j/driver/types/TypeSystemTest.java b/driver/src/test/java/org/neo4j/driver/types/TypeSystemTest.java index dd776b558a..2269cd9ae9 100644 --- a/driver/src/test/java/org/neo4j/driver/types/TypeSystemTest.java +++ b/driver/src/test/java/org/neo4j/driver/types/TypeSystemTest.java @@ -45,14 +45,7 @@ class TypeSystemTest { private final InternalNode node = new InternalNode(42L, String.valueOf(42L), Collections.emptyList(), Collections.emptyMap()); private final InternalRelationship relationship = new InternalRelationship( - 42L, - String.valueOf(42L), - 42L, - String.valueOf(42L), - 43L, - String.valueOf(43L), - "T", - Collections.emptyMap()); + 42L, String.valueOf(42L), 42L, String.valueOf(42L), 43L, String.valueOf(43L), "T", Collections.emptyMap()); private Value integerValue = value(13); private Value floatValue = value(13.1);