[Fixes #4945] Simple string returned by controller action is not a va…

…lid JSON!

src/Microsoft.AspNetCore.Mvc.Core/Formatters/StringOutputFormatter.cs

@@ -11,7 +11,7 @@
 namespace Microsoft.AspNetCore.Mvc.Formatters
 {
     /// <summary>
-    /// Always writes a string value to the response, regardless of requested content type.
+    /// A <see cref="TextOutputFormatter"/> for simple text content.
     /// </summary>
     public class StringOutputFormatter : TextOutputFormatter
     {
@@ -29,18 +29,10 @@ public override bool CanWriteResult(OutputFormatterCanWriteContext context)
                 throw new ArgumentNullException(nameof(context));
             }
 
-            // Ignore the passed in content type, if the object is string
-            // always return it as a text/plain format.
             if (context.ObjectType == typeof(string) || context.Object is string)
             {
-                if (!context.ContentType.HasValue)
-                {
-                    var mediaType = SupportedMediaTypes[0];
-                    var encoding = SupportedEncodings[0];
-                    context.ContentType = new StringSegment(MediaType.ReplaceEncoding(mediaType, encoding));
-                }
-
-                return true;
+                // Call into base to check if the current request's content type is a supported media type.
+                return base.CanWriteResult(context);
             }
 
             return false;

test/Microsoft.AspNetCore.Mvc.Core.Test/Formatters/StringOutputFormatterTests.cs

@@ -12,39 +12,52 @@
 
 namespace Microsoft.AspNetCore.Mvc.Formatters
 {
-    public class TextPlainFormatterTests
+    public class StringOutputFormatterTests
     {
-        public static IEnumerable<object[]> OutputFormatterContextValues
+        public static IEnumerable<object[]> CanWriteStringsData
         {
             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 };
             }
         }
 
-        [Fact]
-        public void CanWriteResult_SetsAcceptContentType()
+        public static TheoryData<object> CannotWriteNonStringsData
+        {
+            get
+            {
+                return new TheoryData<object>()
+                {
+                    null,
+                    new object()
+                };
+            }
+        }
+
+        [Theory]
+        [InlineData("application/json")]
+        [InlineData("application/xml")]
+        public void CannotWriteUnsupportedMediaType(string contentType)
         {
             // Arrange
             var formatter = new StringOutputFormatter();
-            var expectedContentType = new StringSegment("application/json");
+            var expectedContentType = new StringSegment(contentType);
 
             var context = new OutputFormatterWriteContext(
                 new DefaultHttpContext(),
                 new TestHttpResponseStreamWriterFactory().CreateWriter,
                 typeof(string),
                 "Thisisastring");
-            context.ContentType = expectedContentType;
+            context.ContentType = new StringSegment(contentType);
 
             // Act
             var result = formatter.CanWriteResult(context);
 
             // Assert
-            Assert.True(result);
+            Assert.False(result);
             Assert.Equal(expectedContentType, context.ContentType);
         }
 
@@ -53,7 +66,6 @@ public void CanWriteResult_DefaultContentType()
         {
             // Arrange
             var formatter = new StringOutputFormatter();
-
             var context = new OutputFormatterWriteContext(
                 new DefaultHttpContext(),
                 new TestHttpResponseStreamWriterFactory().CreateWriter,
@@ -65,18 +77,17 @@ public void CanWriteResult_DefaultContentType()
 
             // Assert
             Assert.True(result);
-            Assert.Equal(new StringSegment("text/plain; charset=utf-8"), context.ContentType);
+            Assert.Equal(new StringSegment("text/plain"), context.ContentType);
         }
 
         [Theory]
-        [MemberData(nameof(OutputFormatterContextValues))]
-        public void CanWriteResult_ReturnsTrueForStringTypes(
+        [MemberData(nameof(CanWriteStringsData))]
+        public void CanWriteStrings(
             object value,
-            bool useDeclaredTypeAsString,
-            bool expectedCanWriteResult)
+            bool useDeclaredTypeAsString)
         {
             // Arrange
-            var expectedContentType = new StringSegment("application/json");
+            var expectedContentType = new StringSegment("text/plain");
 
             var formatter = new StringOutputFormatter();
             var type = useDeclaredTypeAsString ? typeof(string) : typeof(object);
@@ -86,13 +97,35 @@ public void CanWriteResult_DefaultContentType()
                 new TestHttpResponseStreamWriterFactory().CreateWriter,
                 type,
                 value);
-            context.ContentType = expectedContentType;
+            context.ContentType = new StringSegment("text/plain");
+
+            // Act
+            var result = formatter.CanWriteResult(context);
+
+            // Assert
+            Assert.True(result);
+            Assert.Equal(expectedContentType, context.ContentType);
+        }
+
+        [Theory]
+        [MemberData(nameof(CannotWriteNonStringsData))]
+        public void CannotWriteNonStrings(object value)
+        {
+            // Arrange
+            var expectedContentType = new StringSegment("text/plain");
+            var formatter = new StringOutputFormatter();
+            var context = new OutputFormatterWriteContext(
+                new DefaultHttpContext(),
+                new TestHttpResponseStreamWriterFactory().CreateWriter,
+                typeof(object),
+                value);
+            context.ContentType = new StringSegment("text/plain");
 
             // Act
             var result = formatter.CanWriteResult(context);
 
             // Assert
-            Assert.Equal(expectedCanWriteResult, result);
+            Assert.False(result);
             Assert.Equal(expectedContentType, context.ContentType);
         }
 

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]

test/Microsoft.AspNetCore.Mvc.FunctionalTests/DoNotRespectBrowserAcceptHeaderTests.cs

@@ -0,0 +1,142 @@
+// 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.Net;
+using System.Net.Http;
+using System.Text;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Mvc.Formatters.Xml;
+using Microsoft.AspNetCore.Testing.xunit;
+using Xunit;
+
+namespace Microsoft.AspNetCore.Mvc.FunctionalTests
+{
+    /// <summary>
+    /// These tests are for scenarios when <see cref="MvcOptions.RespectBrowserAcceptHeader"/> is <c>False</c>, which is the default.
+    /// </summary>
+    public class DoNotRespectBrowserAcceptHeaderTests : IClassFixture<MvcTestFixture<FormatterWebSite.Startup>>
+    {
+        public DoNotRespectBrowserAcceptHeaderTests(MvcTestFixture<FormatterWebSite.Startup> fixture)
+        {
+            Client = fixture.Client;
+        }
+
+        public HttpClient Client { get; }
+
+        [Theory]
+        [InlineData("application/xml,*/*;q=0.2")]
+        [InlineData("application/xml,*/*")]
+        public async Task AllMediaRangeAcceptHeader_FirstFormatterInListWritesResponse(string acceptHeader)
+        {
+            // Arrange
+            var request = RequestWithAccept("http://localhost/DoNotRespectBrowserAcceptHeader/EmployeeInfo", acceptHeader);
+
+            // Act
+            var response = await Client.SendAsync(request);
+
+            // Assert
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            Assert.NotNull(response.Content);
+            Assert.NotNull(response.Content.Headers.ContentType);
+            Assert.Equal("application/json; charset=utf-8", response.Content.Headers.ContentType.ToString());
+            var responseData = await response.Content.ReadAsStringAsync();
+            Assert.Equal("{\"id\":10,\"name\":\"John\"}", responseData);
+        }
+
+        [ConditionalTheory]
+        // Mono issue - https://github.com/aspnet/External/issues/18
+        [FrameworkSkipCondition(RuntimeFrameworks.Mono)]
+        [InlineData("application/xml,*/*;q=0.2")]
+        [InlineData("application/xml,*/*")]
+        public async Task AllMediaRangeAcceptHeader_ProducesAttributeIsHonored(string acceptHeader)
+        {
+            // Arrange
+            var request = RequestWithAccept(
+                "http://localhost/DoNotRespectBrowserAcceptHeader/EmployeeInfoWithProduces",
+                acceptHeader);
+            var expectedResponseData =
+                "<DoNotRespectBrowserAcceptHeaderController.Employee xmlns:i=\"http://www.w3.org/2001/XMLSchema-instance\"" +
+                " xmlns=\"http://schemas.datacontract.org/2004/07/FormatterWebSite.Controllers\"><Id>20</Id><Name>Mike" +
+                "</Name></DoNotRespectBrowserAcceptHeaderController.Employee>";
+
+            // Act
+            var response = await Client.SendAsync(request);
+
+            // Assert
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            Assert.NotNull(response.Content);
+            Assert.NotNull(response.Content.Headers.ContentType);
+            Assert.Equal("application/xml; charset=utf-8", response.Content.Headers.ContentType.ToString());
+            var responseData = await response.Content.ReadAsStringAsync();
+            XmlAssert.Equal(expectedResponseData, responseData);
+        }
+
+        [ConditionalTheory]
+        // Mono issue - https://github.com/aspnet/External/issues/18
+        [FrameworkSkipCondition(RuntimeFrameworks.Mono)]
+        [InlineData("application/xml,*/*;q=0.2")]
+        [InlineData("application/xml,*/*")]
+        public async Task AllMediaRangeAcceptHeader_WithContentTypeHeader_ContentTypeIsIgnored(string acceptHeader)
+        {
+            // Arrange
+            var requestData =
+                "<DoNotRespectBrowserAcceptHeaderController.Employee xmlns:i=\"http://www.w3.org/2001/XMLSchema-instance\"" +
+                " xmlns=\"http://schemas.datacontract.org/2004/07/FormatterWebSite.Controllers\"><Id>35</Id><Name>Jimmy" +
+                "</Name></DoNotRespectBrowserAcceptHeaderController.Employee>";
+            var expectedResponseData = @"{""id"":35,""name"":""Jimmy""}";
+            var request = RequestWithAccept("http://localhost/DoNotRespectBrowserAcceptHeader/CreateEmployee", acceptHeader);
+            request.Content = new StringContent(requestData, Encoding.UTF8, "application/xml");
+            request.Method = HttpMethod.Post;
+
+            // Act
+            var response = await Client.SendAsync(request);
+
+            // Assert
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            Assert.NotNull(response.Content);
+            Assert.NotNull(response.Content.Headers.ContentType);
+
+            // Site uses default output formatter (ignores Accept header) because that header contained a wildcard match.
+            Assert.Equal("application/json; charset=utf-8", response.Content.Headers.ContentType.ToString());
+
+            var responseData = await response.Content.ReadAsStringAsync();
+            Assert.Equal(expectedResponseData, responseData);
+        }
+
+        [ConditionalTheory]
+        // Mono issue - https://github.com/aspnet/External/issues/18
+        [FrameworkSkipCondition(RuntimeFrameworks.Mono)]
+        [InlineData("application/xml,application/json;q=0.2")]
+        [InlineData("application/xml,application/json")]
+        public async Task AllMediaRangeAcceptHeader_WithExactMatch_ReturnsExpectedContent(string acceptHeader)
+        {
+            // Arrange
+            var requestData =
+                "<DoNotRespectBrowserAcceptHeaderController.Employee xmlns:i=\"http://www.w3.org/2001/XMLSchema-instance\"" +
+                " xmlns=\"http://schemas.datacontract.org/2004/07/FormatterWebSite.Controllers\"><Id>35</Id><Name>Jimmy" +
+                "</Name></DoNotRespectBrowserAcceptHeaderController.Employee>";
+            var request = RequestWithAccept("http://localhost/DoNotRespectBrowserAcceptHeader/CreateEmployee", acceptHeader);
+            request.Content = new StringContent(requestData, Encoding.UTF8, "application/xml");
+            request.Method = HttpMethod.Post;
+
+            // Act
+            var response = await Client.SendAsync(request);
+
+            // Assert
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            Assert.NotNull(response.Content);
+            Assert.NotNull(response.Content.Headers.ContentType);
+            Assert.Equal("application/xml; charset=utf-8", response.Content.Headers.ContentType.ToString());
+            var responseData = await response.Content.ReadAsStringAsync();
+            Assert.Equal(requestData, responseData);
+        }
+
+        private static HttpRequestMessage RequestWithAccept(string url, string accept)
+        {
+            var request = new HttpRequestMessage(HttpMethod.Get, url);
+            request.Headers.Add("Accept", accept);
+
+            return request;
+        }
+    }
+}