Simplifies the interface inheritance removes SimpleContentTypeMapper (can't be used), improves how mappers are looked up, simplifies other models

src/Umbraco.Abstractions/Collections/DeepCloneableList.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.ComponentModel;
 using System.Linq;
 using Umbraco.Core.Models;
 using Umbraco.Core.Models.Entities;
@@ -80,6 +81,7 @@ namespace Umbraco.Core.Collections
             }
         }
 
+        #region IRememberBeingDirty
         public bool IsDirty()
         {
             return this.OfType<IRememberBeingDirty>().Any(x => x.IsDirty());
@@ -150,5 +152,8 @@ namespace Umbraco.Core.Collections
         {
             return Enumerable.Empty<string>();
         }
+
+        public event PropertyChangedEventHandler PropertyChanged; // noop 
+        #endregion
     }
 }

src/Umbraco.Abstractions/Models/Entities/EntitySlim.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.ComponentModel;
 using System.Linq;
 using System.Runtime.Serialization;
 using Umbraco.Core.Exceptions;
@@ -23,7 +24,7 @@ namespace Umbraco.Core.Models.Entities
         /// Gets an entity representing "root".
         /// </summary>
         public static readonly IEntitySlim Root = new EntitySlim { Path = "-1", Name = "root", HasChildren = true };
-        
+
         // implement IEntity
 
         /// <inheritdoc />
@@ -121,72 +122,10 @@ namespace Umbraco.Core.Models.Entities
 
         #endregion
 
-        #region IRememberBeingDirty
-
-        // IEntitySlim does *not* track changes, but since it indirectly implements IUmbracoEntity,
-        // and therefore IRememberBeingDirty, we have to have those methods - which all throw.
-
-        public bool IsDirty()
-        {
-            throw new WontImplementException();
-        }
-
-        public bool IsPropertyDirty(string propName)
-        {
-            throw new WontImplementException();
-        }
-
-        public IEnumerable<string> GetDirtyProperties()
-        {
-            throw new WontImplementException();
-        }
-
-        public void ResetDirtyProperties()
-        {
-            throw new WontImplementException();
-        }
-
-        public void DisableChangeTracking()
-        {
-            // noop
-        }
-
-        public void EnableChangeTracking()
-        {
-            // noop
-        }
-
-        public bool WasDirty()
-        {
-            throw new WontImplementException();
-        }
-
-        public bool WasPropertyDirty(string propertyName)
-        {
-            throw new WontImplementException();
-        }
-
-        public void ResetWereDirtyProperties()
-        {
-            throw new WontImplementException();
-        }
-
-        public void ResetDirtyProperties(bool rememberDirty)
-        {
-            throw new WontImplementException();
-        }
-
-        public IEnumerable<string> GetWereDirtyProperties()
-        {
-            throw new WontImplementException();
-        }
-
         public void ResetIdentity()
         {
             Id = default;
             Key = Guid.Empty;
         }
-
-        #endregion
     }
 }

src/Umbraco.Abstractions/Models/Entities/ICanBeDirty.cs

@@ -1,4 +1,5 @@
 using System.Collections.Generic;
+using System.ComponentModel;
 
 namespace Umbraco.Core.Models.Entities
 {
@@ -36,5 +37,7 @@ namespace Umbraco.Core.Models.Entities
         /// Enables change tracking.
         /// </summary>
         void EnableChangeTracking();
+
+        event PropertyChangedEventHandler PropertyChanged;
     }
 }

src/Umbraco.Abstractions/Models/Entities/IUmbracoEntity.cs

@@ -2,6 +2,7 @@
 
 namespace Umbraco.Core.Models.Entities
 {
+
     /// <summary>
     /// Represents an entity that can be managed by the entity service.
     /// </summary>
@@ -10,6 +11,6 @@ namespace Umbraco.Core.Models.Entities
     /// <para>IUmbracoEntities can be retrieved with the IEntityService.</para>
     /// <para>An IUmbracoEntity can participate in notifications.</para>
     /// </remarks>
-    public interface IUmbracoEntity : ITreeEntity, IRememberBeingDirty
+    public interface IUmbracoEntity : ITreeEntity
     { }
 }

src/Umbraco.Abstractions/Models/IContentBase.cs

@@ -11,7 +11,7 @@ namespace Umbraco.Core.Models
     /// <para>Content items are documents, medias and members.</para>
     /// <para>Content items have a content type, and properties.</para>
     /// </remarks>
-    public interface IContentBase : IUmbracoEntity
+    public interface IContentBase : IUmbracoEntity, IRememberBeingDirty
     {
         /// <summary>
         /// Integer Id of the default ContentType

src/Umbraco.Abstractions/Models/IContentTypeBase.cs

@@ -8,7 +8,7 @@ namespace Umbraco.Core.Models
     /// Defines the base for a ContentType with properties that
     /// are shared between ContentTypes and MediaTypes.
     /// </summary>
-    public interface IContentTypeBase : IUmbracoEntity
+    public interface IContentTypeBase : IUmbracoEntity, IRememberBeingDirty
     {
         /// <summary>
         /// Gets or Sets the Alias of the ContentType

src/Umbraco.Abstractions/Models/IDataType.cs

@@ -6,7 +6,7 @@ namespace Umbraco.Core.Models
     /// <summary>
     /// Represents a data type.
     /// </summary>
-    public interface IDataType : IUmbracoEntity
+    public interface IDataType : IUmbracoEntity, IRememberBeingDirty
     {
         /// <summary>
         /// Gets or sets the property editor.

src/Umbraco.Abstractions/Models/IDictionaryTranslation.cs

@@ -3,7 +3,7 @@ using Umbraco.Core.Models.Entities;
 
 namespace Umbraco.Core.Models
 {
-    public interface IDictionaryTranslation : IEntity
+    public interface IDictionaryTranslation : IEntity, IRememberBeingDirty
     {
         /// <summary>
         /// Gets or sets the <see cref="Language"/> for the translation

src/Umbraco.Abstractions/Models/IFile.cs

@@ -7,7 +7,7 @@ namespace Umbraco.Core.Models
     /// Defines a File
     /// </summary>
     /// <remarks>Used for Scripts, Stylesheets and Templates</remarks>
-    public interface IFile : IEntity
+    public interface IFile : IEntity, IRememberBeingDirty
     {
         /// <summary>
         /// Gets the Name of the File including extension

src/Umbraco.Abstractions/Models/IPropertyType.cs

@@ -84,7 +84,5 @@ namespace Umbraco.Core.Models
         /// <para>Throws if the value cannot be converted.</para>
         /// </remarks>
         object ConvertAssignedValue(object value);
-
-        event PropertyChangedEventHandler PropertyChanged;        
     }
 }

src/Umbraco.Abstractions/Models/ISimpleContentType.cs

@@ -1,14 +1,15 @@
-using Umbraco.Core.Models.Entities;
+using System;
 
 namespace Umbraco.Core.Models
 {
     /// <summary>
     /// Represents a simplified view of a content type.
     /// </summary>
-    public interface ISimpleContentType : IUmbracoEntity
+    public interface ISimpleContentType
     {
-        new int Id { get; }
-        new string Name { get; }
+        int Id { get; }
+        Guid Key { get; }
+        string Name { get; }
 
         /// <summary>
         /// Gets the alias of the content type.

src/Umbraco.Abstractions/Models/IStylesheet.cs

@@ -3,7 +3,7 @@ using Umbraco.Core.Models.Entities;
 
 namespace Umbraco.Core.Models
 {
-    public interface IStylesheet : IFile, IRememberBeingDirty
+    public interface IStylesheet : IFile
     {
         /// <summary>
         /// Returns a list of umbraco back office enabled stylesheet properties

src/Umbraco.Abstractions/Models/MacroProperty.cs

@@ -9,7 +9,7 @@ namespace Umbraco.Core.Models
     /// </summary>
     [Serializable]
     [DataContract(IsReference = true)]
-    public class MacroProperty : BeingDirtyBase, IMacroProperty, IRememberBeingDirty, IDeepCloneable
+    public class MacroProperty : BeingDirtyBase, IMacroProperty
     {
         public MacroProperty()
         {

src/Umbraco.Core/Models/ContentTypeBaseExtensions.cs

@@ -44,16 +44,13 @@ namespace Umbraco.Core.Models
             // property variation change?
             var hasAnyPropertyVariationChanged = contentType.PropertyTypes.Any(propertyType =>
             {
-                if (!(propertyType is IRememberBeingDirty dirtyProperty))
-                    throw new Exception("oops");
-
                 // skip new properties
                 // TODO: This used to be WasPropertyDirty("HasIdentity") but i don't think that actually worked for detecting new entities this does seem to work properly
-                var isNewProperty = dirtyProperty.WasPropertyDirty("Id");
+                var isNewProperty = propertyType.WasPropertyDirty("Id");
                 if (isNewProperty) return false;
 
                 // variation change?
-                var dirty = dirtyProperty.WasPropertyDirty("Variations");
+                var dirty = propertyType.WasPropertyDirty("Variations");
                 if (dirty)
                     a.Add(propertyType.Alias);
 

src/Umbraco.Core/Models/Identity/BackOfficeIdentityUser.cs

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
 using System.Collections.Specialized;
+using System.ComponentModel;
 using System.Linq;
 using Umbraco.Core.Composing;
 using Umbraco.Core.Models.Entities;
@@ -403,6 +404,18 @@ namespace Umbraco.Core.Models.Identity
             _beingDirty.EnableChangeTracking();
         }
 
+        public event PropertyChangedEventHandler PropertyChanged
+        {
+            add
+            {
+                _beingDirty.PropertyChanged += value;
+            }
+            remove
+            {
+                _beingDirty.PropertyChanged -= value;
+            }
+        }
+
         #endregion
 
         //Custom comparer for enumerables
@@ -413,5 +426,6 @@ namespace Umbraco.Core.Models.Identity
         private static readonly DelegateEqualityComparer<int[]> StartIdsComparer = new DelegateEqualityComparer<int[]>(
             (groups, enumerable) => groups.UnsortedSequenceEqual(enumerable),
             groups => groups.GetHashCode());
+        
     }
 }

src/Umbraco.Core/Models/SimpleContentType.cs

@@ -1,6 +1,4 @@
 using System;
-using System.Collections.Generic;
-using Umbraco.Core.Models.Entities;
 
 namespace Umbraco.Core.Models
 {
@@ -37,6 +35,7 @@ namespace Umbraco.Core.Models
             if (contentType == null) throw new ArgumentNullException(nameof(contentType));
 
             Id = contentType.Id;
+            Key = contentType.Key;
             Alias = contentType.Alias;
             Variations = contentType.Variations;
             Icon = contentType.Icon;
@@ -46,32 +45,26 @@ namespace Umbraco.Core.Models
             IsElement = contentType.IsElement;
         }
 
-        /// <inheritdoc />
         public string Alias { get; }
 
         public int Id { get; }
 
-        /// <inheritdoc />
+        public Guid Key { get; }
+
         public ITemplate DefaultTemplate { get;  }
 
-        /// <inheritdoc />
         public ContentVariation Variations { get; }
 
-        /// <inheritdoc />
         public string Icon { get; }
 
-        /// <inheritdoc />
         public bool IsContainer { get; }
         
         public string Name { get; }
 
-        /// <inheritdoc />
         public bool AllowedAsRoot { get; }
 
-        /// <inheritdoc />
         public bool IsElement { get; }
 
-        /// <inheritdoc />
         public bool SupportsPropertyVariation(string culture, string segment, bool wildcards = false)
         {
             // non-exact validation: can accept a 'null' culture if the property type varies
@@ -85,7 +78,6 @@ namespace Umbraco.Core.Models
             return string.Equals(Alias, other.Alias) && Id == other.Id;
         }
 
-        /// <inheritdoc />
         public override bool Equals(object obj)
         {
             if (ReferenceEquals(null, obj)) return false;
@@ -94,7 +86,6 @@ namespace Umbraco.Core.Models
             return Equals((SimpleContentType) obj);
         }
 
-        /// <inheritdoc />
         public override int GetHashCode()
         {
             unchecked
@@ -102,37 +93,5 @@ namespace Umbraco.Core.Models
                 return ((Alias != null ? Alias.GetHashCode() : 0) * 397) ^ Id;
             }
         }
-
-        // we have to have all this, because we're an IUmbracoEntity, because that is
-        // required by the query expression visitor / SimpleContentTypeMapper
-        // TODO: Make the query expression visitor use a different common interface, or investigate removing IRememberBeingDirty from being a requirement on that interface and expliclty checking for that throughout the code?
-
-        string ITreeEntity.Name { get => this.Name; set => throw new NotImplementedException(); }
-        int IEntity.Id { get => this.Id; set => throw new NotImplementedException(); }
-        bool IEntity.HasIdentity => this.Id != default;
-        void IEntity.ResetIdentity() => throw new NotImplementedException();
-        int ITreeEntity.CreatorId { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
-        int ITreeEntity.ParentId { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
-        int ITreeEntity.Level { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
-        string ITreeEntity.Path { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
-        int ITreeEntity.SortOrder { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
-        bool ITreeEntity.Trashed => throw new NotImplementedException();
-        Guid IEntity.Key { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
-        DateTime IEntity.CreateDate { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
-        DateTime IEntity.UpdateDate { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
-        DateTime? IEntity.DeleteDate { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
-        void ITreeEntity.SetParent(ITreeEntity parent) => throw new NotImplementedException();
-        object IDeepCloneable.DeepClone() => throw new NotImplementedException();
-        bool IRememberBeingDirty.WasDirty() => throw new NotImplementedException();
-        bool IRememberBeingDirty.WasPropertyDirty(string propertyName) => throw new NotImplementedException();
-        void IRememberBeingDirty.ResetWereDirtyProperties() => throw new NotImplementedException();
-        void IRememberBeingDirty.ResetDirtyProperties(bool rememberDirty) => throw new NotImplementedException();
-        IEnumerable<string> IRememberBeingDirty.GetWereDirtyProperties() => throw new NotImplementedException();
-        bool ICanBeDirty.IsDirty() => throw new NotImplementedException();
-        bool ICanBeDirty.IsPropertyDirty(string propName) => throw new NotImplementedException();
-        IEnumerable<string> ICanBeDirty.GetDirtyProperties() => throw new NotImplementedException();
-        void ICanBeDirty.ResetDirtyProperties() => throw new NotImplementedException();
-        public void DisableChangeTracking() => throw new NotImplementedException();
-        public void EnableChangeTracking() => throw new NotImplementedException();
     }
 }

src/Umbraco.Core/Persistence/Mappers/IMapperCollection.cs

@@ -5,6 +5,7 @@ namespace Umbraco.Core.Persistence.Mappers
 {
     public interface IMapperCollection : IBuilderCollection<BaseMapper>
     {
+        bool TryGetMapper(Type type, out BaseMapper mapper);
         BaseMapper this[Type type] { get; }
     }
 }

src/Umbraco.Core/Persistence/Mappers/MapperCollection.cs

@@ -10,27 +10,40 @@ namespace Umbraco.Core.Persistence.Mappers
     {
         public MapperCollection(IEnumerable<BaseMapper> items)
             : base(items)
-        { }
+        {
 
-        // maintain our own index for faster lookup
-        private readonly ConcurrentDictionary<Type, BaseMapper> _index = new ConcurrentDictionary<Type, BaseMapper>();
+            _index = new Lazy<ConcurrentDictionary<Type, BaseMapper>>(() =>
+            {
+                var d = new ConcurrentDictionary<Type, BaseMapper>();
+                foreach(var mapper in this)
+                {
+                    var attributes = mapper.GetType().GetCustomAttributes<MapperForAttribute>(false);
+                    foreach(var a in attributes)
+                    {
+                        d.TryAdd(a.EntityType, mapper);
+                    }
+                }
+                return d;
+            });
+        }
 
+        private readonly Lazy<ConcurrentDictionary<Type, BaseMapper>> _index;
+
+        /// <summary>
+        /// Returns a mapper for this type, throw an exception if not found
+        /// </summary>
+        /// <param name="type"></param>
+        /// <returns></returns>
         public BaseMapper this[Type type]
         {
             get
             {
-                return _index.GetOrAdd(type, t =>
-                {
-                    // check if any of the mappers are assigned to this type
-                    var mapper = this.FirstOrDefault(x => x.GetType()
-                        .GetCustomAttributes<MapperForAttribute>(false)
-                        .Any(m => m.EntityType == type));
-
-                    if (mapper != null) return mapper;
-
-                    throw new Exception($"Could not find a mapper matching type {type.FullName}.");
-                });
+                if (_index.Value.TryGetValue(type, out var mapper))
+                    return mapper;
+                throw new Exception($"Could not find a mapper matching type {type.FullName}.");
             }
         }
+
+        public bool TryGetMapper(Type type, out BaseMapper mapper) => _index.Value.TryGetValue(type, out mapper);
     }
 }

src/Umbraco.Core/Persistence/Mappers/SimpleContentTypeMapper.cs

@@ -1,37 +0,0 @@
-using System;
-using System.Collections.Concurrent;
-using Umbraco.Core.Models;
-using Umbraco.Core.Persistence.Dtos;
-
-namespace Umbraco.Core.Persistence.Mappers
-{
-    [MapperFor(typeof(ISimpleContentType))]
-    [MapperFor(typeof(SimpleContentType))]
-    public sealed class SimpleContentTypeMapper : BaseMapper
-    {
-        public SimpleContentTypeMapper(Lazy<ISqlContext> sqlContext, ConcurrentDictionary<Type, ConcurrentDictionary<string, string>> maps)
-            : base(sqlContext, maps)
-        { }
-
-        protected override void DefineMaps()
-        {
-            DefineMap<ContentType, NodeDto>(nameof(ContentType.Id), nameof(NodeDto.NodeId));
-            DefineMap<ContentType, NodeDto>(nameof(ContentType.CreateDate), nameof(NodeDto.CreateDate));
-            DefineMap<ContentType, NodeDto>(nameof(ContentType.Level), nameof(NodeDto.Level));
-            DefineMap<ContentType, NodeDto>(nameof(ContentType.ParentId), nameof(NodeDto.ParentId));
-            DefineMap<ContentType, NodeDto>(nameof(ContentType.Path), nameof(NodeDto.Path));
-            DefineMap<ContentType, NodeDto>(nameof(ContentType.SortOrder), nameof(NodeDto.SortOrder));
-            DefineMap<ContentType, NodeDto>(nameof(ContentType.Name), nameof(NodeDto.Text));
-            DefineMap<ContentType, NodeDto>(nameof(ContentType.Trashed), nameof(NodeDto.Trashed));
-            DefineMap<ContentType, NodeDto>(nameof(ContentType.Key), nameof(NodeDto.UniqueId));
-            DefineMap<ContentType, NodeDto>(nameof(ContentType.CreatorId), nameof(NodeDto.UserId));
-            DefineMap<ContentType, ContentTypeDto>(nameof(ContentType.Alias), nameof(ContentTypeDto.Alias));
-            DefineMap<ContentType, ContentTypeDto>(nameof(ContentType.AllowedAsRoot), nameof(ContentTypeDto.AllowAtRoot));
-            DefineMap<ContentType, ContentTypeDto>(nameof(ContentType.Description), nameof(ContentTypeDto.Description));
-            DefineMap<ContentType, ContentTypeDto>(nameof(ContentType.Icon), nameof(ContentTypeDto.Icon));
-            DefineMap<ContentType, ContentTypeDto>(nameof(ContentType.IsContainer), nameof(ContentTypeDto.IsContainer));
-            DefineMap<ContentType, ContentTypeDto>(nameof(ContentType.IsElement), nameof(ContentTypeDto.IsElement));
-            DefineMap<ContentType, ContentTypeDto>(nameof(ContentType.Thumbnail), nameof(ContentTypeDto.Thumbnail));
-        }
-    }
-}

src/Umbraco.Core/Persistence/Querying/ModelToSqlExpressionVisitor.cs

@@ -60,8 +60,8 @@ namespace Umbraco.Core.Persistence.Querying
 
             if (m.Expression != null
                 && m.Expression.Type != typeof(T)
-                && TypeHelper.IsTypeAssignableFrom<IUmbracoEntity>(m.Expression.Type) //TODO: Could this just be `IEntity` ? why does it need to be IUmbracoEntity, we aren't even using the reference to that below
-                && EndsWithConstant(m) == false)
+                && EndsWithConstant(m) == false
+                && _mappers.TryGetMapper(m.Expression.Type, out var subMapper))
             {
                 //if this is the case, it means we have a sub expression / nested property access, such as: x.ContentType.Alias == "Test";
                 //and since the sub type (x.ContentType) is not the same as x, we need to resolve a mapper for x.ContentType to get it's mapped SQL column
@@ -69,7 +69,6 @@ namespace Umbraco.Core.Persistence.Querying
                 //don't execute if compiled
                 if (Visited == false)
                 {
-                    var subMapper = _mappers[m.Expression.Type]; // throws if not found
                     var field = subMapper.Map(m.Member.Name);
                     if (field.IsNullOrWhiteSpace())
                         throw new InvalidOperationException($"The mapper returned an empty field for the member name: {m.Member.Name} for type: {m.Expression.Type}");

src/Umbraco.Core/Services/Implement/ContentTypeServiceBaseOfTRepositoryTItemTService.cs

@@ -148,16 +148,13 @@ namespace Umbraco.Core.Services.Implement
                 // existing property alias change?
                 var hasAnyPropertyChangedAlias = contentType.PropertyTypes.Any(propertyType =>
                 {
-                    if (!(propertyType is IRememberBeingDirty dirtyProperty))
-                        throw new Exception("oops");
-
                     // skip new properties
                     // TODO: This used to be WasPropertyDirty("HasIdentity") but i don't think that actually worked for detecting new entities this does seem to work properly
-                    var isNewProperty = dirtyProperty.WasPropertyDirty("Id");
+                    var isNewProperty = propertyType.WasPropertyDirty("Id");
                     if (isNewProperty) return false;
 
                     // alias change?
-                    return dirtyProperty.WasPropertyDirty("Alias");
+                    return propertyType.WasPropertyDirty("Alias");
                 });
 
                 // removed properties?

src/Umbraco.Core/Umbraco.Core.csproj

@@ -343,7 +343,6 @@
     <Compile Include="Persistence\Mappers\AuditEntryMapper.cs" />
     <Compile Include="Persistence\Mappers\AuditItemMapper.cs" />
     <Compile Include="Persistence\Mappers\ConsentMapper.cs" />
-    <Compile Include="Persistence\Mappers\SimpleContentTypeMapper.cs" />
     <Compile Include="Persistence\Repositories\Implement\AuditEntryRepository.cs" />
     <Compile Include="Persistence\Repositories\Implement\ConsentRepository.cs" />
     <Compile Include="Persistence\SqlContextExtensions.cs" />