Make MetaDataProtoEditor reject renaming record types with indexes declared via the (field).index extension

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

@@ -659,7 +659,10 @@
 
         // 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 @@
      *     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.
+     * <li>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.
      * <li>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.)
      * <li>The union already has a field under the new canonical union field name {@code _newRecordTypeName}.
@@ -1139,6 +1144,47 @@
         }
     }
 
+    /**
+     * 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<String> 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

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());

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<String> 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<String> 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());
     }
 
     /**