From 97737072f9f6552fd5fa2bc4ca085793a9b708dd Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 17 Apr 2020 08:36:46 +0100 Subject: [PATCH] Add reflection support for proto3 optional fields This is more involved than might be expected because the synthetic oneofs don't generate the properties we would usually expect to see. --- .../Reflection/DescriptorsTest.cs | 50 +++++++++++++++++++ .../Reflection/FieldAccessTest.cs | 37 ++++++++++++++ .../Reflection/FieldDescriptor.cs | 6 +++ .../Reflection/IFieldAccessor.cs | 3 +- .../Reflection/MessageDescriptor.cs | 24 +++++++++ .../Reflection/OneofAccessor.cs | 47 +++++++++-------- .../Reflection/OneofDescriptor.cs | 40 +++++++++++---- .../Reflection/SingleFieldAccessor.cs | 36 +++++++++---- 8 files changed, 202 insertions(+), 41 deletions(-) diff --git a/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs b/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs index 482db535e2..ebb8394d23 100644 --- a/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs +++ b/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs @@ -32,6 +32,7 @@ using Google.Protobuf.TestProtos; using NUnit.Framework; +using ProtobufUnittest; using System; using System.Collections.Generic; using System.Linq; @@ -247,6 +248,7 @@ namespace Google.Protobuf.Reflection FieldDescriptor enumField = testAllTypesDescriptor.FindDescriptor("single_nested_enum"); FieldDescriptor foreignMessageField = testAllTypesDescriptor.FindDescriptor("single_foreign_message"); FieldDescriptor importMessageField = testAllTypesDescriptor.FindDescriptor("single_import_message"); + FieldDescriptor fieldInOneof = testAllTypesDescriptor.FindDescriptor("oneof_string"); Assert.AreEqual("single_int32", primitiveField.Name); Assert.AreEqual("protobuf_unittest3.TestAllTypes.single_int32", @@ -268,6 +270,10 @@ namespace Google.Protobuf.Reflection Assert.AreEqual("single_import_message", importMessageField.Name); Assert.AreEqual(FieldType.Message, importMessageField.FieldType); Assert.AreEqual(importMessageDescriptor, importMessageField.MessageType); + + // For a field in a regular onoef, ContainingOneof and RealContainingOneof should be the same. + Assert.AreEqual("oneof_field", fieldInOneof.ContainingOneof.Name); + Assert.AreSame(fieldInOneof.ContainingOneof, fieldInOneof.RealContainingOneof); } [Test] @@ -318,6 +324,7 @@ namespace Google.Protobuf.Reflection public void OneofDescriptor() { OneofDescriptor descriptor = TestAllTypes.Descriptor.FindDescriptor("oneof_field"); + Assert.IsFalse(descriptor.IsSynthetic); Assert.AreEqual("oneof_field", descriptor.Name); Assert.AreEqual("protobuf_unittest3.TestAllTypes.oneof_field", descriptor.FullName); @@ -383,5 +390,48 @@ namespace Google.Protobuf.Reflection var importingDescriptor = TestProtos.OldGenerator.OldExtensions1Reflection.Descriptor; Assert.NotNull(importingDescriptor); } + + [Test] + public void Proto3OptionalDescriptors() + { + var descriptor = TestProto3Optional.Descriptor; + var field = descriptor.Fields[TestProto3Optional.OptionalInt32FieldNumber]; + Assert.NotNull(field.ContainingOneof); + Assert.IsTrue(field.ContainingOneof.IsSynthetic); + Assert.Null(field.RealContainingOneof); + } + + + [Test] + public void SyntheticOneofReflection() + { + // Expect every oneof in TestProto3Optional to be synthetic + var proto3OptionalDescriptor = TestProto3Optional.Descriptor; + Assert.AreEqual(0, proto3OptionalDescriptor.RealOneofCount); + foreach (var oneof in proto3OptionalDescriptor.Oneofs) + { + Assert.True(oneof.IsSynthetic); + } + + // Expect no oneof in the original proto3 unit test file to be synthetic. + foreach (var descriptor in ProtobufTestMessages.Proto3.TestMessagesProto3Reflection.Descriptor.MessageTypes) + { + Assert.AreEqual(descriptor.Oneofs.Count, descriptor.RealOneofCount); + foreach (var oneof in descriptor.Oneofs) + { + Assert.False(oneof.IsSynthetic); + } + } + + // Expect no oneof in the original proto2 unit test file to be synthetic. + foreach (var descriptor in ProtobufTestMessages.Proto2.TestMessagesProto2Reflection.Descriptor.MessageTypes) + { + Assert.AreEqual(descriptor.Oneofs.Count, descriptor.RealOneofCount); + foreach (var oneof in descriptor.Oneofs) + { + Assert.False(oneof.IsSynthetic); + } + } + } } } diff --git a/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs b/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs index 9651ec30d8..0d4034c5b1 100644 --- a/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs +++ b/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs @@ -38,6 +38,7 @@ using System.Collections; using System.Collections.Generic; using static Google.Protobuf.TestProtos.Proto2.UnittestExtensions; +using ProtobufUnittest; namespace Google.Protobuf.Reflection { @@ -104,6 +105,21 @@ namespace Google.Protobuf.Reflection Assert.Throws(() => fields[TestProtos.TestAllTypes.SingleBoolFieldNumber].Accessor.HasValue(message)); } + [Test] + public void HasValue_Proto3Optional() + { + IMessage message = new TestProto3Optional + { + OptionalInt32 = 0, + LazyNestedMessage = new TestProto3Optional.Types.NestedMessage() + }; + var fields = message.Descriptor.Fields; + Assert.IsFalse(fields[TestProto3Optional.OptionalInt64FieldNumber].Accessor.HasValue(message)); + Assert.IsFalse(fields[TestProto3Optional.OptionalNestedMessageFieldNumber].Accessor.HasValue(message)); + Assert.IsTrue(fields[TestProto3Optional.LazyNestedMessageFieldNumber].Accessor.HasValue(message)); + Assert.IsTrue(fields[TestProto3Optional.OptionalInt32FieldNumber].Accessor.HasValue(message)); + } + [Test] public void HasValue() { @@ -225,6 +241,27 @@ namespace Google.Protobuf.Reflection Assert.AreEqual(0, mapMessage.MapStringString.Count); } + [Test] + public void Clear_Proto3Optional() + { + TestProto3Optional message = new TestProto3Optional + { + OptionalInt32 = 0, + OptionalNestedMessage = new TestProto3Optional.Types.NestedMessage() + }; + var primitiveField = TestProto3Optional.Descriptor.Fields[TestProto3Optional.OptionalInt32FieldNumber]; + var messageField = TestProto3Optional.Descriptor.Fields[TestProto3Optional.OptionalNestedMessageFieldNumber]; + + Assert.True(message.HasOptionalInt32); + Assert.NotNull(message.OptionalNestedMessage); + + primitiveField.Accessor.Clear(message); + messageField.Accessor.Clear(message); + + Assert.False(message.HasOptionalInt32); + Assert.Null(message.OptionalNestedMessage); + } + [Test] public void FieldDescriptor_ByName() { diff --git a/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs index ddd671aadb..69bab4f010 100644 --- a/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs +++ b/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs @@ -58,6 +58,12 @@ namespace Google.Protobuf.Reflection /// public OneofDescriptor ContainingOneof { get; } + /// + /// Returns the oneof containing this field if it's a "real" oneof, or null if either this + /// field is not part of a oneof, or the oneof is synthetic. + /// + public OneofDescriptor RealContainingOneof => ContainingOneof?.IsSynthetic == false ? ContainingOneof : null; + /// /// The effective JSON name for this field. This is usually the lower-camel-cased form of the field name, /// but can be overridden using the json_name option in the .proto file. diff --git a/csharp/src/Google.Protobuf/Reflection/IFieldAccessor.cs b/csharp/src/Google.Protobuf/Reflection/IFieldAccessor.cs index b48c4f9df5..d73427b73b 100644 --- a/csharp/src/Google.Protobuf/Reflection/IFieldAccessor.cs +++ b/csharp/src/Google.Protobuf/Reflection/IFieldAccessor.cs @@ -59,7 +59,8 @@ namespace Google.Protobuf.Reflection object GetValue(IMessage message); /// - /// Indicates whether the field in the specified message is set. For proto3 fields, this throws an + /// Indicates whether the field in the specified message is set. + /// For proto3 fields that aren't explicitly optional, this throws an /// bool HasValue(IMessage message); diff --git a/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs index eda1965c54..6217081fbc 100644 --- a/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs +++ b/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs @@ -80,6 +80,20 @@ namespace Google.Protobuf.Reflection (oneof, index) => new OneofDescriptor(oneof, file, this, index, generatedCodeInfo?.OneofNames[index])); + int syntheticOneofCount = 0; + foreach (var oneof in Oneofs) + { + if (oneof.IsSynthetic) + { + syntheticOneofCount++; + } + else if (syntheticOneofCount != 0) + { + throw new ArgumentException("All synthetic oneofs should come after real oneofs"); + } + } + RealOneofCount = Oneofs.Count - syntheticOneofCount; + NestedTypes = DescriptorUtil.ConvertAndMakeReadOnly( proto.NestedType, (type, index) => @@ -234,9 +248,19 @@ namespace Google.Protobuf.Reflection /// /// An unmodifiable list of the "oneof" field collections in this message type. + /// All "real" oneofs (where returns false) + /// come before synthetic ones. /// public IList Oneofs { get; } + /// + /// The number of real "oneof" descriptors in this message type. Every element in + /// with an index less than this will have a property value + /// of false; every element with an index greater than or equal to this will have a + /// property value of true. + /// + public int RealOneofCount { get; } + /// /// Finds a field by field name. /// diff --git a/csharp/src/Google.Protobuf/Reflection/OneofAccessor.cs b/csharp/src/Google.Protobuf/Reflection/OneofAccessor.cs index f4bf628a6f..4e040c17ea 100644 --- a/csharp/src/Google.Protobuf/Reflection/OneofAccessor.cs +++ b/csharp/src/Google.Protobuf/Reflection/OneofAccessor.cs @@ -43,19 +43,31 @@ namespace Google.Protobuf.Reflection { private readonly Func caseDelegate; private readonly Action clearDelegate; - private OneofDescriptor descriptor; - internal OneofAccessor(PropertyInfo caseProperty, MethodInfo clearMethod, OneofDescriptor descriptor) + private OneofAccessor(OneofDescriptor descriptor, Func caseDelegate, Action clearDelegate) { - if (!caseProperty.CanRead) - { - throw new ArgumentException("Cannot read from property"); - } - this.descriptor = descriptor; - caseDelegate = ReflectionUtil.CreateFuncIMessageInt32(caseProperty.GetGetMethod()); + Descriptor = descriptor; + this.caseDelegate = caseDelegate; + this.clearDelegate = clearDelegate; + } + + internal static OneofAccessor ForRegularOneof( + OneofDescriptor descriptor, + PropertyInfo caseProperty, + MethodInfo clearMethod) => + new OneofAccessor( + descriptor, + ReflectionUtil.CreateFuncIMessageInt32(caseProperty.GetGetMethod()), + ReflectionUtil.CreateActionIMessage(clearMethod)); - this.descriptor = descriptor; - clearDelegate = ReflectionUtil.CreateActionIMessage(clearMethod); + internal static OneofAccessor ForSyntheticOneof(OneofDescriptor descriptor) + { + // Note: descriptor.Fields will be null when this method is called, because we haven't + // cross-linked yet. But by the time the delgates are called by user code, all will be + // well. (That's why we capture the descriptor itself rather than a field.) + return new OneofAccessor(descriptor, + message => descriptor.Fields[0].Accessor.HasValue(message) ? descriptor.Fields[0].FieldNumber : 0, + message => descriptor.Fields[0].Accessor.Clear(message)); } /// @@ -64,15 +76,12 @@ namespace Google.Protobuf.Reflection /// /// The descriptor of the oneof. /// - public OneofDescriptor Descriptor { get { return descriptor; } } + public OneofDescriptor Descriptor { get; } /// /// Clears the oneof in the specified message. /// - public void Clear(IMessage message) - { - clearDelegate(message); - } + public void Clear(IMessage message) => clearDelegate(message); /// /// Indicates which field in the oneof is set for specified message @@ -80,11 +89,9 @@ namespace Google.Protobuf.Reflection public FieldDescriptor GetCaseFieldDescriptor(IMessage message) { int fieldNumber = caseDelegate(message); - if (fieldNumber > 0) - { - return descriptor.ContainingType.FindFieldByNumber(fieldNumber); - } - return null; + return fieldNumber > 0 + ? Descriptor.ContainingType.FindFieldByNumber(fieldNumber) + : null; } } } diff --git a/csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs index 1e30b92ed6..7cceabd7c3 100644 --- a/csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs +++ b/csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs @@ -33,6 +33,7 @@ using System; using System.Collections.Generic; using System.Collections.ObjectModel; +using System.Linq; using Google.Protobuf.Collections; using Google.Protobuf.Compatibility; @@ -54,8 +55,13 @@ namespace Google.Protobuf.Reflection { this.proto = proto; containingType = parent; - file.DescriptorPool.AddSymbol(this); + + // It's useful to determine whether or not this is a synthetic oneof before cross-linking. That means + // diving into the proto directly rather than using FieldDescriptor, but that's okay. + var firstFieldInOneof = parent.Proto.Field.FirstOrDefault(fieldProto => fieldProto.OneofIndex == index); + IsSynthetic = firstFieldInOneof?.Proto3Optional ?? false; + accessor = CreateAccessor(clrName); } @@ -83,6 +89,12 @@ namespace Google.Protobuf.Reflection /// public IList Fields { get { return fields; } } + /// + /// Returns true if this oneof is a synthetic oneof containing a proto3 optional field; + /// false otherwise. + /// + public bool IsSynthetic { get; } + /// /// Gets an accessor for reflective access to the values associated with the oneof /// in a particular message. @@ -146,18 +158,28 @@ namespace Google.Protobuf.Reflection { return null; } - var caseProperty = containingType.ClrType.GetProperty(clrName + "Case"); - if (caseProperty == null) + if (IsSynthetic) { - throw new DescriptorValidationException(this, $"Property {clrName}Case not found in {containingType.ClrType}"); + return OneofAccessor.ForSyntheticOneof(this); } - var clearMethod = containingType.ClrType.GetMethod("Clear" + clrName); - if (clearMethod == null) + else { - throw new DescriptorValidationException(this, $"Method Clear{clrName} not found in {containingType.ClrType}"); + var caseProperty = containingType.ClrType.GetProperty(clrName + "Case"); + if (caseProperty == null) + { + throw new DescriptorValidationException(this, $"Property {clrName}Case not found in {containingType.ClrType}"); + } + if (!caseProperty.CanRead) + { + throw new ArgumentException($"Cannot read from property {clrName}Case in {containingType.ClrType}"); + } + var clearMethod = containingType.ClrType.GetMethod("Clear" + clrName); + if (clearMethod == null) + { + throw new DescriptorValidationException(this, $"Method Clear{clrName} not found in {containingType.ClrType}"); + } + return OneofAccessor.ForRegularOneof(this, caseProperty, clearMethod); } - - return new OneofAccessor(caseProperty, clearMethod, this); } } } diff --git a/csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs b/csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs index de10226511..ed844bc51d 100644 --- a/csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs +++ b/csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs @@ -57,10 +57,11 @@ namespace Google.Protobuf.Reflection throw new ArgumentException("Not all required properties/methods available"); } setValueDelegate = ReflectionUtil.CreateActionIMessageObject(property.GetSetMethod()); - if (descriptor.File.Syntax == Syntax.Proto3) + if (descriptor.File.Syntax == Syntax.Proto3 && !descriptor.Proto.Proto3Optional) { - hasDelegate = message => { - throw new InvalidOperationException("HasValue is not implemented for proto3 fields"); + hasDelegate = message => + { + throw new InvalidOperationException("HasValue is not implemented for non-optional proto3 fields"); }; var clrType = property.PropertyType; @@ -74,16 +75,29 @@ namespace Google.Protobuf.Reflection } else { - MethodInfo hasMethod = property.DeclaringType.GetRuntimeProperty("Has" + property.Name).GetMethod; - if (hasMethod == null) { - throw new ArgumentException("Not all required properties/methods are available"); + // For message fields, just compare with null and set to null. + // For primitive fields, use the Has/Clear methods. + + if (descriptor.FieldType == FieldType.Message) + { + hasDelegate = message => GetValue(message) != null; + clearDelegate = message => SetValue(message, null); } - hasDelegate = ReflectionUtil.CreateFuncIMessageBool(hasMethod); - MethodInfo clearMethod = property.DeclaringType.GetRuntimeMethod("Clear" + property.Name, ReflectionUtil.EmptyTypes); - if (clearMethod == null) { - throw new ArgumentException("Not all required properties/methods are available"); + else + { + MethodInfo hasMethod = property.DeclaringType.GetRuntimeProperty("Has" + property.Name).GetMethod; + if (hasMethod == null) + { + throw new ArgumentException("Not all required properties/methods are available"); + } + hasDelegate = ReflectionUtil.CreateFuncIMessageBool(hasMethod); + MethodInfo clearMethod = property.DeclaringType.GetRuntimeMethod("Clear" + property.Name, ReflectionUtil.EmptyTypes); + if (clearMethod == null) + { + throw new ArgumentException("Not all required properties/methods are available"); + } + clearDelegate = ReflectionUtil.CreateActionIMessage(clearMethod); } - clearDelegate = ReflectionUtil.CreateActionIMessage(clearMethod); } }