Skip to content

Commit

Permalink
PropertyRecord.getType() now returns PropertyType instead of int
Browse files Browse the repository at this point in the history 
Reading the property type and converting to a PropertyType value
 requires bit shifting and masking that is not achieved by simply
 using the type int value as read from disk. This can lead to
 improper identification of the record type during recovery, among
 other scenarios.
This patch changes the PropertyRecord.getType() method to return
 PropertyType, properly parsed. It adds a getTypeAsInt() method
 to allow for access to the raw int value for serialization
 purposes.
  • Loading branch information
@digitalstain @tinwelint
digitalstain authored and tinwelint committed Feb 7, 2017
1 parent c384a90 commit bfed2b1
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 22 deletions.
23 changes: 23 additions & 0 deletions ...onsistency-check/src/test/java/org/neo4j/consistency/checking/DynamicRecordCheckTest.java
View file
Expand Up @@ -25,11 +25,13 @@
import org.junit.runners.Suite;

import org.neo4j.consistency.report.ConsistencyReport;
import org.neo4j.kernel.impl.store.PropertyType;
import org.neo4j.kernel.impl.store.RecordStore;
import org.neo4j.kernel.impl.store.SchemaStore;
import org.neo4j.kernel.impl.store.format.standard.DynamicRecordFormat;
import org.neo4j.kernel.impl.store.record.DynamicRecord;

import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
Expand Down Expand Up @@ -186,6 +188,27 @@ public void shouldReportRecordWithEmptyNext() throws Exception
verifyNoMoreInteractions( report );
}

@Test
public void shouldReportCorrectTypeBasedOnProperBitsOnly() throws Exception
{
// given
DynamicRecord property = inUse( record( 42 ) );
// Type is 9, which is string, but has an extra bit set at a higher up position
int type = PropertyType.STRING.intValue();
type = type | 0b10000000;

property.setType( type );

// when
PropertyType reportedType = property.getType();

// then
// The type must be string
assertEquals( PropertyType.STRING, reportedType );
// but the type data must be preserved
assertEquals( type, property.getTypeAsInt() );
}

// utilities

DynamicRecord fill( DynamicRecord record )
Expand Down
12 changes: 2 additions & 10 deletions ...y/consistency-check/src/test/java/org/neo4j/consistency/checking/RecordCheckTestBase.java
View file
Expand Up @@ -240,16 +240,8 @@ public static DynamicRecord array( DynamicRecord record )

static PropertyBlock propertyBlock( PropertyKeyTokenRecord key, DynamicRecord value )
{
PropertyType type;
if ( value.getType() == PropertyType.STRING.intValue() )
{
type = PropertyType.STRING;
}
else if ( value.getType() == PropertyType.ARRAY.intValue() )
{
type = PropertyType.ARRAY;
}
else
PropertyType type = value.getType();
if ( value.getType() != PropertyType.STRING && value.getType() != PropertyType.ARRAY )
{
fail( "Dynamic record must be either STRING or ARRAY" );
return null;
Expand Down
12 changes: 6 additions & 6 deletions community/consistency-check/src/test/java/org/neo4j/consistency/store/RecordAccessStub.java
View file
Expand Up @@ -333,15 +333,15 @@ else if ( newRecord instanceof PropertyRecord )
else if ( newRecord instanceof DynamicRecord )
{
DynamicRecord dyn = (DynamicRecord) newRecord;
if ( dyn.getType() == PropertyType.STRING.intValue() )
if ( dyn.getType() == PropertyType.STRING )
{
add( strings, (DynamicRecord) oldRecord, dyn );
}
else if ( dyn.getType() == PropertyType.ARRAY.intValue() )
else if ( dyn.getType() == PropertyType.ARRAY )
{
add( arrays, (DynamicRecord) oldRecord, dyn );
}
else if ( dyn.getType() == SCHEMA_RECORD_TYPE )
else if ( dyn.getTypeAsInt() == SCHEMA_RECORD_TYPE )
{
add( schemata, (DynamicRecord) oldRecord, dyn );
}
Expand Down Expand Up @@ -386,15 +386,15 @@ else if ( record instanceof PropertyRecord )
else if ( record instanceof DynamicRecord )
{
DynamicRecord dyn = (DynamicRecord) record;
if ( dyn.getType() == PropertyType.STRING.intValue() )
if ( dyn.getType() == PropertyType.STRING )
{
addString( dyn );
}
else if ( dyn.getType() == PropertyType.ARRAY.intValue() )
else if ( dyn.getType() == PropertyType.ARRAY )
{
addArray( dyn );
}
else if ( dyn.getType() == SCHEMA_RECORD_TYPE )
else if ( dyn.getTypeAsInt() == SCHEMA_RECORD_TYPE )
{
addSchema( dyn );
}
Expand Down
5 changes: 3 additions & 2 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/store/PropertyStore.java
View file
Expand Up @@ -140,11 +140,12 @@ private void updateDynamicRecords( List<DynamicRecord> records )
{
for ( DynamicRecord valueRecord : records )
{
if ( valueRecord.getType() == PropertyType.STRING.intValue() )
PropertyType recordType = valueRecord.getType();
if ( recordType == PropertyType.STRING )
{
stringStore.updateRecord( valueRecord );
}
else if ( valueRecord.getType() == PropertyType.ARRAY.intValue() )
else if ( recordType == PropertyType.ARRAY )
{
arrayStore.updateRecord( valueRecord );
}
Expand Down
16 changes: 14 additions & 2 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/DynamicRecord.java
View file
Expand Up @@ -93,7 +93,19 @@ public boolean isStartRecord()
return startRecord;
}

public int getType()
/**
* @return The {@link PropertyType} of this record or null if unset or non valid
*/
public PropertyType getType()
{
return PropertyType.getPropertyTypeOrNull( (long) (this.type << 24) );
}

/**
* @return The {@link #type} field of this record, as set by previous invocations to {@link #setType(int)} or
* {@link #initialize(boolean, boolean, long, int, int)}
*/
public int getTypeAsInt()
{
return type;
}
Expand Down Expand Up @@ -148,7 +160,7 @@ public String toString()
.append( getId() )
.append( ",used=" ).append(inUse() ).append( "," )
.append("(" ).append( length ).append( "),type=" );
PropertyType type = PropertyType.getPropertyTypeOrNull( (long) (this.type << 24) );
PropertyType type = getType();
if ( type == null ) buf.append( this.type ); else buf.append( type.name() );
buf.append( ",data=" );
if ( type == PropertyType.STRING && data.length <= MAX_CHARS_IN_TO_STRING )
Expand Down
4 changes: 2 additions & 2 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/Command.java
View file
Expand Up @@ -156,7 +156,7 @@ void writeDynamicRecord( WritableChannel channel, DynamicRecord record ) throws
inUse |= Record.FIRST_IN_CHAIN.byteValue();
}
channel.putLong( record.getId() )
.putInt( record.getType() )
.putInt( record.getTypeAsInt() )
.put( inUse )
.putInt( record.getLength() )
.putLong( record.getNextBlock() );
Expand All @@ -168,7 +168,7 @@ void writeDynamicRecord( WritableChannel channel, DynamicRecord record ) throws
{
byte inUse = Record.NOT_IN_USE.byteValue();
channel.putLong( record.getId() )
.putInt( record.getType() )
.putInt( record.getTypeAsInt() )
.put( inUse );
}
}
Expand Down

0 comments on commit bfed2b1

Please sign in to comment.