RenderForm causes SQL queries because it uses the IContentService

[ronaldbarendse]

Every page with one or more forms causes the following SQL queries, as it uses the IContentService to populate all page elements (to replace page fields like [#pageId] and [#pageName]), so the first page view/form render makes quite a lot of SQL queries:

---

Because the IContentService/IDocumentRepository caches the results, the next page views/renders do not generate the same amount of SQL queries, but because the service uses read-locks, it still executes some SQL queries:

These SQL timings are from a LocalDb database instance running on a beefy developer PC, so they will be considerable slower on live environments, especially if the database server is not local (e.g. on Umbraco Cloud and Azure).

Reproduction

Bug summary

Umbraco Forms should not use Umbraco services when rendering forms, so the SQL queries can be avoided. Using the content service could even result in using unpublished values to be used/exposed!

Specifics

Running Umbraco 8.6.4 and Umbraco Forms 8.4.2 (both latest versions at the time of writing).

Steps to reproduce

Add a form to a page and enable the MiniProfiler results (append ?umbDebug=true to the URL).

Expected result

Use the Umbraco content cache, so it doesn't execute SQL queries and only uses published content.

Actual result

Every request causes at least read-lock SQL queries to be executed, causing the page to unneccesary slow down.

[nul800sebastiaan]

Thanks, this is going on the list of things to consider when we are picking up new issues to improve performance. 👍

[ronaldbarendse]

Using the IContentService here is a well documented anti-pattern, so I'm actually very surprised it gets used for this (especially in a product from HQ). I just can't think of a reason why you couldn't use the current IPublishedContent instead....

The fix is quite easy (thankfully the IPageService can be swapped out), so the following code makes sure it uses the content cache to get the page elements:

using System.Collections;
using Umbraco.Core;
using Umbraco.Core.Composing;
using Umbraco.Core.Models.PublishedContent;
using Umbraco.Forms.Core.Components;
using Umbraco.Forms.Core.Services;
using Umbraco.Web;

public class PublishedContentPageService : IPageService
{
	protected readonly IUmbracoContextAccessor umbracoContextAccessor;

	public PublishedContentPageService(IUmbracoContextAccessor umbracoContextAccessor)
	{
		this.umbracoContextAccessor = umbracoContextAccessor;
	}

	public Hashtable GetPageElements() => this.GetPageElements(this.umbracoContextAccessor.UmbracoContext?.PublishedRequest?.PublishedContent);

	public Hashtable GetPageElements(int contentId) => this.GetPageElements(this.umbracoContextAccessor.UmbracoContext.Content.GetById(contentId));

	public Hashtable GetPageElements(IPublishedContent content)
	{
		var pageElements = new Hashtable();

		if (content != null)
		{
			pageElements.Add("pageID", content.Id);
			pageElements.Add("parentID", content.Parent?.Id ?? -1);
			pageElements.Add("pageName", content.Name);
			pageElements.Add("nodeType", content.ContentType.Id);
			pageElements.Add("nodeTypeAlias", content.ContentType.Alias);
			pageElements.Add("writerName", content.WriterName);
			pageElements.Add("creatorName", content.CreatorName);
			pageElements.Add("createDate", content.CreateDate);
			pageElements.Add("updateDate", content.UpdateDate);
			pageElements.Add("path", content.Path);
			pageElements.Add("splitpath", content.Path.Split(new char[] { ',' }));

			if (content.TemplateId is int templateId)
			{
				pageElements.Add("template", templateId);
			}

			foreach (var property in content.Properties)
			{
				if (!pageElements.ContainsKey(property.Alias))
				{
					pageElements.Add(property.Alias, property.GetSourceValue());
				}
			}
		}

		return pageElements;
	}
}

[ComposeAfter(typeof(UmbracoFormsComposer))]
public class UmbracoFormsPageServiceComposer : IUserComposer
{
	public void Compose(Composition composition)
	{
		composition.Register<IPageService, PublishedContentPageService>(Lifetime.Singleton);
	}
}

[ronaldbarendse]

I've updated and extended the above code, so it's also possible to switch to an implementation that uses the preview version of IPublishedContent (so still from the content cache): https://gist.github.com/ronaldbarendse/82f49fe19021b2ca34e57df50cf4509e.

In a private discussion with @HalldorLyngmo, I've recommended to include both implementations and either:

1. Add a setting to UmbracoForms.config that switches between the implementations and have this set to the published content on new installs and preview content on upgrades (this could be released quickly in a patch version), so it's backwards-compatible: this assumes people expect unpublished/preview content, which I don't think is the case though: they are simply unaware, as in most cases the lastest saved version is also the published one.
2. Use the published content implementation by default and allow switching to preview content using a composer (like in the gist): this will change the implementation on upgrades, so should be marked as a breaking-change (new minor version) with the appropriate documentation on how to revert back to the old behavior.

I would go for solution 2, because using IContentService and/or unpublished/preview content by default should be considered a bug and it avoids adding another setting to UmbracoForms.config for something that can be done from code anyway.

[ronaldbarendse]

Regarding the provided implementation, maybe it's better to use IUmbracoContextFactory instead of IUmbracoContextAccessor, so it can still retrieve the content if there's no current UmbracoContext (e.g. when sending a mail from an async workflow/background task)?

[ronaldbarendse]

The following PublishedContentPageService also works when there's no current UmbracoContext and the PublishedContentPreviewPageService allows you to use current saved (instead of published) content using the preview function. It also uses Value() instead of GetSourceValue(), so you actually get the same value as used in views:

using System.Collections;
using Umbraco.Core.Models.PublishedContent;
using Umbraco.Core.Services;
using Umbraco.Forms.Core.Services;
using Umbraco.Web;
using Umbraco.Web.PublishedCache;

public class PublishedContentPageService : IPageService
{
	protected readonly IUmbracoContextFactory umbracoContextFactory;
	protected readonly IUserService userService;

	public PublishedContentPageService(IUmbracoContextFactory umbracoContextFactory, IUserService userService)
	{
		this.umbracoContextFactory = umbracoContextFactory;
		this.userService = userService;
	}

	public Hashtable GetPageElements() => this.GetPageElements(this.GetContent());

	public Hashtable GetPageElements(int contentId) => this.GetPageElements(this.GetContent(contentId));

	protected virtual IPublishedContent GetContent(int? contentId = null)
	{
		using (var umbracoContextReference = this.umbracoContextFactory.EnsureUmbracoContext())
		{
			if (!contentId.HasValue)
			{
				contentId = umbracoContextReference.UmbracoContext.PublishedRequest?.PublishedContent?.Id;
			}

			if (contentId.HasValue)
			{
				return this.GetContent(umbracoContextReference.UmbracoContext.Content, contentId.Value);
			}

			return null;
		}
	}

	protected virtual IPublishedContent GetContent(IPublishedContentCache contentCache, int contentId) => contentCache.GetById(contentId);

	protected virtual Hashtable GetPageElements(IPublishedContent content)
	{
		var pageElements = new Hashtable();

		if (content != null)
		{
			pageElements.Add("pageID", content.Id);
			pageElements.Add("parentID", content.Parent?.Id ?? -1);
			pageElements.Add("pageName", content.Name);
			pageElements.Add("nodeType", content.ContentType.Id);
			pageElements.Add("nodeTypeAlias", content.ContentType.Alias);
			pageElements.Add("writerName", content.WriterName(this.userService));
			pageElements.Add("creatorName", content.CreatorName(this.userService));
			pageElements.Add("createDate", content.CreateDate);
			pageElements.Add("updateDate", content.UpdateDate);
			pageElements.Add("path", content.Path);
			pageElements.Add("splitpath", content.Path.Split(new char[] { ',' }));

			if (content.TemplateId is int templateId)
			{
				pageElements.Add("template", templateId);
			}

			foreach (var property in content.Properties)
			{
				if (!pageElements.ContainsKey(property.Alias))
				{
					pageElements.Add(property.Alias, property.Value());
				}
			}
		}

		return pageElements;
	}
}

public class PublishedContentPreviewPageService : PublishedContentPageService
{
	public PublishedContentPreviewPageService(IUmbracoContextFactory umbracoContextFactory, IUserService userService)
		: base(umbracoContextFactory, userService)
	{ }

	protected override IPublishedContent GetContent(IPublishedContentCache contentCache, int contentId) => contentCache.GetById(true, contentId);
}

And this ensures only a single service is registered for IPageService (RegisterUnique() always registers services as singletons):

using Umbraco.Core;
using Umbraco.Core.Composing;
using Umbraco.Forms.Core.Services;

[ComposeAfter(typeof(Umbraco.Forms.Core.Components.UmbracoFormsComposer))]
public class UmbracoFormsComposer : IUserComposer
{
	public void Compose(Composition composition)
	{
		composition.RegisterUnique<IPageService, PublishedContentPageService>();
		//composition.RegisterUnique<IPageService, PublishedContentPreviewPageService>();
	}
}

[warrenbuckley]

Fixed in this PR https://github.com/umbraco/Forms/pull/443
Due for release in 8.7.0