Bugfixes for MNTP / EntityController (#11685)

* Added EntityController.GetUrlsByIds support for int & guid + update MNTP

Fixes issue with MNTP (for 8.18) in a partial view macro - GH #11631

Renamed GetUrlsByUdis to match, don't do this in v9 as it would be
breaking there, instead mark it obsolete.

TODO: v9 ensure integration test coverage, more painful here as no
WebApplicationFactory.

* Added partial test coverage for GetUrlsByIds.

This doesn't actually work in the backoffice because of GH #11448

So lets fix that next.

* Failing test demonstrating #11448

* Fix for #11448 getByIds doesn't work as expected.

ParameterSwapControllerActionSelectorAttribute - cached body survived between requests.

* Expand on sync vs async comment for future confused souls.

* Might aswell cache parsed json vs string for performance

* Make ParameterSwapControllerActionSelector remarks more accurate.

* Share deserialized request body between action constraint and model binder

* Be more defensive with RequestBodyAsJObject HttpContext item

Only store if deserialize success.
Don't assume key being present means can cast as JObject.

* Nest constant a little deeper.

* Final defensive tweak

Author: Paul Johnson

src/Umbraco.Core/Constants-HttpContextItemsKeys.cs

@@ -0,0 +1,19 @@
+namespace Umbraco.Cms.Core
+{
+    public static partial class Constants
+    {
+        public static class HttpContext
+        {
+            /// <summary>
+            /// Defines keys for items stored in HttpContext.Items
+            /// </summary>
+            public static class Items
+            {
+                /// <summary>
+                /// Key for current requests body deserialized as JObject.
+                /// </summary>
+                public const string RequestBodyAsJObject = "RequestBodyAsJObject";
+            }
+        }
+    }
+}

src/Umbraco.Web.BackOffice/Controllers/EntityController.cs

@@ -54,6 +54,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers
     [ParameterSwapControllerActionSelector(nameof(GetById), "id", typeof(int), typeof(Guid), typeof(Udi))]
     [ParameterSwapControllerActionSelector(nameof(GetByIds), "ids", typeof(int[]), typeof(Guid[]), typeof(Udi[]))]
     [ParameterSwapControllerActionSelector(nameof(GetUrl), "id", typeof(int), typeof(Udi))]
+    [ParameterSwapControllerActionSelector(nameof(GetUrlsByIds), "ids", typeof(int[]), typeof(Guid[]), typeof(Udi[]))]
     public class EntityController : UmbracoAuthorizedJsonController
     {
         private static readonly string[] _postFilterSplitStrings = { "=", "==", "!=", "<>", ">", "<", ">=", "<=" };
@@ -319,45 +320,182 @@ public IActionResult GetUrl(Udi id, string culture = "*")
         }
 
         /// <summary>
-        ///     Get entity URLs by UDIs
+        /// Get entity URLs by IDs
         /// </summary>
-        /// <param name="udis">
-        ///     A list of UDIs to lookup items by
+        /// <param name="ids">
+        /// A list of IDs to lookup items by
         /// </param>
-        /// <param name="culture">The culture to fetch the URL for</param>
+        /// <param name="type">The entity type to look for.</param>
+        /// <param name="culture">The culture to fetch the URL for.</param>
         /// <returns>Dictionary mapping Udi -> Url</returns>
         /// <remarks>
-        ///     We allow for POST because there could be quite a lot of Ids.
+        /// We allow for POST because there could be quite a lot of Ids.
         /// </remarks>
         [HttpGet]
         [HttpPost]
-        public IDictionary<Udi, string> GetUrlsByUdis([FromJsonPath] Udi[] udis, string culture = null)
+        public IDictionary<int, string> GetUrlsByIds([FromJsonPath] int[] ids, [FromQuery] UmbracoEntityTypes type, [FromQuery] string culture = null)
+        {
+            if (ids == null || !ids.Any())
+            {
+                return new Dictionary<int, string>();
+            }
+
+            string MediaOrDocumentUrl(int id)
+            {
+                switch (type)
+                {
+                    case UmbracoEntityTypes.Document:
+                        return _publishedUrlProvider.GetUrl(id, culture: culture ?? ClientCulture());
+
+                    case UmbracoEntityTypes.Media:
+                    {
+                        IPublishedContent media = _publishedContentQuery.Media(id);
+
+                        // NOTE: If culture is passed here we get an empty string rather than a media item URL.
+                        return _publishedUrlProvider.GetMediaUrl(media, culture: null);
+                    }
+
+                    default:
+                        return null;
+                }
+            }
+
+            return ids
+                .Distinct()
+                .Select(id => new {
+                    Id = id,
+                    Url = MediaOrDocumentUrl(id)
+                }).ToDictionary(x => x.Id, x => x.Url);
+        }
+
+        /// <summary>
+        /// Get entity URLs by IDs
+        /// </summary>
+        /// <param name="ids">
+        /// A list of IDs to lookup items by
+        /// </param>
+        /// <param name="type">The entity type to look for.</param>
+        /// <param name="culture">The culture to fetch the URL for.</param>
+        /// <returns>Dictionary mapping Udi -> Url</returns>
+        /// <remarks>
+        /// We allow for POST because there could be quite a lot of Ids.
+        /// </remarks>
+        [HttpGet]
+        [HttpPost]
+        public IDictionary<Guid, string> GetUrlsByIds([FromJsonPath] Guid[] ids, [FromQuery] UmbracoEntityTypes type, [FromQuery] string culture = null)
         {
-            if (udis == null || udis.Length == 0)
+            if (ids == null || !ids.Any())
+            {
+                return new Dictionary<Guid, string>();
+            }
+
+            string MediaOrDocumentUrl(Guid id)
+            {
+                return type switch
+                {
+                    UmbracoEntityTypes.Document => _publishedUrlProvider.GetUrl(id, culture: culture ?? ClientCulture()),
+
+                    // NOTE: If culture is passed here we get an empty string rather than a media item URL.
+                    UmbracoEntityTypes.Media => _publishedUrlProvider.GetMediaUrl(id, culture: null),
+
+                    _ => null
+                };
+            }
+
+            return ids
+                .Distinct()
+                .Select(id => new {
+                    Id = id,
+                    Url = MediaOrDocumentUrl(id)
+                }).ToDictionary(x => x.Id, x => x.Url);
+        }
+
+        /// <summary>
+        /// Get entity URLs by IDs
+        /// </summary>
+        /// <param name="ids">
+        /// A list of IDs to lookup items by
+        /// </param>
+        /// <param name="type">The entity type to look for.</param>
+        /// <param name="culture">The culture to fetch the URL for.</param>
+        /// <returns>Dictionary mapping Udi -> Url</returns>
+        /// <remarks>
+        /// We allow for POST because there could be quite a lot of Ids.
+        /// </remarks>
+        [HttpGet]
+        [HttpPost]
+        public IDictionary<Udi, string> GetUrlsByIds([FromJsonPath] Udi[] ids, [FromQuery] UmbracoEntityTypes type, [FromQuery] string culture = null)
+        {
+            if (ids == null || !ids.Any())
             {
                 return new Dictionary<Udi, string>();
             }
 
             // TODO: PMJ 2021-09-27 - Should GetUrl(Udi) exist as an extension method on UrlProvider/IUrlProvider (in v9)
-            string MediaOrDocumentUrl(Udi udi)
+            string MediaOrDocumentUrl(Udi id)
             {
-                if (udi is not GuidUdi guidUdi)
+                if (id is not GuidUdi guidUdi)
                 {
                     return null;
                 }
 
-                return guidUdi.EntityType switch
+                return type switch
                 {
-                    Constants.UdiEntityType.Document => _publishedUrlProvider.GetUrl(guidUdi.Guid,
-                        culture: culture ?? ClientCulture()),
-                    // NOTE: If culture is passed here we get an empty string rather than a media item URL WAT
-                    Constants.UdiEntityType.Media => _publishedUrlProvider.GetMediaUrl(guidUdi.Guid, culture: null),
+                    UmbracoEntityTypes.Document => _publishedUrlProvider.GetUrl(guidUdi.Guid, culture: culture ?? ClientCulture()),
+
+                    // NOTE: If culture is passed here we get an empty string rather than a media item URL.
+                    UmbracoEntityTypes.Media => _publishedUrlProvider.GetMediaUrl(guidUdi.Guid, culture: null),
+
                     _ => null
                 };
             }
 
-            return udis
-                .Select(udi => new { Udi = udi, Url = MediaOrDocumentUrl(udi) }).ToDictionary(x => x.Udi, x => x.Url);
+            return ids
+                .Distinct()
+                .Select(id => new {
+                    Id = id,
+                    Url = MediaOrDocumentUrl(id)
+                }).ToDictionary(x => x.Id, x => x.Url);
+        }
+
+        /// <summary>
+        ///     Get entity URLs by UDIs
+        /// </summary>
+        /// <param name="udis">
+        ///     A list of UDIs to lookup items by
+        /// </param>
+        /// <param name="culture">The culture to fetch the URL for</param>
+        /// <returns>Dictionary mapping Udi -> Url</returns>
+        /// <remarks>
+        ///     We allow for POST because there could be quite a lot of Ids.
+        /// </remarks>
+        [HttpGet]
+        [HttpPost]
+        [Obsolete("Use GetUrlsByIds instead.")]
+        public IDictionary<Udi, string> GetUrlsByUdis([FromJsonPath] Udi[] udis, string culture = null)
+        {
+            if (udis == null || !udis.Any())
+            {
+                return new Dictionary<Udi, string>();
+            }
+
+            var udiEntityType = udis.First().EntityType;
+            UmbracoEntityTypes entityType;
+
+            switch (udiEntityType)
+            {
+                case Constants.UdiEntityType.Document:
+                    entityType = UmbracoEntityTypes.Document;
+                    break;
+                case Constants.UdiEntityType.Media:
+                    entityType = UmbracoEntityTypes.Media;
+                    break;
+                default:
+                    entityType = (UmbracoEntityTypes)(-1);
+                    break;
+            }
+
+            return GetUrlsByIds(udis, entityType, culture);
         }
 
         /// <summary>

src/Umbraco.Web.BackOffice/Controllers/ParameterSwapControllerActionSelectorAttribute.cs

@@ -1,22 +1,48 @@
-using System;
+using System;
 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;
 using Newtonsoft.Json.Linq;
+using Umbraco.Cms.Core;
 using Umbraco.Extensions;
 
 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>
+    ///
+    /// [HasParameterFromUriOrBodyOfType("ids", typeof(Guid[]))]
+    /// public IActionResult GetByIds([FromJsonPath] Guid[] ids) { }
+    ///
+    /// [HasParameterFromUriOrBodyOfType("ids", typeof(int[]))]
+    /// public IActionResult GetByIds([FromJsonPath] int[] ids) { }
+    /// </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)
         {
@@ -33,10 +59,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;
@@ -49,23 +77,45 @@ 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();
+                JObject postBodyJson;
 
-                var json = JsonConvert.DeserializeObject<JObject>(strJson);
+                if (httpContext.Items.TryGetValue(Constants.HttpContext.Items.RequestBodyAsJObject, out var value) && value is JObject cached)
+                {
+                    postBodyJson = cached;
+                }
+                else
+                {
+                    // 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).
+                    //
+                    // To expand on the above, if KestrelServerOptions/IISServerOptions is AllowSynchronousIO=false
+                    // And you attempt to read stream sync an InvalidOperationException is thrown with message
+                    // "Synchronous operations are disallowed. Call ReadAsync or set AllowSynchronousIO to true instead."
+                    var rawBody = Task.Run(() => httpContext.Request.GetRawBodyStringAsync()).GetAwaiter().GetResult();
+                    try
+                    {
+                        postBodyJson = JsonConvert.DeserializeObject<JObject>(rawBody);
+                        httpContext.Items[Constants.HttpContext.Items.RequestBodyAsJObject] = postBodyJson;
+                    }
+                    catch (JsonException)
+                    {
+                        postBodyJson = null;
+                    }
+                }
 
-                if (json == null)
+                if (postBodyJson == null)
                 {
                     return null;
                 }
 
-                var requestParam = json[_parameterName];
+                var requestParam = postBodyJson[_parameterName];
 
                 if (requestParam != null)
                 {
@@ -85,11 +135,7 @@ public bool Accept(ActionConstraintContext context)
                                 }
                             }
                         }
-                        catch (JsonReaderException)
-                        {
-                            // can't convert
-                        }
-                        catch (JsonSerializationException)
+                        catch (JsonException)
                         {
                             // can't convert
                         }