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

…lid JSON!

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

@@ -29,18 +29,17 @@ 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)
+                // Call into base to check if the current request's content type is a supported media type.
+                var canWriteResult = base.CanWriteResult(context);
+                if (canWriteResult)
                 {
                     var mediaType = SupportedMediaTypes[0];
                     var encoding = SupportedEncodings[0];
                     context.ContentType = new StringSegment(MediaType.ReplaceEncoding(mediaType, encoding));
+                    return true;
                 }
-
-                return true;
             }
 
             return false;

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<object[]> OutputFormatterContextValues
+        public static IEnumerable<object[]> 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<object[]> 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()
         {

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]