Cleanup MvcJsonHelper (#6529)

pranavkm · web-flow · commit 55ec35bb805f · 2019-01-10T12:34:27.000-08:00
* Cleanup MvcJsonHelper

* Remove dependency on JsonOutputFormatter
* Cache JsonSerializer for the default case
diff --git a/src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/JsonHelperExtensions.cs b/src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/JsonHelperExtensions.cs
@@ -23,6 +23,10 @@ public static class JsonHelperExtensions
         /// The <see cref="JsonSerializerSettings"/> to be used by the serializer.
         /// </param>
         /// <returns>A new <see cref="IHtmlContent"/> containing the serialized JSON.</returns>
+        /// <remarks>
+        /// The value for <see cref="JsonSerializerSettings.StringEscapeHandling" /> from <paramref name="serializerSettings"/>
+        /// is ignored by this method and <see cref="StringEscapeHandling.EscapeHtml"/> is always used.
+        /// </remarks>
         public static IHtmlContent Serialize(
             this IJsonHelper jsonHelper,
             object value,
diff --git a/src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/NewtonsoftJsonHelper.cs b/src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/NewtonsoftJsonHelper.cs
@@ -6,8 +6,8 @@
 using System.Globalization;
 using System.IO;
 using Microsoft.AspNetCore.Html;
-using Microsoft.AspNetCore.Mvc.Formatters;
 using Microsoft.AspNetCore.Mvc.Rendering;
+using Microsoft.Extensions.Options;
 using Newtonsoft.Json;
 
 namespace Microsoft.AspNetCore.Mvc.NewtonsoftJson
@@ -17,97 +17,74 @@ namespace Microsoft.AspNetCore.Mvc.NewtonsoftJson
     /// </summary>
     internal class NewtonsoftJsonHelper : IJsonHelper
     {
-        private readonly NewtonsoftJsonOutputFormatter _jsonOutputFormatter;
-        private readonly ArrayPool<char> _charPool;
+        // Perf: JsonSerializers are relatively expensive to create, and are thread safe. Cache the serializer
+        private readonly JsonSerializer _defaultSettingsJsonSerializer;
+        private readonly IArrayPool<char> _charPool;
 
         /// <summary>
-        /// Initializes a new instance of <see cref="NewtonsoftJsonHelper"/> that is backed by <paramref name="jsonOutputFormatter"/>.
+        /// Initializes a new instance of <see cref="NewtonsoftJsonHelper"/>.
         /// </summary>
-        /// <param name="jsonOutputFormatter">The <see cref="NewtonsoftJsonOutputFormatter"/> used to serialize JSON.</param>
+        /// <param name="options">The <see cref="MvcNewtonsoftJsonOptions"/>.</param>
         /// <param name="charPool">
         /// The <see cref="ArrayPool{Char}"/> for use with custom <see cref="JsonSerializerSettings"/> (see
         /// <see cref="Serialize(object, JsonSerializerSettings)"/>).
         /// </param>
-        public NewtonsoftJsonHelper(NewtonsoftJsonOutputFormatter jsonOutputFormatter, ArrayPool<char> charPool)
+        public NewtonsoftJsonHelper(IOptions<MvcNewtonsoftJsonOptions> options, ArrayPool<char> charPool)
         {
-            if (jsonOutputFormatter == null)
+            if (options == null)
             {
-                throw new ArgumentNullException(nameof(jsonOutputFormatter));
+                throw new ArgumentNullException(nameof(options));
             }
+
             if (charPool == null)
             {
                 throw new ArgumentNullException(nameof(charPool));
             }
 
-            _jsonOutputFormatter = jsonOutputFormatter;
-            _charPool = charPool;
+            _defaultSettingsJsonSerializer = CreateHtmlSafeSerializer(options.Value.SerializerSettings);
+            _charPool = new JsonArrayPool<char>(charPool);
         }
 
-        /// <inheritdoc />
         public IHtmlContent Serialize(object value)
         {
-            var settings = ShallowCopy(_jsonOutputFormatter.PublicSerializerSettings);
-            settings.StringEscapeHandling = StringEscapeHandling.EscapeHtml;
-
-            return Serialize(value, settings);
+            return Serialize(value, _defaultSettingsJsonSerializer);
         }
 
-        /// <inheritdoc />
         public IHtmlContent Serialize(object value, JsonSerializerSettings serializerSettings)
         {
             if (serializerSettings == null)
             {
                 throw new ArgumentNullException(nameof(serializerSettings));
             }
 
-            var jsonOutputFormatter = new NewtonsoftJsonOutputFormatter(serializerSettings, _charPool);
-
-            return SerializeInternal(jsonOutputFormatter, value);
+            var jsonSerializer = CreateHtmlSafeSerializer(serializerSettings);
+            return Serialize(value, jsonSerializer);
         }
 
-        private IHtmlContent SerializeInternal(NewtonsoftJsonOutputFormatter jsonOutputFormatter, object value)
+        private IHtmlContent Serialize(object value, JsonSerializer jsonSerializer)
         {
-            var stringWriter = new StringWriter(CultureInfo.InvariantCulture);
-            jsonOutputFormatter.WriteObject(stringWriter, value);
+            using (var stringWriter = new StringWriter(CultureInfo.InvariantCulture))
+            {
+                var jsonWriter = new JsonTextWriter(stringWriter)
+                {
+                    ArrayPool = _charPool,
+                };
 
-            return new HtmlString(stringWriter.ToString());
+                using (jsonWriter)
+                {
+                    jsonSerializer.Serialize(jsonWriter, value);
+                }
+
+                return new HtmlString(stringWriter.ToString());
+            }
         }
 
-        private static JsonSerializerSettings ShallowCopy(JsonSerializerSettings settings)
+        private static JsonSerializer CreateHtmlSafeSerializer(JsonSerializerSettings serializerSettings)
         {
-            var copiedSettings = new JsonSerializerSettings
-            {
-                FloatParseHandling = settings.FloatParseHandling,
-                FloatFormatHandling = settings.FloatFormatHandling,
-                DateParseHandling = settings.DateParseHandling,
-                DateTimeZoneHandling = settings.DateTimeZoneHandling,
-                DateFormatHandling = settings.DateFormatHandling,
-                Formatting = settings.Formatting,
-                MaxDepth = settings.MaxDepth,
-                DateFormatString = settings.DateFormatString,
-                Context = settings.Context,
-                Error = settings.Error,
-                SerializationBinder = settings.SerializationBinder,
-                TraceWriter = settings.TraceWriter,
-                Culture = settings.Culture,
-                ReferenceResolverProvider = settings.ReferenceResolverProvider,
-                EqualityComparer = settings.EqualityComparer,
-                ContractResolver = settings.ContractResolver,
-                ConstructorHandling = settings.ConstructorHandling,
-                TypeNameAssemblyFormatHandling = settings.TypeNameAssemblyFormatHandling,
-                MetadataPropertyHandling = settings.MetadataPropertyHandling,
-                TypeNameHandling = settings.TypeNameHandling,
-                PreserveReferencesHandling = settings.PreserveReferencesHandling,
-                Converters = settings.Converters,
-                DefaultValueHandling = settings.DefaultValueHandling,
-                NullValueHandling = settings.NullValueHandling,
-                ObjectCreationHandling = settings.ObjectCreationHandling,
-                MissingMemberHandling = settings.MissingMemberHandling,
-                ReferenceLoopHandling = settings.ReferenceLoopHandling,
-                CheckAdditionalContent = settings.CheckAdditionalContent,
-            };
-
-            return copiedSettings;
+            var jsonSerializer = JsonSerializer.Create(serializerSettings);
+            // Ignore the user configured StringEscapeHandling and always escape it.
+            jsonSerializer.StringEscapeHandling = StringEscapeHandling.EscapeHtml;
+            return jsonSerializer;
         }
     }
 }
diff --git a/src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/NewtonsoftJsonOutputFormatter.cs b/src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/NewtonsoftJsonOutputFormatter.cs
@@ -3,7 +3,6 @@
 
 using System;
 using System.Buffers;
-using System.ComponentModel;
 using System.IO;
 using System.Text;
 using System.Threading.Tasks;
@@ -63,36 +62,6 @@ public NewtonsoftJsonOutputFormatter(JsonSerializerSettings serializerSettings,
         /// </remarks>
         protected JsonSerializerSettings SerializerSettings { get; }
 
-        /// <summary>
-        /// Gets the <see cref="JsonSerializerSettings"/> used to configure the <see cref="JsonSerializer"/>.
-        /// </summary>
-        /// <remarks>
-        /// Any modifications to the <see cref="JsonSerializerSettings"/> object after this
-        /// <see cref="NewtonsoftJsonOutputFormatter"/> has been used will have no effect.
-        /// </remarks>
-        [EditorBrowsable(EditorBrowsableState.Never)]
-        public JsonSerializerSettings PublicSerializerSettings => SerializerSettings;
-
-        /// <summary>
-        /// Writes the given <paramref name="value"/> as JSON using the given
-        /// <paramref name="writer"/>.
-        /// </summary>
-        /// <param name="writer">The <see cref="TextWriter"/> used to write the <paramref name="value"/></param>
-        /// <param name="value">The value to write as JSON.</param>
-        public void WriteObject(TextWriter writer, object value)
-        {
-            if (writer == null)
-            {
-                throw new ArgumentNullException(nameof(writer));
-            }
-
-            using (var jsonWriter = CreateJsonWriter(writer))
-            {
-                var jsonSerializer = CreateJsonSerializer();
-                jsonSerializer.Serialize(jsonWriter, value);
-            }
-        }
-
         /// <summary>
         /// Called during serialization to create the <see cref="JsonWriter"/>.
         /// </summary>
@@ -145,7 +114,11 @@ public override async Task WriteResponseBodyAsync(OutputFormatterWriteContext co
             var response = context.HttpContext.Response;
             using (var writer = context.WriterFactory(response.Body, selectedEncoding))
             {
-                WriteObject(writer, context.Object);
+                using (var jsonWriter = CreateJsonWriter(writer))
+                {
+                    var jsonSerializer = CreateJsonSerializer();
+                    jsonSerializer.Serialize(jsonWriter, context.Object);
+                }
 
                 // Perf: call FlushAsync to call WriteAsync on the stream with any content left in the TextWriter's
                 // buffers. This is better than just letting dispose handle it (which would result in a synchronous
diff --git a/src/Mvc/test/Microsoft.AspNetCore.Mvc.FunctionalTests/BasicTests.cs b/src/Mvc/test/Microsoft.AspNetCore.Mvc.FunctionalTests/BasicTests.cs
@@ -265,7 +265,7 @@ public async Task JsonHelper_RendersJson_WithCamelCaseNames()
         public async Task JsonHelperWithSettings_RendersJson_WithNamesUnchanged()
         {
             // Arrange
-            var json = "{\"id\":9000,\"FullName\":\"John <b>Smith</b>\"}";
+            var json = "{\"id\":9000,\"FullName\":\"John \\u003cb\\u003eSmith\\u003c/b\\u003e\"}";
             var expectedBody = string.Format(
                 @"<script type=""text/javascript"">
     var json = {0};
@@ -287,7 +287,7 @@ public async Task JsonHelperWithSettings_RendersJson_WithNamesUnchanged()
         public async Task JsonHelperWithSettings_RendersJson_WithSnakeCaseNames()
         {
             // Arrange
-            var json = "{\"id\":9000,\"full_name\":\"John <b>Smith</b>\"}";
+            var json = "{\"id\":9000,\"full_name\":\"John \\u003cb\\u003eSmith\\u003c/b\\u003e\"}";
             var expectedBody = string.Format(
                 @"<script type=""text/javascript"">
     var json = {0};
diff --git a/src/Mvc/test/Microsoft.AspNetCore.Mvc.NewtonsoftJson.Test/JsonHelperTest.cs b/src/Mvc/test/Microsoft.AspNetCore.Mvc.NewtonsoftJson.Test/JsonHelperTest.cs
diff --git a/src/Mvc/test/Microsoft.AspNetCore.Mvc.NewtonsoftJson.Test/NewtonsoftJsonHelperTest.cs b/src/Mvc/test/Microsoft.AspNetCore.Mvc.NewtonsoftJson.Test/NewtonsoftJsonHelperTest.cs
@@ -0,0 +1,95 @@
+﻿// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System.Buffers;
+using Microsoft.AspNetCore.Html;
+using Microsoft.Extensions.Options;
+using Newtonsoft.Json;
+using Newtonsoft.Json.Serialization;
+using Xunit;
+
+namespace Microsoft.AspNetCore.Mvc.NewtonsoftJson
+{
+    public class NewtonsoftJsonHelperTest
+    {
+        [Fact]
+        public void Serialize_EscapesHtmlByDefault()
+        {
+            // Arrange
+            var options = new MvcNewtonsoftJsonOptions();
+            options.SerializerSettings.StringEscapeHandling = StringEscapeHandling.EscapeNonAscii;
+            var helper = new NewtonsoftJsonHelper(
+                Options.Create(options),
+                ArrayPool<char>.Shared);
+            var obj = new
+            {
+                HTML = "<b>John Doe</b>"
+            };
+            var expectedOutput = "{\"html\":\"\\u003cb\\u003eJohn Doe\\u003c/b\\u003e\"}";
+
+            // Act
+            var result = helper.Serialize(obj);
+
+            // Assert
+            var htmlString = Assert.IsType<HtmlString>(result);
+            Assert.Equal(expectedOutput, htmlString.ToString());
+        }
+
+        [Fact]
+        public void Serialize_MaintainsSettingsAndEscapesHtml()
+        {
+            // Arrange
+            var options = new MvcNewtonsoftJsonOptions();
+            options.SerializerSettings.ContractResolver = new DefaultContractResolver
+            {
+                NamingStrategy = new DefaultNamingStrategy(),
+            };
+            var helper = new NewtonsoftJsonHelper(
+                Options.Create(options),
+                ArrayPool<char>.Shared);
+            var obj = new
+            {
+                FullHtml = "<b>John Doe</b>"
+            };
+            var expectedOutput = "{\"FullHtml\":\"\\u003cb\\u003eJohn Doe\\u003c/b\\u003e\"}";
+
+            // Act
+            var result = helper.Serialize(obj);
+
+            // Assert
+            var htmlString = Assert.IsType<HtmlString>(result);
+            Assert.Equal(expectedOutput, htmlString.ToString());
+        }
+
+        [Fact]
+        public void Serialize_UsesHtmlSafeVersionOfPassedInSettings()
+        {
+            // Arrange
+            var options = new MvcNewtonsoftJsonOptions();
+            var helper = new NewtonsoftJsonHelper(
+                Options.Create(options),
+                ArrayPool<char>.Shared);
+            var obj = new
+            {
+                FullHtml = "<b>John Doe</b>"
+            };
+            var serializerSettings = new JsonSerializerSettings
+            {
+                StringEscapeHandling = StringEscapeHandling.Default,
+                ContractResolver = new DefaultContractResolver
+                {
+                    NamingStrategy = new SnakeCaseNamingStrategy(),
+                }
+            };
+
+            var expectedOutput = "{\"full_html\":\"\\u003cb\\u003eJohn Doe\\u003c/b\\u003e\"}";
+
+            // Act
+            var result = helper.Serialize(obj, serializerSettings);
+
+            // Assert
+            var htmlString = Assert.IsType<HtmlString>(result);
+            Assert.Equal(expectedOutput, htmlString.ToString());
+        }
+    }
+}
diff --git a/src/Mvc/test/Microsoft.AspNetCore.Mvc.NewtonsoftJson.Test/NewtonsoftJsonOutputFormatterTest.cs b/src/Mvc/test/Microsoft.AspNetCore.Mvc.NewtonsoftJson.Test/NewtonsoftJsonOutputFormatterTest.cs
