From ced2080137678f0350b326dd45fa6a196f528328 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Fri, 31 Oct 2025 09:42:32 -0700 Subject: [PATCH 01/12] Introduce proto for DataInKeySpacePath and a serialize method --- .../keyspace/DataInKeySpacePath.java | 1 + .../keyspace/KeySpacePathSerializer.java | 116 ++++++++++++++++++ .../src/main/proto/keyspace.proto | 52 ++++++++ 3 files changed, 169 insertions(+) create mode 100644 fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java create mode 100644 fdb-record-layer-core/src/main/proto/keyspace.proto diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DataInKeySpacePath.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DataInKeySpacePath.java index f2d2284730..729ab6eaa8 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DataInKeySpacePath.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DataInKeySpacePath.java @@ -53,6 +53,7 @@ public DataInKeySpacePath(@Nonnull final KeySpacePath path, @Nullable final Tupl this.value = value; } + @Nonnull public byte[] getValue() { return this.value; } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java new file mode 100644 index 0000000000..dd766cdf05 --- /dev/null +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java @@ -0,0 +1,116 @@ +/* + * KeySpacePathSerializer.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.apple.foundationdb.record.provider.foundationdb.keyspace; + +import com.apple.foundationdb.record.RecordCoreArgumentException; +import com.apple.foundationdb.record.logging.LogMessageKeys; +import com.google.protobuf.ByteString; + +import java.util.List; +import java.util.UUID; + +/** + * Class for serializing/deserializing between {@link DataInKeySpacePath} and {@link KeySpaceProto.DataInKeySpacePath}. + *

+ * This will serialize relative to a root path, such that the serialized form is relative to that path. This can be + * useful to both: + *

+ *

+ * + */ +public class KeySpacePathSerializer { + + private final List root; + + public KeySpacePathSerializer(final KeySpacePath root) { + this.root = root.flatten(); + } + + public ByteString serialize(DataInKeySpacePath data) { + KeySpaceProto.DataInKeySpacePath.Builder builder = KeySpaceProto.DataInKeySpacePath.newBuilder(); + final List dataPath = data.getPath().flatten(); + if (dataPath.size() < root.size() || + !dataPath.get(root.size() - 1).equals(root.get(root.size() - 1))) { + throw new RecordCoreArgumentException("Data is not contained within root path"); + } + for (int i = root.size(); i < dataPath.size(); i++) { + final KeySpacePath keySpacePath = dataPath.get(i); + builder.addPath(serialize(keySpacePath)); + } + if (data.getRemainder() != null) { + builder.setRemainder(ByteString.copyFrom(data.getRemainder().pack())); + } + builder.setValue(ByteString.copyFrom(data.getValue())); + return builder.build().toByteString(); + } + + private static KeySpaceProto.KeySpacePathEntry serialize(final KeySpacePath keySpacePath) { + KeySpaceProto.KeySpacePathEntry.Builder builder = KeySpaceProto.KeySpacePathEntry.newBuilder() + .setName(keySpacePath.getDirectoryName()); + final Object value = keySpacePath.getValue(); + final KeySpaceDirectory.KeyType keyType = keySpacePath.getDirectory().getKeyType(); + try { + switch (keyType) { + case NULL: + builder.setNullValue(true); + break; + case BYTES: + builder.setBytesValue(ByteString.copyFrom((byte[])value)); + break; + case STRING: + builder.setStringValue((String)value); + break; + case LONG: + builder.setLongValue((Long)value); + break; + case FLOAT: + builder.setFloatValue((Float)value); + break; + case DOUBLE: + builder.setDoubleValue((Double)value); + break; + case BOOLEAN: + builder.setBooleanValue((Boolean)value); + break; + case UUID: + final UUID uuid = (UUID)value; + builder.getUuidBuilder() + .setLeastSignificantBits(uuid.getLeastSignificantBits()) + .setMostSignificantBits(uuid.getMostSignificantBits()); + break; + default: + throw new IllegalStateException("Unexpected value: " + keyType); + } + } catch (NullPointerException | ClassCastException e) { + throw new RecordCoreArgumentException("KeySpacePath has incorrect value") + .addLogInfo( + LogMessageKeys.DIR_NAME, keySpacePath.getDirectoryName(), + LogMessageKeys.EXPECTED_TYPE, keyType, + LogMessageKeys.ACTUAL, value); + + } + return builder.build(); + } + +} diff --git a/fdb-record-layer-core/src/main/proto/keyspace.proto b/fdb-record-layer-core/src/main/proto/keyspace.proto new file mode 100644 index 0000000000..2f64e90452 --- /dev/null +++ b/fdb-record-layer-core/src/main/proto/keyspace.proto @@ -0,0 +1,52 @@ +/* + * keyspace.proto + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +syntax = "proto2"; + +package com.apple.foundationdb.record.provider.foundationdb.keyspace; +option java_outer_classname = "KeySpaceProto"; + +message DataInKeySpacePath { + repeated KeySpacePathEntry path = 1; + optional bytes remainder = 2; + optional bytes value = 3; +} + +// Entry representing logical values for a KeySpacePath entry. +message KeySpacePathEntry { + optional string name = 1; + + // specific boolean to indicate this is supposed to be a null + optional bool nullValue = 2; + optional bytes bytesValue = 3; + optional string stringValue = 4; + optional int64 longValue = 5; + optional float floatValue = 6; + optional double doubleValue = 7; + optional bool booleanValue = 8; + optional UUID uuid = 9; + + message UUID { // TODO find out why we use fixed64 and not just int64 + // 2 64-bit fields is two tags, the same as 1 bytes field with a length of 16 would be. + // fixed64 would be closer to how these are really used, but would fail the unsigned validator. + optional sfixed64 most_significant_bits = 1; + optional sfixed64 least_significant_bits = 2; + } +} From 68fd207b5eb6b5a34cb7fa36d34773f1af2de5c8 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 4 Nov 2025 12:32:59 -0500 Subject: [PATCH 02/12] Added deserialize method and tests I also added `equals` to the KeySpacePath implementation to describe how two KeySpacePaths should be equal --- .../foundationdb/keyspace/KeySpacePath.java | 9 + .../keyspace/KeySpacePathSerializer.java | 103 +++- .../keyspace/KeySpacePathSerializerTest.java | 444 ++++++++++++++++++ 3 files changed, 550 insertions(+), 6 deletions(-) create mode 100644 fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePath.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePath.java index ff228ef1be..642fb51d90 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePath.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePath.java @@ -410,4 +410,13 @@ default RecordCursor exportAllData(@Nonnull FDBRecordContext @Nonnull ScanProperties scanProperties) { throw new UnsupportedOperationException("exportAllData is not supported"); } + + /** + * Two {@link KeySpacePath}s are equal if they have equal values, the same directory (reference equality) and their + * parents are the same. + * @param obj another {@link KeySpacePath} + * @return {@code true} if this path equals {@code obj} + */ + @Override + boolean equals(Object obj); } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java index dd766cdf05..760cb3fce0 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java @@ -20,10 +20,14 @@ package com.apple.foundationdb.record.provider.foundationdb.keyspace; +import com.apple.foundationdb.annotation.API; import com.apple.foundationdb.record.RecordCoreArgumentException; import com.apple.foundationdb.record.logging.LogMessageKeys; +import com.apple.foundationdb.tuple.Tuple; import com.google.protobuf.ByteString; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; import java.util.List; import java.util.UUID; @@ -39,17 +43,21 @@ *

* */ +@API(API.Status.EXPERIMENTAL) public class KeySpacePathSerializer { + @Nonnull private final List root; - public KeySpacePathSerializer(final KeySpacePath root) { + public KeySpacePathSerializer(@Nonnull final KeySpacePath root) { this.root = root.flatten(); } - public ByteString serialize(DataInKeySpacePath data) { + @Nonnull + public ByteString serialize(@Nonnull DataInKeySpacePath data) { KeySpaceProto.DataInKeySpacePath.Builder builder = KeySpaceProto.DataInKeySpacePath.newBuilder(); final List dataPath = data.getPath().flatten(); + // two paths are only equal if their parents are equal, so we don't have to validate the whole prefix here if (dataPath.size() < root.size() || !dataPath.get(root.size() - 1).equals(root.get(root.size() - 1))) { throw new RecordCoreArgumentException("Data is not contained within root path"); @@ -65,11 +73,90 @@ public ByteString serialize(DataInKeySpacePath data) { return builder.build().toByteString(); } - private static KeySpaceProto.KeySpacePathEntry serialize(final KeySpacePath keySpacePath) { + @Nonnull + public DataInKeySpacePath deserialize(@Nonnull ByteString bytes) { + try { + KeySpaceProto.DataInKeySpacePath proto = KeySpaceProto.DataInKeySpacePath.parseFrom(bytes); + return deserialize(proto); + } catch (com.google.protobuf.InvalidProtocolBufferException e) { + throw new RecordCoreArgumentException("Failed to parse serialized DataInKeySpacePath", e); + } + } + + @Nonnull + private DataInKeySpacePath deserialize(@Nonnull KeySpaceProto.DataInKeySpacePath proto) { + // Start with the root path + KeySpacePath path = root.get(root.size() - 1); + + // Add each path entry from the proto + for (KeySpaceProto.KeySpacePathEntry entry : proto.getPathList()) { + Object value = deserializeValue(entry); + path = path.add(entry.getName(), value); + } + + // Extract remainder if present + Tuple remainder = null; + if (proto.hasRemainder()) { + remainder = Tuple.fromBytes(proto.getRemainder().toByteArray()); + } + + // Extract value + if (!proto.hasValue()) { + throw new RecordCoreArgumentException("Serialized data must have a value"); + } + byte[] value = proto.getValue().toByteArray(); + + return new DataInKeySpacePath(path, remainder, value); + } + + @Nullable + private static Object deserializeValue(@Nonnull KeySpaceProto.KeySpacePathEntry entry) { + // Check which value field is set and return the appropriate value + if (entry.hasNullValue()) { + return null; + } else if (entry.hasBytesValue()) { + return entry.getBytesValue().toByteArray(); + } else if (entry.hasStringValue()) { + return entry.getStringValue(); + } else if (entry.hasLongValue()) { + return entry.getLongValue(); + } else if (entry.hasFloatValue()) { + return entry.getFloatValue(); + } else if (entry.hasDoubleValue()) { + return entry.getDoubleValue(); + } else if (entry.hasBooleanValue()) { + return entry.getBooleanValue(); + } else if (entry.hasUuid()) { + KeySpaceProto.KeySpacePathEntry.UUID uuidProto = entry.getUuid(); + return new UUID(uuidProto.getMostSignificantBits(), uuidProto.getLeastSignificantBits()); + } else { + throw new RecordCoreArgumentException("KeySpacePathEntry has no value set") + .addLogInfo(LogMessageKeys.DIR_NAME, entry.getName()); + } + } + + @Nonnull + private static KeySpaceProto.KeySpacePathEntry serialize(@Nonnull final KeySpacePath keySpacePath) { KeySpaceProto.KeySpacePathEntry.Builder builder = KeySpaceProto.KeySpacePathEntry.newBuilder() .setName(keySpacePath.getDirectoryName()); final Object value = keySpacePath.getValue(); final KeySpaceDirectory.KeyType keyType = keySpacePath.getDirectory().getKeyType(); + + // Validate null handling: NULL type must have null value, all other types must not have null value + if (keyType == KeySpaceDirectory.KeyType.NULL) { + if (value != null) { + throw new RecordCoreArgumentException("NULL key type must have null value") + .addLogInfo(LogMessageKeys.DIR_NAME, keySpacePath.getDirectoryName(), + LogMessageKeys.ACTUAL, value); + } + } else { + if (value == null) { + throw new RecordCoreArgumentException("Non-NULL key type cannot have null value") + .addLogInfo(LogMessageKeys.DIR_NAME, keySpacePath.getDirectoryName(), + LogMessageKeys.EXPECTED_TYPE, keyType); + } + } + try { switch (keyType) { case NULL: @@ -82,7 +169,11 @@ private static KeySpaceProto.KeySpacePathEntry serialize(final KeySpacePath keyS builder.setStringValue((String)value); break; case LONG: - builder.setLongValue((Long)value); + if (value instanceof Integer) { + builder.setLongValue(((Integer)value).longValue()); + } else { + builder.setLongValue((Long)value); + } break; case FLOAT: builder.setFloatValue((Float)value); @@ -102,8 +193,8 @@ private static KeySpaceProto.KeySpacePathEntry serialize(final KeySpacePath keyS default: throw new IllegalStateException("Unexpected value: " + keyType); } - } catch (NullPointerException | ClassCastException e) { - throw new RecordCoreArgumentException("KeySpacePath has incorrect value") + } catch (ClassCastException e) { + throw new RecordCoreArgumentException("KeySpacePath has incorrect value type", e) .addLogInfo( LogMessageKeys.DIR_NAME, keySpacePath.getDirectoryName(), LogMessageKeys.EXPECTED_TYPE, keyType, diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java new file mode 100644 index 0000000000..18a8ae00b6 --- /dev/null +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java @@ -0,0 +1,444 @@ +/* + * KeySpacePathSerializerTest.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.apple.foundationdb.record.provider.foundationdb.keyspace; + +import com.apple.foundationdb.record.RecordCoreArgumentException; +import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpaceDirectory.KeyType; +import com.apple.foundationdb.tuple.Tuple; +import com.google.protobuf.ByteString; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import java.util.UUID; +import java.util.stream.Stream; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * Tests for {@link KeySpacePathSerializer}. + */ +class KeySpacePathSerializerTest { + + @Test + void testSerializeAndDeserializeSimplePath() { + KeySpace root = new KeySpace( + new KeySpaceDirectory("app", KeyType.STRING, "myapp") + .addSubdirectory(new KeySpaceDirectory("tenant", KeyType.STRING))); + + KeySpacePath rootPath = root.path("app"); + KeySpacePath fullPath = rootPath.add("tenant", "tenant1"); + byte[] value = new byte[]{1, 2, 3, 4}; + + DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, value); + + KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); + ByteString serialized = serializer.serialize(data); + + DataInKeySpacePath deserialized = serializer.deserialize(serialized); + + assertEquals(fullPath.getDirectoryName(), deserialized.getPath().getDirectoryName()); + assertEquals("tenant1", deserialized.getPath().getValue()); + assertNull(deserialized.getRemainder()); + assertArrayEquals(value, deserialized.getValue()); + } + + @Test + void testSerializeAndDeserializeWithRemainder() { + KeySpace root = new KeySpace( + new KeySpaceDirectory("app", KeyType.STRING, "myapp") + .addSubdirectory(new KeySpaceDirectory("records", KeyType.STRING))); + + KeySpacePath rootPath = root.path("app"); + KeySpacePath fullPath = rootPath.add("records", "store1"); + Tuple remainder = Tuple.from("key1", "key2"); + byte[] value = new byte[]{10, 20, 30}; + + DataInKeySpacePath data = new DataInKeySpacePath(fullPath, remainder, value); + + KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); + ByteString serialized = serializer.serialize(data); + + DataInKeySpacePath deserialized = serializer.deserialize(serialized); + + assertEquals("store1", deserialized.getPath().getValue()); + assertNotNull(deserialized.getRemainder()); + assertEquals(remainder, deserialized.getRemainder()); + assertArrayEquals(value, deserialized.getValue()); + } + + @Test + void testSerializeAndDeserializeMultiLevelPath() { + KeySpace root = new KeySpace( + new KeySpaceDirectory("root", KeyType.STRING, "r") + .addSubdirectory(new KeySpaceDirectory("level1", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("level2", KeyType.LONG) + .addSubdirectory(new KeySpaceDirectory("level3", KeyType.STRING))))); + + KeySpacePath rootPath = root.path("root"); + KeySpacePath fullPath = rootPath + .add("level1", "l1value") + .add("level2", 42L) + .add("level3", "l3value"); + byte[] value = new byte[]{100}; + + DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, value); + + KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); + ByteString serialized = serializer.serialize(data); + + DataInKeySpacePath deserialized = serializer.deserialize(serialized); + + assertEquals("l3value", deserialized.getPath().getValue()); + assertArrayEquals(value, deserialized.getValue()); + } + + @ParameterizedTest + @MethodSource("provideKeyTypes") + void testSerializeDeserializeAllKeyTypes(KeyType keyType, Object value) { + KeySpace root = new KeySpace( + new KeySpaceDirectory("root", KeyType.STRING, "root") + .addSubdirectory(new KeySpaceDirectory("typed", keyType))); + + KeySpacePath rootPath = root.path("root"); + KeySpacePath fullPath = rootPath.add("typed", value); + byte[] dataValue = new byte[]{1}; + + DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, dataValue); + + KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); + ByteString serialized = serializer.serialize(data); + + DataInKeySpacePath deserialized = serializer.deserialize(serialized); + + if (value instanceof byte[]) { + assertArrayEquals((byte[]) value, (byte[]) deserialized.getPath().getValue()); + } else { + assertEquals(value, deserialized.getPath().getValue()); + } + assertArrayEquals(dataValue, deserialized.getValue()); + } + + private static Stream provideKeyTypes() { + UUID testUuid = UUID.randomUUID(); + return Stream.of( + Arguments.of(KeyType.NULL, null), + Arguments.of(KeyType.STRING, "test_string"), + Arguments.of(KeyType.LONG, 12345L), + Arguments.of(KeyType.FLOAT, 3.14f), + Arguments.of(KeyType.DOUBLE, 2.71828), + Arguments.of(KeyType.BOOLEAN, true), + Arguments.of(KeyType.BOOLEAN, false), + Arguments.of(KeyType.BYTES, new byte[]{1, 2, 3, (byte) 0xFF}), + Arguments.of(KeyType.UUID, testUuid) + ); + } + + @Test + void testSerializeWithIntegerForLongKeyType() { + KeySpace root = new KeySpace( + new KeySpaceDirectory("root", KeyType.STRING, "root") + .addSubdirectory(new KeySpaceDirectory("long_dir", KeyType.LONG))); + + KeySpacePath rootPath = root.path("root"); + // Pass an Integer for a LONG key type + KeySpacePath fullPath = rootPath.add("long_dir", 42); + byte[] value = new byte[]{1}; + + DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, value); + + KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); + ByteString serialized = serializer.serialize(data); + + DataInKeySpacePath deserialized = serializer.deserialize(serialized); + + // Should deserialize as Long + assertEquals(42L, deserialized.getPath().getValue()); + } + + @Test + void testSerializeRootOnly() { + KeySpace root = new KeySpace( + new KeySpaceDirectory("root", KeyType.STRING, "rootval")); + + KeySpacePath rootPath = root.path("root"); + byte[] value = new byte[]{5, 6, 7}; + + DataInKeySpacePath data = new DataInKeySpacePath(rootPath, null, value); + + KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); + ByteString serialized = serializer.serialize(data); + + DataInKeySpacePath deserialized = serializer.deserialize(serialized); + + assertEquals("rootval", deserialized.getPath().getValue()); + assertArrayEquals(value, deserialized.getValue()); + } + + @Test + void testSerializePathNotContainedInRoot() { + KeySpace root = new KeySpace( + new KeySpaceDirectory("app1", KeyType.STRING, "app1") + .addSubdirectory(new KeySpaceDirectory("tenant", KeyType.STRING))); + + KeySpace otherRoot = new KeySpace( + new KeySpaceDirectory("app2", KeyType.STRING, "app2") + .addSubdirectory(new KeySpaceDirectory("tenant", KeyType.STRING))); + + KeySpacePath rootPath = root.path("app1"); + KeySpacePath otherPath = otherRoot.path("app2").add("tenant", "t1"); + byte[] value = new byte[]{1}; + + DataInKeySpacePath data = new DataInKeySpacePath(otherPath, null, value); + + KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); + + assertThrows(RecordCoreArgumentException.class, () -> serializer.serialize(data)); + } + + @Test + void testSerializeWithEmptyValue() { + KeySpace root = new KeySpace( + new KeySpaceDirectory("root", KeyType.STRING, "root")); + + KeySpacePath rootPath = root.path("root"); + byte[] value = new byte[]{}; + + DataInKeySpacePath data = new DataInKeySpacePath(rootPath, null, value); + + KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); + ByteString serialized = serializer.serialize(data); + + DataInKeySpacePath deserialized = serializer.deserialize(serialized); + + assertArrayEquals(value, deserialized.getValue()); + assertEquals(0, deserialized.getValue().length); + } + + @Test + void testDeserializeInvalidProto() { + KeySpace root = new KeySpace( + new KeySpaceDirectory("root", KeyType.STRING, "root")); + + KeySpacePath rootPath = root.path("root"); + KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); + + // Create invalid ByteString + ByteString invalid = ByteString.copyFrom(new byte[]{1, 2, 3, 4, 5}); + + assertThrows(RecordCoreArgumentException.class, () -> serializer.deserialize(invalid)); + } + + @Test + void testRoundTripWithComplexRemainder() { + KeySpace root = new KeySpace( + new KeySpaceDirectory("db", KeyType.STRING, "database")); + + KeySpacePath rootPath = root.path("db"); + Tuple remainder = Tuple.from("string", 123L, 3.14, true, new byte[]{1, 2}); + byte[] value = new byte[]{9, 8, 7}; + + DataInKeySpacePath data = new DataInKeySpacePath(rootPath, remainder, value); + + KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); + ByteString serialized = serializer.serialize(data); + + DataInKeySpacePath deserialized = serializer.deserialize(serialized); + + assertEquals(remainder, deserialized.getRemainder()); + assertArrayEquals(value, deserialized.getValue()); + } + + @Test + void testSerializeNullKeyType() { + KeySpace root = new KeySpace( + new KeySpaceDirectory("root", KeyType.STRING, "root") + .addSubdirectory(new KeySpaceDirectory("null_dir", KeyType.NULL))); + + KeySpacePath rootPath = root.path("root"); + KeySpacePath fullPath = rootPath.add("null_dir"); + byte[] value = new byte[]{1}; + + DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, value); + + KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); + ByteString serialized = serializer.serialize(data); + + DataInKeySpacePath deserialized = serializer.deserialize(serialized); + + assertNull(deserialized.getPath().getValue()); + } + + @Test + void testSerializeNullKeyTypeWithNonNullValue() { + KeySpace root = new KeySpace( + new KeySpaceDirectory("root", KeyType.STRING, "root") + .addSubdirectory(new KeySpaceDirectory("null_dir", KeyType.NULL))); + + KeySpacePath rootPath = root.path("root"); + // Manually create a path with incorrect value for NULL type + KeySpacePath fullPath = rootPath.add("null_dir", "should_be_null"); + byte[] value = new byte[]{1}; + + DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, value); + + KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); + + assertThrows(RecordCoreArgumentException.class, () -> serializer.serialize(data)); + } + + @Test + void testSerializeDeserializeDifferentRootSameSubHierarchy() { + // Create a KeySpace with two identical sub-hierarchies under different roots + KeySpace keySpace = new KeySpace( + new KeySpaceDirectory("source_app", KeyType.STRING, "app1") + .addSubdirectory(new KeySpaceDirectory("tenant", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("record", KeyType.LONG))), + new KeySpaceDirectory("dest_app", KeyType.STRING, "app2") + .addSubdirectory(new KeySpaceDirectory("tenant", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("record", KeyType.LONG)))); + + // Create data in source hierarchy + KeySpacePath sourcePath = keySpace.path("source_app") + .add("tenant", "tenant1") + .add("record", 42L); + byte[] value = new byte[]{10, 20, 30}; + DataInKeySpacePath sourceData = new DataInKeySpacePath(sourcePath, null, value); + + // Serialize from source + KeySpacePathSerializer sourceSerializer = new KeySpacePathSerializer(keySpace.path("source_app")); + ByteString serialized = sourceSerializer.serialize(sourceData); + + // Deserialize to destination + KeySpacePathSerializer destSerializer = new KeySpacePathSerializer(keySpace.path("dest_app")); + DataInKeySpacePath deserializedData = destSerializer.deserialize(serialized); + + // Verify the logical path values are preserved (but root is different) + assertEquals("dest_app", deserializedData.getPath().getParent().getParent().getDirectoryName()); + assertEquals("tenant1", deserializedData.getPath().getParent().getValue()); + assertEquals(42L, deserializedData.getPath().getValue()); + assertArrayEquals(value, deserializedData.getValue()); + } + + @Test + void testDeserializeIncompatiblePath() { + // Create a KeySpace with two roots having incompatible structures + KeySpace keySpace = new KeySpace( + new KeySpaceDirectory("source", KeyType.STRING, "src") + .addSubdirectory(new KeySpaceDirectory("level1", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("level2", KeyType.LONG))), + new KeySpaceDirectory("dest", KeyType.STRING, "dst") + .addSubdirectory(new KeySpaceDirectory("level1", KeyType.STRING))); + + // Create data in source + KeySpacePath sourcePath = keySpace.path("source") + .add("level1", "value1") + .add("level2", 100L); + byte[] value = new byte[]{1, 2}; + DataInKeySpacePath sourceData = new DataInKeySpacePath(sourcePath, null, value); + + // Serialize from source + KeySpacePathSerializer sourceSerializer = new KeySpacePathSerializer(keySpace.path("source")); + ByteString serialized = sourceSerializer.serialize(sourceData); + + // Try to deserialize to incompatible destination - should fail + KeySpacePathSerializer destSerializer = new KeySpacePathSerializer(keySpace.path("dest")); + assertThrows(NoSuchDirectoryException.class, () -> destSerializer.deserialize(serialized)); + } + + @Test + void testDeserializeWithExtraSubdirectoriesInDestination() { + // Create a KeySpace with two roots where destination has extra subdirectories + KeySpace keySpace = new KeySpace( + new KeySpaceDirectory("source", KeyType.STRING, "src") + .addSubdirectory(new KeySpaceDirectory("data", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("users", KeyType.STRING))), + new KeySpaceDirectory("dest", KeyType.STRING, "dst") + .addSubdirectory(new KeySpaceDirectory("data", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("users", KeyType.STRING)) + .addSubdirectory(new KeySpaceDirectory("groups", KeyType.LONG)) + .addSubdirectory(new KeySpaceDirectory("settings", KeyType.BOOLEAN)))); + + // Create data using only the "data/users" path + KeySpacePath sourcePath = keySpace.path("source").add("data", "records").add("users", "user123"); + byte[] value = new byte[]{5, 6, 7, 8}; + DataInKeySpacePath sourceData = new DataInKeySpacePath(sourcePath, null, value); + + // Serialize from source + KeySpacePathSerializer sourceSerializer = new KeySpacePathSerializer(keySpace.path("source")); + ByteString serialized = sourceSerializer.serialize(sourceData); + + // Deserialize to destination with extra directories - should succeed + // The extra directories (groups, settings) are not part of the data path, so deserialization should work + KeySpacePathSerializer destSerializer = new KeySpacePathSerializer(keySpace.path("dest")); + DataInKeySpacePath deserializedData = destSerializer.deserialize(serialized); + + // Verify data is correctly deserialized + assertEquals("user123", deserializedData.getPath().getValue()); + assertEquals("users", deserializedData.getPath().getDirectoryName()); + assertEquals("records", deserializedData.getPath().getParent().getValue()); + assertArrayEquals(value, deserializedData.getValue()); + } + + @Test + void testDeserializeSameStructureDifferentRootValue() { + // Create a KeySpace with a single root having multiple children with identical structures + KeySpace keySpace = new KeySpace( + new KeySpaceDirectory("environments", KeyType.NULL) + .addSubdirectory(new KeySpaceDirectory("production", KeyType.STRING, "prod") + .addSubdirectory(new KeySpaceDirectory("database", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("table", KeyType.STRING)))) + .addSubdirectory(new KeySpaceDirectory("staging", KeyType.STRING, "stage") + .addSubdirectory(new KeySpaceDirectory("database", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("table", KeyType.STRING))))); + + // Create data in production + KeySpacePath sourcePath = keySpace.path("environments").add("production") + .add("database", "db1") + .add("table", "users"); + Tuple remainder = Tuple.from("primary_key", 12345L); + byte[] value = new byte[]{(byte) 0xFF, 0x00, 0x11}; + DataInKeySpacePath sourceData = new DataInKeySpacePath(sourcePath, remainder, value); + + // Serialize from production + KeySpacePathSerializer sourceSerializer = new KeySpacePathSerializer(keySpace.path("environments").add("production")); + ByteString serialized = sourceSerializer.serialize(sourceData); + + // Deserialize to staging + KeySpacePathSerializer destSerializer = new KeySpacePathSerializer(keySpace.path("environments").add("staging")); + DataInKeySpacePath deserializedData = destSerializer.deserialize(serialized); + + // Verify the root value changed but path structure and data preserved + assertEquals("staging", deserializedData.getPath().getParent().getParent().getDirectoryName()); + assertEquals("stage", deserializedData.getPath().getParent().getParent().getValue()); + + // The logical path values should be preserved + assertEquals("db1", deserializedData.getPath().getParent().getValue()); + assertEquals("users", deserializedData.getPath().getValue()); + assertEquals(remainder, deserializedData.getRemainder()); + assertArrayEquals(value, deserializedData.getValue()); + } +} From 67854b9de02640bae50a0db945fe4c55aac42ea8 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Sun, 9 Nov 2025 15:55:53 -0500 Subject: [PATCH 03/12] Add method to KeySpaceDirectory to validate potential values Ideally this would be called when you call `KeySpacePath.add`, but, since we've never had this validation, I'm scared that adding it would break someone's production environment, and so I want to limit it to the new feature. Later we can rollout this validation universally in a controlled fashion. --- .../keyspace/DirectoryLayerDirectory.java | 13 ++ .../keyspace/KeySpaceDirectory.java | 41 ++++++ .../keyspace/KeySpaceDirectoryTest.java | 121 ++++++++++++++++++ 3 files changed, 175 insertions(+) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DirectoryLayerDirectory.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DirectoryLayerDirectory.java index 73d2c32a98..ed3be326bf 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DirectoryLayerDirectory.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DirectoryLayerDirectory.java @@ -30,6 +30,7 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; +import java.util.Objects; import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.function.Function; @@ -148,6 +149,18 @@ public DirectoryLayerDirectory(@Nonnull String name, @Nullable Object value, this.createHooks = createHooks; } + @Override + protected boolean isValueValid(@Nullable Object value) { + // DirectoryLayerDirectory accepts both String (logical names) and Long (directory layer values), + // but we're making this method stricter, and I hope that using Long is only for a handful of tests, + // despite comments saying that the resolved value should be allowed. + if (value instanceof String) { + // If this directory has a constant value, check that the provided value matches it + return getValue() == KeySpaceDirectory.ANY_VALUE || Objects.equals(getValue(), value); + } + return false; + } + @Override protected void validateConstant(@Nullable Object value) { if (!(value instanceof String)) { diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java index f36b073276..4929227a64 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java @@ -176,6 +176,47 @@ LogMessageKeys.DIR_NAME, getName(), } } + /** + * Validate that the given value can be used with this directory. + * @param value a potential value + * @throws RecordCoreArgumentException if the value is not valid + */ + protected void validateValue(@Nullable Object value) { + // Validate that the value is valid for this directory + if (!isValueValid(value)) { + throw new RecordCoreArgumentException("Value does not match directory requirements") + .addLogInfo(LogMessageKeys.DIR_NAME, name, + LogMessageKeys.EXPECTED_TYPE, getKeyType(), + LogMessageKeys.ACTUAL, value, + "actual_type", value == null ? "null" : value.getClass().getName(), + "expected_value", getValue() != KeySpaceDirectory.ANY_VALUE ? getValue() : "any"); + } + } + + /** + * Checks if the provided value is valid for this directory. This method can be overridden by subclasses + * to provide custom validation logic. For example, {@link DirectoryLayerDirectory} accepts both String + * (logical names) and Long (directory layer values) even though its key type is LONG. + * + * @param value the value to validate + * @return {@code true} if the value is valid for this directory + */ + protected boolean isValueValid(@Nullable Object value) { + // Check if value matches the key type + if (!keyType.isMatch(value)) { + return false; + } + // If this directory has a constant value, check that the provided value matches it + if (this.value != ANY_VALUE) { + if (this.value instanceof byte[] && value instanceof byte[]) { + return Arrays.equals((byte[]) this.value, (byte[]) value); + } else { + return Objects.equals(this.value, value); + } + } + return true; + } + /** * Given a position in a tuple, checks to see if this directory is compatible with the value at the * position, returning either a path indicating that it was compatible or nothing if it was not compatible. diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java index 6c9d2d07d6..2c1144865b 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java @@ -43,10 +43,13 @@ import com.apple.test.Tags; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -67,6 +70,7 @@ import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.IntStream; +import java.util.stream.Stream; import static com.apple.foundationdb.record.TestHelpers.assertThrows; import static com.apple.foundationdb.record.TestHelpers.eventually; @@ -81,6 +85,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -1558,6 +1563,115 @@ private List resolveBatch(FDBRecordContext context, String... names) { return AsyncUtil.getAll(futures).join(); } + private static final Map VALUES = Map.of( + KeySpaceDirectory.KeyType.NULL, new KeyPathValues(() -> null, + // TODO add("v" null) should work for a constant of null and we should test that + List.of(), List.of("not_null", 42, true)), + KeySpaceDirectory.KeyType.STRING, new KeyPathValues(() -> "foo", + List.of("bar", ""), List.of(3, "foo".getBytes(), true, 3.14)), + KeySpaceDirectory.KeyType.LONG, new KeyPathValues(() -> 42L, + List.of(100L, 0L, -5L, 123, ((long)Integer.MAX_VALUE) + 3), List.of("not_long", 3.14, true, new byte[] {1})), + KeySpaceDirectory.KeyType.FLOAT, new KeyPathValues(() -> 3.14f, + List.of(0.0f, -2.5f, 1.5f), List.of("not_float", 42, true, new byte[] {1}, Double.MAX_VALUE)), + KeySpaceDirectory.KeyType.DOUBLE, new KeyPathValues(() -> 2.71828, + List.of(0.0, -1.5, 3.14159), List.of("not_double", 42, true, new byte[] {1}, 1.5f)), + KeySpaceDirectory.KeyType.BOOLEAN, new KeyPathValues(() -> true, + List.of(false), + List.of("true", 1, 0, new byte[] {1})), + KeySpaceDirectory.KeyType.BYTES, new KeyPathValues(() -> new byte[] {1, 2, 3}, + List.of(new byte[] {4, 5}, new byte[] {(byte)0xFF}, new byte[0]), + List.of("not_bytes", 42, true, 3.14)), + KeySpaceDirectory.KeyType.UUID, new KeyPathValues(() -> UUID.fromString("12345678-1234-1234-1234-123456789abc"), + List.of(UUID.fromString("00000000-0000-0000-0000-000000000000")), + List.of("not_uuid", 42, true, new byte[] {1})) + ); + + @Test + void testAllKeyTypesAreCovered() { + // Ensure that all KeyTypes have test data defined + for (KeySpaceDirectory.KeyType keyType : KeySpaceDirectory.KeyType.values()) { + assertNotNull(VALUES.get(keyType), "KeyType " + keyType + " is not covered in VALUES map"); + } + } + + static Stream testValidateConstant() { + return VALUES.entrySet().stream() + // Skip BYTES for constant value testing since array equality doesn't use .equals() + .flatMap(entry -> Stream.concat( + Stream.concat(entry.getValue().otherValidValues.stream(), entry.getValue().invalidValues.stream()) + .map(valueToAdd -> Arguments.of(entry.getKey(), entry.getValue().value.get(), valueToAdd, false)), + Stream.of(Arguments.of(entry.getKey(), entry.getValue().value.get(), entry.getValue().value.get(), true)))); + } + + @ParameterizedTest + @MethodSource + void testValidateConstant(KeySpaceDirectory.KeyType keyType, Object constantValue, Object valueToAdd, boolean isValid) { + final KeySpaceDirectory directory = new KeySpaceDirectory("test_dir", keyType, constantValue); + if (isValid) { + // Should succeed - value matches constant + directory.validateValue(valueToAdd); + } else { + // Should fail - value doesn't match constant or is invalid type + Assertions.assertThrows(RecordCoreArgumentException.class, () -> directory.validateValue(valueToAdd)); + } + } + + static Stream testValidationValidValues() { + return VALUES.entrySet().stream().flatMap(entry -> + Stream.concat( + Stream.of(entry.getValue().value.get()), + entry.getValue().otherValidValues.stream()) + .map(value -> Arguments.of(entry.getKey(), value))); + } + + @ParameterizedTest + @MethodSource + void testValidationValidValues(KeySpaceDirectory.KeyType keyType, Object value) { + // should succeed + new KeySpaceDirectory("test_dir", keyType).validateValue(value); + } + + static Stream testValidationInvalidValues() { + return VALUES.entrySet().stream().flatMap(entry -> + entry.getValue().invalidValues.stream() + .map(value -> Arguments.of(entry.getKey(), value))); + } + + @ParameterizedTest + @MethodSource + void testValidationInvalidValues(KeySpaceDirectory.KeyType keyType, Object value) { + final KeySpaceDirectory directory = new KeySpaceDirectory("test_dir", keyType); + + // Should fail - value doesn't match the key type + Assertions.assertThrows(RecordCoreArgumentException.class, () -> directory.validateValue(value)); + } + + static Stream testValidationNullToNonNullType() { + return Stream.of(KeySpaceDirectory.KeyType.values()) + .filter(type -> type != KeySpaceDirectory.KeyType.NULL); + } + + @ParameterizedTest + @MethodSource + void testValidationNullToNonNullType(KeySpaceDirectory.KeyType keyType) { + final KeySpaceDirectory directory = new KeySpaceDirectory("test_dir", keyType); + + // Should fail - null not allowed for non-NULL types + Assertions.assertThrows(RecordCoreArgumentException.class, () -> directory.validateValue(null)); + } + + static final class KeyPathValues { + private final Supplier value; + private final List otherValidValues; + private final List invalidValues; + + KeyPathValues(final Supplier value, final List otherValidValues, final List invalidValues) { + this.value = value; + this.otherValidValues = otherValidValues; + this.invalidValues = invalidValues; + } + } + /** Used to validate wrapping of path names. */ public static class PathA extends KeySpacePathWrapper { public PathA(KeySpacePath parent) { @@ -1593,6 +1707,13 @@ public ConstantResolvingKeySpaceDirectory(String name, KeyType keyType, Object c protected CompletableFuture toTupleValueAsyncImpl(@Nonnull FDBRecordContext context, Object value) { return CompletableFuture.completedFuture(new PathValue(resolver.apply(value))); } + + @Override + protected boolean isValueValid(@Nullable Object value) { + // ConstantResolvingKeySpaceDirectory accepts any value and transforms it via the resolver + // The resolved value must match the expected key type + return true; + } } } From 2c30eb7f2ddf28a2dc5b655aab133c003813ddc2 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Wed, 12 Nov 2025 16:35:11 -0500 Subject: [PATCH 04/12] Validate that deserializing verifies value is appropriate --- .../keyspace/KeySpacePathSerializer.java | 1 + .../keyspace/KeySpacePathSerializerTest.java | 69 +++++++++++-------- 2 files changed, 41 insertions(+), 29 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java index 760cb3fce0..7e2dc4982e 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java @@ -91,6 +91,7 @@ private DataInKeySpacePath deserialize(@Nonnull KeySpaceProto.DataInKeySpacePath // Add each path entry from the proto for (KeySpaceProto.KeySpacePathEntry entry : proto.getPathList()) { Object value = deserializeValue(entry); + path.getDirectory().getSubdirectory(entry.getName()).validateValue(value); path = path.add(entry.getName(), value); } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java index 18a8ae00b6..7f463a8148 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java @@ -24,11 +24,13 @@ import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpaceDirectory.KeyType; import com.apple.foundationdb.tuple.Tuple; import com.google.protobuf.ByteString; +import org.junit.jupiter.api.Named; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import javax.annotation.Nullable; import java.util.UUID; import java.util.stream.Stream; @@ -292,34 +294,39 @@ void testSerializeNullKeyType() { assertNull(deserialized.getPath().getValue()); } - @Test - void testSerializeNullKeyTypeWithNonNullValue() { - KeySpace root = new KeySpace( - new KeySpaceDirectory("root", KeyType.STRING, "root") - .addSubdirectory(new KeySpaceDirectory("null_dir", KeyType.NULL))); - - KeySpacePath rootPath = root.path("root"); - // Manually create a path with incorrect value for NULL type - KeySpacePath fullPath = rootPath.add("null_dir", "should_be_null"); - byte[] value = new byte[]{1}; - - DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, value); - - KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); - - assertThrows(RecordCoreArgumentException.class, () -> serializer.serialize(data)); + static Stream testSerializeDeserializeDifferentRoot() { + return Stream.of( + Arguments.of(Named.of("Same", new KeySpaceDirectory("tenant", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("record", KeyType.LONG))), + Named.of("successful", null)), + Arguments.of(Named.of("Different constant", + new KeySpaceDirectory("tenant", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("record", KeyType.LONG, 104L))), + RecordCoreArgumentException.class), + Arguments.of(Named.of("Different type", + new KeySpaceDirectory("tenant", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("record", KeyType.STRING))), + RecordCoreArgumentException.class), + Arguments.of(Named.of("Null type", + new KeySpaceDirectory("tenant", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("record", KeyType.NULL))), + RecordCoreArgumentException.class), + Arguments.of(Named.of("Different name", + new KeySpaceDirectory("tenant", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("compact disc", KeyType.STRING))), + NoSuchDirectoryException.class)); } - @Test - void testSerializeDeserializeDifferentRootSameSubHierarchy() { - // Create a KeySpace with two identical sub-hierarchies under different roots + @ParameterizedTest + @MethodSource + void testSerializeDeserializeDifferentRoot(KeySpaceDirectory destDirectory, + @Nullable Class errorType) { KeySpace keySpace = new KeySpace( new KeySpaceDirectory("source_app", KeyType.STRING, "app1") .addSubdirectory(new KeySpaceDirectory("tenant", KeyType.STRING) .addSubdirectory(new KeySpaceDirectory("record", KeyType.LONG))), new KeySpaceDirectory("dest_app", KeyType.STRING, "app2") - .addSubdirectory(new KeySpaceDirectory("tenant", KeyType.STRING) - .addSubdirectory(new KeySpaceDirectory("record", KeyType.LONG)))); + .addSubdirectory(destDirectory)); // Create data in source hierarchy KeySpacePath sourcePath = keySpace.path("source_app") @@ -330,17 +337,21 @@ void testSerializeDeserializeDifferentRootSameSubHierarchy() { // Serialize from source KeySpacePathSerializer sourceSerializer = new KeySpacePathSerializer(keySpace.path("source_app")); - ByteString serialized = sourceSerializer.serialize(sourceData); + ByteString serialized1 = sourceSerializer.serialize(sourceData); // Deserialize to destination - KeySpacePathSerializer destSerializer = new KeySpacePathSerializer(keySpace.path("dest_app")); - DataInKeySpacePath deserializedData = destSerializer.deserialize(serialized); + KeySpacePathSerializer destSerializer1 = new KeySpacePathSerializer(keySpace.path("dest_app")); - // Verify the logical path values are preserved (but root is different) - assertEquals("dest_app", deserializedData.getPath().getParent().getParent().getDirectoryName()); - assertEquals("tenant1", deserializedData.getPath().getParent().getValue()); - assertEquals(42L, deserializedData.getPath().getValue()); - assertArrayEquals(value, deserializedData.getValue()); + if (errorType == null) { + DataInKeySpacePath deserializedData = destSerializer1.deserialize(serialized1); + + assertEquals("dest_app", deserializedData.getPath().getParent().getParent().getDirectoryName()); + assertEquals("tenant1", deserializedData.getPath().getParent().getValue()); + assertEquals(42L, deserializedData.getPath().getValue()); + assertArrayEquals(new byte[] {10, 20, 30}, deserializedData.getValue()); + } else { + assertThrows(errorType, () -> destSerializer1.deserialize(serialized1)); + } } @Test From fe3b2148851aed63dd3107076c2ff613b92a5d09 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Wed, 12 Nov 2025 16:35:34 -0500 Subject: [PATCH 05/12] Improve testing that verifies all key types serialize/deserialize --- .../keyspace/KeySpacePathSerializerTest.java | 52 +++++++++++++------ 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java index 7f463a8148..af3a1f5ede 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java @@ -31,7 +31,12 @@ import org.junit.jupiter.params.provider.MethodSource; import javax.annotation.Nullable; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; import java.util.UUID; +import java.util.stream.Collectors; import java.util.stream.Stream; import static org.junit.jupiter.api.Assertions.assertArrayEquals; @@ -45,6 +50,23 @@ */ class KeySpacePathSerializerTest { + private static final UUID TEST_UUID = UUID.randomUUID(); + private static final Map KEY_TYPE_TEST_VALUES; + + static { + // Using HashMap to allow null values (Map.of() doesn't allow nulls) + Map testValues = new HashMap<>(); + testValues.put(KeyType.NULL, null); + testValues.put(KeyType.STRING, "test_string"); + testValues.put(KeyType.LONG, 12345L); + testValues.put(KeyType.FLOAT, 3.14f); + testValues.put(KeyType.DOUBLE, 2.71828); + testValues.put(KeyType.BOOLEAN, true); + testValues.put(KeyType.BYTES, new byte[]{1, 2, 3, (byte) 0xFF}); + testValues.put(KeyType.UUID, TEST_UUID); + KEY_TYPE_TEST_VALUES = Collections.unmodifiableMap(testValues); + } + @Test void testSerializeAndDeserializeSimplePath() { KeySpace root = new KeySpace( @@ -118,8 +140,21 @@ void testSerializeAndDeserializeMultiLevelPath() { assertArrayEquals(value, deserialized.getValue()); } + private static Stream testSerializeDeserializeAllKeyTypes() { + return KEY_TYPE_TEST_VALUES.entrySet().stream() + .map(entry -> Arguments.of(entry.getKey(), entry.getValue())); + } + + @Test + void testKeyTypeTestValuesIncludesAllKeyTypes() { + // Verify that KEY_TYPE_TEST_VALUES contains all KeyType enum values + var allKeyTypes = Arrays.stream(KeyType.values()).collect(Collectors.toSet()); + var coveredKeyTypes = KEY_TYPE_TEST_VALUES.keySet(); + assertEquals(allKeyTypes, coveredKeyTypes); + } + @ParameterizedTest - @MethodSource("provideKeyTypes") + @MethodSource void testSerializeDeserializeAllKeyTypes(KeyType keyType, Object value) { KeySpace root = new KeySpace( new KeySpaceDirectory("root", KeyType.STRING, "root") @@ -144,21 +179,6 @@ void testSerializeDeserializeAllKeyTypes(KeyType keyType, Object value) { assertArrayEquals(dataValue, deserialized.getValue()); } - private static Stream provideKeyTypes() { - UUID testUuid = UUID.randomUUID(); - return Stream.of( - Arguments.of(KeyType.NULL, null), - Arguments.of(KeyType.STRING, "test_string"), - Arguments.of(KeyType.LONG, 12345L), - Arguments.of(KeyType.FLOAT, 3.14f), - Arguments.of(KeyType.DOUBLE, 2.71828), - Arguments.of(KeyType.BOOLEAN, true), - Arguments.of(KeyType.BOOLEAN, false), - Arguments.of(KeyType.BYTES, new byte[]{1, 2, 3, (byte) 0xFF}), - Arguments.of(KeyType.UUID, testUuid) - ); - } - @Test void testSerializeWithIntegerForLongKeyType() { KeySpace root = new KeySpace( From 76107ba2a7900c746b91d0c797d7c1b9896d08ad Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Wed, 12 Nov 2025 17:04:25 -0500 Subject: [PATCH 06/12] Address teamscale warnings --- .../foundationdb/keyspace/KeySpacePathSerializer.java | 6 +++--- .../keyspace/KeySpacePathSerializerTest.java | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java index 7e2dc4982e..4a95a5db33 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java @@ -55,13 +55,13 @@ public KeySpacePathSerializer(@Nonnull final KeySpacePath root) { @Nonnull public ByteString serialize(@Nonnull DataInKeySpacePath data) { - KeySpaceProto.DataInKeySpacePath.Builder builder = KeySpaceProto.DataInKeySpacePath.newBuilder(); final List dataPath = data.getPath().flatten(); // two paths are only equal if their parents are equal, so we don't have to validate the whole prefix here if (dataPath.size() < root.size() || !dataPath.get(root.size() - 1).equals(root.get(root.size() - 1))) { throw new RecordCoreArgumentException("Data is not contained within root path"); } + KeySpaceProto.DataInKeySpacePath.Builder builder = KeySpaceProto.DataInKeySpacePath.newBuilder(); for (int i = root.size(); i < dataPath.size(); i++) { final KeySpacePath keySpacePath = dataPath.get(i); builder.addPath(serialize(keySpacePath)); @@ -138,8 +138,6 @@ private static Object deserializeValue(@Nonnull KeySpaceProto.KeySpacePathEntry @Nonnull private static KeySpaceProto.KeySpacePathEntry serialize(@Nonnull final KeySpacePath keySpacePath) { - KeySpaceProto.KeySpacePathEntry.Builder builder = KeySpaceProto.KeySpacePathEntry.newBuilder() - .setName(keySpacePath.getDirectoryName()); final Object value = keySpacePath.getValue(); final KeySpaceDirectory.KeyType keyType = keySpacePath.getDirectory().getKeyType(); @@ -158,6 +156,8 @@ private static KeySpaceProto.KeySpacePathEntry serialize(@Nonnull final KeySpace } } + KeySpaceProto.KeySpacePathEntry.Builder builder = KeySpaceProto.KeySpacePathEntry.newBuilder() + .setName(keySpacePath.getDirectoryName()); try { switch (keyType) { case NULL: diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java index af3a1f5ede..51c19323dc 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java @@ -140,11 +140,6 @@ void testSerializeAndDeserializeMultiLevelPath() { assertArrayEquals(value, deserialized.getValue()); } - private static Stream testSerializeDeserializeAllKeyTypes() { - return KEY_TYPE_TEST_VALUES.entrySet().stream() - .map(entry -> Arguments.of(entry.getKey(), entry.getValue())); - } - @Test void testKeyTypeTestValuesIncludesAllKeyTypes() { // Verify that KEY_TYPE_TEST_VALUES contains all KeyType enum values @@ -153,6 +148,11 @@ void testKeyTypeTestValuesIncludesAllKeyTypes() { assertEquals(allKeyTypes, coveredKeyTypes); } + static Stream testSerializeDeserializeAllKeyTypes() { + return KEY_TYPE_TEST_VALUES.entrySet().stream() + .map(entry -> Arguments.of(entry.getKey(), entry.getValue())); + } + @ParameterizedTest @MethodSource void testSerializeDeserializeAllKeyTypes(KeyType keyType, Object value) { From 50a9fedd04db9a35337acbb1f2f96905c182a622 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Wed, 12 Nov 2025 17:09:59 -0500 Subject: [PATCH 07/12] Remove outdated comments --- .../provider/foundationdb/keyspace/KeySpaceDirectoryTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java index 2c1144865b..845db82f77 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java @@ -1565,7 +1565,6 @@ private List resolveBatch(FDBRecordContext context, String... names) { private static final Map VALUES = Map.of( KeySpaceDirectory.KeyType.NULL, new KeyPathValues(() -> null, - // TODO add("v" null) should work for a constant of null and we should test that List.of(), List.of("not_null", 42, true)), KeySpaceDirectory.KeyType.STRING, new KeyPathValues(() -> "foo", List.of("bar", ""), List.of(3, "foo".getBytes(), true, 3.14)), @@ -1596,7 +1595,6 @@ void testAllKeyTypesAreCovered() { static Stream testValidateConstant() { return VALUES.entrySet().stream() - // Skip BYTES for constant value testing since array equality doesn't use .equals() .flatMap(entry -> Stream.concat( Stream.concat(entry.getValue().otherValidValues.stream(), entry.getValue().invalidValues.stream()) .map(valueToAdd -> Arguments.of(entry.getKey(), entry.getValue().value.get(), valueToAdd, false)), From bc59f6237d8c6396f8786a345558b97d49f12032 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Mon, 17 Nov 2025 09:16:49 -0500 Subject: [PATCH 08/12] Fix PMD warnings of using == and != `ANY_VALUE` should use reference equality, but it also doesn't override .equals so this doesn't really make a difference, but is less surprising to readers. The comparison in the logging was unnecessary, the toString for `ANY_VALUE` is `*any*` which works just as well. --- .../provider/foundationdb/keyspace/DirectoryLayerDirectory.java | 2 +- .../provider/foundationdb/keyspace/KeySpaceDirectory.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DirectoryLayerDirectory.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DirectoryLayerDirectory.java index ed3be326bf..ca98847328 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DirectoryLayerDirectory.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DirectoryLayerDirectory.java @@ -156,7 +156,7 @@ protected boolean isValueValid(@Nullable Object value) { // despite comments saying that the resolved value should be allowed. if (value instanceof String) { // If this directory has a constant value, check that the provided value matches it - return getValue() == KeySpaceDirectory.ANY_VALUE || Objects.equals(getValue(), value); + return Objects.equals(getValue(), KeySpaceDirectory.ANY_VALUE) || Objects.equals(getValue(), value); } return false; } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java index 4929227a64..0057b79e4b 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java @@ -189,7 +189,7 @@ protected void validateValue(@Nullable Object value) { LogMessageKeys.EXPECTED_TYPE, getKeyType(), LogMessageKeys.ACTUAL, value, "actual_type", value == null ? "null" : value.getClass().getName(), - "expected_value", getValue() != KeySpaceDirectory.ANY_VALUE ? getValue() : "any"); + "expected_value", getValue()); } } From b15582430bc1cb75f78fff0fb715bb484b086ca2 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Mon, 17 Nov 2025 15:51:31 -0500 Subject: [PATCH 09/12] Test DirectoryLayerDirectory validation --- .../keyspace/KeySpaceDirectoryTest.java | 58 +++++++++++++++++-- 1 file changed, 54 insertions(+), 4 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java index 845db82f77..06b525c4e1 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java @@ -40,6 +40,7 @@ import com.apple.foundationdb.tuple.Tuple; import com.apple.foundationdb.tuple.TupleHelpers; import com.apple.test.BooleanSource; +import com.apple.test.ParameterizedTestUtils; import com.apple.test.Tags; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; @@ -1640,8 +1641,8 @@ static Stream testValidationInvalidValues() { void testValidationInvalidValues(KeySpaceDirectory.KeyType keyType, Object value) { final KeySpaceDirectory directory = new KeySpaceDirectory("test_dir", keyType); - // Should fail - value doesn't match the key type - Assertions.assertThrows(RecordCoreArgumentException.class, () -> directory.validateValue(value)); + Assertions.assertThrows(RecordCoreArgumentException.class, () -> directory.validateValue(value), + "value doesn't match the key type"); } static Stream testValidationNullToNonNullType() { @@ -1654,8 +1655,57 @@ static Stream testValidationNullToNonNullType() { void testValidationNullToNonNullType(KeySpaceDirectory.KeyType keyType) { final KeySpaceDirectory directory = new KeySpaceDirectory("test_dir", keyType); - // Should fail - null not allowed for non-NULL types - Assertions.assertThrows(RecordCoreArgumentException.class, () -> directory.validateValue(null)); + Assertions.assertThrows(RecordCoreArgumentException.class, () -> directory.validateValue(null), + "null not allowed for non-NULL types"); + } + + static Stream testDirectoryLayerDirectoryValidateValueValidStrings() { + return Stream.of( + // ANY_VALUE directory - accepts any string + Arguments.of(false, "foo", true), + Arguments.of(false, "bar", true), + Arguments.of(false, "", true), + Arguments.of(false, "any_string_value", true), + + // Constant directory - only accepts matching constant + Arguments.of(true, "production", true), + Arguments.of(true, "staging", false), + Arguments.of(true, "", false), + Arguments.of(true, "other", false) + ); + } + + @ParameterizedTest + @MethodSource + void testDirectoryLayerDirectoryValidateValueValidStrings(boolean isConstant, String value, boolean shouldSucceed) { + DirectoryLayerDirectory directory = isConstant + ? new DirectoryLayerDirectory("test_dir", "production") + : new DirectoryLayerDirectory("test_dir"); + + if (shouldSucceed) { + directory.validateValue(value); + } else { + Assertions.assertThrows(RecordCoreArgumentException.class, () -> directory.validateValue(value), + "value doesn't match constant"); + } + } + + static Stream testDirectoryLayerDirectoryValidateValueInvalidTypes() { + return ParameterizedTestUtils.cartesianProduct( + ParameterizedTestUtils.booleans("isConstant"), + Stream.of(42L, 123, 3.14f, 2.718, true, new byte[]{1, 2, 3}, UUID.randomUUID(), null) + ); + } + + @ParameterizedTest + @MethodSource + void testDirectoryLayerDirectoryValidateValueInvalidTypes(boolean isConstant, Object value) { + DirectoryLayerDirectory directory = isConstant + ? new DirectoryLayerDirectory("test_dir", "production") + : new DirectoryLayerDirectory("test_dir"); + + Assertions.assertThrows(RecordCoreArgumentException.class, () -> directory.validateValue(value), + "DirectoryLayerDirectory only accepts Strings"); } static final class KeyPathValues { From 066819fbb7e2c0ab23a26e84c765e6bf74a2c1a7 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 2 Dec 2025 11:43:08 -0500 Subject: [PATCH 10/12] Some cleanup; and add tests of serialize/deserialize with a constant --- .../keyspace/DirectoryLayerDirectory.java | 2 +- .../keyspace/KeySpaceDirectory.java | 16 +- .../keyspace/KeySpacePathSerializer.java | 28 ++- .../src/main/proto/keyspace.proto | 4 +- .../keyspace/KeySpaceDirectoryTest.java | 4 +- .../keyspace/KeySpacePathSerializerTest.java | 169 +++++------------- 6 files changed, 89 insertions(+), 134 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DirectoryLayerDirectory.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DirectoryLayerDirectory.java index ca98847328..944fd1b549 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DirectoryLayerDirectory.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/DirectoryLayerDirectory.java @@ -150,7 +150,7 @@ public DirectoryLayerDirectory(@Nonnull String name, @Nullable Object value, } @Override - protected boolean isValueValid(@Nullable Object value) { + public boolean isValueValid(@Nullable Object value) { // DirectoryLayerDirectory accepts both String (logical names) and Long (directory layer values), // but we're making this method stricter, and I hope that using Long is only for a handful of tests, // despite comments saying that the resolved value should be allowed. diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java index 0057b79e4b..5b45f69aff 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java @@ -178,10 +178,17 @@ LogMessageKeys.DIR_NAME, getName(), /** * Validate that the given value can be used with this directory. + *

+ * Ideally this would be called as part of {@link KeySpacePath#add(String, Object)} to ensure the provided value + * is valid, but that code has existed for a long time, so it's possible clients are adding without respecting + * the type. You should call this before calling add to make sure you don't have the same mistakes. At some point + * this will be embedded in {@code add} once there's some confidence that it won't break anyone's environments. + *

* @param value a potential value * @throws RecordCoreArgumentException if the value is not valid */ - protected void validateValue(@Nullable Object value) { + @API(API.Status.EXPERIMENTAL) + public void validateValue(@Nullable Object value) { // Validate that the value is valid for this directory if (!isValueValid(value)) { throw new RecordCoreArgumentException("Value does not match directory requirements") @@ -195,13 +202,14 @@ LogMessageKeys.EXPECTED_TYPE, getKeyType(), /** * Checks if the provided value is valid for this directory. This method can be overridden by subclasses - * to provide custom validation logic. For example, {@link DirectoryLayerDirectory} accepts both String - * (logical names) and Long (directory layer values) even though its key type is LONG. + * to provide custom validation logic. For example, {@link DirectoryLayerDirectory} accepts String + * values (logical names) even though its key type is LONG. * * @param value the value to validate * @return {@code true} if the value is valid for this directory */ - protected boolean isValueValid(@Nullable Object value) { + @API(API.Status.EXPERIMENTAL) + public boolean isValueValid(@Nullable Object value) { // Check if value matches the key type if (!keyType.isMatch(value)) { return false; diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java index 4a95a5db33..5269ee1477 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java @@ -34,11 +34,11 @@ /** * Class for serializing/deserializing between {@link DataInKeySpacePath} and {@link KeySpaceProto.DataInKeySpacePath}. *

- * This will serialize relative to a root path, such that the serialized form is relative to that path. This can be - * useful to both: + * This will serialize relative to a root path, such that the serialized form is relative to that path. *

    - *
  • Reduce the size of the serialized data, particularly when you have a lot of these.
    • - *
  • Allowing as an intermediate if you have two identical sub-hierarchies in your {@link KeySpace}.
    • + *
  • This reduces the size of the serialized data, which is particularly important when you have a lot of these.
    • + *
  • This allows the serialized form to be used as an intermediate if you have two identical sub-hierarchies + * in your {@link KeySpace}.
    • *
*

* @@ -49,10 +49,20 @@ public class KeySpacePathSerializer { @Nonnull private final List root; + /** + * Constructor a new serializer for serializing relative to a given root path. + * @param root a path which is a parent path of all the data you want to serialize + */ public KeySpacePathSerializer(@Nonnull final KeySpacePath root) { this.root = root.flatten(); } + /** + * Serialize the given data relative to the root. + * @param data a piece of data that is contained within the root + * @return the data serialized relative to the root + * @throws RecordCoreArgumentException if the given data is not contained within the root + */ @Nonnull public ByteString serialize(@Nonnull DataInKeySpacePath data) { final List dataPath = data.getPath().flatten(); @@ -73,6 +83,16 @@ public ByteString serialize(@Nonnull DataInKeySpacePath data) { return builder.build().toByteString(); } + /** + * Deserialize data relative to the root. + *

+ * Note: The given data does not need to have come from the same path, but all sub-paths must be valid. + *

+ * @param bytes a serialized form of {@link DataInKeySpacePath} as provided by {@link #serialize(DataInKeySpacePath)} + * @return the deserialized data + * @throws RecordCoreArgumentException if the bytes cannot be parsed, or one of the path entries is not valid + * @throws NoSuchDirectoryException if it refers to a directory that doesn't exist within the root + */ @Nonnull public DataInKeySpacePath deserialize(@Nonnull ByteString bytes) { try { diff --git a/fdb-record-layer-core/src/main/proto/keyspace.proto b/fdb-record-layer-core/src/main/proto/keyspace.proto index 2f64e90452..e4c933a340 100644 --- a/fdb-record-layer-core/src/main/proto/keyspace.proto +++ b/fdb-record-layer-core/src/main/proto/keyspace.proto @@ -43,8 +43,10 @@ message KeySpacePathEntry { optional bool booleanValue = 8; optional UUID uuid = 9; - message UUID { // TODO find out why we use fixed64 and not just int64 + message UUID { // 2 64-bit fields is two tags, the same as 1 bytes field with a length of 16 would be. + // Using int64 would use more space for a lot of numbers since the uuid is random, it could use up to 10 bytes + // instead of the fixed 8 bytes (plus the tag). // fixed64 would be closer to how these are really used, but would fail the unsigned validator. optional sfixed64 most_significant_bits = 1; optional sfixed64 least_significant_bits = 2; diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java index 06b525c4e1..b6b5b518d2 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java @@ -436,7 +436,7 @@ public void testAllTypesAnyValues() throws Exception { } } - public KeyTypeValue pickDifferentType(KeyType keyType) { + private KeyTypeValue pickDifferentType(KeyType keyType) { while (true) { KeyTypeValue kv = valueOfEveryType.get(random.nextInt(valueOfEveryType.size())); if (kv.keyType != keyType) { @@ -1757,7 +1757,7 @@ protected CompletableFuture toTupleValueAsyncImpl(@Nonnull FDBRecordC } @Override - protected boolean isValueValid(@Nullable Object value) { + public boolean isValueValid(@Nullable Object value) { // ConstantResolvingKeySpaceDirectory accepts any value and transforms it via the resolver // The resolved value must match the expected key type return true; diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java index 51c19323dc..81bb59d1ab 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java @@ -23,6 +23,7 @@ import com.apple.foundationdb.record.RecordCoreArgumentException; import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpaceDirectory.KeyType; import com.apple.foundationdb.tuple.Tuple; +import com.apple.test.ParameterizedTestUtils; import com.google.protobuf.ByteString; import org.junit.jupiter.api.Named; import org.junit.jupiter.api.Test; @@ -30,6 +31,7 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import javax.annotation.Nonnull; import javax.annotation.Nullable; import java.util.Arrays; import java.util.Collections; @@ -67,6 +69,14 @@ class KeySpacePathSerializerTest { KEY_TYPE_TEST_VALUES = Collections.unmodifiableMap(testValues); } + @Test + void testKeyTypeTestValuesIncludesAllKeyTypes() { + // Verify that KEY_TYPE_TEST_VALUES contains all KeyType enum values + var allKeyTypes = Arrays.stream(KeyType.values()).collect(Collectors.toSet()); + var coveredKeyTypes = KEY_TYPE_TEST_VALUES.keySet(); + assertEquals(allKeyTypes, coveredKeyTypes); + } + @Test void testSerializeAndDeserializeSimplePath() { KeySpace root = new KeySpace( @@ -78,11 +88,7 @@ void testSerializeAndDeserializeSimplePath() { byte[] value = new byte[]{1, 2, 3, 4}; DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, value); - - KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); - ByteString serialized = serializer.serialize(data); - - DataInKeySpacePath deserialized = serializer.deserialize(serialized); + DataInKeySpacePath deserialized = serializeAndDeserialize(rootPath, data); assertEquals(fullPath.getDirectoryName(), deserialized.getPath().getDirectoryName()); assertEquals("tenant1", deserialized.getPath().getValue()); @@ -102,11 +108,7 @@ void testSerializeAndDeserializeWithRemainder() { byte[] value = new byte[]{10, 20, 30}; DataInKeySpacePath data = new DataInKeySpacePath(fullPath, remainder, value); - - KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); - ByteString serialized = serializer.serialize(data); - - DataInKeySpacePath deserialized = serializer.deserialize(serialized); + DataInKeySpacePath deserialized = serializeAndDeserialize(rootPath, data); assertEquals("store1", deserialized.getPath().getValue()); assertNotNull(deserialized.getRemainder()); @@ -130,46 +132,35 @@ void testSerializeAndDeserializeMultiLevelPath() { byte[] value = new byte[]{100}; DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, value); - - KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); - ByteString serialized = serializer.serialize(data); - - DataInKeySpacePath deserialized = serializer.deserialize(serialized); + DataInKeySpacePath deserialized = serializeAndDeserialize(rootPath, data); assertEquals("l3value", deserialized.getPath().getValue()); assertArrayEquals(value, deserialized.getValue()); } - @Test - void testKeyTypeTestValuesIncludesAllKeyTypes() { - // Verify that KEY_TYPE_TEST_VALUES contains all KeyType enum values - var allKeyTypes = Arrays.stream(KeyType.values()).collect(Collectors.toSet()); - var coveredKeyTypes = KEY_TYPE_TEST_VALUES.keySet(); - assertEquals(allKeyTypes, coveredKeyTypes); - } - static Stream testSerializeDeserializeAllKeyTypes() { - return KEY_TYPE_TEST_VALUES.entrySet().stream() - .map(entry -> Arguments.of(entry.getKey(), entry.getValue())); + return ParameterizedTestUtils.cartesianProduct( + KEY_TYPE_TEST_VALUES.entrySet().stream(), + ParameterizedTestUtils.booleans("isConstant")); } @ParameterizedTest @MethodSource - void testSerializeDeserializeAllKeyTypes(KeyType keyType, Object value) { + void testSerializeDeserializeAllKeyTypes(Map.Entry typeAndValue, boolean isConstant) { + final KeyType keyType = typeAndValue.getKey(); + final Object value = typeAndValue.getValue(); KeySpace root = new KeySpace( new KeySpaceDirectory("root", KeyType.STRING, "root") - .addSubdirectory(new KeySpaceDirectory("typed", keyType))); + .addSubdirectory(isConstant ? + new KeySpaceDirectory("typed", keyType, value) : + new KeySpaceDirectory("typed", keyType))); KeySpacePath rootPath = root.path("root"); KeySpacePath fullPath = rootPath.add("typed", value); byte[] dataValue = new byte[]{1}; DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, dataValue); - - KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); - ByteString serialized = serializer.serialize(data); - - DataInKeySpacePath deserialized = serializer.deserialize(serialized); + final DataInKeySpacePath deserialized = serializeAndDeserialize(rootPath, data); if (value instanceof byte[]) { assertArrayEquals((byte[]) value, (byte[]) deserialized.getPath().getValue()); @@ -191,11 +182,7 @@ void testSerializeWithIntegerForLongKeyType() { byte[] value = new byte[]{1}; DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, value); - - KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); - ByteString serialized = serializer.serialize(data); - - DataInKeySpacePath deserialized = serializer.deserialize(serialized); + final DataInKeySpacePath deserialized = serializeAndDeserialize(rootPath, data); // Should deserialize as Long assertEquals(42L, deserialized.getPath().getValue()); @@ -210,11 +197,7 @@ void testSerializeRootOnly() { byte[] value = new byte[]{5, 6, 7}; DataInKeySpacePath data = new DataInKeySpacePath(rootPath, null, value); - - KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); - ByteString serialized = serializer.serialize(data); - - DataInKeySpacePath deserialized = serializer.deserialize(serialized); + final DataInKeySpacePath deserialized = serializeAndDeserialize(rootPath, data); assertEquals("rootval", deserialized.getPath().getValue()); assertArrayEquals(value, deserialized.getValue()); @@ -235,9 +218,7 @@ void testSerializePathNotContainedInRoot() { byte[] value = new byte[]{1}; DataInKeySpacePath data = new DataInKeySpacePath(otherPath, null, value); - KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); - assertThrows(RecordCoreArgumentException.class, () -> serializer.serialize(data)); } @@ -250,11 +231,7 @@ void testSerializeWithEmptyValue() { byte[] value = new byte[]{}; DataInKeySpacePath data = new DataInKeySpacePath(rootPath, null, value); - - KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); - ByteString serialized = serializer.serialize(data); - - DataInKeySpacePath deserialized = serializer.deserialize(serialized); + final DataInKeySpacePath deserialized = serializeAndDeserialize(rootPath, data); assertArrayEquals(value, deserialized.getValue()); assertEquals(0, deserialized.getValue().length); @@ -284,11 +261,7 @@ void testRoundTripWithComplexRemainder() { byte[] value = new byte[]{9, 8, 7}; DataInKeySpacePath data = new DataInKeySpacePath(rootPath, remainder, value); - - KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); - ByteString serialized = serializer.serialize(data); - - DataInKeySpacePath deserialized = serializer.deserialize(serialized); + final DataInKeySpacePath deserialized = serializeAndDeserialize(rootPath, data); assertEquals(remainder, deserialized.getRemainder()); assertArrayEquals(value, deserialized.getValue()); @@ -305,11 +278,7 @@ void testSerializeNullKeyType() { byte[] value = new byte[]{1}; DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, value); - - KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); - ByteString serialized = serializer.serialize(data); - - DataInKeySpacePath deserialized = serializer.deserialize(serialized); + final DataInKeySpacePath deserialized = serializeAndDeserialize(rootPath, data); assertNull(deserialized.getPath().getValue()); } @@ -319,6 +288,13 @@ static Stream testSerializeDeserializeDifferentRoot() { Arguments.of(Named.of("Same", new KeySpaceDirectory("tenant", KeyType.STRING) .addSubdirectory(new KeySpaceDirectory("record", KeyType.LONG))), Named.of("successful", null)), + Arguments.of(Named.of("Extra subdirectories in destination", + new KeySpaceDirectory("tenant", KeyType.STRING) + .addSubdirectory(new KeySpaceDirectory("users", KeyType.STRING)) + .addSubdirectory(new KeySpaceDirectory("groups", KeyType.BYTES)) + .addSubdirectory(new KeySpaceDirectory("record", KeyType.LONG)) + .addSubdirectory(new KeySpaceDirectory("settings", KeyType.BOOLEAN))), + Named.of("successful", null)), Arguments.of(Named.of("Different constant", new KeySpaceDirectory("tenant", KeyType.STRING) .addSubdirectory(new KeySpaceDirectory("record", KeyType.LONG, 104L))), @@ -334,6 +310,9 @@ static Stream testSerializeDeserializeDifferentRoot() { Arguments.of(Named.of("Different name", new KeySpaceDirectory("tenant", KeyType.STRING) .addSubdirectory(new KeySpaceDirectory("compact disc", KeyType.STRING))), + NoSuchDirectoryException.class), + Arguments.of(Named.of("Missing subdirectory", + new KeySpaceDirectory("tenant", KeyType.STRING)), NoSuchDirectoryException.class)); } @@ -360,80 +339,20 @@ void testSerializeDeserializeDifferentRoot(KeySpaceDirectory destDirectory, ByteString serialized1 = sourceSerializer.serialize(sourceData); // Deserialize to destination - KeySpacePathSerializer destSerializer1 = new KeySpacePathSerializer(keySpace.path("dest_app")); + KeySpacePathSerializer destSerializer = new KeySpacePathSerializer(keySpace.path("dest_app")); if (errorType == null) { - DataInKeySpacePath deserializedData = destSerializer1.deserialize(serialized1); + DataInKeySpacePath deserializedData = destSerializer.deserialize(serialized1); assertEquals("dest_app", deserializedData.getPath().getParent().getParent().getDirectoryName()); assertEquals("tenant1", deserializedData.getPath().getParent().getValue()); assertEquals(42L, deserializedData.getPath().getValue()); assertArrayEquals(new byte[] {10, 20, 30}, deserializedData.getValue()); } else { - assertThrows(errorType, () -> destSerializer1.deserialize(serialized1)); + assertThrows(errorType, () -> destSerializer.deserialize(serialized1)); } } - @Test - void testDeserializeIncompatiblePath() { - // Create a KeySpace with two roots having incompatible structures - KeySpace keySpace = new KeySpace( - new KeySpaceDirectory("source", KeyType.STRING, "src") - .addSubdirectory(new KeySpaceDirectory("level1", KeyType.STRING) - .addSubdirectory(new KeySpaceDirectory("level2", KeyType.LONG))), - new KeySpaceDirectory("dest", KeyType.STRING, "dst") - .addSubdirectory(new KeySpaceDirectory("level1", KeyType.STRING))); - - // Create data in source - KeySpacePath sourcePath = keySpace.path("source") - .add("level1", "value1") - .add("level2", 100L); - byte[] value = new byte[]{1, 2}; - DataInKeySpacePath sourceData = new DataInKeySpacePath(sourcePath, null, value); - - // Serialize from source - KeySpacePathSerializer sourceSerializer = new KeySpacePathSerializer(keySpace.path("source")); - ByteString serialized = sourceSerializer.serialize(sourceData); - - // Try to deserialize to incompatible destination - should fail - KeySpacePathSerializer destSerializer = new KeySpacePathSerializer(keySpace.path("dest")); - assertThrows(NoSuchDirectoryException.class, () -> destSerializer.deserialize(serialized)); - } - - @Test - void testDeserializeWithExtraSubdirectoriesInDestination() { - // Create a KeySpace with two roots where destination has extra subdirectories - KeySpace keySpace = new KeySpace( - new KeySpaceDirectory("source", KeyType.STRING, "src") - .addSubdirectory(new KeySpaceDirectory("data", KeyType.STRING) - .addSubdirectory(new KeySpaceDirectory("users", KeyType.STRING))), - new KeySpaceDirectory("dest", KeyType.STRING, "dst") - .addSubdirectory(new KeySpaceDirectory("data", KeyType.STRING) - .addSubdirectory(new KeySpaceDirectory("users", KeyType.STRING)) - .addSubdirectory(new KeySpaceDirectory("groups", KeyType.LONG)) - .addSubdirectory(new KeySpaceDirectory("settings", KeyType.BOOLEAN)))); - - // Create data using only the "data/users" path - KeySpacePath sourcePath = keySpace.path("source").add("data", "records").add("users", "user123"); - byte[] value = new byte[]{5, 6, 7, 8}; - DataInKeySpacePath sourceData = new DataInKeySpacePath(sourcePath, null, value); - - // Serialize from source - KeySpacePathSerializer sourceSerializer = new KeySpacePathSerializer(keySpace.path("source")); - ByteString serialized = sourceSerializer.serialize(sourceData); - - // Deserialize to destination with extra directories - should succeed - // The extra directories (groups, settings) are not part of the data path, so deserialization should work - KeySpacePathSerializer destSerializer = new KeySpacePathSerializer(keySpace.path("dest")); - DataInKeySpacePath deserializedData = destSerializer.deserialize(serialized); - - // Verify data is correctly deserialized - assertEquals("user123", deserializedData.getPath().getValue()); - assertEquals("users", deserializedData.getPath().getDirectoryName()); - assertEquals("records", deserializedData.getPath().getParent().getValue()); - assertArrayEquals(value, deserializedData.getValue()); - } - @Test void testDeserializeSameStructureDifferentRootValue() { // Create a KeySpace with a single root having multiple children with identical structures @@ -472,4 +391,10 @@ void testDeserializeSameStructureDifferentRootValue() { assertEquals(remainder, deserializedData.getRemainder()); assertArrayEquals(value, deserializedData.getValue()); } + + @Nonnull + private static DataInKeySpacePath serializeAndDeserialize(final KeySpacePath rootPath, final DataInKeySpacePath data) { + final KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); + return serializer.deserialize(serializer.serialize(data)); + } } From 136e9dcad92f4741d24e3a21b9b6bc019bee5501 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 2 Dec 2025 12:27:05 -0500 Subject: [PATCH 11/12] Add support for serializing/deserializing DirectoryLayerDirectory Need to use the logical keyType. --- .../keyspace/KeySpacePathSerializer.java | 21 +--- .../keyspace/KeySpacePathSerializerTest.java | 103 ++++++++++++++++++ 2 files changed, 108 insertions(+), 16 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java index 5269ee1477..e1e1a672d4 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java @@ -159,22 +159,11 @@ private static Object deserializeValue(@Nonnull KeySpaceProto.KeySpacePathEntry @Nonnull private static KeySpaceProto.KeySpacePathEntry serialize(@Nonnull final KeySpacePath keySpacePath) { final Object value = keySpacePath.getValue(); - final KeySpaceDirectory.KeyType keyType = keySpacePath.getDirectory().getKeyType(); - - // Validate null handling: NULL type must have null value, all other types must not have null value - if (keyType == KeySpaceDirectory.KeyType.NULL) { - if (value != null) { - throw new RecordCoreArgumentException("NULL key type must have null value") - .addLogInfo(LogMessageKeys.DIR_NAME, keySpacePath.getDirectoryName(), - LogMessageKeys.ACTUAL, value); - } - } else { - if (value == null) { - throw new RecordCoreArgumentException("Non-NULL key type cannot have null value") - .addLogInfo(LogMessageKeys.DIR_NAME, keySpacePath.getDirectoryName(), - LogMessageKeys.EXPECTED_TYPE, keyType); - } - } + // Use typeOf to get the actual runtime type of the value, rather than the directory's declared keyType. + // This is important for DirectoryLayerDirectory, which has keyType LONG but typically stores String values. + final KeySpaceDirectory.KeyType keyType = value == null + ? KeySpaceDirectory.KeyType.NULL + : KeySpaceDirectory.KeyType.typeOf(value); KeySpaceProto.KeySpacePathEntry.Builder builder = KeySpaceProto.KeySpacePathEntry.newBuilder() .setName(keySpacePath.getDirectoryName()); diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java index 81bb59d1ab..0ad513f020 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializerTest.java @@ -392,6 +392,109 @@ void testDeserializeSameStructureDifferentRootValue() { assertArrayEquals(value, deserializedData.getValue()); } + @Test + void testSerializeDirectoryLayerDirectoryWithStringValue() { + // DirectoryLayerDirectory has KeyType.LONG but typically accepts String values + // The serializer uses KeyType.typeOf to determine the value is a String and serialize it as STRING + KeySpace root = new KeySpace( + new KeySpaceDirectory("app", KeyType.STRING, "myapp") + .addSubdirectory(new DirectoryLayerDirectory("tenant") + .addSubdirectory(new KeySpaceDirectory("data", KeyType.STRING)))); + + KeySpacePath rootPath = root.path("app"); + // Use String value (the typical usage with DirectoryLayerDirectory) + KeySpacePath fullPath = rootPath.add("tenant", "my_tenant").add("data", "mydata"); + byte[] value = new byte[]{1, 2, 3}; + + DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, value); + DataInKeySpacePath deserialized = serializeAndDeserialize(rootPath, data); + + // Verify the String value was preserved + assertEquals("my_tenant", deserialized.getPath().getParent().getValue()); + assertEquals("mydata", deserialized.getPath().getValue()); + assertArrayEquals(value, deserialized.getValue()); + } + + @Test + void testSerializeDirectoryLayerDirectoryWithConstant() { + // Test DirectoryLayerDirectory with a constant String value + KeySpace root = new KeySpace( + new KeySpaceDirectory("app", KeyType.STRING, "myapp") + .addSubdirectory(new DirectoryLayerDirectory("tenant", "my_tenant") + .addSubdirectory(new KeySpaceDirectory("data", KeyType.STRING)))); + + KeySpacePath rootPath = root.path("app"); + KeySpacePath fullPath = rootPath.add("tenant", "my_tenant").add("data", "mydata"); + byte[] value = new byte[]{1, 2, 3}; + + DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, value); + DataInKeySpacePath deserialized = serializeAndDeserialize(rootPath, data); + + // Verify the constant String value was preserved + assertEquals("my_tenant", deserialized.getPath().getParent().getValue()); + assertEquals("mydata", deserialized.getPath().getValue()); + assertArrayEquals(value, deserialized.getValue()); + } + + @Test + void testSerializeDirectoryLayerDirectoryMultiLevel() { + // Test multiple DirectoryLayerDirectory nodes in a path with String values + KeySpace root = new KeySpace( + new KeySpaceDirectory("root", KeyType.STRING, "r") + .addSubdirectory(new DirectoryLayerDirectory("app") + .addSubdirectory(new DirectoryLayerDirectory("tenant") + .addSubdirectory(new KeySpaceDirectory("record", KeyType.LONG))))); + + KeySpacePath rootPath = root.path("root"); + // Use String values for DirectoryLayerDirectory nodes + KeySpacePath fullPath = rootPath + .add("app", "my_app") + .add("tenant", "my_tenant") + .add("record", 12345L); + byte[] value = new byte[]{10, 20}; + + DataInKeySpacePath data = new DataInKeySpacePath(fullPath, null, value); + DataInKeySpacePath deserialized = serializeAndDeserialize(rootPath, data); + + assertEquals("my_app", deserialized.getPath().getParent().getParent().getValue()); + assertEquals("my_tenant", deserialized.getPath().getParent().getValue()); + assertEquals(12345L, deserialized.getPath().getValue()); + assertArrayEquals(value, deserialized.getValue()); + } + + @Test + void testDeserializeDirectoryLayerDirectoryToDifferentRoot() { + // Test that DirectoryLayerDirectory paths can be serialized from one root and deserialized to another + KeySpace keySpace = new KeySpace( + new KeySpaceDirectory("source_app", KeyType.STRING, "app1") + .addSubdirectory(new DirectoryLayerDirectory("tenant") + .addSubdirectory(new KeySpaceDirectory("record", KeyType.LONG))), + new KeySpaceDirectory("dest_app", KeyType.STRING, "app2") + .addSubdirectory(new DirectoryLayerDirectory("tenant") + .addSubdirectory(new KeySpaceDirectory("record", KeyType.LONG)))); + + // Create data in source hierarchy with String value for DirectoryLayerDirectory + KeySpacePath sourcePath = keySpace.path("source_app") + .add("tenant", "tenant1") + .add("record", 123L); + byte[] value = new byte[]{5, 6, 7}; + DataInKeySpacePath sourceData = new DataInKeySpacePath(sourcePath, null, value); + + // Serialize from source + KeySpacePathSerializer sourceSerializer = new KeySpacePathSerializer(keySpace.path("source_app")); + ByteString serialized = sourceSerializer.serialize(sourceData); + + // Deserialize to destination + KeySpacePathSerializer destSerializer = new KeySpacePathSerializer(keySpace.path("dest_app")); + DataInKeySpacePath deserializedData = destSerializer.deserialize(serialized); + + // Verify structure is preserved + assertEquals("dest_app", deserializedData.getPath().getParent().getParent().getDirectoryName()); + assertEquals("tenant1", deserializedData.getPath().getParent().getValue()); + assertEquals(123L, deserializedData.getPath().getValue()); + assertArrayEquals(value, deserializedData.getValue()); + } + @Nonnull private static DataInKeySpacePath serializeAndDeserialize(final KeySpacePath rootPath, final DataInKeySpacePath data) { final KeySpacePathSerializer serializer = new KeySpacePathSerializer(rootPath); From 955ba789fbd0c41832c4b90399641a770b6a37ee Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 2 Dec 2025 12:28:59 -0500 Subject: [PATCH 12/12] Elaborate on comment --- .../provider/foundationdb/keyspace/KeySpacePathSerializer.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java index e1e1a672d4..3c1d19f47a 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathSerializer.java @@ -161,6 +161,8 @@ private static KeySpaceProto.KeySpacePathEntry serialize(@Nonnull final KeySpace final Object value = keySpacePath.getValue(); // Use typeOf to get the actual runtime type of the value, rather than the directory's declared keyType. // This is important for DirectoryLayerDirectory, which has keyType LONG but typically stores String values. + // If we every support something that takes a value that is not supported via KeyType, we'll need to remove this + // dependency, but right now it is convenient to reuse that enum. final KeySpaceDirectory.KeyType keyType = value == null ? KeySpaceDirectory.KeyType.NULL : KeySpaceDirectory.KeyType.typeOf(value);