Respond to PR #3566 comments from @ohadzeliger

Predominately:
- DataInKeySpace now errors if it gets a null value
- Improved javadoc linking KeySpace.resolveFromKeyAsync and
  KeySpacePath.toResolvedPathAsync
- Fix error message in default method
- Reduce visibility of ResolvedKeySPacePath.withRemainder
- new negative tests in DataInKeySpacePathTest
- Better coverage of exporting with mixed-type sub-directories

Author: ScottDugas

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DataInKeySpacePath.java

@@ -21,7 +21,10 @@
 package com.apple.foundationdb.record.provider.foundationdb.keyspace;
 
 import com.apple.foundationdb.KeyValue;
+import com.apple.foundationdb.record.RecordCoreArgumentException;
+import com.apple.foundationdb.record.logging.LogMessageKeys;
 import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext;
+import com.apple.foundationdb.tuple.ByteArrayUtil2;
 
 import javax.annotation.Nonnull;
 import java.util.concurrent.CompletableFuture;
@@ -39,6 +42,10 @@ public class DataInKeySpacePath {
     public DataInKeySpacePath(KeySpacePath path, KeyValue rawKeyValue, FDBRecordContext context) {
         this.resolvedPath = path.toResolvedPathAsync(context, rawKeyValue.getKey());
         this.value = rawKeyValue.getValue();
+        if (this.value == null) {
+            throw new RecordCoreArgumentException("Value cannot be null")
+                    .addLogInfo(LogMessageKeys.KEY, ByteArrayUtil2.loggable(rawKeyValue.getKey()));
+        }
     }
 
     public CompletableFuture<ResolvedKeySpacePath> getResolvedPath() {

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpace.java

@@ -227,9 +227,12 @@ public KeySpacePath pathFromKey(@Nonnull FDBRecordContext context, @Nonnull Tupl
     /**
      * Given a tuple from an FDB key, attempts to determine what path through this directory the tuple
      * represents, returning a <code>ResolvedKeySpacePath</code> representing the leaf-most directory in the path.
-     * If entries remained in the tuple beyond the leaf directory, then {@link KeySpacePath#getRemainder()}
-     * can be used to fetch the remaining portion.
-     *
+     * <p>
+     *     If entries remained in the tuple beyond the leaf directory, then {@link KeySpacePath#getRemainder()} can be
+     *     used to fetch the remaining portion.
+     *     See also {@link KeySpacePath#toResolvedPathAsync(FDBRecordContext, byte[])} if you need to resolve and you
+     *     know that it is part of a given path.
+     * </p>
      * @param context context used, if needed, for any database operations
      * @param key the tuple to be decoded
      * @return a path entry representing the leaf directory entry that corresponds to a value in the

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePath.java

@@ -199,15 +199,22 @@ default Tuple toTuple(@Nonnull FDBRecordContext context) {
     CompletableFuture<ResolvedKeySpacePath> toResolvedPathAsync(@Nonnull FDBRecordContext context);
 
     /**
-     * Resolves the given key within this path.
-     * @param context the transaction to lookup any necessary directory layer entries.
+     * Given a tuple from an FDB key, attempts to determine what sub-path through this directory the tuple
+     * represents, returning a <code>ResolvedKeySpacePath</code> representing the leaf-most directory in the path.
+     * <p>
+     *     If entries remained in the tuple beyond the leaf directory, then {@link KeySpacePath#getRemainder()}
+     *     can be used to fetch the remaining portion.
+     *     See also {@link KeySpace#resolveFromKeyAsync(FDBRecordContext, Tuple)} if you need to resolve from the root.
+     * </p>
+     * @param context  context used, if needed, for any database operations
      * @param key a raw key from the database
      * @return the {@link ResolvedKeySpacePath} corresponding to that key, with a potential remainder.
+     * @throws com.apple.foundationdb.record.RecordCoreArgumentException if the key provided is not part of this path
      */
     @API(API.Status.EXPERIMENTAL)
     @Nonnull
     default CompletableFuture<ResolvedKeySpacePath> toResolvedPathAsync(@Nonnull FDBRecordContext context, byte[] key) {
-        throw new UnsupportedOperationException("toResolvedPath is not supported");
+        throw new UnsupportedOperationException("toResolvedPathAsync is not supported");
     }
 
     /**

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java

@@ -24,6 +24,7 @@
 import com.apple.foundationdb.subspace.Subspace;
 import com.apple.foundationdb.tuple.ByteArrayUtil2;
 import com.apple.foundationdb.tuple.Tuple;
+import com.google.common.annotations.VisibleForTesting;
 
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
@@ -278,7 +279,8 @@ public static void appendValue(StringBuilder sb, Object value) {
      * @return a new {@code ResolvedKeySpacePath} that is the same as this, except with a different {@link #getRemainder()}.
      */
     @Nonnull
-    public ResolvedKeySpacePath withRemainder(@Nullable final Tuple newRemainder) {
+    @VisibleForTesting
+    ResolvedKeySpacePath withRemainder(@Nullable final Tuple newRemainder) {
         // this could probably copy the cachedTuple & cachedSubspace
         return new ResolvedKeySpacePath(parent, inner, value, newRemainder);
     }

fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DataInKeySpacePathTest.java

@@ -22,6 +22,7 @@
 
 import com.apple.foundationdb.KeyValue;
 import com.apple.foundationdb.Transaction;
+import com.apple.foundationdb.record.RecordCoreArgumentException;
 import com.apple.foundationdb.record.provider.foundationdb.FDBDatabase;
 import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext;
 import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpaceDirectory.KeyType;
@@ -39,13 +40,16 @@
 import java.util.List;
 import java.util.UUID;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
 import java.util.concurrent.ExecutionException;
 import java.util.function.Function;
 
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 /**
@@ -328,6 +332,50 @@ void withWrapper() {
         }
     }
 
+    @Test
+    void badPath() {
+        final String rootUuid = UUID.randomUUID().toString();
+        KeySpace root = new KeySpace(
+                new KeySpaceDirectory("test", KeyType.STRING, rootUuid));
+
+        final FDBDatabase database = dbExtension.getDatabase();
+
+        try (FDBRecordContext context = database.openContext()) {
+            KeySpacePath testPath = root.path("test");
+            byte[] keyBytes = Tuple.from("banana", rootUuid).pack();
+            byte[] valueBytes = Tuple.from("accessor_test").pack();
+
+            KeyValue originalKeyValue = new KeyValue(keyBytes, valueBytes);
+            DataInKeySpacePath dataInPath = new DataInKeySpacePath(testPath, originalKeyValue, context);
+            final CompletionException completionException = assertThrows(CompletionException.class,
+                    () -> dataInPath.getResolvedPath().join());
+            assertInstanceOf(RecordCoreArgumentException.class, completionException.getCause());
+        }
+    }
+
+    /**
+     * Test if there is a null value. FDB shouldn't ever return this, and if you got it somehow, you wouldn't be
+     * able to insert it back into a database because {@link com.apple.foundationdb.FDBTransaction#set(byte[], byte[])}
+     * does not support a {@code null} key or value.
+     */
+    @Test
+    void nullValue() {
+        KeySpace root = new KeySpace(
+                new KeySpaceDirectory("test", KeyType.STRING, UUID.randomUUID().toString()));
+
+        final FDBDatabase database = dbExtension.getDatabase();
+
+        try (FDBRecordContext context = database.openContext()) {
+            KeySpacePath testPath = root.path("test");
+            byte[] keyBytes = testPath.toTuple(context).pack();
+            byte[] valueBytes = null;
+
+            KeyValue originalKeyValue = new KeyValue(keyBytes, valueBytes);
+            assertThrows(RecordCoreArgumentException.class,
+                    () -> new DataInKeySpacePath(testPath, originalKeyValue, context));
+        }
+    }
+
     private static ResolvedKeySpacePath assertNameAndDirectoryScopedValue(ResolvedKeySpacePath resolved,
                                                                           String name, Object logicalValue,
                                                                           KeySpacePath path, FDBRecordContext context) {

fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathDataExportTest.java

@@ -46,6 +46,7 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.UUID;
 import java.util.concurrent.ExecutionException;
@@ -212,35 +213,30 @@ void exportAllDataWithDirectoryLayer() {
 
     @Test
     void exportAllDataWithDifferentKeyTypes() {
-        KeySpace root = new KeySpace(
-                new KeySpaceDirectory("mixed", KeyType.STRING, UUID.randomUUID().toString())
-                        .addSubdirectory(new KeySpaceDirectory("strings", KeyType.STRING))
-                        .addSubdirectory(new KeySpaceDirectory("longs", KeyType.LONG))
-                        .addSubdirectory(new KeySpaceDirectory("bytes", KeyType.BYTES))
-                        .addSubdirectory(new KeySpaceDirectory("uuids", KeyType.UUID))
-                        .addSubdirectory(new KeySpaceDirectory("booleans", KeyType.BOOLEAN)));
+        final KeySpaceDirectory rootDirectory = new KeySpaceDirectory("mixed", KeyType.STRING, UUID.randomUUID().toString());
+        Map<KeyType, List<Object>> dataByType = Map.of(
+                KeyType.STRING, List.of("str0", "str1", "str2"),
+                KeyType.LONG, List.of(10L, 11L, 12L),
+                KeyType.BYTES, List.of(new byte[]{0, 1}, new byte[]{1, 2}),
+                KeyType.UUID, List.of(new UUID(0, 0), new UUID(1, 1)),
+                KeyType.BOOLEAN, List.of(true, false),
+                KeyType.NULL, Collections.singletonList(null),
+                KeyType.DOUBLE, List.of(3.1415, -2.718281, 13.23E8),
+                KeyType.FLOAT, List.of(1.4142135f, -5.8f, 130.23f)
+        );
+        dataByType.keySet().forEach(keyType ->
+                rootDirectory.addSubdirectory(new KeySpaceDirectory(keyType.name(), keyType)));
+        KeySpace root = new KeySpace(rootDirectory);
+        assertEquals(Set.of(KeyType.values()), dataByType.keySet());
 
         final FDBDatabase database = dbExtension.getDatabase();
 
         // Store test data with different key types
         try (FDBRecordContext context = database.openContext()) {
             KeySpacePath basePath = root.path("mixed");
-            
-            // String keys (str0, str1, str2 -> string_value_0, string_value_1, string_value_2)
-            setData(List.of("str0", "str1", "str2"), context, basePath, "strings", "string_value_");
-            
-            // Long keys (10, 11, 12 -> long_value_10, long_value_11, long_value_12)
-            setData(List.of(10L, 11L, 12L), context, basePath, "longs", "long_value_");
-            
-            // Bytes keys (arrays -> bytes_value_[0, 1], bytes_value_[1, 2])
-            setData(List.of(new byte[]{0, 1}, new byte[]{1, 2}), context, basePath, "bytes", "bytes_value_");
-            
-            // UUID keys (UUIDs -> uuid_value_UUID)
-            setData(List.of(new UUID(0, 0), new UUID(1, 1)), context, basePath, "uuids", "uuid_value_");
-            
-            // Boolean keys (true, false -> boolean_value_true, boolean_value_false)
-            setData(List.of(true, false), context, basePath, "booleans", "boolean_value_");
-            
+            dataByType.forEach((keyType, data) ->
+                    setData(data, context, basePath, keyType.name(), keyType + "_value_")
+            );
             context.commit();
         }
 
@@ -249,15 +245,15 @@ void exportAllDataWithDifferentKeyTypes() {
             KeySpacePath mixedPath = root.path("mixed");
             final List<DataInKeySpacePath> allData = exportAllData(mixedPath, context);
 
-            // Should have 12 records total (3+3+2+2+2)
-            assertEquals(12, allData.size());
+            assertEquals(dataByType.values().stream().mapToLong(List::size).sum(),
+                    allData.size());
 
             // Verify we have different value types
             Set<String> valueTypes = allData.stream()
                     .map(data -> Tuple.fromBytes(data.getValue()).getString(0).split("_")[0])
                     .collect(Collectors.toSet());
-            assertEquals(5, valueTypes.size());
-            assertTrue(valueTypes.containsAll(Arrays.asList("string", "long", "bytes", "uuid", "boolean")));
+            assertEquals((Arrays.stream(KeyType.values()).map(Enum::name).collect(Collectors.toSet())),
+                    valueTypes);
         }
     }