Layout not rendered when using await IncludeAsync using .NET Core 3

[matthewwren]

Describe the bug Layout not being rendered when using await IncludeAsync using .NET Core 3.

To Reproduce The following code does not render the layout.

@inherits RazorLight.TemplatePage<MyModel>
@{
    Layout = "_Layout.cshtml";
}
<p>
    @{

        await IncludeAsync("_LetterHeader.cshtml", Model);
    }

</p>

Note that MyModel implements an interface that is accepted by _LetterHeader.cshtml, but I assume that this is not the issue as the letter header renders fine.

The following does render the layout.

@inherits RazorLight.TemplatePage<MyModel>
@{
    Layout = "_Layout.cshtml";
}
<p>Hello</p>

Expected behavior The layout should be rendered when using await IncludeAsync.

Information (please complete the following information):

• OS: Windows 10
• Platform: .NET Core 3.0
• RazorLight version: current branch as at 28/11/2019
• Visual Studio version: Visual Studio Community 2019 16.3.4

Additional context None

[DjTrilogic]

I'm facing the same problem...

[HamiltonManalo]

I tested it against master and also found the same issue.

[matthewwren]

I have found the issue: the RazorPageproperty on TemplateRendereris being overwritten by the call to _engineHandler.RenderIncludedTemplateAsync:

        private async Task RenderPageCoreAsync(ITemplatePage page, PageContext context)
        {
            page.PageContext = context;
            page.IncludeFunc = async (key, model) =>
            {
                ITemplatePage template = await _engineHandler.CompileTemplateAsync(key);
                await _engineHandler.RenderIncludedTemplateAsync(template, model, context.Writer, context.ViewBag, this); //This call overwrites the current RazorPage property
            };

            //_pageActivator.Activate(page, context);

            await page.ExecuteAsync().ConfigureAwait(false);
        }

A quick solution I have used is to restore the original RazorPageproperty once the call is complete:

        private async Task RenderPageCoreAsync(ITemplatePage page, PageContext context)
        {
            page.PageContext = context;
            page.IncludeFunc = async (key, model) =>
            {
                var templatePage = RazorPage;
                ITemplatePage template = await _engineHandler.CompileTemplateAsync(key);
                await _engineHandler.RenderIncludedTemplateAsync(template, model, context.Writer, context.ViewBag, this);
                RazorPage = templatePage;
            };

            //_pageActivator.Activate(page, context);

            await page.ExecuteAsync().ConfigureAwait(false);
        }

This is obviously just a hack though. It feels like the proper solution might be not to execute the template renderer in the following way:

        public async Task RenderIncludedTemplateAsync<T>(
            ITemplatePage templatePage,
            T model,
            TextWriter textWriter,
            ExpandoObject viewBag,
            TemplateRenderer templateRenderer)
        {
            SetModelContext(templatePage, textWriter, model, viewBag);

            templateRenderer.RazorPage = templatePage;
            await templateRenderer.RenderAsync().ConfigureAwait(false);
        }

But more like the following:

        public async Task RenderIncludedTemplateAsync<T>(
            ITemplatePage templatePage,
            T model,
            TextWriter textWriter,
            ExpandoObject viewBag,
            TemplateRenderer templateRenderer)
        {
            SetModelContext(templatePage, textWriter, model, viewBag);
            await templateRenderer.RenderAsync(templatePage).ConfigureAwait(false);
        }

In other words if the template renderer is to be reused, then the pages need to be passed by parameter rather than passed on a public property. But I am not at all familiar with the code, so I will defer to you on the final decision.

[HamiltonManalo]

Any chance a fix for this could get into the next release?

[toddams]

@matthewwren Could you please send a PR with your findings? I would appreciate your input :)

[jzabroski]

This is likely still broken in beta3.

[matthewwren]

@matthewwren Could you please send a PR with your findings? I would appreciate your input :)

@toddams I will try to get around to doing this for you, I need to find some time to look a bit more in detail at the code to try and come up with a better approach than the hack I mentioned earlier in this thread.

[ronnypmuliawan]

Hi all, the Layout is also not rendering on ASP.NET Core 2.2 project.

@jzabroski any timeline to fix this issue or workaround that I can implement?

Information (please complete the following information):

OS: Windows 10 Platform: ASP.NET Core 2.2 RazorLight version: 2.0.0-beta.4 Visual Studio version: Visual Studio Pro 2019 16.4.0

[jzabroski]

@ronnypmuliawan , @matthewwren explained a workaround already. https://github.com/toddams/RazorLight/issues/287#issuecomment-559598131

We were waiting on Matthew to produce a PR with his findings, so that we could either investigate further or take any patch he proposes. First steps are to write a unit test to reproduce the issue.

[ronnypmuliawan]

@jzabroski Got it. Was looking for a cleaner workaround than using the changes suggested by Matthew.

I guess I'll just create a fork until you have figured out a proper fix.

Thanks for taking over the maintenance of the library.

[weyert]

I have a similar issue with @section stopped working with the latest beta 4 of Razorlight in .net core 3. I will see if I can make unit tests to cover these issues.

[jzabroski]

@weyert Did it work in beta3?

[HamiltonManalo]

I was able to use @matthewwren solution effectively. I'm trying to put together some unit tests for the issue and i'm getting odd results but I'll try to make a PR this weekend.

[matthewwren]

@HamiltonManalo my first solution is really just a hack. What I go on to propose is for the templateRenderer.RenderAsync method to accept an ITemplatePageas a parameter, since at the moment the parameter is 'passed' by setting the RazorPageproperty on the templateRenderer which is very hacky and is really where the problem in the code lies. This is not a complete solution though just a high level suggestion, and it really needs somebody's time to rethink the design of the renderer class. I will have a look at it at some point, but I am very busy atm so it's not going to be for a while.

[matthewwren]

Ok, I have had a look at this just now, and as a simple experiment made the TemplateRendererstateless, so it looks like this:

using RazorLight.Internal;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text.Encodings.Web;
using System.Threading.Tasks;

namespace RazorLight
{
    public class TemplateRenderer
    {
        private readonly HtmlEncoder _htmlEncoder;
        private readonly IEngineHandler _engineHandler;
        private readonly IViewBufferScope _bufferScope;

        public TemplateRenderer(
            IEngineHandler engineHandler,
            HtmlEncoder htmlEncoder,
            IViewBufferScope bufferScope)
        {
            _engineHandler = engineHandler ?? throw new ArgumentNullException(nameof(engineHandler));
            _bufferScope = bufferScope ?? throw new ArgumentNullException(nameof(bufferScope));
            _htmlEncoder = htmlEncoder ?? throw new ArgumentNullException(nameof(htmlEncoder));
        }

        ///// <summary>
        ///// Gets the sequence of _ViewStart <see cref="ITemplatePage"/> instances that are executed by this view.
        ///// </summary>
        //public IReadOnlyList<ITemplatePage> ViewStartPages { get; }

        /// <inheritdoc />
        public virtual async Task RenderAsync(ITemplatePage razorPage)
        {
            var context = razorPage.PageContext;

            var bodyWriter = await RenderPageAsync(razorPage, context, invokeViewStarts: false).ConfigureAwait(false);
            await RenderLayoutAsync(context, bodyWriter, razorPage).ConfigureAwait(false);
        }

        private async Task<ViewBufferTextWriter> RenderPageAsync(
            ITemplatePage page,
            PageContext context,
            bool invokeViewStarts)
        {
            var writer = context.Writer as ViewBufferTextWriter;
            if (writer == null)
            {
                Debug.Assert(_bufferScope != null);

                // If we get here, this is likely the top-level page (not a partial) - this means
                // that context.Writer is wrapping the output stream. We need to buffer, so create a buffered writer.
                var buffer = new ViewBuffer(_bufferScope, page.Key, ViewBuffer.ViewPageSize);
                writer = new ViewBufferTextWriter(buffer, context.Writer.Encoding, _htmlEncoder, context.Writer);
            }
            else
            {
                // This means we're writing something like a partial, where the output needs to be buffered.
                // Create a new buffer, but without the ability to flush.
                var buffer = new ViewBuffer(_bufferScope, page.Key, ViewBuffer.ViewPageSize);
                writer = new ViewBufferTextWriter(buffer, context.Writer.Encoding);
            }

            // The writer for the body is passed through the PageContext, allowing things like HtmlHelpers
            // and ViewComponents to reference it.
            var oldWriter = context.Writer;
            var oldFilePath = context.ExecutingPageKey;

            context.Writer = writer;
            context.ExecutingPageKey = page.Key;

            try
            {
                //Apply engine-global callbacks
                ExecutePageCallbacks(page, _engineHandler.Options.PreRenderCallbacks.ToList());

                if (invokeViewStarts)
                {
                    // Execute view starts using the same context + writer as the page to render.
                    await RenderViewStartsAsync(context).ConfigureAwait(false);
                }

                await RenderPageCoreAsync(page, context).ConfigureAwait(false);
                return writer;
            }
            finally
            {
                context.Writer = oldWriter;
                context.ExecutingPageKey = oldFilePath;
            }
        }

        private async Task RenderPageCoreAsync(ITemplatePage page, PageContext context)
        {
            page.PageContext = context;
            page.IncludeFunc = async (key, model) =>
            {
                ITemplatePage template = await _engineHandler.CompileTemplateAsync(key);
                await _engineHandler.RenderIncludedTemplateAsync(template, model, context.Writer, context.ViewBag, this);
            };

            //_pageActivator.Activate(page, context);

            await page.ExecuteAsync().ConfigureAwait(false);
        }

        private Task RenderViewStartsAsync(PageContext context)
        {
            return Task.FromResult(0);

            //string layout = null;
            //string oldPageKey = context.ExecutingPageKey;
            //try
            //{
            //    for (var i = 0; i < ViewStartPages.Count; i++)
            //    {
            //        var viewStart = ViewStartPages[i];
            //        context.ExecutingPageKey = viewStart.Key;

            //        // If non-null, copy the layout value from the previous view start to the current. Otherwise leave
            //        // Layout default alone.
            //        if (layout != null)
            //        {
            //            viewStart.Layout = layout;
            //        }

            //        await RenderPageCoreAsync(viewStart, context);

            //        // Pass correct absolute path to next layout or the entry page if this view start set Layout to a
            //        // relative path.
            //        layout = _viewEngine.GetAbsolutePath(viewStart.Key, viewStart.Layout);
            //    }
            //}
            //finally
            //{
            //    context.ExecutingPageKey = oldPageKey;
            //}

            //// If non-null, copy the layout value from the view start page(s) to the entry page.
            //if (layout != null)
            //{
            //    RazorPage.Layout = layout;
            //}
        }

        private async Task RenderLayoutAsync(
            PageContext context,
            ViewBufferTextWriter bodyWriter,
            ITemplatePage razorPage)
        {
            // A layout page can specify another layout page. We'll need to continue
            // looking for layout pages until they're no longer specified.
            var previousPage = razorPage;
            var renderedLayouts = new List<ITemplatePage>();

            // This loop will execute Layout pages from the inside to the outside. With each
            // iteration, bodyWriter is replaced with the aggregate of all the "body" content
            // (including the layout page we just rendered).
            while (!string.IsNullOrEmpty(previousPage.Layout))
            {
                if (!bodyWriter.IsBuffering)
                {
                    // Once a call to RazorPage.FlushAsync is made, we can no longer render Layout pages - content has
                    // already been written to the client and the layout content would be appended rather than surround
                    // the body content. Throwing this exception wouldn't return a 500 (since content has already been
                    // written), but a diagnostic component should be able to capture it.

                    throw new InvalidOperationException("Layout can not be rendered");
                }

                ITemplatePage layoutPage = await _engineHandler.CompileTemplateAsync(previousPage.Layout).ConfigureAwait(false);
                layoutPage.SetModel(context.Model);

                if (renderedLayouts.Count > 0 &&
                    renderedLayouts.Any(l => string.Equals(l.Key, layoutPage.Key, StringComparison.Ordinal)))
                {
                    // If the layout has been previously rendered as part of this view, we're potentially in a layout
                    // rendering cycle.
                    throw new InvalidOperationException($"Layout {layoutPage.Key} has circular reference");
                }

                // Notify the previous page that any writes that are performed on it are part of sections being written
                // in the layout.
                previousPage.IsLayoutBeingRendered = true;
                layoutPage.PreviousSectionWriters = previousPage.SectionWriters;
                layoutPage.BodyContent = bodyWriter.Buffer;
                bodyWriter = await RenderPageAsync(layoutPage, context, invokeViewStarts: false).ConfigureAwait(false);

                renderedLayouts.Add(layoutPage);
                previousPage = layoutPage;
            }

            // Now we've reached and rendered the outer-most layout page. Nothing left to execute.
            // Ensure all defined sections were rendered or RenderBody was invoked for page without defined sections.
            foreach (var layoutPage in renderedLayouts)
            {
                layoutPage.EnsureRenderedBodyOrSections();
            }

            if (bodyWriter.IsBuffering)
            {
                // If IsBuffering - then we've got a bunch of content in the view buffer. How to best deal with it
                // really depends on whether or not we're writing directly to the output or if we're writing to
                // another buffer.
                var viewBufferTextWriter = context.Writer as ViewBufferTextWriter;
                if (viewBufferTextWriter == null || !viewBufferTextWriter.IsBuffering)
                {
                    // This means we're writing to a 'real' writer, probably to the actual output stream.
                    // We're using PagedBufferedTextWriter here to 'smooth' synchronous writes of IHtmlContent values.
                    using (var writer = _bufferScope.CreateWriter(context.Writer))
                    {
                        await bodyWriter.Buffer.WriteToAsync(writer, _htmlEncoder).ConfigureAwait(false);
                    }
                }
                else
                {
                    // This means we're writing to another buffer. Use MoveTo to combine them.
                    bodyWriter.Buffer.MoveTo(viewBufferTextWriter.Buffer);
                }
            }
        }

        private void ExecutePageCallbacks(ITemplatePage page, IList<Action<ITemplatePage>> callbacks)
        {
            if (callbacks?.Count > 0)
            {
                foreach (var callback in callbacks)
                {
                    try
                    {
                        callback(page);
                    }
                    catch (Exception)
                    {
                        //Ignore
                    }
                }
            }
        }
    }
}

Then on the EngineHandler:

        public async Task RenderIncludedTemplateAsync<T>(
            ITemplatePage templatePage,
            T model,
            TextWriter textWriter,
            ExpandoObject viewBag,
            TemplateRenderer templateRenderer)
        {
            SetModelContext(templatePage, textWriter, model, viewBag);
            await templateRenderer.RenderAsync(templatePage).ConfigureAwait(false);
        }

Quickly tested this and it works. Do you want to give that a go @HamiltonManalo ?

[jzabroski]

If you guys patch this with tests I can ship a beta5 asap. Just saying. I am on vacation Feb1-9 though and will be closer to James Harden raining three pointers than I will be my computer.

[jzabroski]

I checked your changes and I guess the only problem is they do break the public API especially EngineHandler. I guess it's not such a big deal and shouldn't affect anyone directly except for people using string rendering (I think), but worth noting.

[weyert]

I have to admit I am not sure about it. I have been using this unofficial build because there were so many changes in master the nuget package was quite outdated. I will have a quick test with the mention version whether it works with that.

A quick look I noticed we might not actually testing sections in our unit tests or include async so I want to add tests for those if they are indeed missing.

Weyert

Sent from my iPhone

On 8 Jan 2020, at 17:34, John Zabroski notifications@github.com wrote:

 @weyert Did it work in beta3?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[matthewwren]

Please also note that my code posted just now might be based on outdated files. The only real changes are the removal of the property RazorPage and therefore also the constructor parameter and member variable, and then changing the signature of RenderAsync(ITemplatePage razorPage), and updating code accordingly such that calling code passes the page by parameter and that the page is passed by parameter to private methods. As I said before, this is only a suggestion and sorry I haven't submitted a PR properly, but hope what I have done is of some help.

[jzabroski]

The most likely reason sections don't work on .NET Core 3 is this code:

                   //In RazorLanguageVersion > 3.0 (at least netcore 3.0) the directives are registed out of the box.
                   if (!RazorLanguageVersion.TryParse("3.0", out var razorLanguageVersion) 
                       || configuration.LanguageVersion.CompareTo(razorLanguageVersion) < 0)
                   {
                       NamespaceDirective.Register(builder);
                       FunctionsDirective.Register(builder);
                       InheritsDirective.Register(builder);
                       SectionDirective.Register(builder);
                   }

Microsoft's test coverage for this feature is here: https://github.com/dotnet/aspnetcore-tooling/blob/72b939d85573202c4406cd1ec1c39e97f870cb08/src/Razor/test/RazorLanguage.Test/Extensions/SectionDirectivePassTest.cs

[jzabroski]

Please also note that my code posted just now might be based on outdated files. The only real changes are the removal of the property RazorPage and therefore also the constructor parameter and member variable, and then changing the signature of RenderAsync(ITemplatePage razorPage), and updating code accordingly such that calling code passes the page by parameter and that the page is passed by parameter to private methods. As I said before, this is only a suggestion and sorry I haven't submitted a PR properly, but hope what I have done is of some help.

If you've never done a PR before and want help learning how, just message me.

[matthewwren]

@jzabroski if you could explain that would be great.

[jzabroski]

Coordinate with me via email johnzabroski@gmail.com on a time and I'll walk you through how. You'll need a computer where you can install software and where ssh port 22 is not blocked, so best to do at home if your corporate IT blocks 22 and/or disallows installing some software.

[matthewwren]

OK so, I will have a call with @jzabroski on Monday 13th January and submit a PR to fix this.

[weyert]

I have created a PR (#309) with some common test cases how I personally use Razorlight, these might be a good regression test to ensure Razorlight works as expected. I have tried to apply the patch listed above but it still doesn't work for me.

[weyert]

@matthewwren Have a look at the following Egghead video which explains how you can make a PR: https://egghead.io/courses/how-to-contribute-to-an-open-source-project-on-github (it's from www.makeapullrequest.com)

[matthewwren]

I believe I have successfully created a Pull Request. Please let me know if that is how you wanted it done. thanks.

[jzabroski]

I have created a PR (#309) with some common test cases how I personally use Razorlight, these might be a good regression test to ensure Razorlight works as expected. I have tried to apply the patch listed above but it still doesn't work for me.

Hi @weyert , The patch above provided by @matthewwren is only for IncludeAsync. So I'm not sure what you mean when you say "it still doesn't work for me". If you're referring to @section syntax, then yes, that would most likely be a separate issue.

[hellokids86]

Is there an estimate when this will be patched?

[chancie86]

329 raised to fix this. Would appreciate a new release with this in!