Skip to content

Commit 7067ab0

Browse files
alecgrieseralecgrieser
authored
Fix unknown type error that the MetaDataEvolutionValidator could hit when validating index evolution on renamed types (#4475)
This closes a hole in the `MetaDataEvolutionValidator`. The exact problem is that when validating an index, we'd check the old meta-data for record types based on the _new_ type's name. We'd only do this if field renames are allowed (either in general or of deprecated only fields), so this required the following confluence of events: 1. There are types that were renamed between two versions the meta-data 1. There are indexes on one of those renamed types 1. Field renames are allowed (potentially only on deprecated types) If the new type name was not present in the original meta-data, this would result in an error. In theory, if the type was present but just referred to a different type (e.g., two types swapped names), then this would result in incorrect validation occurring. This fixes #4474.
1 parent f05475d commit 7067ab0

3 files changed

Lines changed: 179 additions & 3 deletions

File tree

‎fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -695,9 +695,9 @@ private void validateIndex(@Nonnull RecordMetaData oldMetaData, @Nonnull Index o
695695
// The index root expression must be the same, modulo field renames
696696
KeyExpression expectedKeyExpression = null;
697697
if (allowsAnyFieldRenames()) {
698-
for (String oldRecordTypeName : oldRecordTypeNames) {
699-
final Descriptor oldDescriptor = oldMetaData.getRecordType(oldRecordTypeName).getDescriptor();
700-
final Descriptor newDescriptor = newMetaData.getRecordType(typeRenames.getOrDefault(oldRecordTypeName, oldRecordTypeName)).getDescriptor();
698+
for (RecordType oldRecordType : oldMetaData.recordTypesForIndex(oldIndex)) {
699+
final Descriptor oldDescriptor = oldRecordType.getDescriptor();
700+
final Descriptor newDescriptor = newMetaData.getRecordType(typeRenames.getOrDefault(oldRecordType.getName(), oldRecordType.getName())).getDescriptor();
701701
final KeyExpression renamedKeyExpression = RenameFieldsVisitor.renameFields(oldIndex.getRootExpression(), oldDescriptor, newDescriptor);
702702
if (expectedKeyExpression == null) {
703703
expectedKeyExpression = renamedKeyExpression;

‎fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import com.apple.foundationdb.record.TestRecordsEnumProto;
3232
import com.apple.foundationdb.record.TestRecordsIdenticalTypesProto;
3333
import com.apple.foundationdb.record.TestRecordsMultidimensionalProto;
34+
import com.apple.foundationdb.record.TestRecordsSwapProto;
3435
import com.apple.foundationdb.record.TestRecordsWithHeaderProto;
3536
import com.apple.foundationdb.record.evolution.TestHeaderAsGroupProto;
3637
import com.apple.foundationdb.record.evolution.TestMergedNestedTypesProto;
@@ -1123,6 +1124,124 @@ void renameFieldInPrimaryKey(boolean deprecated) {
11231124
fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3);
11241125
}
11251126

1127+
@ParameterizedTest
1128+
@MethodSource("deprecatedArgs")
1129+
void renameFieldOnReplacedTypeWithIndexes(boolean deprecated) {
1130+
final RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor());
1131+
1132+
// Mutate the original file change the following:
1133+
// 1. The MySimpleRecord type is renamed MySimpleRecord__Old
1134+
// 2. Two fields, num_value_3_indexed and num_value_2, are renamed on MySimpleRecord__Old
1135+
// 3. A new type, also named MySimpleRecord, is added with all the fields from the original MySimpleRecord
1136+
// 4. Update the RecordTypeUnion to account for the type name and new record type
1137+
final FileDescriptor mutatedFile = mutateFile(fileBuilder -> {
1138+
DescriptorProtos.DescriptorProto.Builder newType = DescriptorProtos.DescriptorProto.newBuilder()
1139+
.setName("MySimpleRecord");
1140+
1141+
for (DescriptorProtos.DescriptorProto.Builder descriptorProto : fileBuilder.getMessageTypeBuilderList()) {
1142+
if (descriptorProto.getName().equals("MySimpleRecord")) {
1143+
descriptorProto.setName("MySimpleRecord__Old");
1144+
descriptorProto.getFieldList().forEach(newType::addField);
1145+
for (DescriptorProtos.FieldDescriptorProto.Builder field : descriptorProto.getFieldBuilderList()) {
1146+
if (field.getName().equals("num_value_3_indexed") || field.getName().equals("num_value_2")) {
1147+
field.setName(field.getName() + "__old");
1148+
if (deprecated) {
1149+
field.getOptionsBuilder().setDeprecated(true);
1150+
}
1151+
}
1152+
}
1153+
} else if (descriptorProto.getName().equals(RecordMetaDataBuilder.DEFAULT_UNION_NAME)) {
1154+
for (DescriptorProtos.FieldDescriptorProto.Builder fieldProto : descriptorProto.getFieldBuilderList()) {
1155+
if (fieldProto.getTypeName().endsWith("MySimpleRecord")) {
1156+
fieldProto.setTypeName("MySimpleRecord__Old");
1157+
fieldProto.setName(fieldProto.getName() + "__Old");
1158+
}
1159+
}
1160+
addField(descriptorProto)
1161+
.setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL)
1162+
.setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_MESSAGE)
1163+
.setTypeName("MySimpleRecord")
1164+
.setName("_MySimpleRecord");
1165+
}
1166+
}
1167+
1168+
fileBuilder.addMessageType(newType);
1169+
});
1170+
// Construct a meta-data object to match the new type
1171+
final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, mutatedFile, metaData -> {
1172+
for (RecordMetaDataProto.RecordType.Builder recordType : metaData.getRecordTypesBuilderList()) {
1173+
if (recordType.getName().equals("MySimpleRecord")) {
1174+
recordType.setName("MySimpleRecord__Old");
1175+
}
1176+
}
1177+
metaData.addRecordTypesBuilder()
1178+
.setName("MySimpleRecord")
1179+
.setPrimaryKey(Key.Expressions.field("rec_no").toKeyExpression())
1180+
.setSinceVersion(metaData.getVersion());
1181+
});
1182+
1183+
// Should be invalid. The indexes that were previously on the old record type have been moved over to the new
1184+
// type
1185+
fieldRenameChecker.assertInvalidRenaming("new index removes record type", deprecated, metaData1, metaData2);
1186+
1187+
// Rename the record type in the references held by each index on the original MySimpleRecord.
1188+
// Note that the MySimpleRecord$num_value_3_indexed index also needs to update its field. While doing that,
1189+
// change the field name incorrectly, and validate that we catch that.
1190+
RecordMetaData metaData3 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", index ->
1191+
index.toBuilder().clearRecordType().addRecordType("MySimpleRecord__Old").build());
1192+
metaData3 = replaceIndex(metaData3, "MySimpleRecord$num_value_unique", index ->
1193+
index.toBuilder().clearRecordType().addRecordType("MySimpleRecord__Old").build());
1194+
metaData3 = replaceIndex(metaData3, "MySimpleRecord$num_value_3_indexed", index ->
1195+
index.toBuilder().clearRecordType().addRecordType("MySimpleRecord__Old").setRootExpression(Key.Expressions.field("num_value_2__old").toKeyExpression()).build());
1196+
1197+
fieldRenameChecker.assertInvalidRenaming("index key expression does not match required", deprecated, metaData1, metaData3);
1198+
1199+
// Correct the field renaming on that final index
1200+
final RecordMetaData metaData4 = replaceIndex(metaData3, "MySimpleRecord$num_value_3_indexed", index ->
1201+
index.toBuilder().setRootExpression(Key.Expressions.field("num_value_3_indexed__old").toKeyExpression()).build());
1202+
fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData4);
1203+
}
1204+
1205+
@Test
1206+
void swapTypesWithIndexesRequiresRenamingFields() {
1207+
final RecordMetaData metaData1 = RecordMetaData.build(TestRecordsSwapProto.getDescriptor());
1208+
1209+
// The two types, TypeAlpha and TypeBeta, have the same field names, but in different orders.
1210+
// Swapping the types should therefore be okay, but it will require renaming the referenced
1211+
// fields within the indexes.
1212+
1213+
final FileDescriptor mutatedDescriptor = mutateMessageType("Union", TestRecordsSwapProto.getDescriptor(), unionTypeBuilder -> {
1214+
for (DescriptorProtos.FieldDescriptorProto.Builder field : unionTypeBuilder.getFieldBuilderList()) {
1215+
// Swap the union descriptor fields for TypeAlpha and TypeBeta
1216+
if (field.getNumber() == TestRecordsSwapProto.Union._TYPEALPHA_FIELD_NUMBER) {
1217+
field.setNumber(TestRecordsSwapProto.Union._TYPEBETA_FIELD_NUMBER);
1218+
} else if (field.getNumber() == TestRecordsSwapProto.Union._TYPEBETA_FIELD_NUMBER) {
1219+
field.setNumber(TestRecordsSwapProto.Union._TYPEALPHA_FIELD_NUMBER);
1220+
}
1221+
}
1222+
});
1223+
1224+
// Initial evolution validation fails because the indexes all swap types (because their
1225+
// referenced type names still point to the old types)
1226+
final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, mutatedDescriptor);
1227+
fieldRenameChecker.assertInvalidRenaming("new index removes record type", false, metaData1, metaData2);
1228+
1229+
// Fix the types on each index. This still fails to validate as the index root expressions will
1230+
// still use the incorrect names
1231+
RecordMetaData metaData3 = replaceIndex(metaData2, "TypeAlpha$bar", index ->
1232+
index.toBuilder().clearRecordType().addRecordType("TypeBeta").build());
1233+
metaData3 = replaceIndex(metaData3, "TypeBeta$bar", index ->
1234+
index.toBuilder().clearRecordType().addRecordType("TypeAlpha").build());
1235+
fieldRenameChecker.assertInvalidRenaming("index key expression does not match required", false, metaData1, metaData3);
1236+
1237+
// Rename the fields referenced in the index. After that, the evolution should be allowed
1238+
RecordMetaData metaData4 = replaceIndex(metaData3, "TypeAlpha$bar", index ->
1239+
index.toBuilder().setRootExpression(Key.Expressions.field("baz").toKeyExpression()).build());
1240+
metaData4 = replaceIndex(metaData4, "TypeBeta$bar", index ->
1241+
index.toBuilder().setRootExpression(Key.Expressions.field("foo").toKeyExpression()).build());
1242+
fieldRenameChecker.assertValidRenaming(false, metaData1, metaData4);
1243+
}
1244+
11261245
@Test
11271246
void deprecateField() {
11281247
FileDescriptor deprecatedFile = mutateField("MySimpleRecord", "str_value_indexed",

‎fdb-record-layer-core/src/test/proto/test_records_swap.proto‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
/*
2+
* test_records_swap.proto
3+
*
4+
* This source file is part of the FoundationDB open source project
5+
*
6+
* Copyright 2015-2026 Apple Inc. and the FoundationDB project authors
7+
*
8+
* Licensed under the Apache License, Version 2.0 (the "License");
9+
* you may not use this file except in compliance with the License.
10+
* You may obtain a copy of the License at
11+
*
12+
* http://www.apache.org/licenses/LICENSE-2.0
13+
*
14+
* Unless required by applicable law or agreed to in writing, software
15+
* distributed under the License is distributed on an "AS IS" BASIS,
16+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
17+
* See the License for the specific language governing permissions and
18+
* limitations under the License.
19+
*/
20+
syntax = "proto2";
21+
22+
package com.apple.foundationdb.record.transform;
23+
24+
option java_package = "com.apple.foundationdb.record";
25+
option java_outer_classname = "TestRecordsSwapProto";
26+
27+
import "record_metadata_options.proto";
28+
29+
// Define two types. They are structurally identical in that
30+
// they have the same types by field index. However, the field
31+
// names are swapped around.
32+
//
33+
// This is designed to test meta-data evolutions where the field
34+
// type names are swapped. For that to be a legal evolution, the
35+
// index root expressions must also be renamed in order to respect
36+
// the new names.
37+
38+
39+
message TypeAlpha {
40+
optional int64 rec_no = 1 [(field).primary_key = true];
41+
optional int32 foo = 2;
42+
optional string bar = 3 [(field).index = {}];
43+
repeated float baz = 4;
44+
}
45+
46+
message TypeBeta {
47+
optional int64 rec_no = 1 [(field).primary_key = true];
48+
optional int32 bar = 2 [(field).index = {}];
49+
optional string baz = 3;
50+
repeated float foo = 4;
51+
}
52+
53+
message Union {
54+
option (record).usage = UNION;
55+
optional TypeAlpha _TypeAlpha = 1;
56+
optional TypeBeta _TypeBeta = 2;
57+
}

0 commit comments

Comments
 (0)