metadata polishing

7 files changed, 219 insertions, 19 deletions

diff --git a/src/csharp/Grpc.Core.Tests/Grpc.Core.Tests.csproj b/src/csharp/Grpc.Core.Tests/Grpc.Core.Tests.csproj
index d6a8f52570..829effc9a2 100644
--- a/src/csharp/Grpc.Core.Tests/Grpc.Core.Tests.csproj
+++ b/src/csharp/Grpc.Core.Tests/Grpc.Core.Tests.csproj
@@ -82,6 +82,7 @@
     <Compile Include="ResponseHeadersTest.cs" />
     <Compile Include="CompressionTest.cs" />
     <Compile Include="ContextPropagationTest.cs" />
+    <Compile Include="MetadataTest.cs" />
   </ItemGroup>
   <Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />
   <ItemGroup>
diff --git a/src/csharp/Grpc.Core.Tests/MetadataTest.cs b/src/csharp/Grpc.Core.Tests/MetadataTest.cs
new file mode 100644
index 0000000000..6f616cd316
--- /dev/null
+++ b/src/csharp/Grpc.Core.Tests/MetadataTest.cs
@@ -0,0 +1,112 @@
+#region Copyright notice and license
+
+// Copyright 2015, Google Inc.
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+#endregion
+
+using System;
+using System.Diagnostics;
+using System.Runtime.InteropServices;
+using System.Threading;
+using System.Threading.Tasks;
+using Grpc.Core;
+using Grpc.Core.Internal;
+using Grpc.Core.Utils;
+using NUnit.Framework;
+
+namespace Grpc.Core.Tests
+{
+    public class MetadataTest
+    {
+        [Test]
+        public void AsciiEntry()
+        {
+            var entry = new Metadata.Entry("ABC", "XYZ");
+            Assert.AreEqual("abc", entry.Key);  // key is in lowercase.
+            Assert.AreEqual("XYZ", entry.Value);
+            CollectionAssert.AreEqual(new[] { (byte)'X', (byte)'Y', (byte)'Z' }, entry.ValueBytes);
+
+            Assert.Throws(typeof(ArgumentException), () => new Metadata.Entry("abc-bin", "xyz"));
+        }
+
+        [Test]
+        public void BinaryEntry()
+        {
+            var bytes = new byte[] { 1, 2, 3 };
+            var entry = new Metadata.Entry("ABC-BIN", bytes);
+            Assert.AreEqual("abc-bin", entry.Key);  // key is in lowercase.
+            Assert.Throws(typeof(InvalidOperationException), () => { var v = entry.Value; });
+            CollectionAssert.AreEqual(bytes, entry.ValueBytes);
+
+            Assert.Throws(typeof(ArgumentException), () => new Metadata.Entry("abc", bytes));
+        }
+
+        [Test]
+        public void Entry_ConstructionPreconditions()
+        {
+            Assert.Throws(typeof(ArgumentNullException), () => new Metadata.Entry(null, "xyz"));
+            Assert.Throws(typeof(ArgumentNullException), () => new Metadata.Entry("abc", (string)null));
+            Assert.Throws(typeof(ArgumentNullException), () => new Metadata.Entry("abc-bin", (byte[])null));
+        }
+
+        [Test]
+        public void Entry_Immutable()
+        {
+            var origBytes = new byte[] { 1, 2, 3 };
+            var bytes = new byte[] { 1, 2, 3 };
+            var entry = new Metadata.Entry("ABC-BIN", bytes);
+            bytes[0] = 255;  // changing the array passed to constructor should have any effect.
+            CollectionAssert.AreEqual(origBytes, entry.ValueBytes);
+
+            entry.ValueBytes[0] = 255;
+            CollectionAssert.AreEqual(origBytes, entry.ValueBytes);
+        }
+
+        [Test]
+        public void Entry_CreateUnsafe_Ascii()
+        {
+            var bytes = new byte[] { (byte)'X', (byte)'y' };
+            var entry = Metadata.Entry.CreateUnsafe("abc", bytes);
+            Assert.AreEqual("abc", entry.Key);
+            Assert.AreEqual("Xy", entry.Value);
+            CollectionAssert.AreEqual(bytes, entry.ValueBytes);
+        }
+
+        [Test]
+        public void Entry_CreateUnsafe_Binary()
+        {
+            var bytes = new byte[] { 1, 2, 3 };
+            var entry = Metadata.Entry.CreateUnsafe("abc-bin", bytes);
+            Assert.AreEqual("abc-bin", entry.Key);
+            Assert.Throws(typeof(InvalidOperationException), () => { var v = entry.Value; });
+            CollectionAssert.AreEqual(bytes, entry.ValueBytes);
+        }
+    }
+}
diff --git a/src/csharp/Grpc.Core/ClientBase.cs b/src/csharp/Grpc.Core/ClientBase.cs
index 7bc100ca60..903449439b 100644
--- a/src/csharp/Grpc.Core/ClientBase.cs
+++ b/src/csharp/Grpc.Core/ClientBase.cs
@@ -119,7 +119,8 @@ namespace Grpc.Core
         internal static string GetAuthUriBase(string target)
         {
             var match = ChannelTargetPattern.Match(target);
-            if (!match.Success) {
+            if (!match.Success)
+            {
                 return null;
             }
             return "https://" + match.Groups[2].Value + "/";
diff --git a/src/csharp/Grpc.Core/ContextPropagationToken.cs b/src/csharp/Grpc.Core/ContextPropagationToken.cs
index 2e4bfc9e47..a5bf1b5a70 100644
--- a/src/csharp/Grpc.Core/ContextPropagationToken.cs
+++ b/src/csharp/Grpc.Core/ContextPropagationToken.cs
@@ -132,7 +132,6 @@ namespace Grpc.Core
         bool propagateDeadline;
         bool propagateCancellation;
 
-
         /// <summary>
         /// Creates new context propagation options.
         /// </summary>
diff --git a/src/csharp/Grpc.Core/Internal/MetadataArraySafeHandle.cs b/src/csharp/Grpc.Core/Internal/MetadataArraySafeHandle.cs
index 427c16fac6..83994f6762 100644
--- a/src/csharp/Grpc.Core/Internal/MetadataArraySafeHandle.cs
+++ b/src/csharp/Grpc.Core/Internal/MetadataArraySafeHandle.cs
@@ -70,7 +70,8 @@ namespace Grpc.Core.Internal
             var metadataArray = grpcsharp_metadata_array_create(new UIntPtr((ulong)metadata.Count));
             for (int i = 0; i < metadata.Count; i++)
             {
-                grpcsharp_metadata_array_add(metadataArray, metadata[i].Key, metadata[i].ValueBytes, new UIntPtr((ulong)metadata[i].ValueBytes.Length));
+                var valueBytes = metadata[i].GetSerializedValueUnsafe();
+                grpcsharp_metadata_array_add(metadataArray, metadata[i].Key, valueBytes, new UIntPtr((ulong)valueBytes.Length));
             }
             return metadataArray;
         }
@@ -94,7 +95,7 @@ namespace Grpc.Core.Internal
                 string key = Marshal.PtrToStringAnsi(grpcsharp_metadata_array_get_key(metadataArray, index));
                 var bytes = new byte[grpcsharp_metadata_array_get_value_length(metadataArray, index).ToUInt64()];
                 Marshal.Copy(grpcsharp_metadata_array_get_value(metadataArray, index), bytes, 0, bytes.Length);
-                metadata.Add(new Metadata.Entry(key, bytes));
+                metadata.Add(Metadata.Entry.CreateUnsafe(key, bytes));
             }
             return metadata;
         }
diff --git a/src/csharp/Grpc.Core/Metadata.cs b/src/csharp/Grpc.Core/Metadata.cs
index 9db2abf46e..a589b50caa 100644
--- a/src/csharp/Grpc.Core/Metadata.cs
+++ b/src/csharp/Grpc.Core/Metadata.cs
@@ -46,6 +46,11 @@ namespace Grpc.Core
     public sealed class Metadata : IList<Metadata.Entry>
     {
         /// <summary>
+        /// All binary headers should have this suffix.
+        /// </summary>
+        public const string BinaryHeaderSuffix = "-bin";
+
+        /// <summary>
         /// An read-only instance of metadata containing no entries.
         /// </summary>
         public static readonly Metadata Empty = new Metadata().Freeze();
@@ -181,23 +186,49 @@ namespace Grpc.Core
             private static readonly Encoding Encoding = Encoding.ASCII;
 
             readonly string key;
-            string value;
-            byte[] valueBytes;
+            readonly string value;
+            readonly byte[] valueBytes;
+
+            private Entry(string key, string value, byte[] valueBytes)
+            {
+                this.key = key;
+                this.value = value;
+                this.valueBytes = valueBytes;
+            }
 
+            /// <summary>
+            /// Initializes a new instance of the <see cref="Grpc.Core.Metadata+Entry"/> struct with a binary value.
+            /// </summary>
+            /// <param name="key">Metadata key, needs to have suffix indicating a binary valued metadata entry.</param>
+            /// <param name="valueBytes">Value bytes.</param>
             public Entry(string key, byte[] valueBytes)
             {
-                this.key = Preconditions.CheckNotNull(key, "key");
+                this.key = NormalizeKey(key);
+                Preconditions.CheckArgument(this.key.EndsWith(BinaryHeaderSuffix),
+                    "Key for binary valued metadata entry needs to have suffix indicating binary value.");
                 this.value = null;
-                this.valueBytes = Preconditions.CheckNotNull(valueBytes, "valueBytes");
+                Preconditions.CheckNotNull(valueBytes, "valueBytes");
+                this.valueBytes = new byte[valueBytes.Length];
+                Buffer.BlockCopy(valueBytes, 0, this.valueBytes, 0, valueBytes.Length);  // defensive copy to guarantee immutability
             }
 
+            /// <summary>
+            /// Initializes a new instance of the <see cref="Grpc.Core.Metadata+Entry"/> struct holding an ASCII value.
+            /// </summary>
+            /// <param name="key">Metadata key, must not use suffix indicating a binary valued metadata entry.</param>
+            /// <param name="value">Value string. Only ASCII characters are allowed.</param>
             public Entry(string key, string value)
             {
-                this.key = Preconditions.CheckNotNull(key, "key");
+                this.key = NormalizeKey(key);
+                Preconditions.CheckArgument(!this.key.EndsWith(BinaryHeaderSuffix),
+                    "Key for ASCII valued metadata entry cannot have suffix indicating binary value.");
                 this.value = Preconditions.CheckNotNull(value, "value");
                 this.valueBytes = null;
             }
 
+            /// <summary>
+            /// Gets the metadata entry key.
+            /// </summary>
             public string Key
             {
                 get
@@ -206,33 +237,86 @@ namespace Grpc.Core
                 }
             }
 
+            /// <summary>
+            /// Gets the binary value of this metadata entry.
+            /// </summary>
             public byte[] ValueBytes
             {
                 get
                 {
                     if (valueBytes == null)
                     {
-                        valueBytes = Encoding.GetBytes(value);
+                        return Encoding.GetBytes(value);
                     }
-                    return valueBytes;
+
+                    // defensive copy to guarantee immutability
+                    var bytes = new byte[valueBytes.Length];
+                    Buffer.BlockCopy(valueBytes, 0, bytes, 0, valueBytes.Length);
+                    return bytes;
                 }
             }
 
+            /// <summary>
+            /// Gets the string value of this metadata entry.
+            /// </summary>
             public string Value
             {
                 get
                 {
-                    if (value == null)
-                    {
-                        value = Encoding.GetString(valueBytes);
-                    }
-                    return value;
+                    Preconditions.CheckState(!IsBinary, "Cannot access string value of a binary metadata entry");
+                    return value ?? Encoding.GetString(valueBytes);
                 }
             }
-                
+
+            /// <summary>
+            /// Returns <c>true</c> if this entry is a binary-value entry.
+            /// </summary>
+            public bool IsBinary
+            {
+                get
+                {
+                    return value == null;
+                }
+            }
+
+            /// <summary>
+            /// Returns a <see cref="System.String"/> that represents the current <see cref="Grpc.Core.Metadata+Entry"/>.
+            /// </summary>
             public override string ToString()
             {
-                return string.Format("[Entry: key={0}, value={1}]", Key, Value);
+                if (IsBinary)
+                {
+                    return string.Format("[Entry: key={0}, valueBytes={1}]", key, valueBytes);
+                }
+                
+                return string.Format("[Entry: key={0}, value={1}]", key, value);
+            }
+
+            /// <summary>
+            /// Gets the serialized value for this entry. For binary metadata entries, this leaks
+            /// the internal <c>valueBytes</c> byte array and caller must not change contents of it.
+            /// </summary>
+            internal byte[] GetSerializedValueUnsafe()
+            {
+                return valueBytes ?? Encoding.GetBytes(value);
+            }
+
+            /// <summary>
+            /// Creates a binary value or ascii value metadata entry from data received from the native layer.
+            /// We trust C core to give us well-formed data, so we don't perform any checks or defensive copying.
+            /// </summary>
+            internal static Entry CreateUnsafe(string key, byte[] valueBytes)
+            {
+                if (key.EndsWith(BinaryHeaderSuffix))
+                {
+                    return new Entry(key, null, valueBytes);
+                }
+                return new Entry(key, Encoding.GetString(valueBytes), null);
+            }
+
+            private static string NormalizeKey(string key)
+            {
+                return Preconditions.CheckNotNull(key, "key").ToLower();
             }
         }
     }
diff --git a/src/csharp/Grpc.IntegrationTesting/InteropClient.cs b/src/csharp/Grpc.IntegrationTesting/InteropClient.cs
index f4b0a1028f..423da2801e 100644
--- a/src/csharp/Grpc.IntegrationTesting/InteropClient.cs
+++ b/src/csharp/Grpc.IntegrationTesting/InteropClient.cs
@@ -37,13 +37,15 @@ using System.Text.RegularExpressions;
 using System.Threading;
 using System.Threading.Tasks;
 
+using Google.Apis.Auth.OAuth2;
 using Google.ProtocolBuffers;
+
 using grpc.testing;
 using Grpc.Auth;
 using Grpc.Core;
 using Grpc.Core.Utils;
+
 using NUnit.Framework;
-using Google.Apis.Auth.OAuth2;
 
 namespace Grpc.IntegrationTesting
 {