From fcc20ace2a78c24f923ea185fad0cafabd2199ad Mon Sep 17 00:00:00 2001 From: Pranav K Date: Thu, 10 Oct 2019 13:52:02 -0700 Subject: [PATCH] Ensure EnableBuffering works with NewtonsoftJsonInputFormatter (#14870) Fixes https://github.com/aspnet/AspNetCore/issues/14396 --- .../Formatters/JsonInputFormatterTestBase.cs | 28 ++++++++++++++- ...XmlDataContractSerializerInputFormatter.cs | 7 ++-- .../src/XmlSerializerInputFormatter.cs | 7 ++-- ...ataContractSerializerInputFormatterTest.cs | 34 +++++++++++++++++++ .../test/XmlSerializerInputFormatterTest.cs | 34 +++++++++++++++++++ .../src/NewtonsoftJsonInputFormatter.cs | 7 ++-- 6 files changed, 110 insertions(+), 7 deletions(-) diff --git a/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs b/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs index fd76501888..11c8ce9baf 100644 --- a/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs +++ b/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs @@ -12,7 +12,9 @@ using System.Threading.Tasks; 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 @@ namespace Microsoft.AspNetCore.Mvc.Formatters 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(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(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 @@ namespace Microsoft.AspNetCore.Mvc.Formatters 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 index 362c8db997..e23a508053 100644 --- a/src/Mvc/Mvc.Formatters.Xml/src/XmlDataContractSerializerInputFormatter.cs +++ b/src/Mvc/Mvc.Formatters.Xml/src/XmlDataContractSerializerInputFormatter.cs @@ -118,6 +118,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters var request = context.HttpContext.Request; Stream readStream = new NonDisposableStream(request.Body); + var disposeReadStream = false; if (!request.Body.CanSeek && !_options.SuppressInputFormatterBuffering) { @@ -135,6 +136,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters await readStream.DrainAsync(CancellationToken.None); readStream.Seek(0L, SeekOrigin.Begin); + + disposeReadStream = true; } try @@ -162,9 +165,9 @@ namespace Microsoft.AspNetCore.Mvc.Formatters } 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 index 4debb6fe69..78e592370b 100644 --- a/src/Mvc/Mvc.Formatters.Xml/src/XmlSerializerInputFormatter.cs +++ b/src/Mvc/Mvc.Formatters.Xml/src/XmlSerializerInputFormatter.cs @@ -99,6 +99,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters 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 @@ namespace Microsoft.AspNetCore.Mvc.Formatters await readStream.DrainAsync(CancellationToken.None); readStream.Seek(0L, SeekOrigin.Begin); + disposeReadStream = true; } try @@ -155,9 +158,9 @@ namespace Microsoft.AspNetCore.Mvc.Formatters } 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 index 50ddb255a4..aff89dfc78 100644 --- a/src/Mvc/Mvc.Formatters.Xml/test/XmlDataContractSerializerInputFormatterTest.cs +++ b/src/Mvc/Mvc.Formatters.Xml/test/XmlDataContractSerializerInputFormatterTest.cs @@ -12,6 +12,7 @@ using System.Xml; 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 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml Assert.Equal(expectedString, model.sampleString); } + [Fact] + public async Task ReadAsync_DoesNotDisposeBufferedStreamIfItDidNotCreateIt() + { + // Arrange + var expectedInt = 10; + var expectedString = "TestString"; + + var input = "" + + "" + expectedInt + "" + + "" + expectedString + ""; + + var formatter = new XmlDataContractSerializerInputFormatter(new MvcOptions()); + + var contentBytes = Encoding.UTF8.GetBytes(input); + var httpContext = new DefaultHttpContext(); + var testBufferedReadStream = new Mock(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(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 index 27b6fd72f9..366e1a8dbc 100644 --- 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; 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 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml Assert.Equal(XmlConvert.ToDateTime(expectedDateTime, XmlDateTimeSerializationMode.Utc), model.SampleDate); } + [Fact] + public async Task ReadAsync_DoesNotDisposeBufferedStreamIfItDidNotCreateIt() + { + // Arrange + var expectedInt = 10; + var expectedString = "TestString"; + + var input = "" + + "" + expectedInt + "" + + "" + expectedString + ""; + + var formatter = new XmlSerializerInputFormatter(new MvcOptions()); + + var contentBytes = Encoding.UTF8.GetBytes(input); + var httpContext = new DefaultHttpContext(); + var testBufferedReadStream = new Mock(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(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 index 8975f9f426..1d30afcbac 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs +++ b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs @@ -129,6 +129,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters 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 @@ namespace Microsoft.AspNetCore.Mvc.Formatters await readStream.DrainAsync(CancellationToken.None); readStream.Seek(0L, SeekOrigin.Begin); + + disposeReadStream = true; } var successful = true; @@ -170,9 +173,9 @@ namespace Microsoft.AspNetCore.Mvc.Formatters jsonSerializer.Error -= ErrorHandler; ReleaseJsonSerializer(jsonSerializer); - if (readStream is FileBufferingReadStream fileBufferingReadStream) + if (disposeReadStream) { - await fileBufferingReadStream.DisposeAsync(); + await readStream.DisposeAsync(); } } }