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.42k stars 2.67k forks source link

Controller Routes seem broken #16386

Closed ChristopherBass closed 3 months ago

ChristopherBass commented 4 months ago

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

13.3.2

Bug summary

Upgrading to 13.3.2 seems to break custom controller routes

Specifics

In 13.3.0, I had a RenderController with Route("auth"), and an action with HttpGet("signin") - navigating to /auth/signin triggered this action. In 13.3.2, the same controller and action result in the following exception - reverting the csproj files back to 13.3.0 resolves it.

InvalidOperationException: No UmbracoRouteValues feature was found in the HttpContext
Umbraco.Cms.Web.Common.Controllers.UmbracoPageController.get_UmbracoRouteValues()
Umbraco.Cms.Web.Common.Controllers.RenderController.OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, object state, bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextExceptionFilterAsync>g__Awaited|26_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, object state, bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ExceptionContextSealed context)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeNextResourceFilter()
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync()
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
Umbraco.Cms.Web.Common.Middleware.BasicAuthenticationMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+InterfaceMiddlewareBinder+<>c__DisplayClass2_0+<<CreateMiddleware>b__0>d.MoveNext()
Umbraco.Cms.Web.BackOffice.Middleware.BackOfficeExternalLoginProviderErrorMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+InterfaceMiddlewareBinder+<>c__DisplayClass2_0+<<CreateMiddleware>b__0>d.MoveNext()
UrlTracker.Middleware.UrlTrackerRedirectMiddleware.InvokeAsync(HttpContext context)
UrlTracker.Middleware.UrlTrackerClientErrorTrackingMiddleware.InvokeAsync(HttpContext context)
Swashbuckle.AspNetCore.SwaggerUI.SwaggerUIMiddleware.Invoke(HttpContext httpContext)
Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider)
Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Localization.RequestLocalizationMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
StackExchange.Profiling.MiniProfilerMiddleware.Invoke(HttpContext context) in MiniProfilerMiddleware.cs
Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+InterfaceMiddlewareBinder+<>c__DisplayClass2_0+<<CreateMiddleware>b__0>d.MoveNext()
Umbraco.Cms.Web.Common.Middleware.PreviewAuthenticationMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+InterfaceMiddlewareBinder+<>c__DisplayClass2_0+<<CreateMiddleware>b__0>d.MoveNext()
Umbraco.Cms.Web.Common.Middleware.UmbracoRequestLoggingMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+InterfaceMiddlewareBinder+<>c__DisplayClass2_0+<<CreateMiddleware>b__0>d.MoveNext()
SixLabors.ImageSharp.Web.Middleware.ImageSharpMiddleware.Invoke(HttpContext httpContext, bool retry)
NWebsec.AspNetCore.Middleware.Middleware.MiddlewareBase.Invoke(HttpContext context)
NWebsec.AspNetCore.Middleware.Middleware.MiddlewareBase.Invoke(HttpContext context)
Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddlewareImpl.Invoke(HttpContext context)

Steps to reproduce

Build a test RenderController with a Route() and an HttpGet action, run the site and try to access the url for that route in the standard /controller/action syntax. Then update your csproj for the project from 13.3.0 to 13.3.2.

Expected result / actual result

Previously, action was hit as normal, now throws the above-mentioned error.

github-actions[bot] commented 4 months ago

Hi there @ChristopherBass!

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:

nikolajlauridsen commented 4 months ago

Hi @ChristopherBass first of all, thanks for reporting the issue.

I've been looking into this, and I have a workaround for you, and a potential fix, however, to make sure that we're heading down the right path, I'd like to know more about the specific use case you're trying to solve.

I'm wondering why you need it to be a render controller and not an ordinary controller, since it's statically routed, or alternatively, why you're routing the render controller, normally, according to our docs, a render controller is used to control incoming requests for a specific document type, and therefore don't need to be statically routed, so I'm trying to understand why it is in this case πŸ˜„

If you drop this code into a file it should work again however πŸ˜„

using System.Reflection;
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, or the request has the ufprt-token, we don't need to do anything .
        // The ufprt-token is handled by the the <see cref="UmbracoRouteValueTransformer"/> and should not be discarded.
        var candidateCount = candidates.Count;
        if (candidateCount < 2 || string.IsNullOrEmpty(httpContext.Request.GetUfprt()) is false)
        {
            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;
            }

            // We have to ensure that none of the candidates is a render controller or surface controller
            // Normally these shouldn't be statically routed, however some people do it.
            // So we should probably be friendly and check for it.
            // Do not add this to V14.
            ControllerActionDescriptor? controllerDescriptor = routeEndpoint.Metadata.GetMetadata<ControllerActionDescriptor>();
            TypeInfo? controllerTypeInfo = controllerDescriptor?.ControllerTypeInfo;
            if (controllerTypeInfo is not null &&
                (controllerTypeInfo.IsType<RenderController>() || controllerTypeInfo.IsType<SurfaceController>()))
            {
                return;
            }

            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);
    }
}
ChristopherBass commented 4 months ago

Oh, the answer is probably "I'm new at Umbraco and don't know any better". I thought it was a bug since it worked before the upgrade and then didn't after, but maybe it working before was the real bug.

My goal is just to use a standard controller/action, no Umbraco page is needed. In the end it'll redirect the user elsewhere and that's the page that'll end up being shown. As long as I can get the umbraco context to query pages, and as long as the controller will take priority even if there's an Umbraco page present at that URL, I don't care what base type the controller is.

I can give a long-winded explanation of what I'm trying to do if you want, but it's not directly related to this ticket.

I found this documentation that looked like it was doing the right thing: https://docs.umbraco.com/umbraco-cms/reference/routing/custom-controllers but I think I misunderstood what it was for. Is there a simpler controller that'll still give me access to querying Umbraco pages?

ChristopherBass commented 4 months ago

Long-winded story is: A client has a custom authentication setup that they wrote (not oauth) - we ping their third-party URL (with a return address in the url) and sign in on that page, then it pings back to our return address - which then sends SOAP back to them to verify the login before signing you in.

I'd tried it what I think is the 'right' way, with a custom auth handler, but ran into this issue with actually creating the umbraco users

With no path forward, I pulled that code out and just created a standard AuthenticationController with a 'signin' and 'signout' action (and another set of two actions for coming back from the third-party auth). I use UmbracoContext .Content.GetAtRoot().OfType<> to get a configuration page with some settings in it, and that seemed to work. I was able to hit the /auth/signin and auth/signout urls and sign the user in via umbraco IMemberSignInManager. These four controller actions have no visual representation, they just redirect you to the right page.

Annoyingly, it seems like pages that are locked down ('Restrict Public Access') each have to specify an Umbraco page (not just an action) as the thing to do when you need to log in. So I also have to create two dummy pages (/auth/, /auth/signin) so that I can target that, and then have the route overridden by the controller. (Notably, even if I'd managed to get the auth handler right, I'd probably still have had to have these dummy pages since the 'login page' isn't an optional field.)

That part would probably be solved by having /auth/signin be a real template-page, but I think /auth/ still has to 'exist' in order for a page's primary url to be /auth/signin. And the other three actions would still be a controller+action not tied to a page (unless I make another three pages just for receiving those actions)

ChristopherBass commented 4 months ago

In any case, the point is if it was just something that shouldn't have been working all along (without overriding the composer), we can just close this and I'll try to figure out what controller I should be using instead.

AlexanderWagner82 commented 4 months ago

I'm getting the same error in SurfaceControllers when trying to access CurrentPage-property. All of a sudden my clients Umbraco Cloud-site crashed in strange ways because of the auto-upgrade so I had to revert the upgrade.

We've mapped a surface controller via this MapControllerRoute

image

And in the method in the SurfaceController the model is null and CurrentPage thows error

image

I've tried the MyEagerMatcherPolicy-code and that didn't resolve the problem :(

nikolajlauridsen commented 3 months ago

Thank you @ChristopherBass for the great explanation πŸ˜„

This does seem to be a bit of a square peg round hole type of scenario, the intended use of RenderController is to extend upon the Umbraco page dynamic routing essentially. For instance, you may want to enrich your page model with additional data from an external source. Given this these controllers shouldn't be statically routed, it kind of worked by accident before since our UmbracoRouteValueTranformer would always run.

Since your goal seems to be to query some content from your site, I would probably take the approach to instead use a completely stock ASP.Net controller, and inject IPublishedContentQuery to query your content instead, this will also resolve in less overhead.

I've made an example of it here:

using Microsoft.AspNetCore.Mvc;
using Umbraco.Cms.Core;
using Umbraco.Cms.Web.Common.PublishedModels;

namespace Umbraco.Cms.Web.UI;

public class CustomController : Controller
{
    private readonly IPublishedContentQuery _publishedContentQuery;

    public CustomController(IPublishedContentQuery publishedContentQuery)
    {
        _publishedContentQuery = publishedContentQuery;
    }

    public IActionResult Index()
    {
        // Current method
        SettingsPage? settings = _publishedContentQuery.ContentAtRoot().OfType<SettingsPage>().FirstOrDefault();
        string? property = settings?.SomeProperty;

        // Another approach without enumeration.
       // The GUID is the key of the content, found in the info panel in the backoffice
        if (_publishedContentQuery.Content(Guid.Parse("d60b9cbe-e9d7-4440-baeb-8ef2e4e4bb36")) is not SettingsPage settingsPage)
        {
            throw new InvalidOperationException("Wrong page type");
        }

        string? settingsProperty = settingsPage.SomeProperty;

        return View();
    }
}

I'm sorry to hear about your troubles with login, I'm afraid I can't help much with that though since it's a bit out of scope, but I'd like to encourage you to maybe check out our Discord server, there seem to be a bit more activity there these days https://community.umbraco.com/get-involved/community-discord-server/

nikolajlauridsen commented 3 months ago

Hey @AlexanderWagner82 thank you for making us aware of this, it does however seem like a different issue than this one, could I get you to create a separate issue for it? I would also like a bit more information so I can reproduce this properly and make a fix.

Primarily: Normally a surface controller is auto-routed and requested with BeginUmbracoForm this doesn't be the case for you? How are you requesting the endpoint, as in, how is category URL parameter provided, and why does it need to be this way?

Thank you πŸ˜„

AlexanderWagner82 commented 3 months ago

Hi @nikolajlauridsen

Seems that there's another issue with a similar error here: https://github.com/umbraco/Umbraco-CMS/issues/16426

Do you still want me to create a new issue :)?

The category url parameter is passed as a urlsegment, like so: https://site.com/category-articles/Research/

As to why; good question. We just took over this client so it's not my code in the beginning πŸ˜… But I can see that the page model that's passed to that method is of the 404 page, so it feels a bit buggy as it is right now any way.

EDIT: I Rewrote the code so it start from the StartPage since it was buggy to begin with so I solved the problem for my sake :).

image

nikolajlauridsen commented 3 months ago

Yes, that would be great thanks, they're probably related in that they're both routing issues, but the fix is likely different, so I'd prefer to have them separately, also so they can be tested independently πŸ˜„

Ohhhh I think I might understand it a bit better now, I assumed it was a post, since that's what the SurfaceController is normally used for, now I'm not totally in the specific use case here, but from where I'm sitting now it seems like this would be a textbook case for a virtual page controller, it seems like the goal is to crate a virtual page based on the passed in category? https://docs.umbraco.com/umbraco-cms/reference/routing/custom-routes#custom-route-with-ivirtualpagecontroller

AlexanderWagner82 commented 3 months ago

Ahaa! You're probably right. This is an old Umbaco 8-site that I upgraded to 13 and I haven't really refactored the code since it worked prior to 13.2.2 πŸ˜…

nikolajlauridsen commented 3 months ago

Ahh perfectly understandable, and kudos for getting up to V13 we love to see itπŸ’ͺ.

I think for now I'll leave this as is then, but I'll have a chat with the team about what we should do because it is technically behaviorally breaking, but the fact that it worked before seems kind of like a bug in actuality #16015 fixed in #16218 so it's always a bit of a delicate thing which behaviours should be preserved and which were unintentional.

arknu commented 3 months ago

I'm also suddenly seeing previously working RenderControllers failing after updating to 13.3.2. This seems a pretty big breaking change.

For instance, this controller was previously working, but is now broken:

public class JobAgentPageController(ILogger<RenderController> logger, ICompositeViewEngine compositeViewEngine, IUmbracoContextAccessor umbracoContextAccessor, JobAgentService agentService) : RenderController(logger, compositeViewEngine, umbracoContextAccessor)
{
    [HttpGet("jobagent/{token?}", Name = "jobagent_edit")]
    public async Task<IActionResult> JobAgentPage(ContentModel<JobAgentPage> contentModel, string? token)
    {
        JobAgent? jobAgent = null;
        if (!string.IsNullOrEmpty(token))
        {
            jobAgent = await agentService.GetAgentByToken(token);
        }

        var model = new JobAgentPageModel(contentModel.Content)
        {
            Token = token,
            Agent = jobAgent
        };

        return CurrentTemplate(model);
    }

}

public class JobAgentPageModel(JobAgentPage content) : ContentModel<JobAgentPage>(content)
{
    public string? Token { get; set; }

    public JobAgent? Agent { get; set; }
}

I refactored it to the following, which is working:

public class JobAgentPageController(IPublishedContentQuery publishedContentQuery, JobAgentService agentService) : Controller
{
    [HttpGet("jobagent/{token?}", Name = "jobagent_edit")]
    public async Task<IActionResult> JobAgentPage(string? token)
    {
        JobAgent? jobAgent = null;
        if (!string.IsNullOrEmpty(token))
        {
            jobAgent = await agentService.GetAgentByToken(token);
        }

        var model = new JobAgentPageModel(publishedContentQuery.ContentAtRoot().OfType<Home>().DescendantsOrSelf<JobAgentPage>().FirstOrDefault())
        {
            Token = token,
            Agent = jobAgent
        };

        return View("JobAgentPage", model);
    }

}
ChristopherBass commented 3 months ago

@nikolajlauridsen nikolajlauridsen Thank you! That looks like a good path forward for me. Good luck for the team with the related issues! Sounds like other people are having similar things, so I won't single-handedly close it, but I myself should be all set.

nikolajlauridsen commented 3 months ago

Hey @arknu, as I mentioned above it seems like what you're looking for is probably a virtual pager controller.

What you had before works because of a side effect of a bug, however, it doesn't seem that friendly to change this behaviour in a patch, so I've made a fix here #16540 which will be in the next V13 release, however, bear in mind that this will not be ported to V14 since it's a bug that it worked, to begin with πŸ˜„

I have also updated the workaround above: https://github.com/umbraco/Umbraco-CMS/issues/16386#issuecomment-2129073596 to cover both RenderController and SurfaceController if you'd rather do that than wait for the release πŸ˜„

nikolajlauridsen commented 3 months ago

I'll go ahead an close this since it has been fixed in #16540