Merge remote-tracking branch 'origin/v8/dev' into v8/feature/AB4550-segments-ui-variant-picker

src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs

@@ -0,0 +1,124 @@
+using System;
+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 data 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";
+        private const string _fixMediaPathsTitle = "Fix media paths";
+        private const string _fixContentPathsTitle = "Fix content paths";
+
+        public DatabaseIntegrityCheck(IContentService contentService, IMediaService mediaService)
+        {
+            _contentService = contentService;
+            _mediaService = mediaService;
+        }
+
+        /// <summary>
+        /// Get the status for this health check
+        /// </summary>
+        /// <returns></returns>
+        public override IEnumerable<HealthCheckStatus> GetStatus()
+        {
+            //return the statuses
+            return new[]
+            {
+                CheckDocuments(false),
+                CheckMedia(false)
+            };
+        }
+
+        private HealthCheckStatus CheckMedia(bool fix)
+        {
+            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, fix,
+                () => _contentService.CheckDataIntegrity(new ContentDataIntegrityReportOptions {FixIssues = fix}));
+        }
+
+        private HealthCheckStatus CheckPaths(string actionAlias, string actionName, string entityType, bool detailedReport, Func<ContentDataIntegrityReport> doCheck)
+        {
+            var report = doCheck();
+
+            var actions = new List<HealthCheckAction>();
+            if (!report.Ok)
+            {
+                actions.Add(new HealthCheckAction(actionAlias, Id)
+                {
+                    Name = actionName
+                });
+            }
+
+            return new HealthCheckStatus(GetReport(report, entityType, detailedReport))
+            {
+                ResultType = report.Ok ? StatusResultType.Success : StatusResultType.Error,
+                Actions = actions
+            };
+        }
+
+        private static string GetReport(ContentDataIntegrityReport report, string entityType, bool detailed)
+        {
+            var sb = new StringBuilder();
+
+            if (report.Ok)
+            {
+                sb.AppendLine($"<p>All {entityType} paths are valid</p>");
+
+                if (!detailed)
+                    return sb.ToString();
+            }
+            else
+            {
+                sb.AppendLine($"<p>{report.DetectedIssues.Count} invalid {entityType} paths detected.</p>");
+            }
+
+            if (detailed && report.DetectedIssues.Count > 0)
+            {
+                sb.AppendLine("<ul>");
+                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("<li>");
+                    sb.AppendLine($"{countByGroup} issues of type <code>{issueGroup.Key}</code> ... {fixedByGroup} fixed");
+                    sb.AppendLine("</li>");
+                }
+                sb.AppendLine("</ul>");
+            }
+
+            return sb.ToString();
+        }
+
+        public override HealthCheckStatus ExecuteAction(HealthCheckAction action)
+        {
+            switch (action.Alias)
+            {
+                case _fixContentPaths:
+                    return CheckDocuments(true);
+                case _fixMediaPaths:
+                    return CheckMedia(true);
+                default:
+                    throw new InvalidOperationException("Action not supported");
+            }
+        }
+    }
+}

src/Umbraco.Web/PropertyEditors/GridPropertyEditor.cs

@@ -192,6 +192,10 @@ namespace Umbraco.Web.PropertyEditors
             public IEnumerable<UmbracoEntityReference> GetReferences(object value)
             {
                 var rawJson = value == null ? string.Empty : value is string str ? str : value.ToString();
+
+                if (rawJson.IsNullOrWhiteSpace())
+                    yield break;
+
                 DeserializeGridValue(rawJson, out var richTextEditorValues, out var mediaValues);
 
                 foreach (var umbracoEntityReference in richTextEditorValues.SelectMany(x =>

src/Umbraco.Web/PropertyEditors/MediaPickerPropertyEditor.cs

@@ -45,8 +45,11 @@ namespace Umbraco.Web.PropertyEditors
 
                 if (string.IsNullOrEmpty(asString)) yield break;
 
-                if (Udi.TryParse(asString, out var udi))
-                    yield return new UmbracoEntityReference(udi);
+                foreach (var udiStr in asString.Split(','))
+                {
+                    if (Udi.TryParse(udiStr, out var udi))
+                        yield return new UmbracoEntityReference(udi);
+                }
             }
         }
     }

src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs

@@ -502,6 +502,14 @@ namespace Umbraco.Web.PublishedCache.NuCache
             }
         }
 
+        /// <summary>
+        /// Validate the <see cref="ContentNodeKit"/> and try to create a parent <see cref="LinkedNode{ContentNode}"/>
+        /// </summary>
+        /// <param name="kit"></param>
+        /// <param name="parent"></param>
+        /// <returns>
+        /// Returns false if the parent was not found or if the kit validation failed
+        /// </returns>
         private bool BuildKit(ContentNodeKit kit, out LinkedNode<ContentNode> parent)
         {
             // make sure parent exists
@@ -512,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<ContentStore>($"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;
+            }
+
             // make sure the kit is valid
             if (kit.DraftData == null && kit.PublishedData == null)
             {
@@ -800,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);
             }
 
@@ -865,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);
 
@@ -873,9 +894,11 @@ 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);
-                id = link.Value.NextSiblingContentId;
+                var linkValue = link.Value; // capture local since clearing in recurse can clear it
+                ClearBranchLocked(linkValue); // recurse
+                id = linkValue.NextSiblingContentId;
             }
         }
 
@@ -1030,6 +1053,12 @@ namespace Umbraco.Web.PublishedCache.NuCache
 
             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.");
+
             // if parent has no children, clone parent + add as first child
             if (parent.FirstChildContentId < 0)
             {

src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs

@@ -11,7 +11,7 @@
     {
         public LinkedNode(TValue value, long gen, LinkedNode<TValue> next = null)
         {
-            Value = value;
+            Value = value; // This is allowed to be null, we actually explicitly set this to null in ClearLocked
             Gen = gen;
             Next = next;
         }

src/Umbraco.Web/Umbraco.Web.csproj

@@ -156,6 +156,7 @@
     <Compile Include="Editors\MacrosController.cs" />
     <Compile Include="Editors\RelationTypeController.cs" />
     <Compile Include="Editors\TinyMceController.cs" />
+    <Compile Include="HealthCheck\Checks\Data\DatabaseIntegrityCheck.cs" />
     <Compile Include="ImageCropperTemplateCoreExtensions.cs" />
     <Compile Include="IUmbracoContextFactory.cs" />
     <Compile Include="Install\ChangesMonitor.cs" />