umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.41k stars 2.66k forks source link

Custom MVC routes should not go through all content finders #16015

Closed simonech closed 4 months ago

simonech commented 5 months ago

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

13.3.0-rc, but it affects versions already before

Bug summary

If I create a custom MVC route, umbraco goes through all the content finders defined, and even via the last chance content finder. And then it executes my custom MVC route.

If I specify my own MVC route, I know what I want, and I want to go straight to my controller. This causes performance problems since the ContentFinderByRedirectUrl does a DB query to see if it matches some URL in the redirect tracking table. And if we have many custom content finder the performance hit is even bigger, especially since it also goes through the last chance content finder, which also gets ready to render the 404 page.

Steps to reproduce

To reproduce, just create a new project from scratch Install the DB and create a doctype with a property with title as alias

this in the program.cs file (i just add a lastchancefinder and the custom route with the find content method)

builder.CreateUmbracoBuilder()
    .AddBackOffice()
    .AddWebsite()
    .AddDeliveryApi()
    .AddComposers()
    .SetContentLastChanceFinder<My404ContentFinder>()
    .Build();

WebApplication app = builder.Build();

await app.BootUmbracoAsync();

app.UseUmbraco()
    .WithMiddleware(u =>
    {
        u.UseBackOffice();
        u.UseWebsite();
    })
    .WithEndpoints(u =>
    {
        u.EndpointRouteBuilder.MapControllerRoute(
            "PageController",
            "/Page",
            new {Controller = "Page", Action = "Index"}
            ).ForUmbracoPage(FindContent);

        u.UseInstallerEndpoints();
        u.UseBackOfficeEndpoints();
        u.UseWebsiteEndpoints();
    });

IPublishedContent FindContent(ActionExecutingContext context)
{
        var umbracoContextAccessor = context.HttpContext.RequestServices
            .GetRequiredService<IUmbracoContextAccessor>();
        var publishedValueFallback = context.HttpContext.RequestServices
            .GetRequiredService<IPublishedValueFallback>();

        var umbracoContext = umbracoContextAccessor.GetRequiredUmbracoContext();
        var page = umbracoContext.Content.GetById(1059); //This is the id of node in my umbraco DB
        return page;
}

Then create a controller to handle that custom route

using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.ViewEngines;
using Umbraco.Cms.Web.Common.Controllers;

public class PageController : UmbracoPageController
{
    private readonly ILogger<UmbracoPageController> _logger;

    public PageController(
        ILogger<UmbracoPageController> logger,
        ICompositeViewEngine compositeViewEngine)
        : base(logger, compositeViewEngine)
        {
            _logger = logger;
        }

    [HttpGet]
    public IActionResult Index()
    {
        _logger.LogWarning("Page Controller: " + CurrentPage.Value<string>("title"));

        return Ok();
    }
}

And finally the lastchancecontent finder

using Umbraco.Cms.Core.Routing;
using Umbraco.Cms.Core.Web;

public class My404ContentFinder : IContentLastChanceFinder
{
    private readonly IUmbracoContextAccessor _umbracoContextAccessor;

    public My404ContentFinder(IUmbracoContextAccessor umbracoContextAccessor)
    {
        _umbracoContextAccessor = umbracoContextAccessor;
    }

    public Task<bool> TryFindContent(IPublishedRequestBuilder contentRequest)
    {
        if (!_umbracoContextAccessor.TryGetUmbracoContext(out var umbracoContext))
        {
            return Task.FromResult(false);
        }

        var notFoundNode = umbracoContext.Content.GetById(1061); //This is another random id in my DB

        if (notFoundNode is not null)
        {
            contentRequest.SetPublishedContent(notFoundNode);
        }

        return Task.FromResult(contentRequest.PublishedContent is not null);
    }
}

All code is copied almost verbatim from the custom routing pages in umbraco docs

Enabling debug in the logs you see the full list of content finder being hit, including the last chance one.

Expected result / actual result

When I call /Page I would expect to see see just my log entry from my controller (assuming Homepage is the value of the title property in my content [21:30:27 WRN] Page Controller: Homepage

while I get the full list of content finders being executed, even the last-chance content finder

[21:48:56 DBG] FindPublishedContentAndTemplate: Path=/page
[21:48:56 DBG] FindPublishedContent: Begin finders [Timing b752635]
[21:48:56 DBG] Finder Umbraco.Cms.Core.Routing.ContentFinderByPageIdQuery
[21:48:56 DBG] Finder Umbraco.Cms.Core.Routing.ContentFinderByUrl
[21:48:56 DBG] Test route /page
[21:48:56 DBG] No match.
[21:48:56 DBG] Finder Umbraco.Cms.Core.Routing.ContentFinderByIdPath
[21:48:56 DBG] Not a node id
[21:48:56 DBG] Finder Umbraco.Cms.Core.Routing.ContentFinderByUrlAlias
[21:48:56 DBG] Finder Umbraco.Cms.Core.Routing.ContentFinderByRedirectUrl
[21:48:56 DBG] No match for route: /page
[21:48:56 DBG] Found? False, Content: NULL, Template: NULL, Domain: NULL, Culture: en-US, StatusCode: null
[21:48:56 DBG] FindPublishedContent: End finders (2ms) [Timing b752635]
[21:48:56 DBG] HandlePublishedContent: Loop 0
[21:48:56 DBG] HandlePublishedContent: No document, try last chance lookup
[21:48:56 DBG] HandlePublishedContent: Found a document
[21:48:56 DBG] HandlePublishedContent: End
[21:48:56 DBG] GetTemplateModel: Get template id=1057
[21:48:56 DBG] GetTemplateModel: Got template id=1057 alias=Content
[21:48:56 DBG] FindTemplate: Running with template id=1057 alias=Content
[21:48:56 WRN] Page Controller: Homepage

Exactly the same happens even if I use a normal Controller that is not an Umbraco controller:

public class TestController : Controller
{
    private readonly ILogger<TestController> _logger;

    public TestController(ILogger<TestController> logger)
    {
        _logger = logger;
    }

    [HttpGet]
    public IActionResult Index()
    {
        _logger.LogWarning("Test Controller");
        return Ok();
    }
}

        u.EndpointRouteBuilder.MapControllerRoute(
            "TestController",
            "/Test/{action}",
            new {Controller = "Test", Action = "Index"}
            );

And same behaviour if I use a Route["/Test"] attribute instead of the endpoint. And even if I specify this endpoint outside of the app.UseUmbraco context.

The only workaround I found is to add the custom mvc routes in the ReservedPaths settings so that Umbraco ignores them. But doesn't seems right

github-actions[bot] commented 5 months ago

Hi there @simonech!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

simonech commented 4 months ago

Wow, looking at the PR it seems like it was pretty complicate. Thanks for getting to bottom of it. The main issue was the content last chance finder that was setting the current node and then making it impossible to set a node in the custom controller as it was always overwritten by the one in the last chance one.

nikolajlauridsen commented 4 months ago

It did get a bit complicated yeah 😅 Turns out it's an issue that other CMSes has also run into.

But I just wanted to pop in and say thanks for the detailed reproduction steps, they were vital in solving the issue H5YR 🙌