Skip to content
This repository was archived by the owner on Dec 14, 2018. It is now read-only.

Fix #1370 - Always use the provided formatter in JsonResult #1480

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 85 additions & 65 deletions src/Microsoft.AspNet.Mvc.Core/ActionResults/JsonResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,104 +9,124 @@

namespace Microsoft.AspNet.Mvc
{
/// <summary>
/// An action result which formats the given object as JSON.
/// </summary>
public class JsonResult : ActionResult
{
private static readonly IList<MediaTypeHeaderValue> _defaultSupportedContentTypes =
new List<MediaTypeHeaderValue>()
{
MediaTypeHeaderValue.Parse("application/json"),
MediaTypeHeaderValue.Parse("text/json"),
};
private IOutputFormatter _defaultFormatter;

private ObjectResult _objectResult;

public JsonResult(object data) :
this(data, null)
/// <summary>
/// The list of content-types used for formatting when <see cref="ContentTypes"/> is null or empty.
/// </summary>
public static readonly IReadOnlyList<MediaTypeHeaderValue> DefaultContentTypes = new MediaTypeHeaderValue[]
{
}
MediaTypeHeaderValue.Parse("application/json"),
MediaTypeHeaderValue.Parse("text/json"),
};

/// <summary>
/// Creates an instance of <see cref="JsonResult"/> class.
/// Creates a new <see cref="JsonResult"/> with the given <paramref name="data"/>.
/// </summary>
/// <param name="data"></param>
/// <param name="defaultFormatter">If no matching formatter is found,
/// the response is written to using defaultFormatter.</param>
/// <remarks>
/// The default formatter must be able to handle either application/json
/// or text/json.
/// </remarks>
public JsonResult(object data, IOutputFormatter defaultFormatter)
/// <param name="value">The value to format as JSON.</param>
public JsonResult(object value)
: this(value, formatter: null)
{
_defaultFormatter = defaultFormatter;
_objectResult = new ObjectResult(data);
}

public object Value
/// <summary>
/// Creates a new <see cref="JsonResult"/> with the given <paramref name="data"/>.
/// </summary>
/// <param name="value">The value to format as JSON.</param>
/// <param name="formatter">The formatter to use, or <c>null</c> to choose a formatter dynamically.</param>
public JsonResult(object value, IOutputFormatter formatter)
{
get
{
return _objectResult.Value;
}
set
{
_objectResult.Value = value;
}
}
Value = value;
Formatter = formatter;

public IList<MediaTypeHeaderValue> ContentTypes
{
get
{
return _objectResult.ContentTypes;
}
set
{
_objectResult.ContentTypes = value;
}
ContentTypes = new List<MediaTypeHeaderValue>();
}

/// <summary>
/// Gets or sets the list of supported Content-Types.
/// </summary>
public IList<MediaTypeHeaderValue> ContentTypes { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make the property a getter only? Saves the trouble of having to do null checks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus this was a FxCop design violation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is consistent with ObjectResult, leaving it alone.


/// <summary>
/// Gets or sets the formatter.
/// </summary>
public IOutputFormatter Formatter { get; set; }

/// <summary>
/// Gets or sets the value to be formatted.
/// </summary>
public object Value { get; set; }

/// <inheritdoc />
public override async Task ExecuteResultAsync([NotNull] ActionContext context)
{
var objectResult = new ObjectResult(Value);

// Set the content type explicitly to application/json and text/json.
// if the user has not already set it.
if (ContentTypes == null || ContentTypes.Count == 0)
{
ContentTypes = _defaultSupportedContentTypes;
foreach (var contentType in DefaultContentTypes)
{
objectResult.ContentTypes.Add(contentType);
}
}
else
{
objectResult.ContentTypes = ContentTypes;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we always honor the default Declared type which is present in objectResult and pass that around?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the suggestion here? In all possible cases objectResult.DeclaredType will be null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 👍 I'll put it back


var formatterContext = new OutputFormatterContext()
{
DeclaredType = _objectResult.DeclaredType,
ActionContext = context,
DeclaredType = objectResult.DeclaredType,
Object = Value,
};

// Need to call this instead of directly calling _objectResult.ExecuteResultAsync
// as that sets the status to 406 if a formatter is not found.
// this can be cleaned up after https://github.com/aspnet/Mvc/issues/941 gets resolved.
var formatter = SelectFormatter(formatterContext);
var formatter = SelectFormatter(objectResult, formatterContext);
await formatter.WriteAsync(formatterContext);
}

private IOutputFormatter SelectFormatter(OutputFormatterContext formatterContext)
private IOutputFormatter SelectFormatter(ObjectResult objectResult, OutputFormatterContext formatterContext)
{
var defaultFormatters = formatterContext.ActionContext
.HttpContext
.RequestServices
.GetRequiredService<IOutputFormattersProvider>()
.OutputFormatters;

var formatter = _objectResult.SelectFormatter(formatterContext, defaultFormatters);
if (formatter == null)
if (Formatter == null)
{
formatter = _defaultFormatter ?? formatterContext.ActionContext
.HttpContext
.RequestServices
.GetRequiredService<JsonOutputFormatter>();
}
// If no formatter was provided, then run Conneg with the formatters configured in options.
var formatters = formatterContext
.ActionContext
.HttpContext
.RequestServices
.GetRequiredService<IOutputFormattersProvider>()
.OutputFormatters
.OfType<IJsonOutputFormatter>()
.ToArray();

var formatter = objectResult.SelectFormatter(formatterContext, formatters);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoContentFormatter only activates when the declared type is void/object. We don't set declared type in JsonResult.

NoContentFormatter actually does pick this up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool..thanks!...could you add couple of tests for this scenario?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll add a functional test that does null.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (formatter == null)
{
// If the available user-configured formatters can't write this type, then fall back to the
// 'global' one.
formatter = formatterContext
.ActionContext
.HttpContext
.RequestServices
.GetRequiredService<JsonOutputFormatter>();

return formatter;
// Run SelectFormatter again to try to choose a content type that this formatter can do.
objectResult.SelectFormatter(formatterContext, new[] { formatter });
}

return formatter;
}
else
{
// Run SelectFormatter to try to choose a content type that this formatter can do.
objectResult.SelectFormatter(formatterContext, new[] { Formatter });
return Formatter;
}
}
}
}
20 changes: 20 additions & 0 deletions src/Microsoft.AspNet.Mvc.Core/Formatters/IJsonOutputFormatter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// 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.

namespace Microsoft.AspNet.Mvc
{
/// <summary>
/// An output formatter that specializes in writing JSON content.
/// </summary>
/// <remarks>
/// The <see cref="JsonResult"/> class filter the collection of
/// <see cref="IOutputFormattersProvider.OutputFormatters"/> and use only those which implement
/// <see cref="IJsonOutputFormatter"/>.
///
/// To create a custom formatter that can be used by <see cref="JsonResult"/>, derive from
/// <see cref="JsonOutputFormatter"/> or implement <see cref="IJsonOutputFormatter"/>.
/// </remarks>
public interface IJsonOutputFormatter : IOutputFormatter
{
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

namespace Microsoft.AspNet.Mvc
{
public class JsonOutputFormatter : OutputFormatter
public class JsonOutputFormatter : OutputFormatter, IJsonOutputFormatter
{
private JsonSerializerSettings _serializerSettings;

Expand Down
Loading