From 46d4e3014c7041761609be05bc1ec6d6ac8ce9a6 Mon Sep 17 00:00:00 2001 From: Pontus Melke Date: Mon, 30 Oct 2017 09:50:14 +0100 Subject: [PATCH] Removed leak of values into public API We messed up and manage to leak `Value` into a public API. Let's remove as quick as possible. --- .../unsafe/batchinsert/BatchInserter.java | 5 +-- .../internal/BatchInserterImpl.java | 10 ++--- .../FileSystemClosingBatchInserter.java | 5 +-- .../batchinsert/internal/BatchInsertTest.java | 37 ++++++++----------- 4 files changed, 25 insertions(+), 32 deletions(-) diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/BatchInserter.java b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/BatchInserter.java index b0786f9efad6d..d923bb1260ab2 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/BatchInserter.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/BatchInserter.java @@ -25,7 +25,6 @@ import org.neo4j.graphdb.RelationshipType; import org.neo4j.graphdb.schema.ConstraintCreator; import org.neo4j.graphdb.schema.IndexCreator; -import org.neo4j.values.storable.Value; /** * The batch inserter drops support for transactions and concurrency in favor @@ -162,7 +161,7 @@ void setRelationshipProperty( long relationship, * * @return map containing this node's properties. */ - Map getNodeProperties( long nodeId ); + Map getNodeProperties( long nodeId ); /** * Returns an iterable over all the relationship ids connected to node with @@ -226,7 +225,7 @@ void setRelationshipProperties( long rel, * @param relId the id of the relationship. * @return map containing the relationship's properties. */ - Map getRelationshipProperties( long relId ); + Map getRelationshipProperties( long relId ); /** * Removes the property named {@code property} from the node with id diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java index c289e6f088021..799f7d61a8a8b 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java @@ -921,7 +921,7 @@ public boolean nodeExists( long nodeId ) } @Override - public Map getNodeProperties( long nodeId ) + public Map getNodeProperties( long nodeId ) { NodeRecord record = getNodeRecord( nodeId ).forReadingData(); if ( record.getNextProp() != Record.NO_NEXT_PROPERTY.intValue() ) @@ -969,7 +969,7 @@ public BatchRelationship getRelationshipById( long relId ) } @Override - public Map getRelationshipProperties( long relId ) + public Map getRelationshipProperties( long relId ) { RelationshipRecord record = recordAccess.getRelRecords().getOrLoad( relId, null ).forChangingData(); if ( record.getNextProp() != Record.NO_NEXT_PROPERTY.intValue() ) @@ -1026,14 +1026,14 @@ public String toString() return "EmbeddedBatchInserter[" + storeDir + "]"; } - private Map getPropertyChain( long nextProp ) + private Map getPropertyChain( long nextProp ) { - final Map map = new HashMap<>(); + final Map map = new HashMap<>(); propertyTraverser.getPropertyChain( nextProp, recordAccess.getPropertyRecords(), propBlock -> { String key = propertyKeyTokens.byId( propBlock.getKeyIndexId() ).name(); Value propertyValue = propBlock.newPropertyValue( propertyStore ); - map.put( key, propertyValue ); + map.put( key, propertyValue.asObject() ); } ); return map; } diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/FileSystemClosingBatchInserter.java b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/FileSystemClosingBatchInserter.java index 8c5b0400bbc53..3c857a484ba0a 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/FileSystemClosingBatchInserter.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/FileSystemClosingBatchInserter.java @@ -31,7 +31,6 @@ import org.neo4j.kernel.impl.index.IndexConfigStore; import org.neo4j.unsafe.batchinsert.BatchInserter; import org.neo4j.unsafe.batchinsert.BatchRelationship; -import org.neo4j.values.storable.Value; public class FileSystemClosingBatchInserter implements BatchInserter, IndexConfigStoreProvider { @@ -114,7 +113,7 @@ public void setRelationshipProperty( long relationship, String propertyName, Obj } @Override - public Map getNodeProperties( long nodeId ) + public Map getNodeProperties( long nodeId ) { return delegate.getNodeProperties( nodeId ); } @@ -150,7 +149,7 @@ public void setRelationshipProperties( long rel, Map properties ) } @Override - public Map getRelationshipProperties( long relId ) + public Map getRelationshipProperties( long relId ) { return delegate.getRelationshipProperties( relId ); } diff --git a/community/kernel/src/test/java/org/neo4j/unsafe/batchinsert/internal/BatchInsertTest.java b/community/kernel/src/test/java/org/neo4j/unsafe/batchinsert/internal/BatchInsertTest.java index 70ca38787ec0f..2361e7135c1b3 100644 --- a/community/kernel/src/test/java/org/neo4j/unsafe/batchinsert/internal/BatchInsertTest.java +++ b/community/kernel/src/test/java/org/neo4j/unsafe/batchinsert/internal/BatchInsertTest.java @@ -283,7 +283,7 @@ public void shouldUpdateStringArrayPropertiesOnNodesUsingBatchInserter1() throws inserter.setNodeProperty( id2, "array", array2 ); // Then - assertThat( inserter.getNodeProperties( id1 ).get( "array" ), equalTo( Values.of( array1 ) ) ); + assertThat( inserter.getNodeProperties( id1 ).get( "array" ), equalTo( array1 ) ); } @Test @@ -1058,8 +1058,8 @@ public void propertiesCanBeReSetUsingBatchInserter() throws Exception batchInserter.setNodeProperty( nodeId, "additional", "something" ); // THEN there should be no problems doing so - assertEquals( Values.of( "YetAnotherOne" ), batchInserter.getNodeProperties( nodeId ).get( "name" ) ); - assertEquals( Values.of( "something" ), batchInserter.getNodeProperties( nodeId ).get( "additional" ) ); + assertEquals( "YetAnotherOne", batchInserter.getNodeProperties( nodeId ).get( "name" ) ); + assertEquals("something", batchInserter.getNodeProperties( nodeId ).get( "additional" ) ); } /** @@ -1119,7 +1119,7 @@ public void propertiesCanBeReSetUsingBatchInserter2() throws Exception batchInserter.setNodeProperty( id, "test", "small test" ); // THEN - assertEquals( Values.of( "small test" ), batchInserter.getNodeProperties( id ).get( "test" ) ); + assertEquals( "small test", batchInserter.getNodeProperties( id ).get( "test" ) ); } @Test @@ -1138,7 +1138,7 @@ public void replaceWithBiggerPropertySpillsOverIntoNewPropertyRecord() throws Ex batchInserter.setNodeProperty( id, "count", "something" ); // THEN - assertEquals( Values.of( "something" ), batchInserter.getNodeProperties( id ).get( "count" ) ); + assertEquals( "something", batchInserter.getNodeProperties( id ).get( "count" ) ); } @Test @@ -1475,8 +1475,15 @@ private long dbWithIndexAndSingleIndexedNode() throws Exception private void setAndGet( BatchInserter inserter, Object value ) { long nodeId = inserter.createNode( map( "key", value ) ); - Value readValue = inserter.getNodeProperties( nodeId ).get( "key" ); - assertEquals( Values.of( value ), readValue ); + Object readValue = inserter.getNodeProperties( nodeId ).get( "key" ); + if ( readValue.getClass().isArray() ) + { + assertTrue( Arrays.equals( (int[])value, (int[])readValue ) ); + } + else + { + assertEquals( value, readValue ); + } } private int[] intArray() @@ -1536,23 +1543,11 @@ private Pair> manyLabels( int count ) private Map getNodeProperties( BatchInserter inserter, long nodeId ) { - Map asValues = inserter.getNodeProperties( nodeId ); - return valueMapToObjectMap( asValues ); + return inserter.getNodeProperties( nodeId ); } private Map getRelationshipProperties( BatchInserter inserter, long relId ) { - Map asValues = inserter.getRelationshipProperties( relId ); - return valueMapToObjectMap( asValues ); - } - - private Map valueMapToObjectMap( Map asValues ) - { - Map asObjects = new HashMap<>(); - for ( Map.Entry entry : asValues.entrySet() ) - { - asObjects.put( entry.getKey(), entry.getValue().asObjectCopy() ); - } - return asObjects; + return inserter.getRelationshipProperties( relId ); } }