From b1ce8d96921b9004490b5ebf9201ac6e279e4f2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Standa=20Luke=C5=A1?= Date: Mon, 13 Feb 2023 15:28:18 +0100 Subject: [PATCH] Move Head/BodyResourceLinks insertion to compile-time We used to insert the ResourceLink at the end of HtmlGenericControl Render, when head or body element was rendered. After this change, the component is inserted compile time into the proper head or body element. This solves the following problems * when body contained only literals, LiteralOptimizationVisitor would collapse them into a `text: ...` binding, which removed all the resources inside * when is accidentaly used in the page, resources are rendered on some random place * when no body or head tag is found, it would crash Now I'm not sure we want to support this, the error message is reasonable (suggests adding head and body elements) and this "smartness" might now cause problems in markup controls declared as views The change contains large number of test edits for the following reason * new component is inserted into all trees, changing unique ids * BodyResourceLink and HeadResourceLinks are added to the pages, so I needed to ignore them when the control tree is being checked. --- .../Directives/BaseTypeDirectiveCompiler.cs | 14 +- .../MarkupDirectiveCompilerPipeline.cs | 7 +- .../Compilation/ResourceLinksVisitor.cs | 122 ++++++++++++++++++ .../ViewCompiler/ViewCompilingVisitor.cs | 10 +- .../Framework/Controls/HtmlGenericControl.cs | 6 - .../DotVVMServiceCollectionExtensions.cs | 1 + .../Framework/Testing/TestHttpRequest.cs | 2 +- src/Framework/Testing/ControlTestHelper.cs | 10 ++ src/Framework/Testing/DotvvmTestHelper.cs | 5 +- src/Tests/ControlTests/ResourceLinksTests.cs | 73 +++++++++++ .../AutoUITests.FormInRepeater.html | 8 +- .../CompositeControlTests.MenuRepeater.html | 12 +- ...rarchyRepeaterControlWithGeneratedIds.html | 18 +-- ...rappedRepeaterControlWithGeneratedIds.html | 6 +- ...erTests.CommandInMarkupControl-client.html | 10 +- ...erTests.CommandInMarkupControl-server.html | 10 +- .../RepeaterTests.IdGeneration.html | 10 +- ...erTests.IdGeneration_CreatedAtRuntime.html | 10 +- ...sourceLinksTests.MultipleBodyElements.html | 48 +++++++ ...ResourceLinksTests.NoHeadBodyElements.html | 44 +++++++ .../ResourceLinksTests.OnlyLiteralInBody.html | 40 ++++++ .../ControlTree/ServerSideStyleTests.cs | 1 + src/Tests/Runtime/DefaultViewCompilerTests.cs | 116 ++++++++--------- src/Tests/Runtime/DotvvmControlTestBase.cs | 16 ++- 24 files changed, 476 insertions(+), 123 deletions(-) create mode 100644 src/Framework/Framework/Compilation/ResourceLinksVisitor.cs create mode 100644 src/Tests/ControlTests/ResourceLinksTests.cs create mode 100644 src/Tests/ControlTests/testoutputs/ResourceLinksTests.MultipleBodyElements.html create mode 100644 src/Tests/ControlTests/testoutputs/ResourceLinksTests.NoHeadBodyElements.html create mode 100644 src/Tests/ControlTests/testoutputs/ResourceLinksTests.OnlyLiteralInBody.html diff --git a/src/Framework/Framework/Compilation/Directives/BaseTypeDirectiveCompiler.cs b/src/Framework/Framework/Compilation/Directives/BaseTypeDirectiveCompiler.cs index 194d4867e2..8f7bb37212 100644 --- a/src/Framework/Framework/Compilation/Directives/BaseTypeDirectiveCompiler.cs +++ b/src/Framework/Framework/Compilation/Directives/BaseTypeDirectiveCompiler.cs @@ -13,6 +13,7 @@ using System.Collections.Immutable; using System.Security.Cryptography; using System.Text; +using DotVVM.Framework.Configuration; namespace DotVVM.Framework.Compilation.Directives { @@ -22,15 +23,17 @@ public class BaseTypeDirectiveCompiler : DirectiveCompiler imports; + private readonly DotvvmConfiguration configuration; public override string DirectiveName => ParserConstants.BaseTypeDirective; public BaseTypeDirectiveCompiler( - IReadOnlyDictionary> directiveNodesByName, IAbstractTreeBuilder treeBuilder, string fileName, ImmutableList imports) + IReadOnlyDictionary> directiveNodesByName, IAbstractTreeBuilder treeBuilder, string fileName, ImmutableList imports, DotvvmConfiguration configuration) : base(directiveNodesByName, treeBuilder) { this.fileName = fileName; this.imports = imports; + this.configuration = configuration; } protected override IAbstractBaseTypeDirective Resolve(DothtmlDirectiveNode directiveNode) @@ -113,7 +116,14 @@ IEnumerable propertyDirectives /// private ITypeDescriptor GetDefaultWrapperType() { - if (fileName.EndsWith(".dotcontrol", StringComparison.Ordinal)) + var isControl = + fileName.EndsWith(".dotcontrol", StringComparison.Ordinal) || + configuration.Markup.Controls.Any(c => fileName.Equals(c.Src, StringComparison.OrdinalIgnoreCase)) || + this.DirectiveNodesByName.ContainsKey("property") || + this.DirectiveNodesByName.ContainsKey("wrapperTag") || + this.DirectiveNodesByName.ContainsKey("noWrapperTag"); + + if (isControl) { return new ResolvedTypeDescriptor(typeof(DotvvmMarkupControl)); } diff --git a/src/Framework/Framework/Compilation/Directives/MarkupDirectiveCompilerPipeline.cs b/src/Framework/Framework/Compilation/Directives/MarkupDirectiveCompilerPipeline.cs index 9c854e3f9b..2b9509485d 100644 --- a/src/Framework/Framework/Compilation/Directives/MarkupDirectiveCompilerPipeline.cs +++ b/src/Framework/Framework/Compilation/Directives/MarkupDirectiveCompilerPipeline.cs @@ -6,6 +6,7 @@ using System; using System.Linq; using System.Collections.Generic; +using DotVVM.Framework.Configuration; namespace DotVVM.Framework.Compilation.Directives { @@ -14,12 +15,14 @@ public class MarkupDirectiveCompilerPipeline : IMarkupDirectiveCompilerPipeline private readonly IAbstractTreeBuilder treeBuilder; private readonly IControlBuilderFactory controlBuilderFactory; private readonly DotvvmResourceRepository resourceRepository; + private readonly DotvvmConfiguration configuration; - public MarkupDirectiveCompilerPipeline(IAbstractTreeBuilder treeBuilder, IControlBuilderFactory controlBuilderFactory, DotvvmResourceRepository resourceRepository) + public MarkupDirectiveCompilerPipeline(IAbstractTreeBuilder treeBuilder, IControlBuilderFactory controlBuilderFactory, DotvvmResourceRepository resourceRepository, DotvvmConfiguration configuration) { this.treeBuilder = treeBuilder; this.controlBuilderFactory = controlBuilderFactory; this.resourceRepository = resourceRepository; + this.configuration = configuration; } public MarkupPageMetadata Compile(DothtmlRootNode dothtmlRoot, string fileName) @@ -50,7 +53,7 @@ public MarkupPageMetadata Compile(DothtmlRootNode dothtmlRoot, string fileName) var injectedServicesResult = serviceCompiler.Compile(); resolvedDirectives.AddIfAny(serviceCompiler.DirectiveName, injectedServicesResult.Directives); - var baseTypeCompiler = new BaseTypeDirectiveCompiler(directivesByName, treeBuilder, fileName, imports); + var baseTypeCompiler = new BaseTypeDirectiveCompiler(directivesByName, treeBuilder, fileName, imports, configuration); var baseTypeResult = baseTypeCompiler.Compile(); var baseType = baseTypeResult.Artefact; resolvedDirectives.AddIfAny(baseTypeCompiler.DirectiveName, baseTypeResult.Directives); diff --git a/src/Framework/Framework/Compilation/ResourceLinksVisitor.cs b/src/Framework/Framework/Compilation/ResourceLinksVisitor.cs new file mode 100644 index 0000000000..ef3f9977e2 --- /dev/null +++ b/src/Framework/Framework/Compilation/ResourceLinksVisitor.cs @@ -0,0 +1,122 @@ +using System; +using System.Collections.Generic; +using System.Text; +using DotVVM.Framework.Compilation.ControlTree.Resolved; +using DotVVM.Framework.Controls; +using DotVVM.Framework.Utils; +using DotVVM.Framework.Binding; +using DotVVM.Framework.Binding.Properties; +using DotVVM.Framework.Binding.Expressions; +using System.Collections.Immutable; +using DotVVM.Framework.Compilation.ControlTree; +using System.Linq; + +namespace DotVVM.Framework.Compilation +{ + /// Inserts and into head or body element + public class ResourceLinksVisitor : ResolvedControlTreeVisitor + { + private readonly IControlResolver controlResolver; + + private bool headLinksFound = false; + private bool bodyLinksFound = false; + + public ResourceLinksVisitor(IControlResolver controlResolver) + { + this.controlResolver = controlResolver; + } + + + public override void VisitPropertyTemplate(ResolvedPropertyTemplate propertyTemplate) + { + // skip + } + + public override void VisitControl(ResolvedControl control) + { + if (typeof(HeadResourceLinks).IsAssignableFrom(control.Metadata.Type)) + { + headLinksFound = true; + } + else if (typeof(BodyResourceLinks).IsAssignableFrom(control.Metadata.Type)) + { + bodyLinksFound = true; + } + else + { + base.VisitControl(control); + } + } + + private ResolvedControl? TryFindElement(ResolvedControl control, string tagName) + { + // BFS to find the outermost element + // in case somebody mistypes element into a , we don't want to insert resources into it + var queue = new Queue(); + queue.Enqueue(control); + + while (queue.Count > 0) + { + var current = queue.Dequeue(); + if (current.Metadata.Type == typeof(HtmlGenericControl) && current.ConstructorParameters?[0] is string controlTagName && tagName.Equals(controlTagName, StringComparison.OrdinalIgnoreCase)) + { + return current; + } + foreach (var child in current.Content) + { + queue.Enqueue(child); + } + } + return null; + } + + private ResolvedControl CreateLinksControl(Type type, DataContextStack dataContext) + { + var metadata = controlResolver.ResolveControl(new ResolvedTypeDescriptor(type)); + var control = new ResolvedControl((ControlResolverMetadata)metadata, null, dataContext); + control.SetProperty(new ResolvedPropertyValue(Internal.UniqueIDProperty, "cAuto" + type.Name)); + return control; + } + + public override void VisitView(ResolvedTreeRoot view) + { + if (view.MasterPage is {}) + { + // if there is a masterpage, this visitor has already inserted the links into it + return; + } + if (!typeof(Controls.Infrastructure.DotvvmView).IsAssignableFrom(view.ControlBuilderDescriptor.ControlType)) + { + // markup controls + return; + } + if (!headLinksFound) + { + var headElement = TryFindElement(view, "head"); + if (headElement is {}) + { + headElement.Content.Add(CreateLinksControl(typeof(HeadResourceLinks), headElement.DataContextTypeStack)); + } + else + { + // no head element found, and no masterpage -> insert it at the document start + view.Content.Insert(0, CreateLinksControl(typeof(HeadResourceLinks), view.DataContextTypeStack)); + } + } + if (!bodyLinksFound) + { + var bodyElement = TryFindElement(view, "body"); + if (bodyElement is {}) + { + bodyElement.Content.Add(CreateLinksControl(typeof(BodyResourceLinks), bodyElement.DataContextTypeStack)); + } + else + { + // no body element found, and no masterpage -> insert it at the document end + view.Content.Add(CreateLinksControl(typeof(BodyResourceLinks), view.DataContextTypeStack)); + } + } + base.VisitView(view); + } + } +} diff --git a/src/Framework/Framework/Compilation/ViewCompiler/ViewCompilingVisitor.cs b/src/Framework/Framework/Compilation/ViewCompiler/ViewCompilingVisitor.cs index 5619925eb6..21a4aee29b 100644 --- a/src/Framework/Framework/Compilation/ViewCompiler/ViewCompilingVisitor.cs +++ b/src/Framework/Framework/Compilation/ViewCompiler/ViewCompilingVisitor.cs @@ -20,6 +20,7 @@ public class ViewCompilingVisitor : ResolvedControlTreeVisitor protected int currentTemplateIndex; protected string? controlName; + int controlIdIndex = 0; public Func BuildCompiledView { get; set; } @@ -247,8 +248,13 @@ protected string CreateControl(ResolvedControl control) // RawLiterals don't need these helper properties unless in root if (control.Metadata.Type != typeof(RawLiteral) || control.Parent is ResolvedTreeRoot) { - // set unique id - emitter.EmitSetDotvvmProperty(name, Internal.UniqueIDProperty, name); + if (!control.Properties.ContainsKey(Internal.UniqueIDProperty)) + { + var uniqueId = "c" + controlIdIndex; + controlIdIndex++; + // set unique id + emitter.EmitSetDotvvmProperty(name, Internal.UniqueIDProperty, uniqueId); + } if (control.DothtmlNode != null && control.DothtmlNode.Tokens.Count > 0) { diff --git a/src/Framework/Framework/Controls/HtmlGenericControl.cs b/src/Framework/Framework/Controls/HtmlGenericControl.cs index 94dea44e8c..b590935488 100644 --- a/src/Framework/Framework/Controls/HtmlGenericControl.cs +++ b/src/Framework/Framework/Controls/HtmlGenericControl.cs @@ -286,12 +286,6 @@ protected override void RenderBeginTag(IHtmlWriter writer, IDotvvmRequestContext /// protected override void RenderEndTag(IHtmlWriter writer, IDotvvmRequestContext context) { - // render resource link. If the Render is invoked multiple times the resources are rendered on the first invocation. - if (TagName == "head") - new HeadResourceLinks().Render(writer, context); - else if (TagName == "body") - new BodyResourceLinks().Render(writer, context); - if (RendersHtmlTag) { writer.RenderEndTag(); diff --git a/src/Framework/Framework/DependencyInjection/DotVVMServiceCollectionExtensions.cs b/src/Framework/Framework/DependencyInjection/DotVVMServiceCollectionExtensions.cs index 2b3b97d0ac..f4b1004e9e 100644 --- a/src/Framework/Framework/DependencyInjection/DotVVMServiceCollectionExtensions.cs +++ b/src/Framework/Framework/DependencyInjection/DotVVMServiceCollectionExtensions.cs @@ -113,6 +113,7 @@ public static IServiceCollection RegisterDotVVMServices(IServiceCollection servi var requiredResourceControl = controlResolver.ResolveControl(new ResolvedTypeDescriptor(typeof(RequiredResource))); o.TreeVisitors.Add(() => new StyleTreeShufflingVisitor(controlResolver)); o.TreeVisitors.Add(() => new ControlPrecompilationVisitor(s)); + o.TreeVisitors.Add(() => new ResourceLinksVisitor(controlResolver)); o.TreeVisitors.Add(() => new LiteralOptimizationVisitor()); o.TreeVisitors.Add(() => new BindingRequiredResourceVisitor((ControlResolverMetadata)requiredResourceControl)); var requiredGlobalizeControl = controlResolver.ResolveControl(new ResolvedTypeDescriptor(typeof(GlobalizeResource))); diff --git a/src/Framework/Framework/Testing/TestHttpRequest.cs b/src/Framework/Framework/Testing/TestHttpRequest.cs index c97f484421..2bdb7e8fe6 100644 --- a/src/Framework/Framework/Testing/TestHttpRequest.cs +++ b/src/Framework/Framework/Testing/TestHttpRequest.cs @@ -25,7 +25,7 @@ public TestHttpRequest(IHttpContext context) public string? Path { get; set; } - public string? PathBase { get; set; } + public string? PathBase { get; set; } = ""; IPathString IHttpRequest.Path => new TestPathString(this.Path); IPathString IHttpRequest.PathBase => new TestPathString(this.PathBase); diff --git a/src/Framework/Testing/ControlTestHelper.cs b/src/Framework/Testing/ControlTestHelper.cs index 00572cd942..7d0adc17b9 100644 --- a/src/Framework/Testing/ControlTestHelper.cs +++ b/src/Framework/Testing/ControlTestHelper.cs @@ -147,6 +147,16 @@ public async Task RunPage( markup = "" + markup; } markup = $"@viewModel {viewModel.ToString().Replace("+", ".")}\n{directives}\n\n{markup}"; + return await RunPageRaw(markup, markupFiles, fileName, user, culture); + } + /// Run page with the exact markup, without any modifications + public async Task RunPageRaw( + string markup, + Dictionary? markupFiles = null, + [CallerMemberName] string? fileName = null, + ClaimsPrincipal? user = null, + CultureInfo? culture = null) + { var request = PreparePage(markup, markupFiles, fileName, user, culture); await presenter.ProcessRequest(request); return CreatePageResult(request); diff --git a/src/Framework/Testing/DotvvmTestHelper.cs b/src/Framework/Testing/DotvvmTestHelper.cs index e8f19583cc..a3119a7d9a 100644 --- a/src/Framework/Testing/DotvvmTestHelper.cs +++ b/src/Framework/Testing/DotvvmTestHelper.cs @@ -167,7 +167,10 @@ public static ResolvedTreeRoot ParseResolvedTree(string markup, string fileName if (checkErrors) CheckForErrors(root.DothtmlNode); foreach (var v in visitors) { - v().VisitView(root); + var visitor = v(); + if (visitor is ResourceLinksVisitor) + continue; // not wanted in parser tests + visitor.VisitView(root); } if (checkErrors) validator.VisitAndAssert(root); diff --git a/src/Tests/ControlTests/ResourceLinksTests.cs b/src/Tests/ControlTests/ResourceLinksTests.cs new file mode 100644 index 0000000000..96a11c51e9 --- /dev/null +++ b/src/Tests/ControlTests/ResourceLinksTests.cs @@ -0,0 +1,73 @@ +using System; +using System.Collections.Generic; +using System.ComponentModel.DataAnnotations; +using System.Globalization; +using System.Threading.Tasks; +using CheckTestOutput; +using DotVVM.Framework.Compilation; +using DotVVM.Framework.Controls; +using DotVVM.Framework.Tests.Binding; +using DotVVM.Framework.ViewModel; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using DotVVM.Framework.Testing; +using System.Security.Claims; + +namespace DotVVM.Framework.Tests.ControlTests +{ + [TestClass] + public class ResourceLinksTests + { + static readonly ControlTestHelper cth = new ControlTestHelper(config: config => { + }); + OutputChecker check = new OutputChecker("testoutputs"); + + [TestMethod] + public async Task OnlyLiteralInBody() + { + // There was a bug which collapsed the body contents into a `data-bind='text: ...'` binding + var r = await cth.RunPageRaw(""" + @viewModel DotVVM.Framework.Tests.ControlTests.ResourceLinksTests.BasicTestViewModel + + + Id: {{value: Integer}} + + """); + + check.CheckString(r.OutputString, fileExtension: "html"); + } + [TestMethod] + public async Task MultipleBodyElements() + { + // You can put body into the document by accident. In such case, DotVVM should try to select the outermost one + var r = await cth.RunPageRaw(""" + @viewModel DotVVM.Framework.Tests.ControlTests.ResourceLinksTests.BasicTestViewModel + + +
+ {{value: Integer}} +
+ + """); + + check.CheckString(r.OutputString, fileExtension: "html"); + } + [TestMethod] + public async Task NoHeadBodyElements() + { + // No body and head elements, DotVVM should put the resources at the start and end of the document, + // the browser will hopefully correctly infer the head and body elements + var r = await cth.RunPageRaw(""" + @viewModel DotVVM.Framework.Tests.ControlTests.ResourceLinksTests.BasicTestViewModel + +
= 0}>{{value: Integer}}
+ """); + + check.CheckString(r.OutputString, fileExtension: "html"); + } + public class BasicTestViewModel: DotvvmViewModelBase + { + [Bind(Name = "int")] + public int Integer { get; set; } = 10; + } + } +} diff --git a/src/Tests/ControlTests/testoutputs/AutoUITests.FormInRepeater.html b/src/Tests/ControlTests/testoutputs/AutoUITests.FormInRepeater.html index 51c5e1461f..c2c842848a 100644 --- a/src/Tests/ControlTests/testoutputs/AutoUITests.FormInRepeater.html +++ b/src/Tests/ControlTests/testoutputs/AutoUITests.FormInRepeater.html @@ -6,18 +6,18 @@ - + - + - + - + diff --git a/src/Tests/ControlTests/testoutputs/CompositeControlTests.MenuRepeater.html b/src/Tests/ControlTests/testoutputs/CompositeControlTests.MenuRepeater.html index e5bb9146f5..e1fc4a22e0 100644 --- a/src/Tests/ControlTests/testoutputs/CompositeControlTests.MenuRepeater.html +++ b/src/Tests/ControlTests/testoutputs/CompositeControlTests.MenuRepeater.html @@ -7,32 +7,32 @@
-
list-item1
+
list-item1
-
list-item2
+
list-item2
  • - +
-
+
diff --git a/src/Tests/ControlTests/testoutputs/CompositeControlTests.WrappedHierarchyRepeaterControlWithGeneratedIds.html b/src/Tests/ControlTests/testoutputs/CompositeControlTests.WrappedHierarchyRepeaterControlWithGeneratedIds.html index 2156bc59e3..3634a57c36 100644 --- a/src/Tests/ControlTests/testoutputs/CompositeControlTests.WrappedHierarchyRepeaterControlWithGeneratedIds.html +++ b/src/Tests/ControlTests/testoutputs/CompositeControlTests.WrappedHierarchyRepeaterControlWithGeneratedIds.html @@ -5,18 +5,18 @@
    -
  • A
    • +
  • A
    • -
  • B
    • +
  • B
    • -
  • C
    • +
  • C
    • -
  • D
    • +
  • D
    • @@ -25,15 +25,15 @@
      - +
    - -