From c5e8dcc6b1fc7d706736fa4ba4b4caecf8b16a52 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 28 Apr 2026 14:41:39 +0100 Subject: [PATCH] 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..d3e41581a0 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 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. + * + *

+ * 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..d0339971d0 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) + .setAllowFieldRenames(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