Skip to content

Commit 18220d8

Browse files
authored
Improve MetaDataProtoEditor handling of unqualified types and add support for renaming all the types (#3402)
This does two things: - It adds a new `MetaDataProtoEditor.renameRecordTypes` that takes a function to rename all the types. - When trying to add tests for that, I discovered #3428 and resolved it. The solution here is that it converts the protobuf back into a `Descriptors.FileDescriptor`, and lets that do the type resolution. This requires you to be able to get the dependencies in order to do these operations, but it means that we don't have to try to re-implement protobuf's type resolution. And don't be too afraid about the lines changed, it is mostly new json files to try and test more of the rename logic. Historical note: I didn't realize MetaDataProtoEditor existed, so I wrote my own suite of tests, and implementation. Then discovered and tried to merge, at which point I discovered that it couldn't handle the unqualified types in my protobufs, and thus went about fixing it. In hindsight, I should have stopped to fix the issue about ambiguous types and come back to the new method, but I was using the new tests to validate it. Resolves: #3428
1 parent e1e6be9 commit 18220d8

26 files changed

+2164
-552
lines changed

fdb-record-layer-core/fdb-record-layer-core.gradle

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ dependencies {
4747
testImplementation(libs.apache.commonsMath3)
4848
testImplementation(libs.jline)
4949
testImplementation(libs.junit.platform)
50+
testImplementation(libs.protobuf.util)
5051
testCompileOnly(libs.spotbugs.annotations)
5152

5253
testAnnotationProcessor(libs.autoService)

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

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -381,14 +381,20 @@ public RecordMetaDataBuilder setRecords(@Nonnull RecordMetaDataProto.MetaData me
381381
throw new MetaDataException("Records already set.");
382382
}
383383
// Build the recordDescriptor by de-serializing the metaData proto
384+
final Descriptors.FileDescriptor[] dependencies = getDependencies(metaDataProto, this.explicitDependencies);
385+
loadFromProto(metaDataProto, dependencies, processExtensionOptions);
386+
return this;
387+
}
388+
389+
@API(API.Status.INTERNAL)
390+
public static Descriptors.FileDescriptor[] getDependencies(@Nonnull final RecordMetaDataProto.MetaData metaDataProto,
391+
@Nonnull final Map<String, Descriptors.FileDescriptor> explicitDependencies) {
384392
Map<String, DescriptorProtos.FileDescriptorProto> protoDependencies = new TreeMap<>();
385393
for (DescriptorProtos.FileDescriptorProto dependency : metaDataProto.getDependenciesList()) {
386394
protoDependencies.put(dependency.getName(), dependency);
387395
}
388396
Map<String, Descriptors.FileDescriptor> generatedDependencies = initGeneratedDependencies(protoDependencies);
389-
Descriptors.FileDescriptor[] dependencies = getDependencies(metaDataProto.getRecords(), generatedDependencies, protoDependencies);
390-
loadFromProto(metaDataProto, dependencies, processExtensionOptions);
391-
return this;
397+
return getDependencies(metaDataProto.getRecords(), generatedDependencies, protoDependencies, explicitDependencies);
392398
}
393399

394400
/**
@@ -582,26 +588,27 @@ public RecordMetaDataBuilder addDependencies(@Nonnull Descriptors.FileDescriptor
582588
return this;
583589
}
584590

585-
private Descriptors.FileDescriptor[] getDependencies(@Nonnull DescriptorProtos.FileDescriptorProto proto,
586-
@Nonnull Map<String, Descriptors.FileDescriptor> generatedDependencies,
587-
@Nonnull Map<String, DescriptorProtos.FileDescriptorProto> protoDependencies) {
591+
private static Descriptors.FileDescriptor[] getDependencies(@Nonnull DescriptorProtos.FileDescriptorProto proto,
592+
@Nonnull Map<String, Descriptors.FileDescriptor> generatedDependencies,
593+
@Nonnull Map<String, DescriptorProtos.FileDescriptorProto> protoDependencies,
594+
@Nonnull Map<String, Descriptors.FileDescriptor> explicitDependencies) {
588595
if (proto.getDependencyCount() == 0) {
589596
return emptyDependencyList;
590597
}
591598

592599
Descriptors.FileDescriptor[] dependencies = new Descriptors.FileDescriptor[proto.getDependencyCount()];
593600
for (int index = 0; index < proto.getDependencyCount(); index++) {
594601
String key = proto.getDependency(index);
595-
if (this.explicitDependencies.containsKey(key)) {
602+
if (explicitDependencies.containsKey(key)) {
596603
// Provided by caller.
597-
dependencies[index] = this.explicitDependencies.get(key);
604+
dependencies[index] = explicitDependencies.get(key);
598605
} else if (generatedDependencies.containsKey(key)) {
599606
// Generated already.
600607
dependencies[index] = generatedDependencies.get(key);
601608
} else if (protoDependencies.containsKey(key)) {
602609
// Not seen before. Build it.
603610
DescriptorProtos.FileDescriptorProto dependency = protoDependencies.get(key);
604-
dependencies[index] = buildFileDescriptor(dependency, getDependencies(dependency, generatedDependencies, protoDependencies));
611+
dependencies[index] = buildFileDescriptor(dependency, getDependencies(dependency, generatedDependencies, protoDependencies, explicitDependencies));
605612
generatedDependencies.put(key, dependencies[index]);
606613
} else {
607614
// Unknown dependency.
@@ -937,8 +944,9 @@ private void protoFieldOptions(RecordTypeBuilder recordType, Descriptors.FieldDe
937944
}
938945
}
939946

940-
private static Descriptors.FileDescriptor buildFileDescriptor(@Nonnull DescriptorProtos.FileDescriptorProto fileDescriptorProto,
941-
@Nonnull Descriptors.FileDescriptor[] dependencies) {
947+
@API(API.Status.INTERNAL)
948+
public static Descriptors.FileDescriptor buildFileDescriptor(@Nonnull DescriptorProtos.FileDescriptorProto fileDescriptorProto,
949+
@Nonnull Descriptors.FileDescriptor[] dependencies) {
942950
try {
943951
return Descriptors.FileDescriptor.buildFrom(fileDescriptorProto, dependencies);
944952
} catch (Descriptors.DescriptorValidationException ex) {

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/logging/LogMessageKeys.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ public enum LogMessageKeys {
4747
OLD,
4848
NEW,
4949
MESSAGE,
50+
NAME,
5051
CODE,
5152
DESCRIPTION,
5253
UNKNOWN_FIELDS,

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,10 @@
3737
* A <i>synthetic</i> record type is made up of other record types and not actually stored separately in the record store.
3838
* It can, however, be indexed, by passing it to an {@link com.apple.foundationdb.record.provider.foundationdb.IndexMaintainer}
3939
* that will evaluate {@link KeyExpression}s against it.
40+
* Note: If you are adding a new implementation of this, check out
41+
* {@link com.apple.foundationdb.record.provider.foundationdb.MetaDataProtoEditor} to make sure it will appropriately
42+
* update the type names in the synthetic type.
43+
*
4044
* @param <C> type of constituent record types
4145
*/
4246
@API(API.Status.EXPERIMENTAL)

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,8 +166,9 @@ public UnnestedRecordTypeBuilder(@Nonnull RecordMetaDataProto.UnnestedRecordType
166166
}
167167
}
168168

169+
@API(API.Status.INTERNAL)
169170
@Nullable
170-
private static Descriptors.Descriptor findDescriptorByName(@Nonnull Descriptors.FileDescriptor fileDescriptor, @Nonnull String fullName) {
171+
public static Descriptors.Descriptor findDescriptorByName(@Nonnull Descriptors.FileDescriptor fileDescriptor, @Nonnull String fullName) {
171172
// Use the seen set to protect against circular dependencies. Files should be added to the set right before they are searched through, so
172173
// each file will be visited at most once
173174
Set<Descriptors.FileDescriptor> seen = new HashSet<>();

0 commit comments

Comments
 (0)