Adding TextPlainFormatter to always handle returning strings as text\pla... #868
Conversation
| @@ -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)); | |||
There was a problem hiding this comment.
hmm..looks like my comment in other PR didn't make it...named parameter and also indent should be false
There was a problem hiding this comment.
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)
|
Branch updated. |
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Add support for StringBuilder and StringWriter and open a issue to followup @pranavkm when he is done with the RazorTextWriter to see if we think it's useful here as well.
|
Hmm I take it back, there are essential pieces missing here |
I'm ok with the one minor casting change to push in, and followup with the
|
@RickStrahl this I think is a nice way to address your feedback in #629. It's easy enough to opt out of this behavior by simply removing this default formatter (or customizing it) |
| /// </summary> | ||
| public class TextPlainFormatter : IOutputFormatter | ||
| { | ||
| public bool CanWriteResult(OutputFormatterContext context, MediaTypeHeaderValue contentType) |
There was a problem hiding this comment.
make it virtual, so it's easy to understand how to override the content type handling behavior
|
This PR should also include the change removing the string handling from the object content result |
|
The object result pr does remove that. I was planning to push this in post the changes of object result. |
|
Branch Updated. |
| using (var writer = new StreamWriter(response.Body, context.SelectedEncoding, 1024, leaveOpen: true)) | ||
| { | ||
| await writer.WriteAsync(valueAsString); | ||
| } |
There was a problem hiding this comment.
Couple of things to note here:
- When the value is
null, I am not sure if we should be explicitly setting theContent-Lengthheader to0as otherwise the host layers might think that you are sending a chunked response... - In Web API, we prevent double buffering for scenarios where we can already calculate the length of the response content and as part of this
StringContentis not buffered again as we can calculate the length of it...so need to see how its going to be in our case...
yeah, both these above points depend upon the response buffering story that is yet to be decided i think..
/cc: @yishaigalatzer
There was a problem hiding this comment.
Well the first one is certainly doable even if we don't do buffering.
There was a problem hiding this comment.
Open up a followup issue for setting content length in responses. I think the answer is that we should not set it, as it depends heavily on the encoding.
@Tratcher got guidance
There was a problem hiding this comment.
Setting Content-Length is strongly preferred to chunked, if you can calculate it with a reasonable amount of effort. In this case, consider calling context.SelectedEncoding.GetByteCount and see how that affects your perf. The result will vary widely depending on your selected encoding (ASCII = constant, UTF-8 = variable).
There was a problem hiding this comment.
Should we consider putting this in the SendAsync extensions?
https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNet.Http.Extensions/HttpResponseSendingExtensions.cs#L72
There was a problem hiding this comment.
lets follow up on #919
| namespace Microsoft.AspNet.Mvc | ||
| { | ||
| /// <summary> | ||
| /// Writes a string value to the response. |
There was a problem hiding this comment.
Always writes a string value to the response, regardless of requested contenttype
|
Enable the tests for string |
One Functional test will be added after #849