PR comments: doc comments, a missed method, and more tests

Author: dougbu

src/Microsoft.AspNetCore.Http.Features/IResponseCookies.cs

@@ -4,36 +4,39 @@
 namespace Microsoft.AspNetCore.Http
 {
     /// <summary>
-    /// A wrapper for the response Set-Cookie header
+    /// A wrapper for the response Set-Cookie header.
     /// </summary>
     public interface IResponseCookies
     {
         /// <summary>
-        /// Add a new cookie and value
+        /// Add a new cookie and value.
         /// </summary>
-        /// <param name="key"></param>
-        /// <param name="value"></param>
+        /// <param name="key">Name of the new cookie.</param>
+        /// <param name="value">Value of the new cookie.</param>
         void Append(string key, string value);
 
         /// <summary>
-        /// Add a new cookie
+        /// Add a new cookie.
         /// </summary>
-        /// <param name="key"></param>
-        /// <param name="value"></param>
-        /// <param name="options"></param>
+        /// <param name="key">Name of the new cookie.</param>
+        /// <param name="value">Value of the new cookie.</param>
+        /// <param name="options"><see cref="CookieOptions"/> included in the new cookie setting.</param>
         void Append(string key, string value, CookieOptions options);
 
         /// <summary>
-        /// Sets an expired cookie
+        /// Sets an expired cookie.
         /// </summary>
-        /// <param name="key"></param>
+        /// <param name="key">Name of the cookie to expire.</param>
         void Delete(string key);
 
         /// <summary>
-        /// Sets an expired cookie
+        /// Sets an expired cookie.
         /// </summary>
-        /// <param name="key"></param>
-        /// <param name="options"></param>
+        /// <param name="key">Name of the cookie to expire.</param>
+        /// <param name="options">
+        /// <see cref="CookieOptions"/> used to discriminate the particular cookie to expire. The
+        /// <see cref="CookieOptions.Domain"/> and <see cref="CookieOptions.Path"/> values are especially important.
+        /// </param>
         void Delete(string key, CookieOptions options);
     }
 }

src/Microsoft.AspNetCore.Http.Features/IResponseCookiesFeature.cs

@@ -4,12 +4,12 @@
 namespace Microsoft.AspNetCore.Http.Features
 {
     /// <summary>
-    /// Feature containing cookies which will be returned with the response.
+    /// A helper for creating the response Set-Cookie header.
     /// </summary>
     public interface IResponseCookiesFeature
     {
         /// <summary>
-        /// Cookies which will be returned with the response.
+        /// Gets the wrapper for the response Set-Cookie header.
         /// </summary>
         IResponseCookies Cookies { get; }
     }

src/Microsoft.AspNetCore.Http/ResponseCookies.cs

@@ -11,14 +11,14 @@
 namespace Microsoft.AspNetCore.Http.Internal
 {
     /// <summary>
-    /// A wrapper for the response Set-Cookie header
+    /// A wrapper for the response Set-Cookie header.
     /// </summary>
     public class ResponseCookies : IResponseCookies
     {
         private readonly ObjectPool<StringBuilder> _builderPool;
 
         /// <summary>
-        /// Create a new wrapper
+        /// Create a new wrapper.
         /// </summary>
         /// <param name="headers">The <see cref="IHeaderDictionary"/> for the response.</param>
         /// <param name="builderPool">The <see cref="ObjectPool{T}"/>, if available.</param>
@@ -35,11 +35,7 @@ public ResponseCookies(IHeaderDictionary headers, ObjectPool<StringBuilder> buil
 
         private IHeaderDictionary Headers { get; set; }
 
-        /// <summary>
-        /// Add a new cookie and value
-        /// </summary>
-        /// <param name="key"></param>
-        /// <param name="value"></param>
+        /// <inheritdoc />
         public void Append(string key, string value)
         {
             var setCookieHeaderValue = new SetCookieHeaderValue(
@@ -71,12 +67,7 @@ public void Append(string key, string value)
             Headers[HeaderNames.SetCookie] = StringValues.Concat(Headers[HeaderNames.SetCookie], cookieValue);
         }
 
-        /// <summary>
-        /// Add a new cookie
-        /// </summary>
-        /// <param name="key"></param>
-        /// <param name="value"></param>
-        /// <param name="options"></param>
+        /// <inheritdoc />
         public void Append(string key, string value, CookieOptions options)
         {
             if (options == null)
@@ -96,34 +87,34 @@ public void Append(string key, string value, CookieOptions options)
             };
 
             string cookieValue;
-            var stringBuilder = _builderPool.Get();
-            try
+            if (_builderPool == null)
             {
-                setCookieHeaderValue.AppendToStringBuilder(stringBuilder);
-                cookieValue = stringBuilder.ToString();
+                cookieValue = setCookieHeaderValue.ToString();
             }
-            finally
+            else
             {
-                _builderPool.Return(stringBuilder);
+                var stringBuilder = _builderPool.Get();
+                try
+                {
+                    setCookieHeaderValue.AppendToStringBuilder(stringBuilder);
+                    cookieValue = stringBuilder.ToString();
+                }
+                finally
+                {
+                    _builderPool.Return(stringBuilder);
+                }
             }
 
             Headers[HeaderNames.SetCookie] = StringValues.Concat(Headers[HeaderNames.SetCookie], cookieValue);
         }
 
-        /// <summary>
-        /// Sets an expired cookie
-        /// </summary>
-        /// <param name="key"></param>
+        /// <inheritdoc />
         public void Delete(string key)
         {
             Delete(key, new CookieOptions() { Path = "/" });
         }
 
-        /// <summary>
-        /// Sets an expired cookie
-        /// </summary>
-        /// <param name="key"></param>
-        /// <param name="options"></param>
+        /// <inheritdoc />
         public void Delete(string key, CookieOptions options)
         {
             if (options == null)

test/Microsoft.AspNetCore.Http.Tests/ResponseCookiesTest.cs

@@ -14,11 +14,24 @@ public class ResponseCookiesTest
         private static readonly ObjectPool<StringBuilder> _builderPool =
             new DefaultObjectPoolProvider().Create<StringBuilder>(new StringBuilderPooledObjectPolicy());
 
-        [Fact]
-        public void DeleteCookieShouldSetDefaultPath()
+        public static TheoryData BuilderPoolData
+        {
+            get
+            {
+                return new TheoryData<ObjectPool<StringBuilder>>
+                {
+                    null,
+                    _builderPool,
+                };
+            }
+        }
+
+        [Theory]
+        [MemberData(nameof(BuilderPoolData))]
+        public void DeleteCookieShouldSetDefaultPath(ObjectPool<StringBuilder> builderPool)
         {
             var headers = new HeaderDictionary();
-            var cookies = new ResponseCookies(headers, _builderPool);
+            var cookies = new ResponseCookies(headers, builderPool);
             var testcookie = "TestCookie";
 
             cookies.Delete(testcookie);
@@ -30,11 +43,12 @@ public void DeleteCookieShouldSetDefaultPath()
             Assert.Contains("expires=Thu, 01 Jan 1970 00:00:00 GMT", cookieHeaderValues[0]);
         }
 
-        [Fact]
-        public void NoParamsDeleteRemovesCookieCreatedByAdd()
+        [Theory]
+        [MemberData(nameof(BuilderPoolData))]
+        public void NoParamsDeleteRemovesCookieCreatedByAdd(ObjectPool<StringBuilder> builderPool)
         {
             var headers = new HeaderDictionary();
-            var cookies = new ResponseCookies(headers, _builderPool);
+            var cookies = new ResponseCookies(headers, builderPool);
             var testcookie = "TestCookie";
 
             cookies.Append(testcookie, testcookie);
@@ -47,14 +61,33 @@ public void NoParamsDeleteRemovesCookieCreatedByAdd()
             Assert.Contains("expires=Thu, 01 Jan 1970 00:00:00 GMT", cookieHeaderValues[0]);
         }
 
+        public static TheoryData EscapesKeyValuesBeforeSettingCookieData
+        {
+            get
+            {
+                // key, value, object pool, expected
+                return new TheoryData<string, string, ObjectPool<StringBuilder>, string>
+                {
+                    { "key", "value", null, "key=value" },
+                    { "key,", "!value", null, "key%2C=%21value" },
+                    { "ke#y,", "val^ue", null, "ke%23y%2C=val%5Eue" },
+                    { "key", "value", _builderPool, "key=value" },
+                    { "key,", "!value", _builderPool, "key%2C=%21value" },
+                    { "ke#y,", "val^ue", _builderPool, "ke%23y%2C=val%5Eue" },
+                };
+            }
+        }
+
         [Theory]
-        [InlineData("key", "value", "key=value")]
-        [InlineData("key,", "!value", "key%2C=%21value")]
-        [InlineData("ke#y,", "val^ue", "ke%23y%2C=val%5Eue")]
-        public void EscapesKeyValuesBeforeSettingCookie(string key, string value, string expected)
+        [MemberData(nameof(EscapesKeyValuesBeforeSettingCookieData))]
+        public void EscapesKeyValuesBeforeSettingCookie(
+            string key,
+            string value,
+            ObjectPool<StringBuilder> builderPool,
+            string expected)
         {
             var headers = new HeaderDictionary();
-            var cookies = new ResponseCookies(headers, _builderPool);
+            var cookies = new ResponseCookies(headers, builderPool);
 
             cookies.Append(key, value);