Fix #1370 - Always use the provided formatter in JsonResult #1480
Conversation
|
|
||
| public JsonResult(object data) : | ||
| this(data, null) | ||
| private static readonly MediaTypeHeaderValue[] _defaultSupportedContentTypes = new MediaTypeHeaderValue[] |
There was a problem hiding this comment.
Should we make these public, so the user can add to them rather than having to retype if he wants an extra possible content type?
There was a problem hiding this comment.
#918 😆
Yeah, I think it's a good idea to do both.
var result = new JsonResult(...);
result.ContentTypes.AddRange(JsonResult.DefaultContentTypes);
result.ContentTypes.Add(...);
|
looks good to me, I didn't go in details over the tests though, so when you get a review from that you have a |
from me
The change here is to always use the provided formatter, instead of using it as a fallback. This is much less surprising for users. There are some other subtle changes here and cleanup of the tests, as well as documentation additions. The primary change is that we still want to run 'select' on a formatter even if it's the only one. This allows us to choose a content type based on the accept header. In the case of a user-provided formatter, we'll try to honor the best possible combination of Accept and specified ContentTypes (specified ContentTypes win if there's a conflict). If nothing works, we'll still run the user-provided formatter and let it decide what to do. In the case of the default (formatters from options) we do conneg, and if there's a conflict, fall back to a global (from services) JsonOutputFormatter - we let it decide what to do. This should leave us with a defined and tested behavior for all cases.
| .GetRequiredService<IOutputFormattersProvider>() | ||
| .OutputFormatters; | ||
|
|
||
| var formatter = objectResult.SelectFormatter(formatterContext, formatters); |
There was a problem hiding this comment.
Wondering what the behavior should be when someone sets the value to be null or a string type...I ask this because currently, by default, NoContent and TextPlain formatters would come into picture for these values respectively...
My expectation is that when someones explicitly uses JsonResult, they really want json and no other formatter type should come into picture...thoughts?
@harshgMSFT @yishaigalatzer
There was a problem hiding this comment.
NoContentFormatter only activates when the declared type is void/object. We don't set declared type in JsonResult.
NoContentFormatter actually does pick this up.
There was a problem hiding this comment.
cool..thanks!...could you add couple of tests for this scenario?
There was a problem hiding this comment.
Sure, I'll add a functional test that does null.
There was a problem hiding this comment.
Ok, coming back to this after some discussion... It turns out NoContentFormatter does pick this up and write a 204. So I went back to old-MVC, which just does nothing if the value is null.
So in that spirit 204 is actually right.
There was a problem hiding this comment.
Probably we should add a functional test which turns on the TreatNullValueAsNoContent on the HttpNoContentOutputFormatter so that user is able to get output which is formatted with JsonFormatter.
We have a test which adds only modified HttpNoContentOutputFormatter https://github.com/aspnet/Mvc/blob/dev/test/WebSites/ConnegWebSite/Controllers/NoContentDoNotTreatNullValueAsNoContentController.cs
but this seems to be a good opportunity to enhance the tests.
|
|
| } | ||
|
|
||
| var formatterContext = new OutputFormatterContext() | ||
| { | ||
| DeclaredType = _objectResult.DeclaredType, |
There was a problem hiding this comment.
Shouldn't we always honor the default Declared type which is present in objectResult and pass that around?
There was a problem hiding this comment.
What's the suggestion here? In all possible cases objectResult.DeclaredType will be null.
There was a problem hiding this comment.
this is fragile because it depends on this value explicitly being null.
Consider a case in future where objectResult.DeclaredType was set to value?.GetType() in object result constructor.
I was thinking of not removing DeclaredType = _objectResult.DeclaredType
There was a problem hiding this comment.
Makes sense 👍 I'll put it back
|
@harshgMSFT updated |
| public JsonResult(object data, IOutputFormatter defaultFormatter) | ||
| /// <param name="value">The value to format as JSON.</param> | ||
| public JsonResult(object value) | ||
| : this(value, null) |
|
|
|
@pranavkm updated |
|
@harshgMSFT |
|
@rynowak I saw that, but that uses the NoContentFormatter, I wanted to have a scenario where the user wanted the behavior which kiran mentioned So instead of NoContent, they get a valid response but with null content. |
|
|
|
|
The change here is to always use the provided formatter, instead of using
it as a fallback. This is much less surprising for users.
There are some other subtle changes here and cleanup of the tests, as well
as documentation additions.
The primary change is that we still want to run 'select' on a formatter
even if it's the only one. This allows us to choose a content type based
on the accept header.
In the case of a user-provided formatter, we'll try to honor the best
possible combination of Accept and specified ContentTypes (specified
ContentTypes win if there's a conflict). If nothing works, we'll still run
the user-provided formatter and let it decide what to do.
In the case of the default (formatters from options) we do conneg, and if
there's a conflict, fall back to a global (from services)
JsonOutputFormatter - we let it decide what to do.
This should leave us with a defined and tested behavior for all cases.