diff --git a/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs b/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs index 0d4034c5b..b4dcdabdc 100644 --- a/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs +++ b/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs @@ -98,7 +98,48 @@ namespace Google.Protobuf.Reflection } [Test] - public void HasValue_Proto3() + public void HasValue_Proto3_Message() + { + var message = new TestAllTypes(); + var accessor = ((IMessage) message).Descriptor.Fields[TestProtos.TestAllTypes.SingleForeignMessageFieldNumber].Accessor; + Assert.False(accessor.HasValue(message)); + message.SingleForeignMessage = new ForeignMessage(); + Assert.True(accessor.HasValue(message)); + message.SingleForeignMessage = null; + Assert.False(accessor.HasValue(message)); + } + + [Test] + public void HasValue_Proto3_Oneof() + { + TestAllTypes message = new TestAllTypes(); + var accessor = ((IMessage) message).Descriptor.Fields[TestProtos.TestAllTypes.OneofStringFieldNumber].Accessor; + Assert.False(accessor.HasValue(message)); + // Even though it's the default value, we still have a value. + message.OneofString = ""; + Assert.True(accessor.HasValue(message)); + message.OneofString = "hello"; + Assert.True(accessor.HasValue(message)); + message.OneofUint32 = 10; + Assert.False(accessor.HasValue(message)); + } + + [Test] + public void HasValue_Proto3_Primitive_Optional() + { + var message = new TestProto3Optional(); + var accessor = ((IMessage) message).Descriptor.Fields[TestProto3Optional.OptionalInt64FieldNumber].Accessor; + Assert.IsFalse(accessor.HasValue(message)); + message.OptionalInt64 = 5L; + Assert.IsTrue(accessor.HasValue(message)); + message.ClearOptionalInt64(); + Assert.IsFalse(accessor.HasValue(message)); + message.OptionalInt64 = 0L; + Assert.IsTrue(accessor.HasValue(message)); + } + + [Test] + public void HasValue_Proto3_Primitive_NotOptional() { IMessage message = SampleMessages.CreateFullTestAllTypes(); var fields = message.Descriptor.Fields; @@ -106,36 +147,63 @@ namespace Google.Protobuf.Reflection } [Test] - public void HasValue_Proto3Optional() + public void HasValue_Proto3_Repeated() { - 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)); + var message = new TestAllTypes(); + var accessor = ((IMessage) message).Descriptor.Fields[TestProtos.TestAllTypes.RepeatedBoolFieldNumber].Accessor; + Assert.Throws(() => accessor.HasValue(message)); } [Test] - public void HasValue() + public void HasValue_Proto2_Primitive() { - IMessage message = new Proto2.TestAllTypes(); - var fields = message.Descriptor.Fields; - var accessor = fields[Proto2.TestAllTypes.OptionalBoolFieldNumber].Accessor; + var message = new Proto2.TestAllTypes(); + var accessor = ((IMessage) message).Descriptor.Fields[Proto2.TestAllTypes.OptionalInt64FieldNumber].Accessor; + Assert.IsFalse(accessor.HasValue(message)); + message.OptionalInt64 = 5L; + Assert.IsTrue(accessor.HasValue(message)); + message.ClearOptionalInt64(); + Assert.IsFalse(accessor.HasValue(message)); + message.OptionalInt64 = 0L; + Assert.IsTrue(accessor.HasValue(message)); + } + + [Test] + public void HasValue_Proto2_Message() + { + var message = new Proto2.TestAllTypes(); + var field = ((IMessage) message).Descriptor.Fields[Proto2.TestAllTypes.OptionalForeignMessageFieldNumber]; + Assert.False(field.Accessor.HasValue(message)); + message.OptionalForeignMessage = new Proto2.ForeignMessage(); + Assert.True(field.Accessor.HasValue(message)); + message.OptionalForeignMessage = null; + Assert.False(field.Accessor.HasValue(message)); + } + + [Test] + public void HasValue_Proto2_Oneof() + { + var message = new Proto2.TestAllTypes(); + var accessor = ((IMessage) message).Descriptor.Fields[Proto2.TestAllTypes.OneofStringFieldNumber].Accessor; Assert.False(accessor.HasValue(message)); - - accessor.SetValue(message, true); + // Even though it's the default value, we still have a value. + message.OneofString = ""; Assert.True(accessor.HasValue(message)); - - accessor.Clear(message); + message.OneofString = "hello"; + Assert.True(accessor.HasValue(message)); + message.OneofUint32 = 10; Assert.False(accessor.HasValue(message)); } + [Test] + public void HasValue_Proto2_Repeated() + { + var message = new Proto2.TestAllTypes(); + var accessor = ((IMessage) message).Descriptor.Fields[Proto2.TestAllTypes.RepeatedBoolFieldNumber].Accessor; + Assert.Throws(() => accessor.HasValue(message)); + } + [Test] public void SetValue_SingleFields() { @@ -262,6 +330,42 @@ namespace Google.Protobuf.Reflection Assert.Null(message.OptionalNestedMessage); } + [Test] + public void Clear_Proto3_Oneof() + { + var message = new TestAllTypes(); + var accessor = ((IMessage) message).Descriptor.Fields[TestProtos.TestAllTypes.OneofUint32FieldNumber].Accessor; + + // The field accessor Clear method only affects a oneof if the current case is the one being cleared. + message.OneofString = "hello"; + Assert.AreEqual(TestProtos.TestAllTypes.OneofFieldOneofCase.OneofString, message.OneofFieldCase); + accessor.Clear(message); + Assert.AreEqual(TestProtos.TestAllTypes.OneofFieldOneofCase.OneofString, message.OneofFieldCase); + + message.OneofUint32 = 100; + Assert.AreEqual(TestProtos.TestAllTypes.OneofFieldOneofCase.OneofUint32, message.OneofFieldCase); + accessor.Clear(message); + Assert.AreEqual(TestProtos.TestAllTypes.OneofFieldOneofCase.None, message.OneofFieldCase); + } + + [Test] + public void Clear_Proto2_Oneof() + { + var message = new Proto2.TestAllTypes(); + var accessor = ((IMessage) message).Descriptor.Fields[Proto2.TestAllTypes.OneofUint32FieldNumber].Accessor; + + // The field accessor Clear method only affects a oneof if the current case is the one being cleared. + message.OneofString = "hello"; + Assert.AreEqual(Proto2.TestAllTypes.OneofFieldOneofCase.OneofString, message.OneofFieldCase); + accessor.Clear(message); + Assert.AreEqual(Proto2.TestAllTypes.OneofFieldOneofCase.OneofString, message.OneofFieldCase); + + message.OneofUint32 = 100; + Assert.AreEqual(Proto2.TestAllTypes.OneofFieldOneofCase.OneofUint32, message.OneofFieldCase); + accessor.Clear(message); + Assert.AreEqual(Proto2.TestAllTypes.OneofFieldOneofCase.None, message.OneofFieldCase); + } + [Test] public void FieldDescriptor_ByName() { @@ -301,5 +405,32 @@ namespace Google.Protobuf.Reflection message.ClearExtension(RepeatedBoolExtension); Assert.IsNull(message.GetExtension(RepeatedBoolExtension)); } + + [Test] + public void HasPresence() + { + // Proto3 + var fields = TestProtos.TestAllTypes.Descriptor.Fields; + Assert.IsFalse(fields[TestProtos.TestAllTypes.SingleBoolFieldNumber].HasPresence); + Assert.IsTrue(fields[TestProtos.TestAllTypes.OneofBytesFieldNumber].HasPresence); + Assert.IsTrue(fields[TestProtos.TestAllTypes.SingleForeignMessageFieldNumber].HasPresence); + Assert.IsFalse(fields[TestProtos.TestAllTypes.RepeatedBoolFieldNumber].HasPresence); + + fields = TestMap.Descriptor.Fields; + Assert.IsFalse(fields[TestMap.MapBoolBoolFieldNumber].HasPresence); + + fields = TestProto3Optional.Descriptor.Fields; + Assert.IsTrue(fields[TestProto3Optional.OptionalBoolFieldNumber].HasPresence); + + // Proto2 + fields = Proto2.TestAllTypes.Descriptor.Fields; + Assert.IsTrue(fields[Proto2.TestAllTypes.OptionalBoolFieldNumber].HasPresence); + Assert.IsTrue(fields[Proto2.TestAllTypes.OneofBytesFieldNumber].HasPresence); + Assert.IsTrue(fields[Proto2.TestAllTypes.OptionalForeignMessageFieldNumber].HasPresence); + Assert.IsFalse(fields[Proto2.TestAllTypes.RepeatedBoolFieldNumber].HasPresence); + + fields = Proto2.TestRequired.Descriptor.Fields; + Assert.IsTrue(fields[Proto2.TestRequired.AFieldNumber].HasPresence); + } } } diff --git a/csharp/src/Google.Protobuf/Extension.cs b/csharp/src/Google.Protobuf/Extension.cs index a96f8d29b..6dd1ceaa8 100644 --- a/csharp/src/Google.Protobuf/Extension.cs +++ b/csharp/src/Google.Protobuf/Extension.cs @@ -55,6 +55,8 @@ namespace Google.Protobuf /// Gets the field number of this extension /// public int FieldNumber { get; } + + internal abstract bool IsRepeated { get; } } /// @@ -79,6 +81,8 @@ namespace Google.Protobuf internal override Type TargetType => typeof(TTarget); + internal override bool IsRepeated => false; + internal override IExtensionValue CreateValue() { return new ExtensionValue(codec); @@ -105,6 +109,8 @@ namespace Google.Protobuf internal override Type TargetType => typeof(TTarget); + internal override bool IsRepeated => true; + internal override IExtensionValue CreateValue() { return new RepeatedExtensionValue(codec); diff --git a/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs index 3f50fdb3b..7324e3dfc 100644 --- a/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs +++ b/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs @@ -70,6 +70,21 @@ namespace Google.Protobuf.Reflection /// public string JsonName { get; } + /// + /// Indicates whether this field supports presence, either implicitly (e.g. due to it being a message + /// type field) or explicitly via Has/Clear members. If this returns true, it is safe to call + /// and + /// on this field's accessor with a suitable message. + /// + public bool HasPresence => + Extension != null ? !Extension.IsRepeated + : IsRepeated ? false + : IsMap ? false + : FieldType == FieldType.Message ? true + // This covers "real oneof members" and "proto3 optional fields" + : ContainingOneof != null ? true + : File.Syntax == Syntax.Proto2; + internal FieldDescriptorProto Proto { get; } /// diff --git a/csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs b/csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs index ed844bc51..07d84d7fb 100644 --- a/csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs +++ b/csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs @@ -57,63 +57,68 @@ namespace Google.Protobuf.Reflection throw new ArgumentException("Not all required properties/methods available"); } setValueDelegate = ReflectionUtil.CreateActionIMessageObject(property.GetSetMethod()); - if (descriptor.File.Syntax == Syntax.Proto3 && !descriptor.Proto.Proto3Optional) + + // Note: this looks worrying in that we access the containing oneof, which isn't valid until cross-linking + // is complete... but field accessors aren't created until after cross-linking. + // The oneof itself won't be cross-linked yet, but that's okay: the oneof accessor is created + // earlier. + + // Message fields always support presence, via null checks. + if (descriptor.FieldType == FieldType.Message) { - hasDelegate = message => + hasDelegate = message => GetValue(message) != null; + clearDelegate = message => SetValue(message, null); + } + // Oneof fields always support presence, via case checks. + // Note that clearing the field is a no-op unless that specific field is the current "case". + else if (descriptor.RealContainingOneof != null) + { + var oneofAccessor = descriptor.RealContainingOneof.Accessor; + hasDelegate = message => oneofAccessor.GetCaseFieldDescriptor(message) == descriptor; + clearDelegate = message => { - throw new InvalidOperationException("HasValue is not implemented for non-optional proto3 fields"); + // Clear on a field only affects the oneof itself if the current case is the field we're accessing. + if (oneofAccessor.GetCaseFieldDescriptor(message) == descriptor) + { + oneofAccessor.Clear(message); + } }; + } + // Primitive fields always support presence in proto2, and support presence in proto3 for optional fields. + else if (descriptor.File.Syntax == Syntax.Proto2 || descriptor.Proto.Proto3Optional) + { + 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); + } + // What's left? + // Primitive proto3 fields without the optional keyword, which aren't in oneofs. + else + { + hasDelegate = message => { throw new InvalidOperationException("Presence is not implemented for this field"); }; + + // While presence isn't supported, clearing still is; it's just setting to a default value. var clrType = property.PropertyType; - // TODO: Validate that this is a reasonable single field? (Should be a value type, a message type, or string/ByteString.) object defaultValue = - descriptor.FieldType == FieldType.Message ? null - : clrType == typeof(string) ? "" + clrType == typeof(string) ? "" : clrType == typeof(ByteString) ? ByteString.Empty : Activator.CreateInstance(clrType); clearDelegate = message => SetValue(message, defaultValue); } - else - { - // 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); - } - 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); - } - } } - public override void Clear(IMessage message) - { - clearDelegate(message); - } - - public override bool HasValue(IMessage message) - { - return hasDelegate(message); - } - - public override void SetValue(IMessage message, object value) - { - setValueDelegate(message, value); - } + public override void Clear(IMessage message) => clearDelegate(message); + public override bool HasValue(IMessage message) => hasDelegate(message); + public override void SetValue(IMessage message, object value) => setValueDelegate(message, value); } }