From d1fd75722ffec985159e691940bbd9d83856b00c Mon Sep 17 00:00:00 2001 From: Robert Brunel Date: Tue, 4 Aug 2026 19:43:07 +0100 Subject: [PATCH] Optimize `MetaDataProtoEditor.renameRecordTypes()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Replace the naive O(N²) implementation of `renameRecordTypes()`, which renames the record types one by one via `renameRecordType()`, with an efficient batched version that applies all renames in one go. * Cleanly split the per-rename validation from the actual mutation throughout, so that the whole batch is validated before anything is mutated. * Add a `RecordTypeRenames` data structure that indexes renames by fully qualified name to avoid having to scan all renames per item. This brings what would otherwise still be an O(N²) algorithm down to O(N). * Leave `renameRecordType()` itself untouched for the time being. It will be simplified in a subsequent change by reusing the utility methods introduced here. The batched implementation performs the costly compilation of the `FileDescriptor` exactly _once_ (via `Descriptors.FileDescriptor.buildFrom()`, from the original proto) and applies the name mapping in a single walk of the builder. The trivial implementation, by contrast, performs a recompilation of the entire records `FileDescriptor` plus a full `records.toBuilder()` deep-copy _in each iteration_ of the loop over the record types. In the common use case where _all_ top-level record types are to be renamed, that means O(N) descriptor compilations for N record types; a factor which the batched implementation brings down to O(1). Applying multiple renames in the batched manner is equivalent to applying them one-by-one, _provided that_ the name mapping defined by the given `renamer` is collision-free. The batched implementation validates this mapping upfront. This allows it to provide a stronger exception safety guarantee. If the mapping is invalid, an exception will be thrown before _any_ of the renames is performed. The one-by-one implementation, by contrast, may raise an exception halfway through; and that may depend on the order in which the renames are applied. * Cross-check `renameRecordType()` and `renameRecordTypes()` against each other throughout the existing test suite. * Add dedicated tests for every `MetaDataException` reachable from `renameRecordTypes()`, plus a couple of previously-unexercised edge cases. * Fix an unrelated, pre-existing bug in the `Joined.json` test fixture (a join referenced a nonexistent constituent), uncovered along the way. * Add a performance test that renames a large number of record types. According to a local ad-hoc experiment with 50 record types, the batched implementation measured about 17x faster than an equivalent one-by-one sequence of `renameRecordType()` calls (0.1 ms versus 1.5 ms), which is consistent with the O(N) vs. O(N²) difference between the two. --- .../foundationdb/MetaDataProtoEditor.java | 567 +++++++++++++++++- .../MetaDataProtoEditorUnitTest.java | 483 ++++++++++++++- .../src/test/resources/Joined.json | 2 +- .../test/resources/UnnestedShadowedName.json | 110 ++++ 4 files changed, 1125 insertions(+), 37 deletions(-) create mode 100644 fdb-record-layer-core/src/test/resources/UnnestedShadowedName.json 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 5c4213cea4c..1feab1476e1 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 @@ -36,9 +36,15 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import java.util.Objects; -import java.util.function.Function; +import java.util.Set; +import java.util.function.UnaryOperator; import static com.apple.foundationdb.record.RecordMetaDataBuilder.DEFAULT_UNION_NAME; @@ -408,13 +414,189 @@ public static void deprecateRecordType(@Nonnull RecordMetaDataProto.MetaData.Bui } } - public static void renameRecordTypes(@Nonnull RecordMetaDataProto.MetaData.Builder metaDataBuilder, - @Nonnull Function renamer, + /** + * Internal representation of a record type to be renamed by {@link #renameRecordTypes}. + */ + private static final class RecordTypeRename { + /** The current name. */ + @Nonnull + private final String name; + /** The new name. */ + @Nonnull + private final String newName; + /** The fully qualified current name. */ + @Nonnull + private final String fullName; + /** The fully qualified new name. */ + @Nonnull + private final String fullNewName; + /** + * The usage, as determined by looking at the union type. (Initially {@code UNSET}, to be filled in by + * {@link #determineRecordTypeUnionFieldsAndUsages}). + */ + @Nonnull + private RecordTypeOptions.Usage usage = RecordTypeOptions.Usage.UNSET; + /** + * Builder of the referencing union field, if any. (Initially null, to be filled in by + * {@link #determineRecordTypeUnionFieldsAndUsages}). + */ + @Nullable + private DescriptorProtos.FieldDescriptorProto.Builder unionField; + + RecordTypeRename(@Nonnull String namespace, @Nonnull String name, @Nonnull String newName) { + this.name = name; + this.newName = newName; + this.fullName = fullyQualifiedTypeName(namespace, name); + this.fullNewName = fullyQualifiedTypeName(namespace, newName); + } + } + + /** + * A map of {@link RecordTypeRename}s, keyed by the record type's current (simple) name, as built by + * {@link #analyzeRecordTypeRenames}. Also provides a lookup by fully qualified name, built lazily on first use. + */ + private static final class RecordTypeRenames { + @Nonnull + private final Map byName; + @Nullable + private Map byFullName; + + RecordTypeRenames(@Nonnull Map byName) { + this.byName = byName; + } + + boolean isEmpty() { + return byName.isEmpty(); + } + + @Nonnull + Collection values() { + return byName.values(); + } + + @Nullable + RecordTypeRename get(@Nonnull String name) { + return byName.get(name); + } + + /** + * Returns the new name for {@code name}, if there is a rename of {@code name} with the given {@code usage}; + * otherwise, returns {@code null}. + */ + @Nullable + String get(@Nonnull String name, @Nonnull RecordTypeOptions.Usage usage) { + final RecordTypeRename rename = byName.get(name); + return rename != null && rename.usage == usage ? rename.newName : null; + } + + /** + * Returns the rename whose (original) fully qualified name is {@code fullName}, if any. + */ + @Nullable + RecordTypeRename getByFullName(@Nonnull String fullName) { + if (byFullName == null) { + byFullName = new HashMap<>(); + for (final RecordTypeRename rename : byName.values()) { + byFullName.put(rename.fullName, rename); + } + } + return byFullName.get(fullName); + } + } + + /** + * Renames the record types in the metadata, according to the name mapping defined by {@code renamer}. For each + * renamed record type (where {@code renamer} yields a name that is not equal to the current one), this method + * applies the same transformations that {@link #renameRecordType} would, but it operates in an efficient, batched + * manner. The entire mapping is applied in a single walk over the given {@code metadata}, and the records + * {@link Descriptors.FileDescriptor} is compiled exactly once. + * + *

For a collision-free mapping of a single {@code RECORD}-usage type, this is exactly equivalent to the + * corresponding {@link #renameRecordType} call. For anything broader, the two diverge in a few respects, each + * noted below: which types the mapping is applied to, how imported types are treated, and which mappings are + * accepted rather than rejected. Where they differ, it is generally because applying the whole batch at once + * admits mappings that no single ordering of one-by-one renames could express. + * + *

Unlike {@link #renameRecordType}, {@code renamer} is only ever applied to (and can therefore only rename) + * {@code RECORD}-usage top-level types, i.e., those in {@code MetaData.record_types}; it cannot rename + * {@code NESTED} types or the union type itself. + * + *

Imported record types, i.e., those registered in {@code MetaData.record_types} whose message type is defined + * in a dependency file rather than in {@code MetaData.records}, cannot be renamed by this metadata, so + * {@code renamer} is never applied to them. (Note that {@link #renameRecordType}, by contrast, rejects such a + * rename outright, with a “No record type found” {@link MetaDataException}.) An imported record type can still be + * the cause of a collision, as described below. + * + *

Precondition: The {@code renamer} must define a consistent, collision-free mapping. That is, no two + * distinct existing top-level record types may map to the same new name, and no record type may be renamed to a + * name that collides with another (renamed or unchanged) top-level type or an imported record type. If a collision + * is detected, no rename is performed, and a {@link MetaDataException} is thrown. + * + *

Validating the mapping as a whole means a batch may be accepted where the equivalent one-by-one renames + * would fail, and rejected where they would quietly succeed. A batch that permutes existing names, for instance + * swapping {@code Foo} and {@code Bar}, is collision-free and therefore accepted, whereas renaming those types + * one at a time would collide on whichever is renamed first. Conversely, a mapping that would make two union + * fields share a name is rejected here, while {@link #renameRecordType} leaves the second field under its old + * name instead of reporting the conflict. + * + *

The following is an example of a simple, collision-free renaming. It prepends a fixed string to every name: + * 

+     * MetaDataProtoEditor.renameRecordTypes(builder, name -> "prefix_" + name, dependencies);
+     *
+ * + * @param metadata the metadata builder + * @param renamer a function mapping each existing top-level record type name to its new name + * @param dependencies the dependencies of the records file descriptor + * @see #renameRecordType + */ + public static void renameRecordTypes(@Nonnull RecordMetaDataProto.MetaData.Builder metadata, + @Nonnull UnaryOperator renamer, @Nonnull Descriptors.FileDescriptor[] dependencies) { - for (final String recordType : MetaDataProtoEditor.getRecordTypes(metaDataBuilder)) { - MetaDataProtoEditor.renameRecordType(metaDataBuilder, - recordType, renamer.apply(recordType), dependencies); + // Collect the renames into a map, skipping identity renames. This method will throw `MetaDataException` if + // there is any conflict. + final RecordTypeRenames renames = analyzeRecordTypeRenames(metadata, renamer); + if (renames.isEmpty()) { + return; } + + validateNoUserDefinedFunctions(metadata); + + // Build the file descriptor exactly once, from the original `MetaData.records` proto. Every descriptor + // lookup below is done by original name, so we can use this single descriptor for every rename in the + // mapping. + final DescriptorProtos.FileDescriptorProto records = metadata.getRecords(); + final Descriptors.FileDescriptor fileDesc = RecordMetaDataBuilder.buildFileDescriptor(records, dependencies); + + // Validate the `MetaData.unnested_record_types` constituents before making any changes. + validateRecordTypeUsagesInUnnestedRecordTypes(metadata.getUnnestedRecordTypesBuilderList(), fileDesc, renames); + + // Determine the usage of each renamed type by looking at the union message type within `MetaData.records`. + final DescriptorProtos.DescriptorProto.Builder union = fetchUnionBuilder(metadata.getRecordsBuilder()); + if (union.getNestedTypeCount() > 0) { + throw new MetaDataException("Nested types in union type not supported"); + } + determineRecordTypeUnionFieldsAndUsages(renames, fileDesc, union); + + // Validate that renaming the canonical union fields would not cause a collision. + validateUnionFieldRenames(union, renames); + + // Rename the canonical union field, if present, for each renamed type. + renameUnionFields(renames); + + // Rename every message type in `MetaData.records`, and all field type references. + renameRecordTypeUsagesInMessageTypes(metadata.getRecordsBuilder().getMessageTypeBuilderList(), renames, fileDesc); + + // Update `MetaData.record_types` for every top-level RECORD type. + renameRecordTypeUsagesInRecordTypes(metadata.getRecordTypesBuilderList(), renames); + + // Update `MetaData.indexes` for every top-level RECORD type. + renameRecordTypeUsagesInIndexes(metadata.getIndexesBuilderList(), renames); + + // Update `MetaData.joined_record_types` constituents for every renamed type. + renameRecordTypeUsagesInJoinedRecordTypes(metadata.getJoinedRecordTypesBuilderList(), renames); + + // Rename `MetaData.unnested_record_types` constituents for every renamed type. + renameRecordTypeUsagesInUnnestedRecordTypes(metadata.getUnnestedRecordTypesBuilderList(), renames); } /** @@ -692,6 +874,379 @@ private static boolean isNested(@Nonnull Descriptors.Descriptor typeDescriptor, } } + /** + * A helper for {@link #renameRecordTypes} that converts the record-type name mapping defined by {@code renamer} + * into the internal, ordered, old-to-new map. Also validates the mapping against the full set of top-level + * message types, and raises {@link MetaDataException} if it is invalid. + */ + @Nonnull + private static RecordTypeRenames analyzeRecordTypeRenames( + @Nonnull RecordMetaDataProto.MetaData.Builder metaDataBuilder, + @Nonnull UnaryOperator renamer) { + // Apply `renamer` to each record type name and build the map representing the renamings. + final String namespace = metaDataBuilder.getRecords().getPackage(); + // Don’t apply the renamer to imported record types. They are registered in `record_types` but defined in a + // dependency file, so this metadata cannot rename them. + final Set localMessageTypes = new HashSet<>(); + for (final DescriptorProtos.DescriptorProto messageType : metaDataBuilder.getRecords().getMessageTypeList()) { + localMessageTypes.add(messageType.getName()); + } + final Map renames = new LinkedHashMap<>(); + for (final String recordType : getRecordTypes(metaDataBuilder)) { + if (!localMessageTypes.contains(recordType)) { + continue; + } + final String newName = renamer.apply(recordType); + // Skip identity renames, as they require no work. + if (recordType.equals(newName)) { + continue; + } + renames.put(recordType, new RecordTypeRename(namespace, recordType, newName)); + } + + if (renames.isEmpty()) { + return new RecordTypeRenames(renames); + } + + // Build the inverse new-to-old mapping and use it to perform basic validation: + // * No two distinct existing record types may map to the same new name. + // * No record type may be renamed to a name that collides with another (renamed or unchanged) top-level type. + final Map inverse = new HashMap<>(); + for (final RecordTypeRename rename : renames.values()) { + final String previous = inverse.put(rename.newName, rename.name); + if (previous != null) { + throw new MetaDataException("Cannot rename record types `" + previous + "` and `" + rename.name + + "` to the same name `" + rename.newName + "`"); + } + } + + // If a message type is not itself being renamed, then it must not be the target of a rename either. + for (final DescriptorProtos.DescriptorProto messageType : metaDataBuilder.getRecords().getMessageTypeList()) { + final String name = messageType.getName(); + if (!renames.containsKey(name) && inverse.containsKey(name)) { + throw new MetaDataException("Cannot rename record type to " + name + " as it already exists"); + } + } + + // Likewise for the metadata's full record type registry, which (unlike `getMessageTypeList()` above) also + // includes record types imported from a dependency file. + for (final String name : getRecordTypes(metaDataBuilder)) { + if (!renames.containsKey(name) && inverse.containsKey(name)) { + throw new MetaDataException("Cannot rename record type to " + name + " as an imported record type of that name already exists"); + } + } + + return new RecordTypeRenames(renames); + } + + /** + * A helper for {@link #renameRecordTypes} that determines the {@link RecordTypeOptions.Usage Usage} of each + * renamed type by looking at the union. Fills in the {@code usage} and {@code unionField} of the entries + * in {@code renames}. Does not mutate {@code fileDescriptor} and {@code unionBuilder}. + */ + private static void determineRecordTypeUnionFieldsAndUsages( + @Nonnull RecordTypeRenames renames, + @Nonnull Descriptors.FileDescriptor fileDescriptor, + @Nonnull DescriptorProtos.DescriptorProto.Builder unionBuilder) { + final String unionName = unionBuilder.getName(); + final Descriptors.Descriptor unionDescriptor = getMessageTypeByName(fileDescriptor, unionName); + + // Find, for each renamed record type, the union field that references it (if any), in a single pass over the + // union's fields. + for (final DescriptorProtos.FieldDescriptorProto.Builder unionField : unionBuilder.getFieldBuilderList()) { + if (!unionField.hasTypeName() || unionField.getTypeName().isEmpty()) { + continue; + } + + final String fullReferencedName = resolveFieldTypeFullName(unionDescriptor, unionField); + if (fullReferencedName == null) { + continue; + } + + final RecordTypeRename rename = renames.getByFullName(fullReferencedName); + if (rename == null) { + continue; + } + + // If multiple fields reference this record type, prefer the canonically-named one; otherwise, keep the + // one with the highest field number. + if (rename.unionField == null + || isCanonicalUnionFieldName(unionField.getName(), rename.name) + || unionField.getNumber() > rename.unionField.getNumber()) { + rename.unionField = unionField; + } + } + + for (final RecordTypeRename rename : renames.values()) { + // Determine the usage of each renamed record type, based on the union field found above, if any. + // * If the type name equals the union name, the usage is UNION. + // * Otherwise, if the type has a corresponding union field, it is a top-level record type, i.e., RECORD. + // * Otherwise, it can only ever be used as an embedded message type, i.e., NESTED. + if (rename.name.equals(unionName)) { + rename.unionField = null; + rename.usage = RecordTypeOptions.Usage.UNION; + } else { + rename.usage = rename.unionField == null + ? RecordTypeOptions.Usage.NESTED + : RecordTypeOptions.Usage.RECORD; + } + + // Prevent renaming a non-UNION type to the default union name. + if (!rename.usage.equals(RecordTypeOptions.Usage.UNION) && rename.newName.equals(DEFAULT_UNION_NAME)) { + throw new MetaDataException( + "Cannot rename record type to the default union name", + LogMessageKeys.RECORD_TYPE, rename.name); + } + } + } + + /** + * Validates that renaming each rename's canonical union field (if any) to its new canonical name would not + * collide with any other field of the union message type within {@code MetaData.records}, once every rename in + * {@code renames} has been applied. Performs no mutation, so that this can be called before any other edits are + * made. + */ + private static void validateUnionFieldRenames(@Nonnull DescriptorProtos.DescriptorProto.Builder unionBuilder, + @Nonnull RecordTypeRenames renames) { + // Fields that are themselves about to be renamed to their new canonical form never count as a collision + // target below, since they won't keep their current name once this batch of renames is applied. (Without + // this exclusion, a batch that e.g. swaps two type names — Foo -> Baz and Bar -> Foo — could spuriously be + // rejected: Bar's rename to "_Foo" would appear to collide with Foo's own, still-pristine "_Foo" field.) + final Set beingRenamed = new HashSet<>(); + for (final RecordTypeRename rename : renames.values()) { + if (rename.unionField != null && isCanonicalUnionFieldName(rename.unionField.getName(), rename.name)) { + beingRenamed.add(rename.unionField); + } + } + for (final RecordTypeRename rename : renames.values()) { + if (!beingRenamed.contains(rename.unionField)) { + continue; + } + final String newName = canonicalUnionFieldName(rename.newName); + final boolean hasCollision = unionBuilder.getFieldBuilderList().stream() + .anyMatch(fb -> fb != rename.unionField && fb.getName().equals(newName) && !beingRenamed.contains(fb)); + if (hasCollision) { + throw new MetaDataException("Cannot rename union field to " + newName + " as a field of that name already exists", + LogMessageKeys.RECORD_TYPE, rename.name); + } + } + } + + /** + * Renames the canonical union field, if present, for each rename in {@code renames}. The union field is a field + * of the union message type within {@code MetaData.records}. Callers must have already validated the renames + * via {@link #validateUnionFieldRenames}. + */ + private static void renameUnionFields(@Nonnull RecordTypeRenames renames) { + for (final RecordTypeRename rename : renames.values()) { + if (rename.unionField != null && isCanonicalUnionFieldName(rename.unionField.getName(), rename.name)) { + rename.unionField.setName(canonicalUnionFieldName(rename.newName)); + } + } + } + + /** + * A helper for {@link #renameRecordTypes} that applies the name mapping in a single walk over the message types + * in {@code MetaData.records}, using the given compiled file descriptor for type resolution. Field type + * references are resolved (via the original descriptor) to the original type they point at; if that original + * type is, or is nested within, any renamed type, the reference is rewritten to point at the renamed type. + */ + private static void renameRecordTypeUsagesInMessageTypes(@Nonnull List messageTypes, + @Nonnull RecordTypeRenames renames, + @Nonnull Descriptors.FileDescriptor fileDescriptor) { + // Walk every message type, rewriting references to renamed types and renaming the type itself. + for (final DescriptorProtos.DescriptorProto.Builder mtb : messageTypes) { + final String name = mtb.getName(); + + // Rewrite `typeName` field references within the message type. + final Descriptors.Descriptor descriptor = getMessageTypeByName(fileDescriptor, name); + renameRecordTypeUsagesInMessageType(mtb, renames, descriptor); + + final RecordTypeRename rename = renames.get(name); + if (rename == null) { + continue; + } + + // If renaming the union type, be sure that the `record.usage` option is set to UNION. + if (name.equals(DEFAULT_UNION_NAME) && getMessageTypeUsage(mtb) != RecordTypeOptions.Usage.UNION) { + setMessageTypeUsage(mtb, RecordTypeOptions.Usage.UNION); + } + + // Rename the message type itself. + mtb.setName(rename.newName); + } + } + + /** + * Recursively rewrites {@code typeName} field references within a message type and its nested types. For each + * message or enum field, it resolves the original referenced type to its fully-qualified name and, if that type is + * or is nested within any renamed type, rewrites the field's {@code typeName} accordingly. + */ + private static void renameRecordTypeUsagesInMessageType(@Nonnull DescriptorProtos.DescriptorProto.Builder messageTypeBuilder, + @Nonnull RecordTypeRenames renames, + @Nonnull Descriptors.Descriptor descriptorForMessage) { + for (final DescriptorProtos.FieldDescriptorProto.Builder field : messageTypeBuilder.getFieldBuilderList()) { + if (!field.hasTypeName() || field.getTypeName().isEmpty()) { + continue; + } + final String fullReferencedName = resolveFieldTypeFullName(descriptorForMessage, field); + if (fullReferencedName == null) { + continue; + } + + // Check the referenced type, then each of its containing types in turn, against the renamed types: + // truncate `candidateName` at the last '.' to walk from the referenced type up to its outermost + // containing type, which is equivalent to (but cheaper than) repeatedly calling `getContainingType()`. + // For example, if `fullReferencedName` is ".pkg.Outer.Inner" and "Outer" is being renamed to "NewOuter", + // `candidateName` first tries ".pkg.Outer.Inner" (no match), then ".pkg.Outer" (matches), yielding the + // rewritten type name ".pkg.NewOuter" + ".Inner" = ".pkg.NewOuter.Inner". + String candidateName = fullReferencedName; + while (true) { + // If `candidateName` is a renamed type, substitute its new name for the matched prefix, leaving any + // trailing nested-type suffix (e.g. ".InnerRecord") untouched. + final RecordTypeRename rename = renames.getByFullName(candidateName); + if (rename != null) { + field.setTypeName(rename.fullNewName + fullReferencedName.substring(candidateName.length())); + break; + } + // Strip the last name segment to move up to the containing type; stop once there is no more + // package/type prefix left (the leading '.' is always at index 0). + final int lastDot = candidateName.lastIndexOf('.'); + if (lastDot <= 0) { + break; + } + candidateName = candidateName.substring(0, lastDot); + } + } + + // Recurse into nested types, since a field elsewhere in the file may reference one of them. + for (final DescriptorProtos.DescriptorProto.Builder nestedTypeBuilder : messageTypeBuilder.getNestedTypeBuilderList()) { + final Descriptors.Descriptor nestedDescriptor = Objects.requireNonNull( + descriptorForMessage.findNestedTypeByName(nestedTypeBuilder.getName()), + "FileDescriptor does not have nested type that exists in protobuf"); + // Recursively rewrite field type references within the nested type. + renameRecordTypeUsagesInMessageType(nestedTypeBuilder, renames, nestedDescriptor); + } + } + + /** + * Validates that {@code MetaData.user_defined_functions} is empty, since a user-defined function may be a + * string that needs parsing to figure out the record types it references, which renaming does not support. + * Performs no mutation, so that this can be called before any other edits are made. + */ + private static void validateNoUserDefinedFunctions(@Nonnull RecordMetaDataProto.MetaData.Builder metaDataBuilder) { + if (metaDataBuilder.getUserDefinedFunctionsCount() > 0) { + throw new MetaDataException("Renaming record types with UserDefinedFunctions is not supported"); + } + } + + /** + * Rewrites {@code MetaData.record_types} for every {@code RECORD}-usage rename in {@code renames}. Any collision + * with an unrenamed type has already been ruled out upfront, by the caller. + */ + private static void renameRecordTypeUsagesInRecordTypes( + @Nonnull List recordTypes, + @Nonnull RecordTypeRenames renames) { + for (final var recordType : recordTypes) { + final String newName = renames.get(recordType.getName(), RecordTypeOptions.Usage.RECORD); + if (newName != null) { + recordType.setName(newName); + } + } + } + + /** + * Rewrites the record types referenced by any {@code MetaData.indexes} entry, for every {@code RECORD}-usage + * rename in {@code renames}. + */ + private static void renameRecordTypeUsagesInIndexes(@Nonnull List indexes, + @Nonnull RecordTypeRenames renames) { + for (final var index : indexes) { + for (int i = 0; i < index.getRecordTypeCount(); i++) { + final String newName = renames.get(index.getRecordType(i), RecordTypeOptions.Usage.RECORD); + if (newName != null) { + index.setRecordType(i, newName); + } + } + } + } + + /** + * Updates the join constituents in {@code MetaData.joined_record_types} that reference any {@code RECORD}-usage + * rename in {@code renames}; renames of any other usage are ignored. + */ + private static void renameRecordTypeUsagesInJoinedRecordTypes( + @Nonnull List joinedRecordTypes, + @Nonnull RecordTypeRenames renames) { + for (final var joined : joinedRecordTypes) { + for (final var constituent : joined.getJoinConstituentsBuilderList()) { + final RecordTypeRename rename = renames.get(constituent.getRecordType()); + if (rename != null && rename.usage == RecordTypeOptions.Usage.RECORD) { + constituent.setRecordType(rename.newName); + } + } + } + } + + /** + * Validates the nested constituents of {@code MetaData.unnested_record_types} affected by any renamed type: any + * constituent whose type is nested within a renamed type, other than as that type's own (non-nested) + * constituent, causes a {@link MetaDataException}, since renaming a type used by a non-parent unnested + * constituent is not supported. Performs no mutation, so that this can be called before any other edits are + * made. + */ + private static void validateRecordTypeUsagesInUnnestedRecordTypes( + @Nonnull List unnestedRecordTypes, + @Nonnull Descriptors.FileDescriptor fileDescriptor, + @Nonnull RecordTypeRenames renames) { + for (var unnested : unnestedRecordTypes) { + for (var constituent : unnested.getNestedConstituentsBuilderList()) { + // The nested constituents would most likely be nested types, not record types, and thus would not be + // renamed. + if (constituent.getParent().isEmpty()) { + continue; + } + final String name = constituent.getTypeName(); + final Descriptors.Descriptor constituentTypeDescriptor + = UnnestedRecordTypeBuilder.findDescriptorByName(fileDescriptor, name); + if (constituentTypeDescriptor == null) { + throw new MetaDataException("missing descriptor for nested constituent") + .addLogInfo(LogMessageKeys.EXPECTED, name) + .addLogInfo(LogMessageKeys.CONSTITUENT, constituent.getName()); + } + // Walk up the containing-type chain: if the constituent's type, or any type it is nested within, is + // being renamed, this non-parent reference to it can't be safely updated. + for (Descriptors.Descriptor typeDescriptor = constituentTypeDescriptor; + typeDescriptor != null; + typeDescriptor = typeDescriptor.getContainingType()) { + if (renames.getByFullName("." + typeDescriptor.getFullName()) != null) { + throw new MetaDataException("Renaming types used by non-parent unnested constituents is not supported"); + } + } + } + } + } + + /** + * Renames the nested constituents of {@code MetaData.unnested_record_types} that directly name (as their own, + * non-nested type) any rename in {@code renames}. Callers must have already validated the rename via + * {@link #validateRecordTypeUsagesInUnnestedRecordTypes}. + */ + private static void renameRecordTypeUsagesInUnnestedRecordTypes( + @Nonnull List unnestedRecordTypes, + @Nonnull RecordTypeRenames renames) { + for (var unnested : unnestedRecordTypes) { + for (var constituent : unnested.getNestedConstituentsBuilderList()) { + if (constituent.getParent().isEmpty()) { + final RecordTypeRename rename = renames.get(constituent.getTypeName()); + if (rename != null) { + constituent.setTypeName(rename.newName); + } + } + } + } + } + /** * Add a field to a record type. * 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 92d7e494787..b11a6b12aa1 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 @@ -3,7 +3,7 @@ * * This source file is part of the FoundationDB open source project * - * Copyright 2015-2019 Apple Inc. and the FoundationDB project authors + * Copyright 2015-2026 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. @@ -26,31 +26,39 @@ import com.apple.foundationdb.record.TestRecords1Proto; import com.apple.foundationdb.record.TestRecordsDoubleNestedProto; import com.apple.foundationdb.record.TestRecordsEnumProto; +import com.apple.foundationdb.record.metadata.Key; import com.apple.foundationdb.record.metadata.MetaDataException; import com.apple.foundationdb.record.metadata.RecordType; +import com.apple.foundationdb.record.metadata.SyntheticRecordType; import com.apple.foundationdb.record.provider.foundationdb.MetaDataProtoEditor.FieldTypeMatch; import com.apple.test.BooleanSource; +import com.apple.test.Tags; import com.google.protobuf.DescriptorProtos; import com.google.protobuf.Descriptors; import com.google.protobuf.util.JsonFormat; import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.Tag; 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 org.junit.jupiter.params.provider.ValueSource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import javax.annotation.Nonnull; import javax.annotation.Nullable; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.function.Consumer; import java.util.function.Function; +import java.util.function.UnaryOperator; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -62,12 +70,14 @@ import static org.junit.jupiter.api.Assertions.assertThrows; /** - * Tests for the meta-data proto editor. There are more tests for this class (in action) in the {@link FDBMetaDataStoreTest} - * class. Those tests are more end-to-end, and they are for doing things like testing that when the meta-data - * are read from the database, edited, and written back, everything works. These tests focus on just the editor - * itself. + * Unit tests for the metadata proto editor. These tests focus on just the editor itself. + * + *

There are more tests for this class (in action) in {@link FDBMetaDataStoreTest}. Those tests are more end-to-end, + * and they are for doing things like testing that when the metadata are read from the database, edited, and written + * back, everything works. */ public class MetaDataProtoEditorUnitTest { + private static final Logger LOGGER = LoggerFactory.getLogger(MetaDataProtoEditorUnitTest.class); @Nonnull private FieldTypeMatch fieldIsType(@Nonnull DescriptorProtos.FileDescriptorProto.Builder file, @@ -300,6 +310,15 @@ public void nestedFieldIsTypeUnqualified() throws Descriptors.DescriptorValidati assertEquals(Set.of("MiddleRecord"), getNestedTypeNames(renamedDescriptor.findMessageTypeByName("OtterRecord"))); assertEquals(Set.of("InnerRecord"), getNestedTypeNames(renamedDescriptor.findMessageTypeByName("OtterRecord") .findNestedTypeByName("MiddleRecord"))); + + // Cross-check: Renaming the same single type via the batched `renameRecordTypes()` method should produce + // a byte-for-byte identical result. + final RecordMetaDataProto.MetaData.Builder batchedBuilder = metaData.toProto().toBuilder(); + MetaDataProtoEditor.renameRecordTypes( + batchedBuilder, + name -> name.equals("OuterRecord") ? "OtterRecord" : name, + getDependencies(metaData)); + assertEquals(metaDataProtoBuilder.build(), batchedBuilder.build()); } @Nonnull @@ -315,6 +334,57 @@ private static Descriptors.FileDescriptor[] getDependencies(final RecordMetaData return metaData.getRecordsDescriptor().getDependencies().toArray(new Descriptors.FileDescriptor[0]); } + /** + * A naive implementation of {@link MetaDataProtoEditor#renameRecordTypes} for testing purposes. Applies + * {@code renamer} to every top-level record type in {@code originalProto}, one type at a time via + * {@link MetaDataProtoEditor#renameRecordType}. + */ + @Nonnull + private static RecordMetaDataProto.MetaData renameRecordTypesOneByOne( + @Nonnull RecordMetaDataProto.MetaData originalProto, + @Nonnull UnaryOperator renamer, + @Nonnull Descriptors.FileDescriptor[] dependencies) { + final RecordMetaDataProto.MetaData.Builder builder = originalProto.toBuilder(); + for (final String recordType : MetaDataProtoEditor.getRecordTypes(builder)) { + MetaDataProtoEditor.renameRecordType(builder, recordType, renamer.apply(recordType), dependencies); + } + return builder.build(); + } + + /** + * Asserts that applying {@code renamer} via the batched {@link MetaDataProtoEditor#renameRecordTypes} method + * produces byte-for-byte the same metadata as applying it one type at a time via {@link #renameRecordTypesOneByOne}. + * Note though that this cross-check is only applicable to renamings whose validity depends on iteration order + * (see {@link #conflictingName}). + */ + private static void crossCheckRenamedMetaData( + @Nonnull RecordMetaDataProto.MetaData originalProto, + @Nonnull UnaryOperator renamer, + @Nonnull Descriptors.FileDescriptor[] dependencies, + @Nonnull RecordMetaDataProto.MetaData batchedResult) { + final RecordMetaDataProto.MetaData oneByOne = renameRecordTypesOneByOne(originalProto, renamer, dependencies); + // Compare via `RecordMetaData.build().toProto()` rather than the raw protos directly, since building a + // `RecordMetaData` normalizes some fields (e.g., filling in `nullInterpretation`) that may otherwise differ + // in representation, though not in meaning, depending on which path produced the raw proto. + final RecordMetaData batchedRecordMetaData = RecordMetaData.build(batchedResult); + final RecordMetaData oneByOneRecordMetaData = RecordMetaData.build(oneByOne); + assertEquals(batchedRecordMetaData.toProto(), oneByOneRecordMetaData.toProto()); + } + + /** + * Asserts that {@code renamer} is rejected by both the batched {@link MetaDataProtoEditor#renameRecordTypes} and + * an equivalent one-by-one sequence of {@link MetaDataProtoEditor#renameRecordType} calls. + */ + private static void crossRenameRecordTypesIsRejected( + @Nonnull RecordMetaDataProto.MetaData originalProto, + @Nonnull UnaryOperator renamer, + @Nonnull Descriptors.FileDescriptor[] dependencies) { + assertThrows(MetaDataException.class, + () -> MetaDataProtoEditor.renameRecordTypes(originalProto.toBuilder(), renamer, dependencies)); + assertThrows(MetaDataException.class, + () -> renameRecordTypesOneByOne(originalProto, renamer, dependencies)); + } + private void renameFieldTypes(@Nonnull DescriptorProtos.DescriptorProto.Builder messageTypeBuilder, @Nonnull String oldTypeName, @Nonnull String newTypeName) { messageTypeBuilder.getFieldBuilderList().forEach(field -> { if (field.getTypeName().equals(oldTypeName)) { @@ -368,6 +438,12 @@ public void renameOuterTypeWithNestedTypeWithSameName() throws Descriptors.Descr assertSame(renamedOuterOuterInner, renamedOuterOuter.findFieldByName("inner").getMessageType()); assertSame(renamedOuter, renamedOuterOuterInner.findFieldByName("outer").getMessageType()); + // Cross-check: Renaming the same single type via the batched `renameRecordTypes()` should produce a + // byte-for-byte identical result. + final RecordMetaDataProto.MetaData.Builder batchedBuilder = metaData.toProto().toBuilder(); + MetaDataProtoEditor.renameRecordTypes(batchedBuilder, + name -> name.equals("OuterRecord") ? "OtterRecord" : name, getDependencies(metaData)); + assertEquals(metaDataProtoBuilder.build(), batchedBuilder.build()); } public static RecordMetaDataProto.MetaData.Builder loadMetaData(@Nonnull String name) throws IOException { @@ -390,12 +466,32 @@ public static Stream renamableFiles() { "DuplicateUnionFields.json", "OneTypeWithIndexes.json", "MultiTypeIndex.json", - "UniversalIndex.json", - "UnnestedExternalType.json", - "UnnestedInternal.json", - "Joined.json" + "UniversalIndex.json" ).map(filename -> Arguments.of(filename, (Consumer) renamed -> { })), Stream.of( + Arguments.of("UnnestedExternalType.json", + (Consumer) renamed -> { + // "parent" (T1) is a top-level RECORD type and gets renamed; "child" names the + // dependency-defined UUID type, which is not a top-level record type and so is + // left untouched. + assertEquals(Map.of("parent", simpleRename("T1"), "child", "UUID"), + constituentTypeNames(renamed)); + }), + Arguments.of("UnnestedInternal.json", + (Consumer) renamed -> { + // "parent" (T2) is a top-level RECORD type and gets renamed; "child" names T1, + // which has NESTED usage and so is left untouched. + assertEquals(Map.of("parent", simpleRename("T2"), "child", "T1"), + constituentTypeNames(renamed)); + }), + Arguments.of("Joined.json", + (Consumer) renamed -> { + final SyntheticRecordType join = renamed.getSyntheticRecordType("JOIN"); + assertEquals(Set.of(simpleRename("T1"), simpleRename("T2")), + join.getConstituents().stream() + .map(constituent -> constituent.getRecordType().getName()) + .collect(Collectors.toSet())); + }), Arguments.of("AlsoInDependency.json", (Consumer) renamed -> { final Descriptors.Descriptor uuidType = getMessage(renamed, simpleRename("UUID")); @@ -436,6 +532,17 @@ private static Descriptors.Descriptor getFieldMessageType(final RecordMetaData r .findFirst().orElseThrow().getMessageType(); } + /** + * Maps each constituent's own name (e.g. "parent", "child") to the name of the record type it names, for the + * unnested synthetic record type named {@code "__3_syntheticType_1"} in the fixtures that use it. + */ + @Nonnull + private static Map constituentTypeNames(final RecordMetaData renamed) { + return renamed.getSyntheticRecordType("__3_syntheticType_1").getConstituents().stream() + .collect(Collectors.toMap(SyntheticRecordType.Constituent::getName, + constituent -> constituent.getRecordType().getName())); + } + @ParameterizedTest(name = "{0}") @MethodSource("renamableFiles") void simplePrefix(String name, Consumer extraAssertions) throws IOException { @@ -443,18 +550,253 @@ void simplePrefix(String name, Consumer extraAssertions) throws extraAssertions.accept(renamed); } + /** + * The rename must reject a renamer that maps two distinct record types to the same new name. + */ + @Test + void batchedRejectsCollidingRenames() throws IOException { + final RecordMetaDataProto.MetaData originalProto = loadMetaData("TwoBoringTypes.json").build(); + crossRenameRecordTypesIsRejected( + originalProto, + name -> "Collision", + RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + } + + /** + * A batch that swaps two record types' names must succeed, since neither rename target collides with an + * "unrenamed" type — both are moving. This exercises the {@code beingRenamed} exclusion in + * {@code validateUnionFieldRenames()} (and the analogous exclusion in {@code analyzeRecordTypeRenames()}), + * without which this scenario would be spuriously rejected. Unlike the other rename tests here, this one is + * batched-only: like {@link #conflictingName}, a swap is order-dependent for the one-by-one path (renaming + * "UUID" to "T2" first would collide with the not-yet-renamed "T2"). + */ + @Test + void batchedAllowsSwappingTwoNames() throws IOException { + final RecordMetaDataProto.MetaData originalProto = loadMetaData("AlsoInDependency.json").build(); + final RecordMetaDataProto.MetaData.Builder builder = originalProto.toBuilder(); + MetaDataProtoEditor.renameRecordTypes(builder, + name -> name.equals("UUID") ? "T2" : name.equals("T2") ? "UUID" : name, + RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + final RecordMetaData renamed = RecordMetaData.build(builder.build()); + assertEquals(Set.of("UUID", "T2"), renamed.getRecordTypes().keySet()); + // Content moves with the name: UUID's original single-field shape is now under "T2", and vice versa. + assertEquals(1, renamed.getRecordType("T2").getDescriptor().getFields().size()); + assertEquals(3, renamed.getRecordType("UUID").getDescriptor().getFields().size()); + } + + /** + * The rename must reject the schemas that rename a type used by a non-parent unnested constituent. + */ @ParameterizedTest @ValueSource(strings = { "UnnestedRenamed.json", "UnnestedRenamedNested.json", }) void unsupported(String name) throws IOException { - final RecordMetaDataProto.MetaData.Builder builder = loadMetaData(name); + final RecordMetaDataProto.MetaData originalProto = loadMetaData(name).build(); + RecordMetaData.build(originalProto); // ensure original metadata is valid + crossRenameRecordTypesIsRejected( + originalProto, + MetaDataProtoEditorUnitTest::simpleRename, + RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + } + + /** + * The rename must reject a renamer that maps a record type to the name of a distinct, un-renamed existing type. + */ + @Test + void batchedRejectsRenameToExistingType() throws IOException { + final RecordMetaDataProto.MetaData originalProto = loadMetaData("TwoBoringTypes.json").build(); + crossRenameRecordTypesIsRejected( + originalProto, + name -> name.equals("T1") ? "T2" : name, + RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + } + + /** + * The rename must reject a renamer that maps a record type to the name of an existing record type that is + * registered in {@code MetaData.record_types} but has no corresponding message type in {@code MetaData.records} + * (as would be the case for a record type whose message is defined in a dependency file, i.e., "imported"). + */ + @Test + void batchedRejectsRenameToImportedType() throws IOException { + final RecordMetaDataProto.MetaData.Builder builder = loadMetaData("TwoBoringTypes.json"); + builder.addRecordTypes(RecordMetaDataProto.RecordType.newBuilder().setName("Imported").build()); + final RecordMetaDataProto.MetaData originalProto = builder.build(); + crossRenameRecordTypesIsRejected( + originalProto, + name -> name.equals("T1") ? "Imported" : name, + RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + } + + /** + * An imported record type, i.e., one registered in {@code MetaData.record_types} whose message type lives in a + * dependency file rather than in {@code MetaData.records}, must be left untouched by the rename, and the renamer + * must not even be applied to it. This is a documented divergence from + * {@link MetaDataProtoEditor#renameRecordType}, which rejects the same rename outright, so it is asserted + * separately for each path. + */ + @Test + void batchedSkipsImportedType() throws IOException { + final RecordMetaDataProto.MetaData.Builder builder = loadMetaData("TwoBoringTypes.json"); + builder.addRecordTypes(RecordMetaDataProto.RecordType.newBuilder().setName("Imported").build()); final RecordMetaDataProto.MetaData originalProto = builder.build(); + final Descriptors.FileDescriptor[] dependencies = + RecordMetaDataBuilder.getDependencies(originalProto, Map.of()); + + // The batched path renames the real top-level types, and never consults the renamer for the imported one. + final Set renamerSawNames = new LinkedHashSet<>(); + final RecordMetaDataProto.MetaData.Builder batchedBuilder = originalProto.toBuilder(); + MetaDataProtoEditor.renameRecordTypes(batchedBuilder, name -> { + renamerSawNames.add(name); + return simpleRename(name); + }, dependencies); + assertEquals(Set.of("T1", "T2"), renamerSawNames); + assertEquals(List.of(simpleRename("T1"), simpleRename("T2"), "Imported"), + MetaDataProtoEditor.getRecordTypes(batchedBuilder)); + + // The one-by-one path, by contrast, rejects the very same rename. + final MetaDataException exception = assertThrows(MetaDataException.class, + () -> MetaDataProtoEditor.renameRecordType(originalProto.toBuilder(), "Imported", + simpleRename("Imported"), dependencies)); + assertEquals("No record type found with name Imported", exception.getMessage()); + } + + /** + * Renaming a top-level record type must not be blocked by an unrelated nested type that merely shares its simple + * name. In the fixture, the top-level {@code T1} is renamed while an unnested constituent references {@code T2}'s + * own nested type, also named {@code T1}; only a fully-qualified comparison tells the two apart. + */ + @Test + void shadowedNestedTypeNameDoesNotBlockRename() throws IOException { + final RecordMetaDataProto.MetaData originalProto = loadMetaData("UnnestedShadowedName.json").build(); RecordMetaData.build(originalProto); // ensure original metadata is valid - assertThrows(MetaDataException.class, () -> MetaDataProtoEditor.renameRecordTypes(builder, + final RecordMetaData renamed = runRename(originalProto, + name -> name.equals("T1") ? simpleRename("T1") : name, + name -> name.equals(simpleRename("T1")) ? "T1" : name); + assertEquals(Set.of(simpleRename("T1"), "T2"), renamed.getRecordTypes().keySet()); + // The shadowing nested type keeps its own name, and the constituent still points at it rather than at the + // renamed top-level type. (Constituent type names are reported as simple names, so "T1" here is T2's nested + // type; the descriptor's full name below distinguishes it from the renamed top-level one.) + assertEquals(Map.of("parent", "T2", "child", "T1"), constituentTypeNames(renamed)); + assertEquals("T2.T1", renamed.getSyntheticRecordType("__3_syntheticType_1").getConstituents().stream() + .filter(constituent -> constituent.getName().equals("child")) + .findFirst().orElseThrow().getRecordType().getDescriptor().getFullName()); + } + + /** + * The rename must reject a renamer whose canonical union field name would collide with an existing, + * non-canonically-named union field of another (un-renamed) type. Unlike the other exception tests here, this + * one is batched-only: renaming one type at a time via {@link MetaDataProtoEditor#renameRecordType} silently + * leaves the colliding field under its old name instead of throwing (see {@link #conflictingName}). + */ + @Test + void batchedRejectsUnionFieldCollision() throws IOException { + final RecordMetaDataProto.MetaData originalProto = loadMetaData("DuplicateUnionFields.json").build(); + final MetaDataException exception = assertThrows(MetaDataException.class, + () -> MetaDataProtoEditor.renameRecordTypes( + originalProto.toBuilder(), + name -> name.equals("T2") ? "T1_1" : name, + RecordMetaDataBuilder.getDependencies(originalProto, Map.of()))); + Assertions.assertThat(exception.getMessage()) + .startsWith("Cannot rename union field to ") + .endsWith("as a field of that name already exists"); + } + + /** + * The rename must reject any renaming when the metadata has {@code user_defined_functions}, since a + * user-defined function may be a string that references record types by name in ways that renaming cannot + * safely account for. + */ + @Test + void batchedRejectsUserDefinedFunctions() throws IOException { + final RecordMetaDataProto.MetaData.Builder builder = loadMetaData("TwoBoringTypes.json"); + builder.addUserDefinedFunctions(RecordMetaDataProto.PUserDefinedFunction.newBuilder().build()); + final RecordMetaDataProto.MetaData originalProto = builder.build(); + crossRenameRecordTypesIsRejected( + originalProto, + MetaDataProtoEditorUnitTest::simpleRename, + RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + } + + /** + * The rename must reject a renamer that maps a non-union record type to the default union name. + */ + @Test + void batchedRejectsRenameToDefaultUnionName() throws IOException { + final RecordMetaDataProto.MetaData originalProto = loadMetaData("TwoBoringTypes.json").build(); + crossRenameRecordTypesIsRejected( + originalProto, + name -> name.equals("T1") ? RecordMetaDataBuilder.DEFAULT_UNION_NAME : name, + RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + } + + /** + * The rename must reject any renaming when the union message type itself declares a nested type. + */ + @Test + void batchedRejectsNestedTypeInUnion() throws IOException { + final RecordMetaDataProto.MetaData.Builder builder = loadMetaData("TwoBoringTypes.json"); + for (final DescriptorProtos.DescriptorProto.Builder messageType : builder.getRecordsBuilder().getMessageTypeBuilderList()) { + if (messageType.getName().equals(RecordMetaDataBuilder.DEFAULT_UNION_NAME)) { + messageType.addNestedType(DescriptorProtos.DescriptorProto.newBuilder().setName("Nested")); + } + } + final RecordMetaDataProto.MetaData originalProto = builder.build(); + crossRenameRecordTypesIsRejected( + originalProto, + MetaDataProtoEditorUnitTest::simpleRename, + RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + } + + /** + * A record type whose name contains a {@code '.'} cannot correspond to any message type in + * {@code MetaData.records}, since message type names are always simple identifiers. It is therefore treated as + * imported and skipped, rather than rejected. + */ + @Test + void batchedSkipsDottedRecordTypeName() throws IOException { + final RecordMetaDataProto.MetaData.Builder builder = loadMetaData("TwoBoringTypes.json"); + builder.addRecordTypes(RecordMetaDataProto.RecordType.newBuilder().setName("a.b").build()); + final RecordMetaDataProto.MetaData originalProto = builder.build(); + final Descriptors.FileDescriptor[] dependencies = RecordMetaDataBuilder.getDependencies(originalProto, Map.of()); + final RecordMetaDataProto.MetaData.Builder renamed = originalProto.toBuilder(); + MetaDataProtoEditor.renameRecordTypes(renamed, name -> name.equals("a.b") ? "c" : name, dependencies); + assertEquals(List.of("T1", "T2", "a.b"), MetaDataProtoEditor.getRecordTypes(renamed)); + } + + /** + * The rename must reject any renaming when {@code MetaData.records} has no union message type at all. + */ + @Test + void batchedRejectsMissingUnion() throws IOException { + final RecordMetaDataProto.MetaData.Builder builder = loadMetaData("TwoBoringTypes.json"); + final DescriptorProtos.FileDescriptorProto.Builder recordsBuilder = builder.getRecordsBuilder(); + final List withoutUnion = recordsBuilder.getMessageTypeList().stream() + .filter(messageType -> !messageType.getName().equals(RecordMetaDataBuilder.DEFAULT_UNION_NAME)) + .collect(Collectors.toList()); + recordsBuilder.clearMessageType().addAllMessageType(withoutUnion); + final RecordMetaDataProto.MetaData originalProto = builder.build(); + crossRenameRecordTypesIsRejected( + originalProto, + MetaDataProtoEditorUnitTest::simpleRename, + RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + } + + /** + * The rename must reject any renaming when an unnested record type's non-parent constituent names a type that + * cannot be resolved in the file descriptor. + */ + @Test + void batchedRejectsMissingNestedConstituentDescriptor() throws IOException { + final RecordMetaDataProto.MetaData.Builder builder = loadMetaData("UnnestedInternal.json"); + RecordMetaData.build(builder.build()); // ensure original metadata is valid + builder.getUnnestedRecordTypesBuilder(0).getNestedConstituentsBuilder(1).setTypeName("DoesNotExist"); + final RecordMetaDataProto.MetaData originalProto = builder.build(); + crossRenameRecordTypesIsRejected( + originalProto, MetaDataProtoEditorUnitTest::simpleRename, - RecordMetaDataBuilder.getDependencies(builder.build(), Map.of()))); + RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); } @ParameterizedTest(name = "{0}") @@ -463,27 +805,37 @@ void doubleRename(String name) throws IOException { final RecordMetaDataProto.MetaData.Builder builder = loadMetaData(name); final RecordMetaDataProto.MetaData originalProto = builder.build(); final RecordMetaData originalMetaData = RecordMetaData.build(originalProto); - MetaDataProtoEditor.renameRecordTypes(builder, MetaDataProtoEditorUnitTest::simpleRename, - RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + final Descriptors.FileDescriptor[] dependencies = RecordMetaDataBuilder.getDependencies(originalProto, Map.of()); + MetaDataProtoEditor.renameRecordTypes(builder, MetaDataProtoEditorUnitTest::simpleRename, dependencies); final RecordMetaDataProto.MetaData firstRename = builder.build(); + crossCheckRenamedMetaData( + originalProto, + MetaDataProtoEditorUnitTest::simpleRename, + dependencies, + firstRename); basicRenameAsserts(firstRename, originalMetaData, MetaDataProtoEditorUnitTest::simpleRename, MetaDataProtoEditorUnitTest::simpleRenameUndo); // again - MetaDataProtoEditor.renameRecordTypes(builder, MetaDataProtoEditorUnitTest::simpleRename, - RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + MetaDataProtoEditor.renameRecordTypes(builder, MetaDataProtoEditorUnitTest::simpleRename, dependencies); final RecordMetaDataProto.MetaData secondRename = builder.build(); + crossCheckRenamedMetaData( + firstRename, + MetaDataProtoEditorUnitTest::simpleRename, + dependencies, + secondRename); basicRenameAsserts(secondRename, RecordMetaData.build(firstRename), MetaDataProtoEditorUnitTest::simpleRename, MetaDataProtoEditorUnitTest::simpleRenameUndo); final RecordMetaDataProto.MetaData.Builder restartBuilder = originalProto.toBuilder(); - MetaDataProtoEditor.renameRecordTypes(restartBuilder, + MetaDataProtoEditor.renameRecordTypes( + restartBuilder, oldName -> simpleRename(simpleRename(oldName)), - RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + dependencies); assertEquals(builder.build(), secondRename); } @@ -493,8 +845,10 @@ void doubleRename(String name) throws IOException { void conflictingName(boolean t1Conflicts) throws IOException { // In the future we may want to make this work, but for now, I just want to assert that it either succeeds // and produces a valid metadata, or fails with a clear exception. Currently, it is order dependent. + // Note also that this is exactly the kind of scenario where one-by-one renaming is *not* expected to match + // the batched renaming; so we use `runRenameBatchedOnly()` here rather than `runRename()`. final String prefix = "__Q_"; - final RecordMetaData withConflict = runRename(loadMetaData("TwoBoringTypes.json").build(), + final RecordMetaData withConflict = runRenameBatchedOnly(loadMetaData("TwoBoringTypes.json").build(), oldName -> { if (t1Conflicts) { return !oldName.equals("T1") ? prefix + "T1" : oldName; @@ -509,16 +863,64 @@ void conflictingName(boolean t1Conflicts) throws IOException { assertEquals(Set.of("T2", prefix + "T2"), withConflict.getRecordTypes().keySet()); } try { - runRename(withConflict.toProto(), + runRenameBatchedOnly(withConflict.toProto(), oldName -> prefix + oldName, newName -> newName.substring(prefix.length())); } catch (MetaDataException e) { - Assertions.assertThat(e) - .hasMessageStartingWith("Cannot rename record type to ") - .hasMessageEndingWith("as it already exists"); + Assertions.assertThat(e.getMessage()) + .satisfiesAnyOf( + message -> Assertions.assertThat(message).startsWith("Cannot rename record type to ").endsWith("as it already exists"), + message -> Assertions.assertThat(message).startsWith("Cannot rename union field to ").endsWith("as a field of that name already exists")); } } + /** + * A renamer that returns every type's own name unchanged must be a true no-op: the metadata must come out + * byte-for-byte identical to how it went in, confirming that the early-return path for an empty rename set + * never mutates anything. + */ + @ParameterizedTest(name = "{0}") + @MethodSource("renamableFiles") + void identityRenameIsNoOp(String name) throws IOException { + final RecordMetaDataProto.MetaData originalProto = loadMetaData(name).build(); + final RecordMetaDataProto.MetaData.Builder builder = originalProto.toBuilder(); + MetaDataProtoEditor.renameRecordTypes(builder, UnaryOperator.identity(), + RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + assertEquals(originalProto, builder.build()); + } + + /** + * Exercises {@link MetaDataProtoEditor#renameRecordTypes} with a large number of record types, to get a rough + * sense of how it scales. Not a strict benchmark: there is no assertion on timing, only a log line. Run via the + * {@code performanceTest} Gradle task. + */ + @Test + @Tag(Tags.Performance) + void renameManyRecordTypes() { + final int typeCount = 200; + final RecordMetaDataProto.MetaData.Builder builder = RecordMetaDataProto.MetaData.newBuilder(); + builder.setRecords(DescriptorProtos.FileDescriptorProto.newBuilder() + .addMessageType(DescriptorProtos.DescriptorProto.newBuilder().setName(RecordMetaDataBuilder.DEFAULT_UNION_NAME))); + for (int i = 0; i < typeCount; i++) { + MetaDataProtoEditor.addRecordType(builder, + DescriptorProtos.DescriptorProto.newBuilder() + .setName("T" + i) + .addField(DescriptorProtos.FieldDescriptorProto.newBuilder() + .setName("ID") + .setNumber(1) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_INT64)) + .build(), + Key.Expressions.field("ID")); + } + + final long startNanos = System.nanoTime(); + MetaDataProtoEditor.renameRecordTypes(builder, MetaDataProtoEditorUnitTest::simpleRename, new Descriptors.FileDescriptor[0]); + final long elapsedNanos = System.nanoTime() - startNanos; + LOGGER.info("Renamed {} record types in {} ms", typeCount, elapsedNanos / 1e6); + + assertEquals(typeCount, RecordMetaData.build(builder.build()).getRecordTypes().size()); + } + @Test void withAnnotations() { final RecordMetaDataProto.MetaData original = RecordMetaData.newBuilder() @@ -586,19 +988,40 @@ private RecordMetaData runRename(final String name) throws IOException { return runRename(originalProto, MetaDataProtoEditorUnitTest::simpleRename, MetaDataProtoEditorUnitTest::simpleRenameUndo); } + /** + * Performs the rename via the batched {@link MetaDataProtoEditor#renameRecordTypes} method, then cross-checks that + * an equivalent one-by-one sequence of {@link MetaDataProtoEditor#renameRecordType} calls produces byte-for-byte + * the same metadata. + * + * @see #runRenameBatchedOnly + */ @Nonnull private RecordMetaData runRename(final RecordMetaDataProto.MetaData originalProto, - final Function rename, + final UnaryOperator rename, final Function undoRename) { + final RecordMetaData renamed = runRenameBatchedOnly(originalProto, rename, undoRename); + final Descriptors.FileDescriptor[] dependencies = RecordMetaDataBuilder.getDependencies(originalProto, Map.of()); + crossCheckRenamedMetaData(originalProto, rename, dependencies, renamed.toProto()); + return renamed; + } + + /** + * Perform the rename using (only) the batched {@link MetaDataProtoEditor#renameRecordTypes} method, without any + * cross-checking. Use this for renamings whose validity is sensitive to iteration order (see + * {@link #conflictingName}), since the one-by-one path is not expected to match those. + * + * @see #runRename(RecordMetaDataProto.MetaData, UnaryOperator, Function) + */ + @Nonnull + private RecordMetaData runRenameBatchedOnly(final RecordMetaDataProto.MetaData originalProto, + final UnaryOperator rename, + final Function undoRename) { final RecordMetaDataProto.MetaData.Builder builder = originalProto.toBuilder(); final RecordMetaData originalMetaData = RecordMetaData.build(originalProto); - MetaDataProtoEditor.renameRecordTypes(builder, rename, - RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); - + final Descriptors.FileDescriptor[] dependencies = RecordMetaDataBuilder.getDependencies(originalProto, Map.of()); + MetaDataProtoEditor.renameRecordTypes(builder, rename, dependencies); final RecordMetaDataProto.MetaData build = builder.build(); - return basicRenameAsserts(build, originalMetaData, - rename, - undoRename); + return basicRenameAsserts(build, originalMetaData, rename, undoRename); } @Nonnull diff --git a/fdb-record-layer-core/src/test/resources/Joined.json b/fdb-record-layer-core/src/test/resources/Joined.json index dcc930a03da..dfbe010cfef 100644 --- a/fdb-record-layer-core/src/test/resources/Joined.json +++ b/fdb-record-layer-core/src/test/resources/Joined.json @@ -84,7 +84,7 @@ "fan_type": "SCALAR" } }, - "right": "textSearch", + "right": "T2", "right_expression": { "field": { "field_name": "ID", diff --git a/fdb-record-layer-core/src/test/resources/UnnestedShadowedName.json b/fdb-record-layer-core/src/test/resources/UnnestedShadowedName.json new file mode 100644 index 00000000000..19b01af3f0d --- /dev/null +++ b/fdb-record-layer-core/src/test/resources/UnnestedShadowedName.json @@ -0,0 +1,110 @@ +{ + "records": { + "dependency": [ + "record_metadata_options.proto" + ], + "message_type": [ + { + "name": "RecordTypeUnion", + "field": [ + { + "name": "_T1", + "number": 1, + "type": "TYPE_MESSAGE", + "type_name": "T1" + }, + { + "name": "_T2", + "number": 2, + "type": "TYPE_MESSAGE", + "type_name": "T2" + } + ] + }, + { + "name": "T1", + "field": [ + { + "name": "ID", + "number": 1, + "type": "TYPE_INT64" + } + ] + }, + { + "name": "T2", + "field": [ + { + "name": "ID", + "number": 1, + "type": "TYPE_INT64" + }, + { + "name": "sub", + "number": 2, + "label": "LABEL_REPEATED", + "type": "TYPE_MESSAGE", + "type_name": "T2.T1" + } + ], + "nested_type": [ + { + "name": "T1", + "field": [ + { + "name": "V", + "number": 1, + "type": "TYPE_STRING" + } + ] + } + ] + } + ] + }, + "record_types": [ + { + "name": "T1", + "primary_key": { + "field": { + "field_name": "ID", + "fan_type": "SCALAR" + } + } + }, + { + "name": "T2", + "primary_key": { + "field": { + "field_name": "ID", + "fan_type": "SCALAR" + } + } + } + ], + "unnested_record_types": [ + { + "name": "__3_syntheticType_1", + "record_type_key": { + "long_value": -1 + }, + "nested_constituents": [ + { + "name": "parent", + "type_name": "T2" + }, + { + "name": "child", + "parent": "parent", + "type_name": "T2.T1", + "nesting_expression": { + "field": { + "field_name": "sub", + "fan_type": "FAN_OUT" + } + } + } + ] + } + ] +}