Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
simonech opened this issue Apr 8, 2024 · 3 comments
Closed

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

simonech opened this issue Apr 8, 2024 · 3 comments
Labels
category/performance Fixes for performance (generally cpu or memory) fixes release/13.3.1 type/bug

Comments

@simonech
Copy link
Contributor

simonech commented Apr 8, 2024

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

Copy link

github-actions bot commented Apr 8, 2024

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'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
  • If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
  • We'll replicate the issue to ensure that the problem is as described.
  • We'll decide whether the behavior is an issue or if the behavior is intended.

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 🤖 🙂

@andr317c andr317c added the state/needs-investigation This requires input from HQ or community to proceed label Apr 11, 2024
@nikolajlauridsen nikolajlauridsen removed the state/needs-investigation This requires input from HQ or community to proceed label May 6, 2024
@bergmania bergmania added release/13.3.1 category/performance Fixes for performance (generally cpu or memory) fixes labels May 10, 2024
@simonech
Copy link
Contributor Author

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
Copy link
Contributor

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 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/performance Fixes for performance (generally cpu or memory) fixes release/13.3.1 type/bug
Projects
None yet
Development

No branches or pull requests

4 participants