Ensure EnableBuffering works with NewtonsoftJsonInputFormatter (#14870)

Fixes #14396
dotnet · Oct 10, 2019 · fcc20ac · fcc20ac
1 parent 1377ced
commit fcc20ac
Show file tree

Hide file tree

Showing 6 changed files with 110 additions and 7 deletions.
diff --git a/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs b/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs
@@ -12,7 +12,9 @@
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
 using Microsoft.Extensions.Logging.Testing;
+using Microsoft.AspNetCore.WebUtilities;
 using Newtonsoft.Json;
+using Moq;
 using Xunit;
 
 namespace Microsoft.AspNetCore.Mvc.Formatters
@@ -462,6 +464,30 @@ public virtual async Task ReadAsync_RequiredAttribute()
             Assert.Single(formatterContext.ModelState["Person.Name"].Errors);
         }
 
+        [Fact]
+        public async Task ReadAsync_DoesNotDisposeBufferedReadStream()
+        {
+            // Arrange
+            var formatter = GetInputFormatter();
+
+            var content = "{\"name\": \"Test\"}";
+            var contentBytes = Encoding.UTF8.GetBytes(content);
+            var httpContext = GetHttpContext(contentBytes);
+            var testBufferedReadStream = new Mock<FileBufferingReadStream>(httpContext.Request.Body, 1024) { CallBase = true };
+            httpContext.Request.Body = testBufferedReadStream.Object;
+
+            var formatterContext = CreateInputFormatterContext(typeof(ComplexModel), httpContext);
+
+            // Act
+            var result = await formatter.ReadAsync(formatterContext);
+
+            // Assert
+            var userModel = Assert.IsType<ComplexModel>(result.Model);
+            Assert.Equal("Test", userModel.Name);
+
+            testBufferedReadStream.Verify(v => v.DisposeAsync(), Times.Never());
+        }
+
         internal abstract string JsonFormatter_EscapedKeys_Bracket_Expected { get; }
 
         internal abstract string JsonFormatter_EscapedKeys_Expected { get; }
@@ -517,7 +543,7 @@ public virtual async Task ReadAsync_RequiredAttribute()
         protected sealed class ComplexPoco
         {
             public int Id { get; set; }
-            public Person Person{ get; set; }
+            public Person Person { get; set; }
         }
 
         protected sealed class Person

diff --git a/src/Mvc/Mvc.Formatters.Xml/src/XmlDataContractSerializerInputFormatter.cs b/src/Mvc/Mvc.Formatters.Xml/src/XmlDataContractSerializerInputFormatter.cs
@@ -118,6 +118,7 @@ public override async Task<InputFormatterResult> ReadRequestBodyAsync(InputForma
 
             var request = context.HttpContext.Request;
             Stream readStream = new NonDisposableStream(request.Body);
+            var disposeReadStream = false;
 
             if (!request.Body.CanSeek && !_options.SuppressInputFormatterBuffering)
             {
@@ -135,6 +136,8 @@ public override async Task<InputFormatterResult> ReadRequestBodyAsync(InputForma
 
                 await readStream.DrainAsync(CancellationToken.None);
                 readStream.Seek(0L, SeekOrigin.Begin);
+
+                disposeReadStream = true;
             }
 
             try
@@ -162,9 +165,9 @@ public override async Task<InputFormatterResult> ReadRequestBodyAsync(InputForma
             }
             finally
             {
-                if (readStream is FileBufferingReadStream fileBufferingReadStream)
+                if (disposeReadStream)
                 {
-                    await fileBufferingReadStream.DisposeAsync();
+                    await readStream.DisposeAsync();
                 }
             }
         }

diff --git a/src/Mvc/Mvc.Formatters.Xml/src/XmlSerializerInputFormatter.cs b/src/Mvc/Mvc.Formatters.Xml/src/XmlSerializerInputFormatter.cs
@@ -99,6 +99,8 @@ public virtual InputFormatterExceptionPolicy ExceptionPolicy
 
             var request = context.HttpContext.Request;
             Stream readStream = new NonDisposableStream(request.Body);
+            var disposeReadStream = false;
+
             if (!request.Body.CanSeek && !_options.SuppressInputFormatterBuffering)
             {
                 // XmlSerializer does synchronous reads. In order to avoid blocking on the stream, we asynchronously
@@ -115,6 +117,7 @@ public virtual InputFormatterExceptionPolicy ExceptionPolicy
 
                 await readStream.DrainAsync(CancellationToken.None);
                 readStream.Seek(0L, SeekOrigin.Begin);
+                disposeReadStream = true;
             }
 
             try
@@ -155,9 +158,9 @@ public virtual InputFormatterExceptionPolicy ExceptionPolicy
             }
             finally
             {
-                if (readStream is FileBufferingReadStream fileBufferingReadStream)
+                if (disposeReadStream)
                 {
-                    await fileBufferingReadStream.DisposeAsync();
+                    await readStream.DisposeAsync();
                 }
             }
         }

diff --git a/src/Mvc/Mvc.Formatters.Xml/test/XmlDataContractSerializerInputFormatterTest.cs b/src/Mvc/Mvc.Formatters.Xml/test/XmlDataContractSerializerInputFormatterTest.cs
@@ -12,6 +12,7 @@
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
+using Microsoft.AspNetCore.WebUtilities;
 using Moq;
 using Xunit;
 
@@ -166,6 +167,39 @@ public async Task BuffersRequestBody_ByDefault()
             Assert.Equal(expectedString, model.sampleString);
         }
 
+        [Fact]
+        public async Task ReadAsync_DoesNotDisposeBufferedStreamIfItDidNotCreateIt()
+        {
+            // Arrange
+            var expectedInt = 10;
+            var expectedString = "TestString";
+
+            var input = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" +
+                "<TestLevelOne><SampleInt>" + expectedInt + "</SampleInt>" +
+                "<sampleString>" + expectedString + "</sampleString></TestLevelOne>";
+
+            var formatter = new XmlDataContractSerializerInputFormatter(new MvcOptions());
+
+            var contentBytes = Encoding.UTF8.GetBytes(input);
+            var httpContext = new DefaultHttpContext();
+            var testBufferedReadStream = new Mock<FileBufferingReadStream>(new MemoryStream(contentBytes), 1024) { CallBase = true };
+            httpContext.Request.Body = testBufferedReadStream.Object;
+            var context = GetInputFormatterContext(httpContext, typeof(TestLevelOne));
+
+            // Act
+            var result = await formatter.ReadAsync(context);
+
+            // Assert
+            Assert.NotNull(result);
+            Assert.False(result.HasError);
+            var model = Assert.IsType<TestLevelOne>(result.Model);
+
+            Assert.Equal(expectedInt, model.SampleInt);
+            Assert.Equal(expectedString, model.sampleString);
+
+            testBufferedReadStream.Verify(v => v.DisposeAsync(), Times.Never());
+        }
+
         [Fact]
         public async Task SuppressInputFormatterBufferingSetToTrue_DoesNotBufferRequestBody()
         {

diff --git a/src/Mvc/Mvc.Formatters.Xml/test/XmlSerializerInputFormatterTest.cs b/src/Mvc/Mvc.Formatters.Xml/test/XmlSerializerInputFormatterTest.cs
@@ -13,6 +13,7 @@
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
 using Microsoft.AspNetCore.Testing;
+using Microsoft.AspNetCore.WebUtilities;
 using Moq;
 using Xunit;
 
@@ -622,6 +623,39 @@ public async Task ReadAsync_AcceptsUTF16Characters()
             Assert.Equal(XmlConvert.ToDateTime(expectedDateTime, XmlDateTimeSerializationMode.Utc), model.SampleDate);
         }
 
+        [Fact]
+        public async Task ReadAsync_DoesNotDisposeBufferedStreamIfItDidNotCreateIt()
+        {
+            // Arrange
+            var expectedInt = 10;
+            var expectedString = "TestString";
+
+            var input = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" +
+                "<TestLevelOne><SampleInt>" + expectedInt + "</SampleInt>" +
+                "<sampleString>" + expectedString + "</sampleString></TestLevelOne>";
+
+            var formatter = new XmlSerializerInputFormatter(new MvcOptions());
+
+            var contentBytes = Encoding.UTF8.GetBytes(input);
+            var httpContext = new DefaultHttpContext();
+            var testBufferedReadStream = new Mock<FileBufferingReadStream>(new MemoryStream(contentBytes), 1024) { CallBase = true };
+            httpContext.Request.Body = testBufferedReadStream.Object;
+            var context = GetInputFormatterContext(httpContext, typeof(TestLevelOne));
+
+            // Act
+            var result = await formatter.ReadAsync(context);
+
+            // Assert
+            Assert.NotNull(result);
+            Assert.False(result.HasError);
+            var model = Assert.IsType<TestLevelOne>(result.Model);
+
+            Assert.Equal(expectedInt, model.SampleInt);
+            Assert.Equal(expectedString, model.sampleString);
+
+            testBufferedReadStream.Verify(v => v.DisposeAsync(), Times.Never());
+        }
+
         private InputFormatterContext GetInputFormatterContext(byte[] contentBytes, Type modelType)
         {
             var httpContext = GetHttpContext(contentBytes);

diff --git a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs
@@ -129,6 +129,7 @@ public virtual InputFormatterExceptionPolicy ExceptionPolicy
             var suppressInputFormatterBuffering = _options.SuppressInputFormatterBuffering;
 
             var readStream = request.Body;
+            var disposeReadStream = false;
             if (!request.Body.CanSeek && !suppressInputFormatterBuffering)
             {
                 // JSON.Net does synchronous reads. In order to avoid blocking on the stream, we asynchronously
@@ -145,6 +146,8 @@ public virtual InputFormatterExceptionPolicy ExceptionPolicy
 
                 await readStream.DrainAsync(CancellationToken.None);
                 readStream.Seek(0L, SeekOrigin.Begin);
+
+                disposeReadStream = true;
             }
 
             var successful = true;
@@ -170,9 +173,9 @@ public virtual InputFormatterExceptionPolicy ExceptionPolicy
                     jsonSerializer.Error -= ErrorHandler;
                     ReleaseJsonSerializer(jsonSerializer);
 
-                    if (readStream is FileBufferingReadStream fileBufferingReadStream)
+                    if (disposeReadStream)
                     {
-                        await fileBufferingReadStream.DisposeAsync();
+                        await readStream.DisposeAsync();
                     }
                 }
             }