Use strongly typed MediaTypeHeaderValue for content type in action resul... #2450

kichalla · 2015-04-24T22:30:02Z

...ts.

kichalla · 2015-04-24T22:31:09Z

src/Microsoft.AspNet.Mvc.Core/ActionResults/FileContentResult.cs

        /// </summary>
        /// <param name="fileContents">The bytes that represent the file contents.</param>
-        public FileContentResult([NotNull] byte[] fileContents)
-            : base(contentType: null)


If we compare to MVC5, content type was always required:
http://aspnetwebstack.codeplex.com/SourceControl/latest#src/System.Web.Mvc/FileResult.cs

This seems to be like a compatiblity bug fix - should have been dealt with as a separate PR. (Its fine to leave this here for this one, but something to keep in mind in general ).

kichalla · 2015-04-27T14:12:57Z

ping

pranavkm · 2015-04-27T15:35:35Z

src/Microsoft.AspNet.Mvc.Core/ActionResults/ContentResult.cs

@@ -26,17 +29,34 @@ public class ContentResult : ActionResult
        {
            var response = context.HttpContext.Response;

+            // Encoding property on MediaTypeHeaderValue does not return the exact encoding instance that


We shouldn't need to this. If using the encoding set in ContentType is incorrect, we have an issue with the way MediaTypeHeaderValue.Encoding behaves. cc @Tratcher

I'm planning to remove MediaTypeHeaderValue.Encoding, it's causing confusion.

@pranavkm is that a happy or sad panda?

Aren't 🐼 always sad?

kichalla · 2015-04-29T16:52:38Z

Updated. /cc: @pranavkm

davidfowl · 2015-04-29T16:54:24Z

Are we sure we want to do this?

pranavkm · 2015-04-29T16:58:15Z

@davidfowl what's the concern?

kichalla · 2015-04-29T18:59:15Z

Updated /cc: @pranavkm @harshgMSFT @rynowak

In the newest iteration, I added a ResponseEncodingWrapper which ignores the preamble of the original encoding(except the UTF16 little endian which we explicitly set to honor https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.Common/Encodings.cs#L20)

pranavkm · 2015-04-29T21:14:36Z

src/Microsoft.AspNet.Mvc.Core/ActionResults/ContentResult.cs

            }
            else
            {
-                contentTypeHeader = new MediaTypeHeaderValue(ContentType);
+                if(string.IsNullOrEmpty(contentTypeHeader.Charset))


Can this not go away?

pranavkm · 2015-04-29T23:03:20Z

src/Microsoft.AspNet.Mvc.Core/ActionResults/ContentResult.cs


-        public string ContentType { get; set; }
+        public MediaTypeHeaderValue ContentType { get; set; }


While you're touching these files, could you add docs?

pranavkm · 2015-04-30T22:16:33Z

⌚ mostly done. There's a couple of un-addressed items. Would also like @rynowak to have a look at this?

kichalla · 2015-04-30T22:51:29Z

Updated. /cc: @pranavkm @rynowak @ajaybhargavb

pranavkm · 2015-05-01T04:19:55Z

src/Microsoft.AspNet.Mvc.Core/ActionResults/ContentResult.cs


-        public Encoding ContentEncoding { get; set; }
+        public string Content { get; set; }


Docs for this.

pranavkm · 2015-05-01T04:22:52Z

. Try and get @rynowak to look at this before you merge it in.

harshgMSFT · 2015-05-01T20:16:52Z

src/Microsoft.AspNet.Mvc.Core/ActionResults/ContentResult.cs


-        public Encoding ContentEncoding { get; set; }


Are we removing this because ContentEncoding is included in MediaTypeHeaderValue?

yeah, right...(as a side note...this property name is just confusing...as per Http this would represent the encoding like gzip, deflate)..

rynowak · 2015-05-04T18:09:45Z

⌚

harshgMSFT · 2015-05-04T18:14:26Z

src/Microsoft.AspNet.Mvc.Core/ActionResults/FilePathResult.cs

@@ -46,7 +47,7 @@ public FilePathResult([NotNull] string fileName)
        /// <param name="fileName">The path to the file. The path must be an absolute
        /// path. Relative and virtual paths are not supported.</param>
        /// <param name="contentType">The Content-Type header of the response.</param>
-        public FilePathResult([NotNull] string fileName, string contentType)
+        public FilePathResult([NotNull] string fileName, MediaTypeHeaderValue contentType)


[NotNull]

harshgMSFT · 2015-05-04T18:43:12Z

⌚

kichalla · 2015-05-04T20:18:45Z

src/Microsoft.AspNet.Mvc.Core/ActionResults/ViewExecutor.cs


-            actionContext.HttpContext.Response.ContentType = contentType;
-            var wrappedStream = new StreamWrapper(actionContext.HttpContext.Response.Body);
-            var encoding = Encodings.UTF8EncodingWithoutBOM;
            using (var writer = new StreamWriter(wrappedStream, encoding, BufferSize, leaveOpen: true))


As discussed, filed: #2496

kichalla · 2015-05-04T20:22:58Z

Updated as per our discussion. /cc: @pranavkm @rynowak @harshgMSFT

rynowak · 2015-05-04T20:45:23Z

harshgMSFT · 2015-05-04T20:52:43Z

Sorry for commenting on the commit rather than on PR. after addressing the comments.

…sults.

ghost added the cla-already-signed label Apr 24, 2015

kichalla reviewed Apr 24, 2015
View reviewed changes

pranavkm reviewed Apr 27, 2015
View reviewed changes

kichalla force-pushed the kiran/standardize-mediatypeheadervalue branch 3 times, most recently from 258f314 to f17970b Compare April 29, 2015 00:13

kichalla force-pushed the kiran/standardize-mediatypeheadervalue branch from f17970b to 64b77ea Compare April 29, 2015 16:58

pranavkm reviewed Apr 29, 2015
View reviewed changes

kichalla force-pushed the kiran/standardize-mediatypeheadervalue branch from e103bb5 to d3055f7 Compare April 29, 2015 22:46

pranavkm reviewed Apr 29, 2015
View reviewed changes

kichalla force-pushed the kiran/standardize-mediatypeheadervalue branch from 9a0d0d4 to ba3b3e9 Compare May 1, 2015 00:27

pranavkm reviewed May 1, 2015
View reviewed changes

harshgMSFT reviewed May 1, 2015
View reviewed changes

harshgMSFT reviewed May 4, 2015
View reviewed changes

kichalla force-pushed the kiran/standardize-mediatypeheadervalue branch from ba3b3e9 to 29b8c3d Compare May 4, 2015 20:08

kichalla reviewed May 4, 2015
View reviewed changes

Use strongly typed MediaTypeHeaderValue for content type in action re…

7e62325

…sults.

kichalla force-pushed the kiran/standardize-mediatypeheadervalue branch from 29b8c3d to 7e62325 Compare May 4, 2015 21:07

kichalla merged commit 7e62325 into dev May 4, 2015

kichalla deleted the kiran/standardize-mediatypeheadervalue branch May 12, 2015 22:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use strongly typed MediaTypeHeaderValue for content type in action resul... #2450

Use strongly typed MediaTypeHeaderValue for content type in action resul... #2450

kichalla commented Apr 24, 2015

kichalla Apr 24, 2015

harshgMSFT May 4, 2015

kichalla commented Apr 27, 2015

pranavkm Apr 27, 2015

Tratcher Apr 27, 2015

pranavkm Apr 27, 2015

dougbu Apr 27, 2015

pranavkm Apr 27, 2015

kichalla commented Apr 29, 2015

davidfowl commented Apr 29, 2015

pranavkm commented Apr 29, 2015

kichalla commented Apr 29, 2015

pranavkm Apr 29, 2015

pranavkm Apr 29, 2015

pranavkm Apr 30, 2015

pranavkm commented Apr 30, 2015

kichalla commented Apr 30, 2015

pranavkm May 1, 2015

pranavkm commented May 1, 2015

harshgMSFT May 1, 2015

kichalla May 1, 2015

rynowak May 4, 2015

rynowak commented May 4, 2015

harshgMSFT May 4, 2015

harshgMSFT commented May 4, 2015

kichalla May 4, 2015

kichalla commented May 4, 2015

rynowak commented May 4, 2015

harshgMSFT commented May 4, 2015


		public string ContentType { get; set; }
		public MediaTypeHeaderValue ContentType { get; set; }


		public Encoding ContentEncoding { get; set; }
		public string Content { get; set; }

Use strongly typed MediaTypeHeaderValue for content type in action resul... #2450

Use strongly typed MediaTypeHeaderValue for content type in action resul... #2450

Conversation

kichalla commented Apr 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kichalla commented Apr 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kichalla commented Apr 29, 2015

davidfowl commented Apr 29, 2015

pranavkm commented Apr 29, 2015

kichalla commented Apr 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented Apr 30, 2015

kichalla commented Apr 30, 2015

Choose a reason for hiding this comment

pranavkm commented May 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented May 4, 2015

Choose a reason for hiding this comment

harshgMSFT commented May 4, 2015

Choose a reason for hiding this comment

kichalla commented May 4, 2015

rynowak commented May 4, 2015

harshgMSFT commented May 4, 2015