diff --git a/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs b/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs index 29a376e049..9abee95126 100644 --- a/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs +++ b/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs @@ -30,10 +30,11 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #endregion -using System.Linq; using Google.Protobuf.TestProtos; using NUnit.Framework; -using UnitTest.Issues.TestProtos; +using System; +using System.Collections.Generic; +using System.Linq; namespace Google.Protobuf.Reflection { @@ -44,9 +45,32 @@ namespace Google.Protobuf.Reflection public class DescriptorsTest { [Test] - public void FileDescriptor() + public void FileDescriptor_GeneratedCode() + { + TestFileDescriptor( + UnittestProto3Reflection.Descriptor, + UnittestImportProto3Reflection.Descriptor, + UnittestImportPublicProto3Reflection.Descriptor); + } + + [Test] + public void FileDescriptor_BuildFromByteStrings() + { + // The descriptors have to be supplied in an order such that all the + // dependencies come before the descriptors depending on them. + var descriptorData = new List + { + UnittestImportPublicProto3Reflection.Descriptor.Proto.ToByteString(), + UnittestImportProto3Reflection.Descriptor.Proto.ToByteString(), + UnittestProto3Reflection.Descriptor.Proto.ToByteString() + }; + var converted = FileDescriptor.BuildFromByteStrings(descriptorData); + Assert.AreEqual(3, converted.Count); + TestFileDescriptor(converted[2], converted[1], converted[0]); + } + + private void TestFileDescriptor(FileDescriptor file, FileDescriptor importedFile, FileDescriptor importedPublicFile) { - FileDescriptor file = UnittestProto3Reflection.Descriptor; Assert.AreEqual("unittest_proto3.proto", file.Name); Assert.AreEqual("protobuf_unittest3", file.Package); @@ -56,17 +80,12 @@ namespace Google.Protobuf.Reflection // unittest_proto3.proto doesn't have any public imports, but unittest_import_proto3.proto does. Assert.AreEqual(0, file.PublicDependencies.Count); - Assert.AreEqual(1, UnittestImportProto3Reflection.Descriptor.PublicDependencies.Count); - Assert.AreEqual(UnittestImportPublicProto3Reflection.Descriptor, UnittestImportProto3Reflection.Descriptor.PublicDependencies[0]); + Assert.AreEqual(1, importedFile.PublicDependencies.Count); + Assert.AreEqual(importedPublicFile, importedFile.PublicDependencies[0]); Assert.AreEqual(1, file.Dependencies.Count); - Assert.AreEqual(UnittestImportProto3Reflection.Descriptor, file.Dependencies[0]); + Assert.AreEqual(importedFile, file.Dependencies[0]); - MessageDescriptor messageType = TestAllTypes.Descriptor; - Assert.AreSame(typeof(TestAllTypes), messageType.ClrType); - Assert.AreSame(TestAllTypes.Parser, messageType.Parser); - Assert.AreEqual(messageType, file.MessageTypes[0]); - Assert.AreEqual(messageType, file.FindTypeByName("TestAllTypes")); Assert.Null(file.FindTypeByName("NoSuchType")); Assert.Null(file.FindTypeByName("protobuf_unittest3.TestAllTypes")); for (int i = 0; i < file.MessageTypes.Count; i++) @@ -77,8 +96,8 @@ namespace Google.Protobuf.Reflection Assert.AreEqual(file.EnumTypes[0], file.FindTypeByName("ForeignEnum")); Assert.Null(file.FindTypeByName("NoSuchType")); Assert.Null(file.FindTypeByName("protobuf_unittest3.ForeignEnum")); - Assert.AreEqual(1, UnittestImportProto3Reflection.Descriptor.EnumTypes.Count); - Assert.AreEqual("ImportEnum", UnittestImportProto3Reflection.Descriptor.EnumTypes[0].Name); + Assert.AreEqual(1, importedFile.EnumTypes.Count); + Assert.AreEqual("ImportEnum", importedFile.EnumTypes[0].Name); for (int i = 0; i < file.EnumTypes.Count; i++) { Assert.AreEqual(i, file.EnumTypes[i].Index); @@ -97,6 +116,56 @@ namespace Google.Protobuf.Reflection Assert.AreEqual("protobuf_unittest", file.Package); } + [Test] + public void FileDescriptor_BuildFromByteStrings_MissingDependency() + { + var descriptorData = new List + { + UnittestImportProto3Reflection.Descriptor.Proto.ToByteString(), + UnittestProto3Reflection.Descriptor.Proto.ToByteString(), + }; + // This will fail, because we're missing UnittestImportPublicProto3Reflection + Assert.Throws(() => FileDescriptor.BuildFromByteStrings(descriptorData)); + } + + [Test] + public void FileDescriptor_BuildFromByteStrings_DuplicateNames() + { + var descriptorData = new List + { + UnittestImportPublicProto3Reflection.Descriptor.Proto.ToByteString(), + UnittestImportPublicProto3Reflection.Descriptor.Proto.ToByteString(), + }; + // This will fail due to the same name being used twice + Assert.Throws(() => FileDescriptor.BuildFromByteStrings(descriptorData)); + } + + [Test] + public void FileDescriptor_BuildFromByteStrings_IncorrectOrder() + { + var descriptorData = new List + { + UnittestProto3Reflection.Descriptor.Proto.ToByteString(), + UnittestImportPublicProto3Reflection.Descriptor.Proto.ToByteString(), + UnittestImportProto3Reflection.Descriptor.Proto.ToByteString() + }; + // This will fail, because the dependencies should come first + Assert.Throws(() => FileDescriptor.BuildFromByteStrings(descriptorData)); + + } + + [Test] + public void MessageDescriptorFromGeneratedCodeFileDescriptor() + { + var file = UnittestProto3Reflection.Descriptor; + + MessageDescriptor messageType = TestAllTypes.Descriptor; + Assert.AreSame(typeof(TestAllTypes), messageType.ClrType); + Assert.AreSame(TestAllTypes.Parser, messageType.Parser); + Assert.AreEqual(messageType, file.MessageTypes[0]); + Assert.AreEqual(messageType, file.FindTypeByName("TestAllTypes")); + } + [Test] public void MessageDescriptor() { @@ -163,7 +232,7 @@ namespace Google.Protobuf.Reflection Assert.AreEqual("single_nested_enum", enumField.Name); Assert.AreEqual(FieldType.Enum, enumField.FieldType); - // Assert.AreEqual(TestAllTypes.Types.NestedEnum.DescriptorProtoFile, enumField.EnumType); + Assert.AreEqual(messageType.EnumTypes[0], enumField.EnumType); Assert.AreEqual("single_foreign_message", messageField.Name); Assert.AreEqual(FieldType.Message, messageField.FieldType); diff --git a/csharp/src/Google.Protobuf/Reflection/DescriptorPool.cs b/csharp/src/Google.Protobuf/Reflection/DescriptorPool.cs index 99ca4bf34f..9c2160feed 100644 --- a/csharp/src/Google.Protobuf/Reflection/DescriptorPool.cs +++ b/csharp/src/Google.Protobuf/Reflection/DescriptorPool.cs @@ -53,13 +53,13 @@ namespace Google.Protobuf.Reflection private readonly HashSet dependencies; - internal DescriptorPool(FileDescriptor[] dependencyFiles) + internal DescriptorPool(IEnumerable dependencyFiles) { dependencies = new HashSet(); - for (int i = 0; i < dependencyFiles.Length; i++) + foreach (var dependencyFile in dependencyFiles) { - dependencies.Add(dependencyFiles[i]); - ImportPublicDependencies(dependencyFiles[i]); + dependencies.Add(dependencyFile); + ImportPublicDependencies(dependencyFile); } foreach (FileDescriptor dependency in dependencyFiles) diff --git a/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs index 2a3d5c7a2d..152467d802 100644 --- a/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs +++ b/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs @@ -116,13 +116,18 @@ namespace Google.Protobuf.Reflection /// that is the responsibility of the accessor. /// /// - /// The value returned by this property will be non-null for all regular fields. However, - /// if a message containing a map field is introspected, the list of nested messages will include + /// In descriptors for generated code, the value returned by this property will be non-null for all + /// regular fields. However, if a message containing a map field is introspected, the list of nested messages will include /// an auto-generated nested key/value pair message for the field. This is not represented in any /// generated type, and the value of the map field itself is represented by a dictionary in the /// reflection API. There are never instances of those "hidden" messages, so no accessor is provided /// and this property will return null. /// + /// + /// In dynamically loaded descriptors, the value returned by this property will current be null; + /// if and when dynamic messages are supported, it will return a suitable accessor to work with + /// them. + /// /// public IFieldAccessor Accessor => accessor; @@ -330,7 +335,8 @@ namespace Google.Protobuf.Reflection private IFieldAccessor CreateAccessor() { // If we're given no property name, that's because we really don't want an accessor. - // (At the moment, that means it's a map entry message...) + // This could be because it's a map message, or it could be that we're loading a FileDescriptor dynamically. + // TODO: Support dynamic messages. if (propertyName == null) { return null; diff --git a/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs index be94cb100e..799f729106 100644 --- a/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs +++ b/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs @@ -34,6 +34,7 @@ using Google.Protobuf.WellKnownTypes; using System; using System.Collections.Generic; using System.Collections.ObjectModel; +using System.Linq; namespace Google.Protobuf.Reflection { @@ -54,12 +55,12 @@ namespace Google.Protobuf.Reflection ForceReflectionInitialization(); } - private FileDescriptor(ByteString descriptorData, FileDescriptorProto proto, FileDescriptor[] dependencies, DescriptorPool pool, bool allowUnknownDependencies, GeneratedClrTypeInfo generatedCodeInfo) + private FileDescriptor(ByteString descriptorData, FileDescriptorProto proto, IEnumerable dependencies, DescriptorPool pool, bool allowUnknownDependencies, GeneratedClrTypeInfo generatedCodeInfo) { SerializedData = descriptorData; DescriptorPool = pool; Proto = proto; - Dependencies = new ReadOnlyCollection((FileDescriptor[]) dependencies.Clone()); + Dependencies = new ReadOnlyCollection(dependencies.ToList()); PublicDependencies = DeterminePublicDependencies(this, proto, dependencies, allowUnknownDependencies); @@ -67,11 +68,11 @@ namespace Google.Protobuf.Reflection MessageTypes = DescriptorUtil.ConvertAndMakeReadOnly(proto.MessageType, (message, index) => - new MessageDescriptor(message, this, null, index, generatedCodeInfo.NestedTypes[index])); + new MessageDescriptor(message, this, null, index, generatedCodeInfo?.NestedTypes[index])); EnumTypes = DescriptorUtil.ConvertAndMakeReadOnly(proto.EnumType, (enumType, index) => - new EnumDescriptor(enumType, this, null, index, generatedCodeInfo.NestedEnums[index])); + new EnumDescriptor(enumType, this, null, index, generatedCodeInfo?.NestedEnums[index])); Services = DescriptorUtil.ConvertAndMakeReadOnly(proto.Service, (service, index) => @@ -98,13 +99,9 @@ namespace Google.Protobuf.Reflection /// Extracts public dependencies from direct dependencies. This is a static method despite its /// first parameter, as the value we're in the middle of constructing is only used for exceptions. /// - private static IList DeterminePublicDependencies(FileDescriptor @this, FileDescriptorProto proto, FileDescriptor[] dependencies, bool allowUnknownDependencies) + private static IList DeterminePublicDependencies(FileDescriptor @this, FileDescriptorProto proto, IEnumerable dependencies, bool allowUnknownDependencies) { - var nameToFileMap = new Dictionary(); - foreach (var file in dependencies) - { - nameToFileMap[file.Name] = file; - } + var nameToFileMap = dependencies.ToDictionary(file => file.Name); var publicDependencies = new List(); for (int i = 0; i < proto.PublicDependency.Count; i++) { @@ -114,8 +111,7 @@ namespace Google.Protobuf.Reflection throw new DescriptorValidationException(@this, "Invalid public dependency index."); } string name = proto.Dependency[index]; - FileDescriptor file = nameToFileMap[name]; - if (file == null) + if (!nameToFileMap.TryGetValue(name, out var file)) { if (!allowUnknownDependencies) { @@ -315,6 +311,49 @@ namespace Google.Protobuf.Reflection } } + /// + /// Converts the given descriptor binary data into FileDescriptor objects. + /// Note: reflection using the returned FileDescriptors is not currently supported. + /// + /// The binary file descriptor proto data. Must not be null, and any + /// dependencies must come before the descriptor which depends on them. (If A depends on B, and B + /// depends on C, then the descriptors must be presented in the order C, B, A.) This is compatible + /// with the order in which protoc provides descriptors to plugins. + /// The file descriptors corresponding to . + public static IReadOnlyList BuildFromByteStrings(IEnumerable descriptorData) + { + ProtoPreconditions.CheckNotNull(descriptorData, nameof(descriptorData)); + + // TODO: See if we can build a single DescriptorPool instead of building lots of them. + // This will all behave correctly, but it's less efficient than we'd like. + var descriptors = new List(); + var descriptorsByName = new Dictionary(); + foreach (var data in descriptorData) + { + var proto = FileDescriptorProto.Parser.ParseFrom(data); + var dependencies = new List(); + foreach (var dependencyName in proto.Dependency) + { + if (!descriptorsByName.TryGetValue(dependencyName, out var dependency)) + { + throw new ArgumentException($"Dependency missing: {dependencyName}"); + } + dependencies.Add(dependency); + } + var pool = new DescriptorPool(dependencies); + FileDescriptor descriptor = new FileDescriptor( + data, proto, dependencies, pool, + allowUnknownDependencies: false, generatedCodeInfo: null); + descriptors.Add(descriptor); + if (descriptorsByName.ContainsKey(descriptor.Name)) + { + throw new ArgumentException($"Duplicate descriptor name: {descriptor.Name}"); + } + descriptorsByName.Add(descriptor.Name, descriptor); + } + return new ReadOnlyCollection(descriptors); + } + /// /// Returns a that represents this instance. /// diff --git a/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs index 86942acc02..dbb6768b6a 100644 --- a/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs +++ b/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs @@ -72,23 +72,21 @@ namespace Google.Protobuf.Reflection ClrType = generatedCodeInfo?.ClrType; ContainingType = parent; - // Note use of generatedCodeInfo. rather than generatedCodeInfo?. here... we don't expect - // to see any nested oneofs, types or enums in "not actually generated" code... we do - // expect fields though (for map entry messages). + // If generatedCodeInfo is null, we just won't generate an accessor for any fields. Oneofs = DescriptorUtil.ConvertAndMakeReadOnly( proto.OneofDecl, (oneof, index) => - new OneofDescriptor(oneof, file, this, index, generatedCodeInfo.OneofNames[index])); + new OneofDescriptor(oneof, file, this, index, generatedCodeInfo?.OneofNames[index])); NestedTypes = DescriptorUtil.ConvertAndMakeReadOnly( proto.NestedType, (type, index) => - new MessageDescriptor(type, file, this, index, generatedCodeInfo.NestedTypes[index])); + new MessageDescriptor(type, file, this, index, generatedCodeInfo?.NestedTypes[index])); EnumTypes = DescriptorUtil.ConvertAndMakeReadOnly( proto.EnumType, (type, index) => - new EnumDescriptor(type, file, this, index, generatedCodeInfo.NestedEnums[index])); + new EnumDescriptor(type, file, this, index, generatedCodeInfo?.NestedEnums[index])); fieldsInDeclarationOrder = DescriptorUtil.ConvertAndMakeReadOnly( proto.Field, diff --git a/csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs index 5906c2e36d..c026bea677 100644 --- a/csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs +++ b/csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs @@ -85,6 +85,16 @@ namespace Google.Protobuf.Reflection /// Gets an accessor for reflective access to the values associated with the oneof /// in a particular message. /// + /// + /// + /// In descriptors for generated code, the value returned by this property will always be non-null. + /// + /// + /// In dynamically loaded descriptors, the value returned by this property will current be null; + /// if and when dynamic messages are supported, it will return a suitable accessor to work with + /// them. + /// + /// /// /// The accessor used for reflective access. /// @@ -110,6 +120,12 @@ namespace Google.Protobuf.Reflection private OneofAccessor CreateAccessor(string clrName) { + // We won't have a CLR name if this is from a dynamically-loaded FileDescriptor. + // TODO: Support dynamic messages. + if (clrName == null) + { + return null; + } var caseProperty = containingType.ClrType.GetProperty(clrName + "Case"); if (caseProperty == null) {