From 34878cb14eba4578d1d67d2dc93250729d492774 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 17 Jul 2015 06:41:46 +0100 Subject: [PATCH] Fixes from PR review. --- .../WellKnownTypes/WrappersTest.cs | 71 ++++++++++++++++++- .../ProtocolBuffers/Collections/MapField.cs | 2 +- csharp/src/ProtocolBuffers/FieldCodec.cs | 30 +++++--- .../compiler/csharp/csharp_wrapper_field.h | 1 - 4 files changed, 89 insertions(+), 15 deletions(-) diff --git a/csharp/src/ProtocolBuffers.Test/WellKnownTypes/WrappersTest.cs b/csharp/src/ProtocolBuffers.Test/WellKnownTypes/WrappersTest.cs index 6c70601404..ad88c4ebab 100644 --- a/csharp/src/ProtocolBuffers.Test/WellKnownTypes/WrappersTest.cs +++ b/csharp/src/ProtocolBuffers.Test/WellKnownTypes/WrappersTest.cs @@ -34,6 +34,7 @@ using System; using Google.Protobuf.TestProtos; using NUnit.Framework; using System.Collections; +using System.IO; namespace Google.Protobuf.WellKnownTypes { @@ -232,9 +233,44 @@ namespace Google.Protobuf.WellKnownTypes Assert.IsTrue(dictionary.Contains(3)); } - // Merging is odd with wrapper types, due to the way that default values aren't emitted in - // the binary stream. In fact we cheat a little bit - a message with an explicitly present default - // value will have that default value ignored. + [Test] + public void Oneof() + { + var message = new OneofWellKnownTypes { EmptyField = new Empty() }; + // Start off with a non-wrapper + Assert.AreEqual(OneofWellKnownTypes.OneofFieldOneofCase.EmptyField, message.OneofFieldCase); + AssertOneofRoundTrip(message); + + message.StringField = "foo"; + Assert.AreEqual(OneofWellKnownTypes.OneofFieldOneofCase.StringField, message.OneofFieldCase); + AssertOneofRoundTrip(message); + + message.StringField = "foo"; + Assert.AreEqual(OneofWellKnownTypes.OneofFieldOneofCase.StringField, message.OneofFieldCase); + AssertOneofRoundTrip(message); + + message.DoubleField = 0.0f; + Assert.AreEqual(OneofWellKnownTypes.OneofFieldOneofCase.DoubleField, message.OneofFieldCase); + AssertOneofRoundTrip(message); + + message.DoubleField = 1.0f; + Assert.AreEqual(OneofWellKnownTypes.OneofFieldOneofCase.DoubleField, message.OneofFieldCase); + AssertOneofRoundTrip(message); + + message.ClearOneofField(); + Assert.AreEqual(OneofWellKnownTypes.OneofFieldOneofCase.None, message.OneofFieldCase); + AssertOneofRoundTrip(message); + } + + private void AssertOneofRoundTrip(OneofWellKnownTypes message) + { + // Normal roundtrip, but explicitly checking the case... + var bytes = message.ToByteArray(); + var parsed = OneofWellKnownTypes.Parser.ParseFrom(bytes); + Assert.AreEqual(message, parsed); + Assert.AreEqual(message.OneofFieldCase, parsed.OneofFieldCase); + } + [Test] [TestCase("x", "y", "y")] [TestCase("x", "", "x")] @@ -257,5 +293,34 @@ namespace Google.Protobuf.WellKnownTypes originalMessage.MergeFrom(mergingMessage.ToByteArray()); Assert.AreEqual(expected, originalMessage.StringField); } + + // Merging is odd with wrapper types, due to the way that default values aren't emitted in + // the binary stream. In fact we cheat a little bit - a message with an explicitly present default + // value will have that default value ignored. + [Test] + public void MergingCornerCase() + { + var message = new TestWellKnownTypes { Int32Field = 5 }; + + // Create a byte array which has the data of an Int32Value explicitly containing a value of 0. + // This wouldn't normally happen. + byte[] bytes; + var wrapperTag = WireFormat.MakeTag(TestWellKnownTypes.Int32FieldFieldNumber, WireFormat.WireType.LengthDelimited); + var valueTag = WireFormat.MakeTag(Int32Value.ValueFieldNumber, WireFormat.WireType.Varint); + using (var stream = new MemoryStream()) + { + var coded = CodedOutputStream.CreateInstance(stream); + coded.WriteTag(wrapperTag); + coded.WriteLength(2); // valueTag + a value 0, each one byte + coded.WriteTag(valueTag); + coded.WriteInt32(0); + coded.Flush(); + bytes = stream.ToArray(); + } + + message.MergeFrom(bytes); + // A normal implementation would have 0 now, as the explicit default would have been overwritten the 5. + Assert.AreEqual(5, message.Int32Field); + } } } diff --git a/csharp/src/ProtocolBuffers/Collections/MapField.cs b/csharp/src/ProtocolBuffers/Collections/MapField.cs index ead4573767..f5e4fd3aa6 100644 --- a/csharp/src/ProtocolBuffers/Collections/MapField.cs +++ b/csharp/src/ProtocolBuffers/Collections/MapField.cs @@ -51,7 +51,7 @@ namespace Google.Protobuf.Collections public sealed class MapField : IDeepCloneable>, IFreezable, IDictionary, IEquatable>, IDictionary { // TODO: Don't create the map/list until we have an entry. (Assume many maps will be empty.) - private bool allowNullValues; + private readonly bool allowNullValues; private bool frozen; private readonly Dictionary>> map = new Dictionary>>(); diff --git a/csharp/src/ProtocolBuffers/FieldCodec.cs b/csharp/src/ProtocolBuffers/FieldCodec.cs index caf032864e..854627879c 100644 --- a/csharp/src/ProtocolBuffers/FieldCodec.cs +++ b/csharp/src/ProtocolBuffers/FieldCodec.cs @@ -161,20 +161,30 @@ namespace Google.Protobuf null); // Default value for the wrapper } - // Helper code to create codecs for wrapper types. Somewhat ugly with all the + /// + /// Helper code to create codecs for wrapper types. + /// + /// + /// Somewhat ugly with all the static methods, but the conversions involved to/from nullable types make it + /// slightly tricky to improve. So long as we keep the public API (ForClassWrapper, ForStructWrapper) in place, + /// we can refactor later if we come up with something cleaner. + /// private static class WrapperCodecs { + // All the field numbers are the same (1). + private const int WrapperValueFieldNumber = Google.Protobuf.WellKnownTypes.Int32Value.ValueFieldNumber; + private static readonly Dictionary Codecs = new Dictionary { - { typeof(bool), ForBool(WireFormat.MakeTag(1, WireFormat.WireType.Varint)) }, - { typeof(int), ForInt32(WireFormat.MakeTag(1, WireFormat.WireType.Varint)) }, - { typeof(long), ForInt64(WireFormat.MakeTag(1, WireFormat.WireType.Varint)) }, - { typeof(uint), ForUInt32(WireFormat.MakeTag(1, WireFormat.WireType.Varint)) }, - { typeof(ulong), ForUInt64(WireFormat.MakeTag(1, WireFormat.WireType.Varint)) }, - { typeof(float), ForFloat(WireFormat.MakeTag(1, WireFormat.WireType.Fixed32)) }, - { typeof(double), ForDouble(WireFormat.MakeTag(1, WireFormat.WireType.Fixed64)) }, - { typeof(string), ForString(WireFormat.MakeTag(1, WireFormat.WireType.LengthDelimited)) }, - { typeof(ByteString), ForBytes(WireFormat.MakeTag(1, WireFormat.WireType.LengthDelimited)) } + { typeof(bool), ForBool(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Varint)) }, + { typeof(int), ForInt32(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Varint)) }, + { typeof(long), ForInt64(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Varint)) }, + { typeof(uint), ForUInt32(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Varint)) }, + { typeof(ulong), ForUInt64(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Varint)) }, + { typeof(float), ForFloat(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Fixed32)) }, + { typeof(double), ForDouble(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Fixed64)) }, + { typeof(string), ForString(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.LengthDelimited)) }, + { typeof(ByteString), ForBytes(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.LengthDelimited)) } }; /// diff --git a/src/google/protobuf/compiler/csharp/csharp_wrapper_field.h b/src/google/protobuf/compiler/csharp/csharp_wrapper_field.h index 5e2c5bc3b8..6e2414af83 100644 --- a/src/google/protobuf/compiler/csharp/csharp_wrapper_field.h +++ b/src/google/protobuf/compiler/csharp/csharp_wrapper_field.h @@ -83,4 +83,3 @@ class WrapperOneofFieldGenerator : public WrapperFieldGenerator { } // namespace google #endif // GOOGLE_PROTOBUF_COMPILER_CSHARP_WRAPPER_FIELD_H__ -