From 3625acb8511844b1f0ba30ef43431f294e8b185a Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 28 Apr 2026 14:41:39 +0100 Subject: [PATCH 1/7] The `MetaDataEvolutionValidator` can now be configured to ignore field renames only on deprecated types The `MetaDataEvolutionValidator` was given an option to allow for field renames with #4034. That can be problematic in the general case, as we don't really want fields that are in use to change their field name. (For example, if two fields swap places accidentlly but they have identical types, allowing general field renames wouldn't catch that.) However, if a field has been marked as deprecated, then it is likely no longer in use, so allowing field renames for those fields is less problematic. It is also the only way to support a flow that allows for a field to be recreated (e.g., with a new type), though that operation has its own costs. This resolves #4020. --- .../metadata/MetaDataEvolutionValidator.java | 149 +++++- ...MetaDataEvolutionValidatorBuilderTest.java | 16 + .../MetaDataEvolutionValidatorTest.java | 439 +++++++++++++----- 3 files changed, 488 insertions(+), 116 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java index 604dcc5ce3..4a58f7e526 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java @@ -100,6 +100,8 @@ public class MetaDataEvolutionValidator { private final boolean allowNoVersionChange; private final boolean allowNoSinceVersion; private final boolean allowFieldRenames; + private final boolean allowDeprecatedFieldRenames; + private final boolean allowUndeprecatingFields; private final boolean allowIndexRebuilds; private final boolean allowMissingFormerIndexNames; private final boolean allowOlderFormerIndexAddedVersions; @@ -111,6 +113,8 @@ private MetaDataEvolutionValidator() { this.allowNoVersionChange = false; this.allowNoSinceVersion = false; this.allowFieldRenames = false; + this.allowDeprecatedFieldRenames = false; + this.allowUndeprecatingFields = false; this.allowIndexRebuilds = false; this.allowMissingFormerIndexNames = false; this.allowOlderFormerIndexAddedVersions = false; @@ -123,6 +127,8 @@ private MetaDataEvolutionValidator(@Nonnull Builder builder) { this.allowNoVersionChange = builder.allowNoVersionChange; this.allowNoSinceVersion = builder.allowNoSinceVersion; this.allowFieldRenames = builder.allowFieldRenames; + this.allowDeprecatedFieldRenames = builder.allowDeprecatedFieldRenames; + this.allowUndeprecatingFields = builder.allowUndeprecatingFields; this.allowIndexRebuilds = builder.allowIndexRebuilds; this.allowMissingFormerIndexNames = builder.allowMissingFormerIndexNames; this.allowOlderFormerIndexAddedVersions = builder.allowOlderFormerIndexAddedVersions; @@ -271,13 +277,26 @@ private void validateProtoSyntax(@Nonnull Descriptors.Descriptor oldDescriptor, private void validateField(@Nonnull FieldDescriptor oldFieldDescriptor, @Nonnull FieldDescriptor newFieldDescriptor, @Nonnull Set> seenDescriptors) { + final boolean oldDeprecated = oldFieldDescriptor.getOptions().getDeprecated(); + final boolean newDeprecated = newFieldDescriptor.getOptions().getDeprecated(); if (!oldFieldDescriptor.getName().equals(newFieldDescriptor.getName())) { - if (!allowFieldRenames) { + // Field renames are allowed if either: + // 1. We allow all field renames (allowFieldRenames is true) + // 2. We allow deprecated field renames and the field is deprecated, here determined by whether the old + // or new field is deprecated. (Using both the old and new fields means it is okay to change the + // name both when deprecating it and un-deprecating it.) + // We want to throw an error if neither of those is true, hence the statement below. We could theoretically + // use DeMorgan's to rewrite some of that, but at the cost of legibility + if (!(allowFieldRenames || (allowDeprecatedFieldRenames && (oldDeprecated || newDeprecated)))) { throw new MetaDataException("field renamed", LogMessageKeys.OLD_FIELD_NAME, oldFieldDescriptor.getName(), LogMessageKeys.NEW_FIELD_NAME, newFieldDescriptor.getName()); } } + if (!allowUndeprecatingFields && oldDeprecated && !newDeprecated) { + throw new MetaDataException("field is no longer deprecated", + LogMessageKeys.FIELD_NAME, oldFieldDescriptor.getName()); + } if (!oldFieldDescriptor.getType().equals(newFieldDescriptor.getType())) { validateTypeChange(oldFieldDescriptor, newFieldDescriptor); } @@ -376,7 +395,7 @@ private void validateRecordTypes(@Nonnull RecordMetaData oldMetaData, @Nonnull R LogMessageKeys.NEW_VERSION, newRecordType.getSinceVersion()); } final KeyExpression expectedPrimaryKey; - if (allowFieldRenames) { + if (allowsAnyFieldRenames()) { expectedPrimaryKey = RenameFieldsVisitor.renameFields(oldRecordType.getPrimaryKey(), oldRecordType.getDescriptor(), newRecordType.getDescriptor()); } else { expectedPrimaryKey = oldRecordType.getPrimaryKey(); @@ -669,7 +688,7 @@ private void validateIndex(@Nonnull RecordMetaData oldMetaData, @Nonnull Index o } // The index root expression must be the same, modulo field renames KeyExpression expectedKeyExpression = null; - if (allowFieldRenames) { + if (allowsAnyFieldRenames()) { for (String oldRecordTypeName : oldRecordTypeNames) { final Descriptor oldDescriptor = oldMetaData.getRecordType(oldRecordTypeName).getDescriptor(); final Descriptor newDescriptor = newMetaData.getRecordType(typeRenames.getOrDefault(oldRecordTypeName, oldRecordTypeName)).getDescriptor(); @@ -783,12 +802,84 @@ public boolean allowsNoSinceVersion() { * update those definitions. *

* + *

+ * Note that this is a strictly more permissive setting than + * {@linkplain #allowsDeprecatedFieldRenames() allowing deprecated field renames}. If this is set to {@code true}, + * then that other setting is effectively ignored. + *

+ * * @return whether this validator allows field names to change + * @see #allowsDeprecatedFieldRenames() */ public boolean allowsFieldRenames() { return allowFieldRenames; } + /** + * Whether this validator allows deprecated fields to be renamed. If this is {@code true}, then + * this behaves in an analogous fashion to if {@link #allowsFieldRenames()} returns {@code true}, but + * it only allows the change if the field has been deprecated. In general, removing fields is + * not allowed as it can result in unknown fields during Protobuf deserialization. For that reason, + * deprecating fields should be preferred to deleting them. Once a field is deprecated and no longer + * actively accessed, further modifications to the name of the now unused field may be safe. + * Note that this option will result in accepting a meta-data change if a field is deprecated + * in the same change that deprecates it. Additionally, if this validator {@link #allowsUndeprecatingFields()}, + * then this also allows deprecated fields to be renamed in the same change in which they are + * marked as no longer deprecated. + * + *

+ * One reason that a user may want to allow renaming deprecating fields is to support recreating + * an existing field. For example, there are certain incompatible changes that a user may want + * to make to a field (e.g., modifying its Protobuf type from {@code sfixed32} to {@code sfixed64}). + * To accomplish this, the user can deprecate the old field, rename it, and then create a new field + * with the original field's name. This is not an operation that is without cost (the user must, + * for example, delete or at least bump the {@linkplain Index#getLastModifiedVersion() last modified version} of + * any index referencing the field, and they may need to populate the new field with data from the + * deprecated field), but it may be something that a user wants to do while iterating on a test + * meta-data. However, this should generally be avoided in production use cases. + *

+ * + *

+ * Note that this is a strictly less permissive setting than more generally + * {@linkplain #allowsFieldRenames() allowing field renames}. If that configuration is set to {@code true}, then + * this configuration option is effectively ignored. + *

+ * + * @return whether this validator allows deprecated fields to be renamed + * @see #allowsFieldRenames() + */ + public boolean allowsDeprecatedFieldRenames() { + return allowDeprecatedFieldRenames; + } + + /** + * Whether this validator allows any kind of field renames. It captures whether either this validator + * {@link #allowsFieldRenames} or {@link #allowsDeprecatedFieldRenames()}. If this is false, then + * this validator will throw if any kind of field changes its name. The exact set of field renames + * that are allowed will depend on the values of the other two configuration parameters. + * + * @return whether this validator allows any kind of field renames + * @see #allowsFieldRenames() + * @see #allowsDeprecatedFieldRenames() + */ + public boolean allowsAnyFieldRenames() { + return allowFieldRenames || allowDeprecatedFieldRenames; + } + + /** + * Whether this validator allows fields that were previously deprecated to be marked as not + * deprecated. Deprecating a field should generally be a one-way operation, as it represents + * deleting a field from the meta-data. By default, validators will therefore reject any meta-data + * change where a field goes from deprecated to not deprecated. However, especially when iterating + * on a meta-data used only in testing, there may be times when a user wants to un-delete a field. + * This option provides the flexibility for the user to allow that. + * + * @return whether this validator allows fields to be undeprecated + */ + public boolean allowsUndeprecatingFields() { + return allowUndeprecatingFields; + } + /** * Whether this validator allows new meta-data that require existing indexes to be rebuilt. * This can happen if an index is modified and its {@linkplain Index#getLastModifiedVersion() last modified version} @@ -905,6 +996,8 @@ public static class Builder { private boolean allowNoVersionChange; private boolean allowNoSinceVersion; private boolean allowFieldRenames; + private boolean allowDeprecatedFieldRenames; + private boolean allowUndeprecatingFields; private boolean allowIndexRebuilds; private boolean allowMissingFormerIndexNames; private boolean allowOlderFormerIndexAddedVersions; @@ -916,6 +1009,8 @@ private Builder(@Nonnull MetaDataEvolutionValidator validator) { this.allowNoVersionChange = validator.allowNoVersionChange; this.allowNoSinceVersion = validator.allowNoSinceVersion; this.allowFieldRenames = validator.allowFieldRenames; + this.allowDeprecatedFieldRenames = validator.allowDeprecatedFieldRenames; + this.allowUndeprecatingFields = validator.allowUndeprecatingFields; this.allowIndexRebuilds = validator.allowIndexRebuilds; this.allowMissingFormerIndexNames = validator.allowMissingFormerIndexNames; this.allowOlderFormerIndexAddedVersions = validator.allowOlderFormerIndexAddedVersions; @@ -1020,6 +1115,54 @@ public boolean allowsFieldRenames() { return allowFieldRenames; } + /** + * Set whether the validator will allow deprecated fields to be renamed. + * + * @param allowDeprecatedFieldRenames whether the validator will allow deprecated fields to be renamed + * @return this builder + * @see MetaDataEvolutionValidator#allowsDeprecatedFieldRenames() + */ + @CanIgnoreReturnValue + @Nonnull + public Builder setAllowDeprecatedFieldRenames(boolean allowDeprecatedFieldRenames) { + this.allowDeprecatedFieldRenames = allowDeprecatedFieldRenames; + return this; + } + + /** + * Whether the validator will allow deprecated fields to be renamed. + * + * @return whether the validator will allow deprecated fields to be renamed + * @see MetaDataEvolutionValidator#allowsDeprecatedFieldRenames() + */ + public boolean allowsDeprecatedFieldRenames() { + return allowDeprecatedFieldRenames; + } + + /** + * Set whether the validator will allow deprecated fields to be marked as not deprecated. + * + * @param allowUndeprecatingFields whether the validator will allow deprecated fields to be un-deprecated + * @return this builder + * @see MetaDataEvolutionValidator#allowsUndeprecatingFields() + */ + @CanIgnoreReturnValue + @Nonnull + public Builder setAllowUndeprecatingFields(boolean allowUndeprecatingFields) { + this.allowUndeprecatingFields = allowUndeprecatingFields; + return this; + } + + /** + * Whether the validator will allow deprecated fields to be marked as not deprecated. + * + * @return whether the validator will allow deprecated fields to be un-deprecated + * @see MetaDataEvolutionValidator#allowsUndeprecatingFields() + */ + public boolean allowsUndeprecatingFields() { + return allowUndeprecatingFields; + } + /** * Set whether the validator will allow changes to indexes that require rebuilds. * @param allowIndexRebuilds whether the validator will allow changes to indexes that require rebuilds diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorBuilderTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorBuilderTest.java index 69e1e1f362..f707e5bb77 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorBuilderTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorBuilderTest.java @@ -110,6 +110,22 @@ void allowFieldRenames() { MetaDataEvolutionValidator::allowsFieldRenames); } + @Test + void allowDeprecatedFieldRenames() { + testSettingBooleanOption("allowDeprecatedFieldRenames", + MetaDataEvolutionValidator.Builder::setAllowDeprecatedFieldRenames, + MetaDataEvolutionValidator.Builder::allowsDeprecatedFieldRenames, + MetaDataEvolutionValidator::allowsDeprecatedFieldRenames); + } + + @Test + void allowUndeprecatingFields() { + testSettingBooleanOption("allowUndeprecatingFields", + MetaDataEvolutionValidator.Builder::setAllowUndeprecatingFields, + MetaDataEvolutionValidator.Builder::allowsUndeprecatingFields, + MetaDataEvolutionValidator::allowsUndeprecatingFields); + } + @Test void allowIndexRebuilds() { testSettingBooleanOption("allowIndexRebuilds", 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 b05236004d..9fc71b4532 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 @@ -27,6 +27,7 @@ import com.apple.foundationdb.record.RecordMetaDataOptionsProto; import com.apple.foundationdb.record.RecordMetaDataProto; import com.apple.foundationdb.record.TestRecords1Proto; +import com.apple.foundationdb.record.TestRecords4Proto; import com.apple.foundationdb.record.TestRecordsEnumProto; import com.apple.foundationdb.record.TestRecordsIdenticalTypesProto; import com.apple.foundationdb.record.TestRecordsWithHeaderProto; @@ -44,13 +45,17 @@ import com.apple.foundationdb.record.provider.common.text.TextTokenizer; import com.apple.foundationdb.record.provider.foundationdb.IndexMaintainerFactoryRegistryImpl; import com.apple.foundationdb.tuple.Tuple; +import com.apple.test.ParameterizedTestUtils; import com.google.protobuf.ByteString; import com.google.protobuf.DescriptorProtos; import com.google.protobuf.Descriptors; import com.google.protobuf.Descriptors.Descriptor; import com.google.protobuf.Descriptors.FieldDescriptor; import com.google.protobuf.Descriptors.FileDescriptor; +import org.junit.jupiter.api.Named; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -63,6 +68,7 @@ import java.util.function.Consumer; import java.util.function.UnaryOperator; import java.util.stream.Collectors; +import java.util.stream.Stream; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsInAnyOrder; @@ -111,6 +117,95 @@ static void assertInvalid(@Nonnull String errMsg, @Nonnull FileDescriptor oldFil assertInvalid(errMsg, oldFileDescriptor.findMessageTypeByName(RecordMetaDataBuilder.DEFAULT_UNION_NAME), newFileDescriptor.findMessageTypeByName(RecordMetaDataBuilder.DEFAULT_UNION_NAME)); } + private static class FieldRenameChecker { + @Nonnull + private final MetaDataEvolutionValidator baseValidator; + @Nonnull + private final MetaDataEvolutionValidator noRenamesValidator; + @Nonnull + private final MetaDataEvolutionValidator deprecatedOnlyValidator; + @Nonnull + private final MetaDataEvolutionValidator anyRenameValidator; + @Nonnull + private final MetaDataEvolutionValidator allRenamesValidator; + + public FieldRenameChecker(MetaDataEvolutionValidator baseValidator) { + this.baseValidator = baseValidator; + final MetaDataEvolutionValidator.Builder builder = baseValidator.asBuilder(); + + noRenamesValidator = builder + .setAllowDeprecatedFieldRenames(false) + .setAllowDeprecatedFieldRenames(false) + .build(); + assertFalse(noRenamesValidator.allowsAnyFieldRenames()); + assertFalse(noRenamesValidator.allowsDeprecatedFieldRenames()); + assertFalse(noRenamesValidator.allowsFieldRenames()); + + deprecatedOnlyValidator = builder + .setAllowDeprecatedFieldRenames(true) + .build(); + assertTrue(deprecatedOnlyValidator.allowsAnyFieldRenames()); + assertTrue(deprecatedOnlyValidator.allowsDeprecatedFieldRenames()); + assertFalse(deprecatedOnlyValidator.allowsFieldRenames()); + + anyRenameValidator = builder + .setAllowDeprecatedFieldRenames(false) + .setAllowFieldRenames(true) + .build(); + assertTrue(anyRenameValidator.allowsAnyFieldRenames()); + assertFalse(anyRenameValidator.allowsDeprecatedFieldRenames()); + assertTrue(anyRenameValidator.allowsFieldRenames()); + + // This should behave the same as anyRenameValidator, but it is included for completeness + allRenamesValidator = builder + .setAllowDeprecatedFieldRenames(true) + .build(); + assertTrue(allRenamesValidator.allowsAnyFieldRenames()); + assertTrue(allRenamesValidator.allowsDeprecatedFieldRenames()); + assertTrue(allRenamesValidator.allowsFieldRenames()); + } + + @Nonnull + public MetaDataEvolutionValidator getBaseValidator() { + return baseValidator; + } + + public void assertInvalidRenaming(@Nonnull String errMsg, boolean deprecatedOnly, @Nonnull RecordMetaData oldMetaData, @Nonnull RecordMetaData newMetaData) { + assertInvalid("field renamed", noRenamesValidator, oldMetaData, newMetaData); + assertInvalid(deprecatedOnly ? errMsg : "field renamed", deprecatedOnlyValidator, oldMetaData, newMetaData); + assertInvalid(errMsg, anyRenameValidator, oldMetaData, newMetaData); + assertInvalid(errMsg, allRenamesValidator, oldMetaData, newMetaData); + } + + public void assertValidRenaming(boolean deprecatedOnly, @Nonnull RecordMetaData oldMetaData, @Nonnull RecordMetaData newMetaData) { + assertInvalid("field renamed", noRenamesValidator, oldMetaData, newMetaData); + if (deprecatedOnly) { + deprecatedOnlyValidator.validate(oldMetaData, newMetaData); + } else { + assertInvalid("field renamed", deprecatedOnlyValidator, oldMetaData, newMetaData); + } + anyRenameValidator.validate(oldMetaData, newMetaData); + allRenamesValidator.validate(oldMetaData, newMetaData); + } + + public void assertValidRenaming(boolean deprecatedOnly, @Nonnull FileDescriptor oldFileDescriptor, @Nonnull FileDescriptor newFileDescriptor) { + assertValidRenaming(deprecatedOnly, oldFileDescriptor.findMessageTypeByName(RecordMetaDataBuilder.DEFAULT_UNION_NAME), newFileDescriptor.findMessageTypeByName(RecordMetaDataBuilder.DEFAULT_UNION_NAME)); + } + + public void assertValidRenaming(boolean deprecatedOnly, @Nonnull Descriptor oldUnionDescriptor, @Nonnull Descriptor newUnionDescriptor) { + assertInvalid("field renamed", noRenamesValidator, oldUnionDescriptor, newUnionDescriptor); + if (deprecatedOnly) { + deprecatedOnlyValidator.validateUnion(oldUnionDescriptor, newUnionDescriptor); + } else { + assertInvalid("field renamed", deprecatedOnlyValidator, oldUnionDescriptor, newUnionDescriptor); + } + anyRenameValidator.validateUnion(oldUnionDescriptor, newUnionDescriptor); + allRenamesValidator.validateUnion(oldUnionDescriptor, newUnionDescriptor); + } + } + + private final FieldRenameChecker fieldRenameChecker = new FieldRenameChecker(validator); + @Test void doNotChangeVersion() { // Check if a naive removal of the index without updating the version is checked @@ -218,6 +313,10 @@ static DescriptorProtos.FieldDescriptorProto.Builder addField(@Nonnull Descripto .setNumber(maxFieldNumber + 1); } + static void deprecateField(@Nonnull DescriptorProtos.FieldDescriptorProto.Builder field) { + field.getOptionsBuilder().setDeprecated(true); + } + @Test void changeSplitLongRecords() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); @@ -752,29 +851,112 @@ void dropField() { void renameField() { FileDescriptor updatedFile = mutateField("MySimpleRecord", "num_value_2", field -> field.setName("num_value_too")); + fieldRenameChecker.assertValidRenaming(false, TestRecords1Proto.getDescriptor(), updatedFile); - assertFalse(validator.allowsFieldRenames()); - assertInvalid("field renamed", TestRecords1Proto.getDescriptor(), updatedFile); RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, updatedFile); - assertInvalid("field renamed", metaData1, metaData2); + fieldRenameChecker.assertValidRenaming(false, metaData1, metaData2); + } - final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() - .setAllowFieldRenames(true) - .build(); - assertTrue(laxerValidator.allowsFieldRenames()); - laxerValidator.validateUnion(TestRecords1Proto.RecordTypeUnion.getDescriptor(), updatedFile.findMessageTypeByName(RecordMetaDataBuilder.DEFAULT_UNION_NAME)); + @Test + void renameDeprecatedField() { + FileDescriptor deprecatedFile = mutateField("MySimpleRecord", "num_value_2", + MetaDataEvolutionValidatorTest::deprecateField); + FileDescriptor renamedFile = mutateField("MySimpleRecord", "num_value_2", deprecatedFile, + field -> field.setName("num_value_too")); + fieldRenameChecker.assertValidRenaming(true, deprecatedFile, renamedFile); - laxerValidator.validate(metaData1, metaData2); + RecordMetaData metaData1 = RecordMetaData.build(deprecatedFile); + RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, renamedFile); + fieldRenameChecker.assertValidRenaming(true, metaData1, metaData2); } @Test - void renameFieldWithIndex() { + void renameFieldWhenMarkingDeprecated() { + FileDescriptor updatedFile = mutateField("MySimpleRecord", "num_value_2", field -> { + deprecateField(field); + field.setName("num_value_too"); + }); + fieldRenameChecker.assertValidRenaming(true, TestRecords1Proto.getDescriptor(), updatedFile); + + RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); + RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, updatedFile); + fieldRenameChecker.assertValidRenaming(true, metaData1, metaData2); + } + + @Test + void renameFieldWhenUndeprecating() { + // Change the field name at the same time we mark it as not deprecated + FileDescriptor deprecatedFile = mutateField("MySimpleRecord", "num_value_2", + MetaDataEvolutionValidatorTest::deprecateField); + FileDescriptor renamedFile = mutateField("MySimpleRecord", "num_value_2", deprecatedFile, + field -> field.clearOptions().setName("num_value_too")); + RecordMetaData metaData1 = RecordMetaData.build(deprecatedFile); + RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, renamedFile); + + // Under default settings, this is still not allowed (because we ban undeprecating fields) + assertFalse(fieldRenameChecker.getBaseValidator().allowsUndeprecatingFields()); + fieldRenameChecker.assertInvalidRenaming("field is no longer deprecated", true, metaData1, metaData2); + + // If un-deprecating fields is okay, then we get a valid renaming + final FieldRenameChecker laxerFieldRenameChecker = new FieldRenameChecker(fieldRenameChecker.getBaseValidator().asBuilder() + .setAllowUndeprecatingFields(true) + .build()); + assertTrue(laxerFieldRenameChecker.getBaseValidator().allowsUndeprecatingFields()); + laxerFieldRenameChecker.assertValidRenaming(true, metaData1, metaData2); + } + + @Test + void renameMixOfDeprecatedAndUndeprecatedFields() { + FileDescriptor deprecatedFile = mutateField("RestaurantTag", "weight", TestRecords4Proto.getDescriptor(), + MetaDataEvolutionValidatorTest::deprecateField); + deprecatedFile = mutateField("ReviewerStats", "hometown", deprecatedFile, + MetaDataEvolutionValidatorTest::deprecateField); + assertTrue(deprecatedFile.findMessageTypeByName("RestaurantTag").findFieldByName("weight").getOptions().getDeprecated()); + assertTrue(deprecatedFile.findMessageTypeByName("ReviewerStats").findFieldByName("hometown").getOptions().getDeprecated()); + final RecordMetaData metaData1 = RecordMetaData.build(deprecatedFile); + + // Rename one deprecated field + FileDescriptor renamedFile = mutateField("ReviewerStats", "hometown", deprecatedFile, + field -> field.setName("origin")); + assertTrue(renamedFile.findMessageTypeByName("ReviewerStats").findFieldByName("origin").getOptions().getDeprecated()); + fieldRenameChecker.assertValidRenaming(true, deprecatedFile.findMessageTypeByName("UnionDescriptor"), renamedFile.findMessageTypeByName("UnionDescriptor")); + fieldRenameChecker.assertValidRenaming(true, metaData1, replaceRecordsDescriptor(metaData1, renamedFile)); + + // Rename another deprecated field + renamedFile = mutateField("RestaurantTag", "weight", renamedFile, + field -> field.setName("weighting")); + assertTrue(renamedFile.findMessageTypeByName("RestaurantTag").findFieldByName("weighting").getOptions().getDeprecated()); + fieldRenameChecker.assertValidRenaming(true, deprecatedFile.findMessageTypeByName("UnionDescriptor"), renamedFile.findMessageTypeByName("UnionDescriptor")); + fieldRenameChecker.assertValidRenaming(true, metaData1, replaceRecordsDescriptor(metaData1, renamedFile)); + + // Rename a non deprecated field. Now, the fieldRenameChecker should only consider this valid if it allows + // all field renames + renamedFile = mutateField("RestaurantRecord", "reviews", renamedFile, + field -> field.setName("review_list")); + assertFalse(renamedFile.findMessageTypeByName("RestaurantRecord").findFieldByName("review_list").getOptions().getDeprecated()); + fieldRenameChecker.assertValidRenaming(false, deprecatedFile.findMessageTypeByName("UnionDescriptor"), renamedFile.findMessageTypeByName("UnionDescriptor")); + fieldRenameChecker.assertValidRenaming(false, metaData1, replaceRecordsDescriptor(metaData1, renamedFile)); + } + + @Nonnull + static Stream> deprecatedArgs() { + return ParameterizedTestUtils.booleans("deprecated"); + } + + @ParameterizedTest + @MethodSource("deprecatedArgs") + void renameFieldWithIndex(boolean deprecated) { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); FileDescriptor updatedFile = mutateMessageType("MySimpleRecord", simpleRecordType -> { simpleRecordType.getFieldBuilderList().stream() .filter(field -> field.getName().equals("str_value_indexed")) - .forEach(field -> field.setName("str_value_indexed_old")); + .forEach(field -> { + if (deprecated) { + deprecateField(field); + } + field.setName("str_value_indexed_old"); + }); // Add a new field also called str_value_indexed. This is necessary as the validation logic invoked // when building the meta-data will fail if there's an index on a field that doesn't exist @@ -785,14 +967,8 @@ void renameFieldWithIndex() { }); RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, updatedFile); - assertFalse(validator.allowsFieldRenames()); - assertInvalid("field renamed", metaData1, metaData2); - // This is rejected even if we allow field renames as the index expression has not been updated - final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() - .setAllowFieldRenames(true) - .build(); - assertInvalid("index key expression does not match required", laxerValidator, metaData1, metaData2); + fieldRenameChecker.assertInvalidRenaming("index key expression does not match required", deprecated, metaData1, metaData2); // This updates both the field name and its indexes which means that this is actually okay. RecordMetaData metaData3 = replaceRecordsDescriptor(metaData1, updatedFile, protoBuilder -> @@ -802,12 +978,12 @@ void renameFieldWithIndex() { } }) ); - assertInvalid("field renamed", metaData1, metaData3); - laxerValidator.validate(metaData1, metaData3); + fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); } - @Test - void renameFieldInUniversalIndex() { + @ParameterizedTest + @MethodSource("deprecatedArgs") + void renameFieldInUniversalIndex(boolean deprecated) { RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); metaDataBuilder.addUniversalIndex(new Index("all$num_value_2", "num_value_2")); RecordMetaData metaData1 = metaDataBuilder.build(); @@ -815,7 +991,12 @@ void renameFieldInUniversalIndex() { FileDescriptor updatedFile = mutateMessageType("MySimpleRecord", simpleRecordType -> { simpleRecordType.getFieldBuilderList().stream() .filter(field -> field.getName().equals("num_value_2")) - .forEach(field -> field.setName("num_value_2__old")); + .forEach(field -> { + if (deprecated) { + deprecateField(field); + } + field.setName("num_value_2__old"); + }); addField(simpleRecordType) .setName("num_value_2") @@ -824,22 +1005,20 @@ void renameFieldInUniversalIndex() { }); RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, updatedFile); - assertFalse(validator.allowsFieldRenames()); - assertInvalid("field renamed", metaData1, metaData2); - // Still not allowed as the multi-type index requires the new key expression num_value_2__old on one record // type but num_value_2 on another - final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() - .setAllowFieldRenames(true) - .build(); - assertTrue(laxerValidator.allowsFieldRenames()); - assertInvalid("field renames result in inconsistent index definition for multi-type index", laxerValidator, metaData1, metaData2); + fieldRenameChecker.assertInvalidRenaming("field renames result in inconsistent index definition for multi-type index", deprecated, metaData1, metaData2); // Update the other types num_value_2 so now all types rename num_value_2 the same way updatedFile = mutateMessageType("MyOtherRecord", updatedFile, otherRecordType -> { otherRecordType.getFieldBuilderList().stream() .filter(field -> field.getName().equals("num_value_2")) - .forEach(field -> field.setName("num_value_2__old")); + .forEach(field -> { + if (deprecated) { + deprecateField(field); + } + field.setName("num_value_2__old"); + }); addField(otherRecordType) .setName("num_value_2") @@ -847,23 +1026,27 @@ void renameFieldInUniversalIndex() { .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL); }); RecordMetaData metaData3 = replaceRecordsDescriptor(metaData1, updatedFile); - assertInvalid("field renamed", metaData1, metaData3); // Still not allowed as the index hasn't been updated - assertInvalid("index key expression does not match required", laxerValidator, metaData1, metaData3); + fieldRenameChecker.assertInvalidRenaming("index key expression does not match required", deprecated, metaData1, metaData3); RecordMetaData metaData4 = replaceIndex(metaData3, "all$num_value_2", indexProto -> indexProto.toBuilder().setRootExpression(Key.Expressions.field("num_value_2__old").toKeyExpression()).build()); - assertInvalid("field renamed", metaData1, metaData4); - laxerValidator.validate(metaData1, metaData4); + fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData4); } - @Test - void renameFieldInPrimaryKey() { + @ParameterizedTest + @MethodSource("deprecatedArgs") + void renameFieldInPrimaryKey(boolean deprecated) { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); FileDescriptor updatedFile = mutateMessageType("MySimpleRecord", simpleRecordType -> { simpleRecordType.getFieldBuilderList().stream() .filter(field -> field.getName().equals("rec_no")) - .forEach(field -> field.setName("old_rec_no")); + .forEach(field -> { + if (deprecated) { + deprecateField(field); + } + field.setName("old_rec_no"); + }); // Add a new field also called rec_no so that we pass meta-data validation addField(simpleRecordType) @@ -873,14 +1056,8 @@ void renameFieldInPrimaryKey() { }); RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, updatedFile); - assertFalse(validator.allowsFieldRenames()); - assertInvalid("field renamed", metaData1, metaData2); - // This is rejected even if we allow field renames as the primary key has not been updated - final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() - .setAllowFieldRenames(true) - .build(); - assertInvalid("record type primary key does not match required", laxerValidator, metaData1, metaData2); + fieldRenameChecker.assertInvalidRenaming("record type primary key does not match required", deprecated, metaData1, metaData2); // Now update the primary key to match the new record name RecordMetaData metaData3 = replaceRecordsDescriptor(metaData1, updatedFile, protoBuilder -> @@ -890,8 +1067,36 @@ void renameFieldInPrimaryKey() { } }) ); - assertInvalid("field renamed", metaData1, metaData3); - laxerValidator.validate(metaData1, metaData3); + fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); + } + + @Test + void deprecateField() { + FileDescriptor deprecatedFile = mutateField("MySimpleRecord", "str_value_indexed", + MetaDataEvolutionValidatorTest::deprecateField); + RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); + // The str_value_indexed field is used in indexes. We may want to ban those, which we should do in the + // MetaDataValidator (not the evolution validator). If we do, this may fail, and we should change this + // test to use a non-indexed field + RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, deprecatedFile); + // Deprecating fields is okay + validator.validate(metaData1, metaData2); + } + + @Test + void undeprecateField() { + FileDescriptor deprecatedFile = mutateField("MySimpleRecord", "num_value_3_indexed", + MetaDataEvolutionValidatorTest::deprecateField); + RecordMetaData metaData1 = RecordMetaData.build(deprecatedFile); + RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, TestRecords1Proto.getDescriptor()); + assertFalse(validator.allowsUndeprecatingFields()); + assertInvalid("field is no longer deprecated", metaData1, metaData2); + + final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() + .setAllowUndeprecatingFields(true) + .build(); + assertTrue(laxerValidator.allowsUndeprecatingFields()); + laxerValidator.validate(metaData1, metaData2); } @Test @@ -989,7 +1194,16 @@ void selfReferenceChanged() { final Descriptor selfReferenceUnion = TestSelfReferenceProto.RecordTypeUnion.getDescriptor(); final Descriptor unspooledUnion = TestSelfReferenceUnspooledProto.RecordTypeUnion.getDescriptor(); validator.validateUnion(selfReferenceUnion, unspooledUnion); - assertInvalid("field removed", unspooledUnion, selfReferenceUnion); + + // Try the other way. Note that one of the fields in the unspooled LinkedListRecord is deprecated, so we need + // to allow undeprecation in order to catch the field removal error + assertFalse(validator.allowsUndeprecatingFields()); + assertInvalid("field is no longer deprecated", unspooledUnion, selfReferenceUnion); + final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() + .setAllowUndeprecatingFields(true) + .build(); + assertTrue(laxerValidator.allowsUndeprecatingFields()); + assertInvalid("field removed", laxerValidator, unspooledUnion, selfReferenceUnion); FileDescriptor updatedUnspooledFile = mutateMessageType("Node", TestSelfReferenceUnspooledProto.getDescriptor(), message -> message.removeField(0)); @@ -1021,24 +1235,22 @@ void nestedTypeChangesName() { validator.validate(metaData1, metaData2); } - @Test - void nestedTypeChangesFieldName() { - FileDescriptor updatedFile = mutateField("HeaderRecord", "num", TestRecordsWithHeaderProto.getDescriptor(), - field -> field.setName("numb")); - assertFalse(validator.allowsFieldRenames()); - assertInvalid("field renamed", TestRecordsWithHeaderProto.getDescriptor(), updatedFile); - final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() - .setAllowFieldRenames(true) - .build(); - assertTrue(laxerValidator.allowsFieldRenames()); - laxerValidator.validateUnion(TestRecordsWithHeaderProto.RecordTypeUnion.getDescriptor(), updatedFile.findMessageTypeByName(RecordMetaDataBuilder.DEFAULT_UNION_NAME)); + @ParameterizedTest + @MethodSource("deprecatedArgs") + void nestedTypeChangesFieldName(boolean deprecated) { + FileDescriptor updatedFile = mutateField("HeaderRecord", "num", TestRecordsWithHeaderProto.getDescriptor(), field -> { + if (deprecated) { + deprecateField(field); + } + field.setName("numb"); + }); + fieldRenameChecker.assertValidRenaming(deprecated, TestRecordsWithHeaderProto.getDescriptor(), updatedFile); RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecordsWithHeaderProto.getDescriptor()); metaDataBuilder.getRecordType("MyRecord").setPrimaryKey(Key.Expressions.field("header").nest(Key.Expressions.concatenateFields("path", "rec_no"))); RecordMetaData metaData1 = metaDataBuilder.getRecordMetaData(); RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, updatedFile); - assertInvalid("field renamed", metaData1, metaData2); - laxerValidator.validate(metaData1, metaData2); + fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData2); } @Test @@ -1053,24 +1265,23 @@ void nestedTypeChangesFieldType() { assertInvalid("field type changed", metaData1, metaData2); } - @Test - void nestedTypesMerged() { + @ParameterizedTest + @MethodSource("deprecatedArgs") + void nestedTypesMerged(boolean deprecated) { validator.validateUnion(TestUnmergedNestedTypesProto.RecordTypeUnion.getDescriptor(), TestMergedNestedTypesProto.RecordTypeUnion.getDescriptor()); - FileDescriptor updatedMergedFile = mutateField("OneTrueNested", "b", TestMergedNestedTypesProto.getDescriptor(), - field -> field.setName("c")); - assertFalse(validator.allowsFieldRenames()); - assertInvalid("field renamed", TestUnmergedNestedTypesProto.getDescriptor(), updatedMergedFile); - - final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() - .setAllowFieldRenames(true) - .build(); - assertTrue(laxerValidator.allowsFieldRenames()); - laxerValidator.validateUnion(TestUnmergedNestedTypesProto.RecordTypeUnion.getDescriptor(), updatedMergedFile.findMessageTypeByName(RecordMetaDataBuilder.DEFAULT_UNION_NAME)); + FileDescriptor updatedMergedFile = mutateField("OneTrueNested", "b", TestMergedNestedTypesProto.getDescriptor(), field -> { + if (deprecated) { + deprecateField(field); + } + field.setName("c"); + }); + fieldRenameChecker.assertValidRenaming(deprecated, TestUnmergedNestedTypesProto.getDescriptor(), updatedMergedFile); } - @Test - void nestedTypesMergedWithIndexesAndFieldRenames() { + @ParameterizedTest + @MethodSource("deprecatedArgs") + void nestedTypesMergedWithIndexesAndFieldRenames(boolean deprecated) { // Start with two fields in MyRecord, a and b, pointing to a NestedA and Nested B respectively // Then merge the types NestedA and NestedB together. In the merging, field 2 of NestedA is renamed // from a_prime to b, and field 2 of NestedB is renamed from b_prime to b. Validate that indexes @@ -1088,35 +1299,36 @@ void nestedTypesMergedWithIndexesAndFieldRenames() { final RecordMetaData metaData1 = metaDataBuilder.build(); FileDescriptor mergedFile = mutateMessageType("OneTrueNested", TestMergedNestedTypesProto.getDescriptor(), message -> { - addField(message) + final DescriptorProtos.FieldDescriptorProto.Builder aPrime = addField(message) .setName("a_prime") .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_INT32) .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL); - addField(message) + final DescriptorProtos.FieldDescriptorProto.Builder bPrime = addField(message) .setName("b_prime") .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_INT32) .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL); + if (deprecated) { + deprecateField(aPrime); + deprecateField(bPrime); + message.getFieldBuilderList().stream() + .filter(field -> field.getName().equals("a") || field.getName().equals("b")) + .forEach(MetaDataEvolutionValidatorTest::deprecateField); + } }); final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, mergedFile); - assertFalse(validator.allowsFieldRenames()); - assertInvalid("field renamed", metaData1, metaData2); // Even with field renames allowed, this should be rejected as the a.a_prime field has not been updated in the index - final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() - .setAllowFieldRenames(true) - .build(); - assertTrue(laxerValidator.allowsFieldRenames()); - assertInvalid("index key expression does not match required", laxerValidator, metaData1, metaData2); + fieldRenameChecker.assertInvalidRenaming("index key expression does not match required", deprecated, metaData1, metaData2); // Update the index so that it reflects the new field name for a.a_prime -> a.b final RecordMetaData metaData3 = replaceIndex(metaData2, "MyRecord$a.b+b.b", indexProto -> indexProto.toBuilder().setRootExpression(Key.Expressions.concat(Key.Expressions.field("a").nest("b"), Key.Expressions.field("b").nest("b")).toKeyExpression()).build()); - assertInvalid("field renamed", metaData1, metaData3); - laxerValidator.validate(metaData1, metaData3); + fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); } - @Test - void nestedTypesSplit() { + @ParameterizedTest + @MethodSource("deprecatedArgs") + void nestedTypesSplit(boolean deprecated) { validator.validateUnion(TestMergedNestedTypesProto.RecordTypeUnion.getDescriptor(), TestSplitNestedTypesProto.RecordTypeUnion.getDescriptor()); FileDescriptor fieldTypeChangedFile = mutateField("NestedB", "b", TestSplitNestedTypesProto.getDescriptor(), @@ -1124,22 +1336,24 @@ void nestedTypesSplit() { assertInvalid("field type changed", TestUnmergedNestedTypesProto.getDescriptor(), fieldTypeChangedFile); // Put different renames for different fields - FileDescriptor updatedSplitFile = mutateField("NestedA", "b", TestSplitNestedTypesProto.getDescriptor(), - field -> field.setName("b_1")); - updatedSplitFile = mutateField("NestedB", "b", updatedSplitFile, - field -> field.setName("b_2")); - assertFalse(validator.allowsFieldRenames()); - assertInvalid("field renamed", TestMergedNestedTypesProto.getDescriptor(), updatedSplitFile); - - final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() - .setAllowFieldRenames(true) - .build(); - assertTrue(laxerValidator.allowsFieldRenames()); - laxerValidator.validateUnion(TestMergedNestedTypesProto.RecordTypeUnion.getDescriptor(), updatedSplitFile.findMessageTypeByName(RecordMetaDataBuilder.DEFAULT_UNION_NAME)); + FileDescriptor updatedSplitFile = mutateField("NestedA", "b", TestSplitNestedTypesProto.getDescriptor(), field -> { + if (deprecated) { + deprecateField(field); + } + field.setName("b_1"); + }); + updatedSplitFile = mutateField("NestedB", "b", updatedSplitFile, field -> { + if (deprecated) { + deprecateField(field); + } + field.setName("b_2"); + }); + fieldRenameChecker.assertValidRenaming(deprecated, TestMergedNestedTypesProto.getDescriptor(), updatedSplitFile); } - @Test - void nestedTypesSplitWithIndex() { + @ParameterizedTest + @MethodSource("deprecatedArgs") + void nestedTypesSplitWithIndex(boolean deprecated) { // Start with two fields in MyRecord, a and b, both pointing to OneTrueNested with fields a and b // In the split file, a now points to a NestedA and b points to a NestedB // Rename the b field in NestedA to b_1 and the b field in NestedB to b_2 and validate that the indexes @@ -1152,6 +1366,9 @@ void nestedTypesSplitWithIndex() { FileDescriptor splitFile = mutateMessageType("NestedA", TestSplitNestedTypesProto.getDescriptor(), message -> { message.getFieldBuilderList().forEach(field -> { if (field.getName().equals("b")) { + if (deprecated) { + deprecateField(field); + } field.setName("b_1"); } }); @@ -1163,6 +1380,9 @@ void nestedTypesSplitWithIndex() { splitFile = mutateMessageType("NestedB", splitFile, message -> { message.getFieldBuilderList().forEach(field -> { if (field.getName().equals("b")) { + if (deprecated) { + deprecateField(field); + } field.setName("b_2"); } }); @@ -1172,21 +1392,14 @@ void nestedTypesSplitWithIndex() { .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL); }); final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, splitFile); - assertFalse(validator.allowsFieldRenames()); - assertInvalid("field renamed", metaData1, metaData2); - // Even with field renames allowed, this should be rejected as the a.b and b.b fields field have not been updated in the index - final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() - .setAllowFieldRenames(true) - .build(); - assertTrue(laxerValidator.allowsFieldRenames()); - assertInvalid("index key expression does not match required", laxerValidator, metaData1, metaData2); + // Even with field renames allowed, this should be rejected as the a.b and b.b fields have not been updated in the index + fieldRenameChecker.assertInvalidRenaming("index key expression does not match required", deprecated, metaData1, metaData2); // Update the index so that it reflects the new field name for a.b -> a.b_1 and b.b -> b.b_2 final RecordMetaData metaData3 = replaceIndex(metaData2, "MyRecord$a.b+b.b", indexProto -> indexProto.toBuilder().setRootExpression(Key.Expressions.concat(Key.Expressions.field("a").nest("b_1"), Key.Expressions.field("b").nest("b_2")).toKeyExpression()).build()); - assertInvalid("field renamed", metaData1, metaData3); - laxerValidator.validate(metaData1, metaData3); + fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); } @Test From 56d5383185e8ca93c95c639e911997d8d78c63da Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 28 Apr 2026 18:40:51 +0100 Subject: [PATCH 2/7] `MetaDataEvolutionValidator` now validates synthetic record type evolution The `MetaDataEvolutionValidator` has been udpated here to look at the synthetic types in the meta-data. This allows it to validate that the data held in join indexes and unnested indexes is not incorrectly updated during meta-data evolution. There are largely analogous paths followed for the two current synthetic types, `JoinedRecordType`s and `UnnestedRecordType`s. This resolves #4022. --- .../metadata/MetaDataEvolutionValidator.java | 255 +++++++++++++- .../MetaDataEvolutionValidatorTest.java | 313 ++++++++++++------ 2 files changed, 460 insertions(+), 108 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java index 4a58f7e526..9070fbba6b 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java @@ -40,6 +40,7 @@ import com.google.protobuf.Descriptors.FieldDescriptor; import javax.annotation.Nonnull; +import javax.annotation.Nullable; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -154,6 +155,7 @@ public void validate(@Nonnull RecordMetaData oldMetaData, @Nonnull RecordMetaDat validateUnion(oldMetaData.getUnionDescriptor(), newMetaData.getUnionDescriptor()); Map typeRenames = getTypeRenames(oldMetaData.getUnionDescriptor(), newMetaData.getUnionDescriptor()); validateRecordTypes(oldMetaData, newMetaData, typeRenames); + validateSyntheticTypes(oldMetaData, newMetaData, typeRenames); validateCurrentAndFormerIndexes(oldMetaData, newMetaData, typeRenames); } @@ -440,6 +442,255 @@ private void validateRecordTypes(@Nonnull RecordMetaData oldMetaData, @Nonnull R } } + private void validateSyntheticTypes(@Nonnull RecordMetaData oldMetaData, @Nonnull RecordMetaData newMetaData, + @Nonnull Map typeRenames) { + final Map> oldSyntheticTypes = getSyntheticTypesByRecordTypeKey(oldMetaData); + if (oldSyntheticTypes.isEmpty()) { + return; + } + final Map> newSyntheticTypes = getSyntheticTypesByRecordTypeKey(newMetaData); + for (Map.Entry> oldEntry : oldSyntheticTypes.entrySet()) { + Object recordTypeKey = oldEntry.getKey(); + final SyntheticRecordType oldSyntheticType = oldEntry.getValue(); + @Nullable SyntheticRecordType newSyntheticType = newSyntheticTypes.get(recordTypeKey); + if (newSyntheticType == null) { + // If the newSyntheticType is null, then it has been removed. This would mean that all the + // indexes that reference it have likewise been removed (or the meta-data validator would fail). + // So, we can ignore it + continue; + } + validateSyntheticType(oldSyntheticType, newSyntheticType, typeRenames); + } + } + + @Nonnull + private Map> getSyntheticTypesByRecordTypeKey(@Nonnull RecordMetaData metaData) { + final Map> byTypeKeyMap = new HashMap<>(); + for (SyntheticRecordType syntheticType : metaData.getSyntheticRecordTypes().values()) { + byTypeKeyMap.put(syntheticType.getRecordTypeKey(), syntheticType); + } + return byTypeKeyMap; + } + + private void validateSyntheticType(@Nonnull SyntheticRecordType oldType, @Nonnull SyntheticRecordType newType, @Nonnull Map typeRenames) { + if (disallowTypeRenames && !oldType.getName().equals(newType.getName())) { + throw new MetaDataException("synthetic record type name changed", + LogMessageKeys.OLD_RECORD_TYPE, oldType.getName(), + LogMessageKeys.NEW_RECORD_TYPE, newType.getName()); + } + if (!(oldType.getClass().equals(newType.getClass()))) { + throw new MetaDataException("synthetic record type changed type", + LogMessageKeys.EXPECTED, oldType.getName(), + LogMessageKeys.ACTUAL, newType.getClass().getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + if (oldType instanceof JoinedRecordType) { + validateJoinedRecordType((JoinedRecordType) oldType, (JoinedRecordType) newType, typeRenames); + } else if (oldType instanceof UnnestedRecordType) { + validateUnnestedRecordType((UnnestedRecordType) oldType, (UnnestedRecordType) newType, typeRenames); + } else { + throw new MetaDataException("unknown synthetic record type", + LogMessageKeys.OLD_RECORD_TYPE, oldType.getName(), + LogMessageKeys.NEW_RECORD_TYPE, newType.getName(), + LogMessageKeys.RECORD_TYPE, oldType.getClass().getName()); + } + } + + private void validateJoinedRecordType(@Nonnull JoinedRecordType oldType, @Nonnull JoinedRecordType newType, @Nonnull Map typeRenames) { + // Validate constituents + final List oldConstituents = oldType.getConstituents(); + final List newConstituents = newType.getConstituents(); + if (oldConstituents.size() != newConstituents.size()) { + throw new MetaDataException("join constituent count changed", + LogMessageKeys.EXPECTED, oldConstituents.size(), + LogMessageKeys.ACTUAL, newConstituents.size(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + + for (int i = 0; i < oldConstituents.size(); i++) { + // We use the definition order of join constituents here to find a correspondence between + // old and new types. This is important because the position in the definition is how the + // logic within the code identifies different constituents (e.g., the primary key component + // at position i + 1 is the nested primary key of the i'th constituent). + JoinedRecordType.JoinConstituent oldConstituent = oldConstituents.get(i); + JoinedRecordType.JoinConstituent newConstituent = newConstituents.get(i); + + if (!allowFieldRenames && !oldConstituent.getName().equals(newConstituent.getName())) { + // The join constituent names are used in things like index field declarations, so changing + // them is analogous to a field change. Unlike regular field names, the name shouldn't appear + // in queries (only internal data structures like index definitions), so it may be okay to + // loosen this. + throw new MetaDataException("join constituent name changed", + LogMessageKeys.EXPECTED, oldConstituent.getName(), + LogMessageKeys.ACTUAL, newConstituent.getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + final String expectedTypeName = typeRenames.getOrDefault(oldConstituent.getRecordType().getName(), oldConstituent.getRecordType().getName()); + if (!expectedTypeName.equals(newConstituent.getRecordType().getName())) { + throw new MetaDataException("join constituent type changed", + LogMessageKeys.OLD_RECORD_TYPE, oldConstituent.getName(), + LogMessageKeys.NEW_RECORD_TYPE, newConstituent.getName(), + LogMessageKeys.NAME, newConstituent.getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + if (oldConstituent.isOuterJoined() != newConstituent.isOuterJoined()) { + throw new MetaDataException("join constituent outer-joined property changed", + LogMessageKeys.EXPECTED, oldConstituent.isOuterJoined(), + LogMessageKeys.ACTUAL, newConstituent.isOuterJoined(), + LogMessageKeys.NAME, newConstituent.getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + } + + // Validate joins + final List oldJoins = oldType.getJoins(); + final List newJoins = newType.getJoins(); + if (oldJoins.size() != newJoins.size()) { + throw new MetaDataException("join type join count changed", + LogMessageKeys.EXPECTED, oldConstituents.size(), + LogMessageKeys.ACTUAL, newConstituents.size(), + LogMessageKeys.RECORD_TYPE, newType.getClass().getName()); + } + final Map oldNameToConstituentPosMap = constituentNameToPositionMap(oldConstituents); + final Map newNameToConstituentPosMap = constituentNameToPositionMap(newConstituents); + for (int i = 0; i < oldJoins.size(); i++) { + // Technically, these aren't ordered, but we'd otherwise need to enumerate the different joins by constituent + // pair and then search for ones that match. It is simpler here to just require that the joins be in the + // same order + JoinedRecordType.Join oldJoin = oldJoins.get(i); + JoinedRecordType.Join newJoin = newJoins.get(i); + + // Validate the left and right constituents are the same + int oldLeftPos = oldNameToConstituentPosMap.get(oldJoin.getLeft().getName()); + int newLeftPos = newNameToConstituentPosMap.get(newJoin.getLeft().getName()); + if (oldLeftPos != newLeftPos) { + throw new MetaDataException("join changed left constituent", + LogMessageKeys.EXPECTED, newConstituents.get(oldLeftPos).getName(), + LogMessageKeys.ACTUAL, newConstituents.get(newLeftPos).getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + final KeyExpression expectedLeftExpression; + if (allowsAnyFieldRenames()) { + Descriptors.Descriptor oldDescriptor = oldConstituents.get(oldLeftPos).getRecordType().getDescriptor(); + Descriptors.Descriptor newDescriptor = newConstituents.get(newLeftPos).getRecordType().getDescriptor(); + expectedLeftExpression = RenameFieldsVisitor.renameFields(oldJoin.getLeftExpression(), oldDescriptor, newDescriptor); + } else { + expectedLeftExpression = oldJoin.getLeftExpression(); + } + if (!expectedLeftExpression.equals(newJoin.getLeftExpression())) { + throw new MetaDataException("join changed left expression", + LogMessageKeys.EXPECTED, expectedLeftExpression, + LogMessageKeys.ACTUAL, newJoin.getLeftExpression(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + + int oldRightPos = oldNameToConstituentPosMap.get(oldJoin.getRight().getName()); + int newRightPos = newNameToConstituentPosMap.get(newJoin.getRight().getName()); + if (oldRightPos != newRightPos) { + throw new MetaDataException("join changed right constituent", + LogMessageKeys.EXPECTED, newConstituents.get(oldRightPos).getName(), + LogMessageKeys.ACTUAL, newConstituents.get(newRightPos).getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + final KeyExpression expectedRightExpression; + if (allowsAnyFieldRenames()) { + Descriptors.Descriptor oldDescriptor = oldConstituents.get(oldRightPos).getRecordType().getDescriptor(); + Descriptors.Descriptor newDescriptor = newConstituents.get(newRightPos).getRecordType().getDescriptor(); + expectedRightExpression = RenameFieldsVisitor.renameFields(oldJoin.getRightExpression(), oldDescriptor, newDescriptor); + } else { + expectedRightExpression = oldJoin.getLeftExpression(); + } + if (!expectedRightExpression.equals(newJoin.getRightExpression())) { + throw new MetaDataException("join changed right expression", + LogMessageKeys.EXPECTED, expectedRightExpression, + LogMessageKeys.ACTUAL, newJoin.getRightExpression(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + } + } + + private void validateUnnestedRecordType(@Nonnull UnnestedRecordType oldType, @Nonnull UnnestedRecordType newType, @Nonnull Map typeRenames) { + // Validate constituents + final List oldConstituents = oldType.getConstituents(); + final List newConstituents = newType.getConstituents(); + if (oldConstituents.size() != newConstituents.size()) { + throw new MetaDataException("unnested type constituent count changed", + LogMessageKeys.EXPECTED, oldConstituents.size(), + LogMessageKeys.ACTUAL, newConstituents.size(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + + // We will need this to validate that the parent constituents refer to the same entity, even in the presence of renames + final Map oldNameToConstituentPosMap = constituentNameToPositionMap(oldConstituents); + final Map newNameToConstituentPosMap = constituentNameToPositionMap(newConstituents); + + for (int i = 0; i < oldConstituents.size(); i++) { + UnnestedRecordType.NestedConstituent oldConstituent = oldConstituents.get(i); + UnnestedRecordType.NestedConstituent newConstituent = newConstituents.get(i); + + if (!allowFieldRenames && !oldConstituent.getName().equals(newConstituent.getName())) { + throw new MetaDataException("nested constituent name changed", + LogMessageKeys.EXPECTED, oldConstituent.getName(), + LogMessageKeys.ACTUAL, newConstituent.getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + + if (oldConstituent.isParent() != newConstituent.isParent()) { + throw new MetaDataException("nested constituent parent property changed", + LogMessageKeys.EXPECTED, oldConstituent.isParent(), + LogMessageKeys.ACTUAL, newConstituent.isParent(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + + if (oldConstituent.isParent()) { + // Make sure the overall parent has the same record type (subject to renames) + final String oldRecordType = oldConstituent.getRecordType().getName(); + final String expectedTypeName = typeRenames.getOrDefault(oldRecordType, oldRecordType); + if (!expectedTypeName.equals(newConstituent.getRecordType().getName())) { + throw new MetaDataException("unnested type parent record type changed", + LogMessageKeys.EXPECTED, expectedTypeName, + LogMessageKeys.ACTUAL, newConstituent.getRecordType().getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + } else { + // Make sure we are pointing to the same parent + int oldParentPos = oldNameToConstituentPosMap.get(oldConstituent.getName()); + int newParentPos = newNameToConstituentPosMap.get(newConstituent.getName()); + if (oldParentPos != newParentPos) { + throw new MetaDataException("nested constituent changed parent", + LogMessageKeys.EXPECTED, newConstituents.get(oldParentPos).getName(), + LogMessageKeys.ACTUAL, newConstituent.getParentName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + + // Make sure the nesting expression is still the same, possibly accounting for field renames + final KeyExpression expectedNestingExpression; + if (allowsAnyFieldRenames()) { + UnnestedRecordType.NestedConstituent oldParent = Objects.requireNonNull(oldConstituent.getParent()); + UnnestedRecordType.NestedConstituent newParent = Objects.requireNonNull(newConstituent.getParent()); + expectedNestingExpression = RenameFieldsVisitor.renameFields(oldConstituent.getNestingExpression(), oldParent.getRecordType().getDescriptor(), newParent.getRecordType().getDescriptor()); + } else { + expectedNestingExpression = oldConstituent.getNestingExpression(); + } + if (!expectedNestingExpression.equals(newConstituent.getNestingExpression())) { + throw new MetaDataException("nested constituent nesting expression changed", + LogMessageKeys.EXPECTED, expectedNestingExpression, + LogMessageKeys.ACTUAL, newConstituent.getNestingExpression(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + } + } + } + + @Nonnull + private Map constituentNameToPositionMap(@Nonnull List constituents) { + final Map nameToPositionMap = new HashMap<>(); + for (int i = 0; i < constituents.size(); i++) { + nameToPositionMap.put(constituents.get(i).getName(), i); + } + return nameToPositionMap; + } + @Nonnull private Map getFormerIndexMap(@Nonnull RecordMetaData metaData) { Map formerIndexMap; @@ -690,8 +941,8 @@ private void validateIndex(@Nonnull RecordMetaData oldMetaData, @Nonnull Index o KeyExpression expectedKeyExpression = null; if (allowsAnyFieldRenames()) { for (String oldRecordTypeName : oldRecordTypeNames) { - final Descriptor oldDescriptor = oldMetaData.getRecordType(oldRecordTypeName).getDescriptor(); - final Descriptor newDescriptor = newMetaData.getRecordType(typeRenames.getOrDefault(oldRecordTypeName, oldRecordTypeName)).getDescriptor(); + final Descriptor oldDescriptor = oldMetaData.getIndexableRecordType(oldRecordTypeName).getDescriptor(); + final Descriptor newDescriptor = newMetaData.getIndexableRecordType(typeRenames.getOrDefault(oldRecordTypeName, oldRecordTypeName)).getDescriptor(); final KeyExpression renamedKeyExpression = RenameFieldsVisitor.renameFields(oldIndex.getRootExpression(), oldDescriptor, newDescriptor); if (expectedKeyExpression == null) { expectedKeyExpression = renamedKeyExpression; 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 9fc71b4532..67426202a9 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 @@ -66,7 +66,6 @@ import java.util.Locale; import java.util.Set; import java.util.function.Consumer; -import java.util.function.UnaryOperator; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -1029,8 +1028,8 @@ void renameFieldInUniversalIndex(boolean deprecated) { // Still not allowed as the index hasn't been updated fieldRenameChecker.assertInvalidRenaming("index key expression does not match required", deprecated, metaData1, metaData3); - RecordMetaData metaData4 = replaceIndex(metaData3, "all$num_value_2", - indexProto -> indexProto.toBuilder().setRootExpression(Key.Expressions.field("num_value_2__old").toKeyExpression()).build()); + RecordMetaData metaData4 = mutateIndex(metaData3, "all$num_value_2", + indexProto -> indexProto.setRootExpression(Key.Expressions.field("num_value_2__old").toKeyExpression())); fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData4); } @@ -1321,8 +1320,8 @@ void nestedTypesMergedWithIndexesAndFieldRenames(boolean deprecated) { fieldRenameChecker.assertInvalidRenaming("index key expression does not match required", deprecated, metaData1, metaData2); // Update the index so that it reflects the new field name for a.a_prime -> a.b - final RecordMetaData metaData3 = replaceIndex(metaData2, "MyRecord$a.b+b.b", indexProto -> - indexProto.toBuilder().setRootExpression(Key.Expressions.concat(Key.Expressions.field("a").nest("b"), Key.Expressions.field("b").nest("b")).toKeyExpression()).build()); + final RecordMetaData metaData3 = mutateIndex(metaData2, "MyRecord$a.b+b.b", indexProto -> + indexProto.setRootExpression(Key.Expressions.concat(Key.Expressions.field("a").nest("b"), Key.Expressions.field("b").nest("b")).toKeyExpression())); fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); } @@ -1397,8 +1396,8 @@ void nestedTypesSplitWithIndex(boolean deprecated) { fieldRenameChecker.assertInvalidRenaming("index key expression does not match required", deprecated, metaData1, metaData2); // Update the index so that it reflects the new field name for a.b -> a.b_1 and b.b -> b.b_2 - final RecordMetaData metaData3 = replaceIndex(metaData2, "MyRecord$a.b+b.b", indexProto -> - indexProto.toBuilder().setRootExpression(Key.Expressions.concat(Key.Expressions.field("a").nest("b_1"), Key.Expressions.field("b").nest("b_2")).toKeyExpression()).build()); + final RecordMetaData metaData3 = mutateIndex(metaData2, "MyRecord$a.b+b.b", indexProto -> + indexProto.setRootExpression(Key.Expressions.concat(Key.Expressions.field("a").nest("b_1"), Key.Expressions.field("b").nest("b_2")).toKeyExpression())); fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); } @@ -1586,6 +1585,102 @@ void primaryKeyChanged() { assertInvalid("record type primary key changed", metaData1, metaData2); } + // Synthetic type tests + + @Test + void addJoinedType() { + final RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); + + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + metaDataBuilder.setVersion(metaData1.getVersion() + 1); + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); + joinBuilder.addConstituent("l", "MySimpleRecord"); + joinBuilder.addConstituent("r", "MyOtherRecord"); + joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); + metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); + RecordMetaData metaData2 = metaDataBuilder.build(); + + validator.validate(metaData1, metaData2); + } + + @Test + void dropJoinedType() { + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); + joinBuilder.addConstituent("l", "MySimpleRecord"); + joinBuilder.addConstituent("r", "MyOtherRecord"); + joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); + metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); + final RecordMetaData metaData1 = metaDataBuilder.build(); + + metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + metaDataBuilder.setVersion(metaData1.getVersion() + 1); + metaDataBuilder.addFormerIndex(new FormerIndex("joined$l.num_value_unique", metaData1.getVersion(), metaData1.getVersion() + 1, "joined$l.num_value_unique")); + RecordMetaData metaData2 = metaDataBuilder.build(); + + validator.validate(metaData1, metaData2); + } + + @Test + void swapJoinConstituents() { + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); + joinBuilder.addConstituent("l", "MySimpleRecord"); + joinBuilder.addConstituent("r", "MyOtherRecord"); + joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); + metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); + + final RecordMetaData metaData1 = metaDataBuilder.build(); + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", joinedType -> { + final var constituentsList = joinedType.getJoinConstituentsList(); + joinedType.clearJoinConstituents(); + joinedType.addJoinConstituents(constituentsList.get(1)); + joinedType.addJoinConstituents(constituentsList.get(0)); + }); + + assertInvalid("join constituent name changed", metaData1, metaData2); + final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() + .setAllowFieldRenames(true) + .build(); + assertInvalid("join constituent type changed", laxerValidator, metaData1, metaData2); + } + + @Test + void swapSelfJoinConstituents() { + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("self_join_svi"); + joinBuilder.addConstituent("s1", "MySimpleRecord"); + joinBuilder.addConstituent("s2", "MySimpleRecord"); + joinBuilder.addJoin("s1", Key.Expressions.field("str_value_indexed"), "s2", Key.Expressions.field("str_value_indexed")); + metaDataBuilder.addIndex("self_join_svi", "self_join_svi$num_value_unique", Key.Expressions.concat(Key.Expressions.field("s1").nest("num_value_unique"), Key.Expressions.field("s2").nest("num_value_unique"))); + + final RecordMetaData metaData1 = metaDataBuilder.build(); + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "self_join_svi", joinedType -> { + final var constituentsList = joinedType.getJoinConstituentsList(); + joinedType.clearJoinConstituents(); + joinedType.addJoinConstituents(constituentsList.get(1)); + joinedType.addJoinConstituents(constituentsList.get(0)); + }); + + assertInvalid("join constituent name changed", metaData1, metaData2); + final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() + .setAllowFieldRenames(true) + .build(); + assertInvalid("join changed left constituent", laxerValidator, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateJoinedRecordType(metaData2, "self_join_svi", + // Swap the names in the Join definition + joinedType -> joinedType.getJoinsBuilder(0).setLeft("s2").setRight("s1")); + assertInvalid("join constituent name changed", metaData1, metaData3); + assertInvalid("index key expression does not match required", laxerValidator, metaData1, metaData3); + + final RecordMetaData metaData4 = mutateIndex(metaData3, "self_join_svi$num_value_unique", + // Swap the names in the index definition + index -> index.setRootExpression(Key.Expressions.concat(Key.Expressions.field("s2").nest("num_value_unique"), Key.Expressions.field("s1").nest("num_value_unique")).toKeyExpression())); + assertInvalid("join constituent name changed", metaData1, metaData4); + laxerValidator.validate(metaData1, metaData4); + } + // Former index tests @Test @@ -1790,12 +1885,11 @@ void removeIndexAndChangeLastModifiedVersion() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); // Step 1: Update the index definition in a way that updates the last modified version - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", index -> - // Mark the index as unique (and bump its last modified version - index.toBuilder() - .addOptions(RecordMetaDataProto.Index.Option.newBuilder().setKey("unique").setValue("true")) - .setLastModifiedVersion(index.getLastModifiedVersion() + 1) - .build()); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", index -> { + // Mark the index as unique (and bump its last modified version + makeUnique(index); + index.setLastModifiedVersion(index.getLastModifiedVersion() + 1); + }); assertFalse(validator.allowsIndexRebuilds()); assertInvalid("last modified version of index changed", metaData1, metaData2); @@ -1843,52 +1937,65 @@ void defaultIndexRemovalPath() { // Index tests - @Nonnull - private RecordMetaDataProto.Index changeOption(@Nonnull RecordMetaDataProto.Index indexProto, @Nonnull String key, @Nullable String value) { - RecordMetaDataProto.Index.Builder builder = indexProto.toBuilder(); + private void changeOption(@Nonnull RecordMetaDataProto.Index.Builder indexProto, @Nonnull String key, @Nullable String value) { boolean found = false; - for (int i = 0; i < builder.getOptionsCount(); i++) { - final RecordMetaDataProto.Index.Option option = builder.getOptions(i); + int i = 0; + for (RecordMetaDataProto.Index.Option.Builder option : indexProto.getOptionsBuilderList()) { if (key.equals(option.getKey())) { - if (value == null) { - builder.removeOptions(i); - } else { - builder.setOptions(i, RecordMetaDataProto.Index.Option.newBuilder().setKey(key).setValue(value)); + if (value != null) { + option.setValue(value); } found = true; break; } + i++; } - if (!found && value != null) { - builder.addOptions(RecordMetaDataProto.Index.Option.newBuilder().setKey(key).setValue(value)); + if (found && value == null) { + indexProto.removeOptions(i); + } else if (!found && value != null) { + indexProto.addOptions(RecordMetaDataProto.Index.Option.newBuilder().setKey(key).setValue(value)); } - return builder.build(); } - @Nonnull - private RecordMetaDataProto.Index makeUnique(@Nonnull RecordMetaDataProto.Index indexProto) { - return changeOption(indexProto, IndexOptions.UNIQUE_OPTION, "true"); + private void makeUnique(@Nonnull RecordMetaDataProto.Index.Builder indexProto) { + changeOption(indexProto, IndexOptions.UNIQUE_OPTION, "true"); } @Nonnull - private RecordMetaDataProto.Index clearOptions(@Nonnull RecordMetaDataProto.Index indexProto) { - return indexProto.toBuilder().clearOptions().build(); + private void clearOptions(@Nonnull RecordMetaDataProto.Index.Builder indexProto) { + indexProto.clearOptions(); } @Nonnull - private RecordMetaData replaceIndex(@Nonnull RecordMetaData metaData, @Nonnull String indexName, UnaryOperator indexReplacement) { - RecordMetaDataProto.MetaData metaDataProto = metaData.toProto(); - RecordMetaDataProto.MetaData.Builder metaDataProtoBuilder = metaDataProto.toBuilder(); + private RecordMetaData updateMetaData(@Nonnull RecordMetaData metaData, @Nonnull Consumer updater) { + RecordMetaDataProto.MetaData.Builder metaDataProtoBuilder = metaData.toProto().toBuilder(); + updater.accept(metaDataProtoBuilder); metaDataProtoBuilder.setVersion(metaData.getVersion() + 1); - for (int i = 0; i < metaDataProto.getIndexesCount(); i ++) { - RecordMetaDataProto.Index indexProto = metaDataProto.getIndexes(i); - if (indexProto.getName().equals(indexName)) { - metaDataProtoBuilder.setIndexes(i, indexReplacement.apply(indexProto)); - } - } return RecordMetaData.build(metaDataProtoBuilder.build()); } + @Nonnull + private RecordMetaData mutateIndex(@Nonnull RecordMetaData metaData, @Nonnull String indexName, @Nonnull Consumer indexMutator) { + return updateMetaData(metaData, metaDataProtoBuilder -> { + for (RecordMetaDataProto.Index.Builder indexProto : metaDataProtoBuilder.getIndexesBuilderList()) { + if (indexProto.getName().equals(indexName)) { + indexMutator.accept(indexProto); + } + } + }); + } + + @Nonnull + private RecordMetaData mutateJoinedRecordType(@Nonnull RecordMetaData metaData, @Nonnull String typeName, @Nonnull Consumer typeMutator) { + return updateMetaData(metaData, metaDataProtoBuilder -> { + for (RecordMetaDataProto.JoinedRecordType.Builder typeBuilder : metaDataProtoBuilder.getJoinedRecordTypesBuilderList()) { + if (typeBuilder.getName().equals(typeName)) { + typeMutator.accept(typeBuilder); + } + } + }); + } + @Test void silentlyRemoveIndex() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); @@ -1923,8 +2030,8 @@ void indexSubspaceKeyChanged() { // The index subspace key is the thing that determines whether an index is even there, so changing it // is identical to removing the index RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setSubspaceKey(ByteString.copyFrom(Tuple.from("dummy_key").pack())).build() + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> + indexProto.setSubspaceKey(ByteString.copyFrom(Tuple.from("dummy_key").pack())) ); assertInvalid("index missing in new meta-data", metaData1, metaData2); } @@ -1932,11 +2039,10 @@ void indexSubspaceKeyChanged() { @Test void indexNameChanged() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder() + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> + indexProto .setSubspaceKey(ByteString.copyFrom(Tuple.from("MySimpleRecord$str_value_indexed").pack())) .setName("a_different_name") - .build() ); assertInvalid("index name changed", metaData1, metaData2); } @@ -1944,34 +2050,29 @@ void indexNameChanged() { @Test void indexAddedVersionChanged() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setAddedVersion(metaData1.getVersion() + 1).setLastModifiedVersion(metaData1.getVersion() + 1).build() - ); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setAddedVersion(metaData1.getVersion() + 1).setLastModifiedVersion(metaData1.getVersion() + 1)); assertInvalid("new index added version does not match old index added version", metaData1, metaData2); - metaData2 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setAddedVersion(indexProto.getAddedVersion() - 1).build() - ); + metaData2 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setAddedVersion(indexProto.getAddedVersion() - 1)); assertInvalid("new index added version does not match old index added version", metaData1, metaData2); } @Test void indexLastModifiedVersionTooOld() { - RecordMetaData metaData1 = replaceIndex(RecordMetaData.build(TestRecords1Proto.getDescriptor()), "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setLastModifiedVersion(2).build() - ); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setLastModifiedVersion(1).build() - ); + RecordMetaData metaData1 = mutateIndex(RecordMetaData.build(TestRecords1Proto.getDescriptor()), "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setLastModifiedVersion(2)); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setLastModifiedVersion(1)); assertInvalid("old index has last-modified version newer than new index", metaData1, metaData2); } @Test void indexLastModifiedVersionChanged() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setLastModifiedVersion(metaData1.getVersion() + 1).build() - ); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setLastModifiedVersion(metaData1.getVersion() + 1)); assertFalse(validator.allowsIndexRebuilds()); assertInvalid("last modified version of index changed", metaData1, metaData2); @@ -1982,18 +2083,17 @@ void indexLastModifiedVersionChanged() { laxerValidator.validate(metaData1, metaData2); } - private void validateIndexMutation(@Nonnull String errMsg, @Nonnull RecordMetaData metaData1, @Nonnull String indexName, UnaryOperator indexReplacement) { + private void validateIndexMutation(@Nonnull String errMsg, @Nonnull RecordMetaData metaData1, @Nonnull String indexName, Consumer indexReplacement) { MetaDataEvolutionValidator laxerValidator = MetaDataEvolutionValidator.newBuilder() .setAllowIndexRebuilds(true) .build(); - RecordMetaData metaData2 = replaceIndex(metaData1, indexName, indexReplacement); + RecordMetaData metaData2 = mutateIndex(metaData1, indexName, indexReplacement); assertInvalid(errMsg, metaData1, metaData2); assertInvalid(errMsg, laxerValidator, metaData1, metaData2); // Allow the change if and only if the last modified version is updated and the option allowing rebuilds is set - RecordMetaData metaData3 = replaceIndex(metaData2, indexName, indexProto -> - indexProto.toBuilder().setLastModifiedVersion(metaData2.getVersion()).build() - ); + RecordMetaData metaData3 = mutateIndex(metaData2, indexName, + indexProto -> indexProto.setLastModifiedVersion(metaData2.getVersion())); assertInvalid("last modified version of index changed", metaData1, metaData3); laxerValidator.validate(metaData1, metaData3); } @@ -2001,17 +2101,16 @@ private void validateIndexMutation(@Nonnull String errMsg, @Nonnull RecordMetaDa @Test void indexTypeChanged() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - validateIndexMutation("index type changed", metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setType(IndexTypes.RANK).build() + validateIndexMutation("index type changed", metaData1, "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setType(IndexTypes.RANK) ); } @Test void indexKeyExpressionChanged() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - validateIndexMutation("index key expression changed", metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setRootExpression(Key.Expressions.field("num_value_2").toKeyExpression()).build() - ); + validateIndexMutation("index key expression changed", metaData1, "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setRootExpression(Key.Expressions.field("num_value_2").toKeyExpression())); } @Test @@ -2021,8 +2120,8 @@ void indexRecordTypeRemoved() { metaDataBuilder.addMultiTypeIndex(Arrays.asList(metaDataBuilder.getRecordType("MySimpleRecord"), metaDataBuilder.getRecordType("MyOtherRecord")), new Index(indexName, "num_value_2")); RecordMetaData metaData1 = metaDataBuilder.getRecordMetaData(); - validateIndexMutation("new index removes record type", metaData1, indexName, indexProto -> - indexProto.toBuilder().clearRecordType().addRecordType("MySimpleRecord").build() + validateIndexMutation("new index removes record type", metaData1, indexName, + indexProto -> indexProto.clearRecordType().addRecordType("MySimpleRecord") ); } @@ -2030,14 +2129,14 @@ void indexRecordTypeRemoved() { void indexRecordTypeAdded() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); validateIndexMutation("new index adds record type that is not newer than old meta-data", metaData1, "MySimpleRecord$num_value_3_indexed", indexProto -> - indexProto.toBuilder().addRecordType("MyOtherRecord").build() + indexProto.addRecordType("MyOtherRecord").build() ); // Add NewRecord as a record type to the existing meta-data and index and validate that this change is okay // because the new record type is newer the old meta-data version. RecordMetaData tempMetaData = addNewRecordType(metaData1); - RecordMetaData metaData2 = replaceIndex(tempMetaData, "MySimpleRecord$num_value_3_indexed", indexProto -> - indexProto.toBuilder().addRecordType("NewRecord").build()); + RecordMetaData metaData2 = mutateIndex(tempMetaData, "MySimpleRecord$num_value_3_indexed", + indexProto -> indexProto.addRecordType("NewRecord")); validator.validate(metaData1, metaData2); // valid if type and index change happen together assertInvalid("new index adds record type that is not newer than old meta-data", tempMetaData, metaData2); } @@ -2050,8 +2149,8 @@ void indexPrimaryKeyComponentsChanged() { assertThat(metaData1.getIndex("rec_no").hasPrimaryKeyComponentPositions(), is(true)); RecordMetaData tempMetaData = addNewRecordType(metaData1); - RecordMetaData metaData2 = replaceIndex(tempMetaData, "rec_no", indexProto -> - indexProto.toBuilder().addRecordType("NewRecord").build()); + RecordMetaData metaData2 = mutateIndex(tempMetaData, "rec_no", + indexProto -> indexProto.addRecordType("NewRecord")); assertInvalid("new index drops primary key component positions", metaData1, metaData2); // This is essentially the behavior change outlined by: https://github.com/FoundationDB/fdb-record-layer/issues/93 @@ -2079,8 +2178,8 @@ void addRecordTypeWithUniversalIndex() { validator.validate(metaData1, metaData2); // Make the index a multi-type index on the original record types - RecordMetaData metaData3 = replaceIndex(metaData2, "rec_no", indexProto -> - indexProto.toBuilder().addRecordType("MySimpleRecord").addRecordType("MyOtherRecord").build()); + RecordMetaData metaData3 = mutateIndex(metaData2, "rec_no", + indexProto -> indexProto.addRecordType("MySimpleRecord").addRecordType("MyOtherRecord")); MetaDataException e = assertThrows(MetaDataException.class, () -> metaData3.getUniversalIndex("rec_no")); assertThat(e.getMessage(), containsString("Index rec_no not defined")); assertInvalid("new index removes record type", metaData2, metaData3); @@ -2094,10 +2193,10 @@ void uniquenessConstraintChanged() { validateIndexMutation("index adds uniqueness constraint", metaData1, "MySimpleRecord$str_value_indexed", this::makeUnique); // Removing the uniqueness constraint is fine - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", this::makeUnique); - RecordMetaData metaData3 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", this::clearOptions); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", this::makeUnique); + RecordMetaData metaData3 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", this::clearOptions); validator.validate(metaData2, metaData3); - RecordMetaData metaData4 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, IndexOptions.UNIQUE_OPTION, "false")); + RecordMetaData metaData4 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, IndexOptions.UNIQUE_OPTION, "false")); validator.validate(metaData2, metaData4); } @@ -2105,25 +2204,25 @@ void uniquenessConstraintChanged() { void allowedForQueriesChanged() { // Changing this option is always fine RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, IndexOptions.ALLOWED_FOR_QUERY_OPTION, "false")); validator.validate(metaData1, metaData2); - RecordMetaData metaData3 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", + RecordMetaData metaData3 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, IndexOptions.ALLOWED_FOR_QUERY_OPTION, "true")); validator.validate(metaData2, metaData3); - RecordMetaData metaData4 = replaceIndex(metaData3, "MySimpleRecord$str_value_indexed", this::clearOptions); + RecordMetaData metaData4 = mutateIndex(metaData3, "MySimpleRecord$str_value_indexed", this::clearOptions); validator.validate(metaData3, metaData4); } @Test void changeReplacedByIndex() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, IndexOptions.REPLACED_BY_OPTION_PREFIX, "MySimpleRecord$num_value_3_indexed")); assertEquals(Collections.singletonList("MySimpleRecord$num_value_3_indexed"), metaData2.getIndex("MySimpleRecord$str_value_indexed").getReplacedByIndexNames()); validator.validate(metaData1, metaData2); - RecordMetaData metaData3 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", this::clearOptions); + RecordMetaData metaData3 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", this::clearOptions); assertEquals(Collections.emptyList(), metaData3.getIndex("MySimpleRecord$str_value_indexed").getReplacedByIndexNames()); validator.validate(metaData2, metaData3); @@ -2132,14 +2231,14 @@ void changeReplacedByIndex() { @Test void changeReplacedByIndexSet() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - changeOption(changeOption(indexProto, IndexOptions.REPLACED_BY_OPTION_PREFIX + "_0", "MySimpleRecord$num_value_3_indexed"), - IndexOptions.REPLACED_BY_OPTION_PREFIX + "_1", "MySimpleRecord$num_value_unique") - ); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> { + changeOption(indexProto, IndexOptions.REPLACED_BY_OPTION_PREFIX + "_0", "MySimpleRecord$num_value_3_indexed"); + changeOption(indexProto, IndexOptions.REPLACED_BY_OPTION_PREFIX + "_1", "MySimpleRecord$num_value_unique"); + }); assertThat(metaData2.getIndex("MySimpleRecord$str_value_indexed").getReplacedByIndexNames(), containsInAnyOrder("MySimpleRecord$num_value_3_indexed", "MySimpleRecord$num_value_unique")); validator.validate(metaData1, metaData2); - RecordMetaData metaData3 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", this::clearOptions); + RecordMetaData metaData3 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", this::clearOptions); assertEquals(Collections.emptyList(), metaData3.getIndex("MySimpleRecord$str_value_indexed").getReplacedByIndexNames()); validator.validate(metaData2, metaData3); @@ -2150,9 +2249,11 @@ void unknownOptionChanged() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); validateIndexMutation("index option changed", metaData1, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, "dummyOption", "dummyValue")); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", - indexProto -> makeUnique(changeOption(indexProto, "dummyOption", "dummyValue"))); - RecordMetaData metaData3 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> { + makeUnique(indexProto); + changeOption(indexProto, "dummyOption", "dummyValue"); + }); + RecordMetaData metaData3 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, IndexOptions.UNIQUE_OPTION, null)); validator.validate(metaData2, metaData3); validateIndexMutation("index option changed", metaData3, "MySimpleRecord$str_value_indexed", @@ -2172,10 +2273,10 @@ void rankLevelsChanged() { indexProto -> changeOption(indexProto, IndexOptions.RANK_NLEVELS, "" + RankedSet.MAX_LEVELS)); // Setting the default explicitly is fine - RecordMetaData metaData2 = replaceIndex(metaData1, indexName, + RecordMetaData metaData2 = mutateIndex(metaData1, indexName, indexProto -> changeOption(indexProto, IndexOptions.RANK_NLEVELS, "" + RankedSet.DEFAULT_LEVELS)); validator.validate(metaData1, metaData2); - RecordMetaData metaData3 = replaceIndex(metaData2, indexName, this::clearOptions); + RecordMetaData metaData3 = mutateIndex(metaData2, indexName, this::clearOptions); validator.validate(metaData2, metaData3); } @@ -2190,37 +2291,37 @@ void textOptionsChanged() { indexProto -> changeOption(indexProto, IndexOptions.TEXT_TOKENIZER_NAME_OPTION, AllSuffixesTextTokenizer.NAME)); // Setting the default explicitly is fine - RecordMetaData metaData2 = replaceIndex(metaData1, indexName, + RecordMetaData metaData2 = mutateIndex(metaData1, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_TOKENIZER_NAME_OPTION, DefaultTextTokenizer.NAME)); validator.validate(metaData1, metaData2); - RecordMetaData metaData3 = replaceIndex(metaData2, indexName, this::clearOptions); + RecordMetaData metaData3 = mutateIndex(metaData2, indexName, this::clearOptions); validator.validate(metaData2, metaData3); // Increasing the tokenizer version is fine, but decreasing it is not - RecordMetaData metaData4 = replaceIndex(metaData3, indexName, + RecordMetaData metaData4 = mutateIndex(metaData3, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_TOKENIZER_NAME_OPTION, PrefixTextTokenizer.NAME)); - RecordMetaData metaData5 = replaceIndex(metaData4, indexName, + RecordMetaData metaData5 = mutateIndex(metaData4, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_TOKENIZER_VERSION_OPTION, "" + TextTokenizer.GLOBAL_MIN_VERSION)); validator.validate(metaData4, metaData5); - RecordMetaData metaData6 = replaceIndex(metaData5, indexName, + RecordMetaData metaData6 = mutateIndex(metaData5, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_TOKENIZER_VERSION_OPTION, "" + (TextTokenizer.GLOBAL_MIN_VERSION + 1))); validator.validate(metaData5, metaData6); validateIndexMutation("text tokenizer version downgraded", metaData6, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_TOKENIZER_VERSION_OPTION, "" + TextTokenizer.GLOBAL_MIN_VERSION)); // Changing whether aggressive conflict ranges are allowed is safe - RecordMetaData metaData7 = replaceIndex(metaData6, indexName, + RecordMetaData metaData7 = mutateIndex(metaData6, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_ADD_AGGRESSIVE_CONFLICT_RANGES_OPTION, "true")); validator.validate(metaData6, metaData7); - RecordMetaData metaData8 = replaceIndex(metaData7, indexName, + RecordMetaData metaData8 = mutateIndex(metaData7, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_ADD_AGGRESSIVE_CONFLICT_RANGES_OPTION, "false")); validator.validate(metaData7, metaData8); // Changing whether position lists are omitted is safe - RecordMetaData metaData9 = replaceIndex(metaData8, indexName, + RecordMetaData metaData9 = mutateIndex(metaData8, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_OMIT_POSITIONS_OPTION, "true")); validator.validate(metaData8, metaData9); - RecordMetaData metaData10 = replaceIndex(metaData9, indexName, + RecordMetaData metaData10 = mutateIndex(metaData9, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_OMIT_POSITIONS_OPTION, "false")); validator.validate(metaData9, metaData10); } @@ -2228,7 +2329,7 @@ void textOptionsChanged() { @Test void optionChangeAllowedWithCustomIndexValidatorRegistry() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", this::makeUnique); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", this::makeUnique); assertSame(IndexMaintainerFactoryRegistryImpl.instance(), validator.getIndexValidatorRegistry()); assertInvalid("index adds uniqueness constraint", metaData1, metaData2); From 6f07166255f09e971778f32d06f63414170cbc2c Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Wed, 29 Apr 2026 15:18:48 +0100 Subject: [PATCH 3/7] moar joined record type tests --- .../MetaDataEvolutionValidatorTest.java | 461 ++++++++++++++++-- 1 file changed, 408 insertions(+), 53 deletions(-) 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 67426202a9..ea5b9bebc4 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 @@ -26,6 +26,7 @@ import com.apple.foundationdb.record.RecordMetaDataBuilder; import com.apple.foundationdb.record.RecordMetaDataOptionsProto; import com.apple.foundationdb.record.RecordMetaDataProto; +import com.apple.foundationdb.record.TestRecords1EvolvedProto; import com.apple.foundationdb.record.TestRecords1Proto; import com.apple.foundationdb.record.TestRecords4Proto; import com.apple.foundationdb.record.TestRecordsEnumProto; @@ -43,6 +44,7 @@ import com.apple.foundationdb.record.provider.common.text.DefaultTextTokenizer; import com.apple.foundationdb.record.provider.common.text.PrefixTextTokenizer; import com.apple.foundationdb.record.provider.common.text.TextTokenizer; +import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStoreTestBase; import com.apple.foundationdb.record.provider.foundationdb.IndexMaintainerFactoryRegistryImpl; import com.apple.foundationdb.tuple.Tuple; import com.apple.test.ParameterizedTestUtils; @@ -56,6 +58,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -237,15 +240,21 @@ void doNotChangeVersion() { // Schema evolution tests + @Nonnull + static RecordMetaData updateMetaData(@Nonnull RecordMetaData metaData, @Nonnull Consumer updater) { + RecordMetaDataProto.MetaData.Builder metaDataProtoBuilder = metaData.toProto().toBuilder(); + metaDataProtoBuilder.setVersion(metaData.getVersion() + 1); + updater.accept(metaDataProtoBuilder); + return RecordMetaData.build(metaDataProtoBuilder.build()); + } + @Nonnull static RecordMetaData replaceRecordsDescriptor(@Nonnull RecordMetaData metaData, @Nonnull FileDescriptor newDescriptor, @Nonnull Consumer metaDataMutation) { - RecordMetaDataProto.MetaData.Builder protoBuilder = metaData.toProto().toBuilder() - .setVersion(metaData.getVersion() + 1) - .setRecords(newDescriptor.toProto()) - .addDependencies(TestRecords1Proto.getDescriptor().toProto()); - metaDataMutation.accept(protoBuilder); - return RecordMetaData.build(protoBuilder.build()); + return updateMetaData(metaData, protoBuilder -> { + protoBuilder.setRecords(newDescriptor.toProto()); + metaDataMutation.accept(protoBuilder); + }); } @Nonnull @@ -1585,7 +1594,65 @@ void primaryKeyChanged() { assertInvalid("record type primary key changed", metaData1, metaData2); } - // Synthetic type tests + // JoinedRecordType type tests + + private void addNumValue2Join(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); + joinBuilder.addConstituent("l", "MySimpleRecord"); + joinBuilder.addConstituent("r", "MyOtherRecord"); + joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); + + metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); + } + + private void addThreeWayNumValue2Join(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); + joinBuilder.addConstituent("s", "MySimpleRecord"); + joinBuilder.addConstituent("o", "MyOtherRecord"); + joinBuilder.addConstituent("a", "AnotherRecord"); + joinBuilder.addJoin("s", Key.Expressions.field("num_value_2"), "o", Key.Expressions.field("num_value_2")); + joinBuilder.addJoin("s", Key.Expressions.field("num_value_2"), "a", Key.Expressions.field("num_value_2")); + + metaDataBuilder.addIndex("join_nv2", "joined$nv3", Key.Expressions.concat( + Key.Expressions.field("s").nest("num_value_3_indexed"), + Key.Expressions.field("o").nest("num_value_3_indexed"), + Key.Expressions.field("a").nest("num_value_3_indexed")) + ); + } + + private void addStrValueSelfJoin(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("self_join_svi"); + joinBuilder.addConstituent("s1", "MySimpleRecord"); + joinBuilder.addConstituent("s2", "MySimpleRecord"); + joinBuilder.addJoin("s1", Key.Expressions.field("str_value_indexed"), "s2", Key.Expressions.field("str_value_indexed")); + + metaDataBuilder.addIndex("self_join_svi", "self_join_svi$num_value_unique", Key.Expressions.concat(Key.Expressions.field("s1").nest("num_value_unique"), Key.Expressions.field("s2").nest("num_value_unique"))); + } + + @Nonnull + private RecordMetaData createSimpleMetaData(@Nonnull FDBRecordStoreTestBase.RecordMetaDataHook hook) { + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + hook.apply(metaDataBuilder); + return metaDataBuilder.build(); + } + + @Nonnull + private RecordMetaData createEvolvedMetaData(@Nonnull FDBRecordStoreTestBase.RecordMetaDataHook hook) { + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1EvolvedProto.getDescriptor()); + hook.apply(metaDataBuilder); + return metaDataBuilder.build(); + } + + @Nonnull + private RecordMetaData mutateJoinedRecordType(@Nonnull RecordMetaData metaData, @Nonnull String typeName, @Nonnull Consumer typeMutator) { + return updateMetaData(metaData, metaDataProtoBuilder -> { + for (RecordMetaDataProto.JoinedRecordType.Builder typeBuilder : metaDataProtoBuilder.getJoinedRecordTypesBuilderList()) { + if (typeBuilder.getName().equals(typeName)) { + typeMutator.accept(typeBuilder); + } + } + }); + } @Test void addJoinedType() { @@ -1593,11 +1660,7 @@ void addJoinedType() { RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); metaDataBuilder.setVersion(metaData1.getVersion() + 1); - final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); - joinBuilder.addConstituent("l", "MySimpleRecord"); - joinBuilder.addConstituent("r", "MyOtherRecord"); - joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); - metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); + addNumValue2Join(metaDataBuilder); RecordMetaData metaData2 = metaDataBuilder.build(); validator.validate(metaData1, metaData2); @@ -1605,15 +1668,9 @@ void addJoinedType() { @Test void dropJoinedType() { - RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); - final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); - joinBuilder.addConstituent("l", "MySimpleRecord"); - joinBuilder.addConstituent("r", "MyOtherRecord"); - joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); - metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); - final RecordMetaData metaData1 = metaDataBuilder.build(); + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); - metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); metaDataBuilder.setVersion(metaData1.getVersion() + 1); metaDataBuilder.addFormerIndex(new FormerIndex("joined$l.num_value_unique", metaData1.getVersion(), metaData1.getVersion() + 1, "joined$l.num_value_unique")); RecordMetaData metaData2 = metaDataBuilder.build(); @@ -1621,16 +1678,37 @@ void dropJoinedType() { validator.validate(metaData1, metaData2); } + @Test + void swapJoinTypeDefinitions() { + final RecordMetaData metaData1 = createSimpleMetaData(metaDataBuilder -> { + addNumValue2Join(metaDataBuilder); + addStrValueSelfJoin(metaDataBuilder); + }); + + final RecordMetaData metaData2 = updateMetaData(metaData1, metaDataBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Builder type1 = metaDataBuilder.getJoinedRecordTypesBuilder(0); + final RecordMetaDataProto.JoinedRecordType.Builder type2 = metaDataBuilder.getJoinedRecordTypesBuilder(1); + + // Swap the two types' record type keys + RecordKeyExpressionProto.Value type1Key = type1.getRecordTypeKey(); + type1.setRecordTypeKey(type2.getRecordTypeKey()); + type2.setRecordTypeKey(type1Key); + }); + + assertNotEquals(metaData1.getSyntheticRecordType("join_nv2").getRecordTypeKey(), metaData2.getSyntheticRecordType("join_nv2").getRecordTypeKey()); + assertNotEquals(metaData1.getSyntheticRecordType("self_join_svi").getRecordTypeKey(), metaData2.getSyntheticRecordType("self_join_svi").getRecordTypeKey()); + + assertInvalid("join constituent name changed", metaData1, metaData2); + final MetaDataEvolutionValidator stricterValidator = validator.asBuilder() + .setDisallowTypeRenames(true) + .build(); + assertInvalid("synthetic record type name changed", stricterValidator, metaData1, metaData2); + } + @Test void swapJoinConstituents() { - RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); - final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); - joinBuilder.addConstituent("l", "MySimpleRecord"); - joinBuilder.addConstituent("r", "MyOtherRecord"); - joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); - metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); - final RecordMetaData metaData1 = metaDataBuilder.build(); final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", joinedType -> { final var constituentsList = joinedType.getJoinConstituentsList(); joinedType.clearJoinConstituents(); @@ -1647,14 +1725,8 @@ void swapJoinConstituents() { @Test void swapSelfJoinConstituents() { - RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); - final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("self_join_svi"); - joinBuilder.addConstituent("s1", "MySimpleRecord"); - joinBuilder.addConstituent("s2", "MySimpleRecord"); - joinBuilder.addJoin("s1", Key.Expressions.field("str_value_indexed"), "s2", Key.Expressions.field("str_value_indexed")); - metaDataBuilder.addIndex("self_join_svi", "self_join_svi$num_value_unique", Key.Expressions.concat(Key.Expressions.field("s1").nest("num_value_unique"), Key.Expressions.field("s2").nest("num_value_unique"))); + final RecordMetaData metaData1 = createSimpleMetaData(this::addStrValueSelfJoin); - final RecordMetaData metaData1 = metaDataBuilder.build(); final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "self_join_svi", joinedType -> { final var constituentsList = joinedType.getJoinConstituentsList(); joinedType.clearJoinConstituents(); @@ -1681,6 +1753,308 @@ void swapSelfJoinConstituents() { laxerValidator.validate(metaData1, metaData4); } + @ParameterizedTest + @ValueSource(strings = {"MySimpleRecord", "MyOtherRecord"}) + void changeUnderlyingJoinConstituentType(String recordType) { + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); + + // Delete and recreate the MySimpleRecord type. As we can't actually delete types, this is done by + // renaming the old type and deprecating its field in the union descriptor. We then add a new copy of + // it to the list of types, ensuring to assign that new type a new union descriptor field. + // We use a recreated version of the same type to maximize the number of things that are the same: + // the same type name, the same set of fields, etc. The only way that it should notice that something + // is up is that it has to check the position in the union descriptor + FileDescriptor updatedDescriptor = mutateFile(TestRecords1Proto.getDescriptor(), fileBuilder -> { + DescriptorProtos.DescriptorProto oldType = null; + for (DescriptorProtos.DescriptorProto.Builder messageType : fileBuilder.getMessageTypeBuilderList()) { + if (messageType.getName().equals(recordType)) { + // Rename the old MySimpleRecord. Save a copy prior to the rename + oldType = messageType.build(); + messageType.setName(recordType + "__old"); + } else if (messageType.getName().equals(RecordMetaDataBuilder.DEFAULT_UNION_NAME)) { + // Rename the reference in the union descriptor + for (DescriptorProtos.FieldDescriptorProto.Builder fieldBuilder : messageType.getFieldBuilderList()) { + if (fieldBuilder.getTypeName().endsWith(recordType)) { + deprecateField(fieldBuilder); + fieldBuilder + .setName("_" + recordType + "__old") + .setTypeName(recordType + "__old"); + } + } + // Create a field for the recreated MySimpleRecord (added below) + addField(messageType) + .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_MESSAGE) + .setTypeName(recordType) + .setName("_" + recordType); + + } + } + // Add a copy of MySimpleRecord to the fileBuilder. This represents recreating a new type with + // the same name (and in this case, the same set of fields). + fileBuilder.addMessageType(oldType); + }); + final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, updatedDescriptor, metaDataBuilder -> { + // Rename the old record type + for (RecordMetaDataProto.RecordType.Builder recordTypeBuilder : metaDataBuilder.getRecordTypesBuilderList()) { + if (recordTypeBuilder.getName().equals(recordType)) { + recordTypeBuilder.setName(recordType + "__old"); + } + } + // Create a new record type with an updated since version + metaDataBuilder.addRecordTypesBuilder() + .setName(recordType) + .setSinceVersion(metaDataBuilder.getVersion()) + .setPrimaryKey(Key.Expressions.field("rec_no").toKeyExpression()); + + for (RecordMetaDataProto.Index.Builder indexBuilder : metaDataBuilder.getIndexesBuilderList()) { + // Rename any references to the type in the indexes. Note that the joined record type's index is _not_ updated + if (indexBuilder.getRecordTypeList().contains(recordType)) { + final List oldTypes = indexBuilder.getRecordTypeList(); + indexBuilder.clearRecordType(); + oldTypes.stream() + .map(type -> type.equals(recordType) ? (recordType + "__old") : type) + .forEach(indexBuilder::addRecordType); + } + } + }); + + assertFalse(validator.disallowsTypeRenames()); + assertInvalid("join constituent type changed", metaData1, metaData2); + final MetaDataEvolutionValidator stricterValidator = validator.asBuilder() + .setDisallowTypeRenames(true) + .build(); + assertTrue(stricterValidator.disallowsTypeRenames()); + assertInvalid("record type name changed", stricterValidator, metaData1, metaData2); + + RecordMetaData metaData3 = mutateJoinedRecordType(metaData2, "join_nv2", typeBuilder -> { + for (RecordMetaDataProto.JoinedRecordType.JoinConstituent.Builder constituent : typeBuilder.getJoinConstituentsBuilderList()) { + if (constituent.getRecordType().equals(recordType)) { + constituent.setRecordType(recordType + "__old"); + } + } + }); + validator.validate(metaData1, metaData3); + assertInvalid("record type name changed", stricterValidator, metaData1, metaData2); + } + + @Test + void changeJoinConstituentName() { + final RecordMetaData metaData1 = createSimpleMetaData(this::addStrValueSelfJoin); + + final RecordMetaData metaData2 = updateMetaData(metaData1, metaDataBuilder -> { + for (RecordMetaDataProto.JoinedRecordType.Builder joinedTypeBuilder : metaDataBuilder.getJoinedRecordTypesBuilderList()) { + if (joinedTypeBuilder.getName().equals("self_join_svi")) { + for (RecordMetaDataProto.JoinedRecordType.JoinConstituent.Builder constituentBuilder : joinedTypeBuilder.getJoinConstituentsBuilderList()) { + if (constituentBuilder.getName().equals("s1")) { + constituentBuilder.setName("x"); + } + } + joinedTypeBuilder.getJoinsBuilderList().get(0) + .setLeft("x"); + } + } + + for (RecordMetaDataProto.Index.Builder indexBuilder : metaDataBuilder.getIndexesBuilderList()) { + if (indexBuilder.getName().equals("self_join_svi$num_value_unique")) { + indexBuilder.setRootExpression(Key.Expressions.concat(Key.Expressions.field("x").nest("num_value_unique"), Key.Expressions.field("s2").nest("num_value_unique")).toKeyExpression()); + } + } + }); + + assertInvalid("join constituent name changed", metaData1, metaData2); + final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() + .setAllowFieldRenames(true) + .build(); + laxerValidator.validate(metaData1, metaData2); + } + + @Test + void addJoinConstituent() { + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); + + final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, TestRecords1EvolvedProto.getDescriptor(), metaDataBuilder -> { + metaDataBuilder.addRecordTypesBuilder() + .setName("AnotherRecord") + .setPrimaryKey(Key.Expressions.field("rec_no").toKeyExpression()) + .setSinceVersion(metaDataBuilder.getVersion()); + + for (RecordMetaDataProto.JoinedRecordType.Builder joinTypeBuilder : metaDataBuilder.getJoinedRecordTypesBuilderList()) { + joinTypeBuilder.addJoinConstituentsBuilder() + .setName("x") + .setRecordType("AnotherRecord") + .setOuterJoined(false); + joinTypeBuilder.addJoinsBuilder() + .setLeft("l") + .setLeftExpression(Key.Expressions.field("num_value_2").toKeyExpression()) + .setRight("x") + .setRightExpression(Key.Expressions.field("num_value_2").toKeyExpression()); + } + }); + assertInvalid("join constituent count changed", metaData1, metaData2); + } + + @Test + void removeJoinConstituent() { + final RecordMetaData metaData1 = createEvolvedMetaData(this::addThreeWayNumValue2Join); + + final RecordMetaData metaData2 = updateMetaData(metaData1, metaDataBuilder -> { + for (RecordMetaDataProto.JoinedRecordType.Builder joinedTypeBuilder : metaDataBuilder.getJoinedRecordTypesBuilderList()) { + if (joinedTypeBuilder.getName().equals("join_nv2")) { + joinedTypeBuilder.removeJoinConstituents(0); + joinedTypeBuilder.clearJoins(); + joinedTypeBuilder.addJoinsBuilder() + .setLeft("o") + .setLeftExpression(Key.Expressions.field("num_value_2").toKeyExpression()) + .setRight("a") + .setRightExpression(Key.Expressions.field("num_value_2").toKeyExpression()); + } + } + for (RecordMetaDataProto.Index.Builder index : metaDataBuilder.getIndexesBuilderList()) { + if (index.getRecordTypeList().contains("join_nv2")) { + index.setRootExpression(Key.Expressions.concat( + Key.Expressions.field("o").nest("num_value_3_indexed"), + Key.Expressions.field("a").nest("num_value_3_indexed") + ).toKeyExpression()); + } + } + }); + assertInvalid("join constituent count changed", metaData1, metaData2); + } + + @Test + void addJoinCondition() { + final RecordMetaData metaData1 = createEvolvedMetaData(this::addThreeWayNumValue2Join); + + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", joinedTypeBuilder -> + // Technically, this join criterion is implied by the existing two criteria via transitivity, so + // this may need to change if we ever modify the check to account for that + joinedTypeBuilder.addJoinsBuilder() + .setLeft("o") + .setLeftExpression(Key.Expressions.field("num_value_2").toKeyExpression()) + .setLeft("a") + .setRightExpression(Key.Expressions.field("num_value_2").toKeyExpression())); + + assertInvalid("join type join count changed", metaData1, metaData2); + } + + @Test + void removeJoinCondition() { + final RecordMetaData metaData1 = createEvolvedMetaData(this::addThreeWayNumValue2Join); + + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", + joinedTypeBuilder -> joinedTypeBuilder.removeJoins(0)); + assertInvalid("join type join count changed", metaData1, metaData2); + } + + @Test + void changeOuterJoinedProperty() { + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); + + // Assert cannot go from not outer joined to outer joined + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", + joinedTypeBuilder -> joinedTypeBuilder.getJoinConstituentsBuilder(0).setOuterJoined(true)); + assertInvalid("join constituent outer-joined property changed", metaData1, metaData2); + + // Assert cannot go from outer joined to not outer joined + final RecordMetaData metaData3 = updateMetaData(metaData1, metaDataBuilder -> metaDataBuilder.setVersion(metaData2.getVersion() + 1)); + assertInvalid("join constituent outer-joined property changed", metaData2, metaData3); + } + + @Test + void changeJoinLeftComponents() { + final RecordMetaData metaData1 = createEvolvedMetaData(this::addThreeWayNumValue2Join); + + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", joinedTypeBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Join.Builder joinBuilder = joinedTypeBuilder.getJoinsBuilder(0); + joinBuilder.setLeft("a"); + }); + assertInvalid("join changed left constituent", metaData1, metaData2); + + final RecordMetaData metaData3 = mutateJoinedRecordType(metaData1, "join_nv2", joinedTypeBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Join.Builder joinBuilder = joinedTypeBuilder.getJoinsBuilder(0); + joinBuilder.setLeftExpression(Key.Expressions.field("num_value_unique").toKeyExpression()); + }); + assertInvalid("join changed left expression", metaData1, metaData3); + } + + @Test + void changeJoinRightComponents() { + final RecordMetaData metaData1 = createEvolvedMetaData(this::addThreeWayNumValue2Join); + + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", joinedTypeBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Join.Builder joinBuilder = joinedTypeBuilder.getJoinsBuilder(0); + joinBuilder.setRight("a"); + }); + assertInvalid("join changed right constituent", metaData1, metaData2); + + final RecordMetaData metaData3 = mutateJoinedRecordType(metaData1, "join_nv2", joinedTypeBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Join.Builder joinBuilder = joinedTypeBuilder.getJoinsBuilder(0); + joinBuilder.setRightExpression(Key.Expressions.field("num_value_3_indexed").toKeyExpression()); + }); + assertInvalid("join changed right expression", metaData1, metaData3); + } + + @ParameterizedTest + @MethodSource("deprecatedArgs") + void renameFieldInJoinExpression(boolean deprecated) { + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); + + // Change a field in MySimpleRecord used in the join + FileDescriptor renamedSimpleNum2 = mutateMessageType("MySimpleRecord", typeBuilder -> { + for (DescriptorProtos.FieldDescriptorProto.Builder fieldBuilder : typeBuilder.getFieldBuilderList()) { + if (fieldBuilder.getName().equals("num_value_2")) { + if (deprecated) { + deprecateField(fieldBuilder); + } + fieldBuilder.setName("num_value_2__old"); + } + } + addField(typeBuilder) + .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_SFIXED64) + .setName("num_value_2"); + }); + final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, renamedSimpleNum2); + fieldRenameChecker.assertInvalidRenaming("join changed left expression", deprecated, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateJoinedRecordType(metaData2, "join_nv2", joinedTypeBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Join.Builder joinBuilder = joinedTypeBuilder.getJoinsBuilder(0); + joinBuilder.setLeftExpression(Key.Expressions.field("num_value_2__old").toKeyExpression()); + }); + fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); + + // Now change the same field in MyOtherRecord + FileDescriptor renamedOtherNum2 = mutateMessageType("MyOtherRecord", renamedSimpleNum2, typeBuilder -> { + for (DescriptorProtos.FieldDescriptorProto.Builder fieldBuilder : typeBuilder.getFieldBuilderList()) { + if (fieldBuilder.getName().equals("num_value_2")) { + if (deprecated) { + deprecateField(fieldBuilder); + } + fieldBuilder.setName("num_value_2__old"); + } + } + addField(typeBuilder) + .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_SFIXED64) + .setName("num_value_2"); + }); + final RecordMetaData metaData4 = replaceRecordsDescriptor(metaData3, renamedOtherNum2); + fieldRenameChecker.assertInvalidRenaming("join changed right expression", deprecated, metaData1, metaData4); + fieldRenameChecker.assertInvalidRenaming("join changed right expression", deprecated, metaData3, metaData4); + + final RecordMetaData metaData5 = mutateJoinedRecordType(metaData4, "join_nv2", joinedTypeBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Join.Builder joinBuilder = joinedTypeBuilder.getJoinsBuilder(0); + joinBuilder.setRightExpression(Key.Expressions.field("num_value_2__old").toKeyExpression()); + }); + fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData5); + fieldRenameChecker.assertValidRenaming(deprecated, metaData3, metaData5); + } + + // UnnestedRecordTypeTests + + + // Former index tests @Test @@ -1966,14 +2340,6 @@ private void clearOptions(@Nonnull RecordMetaDataProto.Index.Builder indexProto) indexProto.clearOptions(); } - @Nonnull - private RecordMetaData updateMetaData(@Nonnull RecordMetaData metaData, @Nonnull Consumer updater) { - RecordMetaDataProto.MetaData.Builder metaDataProtoBuilder = metaData.toProto().toBuilder(); - updater.accept(metaDataProtoBuilder); - metaDataProtoBuilder.setVersion(metaData.getVersion() + 1); - return RecordMetaData.build(metaDataProtoBuilder.build()); - } - @Nonnull private RecordMetaData mutateIndex(@Nonnull RecordMetaData metaData, @Nonnull String indexName, @Nonnull Consumer indexMutator) { return updateMetaData(metaData, metaDataProtoBuilder -> { @@ -1985,17 +2351,6 @@ private RecordMetaData mutateIndex(@Nonnull RecordMetaData metaData, @Nonnull St }); } - @Nonnull - private RecordMetaData mutateJoinedRecordType(@Nonnull RecordMetaData metaData, @Nonnull String typeName, @Nonnull Consumer typeMutator) { - return updateMetaData(metaData, metaDataProtoBuilder -> { - for (RecordMetaDataProto.JoinedRecordType.Builder typeBuilder : metaDataProtoBuilder.getJoinedRecordTypesBuilderList()) { - if (typeBuilder.getName().equals(typeName)) { - typeMutator.accept(typeBuilder); - } - } - }); - } - @Test void silentlyRemoveIndex() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); From 1e880e8921469906ed0c92d54fbf4955df78e69e Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Wed, 29 Apr 2026 16:22:53 +0100 Subject: [PATCH 4/7] fix test that relied on dependency being available --- .../record/metadata/MetaDataEvolutionValidatorTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 ea5b9bebc4..9957675e08 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 @@ -245,7 +245,10 @@ static RecordMetaData updateMetaData(@Nonnull RecordMetaData metaData, @Nonnull RecordMetaDataProto.MetaData.Builder metaDataProtoBuilder = metaData.toProto().toBuilder(); metaDataProtoBuilder.setVersion(metaData.getVersion() + 1); updater.accept(metaDataProtoBuilder); - return RecordMetaData.build(metaDataProtoBuilder.build()); + return RecordMetaData.newBuilder() + .addDependency(TestRecords1Proto.getDescriptor()) + .setRecords(metaDataProtoBuilder.build()) + .build(); } @Nonnull From d3ad482b1a174e7a59c8722811d06613034462bd Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Wed, 29 Apr 2026 19:55:25 +0100 Subject: [PATCH 5/7] add tests for unnested types modified under various conditions --- .../metadata/MetaDataEvolutionValidator.java | 8 +- .../MetaDataEvolutionValidatorTest.java | 301 ++++++++++++++++++ 2 files changed, 305 insertions(+), 4 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java index 9070fbba6b..76463f464d 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java @@ -654,8 +654,10 @@ private void validateUnnestedRecordType(@Nonnull UnnestedRecordType oldType, @No } } else { // Make sure we are pointing to the same parent - int oldParentPos = oldNameToConstituentPosMap.get(oldConstituent.getName()); - int newParentPos = newNameToConstituentPosMap.get(newConstituent.getName()); + final UnnestedRecordType.NestedConstituent oldParent = Objects.requireNonNull(oldConstituent.getParent()); + final UnnestedRecordType.NestedConstituent newParent = Objects.requireNonNull(newConstituent.getParent()); + int oldParentPos = oldNameToConstituentPosMap.get(oldParent.getName()); + int newParentPos = newNameToConstituentPosMap.get(newParent.getName()); if (oldParentPos != newParentPos) { throw new MetaDataException("nested constituent changed parent", LogMessageKeys.EXPECTED, newConstituents.get(oldParentPos).getName(), @@ -666,8 +668,6 @@ private void validateUnnestedRecordType(@Nonnull UnnestedRecordType oldType, @No // Make sure the nesting expression is still the same, possibly accounting for field renames final KeyExpression expectedNestingExpression; if (allowsAnyFieldRenames()) { - UnnestedRecordType.NestedConstituent oldParent = Objects.requireNonNull(oldConstituent.getParent()); - UnnestedRecordType.NestedConstituent newParent = Objects.requireNonNull(newConstituent.getParent()); expectedNestingExpression = RenameFieldsVisitor.renameFields(oldConstituent.getNestingExpression(), oldParent.getRecordType().getDescriptor(), newParent.getRecordType().getDescriptor()); } else { expectedNestingExpression = oldConstituent.getNestingExpression(); 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 9957675e08..7143dabcd0 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 @@ -29,6 +29,7 @@ import com.apple.foundationdb.record.TestRecords1EvolvedProto; import com.apple.foundationdb.record.TestRecords1Proto; import com.apple.foundationdb.record.TestRecords4Proto; +import com.apple.foundationdb.record.TestRecordsDoubleNestedProto; import com.apple.foundationdb.record.TestRecordsEnumProto; import com.apple.foundationdb.record.TestRecordsIdenticalTypesProto; import com.apple.foundationdb.record.TestRecordsWithHeaderProto; @@ -40,6 +41,7 @@ import com.apple.foundationdb.record.evolution.TestSplitNestedTypesProto; import com.apple.foundationdb.record.evolution.TestUnmergedNestedTypesProto; import com.apple.foundationdb.record.expressions.RecordKeyExpressionProto; +import com.apple.foundationdb.record.metadata.expressions.KeyExpression; import com.apple.foundationdb.record.provider.common.text.AllSuffixesTextTokenizer; import com.apple.foundationdb.record.provider.common.text.DefaultTextTokenizer; import com.apple.foundationdb.record.provider.common.text.PrefixTextTokenizer; @@ -67,6 +69,7 @@ import java.util.Collections; import java.util.List; import java.util.Locale; +import java.util.Objects; import java.util.Set; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -2056,7 +2059,305 @@ void renameFieldInJoinExpression(boolean deprecated) { // UnnestedRecordTypeTests + private void addUnnestedManyMiddleType(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final UnnestedRecordTypeBuilder unnestedBuilder = metaDataBuilder.addUnnestedRecordType("unnest_many_middle"); + unnestedBuilder.addParentConstituent("outer", metaDataBuilder.getRecordType("OuterRecord")); + unnestedBuilder.addNestedConstituent("middle", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.getDescriptor(), "outer", Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut)); + metaDataBuilder.addIndex("unnest_many_middle", new Index("unnest$other_int", Key.Expressions.concat(Key.Expressions.field("outer").nest("other_int"), Key.Expressions.field("middle").nest("other_int")))); + } + + private void addUnnestedManyMiddleAndInnerType(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final UnnestedRecordTypeBuilder unnestedBuilder = metaDataBuilder.addUnnestedRecordType("unnest_many_middle_and_inner"); + unnestedBuilder.addParentConstituent("outer", metaDataBuilder.getRecordType("OuterRecord")); + unnestedBuilder.addNestedConstituent("middle", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.getDescriptor(), "outer", Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut)); + unnestedBuilder.addNestedConstituent("inner", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.InnerRecord.getDescriptor(), "middle", Key.Expressions.field("inner", KeyExpression.FanType.FanOut)); + + metaDataBuilder.addIndex("unnest_many_middle_and_inner", new Index("unnest$other_int+foo", Key.Expressions.concat(Key.Expressions.field("middle").nest("other_int"), Key.Expressions.field("inner").nest("foo")))); + } + + private void addCrossProduceManyMiddleType(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final UnnestedRecordTypeBuilder unnestedBuilder = metaDataBuilder.addUnnestedRecordType("unnest_cross_many_middle"); + unnestedBuilder.addParentConstituent("outer", metaDataBuilder.getRecordType("OuterRecord")); + unnestedBuilder.addNestedConstituent("m1", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.getDescriptor(), "outer", Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut)); + unnestedBuilder.addNestedConstituent("m2", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.getDescriptor(), "outer", Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut)); + + metaDataBuilder.addIndex("unnest_cross_many_middle", new Index("unnest$all_other_int", Key.Expressions.concat( + Key.Expressions.field("outer").nest("other_int"), + Key.Expressions.field("m1").nest("other_int"), + Key.Expressions.field("m2").nest("other_int")))); + } + + private void addCrossProductManyMiddleAndInnerType(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final UnnestedRecordTypeBuilder unnestedBuilder = metaDataBuilder.addUnnestedRecordType("unnest_cross_many_middle_and_inner"); + unnestedBuilder.addParentConstituent("outer", metaDataBuilder.getRecordType("OuterRecord")); + unnestedBuilder.addNestedConstituent("m1", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.getDescriptor(), "outer", Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut)); + unnestedBuilder.addNestedConstituent("m2", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.getDescriptor(), "outer", Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut)); + unnestedBuilder.addNestedConstituent("inner", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.InnerRecord.getDescriptor(), "m1", Key.Expressions.field("inner", KeyExpression.FanType.FanOut)); + + metaDataBuilder.addIndex("unnest_cross_many_middle_and_inner", new Index("unnest$inner_foo_bar", + Key.Expressions.field("inner").nest(Key.Expressions.concatenateFields("foo", "bar")))); + } + + @Nonnull + private RecordMetaData mutateUnnestedRecordType(@Nonnull RecordMetaData metaData, @Nonnull String typeName, @Nonnull Consumer typeMutator) { + return updateMetaData(metaData, metaDataProtoBuilder -> { + for (RecordMetaDataProto.UnnestedRecordType.Builder typeBuilder : metaDataProtoBuilder.getUnnestedRecordTypesBuilderList()) { + if (typeBuilder.getName().equals(typeName)) { + typeMutator.accept(typeBuilder); + } + } + }); + } + + @Nonnull + private RecordMetaData createDoubleNestedMetaData(@Nonnull FDBRecordStoreTestBase.RecordMetaDataHook hook) { + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecordsDoubleNestedProto.getDescriptor()); + hook.apply(metaDataBuilder); + return metaDataBuilder.build(); + } + + @Test + void addUnnestedRecordType() { + final RecordMetaData metaData1 = RecordMetaData.build(TestRecordsDoubleNestedProto.getDescriptor()); + + final RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecordsDoubleNestedProto.getDescriptor()); + metaDataBuilder.setVersion(metaData1.getVersion() + 1); + addUnnestedManyMiddleType(metaDataBuilder); + final RecordMetaData metaData2 = metaDataBuilder.build(); + + validator.validate(metaData1, metaData2); + } + + @Test + void dropUnnestedRecordType() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + final RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecordsDoubleNestedProto.getDescriptor()); + metaDataBuilder.setVersion(metaData1.getVersion() + 1); + metaDataBuilder.addFormerIndex(new FormerIndex("unnest$other_int", metaData1.getVersion(), metaData1.getVersion() + 1, "unnest$other_int")); + final RecordMetaData metaData2 = metaDataBuilder.build(); + + validator.validate(metaData1, metaData2); + } + + @Test + void addUnnestedConstituent() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + final RecordMetaData metaData2 = mutateUnnestedRecordType(metaData1, "unnest_many_middle", + unnestedTypeBuilder -> unnestedTypeBuilder.addNestedConstituentsBuilder() + .setName("inner") + .setParent("middle") + .setNestingExpression(Key.Expressions.field("inner").toKeyExpression()) + .setTypeName(TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.InnerRecord.getDescriptor().getFullName())); + + assertInvalid("unnested type constituent count changed", metaData1, metaData2); + } + + @Test + void flipNestedConstituents() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addCrossProduceManyMiddleType); + + final RecordMetaData metaData2 = mutateUnnestedRecordType(metaData1, "unnest_cross_many_middle", unnestedTypeBuilder -> { + // Flip the order of the m2 and m1 constituents + final RecordMetaDataProto.UnnestedRecordType.NestedConstituent m1 = unnestedTypeBuilder.getNestedConstituents(1); + final RecordMetaDataProto.UnnestedRecordType.NestedConstituent m2 = unnestedTypeBuilder.getNestedConstituents(2); + unnestedTypeBuilder.removeNestedConstituents(2); + unnestedTypeBuilder.removeNestedConstituents(1); + unnestedTypeBuilder.addNestedConstituents(m2); + unnestedTypeBuilder.addNestedConstituents(m1); + }); + + assertInvalid("nested constituent name changed", metaData1, metaData2); + final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() + .setAllowFieldRenames(true) + .build(); + assertInvalid("index key expression does not match required", laxerValidator, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateIndex(metaData2, "unnest$all_other_int", + indexBuilder -> indexBuilder.setRootExpression(Key.Expressions.concat( + Key.Expressions.field("outer").nest("other_int"), + Key.Expressions.field("m2").nest("other_int"), + Key.Expressions.field("m1").nest("other_int") + ).toKeyExpression())); + assertInvalid("nested constituent name changed", metaData1, metaData3); + laxerValidator.validate(metaData1, metaData3); + } + + @Test + void changeParentType() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + final RecordMetaData metaData2 = mutateUnnestedRecordType(metaData1, "unnest_many_middle", unnestedTypeBuilder -> { + // Change the parent from OuterRecord to middle record + final RecordMetaDataProto.UnnestedRecordType.NestedConstituent.Builder parent = unnestedTypeBuilder.getNestedConstituentsBuilder(0); + parent.setTypeName("MiddleRecord"); + + // Update the path for the nesting key expression. It still actually points to the same nested type as before, it just takes a more interesting path to get there + final RecordMetaDataProto.UnnestedRecordType.NestedConstituent.Builder middle = unnestedTypeBuilder.getNestedConstituentsBuilder(1); + middle.setNestingExpression(Key.Expressions.field("other_middle").nest(Key.Expressions.field("other").nest(Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut))).toKeyExpression()); + }); + + assertInvalid("unnested type parent record type changed", metaData1, metaData2); + } + + @Test + void renameParentType() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + FileDescriptor renamedDescriptor = mutateFile(TestRecordsDoubleNestedProto.getDescriptor(), fileBuilder -> { + DescriptorProtos.DescriptorProto outerRecord = null; + for (DescriptorProtos.DescriptorProto.Builder messageBuilder : fileBuilder.getMessageTypeBuilderList()) { + if (messageBuilder.getName().equals("OuterRecord")) { + outerRecord = messageBuilder.build(); + messageBuilder.setName("OuterRecord__old"); + } else if (messageBuilder.getName().equals(RecordMetaDataBuilder.DEFAULT_UNION_NAME)) { + for (DescriptorProtos.FieldDescriptorProto.Builder fieldBuilder : messageBuilder.getFieldBuilderList()) { + if (fieldBuilder.getTypeName().endsWith("OuterRecord")) { + deprecateField(fieldBuilder); + fieldBuilder.setTypeName("OuterRecord__old").setName("_OuterRecord__old"); + } + } + addField(messageBuilder) + .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_MESSAGE) + .setTypeName("OuterRecord") + .setName("_OuterRecord"); + + } + } + fileBuilder.addMessageType(Objects.requireNonNull(outerRecord)); + }); + final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, renamedDescriptor, metaDataBuilder -> { + for (RecordMetaDataProto.RecordType.Builder recordTypeBuilder : metaDataBuilder.getRecordTypesBuilderList()) { + if (recordTypeBuilder.getName().equals("OuterRecord")) { + recordTypeBuilder.setName("OuterRecord__old"); + } + } + metaDataBuilder.addRecordTypesBuilder() + .setName("OuterRecord") + .setSinceVersion(metaDataBuilder.getVersion()) + .setPrimaryKey(Key.Expressions.field("rec_no").toKeyExpression()); + }); + + assertInvalid("unnested type parent record type changed", metaData1, metaData2); + final MetaDataEvolutionValidator stricterValidator = validator.asBuilder() + .setDisallowTypeRenames(true) + .build(); + assertInvalid("record type name changed", stricterValidator, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateUnnestedRecordType(metaData2, "unnest_many_middle", + unnestedTypeBuilder -> unnestedTypeBuilder.getNestedConstituentsBuilder(0).setTypeName("OuterRecord__old")); + validator.validate(metaData1, metaData3); + assertInvalid("record type name changed", stricterValidator, metaData1, metaData3); + } + + @Test + void changeNestedParent() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addCrossProductManyMiddleAndInnerType); + + final RecordMetaData metaData2 = mutateUnnestedRecordType(metaData1, "unnest_cross_many_middle_and_inner", + // Change the inner from pointing to m2 to m1 + unnestedRecordType -> unnestedRecordType.getNestedConstituentsBuilder(3).setParent("m2")); + assertInvalid("nested constituent changed parent", metaData1, metaData2); + + final RecordMetaData metaData3 = mutateUnnestedRecordType(metaData2, "unnest_cross_many_middle_and_inner", unnestedRecordType -> { + // Swap the positions of m1 and m2 so that now m2 is back to referring to the first middle + final List nestedConstituents = List.of( + unnestedRecordType.getNestedConstituents(0), + unnestedRecordType.getNestedConstituents(2), + unnestedRecordType.getNestedConstituents(1), + unnestedRecordType.getNestedConstituents(3) + ); + unnestedRecordType.clearNestedConstituents().addAllNestedConstituents(nestedConstituents); + }); + assertInvalid("nested constituent name changed", metaData1, metaData3); + final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() + .setAllowFieldRenames(true) + .build(); + laxerValidator.validate(metaData1, metaData3); + } + + @Test + void changeNestedConstituentExpression() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + final RecordMetaData metaData2 = mutateUnnestedRecordType(metaData1, "unnest_many_middle", unnestedTypeBuilder -> + unnestedTypeBuilder.getNestedConstituentsBuilder(1).setNestingExpression( + Key.Expressions.field("other").nest(Key.Expressions.field("outer").nest(Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut))).toKeyExpression())); + + assertInvalid("nested constituent nesting expression changed", metaData1, metaData2); + } + + @Test + void changeNestedConstituentTypeName() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleAndInnerType); + + final String newFullTypeName = TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.InnerRecord.getDescriptor().getFullName() + "_new"; + FileDescriptor renamedFileDescriptor = mutateFile(TestRecordsDoubleNestedProto.getDescriptor(), fileBuilder -> { + for (DescriptorProtos.DescriptorProto.Builder outerMessageBuilder : fileBuilder.getMessageTypeBuilderList()) { + if (outerMessageBuilder.getName().equals("OuterRecord")) { + for (DescriptorProtos.FieldDescriptorProto.Builder outerFieldBuilder : outerMessageBuilder.getFieldBuilderList()) { + if (outerFieldBuilder.getTypeName().endsWith("InnerRecord")) { + outerFieldBuilder.setTypeName(newFullTypeName); + } + } + for (DescriptorProtos.DescriptorProto.Builder middleMessageBuilder : outerMessageBuilder.getNestedTypeBuilderList()) { + for (DescriptorProtos.FieldDescriptorProto.Builder middleFieldBuilder : middleMessageBuilder.getFieldBuilderList()) { + if (middleFieldBuilder.getTypeName().endsWith("InnerRecord")) { + middleFieldBuilder.setTypeName(newFullTypeName); + } + } + for (DescriptorProtos.DescriptorProto.Builder innerMessageBuilder : middleMessageBuilder.getNestedTypeBuilderList()) { + if (innerMessageBuilder.getName().equals("InnerRecord")) { + innerMessageBuilder.setName("InnerRecord_new"); + } + } + } + } + } + }); + final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, renamedFileDescriptor, metaDataBuilder -> { + for (RecordMetaDataProto.UnnestedRecordType.Builder unnestedTypeBuilder : metaDataBuilder.getUnnestedRecordTypesBuilderList()) { + for (RecordMetaDataProto.UnnestedRecordType.NestedConstituent.Builder constituentBuilder : unnestedTypeBuilder.getNestedConstituentsBuilderList()) { + if (constituentBuilder.getTypeName().endsWith("InnerRecord")) { + constituentBuilder.setTypeName(newFullTypeName); + } + } + } + }); + + validator.validate(metaData1, metaData2); + } + + @ParameterizedTest + @MethodSource("deprecatedArgs") + void renameFieldInNestingExpression(boolean deprecated) { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + FileDescriptor renamedDescriptor = mutateMessageType("OuterRecord", TestRecordsDoubleNestedProto.getDescriptor(), messageType -> { + for (DescriptorProtos.FieldDescriptorProto.Builder fieldBuilder : messageType.getFieldBuilderList()) { + if (fieldBuilder.getName().equals("many_middle")) { + if (deprecated) { + deprecateField(fieldBuilder); + } + fieldBuilder.setName("many_middle_old"); + } + } + addField(messageType) + .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_REPEATED) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_MESSAGE) + .setTypeName(TestRecordsDoubleNestedProto.MiddleRecord.getDescriptor().getFullName()) + .setName("many_middle"); + }); + final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, renamedDescriptor); + fieldRenameChecker.assertInvalidRenaming("nested constituent nesting expression changed", deprecated, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateUnnestedRecordType(metaData2, "unnest_many_middle", unnestedTypeBuilder -> + unnestedTypeBuilder.getNestedConstituentsBuilder(1).setNestingExpression(Key.Expressions.field("many_middle_old", KeyExpression.FanType.FanOut).toKeyExpression())); + fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); + } // Former index tests From a50f97cf8c9e514403bcd7e63ee01b6048c8390b Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Fri, 1 May 2026 12:20:12 +0100 Subject: [PATCH 6/7] tests for renaming synthetic record types --- .../metadata/MetaDataEvolutionValidator.java | 12 +- .../MetaDataEvolutionValidatorTest.java | 156 ++++++++++++++++++ 2 files changed, 164 insertions(+), 4 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java index 76463f464d..218ff6d3dc 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java @@ -473,10 +473,14 @@ private Map> getSyntheticTypesByRecordTypeKey(@No } private void validateSyntheticType(@Nonnull SyntheticRecordType oldType, @Nonnull SyntheticRecordType newType, @Nonnull Map typeRenames) { - if (disallowTypeRenames && !oldType.getName().equals(newType.getName())) { - throw new MetaDataException("synthetic record type name changed", - LogMessageKeys.OLD_RECORD_TYPE, oldType.getName(), - LogMessageKeys.NEW_RECORD_TYPE, newType.getName()); + if (!oldType.getName().equals(newType.getName())) { + if (disallowTypeRenames) { + throw new MetaDataException("synthetic record type name changed", + LogMessageKeys.OLD_RECORD_TYPE, oldType.getName(), + LogMessageKeys.NEW_RECORD_TYPE, newType.getName()); + } else { + typeRenames.put(oldType.getName(), newType.getName()); + } } if (!(oldType.getClass().equals(newType.getClass()))) { throw new MetaDataException("synthetic record type changed type", 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 7143dabcd0..02a5c97c50 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 @@ -1684,6 +1684,40 @@ void dropJoinedType() { validator.validate(metaData1, metaData2); } + @Test + void renameJoinedType() { + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); + + final RecordMetaData metaData2 = updateMetaData(metaData1, metaDataBuilder -> { + RecordMetaDataProto.JoinedRecordType.Builder joinedTypeBuilder = metaDataBuilder.getJoinedRecordTypesBuilder(0); + + // Add a copy with the same name but an additional join that indexes will point to + metaDataBuilder.addJoinedRecordTypes(joinedTypeBuilder.build().toBuilder() + .setRecordTypeKey(RecordKeyExpressionProto.Value.newBuilder().setLongValue(-2)) + .addJoins(RecordMetaDataProto.JoinedRecordType.Join.newBuilder() + .setLeft("l") + .setLeftExpression(Key.Expressions.field("num_value_unique").toKeyExpression()) + .setRight("r") + .setRightExpression(Key.Expressions.field("num_value_3_indexed").toKeyExpression()) + ) + ); + + // Rename the original type + joinedTypeBuilder.setName("join_nv2_old"); + }); + + assertInvalid("new index removes record type", metaData1, metaData2); + final MetaDataEvolutionValidator stricterValidator = validator.asBuilder() + .setDisallowTypeRenames(true) + .build(); + assertInvalid("synthetic record type name changed", stricterValidator, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateIndex(metaData2, "joined$l.num_value_unique", indexBuilder -> + indexBuilder.clearRecordType().addRecordType("join_nv2_old")); + validator.validate(metaData1, metaData3); + assertInvalid("synthetic record type name changed", stricterValidator, metaData1, metaData3); + } + @Test void swapJoinTypeDefinitions() { final RecordMetaData metaData1 = createSimpleMetaData(metaDataBuilder -> { @@ -2141,6 +2175,40 @@ void dropUnnestedRecordType() { validator.validate(metaData1, metaData2); } + @Test + void renameUnnestedRecordType() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + final RecordMetaData metaData2 = updateMetaData(metaData1, metaDataBuilder -> { + RecordMetaDataProto.UnnestedRecordType.Builder unnestedTypeBuilder = metaDataBuilder.getUnnestedRecordTypesBuilder(0); + + // Make a copy with the old name and an additional constituent that old indexes will point to + metaDataBuilder.addUnnestedRecordTypes( + unnestedTypeBuilder.build().toBuilder() + .setRecordTypeKey(RecordKeyExpressionProto.Value.newBuilder().setLongValue(-2)) + .addNestedConstituents(RecordMetaDataProto.UnnestedRecordType.NestedConstituent.newBuilder() + .setName("inner") + .setParent("middle") + .setTypeName(TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.InnerRecord.getDescriptor().getFullName()) + .setNestingExpression(Key.Expressions.field("inner", KeyExpression.FanType.FanOut).toKeyExpression()) + ) + ); + // Update the name of the old one + unnestedTypeBuilder.setName("unnest_type_old"); + }); + + assertInvalid("new index removes record type", metaData1, metaData2); + final MetaDataEvolutionValidator stricterValidator = validator.asBuilder() + .setDisallowTypeRenames(true) + .build(); + assertInvalid("synthetic record type name changed", stricterValidator, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateIndex(metaData2, "unnest$other_int", indexBuilder -> + indexBuilder.clearRecordType().addRecordType("unnest_type_old")); + validator.validate(metaData1, metaData3); + assertInvalid("synthetic record type name changed", stricterValidator, metaData1, metaData3); + } + @Test void addUnnestedConstituent() { final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); @@ -2185,6 +2253,21 @@ void flipNestedConstituents() { laxerValidator.validate(metaData1, metaData3); } + @Test + void swapParentAndChildConstituents() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + // Swapping the parent and child is not allowed as the parent has to come first. But if we ever loosen that, we + // should update this test to validate that the validator rejects this change + assertThrows(MetaDataException.class, () -> mutateUnnestedRecordType(metaData1, "unnest_many_middle", unnestedTypeBuilder -> { + RecordMetaDataProto.UnnestedRecordType.NestedConstituent parentConstituent = unnestedTypeBuilder.getNestedConstituents(0); + RecordMetaDataProto.UnnestedRecordType.NestedConstituent childConstituent = unnestedTypeBuilder.getNestedConstituents(1); + unnestedTypeBuilder.clearNestedConstituents(); + unnestedTypeBuilder.addNestedConstituents(childConstituent); + unnestedTypeBuilder.addNestedConstituents(parentConstituent); + })); + } + @Test void changeParentType() { final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); @@ -2290,6 +2373,41 @@ void changeNestedConstituentExpression() { assertInvalid("nested constituent nesting expression changed", metaData1, metaData2); } + @Test + void evolveNestedConstituentType() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + // Make a valid change to a field mentioned in by an unnested record type + FileDescriptor updatedDescriptor = mutateMessageType("OuterRecord", TestRecordsDoubleNestedProto.getDescriptor(), messageBuilder -> { + final DescriptorProtos.DescriptorProto.Builder middleRecordBuilder = messageBuilder.getNestedTypeBuilder(0); + addField(middleRecordBuilder) + .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_STRING) + .setName("blah"); + }); + RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, updatedDescriptor, metaDataBuilder -> + metaDataBuilder.addIndexesBuilder() + .setName("unnest$blah") + .addRecordType("unnest_many_middle") + .setRootExpression(Key.Expressions.concat(Key.Expressions.field("outer").nest("other_int"), Key.Expressions.field("middle").nest("blah")).toKeyExpression()) + .setType(IndexTypes.VALUE) + .setAddedVersion(metaDataBuilder.getVersion()) + .setLastModifiedVersion(metaDataBuilder.getVersion())); + validator.validate(metaData1, metaData2); + + // Make an invalid change + FileDescriptor updatedAgainDescriptor = mutateMessageType("OuterRecord", updatedDescriptor, messageBuilder -> { + final DescriptorProtos.DescriptorProto.Builder middleRecordBuilder = messageBuilder.getNestedTypeBuilder(0); + for (DescriptorProtos.FieldDescriptorProto.Builder fieldBuilder : middleRecordBuilder.getFieldBuilderList()) { + if (fieldBuilder.getName().equals("blah")) { + fieldBuilder.setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_BYTES); + } + } + }); + RecordMetaData metaData3 = replaceRecordsDescriptor(metaData2, updatedAgainDescriptor); + assertInvalid("field type changed", metaData2, metaData3); + } + @Test void changeNestedConstituentTypeName() { final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleAndInnerType); @@ -2359,6 +2477,44 @@ void renameFieldInNestingExpression(boolean deprecated) { fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); } + @Test + void unnestedTypeBecomesJoinedRecordType() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + // Update the meta-data so that we now have a joined type instead of an unnested record type with the same key + final RecordMetaData metaData2 = updateMetaData(metaData1, metaDataBuilder -> { + final RecordMetaDataProto.UnnestedRecordType unnestedType = metaDataBuilder.getUnnestedRecordTypes(0); + metaDataBuilder.clearUnnestedRecordTypes(); + + // Copy the same constituent name, record type key, and constituent names from the unnested type so that + // any indexes defined on it pass local validation + final RecordMetaDataProto.UnnestedRecordType.NestedConstituent outer = unnestedType.getNestedConstituents(0); + final RecordMetaDataProto.UnnestedRecordType.NestedConstituent middle = unnestedType.getNestedConstituents(1); + metaDataBuilder.addJoinedRecordTypesBuilder() + .setName(unnestedType.getName()) + .setRecordTypeKey(unnestedType.getRecordTypeKey()) + .addJoinConstituents(RecordMetaDataProto.JoinedRecordType.JoinConstituent.newBuilder() + .setName(outer.getName()) + .setRecordType(outer.getTypeName()) + .setOuterJoined(false)) + .addJoinConstituents(RecordMetaDataProto.JoinedRecordType.JoinConstituent.newBuilder() + .setName(middle.getName()) + .setRecordType("MiddleRecord") + .setOuterJoined(false)) + .addJoins(RecordMetaDataProto.JoinedRecordType.Join.newBuilder() + .setLeft(outer.getName()) + .setLeftExpression(Key.Expressions.field("middle").nest("other_int").toKeyExpression()) + .setRight(middle.getName()) + .setRightExpression(Key.Expressions.field("other_middle").nest("other_int").toKeyExpression())); + }); + + assertInvalid("synthetic record type changed type", metaData1, metaData2); + + // Should also be a problem if we change back + final RecordMetaData metaData3 = updateMetaData(metaData1, metaDataBuilder -> metaDataBuilder.setVersion(metaData2.getVersion() + 1)); + assertInvalid("synthetic record type changed type", metaData2, metaData3); + } + // Former index tests @Test From d9f73c932d5e73406d8336398832c47b89cde555 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 5 May 2026 12:49:06 +0100 Subject: [PATCH 7/7] some relatively small cleanup fixes --- .../record/metadata/MetaDataEvolutionValidator.java | 6 +++--- .../record/metadata/MetaDataEvolutionValidatorTest.java | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java index 218ff6d3dc..bd16aca3cc 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java @@ -1077,8 +1077,8 @@ public boolean allowsFieldRenames() { * not allowed as it can result in unknown fields during Protobuf deserialization. For that reason, * deprecating fields should be preferred to deleting them. Once a field is deprecated and no longer * actively accessed, further modifications to the name of the now unused field may be safe. - * Note that this option will result in accepting a meta-data change if a field is deprecated - * in the same change that deprecates it. Additionally, if this validator {@link #allowsUndeprecatingFields()}, + * Note that this option will result in accepting a meta-data change if a field is renamed + * and deprecated in the same change. Additionally, if this validator {@link #allowsUndeprecatingFields()}, * then this also allows deprecated fields to be renamed in the same change in which they are * marked as no longer deprecated. * @@ -1109,7 +1109,7 @@ public boolean allowsDeprecatedFieldRenames() { /** * Whether this validator allows any kind of field renames. It captures whether either this validator - * {@link #allowsFieldRenames} or {@link #allowsDeprecatedFieldRenames()}. If this is false, then + * {@link #allowsFieldRenames()} or {@link #allowsDeprecatedFieldRenames()}. If this is false, then * this validator will throw if any kind of field changes its name. The exact set of field renames * that are allowed will depend on the values of the other two configuration parameters. * 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 02a5c97c50..6a8e235b0c 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 @@ -140,7 +140,7 @@ public FieldRenameChecker(MetaDataEvolutionValidator baseValidator) { noRenamesValidator = builder .setAllowDeprecatedFieldRenames(false) - .setAllowDeprecatedFieldRenames(false) + .setAllowFieldRenames(false) .build(); assertFalse(noRenamesValidator.allowsAnyFieldRenames()); assertFalse(noRenamesValidator.allowsDeprecatedFieldRenames());