-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Adding TextPlainFormatter to always handle returning strings as text\pla... #868
Changes from 1 commit
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 |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| // Copyright (c) Microsoft Open Technologies, Inc. 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.Text; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.AspNet.Http; | ||
| using Microsoft.AspNet.Mvc; | ||
| using Microsoft.AspNet.Mvc.HeaderValueAbstractions; | ||
|
|
||
| namespace Microsoft.AspNet.Mvc | ||
| { | ||
| /// <summary> | ||
| /// Writes a string value to the response. | ||
| /// </summary> | ||
| public class TextPlainFormatter : OutputFormatter | ||
|
Contributor
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. PlanTextFormatter?
Contributor
Author
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. followed the pattern of MediaTypeMediaSubType. |
||
| { | ||
| public TextPlainFormatter() | ||
| { | ||
| SupportedMediaTypes.Add(MediaTypeHeaderValue.Parse("text/plain")); | ||
|
Contributor
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 should just be cached on the class, or even better just cached in a static class
Contributor
Author
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. |
||
| SupportedEncodings.Add(Encoding.GetEncoding("utf-8")); | ||
|
Contributor
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. Shouldn't this be
Contributor
Author
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. you are right, however this doesn't get used. Will update |
||
| } | ||
|
|
||
| public override bool CanWriteResult(OutputFormatterContext context, MediaTypeHeaderValue contentType) | ||
| { | ||
| if (base.CanWriteResult(context, contentType)) | ||
|
Contributor
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.
Contributor
Author
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. Orthogonal to Pranav's comment, with the current implementation this is going to participate in content negotiation, if accept header or content type (set by using ProducesContent attr) is set. Now as per my discussion with @yishaigalatzer we want to always return text/plain format if the object is a string. However this means that if some action (or class ) had a [ProducesContent("application/json")]then the produces content attribute gets ignored which in my opinion is not right.
Contributor
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.
-----Original Message----- In src/Microsoft.AspNet.Mvc.Core/Formatters/TextPlainFormatter.cs:
Contributor
Author
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. well,not honoring the attribute does not sound good, however if the 99% scenario is it will be text/plain for a string (which I think it is) then this is fine.
Contributor
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. That's the intended design. What does the base do? -----Original Message----- In src/Microsoft.AspNet.Mvc.Core/Formatters/TextPlainFormatter.cs:
|
||
| { | ||
| var valueAsString = context.Object as string; | ||
| if (valueAsString != null) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
|
Contributor
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. Add support for
Contributor
Author
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. Next PR> |
||
| } | ||
|
|
||
| public override void WriteResponseContentHeaders(OutputFormatterContext context) | ||
| { | ||
| // Ignore the accept-charset field, as this will always write utf-8. | ||
| var response = context.ActionContext.HttpContext.Response; | ||
| response.ContentType = "text/plain;charset=utf-8"; | ||
| } | ||
|
|
||
| public override async Task WriteResponseBodyAsync(OutputFormatterContext context) | ||
| { | ||
| var response = context.ActionContext.HttpContext.Response; | ||
| var valueAsString = context.Object as string; | ||
|
Contributor
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. do an explicit cast instead of an
Contributor
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. make the content type declarative on a property |
||
| await response.WriteAsync(valueAsString); | ||
|
Contributor
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 you aren't doing any async work in this, you could return the result of WriteAsync |
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ public void Setup(MvcOptions options) | |
| options.ModelBinders.Add(new ComplexModelDtoModelBinder()); | ||
|
|
||
| // Set up default output formatters. | ||
| options.OutputFormatters.Add(new TextPlainFormatter()); | ||
| options.OutputFormatters.Add(new JsonOutputFormatter(JsonOutputFormatter.CreateDefaultSettings(), true)); | ||
|
Member
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. hmm..looks like my comment in other PR didn't make it...named parameter and also indent should be
Contributor
Author
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 is my bad- actually I had to base it off another PR ( the actual code in the repo has the correct value https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc/MvcOptionsSetup.cs) |
||
|
|
||
| // Set up ValueProviders | ||
|
|
||
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.
Always writes a string value to the response, regardless of requested contenttype