Prohibit null values in map fields

On deserialization, missing values for message types
are replaced with a "default" message.

1 files changed, 20 insertions, 33 deletions

diff --git a/csharp/src/Google.Protobuf/Collections/MapField.cs b/csharp/src/Google.Protobuf/Collections/MapField.cs
index c0ed28ae..04754ae3 100644
--- a/csharp/src/Google.Protobuf/Collections/MapField.cs
+++ b/csharp/src/Google.Protobuf/Collections/MapField.cs
@@ -53,6 +53,13 @@ namespace Google.Protobuf.Collections
     /// For string keys, the equality comparison is provided by <see cref="StringComparer.Ordinal" />.
     /// </para>
     /// <para>
+    /// Null values are not permitted in the map, either for wrapper types or regular messages.
+    /// If a map is deserialized from a data stream and the value is missing from an entry, a default value
+    /// is created instead. For primitive types, that is the regular default value (0, the empty string and so
+    /// on); for message types, an empty instance of the message is created, as if the map entry contained a 0-length
+    /// encoded value for the field.
+    /// </para>
+    /// <para>
     /// This implementation does not generally prohibit the use of key/value types which are not
     /// supported by Protocol Buffers (e.g. using a key type of <code>byte</code>) but nor does it guarantee
     /// that all operations will work in such cases.
@@ -61,35 +68,11 @@ namespace Google.Protobuf.Collections
     public sealed class MapField<TKey, TValue> : IDeepCloneable<MapField<TKey, TValue>>, 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 readonly bool allowNullValues;
         private readonly Dictionary<TKey, LinkedListNode<KeyValuePair<TKey, TValue>>> map =
             new Dictionary<TKey, LinkedListNode<KeyValuePair<TKey, TValue>>>();
         private readonly LinkedList<KeyValuePair<TKey, TValue>> list = new LinkedList<KeyValuePair<TKey, TValue>>();
 
         /// <summary>
-        /// Constructs a new map field, defaulting the value nullability to only allow null values for message types
-        /// and non-nullable value types.
-        /// </summary>
-        public MapField() : this(typeof(IMessage).IsAssignableFrom(typeof(TValue)) || Nullable.GetUnderlyingType(typeof(TValue)) != null)
-        {
-        }
-
-        /// <summary>
-        /// Constructs a new map field, overriding the choice of whether null values are permitted in the map.
-        /// This is used by wrapper types, where maps with string and bytes wrappers as the value types
-        /// support null values.
-        /// </summary>
-        /// <param name="allowNullValues">Whether null values are permitted in the map or not.</param>
-        public MapField(bool allowNullValues)
-        {
-            if (allowNullValues && typeof(TValue).IsValueType() && Nullable.GetUnderlyingType(typeof(TValue)) == null)
-            {
-                throw new ArgumentException("allowNullValues", "Non-nullable value types do not support null values");
-            }
-            this.allowNullValues = allowNullValues;
-        }
-
-        /// <summary>
         /// Creates a deep clone of this object.
         /// </summary>
         /// <returns>
@@ -97,13 +80,13 @@ namespace Google.Protobuf.Collections
         /// </returns>
         public MapField<TKey, TValue> Clone()
         {
-            var clone = new MapField<TKey, TValue>(allowNullValues);
+            var clone = new MapField<TKey, TValue>();
             // Keys are never cloneable. Values might be.
             if (typeof(IDeepCloneable<TValue>).IsAssignableFrom(typeof(TValue)))
             {
                 foreach (var pair in list)
                 {
-                    clone.Add(pair.Key, pair.Value == null ? pair.Value : ((IDeepCloneable<TValue>)pair.Value).Clone());
+                    clone.Add(pair.Key, ((IDeepCloneable<TValue>)pair.Value).Clone());
                 }
             }
             else
@@ -217,7 +200,7 @@ namespace Google.Protobuf.Collections
             {
                 Preconditions.CheckNotNullUnconstrained(key, "key");
                 // value == null check here is redundant, but avoids boxing.
-                if (value == null && !allowNullValues)
+                if (value == null)
                 {
                     Preconditions.CheckNotNullUnconstrained(value, "value");
                 }
@@ -246,7 +229,7 @@ namespace Google.Protobuf.Collections
         public ICollection<TValue> Values { get { return new MapView<TValue>(this, pair => pair.Value, ContainsValue); } }
 
         /// <summary>
-        /// Adds the specified entries to the map.
+        /// Adds the specified entries to the map. The keys and values are not automatically cloned.
         /// </summary>
         /// <param name="entries">The entries to add to the map.</param>
         public void Add(IDictionary<TKey, TValue> entries)
@@ -347,11 +330,6 @@ namespace Google.Protobuf.Collections
         }
 
         /// <summary>
-        /// Returns whether or not this map allows values to be null.
-        /// </summary>
-        public bool AllowsNullValues { get { return allowNullValues; } }
-
-        /// <summary>
         /// Gets the number of elements contained in the map.
         /// </summary>
         public int Count { get { return list.Count; } }
@@ -632,6 +610,8 @@ namespace Google.Protobuf.Collections
             /// </summary>
             internal class MessageAdapter : IMessage
             {
+                private static readonly byte[] ZeroLengthMessageStreamData = new byte[] { 0 };
+
                 private readonly Codec codec;
                 internal TKey Key { get; set; }
                 internal TValue Value { get; set; }
@@ -665,6 +645,13 @@ namespace Google.Protobuf.Collections
                             input.SkipLastField();
                         }
                     }
+
+                    // Corner case: a map entry with a key but no value, where the value type is a message.
+                    // Read it as if we'd seen an input stream with no data (i.e. create a "default" message).
+                    if (Value == null)
+                    {
+                        Value = codec.valueCodec.Read(new CodedInputStream(ZeroLengthMessageStreamData));
+                    }
                 }
 
                 public void WriteTo(CodedOutputStream output)