-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Use strongly typed MediaTypeHeaderValue for content type in action resul... #2450
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
// 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; | ||
using System.IO; | ||
using System.Text; | ||
using System.Threading.Tasks; | ||
using Microsoft.AspNet.Http; | ||
|
@@ -11,11 +13,20 @@ namespace Microsoft.AspNet.Mvc | |
{ | ||
public class ContentResult : ActionResult | ||
{ | ||
public string Content { get; set; } | ||
private readonly MediaTypeHeaderValue DefaultContentType = new MediaTypeHeaderValue("text/plain") | ||
{ | ||
Encoding = Encodings.UTF8EncodingWithoutBOM | ||
}; | ||
|
||
public Encoding ContentEncoding { get; set; } | ||
/// <summary> | ||
/// Gets or set the content representing the body of the response. | ||
/// </summary> | ||
public string Content { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docs for this. |
||
|
||
public string ContentType { get; set; } | ||
/// <summary> | ||
/// Gets or sets the <see cref="MediaTypeHeaderValue"/> representing the Content-Type header of the response. | ||
/// </summary> | ||
public MediaTypeHeaderValue ContentType { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While you're touching these files, could you add docs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bump. |
||
|
||
/// <summary> | ||
/// Gets or sets the HTTP status code. | ||
|
@@ -26,17 +37,30 @@ public override async Task ExecuteResultAsync([NotNull] ActionContext context) | |
{ | ||
var response = context.HttpContext.Response; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't change the code to be less correct 😆 |
||
MediaTypeHeaderValue contentTypeHeader; | ||
if (string.IsNullOrEmpty(ContentType)) | ||
var contentTypeHeader = ContentType; | ||
Encoding encoding; | ||
if (contentTypeHeader == null) | ||
{ | ||
contentTypeHeader = new MediaTypeHeaderValue("text/plain"); | ||
contentTypeHeader = DefaultContentType; | ||
encoding = Encodings.UTF8EncodingWithoutBOM; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
else | ||
{ | ||
contentTypeHeader = new MediaTypeHeaderValue(ContentType); | ||
if (contentTypeHeader.Encoding == null) | ||
{ | ||
// 1. Do not modify the user supplied content type | ||
// 2. Parse here to handle parameters apart from charset | ||
contentTypeHeader = MediaTypeHeaderValue.Parse(contentTypeHeader.ToString()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't fully follow - why are we There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we do not want to modify the instance of MediaTypeHeaderValue value that the user supplied to ContentType property and also there could be additional parameters...test: https://github.com/aspnet/Mvc/pull/2450/files#diff-3be60edcf4b71964bc2ec1b25a0984fbR99 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a .Clone hack. See aspnet/HttpAbstractions#176 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
contentTypeHeader.Encoding = Encodings.UTF8EncodingWithoutBOM; | ||
|
||
encoding = Encodings.UTF8EncodingWithoutBOM; | ||
} | ||
else | ||
{ | ||
encoding = contentTypeHeader.Encoding; | ||
} | ||
} | ||
|
||
contentTypeHeader.Encoding = ContentEncoding ?? Encodings.UTF8EncodingWithoutBOM; | ||
response.ContentType = contentTypeHeader.ToString(); | ||
|
||
if (StatusCode != null) | ||
|
@@ -46,7 +70,7 @@ public override async Task ExecuteResultAsync([NotNull] ActionContext context) | |
|
||
if (Content != null) | ||
{ | ||
await response.WriteAsync(Content, contentTypeHeader.Encoding); | ||
await response.WriteAsync(Content, encoding); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
using System.Threading.Tasks; | ||
using Microsoft.AspNet.Http; | ||
using Microsoft.Framework.Internal; | ||
using Microsoft.Net.Http.Headers; | ||
|
||
namespace Microsoft.AspNet.Mvc | ||
{ | ||
|
@@ -19,13 +20,14 @@ public class FileContentResult : FileResult | |
|
||
/// <summary> | ||
/// Creates a new <see cref="FileContentResult"/> instance with | ||
/// the provided <paramref name="fileContents"/>. | ||
/// the provided <paramref name="fileContents"/> and the | ||
/// provided <paramref name="contentType"/>. | ||
/// </summary> | ||
/// <param name="fileContents">The bytes that represent the file contents.</param> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we compare to MVC5, content type was always required: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ). |
||
public FileContentResult([NotNull] byte[] fileContents) | ||
: base(contentType: null) | ||
/// <param name="contentType">The Content-Type header of the response.</param> | ||
public FileContentResult([NotNull] byte[] fileContents, [NotNull] string contentType) | ||
: this(fileContents, new MediaTypeHeaderValue(contentType)) | ||
{ | ||
FileContents = fileContents; | ||
} | ||
|
||
/// <summary> | ||
|
@@ -35,7 +37,7 @@ public FileContentResult([NotNull] byte[] fileContents) | |
/// </summary> | ||
/// <param name="fileContents">The bytes that represent the file contents.</param> | ||
/// <param name="contentType">The Content-Type header of the response.</param> | ||
public FileContentResult([NotNull] byte[] fileContents, string contentType) | ||
public FileContentResult([NotNull] byte[] fileContents, [NotNull] MediaTypeHeaderValue contentType) | ||
: base(contentType) | ||
{ | ||
FileContents = fileContents; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
using Microsoft.AspNet.Mvc.Core; | ||
using Microsoft.Framework.DependencyInjection; | ||
using Microsoft.Framework.Internal; | ||
using Microsoft.Net.Http.Headers; | ||
|
||
namespace Microsoft.AspNet.Mvc | ||
{ | ||
|
@@ -27,15 +28,15 @@ public class FilePathResult : FileResult | |
|
||
/// <summary> | ||
/// Creates a new <see cref="FilePathResult"/> instance with | ||
/// the provided <paramref name="fileName"/> | ||
/// the provided <paramref name="fileName"/> and the | ||
/// provided <paramref name="contentType"/>. | ||
/// </summary> | ||
/// <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) | ||
: base(contentType: null) | ||
public FilePathResult([NotNull] string fileName, [NotNull] string contentType) | ||
: this(fileName, new MediaTypeHeaderValue(contentType)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this throw if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, it does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe throw a |
||
{ | ||
FileName = fileName; | ||
} | ||
|
||
/// <summary> | ||
|
@@ -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, [NotNull] MediaTypeHeaderValue contentType) | ||
: base(contentType) | ||
{ | ||
FileName = fileName; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,15 +24,25 @@ public abstract class FileResult : ActionResult | |
/// the provided <paramref name="contentType"/>. | ||
/// </summary> | ||
/// <param name="contentType">The Content-Type header of the response.</param> | ||
protected FileResult(string contentType) | ||
protected FileResult([NotNull] string contentType) | ||
: this(new MediaTypeHeaderValue(contentType)) | ||
{ | ||
} | ||
|
||
/// <summary> | ||
/// Creates a new <see cref="FileResult"/> instance with | ||
/// the provided <paramref name="contentType"/>. | ||
/// </summary> | ||
/// <param name="contentType">The Content-Type header of the response.</param> | ||
protected FileResult([NotNull] MediaTypeHeaderValue contentType) | ||
{ | ||
ContentType = contentType; | ||
} | ||
|
||
/// <summary> | ||
/// Gets or sets the Content-Type header value that will be written to the response. | ||
/// Gets the <see cref="MediaTypeHeaderValue"/> representing the Content-Type header of the response. | ||
/// </summary> | ||
public string ContentType { get; set; } | ||
public MediaTypeHeaderValue ContentType { get; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it intentional to make this readonly property ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think it's intentional because the underlying property on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🆗 |
||
|
||
/// <summary> | ||
/// Gets the file name that will be used in the Content-Disposition header of the response. | ||
|
@@ -47,11 +57,7 @@ public string FileDownloadName | |
public override Task ExecuteResultAsync([NotNull] ActionContext context) | ||
{ | ||
var response = context.HttpContext.Response; | ||
|
||
if (ContentType != null) | ||
{ | ||
response.ContentType = ContentType; | ||
} | ||
response.ContentType = ContentType.ToString(); | ||
|
||
if (!string.IsNullOrEmpty(FileDownloadName)) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
using System.Threading.Tasks; | ||
using Microsoft.AspNet.Http; | ||
using Microsoft.Framework.Internal; | ||
using Microsoft.Net.Http.Headers; | ||
|
||
namespace Microsoft.AspNet.Mvc | ||
{ | ||
|
@@ -23,13 +24,14 @@ public class FileStreamResult : FileResult | |
|
||
/// <summary> | ||
/// Creates a new <see cref="FileStreamResult"/> instance with | ||
/// the provided <paramref name="fileStream"/>. | ||
/// the provided <paramref name="fileStream"/> and the | ||
/// provided <paramref name="contentType"/>. | ||
/// </summary> | ||
/// <param name="fileStream">The stream with the file.</param> | ||
public FileStreamResult([NotNull] Stream fileStream) | ||
: base(contentType: null) | ||
/// <param name="contentType">The Content-Type header of the response.</param> | ||
public FileStreamResult([NotNull] Stream fileStream, [NotNull] string contentType) | ||
: this(fileStream, new MediaTypeHeaderValue(contentType)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, is this intentional - the single overload ctor disappeared. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes it is intentional https://github.com/aspnet/Mvc/pull/2450/files#r29091219 |
||
{ | ||
FileStream = fileStream; | ||
} | ||
|
||
/// <summary> | ||
|
@@ -39,7 +41,7 @@ public FileStreamResult([NotNull] Stream fileStream) | |
/// </summary> | ||
/// <param name="fileStream">The stream with the file.</param> | ||
/// <param name="contentType">The Content-Type header of the response.</param> | ||
public FileStreamResult([NotNull] Stream fileStream, string contentType) | ||
public FileStreamResult([NotNull] Stream fileStream, [NotNull] MediaTypeHeaderValue contentType) | ||
: base(contentType) | ||
{ | ||
FileStream = fileStream; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,13 @@ | ||
// 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; | ||
using System.IO; | ||
using System.Text; | ||
using System.Threading.Tasks; | ||
using Microsoft.AspNet.Mvc.Rendering; | ||
using Microsoft.Framework.Internal; | ||
using Microsoft.Net.Http.Headers; | ||
|
||
namespace Microsoft.AspNet.Mvc | ||
{ | ||
|
@@ -14,7 +17,10 @@ namespace Microsoft.AspNet.Mvc | |
public static class ViewExecutor | ||
{ | ||
private const int BufferSize = 1024; | ||
private const string ContentType = "text/html; charset=utf-8"; | ||
private static readonly MediaTypeHeaderValue DefaultContentType = new MediaTypeHeaderValue("text/html") | ||
{ | ||
Encoding = Encodings.UTF8EncodingWithoutBOM | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comments here |
||
|
||
/// <summary> | ||
/// Asynchronously renders the specified <paramref name="view"/> to the response body. | ||
|
@@ -23,21 +29,43 @@ public static class ViewExecutor | |
/// <param name="actionContext">The <see cref="ActionContext"/> for the current executing action.</param> | ||
/// <param name="viewData">The <see cref="ViewDataDictionary"/> for the view being rendered.</param> | ||
/// <param name="tempData">The <see cref="ITempDataDictionary"/> for the view being rendered.</param> | ||
/// <returns>A <see cref="Task"/> that represents the asychronous rendering.</returns> | ||
/// <returns>A <see cref="Task"/> that represents the asynchronous rendering.</returns> | ||
public static async Task ExecuteAsync([NotNull] IView view, | ||
[NotNull] ActionContext actionContext, | ||
[NotNull] ViewDataDictionary viewData, | ||
[NotNull] ITempDataDictionary tempData, | ||
string contentType) | ||
MediaTypeHeaderValue contentType) | ||
{ | ||
if (string.IsNullOrEmpty(contentType)) | ||
var response = actionContext.HttpContext.Response; | ||
|
||
var contentTypeHeader = contentType; | ||
Encoding encoding; | ||
if (contentTypeHeader == null) | ||
{ | ||
contentType = ContentType; | ||
contentTypeHeader = DefaultContentType; | ||
encoding = Encodings.UTF8EncodingWithoutBOM; | ||
} | ||
else | ||
{ | ||
if (contentTypeHeader.Encoding == null) | ||
{ | ||
// 1. Do not modify the user supplied content type | ||
// 2. Parse here to handle parameters apart from charset | ||
contentTypeHeader = MediaTypeHeaderValue.Parse(contentTypeHeader.ToString()); | ||
contentTypeHeader.Encoding = Encodings.UTF8EncodingWithoutBOM; | ||
|
||
encoding = Encodings.UTF8EncodingWithoutBOM; | ||
} | ||
else | ||
{ | ||
encoding = contentTypeHeader.Encoding; | ||
} | ||
} | ||
|
||
response.ContentType = contentTypeHeader.ToString(); | ||
|
||
var wrappedStream = new StreamWrapper(response.Body); | ||
|
||
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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed, filed: #2496 |
||
{ | ||
try | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we removing this because ContentEncoding is included in
MediaTypeHeaderValue
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, right...(as a side note...this property name is just confusing...as per Http this would represent the encoding like gzip, deflate)..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍