TreeController implementations can return incorrect routes

[nathanwoulfe]

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

version 11.2.0-rc.preview.14

Bug summary

If two packages implement tree controllers with the same class name eg SettingsTreeController, whichever is registered first in the ActionDescriptionCollection will provide the route information both tree instances.

This was raised as a bug in Workflow, when running alongside Vendr, original bug report is here => https://github.com/umbraco/Umbraco.Workflow.Issues/issues/11. Bug was raised on Umbraco+Workflow 10, so impacts at least Umbraco 10 and 11.

Steps to reproduce

To reproduce, add the below class to a clean v11 install.

namespace MySite.Web.Trees;

[Tree("settings", "myCustomTree", TreeTitle = "My Custom Tree", SortOrder = 1)]
[PluginController("myPlugin")]
public class StylesheetsTreeController : TreeController
{
    public StylesheetsTreeController(
        ILocalizedTextService textService,
        UmbracoApiControllerTypeCollection umbracoApiControllerTypeCollection,
        IEventAggregator eventAggregator)
        : base(textService, umbracoApiControllerTypeCollection, eventAggregator)
    {

    }

    public ActionResult<TreeNode?> CreateRootNode(FormCollection queryStrings, string section, string routePath, string icon, bool hasChildren = false, string? menuUrl = null)
    {
        ActionResult<TreeNode?>? root = base.CreateRootNode(queryStrings);

        if (root.Value is null)
        {
            return root;
        }

        root.Value.RoutePath = section + "/" + routePath + "/overview";
        root.Value.Icon = icon;
        root.Value.HasChildren = hasChildren;
        root.Value.MenuUrl = menuUrl;

        return root;
    }

    protected override ActionResult<TreeNodeCollection> GetTreeNodes(string id, [ModelBinder(typeof(HttpQueryStringModelBinder))] FormCollection queryStrings) => new TreeNodeCollection();
    protected override ActionResult<MenuItemCollection> GetMenuForNode(string id, [ModelBinder(typeof(HttpQueryStringModelBinder))] FormCollection queryStrings) => null!;
}

Starting the site will add a tree to the settings section, in the Third Party group, labelled My Custom Tree (that's all expected behaviour). However, it also replaces the Stylesheets tree in the Templating section with a second instance of My Custom Tree (this is not cool).

I wasn't going to investigate this one, had intended just raising the issue but started poking around and found the likely cause. In ApplicationTreeController, we have this:

https://github.com/umbraco/Umbraco-CMS/blob/066276a532522defcdbb339d0e3b51e94a4c1c65/src/Umbraco.Web.BackOffice/Trees/ApplicationTreeController.cs#L352-L356

Can we see the problem? We select the first ControllerActionDescriptor matching the controller name and action. then use that to instantiate an instance of the tree controller, but when we have two controllers of the same name, we hit this issue.

Figured we could fix it with this change - adding the additional condition to ensure the descriptor comes from the same namespace as the currently rendering tree - but I'm not sure if this is the best approach:

        ControllerActionDescriptor? actionDescriptor = _actionDescriptorCollectionProvider.ActionDescriptors.Items
            .Cast<ControllerActionDescriptor>()
            .First(x =>
                x.ControllerName.Equals(controllerName) &&
               (x.ControllerTypeInfo.Namespace ?? string.Empty).Equals(controllerType.Namespace) &&
                x.ActionName == action);

Expected result / actual result

Trees with the same controller name in different namespaces should render with the correct route data.

---

_This item has been added to our backlog AB#30925_

[elit0451]

Hey @nathanwoulfe 😊

Thank you for all the details you provided about the issue 🙌 and kudos for finding where the problem begins in the codebase!

I was able to reproduce and try out your solution, however, it appears that the descending items of the original tree are "copied over", even though the tree names go back to being the correct ones. When trying to delete them from the second tree, we get an error that they don't exist, which is true but that just shows that there is more to the issue than the additional check that you suggested.

I will mark this as up for grabs as it is a valid issue. 🙂

[ustadstar]

I am working on this!

[ustadstar]

@elit0451 I have researched this issue and found what causes the problem, it occurs to be a problem with how routes are resolved. Unfortunately I am stuck and do not know how to solve this. So if I can get some guidance on this issue I will try and fix it if possible.

I can confirm that the fix of @nathanwoulfe solves the issue with the rootnode, but the childItems of Stylesheets are shown / copied in MyCustomTree.

When loading the tree nodes, a function named GetRootNode is called in ApplicationTreeController.cs: https://github.com/umbraco/Umbraco-CMS/blob/066276a532522defcdbb339d0e3b51e94a4c1c65/src/Umbraco.Web.BackOffice/Trees/ApplicationTreeController.cs#L271

GetRootNode further calls another function CreateRootNode, which then calls the function GetTreeUrl:

https://github.com/umbraco/Umbraco-CMS/blob/066276a532522defcdbb339d0e3b51e94a4c1c65/src/Umbraco.Web.BackOffice/Trees/TreeControllerBase.cs#L250-L254

The GetUmbracoApiService is then called and at last url.Action() which generates / gets the url for getting the childItems of a rootNode, The issue lies here:

https://github.com/umbraco/Umbraco-CMS/blob/066276a532522defcdbb339d0e3b51e94a4c1c65/src/Umbraco.Web.Common/Extensions/UrlHelperExtensions.cs#L198-L201

url.Action() returns the following url for our custom stylesheetcontroller: /umbraco/backoffice/api/stylesheetstree/GetNodes?area=myPlugin?id=-1&application=settings&tree=&use=main&culture=

As you can see the area variable (myPlugin) which has been passed to the url.Action method gets placed in the query string. I don't think this is default behaviour. When I call this url the stylesheetcontroller of core umbraco is being called and not our newly created custom stylesheetcontroller.

I have tested this with another custom conroller, named MyCustomTreeController. The url.Action method returns: /umbraco/backoffice/myplugin/mycustomtree/GetNodes?id=-1&application=settings&tree=&use=main&culture

So it seems to me that when we use core umbraco controller names, the area attribute [PluginController("myPlugin") is being ignored, and the url of the core umbraco controller is being returned.

[elit0451]

We are quite busy at the moment but I will bring this up with the team and get back to you 🙂 Great work 💪

[nathanwoulfe]

Bit more info for this one - my original hypothesis around class names is not entirely correct. While the issue does present when class names appear in multiple packages (ie Workflow and Vendr, per the original issue).

However, I've had a fresh report of the same issue, where the tree controllers do not share the same class name. In this case, the issue affects uSupport and Workflow, where the tree controllers look like this:

// uSupport settings tree controller
[PluginController(uSupportConstants.SectionAlias)] // "uSupport"
[Tree(uSupportConstants.SectionAlias, "settings", SortOrder = 5, TreeTitle = "Settings", TreeGroup = TreeGroupAlias)] // SectionAlias => "uSupport", TreeGroupAlias => "uSupportGroup"
public class uSupportSettingsTreeController : TreeController {}

// Workflow settings tree controller
[PluginController(Constants.ApplicationName)] // "Workflow"
[Tree(Constants.ApplicationName, Constants.Trees.Settings, SortOrder = 60)] // "Workflow", "settings"
public sealed class WorkflowSettingsTreeController : WorkflowTreeController {}

Based on those examples, it looks like @ustadstar is on the right track - the area name in the PluginController attribute isn't being used to correctly determine the tree to render.

@dalbard, tagging you here as the issue raised for Workflow (https://github.com/umbraco/Umbraco.Workflow.Issues/issues/16) needs to be fixed in the CMS.

[nathanwoulfe]

Digging into this again today, there's definitely an issue in the client-side logic when generating paths for tree templates, as we only use the tree alias to determine the package, which will fail when multiple packages have trees with the same alias.

Will see what I can do to fix, but I suspect this is additional to the original problem.

[nathanwoulfe]

I think I've tracked this down to a missing attribute on the StylesheetsTreeController class... When I add [PluginController(Constants.Web.Mvc.BackOfficeTreeArea)] to the core StylesheetsTreeController, my custom StylesheetsTreeController works correctly (ie both render the correct child nodes).

I've updated the linked PR with some additional changes, including adding the attribute to the stylesheets controller. I don't know if that attribute was intentionally omitted, so will need a review from someone who knows trees better than I do.

[elit0451]

Fixed in: #14268