From 47f2017cf3880821727deb5c12c9b850b01fca9c Mon Sep 17 00:00:00 2001
From: Sydney Acksman <obsidianminor@gmail.com>
Date: Tue, 2 Jul 2019 18:06:55 -0500
Subject: [PATCH] Review changes * Add Syntax enum and make file syntax checks
check the enum * Made IsPacked a normal get method without ternary op *
Moved IFieldAccessor.HasValue to IFieldPresenceAccessor * Change naming of
GetIsExtensionsInitialized * Fixed stray text in summary text
---
.../Reflection/FieldAccessTest.cs | 4 +-
.../src/Google.Protobuf/MessageExtensions.cs | 8 ++--
.../Reflection/FieldDescriptor.cs | 15 +++++++-
.../Reflection/FileDescriptor.cs | 37 +++++++++++++++++++
.../Reflection/IFieldAccessor.cs | 16 +++++---
.../Reflection/MessageDescriptor.cs | 2 +-
.../Reflection/ReflectionUtil.cs | 2 +-
.../Reflection/SingleFieldAccessor.cs | 4 +-
8 files changed, 72 insertions(+), 16 deletions(-)
diff --git a/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs b/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs
index fcfff4f7ac..dd753e8df4 100644
--- a/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs
+++ b/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs
@@ -101,7 +101,7 @@ namespace Google.Protobuf.Reflection
{
IMessage message = SampleMessages.CreateFullTestAllTypes();
var fields = message.Descriptor.Fields;
- Assert.Throws<InvalidOperationException>(() => fields[TestProtos.TestAllTypes.SingleBoolFieldNumber].Accessor.HasValue(message));
+ Assert.Throws<InvalidOperationException>(() => (fields[TestProtos.TestAllTypes.SingleBoolFieldNumber].Accessor as IFieldPresenceAccessor).HasValue(message));
}
[Test]
@@ -109,7 +109,7 @@ namespace Google.Protobuf.Reflection
{
IMessage message = new Proto2.TestAllTypes();
var fields = message.Descriptor.Fields;
- var accessor = fields[Proto2.TestAllTypes.OptionalBoolFieldNumber].Accessor;
+ var accessor = fields[Proto2.TestAllTypes.OptionalBoolFieldNumber].Accessor as IFieldPresenceAccessor;
Assert.False(accessor.HasValue(message));
diff --git a/csharp/src/Google.Protobuf/MessageExtensions.cs b/csharp/src/Google.Protobuf/MessageExtensions.cs
index d297a118c5..986c3798dd 100644
--- a/csharp/src/Google.Protobuf/MessageExtensions.cs
+++ b/csharp/src/Google.Protobuf/MessageExtensions.cs
@@ -148,12 +148,12 @@ namespace Google.Protobuf
/// </summary>
public static bool IsInitialized(this IMessage message)
{
- if (message.Descriptor.File.Proto.Syntax == "proto3")
+ if (message.Descriptor.File.Syntax == Syntax.Proto3)
{
return true;
}
- if (!message.Descriptor.GetIsExtensionsInitialized(message))
+ if (!message.Descriptor.IsExtensionsInitialized(message))
{
return false;
}
@@ -183,7 +183,7 @@ namespace Google.Protobuf
}
else if (f.FieldType == FieldType.Message || f.FieldType == FieldType.Group)
{
- if (f.Accessor.HasValue(message))
+ if ((f.Accessor as IFieldPresenceAccessor).HasValue(message))
{
return ((IMessage)f.Accessor.GetValue(message)).IsInitialized();
}
@@ -194,7 +194,7 @@ namespace Google.Protobuf
}
else if (f.IsRequired)
{
- return f.Accessor.HasValue(message);
+ return (f.Accessor as IFieldPresenceAccessor).HasValue(message);
}
else
{
diff --git a/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs
index 807e6ce0d8..a8b9e69f44 100644
--- a/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs
+++ b/csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs
@@ -201,7 +201,20 @@ namespace Google.Protobuf.Reflection
/// <summary>
/// Returns <c>true</c> if this field is a packed, repeated field; <c>false</c> otherwise.
/// </summary>
- public bool IsPacked => File.Proto.Syntax != "proto3" ? Proto.Options?.Packed ?? false : !Proto.Options.HasPacked || Proto.Options.Packed;
+ public bool IsPacked
+ {
+ get
+ {
+ if (File.Syntax != Syntax.Proto3)
+ {
+ return Proto.Options?.Packed ?? false;
+ }
+ else
+ {
+ return !Proto.Options.HasPacked || Proto.Options.Packed;
+ }
+ }
+ }
/// <summary>
/// Returns <c>true</c> if this field extends another message type; <c>false</c> otherwise.
diff --git a/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs
index 13266f725d..993f428cab 100644
--- a/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs
+++ b/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs
@@ -42,6 +42,25 @@ using static Google.Protobuf.Reflection.SourceCodeInfo.Types;
namespace Google.Protobuf.Reflection
{
+ /// <summary>
+ /// The syntax of a .proto file
+ /// </summary>
+ public enum Syntax
+ {
+ /// <summary>
+ /// Proto2 syntax
+ /// </summary>
+ Proto2,
+ /// <summary>
+ /// Proto3 syntax
+ /// </summary>
+ Proto3,
+ /// <summary>
+ /// An unknown declared syntax
+ /// </summary>
+ Unknown
+ }
+
/// <summary>
/// Describes a .proto file, including everything defined within.
/// IDescriptor is implemented such that the File property returns this descriptor,
@@ -87,6 +106,19 @@ namespace Google.Protobuf.Reflection
Extensions = new ExtensionCollection(this, generatedCodeInfo?.Extensions);
declarations = new Lazy<Dictionary<IDescriptor, DescriptorDeclaration>>(CreateDeclarationMap, LazyThreadSafetyMode.ExecutionAndPublication);
+
+ if (!proto.HasSyntax || proto.Syntax == "proto2")
+ {
+ Syntax = Syntax.Proto2;
+ }
+ else if (proto.Syntax == "proto3")
+ {
+ Syntax = Syntax.Proto3;
+ }
+ else
+ {
+ Syntax = Syntax.Unknown;
+ }
}
private Dictionary<IDescriptor, DescriptorDeclaration> CreateDeclarationMap()
@@ -217,6 +249,11 @@ namespace Google.Protobuf.Reflection
/// </value>
internal FileDescriptorProto Proto { get; }
+ /// <summary>
+ /// The syntax of the file
+ /// </summary>
+ public Syntax Syntax { get; }
+
/// <value>
/// The file name.
/// </value>
diff --git a/csharp/src/Google.Protobuf/Reflection/IFieldAccessor.cs b/csharp/src/Google.Protobuf/Reflection/IFieldAccessor.cs
index 8f7ef3c67c..373984e9b1 100644
--- a/csharp/src/Google.Protobuf/Reflection/IFieldAccessor.cs
+++ b/csharp/src/Google.Protobuf/Reflection/IFieldAccessor.cs
@@ -51,11 +51,6 @@ namespace Google.Protobuf.Reflection
/// </summary>
void Clear(IMessage message);
- /// <summary>
- /// Indicates whether the field in the specified message is set. For proto3 fields, this throws an <see cref="InvalidOperationException"/>
- /// </summary>
- bool HasValue(IMessage message);
-
/// <summary>
/// Fetches the field value. For repeated values, this will be an
/// <see cref="IList"/> implementation. For map values, this will be an
@@ -73,4 +68,15 @@ namespace Google.Protobuf.Reflection
/// <exception cref="InvalidOperationException">The field is not a "simple" field.</exception>
void SetValue(IMessage message, object value);
}
+
+ /// <summary>
+ /// Allows field presence to be checked reflectively. This is implemented for all single field accessors
+ /// </summary>
+ public interface IFieldPresenceAccessor : IFieldAccessor
+ {
+ /// <summary>
+ /// Indicates whether the field in the specified message is set. For proto3 fields, this throws an <see cref="InvalidOperationException"/>
+ /// </summary>
+ bool HasValue(IMessage message);
+ }
}
\ No newline at end of file
diff --git a/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs
index a6ecfb984b..6a93a8faa8 100644
--- a/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs
+++ b/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs
@@ -136,7 +136,7 @@ namespace Google.Protobuf.Reflection
internal DescriptorProto Proto { get; }
- internal bool GetIsExtensionsInitialized(IMessage message)
+ internal bool IsExtensionsInitialized(IMessage message)
{
if (Proto.ExtensionRange.Count == 0)
{
diff --git a/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs b/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs
index 5e66b6f2e4..28f724b2d8 100644
--- a/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs
+++ b/csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs
@@ -71,7 +71,7 @@ namespace Google.Protobuf.Reflection
/// <summary>
/// Empty Type[] used when calling GetProperty to force property instead of indexer fetching.
- /// </summary>getFieldFunc
+ /// </summary>
internal static readonly Type[] EmptyTypes = new Type[0];
/// <summary>
diff --git a/csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs b/csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs
index dc6e0ca9ae..d5a8e6ff7d 100644
--- a/csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs
+++ b/csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs
@@ -39,7 +39,7 @@ namespace Google.Protobuf.Reflection
/// <summary>
/// Accessor for single fields.
/// </summary>
- internal sealed class SingleFieldAccessor : FieldAccessorBase
+ internal sealed class SingleFieldAccessor : FieldAccessorBase, IFieldPresenceAccessor
{
// All the work here is actually done in the constructor - it creates the appropriate delegates.
// There are various cases to consider, based on the property type (message, string/bytes, or "genuine" primitive)
@@ -57,7 +57,7 @@ namespace Google.Protobuf.Reflection
throw new ArgumentException("Not all required properties/methods available");
}
setValueDelegate = ReflectionUtil.CreateActionIMessageObject(property.GetSetMethod());
- if (descriptor.File.Proto.Syntax == "proto3")
+ if (descriptor.File.Syntax == Syntax.Proto3)
{
hasDelegate = message => {
throw new InvalidOperationException("HasValue is not implemented for proto3 fields");