From 489fc52df854ff98c8777cfaa0781ce8d38f18f9 Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Fri, 6 Mar 2015 11:49:59 -0800 Subject: [PATCH] [Fixes #2108] StringOutputFormatter fails when HttpNotAcceptableOutputFormatter is used --- .../ActionResults/ObjectResult.cs | 154 +++++++++++------- .../HttpNotAcceptableOutputFormatter.cs | 7 +- .../Formatters/OutputFormatterContext.cs | 11 ++ .../ActionResults/ObjectResultTests.cs | 128 +++++++++++++++ .../ContentNegotiationTest.cs | 43 +++++ .../MvcSampleTests.cs | 1 + .../FallbackOnTypeBasedMatchController.cs | 18 ++ 7 files changed, 295 insertions(+), 67 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/ActionResults/ObjectResult.cs b/src/Microsoft.AspNet.Mvc.Core/ActionResults/ObjectResult.cs index 985e988af9..4126b035c4 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ActionResults/ObjectResult.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ActionResults/ObjectResult.cs @@ -68,101 +68,77 @@ namespace Microsoft.AspNet.Mvc await selectedFormatter.WriteAsync(formatterContext); } - public virtual IOutputFormatter SelectFormatter(OutputFormatterContext formatterContext, - IEnumerable formatters) + public virtual IOutputFormatter SelectFormatter( + OutputFormatterContext formatterContext, + IEnumerable formatters) { + // Check if any content-type was explicitly set (for example, via ProducesAttribute + // or Url path extension mapping). If yes, then ignore content-negotiation and use this content-type. if (ContentTypes.Count == 1) { - // There is only one content type specified so we can skip looking at the accept headers. return SelectFormatterUsingAnyAcceptableContentType(formatterContext, formatters, ContentTypes); } - var incomingAcceptHeaderMediaTypes = formatterContext.ActionContext.HttpContext.Request.GetTypedHeaders().Accept ?? - new MediaTypeHeaderValue[] { }; - - // By default we want to ignore considering accept headers for content negotiation when - // they have a media type like */* in them. Browsers typically have these media types. - // In these cases we would want the first formatter in the list of output formatters to - // write the response. This default behavior can be changed through options, so checking here. - var options = formatterContext.ActionContext.HttpContext - .RequestServices - .GetRequiredService>() - .Options; - - var respectAcceptHeader = true; - if (options.RespectBrowserAcceptHeader == false - && incomingAcceptHeaderMediaTypes.Any(mediaType => mediaType.MatchesAllTypes)) - { - respectAcceptHeader = false; - } - - IEnumerable sortedAcceptHeaderMediaTypes = null; - if (respectAcceptHeader) - { - sortedAcceptHeaderMediaTypes = SortMediaTypeHeaderValues(incomingAcceptHeaderMediaTypes) - .Where(header => header.Quality != HeaderQuality.NoMatch); - } + var sortedAcceptHeaderMediaTypes = GetSortedAcceptHeaderMediaTypes(formatterContext); IOutputFormatter selectedFormatter = null; if (ContentTypes == null || ContentTypes.Count == 0) { - if (respectAcceptHeader) + // Check if we have enough information to do content-negotiation, otherwise get the first formatter + // which can write the type. + MediaTypeHeaderValue requestContentType = null; + MediaTypeHeaderValue.TryParse( + formatterContext.ActionContext.HttpContext.Request.ContentType, + out requestContentType); + if (!sortedAcceptHeaderMediaTypes.Any() && requestContentType == null) + { + return SelectFormatterBasedOnTypeMatch(formatterContext, formatters); + } + + // + // Content-Negotiation starts from this point on. + // + + // 1. Select based on sorted accept headers. + if (sortedAcceptHeaderMediaTypes.Any()) { - // Select based on sorted accept headers. selectedFormatter = SelectFormatterUsingSortedAcceptHeaders( formatterContext, formatters, sortedAcceptHeaderMediaTypes); } - if (selectedFormatter == null) + // 2. No formatter was found based on accept headers, fall back on request Content-Type header. + if (selectedFormatter == null && requestContentType != null) { - var requestContentType = formatterContext.ActionContext.HttpContext.Request.ContentType; - - // No formatter found based on accept headers, fall back on request contentType. - MediaTypeHeaderValue incomingContentType = null; - MediaTypeHeaderValue.TryParse(requestContentType, out incomingContentType); - - // In case the incomingContentType is null (as can be the case with get requests), - // we need to pick the first formatter which - // can support writing this type. - var contentTypes = new[] { incomingContentType }; selectedFormatter = SelectFormatterUsingAnyAcceptableContentType( formatterContext, formatters, - contentTypes); + new[] { requestContentType }); + } - // This would be the case when no formatter could write the type base on the - // accept headers and the request content type. Fallback on type based match. - if (selectedFormatter == null) - { - foreach (var formatter in formatters) - { - var supportedContentTypes = formatter.GetSupportedContentTypes( - formatterContext.DeclaredType, - formatterContext.Object?.GetType(), - contentType: null); + // 3. No formatter was found based on Accept and request Content-Type headers, so + // fallback on type based match. + if (selectedFormatter == null) + { + // Set this flag to indicate that content-negotiation has failed to let formatters decide + // if they want to write the response or not. + formatterContext.FailedContentNegotiation = true; - if (formatter.CanWriteResult(formatterContext, supportedContentTypes?.FirstOrDefault())) - { - return formatter; - } - } - } + return SelectFormatterBasedOnTypeMatch(formatterContext, formatters); } } else { - if (respectAcceptHeader) + if (sortedAcceptHeaderMediaTypes.Any()) { // Filter and remove accept headers which cannot support any of the user specified content types. var filteredAndSortedAcceptHeaders = sortedAcceptHeaderMediaTypes - .Where(acceptHeader => - ContentTypes - .Any(contentType => - contentType.IsSubsetOf(acceptHeader))); + .Where(acceptHeader => + ContentTypes.Any(contentType => + contentType.IsSubsetOf(acceptHeader))); selectedFormatter = SelectFormatterUsingSortedAcceptHeaders( formatterContext, @@ -188,6 +164,26 @@ namespace Microsoft.AspNet.Mvc return selectedFormatter; } + public virtual IOutputFormatter SelectFormatterBasedOnTypeMatch( + OutputFormatterContext formatterContext, + IEnumerable formatters) + { + foreach (var formatter in formatters) + { + var supportedContentTypes = formatter.GetSupportedContentTypes( + formatterContext.DeclaredType, + formatterContext.Object?.GetType(), + contentType: null); + + if (formatter.CanWriteResult(formatterContext, supportedContentTypes?.FirstOrDefault())) + { + return formatter; + } + } + + return null; + } + public virtual IOutputFormatter SelectFormatterUsingSortedAcceptHeaders( OutputFormatterContext formatterContext, IEnumerable formatters, @@ -224,6 +220,38 @@ namespace Microsoft.AspNet.Mvc return selectedFormatter; } + private IEnumerable GetSortedAcceptHeaderMediaTypes( + OutputFormatterContext formatterContext) + { + var request = formatterContext.ActionContext.HttpContext.Request; + var incomingAcceptHeaderMediaTypes = request.GetTypedHeaders().Accept ?? new MediaTypeHeaderValue[] { }; + + // By default we want to ignore considering accept headers for content negotiation when + // they have a media type like */* in them. Browsers typically have these media types. + // In these cases we would want the first formatter in the list of output formatters to + // write the response. This default behavior can be changed through options, so checking here. + var options = formatterContext.ActionContext.HttpContext + .RequestServices + .GetRequiredService>() + .Options; + + var respectAcceptHeader = true; + if (options.RespectBrowserAcceptHeader == false + && incomingAcceptHeaderMediaTypes.Any(mediaType => mediaType.MatchesAllTypes)) + { + respectAcceptHeader = false; + } + + var sortedAcceptHeaderMediaTypes = Enumerable.Empty(); + if (respectAcceptHeader) + { + sortedAcceptHeaderMediaTypes = SortMediaTypeHeaderValues(incomingAcceptHeaderMediaTypes) + .Where(header => header.Quality != HeaderQuality.NoMatch); + } + + return sortedAcceptHeaderMediaTypes; + } + private void ThrowIfUnsupportedContentType() { var matchAllContentType = ContentTypes?.FirstOrDefault( diff --git a/src/Microsoft.AspNet.Mvc.Core/Formatters/HttpNotAcceptableOutputFormatter.cs b/src/Microsoft.AspNet.Mvc.Core/Formatters/HttpNotAcceptableOutputFormatter.cs index ab91475c43..149fcaf086 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Formatters/HttpNotAcceptableOutputFormatter.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Formatters/HttpNotAcceptableOutputFormatter.cs @@ -10,15 +10,14 @@ using Microsoft.Net.Http.Headers; namespace Microsoft.AspNet.Mvc { /// - /// A formatter which does not have a supported content type and selects itself if no content type is passed to it. - /// Sets the status code to 406 if selected. + /// A formatter which selects itself when content-negotiation has failed and writes a 406 Not Acceptable response. /// public class HttpNotAcceptableOutputFormatter : IOutputFormatter { /// public bool CanWriteResult(OutputFormatterContext context, MediaTypeHeaderValue contentType) { - return contentType == null; + return context.FailedContentNegotiation ?? false; } /// @@ -37,4 +36,4 @@ namespace Microsoft.AspNet.Mvc return Task.FromResult(true); } } -} +} \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/Formatters/OutputFormatterContext.cs b/src/Microsoft.AspNet.Mvc.Core/Formatters/OutputFormatterContext.cs index ef139c378b..c88e17822f 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Formatters/OutputFormatterContext.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Formatters/OutputFormatterContext.cs @@ -45,5 +45,16 @@ namespace Microsoft.AspNet.Mvc /// Null indicates no value set by the . /// public int? StatusCode { get; set; } + + /// + /// Gets or sets a flag to indicate that content-negotiation could not find a formatter based on the + /// information on the . + /// + /// + /// A can use this information to decide how to write a response. + /// For example, the sets a 406 Not Acceptable response + /// when content negotiation has failed. + /// + public bool? FailedContentNegotiation { get; set; } } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ActionResults/ObjectResultTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ActionResults/ObjectResultTests.cs index 68b912cbd7..b34ceab306 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ActionResults/ObjectResultTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ActionResults/ObjectResultTests.cs @@ -107,6 +107,37 @@ namespace Microsoft.AspNet.Mvc.Core.Test.ActionResults Assert.Equal(input, result.Value); } + [Fact] + public async Task NoAcceptAndContentTypeHeaders_406Formatter_DoesNotTakeEffect() + { + // Arrange + var expectedContentType = "application/json; charset=utf-8"; + + var input = 123; + var httpResponse = new DefaultHttpContext().Response; + httpResponse.Body = new MemoryStream(); + var actionContext = CreateMockActionContext( + outputFormatters: new IOutputFormatter[] + { + new HttpNotAcceptableOutputFormatter(), + new JsonOutputFormatter() + }, + response: httpResponse, + requestAcceptHeader: null, + requestContentType: null, + requestAcceptCharsetHeader: null); + + var result = new ObjectResult(input); + result.ContentTypes = new List(); + result.ContentTypes.Add(MediaTypeHeaderValue.Parse(expectedContentType)); + + // Act + await result.ExecuteResultAsync(actionContext); + + // Assert + Assert.Equal(expectedContentType, httpResponse.ContentType); + } + [Fact] public async Task ObjectResult_WithSingleContentType_TheGivenContentTypeIsSelected() { @@ -643,6 +674,103 @@ namespace Microsoft.AspNet.Mvc.Core.Test.ActionResults Assert.Equal(expectedMessage, exception.Message); } + [Fact] + public async Task ObjectResult_WithStringType_WritesTextPlain_Ignoring406Formatter() + { + // Arrange + var expectedData = "Hello World!"; + var objectResult = new ObjectResult(expectedData); + var outputFormatters = new IOutputFormatter[] + { + new HttpNotAcceptableOutputFormatter(), + new StringOutputFormatter(), + new JsonOutputFormatter() + }; + + var response = new Mock(); + var responseStream = new MemoryStream(); + response.SetupGet(r => r.Body).Returns(responseStream); + + var actionContext = CreateMockActionContext( + outputFormatters, + response.Object, + requestAcceptHeader: "application/json"); + + // Act + await objectResult.ExecuteResultAsync(actionContext); + + // Assert + response.VerifySet(r => r.ContentType = "text/plain; charset=utf-8"); + responseStream.Position = 0; + var actual = new StreamReader(responseStream).ReadToEnd(); + Assert.Equal(expectedData, actual); + } + + [Fact] + public async Task ObjectResult_WithSingleContentType_Ignores406Formatter() + { + // Arrange + var objectResult = new ObjectResult(new Person() { Name = "John" }); + objectResult.ContentTypes.Add(new MediaTypeHeaderValue("application/json")); + var outputFormatters = new IOutputFormatter[] + { + new HttpNotAcceptableOutputFormatter(), + new JsonOutputFormatter() + }; + var response = new Mock(); + var responseStream = new MemoryStream(); + response.SetupGet(r => r.Body).Returns(responseStream); + var expectedData = "{\"Name\":\"John\"}"; + + var actionContext = CreateMockActionContext( + outputFormatters, + response.Object, + requestAcceptHeader: "application/non-existing", + requestContentType: "application/non-existing"); + + // Act + await objectResult.ExecuteResultAsync(actionContext); + + // Assert + response.VerifySet(r => r.ContentType = "application/json; charset=utf-8"); + responseStream.Position = 0; + var actual = new StreamReader(responseStream).ReadToEnd(); + Assert.Equal(expectedData, actual); + } + + [Fact] + public async Task ObjectResult_WithMultipleContentTypes_Ignores406Formatter() + { + // Arrange + var objectResult = new ObjectResult(new Person() { Name = "John" }); + objectResult.ContentTypes.Add(new MediaTypeHeaderValue("application/foo")); + objectResult.ContentTypes.Add(new MediaTypeHeaderValue("application/json")); + var outputFormatters = new IOutputFormatter[] + { + new HttpNotAcceptableOutputFormatter(), + new JsonOutputFormatter() + }; + var response = new Mock(); + var responseStream = new MemoryStream(); + response.SetupGet(r => r.Body).Returns(responseStream); + var expectedData = "{\"Name\":\"John\"}"; + + var actionContext = CreateMockActionContext( + outputFormatters, + response.Object, + requestAcceptHeader: "application/non-existing", + requestContentType: "application/non-existing"); + + // Act + await objectResult.ExecuteResultAsync(actionContext); + + // Assert + response.VerifySet(r => r.ContentType = "application/json; charset=utf-8"); + responseStream.Position = 0; + var actual = new StreamReader(responseStream).ReadToEnd(); + Assert.Equal(expectedData, actual); + } + private static ActionContext CreateMockActionContext( HttpResponse response = null, string requestAcceptHeader = "application/*", diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ContentNegotiationTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ContentNegotiationTest.cs index 00fb9a5aa4..4c74a4b249 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ContentNegotiationTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ContentNegotiationTest.cs @@ -132,6 +132,26 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests XmlAssert.Equal(expectedOutput, actual); } + [Theory] + [InlineData("http://localhost/FallbackOnTypeBasedMatch/UseTheFallback_WithDefaultFormatters")] + [InlineData("http://localhost/FallbackOnTypeBasedMatch/OverrideTheFallback_WithDefaultFormatters")] + public async Task NoAcceptAndRequestContentTypeHeaders_UsesFirstFormatterWhichCanWriteType(string url) + { + // Arrange + var server = TestHelper.CreateServer(_app, SiteName); + var client = server.CreateClient(); + var expectedContentType = MediaTypeHeaderValue.Parse("application/json;charset=utf-8"); + + // Act + var response = await client.GetAsync(url + "?input=100"); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(expectedContentType, response.Content.Headers.ContentType); + var actual = await response.Content.ReadAsStringAsync(); + Assert.Equal("100", actual); + } + [Fact] public async Task NoMatchingFormatter_ForTheGivenContentType_Returns406() { @@ -408,6 +428,29 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests Assert.Equal(expectedBody, body); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task ObjectResult_WithStringReturnType_WritesTextPlainFormat(bool matchFormatterOnObjectType) + { + // Arrange + var server = TestHelper.CreateServer(_app, SiteName); + var client = server.CreateClient(); + var targetUri = "http://localhost/FallbackOnTypeBasedMatch/ReturnString?matchFormatterOnObjectType=" + + matchFormatterOnObjectType; + var request = new HttpRequestMessage(HttpMethod.Get, targetUri); + request.Headers.Accept.Add(MediaTypeWithQualityHeaderValue.Parse("application/json")); + + // Act + var response = await client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal("text/plain", response.Content.Headers.ContentType.MediaType); + var actualBody = await response.Content.ReadAsStringAsync(); + Assert.Equal("Hello World!", actualBody); + } + [Theory] [InlineData("OverrideTheFallback_WithDefaultFormatters")] [InlineData("OverrideTheFallback_UsingCustomFormatters")] diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/MvcSampleTests.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/MvcSampleTests.cs index 639ce67d8b..29bb157a80 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/MvcSampleTests.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/MvcSampleTests.cs @@ -69,6 +69,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests var server = TestHelper.CreateServer(_app, SiteName, SamplesFolder); var client = server.CreateClient(); var request = new HttpRequestMessage(HttpMethod.Post, "http://localhost/FormUrlEncoded/IsValidPerson"); + request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); request.Content = new StringContent(input, Encoding.UTF8, "application/x-www-form-urlencoded"); // Act diff --git a/test/WebSites/ContentNegotiationWebSite/Controllers/FallbackOnTypeBasedMatchController.cs b/test/WebSites/ContentNegotiationWebSite/Controllers/FallbackOnTypeBasedMatchController.cs index f4a102bdf4..bc7f6576aa 100644 --- a/test/WebSites/ContentNegotiationWebSite/Controllers/FallbackOnTypeBasedMatchController.cs +++ b/test/WebSites/ContentNegotiationWebSite/Controllers/FallbackOnTypeBasedMatchController.cs @@ -58,5 +58,23 @@ namespace ContentNegotiationWebSite return objectResult; } + + public IActionResult ReturnString( + bool matchFormatterOnObjectType, + [FromServices] IOutputFormattersProvider outputFormattersProvider) + { + var objectResult = new ObjectResult("Hello World!"); + if (matchFormatterOnObjectType) + { + objectResult.Formatters.Add(new HttpNotAcceptableOutputFormatter()); + } + + foreach (var formatter in outputFormattersProvider.OutputFormatters) + { + objectResult.Formatters.Add(formatter); + } + + return objectResult; + } } } \ No newline at end of file