Index: oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapEntry.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapEntry.java (date 1488193417000) +++ oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapEntry.java (date 1488140787000) @@ -29,6 +29,7 @@ import javax.annotation.Nullable; import com.google.common.collect.ComparisonChain; +import com.google.common.collect.Ordering; import org.apache.jackrabbit.oak.spi.state.AbstractChildNodeEntry; /** @@ -49,18 +50,66 @@ @CheckForNull private final RecordId value; - MapEntry(@Nonnull SegmentReader reader, @Nonnull String name, - @Nonnull RecordId key, @Nullable RecordId value) { + private MapEntry( + @Nonnull SegmentReader reader, + @Nonnull String name, + @Nonnull RecordId key, + @Nullable RecordId value) { this.reader = checkNotNull(reader); this.name = checkNotNull(name); this.key = checkNotNull(key); this.value = value; } + /** + * Create a new instance of a {@code MapEntry} as it has been read from a segment. + + * @param reader segment reader for reading the node state this map entry's value points to. + * @param name name of the key + * @param key record id of the key + * @param value record id of the value + */ + static MapEntry newMapEntry( + @Nonnull SegmentReader reader, + @Nonnull String name, + @Nonnull RecordId key, + @Nonnull RecordId value) { + return new MapEntry(reader, name, key, checkNotNull(value)); + } + + /** + * Create a new instance of a {@code MapEntry} to be written to a segment. Here the passed + * {@code value} might be {@code null} to indicate an existing mapping for the this {@code key} + * should be deleted. In this case calls to {@link #getValue()} should be guarded with a prior + * call to {@link #isDeleted()} to prevent the former throwing an {@code IllegalStateException}. + * + * @param reader segment reader for reading the node state this map entry's value points to. + * @param name name of the key + * @param key record id of the key + * @param value record id of the value or {@code null}. + * + * @see #isDeleted() + * @see #getValue() + */ + static MapEntry newModifiedMapEntry( + @Nonnull SegmentReader reader, + @Nonnull String name, + @Nonnull RecordId key, + @Nullable RecordId value) { + return new MapEntry(reader, name, key, value); + } + public int getHash() { return MapRecord.getHash(name); } + /** + * @return {@code true} to indicate that this value is to be deleted. + */ + boolean isDeleted() { + return value == null; + } + //----------------------------------------------------< ChildNodeEntry >-- @Override @Nonnull @@ -82,9 +131,14 @@ return key; } - @CheckForNull + /** + * @return the value of this mapping. + * @throws IllegalStateException if {@link #isDeleted()} is {@code true}. + */ + @Nonnull @Override public RecordId getValue() { + checkState(value != null); return value; } @@ -100,7 +154,7 @@ return ComparisonChain.start() .compare(getHash() & HASH_MASK, that.getHash() & HASH_MASK) .compare(name, that.name) - .compare(value, that.value) // FIXME OAK-5301: Possible null dereference in MapRecord + .compare(value, that.value, Ordering.natural().nullsLast()) .result(); } Index: oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapRecord.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapRecord.java (date 1488193417000) +++ oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapRecord.java (date 1488140787000) @@ -24,6 +24,7 @@ import static java.lang.Integer.bitCount; import static java.lang.Integer.highestOneBit; import static java.lang.Integer.numberOfTrailingZeros; +import static org.apache.jackrabbit.oak.segment.MapEntry.newMapEntry; import java.util.Arrays; import java.util.Collections; @@ -165,7 +166,7 @@ RecordId key = segment.readRecordId(getRecordNumber(), 8); if (name.equals(reader.readString(key))) { RecordId value = segment.readRecordId(getRecordNumber(), 8, 1); - return new MapEntry(reader, name, key, value); + return newMapEntry(reader, name, key, value); } } RecordId base = segment.readRecordId(getRecordNumber(), 8, 2); @@ -218,7 +219,7 @@ RecordId valueId = segment.readRecordId(getRecordNumber(), 4 + size * 4, i * 2 + 1); diff = reader.readString(keyId).compareTo(name); if (diff == 0) { - return new MapEntry(reader, name, keyId, valueId); + return newMapEntry(reader, name, keyId, valueId); } } @@ -369,7 +370,7 @@ value = segment.readRecordId(getRecordNumber(), 4 + size * 4, i * 2 + 1); } String name = reader.readString(key); - entries[i] = new MapEntry(reader, name, key, value); + entries[i] = newMapEntry(reader, name, key, value); } return Arrays.asList(entries); } @@ -484,9 +485,7 @@ } else if (d == 0) { assert beforeEntry != null; assert afterEntry != null; - RecordId beforeValue = beforeEntry.getValue(); - assert beforeValue != null; - if (!beforeValue.equals(afterEntry.getValue()) + if (!beforeEntry.getValue().equals(afterEntry.getValue()) && !diff.childNodeChanged( beforeEntry.getName(), beforeEntry.getNodeState(), Index: oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/Segment.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/Segment.java (date 1488193417000) +++ oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/Segment.java (date 1488140787000) @@ -45,6 +45,8 @@ import javax.annotation.CheckForNull; import javax.annotation.Nonnull; +import com.google.common.base.Charsets; +import com.google.common.collect.AbstractIterator; import org.apache.commons.io.HexDump; import org.apache.commons.io.output.ByteArrayOutputStream; import org.apache.jackrabbit.oak.api.PropertyState; @@ -53,9 +55,6 @@ import org.apache.jackrabbit.oak.plugins.memory.PropertyStates; import org.apache.jackrabbit.oak.segment.RecordNumbers.Entry; -import com.google.common.base.Charsets; -import com.google.common.collect.AbstractIterator; - /** * A list of records. *

@@ -466,6 +465,7 @@ d.get(buffer, offset, length); } + @Nonnull RecordId readRecordId(int recordNumber, int rawOffset, int recordIdOffset) { return internalReadRecordId(pos(recordNumber, rawOffset, recordIdOffset, RECORD_ID_BYTES)); } @@ -478,6 +478,7 @@ return readRecordId(recordNumber, 0, 0); } + @Nonnull private RecordId internalReadRecordId(int pos) { SegmentId segmentId = dereferenceSegmentId(asUnsigned(data.getShort(pos))); return new RecordId(segmentId, data.getInt(pos + 2)); @@ -487,6 +488,7 @@ return value & 0xffff; } + @Nonnull private SegmentId dereferenceSegmentId(int reference) { if (reference == 0) { return id; Index: oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java (date 1488193417000) +++ oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java (date 1488140787000) @@ -42,6 +42,7 @@ import static org.apache.jackrabbit.oak.api.Type.NAME; import static org.apache.jackrabbit.oak.api.Type.NAMES; import static org.apache.jackrabbit.oak.api.Type.STRING; +import static org.apache.jackrabbit.oak.segment.MapEntry.newModifiedMapEntry; import static org.apache.jackrabbit.oak.segment.MapRecord.BUCKETS_PER_LEVEL; import static org.apache.jackrabbit.oak.segment.RecordWriters.newNodeStateWriter; @@ -501,7 +502,7 @@ } if (keyId != null) { - entries.add(new MapEntry(reader, key, keyId, entry.getValue())); + entries.add(newModifiedMapEntry(reader, key, keyId, entry.getValue())); } } return writeMapBucket(base, entries, 0); @@ -567,10 +568,10 @@ map.put(entry.getName(), entry); } for (MapEntry entry : entries) { - if (entry.getValue() != null) { - map.put(entry.getName(), entry); - } else { + if (entry.isDeleted()) { map.remove(entry.getName()); + } else { + map.put(entry.getName(), entry); } } return writeMapBucket(null, map.values(), level);