Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ToProto() method to all C# descriptor classes #9426

Merged
merged 1 commit into from Jan 28, 2022
Merged

Add ToProto() method to all C# descriptor classes #9426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 24 additions & 0 deletions csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs
View file
Open in desktop
Expand Up @@ -124,6 +124,7 @@ private void TestFileDescriptor(FileDescriptor file, FileDescriptor importedFile
}

Assert.AreEqual(10, file.SerializedData[0]);
TestDescriptorToProto(file.ToProto, file.Proto);
}

[Test]
Expand Down Expand Up @@ -231,6 +232,7 @@ public void MessageDescriptor()
{
Assert.AreEqual(i, messageType.EnumTypes[i].Index);
}
TestDescriptorToProto(messageType.ToProto, messageType.Proto);
}

[Test]
Expand Down Expand Up @@ -294,6 +296,11 @@ public void FieldDescriptor_BuildFromByteStrings()
// 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);

TestDescriptorToProto(primitiveField.ToProto, primitiveField.Proto);
TestDescriptorToProto(enumField.ToProto, enumField.Proto);
TestDescriptorToProto(foreignMessageField.ToProto, foreignMessageField.Proto);
TestDescriptorToProto(fieldInOneof.ToProto, fieldInOneof.Proto);
}

[Test]
Expand Down Expand Up @@ -338,6 +345,8 @@ public void EnumDescriptor()
{
Assert.AreEqual(i, enumType.Values[i].Index);
}
TestDescriptorToProto(enumType.ToProto, enumType.Proto);
TestDescriptorToProto(nestedType.ToProto, nestedType.Proto);
}

[Test]
Expand All @@ -361,6 +370,7 @@ public void OneofDescriptor()
}

CollectionAssert.AreEquivalent(expectedFields, descriptor.Fields);
TestDescriptorToProto(descriptor.ToProto, descriptor.Proto);
}

[Test]
Expand All @@ -370,6 +380,7 @@ public void MapEntryMessageDescriptor()
Assert.IsNull(descriptor.Parser);
Assert.IsNull(descriptor.ClrType);
Assert.IsNull(descriptor.Fields[1].Accessor);
TestDescriptorToProto(descriptor.ToProto, descriptor.Proto);
}

// From TestFieldOrdering:
Expand All @@ -391,6 +402,7 @@ public void DescriptorProtoFileDescriptor()
{
var descriptor = Google.Protobuf.Reflection.FileDescriptor.DescriptorProtoFileDescriptor;
Assert.AreEqual("google/protobuf/descriptor.proto", descriptor.Name);
TestDescriptorToProto(descriptor.ToProto, descriptor.Proto);
}

[Test]
Expand Down Expand Up @@ -453,5 +465,17 @@ public void SyntheticOneofReflection()
}
}
}

private static void TestDescriptorToProto(Func<IMessage> toProtoFunction, IMessage expectedProto)
{
var clone1 = toProtoFunction();
var clone2 = toProtoFunction();
Assert.AreNotSame(clone1, clone2);
Assert.AreNotSame(clone1, expectedProto);
Assert.AreNotSame(clone2, expectedProto);

Assert.AreEqual(clone1, clone2);
Assert.AreEqual(clone1, expectedProto);
}
}
}
8 changes: 8 additions & 0 deletions csharp/src/Google.Protobuf/Reflection/EnumDescriptor.cs
View file
Open in desktop
Expand Up @@ -68,6 +68,14 @@ internal EnumDescriptor(EnumDescriptorProto proto, FileDescriptor file, MessageD

internal EnumDescriptorProto Proto { get { return proto; } }

/// <summary>
/// Returns a clone of the underlying <see cref="EnumDescriptorProto"/> describing this enum.
/// Note that a copy is taken every time this method is called, so clients using it frequently
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: worth mentioning that a deep clone is taken? (which also explains why it's safe to to .Clone() internally since it provides a deep clone).
Same below.
Leaving resolution up to you - I don't feel strongly about this at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it specifically says "Returns a clone" and "Note that a copy is taken" - I don't think we need to specifically say it's a deep clone, as a shallow clone would be really odd.

/// (and not modifying it) may want to cache the returned value.
/// </summary>
/// <returns>A protobuf representation of this enum descriptor.</returns>
public EnumDescriptorProto ToProto() => Proto.Clone();

/// <summary>
/// The brief name of the descriptor's target.
/// </summary>
Expand Down
8 changes: 8 additions & 0 deletions csharp/src/Google.Protobuf/Reflection/EnumValueDescriptor.cs
View file
Open in desktop
Expand Up @@ -55,6 +55,14 @@ public sealed class EnumValueDescriptor : DescriptorBase

internal EnumValueDescriptorProto Proto { get { return proto; } }

/// <summary>
/// Returns a clone of the underlying <see cref="EnumValueDescriptorProto"/> describing this enum value.
/// Note that a copy is taken every time this method is called, so clients using it frequently
/// (and not modifying it) may want to cache the returned value.
/// </summary>
/// <returns>A protobuf representation of this enum value descriptor.</returns>
public EnumValueDescriptorProto ToProto() => Proto.Clone();

/// <summary>
/// Returns the name of the enum value described by this object.
/// </summary>
Expand Down
8 changes: 8 additions & 0 deletions csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs
View file
Open in desktop
Expand Up @@ -91,6 +91,14 @@ public sealed class FieldDescriptor : DescriptorBase, IComparable<FieldDescripto

internal FieldDescriptorProto Proto { get; }

/// <summary>
/// Returns a clone of the underlying <see cref="FieldDescriptorProto"/> describing this field.
/// Note that a copy is taken every time this method is called, so clients using it frequently
/// (and not modifying it) may want to cache the returned value.
/// </summary>
/// <returns>A protobuf representation of this field descriptor.</returns>
public FieldDescriptorProto ToProto() => Proto.Clone();

/// <summary>
/// An extension identifier for this field, or <c>null</c> if this field isn't an extension.
/// </summary>
Expand Down
8 changes: 8 additions & 0 deletions csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs
View file
Open in desktop
Expand Up @@ -249,6 +249,14 @@ private static IList<FileDescriptor> DeterminePublicDependencies(FileDescriptor
/// </value>
internal FileDescriptorProto Proto { get; }

/// <summary>
/// Returns a clone of the underlying <see cref="FileDescriptorProto"/> describing this file.
/// Note that a copy is taken every time this method is called, so clients using it frequently
/// (and not modifying it) may want to cache the returned value.
/// </summary>
/// <returns>A protobuf representation of this file descriptor.</returns>
public FileDescriptorProto ToProto() => Proto.Clone();

/// <summary>
/// The syntax of the file
/// </summary>
Expand Down
8 changes: 8 additions & 0 deletions csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs
View file
Open in desktop
Expand Up @@ -151,6 +151,14 @@ internal override IReadOnlyList<DescriptorBase> GetNestedDescriptorListForField(

internal DescriptorProto Proto { get; }

/// <summary>
/// Returns a clone of the underlying <see cref="DescriptorProto"/> describing this message.
/// Note that a copy is taken every time this method is called, so clients using it frequently
/// (and not modifying it) may want to cache the returned value.
/// </summary>
/// <returns>A protobuf representation of this message descriptor.</returns>
public DescriptorProto ToProto() => Proto.Clone();

internal bool IsExtensionsInitialized(IMessage message)
{
if (Proto.ExtensionRange.Count == 0)
Expand Down
8 changes: 8 additions & 0 deletions csharp/src/Google.Protobuf/Reflection/MethodDescriptor.cs
View file
Open in desktop
Expand Up @@ -114,6 +114,14 @@ public RepeatedField<T> GetOption<T>(RepeatedExtension<MethodOptions, T> extensi

internal MethodDescriptorProto Proto { get { return proto; } }

/// <summary>
/// Returns a clone of the underlying <see cref="MethodDescriptorProto"/> describing this method.
/// Note that a copy is taken every time this method is called, so clients using it frequently
/// (and not modifying it) may want to cache the returned value.
/// </summary>
/// <returns>A protobuf representation of this method descriptor.</returns>
public MethodDescriptorProto ToProto() => Proto.Clone();

/// <summary>
/// The brief name of the descriptor's target.
/// </summary>
Expand Down
24 changes: 17 additions & 7 deletions csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs
View file
Open in desktop
Expand Up @@ -45,15 +45,14 @@ namespace Google.Protobuf.Reflection
/// </summary>
public sealed class OneofDescriptor : DescriptorBase
{
private readonly OneofDescriptorProto proto;
private MessageDescriptor containingType;
private IList<FieldDescriptor> fields;
private readonly OneofAccessor accessor;

internal OneofDescriptor(OneofDescriptorProto proto, FileDescriptor file, MessageDescriptor parent, int index, string clrName)
: base(file, file.ComputeFullName(parent, proto.Name), index)
{
this.proto = proto;
this.Proto = proto;
containingType = parent;
file.DescriptorPool.AddSymbol(this);

Expand All @@ -68,7 +67,18 @@ internal OneofDescriptor(OneofDescriptorProto proto, FileDescriptor file, Messag
/// <summary>
/// The brief name of the descriptor's target.
/// </summary>
public override string Name { get { return proto.Name; } }
public override string Name => Proto.Name;

// Visible for testing
internal OneofDescriptorProto Proto { get; }

/// <summary>
/// Returns a clone of the underlying <see cref="OneofDescriptorProto"/> describing this oneof.
/// Note that a copy is taken every time this method is called, so clients using it frequently
/// (and not modifying it) may want to cache the returned value.
/// </summary>
/// <returns>A protobuf representation of this oneof descriptor.</returns>
public OneofDescriptorProto ToProto() => Proto.Clone();

/// <summary>
/// Gets the message type containing this oneof.
Expand Down Expand Up @@ -118,23 +128,23 @@ public MessageDescriptor ContainingType
/// The (possibly empty) set of custom options for this oneof.
/// </summary>
[Obsolete("CustomOptions are obsolete. Use the GetOptions method.")]
public CustomOptions CustomOptions => new CustomOptions(proto.Options?._extensions?.ValuesByNumber);
public CustomOptions CustomOptions => new CustomOptions(Proto.Options?._extensions?.ValuesByNumber);

/// <summary>
/// The <c>OneofOptions</c>, defined in <c>descriptor.proto</c>.
/// If the options message is not present (i.e. there are no options), <c>null</c> is returned.
/// Custom options can be retrieved as extensions of the returned message.
/// NOTE: A defensive copy is created each time this property is retrieved.
/// </summary>
public OneofOptions GetOptions() => proto.Options?.Clone();
public OneofOptions GetOptions() => Proto.Options?.Clone();

/// <summary>
/// Gets a single value oneof option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the GetOptions() method.")]
public T GetOption<T>(Extension<OneofOptions, T> extension)
{
var value = proto.Options.GetExtension(extension);
var value = Proto.Options.GetExtension(extension);
return value is IDeepCloneable<T> ? (value as IDeepCloneable<T>).Clone() : value;
}

Expand All @@ -144,7 +154,7 @@ public T GetOption<T>(Extension<OneofOptions, T> extension)
[Obsolete("GetOption is obsolete. Use the GetOptions() method.")]
public RepeatedField<T> GetOption<T>(RepeatedExtension<OneofOptions, T> extension)
{
return proto.Options.GetExtension(extension).Clone();
return Proto.Options.GetExtension(extension).Clone();
}

internal void CrossLink()
Expand Down
8 changes: 8 additions & 0 deletions csharp/src/Google.Protobuf/Reflection/ServiceDescriptor.cs
View file
Open in desktop
Expand Up @@ -73,6 +73,14 @@ internal override IReadOnlyList<DescriptorBase> GetNestedDescriptorListForField(

internal ServiceDescriptorProto Proto { get { return proto; } }

/// <summary>
/// Returns a clone of the underlying <see cref="ServiceDescriptorProto"/> describing this service.
/// Note that a copy is taken every time this method is called, so clients using it frequently
/// (and not modifying it) may want to cache the returned value.
/// </summary>
/// <returns>A protobuf representation of this service descriptor.</returns>
public ServiceDescriptorProto ToProto() => Proto.Clone();

/// <value>
/// An unmodifiable list of methods in this service.
/// </value>
Expand Down