From 13a1c025078e71c9a5bbc4662795b05e69ecf093 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 21 Feb 2019 14:46:14 +1100 Subject: [PATCH] Fixes content service Saving events so that they are fired before the model's values are published when the service is publishing --- .../Models/ContentRepositoryExtensions.cs | 58 +--- src/Umbraco.Core/Models/Property.cs | 50 ---- src/Umbraco.Core/Models/PropertyType.cs | 12 - .../Services/Implement/ContentService.cs | 278 ++++++++++++------ .../Services/PropertyValidationService.cs | 133 +++++++++ src/Umbraco.Core/Umbraco.Core.csproj | 1 + src/Umbraco.Tests/Models/VariationTests.cs | 7 +- .../Services/ContentServiceEventTests.cs | 115 ++++++-- .../Services/ContentServiceTests.cs | 6 +- 9 files changed, 438 insertions(+), 222 deletions(-) create mode 100644 src/Umbraco.Core/Services/PropertyValidationService.cs diff --git a/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs b/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs index 01812f8469..8f57842f34 100644 --- a/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs +++ b/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs @@ -1,7 +1,11 @@ using System; using System.Collections.Generic; using System.Linq; +using Umbraco.Core.Collections; +using Umbraco.Core.Composing; using Umbraco.Core.Exceptions; +using Umbraco.Core.PropertyEditors; +using Umbraco.Core.Services; namespace Umbraco.Core.Models { @@ -101,35 +105,6 @@ namespace Umbraco.Core.Models } } - /// - /// Validates the content item's properties pass variant rules - /// - /// If the content type is variant, then culture can be either '*' or an actual culture, but neither 'null' nor - /// 'empty'. If the content type is invariant, then culture can be either '*' or null or empty. - public static Property[] ValidateProperties(this IContentBase content, string culture = "*") - { - // select invalid properties - return content.Properties.Where(x => - { - // if culture is null, we validate invariant properties only - // if culture is '*' we validate both variant and invariant properties, automatically - // if culture is specific eg 'en-US' we both too, but explicitly - - var varies = x.PropertyType.VariesByCulture(); - - if (culture == null) - return !(varies || x.IsValid(null)); // validate invariant property, invariant culture - - if (culture == "*") - return !x.IsValid(culture); // validate property, all cultures - - return varies - ? !x.IsValid(culture) // validate variant property, explicit culture - : !x.IsValid(null); // validate invariant property, explicit culture - }) - .ToArray(); - } - public static void SetPublishInfo(this IContent content, string culture, string name, DateTime date) { if (string.IsNullOrWhiteSpace(name)) @@ -191,15 +166,19 @@ namespace Umbraco.Core.Models content.CultureInfos.AddOrUpdate(culture, name, date); } - public static bool PublishCulture(this IContent content, string culture = "*") + /// + /// This will set the publishing values for names and properties for the content/culture + /// + /// + /// + /// + /// A boolean if it's possible to publish the values for the provided culture. This may fail required names are not set. + /// + /// + /// This does not validation property data + /// + public static bool PublishCulture(this IContent content,string culture = "*") { - return PublishCulture(content, out _, culture); - } - - public static bool PublishCulture(this IContent content, out Property[] invalidProperties, string culture = "*") - { - invalidProperties = null; - culture = culture.NullOrWhiteSpaceAsNull(); // the variation should be supported by the content type properties @@ -208,11 +187,6 @@ namespace Umbraco.Core.Models if (!content.ContentType.SupportsPropertyVariation(culture, "*", true)) throw new NotSupportedException($"Culture \"{culture}\" is not supported by content type \"{content.ContentType.Alias}\" with variation \"{content.ContentType.Variations}\"."); - // the values we want to publish should be valid - invalidProperties = content.ValidateProperties(culture); - if (invalidProperties.Length > 0) - return false; - var alsoInvariant = false; if (culture == "*") // all cultures { diff --git a/src/Umbraco.Core/Models/Property.cs b/src/Umbraco.Core/Models/Property.cs index 726ddc1cf5..803b8eb5a5 100644 --- a/src/Umbraco.Core/Models/Property.cs +++ b/src/Umbraco.Core/Models/Property.cs @@ -332,56 +332,6 @@ namespace Umbraco.Core.Models return (pvalue, change); } - /// - /// Gets a value indicating whether the property has valid values. - /// - internal bool IsValid(string culture = "*", string segment = "*") - { - // TODO: validating values shouldn't be done here, this calls in to IsValidValue - - culture = culture.NullOrWhiteSpaceAsNull(); - segment = segment.NullOrWhiteSpaceAsNull(); - - // if validating invariant/neutral, and it is supported, validate - // (including ensuring that the value exists, if mandatory) - if ((culture == null || culture == "*") && (segment == null || segment == "*") && PropertyType.SupportsVariation(null, null)) - if (!IsValidValue(_pvalue?.EditedValue)) - return false; - - // if validating only invariant/neutral, we are good - if (culture == null && segment == null) - return true; - - // if nothing else to validate, we are good - if ((culture == null || culture == "*") && (segment == null || segment == "*") && !PropertyType.VariesByCulture()) - return true; - - // for anything else, validate the existing values (including mandatory), - // but we cannot validate mandatory globally (we don't know the possible cultures and segments) - - if (_vvalues == null) return culture == "*" || IsValidValue(null); - - var pvalues = _vvalues.Where(x => - PropertyType.SupportsVariation(x.Value.Culture, x.Value.Segment, true) && // the value variation is ok - (culture == "*" || x.Value.Culture.InvariantEquals(culture)) && // the culture matches - (segment == "*" || x.Value.Segment.InvariantEquals(segment))) // the segment matches - .Select(x => x.Value) - .ToList(); - - return pvalues.Count == 0 || pvalues.All(x => IsValidValue(x.EditedValue)); - } - - /// - /// Boolean indicating whether the passed in value is valid - /// - /// - /// True is property value is valid, otherwise false - private bool IsValidValue(object value) - { - // TODO: this shouldn't exist here, the model itself shouldn't be responsible for it's own validation and this requires singleton access - return PropertyType.IsPropertyValueValid(value); - } - protected override void PerformDeepClone(object clone) { base.PerformDeepClone(clone); diff --git a/src/Umbraco.Core/Models/PropertyType.cs b/src/Umbraco.Core/Models/PropertyType.cs index d3a47c281c..e1da631868 100644 --- a/src/Umbraco.Core/Models/PropertyType.cs +++ b/src/Umbraco.Core/Models/PropertyType.cs @@ -363,18 +363,6 @@ namespace Umbraco.Core.Models } - // TODO: this and other value validation methods should be a service level (not a model) thing. Changing this to internal for now - /// - /// Determines whether a value is valid for this property type. - /// - internal bool IsPropertyValueValid(object value) - { - var editor = Current.PropertyEditors[_propertyEditorAlias]; // TODO: inject - var configuration = Current.Services.DataTypeService.GetDataType(_dataTypeId).Configuration; // TODO: inject - var valueEditor = editor.GetValueEditor(configuration); - return !valueEditor.Validate(value, Mandatory, ValidationRegExp).Any(); - } - /// /// Sanitizes a property type alias. /// diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index 922eb4e2db..412f2a1160 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -10,6 +10,7 @@ using Umbraco.Core.Models.Membership; using Umbraco.Core.Persistence.DatabaseModelDefinitions; using Umbraco.Core.Persistence.Querying; using Umbraco.Core.Persistence.Repositories; +using Umbraco.Core.PropertyEditors; using Umbraco.Core.Scoping; using Umbraco.Core.Services.Changes; @@ -27,6 +28,8 @@ namespace Umbraco.Core.Services.Implement private readonly IDocumentBlueprintRepository _documentBlueprintRepository; private readonly ILanguageRepository _languageRepository; private IQuery _queryNotTrashed; + //TODO: The non-lazy object should be injected + private readonly Lazy _propertyValidationService = new Lazy(() => new PropertyValidationService()); #region Constructors @@ -193,7 +196,7 @@ namespace Umbraco.Core.Services.Implement var content = new Content(name, parentId, contentType); using (var scope = ScopeProvider.CreateScope()) { - CreateContent(scope, content, parent, userId, false); + CreateContent(scope, content, userId, false); scope.Complete(); } @@ -227,7 +230,7 @@ namespace Umbraco.Core.Services.Implement throw new ArgumentException("No content type with that alias.", nameof(contentTypeAlias)); // causes rollback var content = new Content(name, parent, contentType); - CreateContent(scope, content, parent, userId, false); + CreateContent(scope, content, userId, false); scope.Complete(); return content; @@ -261,7 +264,7 @@ namespace Umbraco.Core.Services.Implement throw new ArgumentException("No content with that id.", nameof(parentId)); // causes rollback var content = parentId > 0 ? new Content(name, parent, contentType) : new Content(name, parentId, contentType); - CreateContent(scope, content, parent, userId, true); + CreateContent(scope, content, userId, true); scope.Complete(); return content; @@ -293,14 +296,14 @@ namespace Umbraco.Core.Services.Implement throw new ArgumentException("No content type with that alias.", nameof(contentTypeAlias)); // causes rollback var content = new Content(name, parent, contentType); - CreateContent(scope, content, parent, userId, true); + CreateContent(scope, content, userId, true); scope.Complete(); return content; } } - private void CreateContent(IScope scope, Content content, IContent parent, int userId, bool withIdentity) + private void CreateContent(IScope scope, IContent content, int userId, bool withIdentity) { content.CreatorId = userId; content.WriterId = userId; @@ -311,12 +314,12 @@ namespace Umbraco.Core.Services.Implement // if saving is cancelled, content remains without an identity var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); - if (scope.Events.DispatchCancelable(Saving, this, saveEventArgs, "Saving")) + if (scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) return; _documentRepository.Save(content); - scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), "Saved"); + scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), nameof(Saved)); scope.Events.Dispatch(TreeChanged, this, new TreeChange(content, TreeChangeTypes.RefreshNode).ToEventArgs()); } @@ -758,7 +761,7 @@ namespace Umbraco.Core.Services.Implement using (var scope = ScopeProvider.CreateScope()) { var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); - if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, "Saving")) + if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) { scope.Complete(); return OperationResult.Cancel(evtMsgs); @@ -783,7 +786,7 @@ namespace Umbraco.Core.Services.Implement if (raiseEvents) { - scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), "Saved"); + scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), nameof(Saved)); } var changeType = TreeChangeTypes.RefreshNode; scope.Events.Dispatch(TreeChanged, this, new TreeChange(content, changeType).ToEventArgs()); @@ -813,7 +816,7 @@ namespace Umbraco.Core.Services.Implement using (var scope = ScopeProvider.CreateScope()) { var saveEventArgs = new ContentSavingEventArgs(contentsA, evtMsgs); - if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, "Saving")) + if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) { scope.Complete(); return OperationResult.Cancel(evtMsgs); @@ -833,7 +836,7 @@ namespace Umbraco.Core.Services.Implement if (raiseEvents) { - scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), "Saved"); + scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), nameof(Saved)); } scope.Events.Dispatch(TreeChanged, this, treeChanges.ToEventArgs()); Audit(AuditType.Save, userId == -1 ? 0 : userId, Constants.System.Root, "Saved multiple content"); @@ -866,35 +869,51 @@ namespace Umbraco.Core.Services.Implement throw new NotSupportedException($"Culture \"{culture}\" is not supported by invariant content types."); } - // if culture is specific, first publish the invariant values, then publish the culture itself. - // if culture is '*', then publish them all (including variants) - - Property[] invalidProperties; - - // explicitly SaveAndPublish a specific culture also publishes invariant values - if (!culture.IsNullOrWhiteSpace() && culture != "*") + using (var scope = ScopeProvider.CreateScope()) { - // publish the invariant values - var publishInvariant = content.PublishCulture(out invalidProperties, null); - if (!publishInvariant) + scope.WriteLock(Constants.Locks.ContentTree); + + var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); + if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, content); + + Property[] invalidProperties; + + // if culture is specific, first publish the invariant values, then publish the culture itself. + // if culture is '*', then publish them all (including variants) + + // explicitly SaveAndPublish a specific culture also publishes invariant values + if (!culture.IsNullOrWhiteSpace() && culture != "*") + { + // publish the invariant values + var publishInvariant = content.PublishCulture(null); + if (!publishInvariant) + return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content); + + //validate the property values + if (!_propertyValidationService.Value.IsPropertyDataValid(content, out invalidProperties)) + return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content) + { + InvalidProperties = invalidProperties + }; + } + + // publish the culture(s) + var publishCulture = content.PublishCulture(culture); + if (!publishCulture) + return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content); + + //validate the property values + if (!_propertyValidationService.Value.IsPropertyDataValid(content, out invalidProperties)) return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content) { - InvalidProperties = invalidProperties ?? Enumerable.Empty() + InvalidProperties = invalidProperties }; + var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, userId, raiseEvents); + scope.Complete(); + return result; } - - // publish the culture(s) - var publishCulture = content.PublishCulture(out invalidProperties, culture); - if (!publishCulture) - return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content) - { - InvalidProperties = invalidProperties ?? Enumerable.Empty() - }; - - // finally, "save publishing" - // what happens next depends on whether the content can be published or not - return CommitDocumentChanges(content, userId, raiseEvents); } /// @@ -903,23 +922,40 @@ namespace Umbraco.Core.Services.Implement if (content == null) throw new ArgumentNullException(nameof(content)); if (cultures == null) throw new ArgumentNullException(nameof(cultures)); - var evtMsgs = EventMessagesFactory.Get(); - - var varies = content.ContentType.VariesByCulture(); - - if (cultures.Length == 0) + using (var scope = ScopeProvider.CreateScope()) { - //no cultures specified and doesn't vary, so publish it, else nothing to publish - return !varies - ? SaveAndPublish(content, userId: userId, raiseEvents: raiseEvents) - : new PublishResult(PublishResultType.FailedPublishNothingToPublish, evtMsgs, content); + scope.WriteLock(Constants.Locks.ContentTree); + + var evtMsgs = EventMessagesFactory.Get(); + var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); + if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, content); + + var varies = content.ContentType.VariesByCulture(); + + if (cultures.Length == 0) + { + //no cultures specified and doesn't vary, so publish it, else nothing to publish + return !varies + ? SaveAndPublish(content, userId: userId, raiseEvents: raiseEvents) + : new PublishResult(PublishResultType.FailedPublishNothingToPublish, evtMsgs, content); + } + + + if (cultures.Select(content.PublishCulture).Any(isValid => !isValid)) + return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content); + + //validate the property values + if (!_propertyValidationService.Value.IsPropertyDataValid(content, out var invalidProperties)) + return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content) + { + InvalidProperties = invalidProperties + }; + + var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, userId, raiseEvents); + scope.Complete(); + return result; } - - // TODO: currently, no way to know which one failed - if (cultures.Select(content.PublishCulture).Any(isValid => !isValid)) - return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content); - - return CommitDocumentChanges(content, userId, raiseEvents); } /// @@ -952,56 +988,87 @@ namespace Umbraco.Core.Services.Implement if (!content.Published) return new PublishResult(PublishResultType.SuccessUnpublishAlready, evtMsgs, content); - // all cultures = unpublish whole - if (culture == "*" || (!content.ContentType.VariesByCulture() && culture == null)) + using (var scope = ScopeProvider.CreateScope()) { - content.PublishedState = PublishedState.Unpublishing; - } - else - { - // If the culture we want to unpublish was already unpublished, nothing to do. - // To check for that we need to lookup the persisted content item - var persisted = content.HasIdentity ? GetById(content.Id) : null; + scope.WriteLock(Constants.Locks.ContentTree); - if (persisted != null && !persisted.IsCulturePublished(culture)) - return new PublishResult(PublishResultType.SuccessUnpublishAlready, evtMsgs, content); + var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); + if (scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, content); - // unpublish the culture - content.UnpublishCulture(culture); + // all cultures = unpublish whole + if (culture == "*" || (!content.ContentType.VariesByCulture() && culture == null)) + { + content.PublishedState = PublishedState.Unpublishing; + } + else + { + // If the culture we want to unpublish was already unpublished, nothing to do. + // To check for that we need to lookup the persisted content item + var persisted = content.HasIdentity ? GetById(content.Id) : null; + + if (persisted != null && !persisted.IsCulturePublished(culture)) + return new PublishResult(PublishResultType.SuccessUnpublishAlready, evtMsgs, content); + + // unpublish the culture + content.UnpublishCulture(culture); + } + + var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, userId); + scope.Complete(); + return result; } - // finally, "save publishing" - return CommitDocumentChanges(content, userId); } /// /// Saves a document and publishes/unpublishes any pending publishing changes made to the document. /// /// + /// + /// This MUST NOT be called from within this service, this used to be a public API and must only be used outside of this service. + /// Internally in this service, calls must be made to CommitDocumentChangesInternal + /// + /// /// This is the underlying logic for both publishing and unpublishing any document - /// Pending publishing/unpublishing changes on a document are made with calls to and - /// . + /// Pending publishing/unpublishing changes on a document are made with calls to and + /// . /// When publishing or unpublishing a single culture, or all cultures, use /// and . But if the flexibility to both publish and unpublish in a single operation is required - /// then this method needs to be used in combination with and + /// then this method needs to be used in combination with and /// on the content itself - this prepares the content, but does not commit anything - and then, invoke /// to actually commit the changes to the database. /// The document is *always* saved, even when publishing fails. /// - internal PublishResult CommitDocumentChanges(IContent content, int userId = Constants.Security.SuperUserId, bool raiseEvents = true) + internal PublishResult CommitDocumentChanges(IContent content, + int userId = Constants.Security.SuperUserId, bool raiseEvents = true) { using (var scope = ScopeProvider.CreateScope()) { + var evtMsgs = EventMessagesFactory.Get(); + scope.WriteLock(Constants.Locks.ContentTree); - var result = CommitDocumentChangesInternal(scope, content, userId, raiseEvents); + + var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); + if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, content); + + var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, userId, raiseEvents); scope.Complete(); return result; } } - private PublishResult CommitDocumentChangesInternal(IScope scope, IContent content, int userId = Constants.Security.SuperUserId, bool raiseEvents = true, bool branchOne = false, bool branchRoot = false) + private PublishResult CommitDocumentChangesInternal(IScope scope, IContent content, + ContentSavingEventArgs saveEventArgs, + int userId = Constants.Security.SuperUserId, bool raiseEvents = true, bool branchOne = false, bool branchRoot = false) { - var evtMsgs = EventMessagesFactory.Get(); + if (scope == null) throw new ArgumentNullException(nameof(scope)); + if (content == null) throw new ArgumentNullException(nameof(content)); + if (saveEventArgs == null) throw new ArgumentNullException(nameof(saveEventArgs)); + + var evtMsgs = saveEventArgs.Messages; + PublishResult publishResult = null; PublishResult unpublishResult = null; @@ -1027,10 +1094,6 @@ namespace Umbraco.Core.Services.Implement var changeType = isNew ? TreeChangeTypes.RefreshNode : TreeChangeTypes.RefreshBranch; var previouslyPublished = content.HasIdentity && content.Published; - // always save - var saveEventArgs = new ContentSavingEventArgs(content, evtMsgs); - if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, "Saving")) - return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, content); if (publishing) { @@ -1116,7 +1179,7 @@ namespace Umbraco.Core.Services.Implement // raise the Saved event, always if (raiseEvents) { - scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), "Saved"); + scope.Events.Dispatch(Saved, this, saveEventArgs.ToContentSavedEventArgs(), nameof(Saved)); } if (unpublishing) // we have tried to unpublish - won't happen in a branch @@ -1251,7 +1314,13 @@ namespace Umbraco.Core.Services.Implement .Distinct() .ToList(); - Property[] invalidProperties = null; + if (pendingCultures.Count == 0) + break; //shouldn't happen but no point in continuing if there's nothing there + + var saveEventArgs = new ContentSavingEventArgs(d, evtMsgs); + if (scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + yield return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, d); + var publishing = true; foreach (var culture in pendingCultures) { @@ -1260,19 +1329,24 @@ namespace Umbraco.Core.Services.Implement if (d.Trashed) continue; // won't publish - publishing &= d.PublishCulture(out invalidProperties, culture); //set the culture to be published + //publish the culture values and validate the property values, if validation fails, log the invalid properties so the develeper has an idea of what has failed + Property[] invalidProperties = null; + var tryPublish = d.PublishCulture(culture) && _propertyValidationService.Value.IsPropertyDataValid(d, out invalidProperties); + if (invalidProperties != null && invalidProperties.Length > 0) + Logger.Warn("Scheduled publishing will fail for document {DocumentId} and culture {Culture} because of invalid properties {InvalidProperties}", + d.Id, culture, string.Join(",", invalidProperties.Select(x => x.Alias))); + + publishing &= tryPublish; //set the culture to be published if (!publishing) break; // no point continuing } if (d.Trashed) result = new PublishResult(PublishResultType.FailedPublishIsTrashed, evtMsgs, d); else if (!publishing) - result = new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, d) - { - InvalidProperties = invalidProperties ?? Enumerable.Empty() - }; + result = new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, d); else - result = CommitDocumentChanges(d, d.WriterId); + result = CommitDocumentChangesInternal(scope, d, saveEventArgs, d.WriterId); + if (result.Success == false) Logger.Error(null, "Failed to publish document id={DocumentId}, reason={Reason}.", d.Id, result.Result); @@ -1306,6 +1380,13 @@ namespace Umbraco.Core.Services.Implement .Distinct() .ToList(); + if (pendingCultures.Count == 0) + break; //shouldn't happen but no point in continuing if there's nothing there + + var saveEventArgs = new ContentSavingEventArgs(d, evtMsgs); + if (scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + yield return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, d); + foreach (var c in pendingCultures) { //Clear this schedule for this culture @@ -1314,13 +1395,11 @@ namespace Umbraco.Core.Services.Implement d.UnpublishCulture(c); } - if (pendingCultures.Count > 0) - { - result = CommitDocumentChanges(d, d.WriterId); - if (result.Success == false) - Logger.Error(null, "Failed to publish document id={DocumentId}, reason={Reason}.", d.Id, result.Result); - yield return result; - } + result = CommitDocumentChangesInternal(scope, d, saveEventArgs, d.WriterId); + if (result.Success == false) + Logger.Error(null, "Failed to publish document id={DocumentId}, reason={Reason}.", d.Id, result.Result); + yield return result; + } else { @@ -1341,13 +1420,15 @@ namespace Umbraco.Core.Services.Implement } } - private bool SaveAndPublishBranch_PublishCultures(IContent c, HashSet culturesToPublish) + private bool SaveAndPublishBranch_PublishCultures(IContent content, HashSet culturesToPublish) { + //TODO: This does not support being able to return invalid property details to bubble up to the UI + // variant content type - publish specified cultures // invariant content type - publish only the invariant culture - return c.ContentType.VariesByCulture() - ? culturesToPublish.All(c.PublishCulture) - : c.PublishCulture(); + return content.ContentType.VariesByCulture() + ? culturesToPublish.All(culture => content.PublishCulture(culture) && _propertyValidationService.Value.IsPropertyDataValid(content, out _)) + : content.PublishCulture() && _propertyValidationService.Value.IsPropertyDataValid(content, out _); } private HashSet SaveAndPublishBranch_ShouldPublish3(ref HashSet cultures, string c, bool published, bool edited, bool isRoot, bool force) @@ -1547,15 +1628,18 @@ namespace Umbraco.Core.Services.Implement if (culturesToPublish.Count == 0) // empty = already published return new PublishResult(PublishResultType.SuccessPublishAlready, evtMsgs, document); + var saveEventArgs = new ContentSavingEventArgs(document, evtMsgs); + if (scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) + return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, document); + // publish & check if values are valid if (!publishCultures(document, culturesToPublish)) { //TODO: Based on this callback behavior there is no way to know which properties may have been invalid if this failed, see other results of FailedPublishContentInvalid return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, document); - } - + } - var result = CommitDocumentChangesInternal(scope, document, userId, branchOne: true, branchRoot: isRoot); + var result = CommitDocumentChangesInternal(scope, document, saveEventArgs, userId, branchOne: true, branchRoot: isRoot); if (result.Success) publishedDocuments.Add(document); return result; diff --git a/src/Umbraco.Core/Services/PropertyValidationService.cs b/src/Umbraco.Core/Services/PropertyValidationService.cs new file mode 100644 index 0000000000..538ec323e9 --- /dev/null +++ b/src/Umbraco.Core/Services/PropertyValidationService.cs @@ -0,0 +1,133 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; +using Umbraco.Core.Collections; +using Umbraco.Core.Composing; +using Umbraco.Core.Models; +using Umbraco.Core.PropertyEditors; + +namespace Umbraco.Core.Services +{ + //TODO: We should make this an interface and inject it into the ContentService + internal class PropertyValidationService + { + private readonly PropertyEditorCollection _propertyEditors; + private readonly IDataTypeService _dataTypeService; + + public PropertyValidationService(PropertyEditorCollection propertyEditors, IDataTypeService dataTypeService) + { + _propertyEditors = propertyEditors; + _dataTypeService = dataTypeService; + } + + //TODO: Remove this method in favor of the overload specifying all dependencies + public PropertyValidationService() + : this(Current.PropertyEditors, Current.Services.DataTypeService) + { + } + + /// + /// Validates the content item's properties pass validation rules + /// + /// If the content type is variant, then culture can be either '*' or an actual culture, but neither 'null' nor + /// 'empty'. If the content type is invariant, then culture can be either '*' or null or empty. + public bool IsPropertyDataValid(IContentBase content, out Property[] invalidProperties, string culture = "*") + { + // select invalid properties + invalidProperties = content.Properties.Where(x => + { + // if culture is null, we validate invariant properties only + // if culture is '*' we validate both variant and invariant properties, automatically + // if culture is specific eg 'en-US' we both too, but explicitly + + var varies = x.PropertyType.VariesByCulture(); + + if (culture == null) + return !(varies || IsPropertyValid(x, null)); // validate invariant property, invariant culture + + if (culture == "*") + return !IsPropertyValid(x, culture); // validate property, all cultures + + return varies + ? !IsPropertyValid(x, culture) // validate variant property, explicit culture + : !IsPropertyValid(x, null); // validate invariant property, explicit culture + }) + .ToArray(); + + return invalidProperties.Length == 0; + } + + /// + /// Gets a value indicating whether the property has valid values. + /// + public bool IsPropertyValid(Property property, string culture = "*", string segment = "*") + { + //NOTE - the pvalue and vvalues logic in here is borrowed directly from the Property.Values setter so if you are wondering what that's all about, look there. + // The underlying Property._pvalue and Property._vvalues are not exposed but we can re-create these values ourselves which is what it's doing. + + culture = culture.NullOrWhiteSpaceAsNull(); + segment = segment.NullOrWhiteSpaceAsNull(); + + Property.PropertyValue pvalue = null; + + // if validating invariant/neutral, and it is supported, validate + // (including ensuring that the value exists, if mandatory) + if ((culture == null || culture == "*") && (segment == null || segment == "*") && property.PropertyType.SupportsVariation(null, null)) + { + pvalue = property.Values.FirstOrDefault(x => x.Culture == null && x.Segment == null); + if (!IsValidPropertyValue(property, pvalue?.EditedValue)) + return false; + } + + // if validating only invariant/neutral, we are good + if (culture == null && segment == null) + return true; + + // if nothing else to validate, we are good + if ((culture == null || culture == "*") && (segment == null || segment == "*") && !property.PropertyType.VariesByCulture()) + return true; + + // for anything else, validate the existing values (including mandatory), + // but we cannot validate mandatory globally (we don't know the possible cultures and segments) + + var vvalues = property.Values.Count > (pvalue == null ? 0 : 1) + ? property.Values.Where(x => x != pvalue).ToDictionary(x => new CompositeNStringNStringKey(x.Culture, x.Segment), x => x) + : null; + + if (vvalues == null) return culture == "*" || IsValidPropertyValue(property,null); + + var pvalues = vvalues.Where(x => + property.PropertyType.SupportsVariation(x.Value.Culture, x.Value.Segment, true) && // the value variation is ok + (culture == "*" || x.Value.Culture.InvariantEquals(culture)) && // the culture matches + (segment == "*" || x.Value.Segment.InvariantEquals(segment))) // the segment matches + .Select(x => x.Value) + .ToList(); + + return pvalues.Count == 0 || pvalues.All(x => IsValidPropertyValue(property, x.EditedValue)); + } + + /// + /// Boolean indicating whether the passed in value is valid + /// + /// + /// + /// True is property value is valid, otherwise false + private bool IsValidPropertyValue(Property property, object value) + { + return IsPropertyValueValid(property.PropertyType, value); + } + + /// + /// Determines whether a value is valid for this property type. + /// + private bool IsPropertyValueValid(PropertyType propertyType, object value) + { + var editor = _propertyEditors[propertyType.PropertyEditorAlias]; + var configuration = _dataTypeService.GetDataType(propertyType.DataTypeId).Configuration; + var valueEditor = editor.GetValueEditor(configuration); + return !valueEditor.Validate(value, propertyType.Mandatory, propertyType.ValidationRegExp).Any(); + } + } +} diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index c7e7db18a7..7ed4c398dc 100755 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -210,6 +210,7 @@ + diff --git a/src/Umbraco.Tests/Models/VariationTests.cs b/src/Umbraco.Tests/Models/VariationTests.cs index 36fb399fa7..66173fc5ba 100644 --- a/src/Umbraco.Tests/Models/VariationTests.cs +++ b/src/Umbraco.Tests/Models/VariationTests.cs @@ -486,14 +486,15 @@ namespace Umbraco.Tests.Models prop.SetValue("a"); Assert.AreEqual("a", prop.GetValue()); Assert.IsNull(prop.GetValue(published: true)); + var propertyValidationService = new PropertyValidationService(Current.Factory.GetInstance(), Current.Factory.GetInstance().DataTypeService); - Assert.IsTrue(prop.IsValid()); + Assert.IsTrue(propertyValidationService.IsPropertyValid(prop)); propertyType.Mandatory = true; - Assert.IsTrue(prop.IsValid()); + Assert.IsTrue(propertyValidationService.IsPropertyValid(prop)); prop.SetValue(null); - Assert.IsFalse(prop.IsValid()); + Assert.IsFalse(propertyValidationService.IsPropertyValid(prop)); // can publish, even though invalid prop.PublishValues(); diff --git a/src/Umbraco.Tests/Services/ContentServiceEventTests.cs b/src/Umbraco.Tests/Services/ContentServiceEventTests.cs index 2c0cde0a94..7342665379 100644 --- a/src/Umbraco.Tests/Services/ContentServiceEventTests.cs +++ b/src/Umbraco.Tests/Services/ContentServiceEventTests.cs @@ -81,9 +81,15 @@ namespace Umbraco.Tests.Services ContentService.Saving += OnSaving; ContentService.Saved += OnSaved; - contentService.Save(document); - ContentService.Saving -= OnSaving; - ContentService.Saved -= OnSaved; + try + { + contentService.Save(document); + } + finally + { + ContentService.Saving -= OnSaving; + ContentService.Saved -= OnSaved; + } } [Test] @@ -123,9 +129,16 @@ namespace Umbraco.Tests.Services ContentService.Saving += OnSaving; ContentService.Saved += OnSaved; - contentService.Save(document); - ContentService.Saving -= OnSaving; - ContentService.Saved -= OnSaved; + try + { + contentService.Save(document); + } + finally + { + ContentService.Saving -= OnSaving; + ContentService.Saved -= OnSaved; + } + } [Test] @@ -179,9 +192,15 @@ namespace Umbraco.Tests.Services ContentService.Publishing += OnPublishing; ContentService.Published += OnPublished; - contentService.SaveAndPublish(document, "fr-FR"); - ContentService.Publishing -= OnPublishing; - ContentService.Published -= OnPublished; + try + { + contentService.SaveAndPublish(document, "fr-FR"); + } + finally + { + ContentService.Publishing -= OnPublishing; + ContentService.Published -= OnPublished; + } document = contentService.GetById(document.Id); @@ -225,13 +244,69 @@ namespace Umbraco.Tests.Services Assert.AreEqual("title", propValue.PublishedValue); } - //We are binding to Saving (not Publishing), because it shouldn't make a difference, when setting a property value - //during Saving, this should become published during a SaveAndPublish operation + //We are binding to Saving (not Publishing), because the Publishing event is really just used for cancelling, it should not be + //used for setting values and it won't actually work! This is because the Publishing event is raised AFTER the values on the model + //are published, but Saving is raised BEFORE. ContentService.Saving += OnSaving; ContentService.Saved += OnSaved; - contentService.SaveAndPublish(document); - ContentService.Saving -= OnSaving; - ContentService.Saved -= OnSaved; + try + { + contentService.SaveAndPublish(document); + } + finally + { + ContentService.Saving -= OnSaving; + ContentService.Saved -= OnSaved; + } + } + + [Test] + public void Publishing_Set_Mandatory_Value() + { + var contentTypeService = ServiceContext.ContentTypeService; + + var contentType = MockedContentTypes.CreateTextPageContentType(); + var titleProperty = contentType.PropertyTypes.First(x => x.Alias == "title"); + titleProperty.Mandatory = true; // make this required! + ServiceContext.FileService.SaveTemplate(contentType.DefaultTemplate); + contentTypeService.Save(contentType); + + var contentService = ServiceContext.ContentService; + + IContent document = new Content("content", -1, contentType); + + var result = contentService.SaveAndPublish(document); + Assert.IsFalse(result.Success); + Assert.AreEqual("title", result.InvalidProperties.First().Alias); + + //TODO: The ContentService doesn't reset the document's PublishedState so since the above fails, if we then try to do + // a SaveAndPublish again, we will get an exception: "Cannot save-and-publish (un)publishing content, use the dedicated CommitDocumentChanges method." + // but this exception is misleading and is caused because the document's PublishedState wasn't reset. + // So instead, we'll just re-create it. + document = new Content("content", -1, contentType); + + void OnSaving(IContentService sender, ContentSavingEventArgs e) + { + var saved = e.SavedEntities.First(); + + Assert.IsTrue(document.GetValue("title").IsNullOrWhiteSpace()); + + saved.SetValue("title", "title"); + } + + //We are binding to Saving (not Publishing), because the Publishing event is really just used for cancelling, it should not be + //used for setting values and it won't actually work! This is because the Publishing event is raised AFTER the values on the model + //are published, but Saving is raised BEFORE. + ContentService.Saving += OnSaving; + try + { + result = contentService.SaveAndPublish(document); + Assert.IsTrue(result.Success); //will succeed now because we were able to specify the required value in the Saving event + } + finally + { + ContentService.Saving -= OnSaving; + } } [Test] @@ -293,9 +368,15 @@ namespace Umbraco.Tests.Services ContentService.Publishing += OnPublishing; ContentService.Published += OnPublished; - contentService.CommitDocumentChanges(document); - ContentService.Publishing -= OnPublishing; - ContentService.Published -= OnPublished; + try + { + contentService.CommitDocumentChanges(document); + } + finally + { + ContentService.Publishing -= OnPublishing; + ContentService.Published -= OnPublished; + } document = contentService.GetById(document.Id); diff --git a/src/Umbraco.Tests/Services/ContentServiceTests.cs b/src/Umbraco.Tests/Services/ContentServiceTests.cs index bcbf01d3f0..04cdc2aab7 100644 --- a/src/Umbraco.Tests/Services/ContentServiceTests.cs +++ b/src/Umbraco.Tests/Services/ContentServiceTests.cs @@ -18,6 +18,7 @@ using Umbraco.Core.Services.Implement; using Umbraco.Tests.Testing; using Umbraco.Core.Persistence.DatabaseModelDefinitions; using Umbraco.Core.Cache; +using Umbraco.Core.PropertyEditors; using Umbraco.Tests.LegacyXmlPublishedCache; namespace Umbraco.Tests.Services @@ -986,7 +987,10 @@ namespace Umbraco.Tests.Services Assert.IsTrue(parent.Published); // content cannot publish values because they are invalid - Assert.IsNotEmpty(content.ValidateProperties()); + var propertyValidationService = new PropertyValidationService(Factory.GetInstance(), ServiceContext.DataTypeService); + var isValid = propertyValidationService.IsPropertyDataValid(content, out var invalidProperties); + Assert.IsFalse(isValid); + Assert.IsNotEmpty(invalidProperties); // and therefore cannot be published, // because it did not have a published version at all