From cea3653a2ea4fa08ba8fa59b3f06cad27b5af074 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Wed, 20 Jan 2021 08:06:34 +0000 Subject: [PATCH 1/2] Include the size of values (and tags) for extensions, even if the value is the default Fixes #8218. --- .../Google.Protobuf.Test/ExtensionSetTest.cs | 17 ++++++++++++++++- csharp/src/Google.Protobuf/ExtensionValue.cs | 2 +- csharp/src/Google.Protobuf/FieldCodec.cs | 6 ++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/csharp/src/Google.Protobuf.Test/ExtensionSetTest.cs b/csharp/src/Google.Protobuf.Test/ExtensionSetTest.cs index aceb4a68f7..951e856a59 100644 --- a/csharp/src/Google.Protobuf.Test/ExtensionSetTest.cs +++ b/csharp/src/Google.Protobuf.Test/ExtensionSetTest.cs @@ -116,7 +116,22 @@ namespace Google.Protobuf var other = message.Clone(); Assert.AreEqual(message, other); - Assert.AreEqual(message.CalculateSize(), message.CalculateSize()); + Assert.AreEqual(message.CalculateSize(), other.CalculateSize()); + } + + [Test] + public void TestDefaultValueRoundTrip() + { + var message = new TestAllExtensions(); + message.SetExtension(OptionalBoolExtension, false); + Assert.IsFalse(message.GetExtension(OptionalBoolExtension)); + Assert.IsTrue(message.HasExtension(OptionalBoolExtension)); + + var bytes = message.ToByteArray(); + var registry = new ExtensionRegistry { OptionalBoolExtension }; + var parsed = TestAllExtensions.Parser.WithExtensionRegistry(registry).ParseFrom(bytes); + Assert.IsFalse(parsed.GetExtension(OptionalBoolExtension)); + Assert.IsTrue(parsed.HasExtension(OptionalBoolExtension)); } } } diff --git a/csharp/src/Google.Protobuf/ExtensionValue.cs b/csharp/src/Google.Protobuf/ExtensionValue.cs index ada5d79c80..5257c4c955 100644 --- a/csharp/src/Google.Protobuf/ExtensionValue.cs +++ b/csharp/src/Google.Protobuf/ExtensionValue.cs @@ -59,7 +59,7 @@ namespace Google.Protobuf public int CalculateSize() { - return codec.CalculateSizeWithTag(field); + return codec.CalculateUnconditionalSizeWithTag(field); } public IExtensionValue Clone() diff --git a/csharp/src/Google.Protobuf/FieldCodec.cs b/csharp/src/Google.Protobuf/FieldCodec.cs index 158739d50a..ee6bd6a0eb 100644 --- a/csharp/src/Google.Protobuf/FieldCodec.cs +++ b/csharp/src/Google.Protobuf/FieldCodec.cs @@ -876,6 +876,12 @@ namespace Google.Protobuf /// public int CalculateSizeWithTag(T value) => IsDefault(value) ? 0 : ValueSizeCalculator(value) + tagSize; + /// + /// Calculates the size required to write the given value, with a tag, even + /// if the value is the default. + /// + internal int CalculateUnconditionalSizeWithTag(T value) => ValueSizeCalculator(value) + tagSize; + private bool IsDefault(T value) => EqualityComparer.Equals(value, DefaultValue); } } From f01d9e5601a82acbda9a2927fa9828fff5c0c6fb Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Wed, 20 Jan 2021 08:11:27 +0000 Subject: [PATCH 2/2] Add an extension registry option when parsing file descriptors This is important when parsing descriptor sets that contain extensions. (The alternative is to get the descriptor bytes again for individual elements, e.g. message descriptors, and reparse them with the appropriate extensions. It's really ugly.) --- .../Reflection/DescriptorsTest.cs | 20 +++++++++++++++++++ .../Reflection/FileDescriptor.cs | 19 ++++++++++++++++-- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs b/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs index ebb8394d23..fab983d463 100644 --- a/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs +++ b/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs @@ -35,7 +35,9 @@ using NUnit.Framework; using ProtobufUnittest; using System; using System.Collections.Generic; +using System.IO; using System.Linq; +using UnitTest.Issues.TestProtos; namespace Google.Protobuf.Reflection { @@ -70,6 +72,24 @@ namespace Google.Protobuf.Reflection TestFileDescriptor(converted[2], converted[1], converted[0]); } + [Test] + public void FileDescriptor_BuildFromByteStrings_WithExtensionRegistry() + { + var extension = UnittestCustomOptionsProto3Extensions.MessageOpt1; + + var byteStrings = new[] + { + DescriptorReflection.Descriptor.Proto.ToByteString(), + UnittestCustomOptionsProto3Reflection.Descriptor.Proto.ToByteString() + }; + var registry = new ExtensionRegistry { extension }; + + var descriptor = FileDescriptor.BuildFromByteStrings(byteStrings, registry).Last(); + var message = descriptor.MessageTypes.Single(t => t.Name == nameof(TestMessageWithCustomOptions)); + var extensionValue = message.GetOptions().GetExtension(extension); + Assert.AreEqual(-56, extensionValue); + } + private void TestFileDescriptor(FileDescriptor file, FileDescriptor importedFile, FileDescriptor importedPublicFile) { Assert.AreEqual("unittest_proto3.proto", file.Name); diff --git a/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs index 88e4a9de96..724bb3a04d 100644 --- a/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs +++ b/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs @@ -481,18 +481,21 @@ namespace Google.Protobuf.Reflection /// 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 extension registry to use when parsing, or null if no extensions are required. /// The file descriptors corresponding to . - public static IReadOnlyList BuildFromByteStrings(IEnumerable descriptorData) + public static IReadOnlyList BuildFromByteStrings(IEnumerable descriptorData, ExtensionRegistry registry) { ProtoPreconditions.CheckNotNull(descriptorData, nameof(descriptorData)); + var parser = FileDescriptorProto.Parser.WithExtensionRegistry(registry); + // 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 proto = parser.ParseFrom(data); var dependencies = new List(); foreach (var dependencyName in proto.Dependency) { @@ -518,6 +521,18 @@ namespace Google.Protobuf.Reflection return new ReadOnlyCollection(descriptors); } + /// + /// 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) => + BuildFromByteStrings(descriptorData, null); + /// /// Returns a that represents this instance. ///