diff options
author | Jon Skeet <jonskeet@google.com> | 2015-07-17 06:41:46 +0100 |
---|---|---|
committer | Jon Skeet <jonskeet@google.com> | 2015-07-17 06:41:46 +0100 |
commit | 34878cb14eba4578d1d67d2dc93250729d492774 (patch) | |
tree | 6764c5a6f20e7ac4f2c7205638222d7cbbe4a90f | |
parent | 3da90e9f54f9648a76a0172e5af94f4a0a00a5e8 (diff) | |
download | protobuf-34878cb14eba4578d1d67d2dc93250729d492774.tar.gz protobuf-34878cb14eba4578d1d67d2dc93250729d492774.tar.bz2 protobuf-34878cb14eba4578d1d67d2dc93250729d492774.zip |
Fixes from PR review.
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 6c706014..ad88c4eb 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 ead45737..f5e4fd3a 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<TKey, TValue> : IDeepCloneable<MapField<TKey, TValue>>, IFreezable, IDictionary<TKey, TValue>, IEquatable<MapField<TKey, TValue>>, 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<TKey, LinkedListNode<KeyValuePair<TKey, TValue>>> map = new Dictionary<TKey, LinkedListNode<KeyValuePair<TKey, TValue>>>(); diff --git a/csharp/src/ProtocolBuffers/FieldCodec.cs b/csharp/src/ProtocolBuffers/FieldCodec.cs index caf03286..85462787 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 + /// <summary> + /// Helper code to create codecs for wrapper types. + /// </summary> + /// <remarks> + /// 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. + /// </remarks> 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<Type, object> Codecs = new Dictionary<Type, object> { - { 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)) } }; /// <summary> diff --git a/src/google/protobuf/compiler/csharp/csharp_wrapper_field.h b/src/google/protobuf/compiler/csharp/csharp_wrapper_field.h index 5e2c5bc3..6e2414af 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__ - |