From 0c3585f694811e19f97b89916b3d8f4757f1e8f9 Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Thu, 5 Jan 2017 17:03:35 -0800 Subject: [PATCH] [Fixes #4945] Simple string returned by controller action is not a valid JSON! --- .../Formatters/StringOutputFormatter.cs | 19 ++++- .../Formatters/StringOutputFormatterTests.cs | 72 ++++++++++++++----- .../ContentNegotiationTest.cs | 28 ++++---- 3 files changed, 84 insertions(+), 35 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Formatters/StringOutputFormatter.cs b/src/Microsoft.AspNetCore.Mvc.Core/Formatters/StringOutputFormatter.cs index 5e417f420f..2e0ac904bc 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Formatters/StringOutputFormatter.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Formatters/StringOutputFormatter.cs @@ -33,19 +33,32 @@ public override bool CanWriteResult(OutputFormatterCanWriteContext context) // always return it as a text/plain format. if (context.ObjectType == typeof(string) || context.Object is string) { - if (!context.ContentType.HasValue) + if (!context.ContentType.HasValue || IsSupportedMediaType(context.ContentType.Value)) { var mediaType = SupportedMediaTypes[0]; var encoding = SupportedEncodings[0]; context.ContentType = new StringSegment(MediaType.ReplaceEncoding(mediaType, encoding)); + return true; } - - return true; } return false; } + private bool IsSupportedMediaType(string mediaType) + { + var parsedContentType = new MediaType(mediaType); + for (var i = 0; i < SupportedMediaTypes.Count; i++) + { + var supportedMediaType = new MediaType(SupportedMediaTypes[i]); + if (parsedContentType.IsSubsetOf(supportedMediaType)) + { + return true; + } + } + return false; + } + public override Task WriteResponseBodyAsync(OutputFormatterWriteContext context, Encoding encoding) { if (context == null) diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Formatters/StringOutputFormatterTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Formatters/StringOutputFormatterTests.cs index 5f14fb2d60..8af6617ac9 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Formatters/StringOutputFormatterTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Formatters/StringOutputFormatterTests.cs @@ -12,22 +12,31 @@ namespace Microsoft.AspNetCore.Mvc.Formatters { - public class TextPlainFormatterTests + public class StringOutputFormatterTests { - public static IEnumerable OutputFormatterContextValues + public static IEnumerable CanWriteResultForStringTypesData { get { - // object value, bool useDeclaredTypeAsString, bool expectedCanWriteResult - yield return new object[] { "valid value", true, true }; - yield return new object[] { null, true, true }; - yield return new object[] { null, false, false }; - yield return new object[] { new object(), false, false }; + // object value, bool useDeclaredTypeAsString + yield return new object[] { "declared and runtime type are same", true }; + yield return new object[] { "declared and runtime type are different", false }; + yield return new object[] { null, true }; + } + } + + public static IEnumerable CannotWriteResultForNonStringTypesData + { + get + { + // object value, bool useDeclaredTypeAsString + yield return new object[] { null, false }; + yield return new object[] { new object(), false }; } } [Fact] - public void CanWriteResult_SetsAcceptContentType() + public void CannotWriteResult_ForNonTextPlainOrNonBrowserMediaTypes() { // Arrange var formatter = new StringOutputFormatter(); @@ -44,7 +53,7 @@ public void CanWriteResult_SetsAcceptContentType() var result = formatter.CanWriteResult(context); // Assert - Assert.True(result); + Assert.False(result); Assert.Equal(expectedContentType, context.ContentType); } @@ -53,13 +62,17 @@ public void CanWriteResult_DefaultContentType() { // Arrange var formatter = new StringOutputFormatter(); - var context = new OutputFormatterWriteContext( new DefaultHttpContext(), new TestHttpResponseStreamWriterFactory().CreateWriter, typeof(string), "Thisisastring"); + // For example, this can happen when a request is received without any Accept header OR a request + // is from a browser (in which case this ContentType is set to null by the infrastructure when + // RespectBrowserAcceptHeader is set to false) + context.ContentType = new StringSegment(); + // Act var result = formatter.CanWriteResult(context); @@ -69,14 +82,13 @@ public void CanWriteResult_DefaultContentType() } [Theory] - [MemberData(nameof(OutputFormatterContextValues))] - public void CanWriteResult_ReturnsTrueForStringTypes( + [MemberData(nameof(CanWriteResultForStringTypesData))] + public void CanWriteResult_ForStringTypes( object value, - bool useDeclaredTypeAsString, - bool expectedCanWriteResult) + bool useDeclaredTypeAsString) { // Arrange - var expectedContentType = new StringSegment("application/json"); + var expectedContentType = new StringSegment("text/plain; charset=utf-8"); var formatter = new StringOutputFormatter(); var type = useDeclaredTypeAsString ? typeof(string) : typeof(object); @@ -86,16 +98,42 @@ public void CanWriteResult_ReturnsTrueForStringTypes( new TestHttpResponseStreamWriterFactory().CreateWriter, type, value); - context.ContentType = expectedContentType; + context.ContentType = new StringSegment("text/plain"); // Act var result = formatter.CanWriteResult(context); // Assert - Assert.Equal(expectedCanWriteResult, result); + Assert.True(result); Assert.Equal(expectedContentType, context.ContentType); } + [Theory] + [MemberData(nameof(CannotWriteResultForNonStringTypesData))] + public void CannotWriteResult_ForNonStringTypes( + object value, + bool useDeclaredTypeAsString) + { + // Arrange + var expectedContentType = new StringSegment("text/plain; charset=utf-8"); + + var formatter = new StringOutputFormatter(); + var type = useDeclaredTypeAsString ? typeof(string) : typeof(object); + + var context = new OutputFormatterWriteContext( + new DefaultHttpContext(), + new TestHttpResponseStreamWriterFactory().CreateWriter, + type, + value); + context.ContentType = new StringSegment("text/plain"); + + // Act + var result = formatter.CanWriteResult(context); + + // Assert + Assert.False(result); + } + [Fact] public async Task WriteAsync_DoesNotWriteNullStrings() { diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ContentNegotiationTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ContentNegotiationTest.cs index 41cd535e49..746e0c2b6d 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ContentNegotiationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ContentNegotiationTest.cs @@ -345,33 +345,31 @@ public async Task XmlFormatter_SupportedMediaType_DoesNotChangeAcrossRequests() } [Theory] - [InlineData(true)] - [InlineData(false)] - public async Task ObjectResult_WithStringReturnType_DefaultToTextPlain(bool matchFormatterOnObjectType) + [InlineData(null)] + [InlineData("text/plain")] + [InlineData("text/plain; charset=utf-8")] + [InlineData("text/html, application/xhtml+xml, image/jxr, */*")] // typical browser accept header + public async Task ObjectResult_WithStringReturnType_DefaultToTextPlain(string acceptMediaType) { // Arrange - var targetUri = "http://localhost/FallbackOnTypeBasedMatch/ReturnString?matchFormatterOnObjectType=true" + - matchFormatterOnObjectType; - var request = new HttpRequestMessage(HttpMethod.Get, targetUri); + var request = new HttpRequestMessage(HttpMethod.Get, "FallbackOnTypeBasedMatch/ReturnString"); + request.Headers.Accept.ParseAdd(acceptMediaType); // Act var response = await Client.SendAsync(request); // Assert Assert.Equal(HttpStatusCode.OK, response.StatusCode); - Assert.Equal("text/plain", response.Content.Headers.ContentType.MediaType); + Assert.Equal("text/plain; charset=utf-8", response.Content.Headers.ContentType.ToString()); var actualBody = await response.Content.ReadAsStringAsync(); Assert.Equal("Hello World!", actualBody); } - [Theory] - [InlineData(true)] - [InlineData(false)] - public async Task ObjectResult_WithStringReturnType_SetsMediaTypeToAccept(bool matchFormatterOnObjectType) + [Fact] + public async Task ObjectResult_WithStringReturnType_AndNonTextPlainMediaType_DoesNotReturnTextPlain() { // Arrange - var targetUri = "http://localhost/FallbackOnTypeBasedMatch/ReturnString?matchFormatterOnObjectType=" + - matchFormatterOnObjectType; + var targetUri = "http://localhost/FallbackOnTypeBasedMatch/ReturnString"; var request = new HttpRequestMessage(HttpMethod.Get, targetUri); request.Headers.Accept.Add(MediaTypeWithQualityHeaderValue.Parse("application/json")); @@ -380,9 +378,9 @@ public async Task ObjectResult_WithStringReturnType_SetsMediaTypeToAccept(bool m // Assert Assert.Equal(HttpStatusCode.OK, response.StatusCode); - Assert.Equal("application/json", response.Content.Headers.ContentType.MediaType); + Assert.Equal("application/json; charset=utf-8", response.Content.Headers.ContentType.ToString()); var actualBody = await response.Content.ReadAsStringAsync(); - Assert.Equal("Hello World!", actualBody); + Assert.Equal("\"Hello World!\"", actualBody); } [Fact]