diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditor.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditor.java
index 3458444141..a17c7529c9 100644
--- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditor.java
+++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditor.java
@@ -660,6 +660,9 @@ private static void applyRecordTypeRenames(@Nonnull RecordMetaDataProto.MetaData
// Determine the usage of each renamed type by looking at the union message type within `MetaData.records`.
determineRecordTypeUnionFieldsAndUsages(renames, unionDescriptor, union);
+ // Reject the renaming if there’s a record type with an index declared (only) via the `(field).index` extension.
+ validateNoExtensionOnlyIndexes(metadata, renames, fileDesc);
+
// Validate that renaming the canonical union fields would not cause a collision.
validateUnionFieldRenames(union, renames);
@@ -701,6 +704,8 @@ private static void applyRecordTypeRenames(@Nonnull RecordMetaDataProto.MetaData
* record type, or, if the type has {@code RECORD} usage, by any other record type. The latter includes a record
* type imported from a dependency file and a record type that is backed by a message type nested within another
* message type.
+ * The type has {@code RECORD} usage and an index that is declared through (only) the {@code (field).index}
+ * extension declares, i.e., a field with that extension for which {@code MetaData.indexes} lists no index.
* The type is not the union, and either its current name or {@code newRecordTypeName} is the default union
* name. (The union itself can be renamed freely, including to or from the default name.)
* The union already has a field under the new canonical union field name {@code _newRecordTypeName}.
@@ -1139,6 +1144,47 @@ private static void renameRecordTypeUsagesInMessageType(
}
}
+ /**
+ * Rejects the renaming if there is a {@code RECORD}-usage type in {@code renames} with an index that is declared
+ * through the {@code (field).index} extension but not listed in {@code MetaData.indexes}. When the metadata is
+ * built with extension options processed, {@link RecordMetaDataBuilder} derives such an index from a top-level
+ * field of the record type and names it {@code RecordType$field}. Since the subspace key of an index defaults to
+ * its name, renaming the record type would turn such an index into a different index, whose existing entries are no
+ * longer found. (An index that is listed in {@code MetaData.indexes} will keep its name across the rename.)
+ */
+ @SuppressWarnings("deprecation") // for `FieldOptions.hasIndexed()`, which `RecordMetaDataBuilder` still honors
+ private static void validateNoExtensionOnlyIndexes(@Nonnull RecordMetaDataProto.MetaData.Builder metadata,
+ @Nonnull RecordTypeRenames renames,
+ @Nonnull Descriptors.FileDescriptor fileDesc) {
+ Set listedIndexNames = null;
+ for (final RecordTypeRename rename : renames.values()) {
+ if (rename.usage != RecordTypeOptions.Usage.RECORD) {
+ continue;
+ }
+ for (final Descriptors.FieldDescriptor field : getMessageTypeByName(fileDesc, rename.name).getFields()) {
+ final RecordMetaDataOptionsProto.FieldOptions fieldOptions =
+ field.getOptions().getExtension(RecordMetaDataOptionsProto.field);
+ if (!fieldOptions.hasIndex() && !fieldOptions.hasIndexed()) {
+ continue;
+ }
+ if (listedIndexNames == null) {
+ listedIndexNames = new HashSet<>();
+ for (final RecordMetaDataProto.Index index : metadata.getIndexesList()) {
+ listedIndexNames.add(index.getName());
+ }
+ }
+ // This is the name `RecordMetaDataBuilder.protoFieldOptions()` gives an index derived from the field.
+ final String indexName = rename.name + "$" + field.getName();
+ if (!listedIndexNames.contains(indexName)) {
+ throw new MetaDataException(
+ "Cannot rename record type with an index that only the field index extension declares",
+ LogMessageKeys.RECORD_TYPE, rename.name,
+ LogMessageKeys.INDEX_NAME, indexName);
+ }
+ }
+ }
+ }
+
/**
* Validates that {@code MetaData.user_defined_functions}, {@code MetaData.views} and
* {@code MetaData.stored_queries} are all empty. Each of them holds a string that would need parsing to figure out
diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java
index 8a8dd85143..882dcfefb3 100644
--- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java
+++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java
@@ -2101,6 +2101,44 @@ void indexSubspaceKeyChanged() {
assertInvalid("index missing in new meta-data", metaData1, metaData2);
}
+ /**
+ * Pins the behavior that renaming a record type by hand turns an index that is declared (only) via the
+ * {@code (field).index} extension into effectively a different index. {@link RecordMetaDataBuilder} names the
+ * derived index after the record type, and since the subspace key of an index defaults to its name, the validator
+ * treats the renamed index as a removal.
+ */
+ @Test
+ void renameRecordTypeWithExtensionOnlyIndex() {
+ final RecordMetaDataProto.MetaData originalProto = RecordMetaData.build(TestRecords1Proto.getDescriptor())
+ .toProto().toBuilder().clearIndexes().build();
+ final RecordMetaDataProto.MetaData.Builder builder = originalProto.toBuilder();
+ // Rename the message type, the type of its union field, and its entry in the record type list.
+ for (final DescriptorProtos.DescriptorProto.Builder messageType :
+ builder.getRecordsBuilder().getMessageTypeBuilderList()) {
+ if (messageType.getName().equals("MySimpleRecord")) {
+ messageType.setName("MyNewSimpleRecord");
+ } else if (messageType.getName().equals(RecordMetaDataBuilder.DEFAULT_UNION_NAME)) {
+ for (final DescriptorProtos.FieldDescriptorProto.Builder field : messageType.getFieldBuilderList()) {
+ if (field.getTypeName().endsWith("MySimpleRecord")) {
+ field.setTypeName(field.getTypeName().replace("MySimpleRecord", "MyNewSimpleRecord"));
+ }
+ }
+ }
+ }
+ for (final RecordMetaDataProto.RecordType.Builder recordType : builder.getRecordTypesBuilderList()) {
+ if (recordType.getName().equals("MySimpleRecord")) {
+ recordType.setName("MyNewSimpleRecord");
+ }
+ }
+
+ final RecordMetaData metaData1 = RecordMetaData.newBuilder().setRecords(originalProto, true).build();
+ final RecordMetaData metaData2 = RecordMetaData.newBuilder().setRecords(builder.build(), true).build();
+ assertNotNull(metaData1.getIndex("MySimpleRecord$num_value_3_indexed"));
+ assertNotNull(metaData2.getIndex("MyNewSimpleRecord$num_value_3_indexed"));
+ assertInvalid("index missing in new meta-data",
+ MetaDataEvolutionValidator.newBuilder().setAllowNoVersionChange(true).build(), metaData1, metaData2);
+ }
+
@Test
void indexNameChanged() {
RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor());
diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditorUnitTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditorUnitTest.java
index 6f9ba07c2c..35afeba484 100644
--- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditorUnitTest.java
+++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditorUnitTest.java
@@ -29,6 +29,9 @@
import com.apple.foundationdb.record.TestRecordsEnumProto;
import com.apple.foundationdb.record.TestRecordsImportProto;
import com.apple.foundationdb.record.TestRecordsImportedAndNewProto;
+import com.apple.foundationdb.record.TestRecordsIndexCompatProto;
+import com.apple.foundationdb.record.logging.LogMessageKeys;
+import com.apple.foundationdb.record.metadata.Index;
import com.apple.foundationdb.record.metadata.Key;
import com.apple.foundationdb.record.metadata.MetaDataEvolutionValidator;
import com.apple.foundationdb.record.metadata.MetaDataException;
@@ -73,6 +76,7 @@
import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
/**
* Unit tests for the metadata proto editor. These tests focus on just the editor itself.
@@ -952,44 +956,129 @@ void renameRecordTypeRejectsNameOfImportedTypeMissingFromRecordTypes() {
}
/**
- * Tests that the rename determines the record types from the union message type rather than from
- * {@code MetaData.record_types}, the same way {@link RecordMetaDataBuilder} does. In the fixture,
- * {@code MetaData.record_types} is empty, which is valid as long as the primary keys come from the
- * {@code (field).primary_key} extension instead. This test is batched-only, because the one-by-one renaming of
- * {@link #renameRecordTypesOneByOne} only visits the record types listed in {@code MetaData.record_types}. See
- * {@link #renameRecordTypeRenamesRecordTypeMissingFromRecordTypes} for the {@code renameRecordType} counterpart.
+ * Tests that the batched rename rejects renaming a record type with an index that only the {@code (field).index}
+ * extension declares, while it still determines the record types from the union message type rather than from
+ * {@code MetaData.record_types}. In the fixture, {@code MetaData.record_types} and {@code MetaData.indexes} are
+ * empty, which is valid as long as the primary keys and indexes come from the extensions. {@code MySimpleRecord}
+ * has such indexes, whereas {@code MyOtherRecord} has none and can still be renamed. This test is batched-only:
+ * the one-by-one renaming of {@link #renameRecordTypesOneByOne} only visits the record types listed in
+ * {@code MetaData.record_types}, so with that list empty, it silently renames nothing at all.
+ * (The one-by-one renaming is covered by {@link #renameRejectsExtensionOnlyIndexOfListedRecordType}.)
*/
@Test
- void renameRecordTypesRenamesRecordTypesMissingFromRecordTypes() {
+ void renameRecordTypesRejectsExtensionOnlyIndexOfRecordTypeMissingFromRecordTypes() {
final RecordMetaDataProto.MetaData originalProto = RecordMetaData.build(TestRecords1Proto.getDescriptor())
.toProto().toBuilder().clearRecordTypes().clearIndexes().build();
final RecordMetaData originalMetaData = RecordMetaData.newBuilder().setRecords(originalProto, true).build();
assertEquals(Set.of("MySimpleRecord", "MyOtherRecord"), originalMetaData.getRecordTypes().keySet());
+ assertNotNull(originalMetaData.getIndex("MySimpleRecord$num_value_3_indexed"));
+ final Descriptors.FileDescriptor[] dependencies =
+ RecordMetaDataBuilder.getDependencies(originalProto, Map.of());
- final Set renamerSawNames = new LinkedHashSet<>();
+ // Renaming MySimpleRecord is rejected before anything has been modified.
final RecordMetaDataProto.MetaData.Builder builder = originalProto.toBuilder();
- MetaDataProtoEditor.renameRecordTypes(builder, name -> {
- renamerSawNames.add(name);
- return simpleRename(name);
- }, RecordMetaDataBuilder.getDependencies(originalProto, Map.of()));
- assertEquals(Set.of("MySimpleRecord", "MyOtherRecord"), renamerSawNames);
- assertEquals(List.of(), MetaDataProtoEditor.getRecordTypes(builder));
+ final MetaDataException exception = assertThrows(MetaDataException.class,
+ () -> MetaDataProtoEditor.renameRecordTypes(builder, MetaDataProtoEditorUnitTest::simpleRename,
+ dependencies));
+ assertEquals("Cannot rename record type with an index that only the field index extension declares",
+ exception.getMessage());
+ assertEquals("MySimpleRecord", exception.getLogInfo().get(LogMessageKeys.RECORD_TYPE.toString()));
+ assertEquals("MySimpleRecord$str_value_indexed",
+ exception.getLogInfo().get(LogMessageKeys.INDEX_NAME.toString()));
+ assertEquals(originalProto, builder.build());
+ // Renaming only MyOtherRecord is accepted, and leaves the derived indexes of MySimpleRecord as they are.
+ MetaDataProtoEditor.renameRecordTypes(builder,
+ name -> name.equals("MyOtherRecord") ? simpleRename(name) : name, dependencies);
+ assertEquals(List.of(), MetaDataProtoEditor.getRecordTypes(builder));
final RecordMetaData renamed = RecordMetaData.newBuilder().setRecords(builder.build(), true).build();
- assertEquals(Set.of(simpleRename("MySimpleRecord"), simpleRename("MyOtherRecord")),
- renamed.getRecordTypes().keySet());
- // Indexes declared through the `(field).index` extension are named after their record type, so they follow
- // the rename too.
+ assertEquals(Set.of("MySimpleRecord", simpleRename("MyOtherRecord")), renamed.getRecordTypes().keySet());
+ assertEquals(originalMetaData.getAllIndexes().stream().map(Index::getName).collect(Collectors.toSet()),
+ renamed.getAllIndexes().stream().map(Index::getName).collect(Collectors.toSet()));
+ }
+
+ /**
+ * Tests that the rename rejects renaming a record type that {@code MetaData.record_types} lists, but whose indexes
+ * only the {@code (field).index} extension declares. {@link RecordMetaDataBuilder} derives such an index, named
+ * after the record type, when the metadata is built with extension options processed, so renaming the record type
+ * would turn the index into a different index.
+ */
+ @Test
+ void renameRejectsExtensionOnlyIndexOfListedRecordType() {
+ final RecordMetaDataProto.MetaData originalProto = RecordMetaData.build(TestRecords1Proto.getDescriptor())
+ .toProto().toBuilder().clearIndexes().build();
+ final RecordMetaData originalMetaData = RecordMetaData.newBuilder().setRecords(originalProto, true).build();
assertNotNull(originalMetaData.getIndex("MySimpleRecord$num_value_3_indexed"));
- assertNotNull(renamed.getIndex(simpleRename("MySimpleRecord") + "$num_value_3_indexed"));
- // Assert that `MetaDataEvolutionValidator` rejects the result as an evolution of the original. Since the
- // subspace key of an index defaults to its name, a renamed index is a different index.
- final MetaDataException exception = assertThrows(MetaDataException.class,
- () -> MetaDataEvolutionValidator.newBuilder()
- .setAllowNoVersionChange(true)
- .build()
- .validate(originalMetaData, renamed));
- assertEquals("index missing in new meta-data", exception.getMessage());
+ final String expectedMessage =
+ "Cannot rename record type with an index that only the field index extension declares";
+ crossCheckRenameRecordTypesIsRejected(originalProto, MetaDataProtoEditorUnitTest::simpleRename,
+ RecordMetaDataBuilder.getDependencies(originalProto, Map.of()), expectedMessage);
+ assertRenameRecordTypeRejected(originalProto, "MySimpleRecord", simpleRename("MySimpleRecord"),
+ expectedMessage);
+ }
+
+ /**
+ * Tests that the rename also rejects renaming a record type whose index only the deprecated {@code (field).indexed}
+ * extension declares, the same way it does for the {@code (field).index} extension. In the fixture,
+ * {@code MyCompatRecord} declares its indexes through {@code indexed}, whereas {@code MyModernRecord} uses
+ * {@code index}.
+ */
+ @Test
+ void renameRejectsExtensionOnlyIndexOfDeprecatedIndexedOption() {
+ final RecordMetaDataProto.MetaData originalProto =
+ RecordMetaData.build(TestRecordsIndexCompatProto.getDescriptor())
+ .toProto().toBuilder().clearIndexes().build();
+ final RecordMetaData originalMetaData = RecordMetaData.newBuilder().setRecords(originalProto, true).build();
+ assertNotNull(originalMetaData.getIndex("MyCompatRecord$index"));
+ final String expectedMessage =
+ "Cannot rename record type with an index that only the field index extension declares";
+ crossCheckRenameRecordTypesIsRejected(originalProto,
+ name -> name.equals("MyCompatRecord") ? simpleRename(name) : name,
+ RecordMetaDataBuilder.getDependencies(originalProto, Map.of()), expectedMessage);
+ assertRenameRecordTypeRejected(originalProto, "MyCompatRecord", simpleRename("MyCompatRecord"),
+ expectedMessage);
+ }
+
+ /**
+ * Tests that {@link MetaDataProtoEditor#renameRecordType} still renames a type that is not a {@code RECORD}-usage
+ * type, even if one of its fields carries the {@code (field).index} extension, since no index is derived from
+ * such a field. This covers a {@code NESTED} type and the union type.
+ */
+ @Test
+ void renameAcceptsNonRecordTypeWithExtensionIndexedField() {
+ final RecordMetaDataProto.MetaData.Builder builder = RecordMetaData.build(TestRecords1Proto.getDescriptor())
+ .toProto().toBuilder().clearIndexes();
+ builder.getRecordsBuilder().addMessageType(DescriptorProtos.DescriptorProto.newBuilder()
+ .setName("MyNestedRecord")
+ .addField(DescriptorProtos.FieldDescriptorProto.newBuilder()
+ .setName("str_value")
+ .setNumber(1)
+ .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_STRING)
+ .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL)
+ .setOptions(DescriptorProtos.FieldOptions.newBuilder()
+ .setExtension(RecordMetaDataOptionsProto.field,
+ RecordMetaDataOptionsProto.FieldOptions.newBuilder()
+ .setIndex(RecordMetaDataOptionsProto.FieldOptions.IndexOption
+ .getDefaultInstance())
+ .build())))
+ .setOptions(DescriptorProtos.MessageOptions.newBuilder()
+ .setExtension(RecordMetaDataOptionsProto.record,
+ RecordMetaDataOptionsProto.RecordTypeOptions.newBuilder()
+ .setUsage(RecordMetaDataOptionsProto.RecordTypeOptions.Usage.NESTED)
+ .build())));
+ final Descriptors.FileDescriptor[] dependencies =
+ RecordMetaDataBuilder.getDependencies(builder.build(), Map.of());
+
+ MetaDataProtoEditor.renameRecordType(builder, "MyNestedRecord", "MyRenamedNestedRecord", dependencies);
+ MetaDataProtoEditor.renameRecordType(builder, RecordMetaDataBuilder.DEFAULT_UNION_NAME, "RecordsOneUnion",
+ dependencies);
+
+ final Set messageTypeNames = builder.getRecords().getMessageTypeList().stream()
+ .map(DescriptorProtos.DescriptorProto::getName)
+ .collect(Collectors.toSet());
+ assertTrue(messageTypeNames.containsAll(Set.of("MyRenamedNestedRecord", "RecordsOneUnion")));
+ assertEquals(Set.of("MySimpleRecord", "MyOtherRecord"),
+ RecordMetaData.newBuilder().setRecords(builder.build(), true).build().getRecordTypes().keySet());
}
/**