From 3fac40b4b5dbadd0e099badc7f6745478b0393e1 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 10 Feb 2020 20:15:12 +1100 Subject: [PATCH 01/39] loads preview via sql paging instead of one single query --- .../Repositories/ContentRepository.cs | 67 ++++++++++++------- 1 file changed, 44 insertions(+), 23 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs index a0b211b6b2..24c9e84873 100644 --- a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs @@ -837,6 +837,8 @@ order by umbracoNode.{2}, umbracoNode.parentID, umbracoNode.sortOrder", public XmlDocument BuildPreviewXmlCache() { + + var xmlDoc = new XmlDocument(); var doctype = xmlDoc.CreateDocumentType("root", null, null, ApplicationContext.Current.Services.ContentTypeService.GetContentTypesDtd()); @@ -851,42 +853,61 @@ order by umbracoNode.{2}, umbracoNode.parentID, umbracoNode.sortOrder", var sql = string.Format(@"select umbracoNode.id, umbracoNode.parentID, umbracoNode.sortOrder, cmsPreviewXml.{0}, umbracoNode.{1} from umbracoNode inner join cmsPreviewXml on cmsPreviewXml.nodeId = umbracoNode.id and umbracoNode.nodeObjectType = @type inner join cmsDocument on cmsPreviewXml.versionId = cmsDocument.versionId and cmsDocument.newest=1 -where umbracoNode.trashed = 0 -order by umbracoNode.{2}, umbracoNode.parentID, umbracoNode.sortOrder", +where (umbracoNode.trashed = 0) +order by (umbracoNode.{2}), (umbracoNode.parentID), (umbracoNode.sortOrder)", SqlSyntax.GetQuotedColumnName("xml"), SqlSyntax.GetQuotedColumnName("level"), SqlSyntax.GetQuotedColumnName("level")); + var args = new object[] { new { type = NodeObjectTypeId } }; XmlElement last = null; - //NOTE: Query creates a reader - does not load all into memory - foreach (var row in Database.Query(sql, new { type = NodeObjectTypeId })) + long pageSize = 500; + int? itemCount = null; + long currPage = 0; + do { - string parentId = ((int)row.parentID).ToInvariantString(); - string xml = row.xml; - int sortOrder = row.sortOrder; - //if the parentid is changing - if (last != null && last.GetAttribute("parentID") != parentId) + // Get the paged queries + Database.BuildPageQueries(currPage, pageSize, sql, ref args, out var sqlCount, out var sqlPage); + + // get the item count once + if (itemCount == null) { - parent = xmlDoc.GetElementById(parentId); - if (parent == null) + itemCount = Database.ExecuteScalar(sqlCount, args); + } + currPage++; + + // iterate over rows without allocating all items to memory (Query vs Fetch) + foreach (var row in Database.Query(sqlPage, args)) + { + string parentId = ((int)row.parentID).ToInvariantString(); + string xml = row.xml; + int sortOrder = row.sortOrder; + + //if the parentid is changing + if (last != null && last.GetAttribute("parentID") != parentId) { - //Need to short circuit here, if the parent is not there it means that the parent is unpublished - // and therefore the child is not published either so cannot be included in the xml cache - continue; + parent = xmlDoc.GetElementById(parentId); + if (parent == null) + { + //Need to short circuit here, if the parent is not there it means that the parent is unpublished + // and therefore the child is not published either so cannot be included in the xml cache + continue; + } } + + var xmlDocFragment = xmlDoc.CreateDocumentFragment(); + xmlDocFragment.InnerXml = xml; + + last = (XmlElement)parent.AppendChild(xmlDocFragment); + + // fix sortOrder - see notes in UpdateSortOrder + last.Attributes["sortOrder"].Value = sortOrder.ToInvariantString(); } - var xmlDocFragment = xmlDoc.CreateDocumentFragment(); - xmlDocFragment.InnerXml = xml; - - last = (XmlElement)parent.AppendChild(xmlDocFragment); - - // fix sortOrder - see notes in UpdateSortOrder - last.Attributes["sortOrder"].Value = sortOrder.ToInvariantString(); - } - + } while (itemCount == pageSize); + return xmlDoc; } From e748d842a48282e74ba47edfdb25efad6a9bdc9a Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 26 Mar 2020 22:50:47 +1100 Subject: [PATCH 02/39] Fixes Memory Leak in GetCacheItem cache dependency #7773 --- .../Cache/HttpRuntimeCacheProvider.cs | 59 +++++++------------ src/umbraco.businesslogic/CacheHelper.cs | 31 ---------- .../umbraco.businesslogic.csproj | 1 - 3 files changed, 22 insertions(+), 69 deletions(-) delete mode 100644 src/umbraco.businesslogic/CacheHelper.cs diff --git a/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs index 0e98cd5318..30ce3c9926 100644 --- a/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs +++ b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs @@ -72,21 +72,16 @@ namespace Umbraco.Core.Cache /// public override object GetCacheItem(string cacheKey, Func getCacheItem) { - return GetCacheItem(cacheKey, getCacheItem, null, dependentFiles: null); + return GetCacheItemInternal(cacheKey, getCacheItem, null); } - /// - /// This overload is here for legacy purposes - /// - /// - /// - /// - /// - /// - /// - /// - /// + [Obsolete("This is here for legacy reasons only, do not use this method, it has memory leaks")] internal object GetCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, CacheDependency dependency = null) + { + return GetCacheItemInternal(cacheKey, getCacheItem, timeout, isSliding, priority, removedCallback, () => dependency); + } + + private object GetCacheItemInternal(string cacheKey, Func getCacheItem, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, Func dependency = null) { cacheKey = GetCacheKey(cacheKey); @@ -141,7 +136,7 @@ namespace Umbraco.Core.Cache lck.UpgradeToWriteLock(); //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! - _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); + _cache.Insert(cacheKey, result, dependency?.Invoke(), absolute, sliding, priority, removedCallback); } } @@ -160,29 +155,22 @@ namespace Umbraco.Core.Cache public object GetCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { - CacheDependency dependency = null; - if (dependentFiles != null && dependentFiles.Any()) - { - dependency = new CacheDependency(dependentFiles); - } - return GetCacheItem(cacheKey, getCacheItem, timeout, isSliding, priority, removedCallback, dependency); + return GetCacheItemInternal(cacheKey, getCacheItem, timeout, isSliding, priority, removedCallback, + // Don't create a CacheDependency object unless we need it, see https://github.com/umbraco/Umbraco-CMS/issues/7773 + () => dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null); } #endregion #region Insert - /// - /// This overload is here for legacy purposes - /// - /// - /// - /// - /// - /// - /// - /// + [Obsolete("This is here for legacy reasons only, do not use this method, it has memory leaks")] internal void InsertCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, CacheDependency dependency = null) + { + InsertCacheItemInternal(cacheKey, getCacheItem, timeout, isSliding, priority, removedCallback, () => dependency); + } + + private void InsertCacheItemInternal(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, Func dependency = null) { // NOTE - here also we must insert a Lazy but we can evaluate it right now // and make sure we don't store a null value. @@ -199,20 +187,17 @@ namespace Umbraco.Core.Cache using (new WriteLock(_locker)) { //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! - _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); + _cache.Insert(cacheKey, result, dependency?.Invoke(), absolute, sliding, priority, removedCallback); } } public void InsertCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { - CacheDependency dependency = null; - if (dependentFiles != null && dependentFiles.Any()) - { - dependency = new CacheDependency(dependentFiles); - } - InsertCacheItem(cacheKey, getCacheItem, timeout, isSliding, priority, removedCallback, dependency); + InsertCacheItemInternal(cacheKey, getCacheItem, timeout, isSliding, priority, removedCallback, + // Don't create a CacheDependency object unless we need it, see https://github.com/umbraco/Umbraco-CMS/issues/7773 + () => dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null); } #endregion } -} \ No newline at end of file +} diff --git a/src/umbraco.businesslogic/CacheHelper.cs b/src/umbraco.businesslogic/CacheHelper.cs deleted file mode 100644 index 243da8eadd..0000000000 --- a/src/umbraco.businesslogic/CacheHelper.cs +++ /dev/null @@ -1,31 +0,0 @@ -//using System; -//using System.Web.Caching; - -//namespace umbraco.BusinessLogic -//{ -// internal class CacheHelper -// { -// public delegate TT GetCacheItemDelegate(); -// public static TT GetCacheItem(string cacheKey, object syncLock, -// CacheItemPriority priority, CacheItemRemovedCallback refreshAction, -// CacheDependency cacheDependency, TimeSpan timeout, GetCacheItemDelegate getCacheItem) -// { -// object result = System.Web.HttpRuntime.Cache.Get(cacheKey); -// if (result == null) -// { -// lock (syncLock) -// { -// result = System.Web.HttpRuntime.Cache.Get(cacheKey); -// if (result == null) -// { -// result = getCacheItem(); -// System.Web.HttpRuntime.Cache.Add(cacheKey, result, cacheDependency, -// DateTime.Now.Add(timeout), TimeSpan.Zero, priority, refreshAction); -// } -// } -// } -// return (TT)result; -// } - -// } -//} diff --git a/src/umbraco.businesslogic/umbraco.businesslogic.csproj b/src/umbraco.businesslogic/umbraco.businesslogic.csproj index f3daf20bb4..aef9f365ad 100644 --- a/src/umbraco.businesslogic/umbraco.businesslogic.csproj +++ b/src/umbraco.businesslogic/umbraco.businesslogic.csproj @@ -203,7 +203,6 @@ ASPXCodeBehind - From f1b398fab63cdf7399c6058f6284638ec4ee00c4 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 1 Apr 2020 11:06:21 +1100 Subject: [PATCH 03/39] Adds some null checks, need to see if tests pass --- src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs | 5 +++++ src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs | 6 ++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index f92d8adebb..e5b66e259c 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -1028,8 +1028,13 @@ namespace Umbraco.Web.PublishedCache.NuCache { parentLink = parentLink ?? GetRequiredParentLink(content, null); + // TODO: This can result in a null value? see https://github.com/umbraco/Umbraco-CMS/issues/7868 + // It seems to be related to having corrupt Paths in the umbracoNode table. var parent = parentLink.Value; + if (parent == null) + throw new PanicException($"A null Value was returned on the {nameof(parentLink)} LinkedNode with id={content.ParentContentId}, potentially your database paths are corrupted, please see the HealthCheck dashboard and fixup data inconsistencies."); + // if parent has no children, clone parent + add as first child if (parent.FirstChildContentId < 0) { diff --git a/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs b/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs index d187996df8..8794978852 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs @@ -1,4 +1,6 @@ -namespace Umbraco.Web.PublishedCache.NuCache.Snap +using System; + +namespace Umbraco.Web.PublishedCache.NuCache.Snap { //NOTE: This cannot be struct because it references itself @@ -11,7 +13,7 @@ { public LinkedNode(TValue value, long gen, LinkedNode next = null) { - Value = value; + Value = value ?? throw new ArgumentNullException(nameof(value)); Gen = gen; Next = next; } From c9c869356001e55dcd30a3c3f7076ba0d01bb3b4 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 1 Apr 2020 13:13:15 +1100 Subject: [PATCH 04/39] removes one null check, adds notes --- src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs | 8 ++++++++ src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs | 6 ++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index e5b66e259c..9298201ecf 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -502,6 +502,14 @@ namespace Umbraco.Web.PublishedCache.NuCache } } + /// + /// Validate the and try to create a parent + /// + /// + /// + /// + /// Returns false if the parent was not found or if the kit validation failed + /// private bool BuildKit(ContentNodeKit kit, out LinkedNode parent) { // make sure parent exists diff --git a/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs b/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs index 8794978852..94f83ac4e5 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs @@ -1,6 +1,4 @@ -using System; - -namespace Umbraco.Web.PublishedCache.NuCache.Snap +namespace Umbraco.Web.PublishedCache.NuCache.Snap { //NOTE: This cannot be struct because it references itself @@ -13,7 +11,7 @@ namespace Umbraco.Web.PublishedCache.NuCache.Snap { public LinkedNode(TValue value, long gen, LinkedNode next = null) { - Value = value ?? throw new ArgumentNullException(nameof(value)); + Value = value; // This is allowed to be null, we actually explicitly set this to null in ClearLocked Gen = gen; Next = next; } From 50e9f79ae6f878e837fc0d4748a803378c8d0216 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 6 Apr 2020 23:10:52 +1000 Subject: [PATCH 05/39] Adds a null check if things are out of order when building up the tree/linked list --- .../PublishedCache/NuCache/ContentStore.cs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 9298201ecf..474c390027 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -520,6 +520,15 @@ namespace Umbraco.Web.PublishedCache.NuCache return false; } + // We cannot continue if there's no value. This shouldn't happen but it can happen if the database umbracoNode.path + // data is invalid/corrupt. If that is the case, the parentId might be ok but not the Path which can result in null + // because the data sort operation is by path. + if (parent.Value == null) + { + _logger.Warn($"Skip item id={kit.Node.Id}, no Data assigned for linked node with path {kit.Node.Path} and parent id {kit.Node.ParentContentId}. This can indicate data corruption for the Path value for node {kit.Node.Id}."); + return false; + } + // make sure the kit is valid if (kit.DraftData == null && kit.PublishedData == null) { @@ -1036,12 +1045,13 @@ namespace Umbraco.Web.PublishedCache.NuCache { parentLink = parentLink ?? GetRequiredParentLink(content, null); - // TODO: This can result in a null value? see https://github.com/umbraco/Umbraco-CMS/issues/7868 - // It seems to be related to having corrupt Paths in the umbracoNode table. var parent = parentLink.Value; + // We are doing a null check here but this should no longer be possible because we have a null check in BuildKit + // for the parent.Value property and we'll output a warning. However I'll leave this additional null check in place. + // see https://github.com/umbraco/Umbraco-CMS/issues/7868 if (parent == null) - throw new PanicException($"A null Value was returned on the {nameof(parentLink)} LinkedNode with id={content.ParentContentId}, potentially your database paths are corrupted, please see the HealthCheck dashboard and fixup data inconsistencies."); + throw new PanicException($"A null Value was returned on the {nameof(parentLink)} LinkedNode with id={content.ParentContentId}, potentially your database paths are corrupted."); // if parent has no children, clone parent + add as first child if (parent.FirstChildContentId < 0) From 4b467bf4702b2662cccf478cdb61b60eea17d70b Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Apr 2020 01:02:08 +1000 Subject: [PATCH 06/39] Creates data integrity health checks --- .../Repositories/IContentRepository.cs | 7 + .../Implement/ContentRepositoryBase.cs | 165 +++++++++++++++++- src/Umbraco.Core/Services/IContentService.cs | 2 +- .../Services/IContentServiceBase.cs | 13 +- .../Services/Implement/ContentService.cs | 20 +++ .../Services/Implement/MediaService.cs | 24 +++ .../Checks/Data/DatabaseIntegrityCheck.cs | 97 ++++++++++ src/Umbraco.Web/Umbraco.Web.csproj | 1 + 8 files changed, 326 insertions(+), 3 deletions(-) create mode 100644 src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs diff --git a/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs index 217719e144..aff7a58652 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs @@ -77,5 +77,12 @@ namespace Umbraco.Core.Persistence.Repositories /// Here, can be null but cannot. IEnumerable GetPage(IQuery query, long pageIndex, int pageSize, out long totalRecords, IQuery filter, Ordering ordering); + + /// + /// Checks the data integrity of the node paths stored in the database + /// + bool VerifyNodePaths(out int[] invalidIds); + + void FixNodePaths(); } } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index 13b687eb4e..cd79923323 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -403,7 +403,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement } // content type alias is invariant - if(ordering.OrderBy.InvariantEquals("contentTypeAlias")) + if (ordering.OrderBy.InvariantEquals("contentTypeAlias")) { var joins = Sql() .InnerJoin("ctype").On((content, contentType) => content.ContentTypeId == contentType.NodeId, aliasRight: "ctype"); @@ -477,6 +477,169 @@ namespace Umbraco.Core.Persistence.Repositories.Implement IQuery filter, Ordering ordering); + public bool VerifyNodePaths(out int[] invalidIds) + { + var invalid = new List(); + + var sql = SqlContext.Sql() + .Select() + .From() + .Where(x => x.NodeObjectType == NodeObjectTypeId) + .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); + + // TODO: Could verify sort orders here too + + var currentParentIds = new HashSet { -1 }; + var prevParentIds = currentParentIds; + var lastLevel = -1; + + // use a forward cursor (query) + foreach (var node in Database.Query(sql)) + { + if (node.Level != lastLevel) + { + // changing levels + prevParentIds = currentParentIds; + currentParentIds = null; + lastLevel = node.Level; + } + + if (currentParentIds == null) + { + // we're reset + currentParentIds = new HashSet(); + } + + currentParentIds.Add(node.NodeId); + + var pathParts = node.Path.Split(','); + + if (!prevParentIds.Contains(node.ParentId)) + { + // invalid, this will be because the level is wrong + invalid.Add(node.NodeId); + } + else if (pathParts.Length < 2) + { + // invalid path + invalid.Add(node.NodeId); + } + else if (pathParts.Length - 1 != node.Level) + { + // invalid, either path or level is wrong + invalid.Add(node.NodeId); + } + else if (pathParts[pathParts.Length - 1] != node.NodeId.ToString()) + { + // invalid path + invalid.Add(node.NodeId); + } + else if (pathParts[pathParts.Length - 2] != node.ParentId.ToString()) + { + // invalid path + invalid.Add(node.NodeId); + } + } + + invalidIds = invalid.ToArray(); + return invalid.Count == 0; + } + + public void FixNodePaths() + { + // TODO: We can probably combine this logic with the above + + var invalid = new List<(int child, int parent)>(); + + var sql = SqlContext.Sql() + .Select() + .From() + .Where(x => x.NodeObjectType == NodeObjectTypeId) + .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); + + // TODO: Could verify sort orders here too + + var updated = new List(); + var missingParentIds = new Dictionary>(); + var currentParentIds = new HashSet { -1 }; + var prevParentIds = currentParentIds; + var lastLevel = -1; + + // use a forward cursor (query) + foreach (var node in Database.Query(sql)) + { + if (node.Level != lastLevel) + { + // changing levels + prevParentIds = currentParentIds; + currentParentIds = null; + lastLevel = node.Level; + } + + if (currentParentIds == null) + { + // we're reset + currentParentIds = new HashSet(); + } + + currentParentIds.Add(node.NodeId); + + var pathParts = node.Path.Split(','); + + if (!prevParentIds.Contains(node.ParentId)) + { + // invalid, this will be because the level is wrong (which prob means path is wrong too) + invalid.Add((node.NodeId, node.ParentId)); + if (missingParentIds.TryGetValue(node.ParentId, out var childIds)) + childIds.Add(node); + else + missingParentIds[node.ParentId] = new List {node}; + } + else if (pathParts.Length < 2) + { + // invalid path + invalid.Add((node.NodeId, node.ParentId)); + } + else if (pathParts.Length - 1 != node.Level) + { + // invalid, either path or level is wrong + invalid.Add((node.NodeId, node.ParentId)); + } + else if (pathParts[pathParts.Length - 1] != node.NodeId.ToString()) + { + // invalid path + invalid.Add((node.NodeId, node.ParentId)); + } + else if (pathParts[pathParts.Length - 2] != node.ParentId.ToString()) + { + // invalid path + invalid.Add((node.NodeId, node.ParentId)); + } + else + { + // it's valid + + if (missingParentIds.TryGetValue(node.NodeId, out var invalidNodes)) + { + // this parent has been flagged as missing which means one or more of it's children was ordered + // wrong and was checked first. So now we can try to rebuild the invalid paths. + + foreach (var invalidNode in invalidNodes) + { + invalidNode.Level = (short) (node.Level + 1); + invalidNode.Path = node.Path + "," + invalidNode.NodeId; + updated.Add(invalidNode); + } + } + } + } + + foreach (var node in updated) + { + Database.Update(node); + } + } + // here, filter can be null and ordering cannot protected IEnumerable GetPage(IQuery query, long pageIndex, int pageSize, out long totalRecords, diff --git a/src/Umbraco.Core/Services/IContentService.cs b/src/Umbraco.Core/Services/IContentService.cs index 6f9ca58821..58279fb4da 100644 --- a/src/Umbraco.Core/Services/IContentService.cs +++ b/src/Umbraco.Core/Services/IContentService.cs @@ -526,6 +526,6 @@ namespace Umbraco.Core.Services OperationResult Rollback(int id, int versionId, string culture = "*", int userId = Constants.Security.SuperUserId); #endregion - + } } diff --git a/src/Umbraco.Core/Services/IContentServiceBase.cs b/src/Umbraco.Core/Services/IContentServiceBase.cs index 439c55d0d0..1c04e0b4a3 100644 --- a/src/Umbraco.Core/Services/IContentServiceBase.cs +++ b/src/Umbraco.Core/Services/IContentServiceBase.cs @@ -5,5 +5,16 @@ /// TODO: Start sharing the logic! /// public interface IContentServiceBase : IService - { } + { + + /// + /// Checks the data integrity of the node paths/levels stored in the database + /// + bool VerifyNodePaths(out int[] invalidIds); + + /// + /// Fixes the data integrity of node paths/levels stored in the database + /// + void FixNodePaths(); + } } diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index 1558b0170b..5d010d321f 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -2375,6 +2375,26 @@ namespace Umbraco.Core.Services.Implement return OperationResult.Succeed(evtMsgs); } + public bool VerifyNodePaths(out int[] invalidIds) + { + using (var scope = ScopeProvider.CreateScope(autoComplete: true)) + { + scope.ReadLock(Constants.Locks.ContentTree); + return _documentRepository.VerifyNodePaths(out invalidIds); + } + } + + public void FixNodePaths() + { + using (var scope = ScopeProvider.CreateScope(autoComplete: true)) + { + scope.WriteLock(Constants.Locks.ContentTree); + _documentRepository.FixNodePaths(); + + // TODO: We're going to have to clear all caches + } + } + #endregion #region Internal Methods diff --git a/src/Umbraco.Core/Services/Implement/MediaService.cs b/src/Umbraco.Core/Services/Implement/MediaService.cs index 528d0a0bf9..94f57bd859 100644 --- a/src/Umbraco.Core/Services/Implement/MediaService.cs +++ b/src/Umbraco.Core/Services/Implement/MediaService.cs @@ -1139,6 +1139,28 @@ namespace Umbraco.Core.Services.Implement } return true; + + } + + + public bool VerifyNodePaths(out int[] invalidIds) + { + using (var scope = ScopeProvider.CreateScope(autoComplete: true)) + { + scope.ReadLock(Constants.Locks.MediaTree); + return _mediaRepository.VerifyNodePaths(out invalidIds); + } + } + + public void FixNodePaths() + { + using (var scope = ScopeProvider.CreateScope(autoComplete: true)) + { + scope.WriteLock(Constants.Locks.MediaTree); + _mediaRepository.FixNodePaths(); + + // TODO: We're going to have to clear all caches + } } #endregion @@ -1358,5 +1380,7 @@ namespace Umbraco.Core.Services.Implement } #endregion + + } } diff --git a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs new file mode 100644 index 0000000000..d7bf62067f --- /dev/null +++ b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs @@ -0,0 +1,97 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; +using Umbraco.Core.Services; + +namespace Umbraco.Web.HealthCheck.Checks.Data +{ + [HealthCheck( + "73DD0C1C-E0CA-4C31-9564-1DCA509788AF", + "Database integrity check", + Description = "Checks for various data integrity issues in the Umbraco database.", + Group = "Data Integrity")] + public class DatabaseIntegrityCheck : HealthCheck + { + private readonly IContentService _contentService; + private readonly IMediaService _mediaService; + private const string _fixMediaPaths = "fixMediaPaths"; + private const string _fixContentPaths = "fixContentPaths"; + + public DatabaseIntegrityCheck(IContentService contentService, IMediaService mediaService) + { + _contentService = contentService; + _mediaService = mediaService; + } + + /// + /// Get the status for this health check + /// + /// + public override IEnumerable GetStatus() + { + //return the statuses + return new[] + { + CheckContent(), + CheckMedia() + }; + } + + private HealthCheckStatus CheckMedia() + { + return CheckPaths(_fixMediaPaths, "Fix media paths", "media", () => + { + var mediaPaths = _mediaService.VerifyNodePaths(out var invalidMediaPaths); + return (mediaPaths, invalidMediaPaths); + }); + } + + private HealthCheckStatus CheckContent() + { + return CheckPaths(_fixContentPaths, "Fix content paths", "content", () => + { + var contentPaths = _contentService.VerifyNodePaths(out var invalidContentPaths); + return (contentPaths, invalidContentPaths); + }); + } + + private HealthCheckStatus CheckPaths(string actionAlias, string actionName, string entityType, Func<(bool success, int[] invalidPaths)> doCheck) + { + var result = doCheck(); + + var actions = new List(); + if (!result.success) + { + actions.Add(new HealthCheckAction(actionAlias, Id) + { + Name = actionName + }); + } + + return new HealthCheckStatus(result.success + ? $"All {entityType} paths are valid" + : $"There are {result.invalidPaths.Length} invalid {entityType} paths") + { + ResultType = result.success ? StatusResultType.Success : StatusResultType.Error, + Actions = actions + }; + } + + public override HealthCheckStatus ExecuteAction(HealthCheckAction action) + { + switch (action.Alias) + { + case _fixContentPaths: + _contentService.FixNodePaths(); + return CheckContent(); + case _fixMediaPaths: + _mediaService.FixNodePaths(); + return CheckMedia(); + default: + throw new InvalidOperationException("Action not supported"); + } + } + } +} diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index c3024f63ae..e39687bed8 100755 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -156,6 +156,7 @@ + From 727578e51c57b202cad93d04858b64bf98b3893c Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Apr 2020 13:29:00 +1000 Subject: [PATCH 07/39] Fixes media moving and massively improves moving performance --- src/Umbraco.Core/Constants-SqlTemplates.cs | 17 +++++ .../Implement/ContentRepositoryBase.cs | 14 ++-- .../Repositories/Implement/MediaRepository.cs | 72 +++++++++++-------- .../Services/Implement/MediaService.cs | 43 ++++++----- src/Umbraco.Core/Umbraco.Core.csproj | 1 + 5 files changed, 92 insertions(+), 55 deletions(-) create mode 100644 src/Umbraco.Core/Constants-SqlTemplates.cs diff --git a/src/Umbraco.Core/Constants-SqlTemplates.cs b/src/Umbraco.Core/Constants-SqlTemplates.cs new file mode 100644 index 0000000000..33c94f0f93 --- /dev/null +++ b/src/Umbraco.Core/Constants-SqlTemplates.cs @@ -0,0 +1,17 @@ +namespace Umbraco.Core +{ + public static partial class Constants + { + public static class SqlTemplates + { + public const string VersionableRepositoryGetVersionIds = "Umbraco.Core.VersionableRepository.GetVersionIds"; + public const string VersionableRepositoryGetVersion = "Umbraco.Core.VersionableRepository.GetVersion"; + public const string VersionableRepositoryGetVersions = "Umbraco.Core.VersionableRepository.GetVersions"; + public const string VersionableRepositoryEnsureUniqueNodeName = "Umbraco.Core.VersionableRepository.EnsureUniqueNodeName"; + public const string VersionableRepositoryGetSortOrder = "Umbraco.Core.VersionableRepository.GetSortOrder"; + public const string VersionableRepositoryGetParentNode = "Umbraco.Core.VersionableRepository.GetParentNode"; + public const string VersionableRepositoryGetReservedId = "Umbraco.Core.VersionableRepository.GetReservedId"; + + } + } +} diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index cd79923323..cf67244fbe 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -83,7 +83,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // gets all version ids, current first public virtual IEnumerable GetVersionIds(int nodeId, int maxRows) { - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.GetVersionIds", tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetVersionIds, tsql => tsql.Select(x => x.Id) .From() .Where(x => x.NodeId == SqlTemplate.Arg("nodeId")) @@ -99,7 +99,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // TODO: test object node type? // get the version we want to delete - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.GetVersion", tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetVersion, tsql => tsql.Select().From().Where(x => x.Id == SqlTemplate.Arg("versionId")) ); var versionDto = Database.Fetch(template.Sql(new { versionId })).FirstOrDefault(); @@ -121,7 +121,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // TODO: test object node type? // get the versions we want to delete, excluding the current one - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.GetVersions", tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetVersions, tsql => tsql.Select().From().Where(x => x.NodeId == SqlTemplate.Arg("nodeId") && !x.Current && x.VersionDate < SqlTemplate.Arg("versionDate")) ); var versionDtos = Database.Fetch(template.Sql(new { nodeId, versionDate })); @@ -933,7 +933,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual string EnsureUniqueNodeName(int parentId, string nodeName, int id = 0) { - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.EnsureUniqueNodeName", tsql => tsql + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryEnsureUniqueNodeName, tsql => tsql .Select(x => Alias(x.NodeId, "id"), x => Alias(x.Text, "name")) .From() .Where(x => x.NodeObjectType == SqlTemplate.Arg("nodeObjectType") && x.ParentId == SqlTemplate.Arg("parentId"))); @@ -946,7 +946,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual int GetNewChildSortOrder(int parentId, int first) { - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.GetSortOrder", tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetSortOrder, tsql => tsql.Select($"COALESCE(MAX(sortOrder),{first - 1})").From().Where(x => x.ParentId == SqlTemplate.Arg("parentId") && x.NodeObjectType == NodeObjectTypeId) ); @@ -955,7 +955,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual NodeDto GetParentNodeDto(int parentId) { - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.GetParentNode", tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetParentNode, tsql => tsql.Select().From().Where(x => x.NodeId == SqlTemplate.Arg("parentId")) ); @@ -964,7 +964,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual int GetReservedId(Guid uniqueId) { - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.GetReservedId", tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetReservedId, tsql => tsql.Select(x => x.NodeId).From().Where(x => x.UniqueId == SqlTemplate.Arg("uniqueId") && x.NodeObjectType == Constants.ObjectTypes.IdReservation) ); var id = Database.ExecuteScalar(template.Sql(new { uniqueId = uniqueId })); diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs index 081efcfdf6..8518da8d8c 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs @@ -219,7 +219,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected override void PersistNewItem(IMedia entity) { - var media = (Models.Media) entity; + var media = entity; entity.AddingEntity(); // ensure unique name on the same level @@ -298,26 +298,35 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected override void PersistUpdatedItem(IMedia entity) { - var media = (Models.Media) entity; + var media = entity; // update media.UpdatingEntity(); - // ensure unique name on the same level - entity.Name = EnsureUniqueNodeName(entity.ParentId, entity.Name, entity.Id); + // Check if this entity is being moved as a descendant as part of a bulk moving operations. + // When this occurs, only Path + Level + UpdateDate are being changed. In this case we can bypass a lot of the below + // operations which will make this whole operation go much faster. When moving we don't need to create + // new versions, etc... because we cannot roll this operation back anyways. + var isMoving = entity.GetDirtyProperties().All(x => x == nameof(entity.Path) || x == nameof(entity.Level) || x == nameof(entity.UpdateDate)); - // ensure that strings don't contain characters that are invalid in xml - // TODO: do we really want to keep doing this here? - entity.SanitizeEntityPropertiesForXmlStorage(); - - // if parent has changed, get path, level and sort order - if (entity.IsPropertyDirty("ParentId")) + if (!isMoving) { - var parent = GetParentNodeDto(entity.ParentId); + // ensure unique name on the same level + entity.Name = EnsureUniqueNodeName(entity.ParentId, entity.Name, entity.Id); - entity.Path = string.Concat(parent.Path, ",", entity.Id); - entity.Level = parent.Level + 1; - entity.SortOrder = GetNewChildSortOrder(entity.ParentId, 0); + // ensure that strings don't contain characters that are invalid in xml + // TODO: do we really want to keep doing this here? + entity.SanitizeEntityPropertiesForXmlStorage(); + + // if parent has changed, get path, level and sort order + if (entity.IsPropertyDirty(nameof(entity.ParentId))) + { + var parent = GetParentNodeDto(entity.ParentId); + + entity.Path = string.Concat(parent.Path, ",", entity.Id); + entity.Level = parent.Level + 1; + entity.SortOrder = GetNewChildSortOrder(entity.ParentId, 0); + } } // create the dto @@ -328,26 +337,29 @@ namespace Umbraco.Core.Persistence.Repositories.Implement nodeDto.ValidatePathWithException(); Database.Update(nodeDto); - // update the content dto - Database.Update(dto.ContentDto); + if (!isMoving) + { + // update the content dto + Database.Update(dto.ContentDto); - // update the content & media version dtos - var contentVersionDto = dto.MediaVersionDto.ContentVersionDto; - var mediaVersionDto = dto.MediaVersionDto; - contentVersionDto.Current = true; - Database.Update(contentVersionDto); - Database.Update(mediaVersionDto); + // update the content & media version dtos + var contentVersionDto = dto.MediaVersionDto.ContentVersionDto; + var mediaVersionDto = dto.MediaVersionDto; + contentVersionDto.Current = true; + Database.Update(contentVersionDto); + Database.Update(mediaVersionDto); - // replace the property data - var deletePropertyDataSql = SqlContext.Sql().Delete().Where(x => x.VersionId == media.VersionId); - Database.Execute(deletePropertyDataSql); - var propertyDataDtos = PropertyFactory.BuildDtos(media.ContentType.Variations, media.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); - foreach (var propertyDataDto in propertyDataDtos) - Database.Insert(propertyDataDto); + // replace the property data + var deletePropertyDataSql = SqlContext.Sql().Delete().Where(x => x.VersionId == media.VersionId); + Database.Execute(deletePropertyDataSql); + var propertyDataDtos = PropertyFactory.BuildDtos(media.ContentType.Variations, media.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); + foreach (var propertyDataDto in propertyDataDtos) + Database.Insert(propertyDataDto); - SetEntityTags(entity, _tagRepository); + SetEntityTags(entity, _tagRepository); - PersistRelations(entity); + PersistRelations(entity); + } OnUowRefreshedEntity(new ScopedEntityEventArgs(AmbientScope, entity)); diff --git a/src/Umbraco.Core/Services/Implement/MediaService.cs b/src/Umbraco.Core/Services/Implement/MediaService.cs index 94f57bd859..aa3e07fa81 100644 --- a/src/Umbraco.Core/Services/Implement/MediaService.cs +++ b/src/Umbraco.Core/Services/Implement/MediaService.cs @@ -530,23 +530,27 @@ namespace Umbraco.Core.Services.Implement totalChildren = 0; return Enumerable.Empty(); } - return GetPagedDescendantsLocked(mediaPath[0].Path, pageIndex, pageSize, out totalChildren, filter, ordering); + return GetPagedLocked(GetPagedDescendantQuery(mediaPath[0].Path), pageIndex, pageSize, out totalChildren, filter, ordering); } - return GetPagedDescendantsLocked(null, pageIndex, pageSize, out totalChildren, filter, ordering); + return GetPagedLocked(GetPagedDescendantQuery(null), pageIndex, pageSize, out totalChildren, filter, ordering); } } - private IEnumerable GetPagedDescendantsLocked(string mediaPath, long pageIndex, int pageSize, out long totalChildren, + private IQuery GetPagedDescendantQuery(string mediaPath) + { + var query = Query(); + if (!mediaPath.IsNullOrWhiteSpace()) + query.Where(x => x.Path.SqlStartsWith(mediaPath + ",", TextColumnType.NVarchar)); + return query; + } + + private IEnumerable GetPagedLocked(IQuery query, long pageIndex, int pageSize, out long totalChildren, IQuery filter, Ordering ordering) { if (pageIndex < 0) throw new ArgumentOutOfRangeException(nameof(pageIndex)); if (pageSize <= 0) throw new ArgumentOutOfRangeException(nameof(pageSize)); if (ordering == null) throw new ArgumentNullException(nameof(ordering)); - var query = Query(); - if (!mediaPath.IsNullOrWhiteSpace()) - query.Where(x => x.Path.SqlStartsWith(mediaPath + ",", TextColumnType.NVarchar)); - return _mediaRepository.GetPage(query, pageIndex, pageSize, out totalChildren, filter, ordering); } @@ -888,7 +892,7 @@ namespace Umbraco.Core.Services.Implement public Attempt MoveToRecycleBin(IMedia media, int userId = Constants.Security.SuperUserId) { var evtMsgs = EventMessagesFactory.Get(); - var moves = new List>(); + var moves = new List<(IMedia, string)>(); using (var scope = ScopeProvider.CreateScope()) { @@ -940,7 +944,7 @@ namespace Umbraco.Core.Services.Implement return OperationResult.Attempt.Succeed(evtMsgs); } - var moves = new List>(); + var moves = new List<(IMedia, string)>(); using (var scope = ScopeProvider.CreateScope()) { @@ -979,7 +983,7 @@ namespace Umbraco.Core.Services.Implement // MUST be called from within WriteLock // trash indicates whether we are trashing, un-trashing, or not changing anything - private void PerformMoveLocked(IMedia media, int parentId, IMedia parent, int userId, ICollection> moves, bool? trash) + private void PerformMoveLocked(IMedia media, int parentId, IMedia parent, int userId, ICollection<(IMedia, string)> moves, bool? trash) { media.ParentId = parentId; @@ -989,7 +993,7 @@ namespace Umbraco.Core.Services.Implement var paths = new Dictionary(); - moves.Add(Tuple.Create(media, media.Path)); // capture original path + moves.Add((media, media.Path)); // capture original path //need to store the original path to lookup descendants based on it below var originalPath = media.Path; @@ -1006,21 +1010,24 @@ namespace Umbraco.Core.Services.Implement paths[media.Id] = (parent == null ? (parentId == Constants.System.RecycleBinMedia ? "-1,-21" : Constants.System.RootString) : parent.Path) + "," + media.Id; const int pageSize = 500; - var page = 0; - var total = long.MaxValue; - while (page * pageSize < total) + var query = GetPagedDescendantQuery(originalPath); + long total; + do { - var descendants = GetPagedDescendantsLocked(originalPath, page++, pageSize, out total, null, Ordering.By("Path", Direction.Ascending)); + var descendants = GetPagedLocked(query, 0, pageSize, out total, null, Ordering.By("Path", Direction.Ascending)); + foreach (var descendant in descendants) { - moves.Add(Tuple.Create(descendant, descendant.Path)); // capture original path + moves.Add((descendant, descendant.Path)); // capture original path // update path and level since we do not update parentId descendant.Path = paths[descendant.Id] = paths[descendant.ParentId] + "," + descendant.Id; descendant.Level += levelDelta; PerformMoveMediaLocked(descendant, userId, trash); } - } + + } while (total > pageSize); + } private void PerformMoveMediaLocked(IMedia media, int userId, bool? trash) @@ -1299,7 +1306,7 @@ namespace Umbraco.Core.Services.Implement // which we need for many things like keeping caches in sync, but we can surely do this MUCH better. var changes = new List>(); - var moves = new List>(); + var moves = new List<(IMedia, string)>(); var mediaTypeIdsA = mediaTypeIds.ToArray(); using (var scope = ScopeProvider.CreateScope()) diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 24954e316c..efb76f2756 100755 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -128,6 +128,7 @@ --> + From ad698f9c19c6442dfbc66c80e3bbe299c235cf4c Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Apr 2020 15:02:08 +1000 Subject: [PATCH 08/39] Fixes content service/repository for moving things correctly along with the perf improvements that were done to media. --- src/Umbraco.Core/ContentExtensions.cs | 10 + .../Implement/DocumentRepository.cs | 301 +++++++++--------- .../Repositories/Implement/MediaRepository.cs | 7 +- .../Services/Implement/ContentService.cs | 42 ++- .../Services/Implement/MediaService.cs | 1 + 5 files changed, 196 insertions(+), 165 deletions(-) diff --git a/src/Umbraco.Core/ContentExtensions.cs b/src/Umbraco.Core/ContentExtensions.cs index 3edad0c963..3ee2a75b09 100644 --- a/src/Umbraco.Core/ContentExtensions.cs +++ b/src/Umbraco.Core/ContentExtensions.cs @@ -60,6 +60,16 @@ namespace Umbraco.Core #endregion + internal static bool IsMoving(this IContentBase entity) + { + // Check if this entity is being moved as a descendant as part of a bulk moving operations. + // When this occurs, only Path + Level + UpdateDate are being changed. In this case we can bypass a lot of the below + // operations which will make this whole operation go much faster. When moving we don't need to create + // new versions, etc... because we cannot roll this operation back anyways. + var isMoving = entity.GetDirtyProperties().All(x => x == nameof(entity.Path) || x == nameof(entity.Level) || x == nameof(entity.UpdateDate)); + return isMoving; + } + /// /// Removes characters that are not valid XML characters from all entity properties /// of type string. See: http://stackoverflow.com/a/961504/5018 diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs index ccfa8209fb..a75165a7b9 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs @@ -321,7 +321,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement .InnerJoin() .On((c, d) => c.Id == d.Id) .Where(x => x.NodeId == SqlTemplate.Arg("nodeId") && !x.Current && x.VersionDate < SqlTemplate.Arg("versionDate")) - .Where( x => !x.Published) + .Where(x => !x.Published) ); var versionDtos = Database.Fetch(template.Sql(new { nodeId, versionDate })); foreach (var versionDto in versionDtos) @@ -519,8 +519,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected override void PersistUpdatedItem(IContent entity) { - var entityBase = entity as EntityBase; - var isEntityDirty = entityBase != null && entityBase.IsDirty(); + var isEntityDirty = entity.IsDirty(); // check if we need to make any database changes at all if ((entity.PublishedState == PublishedState.Published || entity.PublishedState == PublishedState.Unpublished) @@ -535,29 +534,37 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // update entity.UpdatingEntity(); + // Check if this entity is being moved as a descendant as part of a bulk moving operations. + // In this case we can bypass a lot of the below operations which will make this whole operation go much faster. + // When moving we don't need to create new versions, etc... because we cannot roll this operation back anyways. + var isMoving = entity.IsMoving(); + var publishing = entity.PublishedState == PublishedState.Publishing; - // check if we need to create a new version - if (publishing && entity.PublishedVersionId > 0) + if (!isMoving) { - // published version is not published anymore - Database.Execute(Sql().Update(u => u.Set(x => x.Published, false)).Where(x => x.Id == entity.PublishedVersionId)); - } + // check if we need to create a new version + if (publishing && entity.PublishedVersionId > 0) + { + // published version is not published anymore + Database.Execute(Sql().Update(u => u.Set(x => x.Published, false)).Where(x => x.Id == entity.PublishedVersionId)); + } - // sanitize names - SanitizeNames(entity, publishing); + // sanitize names + SanitizeNames(entity, publishing); - // ensure that strings don't contain characters that are invalid in xml - // TODO: do we really want to keep doing this here? - entity.SanitizeEntityPropertiesForXmlStorage(); + // ensure that strings don't contain characters that are invalid in xml + // TODO: do we really want to keep doing this here? + entity.SanitizeEntityPropertiesForXmlStorage(); - // if parent has changed, get path, level and sort order - if (entity.IsPropertyDirty("ParentId")) - { - var parent = GetParentNodeDto(entity.ParentId); - entity.Path = string.Concat(parent.Path, ",", entity.Id); - entity.Level = parent.Level + 1; - entity.SortOrder = GetNewChildSortOrder(entity.ParentId, 0); + // if parent has changed, get path, level and sort order + if (entity.IsPropertyDirty("ParentId")) + { + var parent = GetParentNodeDto(entity.ParentId); + entity.Path = string.Concat(parent.Path, ",", entity.Id); + entity.Level = parent.Level + 1; + entity.SortOrder = GetNewChildSortOrder(entity.ParentId, 0); + } } // create the dto @@ -568,146 +575,152 @@ namespace Umbraco.Core.Persistence.Repositories.Implement nodeDto.ValidatePathWithException(); Database.Update(nodeDto); - // update the content dto - Database.Update(dto.ContentDto); - - // update the content & document version dtos - var contentVersionDto = dto.DocumentVersionDto.ContentVersionDto; - var documentVersionDto = dto.DocumentVersionDto; - if (publishing) + if (!isMoving) { - documentVersionDto.Published = true; // now published - contentVersionDto.Current = false; // no more current - } - Database.Update(contentVersionDto); - Database.Update(documentVersionDto); + // update the content dto + Database.Update(dto.ContentDto); - // and, if publishing, insert new content & document version dtos - if (publishing) - { - entity.PublishedVersionId = entity.VersionId; - - contentVersionDto.Id = 0; // want a new id - contentVersionDto.Current = true; // current version - contentVersionDto.Text = entity.Name; - Database.Insert(contentVersionDto); - entity.VersionId = documentVersionDto.Id = contentVersionDto.Id; // get the new id - - documentVersionDto.Published = false; // non-published version - Database.Insert(documentVersionDto); - } - - // replace the property data (rather than updating) - // only need to delete for the version that existed, the new version (if any) has no property data yet - var versionToDelete = publishing ? entity.PublishedVersionId : entity.VersionId; - var deletePropertyDataSql = Sql().Delete().Where(x => x.VersionId == versionToDelete); - Database.Execute(deletePropertyDataSql); - - // insert property data - var propertyDataDtos = PropertyFactory.BuildDtos(entity.ContentType.Variations, entity.VersionId, publishing ? entity.PublishedVersionId : 0, - entity.Properties, LanguageRepository, out var edited, out var editedCultures); - foreach (var propertyDataDto in propertyDataDtos) - Database.Insert(propertyDataDto); - - // if !publishing, we may have a new name != current publish name, - // also impacts 'edited' - if (!publishing && entity.PublishName != entity.Name) - edited = true; - - if (entity.ContentType.VariesByCulture()) - { - // bump dates to align cultures to version + // update the content & document version dtos + var contentVersionDto = dto.DocumentVersionDto.ContentVersionDto; + var documentVersionDto = dto.DocumentVersionDto; if (publishing) - entity.AdjustDates(contentVersionDto.VersionDate); + { + documentVersionDto.Published = true; // now published + contentVersionDto.Current = false; // no more current + } + Database.Update(contentVersionDto); + Database.Update(documentVersionDto); - // names also impact 'edited' - // ReSharper disable once UseDeconstruction - foreach (var cultureInfo in entity.CultureInfos) - if (cultureInfo.Name != entity.GetPublishName(cultureInfo.Culture)) - { - edited = true; - (editedCultures ?? (editedCultures = new HashSet(StringComparer.OrdinalIgnoreCase))).Add(cultureInfo.Culture); + // and, if publishing, insert new content & document version dtos + if (publishing) + { + entity.PublishedVersionId = entity.VersionId; - // TODO: change tracking - // at the moment, we don't do any dirty tracking on property values, so we don't know whether the - // culture has just been edited or not, so we don't update its update date - that date only changes - // when the name is set, and it all works because the controller does it - but, if someone uses a - // service to change a property value and save (without setting name), the update date does not change. - } + contentVersionDto.Id = 0; // want a new id + contentVersionDto.Current = true; // current version + contentVersionDto.Text = entity.Name; + Database.Insert(contentVersionDto); + entity.VersionId = documentVersionDto.Id = contentVersionDto.Id; // get the new id - // replace the content version variations (rather than updating) + documentVersionDto.Published = false; // non-published version + Database.Insert(documentVersionDto); + } + + // replace the property data (rather than updating) // only need to delete for the version that existed, the new version (if any) has no property data yet - var deleteContentVariations = Sql().Delete().Where(x => x.VersionId == versionToDelete); - Database.Execute(deleteContentVariations); + var versionToDelete = publishing ? entity.PublishedVersionId : entity.VersionId; + var deletePropertyDataSql = Sql().Delete().Where(x => x.VersionId == versionToDelete); + Database.Execute(deletePropertyDataSql); - // replace the document version variations (rather than updating) - var deleteDocumentVariations = Sql().Delete().Where(x => x.NodeId == entity.Id); - Database.Execute(deleteDocumentVariations); + // insert property data + var propertyDataDtos = PropertyFactory.BuildDtos(entity.ContentType.Variations, entity.VersionId, publishing ? entity.PublishedVersionId : 0, + entity.Properties, LanguageRepository, out var edited, out var editedCultures); + foreach (var propertyDataDto in propertyDataDtos) + Database.Insert(propertyDataDto); - // TODO: NPoco InsertBulk issue? - // we should use the native NPoco InsertBulk here but it causes problems (not sure exactly all scenarios) - // but by using SQL Server and updating a variants name will cause: Unable to cast object of type - // 'Umbraco.Core.Persistence.FaultHandling.RetryDbConnection' to type 'System.Data.SqlClient.SqlConnection'. - // (same in PersistNewItem above) + // if !publishing, we may have a new name != current publish name, + // also impacts 'edited' + if (!publishing && entity.PublishName != entity.Name) + edited = true; - // insert content variations - Database.BulkInsertRecords(GetContentVariationDtos(entity, publishing)); + if (entity.ContentType.VariesByCulture()) + { + // bump dates to align cultures to version + if (publishing) + entity.AdjustDates(contentVersionDto.VersionDate); - // insert document variations - Database.BulkInsertRecords(GetDocumentVariationDtos(entity, editedCultures)); + // names also impact 'edited' + // ReSharper disable once UseDeconstruction + foreach (var cultureInfo in entity.CultureInfos) + if (cultureInfo.Name != entity.GetPublishName(cultureInfo.Culture)) + { + edited = true; + (editedCultures ?? (editedCultures = new HashSet(StringComparer.OrdinalIgnoreCase))).Add(cultureInfo.Culture); + + // TODO: change tracking + // at the moment, we don't do any dirty tracking on property values, so we don't know whether the + // culture has just been edited or not, so we don't update its update date - that date only changes + // when the name is set, and it all works because the controller does it - but, if someone uses a + // service to change a property value and save (without setting name), the update date does not change. + } + + // replace the content version variations (rather than updating) + // only need to delete for the version that existed, the new version (if any) has no property data yet + var deleteContentVariations = Sql().Delete().Where(x => x.VersionId == versionToDelete); + Database.Execute(deleteContentVariations); + + // replace the document version variations (rather than updating) + var deleteDocumentVariations = Sql().Delete().Where(x => x.NodeId == entity.Id); + Database.Execute(deleteDocumentVariations); + + // TODO: NPoco InsertBulk issue? + // we should use the native NPoco InsertBulk here but it causes problems (not sure exactly all scenarios) + // but by using SQL Server and updating a variants name will cause: Unable to cast object of type + // 'Umbraco.Core.Persistence.FaultHandling.RetryDbConnection' to type 'System.Data.SqlClient.SqlConnection'. + // (same in PersistNewItem above) + + // insert content variations + Database.BulkInsertRecords(GetContentVariationDtos(entity, publishing)); + + // insert document variations + Database.BulkInsertRecords(GetDocumentVariationDtos(entity, editedCultures)); + } + + // refresh content + entity.SetCultureEdited(editedCultures); + + // update the document dto + // at that point, when un/publishing, the entity still has its old Published value + // so we need to explicitly update the dto to persist the correct value + if (entity.PublishedState == PublishedState.Publishing) + dto.Published = true; + else if (entity.PublishedState == PublishedState.Unpublishing) + dto.Published = false; + entity.Edited = dto.Edited = !dto.Published || edited; // if not published, always edited + Database.Update(dto); + + //update the schedule + if (entity.IsPropertyDirty("ContentSchedule")) + PersistContentSchedule(entity, true); + + // if entity is publishing, update tags, else leave tags there + // means that implicitly unpublished, or trashed, entities *still* have tags in db + if (entity.PublishedState == PublishedState.Publishing) + SetEntityTags(entity, _tagRepository); } - // refresh content - entity.SetCultureEdited(editedCultures); - - // update the document dto - // at that point, when un/publishing, the entity still has its old Published value - // so we need to explicitly update the dto to persist the correct value - if (entity.PublishedState == PublishedState.Publishing) - dto.Published = true; - else if (entity.PublishedState == PublishedState.Unpublishing) - dto.Published = false; - entity.Edited = dto.Edited = !dto.Published || edited; // if not published, always edited - Database.Update(dto); - - //update the schedule - if (entity.IsPropertyDirty("ContentSchedule")) - PersistContentSchedule(entity, true); - - // if entity is publishing, update tags, else leave tags there - // means that implicitly unpublished, or trashed, entities *still* have tags in db - if (entity.PublishedState == PublishedState.Publishing) - SetEntityTags(entity, _tagRepository); - // trigger here, before we reset Published etc OnUowRefreshedEntity(new ScopedEntityEventArgs(AmbientScope, entity)); - // flip the entity's published property - // this also flips its published state - if (entity.PublishedState == PublishedState.Publishing) + if (!isMoving) { - entity.Published = true; - entity.PublishTemplateId = entity.TemplateId; - entity.PublisherId = entity.WriterId; - entity.PublishName = entity.Name; - entity.PublishDate = entity.UpdateDate; + // flip the entity's published property + // this also flips its published state + if (entity.PublishedState == PublishedState.Publishing) + { + entity.Published = true; + entity.PublishTemplateId = entity.TemplateId; + entity.PublisherId = entity.WriterId; + entity.PublishName = entity.Name; + entity.PublishDate = entity.UpdateDate; - SetEntityTags(entity, _tagRepository); + SetEntityTags(entity, _tagRepository); + } + else if (entity.PublishedState == PublishedState.Unpublishing) + { + entity.Published = false; + entity.PublishTemplateId = null; + entity.PublisherId = null; + entity.PublishName = null; + entity.PublishDate = null; + + ClearEntityTags(entity, _tagRepository); + } + + PersistRelations(entity); + + // TODO: note re. tags: explicitly unpublished entities have cleared tags, but masked or trashed entities *still* have tags in the db - so what? } - else if (entity.PublishedState == PublishedState.Unpublishing) - { - entity.Published = false; - entity.PublishTemplateId = null; - entity.PublisherId = null; - entity.PublishName = null; - entity.PublishDate = null; - - ClearEntityTags(entity, _tagRepository); - } - - PersistRelations(entity); - - // TODO: note re. tags: explicitly unpublished entities have cleared tags, but masked or trashed entities *still* have tags in the db - so what? entity.ResetDirtyProperties(); @@ -1183,7 +1196,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (temp.Template2Id.HasValue && templates.ContainsKey(temp.Template2Id.Value)) temp.Content.PublishTemplateId = temp.Template2Id; } - + // set properties if (loadProperties) @@ -1216,7 +1229,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement SetVariations(temp.Content, contentVariations, documentVariations); } } - + foreach (var c in content) @@ -1430,7 +1443,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement yield return dto; } - + } private class ContentVariation diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs index 8518da8d8c..b9ef2d4086 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs @@ -304,10 +304,9 @@ namespace Umbraco.Core.Persistence.Repositories.Implement media.UpdatingEntity(); // Check if this entity is being moved as a descendant as part of a bulk moving operations. - // When this occurs, only Path + Level + UpdateDate are being changed. In this case we can bypass a lot of the below - // operations which will make this whole operation go much faster. When moving we don't need to create - // new versions, etc... because we cannot roll this operation back anyways. - var isMoving = entity.GetDirtyProperties().All(x => x == nameof(entity.Path) || x == nameof(entity.Level) || x == nameof(entity.UpdateDate)); + // In this case we can bypass a lot of the below operations which will make this whole operation go much faster. + // When moving we don't need to create new versions, etc... because we cannot roll this operation back anyways. + var isMoving = entity.IsMoving(); if (!isMoving) { diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index 5d010d321f..f5f3d03abb 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -601,23 +601,27 @@ namespace Umbraco.Core.Services.Implement totalChildren = 0; return Enumerable.Empty(); } - return GetPagedDescendantsLocked(contentPath[0].Path, pageIndex, pageSize, out totalChildren, filter, ordering); + return GetPagedLocked(GetPagedDescendantQuery(contentPath[0].Path), pageIndex, pageSize, out totalChildren, filter, ordering); } - return GetPagedDescendantsLocked(null, pageIndex, pageSize, out totalChildren, filter, ordering); + return GetPagedLocked(null, pageIndex, pageSize, out totalChildren, filter, ordering); } } - private IEnumerable GetPagedDescendantsLocked(string contentPath, long pageIndex, int pageSize, out long totalChildren, + private IQuery GetPagedDescendantQuery(string contentPath) + { + var query = Query(); + if (!contentPath.IsNullOrWhiteSpace()) + query.Where(x => x.Path.SqlStartsWith($"{contentPath},", TextColumnType.NVarchar)); + return query; + } + + private IEnumerable GetPagedLocked(IQuery query, long pageIndex, int pageSize, out long totalChildren, IQuery filter, Ordering ordering) { if (pageIndex < 0) throw new ArgumentOutOfRangeException(nameof(pageIndex)); if (pageSize <= 0) throw new ArgumentOutOfRangeException(nameof(pageSize)); if (ordering == null) throw new ArgumentNullException(nameof(ordering)); - var query = Query(); - if (!contentPath.IsNullOrWhiteSpace()) - query.Where(x => x.Path.SqlStartsWith($"{contentPath},", TextColumnType.NVarchar)); - return _documentRepository.GetPage(query, pageIndex, pageSize, out totalChildren, filter, ordering); } @@ -1866,7 +1870,7 @@ namespace Umbraco.Core.Services.Implement public OperationResult MoveToRecycleBin(IContent content, int userId) { var evtMsgs = EventMessagesFactory.Get(); - var moves = new List>(); + var moves = new List<(IContent, string)>(); using (var scope = ScopeProvider.CreateScope()) { @@ -1925,7 +1929,7 @@ namespace Umbraco.Core.Services.Implement return; } - var moves = new List>(); + var moves = new List<(IContent, string)>(); using (var scope = ScopeProvider.CreateScope()) { @@ -1978,7 +1982,7 @@ namespace Umbraco.Core.Services.Implement // MUST be called from within WriteLock // trash indicates whether we are trashing, un-trashing, or not changing anything private void PerformMoveLocked(IContent content, int parentId, IContent parent, int userId, - ICollection> moves, + ICollection<(IContent, string)> moves, bool? trash) { content.WriterId = userId; @@ -1990,7 +1994,7 @@ namespace Umbraco.Core.Services.Implement var paths = new Dictionary(); - moves.Add(Tuple.Create(content, content.Path)); // capture original path + moves.Add((content, content.Path)); // capture original path //need to store the original path to lookup descendants based on it below var originalPath = content.Path; @@ -2007,20 +2011,24 @@ namespace Umbraco.Core.Services.Implement paths[content.Id] = (parent == null ? (parentId == Constants.System.RecycleBinContent ? "-1,-20" : Constants.System.RootString) : parent.Path) + "," + content.Id; const int pageSize = 500; - var total = long.MaxValue; - while (total > 0) + var query = GetPagedDescendantQuery(originalPath); + long total; + do { - var descendants = GetPagedDescendantsLocked(originalPath, 0, pageSize, out total, null, Ordering.By("Path", Direction.Ascending)); + // We always page a page 0 because for each page, we are moving the result so the resulting total will be reduced + var descendants = GetPagedLocked(query, 0, pageSize, out total, null, Ordering.By("Path", Direction.Ascending)); + foreach (var descendant in descendants) { - moves.Add(Tuple.Create(descendant, descendant.Path)); // capture original path + moves.Add((descendant, descendant.Path)); // capture original path // update path and level since we do not update parentId descendant.Path = paths[descendant.Id] = paths[descendant.ParentId] + "," + descendant.Id; descendant.Level += levelDelta; PerformMoveContentLocked(descendant, userId, trash); } - } + + } while (total > pageSize); } @@ -2832,7 +2840,7 @@ namespace Umbraco.Core.Services.Implement // which we need for many things like keeping caches in sync, but we can surely do this MUCH better. var changes = new List>(); - var moves = new List>(); + var moves = new List<(IContent, string)>(); var contentTypeIdsA = contentTypeIds.ToArray(); // using an immediate uow here because we keep making changes with diff --git a/src/Umbraco.Core/Services/Implement/MediaService.cs b/src/Umbraco.Core/Services/Implement/MediaService.cs index aa3e07fa81..219beb7088 100644 --- a/src/Umbraco.Core/Services/Implement/MediaService.cs +++ b/src/Umbraco.Core/Services/Implement/MediaService.cs @@ -1014,6 +1014,7 @@ namespace Umbraco.Core.Services.Implement long total; do { + // We always page a page 0 because for each page, we are moving the result so the resulting total will be reduced var descendants = GetPagedLocked(query, 0, pageSize, out total, null, Ordering.By("Path", Direction.Ascending)); foreach (var descendant in descendants) From 18d9ad3c7367550fb2d6b9d49995c86330d8b9d4 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Apr 2020 16:42:21 +1000 Subject: [PATCH 09/39] udpates health check and service methods to be flexible for data integrity checks, fixes tests --- src/Umbraco.Core/ContentExtensions.cs | 5 +- .../Models/ContentDataIntegrityReport.cs | 44 +++++ .../Models/ContentDataIntegrityReportEntry.cs | 13 ++ .../ContentDataIntegrityReportOptions.cs | 13 ++ .../Repositories/IContentRepository.cs | 8 +- .../Implement/ContentRepositoryBase.cs | 151 ++++++------------ .../Services/IContentServiceBase.cs | 14 +- .../Services/Implement/ContentService.cs | 14 +- .../Services/Implement/MediaService.cs | 15 +- src/Umbraco.Core/Umbraco.Core.csproj | 3 + .../Checks/Data/DatabaseIntegrityCheck.cs | 53 +++--- .../PublishedCache/NuCache/ContentStore.cs | 2 +- 12 files changed, 163 insertions(+), 172 deletions(-) create mode 100644 src/Umbraco.Core/Models/ContentDataIntegrityReport.cs create mode 100644 src/Umbraco.Core/Models/ContentDataIntegrityReportEntry.cs create mode 100644 src/Umbraco.Core/Models/ContentDataIntegrityReportOptions.cs diff --git a/src/Umbraco.Core/ContentExtensions.cs b/src/Umbraco.Core/ContentExtensions.cs index 3ee2a75b09..7bce23e98e 100644 --- a/src/Umbraco.Core/ContentExtensions.cs +++ b/src/Umbraco.Core/ContentExtensions.cs @@ -66,7 +66,10 @@ namespace Umbraco.Core // When this occurs, only Path + Level + UpdateDate are being changed. In this case we can bypass a lot of the below // operations which will make this whole operation go much faster. When moving we don't need to create // new versions, etc... because we cannot roll this operation back anyways. - var isMoving = entity.GetDirtyProperties().All(x => x == nameof(entity.Path) || x == nameof(entity.Level) || x == nameof(entity.UpdateDate)); + var isMoving = entity.IsPropertyDirty(nameof(entity.Path)) + && entity.IsPropertyDirty(nameof(entity.Level)) + && entity.IsPropertyDirty(nameof(entity.UpdateDate)); + return isMoving; } diff --git a/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs b/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs new file mode 100644 index 0000000000..60683ee72f --- /dev/null +++ b/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs @@ -0,0 +1,44 @@ +using System.Collections.Generic; + +namespace Umbraco.Core.Models +{ + public class ContentDataIntegrityReport + { + public ContentDataIntegrityReport(IReadOnlyDictionary detectedIssues) + { + DetectedIssues = detectedIssues; + } + + public bool Ok => DetectedIssues.Count == 0; + + public IReadOnlyDictionary DetectedIssues { get; } + + public enum IssueType + { + /// + /// The item's level and path are inconsistent with it's parent's path and level + /// + InvalidPathAndLevelByParentId, + + /// + /// The item's path doesn't contain all required parts + /// + InvalidPathEmpty, + + /// + /// The item's path parts are inconsistent with it's level value + /// + InvalidPathLevelMismatch, + + /// + /// The item's path does not end with it's own ID + /// + InvalidPathById, + + /// + /// The item's path does not have it's parent Id as the 2nd last entry + /// + InvalidPathByParentId, + } + } +} diff --git a/src/Umbraco.Core/Models/ContentDataIntegrityReportEntry.cs b/src/Umbraco.Core/Models/ContentDataIntegrityReportEntry.cs new file mode 100644 index 0000000000..517b9e80dc --- /dev/null +++ b/src/Umbraco.Core/Models/ContentDataIntegrityReportEntry.cs @@ -0,0 +1,13 @@ +namespace Umbraco.Core.Models +{ + public class ContentDataIntegrityReportEntry + { + public ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType issueType) + { + IssueType = issueType; + } + + public ContentDataIntegrityReport.IssueType IssueType { get; } + public bool Fixed { get; set; } + } +} diff --git a/src/Umbraco.Core/Models/ContentDataIntegrityReportOptions.cs b/src/Umbraco.Core/Models/ContentDataIntegrityReportOptions.cs new file mode 100644 index 0000000000..c4689467c1 --- /dev/null +++ b/src/Umbraco.Core/Models/ContentDataIntegrityReportOptions.cs @@ -0,0 +1,13 @@ +namespace Umbraco.Core.Models +{ + public class ContentDataIntegrityReportOptions + { + /// + /// Set to true to try to automatically resolve data integrity issues + /// + public bool FixIssues { get; set; } + + // TODO: We could define all sorts of options for the data integrity check like what to check for, what to fix, etc... + // things like Tag data consistency, etc... + } +} diff --git a/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs index aff7a58652..ad9e2d27c1 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using Umbraco.Core.Models; using Umbraco.Core.Models.Entities; using Umbraco.Core.Persistence.DatabaseModelDefinitions; using Umbraco.Core.Persistence.Querying; @@ -78,11 +79,6 @@ namespace Umbraco.Core.Persistence.Repositories IEnumerable GetPage(IQuery query, long pageIndex, int pageSize, out long totalRecords, IQuery filter, Ordering ordering); - /// - /// Checks the data integrity of the node paths stored in the database - /// - bool VerifyNodePaths(out int[] invalidIds); - - void FixNodePaths(); + ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options); } } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index cf67244fbe..42d7e67bc0 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -477,9 +477,9 @@ namespace Umbraco.Core.Persistence.Repositories.Implement IQuery filter, Ordering ordering); - public bool VerifyNodePaths(out int[] invalidIds) + public ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options) { - var invalid = new List(); + var report = new Dictionary(); var sql = SqlContext.Sql() .Select() @@ -487,80 +487,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement .Where(x => x.NodeObjectType == NodeObjectTypeId) .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); - // TODO: Could verify sort orders here too - - var currentParentIds = new HashSet { -1 }; - var prevParentIds = currentParentIds; - var lastLevel = -1; - - // use a forward cursor (query) - foreach (var node in Database.Query(sql)) - { - if (node.Level != lastLevel) - { - // changing levels - prevParentIds = currentParentIds; - currentParentIds = null; - lastLevel = node.Level; - } - - if (currentParentIds == null) - { - // we're reset - currentParentIds = new HashSet(); - } - - currentParentIds.Add(node.NodeId); - - var pathParts = node.Path.Split(','); - - if (!prevParentIds.Contains(node.ParentId)) - { - // invalid, this will be because the level is wrong - invalid.Add(node.NodeId); - } - else if (pathParts.Length < 2) - { - // invalid path - invalid.Add(node.NodeId); - } - else if (pathParts.Length - 1 != node.Level) - { - // invalid, either path or level is wrong - invalid.Add(node.NodeId); - } - else if (pathParts[pathParts.Length - 1] != node.NodeId.ToString()) - { - // invalid path - invalid.Add(node.NodeId); - } - else if (pathParts[pathParts.Length - 2] != node.ParentId.ToString()) - { - // invalid path - invalid.Add(node.NodeId); - } - } - - invalidIds = invalid.ToArray(); - return invalid.Count == 0; - } - - public void FixNodePaths() - { - // TODO: We can probably combine this logic with the above - - var invalid = new List<(int child, int parent)>(); - - var sql = SqlContext.Sql() - .Select() - .From() - .Where(x => x.NodeObjectType == NodeObjectTypeId) - .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); - - // TODO: Could verify sort orders here too - - var updated = new List(); - var missingParentIds = new Dictionary>(); + var nodesToRebuild = new Dictionary>(); + var validNodes = new Dictionary(); var currentParentIds = new HashSet { -1 }; var prevParentIds = currentParentIds; var lastLevel = -1; @@ -589,54 +517,77 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (!prevParentIds.Contains(node.ParentId)) { // invalid, this will be because the level is wrong (which prob means path is wrong too) - invalid.Add((node.NodeId, node.ParentId)); - if (missingParentIds.TryGetValue(node.ParentId, out var childIds)) - childIds.Add(node); - else - missingParentIds[node.ParentId] = new List {node}; + report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathAndLevelByParentId)); + AppendNodeToFix(nodesToRebuild, node); } else if (pathParts.Length < 2) { // invalid path - invalid.Add((node.NodeId, node.ParentId)); + report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathEmpty)); + AppendNodeToFix(nodesToRebuild, node); } else if (pathParts.Length - 1 != node.Level) { // invalid, either path or level is wrong - invalid.Add((node.NodeId, node.ParentId)); + report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathLevelMismatch)); + AppendNodeToFix(nodesToRebuild, node); } else if (pathParts[pathParts.Length - 1] != node.NodeId.ToString()) { // invalid path - invalid.Add((node.NodeId, node.ParentId)); + report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathById)); + AppendNodeToFix(nodesToRebuild, node); } else if (pathParts[pathParts.Length - 2] != node.ParentId.ToString()) { // invalid path - invalid.Add((node.NodeId, node.ParentId)); + report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathByParentId)); + AppendNodeToFix(nodesToRebuild, node); } else { - // it's valid + // it's valid! - if (missingParentIds.TryGetValue(node.NodeId, out var invalidNodes)) - { - // this parent has been flagged as missing which means one or more of it's children was ordered - // wrong and was checked first. So now we can try to rebuild the invalid paths. - - foreach (var invalidNode in invalidNodes) - { - invalidNode.Level = (short) (node.Level + 1); - invalidNode.Path = node.Path + "," + invalidNode.NodeId; - updated.Add(invalidNode); - } - } + // don't track unless we are configured to fix + if (options.FixIssues) + validNodes.Add(node.NodeId, node); } } - foreach (var node in updated) + var updated = new List(); + + if (options.FixIssues) { - Database.Update(node); + // iterate all valid nodes to see if these are parents for invalid nodes + foreach (var (nodeId, node) in validNodes) + { + if (!nodesToRebuild.TryGetValue(nodeId, out var invalidNodes)) continue; + + // now we can try to rebuild the invalid paths. + + foreach (var invalidNode in invalidNodes) + { + invalidNode.Level = (short)(node.Level + 1); + invalidNode.Path = node.Path + "," + invalidNode.NodeId; + updated.Add(invalidNode); + } + } + + foreach (var node in updated) + { + Database.Update(node); + } + } + + return new ContentDataIntegrityReport(report); + + // inline method used to append nodes to rebuild + static void AppendNodeToFix(IDictionary> nodesToRebuild, NodeDto node) + { + if (nodesToRebuild.TryGetValue(node.ParentId, out var childIds)) + childIds.Add(node); + else + nodesToRebuild[node.ParentId] = new List { node }; } } diff --git a/src/Umbraco.Core/Services/IContentServiceBase.cs b/src/Umbraco.Core/Services/IContentServiceBase.cs index 1c04e0b4a3..c40f49347f 100644 --- a/src/Umbraco.Core/Services/IContentServiceBase.cs +++ b/src/Umbraco.Core/Services/IContentServiceBase.cs @@ -1,4 +1,6 @@ -namespace Umbraco.Core.Services +using Umbraco.Core.Models; + +namespace Umbraco.Core.Services { /// /// Placeholder for sharing logic between the content, media (and member) services @@ -6,15 +8,9 @@ /// public interface IContentServiceBase : IService { - /// - /// Checks the data integrity of the node paths/levels stored in the database + /// Checks/fixes the data integrity of node paths/levels stored in the database /// - bool VerifyNodePaths(out int[] invalidIds); - - /// - /// Fixes the data integrity of node paths/levels stored in the database - /// - void FixNodePaths(); + ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options); } } diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index f5f3d03abb..1f384d694f 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -2383,23 +2383,13 @@ namespace Umbraco.Core.Services.Implement return OperationResult.Succeed(evtMsgs); } - public bool VerifyNodePaths(out int[] invalidIds) - { - using (var scope = ScopeProvider.CreateScope(autoComplete: true)) - { - scope.ReadLock(Constants.Locks.ContentTree); - return _documentRepository.VerifyNodePaths(out invalidIds); - } - } - - public void FixNodePaths() + public ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options) { using (var scope = ScopeProvider.CreateScope(autoComplete: true)) { scope.WriteLock(Constants.Locks.ContentTree); - _documentRepository.FixNodePaths(); - // TODO: We're going to have to clear all caches + return _documentRepository.CheckDataIntegrity(options); } } diff --git a/src/Umbraco.Core/Services/Implement/MediaService.cs b/src/Umbraco.Core/Services/Implement/MediaService.cs index 219beb7088..4a2b37ca31 100644 --- a/src/Umbraco.Core/Services/Implement/MediaService.cs +++ b/src/Umbraco.Core/Services/Implement/MediaService.cs @@ -1150,24 +1150,13 @@ namespace Umbraco.Core.Services.Implement } - - public bool VerifyNodePaths(out int[] invalidIds) - { - using (var scope = ScopeProvider.CreateScope(autoComplete: true)) - { - scope.ReadLock(Constants.Locks.MediaTree); - return _mediaRepository.VerifyNodePaths(out invalidIds); - } - } - - public void FixNodePaths() + public ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options) { using (var scope = ScopeProvider.CreateScope(autoComplete: true)) { scope.WriteLock(Constants.Locks.MediaTree); - _mediaRepository.FixNodePaths(); - // TODO: We're going to have to clear all caches + return _mediaRepository.CheckDataIntegrity(options); } } diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index efb76f2756..f15b098473 100755 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -132,6 +132,9 @@ + + + diff --git a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs index d7bf62067f..e40be33fff 100644 --- a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs +++ b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs @@ -3,13 +3,15 @@ using System.Collections.Generic; using System.Linq; using System.Text; using System.Threading.Tasks; +using Serilog.Core; +using Umbraco.Core.Models; using Umbraco.Core.Services; namespace Umbraco.Web.HealthCheck.Checks.Data { [HealthCheck( "73DD0C1C-E0CA-4C31-9564-1DCA509788AF", - "Database integrity check", + "Database data integrity check", Description = "Checks for various data integrity issues in the Umbraco database.", Group = "Data Integrity")] public class DatabaseIntegrityCheck : HealthCheck @@ -18,6 +20,8 @@ namespace Umbraco.Web.HealthCheck.Checks.Data private readonly IMediaService _mediaService; private const string _fixMediaPaths = "fixMediaPaths"; private const string _fixContentPaths = "fixContentPaths"; + private const string _fixMediaPathsTitle = "Fix media paths"; + private const string _fixContentPathsTitle = "Fix content paths"; public DatabaseIntegrityCheck(IContentService contentService, IMediaService mediaService) { @@ -34,35 +38,29 @@ namespace Umbraco.Web.HealthCheck.Checks.Data //return the statuses return new[] { - CheckContent(), - CheckMedia() + CheckDocuments(false), + CheckMedia(false) }; } - private HealthCheckStatus CheckMedia() + private HealthCheckStatus CheckMedia(bool fix) { - return CheckPaths(_fixMediaPaths, "Fix media paths", "media", () => - { - var mediaPaths = _mediaService.VerifyNodePaths(out var invalidMediaPaths); - return (mediaPaths, invalidMediaPaths); - }); + return CheckPaths(_fixMediaPaths, _fixMediaPathsTitle, Core.Constants.UdiEntityType.Media, + () => _mediaService.CheckDataIntegrity(new ContentDataIntegrityReportOptions {FixIssues = fix})); } - private HealthCheckStatus CheckContent() + private HealthCheckStatus CheckDocuments(bool fix) { - return CheckPaths(_fixContentPaths, "Fix content paths", "content", () => - { - var contentPaths = _contentService.VerifyNodePaths(out var invalidContentPaths); - return (contentPaths, invalidContentPaths); - }); + return CheckPaths(_fixContentPaths, _fixContentPathsTitle, Core.Constants.UdiEntityType.Document, + () => _contentService.CheckDataIntegrity(new ContentDataIntegrityReportOptions {FixIssues = fix})); } - private HealthCheckStatus CheckPaths(string actionAlias, string actionName, string entityType, Func<(bool success, int[] invalidPaths)> doCheck) + private HealthCheckStatus CheckPaths(string actionAlias, string actionName, string entityType, Func doCheck) { var result = doCheck(); var actions = new List(); - if (!result.success) + if (!result.Ok) { actions.Add(new HealthCheckAction(actionAlias, Id) { @@ -70,28 +68,23 @@ namespace Umbraco.Web.HealthCheck.Checks.Data }); } - return new HealthCheckStatus(result.success + return new HealthCheckStatus(result.Ok ? $"All {entityType} paths are valid" - : $"There are {result.invalidPaths.Length} invalid {entityType} paths") + : $"There are {result.DetectedIssues.Count} invalid {entityType} paths") { - ResultType = result.success ? StatusResultType.Success : StatusResultType.Error, + ResultType = result.Ok ? StatusResultType.Success : StatusResultType.Error, Actions = actions }; } public override HealthCheckStatus ExecuteAction(HealthCheckAction action) { - switch (action.Alias) + return action.Alias switch { - case _fixContentPaths: - _contentService.FixNodePaths(); - return CheckContent(); - case _fixMediaPaths: - _mediaService.FixNodePaths(); - return CheckMedia(); - default: - throw new InvalidOperationException("Action not supported"); - } + _fixContentPaths => CheckDocuments(true), + _fixMediaPaths => CheckMedia(true), + _ => throw new InvalidOperationException("Action not supported") + }; } } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 474c390027..e40e7130d0 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -525,7 +525,7 @@ namespace Umbraco.Web.PublishedCache.NuCache // because the data sort operation is by path. if (parent.Value == null) { - _logger.Warn($"Skip item id={kit.Node.Id}, no Data assigned for linked node with path {kit.Node.Path} and parent id {kit.Node.ParentContentId}. This can indicate data corruption for the Path value for node {kit.Node.Id}."); + _logger.Warn($"Skip item id={kit.Node.Id}, no Data assigned for linked node with path {kit.Node.Path} and parent id {kit.Node.ParentContentId}. This can indicate data corruption for the Path value for node {kit.Node.Id}. See the Health Check dashboard in Settings to resolve data integrity issues."); return false; } From 3ef188376517a8afc024d2e4b39170fe351013fd Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Apr 2020 16:53:54 +1000 Subject: [PATCH 10/39] fix build (non latest c#) --- .../Implement/ContentRepositoryBase.cs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index 42d7e67bc0..93c4078f46 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -580,15 +580,14 @@ namespace Umbraco.Core.Persistence.Repositories.Implement } return new ContentDataIntegrityReport(report); + } - // inline method used to append nodes to rebuild - static void AppendNodeToFix(IDictionary> nodesToRebuild, NodeDto node) - { - if (nodesToRebuild.TryGetValue(node.ParentId, out var childIds)) - childIds.Add(node); - else - nodesToRebuild[node.ParentId] = new List { node }; - } + private static void AppendNodeToFix(IDictionary> nodesToRebuild, NodeDto node) + { + if (nodesToRebuild.TryGetValue(node.ParentId, out var childIds)) + childIds.Add(node); + else + nodesToRebuild[node.ParentId] = new List { node }; } // here, filter can be null and ordering cannot From 19f0793cba726ce03acea4da15ed1f4ff59bc09c Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Apr 2020 17:38:40 +1000 Subject: [PATCH 11/39] updates health check --- .../Implement/ContentRepositoryBase.cs | 12 +++--- .../Checks/Data/DatabaseIntegrityCheck.cs | 38 +++++++++++++++---- 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index 93c4078f46..9999e9f2d6 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -489,7 +489,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement var nodesToRebuild = new Dictionary>(); var validNodes = new Dictionary(); - var currentParentIds = new HashSet { -1 }; + var rootIds = new[] {Constants.System.Root, Constants.System.RecycleBinContent, Constants.System.RecycleBinMedia}; + var currentParentIds = new HashSet(rootIds); var prevParentIds = currentParentIds; var lastLevel = -1; @@ -512,7 +513,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement currentParentIds.Add(node.NodeId); - var pathParts = node.Path.Split(','); + // paths parts without the roots + var pathParts = node.Path.Split(',').Where(x => !rootIds.Contains(int.Parse(x))).ToArray(); if (!prevParentIds.Contains(node.ParentId)) { @@ -520,13 +522,13 @@ namespace Umbraco.Core.Persistence.Repositories.Implement report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathAndLevelByParentId)); AppendNodeToFix(nodesToRebuild, node); } - else if (pathParts.Length < 2) + else if (pathParts.Length == 0) { // invalid path report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathEmpty)); AppendNodeToFix(nodesToRebuild, node); } - else if (pathParts.Length - 1 != node.Level) + else if (pathParts.Length != node.Level) { // invalid, either path or level is wrong report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathLevelMismatch)); @@ -538,7 +540,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathById)); AppendNodeToFix(nodesToRebuild, node); } - else if (pathParts[pathParts.Length - 2] != node.ParentId.ToString()) + else if (!rootIds.Contains(node.ParentId) && pathParts[pathParts.Length - 2] != node.ParentId.ToString()) { // invalid path report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathByParentId)); diff --git a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs index e40be33fff..876ad9f1d7 100644 --- a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs +++ b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs @@ -45,22 +45,22 @@ namespace Umbraco.Web.HealthCheck.Checks.Data private HealthCheckStatus CheckMedia(bool fix) { - return CheckPaths(_fixMediaPaths, _fixMediaPathsTitle, Core.Constants.UdiEntityType.Media, + return CheckPaths(_fixMediaPaths, _fixMediaPathsTitle, Core.Constants.UdiEntityType.Media, fix, () => _mediaService.CheckDataIntegrity(new ContentDataIntegrityReportOptions {FixIssues = fix})); } private HealthCheckStatus CheckDocuments(bool fix) { - return CheckPaths(_fixContentPaths, _fixContentPathsTitle, Core.Constants.UdiEntityType.Document, + return CheckPaths(_fixContentPaths, _fixContentPathsTitle, Core.Constants.UdiEntityType.Document, fix, () => _contentService.CheckDataIntegrity(new ContentDataIntegrityReportOptions {FixIssues = fix})); } - private HealthCheckStatus CheckPaths(string actionAlias, string actionName, string entityType, Func doCheck) + private HealthCheckStatus CheckPaths(string actionAlias, string actionName, string entityType, bool detailedReport, Func doCheck) { - var result = doCheck(); + var report = doCheck(); var actions = new List(); - if (!result.Ok) + if (!report.Ok) { actions.Add(new HealthCheckAction(actionAlias, Id) { @@ -68,15 +68,37 @@ namespace Umbraco.Web.HealthCheck.Checks.Data }); } - return new HealthCheckStatus(result.Ok + return new HealthCheckStatus(report.Ok ? $"All {entityType} paths are valid" - : $"There are {result.DetectedIssues.Count} invalid {entityType} paths") + : GetInvalidReport(report, entityType, detailedReport)) { - ResultType = result.Ok ? StatusResultType.Success : StatusResultType.Error, + ResultType = report.Ok ? StatusResultType.Success : StatusResultType.Error, Actions = actions }; } + private static string GetInvalidReport(ContentDataIntegrityReport report, string entityType, bool detailed) + { + var sb = new StringBuilder(); + sb.AppendLine($"There are {report.DetectedIssues.Count} invalid {entityType} paths"); + + if (true && report.DetectedIssues.Count > 0) + { + sb.AppendLine("
    "); + foreach (var issueGroup in report.DetectedIssues.GroupBy(x => x.Value.IssueType)) + { + var countByGroup = issueGroup.Count(); + var fixedByGroup = issueGroup.Count(x => x.Value.Fixed); + sb.AppendLine("
  • "); + sb.AppendLine($"{countByGroup} issues of type {issueGroup.Key} ... {fixedByGroup} fixed"); + sb.AppendLine("
    • "); + } + sb.AppendLine("
"); + } + + return sb.ToString(); + } + public override HealthCheckStatus ExecuteAction(HealthCheckAction action) { return action.Alias switch From 49c6fd2c76f76f09f7f14868ad827a67a1af6c3d Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 8 Apr 2020 10:38:02 +1000 Subject: [PATCH 12/39] data integrity report working --- .../Models/ContentDataIntegrityReport.cs | 3 ++- .../Implement/ContentRepositoryBase.cs | 2 ++ .../Checks/Data/DatabaseIntegrityCheck.cs | 21 +++++++++++++------ 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs b/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs index 60683ee72f..d3660679a7 100644 --- a/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs +++ b/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using System.Linq; namespace Umbraco.Core.Models { @@ -9,7 +10,7 @@ namespace Umbraco.Core.Models DetectedIssues = detectedIssues; } - public bool Ok => DetectedIssues.Count == 0; + public bool Ok => DetectedIssues.Count == 0 || DetectedIssues.Count == DetectedIssues.Values.Count(x => x.Fixed); public IReadOnlyDictionary DetectedIssues { get; } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index 9999e9f2d6..4aa9655249 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -578,6 +578,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement foreach (var node in updated) { Database.Update(node); + if (report.TryGetValue(node.NodeId, out var entry)) + entry.Fixed = true; } } diff --git a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs index 876ad9f1d7..b7d9aea3b9 100644 --- a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs +++ b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs @@ -68,21 +68,30 @@ namespace Umbraco.Web.HealthCheck.Checks.Data }); } - return new HealthCheckStatus(report.Ok - ? $"All {entityType} paths are valid" - : GetInvalidReport(report, entityType, detailedReport)) + return new HealthCheckStatus(GetReport(report, entityType, detailedReport)) { ResultType = report.Ok ? StatusResultType.Success : StatusResultType.Error, Actions = actions }; } - private static string GetInvalidReport(ContentDataIntegrityReport report, string entityType, bool detailed) + private static string GetReport(ContentDataIntegrityReport report, string entityType, bool detailed) { var sb = new StringBuilder(); - sb.AppendLine($"There are {report.DetectedIssues.Count} invalid {entityType} paths"); - if (true && report.DetectedIssues.Count > 0) + if (report.Ok) + { + sb.AppendLine($"

All {entityType} paths are valid

"); + + if (!detailed) + return sb.ToString(); + } + else + { + sb.AppendLine($"

{report.DetectedIssues.Count} invalid {entityType} paths detected.

"); + } + + if (detailed && report.DetectedIssues.Count > 0) { sb.AppendLine("
    "); foreach (var issueGroup in report.DetectedIssues.GroupBy(x => x.Value.IssueType)) From d54eed5cb9396d23094bf712458e23588703f341 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 8 Apr 2020 10:54:48 +1000 Subject: [PATCH 13/39] argh! can't use switch expressions in v8 --- .../Checks/Data/DatabaseIntegrityCheck.cs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs index b7d9aea3b9..0c3e2f3d91 100644 --- a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs +++ b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs @@ -110,12 +110,15 @@ namespace Umbraco.Web.HealthCheck.Checks.Data public override HealthCheckStatus ExecuteAction(HealthCheckAction action) { - return action.Alias switch + switch (action.Alias) { - _fixContentPaths => CheckDocuments(true), - _fixMediaPaths => CheckMedia(true), - _ => throw new InvalidOperationException("Action not supported") - }; + case _fixContentPaths: + return CheckDocuments(true); + case _fixMediaPaths: + return CheckMedia(true); + default: + throw new InvalidOperationException("Action not supported"); + } } } } From 36c9cd47a106a14b1944f708ae96ea933aa6181a Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 8 Apr 2020 11:22:15 +1000 Subject: [PATCH 14/39] Adds notes --- .../Repositories/Implement/DocumentRepository.cs | 4 ++++ src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs | 9 +++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs index a75165a7b9..db1e2b350d 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs @@ -538,6 +538,10 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // In this case we can bypass a lot of the below operations which will make this whole operation go much faster. // When moving we don't need to create new versions, etc... because we cannot roll this operation back anyways. var isMoving = entity.IsMoving(); + // TODO: I'm sure we can also detect a "Copy" (of a descendant) operation and probably perform similar checks below. + // There is probably more stuff that would be required for copying but I'm sure not all of this logic would be, we could more than likely boost + // copy performance by 95% just like we did for Move + var publishing = entity.PublishedState == PublishedState.Publishing; diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index e40e7130d0..b5fc1abd7f 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -817,7 +817,7 @@ namespace Umbraco.Web.PublishedCache.NuCache { //this zero's out the branch (recursively), if we're in a new gen this will add a NULL placeholder for the gen ClearBranchLocked(existing); - //TODO: This removes the current GEN from the tree - do we really want to do that? + //TODO: This removes the current GEN from the tree - do we really want to do that? (not sure if this is still an issue....) RemoveTreeNodeLocked(existing); } @@ -882,6 +882,10 @@ namespace Umbraco.Web.PublishedCache.NuCache private void ClearBranchLocked(ContentNode content) { + // This should never be null, all code that calls this method is null checking but we've seen + // issues of null ref exceptions in issue reports so we'll double check here + if (content == null) throw new ArgumentNullException(nameof(content)); + SetValueLocked(_contentNodes, content.Id, null); if (_localDb != null) RegisterChange(content.Id, ContentNodeKit.Null); @@ -890,8 +894,9 @@ namespace Umbraco.Web.PublishedCache.NuCache var id = content.FirstChildContentId; while (id > 0) { + // get the required link node, this ensures that both `link` and `link.Value` are not null var link = GetRequiredLinkedNode(id, "child", null); - ClearBranchLocked(link.Value); + ClearBranchLocked(link.Value); // recurse id = link.Value.NextSiblingContentId; } } From 64e7a29795cd62bf5253ab997250a510a84f07bc Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 8 Apr 2020 15:03:21 +1000 Subject: [PATCH 15/39] Ensures caches are refreshed after data errors are fixed --- .../Models/ContentDataIntegrityReport.cs | 3 +++ .../Services/Implement/ContentService.cs | 13 +++++++++++-- src/Umbraco.Core/Services/Implement/MediaService.cs | 13 +++++++++++-- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs b/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs index d3660679a7..9f0f147083 100644 --- a/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs +++ b/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs @@ -14,6 +14,9 @@ namespace Umbraco.Core.Models public IReadOnlyDictionary DetectedIssues { get; } + public IReadOnlyDictionary FixedIssues + => DetectedIssues.Where(x => x.Value.Fixed).ToDictionary(x => x.Key, x => x.Value); + public enum IssueType { /// diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index 1f384d694f..068864a558 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -2388,8 +2388,17 @@ namespace Umbraco.Core.Services.Implement using (var scope = ScopeProvider.CreateScope(autoComplete: true)) { scope.WriteLock(Constants.Locks.ContentTree); - // TODO: We're going to have to clear all caches - return _documentRepository.CheckDataIntegrity(options); + + var report = _documentRepository.CheckDataIntegrity(options); + + if (report.FixedIssues.Count > 0) + { + //The event args needs a content item so we'll make a fake one with enough properties to not cause a null ref + var root = new Content("root", -1, new ContentType(-1)) {Id = -1, Key = Guid.Empty}; + scope.Events.Dispatch(TreeChanged, this, new TreeChange.EventArgs(new TreeChange(root, TreeChangeTypes.RefreshAll))); + } + + return report; } } diff --git a/src/Umbraco.Core/Services/Implement/MediaService.cs b/src/Umbraco.Core/Services/Implement/MediaService.cs index 4a2b37ca31..ecd4cccc8d 100644 --- a/src/Umbraco.Core/Services/Implement/MediaService.cs +++ b/src/Umbraco.Core/Services/Implement/MediaService.cs @@ -1155,8 +1155,17 @@ namespace Umbraco.Core.Services.Implement using (var scope = ScopeProvider.CreateScope(autoComplete: true)) { scope.WriteLock(Constants.Locks.MediaTree); - // TODO: We're going to have to clear all caches - return _mediaRepository.CheckDataIntegrity(options); + + var report = _mediaRepository.CheckDataIntegrity(options); + + if (report.FixedIssues.Count > 0) + { + //The event args needs a content item so we'll make a fake one with enough properties to not cause a null ref + var root = new Models.Media("root", -1, new MediaType(-1)) { Id = -1, Key = Guid.Empty }; + scope.Events.Dispatch(TreeChanged, this, new TreeChange.EventArgs(new TreeChange(root, TreeChangeTypes.RefreshAll))); + } + + return report; } } From 188d868f553e7938bf5d5f691e0c877e57ec9091 Mon Sep 17 00:00:00 2001 From: Rachel Breeze Date: Sat, 11 Apr 2020 14:45:14 +0100 Subject: [PATCH 16/39] Updated tinyMCE to 4.9.9 --- src/Umbraco.Web.UI.Client/bower.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Web.UI.Client/bower.json b/src/Umbraco.Web.UI.Client/bower.json index b173909c63..ab438e1d8d 100644 --- a/src/Umbraco.Web.UI.Client/bower.json +++ b/src/Umbraco.Web.UI.Client/bower.json @@ -25,7 +25,7 @@ "jquery-migrate": "1.4.0", "angular-dynamic-locale": "0.1.28", "ng-file-upload": "~7.3.8", - "tinymce": "~4.9.4", + "tinymce": "~4.9.9", "codemirror": "~5.3.0", "angular-local-storage": "~0.2.3", "moment": "~2.10.3", From 3b03d812f87a332b7ea61be9c1289665fc04ff6b Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 15 Apr 2020 11:55:12 +1000 Subject: [PATCH 17/39] fixes paging --- .../Persistence/Repositories/ContentRepository.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs index 24c9e84873..11b56c5bf0 100644 --- a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs @@ -862,21 +862,21 @@ order by (umbracoNode.{2}), (umbracoNode.parentID), (umbracoNode.sortOrder)", XmlElement last = null; - long pageSize = 500; + const long pageSize = 500; int? itemCount = null; - long currPage = 0; + long pageIndex = 0; do { // Get the paged queries - Database.BuildPageQueries(currPage, pageSize, sql, ref args, out var sqlCount, out var sqlPage); + Database.BuildPageQueries(pageIndex * pageSize, pageSize, sql, ref args, out var sqlCount, out var sqlPage); // get the item count once if (itemCount == null) { itemCount = Database.ExecuteScalar(sqlCount, args); } - currPage++; + pageIndex++; // iterate over rows without allocating all items to memory (Query vs Fetch) foreach (var row in Database.Query(sqlPage, args)) @@ -906,7 +906,7 @@ order by (umbracoNode.{2}), (umbracoNode.parentID), (umbracoNode.sortOrder)", last.Attributes["sortOrder"].Value = sortOrder.ToInvariantString(); } - } while (itemCount == pageSize); + } while ((pageIndex * pageSize) < itemCount); return xmlDoc; From 61c39eea64c9fe7a1de4a9cb38fb89834dc754df Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 15 Apr 2020 12:49:38 +1000 Subject: [PATCH 18/39] Nicer sql template constants --- src/Umbraco.Core/Constants-SqlTemplates.cs | 17 ++++++++++------- .../Implement/ContentRepositoryBase.cs | 14 +++++++------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/Umbraco.Core/Constants-SqlTemplates.cs b/src/Umbraco.Core/Constants-SqlTemplates.cs index 33c94f0f93..984bc495b0 100644 --- a/src/Umbraco.Core/Constants-SqlTemplates.cs +++ b/src/Umbraco.Core/Constants-SqlTemplates.cs @@ -4,13 +4,16 @@ { public static class SqlTemplates { - public const string VersionableRepositoryGetVersionIds = "Umbraco.Core.VersionableRepository.GetVersionIds"; - public const string VersionableRepositoryGetVersion = "Umbraco.Core.VersionableRepository.GetVersion"; - public const string VersionableRepositoryGetVersions = "Umbraco.Core.VersionableRepository.GetVersions"; - public const string VersionableRepositoryEnsureUniqueNodeName = "Umbraco.Core.VersionableRepository.EnsureUniqueNodeName"; - public const string VersionableRepositoryGetSortOrder = "Umbraco.Core.VersionableRepository.GetSortOrder"; - public const string VersionableRepositoryGetParentNode = "Umbraco.Core.VersionableRepository.GetParentNode"; - public const string VersionableRepositoryGetReservedId = "Umbraco.Core.VersionableRepository.GetReservedId"; + public static class VersionableRepository + { + public const string GetVersionIds = "Umbraco.Core.VersionableRepository.GetVersionIds"; + public const string GetVersion = "Umbraco.Core.VersionableRepository.GetVersion"; + public const string GetVersions = "Umbraco.Core.VersionableRepository.GetVersions"; + public const string EnsureUniqueNodeName = "Umbraco.Core.VersionableRepository.EnsureUniqueNodeName"; + public const string GetSortOrder = "Umbraco.Core.VersionableRepository.GetSortOrder"; + public const string GetParentNode = "Umbraco.Core.VersionableRepository.GetParentNode"; + public const string GetReservedId = "Umbraco.Core.VersionableRepository.GetReservedId"; + } } } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index 4aa9655249..845006891d 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -83,7 +83,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // gets all version ids, current first public virtual IEnumerable GetVersionIds(int nodeId, int maxRows) { - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetVersionIds, tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.GetVersionIds, tsql => tsql.Select(x => x.Id) .From() .Where(x => x.NodeId == SqlTemplate.Arg("nodeId")) @@ -99,7 +99,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // TODO: test object node type? // get the version we want to delete - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetVersion, tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.GetVersion, tsql => tsql.Select().From().Where(x => x.Id == SqlTemplate.Arg("versionId")) ); var versionDto = Database.Fetch(template.Sql(new { versionId })).FirstOrDefault(); @@ -121,7 +121,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // TODO: test object node type? // get the versions we want to delete, excluding the current one - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetVersions, tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.GetVersions, tsql => tsql.Select().From().Where(x => x.NodeId == SqlTemplate.Arg("nodeId") && !x.Current && x.VersionDate < SqlTemplate.Arg("versionDate")) ); var versionDtos = Database.Fetch(template.Sql(new { nodeId, versionDate })); @@ -887,7 +887,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual string EnsureUniqueNodeName(int parentId, string nodeName, int id = 0) { - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryEnsureUniqueNodeName, tsql => tsql + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.EnsureUniqueNodeName, tsql => tsql .Select(x => Alias(x.NodeId, "id"), x => Alias(x.Text, "name")) .From() .Where(x => x.NodeObjectType == SqlTemplate.Arg("nodeObjectType") && x.ParentId == SqlTemplate.Arg("parentId"))); @@ -900,7 +900,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual int GetNewChildSortOrder(int parentId, int first) { - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetSortOrder, tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.GetSortOrder, tsql => tsql.Select($"COALESCE(MAX(sortOrder),{first - 1})").From().Where(x => x.ParentId == SqlTemplate.Arg("parentId") && x.NodeObjectType == NodeObjectTypeId) ); @@ -909,7 +909,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual NodeDto GetParentNodeDto(int parentId) { - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetParentNode, tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.GetParentNode, tsql => tsql.Select().From().Where(x => x.NodeId == SqlTemplate.Arg("parentId")) ); @@ -918,7 +918,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual int GetReservedId(Guid uniqueId) { - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetReservedId, tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.GetReservedId, tsql => tsql.Select(x => x.NodeId).From().Where(x => x.UniqueId == SqlTemplate.Arg("uniqueId") && x.NodeObjectType == Constants.ObjectTypes.IdReservation) ); var id = Database.ExecuteScalar(template.Sql(new { uniqueId = uniqueId })); From 6cd7e5d05558bb30a33434c685d43a1982a9a7a5 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 16 Apr 2020 22:04:27 +1000 Subject: [PATCH 19/39] removes the extra unneeded variable --- .../Repositories/Implement/MediaRepository.cs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs index b9ef2d4086..242f21c749 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs @@ -219,7 +219,6 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected override void PersistNewItem(IMedia entity) { - var media = entity; entity.AddingEntity(); // ensure unique name on the same level @@ -274,15 +273,15 @@ namespace Umbraco.Core.Persistence.Repositories.Implement contentVersionDto.NodeId = nodeDto.NodeId; contentVersionDto.Current = true; Database.Insert(contentVersionDto); - media.VersionId = contentVersionDto.Id; + entity.VersionId = contentVersionDto.Id; // persist the media version dto var mediaVersionDto = dto.MediaVersionDto; - mediaVersionDto.Id = media.VersionId; + mediaVersionDto.Id = entity.VersionId; Database.Insert(mediaVersionDto); // persist the property data - var propertyDataDtos = PropertyFactory.BuildDtos(media.ContentType.Variations, media.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); + var propertyDataDtos = PropertyFactory.BuildDtos(entity.ContentType.Variations, entity.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); foreach (var propertyDataDto in propertyDataDtos) Database.Insert(propertyDataDto); @@ -298,10 +297,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected override void PersistUpdatedItem(IMedia entity) { - var media = entity; - // update - media.UpdatingEntity(); + entity.UpdatingEntity(); // Check if this entity is being moved as a descendant as part of a bulk moving operations. // In this case we can bypass a lot of the below operations which will make this whole operation go much faster. @@ -349,9 +346,9 @@ namespace Umbraco.Core.Persistence.Repositories.Implement Database.Update(mediaVersionDto); // replace the property data - var deletePropertyDataSql = SqlContext.Sql().Delete().Where(x => x.VersionId == media.VersionId); + var deletePropertyDataSql = SqlContext.Sql().Delete().Where(x => x.VersionId == entity.VersionId); Database.Execute(deletePropertyDataSql); - var propertyDataDtos = PropertyFactory.BuildDtos(media.ContentType.Variations, media.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); + var propertyDataDtos = PropertyFactory.BuildDtos(entity.ContentType.Variations, entity.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); foreach (var propertyDataDto in propertyDataDtos) Database.Insert(propertyDataDto); From d5e2c12685e46e55b360c385b97eb424855b54ed Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 17 Apr 2020 13:20:57 +1000 Subject: [PATCH 20/39] fixes merge issue --- src/Umbraco.Web.UI/Umbraco.Web.UI.csproj | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj b/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj index 57a71a2cf0..1d77505fc6 100644 --- a/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj +++ b/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj @@ -347,10 +347,7 @@ True 8700 / - http://localhost:8700 - 8610 - / - http://localhost:8610 + http://localhost:8700 False False From 9335f39495f5a4a461cdd5570b074058ee07f61b Mon Sep 17 00:00:00 2001 From: Warren Buckley Date: Sat, 18 Apr 2020 22:01:39 +0100 Subject: [PATCH 21/39] Update Readme.md in netcore branch to see if LGTM.com will analyze C# --- .github/README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/README.md b/.github/README.md index 467ca6e5e6..d4565a8cb5 100644 --- a/.github/README.md +++ b/.github/README.md @@ -37,4 +37,3 @@ Besides "Our", we all support each other also via Twitter: [Umbraco HQ](https:// ## Contributing Umbraco is contribution-focused and community-driven. If you want to contribute back to the Umbraco source code, please check out our [guide to contributing](CONTRIBUTING.md). - From c0ec1bf6ca9c12b6417dcf28c3f929eaed0012fa Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Mon, 20 Apr 2020 06:19:59 +0200 Subject: [PATCH 22/39] Introduced GenericDictionaryRequestAppCache, to use the HttpContext.Items from .NET Core --- .../Cache/GenericDictionaryRequestAppCache.cs | 190 ++++++++++++++++++ .../Runtime/CoreRuntime.cs | 27 ++- .../Runtime/WebRuntime.cs | 90 --------- src/Umbraco.Tests.Integration/RuntimeTests.cs | 8 +- .../Testing/UmbracoIntegrationTest.cs | 2 +- .../Routing/RenderRouteHandlerTests.cs | 6 +- .../Runtimes/CoreRuntimeTests.cs | 3 +- src/Umbraco.Tests/Runtimes/StandaloneTests.cs | 10 +- .../UmbracoCoreServiceCollectionExtensions.cs | 62 ++++-- src/Umbraco.Web/UmbracoApplication.cs | 13 +- 10 files changed, 280 insertions(+), 131 deletions(-) create mode 100644 src/Umbraco.Core/Cache/GenericDictionaryRequestAppCache.cs delete mode 100644 src/Umbraco.Infrastructure/Runtime/WebRuntime.cs diff --git a/src/Umbraco.Core/Cache/GenericDictionaryRequestAppCache.cs b/src/Umbraco.Core/Cache/GenericDictionaryRequestAppCache.cs new file mode 100644 index 0000000000..193235ca7e --- /dev/null +++ b/src/Umbraco.Core/Cache/GenericDictionaryRequestAppCache.cs @@ -0,0 +1,190 @@ +using System; +using System.Collections; +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using Umbraco.Core.Composing; + +namespace Umbraco.Core.Cache +{ + /// + /// Implements a fast on top of HttpContext.Items. + /// + /// + /// If no current HttpContext items can be found (no current HttpContext, + /// or no Items...) then this cache acts as a pass-through and does not cache + /// anything. + /// + public class GenericDictionaryRequestAppCache : FastDictionaryAppCacheBase, IRequestCache + { + /// + /// Initializes a new instance of the class with a context, for unit tests! + /// + public GenericDictionaryRequestAppCache(Func> requestItems) : base() + { + ContextItems = requestItems; + } + + private Func> ContextItems { get; } + + public bool IsAvailable => TryGetContextItems(out _); + + private bool TryGetContextItems(out IDictionary items) + { + items = ContextItems?.Invoke(); + return items != null; + } + + /// + public override object Get(string key, Func factory) + { + //no place to cache so just return the callback result + if (!TryGetContextItems(out var items)) return factory(); + + key = GetCacheKey(key); + + Lazy result; + + try + { + EnterWriteLock(); + result = items[key] as Lazy; // null if key not found + + // cannot create value within the lock, so if result.IsValueCreated is false, just + // do nothing here - means that if creation throws, a race condition could cause + // more than one thread to reach the return statement below and throw - accepted. + + if (result == null || SafeLazy.GetSafeLazyValue(result, true) == null) // get non-created as NonCreatedValue & exceptions as null + { + result = SafeLazy.GetSafeLazy(factory); + items[key] = result; + } + } + finally + { + ExitWriteLock(); + } + + // using GetSafeLazy and GetSafeLazyValue ensures that we don't cache + // exceptions (but try again and again) and silently eat them - however at + // some point we have to report them - so need to re-throw here + + // this does not throw anymore + //return result.Value; + + var value = result.Value; // will not throw (safe lazy) + if (value is SafeLazy.ExceptionHolder eh) eh.Exception.Throw(); // throw once! + return value; + } + + public bool Set(string key, object value) + { + //no place to cache so just return the callback result + if (!TryGetContextItems(out var items)) return false; + key = GetCacheKey(key); + try + { + + EnterWriteLock(); + items[key] = SafeLazy.GetSafeLazy(() => value); + } + finally + { + ExitWriteLock(); + } + return true; + } + + public bool Remove(string key) + { + //no place to cache so just return the callback result + if (!TryGetContextItems(out var items)) return false; + key = GetCacheKey(key); + try + { + + EnterWriteLock(); + items.Remove(key); + } + finally + { + ExitWriteLock(); + } + return true; + } + + #region Entries + + protected override IEnumerable GetDictionaryEntries() + { + const string prefix = CacheItemPrefix + "-"; + + if (!TryGetContextItems(out var items)) return Enumerable.Empty(); + + return items.Cast() + .Where(x => x.Key is string s && s.StartsWith(prefix)); + } + + protected override void RemoveEntry(string key) + { + if (!TryGetContextItems(out var items)) return; + + items.Remove(key); + } + + protected override object GetEntry(string key) + { + return !TryGetContextItems(out var items) ? null : items[key]; + } + + #endregion + + #region Lock + + private const string ContextItemsLockKey = "Umbraco.Core.Cache.HttpRequestCache::LockEntered"; + + protected override void EnterReadLock() => EnterWriteLock(); + + protected override void EnterWriteLock() + { + if (!TryGetContextItems(out var items)) return; + + // note: cannot keep 'entered' as a class variable here, + // since there is one per request - so storing it within + // ContextItems - which is locked, so this should be safe + + var entered = false; + Monitor.Enter(items, ref entered); + items[ContextItemsLockKey] = entered; + } + + protected override void ExitReadLock() => ExitWriteLock(); + + protected override void ExitWriteLock() + { + if (!TryGetContextItems(out var items)) return; + + var entered = (bool?)items[ContextItemsLockKey] ?? false; + if (entered) + Monitor.Exit(items); + items.Remove(ContextItemsLockKey); + } + + #endregion + + public IEnumerator> GetEnumerator() + { + if (!TryGetContextItems(out var items)) + { + yield break; + } + + foreach (var item in items) + { + yield return new KeyValuePair(item.Key.ToString(), item.Value); + } + } + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } +} diff --git a/src/Umbraco.Infrastructure/Runtime/CoreRuntime.cs b/src/Umbraco.Infrastructure/Runtime/CoreRuntime.cs index 92e47771a6..1aa94ff2a9 100644 --- a/src/Umbraco.Infrastructure/Runtime/CoreRuntime.cs +++ b/src/Umbraco.Infrastructure/Runtime/CoreRuntime.cs @@ -25,6 +25,7 @@ namespace Umbraco.Core.Runtime private IFactory _factory; private readonly RuntimeState _state; private readonly IUmbracoBootPermissionChecker _umbracoBootPermissionChecker; + private readonly IRequestCache _requestCache; private readonly IGlobalSettings _globalSettings; private readonly IConnectionStrings _connectionStrings; @@ -39,7 +40,8 @@ namespace Umbraco.Core.Runtime IBackOfficeInfo backOfficeInfo, IDbProviderFactoryCreator dbProviderFactoryCreator, IMainDom mainDom, - ITypeFinder typeFinder) + ITypeFinder typeFinder, + IRequestCache requestCache) { IOHelper = ioHelper; Configs = configs; @@ -50,6 +52,7 @@ namespace Umbraco.Core.Runtime DbProviderFactoryCreator = dbProviderFactoryCreator; _umbracoBootPermissionChecker = umbracoBootPermissionChecker; + _requestCache = requestCache; Logger = logger; MainDom = mainDom; @@ -110,6 +113,7 @@ namespace Umbraco.Core.Runtime { if (register is null) throw new ArgumentNullException(nameof(register)); + // create and register the essential services // ie the bare minimum required to boot @@ -129,12 +133,25 @@ namespace Umbraco.Core.Runtime "Booted.", "Boot failed.")) { - Logger.Info("Booting Core"); + + Logger.Info("Booting site '{HostingSiteName}', app '{HostingApplicationId}', path '{HostingPhysicalPath}', server '{MachineName}'.", + HostingEnvironment?.SiteName, + HostingEnvironment?.ApplicationId, + HostingEnvironment?.ApplicationPhysicalPath, + NetworkHelper.MachineName); Logger.Debug("Runtime: {Runtime}", GetType().FullName); // application environment ConfigureUnhandledException(); - return _factory = Configure(register, timer); + _factory = Configure(register, timer); + + // now (and only now) is the time to switch over to perWebRequest scopes. + // up until that point we may not have a request, and scoped services would + // fail to resolve - but we run Initialize within a factory scope - and then, + // here, we switch the factory to bind scopes to requests + _factory.EnablePerWebRequestScope(); + + return _factory; } } @@ -151,7 +168,7 @@ namespace Umbraco.Core.Runtime try { - + // run handlers RuntimeOptions.DoRuntimeBoot(ProfilingLogger); @@ -350,7 +367,7 @@ namespace Umbraco.Core.Runtime return new AppCaches( new DeepCloneAppCache(new ObjectCacheAppCache()), - NoAppCache.Instance, + _requestCache, new IsolatedCaches(type => new DeepCloneAppCache(new ObjectCacheAppCache()))); } diff --git a/src/Umbraco.Infrastructure/Runtime/WebRuntime.cs b/src/Umbraco.Infrastructure/Runtime/WebRuntime.cs deleted file mode 100644 index fc2a019023..0000000000 --- a/src/Umbraco.Infrastructure/Runtime/WebRuntime.cs +++ /dev/null @@ -1,90 +0,0 @@ -using Umbraco.Core; -using Umbraco.Core.Cache; -using Umbraco.Core.Composing; -using Umbraco.Core.Configuration; -using Umbraco.Core.Hosting; -using Umbraco.Core.IO; -using Umbraco.Core.Logging; -using Umbraco.Core.Persistence; -using Umbraco.Core.Runtime; - -namespace Umbraco.Web.Runtime -{ - /// - /// Represents the Web Umbraco runtime. - /// - /// On top of CoreRuntime, handles all of the web-related runtime aspects of Umbraco. - public class WebRuntime : CoreRuntime - { - private readonly IRequestCache _requestCache; - - /// - /// Initializes a new instance of the class. - /// - public WebRuntime( - Configs configs, - IUmbracoVersion umbracoVersion, - IIOHelper ioHelper, - ILogger logger, - IProfiler profiler, - IHostingEnvironment hostingEnvironment, - IBackOfficeInfo backOfficeInfo, - IDbProviderFactoryCreator dbProviderFactoryCreator, - IMainDom mainDom, - ITypeFinder typeFinder, - IRequestCache requestCache, - IUmbracoBootPermissionChecker umbracoBootPermissionChecker): - base(configs, umbracoVersion, ioHelper, logger, profiler ,umbracoBootPermissionChecker, hostingEnvironment, backOfficeInfo, dbProviderFactoryCreator, mainDom, typeFinder) - { - _requestCache = requestCache; - } - - /// - public override IFactory Configure(IRegister register) - { - - var profilingLogger = new ProfilingLogger(Logger, Profiler); - var umbracoVersion = new UmbracoVersion(); - using (var timer = profilingLogger.TraceDuration( - $"Booting Umbraco {umbracoVersion.SemanticVersion.ToSemanticString()}.", - "Booted.", - "Boot failed.")) - { - Logger.Info("Booting site '{HostingSiteName}', app '{HostingApplicationId}', path '{HostingPhysicalPath}', server '{MachineName}'.", - HostingEnvironment.SiteName, - HostingEnvironment.ApplicationId, - HostingEnvironment.ApplicationPhysicalPath, - NetworkHelper.MachineName); - Logger.Debug("Runtime: {Runtime}", GetType().FullName); - - var factory = base.Configure(register); - - // now (and only now) is the time to switch over to perWebRequest scopes. - // up until that point we may not have a request, and scoped services would - // fail to resolve - but we run Initialize within a factory scope - and then, - // here, we switch the factory to bind scopes to requests - factory.EnablePerWebRequestScope(); - - return factory; - } - - - } - - #region Getters - - protected override AppCaches GetAppCaches() => new AppCaches( - // we need to have the dep clone runtime cache provider to ensure - // all entities are cached properly (cloned in and cloned out) - new DeepCloneAppCache(new ObjectCacheAppCache()), - // we need request based cache when running in web-based context - _requestCache, - new IsolatedCaches(type => - // we need to have the dep clone runtime cache provider to ensure - // all entities are cached properly (cloned in and cloned out) - new DeepCloneAppCache(new ObjectCacheAppCache()))); - - #endregion - } -} - diff --git a/src/Umbraco.Tests.Integration/RuntimeTests.cs b/src/Umbraco.Tests.Integration/RuntimeTests.cs index 52c29d2037..aa364210d0 100644 --- a/src/Umbraco.Tests.Integration/RuntimeTests.cs +++ b/src/Umbraco.Tests.Integration/RuntimeTests.cs @@ -4,8 +4,10 @@ using Moq; using NUnit.Framework; using System.Threading.Tasks; using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; using Smidge; using Umbraco.Core; +using Umbraco.Core.Cache; using Umbraco.Core.Composing; using Umbraco.Core.Logging; using Umbraco.Core.Runtime; @@ -57,7 +59,7 @@ namespace Umbraco.Tests.Integration var coreRuntime = new CoreRuntime(testHelper.GetConfigs(), testHelper.GetUmbracoVersion(), testHelper.IOHelper, testHelper.Logger, testHelper.Profiler, testHelper.UmbracoBootPermissionChecker, testHelper.GetHostingEnvironment(), testHelper.GetBackOfficeInfo(), testHelper.DbProviderFactoryCreator, - testHelper.MainDom, testHelper.GetTypeFinder()); + testHelper.MainDom, testHelper.GetTypeFinder(), NoAppCache.Instance); // boot it! var factory = coreRuntime.Configure(umbracoContainer); @@ -99,7 +101,7 @@ namespace Umbraco.Tests.Integration // Add it! services.AddUmbracoConfiguration(hostContext.Configuration); - services.AddUmbracoCore(webHostEnvironment, umbracoContainer, GetType().Assembly, out _); + services.AddUmbracoCore(webHostEnvironment, umbracoContainer, GetType().Assembly, NoAppCache.Instance, null, out _); }); var host = await hostBuilder.StartAsync(); @@ -138,7 +140,7 @@ namespace Umbraco.Tests.Integration // Add it! services.AddUmbracoConfiguration(hostContext.Configuration); - services.AddUmbracoCore(webHostEnvironment, umbracoContainer, GetType().Assembly, out _); + services.AddUmbracoCore(webHostEnvironment, umbracoContainer, GetType().Assembly, NoAppCache.Instance, null, out _); }); var host = await hostBuilder.StartAsync(); diff --git a/src/Umbraco.Tests.Integration/Testing/UmbracoIntegrationTest.cs b/src/Umbraco.Tests.Integration/Testing/UmbracoIntegrationTest.cs index 101feb79a4..41eb082dc2 100644 --- a/src/Umbraco.Tests.Integration/Testing/UmbracoIntegrationTest.cs +++ b/src/Umbraco.Tests.Integration/Testing/UmbracoIntegrationTest.cs @@ -108,7 +108,7 @@ namespace Umbraco.Tests.Integration.Testing // Add it! services.AddUmbracoConfiguration(hostContext.Configuration); - services.AddUmbracoCore(webHostEnvironment, umbracoContainer, GetType().Assembly, out _); + services.AddUmbracoCore(webHostEnvironment, umbracoContainer, GetType().Assembly, NoAppCache.Instance, null, out _); }); var host = await hostBuilder.StartAsync(); diff --git a/src/Umbraco.Tests/Routing/RenderRouteHandlerTests.cs b/src/Umbraco.Tests/Routing/RenderRouteHandlerTests.cs index b900453a5e..4763d3dbc6 100644 --- a/src/Umbraco.Tests/Routing/RenderRouteHandlerTests.cs +++ b/src/Umbraco.Tests/Routing/RenderRouteHandlerTests.cs @@ -16,10 +16,10 @@ using Umbraco.Web.Mvc; using Umbraco.Web.WebApi; using Umbraco.Core.Strings; using Umbraco.Core.Configuration; -using Umbraco.Core.Dictionary; using Umbraco.Core.Hosting; using Umbraco.Core.IO; using Umbraco.Core.Models.PublishedContent; +using Umbraco.Core.Runtime; using Umbraco.Core.Services; using Umbraco.Tests.PublishedContent; using Umbraco.Tests.Testing; @@ -47,10 +47,10 @@ namespace Umbraco.Tests.Routing HostingEnvironment); } - public class TestRuntime : WebRuntime + public class TestRuntime : CoreRuntime { public TestRuntime(Configs configs, IUmbracoVersion umbracoVersion, IIOHelper ioHelper, ILogger logger, IHostingEnvironment hostingEnvironment, IBackOfficeInfo backOfficeInfo) - : base(configs, umbracoVersion, ioHelper, Mock.Of(), Mock.Of(), hostingEnvironment, backOfficeInfo, TestHelper.DbProviderFactoryCreator, TestHelper.MainDom, TestHelper.GetTypeFinder(), TestHelper.GetRequestCache(), new AspNetUmbracoBootPermissionChecker()) + : base(configs, umbracoVersion, ioHelper, Mock.Of(), Mock.Of(), new AspNetUmbracoBootPermissionChecker(), hostingEnvironment, backOfficeInfo, TestHelper.DbProviderFactoryCreator, TestHelper.MainDom, TestHelper.GetTypeFinder(), TestHelper.GetRequestCache()) { } diff --git a/src/Umbraco.Tests/Runtimes/CoreRuntimeTests.cs b/src/Umbraco.Tests/Runtimes/CoreRuntimeTests.cs index 488a4f6dad..0b90457b1e 100644 --- a/src/Umbraco.Tests/Runtimes/CoreRuntimeTests.cs +++ b/src/Umbraco.Tests/Runtimes/CoreRuntimeTests.cs @@ -5,6 +5,7 @@ using Examine; using Moq; using NUnit.Framework; using Umbraco.Core; +using Umbraco.Core.Cache; using Umbraco.Core.Composing; using Umbraco.Core.Configuration; using Umbraco.Core.Configuration.UmbracoSettings; @@ -116,7 +117,7 @@ namespace Umbraco.Tests.Runtimes public class TestRuntime : CoreRuntime { public TestRuntime(Configs configs, IUmbracoVersion umbracoVersion, IIOHelper ioHelper, ILogger logger, IProfiler profiler, IHostingEnvironment hostingEnvironment, IBackOfficeInfo backOfficeInfo) - :base(configs, umbracoVersion, ioHelper, logger, profiler, new AspNetUmbracoBootPermissionChecker(), hostingEnvironment, backOfficeInfo, TestHelper.DbProviderFactoryCreator, TestHelper.MainDom, TestHelper.GetTypeFinder()) + :base(configs, umbracoVersion, ioHelper, logger, profiler, new AspNetUmbracoBootPermissionChecker(), hostingEnvironment, backOfficeInfo, TestHelper.DbProviderFactoryCreator, TestHelper.MainDom, TestHelper.GetTypeFinder(), NoAppCache.Instance) { } diff --git a/src/Umbraco.Tests/Runtimes/StandaloneTests.cs b/src/Umbraco.Tests/Runtimes/StandaloneTests.cs index 1a4c7f2040..5100e2e21c 100644 --- a/src/Umbraco.Tests/Runtimes/StandaloneTests.cs +++ b/src/Umbraco.Tests/Runtimes/StandaloneTests.cs @@ -76,7 +76,7 @@ namespace Umbraco.Tests.Runtimes var runtimeState = new RuntimeState(logger, null, umbracoVersion, backOfficeInfo); var configs = TestHelper.GetConfigs(); var variationContextAccessor = TestHelper.VariationContextAccessor; - + // create the register and the composition var register = TestHelper.GetRegister(); @@ -84,7 +84,7 @@ namespace Umbraco.Tests.Runtimes composition.RegisterEssentials(logger, profiler, profilingLogger, mainDom, appCaches, databaseFactory, typeLoader, runtimeState, typeFinder, ioHelper, umbracoVersion, TestHelper.DbProviderFactoryCreator, hostingEnvironment, backOfficeInfo); // create the core runtime and have it compose itself - var coreRuntime = new CoreRuntime(configs, umbracoVersion, ioHelper, logger, profiler, new AspNetUmbracoBootPermissionChecker(), hostingEnvironment, backOfficeInfo, TestHelper.DbProviderFactoryCreator, TestHelper.MainDom, typeFinder); + var coreRuntime = new CoreRuntime(configs, umbracoVersion, ioHelper, logger, profiler, new AspNetUmbracoBootPermissionChecker(), hostingEnvironment, backOfficeInfo, TestHelper.DbProviderFactoryCreator, TestHelper.MainDom, typeFinder, NoAppCache.Instance); // determine actual runtime level runtimeState.DetermineRuntimeLevel(databaseFactory, logger); @@ -278,7 +278,7 @@ namespace Umbraco.Tests.Runtimes composition.RegisterEssentials(logger, profiler, profilingLogger, mainDom, appCaches, databaseFactory, typeLoader, runtimeState, typeFinder, ioHelper, umbracoVersion, TestHelper.DbProviderFactoryCreator, hostingEnvironment, backOfficeInfo); // create the core runtime and have it compose itself - var coreRuntime = new CoreRuntime(configs, umbracoVersion, ioHelper, logger, profiler, new AspNetUmbracoBootPermissionChecker(), hostingEnvironment, backOfficeInfo, TestHelper.DbProviderFactoryCreator, TestHelper.MainDom, typeFinder); + var coreRuntime = new CoreRuntime(configs, umbracoVersion, ioHelper, logger, profiler, new AspNetUmbracoBootPermissionChecker(), hostingEnvironment, backOfficeInfo, TestHelper.DbProviderFactoryCreator, TestHelper.MainDom, typeFinder, NoAppCache.Instance); // get the components // all of them? @@ -322,8 +322,8 @@ namespace Umbraco.Tests.Runtimes Assert.AreEqual(0, results.Count); } - - + + } } diff --git a/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs b/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs index 843620d571..484572f010 100644 --- a/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs @@ -1,4 +1,6 @@ using System; +using System.Collections; +using System.Collections.Generic; using System.Data.Common; using System.IO; using System.Reflection; @@ -71,7 +73,13 @@ namespace Umbraco.Web.Common.Extensions var umbContainer = UmbracoServiceProviderFactory.UmbracoContainer; - services.AddUmbracoCore(webHostEnvironment, umbContainer, Assembly.GetEntryAssembly(), out factory); + + IHttpContextAccessor httpContextAccessor = new HttpContextAccessor(); + services.AddSingleton(httpContextAccessor); + + var requestCache = new GenericDictionaryRequestAppCache(() => httpContextAccessor.HttpContext.Items); + + services.AddUmbracoCore(webHostEnvironment, umbContainer, Assembly.GetEntryAssembly(), requestCache, httpContextAccessor, out factory); return services; } @@ -83,20 +91,28 @@ namespace Umbraco.Web.Common.Extensions /// /// /// + /// /// /// - public static IServiceCollection AddUmbracoCore(this IServiceCollection services, IWebHostEnvironment webHostEnvironment, IRegister umbContainer, Assembly entryAssembly, out IFactory factory) + public static IServiceCollection AddUmbracoCore( + this IServiceCollection services, + IWebHostEnvironment webHostEnvironment, + IRegister umbContainer, + Assembly entryAssembly, + IRequestCache requestCache, + IHttpContextAccessor httpContextAccessor, + out IFactory factory) { if (services is null) throw new ArgumentNullException(nameof(services)); var container = umbContainer; if (container is null) throw new ArgumentNullException(nameof(container)); if (entryAssembly is null) throw new ArgumentNullException(nameof(entryAssembly)); - // Special case! The generic host adds a few default services but we need to manually add this one here NOW because - // we resolve it before the host finishes configuring in the call to CreateCompositionRoot - services.AddSingleton(); + var serviceProvider = services.BuildServiceProvider(); + var configs = serviceProvider.GetService(); - CreateCompositionRoot(services, webHostEnvironment, out var logger, out var configs, out var ioHelper, out var hostingEnvironment, out var backOfficeInfo, out var profiler); + + CreateCompositionRoot(services, configs, httpContextAccessor, webHostEnvironment, out var logger, out var ioHelper, out var hostingEnvironment, out var backOfficeInfo, out var profiler); var globalSettings = configs.Global(); var umbracoVersion = new UmbracoVersion(globalSettings); @@ -109,7 +125,8 @@ namespace Umbraco.Web.Common.Extensions profiler, hostingEnvironment, backOfficeInfo, - CreateTypeFinder(logger, profiler, webHostEnvironment, entryAssembly)); + CreateTypeFinder(logger, profiler, webHostEnvironment, entryAssembly), + requestCache); factory = coreRuntime.Configure(container); @@ -126,9 +143,10 @@ namespace Umbraco.Web.Common.Extensions return new TypeFinder(logger, new DefaultUmbracoAssemblyProvider(entryAssembly), runtimeHash); } - private static IRuntime GetCoreRuntime(Configs configs, IUmbracoVersion umbracoVersion, IIOHelper ioHelper, ILogger logger, + private static IRuntime GetCoreRuntime( + Configs configs, IUmbracoVersion umbracoVersion, IIOHelper ioHelper, ILogger logger, IProfiler profiler, Core.Hosting.IHostingEnvironment hostingEnvironment, IBackOfficeInfo backOfficeInfo, - ITypeFinder typeFinder) + ITypeFinder typeFinder, IRequestCache requestCache) { var connectionStringConfig = configs.ConnectionStrings()[Constants.System.UmbracoConnectionName]; var dbProviderFactoryCreator = new SqlServerDbProviderFactoryCreator( @@ -145,22 +163,27 @@ namespace Umbraco.Web.Common.Extensions var mainDom = new MainDom(logger, mainDomLock); - var coreRuntime = new CoreRuntime(configs, umbracoVersion, ioHelper, logger, profiler, new AspNetCoreBootPermissionsChecker(), - hostingEnvironment, backOfficeInfo, dbProviderFactoryCreator, mainDom, typeFinder); + var coreRuntime = new CoreRuntime( + configs, + umbracoVersion, + ioHelper, + logger, + profiler, + new AspNetCoreBootPermissionsChecker(), + hostingEnvironment, + backOfficeInfo, + dbProviderFactoryCreator, + mainDom, + typeFinder, + requestCache); return coreRuntime; } - private static IServiceCollection CreateCompositionRoot(IServiceCollection services, IWebHostEnvironment webHostEnvironment, - out ILogger logger, out Configs configs, out IIOHelper ioHelper, out Core.Hosting.IHostingEnvironment hostingEnvironment, + private static IServiceCollection CreateCompositionRoot(IServiceCollection services, Configs configs, IHttpContextAccessor httpContextAccessor, IWebHostEnvironment webHostEnvironment, + out ILogger logger, out IIOHelper ioHelper, out Core.Hosting.IHostingEnvironment hostingEnvironment, out IBackOfficeInfo backOfficeInfo, out IProfiler profiler) { - // TODO: We need to avoid this, surely there's a way? See ContainerTests.BuildServiceProvider_Before_Host_Is_Configured - var serviceProvider = services.BuildServiceProvider(); - - var httpContextAccessor = serviceProvider.GetRequiredService(); - - configs = serviceProvider.GetService(); if (configs == null) throw new InvalidOperationException($"Could not resolve type {typeof(Configs)} from the container, ensure {nameof(AddUmbracoConfiguration)} is called before calling {nameof(AddUmbracoCore)}"); @@ -216,4 +239,5 @@ namespace Umbraco.Web.Common.Extensions } + } diff --git a/src/Umbraco.Web/UmbracoApplication.cs b/src/Umbraco.Web/UmbracoApplication.cs index cd72b2faf9..7679da2e2e 100644 --- a/src/Umbraco.Web/UmbracoApplication.cs +++ b/src/Umbraco.Web/UmbracoApplication.cs @@ -1,4 +1,7 @@ -using System.Threading; +using System.Collections; +using System.Collections.Generic; +using System.Linq; +using System.Threading; using System.Web; using Umbraco.Core; using Umbraco.Core.Cache; @@ -34,10 +37,12 @@ namespace Umbraco.Web var mainDom = new MainDom(logger, mainDomLock); - var requestCache = new HttpRequestAppCache(() => HttpContext.Current?.Items); + var requestCache = new HttpRequestAppCache(() => HttpContext.Current != null ? HttpContext.Current.Items : null); var umbracoBootPermissionChecker = new AspNetUmbracoBootPermissionChecker(); - return new WebRuntime(configs, umbracoVersion, ioHelper, logger, profiler, hostingEnvironment, backOfficeInfo, dbProviderFactoryCreator, mainDom, - GetTypeFinder(hostingEnvironment, logger, profiler), requestCache, umbracoBootPermissionChecker); + return new CoreRuntime(configs, umbracoVersion, ioHelper, logger, profiler, umbracoBootPermissionChecker, hostingEnvironment, backOfficeInfo, dbProviderFactoryCreator, mainDom, + GetTypeFinder(hostingEnvironment, logger, profiler), requestCache); } + + } } From 487e3c54a8d28a40c046edde2a8b9057c60528dc Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Mon, 20 Apr 2020 22:55:40 +0200 Subject: [PATCH 23/39] Reimplementing action filters to asp.net core --- .../ActionExecutedEventArgs.cs | 17 ++++++++ .../Filters/DisableBrowserCacheAttribute.cs | 33 +++++++++++++++ .../PreRenderViewActionFilterAttribute.cs | 42 +++++++++++++++++++ .../Filters/StatusCodeResultAttribute.cs | 37 ++++++++++++++++ 4 files changed, 129 insertions(+) create mode 100644 src/Umbraco.Web.BackOffice/ActionExecutedEventArgs.cs create mode 100644 src/Umbraco.Web.BackOffice/Filters/DisableBrowserCacheAttribute.cs create mode 100644 src/Umbraco.Web.BackOffice/Filters/PreRenderViewActionFilterAttribute.cs create mode 100644 src/Umbraco.Web.BackOffice/Filters/StatusCodeResultAttribute.cs diff --git a/src/Umbraco.Web.BackOffice/ActionExecutedEventArgs.cs b/src/Umbraco.Web.BackOffice/ActionExecutedEventArgs.cs new file mode 100644 index 0000000000..a2ac5701be --- /dev/null +++ b/src/Umbraco.Web.BackOffice/ActionExecutedEventArgs.cs @@ -0,0 +1,17 @@ +using System; +using Microsoft.AspNetCore.Mvc; + +namespace Umbraco.Web.BackOffice +{ + public class ActionExecutedEventArgs : EventArgs + { + public Controller Controller { get; set; } + public object Model { get; set; } + + public ActionExecutedEventArgs(Controller controller, object model) + { + Controller = controller; + Model = model; + } + } +} diff --git a/src/Umbraco.Web.BackOffice/Filters/DisableBrowserCacheAttribute.cs b/src/Umbraco.Web.BackOffice/Filters/DisableBrowserCacheAttribute.cs new file mode 100644 index 0000000000..e2dc357fa9 --- /dev/null +++ b/src/Umbraco.Web.BackOffice/Filters/DisableBrowserCacheAttribute.cs @@ -0,0 +1,33 @@ +using System; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.Net.Http.Headers; + +namespace Umbraco.Web.BackOffice.Filters +{ + /// + /// Ensures that the request is not cached by the browser + /// + public class DisableBrowserCacheAttribute : ActionFilterAttribute + { + public override void OnResultExecuting(ResultExecutingContext context) + { + base.OnResultExecuting(context); + + if (context.HttpContext.Response.StatusCode != 200) return; + + context.HttpContext.Response.GetTypedHeaders().CacheControl = + new CacheControlHeaderValue() + { + NoCache = true, + MaxAge = TimeSpan.Zero, + MustRevalidate = true, + NoStore = true + }; + + context.HttpContext.Response.Headers[HeaderNames.LastModified] = DateTime.Now.ToString("R"); // Format RFC1123 + context.HttpContext.Response.Headers[HeaderNames.Pragma] = "no-cache"; + context.HttpContext.Response.Headers[HeaderNames.Expires] = new DateTime(1990, 1, 1, 0, 0, 0).ToString("R"); + } + } +} diff --git a/src/Umbraco.Web.BackOffice/Filters/PreRenderViewActionFilterAttribute.cs b/src/Umbraco.Web.BackOffice/Filters/PreRenderViewActionFilterAttribute.cs new file mode 100644 index 0000000000..8c8a9a135a --- /dev/null +++ b/src/Umbraco.Web.BackOffice/Filters/PreRenderViewActionFilterAttribute.cs @@ -0,0 +1,42 @@ +using System; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Filters; + +namespace Umbraco.Web.BackOffice.Filters +{ + public class PreRenderViewActionFilterAttribute : ActionFilterAttribute + { + public override void OnActionExecuted(ActionExecutedContext context) + { + if (!(context.Controller is Controller umbController) || !(context.Result is ViewResult result)) + { + return; + } + + var model = result.Model; + if (model == null) + { + return; + } + + var args = new ActionExecutedEventArgs(umbController, model); + OnActionExecuted(args); + + if (args.Model != model) + { + result.ViewData.Model = args.Model; + } + + base.OnActionExecuted(context); + } + + + public static event EventHandler ActionExecuted; + + private static void OnActionExecuted(ActionExecutedEventArgs e) + { + var handler = ActionExecuted; + handler?.Invoke(null, e); + } + } +} diff --git a/src/Umbraco.Web.BackOffice/Filters/StatusCodeResultAttribute.cs b/src/Umbraco.Web.BackOffice/Filters/StatusCodeResultAttribute.cs new file mode 100644 index 0000000000..870c016b38 --- /dev/null +++ b/src/Umbraco.Web.BackOffice/Filters/StatusCodeResultAttribute.cs @@ -0,0 +1,37 @@ +using Microsoft.AspNetCore.Mvc.Filters; +using System.Net; +using Microsoft.AspNetCore.Diagnostics; +using Microsoft.Extensions.DependencyInjection; +using Umbraco.Core.Configuration.UmbracoSettings; + +namespace Umbraco.Web.BackOffice.Filters +{ + /// + /// Forces the response to have a specific http status code + /// + public class StatusCodeResultAttribute : ActionFilterAttribute + { + private readonly HttpStatusCode _statusCode; + + public StatusCodeResultAttribute(HttpStatusCode statusCode) + { + _statusCode = statusCode; + } + + public override void OnActionExecuted(ActionExecutedContext context) + { + base.OnActionExecuted(context); + + context.HttpContext.Response.StatusCode = (int)_statusCode; + + var disableIisCustomErrors = context.HttpContext.RequestServices.GetService().TrySkipIisCustomErrors; + var statusCodePagesFeature = context.HttpContext.Features.Get(); + + if (statusCodePagesFeature != null) + { + // if IIS Custom Errors are disabled, we won't enable the Status Code Pages + statusCodePagesFeature.Enabled = !disableIisCustomErrors; + } + } + } +} From d483f2ccbd5152425fa09db2c53a549ce0dae676 Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Mon, 20 Apr 2020 22:56:36 +0200 Subject: [PATCH 24/39] Marking asp.net action filters as migrated to net core --- src/Umbraco.Web/Mvc/MinifyJavaScriptResultAttribute.cs | 1 + src/Umbraco.Web/Mvc/PreRenderViewActionFilterAttribute.cs | 1 + src/Umbraco.Web/Mvc/StatusCodeFilterAttribute.cs | 1 + 3 files changed, 3 insertions(+) diff --git a/src/Umbraco.Web/Mvc/MinifyJavaScriptResultAttribute.cs b/src/Umbraco.Web/Mvc/MinifyJavaScriptResultAttribute.cs index 227c15b344..635a7314c5 100644 --- a/src/Umbraco.Web/Mvc/MinifyJavaScriptResultAttribute.cs +++ b/src/Umbraco.Web/Mvc/MinifyJavaScriptResultAttribute.cs @@ -12,6 +12,7 @@ namespace Umbraco.Web.Mvc /// /// Only minifies in release mode /// + /// Migrated already to .Net Core public class MinifyJavaScriptResultAttribute : ActionFilterAttribute { private readonly IHostingEnvironment _hostingEnvironment; diff --git a/src/Umbraco.Web/Mvc/PreRenderViewActionFilterAttribute.cs b/src/Umbraco.Web/Mvc/PreRenderViewActionFilterAttribute.cs index 54e20f5d56..2e659eccf6 100644 --- a/src/Umbraco.Web/Mvc/PreRenderViewActionFilterAttribute.cs +++ b/src/Umbraco.Web/Mvc/PreRenderViewActionFilterAttribute.cs @@ -3,6 +3,7 @@ using System.Web.Mvc; namespace Umbraco.Web.Mvc { + /// Migrated already to .Net Core public class PreRenderViewActionFilterAttribute : ActionFilterAttribute { public override void OnActionExecuted(ActionExecutedContext filterContext) diff --git a/src/Umbraco.Web/Mvc/StatusCodeFilterAttribute.cs b/src/Umbraco.Web/Mvc/StatusCodeFilterAttribute.cs index b1836c6ba4..727c29b93c 100644 --- a/src/Umbraco.Web/Mvc/StatusCodeFilterAttribute.cs +++ b/src/Umbraco.Web/Mvc/StatusCodeFilterAttribute.cs @@ -8,6 +8,7 @@ namespace Umbraco.Web.Mvc /// /// Forces the response to have a specific http status code /// + /// Migrated already to .Net Core internal class StatusCodeResultAttribute : ActionFilterAttribute { private readonly HttpStatusCode _statusCode; From ee37d3aa7ac354fe35d7bfc085c9872498d24764 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 21 Apr 2020 13:07:20 +1000 Subject: [PATCH 25/39] Fixes up usage of AppDomain.CurrentDomain.BaseDirectory in log viewer which cannot work in netcore since this is inconsistent with net framework, adds notes, starts configuring serilog with UseUmbraco --- .../Hosting/IHostingEnvironment.cs | 18 +++- .../Composing/HostBuilderExtensions.cs | 4 +- .../Logging/Serilog/LoggerConfigExtensions.cs | 12 +++ .../Logging/Serilog/SerilogLogger.cs | 2 + .../Logging/Viewer/ILogViewerConfig.cs | 12 +++ .../Logging/Viewer/JsonLogViewer.cs | 21 +++-- .../Logging/Viewer/LogViewerComposer.cs | 3 +- .../Logging/Viewer/LogViewerConfig.cs | 84 +++++++++++++++++++ .../Logging/Viewer/LogViewerSourceBase.cs | 80 ++---------------- src/Umbraco.Tests/Logging/LogviewerTests.cs | 8 +- .../Extensions/HostBuilderExtensions.cs | 21 +++++ .../Umbraco.Web.Common.csproj | 1 + .../Config/logviewer.searches.config.js | 42 ++++++++++ src/Umbraco.Web.UI.NetCore/Program.cs | 3 +- .../Umbraco.Web.UI.NetCore.csproj | 8 ++ 15 files changed, 229 insertions(+), 90 deletions(-) create mode 100644 src/Umbraco.Infrastructure/Logging/Viewer/ILogViewerConfig.cs create mode 100644 src/Umbraco.Infrastructure/Logging/Viewer/LogViewerConfig.cs create mode 100644 src/Umbraco.Web.Common/Extensions/HostBuilderExtensions.cs create mode 100644 src/Umbraco.Web.UI.NetCore/Config/logviewer.searches.config.js diff --git a/src/Umbraco.Core/Hosting/IHostingEnvironment.cs b/src/Umbraco.Core/Hosting/IHostingEnvironment.cs index 0bdfe5c425..b653f535fa 100644 --- a/src/Umbraco.Core/Hosting/IHostingEnvironment.cs +++ b/src/Umbraco.Core/Hosting/IHostingEnvironment.cs @@ -6,6 +6,10 @@ namespace Umbraco.Core.Hosting { string SiteName { get; } string ApplicationId { get; } + + /// + /// Will return the physical path to the root of the application + /// string ApplicationPhysicalPath { get; } string LocalTempPath { get; } @@ -27,10 +31,22 @@ namespace Umbraco.Core.Hosting bool IsHosted { get; } Version IISVersion { get; } + + // TODO: Should we change this name to MapPathWebRoot ? and also have a new MapPathContentRoot ? + + /// + /// Maps a virtual path to a physical path to the application's web root + /// + /// + /// + /// + /// Depending on the runtime 'web root', this result can vary. For example in Net Framework the web root and the content root are the same, however + /// in netcore the web root is /www therefore this will Map to a physical path within www. + /// string MapPath(string path); /// - /// Maps a virtual path to the application's web root + /// Converts a virtual path to an absolute URL path based on the application's web root /// /// The virtual path. Must start with either ~/ or / else an exception is thrown. /// diff --git a/src/Umbraco.Infrastructure/Composing/HostBuilderExtensions.cs b/src/Umbraco.Infrastructure/Composing/HostBuilderExtensions.cs index 2099778185..3ccc3b8a8d 100644 --- a/src/Umbraco.Infrastructure/Composing/HostBuilderExtensions.cs +++ b/src/Umbraco.Infrastructure/Composing/HostBuilderExtensions.cs @@ -11,10 +11,8 @@ namespace Umbraco.Core.Composing /// Assigns a custom service provider factory to use Umbraco's container /// /// + /// /// - public static IHostBuilder UseUmbraco(this IHostBuilder builder) - => builder.UseUmbraco(new UmbracoServiceProviderFactory()); - public static IHostBuilder UseUmbraco(this IHostBuilder builder, UmbracoServiceProviderFactory umbracoServiceProviderFactory) => builder.UseServiceProviderFactory(umbracoServiceProviderFactory); } diff --git a/src/Umbraco.Infrastructure/Logging/Serilog/LoggerConfigExtensions.cs b/src/Umbraco.Infrastructure/Logging/Serilog/LoggerConfigExtensions.cs index f4e8f85281..d810a08648 100644 --- a/src/Umbraco.Infrastructure/Logging/Serilog/LoggerConfigExtensions.cs +++ b/src/Umbraco.Infrastructure/Logging/Serilog/LoggerConfigExtensions.cs @@ -29,6 +29,8 @@ namespace Umbraco.Core.Logging.Serilog { global::Serilog.Debugging.SelfLog.Enable(msg => System.Diagnostics.Debug.WriteLine(msg)); + // TODO: we can't use AppDomain.CurrentDomain.BaseDirectory, need a consistent approach between netcore/netframework + //Set this environment variable - so that it can be used in external config file //add key="serilog:write-to:RollingFile.pathFormat" value="%BASEDIR%\logs\log.txt" /> Environment.SetEnvironmentVariable("BASEDIR", AppDomain.CurrentDomain.BaseDirectory, EnvironmentVariableTarget.Process); @@ -57,6 +59,8 @@ namespace Umbraco.Core.Logging.Serilog /// The number of days to keep log files. Default is set to null which means all logs are kept public static LoggerConfiguration OutputDefaultTextFile(this LoggerConfiguration logConfig, LogEventLevel minimumLevel = LogEventLevel.Verbose, int? retainedFileCount = null) { + // TODO: we can't use AppDomain.CurrentDomain.BaseDirectory, need a consistent approach between netcore/netframework + //Main .txt logfile - in similar format to older Log4Net output //Ends with ..txt as Date is inserted before file extension substring logConfig.WriteTo.File($@"{AppDomain.CurrentDomain.BaseDirectory}\App_Data\Logs\UmbracoTraceLog.{Environment.MachineName}..txt", @@ -85,6 +89,8 @@ namespace Umbraco.Core.Logging.Serilog Encoding encoding = null ) { + // TODO: Deal with this method call since it's obsolete, we need to change this + return configuration.Async( asyncConfiguration => asyncConfiguration.Map(AppDomainId, (_,mapConfiguration) => mapConfiguration.File( @@ -113,6 +119,8 @@ namespace Umbraco.Core.Logging.Serilog /// The number of days to keep log files. Default is set to null which means all logs are kept public static LoggerConfiguration OutputDefaultJsonFile(this LoggerConfiguration logConfig, LogEventLevel minimumLevel = LogEventLevel.Verbose, int? retainedFileCount = null) { + // TODO: we can't use AppDomain.CurrentDomain.BaseDirectory, need a consistent approach between netcore/netframework + //.clef format (Compact log event format, that can be imported into local SEQ & will make searching/filtering logs easier) //Ends with ..txt as Date is inserted before file extension substring logConfig.WriteTo.File(new CompactJsonFormatter(), $@"{AppDomain.CurrentDomain.BaseDirectory}\App_Data\Logs\UmbracoTraceLog.{Environment.MachineName}..json", @@ -131,6 +139,8 @@ namespace Umbraco.Core.Logging.Serilog /// A Serilog LoggerConfiguration public static LoggerConfiguration ReadFromConfigFile(this LoggerConfiguration logConfig) { + // TODO: we can't use AppDomain.CurrentDomain.BaseDirectory, need a consistent approach between netcore/netframework + //Read from main serilog.config file logConfig.ReadFrom.AppSettings(filePath: AppDomain.CurrentDomain.BaseDirectory + @"\config\serilog.config"); @@ -144,6 +154,8 @@ namespace Umbraco.Core.Logging.Serilog /// A Serilog LoggerConfiguration public static LoggerConfiguration ReadFromUserConfigFile(this LoggerConfiguration logConfig) { + // TODO: we can't use AppDomain.CurrentDomain.BaseDirectory, need a consistent approach between netcore/netframework + //A nested logger - where any user configured sinks via config can not effect the main 'umbraco' logger above logConfig.WriteTo.Logger(cfg => cfg.ReadFrom.AppSettings(filePath: AppDomain.CurrentDomain.BaseDirectory + @"\config\serilog.user.config")); diff --git a/src/Umbraco.Infrastructure/Logging/Serilog/SerilogLogger.cs b/src/Umbraco.Infrastructure/Logging/Serilog/SerilogLogger.cs index bb77869e28..497de58bdd 100644 --- a/src/Umbraco.Infrastructure/Logging/Serilog/SerilogLogger.cs +++ b/src/Umbraco.Infrastructure/Logging/Serilog/SerilogLogger.cs @@ -32,6 +32,8 @@ namespace Umbraco.Core.Logging.Serilog _ioHelper = ioHelper; _marchal = marchal; + // TODO: we can't use AppDomain.CurrentDomain.BaseDirectory, need a consistent approach between netcore/netframework + Log.Logger = new LoggerConfiguration() .ReadFrom.AppSettings(filePath: AppDomain.CurrentDomain.BaseDirectory + logConfigFile) .CreateLogger(); diff --git a/src/Umbraco.Infrastructure/Logging/Viewer/ILogViewerConfig.cs b/src/Umbraco.Infrastructure/Logging/Viewer/ILogViewerConfig.cs new file mode 100644 index 0000000000..14f35361e6 --- /dev/null +++ b/src/Umbraco.Infrastructure/Logging/Viewer/ILogViewerConfig.cs @@ -0,0 +1,12 @@ +using System; +using System.Collections.Generic; + +namespace Umbraco.Core.Logging.Viewer +{ + public interface ILogViewerConfig + { + IReadOnlyList GetSavedSearches(); + IReadOnlyList AddSavedSearch(string name, string query); + IReadOnlyList DeleteSavedSearch(string name, string query); + } +} diff --git a/src/Umbraco.Infrastructure/Logging/Viewer/JsonLogViewer.cs b/src/Umbraco.Infrastructure/Logging/Viewer/JsonLogViewer.cs index aea1c8fae4..54dd58ec03 100644 --- a/src/Umbraco.Infrastructure/Logging/Viewer/JsonLogViewer.cs +++ b/src/Umbraco.Infrastructure/Logging/Viewer/JsonLogViewer.cs @@ -5,6 +5,7 @@ using System.Linq; using Newtonsoft.Json; using Serilog.Events; using Serilog.Formatting.Compact.Reader; +using Umbraco.Core.Hosting; using Umbraco.Core.IO; namespace Umbraco.Core.Logging.Viewer @@ -13,14 +14,19 @@ namespace Umbraco.Core.Logging.Viewer { private readonly string _logsPath; private readonly ILogger _logger; + private readonly IHostingEnvironment _hostingEnvironment; - public JsonLogViewer(ILogger logger, IIOHelper ioHelper, string logsPath = "", string searchPath = "") : base(ioHelper, searchPath) + public JsonLogViewer(ILogger logger, ILogViewerConfig logViewerConfig, IHostingEnvironment hostingEnvironment) : base(logViewerConfig) { - if (string.IsNullOrEmpty(logsPath)) - logsPath = $@"{AppDomain.CurrentDomain.BaseDirectory}\App_Data\Logs\"; - - _logsPath = logsPath; + _hostingEnvironment = hostingEnvironment; _logger = logger; + + // TODO: this path is hard coded but it can actually be configured, but that is done via Serilog and we don't have a different abstraction/config + // for the logging path. We could make that, but then how would we get that abstraction into the Serilog config? I'm sure there is a way but + // don't have time right now to resolve that (since this was hard coded before). We could have a single/simple ILogConfig for umbraco that purely specifies + // the logging path and then we can have a special token that we replace in the serilog config that maps to that location? then at least we could inject + // that config in places where we are hard coding this path. + _logsPath = Path.Combine(_hostingEnvironment.ApplicationPhysicalPath, @"App_Data\Logs\"); } private const int FileSizeCap = 100; @@ -62,9 +68,6 @@ namespace Umbraco.Core.Logging.Viewer { var logs = new List(); - //Log Directory - var logDirectory = $@"{AppDomain.CurrentDomain.BaseDirectory}\App_Data\Logs\"; - var count = 0; //foreach full day in the range - see if we can find one or more filenames that end with @@ -74,7 +77,7 @@ namespace Umbraco.Core.Logging.Viewer //Filename ending to search for (As could be multiple) var filesToFind = GetSearchPattern(day); - var filesForCurrentDay = Directory.GetFiles(logDirectory, filesToFind); + var filesForCurrentDay = Directory.GetFiles(_logsPath, filesToFind); //Foreach file we find - open it foreach (var filePath in filesForCurrentDay) diff --git a/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerComposer.cs b/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerComposer.cs index 79680a3d53..e4acde1265 100644 --- a/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerComposer.cs +++ b/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerComposer.cs @@ -9,7 +9,8 @@ namespace Umbraco.Core.Logging.Viewer { public void Compose(Composition composition) { - composition.SetLogViewer(factory => new JsonLogViewer(composition.Logger, factory.GetInstance())); + composition.RegisterUnique(); + composition.SetLogViewer(); } } } diff --git a/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerConfig.cs b/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerConfig.cs new file mode 100644 index 0000000000..5511cd87c7 --- /dev/null +++ b/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerConfig.cs @@ -0,0 +1,84 @@ +using System.Collections.Generic; +using System.IO; +using System.Linq; +using Newtonsoft.Json; +using Umbraco.Core.Hosting; +using Formatting = Newtonsoft.Json.Formatting; + +namespace Umbraco.Core.Logging.Viewer +{ + public class LogViewerConfig : ILogViewerConfig + { + private readonly IHostingEnvironment _hostingEnvironment; + private const string _pathToSearches = "~/Config/logviewer.searches.config.js"; + private readonly FileInfo _searchesConfig; + + public LogViewerConfig(IHostingEnvironment hostingEnvironment) + { + _hostingEnvironment = hostingEnvironment; + var trimmedPath = _pathToSearches.TrimStart('~', '/').Replace('/', Path.DirectorySeparatorChar); + var absolutePath = Path.Combine(_hostingEnvironment.ApplicationPhysicalPath, trimmedPath); + _searchesConfig = new FileInfo(absolutePath); + } + + public IReadOnlyList GetSavedSearches() + { + //Our default implementation + + //If file does not exist - lets create it with an empty array + EnsureFileExists(); + + var rawJson = System.IO.File.ReadAllText(_searchesConfig.FullName); + return JsonConvert.DeserializeObject(rawJson); + } + + public IReadOnlyList AddSavedSearch(string name, string query) + { + //Get the existing items + var searches = GetSavedSearches().ToList(); + + //Add the new item to the bottom of the list + searches.Add(new SavedLogSearch { Name = name, Query = query }); + + //Serialize to JSON string + var rawJson = JsonConvert.SerializeObject(searches, Formatting.Indented); + + //If file does not exist - lets create it with an empty array + EnsureFileExists(); + + //Write it back down to file + System.IO.File.WriteAllText(_searchesConfig.FullName, rawJson); + + //Return the updated object - so we can instantly reset the entire array from the API response + //As opposed to push a new item into the array + return searches; + } + + public IReadOnlyList DeleteSavedSearch(string name, string query) + { + //Get the existing items + var searches = GetSavedSearches().ToList(); + + //Removes the search + searches.RemoveAll(s => s.Name.Equals(name) && s.Query.Equals(query)); + + //Serialize to JSON string + var rawJson = JsonConvert.SerializeObject(searches, Formatting.Indented); + + //Write it back down to file + System.IO.File.WriteAllText(_searchesConfig.FullName, rawJson); + + //Return the updated object - so we can instantly reset the entire array from the API response + return searches; + } + + private void EnsureFileExists() + { + if (_searchesConfig.Exists) return; + using (var writer = _searchesConfig.CreateText()) + { + writer.Write("[]"); + } + } + } +} diff --git a/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerSourceBase.cs b/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerSourceBase.cs index 4cc70eaf42..aae2976044 100644 --- a/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerSourceBase.cs +++ b/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerSourceBase.cs @@ -1,31 +1,20 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Xml; -using Newtonsoft.Json; using Serilog; using Serilog.Events; -using Umbraco.Core.Composing; -using Umbraco.Core.IO; using Umbraco.Core.Models; -using Umbraco.Core.Persistence.DatabaseModelDefinitions; -using Formatting = Newtonsoft.Json.Formatting; namespace Umbraco.Core.Logging.Viewer { + public abstract class LogViewerSourceBase : ILogViewer { - private readonly string _searchesConfigPath; - private readonly IIOHelper _ioHelper; + private readonly ILogViewerConfig _logViewerConfig; - protected LogViewerSourceBase(IIOHelper ioHelper, string pathToSearches = "") - { - if (string.IsNullOrEmpty(pathToSearches)) - // ReSharper disable once StringLiteralTypo - pathToSearches = ioHelper.MapPath("~/Config/logviewer.searches.config.js"); - - _searchesConfigPath = pathToSearches; - _ioHelper = ioHelper; + protected LogViewerSourceBase(ILogViewerConfig logViewerConfig) + { + _logViewerConfig = logViewerConfig; } public abstract bool CanHandleLargeLogs { get; } @@ -38,55 +27,13 @@ namespace Umbraco.Core.Logging.Viewer public abstract bool CheckCanOpenLogs(LogTimePeriod logTimePeriod); public virtual IReadOnlyList GetSavedSearches() - { - //Our default implementation - - //If file does not exist - lets create it with an empty array - EnsureFileExists(_searchesConfigPath, "[]", _ioHelper); - - var rawJson = System.IO.File.ReadAllText(_searchesConfigPath); - return JsonConvert.DeserializeObject(rawJson); - } + => _logViewerConfig.GetSavedSearches(); public virtual IReadOnlyList AddSavedSearch(string name, string query) - { - //Get the existing items - var searches = GetSavedSearches().ToList(); - - //Add the new item to the bottom of the list - searches.Add(new SavedLogSearch { Name = name, Query = query }); - - //Serialize to JSON string - var rawJson = JsonConvert.SerializeObject(searches, Formatting.Indented); - - //If file does not exist - lets create it with an empty array - EnsureFileExists(_searchesConfigPath, "[]", _ioHelper); - - //Write it back down to file - System.IO.File.WriteAllText(_searchesConfigPath, rawJson); - - //Return the updated object - so we can instantly reset the entire array from the API response - //As opposed to push a new item into the array - return searches; - } + => _logViewerConfig.AddSavedSearch(name, query); public virtual IReadOnlyList DeleteSavedSearch(string name, string query) - { - //Get the existing items - var searches = GetSavedSearches().ToList(); - - //Removes the search - searches.RemoveAll(s => s.Name.Equals(name) && s.Query.Equals(query)); - - //Serialize to JSON string - var rawJson = JsonConvert.SerializeObject(searches, Formatting.Indented); - - //Write it back down to file - System.IO.File.WriteAllText(_searchesConfigPath, rawJson); - - //Return the updated object - so we can instantly reset the entire array from the API response - return searches; - } + => _logViewerConfig.DeleteSavedSearch(name, query); public int GetNumberOfErrors(LogTimePeriod logTimePeriod) { @@ -182,15 +129,6 @@ namespace Umbraco.Core.Logging.Viewer }; } - private static void EnsureFileExists(string path, string contents, IIOHelper ioHelper) - { - var absolutePath = ioHelper.MapPath(path); - if (System.IO.File.Exists(absolutePath)) return; - - using (var writer = System.IO.File.CreateText(absolutePath)) - { - writer.Write(contents); - } - } + } } diff --git a/src/Umbraco.Tests/Logging/LogviewerTests.cs b/src/Umbraco.Tests/Logging/LogviewerTests.cs index 87cc19a2c6..75ff81a6d5 100644 --- a/src/Umbraco.Tests/Logging/LogviewerTests.cs +++ b/src/Umbraco.Tests/Logging/LogviewerTests.cs @@ -33,13 +33,14 @@ namespace Umbraco.Tests.Logging //Create an example JSON log file to check results //As a one time setup for all tets in this class/fixture var ioHelper = TestHelper.IOHelper; + var hostingEnv = TestHelper.GetHostingEnvironment(); var exampleLogfilePath = Path.Combine(TestContext.CurrentContext.TestDirectory, @"Logging\", _logfileName); - _newLogfileDirPath = Path.Combine(TestContext.CurrentContext.TestDirectory, @"App_Data\Logs\"); + _newLogfileDirPath = Path.Combine(hostingEnv.ApplicationPhysicalPath, @"App_Data\Logs\"); _newLogfilePath = Path.Combine(_newLogfileDirPath, _logfileName); var exampleSearchfilePath = Path.Combine(TestContext.CurrentContext.TestDirectory, @"Logging\", _searchfileName); - _newSearchfileDirPath = Path.Combine(TestContext.CurrentContext.TestDirectory, @"Config\"); + _newSearchfileDirPath = Path.Combine(hostingEnv.ApplicationPhysicalPath, @"Config\"); _newSearchfilePath = Path.Combine(_newSearchfileDirPath, _searchfileName); //Create/ensure Directory exists @@ -51,7 +52,8 @@ namespace Umbraco.Tests.Logging File.Copy(exampleSearchfilePath, _newSearchfilePath, true); var logger = Mock.Of(); - _logViewer = new JsonLogViewer(logger, ioHelper, logsPath: _newLogfileDirPath, searchPath: _newSearchfilePath); + var logViewerConfig = new LogViewerConfig(hostingEnv); + _logViewer = new JsonLogViewer(logger, logViewerConfig, hostingEnv); } [OneTimeTearDown] diff --git a/src/Umbraco.Web.Common/Extensions/HostBuilderExtensions.cs b/src/Umbraco.Web.Common/Extensions/HostBuilderExtensions.cs new file mode 100644 index 0000000000..3cb0922837 --- /dev/null +++ b/src/Umbraco.Web.Common/Extensions/HostBuilderExtensions.cs @@ -0,0 +1,21 @@ +using Microsoft.Extensions.Hosting; +using Serilog; +using Umbraco.Core.Composing; + +namespace Umbraco.Web.Common.Extensions +{ + public static class HostBuilderExtensions + { + /// + /// Assigns a custom service provider factory to use Umbraco's container + /// + /// + /// + public static IHostBuilder UseUmbraco(this IHostBuilder builder) + { + return builder + .UseSerilog() + .UseUmbraco(new UmbracoServiceProviderFactory()); + } + } +} diff --git a/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj b/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj index 7203c4ba29..711659730f 100644 --- a/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj +++ b/src/Umbraco.Web.Common/Umbraco.Web.Common.csproj @@ -18,6 +18,7 @@ + diff --git a/src/Umbraco.Web.UI.NetCore/Config/logviewer.searches.config.js b/src/Umbraco.Web.UI.NetCore/Config/logviewer.searches.config.js new file mode 100644 index 0000000000..345fe23764 --- /dev/null +++ b/src/Umbraco.Web.UI.NetCore/Config/logviewer.searches.config.js @@ -0,0 +1,42 @@ +[ + { + "name": "Find all logs where the Level is NOT Verbose and NOT Debug", + "query": "Not(@Level='Verbose') and Not(@Level='Debug')" + }, + { + "name": "Find all logs that has an exception property (Warning, Error & Fatal with Exceptions)", + "query": "Has(@Exception)" + }, + { + "name": "Find all logs that have the property 'Duration'", + "query": "Has(Duration)" + }, + { + "name": "Find all logs that have the property 'Duration' and the duration is greater than 1000ms", + "query": "Has(Duration) and Duration > 1000" + }, + { + "name": "Find all logs that are from the namespace 'Umbraco.Core'", + "query": "StartsWith(SourceContext, 'Umbraco.Core')" + }, + { + "name": "Find all logs that use a specific log message template", + "query": "@MessageTemplate = '[Timing {TimingId}] {EndMessage} ({TimingDuration}ms)'" + }, + { + "name": "Find logs where one of the items in the SortedComponentTypes property array is equal to", + "query": "SortedComponentTypes[?] = 'Umbraco.Web.Search.ExamineComponent'" + }, + { + "name": "Find logs where one of the items in the SortedComponentTypes property array contains", + "query": "Contains(SortedComponentTypes[?], 'DatabaseServer')" + }, + { + "name": "Find all logs that the message has localhost in it with SQL like", + "query": "@Message like '%localhost%'" + }, + { + "name": "Find all logs that the message that starts with 'end' in it with SQL like", + "query": "@Message like 'end%'" + } +] diff --git a/src/Umbraco.Web.UI.NetCore/Program.cs b/src/Umbraco.Web.UI.NetCore/Program.cs index f0504d77e3..1fe66f6664 100644 --- a/src/Umbraco.Web.UI.NetCore/Program.cs +++ b/src/Umbraco.Web.UI.NetCore/Program.cs @@ -1,8 +1,7 @@ using Microsoft.AspNetCore.Hosting; using Microsoft.Extensions.Hosting; -using Microsoft.Extensions.Logging; -using Serilog; using Umbraco.Core.Composing; +using Umbraco.Web.Common.Extensions; namespace Umbraco.Web.UI.BackOffice { diff --git a/src/Umbraco.Web.UI.NetCore/Umbraco.Web.UI.NetCore.csproj b/src/Umbraco.Web.UI.NetCore/Umbraco.Web.UI.NetCore.csproj index 72f29f3c4b..2a3b90d5f1 100644 --- a/src/Umbraco.Web.UI.NetCore/Umbraco.Web.UI.NetCore.csproj +++ b/src/Umbraco.Web.UI.NetCore/Umbraco.Web.UI.NetCore.csproj @@ -35,4 +35,12 @@ + + + + + + + + From d0f1ea0e595f31c97536b400a5a0bfeacc3899de Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 21 Apr 2020 13:31:55 +1000 Subject: [PATCH 26/39] Gets logging working in netcore, removes remaining usages of AppDomain.CurrentDomain.BaseDirectory and reduces more IOHelper usages --- .gitignore | 1 + .../Diagnostics/MiniDump.cs | 9 ++-- .../Logging/Serilog/LoggerConfigExtensions.cs | 29 +++++-------- .../Logging/Serilog/SerilogLogger.cs | 24 +++++------ src/Umbraco.Tests/Testing/UmbracoTestBase.cs | 2 +- .../UmbracoExamine/ExamineBaseTest.cs | 2 +- .../Config/serilog.Release.config | 41 +++++++++++++++++++ .../Config/serilog.config | 41 +++++++++++++++++++ .../Config/serilog.user.Release.config | 39 ++++++++++++++++++ .../Config/serilog.user.config | 39 ++++++++++++++++++ .../Umbraco.Web.UI.NetCore.csproj | 25 +++++++++++ 11 files changed, 214 insertions(+), 38 deletions(-) create mode 100644 src/Umbraco.Web.UI.NetCore/Config/serilog.Release.config create mode 100644 src/Umbraco.Web.UI.NetCore/Config/serilog.config create mode 100644 src/Umbraco.Web.UI.NetCore/Config/serilog.user.Release.config create mode 100644 src/Umbraco.Web.UI.NetCore/Config/serilog.user.config diff --git a/.gitignore b/.gitignore index 5f2432313f..d8c3f27d5a 100644 --- a/.gitignore +++ b/.gitignore @@ -176,3 +176,4 @@ build/temp/ /src/Umbraco.Web.UI.NetCore/wwwroot/Umbraco/lib/* /src/Umbraco.Web.UI.NetCore/wwwroot/Umbraco/views/* /src/Umbraco.Web.UI.NetCore/wwwroot/App_Data/TEMP/* +/src/Umbraco.Web.UI.NetCore/App_Data/Logs/* diff --git a/src/Umbraco.Infrastructure/Diagnostics/MiniDump.cs b/src/Umbraco.Infrastructure/Diagnostics/MiniDump.cs index 9bc0b1c3fb..57e9b5204b 100644 --- a/src/Umbraco.Infrastructure/Diagnostics/MiniDump.cs +++ b/src/Umbraco.Infrastructure/Diagnostics/MiniDump.cs @@ -3,6 +3,7 @@ using System.Diagnostics; using System.IO; using System.Runtime.InteropServices; using Umbraco.Core.Composing; +using Umbraco.Core.Hosting; using Umbraco.Core.IO; namespace Umbraco.Core.Diagnostics @@ -100,7 +101,7 @@ namespace Umbraco.Core.Diagnostics return bRet; } - public static bool Dump(IMarchal marchal, IIOHelper ioHelper, Option options = Option.WithFullMemory, bool withException = false) + public static bool Dump(IMarchal marchal, IHostingEnvironment hostingEnvironment, Option options = Option.WithFullMemory, bool withException = false) { lock (LockO) { @@ -110,7 +111,7 @@ namespace Umbraco.Core.Diagnostics // filter everywhere in our code = not! var stacktrace = withException ? Environment.StackTrace : string.Empty; - var filepath = ioHelper.MapPath("~/App_Data/MiniDump"); + var filepath = Path.Combine(hostingEnvironment.ApplicationPhysicalPath, "App_Data/MiniDump"); if (Directory.Exists(filepath) == false) Directory.CreateDirectory(filepath); @@ -122,11 +123,11 @@ namespace Umbraco.Core.Diagnostics } } - public static bool OkToDump(IIOHelper ioHelper) + public static bool OkToDump(IHostingEnvironment hostingEnvironment) { lock (LockO) { - var filepath = ioHelper.MapPath("~/App_Data/MiniDump"); + var filepath = Path.Combine(hostingEnvironment.ApplicationPhysicalPath, "App_Data/MiniDump"); if (Directory.Exists(filepath) == false) return true; var count = Directory.GetFiles(filepath, "*.dmp").Length; return count < 8; diff --git a/src/Umbraco.Infrastructure/Logging/Serilog/LoggerConfigExtensions.cs b/src/Umbraco.Infrastructure/Logging/Serilog/LoggerConfigExtensions.cs index d810a08648..b0521c6da8 100644 --- a/src/Umbraco.Infrastructure/Logging/Serilog/LoggerConfigExtensions.cs +++ b/src/Umbraco.Infrastructure/Logging/Serilog/LoggerConfigExtensions.cs @@ -1,4 +1,5 @@ using System; +using System.IO; using System.Text; using Serilog; using Serilog.Configuration; @@ -29,11 +30,9 @@ namespace Umbraco.Core.Logging.Serilog { global::Serilog.Debugging.SelfLog.Enable(msg => System.Diagnostics.Debug.WriteLine(msg)); - // TODO: we can't use AppDomain.CurrentDomain.BaseDirectory, need a consistent approach between netcore/netframework - //Set this environment variable - so that it can be used in external config file //add key="serilog:write-to:RollingFile.pathFormat" value="%BASEDIR%\logs\log.txt" /> - Environment.SetEnvironmentVariable("BASEDIR", AppDomain.CurrentDomain.BaseDirectory, EnvironmentVariableTarget.Process); + Environment.SetEnvironmentVariable("BASEDIR", hostingEnvironment.ApplicationPhysicalPath, EnvironmentVariableTarget.Process); Environment.SetEnvironmentVariable("MACHINENAME", Environment.MachineName, EnvironmentVariableTarget.Process); logConfig.MinimumLevel.Verbose() //Set to highest level of logging (as any sinks may want to restrict it to Errors only) @@ -57,13 +56,11 @@ namespace Umbraco.Core.Logging.Serilog /// A Serilog LoggerConfiguration /// The log level you wish the JSON file to collect - default is Verbose (highest) /// The number of days to keep log files. Default is set to null which means all logs are kept - public static LoggerConfiguration OutputDefaultTextFile(this LoggerConfiguration logConfig, LogEventLevel minimumLevel = LogEventLevel.Verbose, int? retainedFileCount = null) + public static LoggerConfiguration OutputDefaultTextFile(this LoggerConfiguration logConfig, IHostingEnvironment hostingEnvironment, LogEventLevel minimumLevel = LogEventLevel.Verbose, int? retainedFileCount = null) { - // TODO: we can't use AppDomain.CurrentDomain.BaseDirectory, need a consistent approach between netcore/netframework - //Main .txt logfile - in similar format to older Log4Net output //Ends with ..txt as Date is inserted before file extension substring - logConfig.WriteTo.File($@"{AppDomain.CurrentDomain.BaseDirectory}\App_Data\Logs\UmbracoTraceLog.{Environment.MachineName}..txt", + logConfig.WriteTo.File(Path.Combine(hostingEnvironment.ApplicationPhysicalPath, $@"App_Data\Logs\UmbracoTraceLog.{Environment.MachineName}..txt"), shared: true, rollingInterval: RollingInterval.Day, restrictedToMinimumLevel: minimumLevel, @@ -117,13 +114,11 @@ namespace Umbraco.Core.Logging.Serilog /// A Serilog LoggerConfiguration /// The log level you wish the JSON file to collect - default is Verbose (highest) /// The number of days to keep log files. Default is set to null which means all logs are kept - public static LoggerConfiguration OutputDefaultJsonFile(this LoggerConfiguration logConfig, LogEventLevel minimumLevel = LogEventLevel.Verbose, int? retainedFileCount = null) + public static LoggerConfiguration OutputDefaultJsonFile(this LoggerConfiguration logConfig, IHostingEnvironment hostingEnvironment, LogEventLevel minimumLevel = LogEventLevel.Verbose, int? retainedFileCount = null) { - // TODO: we can't use AppDomain.CurrentDomain.BaseDirectory, need a consistent approach between netcore/netframework - //.clef format (Compact log event format, that can be imported into local SEQ & will make searching/filtering logs easier) //Ends with ..txt as Date is inserted before file extension substring - logConfig.WriteTo.File(new CompactJsonFormatter(), $@"{AppDomain.CurrentDomain.BaseDirectory}\App_Data\Logs\UmbracoTraceLog.{Environment.MachineName}..json", + logConfig.WriteTo.File(new CompactJsonFormatter(), Path.Combine(hostingEnvironment.ApplicationPhysicalPath, $@"App_Data\Logs\UmbracoTraceLog.{Environment.MachineName}..json"), shared: true, rollingInterval: RollingInterval.Day, //Create a new JSON file every day retainedFileCountLimit: retainedFileCount, //Setting to null means we keep all files - default is 31 days @@ -137,12 +132,10 @@ namespace Umbraco.Core.Logging.Serilog /// That allows the main logging pipeline to be configured /// /// A Serilog LoggerConfiguration - public static LoggerConfiguration ReadFromConfigFile(this LoggerConfiguration logConfig) + public static LoggerConfiguration ReadFromConfigFile(this LoggerConfiguration logConfig, IHostingEnvironment hostingEnvironment) { - // TODO: we can't use AppDomain.CurrentDomain.BaseDirectory, need a consistent approach between netcore/netframework - //Read from main serilog.config file - logConfig.ReadFrom.AppSettings(filePath: AppDomain.CurrentDomain.BaseDirectory + @"\config\serilog.config"); + logConfig.ReadFrom.AppSettings(filePath: Path.Combine(hostingEnvironment.ApplicationPhysicalPath, @"config\serilog.config")); return logConfig; } @@ -152,13 +145,11 @@ namespace Umbraco.Core.Logging.Serilog /// That allows a separate logging pipeline to be configured that will not affect the main Umbraco log /// /// A Serilog LoggerConfiguration - public static LoggerConfiguration ReadFromUserConfigFile(this LoggerConfiguration logConfig) + public static LoggerConfiguration ReadFromUserConfigFile(this LoggerConfiguration logConfig, IHostingEnvironment hostingEnvironment) { - // TODO: we can't use AppDomain.CurrentDomain.BaseDirectory, need a consistent approach between netcore/netframework - //A nested logger - where any user configured sinks via config can not effect the main 'umbraco' logger above logConfig.WriteTo.Logger(cfg => - cfg.ReadFrom.AppSettings(filePath: AppDomain.CurrentDomain.BaseDirectory + @"\config\serilog.user.config")); + cfg.ReadFrom.AppSettings(filePath: Path.Combine(hostingEnvironment.ApplicationPhysicalPath, @"config\serilog.user.config"))); return logConfig; } diff --git a/src/Umbraco.Infrastructure/Logging/Serilog/SerilogLogger.cs b/src/Umbraco.Infrastructure/Logging/Serilog/SerilogLogger.cs index 497de58bdd..e4695dedd1 100644 --- a/src/Umbraco.Infrastructure/Logging/Serilog/SerilogLogger.cs +++ b/src/Umbraco.Infrastructure/Logging/Serilog/SerilogLogger.cs @@ -19,30 +19,28 @@ namespace Umbraco.Core.Logging.Serilog public class SerilogLogger : ILogger, IDisposable { private readonly ICoreDebugSettings _coreDebugSettings; - private readonly IIOHelper _ioHelper; + private readonly IHostingEnvironment _hostingEnvironment; private readonly IMarchal _marchal; /// /// Initialize a new instance of the class with a configuration file. /// /// - public SerilogLogger(ICoreDebugSettings coreDebugSettings, IIOHelper ioHelper, IMarchal marchal, FileInfo logConfigFile) + public SerilogLogger(ICoreDebugSettings coreDebugSettings, IHostingEnvironment hostingEnvironment, IMarchal marchal, FileInfo logConfigFile) { _coreDebugSettings = coreDebugSettings; - _ioHelper = ioHelper; + _hostingEnvironment = hostingEnvironment; _marchal = marchal; - // TODO: we can't use AppDomain.CurrentDomain.BaseDirectory, need a consistent approach between netcore/netframework - Log.Logger = new LoggerConfiguration() - .ReadFrom.AppSettings(filePath: AppDomain.CurrentDomain.BaseDirectory + logConfigFile) + .ReadFrom.AppSettings(filePath: logConfigFile.FullName) .CreateLogger(); } - public SerilogLogger(ICoreDebugSettings coreDebugSettings, IIOHelper ioHelper, IMarchal marchal, LoggerConfiguration logConfig) + public SerilogLogger(ICoreDebugSettings coreDebugSettings, IHostingEnvironment hostingEnvironment, IMarchal marchal, LoggerConfiguration logConfig) { _coreDebugSettings = coreDebugSettings; - _ioHelper = ioHelper; + _hostingEnvironment = hostingEnvironment; _marchal = marchal; //Configure Serilog static global logger with config passed in @@ -58,10 +56,10 @@ namespace Umbraco.Core.Logging.Serilog var loggerConfig = new LoggerConfiguration(); loggerConfig .MinimalConfiguration(hostingEnvironment, sessionIdResolver, requestCacheGetter) - .ReadFromConfigFile() - .ReadFromUserConfigFile(); + .ReadFromConfigFile(hostingEnvironment) + .ReadFromUserConfigFile(hostingEnvironment); - return new SerilogLogger(coreDebugSettings, ioHelper, marchal, loggerConfig); + return new SerilogLogger(coreDebugSettings, hostingEnvironment, marchal, loggerConfig); } /// @@ -184,14 +182,14 @@ namespace Umbraco.Core.Logging.Serilog dump = _coreDebugSettings.DumpOnTimeoutThreadAbort || IsMonitorEnterThreadAbortException(exception); // dump if it is ok to dump (might have a cap on number of dump...) - dump &= MiniDump.OkToDump(_ioHelper); + dump &= MiniDump.OkToDump(_hostingEnvironment); } if (dump) { try { - var dumped = MiniDump.Dump(_marchal, _ioHelper, withException: true); + var dumped = MiniDump.Dump(_marchal, _hostingEnvironment, withException: true); messageTemplate += dumped ? "\r\nA minidump was created in App_Data/MiniDump" : "\r\nFailed to create a minidump"; diff --git a/src/Umbraco.Tests/Testing/UmbracoTestBase.cs b/src/Umbraco.Tests/Testing/UmbracoTestBase.cs index 901192c609..f62a69177a 100644 --- a/src/Umbraco.Tests/Testing/UmbracoTestBase.cs +++ b/src/Umbraco.Tests/Testing/UmbracoTestBase.cs @@ -264,7 +264,7 @@ namespace Umbraco.Tests.Testing profiler = Mock.Of(); break; case UmbracoTestOptions.Logger.Serilog: - logger = new SerilogLogger(TestHelper.CoreDebugSettings, IOHelper, TestHelper.Marchal, new FileInfo(TestHelper.MapPathForTestFiles("~/unit-test.config"))); + logger = new SerilogLogger(TestHelper.CoreDebugSettings, HostingEnvironment, TestHelper.Marchal, new FileInfo(TestHelper.MapPathForTestFiles("~/unit-test.config"))); profiler = new LogProfiler(logger); break; case UmbracoTestOptions.Logger.Console: diff --git a/src/Umbraco.Tests/UmbracoExamine/ExamineBaseTest.cs b/src/Umbraco.Tests/UmbracoExamine/ExamineBaseTest.cs index a6c544062e..76b989a7a3 100644 --- a/src/Umbraco.Tests/UmbracoExamine/ExamineBaseTest.cs +++ b/src/Umbraco.Tests/UmbracoExamine/ExamineBaseTest.cs @@ -18,7 +18,7 @@ namespace Umbraco.Tests.UmbracoExamine public void InitializeFixture() { - var logger = new SerilogLogger(TestHelper.CoreDebugSettings, IOHelper, TestHelper.Marchal, new FileInfo(TestHelper.MapPathForTestFiles("~/unit-test.config"))); + var logger = new SerilogLogger(TestHelper.CoreDebugSettings, HostingEnvironment, TestHelper.Marchal, new FileInfo(TestHelper.MapPathForTestFiles("~/unit-test.config"))); _profilingLogger = new ProfilingLogger(logger, new LogProfiler(logger)); } diff --git a/src/Umbraco.Web.UI.NetCore/Config/serilog.Release.config b/src/Umbraco.Web.UI.NetCore/Config/serilog.Release.config new file mode 100644 index 0000000000..e3cf52b3c5 --- /dev/null +++ b/src/Umbraco.Web.UI.NetCore/Config/serilog.Release.config @@ -0,0 +1,41 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/Umbraco.Web.UI.NetCore/Config/serilog.config b/src/Umbraco.Web.UI.NetCore/Config/serilog.config new file mode 100644 index 0000000000..e3cf52b3c5 --- /dev/null +++ b/src/Umbraco.Web.UI.NetCore/Config/serilog.config @@ -0,0 +1,41 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/Umbraco.Web.UI.NetCore/Config/serilog.user.Release.config b/src/Umbraco.Web.UI.NetCore/Config/serilog.user.Release.config new file mode 100644 index 0000000000..8f207406e3 --- /dev/null +++ b/src/Umbraco.Web.UI.NetCore/Config/serilog.user.Release.config @@ -0,0 +1,39 @@ + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/Umbraco.Web.UI.NetCore/Config/serilog.user.config b/src/Umbraco.Web.UI.NetCore/Config/serilog.user.config new file mode 100644 index 0000000000..8f207406e3 --- /dev/null +++ b/src/Umbraco.Web.UI.NetCore/Config/serilog.user.config @@ -0,0 +1,39 @@ + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/Umbraco.Web.UI.NetCore/Umbraco.Web.UI.NetCore.csproj b/src/Umbraco.Web.UI.NetCore/Umbraco.Web.UI.NetCore.csproj index 2a3b90d5f1..59bb76bb7a 100644 --- a/src/Umbraco.Web.UI.NetCore/Umbraco.Web.UI.NetCore.csproj +++ b/src/Umbraco.Web.UI.NetCore/Umbraco.Web.UI.NetCore.csproj @@ -35,6 +35,11 @@ + + + + + @@ -43,4 +48,24 @@ + + + Designer + serilog.config + + + Designer + serilog.user.config + + + + + + Designer + + + Designer + + + From 77a06efa911c4bbe2174aeba639946d8d850c9b6 Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Tue, 21 Apr 2020 09:19:18 +0200 Subject: [PATCH 27/39] Moved filters to Umbraco.Web.Common project --- .../Events}/ActionExecutedEventArgs.cs | 2 +- .../Filters/DisableBrowserCacheAttribute.cs | 14 ++++++++------ .../Filters/PreRenderViewActionFilterAttribute.cs | 3 ++- .../Filters/StatusCodeResultAttribute.cs | 14 ++++++++------ 4 files changed, 19 insertions(+), 14 deletions(-) rename src/{Umbraco.Web.BackOffice => Umbraco.Web.Common/Events}/ActionExecutedEventArgs.cs (91%) rename src/{Umbraco.Web.BackOffice => Umbraco.Web.Common}/Filters/DisableBrowserCacheAttribute.cs (57%) rename src/{Umbraco.Web.BackOffice => Umbraco.Web.Common}/Filters/PreRenderViewActionFilterAttribute.cs (93%) rename src/{Umbraco.Web.BackOffice => Umbraco.Web.Common}/Filters/StatusCodeResultAttribute.cs (67%) diff --git a/src/Umbraco.Web.BackOffice/ActionExecutedEventArgs.cs b/src/Umbraco.Web.Common/Events/ActionExecutedEventArgs.cs similarity index 91% rename from src/Umbraco.Web.BackOffice/ActionExecutedEventArgs.cs rename to src/Umbraco.Web.Common/Events/ActionExecutedEventArgs.cs index a2ac5701be..b33cbc7d8a 100644 --- a/src/Umbraco.Web.BackOffice/ActionExecutedEventArgs.cs +++ b/src/Umbraco.Web.Common/Events/ActionExecutedEventArgs.cs @@ -1,7 +1,7 @@ using System; using Microsoft.AspNetCore.Mvc; -namespace Umbraco.Web.BackOffice +namespace Umbraco.Web.Common.Events { public class ActionExecutedEventArgs : EventArgs { diff --git a/src/Umbraco.Web.BackOffice/Filters/DisableBrowserCacheAttribute.cs b/src/Umbraco.Web.Common/Filters/DisableBrowserCacheAttribute.cs similarity index 57% rename from src/Umbraco.Web.BackOffice/Filters/DisableBrowserCacheAttribute.cs rename to src/Umbraco.Web.Common/Filters/DisableBrowserCacheAttribute.cs index e2dc357fa9..0fe251bac4 100644 --- a/src/Umbraco.Web.BackOffice/Filters/DisableBrowserCacheAttribute.cs +++ b/src/Umbraco.Web.Common/Filters/DisableBrowserCacheAttribute.cs @@ -3,7 +3,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.Net.Http.Headers; -namespace Umbraco.Web.BackOffice.Filters +namespace Umbraco.Web.Common.Filters { /// /// Ensures that the request is not cached by the browser @@ -14,9 +14,11 @@ namespace Umbraco.Web.BackOffice.Filters { base.OnResultExecuting(context); - if (context.HttpContext.Response.StatusCode != 200) return; + var httpResponse = context.HttpContext.Response; - context.HttpContext.Response.GetTypedHeaders().CacheControl = + if (httpResponse.StatusCode != 200) return; + + httpResponse.GetTypedHeaders().CacheControl = new CacheControlHeaderValue() { NoCache = true, @@ -25,9 +27,9 @@ namespace Umbraco.Web.BackOffice.Filters NoStore = true }; - context.HttpContext.Response.Headers[HeaderNames.LastModified] = DateTime.Now.ToString("R"); // Format RFC1123 - context.HttpContext.Response.Headers[HeaderNames.Pragma] = "no-cache"; - context.HttpContext.Response.Headers[HeaderNames.Expires] = new DateTime(1990, 1, 1, 0, 0, 0).ToString("R"); + httpResponse.Headers[HeaderNames.LastModified] = DateTime.Now.ToString("R"); // Format RFC1123 + httpResponse.Headers[HeaderNames.Pragma] = "no-cache"; + httpResponse.Headers[HeaderNames.Expires] = new DateTime(1990, 1, 1, 0, 0, 0).ToString("R"); } } } diff --git a/src/Umbraco.Web.BackOffice/Filters/PreRenderViewActionFilterAttribute.cs b/src/Umbraco.Web.Common/Filters/PreRenderViewActionFilterAttribute.cs similarity index 93% rename from src/Umbraco.Web.BackOffice/Filters/PreRenderViewActionFilterAttribute.cs rename to src/Umbraco.Web.Common/Filters/PreRenderViewActionFilterAttribute.cs index 8c8a9a135a..2ba58a8fd7 100644 --- a/src/Umbraco.Web.BackOffice/Filters/PreRenderViewActionFilterAttribute.cs +++ b/src/Umbraco.Web.Common/Filters/PreRenderViewActionFilterAttribute.cs @@ -1,8 +1,9 @@ using System; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Filters; +using Umbraco.Web.Common.Events; -namespace Umbraco.Web.BackOffice.Filters +namespace Umbraco.Web.Common.Filters { public class PreRenderViewActionFilterAttribute : ActionFilterAttribute { diff --git a/src/Umbraco.Web.BackOffice/Filters/StatusCodeResultAttribute.cs b/src/Umbraco.Web.Common/Filters/StatusCodeResultAttribute.cs similarity index 67% rename from src/Umbraco.Web.BackOffice/Filters/StatusCodeResultAttribute.cs rename to src/Umbraco.Web.Common/Filters/StatusCodeResultAttribute.cs index 870c016b38..8f3fcf3a95 100644 --- a/src/Umbraco.Web.BackOffice/Filters/StatusCodeResultAttribute.cs +++ b/src/Umbraco.Web.Common/Filters/StatusCodeResultAttribute.cs @@ -1,10 +1,10 @@ -using Microsoft.AspNetCore.Mvc.Filters; -using System.Net; +using System.Net; using Microsoft.AspNetCore.Diagnostics; +using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.Extensions.DependencyInjection; using Umbraco.Core.Configuration.UmbracoSettings; -namespace Umbraco.Web.BackOffice.Filters +namespace Umbraco.Web.Common.Filters { /// /// Forces the response to have a specific http status code @@ -22,10 +22,12 @@ namespace Umbraco.Web.BackOffice.Filters { base.OnActionExecuted(context); - context.HttpContext.Response.StatusCode = (int)_statusCode; + var httpContext = context.HttpContext; - var disableIisCustomErrors = context.HttpContext.RequestServices.GetService().TrySkipIisCustomErrors; - var statusCodePagesFeature = context.HttpContext.Features.Get(); + httpContext.Response.StatusCode = (int)_statusCode; + + var disableIisCustomErrors = httpContext.RequestServices.GetService().TrySkipIisCustomErrors; + var statusCodePagesFeature = httpContext.Features.Get(); if (statusCodePagesFeature != null) { From 65b45eb48d41430a437444007ffe8a43eef63722 Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Tue, 21 Apr 2020 09:25:15 +0200 Subject: [PATCH 28/39] Marking classes as moved --- src/Umbraco.Web/Mvc/ActionExecutedEventArgs.cs | 1 + src/Umbraco.Web/Mvc/Constants.cs | 1 + .../Mvc/EnsurePartialViewMacroViewContextFilterAttribute.cs | 1 + src/Umbraco.Web/Mvc/IRenderController.cs | 1 + 4 files changed, 4 insertions(+) diff --git a/src/Umbraco.Web/Mvc/ActionExecutedEventArgs.cs b/src/Umbraco.Web/Mvc/ActionExecutedEventArgs.cs index 1c596ff80c..6904aa103a 100644 --- a/src/Umbraco.Web/Mvc/ActionExecutedEventArgs.cs +++ b/src/Umbraco.Web/Mvc/ActionExecutedEventArgs.cs @@ -3,6 +3,7 @@ using System.Web.Mvc; namespace Umbraco.Web.Mvc { + /// Migrated already to .Net Core public class ActionExecutedEventArgs : EventArgs { public Controller Controller { get; set; } diff --git a/src/Umbraco.Web/Mvc/Constants.cs b/src/Umbraco.Web/Mvc/Constants.cs index 1794345746..c71ed6b104 100644 --- a/src/Umbraco.Web/Mvc/Constants.cs +++ b/src/Umbraco.Web/Mvc/Constants.cs @@ -3,6 +3,7 @@ /// /// constants /// + /// Migrated already to .Net Core internal static class Constants { internal const string ViewLocation = "~/Views"; diff --git a/src/Umbraco.Web/Mvc/EnsurePartialViewMacroViewContextFilterAttribute.cs b/src/Umbraco.Web/Mvc/EnsurePartialViewMacroViewContextFilterAttribute.cs index 34b857dfb4..f443abbb70 100644 --- a/src/Umbraco.Web/Mvc/EnsurePartialViewMacroViewContextFilterAttribute.cs +++ b/src/Umbraco.Web/Mvc/EnsurePartialViewMacroViewContextFilterAttribute.cs @@ -19,6 +19,7 @@ namespace Umbraco.Web.Mvc /// this DataToken exists before the action executes in case the developer resolves an RTE value that contains /// a partial view macro form. /// + /// Migrated already to .Net Core internal class EnsurePartialViewMacroViewContextFilterAttribute : ActionFilterAttribute { /// diff --git a/src/Umbraco.Web/Mvc/IRenderController.cs b/src/Umbraco.Web/Mvc/IRenderController.cs index 103a484869..0de585959c 100644 --- a/src/Umbraco.Web/Mvc/IRenderController.cs +++ b/src/Umbraco.Web/Mvc/IRenderController.cs @@ -5,6 +5,7 @@ namespace Umbraco.Web.Mvc /// /// A marker interface to designate that a controller will be used for Umbraco front-end requests and/or route hijacking /// + /// Migrated already to .Net Core public interface IRenderController : IController { From 138b8099fc119408241b5d1ba5f6470affae2981 Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Tue, 21 Apr 2020 09:28:22 +0200 Subject: [PATCH 29/39] Reimplementing EnsurePartialViewMacroViewContextFilterAttribute and other required classes --- src/Umbraco.Web.Common/Constants/Constants.cs | 12 +++ .../Controllers/RenderController.cs | 9 ++ ...tialViewMacroViewContextFilterAttribute.cs | 87 +++++++++++++++++++ 3 files changed, 108 insertions(+) create mode 100644 src/Umbraco.Web.Common/Constants/Constants.cs create mode 100644 src/Umbraco.Web.Common/Controllers/RenderController.cs create mode 100644 src/Umbraco.Web.Common/Filters/EnsurePartialViewMacroViewContextFilterAttribute.cs diff --git a/src/Umbraco.Web.Common/Constants/Constants.cs b/src/Umbraco.Web.Common/Constants/Constants.cs new file mode 100644 index 0000000000..1acbe761c1 --- /dev/null +++ b/src/Umbraco.Web.Common/Constants/Constants.cs @@ -0,0 +1,12 @@ +namespace Umbraco.Web.Common.Constants +{ + /// + /// constants + /// + internal static class Constants + { + internal const string ViewLocation = "~/Views"; + + internal const string DataTokenCurrentViewContext = "umbraco-current-view-context"; + } +} diff --git a/src/Umbraco.Web.Common/Controllers/RenderController.cs b/src/Umbraco.Web.Common/Controllers/RenderController.cs new file mode 100644 index 0000000000..43058616de --- /dev/null +++ b/src/Umbraco.Web.Common/Controllers/RenderController.cs @@ -0,0 +1,9 @@ +using Microsoft.AspNetCore.Mvc; + +namespace Umbraco.Web.Common.Controllers +{ + public abstract class RenderController : Controller + { + + } +} diff --git a/src/Umbraco.Web.Common/Filters/EnsurePartialViewMacroViewContextFilterAttribute.cs b/src/Umbraco.Web.Common/Filters/EnsurePartialViewMacroViewContextFilterAttribute.cs new file mode 100644 index 0000000000..bddb8b3653 --- /dev/null +++ b/src/Umbraco.Web.Common/Filters/EnsurePartialViewMacroViewContextFilterAttribute.cs @@ -0,0 +1,87 @@ +using System.IO; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.AspNetCore.Mvc.Rendering; +using Microsoft.AspNetCore.Mvc.ViewEngines; +using Microsoft.AspNetCore.Mvc.ViewFeatures; +using Umbraco.Web.Common.Controllers; + +namespace Umbraco.Web.Common.Filters +{ + /// + /// This is a special filter which is required for the RTE to be able to render Partial View Macros that + /// contain forms when the RTE value is resolved outside of an MVC view being rendered + /// + /// + /// The entire way that we support partial view macros that contain forms isn't really great, these forms + /// need to be executed as ChildActions so that the ModelState,ViewData,TempData get merged into that action + /// so the form can show errors, viewdata, etc... + /// Under normal circumstances, macros will be rendered after a ViewContext is created but in some cases + /// developers will resolve the RTE value in the controller, in this case the Form won't be rendered correctly + /// with merged ModelState from the controller because the special DataToken hasn't been set yet (which is + /// normally done in the UmbracoViewPageOfModel when a real ViewContext is available. + /// So we need to detect if the currently rendering controller is IRenderController and if so we'll ensure that + /// this DataToken exists before the action executes in case the developer resolves an RTE value that contains + /// a partial view macro form. + /// + internal class EnsurePartialViewMacroViewContextFilterAttribute : ActionFilterAttribute + { + + /// + /// Ensures the custom ViewContext datatoken is set before the RenderController action is invoked, + /// this ensures that any calls to GetPropertyValue with regards to RTE or Grid editors can still + /// render any PartialViewMacro with a form and maintain ModelState + /// + /// + public override void OnActionExecuting(ActionExecutingContext context) + { + if (!(context.Controller is Controller controller)) return; + + //ignore anything that is not IRenderController + if (!(controller is RenderController)) return; + + SetViewContext(context, controller); + } + + /// + /// Ensures that the custom ViewContext datatoken is set after the RenderController action is invoked, + /// this ensures that any custom ModelState that may have been added in the RenderController itself is + /// passed onwards in case it is required when rendering a PartialViewMacro with a form + /// + /// The filter context. + public override void OnResultExecuting(ResultExecutingContext context) + { + if (!(context.Controller is Controller controller)) return; + + //ignore anything that is not IRenderController + if (!(controller is RenderController)) return; + + SetViewContext(context, controller); + } + + private void SetViewContext(ActionContext context, Controller controller) + { + var viewCtx = new ViewContext( + context, + new DummyView(), + controller.ViewData, + controller.TempData, + new StringWriter(), + new HtmlHelperOptions()); + + //set the special data token + context.RouteData.DataTokens[Constants.Constants.DataTokenCurrentViewContext] = viewCtx; + } + + private class DummyView : IView + { + public Task RenderAsync(ViewContext context) + { + return Task.CompletedTask; + } + + public string Path { get; } + } + } +} From 16aab7a3cbd4a2d6d39cac14ea5f3f5b94c4480e Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Tue, 21 Apr 2020 09:33:39 +0200 Subject: [PATCH 30/39] Refactoring --- .../Filters/MinifyJavaScriptResultAttribute.cs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/Filters/MinifyJavaScriptResultAttribute.cs b/src/Umbraco.Web.BackOffice/Filters/MinifyJavaScriptResultAttribute.cs index 0ed0fd658a..65c2be051f 100644 --- a/src/Umbraco.Web.BackOffice/Filters/MinifyJavaScriptResultAttribute.cs +++ b/src/Umbraco.Web.BackOffice/Filters/MinifyJavaScriptResultAttribute.cs @@ -2,9 +2,7 @@ using Microsoft.AspNetCore.Mvc.Filters; using Umbraco.Core.Hosting; using Microsoft.Extensions.DependencyInjection; -using Umbraco.Core.Runtime; using Umbraco.Core.WebAssets; -using Umbraco.Web.BackOffice.Controllers; using Umbraco.Web.Common.ActionResults; namespace Umbraco.Web.BackOffice.Filters @@ -14,10 +12,11 @@ namespace Umbraco.Web.BackOffice.Filters public override async Task OnResultExecutionAsync(ResultExecutingContext context, ResultExecutionDelegate next) { // logic before action goes here - var hostingEnvironment = context.HttpContext.RequestServices.GetService(); + var serviceProvider = context.HttpContext.RequestServices; + var hostingEnvironment = serviceProvider.GetService(); if (!hostingEnvironment.IsDebugMode) { - var runtimeMinifier = context.HttpContext.RequestServices.GetService(); + var runtimeMinifier = serviceProvider.GetService(); if (context.Result is JavaScriptResult jsResult) { From 28f5033cb727244747077cd13dd11bc9a5e7780c Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Tue, 21 Apr 2020 10:14:03 +0200 Subject: [PATCH 31/39] Renaming a file for constants. Clarifying the proj for other Constants file --- .../Constants/{Constants.cs => ViewConstants.cs} | 2 +- .../Extensions/UmbracoCoreServiceCollectionExtensions.cs | 4 ++-- .../EnsurePartialViewMacroViewContextFilterAttribute.cs | 3 ++- .../RuntimeMinification/SmidgeRuntimeMinifier.cs | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) rename src/Umbraco.Web.Common/Constants/{Constants.cs => ViewConstants.cs} (86%) diff --git a/src/Umbraco.Web.Common/Constants/Constants.cs b/src/Umbraco.Web.Common/Constants/ViewConstants.cs similarity index 86% rename from src/Umbraco.Web.Common/Constants/Constants.cs rename to src/Umbraco.Web.Common/Constants/ViewConstants.cs index 1acbe761c1..5da1a74f55 100644 --- a/src/Umbraco.Web.Common/Constants/Constants.cs +++ b/src/Umbraco.Web.Common/Constants/ViewConstants.cs @@ -3,7 +3,7 @@ /// /// constants /// - internal static class Constants + internal static class ViewConstants { internal const string ViewLocation = "~/Views"; diff --git a/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs b/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs index 843620d571..74477bbe6c 100644 --- a/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs @@ -130,7 +130,7 @@ namespace Umbraco.Web.Common.Extensions IProfiler profiler, Core.Hosting.IHostingEnvironment hostingEnvironment, IBackOfficeInfo backOfficeInfo, ITypeFinder typeFinder) { - var connectionStringConfig = configs.ConnectionStrings()[Constants.System.UmbracoConnectionName]; + var connectionStringConfig = configs.ConnectionStrings()[Core.Constants.System.UmbracoConnectionName]; var dbProviderFactoryCreator = new SqlServerDbProviderFactoryCreator( connectionStringConfig?.ProviderName, DbProviderFactories.GetFactory); @@ -185,7 +185,7 @@ namespace Umbraco.Web.Common.Extensions public static IServiceCollection AddUmbracoRuntimeMinifier(this IServiceCollection services, IConfiguration configuration) { - services.AddSmidge(configuration.GetSection(Constants.Configuration.ConfigRuntimeMinification)); + services.AddSmidge(configuration.GetSection(Core.Constants.Configuration.ConfigRuntimeMinification)); services.AddSmidgeNuglify(); return services; diff --git a/src/Umbraco.Web.Common/Filters/EnsurePartialViewMacroViewContextFilterAttribute.cs b/src/Umbraco.Web.Common/Filters/EnsurePartialViewMacroViewContextFilterAttribute.cs index bddb8b3653..e2046209d5 100644 --- a/src/Umbraco.Web.Common/Filters/EnsurePartialViewMacroViewContextFilterAttribute.cs +++ b/src/Umbraco.Web.Common/Filters/EnsurePartialViewMacroViewContextFilterAttribute.cs @@ -5,6 +5,7 @@ using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.Rendering; using Microsoft.AspNetCore.Mvc.ViewEngines; using Microsoft.AspNetCore.Mvc.ViewFeatures; +using Umbraco.Web.Common.Constants; using Umbraco.Web.Common.Controllers; namespace Umbraco.Web.Common.Filters @@ -71,7 +72,7 @@ namespace Umbraco.Web.Common.Filters new HtmlHelperOptions()); //set the special data token - context.RouteData.DataTokens[Constants.Constants.DataTokenCurrentViewContext] = viewCtx; + context.RouteData.DataTokens[ViewConstants.DataTokenCurrentViewContext] = viewCtx; } private class DummyView : IView diff --git a/src/Umbraco.Web.Common/RuntimeMinification/SmidgeRuntimeMinifier.cs b/src/Umbraco.Web.Common/RuntimeMinification/SmidgeRuntimeMinifier.cs index ba3c6c289f..c4b9522099 100644 --- a/src/Umbraco.Web.Common/RuntimeMinification/SmidgeRuntimeMinifier.cs +++ b/src/Umbraco.Web.Common/RuntimeMinification/SmidgeRuntimeMinifier.cs @@ -112,7 +112,7 @@ namespace Umbraco.Web.Common.RuntimeMinification public void Reset() { var version = DateTime.UtcNow.Ticks.ToString(); - _configManipulator.SaveConfigValue(Constants.Configuration.ConfigRuntimeMinificationVersion, version.ToString()); + _configManipulator.SaveConfigValue(Core.Constants.Configuration.ConfigRuntimeMinificationVersion, version.ToString()); } From c185b67ed0551d61f04866ec4a927d1f5a957d15 Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Tue, 21 Apr 2020 10:55:01 +0200 Subject: [PATCH 32/39] Adding UseStatusCodePages for disabling/enabling status code pages (ex: from IIS) --- src/Umbraco.Web.UI.NetCore/Startup.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Umbraco.Web.UI.NetCore/Startup.cs b/src/Umbraco.Web.UI.NetCore/Startup.cs index 75b2d6f48e..b986ac2c17 100644 --- a/src/Umbraco.Web.UI.NetCore/Startup.cs +++ b/src/Umbraco.Web.UI.NetCore/Startup.cs @@ -75,6 +75,7 @@ namespace Umbraco.Web.UI.BackOffice { app.UseDeveloperExceptionPage(); } + app.UseStatusCodePages(); app.UseUmbracoCore(); app.UseUmbracoWebsite(); app.UseUmbracoBackOffice(); From fbe6f6cdd2836f6fe82385c60adfcda47a05f700 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Tue, 21 Apr 2020 11:00:30 +0200 Subject: [PATCH 33/39] AB#6232 - Moved EnablePerWebRequestScope from configure to start --- src/Umbraco.Infrastructure/Runtime/CoreRuntime.cs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Umbraco.Infrastructure/Runtime/CoreRuntime.cs b/src/Umbraco.Infrastructure/Runtime/CoreRuntime.cs index 1aa94ff2a9..ebd91f52a2 100644 --- a/src/Umbraco.Infrastructure/Runtime/CoreRuntime.cs +++ b/src/Umbraco.Infrastructure/Runtime/CoreRuntime.cs @@ -145,12 +145,6 @@ namespace Umbraco.Core.Runtime ConfigureUnhandledException(); _factory = Configure(register, timer); - // now (and only now) is the time to switch over to perWebRequest scopes. - // up until that point we may not have a request, and scoped services would - // fail to resolve - but we run Initialize within a factory scope - and then, - // here, we switch the factory to bind scopes to requests - _factory.EnablePerWebRequestScope(); - return _factory; } } @@ -261,6 +255,13 @@ namespace Umbraco.Core.Runtime // create & initialize the components _components = _factory.GetInstance(); _components.Initialize(); + + + // now (and only now) is the time to switch over to perWebRequest scopes. + // up until that point we may not have a request, and scoped services would + // fail to resolve - but we run Initialize within a factory scope - and then, + // here, we switch the factory to bind scopes to requests + _factory.EnablePerWebRequestScope(); } protected virtual void ConfigureUnhandledException() From acad9d1f840f8e5a92cf24517df94b28d316fa1a Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Tue, 21 Apr 2020 11:03:04 +0200 Subject: [PATCH 34/39] Making class public --- .../Filters/EnsurePartialViewMacroViewContextFilterAttribute.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Web.Common/Filters/EnsurePartialViewMacroViewContextFilterAttribute.cs b/src/Umbraco.Web.Common/Filters/EnsurePartialViewMacroViewContextFilterAttribute.cs index e2046209d5..269e437d0d 100644 --- a/src/Umbraco.Web.Common/Filters/EnsurePartialViewMacroViewContextFilterAttribute.cs +++ b/src/Umbraco.Web.Common/Filters/EnsurePartialViewMacroViewContextFilterAttribute.cs @@ -26,7 +26,7 @@ namespace Umbraco.Web.Common.Filters /// this DataToken exists before the action executes in case the developer resolves an RTE value that contains /// a partial view macro form. /// - internal class EnsurePartialViewMacroViewContextFilterAttribute : ActionFilterAttribute + public class EnsurePartialViewMacroViewContextFilterAttribute : ActionFilterAttribute { /// From 062441c650c2f42c2d36e1e908f0474bc28bf72b Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Tue, 21 Apr 2020 13:48:06 +0200 Subject: [PATCH 35/39] Passing NoAppCache to CoreRuntime base ctor --- src/Umbraco.Tests/Routing/RenderRouteHandlerTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Tests/Routing/RenderRouteHandlerTests.cs b/src/Umbraco.Tests/Routing/RenderRouteHandlerTests.cs index 4763d3dbc6..960d355c0d 100644 --- a/src/Umbraco.Tests/Routing/RenderRouteHandlerTests.cs +++ b/src/Umbraco.Tests/Routing/RenderRouteHandlerTests.cs @@ -50,7 +50,7 @@ namespace Umbraco.Tests.Routing public class TestRuntime : CoreRuntime { public TestRuntime(Configs configs, IUmbracoVersion umbracoVersion, IIOHelper ioHelper, ILogger logger, IHostingEnvironment hostingEnvironment, IBackOfficeInfo backOfficeInfo) - : base(configs, umbracoVersion, ioHelper, Mock.Of(), Mock.Of(), new AspNetUmbracoBootPermissionChecker(), hostingEnvironment, backOfficeInfo, TestHelper.DbProviderFactoryCreator, TestHelper.MainDom, TestHelper.GetTypeFinder(), TestHelper.GetRequestCache()) + : base(configs, umbracoVersion, ioHelper, Mock.Of(), Mock.Of(), new AspNetUmbracoBootPermissionChecker(), hostingEnvironment, backOfficeInfo, TestHelper.DbProviderFactoryCreator, TestHelper.MainDom, TestHelper.GetTypeFinder(), NoAppCache.Instance) { } From 46e14f769227453bc1ca806ed0dec2bb9157a2db Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 22 Apr 2020 14:23:56 +1000 Subject: [PATCH 36/39] Fixes up all logging. Configures serilog logging to replace MS logging just like serilog does, adds new ILoggerConfiguration so we aren't hard coding things, dynamically adds enrichers when the container is ready to resolve services. --- .../Logging/ILoggingConfiguration.cs | 13 ++ .../Logging/LoggingConfiguration.cs | 20 ++++ .../Logging/MessageTemplates.cs | 8 +- .../Enrichers/HttpRequestIdEnricher.cs | 13 +- .../Enrichers/HttpRequestNumberEnricher.cs | 13 +- .../Enrichers/HttpSessionIdEnricher.cs | 2 +- .../Logging/Serilog/LoggerConfigExtensions.cs | 46 +++---- .../Logging/Serilog/SerilogLogger.cs | 113 +++--------------- .../Serilog/ThreadAbortExceptionEnricher.cs | 98 +++++++++++++++ .../Logging/Viewer/ILogViewer.cs | 4 +- .../Logging/Viewer/LogViewerComposer.cs | 2 +- ...onLogViewer.cs => SerilogJsonLogViewer.cs} | 21 ++-- ...eBase.cs => SerilogLogViewerSourceBase.cs} | 8 +- src/Umbraco.Tests.Common/TestHelperBase.cs | 9 ++ .../Implementations/TestHostingEnvironment.cs | 1 + src/Umbraco.Tests.Integration/RuntimeTests.cs | 4 +- .../Testing/UmbracoIntegrationTest.cs | 2 +- src/Umbraco.Tests/Logging/LogviewerTests.cs | 8 +- src/Umbraco.Tests/TestHelpers/TestHelper.cs | 2 + src/Umbraco.Tests/Testing/UmbracoTestBase.cs | 2 +- .../UmbracoExamine/ExamineBaseTest.cs | 2 +- .../UmbracoApplicationBuilderExtensions.cs | 11 ++ .../AspNetCoreHostingEnvironment.cs | 4 +- .../Extensions/HostBuilderExtensions.cs | 4 +- .../UmbracoCoreServiceCollectionExtensions.cs | 71 +++++++++-- .../UmbracoRequestLoggingMiddleware.cs | 36 ++++++ .../Middleware/UmbracoRequestMiddleware.cs | 1 - .../Config/serilog.Release.config | 4 +- .../Config/serilog.config | 4 +- src/Umbraco.Web.UI.NetCore/Startup.cs | 2 + .../config/serilog.Release.config | 4 +- src/Umbraco.Web/UmbracoApplicationBase.cs | 18 ++- 32 files changed, 359 insertions(+), 191 deletions(-) create mode 100644 src/Umbraco.Core/Logging/ILoggingConfiguration.cs create mode 100644 src/Umbraco.Core/Logging/LoggingConfiguration.cs create mode 100644 src/Umbraco.Infrastructure/Logging/Serilog/ThreadAbortExceptionEnricher.cs rename src/Umbraco.Infrastructure/Logging/Viewer/{JsonLogViewer.cs => SerilogJsonLogViewer.cs} (79%) rename src/Umbraco.Infrastructure/Logging/Viewer/{LogViewerSourceBase.cs => SerilogLogViewerSourceBase.cs} (93%) create mode 100644 src/Umbraco.Web.Common/Middleware/UmbracoRequestLoggingMiddleware.cs diff --git a/src/Umbraco.Core/Logging/ILoggingConfiguration.cs b/src/Umbraco.Core/Logging/ILoggingConfiguration.cs new file mode 100644 index 0000000000..47e2d8fa7c --- /dev/null +++ b/src/Umbraco.Core/Logging/ILoggingConfiguration.cs @@ -0,0 +1,13 @@ +namespace Umbraco.Core.Logging +{ + + public interface ILoggingConfiguration + { + /// + /// The physical path where logs are stored + /// + string LogDirectory { get; } + string LogConfigurationFile { get; } + string UserLogConfigurationFile { get; } + } +} diff --git a/src/Umbraco.Core/Logging/LoggingConfiguration.cs b/src/Umbraco.Core/Logging/LoggingConfiguration.cs new file mode 100644 index 0000000000..c657c9d430 --- /dev/null +++ b/src/Umbraco.Core/Logging/LoggingConfiguration.cs @@ -0,0 +1,20 @@ +using System; + +namespace Umbraco.Core.Logging +{ + public class LoggingConfiguration : ILoggingConfiguration + { + public LoggingConfiguration(string logDirectory, string logConfigurationFile, string userLogConfigurationFile) + { + LogDirectory = logDirectory ?? throw new ArgumentNullException(nameof(logDirectory)); + LogConfigurationFile = logConfigurationFile ?? throw new ArgumentNullException(nameof(logConfigurationFile)); + UserLogConfigurationFile = userLogConfigurationFile ?? throw new ArgumentNullException(nameof(userLogConfigurationFile)); + } + + public string LogDirectory { get; } + + public string LogConfigurationFile { get; } + + public string UserLogConfigurationFile { get; } + } +} diff --git a/src/Umbraco.Infrastructure/Logging/MessageTemplates.cs b/src/Umbraco.Infrastructure/Logging/MessageTemplates.cs index 4640007e1a..712ff85e16 100644 --- a/src/Umbraco.Infrastructure/Logging/MessageTemplates.cs +++ b/src/Umbraco.Infrastructure/Logging/MessageTemplates.cs @@ -16,14 +16,14 @@ namespace Umbraco.Core.Logging // but it only has a pre-release NuGet package. So, we've got to use Serilog's code, which // means we cannot get rid of Serilog entirely. We may want to revisit this at some point. + // TODO: Do we still need this, is there a non-pre release package shipped? + private static readonly Lazy MinimalLogger = new Lazy(() => new LoggerConfiguration().CreateLogger()); public string Render(string messageTemplate, params object[] args) { - // by default, unless initialized otherwise, Log.Logger is SilentLogger which cannot bind message - // templates. Log.Logger is set to a true Logger when initializing Umbraco's logger, but in case - // that has not been done already - use a temp minimal logger (eg for tests). - var logger = Log.Logger as global::Serilog.Core.Logger ?? MinimalLogger.Value; + // resolve a minimal logger instance which is used to bind message templates + var logger = MinimalLogger.Value; var bound = logger.BindMessageTemplate(messageTemplate, args, out var parsedTemplate, out var boundProperties); diff --git a/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/HttpRequestIdEnricher.cs b/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/HttpRequestIdEnricher.cs index 45468ace9f..704e80d302 100644 --- a/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/HttpRequestIdEnricher.cs +++ b/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/HttpRequestIdEnricher.cs @@ -11,13 +11,13 @@ namespace Umbraco.Core.Logging.Serilog.Enrichers /// Original source - https://github.com/serilog-web/classic/blob/master/src/SerilogWeb.Classic/Classic/Enrichers/HttpRequestIdEnricher.cs /// Nupkg: 'Serilog.Web.Classic' contains handlers & extra bits we do not want /// - internal class HttpRequestIdEnricher : ILogEventEnricher + public class HttpRequestIdEnricher : ILogEventEnricher { - private readonly Func _requestCacheGetter; + private readonly IRequestCache _requestCache; - public HttpRequestIdEnricher(Func requestCacheGetter) + public HttpRequestIdEnricher(IRequestCache requestCache) { - _requestCacheGetter = requestCacheGetter; + _requestCache = requestCache ?? throw new ArgumentNullException(nameof(requestCache)); } /// @@ -34,11 +34,8 @@ namespace Umbraco.Core.Logging.Serilog.Enrichers { if (logEvent == null) throw new ArgumentNullException(nameof(logEvent)); - var requestCache = _requestCacheGetter(); - if(requestCache is null) return; - Guid requestId; - if (!LogHttpRequest.TryGetCurrentHttpRequestId(out requestId, requestCache)) + if (!LogHttpRequest.TryGetCurrentHttpRequestId(out requestId, _requestCache)) return; var requestIdProperty = new LogEventProperty(HttpRequestIdPropertyName, new ScalarValue(requestId)); diff --git a/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/HttpRequestNumberEnricher.cs b/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/HttpRequestNumberEnricher.cs index 08eb6b93f0..20643ff539 100644 --- a/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/HttpRequestNumberEnricher.cs +++ b/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/HttpRequestNumberEnricher.cs @@ -13,9 +13,9 @@ namespace Umbraco.Core.Logging.Serilog.Enrichers /// Original source - https://github.com/serilog-web/classic/blob/master/src/SerilogWeb.Classic/Classic/Enrichers/HttpRequestNumberEnricher.cs /// Nupkg: 'Serilog.Web.Classic' contains handlers & extra bits we do not want /// - internal class HttpRequestNumberEnricher : ILogEventEnricher + public class HttpRequestNumberEnricher : ILogEventEnricher { - private readonly Func _requestCacheGetter; + private readonly IRequestCache _requestCache; private static int _lastRequestNumber; private static readonly string _requestNumberItemName = typeof(HttpRequestNumberEnricher).Name + "+RequestNumber"; @@ -25,9 +25,9 @@ namespace Umbraco.Core.Logging.Serilog.Enrichers private const string _httpRequestNumberPropertyName = "HttpRequestNumber"; - public HttpRequestNumberEnricher(Func requestCacheGetter) + public HttpRequestNumberEnricher(IRequestCache requestCache) { - _requestCacheGetter = requestCacheGetter; + _requestCache = requestCache ?? throw new ArgumentNullException(nameof(requestCache)); } /// @@ -39,10 +39,7 @@ namespace Umbraco.Core.Logging.Serilog.Enrichers { if (logEvent == null) throw new ArgumentNullException(nameof(logEvent)); - var requestCache = _requestCacheGetter(); - if (requestCache is null) return; - - var requestNumber = requestCache.Get(_requestNumberItemName, + var requestNumber = _requestCache.Get(_requestNumberItemName, () => Interlocked.Increment(ref _lastRequestNumber)); var requestNumberProperty = new LogEventProperty(_httpRequestNumberPropertyName, new ScalarValue(requestNumber)); diff --git a/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/HttpSessionIdEnricher.cs b/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/HttpSessionIdEnricher.cs index 1558cdcf21..19572b5b42 100644 --- a/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/HttpSessionIdEnricher.cs +++ b/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/HttpSessionIdEnricher.cs @@ -10,7 +10,7 @@ namespace Umbraco.Core.Logging.Serilog.Enrichers /// Original source - https://github.com/serilog-web/classic/blob/master/src/SerilogWeb.Classic/Classic/Enrichers/HttpSessionIdEnricher.cs /// Nupkg: 'Serilog.Web.Classic' contains handlers & extra bits we do not want /// - internal class HttpSessionIdEnricher : ILogEventEnricher + public class HttpSessionIdEnricher : ILogEventEnricher { private readonly ISessionIdResolver _sessionIdResolver; diff --git a/src/Umbraco.Infrastructure/Logging/Serilog/LoggerConfigExtensions.cs b/src/Umbraco.Infrastructure/Logging/Serilog/LoggerConfigExtensions.cs index b0521c6da8..dfcc401ea3 100644 --- a/src/Umbraco.Infrastructure/Logging/Serilog/LoggerConfigExtensions.cs +++ b/src/Umbraco.Infrastructure/Logging/Serilog/LoggerConfigExtensions.cs @@ -7,11 +7,8 @@ using Serilog.Core; using Serilog.Events; using Serilog.Formatting; using Serilog.Formatting.Compact; -using Umbraco.Core.Cache; -using Umbraco.Core.Composing; using Umbraco.Core.Hosting; using Umbraco.Core.Logging.Serilog.Enrichers; -using Umbraco.Net; namespace Umbraco.Core.Logging.Serilog { @@ -25,27 +22,30 @@ namespace Umbraco.Core.Logging.Serilog /// It is highly recommended that you keep/use this default in your own logging config customizations /// /// A Serilog LoggerConfiguration - /// - public static LoggerConfiguration MinimalConfiguration(this LoggerConfiguration logConfig, IHostingEnvironment hostingEnvironment, ISessionIdResolver sessionIdResolver, Func requestCacheGetter) + /// + /// + public static LoggerConfiguration MinimalConfiguration( + this LoggerConfiguration logConfig, + IHostingEnvironment hostingEnvironment, + ILoggingConfiguration loggingConfiguration) { global::Serilog.Debugging.SelfLog.Enable(msg => System.Diagnostics.Debug.WriteLine(msg)); //Set this environment variable - so that it can be used in external config file //add key="serilog:write-to:RollingFile.pathFormat" value="%BASEDIR%\logs\log.txt" /> + Environment.SetEnvironmentVariable("UMBLOGDIR", loggingConfiguration.LogDirectory, EnvironmentVariableTarget.Process); Environment.SetEnvironmentVariable("BASEDIR", hostingEnvironment.ApplicationPhysicalPath, EnvironmentVariableTarget.Process); - Environment.SetEnvironmentVariable("MACHINENAME", Environment.MachineName, EnvironmentVariableTarget.Process); + Environment.SetEnvironmentVariable("MACHINENAME", Environment.MachineName, EnvironmentVariableTarget.Process); logConfig.MinimumLevel.Verbose() //Set to highest level of logging (as any sinks may want to restrict it to Errors only) .Enrich.WithProcessId() .Enrich.WithProcessName() .Enrich.WithThreadId() - .Enrich.WithProperty(AppDomainId, AppDomain.CurrentDomain.Id) + .Enrich.WithProperty(AppDomainId, AppDomain.CurrentDomain.Id) .Enrich.WithProperty("AppDomainAppId", hostingEnvironment.ApplicationId.ReplaceNonAlphanumericChars(string.Empty)) .Enrich.WithProperty("MachineName", Environment.MachineName) .Enrich.With() - .Enrich.With(new HttpSessionIdEnricher(sessionIdResolver)) - .Enrich.With(new HttpRequestNumberEnricher(requestCacheGetter)) - .Enrich.With(new HttpRequestIdEnricher(requestCacheGetter)); + .Enrich.FromLogContext(); // allows us to dynamically enrich return logConfig; } @@ -54,13 +54,14 @@ namespace Umbraco.Core.Logging.Serilog /// Outputs a .txt format log at /App_Data/Logs/ /// /// A Serilog LoggerConfiguration + /// /// The log level you wish the JSON file to collect - default is Verbose (highest) /// The number of days to keep log files. Default is set to null which means all logs are kept - public static LoggerConfiguration OutputDefaultTextFile(this LoggerConfiguration logConfig, IHostingEnvironment hostingEnvironment, LogEventLevel minimumLevel = LogEventLevel.Verbose, int? retainedFileCount = null) + public static LoggerConfiguration OutputDefaultTextFile(this LoggerConfiguration logConfig, ILoggingConfiguration loggingConfiguration, LogEventLevel minimumLevel = LogEventLevel.Verbose, int? retainedFileCount = null) { //Main .txt logfile - in similar format to older Log4Net output //Ends with ..txt as Date is inserted before file extension substring - logConfig.WriteTo.File(Path.Combine(hostingEnvironment.ApplicationPhysicalPath, $@"App_Data\Logs\UmbracoTraceLog.{Environment.MachineName}..txt"), + logConfig.WriteTo.File(Path.Combine(loggingConfiguration.LogDirectory, $@"UmbracoTraceLog.{Environment.MachineName}..txt"), shared: true, rollingInterval: RollingInterval.Day, restrictedToMinimumLevel: minimumLevel, @@ -86,8 +87,6 @@ namespace Umbraco.Core.Logging.Serilog Encoding encoding = null ) { - // TODO: Deal with this method call since it's obsolete, we need to change this - return configuration.Async( asyncConfiguration => asyncConfiguration.Map(AppDomainId, (_,mapConfiguration) => mapConfiguration.File( @@ -102,7 +101,8 @@ namespace Umbraco.Core.Logging.Serilog rollingInterval, rollOnFileSizeLimit, retainedFileCountLimit, - encoding), + encoding, + null), sinkMapCountLimit:0) ); } @@ -112,13 +112,14 @@ namespace Umbraco.Core.Logging.Serilog /// Outputs a CLEF format JSON log at /App_Data/Logs/ /// /// A Serilog LoggerConfiguration + /// /// The log level you wish the JSON file to collect - default is Verbose (highest) /// The number of days to keep log files. Default is set to null which means all logs are kept - public static LoggerConfiguration OutputDefaultJsonFile(this LoggerConfiguration logConfig, IHostingEnvironment hostingEnvironment, LogEventLevel minimumLevel = LogEventLevel.Verbose, int? retainedFileCount = null) + public static LoggerConfiguration OutputDefaultJsonFile(this LoggerConfiguration logConfig, ILoggingConfiguration loggingConfiguration, LogEventLevel minimumLevel = LogEventLevel.Verbose, int? retainedFileCount = null) { //.clef format (Compact log event format, that can be imported into local SEQ & will make searching/filtering logs easier) //Ends with ..txt as Date is inserted before file extension substring - logConfig.WriteTo.File(new CompactJsonFormatter(), Path.Combine(hostingEnvironment.ApplicationPhysicalPath, $@"App_Data\Logs\UmbracoTraceLog.{Environment.MachineName}..json"), + logConfig.WriteTo.File(new CompactJsonFormatter(), Path.Combine(loggingConfiguration.LogDirectory, $@"UmbracoTraceLog.{Environment.MachineName}..json"), shared: true, rollingInterval: RollingInterval.Day, //Create a new JSON file every day retainedFileCountLimit: retainedFileCount, //Setting to null means we keep all files - default is 31 days @@ -132,10 +133,11 @@ namespace Umbraco.Core.Logging.Serilog /// That allows the main logging pipeline to be configured /// /// A Serilog LoggerConfiguration - public static LoggerConfiguration ReadFromConfigFile(this LoggerConfiguration logConfig, IHostingEnvironment hostingEnvironment) + /// + public static LoggerConfiguration ReadFromConfigFile(this LoggerConfiguration logConfig, ILoggingConfiguration loggingConfiguration) { //Read from main serilog.config file - logConfig.ReadFrom.AppSettings(filePath: Path.Combine(hostingEnvironment.ApplicationPhysicalPath, @"config\serilog.config")); + logConfig.ReadFrom.AppSettings(filePath: loggingConfiguration.LogConfigurationFile); return logConfig; } @@ -145,13 +147,15 @@ namespace Umbraco.Core.Logging.Serilog /// That allows a separate logging pipeline to be configured that will not affect the main Umbraco log /// /// A Serilog LoggerConfiguration - public static LoggerConfiguration ReadFromUserConfigFile(this LoggerConfiguration logConfig, IHostingEnvironment hostingEnvironment) + /// + public static LoggerConfiguration ReadFromUserConfigFile(this LoggerConfiguration logConfig, ILoggingConfiguration loggingConfiguration) { //A nested logger - where any user configured sinks via config can not effect the main 'umbraco' logger above logConfig.WriteTo.Logger(cfg => - cfg.ReadFrom.AppSettings(filePath: Path.Combine(hostingEnvironment.ApplicationPhysicalPath, @"config\serilog.user.config"))); + cfg.ReadFrom.AppSettings(filePath: loggingConfiguration.UserLogConfigurationFile)); return logConfig; } + } } diff --git a/src/Umbraco.Infrastructure/Logging/Serilog/SerilogLogger.cs b/src/Umbraco.Infrastructure/Logging/Serilog/SerilogLogger.cs index e4695dedd1..38af9554ab 100644 --- a/src/Umbraco.Infrastructure/Logging/Serilog/SerilogLogger.cs +++ b/src/Umbraco.Infrastructure/Logging/Serilog/SerilogLogger.cs @@ -1,72 +1,56 @@ using System; using System.IO; -using System.Reflection; -using System.Threading; using Serilog; using Serilog.Events; -using Umbraco.Core.Cache; -using Umbraco.Core.Configuration; -using Umbraco.Core.Diagnostics; using Umbraco.Core.Hosting; -using Umbraco.Core.IO; -using Umbraco.Net; namespace Umbraco.Core.Logging.Serilog { + /// /// Implements on top of Serilog. /// public class SerilogLogger : ILogger, IDisposable { - private readonly ICoreDebugSettings _coreDebugSettings; - private readonly IHostingEnvironment _hostingEnvironment; - private readonly IMarchal _marchal; + public global::Serilog.ILogger SerilogLog { get; } /// /// Initialize a new instance of the class with a configuration file. /// /// - public SerilogLogger(ICoreDebugSettings coreDebugSettings, IHostingEnvironment hostingEnvironment, IMarchal marchal, FileInfo logConfigFile) + public SerilogLogger(FileInfo logConfigFile) { - _coreDebugSettings = coreDebugSettings; - _hostingEnvironment = hostingEnvironment; - _marchal = marchal; - - Log.Logger = new LoggerConfiguration() + SerilogLog = new LoggerConfiguration() .ReadFrom.AppSettings(filePath: logConfigFile.FullName) .CreateLogger(); } - public SerilogLogger(ICoreDebugSettings coreDebugSettings, IHostingEnvironment hostingEnvironment, IMarchal marchal, LoggerConfiguration logConfig) + public SerilogLogger(LoggerConfiguration logConfig) { - _coreDebugSettings = coreDebugSettings; - _hostingEnvironment = hostingEnvironment; - _marchal = marchal; - //Configure Serilog static global logger with config passed in - Log.Logger = logConfig.CreateLogger(); + SerilogLog = logConfig.CreateLogger(); } /// /// Creates a logger with some pre-defined configuration and remainder from config file /// /// Used by UmbracoApplicationBase to get its logger. - public static SerilogLogger CreateWithDefaultConfiguration(IHostingEnvironment hostingEnvironment, ISessionIdResolver sessionIdResolver, Func requestCacheGetter, ICoreDebugSettings coreDebugSettings, IIOHelper ioHelper, IMarchal marchal) + public static SerilogLogger CreateWithDefaultConfiguration(IHostingEnvironment hostingEnvironment, ILoggingConfiguration loggingConfiguration) { var loggerConfig = new LoggerConfiguration(); loggerConfig - .MinimalConfiguration(hostingEnvironment, sessionIdResolver, requestCacheGetter) - .ReadFromConfigFile(hostingEnvironment) - .ReadFromUserConfigFile(hostingEnvironment); + .MinimalConfiguration(hostingEnvironment, loggingConfiguration) + .ReadFromConfigFile(loggingConfiguration) + .ReadFromUserConfigFile(loggingConfiguration); - return new SerilogLogger(coreDebugSettings, hostingEnvironment, marchal, loggerConfig); + return new SerilogLogger(loggerConfig); } /// /// Gets a contextualized logger. /// private global::Serilog.ILogger LoggerFor(Type reporting) - => Log.Logger.ForContext(reporting); + => SerilogLog.ForContext(reporting); /// /// Maps Umbraco's log level to Serilog's. @@ -99,8 +83,7 @@ namespace Umbraco.Core.Logging.Serilog /// public void Fatal(Type reporting, Exception exception, string message) { - var logger = LoggerFor(reporting); - DumpThreadAborts(logger, LogEventLevel.Fatal, exception, ref message); + var logger = LoggerFor(reporting); logger.Fatal(exception, message); } @@ -108,8 +91,7 @@ namespace Umbraco.Core.Logging.Serilog public void Fatal(Type reporting, Exception exception) { var logger = LoggerFor(reporting); - var message = "Exception."; - DumpThreadAborts(logger, LogEventLevel.Fatal, exception, ref message); + var message = "Exception."; logger.Fatal(exception, message); } @@ -128,16 +110,14 @@ namespace Umbraco.Core.Logging.Serilog /// public void Fatal(Type reporting, Exception exception, string messageTemplate, params object[] propertyValues) { - var logger = LoggerFor(reporting); - DumpThreadAborts(logger, LogEventLevel.Fatal, exception, ref messageTemplate); + var logger = LoggerFor(reporting); logger.Fatal(exception, messageTemplate, propertyValues); } /// public void Error(Type reporting, Exception exception, string message) { - var logger = LoggerFor(reporting); - DumpThreadAborts(logger, LogEventLevel.Error, exception, ref message); + var logger = LoggerFor(reporting); logger.Error(exception, message); } @@ -146,7 +126,6 @@ namespace Umbraco.Core.Logging.Serilog { var logger = LoggerFor(reporting); var message = "Exception"; - DumpThreadAborts(logger, LogEventLevel.Error, exception, ref message); logger.Error(exception, message); } @@ -166,67 +145,9 @@ namespace Umbraco.Core.Logging.Serilog public void Error(Type reporting, Exception exception, string messageTemplate, params object[] propertyValues) { var logger = LoggerFor(reporting); - DumpThreadAborts(logger, LogEventLevel.Error, exception, ref messageTemplate); logger.Error(exception, messageTemplate, propertyValues); } - private void DumpThreadAborts(global::Serilog.ILogger logger, LogEventLevel level, Exception exception, ref string messageTemplate) - { - var dump = false; - - if (IsTimeoutThreadAbortException(exception)) - { - messageTemplate += "\r\nThe thread has been aborted, because the request has timed out."; - - // dump if configured, or if stacktrace contains Monitor.ReliableEnter - dump = _coreDebugSettings.DumpOnTimeoutThreadAbort || IsMonitorEnterThreadAbortException(exception); - - // dump if it is ok to dump (might have a cap on number of dump...) - dump &= MiniDump.OkToDump(_hostingEnvironment); - } - - if (dump) - { - try - { - var dumped = MiniDump.Dump(_marchal, _hostingEnvironment, withException: true); - messageTemplate += dumped - ? "\r\nA minidump was created in App_Data/MiniDump" - : "\r\nFailed to create a minidump"; - } - catch (Exception ex) - { - messageTemplate += "\r\nFailed to create a minidump"; - - //Log a new entry (as opposed to appending to same log entry) - logger.Write(level, ex, "Failed to create a minidump ({ExType}: {ExMessage})", - new object[]{ ex.GetType().FullName, ex.Message }); - } - } - } - - private static bool IsMonitorEnterThreadAbortException(Exception exception) - { - if (!(exception is ThreadAbortException abort)) return false; - - var stacktrace = abort.StackTrace; - return stacktrace.Contains("System.Threading.Monitor.ReliableEnter"); - } - - private static bool IsTimeoutThreadAbortException(Exception exception) - { - if (!(exception is ThreadAbortException abort)) return false; - if (abort.ExceptionState == null) return false; - - var stateType = abort.ExceptionState.GetType(); - if (stateType.FullName != "System.Web.HttpApplication+CancelModuleException") return false; - - var timeoutField = stateType.GetField("_timeout", BindingFlags.Instance | BindingFlags.NonPublic); - if (timeoutField == null) return false; - - return (bool) timeoutField.GetValue(abort.ExceptionState); - } - /// public void Warn(Type reporting, string message) { @@ -289,7 +210,7 @@ namespace Umbraco.Core.Logging.Serilog public void Dispose() { - Log.CloseAndFlush(); + SerilogLog.DisposeIfDisposable(); } } } diff --git a/src/Umbraco.Infrastructure/Logging/Serilog/ThreadAbortExceptionEnricher.cs b/src/Umbraco.Infrastructure/Logging/Serilog/ThreadAbortExceptionEnricher.cs new file mode 100644 index 0000000000..2a7d35b636 --- /dev/null +++ b/src/Umbraco.Infrastructure/Logging/Serilog/ThreadAbortExceptionEnricher.cs @@ -0,0 +1,98 @@ +using System; +using System.Reflection; +using System.Threading; +using Serilog.Core; +using Serilog.Events; +using Umbraco.Core.Configuration; +using Umbraco.Core.Diagnostics; +using Umbraco.Core.Hosting; + +namespace Umbraco.Core.Logging.Serilog +{ + /// + /// Enriches the log if there are ThreadAbort exceptions and will automatically create a minidump if it can + /// + public class ThreadAbortExceptionEnricher : ILogEventEnricher + { + private readonly ICoreDebugSettings _coreDebugSettings; + private readonly IHostingEnvironment _hostingEnvironment; + private readonly IMarchal _marchal; + + public ThreadAbortExceptionEnricher(ICoreDebugSettings coreDebugSettings, IHostingEnvironment hostingEnvironment, IMarchal marchal) + { + _coreDebugSettings = coreDebugSettings; + _hostingEnvironment = hostingEnvironment; + _marchal = marchal; + } + + public void Enrich(LogEvent logEvent, ILogEventPropertyFactory propertyFactory) + { + switch (logEvent.Level) + { + case LogEventLevel.Error: + case LogEventLevel.Fatal: + DumpThreadAborts(logEvent, propertyFactory); + break; + } + } + + private void DumpThreadAborts(LogEvent logEvent, ILogEventPropertyFactory propertyFactory) + { + if (!IsTimeoutThreadAbortException(logEvent.Exception)) return; + + var message = "The thread has been aborted, because the request has timed out."; + + // dump if configured, or if stacktrace contains Monitor.ReliableEnter + var dump = _coreDebugSettings.DumpOnTimeoutThreadAbort || IsMonitorEnterThreadAbortException(logEvent.Exception); + + // dump if it is ok to dump (might have a cap on number of dump...) + dump &= MiniDump.OkToDump(_hostingEnvironment); + + if (!dump) + { + message += ". No minidump was created."; + logEvent.AddPropertyIfAbsent(propertyFactory.CreateProperty("ThreadAbortExceptionInfo", message)); + } + else + { + try + { + var dumped = MiniDump.Dump(_marchal, _hostingEnvironment, withException: true); + message += dumped + ? ". A minidump was created in App_Data/MiniDump." + : ". Failed to create a minidump."; + logEvent.AddPropertyIfAbsent(propertyFactory.CreateProperty("ThreadAbortExceptionInfo", message)); + } + catch (Exception ex) + { + message = "Failed to create a minidump. " + ex; + logEvent.AddPropertyIfAbsent(propertyFactory.CreateProperty("ThreadAbortExceptionInfo", message)); + } + } + } + + private static bool IsTimeoutThreadAbortException(Exception exception) + { + if (!(exception is ThreadAbortException abort)) return false; + if (abort.ExceptionState == null) return false; + + var stateType = abort.ExceptionState.GetType(); + if (stateType.FullName != "System.Web.HttpApplication+CancelModuleException") return false; + + var timeoutField = stateType.GetField("_timeout", BindingFlags.Instance | BindingFlags.NonPublic); + if (timeoutField == null) return false; + + return (bool)timeoutField.GetValue(abort.ExceptionState); + } + + private static bool IsMonitorEnterThreadAbortException(Exception exception) + { + if (!(exception is ThreadAbortException abort)) return false; + + var stacktrace = abort.StackTrace; + return stacktrace.Contains("System.Threading.Monitor.ReliableEnter"); + } + + + } +} diff --git a/src/Umbraco.Infrastructure/Logging/Viewer/ILogViewer.cs b/src/Umbraco.Infrastructure/Logging/Viewer/ILogViewer.cs index dbdd7842ba..6763b0ebbb 100644 --- a/src/Umbraco.Infrastructure/Logging/Viewer/ILogViewer.cs +++ b/src/Umbraco.Infrastructure/Logging/Viewer/ILogViewer.cs @@ -1,7 +1,5 @@ -using System; -using System.Collections.Generic; +using System.Collections.Generic; using Umbraco.Core.Models; -using Umbraco.Core.Persistence.DatabaseModelDefinitions; namespace Umbraco.Core.Logging.Viewer { diff --git a/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerComposer.cs b/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerComposer.cs index e4acde1265..ee115be325 100644 --- a/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerComposer.cs +++ b/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerComposer.cs @@ -10,7 +10,7 @@ namespace Umbraco.Core.Logging.Viewer public void Compose(Composition composition) { composition.RegisterUnique(); - composition.SetLogViewer(); + composition.SetLogViewer(); } } } diff --git a/src/Umbraco.Infrastructure/Logging/Viewer/JsonLogViewer.cs b/src/Umbraco.Infrastructure/Logging/Viewer/SerilogJsonLogViewer.cs similarity index 79% rename from src/Umbraco.Infrastructure/Logging/Viewer/JsonLogViewer.cs rename to src/Umbraco.Infrastructure/Logging/Viewer/SerilogJsonLogViewer.cs index 54dd58ec03..366a0fb9de 100644 --- a/src/Umbraco.Infrastructure/Logging/Viewer/JsonLogViewer.cs +++ b/src/Umbraco.Infrastructure/Logging/Viewer/SerilogJsonLogViewer.cs @@ -10,23 +10,20 @@ using Umbraco.Core.IO; namespace Umbraco.Core.Logging.Viewer { - internal class JsonLogViewer : LogViewerSourceBase + internal class SerilogJsonLogViewer : SerilogLogViewerSourceBase { private readonly string _logsPath; private readonly ILogger _logger; - private readonly IHostingEnvironment _hostingEnvironment; - public JsonLogViewer(ILogger logger, ILogViewerConfig logViewerConfig, IHostingEnvironment hostingEnvironment) : base(logViewerConfig) + public SerilogJsonLogViewer( + ILogger logger, + ILogViewerConfig logViewerConfig, + ILoggingConfiguration loggingConfiguration, + global::Serilog.ILogger serilogLog) + : base(logViewerConfig, serilogLog) { - _hostingEnvironment = hostingEnvironment; _logger = logger; - - // TODO: this path is hard coded but it can actually be configured, but that is done via Serilog and we don't have a different abstraction/config - // for the logging path. We could make that, but then how would we get that abstraction into the Serilog config? I'm sure there is a way but - // don't have time right now to resolve that (since this was hard coded before). We could have a single/simple ILogConfig for umbraco that purely specifies - // the logging path and then we can have a special token that we replace in the serilog config that maps to that location? then at least we could inject - // that config in places where we are hard coding this path. - _logsPath = Path.Combine(_hostingEnvironment.ApplicationPhysicalPath, @"App_Data\Logs\"); + _logsPath = loggingConfiguration.LogDirectory; } private const int FileSizeCap = 100; @@ -133,7 +130,7 @@ namespace Umbraco.Core.Logging.Viewer { // As we are reading/streaming one line at a time in the JSON file // Thus we can not report the line number, as it will always be 1 - _logger.Error(ex, "Unable to parse a line in the JSON log file"); + _logger.Error(ex, "Unable to parse a line in the JSON log file"); evt = null; return true; diff --git a/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerSourceBase.cs b/src/Umbraco.Infrastructure/Logging/Viewer/SerilogLogViewerSourceBase.cs similarity index 93% rename from src/Umbraco.Infrastructure/Logging/Viewer/LogViewerSourceBase.cs rename to src/Umbraco.Infrastructure/Logging/Viewer/SerilogLogViewerSourceBase.cs index aae2976044..7c8503a37e 100644 --- a/src/Umbraco.Infrastructure/Logging/Viewer/LogViewerSourceBase.cs +++ b/src/Umbraco.Infrastructure/Logging/Viewer/SerilogLogViewerSourceBase.cs @@ -8,13 +8,15 @@ using Umbraco.Core.Models; namespace Umbraco.Core.Logging.Viewer { - public abstract class LogViewerSourceBase : ILogViewer + public abstract class SerilogLogViewerSourceBase : ILogViewer { private readonly ILogViewerConfig _logViewerConfig; + private readonly global::Serilog.ILogger _serilogLog; - protected LogViewerSourceBase(ILogViewerConfig logViewerConfig) + protected SerilogLogViewerSourceBase(ILogViewerConfig logViewerConfig, global::Serilog.ILogger serilogLog) { _logViewerConfig = logViewerConfig; + _serilogLog = serilogLog; } public abstract bool CanHandleLargeLogs { get; } @@ -48,7 +50,7 @@ namespace Umbraco.Core.Logging.Viewer /// public string GetLogLevel() { - var logLevel = Enum.GetValues(typeof(LogEventLevel)).Cast().Where(Log.Logger.IsEnabled)?.Min() ?? null; + var logLevel = Enum.GetValues(typeof(LogEventLevel)).Cast().Where(_serilogLog.IsEnabled)?.Min() ?? null; return logLevel?.ToString() ?? ""; } diff --git a/src/Umbraco.Tests.Common/TestHelperBase.cs b/src/Umbraco.Tests.Common/TestHelperBase.cs index 42b1e6c0dd..21b1f66395 100644 --- a/src/Umbraco.Tests.Common/TestHelperBase.cs +++ b/src/Umbraco.Tests.Common/TestHelperBase.cs @@ -152,5 +152,14 @@ namespace Umbraco.Tests.Common return mock.Object; } + + public ILoggingConfiguration GetLoggingConfiguration(IHostingEnvironment hostingEnv = null) + { + hostingEnv = hostingEnv ?? GetHostingEnvironment(); + return new LoggingConfiguration( + Path.Combine(hostingEnv.ApplicationPhysicalPath, "App_Data\\Logs"), + Path.Combine(hostingEnv.ApplicationPhysicalPath, "config\\serilog.config"), + Path.Combine(hostingEnv.ApplicationPhysicalPath, "config\\serilog.user.config")); + } } } diff --git a/src/Umbraco.Tests.Integration/Implementations/TestHostingEnvironment.cs b/src/Umbraco.Tests.Integration/Implementations/TestHostingEnvironment.cs index 6430291bc2..9f29b14858 100644 --- a/src/Umbraco.Tests.Integration/Implementations/TestHostingEnvironment.cs +++ b/src/Umbraco.Tests.Integration/Implementations/TestHostingEnvironment.cs @@ -1,6 +1,7 @@ using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Http; using Umbraco.Core.Configuration; +using Umbraco.Core.Logging; using Umbraco.Web.Common.AspNetCore; namespace Umbraco.Tests.Integration.Implementations diff --git a/src/Umbraco.Tests.Integration/RuntimeTests.cs b/src/Umbraco.Tests.Integration/RuntimeTests.cs index 52c29d2037..9bc8d31ded 100644 --- a/src/Umbraco.Tests.Integration/RuntimeTests.cs +++ b/src/Umbraco.Tests.Integration/RuntimeTests.cs @@ -99,7 +99,7 @@ namespace Umbraco.Tests.Integration // Add it! services.AddUmbracoConfiguration(hostContext.Configuration); - services.AddUmbracoCore(webHostEnvironment, umbracoContainer, GetType().Assembly, out _); + services.AddUmbracoCore(webHostEnvironment, umbracoContainer, GetType().Assembly, testHelper.GetLoggingConfiguration(), out _); }); var host = await hostBuilder.StartAsync(); @@ -138,7 +138,7 @@ namespace Umbraco.Tests.Integration // Add it! services.AddUmbracoConfiguration(hostContext.Configuration); - services.AddUmbracoCore(webHostEnvironment, umbracoContainer, GetType().Assembly, out _); + services.AddUmbracoCore(webHostEnvironment, umbracoContainer, GetType().Assembly, testHelper.GetLoggingConfiguration(), out _); }); var host = await hostBuilder.StartAsync(); diff --git a/src/Umbraco.Tests.Integration/Testing/UmbracoIntegrationTest.cs b/src/Umbraco.Tests.Integration/Testing/UmbracoIntegrationTest.cs index 101feb79a4..f3a2bcf011 100644 --- a/src/Umbraco.Tests.Integration/Testing/UmbracoIntegrationTest.cs +++ b/src/Umbraco.Tests.Integration/Testing/UmbracoIntegrationTest.cs @@ -108,7 +108,7 @@ namespace Umbraco.Tests.Integration.Testing // Add it! services.AddUmbracoConfiguration(hostContext.Configuration); - services.AddUmbracoCore(webHostEnvironment, umbracoContainer, GetType().Assembly, out _); + services.AddUmbracoCore(webHostEnvironment, umbracoContainer, GetType().Assembly, testHelper.GetLoggingConfiguration(), out _); }); var host = await hostBuilder.StartAsync(); diff --git a/src/Umbraco.Tests/Logging/LogviewerTests.cs b/src/Umbraco.Tests/Logging/LogviewerTests.cs index 75ff81a6d5..0a193b4446 100644 --- a/src/Umbraco.Tests/Logging/LogviewerTests.cs +++ b/src/Umbraco.Tests/Logging/LogviewerTests.cs @@ -1,9 +1,11 @@ using Moq; using NUnit.Framework; +using Serilog; using System; using System.IO; using System.Linq; using Umbraco.Core; +using Umbraco.Core.Logging; using Umbraco.Core.Logging.Viewer; using Umbraco.Tests.TestHelpers; @@ -35,8 +37,10 @@ namespace Umbraco.Tests.Logging var ioHelper = TestHelper.IOHelper; var hostingEnv = TestHelper.GetHostingEnvironment(); + var loggingConfiguration = TestHelper.GetLoggingConfiguration(hostingEnv); + var exampleLogfilePath = Path.Combine(TestContext.CurrentContext.TestDirectory, @"Logging\", _logfileName); - _newLogfileDirPath = Path.Combine(hostingEnv.ApplicationPhysicalPath, @"App_Data\Logs\"); + _newLogfileDirPath = loggingConfiguration.LogDirectory; _newLogfilePath = Path.Combine(_newLogfileDirPath, _logfileName); var exampleSearchfilePath = Path.Combine(TestContext.CurrentContext.TestDirectory, @"Logging\", _searchfileName); @@ -53,7 +57,7 @@ namespace Umbraco.Tests.Logging var logger = Mock.Of(); var logViewerConfig = new LogViewerConfig(hostingEnv); - _logViewer = new JsonLogViewer(logger, logViewerConfig, hostingEnv); + _logViewer = new SerilogJsonLogViewer(logger, logViewerConfig, loggingConfiguration, Log.Logger); } [OneTimeTearDown] diff --git a/src/Umbraco.Tests/TestHelpers/TestHelper.cs b/src/Umbraco.Tests/TestHelpers/TestHelper.cs index 7eca49183d..6fcba1ba1a 100644 --- a/src/Umbraco.Tests/TestHelpers/TestHelper.cs +++ b/src/Umbraco.Tests/TestHelpers/TestHelper.cs @@ -324,6 +324,8 @@ namespace Umbraco.Tests.TestHelpers public static IHostingEnvironment GetHostingEnvironment() => _testHelperInternal.GetHostingEnvironment(); + public static ILoggingConfiguration GetLoggingConfiguration(IHostingEnvironment hostingEnv) => _testHelperInternal.GetLoggingConfiguration(hostingEnv); + public static IApplicationShutdownRegistry GetHostingEnvironmentLifetime() => _testHelperInternal.GetHostingEnvironmentLifetime(); public static IIpResolver GetIpResolver() => _testHelperInternal.GetIpResolver(); diff --git a/src/Umbraco.Tests/Testing/UmbracoTestBase.cs b/src/Umbraco.Tests/Testing/UmbracoTestBase.cs index f62a69177a..80f6ab9c9e 100644 --- a/src/Umbraco.Tests/Testing/UmbracoTestBase.cs +++ b/src/Umbraco.Tests/Testing/UmbracoTestBase.cs @@ -264,7 +264,7 @@ namespace Umbraco.Tests.Testing profiler = Mock.Of(); break; case UmbracoTestOptions.Logger.Serilog: - logger = new SerilogLogger(TestHelper.CoreDebugSettings, HostingEnvironment, TestHelper.Marchal, new FileInfo(TestHelper.MapPathForTestFiles("~/unit-test.config"))); + logger = new SerilogLogger(new FileInfo(TestHelper.MapPathForTestFiles("~/unit-test.config"))); profiler = new LogProfiler(logger); break; case UmbracoTestOptions.Logger.Console: diff --git a/src/Umbraco.Tests/UmbracoExamine/ExamineBaseTest.cs b/src/Umbraco.Tests/UmbracoExamine/ExamineBaseTest.cs index 76b989a7a3..0d55fd99d7 100644 --- a/src/Umbraco.Tests/UmbracoExamine/ExamineBaseTest.cs +++ b/src/Umbraco.Tests/UmbracoExamine/ExamineBaseTest.cs @@ -18,7 +18,7 @@ namespace Umbraco.Tests.UmbracoExamine public void InitializeFixture() { - var logger = new SerilogLogger(TestHelper.CoreDebugSettings, HostingEnvironment, TestHelper.Marchal, new FileInfo(TestHelper.MapPathForTestFiles("~/unit-test.config"))); + var logger = new SerilogLogger(new FileInfo(TestHelper.MapPathForTestFiles("~/unit-test.config"))); _profilingLogger = new ProfilingLogger(logger, new LogProfiler(logger)); } diff --git a/src/Umbraco.Web.BackOffice/AspNetCore/UmbracoApplicationBuilderExtensions.cs b/src/Umbraco.Web.BackOffice/AspNetCore/UmbracoApplicationBuilderExtensions.cs index ddf06e6532..88b9b4ca9e 100644 --- a/src/Umbraco.Web.BackOffice/AspNetCore/UmbracoApplicationBuilderExtensions.cs +++ b/src/Umbraco.Web.BackOffice/AspNetCore/UmbracoApplicationBuilderExtensions.cs @@ -1,9 +1,11 @@ using System; using Microsoft.AspNetCore.Builder; using Microsoft.Extensions.DependencyInjection; +using Serilog.Context; using Smidge; using Umbraco.Core; using Umbraco.Core.Hosting; +using Umbraco.Web.Common.Middleware; namespace Umbraco.Web.BackOffice.AspNetCore { @@ -64,6 +66,15 @@ namespace Umbraco.Web.BackOffice.AspNetCore } } + public static IApplicationBuilder UseUmbracoRequestLogging(this IApplicationBuilder app) + { + if (app == null) throw new ArgumentNullException(nameof(app)); + + app.UseMiddleware(); + + return app; + } + public static IApplicationBuilder UseUmbracoRuntimeMinification(this IApplicationBuilder app) { if (app == null) throw new ArgumentNullException(nameof(app)); diff --git a/src/Umbraco.Web.Common/AspNetCore/AspNetCoreHostingEnvironment.cs b/src/Umbraco.Web.Common/AspNetCore/AspNetCoreHostingEnvironment.cs index decfcfa660..b3ec11c241 100644 --- a/src/Umbraco.Web.Common/AspNetCore/AspNetCoreHostingEnvironment.cs +++ b/src/Umbraco.Web.Common/AspNetCore/AspNetCoreHostingEnvironment.cs @@ -5,13 +5,12 @@ using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Http; using Umbraco.Core; using Umbraco.Core.Configuration; +using Umbraco.Core.Logging; namespace Umbraco.Web.Common.AspNetCore { public class AspNetCoreHostingEnvironment : Umbraco.Core.Hosting.IHostingEnvironment { - - private readonly IHostingSettings _hostingSettings; private readonly IWebHostEnvironment _webHostEnvironment; @@ -28,6 +27,7 @@ namespace Umbraco.Web.Common.AspNetCore ApplicationVirtualPath = "/"; //TODO how to find this, This is a server thing, not application thing. IISVersion = new Version(0, 0); // TODO not necessary IIS + } public bool IsHosted { get; } = true; diff --git a/src/Umbraco.Web.Common/Extensions/HostBuilderExtensions.cs b/src/Umbraco.Web.Common/Extensions/HostBuilderExtensions.cs index 3cb0922837..d314a2ae30 100644 --- a/src/Umbraco.Web.Common/Extensions/HostBuilderExtensions.cs +++ b/src/Umbraco.Web.Common/Extensions/HostBuilderExtensions.cs @@ -1,5 +1,4 @@ using Microsoft.Extensions.Hosting; -using Serilog; using Umbraco.Core.Composing; namespace Umbraco.Web.Common.Extensions @@ -13,8 +12,7 @@ namespace Umbraco.Web.Common.Extensions /// public static IHostBuilder UseUmbraco(this IHostBuilder builder) { - return builder - .UseSerilog() + return builder .UseUmbraco(new UmbracoServiceProviderFactory()); } } diff --git a/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs b/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs index 843620d571..80482852f3 100644 --- a/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs @@ -6,6 +6,10 @@ using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Serilog; +using Serilog.Extensions.Hosting; +using Serilog.Extensions.Logging; using Smidge; using Smidge.Nuglify; using Umbraco.Composing; @@ -71,7 +75,12 @@ namespace Umbraco.Web.Common.Extensions var umbContainer = UmbracoServiceProviderFactory.UmbracoContainer; - services.AddUmbracoCore(webHostEnvironment, umbContainer, Assembly.GetEntryAssembly(), out factory); + var loggingConfig = new LoggingConfiguration( + Path.Combine(webHostEnvironment.ContentRootPath, "App_Data\\Logs"), + Path.Combine(webHostEnvironment.ContentRootPath, "config\\serilog.config"), + Path.Combine(webHostEnvironment.ContentRootPath, "config\\serilog.user.config")); + + services.AddUmbracoCore(webHostEnvironment, umbContainer, Assembly.GetEntryAssembly(), loggingConfig, out factory); return services; } @@ -83,9 +92,16 @@ namespace Umbraco.Web.Common.Extensions /// /// /// + /// /// /// - public static IServiceCollection AddUmbracoCore(this IServiceCollection services, IWebHostEnvironment webHostEnvironment, IRegister umbContainer, Assembly entryAssembly, out IFactory factory) + public static IServiceCollection AddUmbracoCore( + this IServiceCollection services, + IWebHostEnvironment webHostEnvironment, + IRegister umbContainer, + Assembly entryAssembly, + ILoggingConfiguration loggingConfiguration, + out IFactory factory) { if (services is null) throw new ArgumentNullException(nameof(services)); var container = umbContainer; @@ -96,7 +112,7 @@ namespace Umbraco.Web.Common.Extensions // we resolve it before the host finishes configuring in the call to CreateCompositionRoot services.AddSingleton(); - CreateCompositionRoot(services, webHostEnvironment, out var logger, out var configs, out var ioHelper, out var hostingEnvironment, out var backOfficeInfo, out var profiler); + CreateCompositionRoot(services, webHostEnvironment, loggingConfiguration, out var logger, out var configs, out var ioHelper, out var hostingEnvironment, out var backOfficeInfo, out var profiler); var globalSettings = configs.Global(); var umbracoVersion = new UmbracoVersion(globalSettings); @@ -116,7 +132,7 @@ namespace Umbraco.Web.Common.Extensions return services; } - private static ITypeFinder CreateTypeFinder(ILogger logger, IProfiler profiler, IWebHostEnvironment webHostEnvironment, Assembly entryAssembly) + private static ITypeFinder CreateTypeFinder(Core.Logging.ILogger logger, IProfiler profiler, IWebHostEnvironment webHostEnvironment, Assembly entryAssembly) { // TODO: Currently we are not passing in any TypeFinderConfig (with ITypeFinderSettings) which we should do, however // this is not critical right now and would require loading in some config before boot time so just leaving this as-is for now. @@ -126,7 +142,7 @@ namespace Umbraco.Web.Common.Extensions return new TypeFinder(logger, new DefaultUmbracoAssemblyProvider(entryAssembly), runtimeHash); } - private static IRuntime GetCoreRuntime(Configs configs, IUmbracoVersion umbracoVersion, IIOHelper ioHelper, ILogger logger, + private static IRuntime GetCoreRuntime(Configs configs, IUmbracoVersion umbracoVersion, IIOHelper ioHelper, Core.Logging.ILogger logger, IProfiler profiler, Core.Hosting.IHostingEnvironment hostingEnvironment, IBackOfficeInfo backOfficeInfo, ITypeFinder typeFinder) { @@ -151,8 +167,11 @@ namespace Umbraco.Web.Common.Extensions return coreRuntime; } - private static IServiceCollection CreateCompositionRoot(IServiceCollection services, IWebHostEnvironment webHostEnvironment, - out ILogger logger, out Configs configs, out IIOHelper ioHelper, out Core.Hosting.IHostingEnvironment hostingEnvironment, + private static IServiceCollection CreateCompositionRoot( + IServiceCollection services, + IWebHostEnvironment webHostEnvironment, + ILoggingConfiguration loggingConfiguration, + out Core.Logging.ILogger logger, out Configs configs, out IIOHelper ioHelper, out Core.Hosting.IHostingEnvironment hostingEnvironment, out IBackOfficeInfo backOfficeInfo, out IProfiler profiler) { // TODO: We need to avoid this, surely there's a way? See ContainerTests.BuildServiceProvider_Before_Host_Is_Configured @@ -170,11 +189,7 @@ namespace Umbraco.Web.Common.Extensions hostingEnvironment = new AspNetCoreHostingEnvironment(hostingSettings, webHostEnvironment); ioHelper = new IOHelper(hostingEnvironment, globalSettings); - logger = SerilogLogger.CreateWithDefaultConfiguration(hostingEnvironment, - new AspNetCoreSessionManager(httpContextAccessor), - // TODO: We need to avoid this, surely there's a way? See ContainerTests.BuildServiceProvider_Before_Host_Is_Configured - () => services.BuildServiceProvider().GetService(), coreDebug, ioHelper, - new AspNetCoreMarchal()); + logger = AddLogger(services, hostingEnvironment, loggingConfiguration); backOfficeInfo = new AspNetCoreBackOfficeInfo(globalSettings); profiler = GetWebProfiler(hostingEnvironment, httpContextAccessor); @@ -182,6 +197,38 @@ namespace Umbraco.Web.Common.Extensions return services; } + /// + /// Create and configure the logger + /// + /// + private static Core.Logging.ILogger AddLogger(IServiceCollection services, Core.Hosting.IHostingEnvironment hostingEnvironment, ILoggingConfiguration loggingConfiguration) + { + // Create a serilog logger + var logger = SerilogLogger.CreateWithDefaultConfiguration(hostingEnvironment, loggingConfiguration); + + // Wire up all the bits that serilog needs. We need to use our own code since the Serilog ext methods don't cater to our needs since + // we don't want to use the global serilog `Log` object and we don't have our own ILogger implementation before the HostBuilder runs which + // is the only other option that these ext methods allow. + // I have created a PR to make this nicer https://github.com/serilog/serilog-extensions-hosting/pull/19 but we'll need to wait for that. + // Also see : https://github.com/serilog/serilog-extensions-hosting/blob/dev/src/Serilog.Extensions.Hosting/SerilogHostBuilderExtensions.cs + + services.AddSingleton(services => new SerilogLoggerFactory(logger.SerilogLog, false)); + + // This won't (and shouldn't) take ownership of the logger. + services.AddSingleton(logger.SerilogLog); + + // Registered to provide two services... + var diagnosticContext = new DiagnosticContext(logger.SerilogLog); + + // Consumed by e.g. middleware + services.AddSingleton(diagnosticContext); + + // Consumed by user code + services.AddSingleton(diagnosticContext); + + return logger; + } + public static IServiceCollection AddUmbracoRuntimeMinifier(this IServiceCollection services, IConfiguration configuration) { diff --git a/src/Umbraco.Web.Common/Middleware/UmbracoRequestLoggingMiddleware.cs b/src/Umbraco.Web.Common/Middleware/UmbracoRequestLoggingMiddleware.cs new file mode 100644 index 0000000000..f2034dbd82 --- /dev/null +++ b/src/Umbraco.Web.Common/Middleware/UmbracoRequestLoggingMiddleware.cs @@ -0,0 +1,36 @@ +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Serilog.Context; +using Umbraco.Core.Cache; +using Umbraco.Core.Logging.Serilog.Enrichers; +using Umbraco.Net; + +namespace Umbraco.Web.Common.Middleware +{ + public class UmbracoRequestLoggingMiddleware + { + readonly RequestDelegate _next; + private readonly ISessionIdResolver _sessionIdResolver; + private readonly IRequestCache _requestCache; + + public UmbracoRequestLoggingMiddleware(RequestDelegate next, ISessionIdResolver sessionIdResolver, IRequestCache requestCache) + { + _next = next; + _sessionIdResolver = sessionIdResolver; + _requestCache = requestCache; + } + + public async Task Invoke(HttpContext httpContext) + { + // TODO: Need to decide if we want this stuff still, there's new request logging in serilog: + // https://github.com/serilog/serilog-aspnetcore#request-logging which i think would suffice and replace all of this? + + using (LogContext.Push(new HttpSessionIdEnricher(_sessionIdResolver))) + using (LogContext.Push(new HttpRequestNumberEnricher(_requestCache))) + using (LogContext.Push(new HttpRequestIdEnricher(_requestCache))) + { + await _next(httpContext); + } + } + } +} diff --git a/src/Umbraco.Web.Common/Middleware/UmbracoRequestMiddleware.cs b/src/Umbraco.Web.Common/Middleware/UmbracoRequestMiddleware.cs index 93461fc1d5..e8695f3c9c 100644 --- a/src/Umbraco.Web.Common/Middleware/UmbracoRequestMiddleware.cs +++ b/src/Umbraco.Web.Common/Middleware/UmbracoRequestMiddleware.cs @@ -22,5 +22,4 @@ namespace Umbraco.Web.Common.Middleware _umbracoRequestLifetimeManager.EndRequest(context); } } - } diff --git a/src/Umbraco.Web.UI.NetCore/Config/serilog.Release.config b/src/Umbraco.Web.UI.NetCore/Config/serilog.Release.config index e3cf52b3c5..9aca408b36 100644 --- a/src/Umbraco.Web.UI.NetCore/Config/serilog.Release.config +++ b/src/Umbraco.Web.UI.NetCore/Config/serilog.Release.config @@ -19,7 +19,7 @@ - + @@ -27,7 +27,7 @@ - + diff --git a/src/Umbraco.Web.UI.NetCore/Config/serilog.config b/src/Umbraco.Web.UI.NetCore/Config/serilog.config index e3cf52b3c5..9aca408b36 100644 --- a/src/Umbraco.Web.UI.NetCore/Config/serilog.config +++ b/src/Umbraco.Web.UI.NetCore/Config/serilog.config @@ -19,7 +19,7 @@ - + @@ -27,7 +27,7 @@ - + diff --git a/src/Umbraco.Web.UI.NetCore/Startup.cs b/src/Umbraco.Web.UI.NetCore/Startup.cs index 75b2d6f48e..d79fa0d917 100644 --- a/src/Umbraco.Web.UI.NetCore/Startup.cs +++ b/src/Umbraco.Web.UI.NetCore/Startup.cs @@ -54,6 +54,7 @@ namespace Umbraco.Web.UI.BackOffice }); //Finally initialize Current + // TODO: This should be moved to the UmbracoServiceProviderFactory when the container is cross-wired and then don't use the overload above to `out var factory` Current.Initialize( factory.GetInstance (), factory.GetInstance(), @@ -76,6 +77,7 @@ namespace Umbraco.Web.UI.BackOffice app.UseDeveloperExceptionPage(); } app.UseUmbracoCore(); + app.UseUmbracoRequestLogging(); app.UseUmbracoWebsite(); app.UseUmbracoBackOffice(); app.UseRouting(); diff --git a/src/Umbraco.Web.UI/config/serilog.Release.config b/src/Umbraco.Web.UI/config/serilog.Release.config index e3cf52b3c5..9aca408b36 100644 --- a/src/Umbraco.Web.UI/config/serilog.Release.config +++ b/src/Umbraco.Web.UI/config/serilog.Release.config @@ -19,7 +19,7 @@ - + @@ -27,7 +27,7 @@ - + diff --git a/src/Umbraco.Web/UmbracoApplicationBase.cs b/src/Umbraco.Web/UmbracoApplicationBase.cs index d884366bf1..1f90bc7d13 100644 --- a/src/Umbraco.Web/UmbracoApplicationBase.cs +++ b/src/Umbraco.Web/UmbracoApplicationBase.cs @@ -1,4 +1,5 @@ -using System; +using Serilog.Context; +using System; using System.IO; using System.Reflection; using System.Threading; @@ -12,6 +13,8 @@ using Umbraco.Core.Hosting; using Umbraco.Core.IO; using Umbraco.Core.Logging; using Umbraco.Core.Logging.Serilog; +using Umbraco.Core.Logging.Serilog.Enrichers; +using Umbraco.Net; using Umbraco.Web.AspNet; using Umbraco.Web.Hosting; using Umbraco.Web.Logging; @@ -34,12 +37,16 @@ namespace Umbraco.Web var configFactory = new ConfigsFactory(); var hostingSettings = configFactory.HostingSettings; - var coreDebug = configFactory.CoreDebugSettings; var globalSettings = configFactory.GlobalSettings; var hostingEnvironment = new AspNetHostingEnvironment(hostingSettings); + var loggingConfiguration = new LoggingConfiguration( + Path.Combine(hostingEnvironment.ApplicationPhysicalPath, "App_Data\\Logs"), + Path.Combine(hostingEnvironment.ApplicationPhysicalPath, "config\\serilog.config"), + Path.Combine(hostingEnvironment.ApplicationPhysicalPath, "config\\serilog.user.config")); var ioHelper = new IOHelper(hostingEnvironment, globalSettings); - var logger = SerilogLogger.CreateWithDefaultConfiguration(hostingEnvironment, new AspNetSessionManager(), () => _factory?.GetInstance(), coreDebug, ioHelper, new FrameworkMarchal()); + var logger = SerilogLogger.CreateWithDefaultConfiguration(hostingEnvironment, loggingConfiguration); + var configs = configFactory.Create(); var backOfficeInfo = new AspNetBackOfficeInfo(globalSettings, ioHelper, logger, configFactory.WebRoutingSettings); @@ -168,6 +175,11 @@ namespace Umbraco.Web Umbraco.Composing.Current.BackOfficeInfo); _factory = Current.Factory = _runtime.Configure(register); + // now we can add our request based logging enrichers (globally, which is what we were doing in netframework before) + LogContext.Push(new HttpSessionIdEnricher(_factory.GetInstance())); + LogContext.Push(new HttpRequestNumberEnricher(_factory.GetInstance())); + LogContext.Push(new HttpRequestIdEnricher(_factory.GetInstance())); + _runtime.Start(); } From 2ef8f26b2858cd7c1bde3bd8e7fc4b5aeb16d1f2 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 22 Apr 2020 14:29:01 +1000 Subject: [PATCH 37/39] removes unneeded extra HostBuilderExtensions --- .../Composing/HostBuilderExtensions.cs | 11 +++++++++++ .../Extensions/HostBuilderExtensions.cs | 19 ------------------- src/Umbraco.Web.UI.NetCore/Program.cs | 1 - 3 files changed, 11 insertions(+), 20 deletions(-) delete mode 100644 src/Umbraco.Web.Common/Extensions/HostBuilderExtensions.cs diff --git a/src/Umbraco.Infrastructure/Composing/HostBuilderExtensions.cs b/src/Umbraco.Infrastructure/Composing/HostBuilderExtensions.cs index 3ccc3b8a8d..ab758d42af 100644 --- a/src/Umbraco.Infrastructure/Composing/HostBuilderExtensions.cs +++ b/src/Umbraco.Infrastructure/Composing/HostBuilderExtensions.cs @@ -7,6 +7,17 @@ namespace Umbraco.Core.Composing /// public static class HostBuilderExtensions { + /// + /// Assigns a custom service provider factory to use Umbraco's container + /// + /// + /// + public static IHostBuilder UseUmbraco(this IHostBuilder builder) + { + return builder + .UseUmbraco(new UmbracoServiceProviderFactory()); + } + /// /// Assigns a custom service provider factory to use Umbraco's container /// diff --git a/src/Umbraco.Web.Common/Extensions/HostBuilderExtensions.cs b/src/Umbraco.Web.Common/Extensions/HostBuilderExtensions.cs deleted file mode 100644 index d314a2ae30..0000000000 --- a/src/Umbraco.Web.Common/Extensions/HostBuilderExtensions.cs +++ /dev/null @@ -1,19 +0,0 @@ -using Microsoft.Extensions.Hosting; -using Umbraco.Core.Composing; - -namespace Umbraco.Web.Common.Extensions -{ - public static class HostBuilderExtensions - { - /// - /// Assigns a custom service provider factory to use Umbraco's container - /// - /// - /// - public static IHostBuilder UseUmbraco(this IHostBuilder builder) - { - return builder - .UseUmbraco(new UmbracoServiceProviderFactory()); - } - } -} diff --git a/src/Umbraco.Web.UI.NetCore/Program.cs b/src/Umbraco.Web.UI.NetCore/Program.cs index 1fe66f6664..1151f16be8 100644 --- a/src/Umbraco.Web.UI.NetCore/Program.cs +++ b/src/Umbraco.Web.UI.NetCore/Program.cs @@ -1,7 +1,6 @@ using Microsoft.AspNetCore.Hosting; using Microsoft.Extensions.Hosting; using Umbraco.Core.Composing; -using Umbraco.Web.Common.Extensions; namespace Umbraco.Web.UI.BackOffice { From 18b285f088df1508fb7b9686e78d750b962e7c3a Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 22 Apr 2020 15:23:51 +1000 Subject: [PATCH 38/39] Splits profiler so it doesn't have to take a dependency since we don't want to pre-resolve. Adds enrichers to the container. --- src/Umbraco.Core/Logging/IProfiler.cs | 8 +-- src/Umbraco.Core/Logging/IProfilerHtml.cs | 15 ++++++ src/Umbraco.Core/Logging/LogProfiler.cs | 6 --- src/Umbraco.Core/Logging/VoidProfiler.cs | 5 -- .../ThreadAbortExceptionEnricher.cs | 6 +-- .../Logging/Serilog/SerilogComposer.cs | 21 ++++++++ .../TestHelpers/Stubs/TestProfiler.cs | 5 -- .../UmbracoApplicationBuilderExtensions.cs | 6 +++ .../UmbracoCoreServiceCollectionExtensions.cs | 9 ++-- .../UmbracoRequestLoggingMiddleware.cs | 21 +++++--- .../Runtime/AspNetCoreComposer.cs | 7 +++ .../Runtime/Profiler/WebProfiler.cs | 44 +--------------- .../Runtime/Profiler/WebProfilerComposer.cs | 9 +++- .../Runtime/Profiler/WebProfilerHtml.cs | 51 +++++++++++++++++++ src/Umbraco.Web.UI.NetCore/Startup.cs | 3 +- src/Umbraco.Web/Composing/Current.cs | 2 + src/Umbraco.Web/HtmlHelperRenderExtensions.cs | 2 +- src/Umbraco.Web/Logging/WebProfiler.cs | 2 +- src/Umbraco.Web/Runtime/WebInitialComposer.cs | 6 ++- 19 files changed, 140 insertions(+), 88 deletions(-) create mode 100644 src/Umbraco.Core/Logging/IProfilerHtml.cs rename src/Umbraco.Infrastructure/Logging/Serilog/{ => Enrichers}/ThreadAbortExceptionEnricher.cs (98%) create mode 100644 src/Umbraco.Infrastructure/Logging/Serilog/SerilogComposer.cs create mode 100644 src/Umbraco.Web.Common/Runtime/Profiler/WebProfilerHtml.cs diff --git a/src/Umbraco.Core/Logging/IProfiler.cs b/src/Umbraco.Core/Logging/IProfiler.cs index 1327651197..d855612c95 100644 --- a/src/Umbraco.Core/Logging/IProfiler.cs +++ b/src/Umbraco.Core/Logging/IProfiler.cs @@ -2,18 +2,12 @@ namespace Umbraco.Core.Logging { + /// /// Defines the profiling service. /// public interface IProfiler { - /// - /// Renders the profiling results. - /// - /// The profiling results. - /// Generally used for HTML rendering. - string Render(); - /// /// Gets an that will time the code between its creation and disposal. /// diff --git a/src/Umbraco.Core/Logging/IProfilerHtml.cs b/src/Umbraco.Core/Logging/IProfilerHtml.cs new file mode 100644 index 0000000000..4f9ee62e0b --- /dev/null +++ b/src/Umbraco.Core/Logging/IProfilerHtml.cs @@ -0,0 +1,15 @@ +namespace Umbraco.Core.Logging +{ + /// + /// Used to render a profiler in a web page + /// + public interface IProfilerHtml + { + /// + /// Renders the profiling results. + /// + /// The profiling results. + /// Generally used for HTML rendering. + string Render(); + } +} diff --git a/src/Umbraco.Core/Logging/LogProfiler.cs b/src/Umbraco.Core/Logging/LogProfiler.cs index 294f92dad3..a1d2a2e61f 100644 --- a/src/Umbraco.Core/Logging/LogProfiler.cs +++ b/src/Umbraco.Core/Logging/LogProfiler.cs @@ -15,12 +15,6 @@ namespace Umbraco.Core.Logging _logger = logger; } - /// - public string Render() - { - return string.Empty; - } - /// public IDisposable Step(string name) { diff --git a/src/Umbraco.Core/Logging/VoidProfiler.cs b/src/Umbraco.Core/Logging/VoidProfiler.cs index 51bec521a3..d771fd7630 100644 --- a/src/Umbraco.Core/Logging/VoidProfiler.cs +++ b/src/Umbraco.Core/Logging/VoidProfiler.cs @@ -6,11 +6,6 @@ namespace Umbraco.Core.Logging { private readonly VoidDisposable _disposable = new VoidDisposable(); - public string Render() - { - return string.Empty; - } - public IDisposable Step(string name) { return _disposable; diff --git a/src/Umbraco.Infrastructure/Logging/Serilog/ThreadAbortExceptionEnricher.cs b/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/ThreadAbortExceptionEnricher.cs similarity index 98% rename from src/Umbraco.Infrastructure/Logging/Serilog/ThreadAbortExceptionEnricher.cs rename to src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/ThreadAbortExceptionEnricher.cs index 2a7d35b636..1f495d3a50 100644 --- a/src/Umbraco.Infrastructure/Logging/Serilog/ThreadAbortExceptionEnricher.cs +++ b/src/Umbraco.Infrastructure/Logging/Serilog/Enrichers/ThreadAbortExceptionEnricher.cs @@ -7,7 +7,7 @@ using Umbraco.Core.Configuration; using Umbraco.Core.Diagnostics; using Umbraco.Core.Hosting; -namespace Umbraco.Core.Logging.Serilog +namespace Umbraco.Infrastructure.Logging.Serilog.Enrichers { /// /// Enriches the log if there are ThreadAbort exceptions and will automatically create a minidump if it can @@ -54,7 +54,6 @@ namespace Umbraco.Core.Logging.Serilog logEvent.AddPropertyIfAbsent(propertyFactory.CreateProperty("ThreadAbortExceptionInfo", message)); } else - { try { var dumped = MiniDump.Dump(_marchal, _hostingEnvironment, withException: true); @@ -68,7 +67,6 @@ namespace Umbraco.Core.Logging.Serilog message = "Failed to create a minidump. " + ex; logEvent.AddPropertyIfAbsent(propertyFactory.CreateProperty("ThreadAbortExceptionInfo", message)); } - } } private static bool IsTimeoutThreadAbortException(Exception exception) @@ -93,6 +91,6 @@ namespace Umbraco.Core.Logging.Serilog return stacktrace.Contains("System.Threading.Monitor.ReliableEnter"); } - + } } diff --git a/src/Umbraco.Infrastructure/Logging/Serilog/SerilogComposer.cs b/src/Umbraco.Infrastructure/Logging/Serilog/SerilogComposer.cs new file mode 100644 index 0000000000..18b417d428 --- /dev/null +++ b/src/Umbraco.Infrastructure/Logging/Serilog/SerilogComposer.cs @@ -0,0 +1,21 @@ +using System; +using System.Collections.Generic; +using System.Text; +using Umbraco.Core; +using Umbraco.Core.Composing; +using Umbraco.Core.Logging.Serilog.Enrichers; +using Umbraco.Infrastructure.Logging.Serilog.Enrichers; + +namespace Umbraco.Infrastructure.Logging.Serilog +{ + public class SerilogComposer : ICoreComposer + { + public void Compose(Composition composition) + { + composition.RegisterUnique(); + composition.RegisterUnique(); + composition.RegisterUnique(); + composition.RegisterUnique(); + } + } +} diff --git a/src/Umbraco.Tests/TestHelpers/Stubs/TestProfiler.cs b/src/Umbraco.Tests/TestHelpers/Stubs/TestProfiler.cs index 39cac6e24f..ea0f9cc44f 100644 --- a/src/Umbraco.Tests/TestHelpers/Stubs/TestProfiler.cs +++ b/src/Umbraco.Tests/TestHelpers/Stubs/TestProfiler.cs @@ -19,11 +19,6 @@ namespace Umbraco.Tests.TestHelpers.Stubs private static bool _enabled; - public string Render() - { - return string.Empty; - } - public IDisposable Step(string name) { return _enabled ? MiniProfiler.Current.Step(name) : null; diff --git a/src/Umbraco.Web.BackOffice/AspNetCore/UmbracoApplicationBuilderExtensions.cs b/src/Umbraco.Web.BackOffice/AspNetCore/UmbracoApplicationBuilderExtensions.cs index 88b9b4ca9e..a27113e881 100644 --- a/src/Umbraco.Web.BackOffice/AspNetCore/UmbracoApplicationBuilderExtensions.cs +++ b/src/Umbraco.Web.BackOffice/AspNetCore/UmbracoApplicationBuilderExtensions.cs @@ -4,7 +4,9 @@ using Microsoft.Extensions.DependencyInjection; using Serilog.Context; using Smidge; using Umbraco.Core; +using Umbraco.Core.Configuration; using Umbraco.Core.Hosting; +using Umbraco.Infrastructure.Logging.Serilog.Enrichers; using Umbraco.Web.Common.Middleware; namespace Umbraco.Web.BackOffice.AspNetCore @@ -33,6 +35,10 @@ namespace Umbraco.Web.BackOffice.AspNetCore var runtimeShutdown = new CoreRuntimeShutdown(runtime, hostLifetime); hostLifetime.RegisterObject(runtimeShutdown); + // Register our global threadabort enricher for logging + var threadAbortEnricher = app.ApplicationServices.GetRequiredService(); + LogContext.Push(threadAbortEnricher); // NOTE: We are not in a using clause because we are not removing it, it is on the global context + // Start the runtime! runtime.Start(); diff --git a/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs b/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs index 80482852f3..2439243f30 100644 --- a/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.Common/Extensions/UmbracoCoreServiceCollectionExtensions.cs @@ -177,14 +177,11 @@ namespace Umbraco.Web.Common.Extensions // TODO: We need to avoid this, surely there's a way? See ContainerTests.BuildServiceProvider_Before_Host_Is_Configured var serviceProvider = services.BuildServiceProvider(); - var httpContextAccessor = serviceProvider.GetRequiredService(); - configs = serviceProvider.GetService(); if (configs == null) throw new InvalidOperationException($"Could not resolve type {typeof(Configs)} from the container, ensure {nameof(AddUmbracoConfiguration)} is called before calling {nameof(AddUmbracoCore)}"); var hostingSettings = configs.Hosting(); - var coreDebug = configs.CoreDebug(); var globalSettings = configs.Global(); hostingEnvironment = new AspNetCoreHostingEnvironment(hostingSettings, webHostEnvironment); @@ -192,7 +189,7 @@ namespace Umbraco.Web.Common.Extensions logger = AddLogger(services, hostingEnvironment, loggingConfiguration); backOfficeInfo = new AspNetCoreBackOfficeInfo(globalSettings); - profiler = GetWebProfiler(hostingEnvironment, httpContextAccessor); + profiler = GetWebProfiler(hostingEnvironment); return services; } @@ -238,7 +235,7 @@ namespace Umbraco.Web.Common.Extensions return services; } - private static IProfiler GetWebProfiler(Umbraco.Core.Hosting.IHostingEnvironment hostingEnvironment, IHttpContextAccessor httpContextAccessor) + private static IProfiler GetWebProfiler(Umbraco.Core.Hosting.IHostingEnvironment hostingEnvironment) { // create and start asap to profile boot if (!hostingEnvironment.IsDebugMode) @@ -248,7 +245,7 @@ namespace Umbraco.Web.Common.Extensions return new VoidProfiler(); } - var webProfiler = new WebProfiler(httpContextAccessor); + var webProfiler = new WebProfiler(); webProfiler.StartBoot(); return webProfiler; diff --git a/src/Umbraco.Web.Common/Middleware/UmbracoRequestLoggingMiddleware.cs b/src/Umbraco.Web.Common/Middleware/UmbracoRequestLoggingMiddleware.cs index f2034dbd82..0e2158c939 100644 --- a/src/Umbraco.Web.Common/Middleware/UmbracoRequestLoggingMiddleware.cs +++ b/src/Umbraco.Web.Common/Middleware/UmbracoRequestLoggingMiddleware.cs @@ -10,14 +10,19 @@ namespace Umbraco.Web.Common.Middleware public class UmbracoRequestLoggingMiddleware { readonly RequestDelegate _next; - private readonly ISessionIdResolver _sessionIdResolver; - private readonly IRequestCache _requestCache; + private readonly HttpSessionIdEnricher _sessionIdEnricher; + private readonly HttpRequestNumberEnricher _requestNumberEnricher; + private readonly HttpRequestIdEnricher _requestIdEnricher; - public UmbracoRequestLoggingMiddleware(RequestDelegate next, ISessionIdResolver sessionIdResolver, IRequestCache requestCache) + public UmbracoRequestLoggingMiddleware(RequestDelegate next, + HttpSessionIdEnricher sessionIdEnricher, + HttpRequestNumberEnricher requestNumberEnricher, + HttpRequestIdEnricher requestIdEnricher) { _next = next; - _sessionIdResolver = sessionIdResolver; - _requestCache = requestCache; + _sessionIdEnricher = sessionIdEnricher; + _requestNumberEnricher = requestNumberEnricher; + _requestIdEnricher = requestIdEnricher; } public async Task Invoke(HttpContext httpContext) @@ -25,9 +30,9 @@ namespace Umbraco.Web.Common.Middleware // TODO: Need to decide if we want this stuff still, there's new request logging in serilog: // https://github.com/serilog/serilog-aspnetcore#request-logging which i think would suffice and replace all of this? - using (LogContext.Push(new HttpSessionIdEnricher(_sessionIdResolver))) - using (LogContext.Push(new HttpRequestNumberEnricher(_requestCache))) - using (LogContext.Push(new HttpRequestIdEnricher(_requestCache))) + using (LogContext.Push(_sessionIdEnricher)) + using (LogContext.Push(_requestNumberEnricher)) + using (LogContext.Push(_requestIdEnricher)) { await _next(httpContext); } diff --git a/src/Umbraco.Web.Common/Runtime/AspNetCoreComposer.cs b/src/Umbraco.Web.Common/Runtime/AspNetCoreComposer.cs index 79c7d3ec25..8af2824c03 100644 --- a/src/Umbraco.Web.Common/Runtime/AspNetCoreComposer.cs +++ b/src/Umbraco.Web.Common/Runtime/AspNetCoreComposer.cs @@ -7,6 +7,9 @@ using Umbraco.Core.Runtime; using Umbraco.Core.Security; using Umbraco.Web.Common.AspNetCore; using Umbraco.Web.Common.Lifetime; +using Umbraco.Core.Diagnostics; +using Umbraco.Web.Common.Runtime.Profiler; +using Umbraco.Core.Logging; namespace Umbraco.Web.Common.Runtime { @@ -42,6 +45,10 @@ namespace Umbraco.Web.Common.Runtime composition.RegisterUnique(); composition.RegisterMultipleUnique(); + + composition.RegisterUnique(); + + composition.RegisterUnique(); } } } diff --git a/src/Umbraco.Web.Common/Runtime/Profiler/WebProfiler.cs b/src/Umbraco.Web.Common/Runtime/Profiler/WebProfiler.cs index bdbc6f164d..958e134bab 100644 --- a/src/Umbraco.Web.Common/Runtime/Profiler/WebProfiler.cs +++ b/src/Umbraco.Web.Common/Runtime/Profiler/WebProfiler.cs @@ -1,61 +1,21 @@ using System; -using System.Collections.Generic; using System.Linq; using System.Threading; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Extensions; using StackExchange.Profiling; -using StackExchange.Profiling.Internal; using Umbraco.Core; using Umbraco.Core.Logging; +// TODO: This namespace is strange, not sure why i has "Runtime" in the name? namespace Umbraco.Web.Common.Runtime.Profiler { + public class WebProfiler : IProfiler { private MiniProfiler _startupProfiler; - - private readonly IHttpContextAccessor _httpContextAccessor; private int _first; - public WebProfiler(IHttpContextAccessor httpContextAccessor) - { - // create our own provider, which can provide a profiler even during boot - _httpContextAccessor = httpContextAccessor; - } - - /// - /// - /// - /// - /// Normally we would call MiniProfiler.Current.RenderIncludes(...), but because the requeststate is not set, this method does not work. - /// We fake the requestIds from the RequestState here. - /// - public string Render() - { - - var profiler = MiniProfiler.Current; - if (profiler == null) return string.Empty; - - var context = _httpContextAccessor.HttpContext; - - var path = (profiler.Options as MiniProfilerOptions)?.RouteBasePath.Value.EnsureTrailingSlash(); - - var result = StackExchange.Profiling.Internal.Render.Includes( - profiler, - path: context.Request.PathBase + path, - isAuthorized: true, - requestIDs: new List{ profiler.Id }, - position: RenderPosition.Right, - showTrivial: profiler.Options.PopupShowTrivial, - showTimeWithChildren: profiler.Options.PopupShowTimeWithChildren, - maxTracesToShow: profiler.Options.PopupMaxTracesToShow, - showControls:profiler.Options.ShowControls, - startHidden: profiler.Options.PopupStartHidden); - - return result; - } - public IDisposable Step(string name) { return MiniProfiler.Current?.Step(name); diff --git a/src/Umbraco.Web.Common/Runtime/Profiler/WebProfilerComposer.cs b/src/Umbraco.Web.Common/Runtime/Profiler/WebProfilerComposer.cs index 688a3e5c28..523faf2da5 100644 --- a/src/Umbraco.Web.Common/Runtime/Profiler/WebProfilerComposer.cs +++ b/src/Umbraco.Web.Common/Runtime/Profiler/WebProfilerComposer.cs @@ -1,8 +1,15 @@ -using Umbraco.Core.Composing; +using Umbraco.Core; +using Umbraco.Core.Composing; namespace Umbraco.Web.Common.Runtime.Profiler { internal class WebProfilerComposer : ComponentComposer, ICoreComposer { + public override void Compose(Composition composition) + { + base.Compose(composition); + + composition.RegisterUnique(); + } } } diff --git a/src/Umbraco.Web.Common/Runtime/Profiler/WebProfilerHtml.cs b/src/Umbraco.Web.Common/Runtime/Profiler/WebProfilerHtml.cs new file mode 100644 index 0000000000..9e989d6b5c --- /dev/null +++ b/src/Umbraco.Web.Common/Runtime/Profiler/WebProfilerHtml.cs @@ -0,0 +1,51 @@ +using System; +using System.Collections.Generic; +using Microsoft.AspNetCore.Http; +using StackExchange.Profiling; +using StackExchange.Profiling.Internal; +using Umbraco.Core.Logging; + +// TODO: This namespace is strange, not sure why i has "Runtime" in the name? +namespace Umbraco.Web.Common.Runtime.Profiler +{ + public class WebProfilerHtml : IProfilerHtml + { + private readonly IHttpContextAccessor _httpContextAccessor; + + public WebProfilerHtml(IHttpContextAccessor httpContextAccessor) + { + // create our own provider, which can provide a profiler even during boot + _httpContextAccessor = httpContextAccessor; + } + + /// + /// + /// Normally we would call MiniProfiler.Current.RenderIncludes(...), but because the requeststate is not set, this method does not work. + /// We fake the requestIds from the RequestState here. + /// + public string Render() + { + + var profiler = MiniProfiler.Current; + if (profiler == null) return string.Empty; + + var context = _httpContextAccessor.HttpContext; + + var path = (profiler.Options as MiniProfilerOptions)?.RouteBasePath.Value.EnsureTrailingSlash(); + + var result = StackExchange.Profiling.Internal.Render.Includes( + profiler, + path: context.Request.PathBase + path, + isAuthorized: true, + requestIDs: new List { profiler.Id }, + position: RenderPosition.Right, + showTrivial: profiler.Options.PopupShowTrivial, + showTimeWithChildren: profiler.Options.PopupShowTimeWithChildren, + maxTracesToShow: profiler.Options.PopupMaxTracesToShow, + showControls: profiler.Options.ShowControls, + startHidden: profiler.Options.PopupStartHidden); + + return result; + } + } +} diff --git a/src/Umbraco.Web.UI.NetCore/Startup.cs b/src/Umbraco.Web.UI.NetCore/Startup.cs index d79fa0d917..8602d5bed6 100644 --- a/src/Umbraco.Web.UI.NetCore/Startup.cs +++ b/src/Umbraco.Web.UI.NetCore/Startup.cs @@ -91,7 +91,8 @@ namespace Umbraco.Web.UI.BackOffice }); endpoints.MapGet("/", async context => { - await context.Response.WriteAsync($"Hello World!{Current.Profiler.Render()}"); + var profilerHtml = app.ApplicationServices.GetRequiredService(); + await context.Response.WriteAsync($"Hello World!{profilerHtml.Render()}"); }); }); } diff --git a/src/Umbraco.Web/Composing/Current.cs b/src/Umbraco.Web/Composing/Current.cs index f9f056ff32..1ed217cc78 100644 --- a/src/Umbraco.Web/Composing/Current.cs +++ b/src/Umbraco.Web/Composing/Current.cs @@ -248,6 +248,8 @@ namespace Umbraco.Web.Composing public static IProfiler Profiler => Factory.GetInstance(); + public static IProfilerHtml ProfilerHtml => Factory.GetInstance(); + public static IProfilingLogger ProfilingLogger => Factory.GetInstance(); public static AppCaches AppCaches => Factory.GetInstance(); diff --git a/src/Umbraco.Web/HtmlHelperRenderExtensions.cs b/src/Umbraco.Web/HtmlHelperRenderExtensions.cs index 6c207dd15a..2795fe66c6 100644 --- a/src/Umbraco.Web/HtmlHelperRenderExtensions.cs +++ b/src/Umbraco.Web/HtmlHelperRenderExtensions.cs @@ -30,7 +30,7 @@ namespace Umbraco.Web /// public static IHtmlString RenderProfiler(this HtmlHelper helper) { - return new HtmlString(Current.Profiler.Render()); + return new HtmlString(Current.ProfilerHtml.Render()); } /// diff --git a/src/Umbraco.Web/Logging/WebProfiler.cs b/src/Umbraco.Web/Logging/WebProfiler.cs index e390950c0b..6f15be7ecd 100755 --- a/src/Umbraco.Web/Logging/WebProfiler.cs +++ b/src/Umbraco.Web/Logging/WebProfiler.cs @@ -14,7 +14,7 @@ namespace Umbraco.Web.Logging /// /// Profiling only runs when the app is in debug mode, see WebRuntime for how this gets created /// - internal class WebProfiler : IProfiler + internal class WebProfiler : IProfiler, IProfilerHtml { private const string BootRequestItemKey = "Umbraco.Core.Logging.WebProfiler__isBootRequest"; private readonly WebProfilerProvider _provider; diff --git a/src/Umbraco.Web/Runtime/WebInitialComposer.cs b/src/Umbraco.Web/Runtime/WebInitialComposer.cs index ec3b463d4c..5b59b632eb 100644 --- a/src/Umbraco.Web/Runtime/WebInitialComposer.cs +++ b/src/Umbraco.Web/Runtime/WebInitialComposer.cs @@ -25,6 +25,9 @@ using Umbraco.Web.Trees; using Umbraco.Web.WebApi; using Umbraco.Net; using Umbraco.Web.AspNet; +using Umbraco.Core.Diagnostics; +using Umbraco.Core.Logging; +using Umbraco.Web.Logging; namespace Umbraco.Web.Runtime { @@ -50,7 +53,8 @@ namespace Umbraco.Web.Runtime composition.Register(Lifetime.Singleton); - + composition.RegisterUnique(); + composition.RegisterUnique(); composition.ComposeWebMappingProfiles(); From fce1df08f99978b2c4085ad59c2ce632f1e9e944 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Wed, 22 Apr 2020 10:55:13 +0200 Subject: [PATCH 39/39] Fix test --- src/Umbraco.Tests/Runtimes/CoreRuntimeTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Umbraco.Tests/Runtimes/CoreRuntimeTests.cs b/src/Umbraco.Tests/Runtimes/CoreRuntimeTests.cs index 0b90457b1e..f63c56b64e 100644 --- a/src/Umbraco.Tests/Runtimes/CoreRuntimeTests.cs +++ b/src/Umbraco.Tests/Runtimes/CoreRuntimeTests.cs @@ -17,6 +17,7 @@ using Umbraco.Core.Logging; using Umbraco.Core.Persistence; using Umbraco.Core.Runtime; using Umbraco.Core.Scoping; +using Umbraco.Net; using Umbraco.Tests.TestHelpers; using Umbraco.Tests.TestHelpers.Stubs; using Umbraco.Web; @@ -135,6 +136,7 @@ namespace Umbraco.Tests.Runtimes public override IFactory Configure(IRegister container) { container.Register(Lifetime.Singleton); + container.Register(Lifetime.Singleton); var factory = base.Configure(container); return factory;