Custom routes breaking on 13.3.1 update

[joshanangelofgrace]

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

13.3.1

Bug summary

After updating to 13.3.1 a key custom route on our website has now broken and returning a 500 error. Prior to the update the route was working fine

Specifics

Example URL: https://dev-kingfisher-fiber.useast01.umbraco.io/contact/in/us

The code for this route

using KingfisherFiber.Core.Infrastructure.Repositories;
using KingfisherFiber.Core.Models.ViewModels;
using KingfisherFiber.Core.Services;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.ViewEngines;
using Microsoft.Extensions.Logging;
using Umbraco.Cms.Core.Models.PublishedContent;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Web;
using Umbraco.Cms.Infrastructure.Scoping;
using Umbraco.Cms.Web.Common.Controllers;

namespace KingfisherFiber.Core.Controllers.Surface
{
    [Route("contact")]
    public class ContactController : RenderController
    {
        private readonly ILogger<RenderController> logger;
        private readonly CountryRepository countryRepository;
        private readonly ContactRepository contactRepository;
        private readonly IScopeProvider provider;
        private readonly IPersonalisationService personalisationService;
        private readonly IVariationContextAccessor variationContextAccessor;
        private readonly ServiceContext serviceContext;

        public ContactController(CountryRepository countryRepository, ContactRepository contactRepository,
                                 IScopeProvider provider, IPersonalisationService personalisationService,
                                 ILogger<RenderController> logger, ICompositeViewEngine compositeViewEngine,
                                 IUmbracoContextAccessor umbracoContextAccessor,
                                 IVariationContextAccessor variationContextAccessor, ServiceContext serviceContext)
            : base(logger, compositeViewEngine, umbracoContextAccessor)
        {
            this.variationContextAccessor = variationContextAccessor;
            this.serviceContext = serviceContext;
            this.countryRepository = countryRepository;
            this.contactRepository = contactRepository;
            this.provider = provider;
            this.personalisationService = personalisationService;
            this.logger = logger;
        }

        [Route("in/{countryKey}")]
        public IActionResult In(string countryKey)
        {
            logger.LogDebug("Retrieving information for country {countryKey}", countryKey);

            using (provider.CreateScope(autoComplete: true))
            {
                personalisationService.EnsureSessionProps(HttpContext);
                // ensure country URL uses ISO codes, and is lower case.
                var country = countryRepository.Find(countryKey);
                var sourceCountry = country;
                if (country == null)
                {
                    return new RedirectResult($"/contact/in/");
                }
                else if (countryKey != country.IsoKey.ToLower())
                {
                    return new RedirectResult($"/contact/in/{country.IsoKey.ToLower()}/");
                }

                var distributors = contactRepository.Get(ContactType.Distributor, sourceCountry).ToList();
                var reps = contactRepository.Get(ContactType.SalesRep, sourceCountry).ToList();

                if (distributors.Count + reps.Count == 0)
                {
                    // attempt to redirect
                    sourceCountry = countryRepository.FindRedirected(country);
                    distributors = contactRepository.Get(ContactType.Distributor, sourceCountry).ToList();
                    reps = contactRepository.Get(ContactType.SalesRep, sourceCountry).ToList();
                }
                if (distributors.Count + reps.Count == 0)
                {
                    // ignore redirect, expand to region
                    sourceCountry = country;
                    distributors = contactRepository.GetFromRegion(ContactType.Distributor, country.RegionName).ToList();
                    reps = contactRepository.GetFromRegion(ContactType.SalesRep, country.RegionName).ToList();
                }


                var model = new ContactPageViewModel(CurrentPage, new PublishedValueFallback(serviceContext, variationContextAccessor))
                {
                    IsCurrentCountry = personalisationService.UserSession.Country?.Code == country.IsoKey,
                    Country = country,
                    RedirectedTo = sourceCountry,
                    Distributors = distributors,
                    SalesReps = reps
                };

                logger.LogDebug("Country Information: {@Model}", model);

                return PartialView("CountryView", model);
            }
        }
    }
}

Steps to reproduce

Update the site to 13.3.1 and navigate to this URL

Expected result / actual result

Routes to the URL as per normal prior to the update

[github-actions]

Hi there @joshanangelofgrace!

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

[jemayn]

Just wanted to pitch in and say we have the same issue on an IVirtualPageController which has worked just fine for weeks before Cloud auto upgraded our site yesterday.

[karl-sjogren]

This relates to #16332 and probably #16218 (which was included in 13.3.1).

[bergmania]

Anyone that can help check, if this fix will work for you ?

The following code will replace the EagerMatcherPolicy from 13.3.1 with a custom (equal to the candidate for 13.3.2)

using Microsoft.AspNetCore.Mvc.Controllers;
using Microsoft.AspNetCore.Routing.Matching;
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.Routing;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Web.Common.Controllers;
using Umbraco.Cms.Web.Website.Routing;

namespace Umbraco.Cms.Web.UI;

public class MyComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        var serviceToRemove = builder.Services.First(x => x.ImplementationType?.Name == "EagerMatcherPolicy");
        builder.Services.Remove(serviceToRemove);

        builder.Services.AddSingleton<MatcherPolicy, MyEagerMatcherPolicy>();
    }
}

internal class MyEagerMatcherPolicy : MatcherPolicy, IEndpointSelectorPolicy
{
    private readonly IRuntimeState _runtimeState;
    private readonly EndpointDataSource _endpointDataSource;
    private readonly UmbracoRequestPaths _umbracoRequestPaths;
    private GlobalSettings _globalSettings;
    private readonly Lazy<Endpoint> _installEndpoint;
    private readonly Lazy<Endpoint> _renderEndpoint;

    public MyEagerMatcherPolicy(
        IRuntimeState runtimeState,
        EndpointDataSource endpointDataSource,
        UmbracoRequestPaths umbracoRequestPaths,
        IOptionsMonitor<GlobalSettings> globalSettings)
    {
        _runtimeState = runtimeState;
        _endpointDataSource = endpointDataSource;
        _umbracoRequestPaths = umbracoRequestPaths;
        _globalSettings = globalSettings.CurrentValue;
        globalSettings.OnChange(settings => _globalSettings = settings);
        _installEndpoint = new Lazy<Endpoint>(GetInstallEndpoint);
        _renderEndpoint = new Lazy<Endpoint>(GetRenderEndpoint);
    }

    // We want this to run as the very first policy, so we can discard the UmbracoRouteValueTransformer before the framework runs it.
    public override int Order => int.MinValue + 10;

    // We know we don't have to run this matcher against the backoffice endpoints.
    public bool AppliesToEndpoints(IReadOnlyList<Endpoint> endpoints) => true;

    public async Task ApplyAsync(HttpContext httpContext, CandidateSet candidates)
    {
        if (_runtimeState.Level != RuntimeLevel.Run)
        {
            var handled = await HandleInstallUpgrade(httpContext, candidates);
            if (handled)
            {
                return;
            }
        }

        // If there's only one candidate, we don't need to do anything.
        var candidateCount = candidates.Count;
        if (candidateCount < 2)
        {
            return;
        }

        // If there are multiple candidates, we want to discard the catch-all (slug)
        // IF there is any candidates with a lower order. Since this will be a statically routed endpoint registered before the dynamic route.
        // Which means that we don't have to run our UmbracoRouteValueTransformer to route dynamically (expensive).
        var lowestOrder = int.MaxValue;
        int? dynamicId = null;
        RouteEndpoint? dynamicEndpoint = null;
        for (var i = 0; i < candidates.Count; i++)
        {
            if (candidates.IsValidCandidate(i) is false)
            {
                // If the candidate is not valid we reduce the candidate count so we can later ensure that there is always
                // at least 1 candidate.
                candidateCount -= 1;
                continue;
            }

            CandidateState candidate = candidates[i];

            // If it's not a RouteEndpoint there's not much we can do to count it in the order.
            if (candidate.Endpoint is not RouteEndpoint routeEndpoint)
            {
                continue;
            }

            if (routeEndpoint.Order < lowestOrder)
            {
                // We have to ensure that the route is valid for the current request method.
                // This is because attribute routing will always have an order of 0.
                // This means that you could attribute route a POST to /example, but also have an umbraco page at /example
                // This would then result in a 404, because we'd see the attribute route with order 0, and always consider that the lowest order
                // We'd then disable the dynamic endpoint since another endpoint has a lower order, and end up with only 1 invalid endpoint.
                // (IsValidCandidate does not take this into account since the candidate itself is still valid)
                HttpMethodMetadata? methodMetaData = routeEndpoint.Metadata.GetMetadata<HttpMethodMetadata>();
                if (methodMetaData?.HttpMethods.Contains(httpContext.Request.Method) is false)
                {
                    continue;
                }

                lowestOrder = routeEndpoint.Order;
            }

            // We only want to consider our dynamic route, this way it's still possible to register your own custom route before ours.
            if (routeEndpoint.DisplayName != Constants.Web.Routing.DynamicRoutePattern)
            {
                continue;
            }

            dynamicEndpoint = routeEndpoint;
            dynamicId = i;
        }

        // Invalidate the dynamic route if another route has a lower order.
        // This means that if you register your static route after the dynamic route, the dynamic route will take precedence
        // This more closely resembles the existing behaviour.
        if (dynamicEndpoint is not null && dynamicId is not null && dynamicEndpoint.Order > lowestOrder && candidateCount > 1)
        {
            candidates.SetValidity(dynamicId.Value, false);
        }
    }

    /// <summary>
    /// Replaces the first endpoint candidate with the specified endpoint, invalidating all other candidates,
    /// guaranteeing that the specified endpoint will be hit.
    /// </summary>
    /// <param name="candidates">The candidate set to manipulate.</param>
    /// <param name="endpoint">The target endpoint that will be hit.</param>
    /// <param name="routeValueDictionary"></param>
    private static void SetEndpoint(CandidateSet candidates, Endpoint endpoint, RouteValueDictionary routeValueDictionary)
    {
        candidates.ReplaceEndpoint(0, endpoint, routeValueDictionary);

        for (int i = 1; i < candidates.Count; i++)
        {
            candidates.SetValidity(1, false);
        }
    }

    private Endpoint GetInstallEndpoint()
    {
        Endpoint endpoint = _endpointDataSource.Endpoints.First(x =>
        {
            ControllerActionDescriptor? descriptor = x.Metadata.GetMetadata<ControllerActionDescriptor>();
            return descriptor?.ControllerTypeInfo.Name == "InstallController"
                   && descriptor.ActionName == "Index";
        });

        return endpoint;
    }

    private Endpoint GetRenderEndpoint()
    {
        Endpoint endpoint = _endpointDataSource.Endpoints.First(x =>
        {
            ControllerActionDescriptor? descriptor = x.Metadata.GetMetadata<ControllerActionDescriptor>();
            return descriptor?.ControllerTypeInfo == typeof(RenderController)
                   && descriptor.ActionName == nameof(RenderController.Index);
        });

        return endpoint;
    }

    private Task<bool> HandleInstallUpgrade(HttpContext httpContext, CandidateSet candidates)
    {
        if (_runtimeState.Level != RuntimeLevel.Upgrade)
        {
            // We need to let the installer API requests through
            // Currently we do this with a check for the installer path
            // Ideally we should do this in a more robust way, for instance with a dedicated attribute we can then check for.
            if (_umbracoRequestPaths.IsInstallerRequest(httpContext.Request.Path))
            {
                return Task.FromResult(true);
            }

            SetEndpoint(candidates, _installEndpoint.Value, new RouteValueDictionary
            {
                [Constants.Web.Routing.ControllerToken] = "Install",
                [Constants.Web.Routing.ActionToken] = "Index",
                [Constants.Web.Routing.AreaToken] = Constants.Web.Mvc.InstallArea,
            });

            return Task.FromResult(true);
        }

        // Check if maintenance page should be shown
        // Current behaviour is that statically routed endpoints still work in upgrade state
        // This means that IF there is a static route, we should not show the maintenance page.
        // And instead carry on as we normally would.
        var hasStaticRoute = false;
        for (var i = 0; i < candidates.Count; i++)
        {
            CandidateState candidate = candidates[i];
            IDynamicEndpointMetadata? dynamicEndpointMetadata = candidate.Endpoint.Metadata.GetMetadata<IDynamicEndpointMetadata>();
            if (dynamicEndpointMetadata is null || dynamicEndpointMetadata.IsDynamic is false)
            {
                hasStaticRoute = true;
                break;
            }
        }

        if (_runtimeState.Level != RuntimeLevel.Upgrade
            || _globalSettings.ShowMaintenancePageWhenInUpgradeState is false
            || hasStaticRoute)
        {
            return Task.FromResult(false);
        }

        // Otherwise we'll re-route to the render controller (this will in turn show the maintenance page through a filter)
        // With this approach however this could really just be a plain old endpoint instead of a filter.
        SetEndpoint(candidates, _renderEndpoint.Value, new RouteValueDictionary
        {
            [Constants.Web.Routing.ControllerToken] = ControllerExtensions.GetControllerName<RenderController>(),
            [Constants.Web.Routing.ActionToken] = nameof(RenderController.Index),
        });

        return Task.FromResult(true);

    }
}