Skip to content

Adding error check of unsupported types #482

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 3 commits into from
Apr 9, 2018
Merged

Adding error check of unsupported types #482

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
13a4cfd
Adding error check of unsupported types
Apr 3, 2018
af8c5b7
Adding more checks around the allowed input parameter types
Apr 4, 2018
76afa48
Added the server side packing logic for node, rel, and path back in t…
Apr 5, 2018
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
7 changes: 7 additions & 0 deletions driver/src/main/java/org/neo4j/driver/v1/Values.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,11 @@
import org.neo4j.driver.internal.value.LocalDateTimeValue;
import org.neo4j.driver.internal.value.LocalTimeValue;
import org.neo4j.driver.internal.value.MapValue;
import org.neo4j.driver.internal.value.NodeValue;
import org.neo4j.driver.internal.value.NullValue;
import org.neo4j.driver.internal.value.PathValue;
import org.neo4j.driver.internal.value.PointValue;
import org.neo4j.driver.internal.value.RelationshipValue;
import org.neo4j.driver.internal.value.StringValue;
import org.neo4j.driver.internal.value.TimeValue;
import org.neo4j.driver.v1.exceptions.ClientException;
Expand Down Expand Up @@ -91,6 +94,10 @@ public static Value value( Object value )
{
if ( value == null ) { return NullValue.NULL; }

if ( value instanceof NodeValue || value instanceof RelationshipValue || value instanceof PathValue )
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will be better to prohibit node, relationship and path in PackStreamMessageFormatV1. It is clear that struct packing code is the single point that writes structs to the channel. While Values#value() is just one of many factory methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason to do it outside/earlier is because when we got an error there, we will kill the connection, which seems too aggressive. So if we could found the error in advance, then we will simply pop an error to the surface.

{
throw new ClientException( "Unsupported param type " + ((AsValue) value).asValue().type().name() );
}
if ( value instanceof AsValue ) { return ((AsValue) value).asValue(); }
if ( value instanceof Boolean ) { return value( (boolean) value ); }
if ( value instanceof String ) { return value( (String) value ); }
Expand Down
42 changes: 42 additions & 0 deletions driver/src/test/java/org/neo4j/driver/internal/ValuesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@
import org.neo4j.driver.internal.value.LocalDateTimeValue;
import org.neo4j.driver.internal.value.LocalTimeValue;
import org.neo4j.driver.internal.value.MapValue;
import org.neo4j.driver.internal.value.NodeValue;
import org.neo4j.driver.internal.value.PathValue;
import org.neo4j.driver.internal.value.RelationshipValue;
import org.neo4j.driver.internal.value.TimeValue;
import org.neo4j.driver.v1.Value;
import org.neo4j.driver.v1.Values;
Expand All @@ -60,6 +63,9 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertThat;
import static org.neo4j.driver.internal.value.NodeValueTest.emptyNodeValue;
import static org.neo4j.driver.internal.value.PathValueTest.pathValue;
import static org.neo4j.driver.internal.value.RelationshipValueTest.emptyRelationshipValue;
import static org.neo4j.driver.v1.Values.isoDuration;
import static org.neo4j.driver.v1.Values.ofDouble;
import static org.neo4j.driver.v1.Values.ofFloat;
Expand Down Expand Up @@ -483,4 +489,40 @@ public void shouldCreateValueFromPoint3D()
assertEquals( point3D, point3DValue2.asPoint() );
assertEquals( point3DValue1, point3DValue2 );
}

@Test
public void shouldComplainAboutNodeType() throws Throwable
{
// Expect
exception.expect( ClientException.class );
exception.expectMessage( "Unsupported param type NODE" );

// When
NodeValue node = emptyNodeValue();
value( node );
}

@Test
public void shouldComplainAboutRelationshipType() throws Throwable
{
// Expect
exception.expect( ClientException.class );
exception.expectMessage( "Unsupported param type RELATIONSHIP" );

// When
RelationshipValue rel = emptyRelationshipValue();
value( rel );
}

@Test
public void shouldComplainAboutPathType() throws Throwable
{
// Expect
exception.expect( ClientException.class );
exception.expectMessage( "Unsupported param type PATH" );

// When
PathValue path = pathValue();
value( path );
}
}
4 changes: 2 additions & 2 deletions driver/src/test/java/org/neo4j/driver/internal/value/NodeValueTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,12 @@ public void shouldTypeAsNode()
assertThat( value.typeConstructor(), equalTo( TypeConstructor.NODE ) );
}

private NodeValue emptyNodeValue()
public static NodeValue emptyNodeValue()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please extract NodeValueTest#emptyNodeValue(), PathValueTest#pathValue() and RelationshipValueTest#emptyRelationshipValue() into a test utility class. Alternatively ValuesTest could just create it's own node, relationship and path values.

{
return new NodeValue( new InternalNode( 1234, singletonList( "User" ), new HashMap<String, Value>() ) );
}

private NodeValue filledNodeValue()
private static NodeValue filledNodeValue()
{
return new NodeValue( new InternalNode( 1234, singletonList( "User" ), singletonMap( "name", value( "Dodo" ) ) ) );
}
Expand Down
2 changes: 1 addition & 1 deletion driver/src/test/java/org/neo4j/driver/internal/value/PathValueTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public void shouldHaveCorrectType() throws Throwable
assertThat(pathValue().type(), equalTo( InternalTypeSystem.TYPE_SYSTEM.PATH() ));
}

private PathValue pathValue()
public static PathValue pathValue()
{
return new PathValue( new InternalPath( new InternalNode(42L), new InternalRelationship( 43L, 42L, 44L, "T" ), new InternalNode( 44L ) ) );
}
Expand Down
4 changes: 2 additions & 2 deletions driver/src/test/java/org/neo4j/driver/internal/value/RelationshipValueTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,12 @@ public void shouldTypeAsRelationship()
assertThat( value.typeConstructor(), equalTo( TypeConstructor.RELATIONSHIP ) );
}

private RelationshipValue emptyRelationshipValue()
public static RelationshipValue emptyRelationshipValue()
{
return new RelationshipValue( new InternalRelationship( 1234, 1, 2, "KNOWS" ) );
}

private RelationshipValue filledRelationshipValue()
private static RelationshipValue filledRelationshipValue()
{
return new RelationshipValue( new InternalRelationship( 1234, 1, 2, "KNOWS", singletonMap( "name", value( "Dodo" ) ) ) );
}
Expand Down