Skip to content

Commit

Permalink
Fix for #11448 getByIds doesn't work as expected.
Browse files Browse the repository at this point in the history
ParameterSwapControllerActionSelectorAttribute - cached body survived between requests.
  • Loading branch information
Paul Johnson committed Nov 23, 2021
1 parent 7f1563d commit 14014b3
Showing 1 changed file with 52 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Linq;
using System.Net.Http;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.ActionConstraints;
using Microsoft.AspNetCore.Mvc.Controllers;
using Newtonsoft.Json;
Expand All @@ -10,13 +11,32 @@

namespace Umbraco.Cms.Web.BackOffice.Controllers
{
/// <remarks>
/// <para>
/// This attribute is odd because it applies at class level where some methods may use it whilst others don't.
/// </para>
///
/// <para>
/// What we should probably have (if we really even need something like this at all) is an attribute for method level.
///
/// <example>
/// <code>
/// public IActionResult Foo([FromUriOrBodyOfType(typeof(Guid))] Guid id) GetById {}
/// public IActionResult Foo([FromUriOrBodyOfType(typeof(int))] int id) GetById {}
/// </code>
/// </example>
/// </para>
///
/// <para>
/// That way we wouldn't need confusing things like Accept returning true when action name doesn't even match attribute metadata.
/// </para>
/// </remarks>
[AttributeUsage(AttributeTargets.Class, AllowMultiple = true, Inherited = true)]
internal class ParameterSwapControllerActionSelectorAttribute : Attribute, IActionConstraint
{
private readonly string _actionName;
private readonly string _parameterName;
private readonly Type[] _supportedTypes;
private string _requestBody;

public ParameterSwapControllerActionSelectorAttribute(string actionName, string parameterName, params Type[] supportedTypes)
{
Expand All @@ -33,10 +53,12 @@ public bool Accept(ActionConstraintContext context)
{
if (!IsValidCandidate(context.CurrentCandidate))
{
// See remarks on class, required because we apply at class level
// and some controllers have some actions with parameter swaps and others without.
return true;
}

var chosenCandidate = SelectAction(context);
ActionSelectorCandidate? chosenCandidate = SelectAction(context);

var found = context.CurrentCandidate.Equals(chosenCandidate);
return found;
Expand All @@ -49,16 +71,37 @@ public bool Accept(ActionConstraintContext context)
return candidate;
}

HttpContext httpContext = context.RouteContext.HttpContext;

// if it's a post we can try to read from the body and bind from the json value
if (context.RouteContext.HttpContext.Request.Method == HttpMethod.Post.ToString())
if (context.RouteContext.HttpContext.Request.Method.Equals(HttpMethod.Post.Method))
{
// We need to use the asynchronous method here if synchronous IO is not allowed (it may or may not be, depending
// on configuration in UmbracoBackOfficeServiceCollectionExtensions.AddUmbraco()).
// We can't use async/await due to the need to override IsValidForRequest, which doesn't have an async override, so going with
// this, which seems to be the least worst option for "sync to async" (https://stackoverflow.com/a/32429753/489433).
var strJson = _requestBody ??= Task.Run(() => context.RouteContext.HttpContext.Request.GetRawBodyStringAsync()).GetAwaiter().GetResult();
string body;
var requestItemKey = $"{nameof(ParameterSwapControllerActionSelectorAttribute)}_Body";

if (httpContext.Items.TryGetValue(requestItemKey, out var cached))
{
body = cached.ToString();
}
else
{
// Note: Copied the below comment, it's not clear, there's a sync GetRawBodyString extension that doesn't care about config?
// Taking their word for it for now.

// We need to use the asynchronous method here if synchronous IO is not allowed (it may or may not be, depending
// on configuration in UmbracoBackOfficeServiceCollectionExtensions.AddUmbraco()).
// We can't use async/await due to the need to override IsValidForRequest, which doesn't have an async override, so going with
// this, which seems to be the least worst option for "sync to async" (https://stackoverflow.com/a/32429753/489433).
body = Task.Run(() => context.RouteContext.HttpContext.Request.GetRawBodyStringAsync()).GetAwaiter().GetResult();
httpContext.Items[requestItemKey] = body;
}

if (body == null)
{
return null;
}

var json = JsonConvert.DeserializeObject<JObject>(strJson);
var json = JsonConvert.DeserializeObject<JObject>(body);

if (json == null)
{
Expand Down

0 comments on commit 14014b3

Please sign in to comment.