From f9797922901f51d185a78efd9cacfa51eb330d5b Mon Sep 17 00:00:00 2001 From: Stephan Date: Mon, 18 Dec 2017 16:53:58 +0100 Subject: [PATCH] U4-10764 - workaround broken events --- .../Events/ScopeEventDispatcherBase.cs | 324 +++++++++--------- .../Scoping/ScopeEventDispatcherTests.cs | 41 ++- 2 files changed, 193 insertions(+), 172 deletions(-) diff --git a/src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs b/src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs index c703a10cb4..d7a2ca0fd8 100644 --- a/src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs +++ b/src/Umbraco.Core/Events/ScopeEventDispatcherBase.cs @@ -76,260 +76,254 @@ namespace Umbraco.Core.Events { if (_events == null) return Enumerable.Empty(); - + + IReadOnlyList events; switch (filter) { case EventDefinitionFilter.All: - return FilterSupersededAndUpdateToLatestEntity(_events); + events = _events; + break; case EventDefinitionFilter.FirstIn: var l1 = new OrderedHashSet(); foreach (var e in _events) - { l1.Add(e); - } - return FilterSupersededAndUpdateToLatestEntity(l1); + events = l1; + break; case EventDefinitionFilter.LastIn: var l2 = new OrderedHashSet(keepOldest: false); foreach (var e in _events) - { l2.Add(e); - } - return FilterSupersededAndUpdateToLatestEntity(l2); + events = l2; + break; default: throw new ArgumentOutOfRangeException("filter", filter, null); } + + return FilterSupersededAndUpdateToLatestEntity(events); } - - private class EventDefinitionTypeData + + private class EventDefinitionInfos { public IEventDefinition EventDefinition { get; set; } - public Type EventArgType { get; set; } - public SupersedeEventAttribute[] SupersedeAttributes { get; set; } + public Type[] SupersedeTypes { get; set; } } - - /// - /// This will iterate over the events (latest first) and filter out any events or entities in event args that are included - /// in more recent events that Supersede previous ones. For example, If an Entity has been Saved and then Deleted, we don't want - /// to raise the Saved event (well actually we just don't want to include it in the args for that saved event) - /// - /// - /// - private static IEnumerable FilterSupersededAndUpdateToLatestEntity(IReadOnlyList events) + + // fixme + // this is way too convoluted, the superceede attribute is used only on DeleteEventargs to specify + // that it superceeds save, publish, move and copy - BUT - publish event args is also used for + // unpublishing and should NOT be superceeded - so really it should not be managed at event args + // level but at event level + // + // what we want is: + // if an entity is deleted, then all Saved, Moved, Copied, Published events prior to this should + // not trigger for the entity - and even though, does it make any sense? making a copy of an entity + // should ... trigger? + // + // not going to refactor it all - we probably want to *always* trigger event but tell people that + // due to scopes, they should not expected eg a saved entity to still be around - however, now, + // going to write a ugly condition to deal with U4-10764 + + // iterates over the events (latest first) and filter out any events or entities in event args that are included + // in more recent events that Supersede previous ones. For example, If an Entity has been Saved and then Deleted, we don't want + // to raise the Saved event (well actually we just don't want to include it in the args for that saved event) + internal static IEnumerable FilterSupersededAndUpdateToLatestEntity(IReadOnlyList events) { - //used to keep the 'latest' entity and associated event definition data - var allEntities = new List>(); - - //tracks all CancellableObjectEventArgs instances in the events which is the only type of args we can work with - var cancelableArgs = new List(); + // keeps the 'latest' entity and associated event data + var entities = new List>(); + // collects the event definitions + // collects the arguments in result, that require their entities to be updated var result = new List(); + var resultArgs = new List(); - //This will eagerly load all of the event arg types and their attributes so we don't have to continuously look this data up - var allArgTypesWithAttributes = events.Select(x => x.Args.GetType()) + // eagerly fetch superceeded arg types for each arg type + var argTypeSuperceeding = events.Select(x => x.Args.GetType()) .Distinct() - .ToDictionary(x => x, x => x.GetCustomAttributes(false).ToArray()); - - //Iterate all events and collect the actual entities in them and relates them to their corresponding EventDefinitionTypeData - //we'll process the list in reverse because events are added in the order they are raised and we want to filter out - //any entities from event args that are not longer relevant - //(i.e. if an item is Deleted after it's Saved, we won't include the item in the Saved args) + .ToDictionary(x => x, x => x.GetCustomAttributes(false).Select(y => y.SupersededEventArgsType).ToArray()); + + // iterate over all events and filter + // + // process the list in reverse, because events are added in the order they are raised and we want to keep + // the latest (most recent) entities and filter out what is not relevant anymore (too old), eg if an entity + // is Deleted after being Saved, we want to filter out the Saved event for (var index = events.Count - 1; index >= 0; index--) { - var eventDefinition = events[index]; + var def = events[index]; - var argType = eventDefinition.Args.GetType(); - var attributes = allArgTypesWithAttributes[eventDefinition.Args.GetType()]; - - var meta = new EventDefinitionTypeData + var infos = new EventDefinitionInfos { - EventDefinition = eventDefinition, - EventArgType = argType, - SupersedeAttributes = attributes + EventDefinition = def, + SupersedeTypes = argTypeSuperceeding[def.Args.GetType()] }; - var args = eventDefinition.Args as CancellableObjectEventArgs; - if (args != null) + var args = def.Args as CancellableObjectEventArgs; + if (args == null) { - var list = TypeHelper.CreateGenericEnumerableFromObject(args.EventObject); - - if (list == null) + // not a cancellable event arg, include event definition in result + result.Add(def); + } + else + { + // event object can either be a single object or an enumerable of objects + // try to get as an enumerable, get null if it's not + var eventObjects = TypeHelper.CreateGenericEnumerableFromObject(args.EventObject); + if (eventObjects == null) { - //extract the event object - var obj = args.EventObject as IEntity; - if (obj != null) + // single object, cast as an IEntity + // if cannot cast, cannot filter, nothing to do - just add to output FIXME not? + var eventEntity = args.EventObject as IEntity; + if (eventEntity == null) + continue; + + // look for this entity in superceding event args + // found = must be removed (ie not added), else track + if (IsSuperceeded(eventEntity, infos, entities) == false) { - //Now check if this entity already exists in other event args that supersede this current event arg type - if (IsFiltered(obj, meta, allEntities) == false) - { - //if it's not filtered we can adde these args to the response - cancelableArgs.Add(args); - result.Add(eventDefinition); - //track the entity - allEntities.Add(Tuple.Create(obj, meta)); - } - } - else - { - //Can't retrieve the entity so cant' filter or inspect, just add to the output - result.Add(eventDefinition); + // track + entities.Add(Tuple.Create(eventEntity, infos)); + + // track result arguments + // include event definition in result + resultArgs.Add(args); + result.Add(def); } } else { + // enumerable of objects var toRemove = new List(); - foreach (var entity in list) + foreach (var eventObject in eventObjects) { - //extract the event object - var obj = entity as IEntity; - if (obj != null) - { - //Now check if this entity already exists in other event args that supersede this current event arg type - if (IsFiltered(obj, meta, allEntities)) - { - //track it to be removed - toRemove.Add(obj); - } - else - { - //track the entity, it's not filtered - allEntities.Add(Tuple.Create(obj, meta)); - } - } + // extract the event object, cast as an IEntity + // if cannot cast, cannot filter, nothing to do - just leave it in the list & continue + var eventEntity = eventObject as IEntity; + if (eventEntity == null) + continue; + + // look for this entity in superceding event args + // found = must be removed, else track + if (IsSuperceeded(eventEntity, infos, entities)) + toRemove.Add(eventEntity); else - { - //we don't need to do anything here, we can't cast to IEntity so we cannot filter, so it will just remain in the list - } + entities.Add(Tuple.Create(eventEntity, infos)); } - //remove anything that has been filtered + // remove superceded entities foreach (var entity in toRemove) - { - list.Remove(entity); - } + eventObjects.Remove(entity); - //track the event and include in the response if there's still entities remaining in the list - if (list.Count > 0) + // if there are still entities in the list, keep the event definition + if (eventObjects.Count > 0) { if (toRemove.Count > 0) { - //re-assign if the items have changed - args.EventObject = list; + // re-assign if changed + args.EventObject = eventObjects; } - cancelableArgs.Add(args); - result.Add(eventDefinition); + + // track result arguments + // include event definition in result + resultArgs.Add(args); + result.Add(def); } } } - else - { - //it's not a cancelable event arg so we just include it in the result - result.Add(eventDefinition); - } } - //Now we'll deal with ensuring that only the latest(non stale) entities are used throughout all event args - UpdateToLatestEntities(allEntities, cancelableArgs); + // go over all args in result, and update them with the latest instanceof each entity + UpdateToLatestEntities(entities, resultArgs); - //we need to reverse the result since we've been adding by latest added events first! + // reverse, since we processed the list in reverse result.Reverse(); return result; } - private static void UpdateToLatestEntities(IEnumerable> allEntities, IEnumerable cancelableArgs) + // edits event args to use the latest instance of each entity + private static void UpdateToLatestEntities(IEnumerable> entities, IEnumerable args) { - //Now we'll deal with ensuring that only the latest(non stale) entities are used throughout all event args - + // get the latest entities + // ordered hash set + keepOldest will keep the latest inserted entity (in case of duplicates) var latestEntities = new OrderedHashSet(keepOldest: true); - foreach (var entity in allEntities.OrderByDescending(entity => entity.Item1.UpdateDate)) - { + foreach (var entity in entities.OrderByDescending(entity => entity.Item1.UpdateDate)) latestEntities.Add(entity.Item1); - } - foreach (var args in cancelableArgs) + foreach (var arg in args) { - var list = TypeHelper.CreateGenericEnumerableFromObject(args.EventObject); - if (list == null) + // event object can either be a single object or an enumerable of objects + // try to get as an enumerable, get null if it's not + var eventObjects = TypeHelper.CreateGenericEnumerableFromObject(arg.EventObject); + if (eventObjects == null) { - //try to find the args entity in the latest entity - based on the equality operators, this will - //match by Id since that is the default equality checker for IEntity. If one is found, than it is - //the most recent entity instance so update the args with that instance so we don't emit a stale instance. - var foundEntity = latestEntities.FirstOrDefault(x => Equals(x, args.EventObject)); + // single object + // look for a more recent entity for that object, and replace if any + // works by "equalling" entities ie the more recent one "equals" this one (though different object) + var foundEntity = latestEntities.FirstOrDefault(x => Equals(x, arg.EventObject)); if (foundEntity != null) - { - args.EventObject = foundEntity; - } + arg.EventObject = foundEntity; } else { + // enumerable of objects + // same as above but for each object var updated = false; - - for (int i = 0; i < list.Count; i++) + for (var i = 0; i < eventObjects.Count; i++) { - //try to find the args entity in the latest entity - based on the equality operators, this will - //match by Id since that is the default equality checker for IEntity. If one is found, than it is - //the most recent entity instance so update the args with that instance so we don't emit a stale instance. - var foundEntity = latestEntities.FirstOrDefault(x => Equals(x, list[i])); - if (foundEntity != null) - { - list[i] = foundEntity; - updated = true; - } + var foundEntity = latestEntities.FirstOrDefault(x => Equals(x, eventObjects[i])); + if (foundEntity == null) continue; + eventObjects[i] = foundEntity; + updated = true; } if (updated) - { - args.EventObject = list; - } + arg.EventObject = eventObjects; } } } - /// - /// This will check against all of the processed entity/events (allEntities) to see if this entity already exists in - /// event args that supersede the event args being passed in and if so returns true. - /// - /// - /// - /// - /// - private static bool IsFiltered( - IEntity entity, - EventDefinitionTypeData eventDef, - List> allEntities) + // determines if a given entity, appearing in a given event definition, should be filtered out, + // considering the entities that have already been visited - an entity is filtered out if it + // appears in another even definition, which superceedes this event definition. + private static bool IsSuperceeded(IEntity entity, EventDefinitionInfos infos, List> entities) { - var argType = eventDef.EventDefinition.Args.GetType(); + //var argType = meta.EventArgsType; + var argType = infos.EventDefinition.Args.GetType(); - //check if the entity is found in any processed event data that could possible supersede this one - var foundByEntity = allEntities - .Where(x => x.Item2.SupersedeAttributes.Length > 0 - //if it's the same arg type than it cannot supersede - && x.Item2.EventArgType != argType - && Equals(x.Item1, entity)) + // look for other instances of the same entity, coming from an event args that supercedes other event args, + // ie is marked with the attribute, and is not this event args (cannot supersede itself) + var superceeding = entities + .Where(x => x.Item2.SupersedeTypes.Length > 0 // has the attribute + && x.Item2.EventDefinition.Args.GetType() != argType // is not the same + && Equals(x.Item1, entity)) // same entity .ToArray(); - //no args have been processed with this entity so it should not be filtered - if (foundByEntity.Length == 0) + // first time we see this entity = not filtered + if (superceeding.Length == 0) return false; + // fixme see notes above + // delete event args does NOT superceedes 'unpublished' event + if (argType.IsGenericType && argType.GetGenericTypeDefinition() == typeof(PublishEventArgs<>) && infos.EventDefinition.EventName == "UnPublished") + return false; + + // found occurences, need to determine if this event args is superceded if (argType.IsGenericType) { - var supercededBy = foundByEntity - .FirstOrDefault(x => - x.Item2.SupersedeAttributes.Any(y => - //if the attribute type is a generic type def then compare with the generic type def of the event arg - (y.SupersededEventArgsType.IsGenericTypeDefinition && y.SupersededEventArgsType == argType.GetGenericTypeDefinition()) - //if the attribute type is not a generic type def then compare with the normal type of the event arg - || (y.SupersededEventArgsType.IsGenericTypeDefinition == false && y.SupersededEventArgsType == argType))); + // generic, must compare type arguments + var supercededBy = superceeding.FirstOrDefault(x => + x.Item2.SupersedeTypes.Any(y => + // superceeding a generic type which has the same generic type definition + // fixme no matter the generic type parameters? could be different? + y.IsGenericTypeDefinition && y == argType.GetGenericTypeDefinition() + // or superceeding a non-generic type which is ... fixme how is this ever possible? argType *is* generic? + || y.IsGenericTypeDefinition == false && y == argType)); return supercededBy != null; } else { - var supercededBy = foundByEntity - .FirstOrDefault(x => - x.Item2.SupersedeAttributes.Any(y => - //since the event arg type is not a generic type, then we just compare type 1:1 - y.SupersededEventArgsType == argType)); + // non-generic, can compare types 1:1 + var supercededBy = superceeding.FirstOrDefault(x => + x.Item2.SupersedeTypes.Any(y => y == argType)); return supercededBy != null; } } diff --git a/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs b/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs index 825432756b..08e66afa1a 100644 --- a/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeEventDispatcherTests.cs @@ -3,10 +3,13 @@ using System.Collections.Generic; using System.Linq; using Moq; using NUnit.Framework; +using umbraco.cms.businesslogic; +using Umbraco.Core; using Umbraco.Core.Events; using Umbraco.Core.Models; using Umbraco.Core.Persistence; using Umbraco.Core.Scoping; +using Umbraco.Core.Services; using Umbraco.Tests.TestHelpers; using Umbraco.Tests.TestHelpers.Entities; @@ -99,7 +102,7 @@ namespace Umbraco.Tests.Scoping DoDeleteForContent += OnDoThingFail; DoForTestArgs += OnDoThingFail; DoForTestArgs2 += OnDoThingFail; - + var contentType = MockedContentTypes.CreateBasicContentType(); var content1 = MockedContent.CreateBasicContent(contentType); @@ -114,7 +117,7 @@ namespace Umbraco.Tests.Scoping var scopeProvider = new ScopeProvider(Mock.Of()); using (var scope = scopeProvider.CreateScope(eventDispatcher: new PassiveEventDispatcher())) { - + //content1 will be filtered from the args scope.Events.Dispatch(DoSaveForContent, this, new SaveEventArgs(new[]{ content1 , content3})); scope.Events.Dispatch(DoDeleteForContent, this, new DeleteEventArgs(content1)); @@ -141,15 +144,36 @@ namespace Umbraco.Tests.Scoping } } + [Test] + public void SupersededEvents2() + { + var contentService = Mock.Of(); + var content = Mock.Of(); + var l1 = new List + { + new EventDefinition>(Test_UnPublished, contentService, new PublishEventArgs(new [] { content }), "UnPublished"), + new EventDefinition>(Test_Deleted, contentService, new DeleteEventArgs(new [] { content }), "Deleted") + }; + + var l2 = new OrderedHashSet(keepOldest: false); + foreach (var e in l1) + l2.Add(e); + + var l3 = ScopeEventDispatcherBase.FilterSupersededAndUpdateToLatestEntity(l2); + + // see U4-10764 + Assert.AreEqual(2, l3.Count()); + } + /// /// This will test that when we track events that before we Get the events we normalize all of the - /// event entities to be the latest one (most current) found amongst the event so that there is + /// event entities to be the latest one (most current) found amongst the event so that there is /// no 'stale' entities in any of the args /// [Test] public void LatestEntities() { - DoSaveForContent += OnDoThingFail; + DoSaveForContent += OnDoThingFail; var now = DateTime.Now; var contentType = MockedContentTypes.CreateBasicContentType(); @@ -165,7 +189,7 @@ namespace Umbraco.Tests.Scoping var scopeProvider = new ScopeProvider(Mock.Of()); using (var scope = scopeProvider.CreateScope(eventDispatcher: new PassiveEventDispatcher())) - { + { scope.Events.Dispatch(DoSaveForContent, this, new SaveEventArgs(content1)); scope.Events.Dispatch(DoSaveForContent, this, new SaveEventArgs(content2)); scope.Events.Dispatch(DoSaveForContent, this, new SaveEventArgs(content3)); @@ -173,7 +197,7 @@ namespace Umbraco.Tests.Scoping // events have been queued var events = scope.Events.GetEvents(EventDefinitionFilter.All).ToArray(); Assert.AreEqual(3, events.Length); - + foreach (var t in events) { var args = (SaveEventArgs)t.Args; @@ -212,7 +236,7 @@ namespace Umbraco.Tests.Scoping // events have been queued var events = scope.Events.GetEvents(EventDefinitionFilter.FirstIn).ToArray(); - Assert.AreEqual(1, events.Length); + Assert.AreEqual(1, events.Length); Assert.AreEqual(content1, ((SaveEventArgs) events[0].Args).SavedEntities.First()); Assert.IsTrue(object.ReferenceEquals(content1, ((SaveEventArgs)events[0].Args).SavedEntities.First())); Assert.AreEqual(content1.UpdateDate, ((SaveEventArgs) events[0].Args).SavedEntities.First().UpdateDate); @@ -350,6 +374,9 @@ namespace Umbraco.Tests.Scoping public static event TypedEventHandler> DoThing3; + public static event TypedEventHandler> Test_UnPublished; + public static event TypedEventHandler> Test_Deleted; + public class TestEventArgs : CancellableObjectEventArgs { public TestEventArgs(object eventObject) : base(eventObject)