From d40b7087f437a51e06143ab28f6bb072d597c887 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 26 Mar 2020 19:56:21 +1100 Subject: [PATCH] Fixes issue with deep cloning Property main problem was that it was duplicating the _pvalue value when it shouldn't have. The real fix is that the Values property solves all these problems with it's setter but that wasn't being used by the DeepClone engine because that explicitly looks for certain collection types and IReadOnlyCollection wasn't one of them. With that included and PropertyValue marked as IDeepCloneable, it all just works. I've added tests. --- src/Umbraco.Core/Models/DeepCloneHelper.cs | 5 +- src/Umbraco.Core/Models/Property.cs | 26 +++----- src/Umbraco.Tests/Models/ContentTests.cs | 73 ++++++++++++---------- src/Umbraco.Tests/Models/PropertyTests.cs | 63 +++++++++++++++++++ src/Umbraco.Tests/Umbraco.Tests.csproj | 1 + 5 files changed, 116 insertions(+), 52 deletions(-) create mode 100644 src/Umbraco.Tests/Models/PropertyTests.cs diff --git a/src/Umbraco.Core/Models/DeepCloneHelper.cs b/src/Umbraco.Core/Models/DeepCloneHelper.cs index 6470de912b..453b455d4b 100644 --- a/src/Umbraco.Core/Models/DeepCloneHelper.cs +++ b/src/Umbraco.Core/Models/DeepCloneHelper.cs @@ -81,9 +81,10 @@ namespace Umbraco.Core.Models if (propertyInfo.PropertyType.IsGenericType && (propertyInfo.PropertyType.GetGenericTypeDefinition() == typeof(IEnumerable<>) || propertyInfo.PropertyType.GetGenericTypeDefinition() == typeof(ICollection<>) - || propertyInfo.PropertyType.GetGenericTypeDefinition() == typeof(IList<>))) + || propertyInfo.PropertyType.GetGenericTypeDefinition() == typeof(IList<>) + || propertyInfo.PropertyType.GetGenericTypeDefinition() == typeof(IReadOnlyCollection<>))) { - //if it is a IEnumerable<>, IList or ICollection<> we'll use a List<> + //if it is a IEnumerable<>, IReadOnlyCollection, IList or ICollection<> we'll use a List<> since it implements them all var genericType = typeof(List<>).MakeGenericType(propertyInfo.PropertyType.GetGenericArguments()); return new ClonePropertyInfo(propertyInfo) { GenericListType = genericType }; } diff --git a/src/Umbraco.Core/Models/Property.cs b/src/Umbraco.Core/Models/Property.cs index f51f70faf6..1d044aa20d 100644 --- a/src/Umbraco.Core/Models/Property.cs +++ b/src/Umbraco.Core/Models/Property.cs @@ -50,7 +50,7 @@ namespace Umbraco.Core.Models /// /// Represents a property value. /// - public class PropertyValue : IEquatable + public class PropertyValue : IDeepCloneable, IEquatable { // TODO: Either we allow change tracking at this class level, or we add some special change tracking collections to the Property // class to deal with change tracking which variants have changed @@ -96,6 +96,8 @@ namespace Umbraco.Core.Models public PropertyValue Clone() => new PropertyValue { _culture = _culture, _segment = _segment, PublishedValue = PublishedValue, EditedValue = EditedValue }; + public object DeepClone() => Clone(); + public override bool Equals(object obj) { return Equals(obj as PropertyValue); @@ -105,14 +107,18 @@ namespace Umbraco.Core.Models { return other != null && _culture == other._culture && - _segment == other._segment; + _segment == other._segment && + EqualityComparer.Default.Equals(EditedValue, other.EditedValue) && + EqualityComparer.Default.Equals(PublishedValue, other.PublishedValue); } public override int GetHashCode() { - var hashCode = -1254204277; + var hashCode = 1885328050; hashCode = hashCode * -1521134295 + EqualityComparer.Default.GetHashCode(_culture); hashCode = hashCode * -1521134295 + EqualityComparer.Default.GetHashCode(_segment); + hashCode = hashCode * -1521134295 + EqualityComparer.Default.GetHashCode(EditedValue); + hashCode = hashCode * -1521134295 + EqualityComparer.Default.GetHashCode(PublishedValue); return hashCode; } } @@ -358,20 +364,6 @@ namespace Umbraco.Core.Models var clonedEntity = (Property)clone; - //manually clone _values, _pvalue, _vvalues - clonedEntity._values = _values?.Select(x => x.Clone()).ToList(); // all values get copied - clonedEntity._pvalue = _pvalue?.Clone(); - // the tricky part here is that _values contains ALL values including the values in the _vvalues - // dictionary and they are by reference which is why we have equality overloads on PropertyValue - if (clonedEntity._vvalues != null) - { - clonedEntity._vvalues = new Dictionary(); - foreach (var item in _vvalues) - { - clonedEntity._vvalues[item.Key] = clonedEntity._values.First(x => x.Equals(item.Value)); - } - } - //need to manually assign since this is a readonly property clonedEntity.PropertyType = (PropertyType) PropertyType.DeepClone(); } diff --git a/src/Umbraco.Tests/Models/ContentTests.cs b/src/Umbraco.Tests/Models/ContentTests.cs index 3116087669..5bb30e1606 100644 --- a/src/Umbraco.Tests/Models/ContentTests.cs +++ b/src/Umbraco.Tests/Models/ContentTests.cs @@ -24,7 +24,6 @@ using Umbraco.Web.PropertyEditors; namespace Umbraco.Tests.Models { - [TestFixture] public class ContentTests : UmbracoTestBase { @@ -42,7 +41,7 @@ namespace Umbraco.Tests.Models // all this is required so we can validate properties... var editor = new TextboxPropertyEditor(Mock.Of()) { Alias = "test" }; - Composition.Register(_ => new DataEditorCollection(new [] { editor })); + Composition.Register(_ => new DataEditorCollection(new[] { editor })); Composition.Register(); var dataType = Mock.Of(); Mock.Get(dataType).Setup(x => x.Configuration).Returns(() => new object()); @@ -55,7 +54,7 @@ namespace Umbraco.Tests.Models var mediaTypeService = Mock.Of(); var memberTypeService = Mock.Of(); Composition.Register(_ => ServiceContext.CreatePartial(dataTypeService: dataTypeService, contentTypeBaseServiceProvider: new ContentTypeBaseServiceProvider(_contentTypeService, mediaTypeService, memberTypeService))); - + } [Test] @@ -458,7 +457,7 @@ namespace Umbraco.Tests.Models Assert.IsTrue(prop.WasPropertyDirty("Id")); } Assert.IsTrue(content.WasPropertyDirty("CultureInfos")); - foreach(var culture in content.CultureInfos) + foreach (var culture in content.CultureInfos) { Assert.IsTrue(culture.WasDirty()); Assert.IsTrue(culture.WasPropertyDirty("Name")); @@ -539,7 +538,7 @@ namespace Umbraco.Tests.Models var content = MockedContent.CreateTextpageContent(contentType, "Textpage", -1); // Act - content.PropertyValues(new { title = "This is the new title"}); + content.PropertyValues(new { title = "This is the new title" }); // Assert Assert.That(content.Properties.Any(), Is.True); @@ -603,13 +602,13 @@ namespace Umbraco.Tests.Models // Act contentType.PropertyGroups["Content"].PropertyTypes.Add(new PropertyType("test", ValueStorageType.Ntext, "subtitle") - { - Name = "Subtitle", - Description = "Optional subtitle", - Mandatory = false, - SortOrder = 3, - DataTypeId = -88 - }); + { + Name = "Subtitle", + Description = "Optional subtitle", + Mandatory = false, + SortOrder = 3, + DataTypeId = -88 + }); // Assert Assert.That(contentType.PropertyGroups["Content"].PropertyTypes.Count, Is.EqualTo(3)); @@ -626,9 +625,13 @@ namespace Umbraco.Tests.Models // Act var propertyType = new PropertyType("test", ValueStorageType.Ntext, "subtitle") - { - Name = "Subtitle", Description = "Optional subtitle", Mandatory = false, SortOrder = 3, DataTypeId = -88 - }; + { + Name = "Subtitle", + Description = "Optional subtitle", + Mandatory = false, + SortOrder = 3, + DataTypeId = -88 + }; contentType.PropertyGroups["Content"].PropertyTypes.Add(propertyType); var newProperty = new Property(propertyType); newProperty.SetValue("This is a subtitle Test"); @@ -650,14 +653,14 @@ namespace Umbraco.Tests.Models // Act var propertyType = new PropertyType("test", ValueStorageType.Ntext, "subtitle") - { - Name = "Subtitle", - Description = "Optional subtitle", - Mandatory = false, - SortOrder = 3, - DataTypeId = -88 - }; - var propertyGroup = new PropertyGroup(true) { Name = "Test Group", SortOrder = 3}; + { + Name = "Subtitle", + Description = "Optional subtitle", + Mandatory = false, + SortOrder = 3, + DataTypeId = -88 + }; + var propertyGroup = new PropertyGroup(true) { Name = "Test Group", SortOrder = 3 }; propertyGroup.PropertyTypes.Add(propertyType); contentType.PropertyGroups.Add(propertyGroup); var newProperty = new Property(propertyType); @@ -681,9 +684,13 @@ namespace Umbraco.Tests.Models // Act - note that the PropertyType's properties like SortOrder is not updated through the Content object var propertyType = new PropertyType("test", ValueStorageType.Ntext, "title") - { - Name = "Title", Description = "Title description added", Mandatory = false, SortOrder = 10, DataTypeId = -88 - }; + { + Name = "Title", + Description = "Title description added", + Mandatory = false, + SortOrder = 10, + DataTypeId = -88 + }; content.Properties.Add(new Property(propertyType)); // Assert @@ -911,13 +918,13 @@ namespace Umbraco.Tests.Models // Act var propertyType = new PropertyType("test", ValueStorageType.Ntext, "subtitle") - { - Name = "Subtitle", - Description = "Optional subtitle", - Mandatory = false, - SortOrder = 3, - DataTypeId = -88 - }; + { + Name = "Subtitle", + Description = "Optional subtitle", + Mandatory = false, + SortOrder = 3, + DataTypeId = -88 + }; contentType.PropertyGroups["Content"].PropertyTypes.Add(propertyType); // Assert diff --git a/src/Umbraco.Tests/Models/PropertyTests.cs b/src/Umbraco.Tests/Models/PropertyTests.cs new file mode 100644 index 0000000000..a95afcdd5c --- /dev/null +++ b/src/Umbraco.Tests/Models/PropertyTests.cs @@ -0,0 +1,63 @@ +using System; +using System.Linq; +using NUnit.Framework; +using Umbraco.Core.Models; +using Umbraco.Tests.Testing; + +namespace Umbraco.Tests.Models +{ + [TestFixture] + public class PropertyTests : UmbracoTestBase + { + [Test] + public void Can_Deep_Clone() + { + // needs to be within collection to support publishing + var ptCollection = new PropertyTypeCollection(true, new[] {new PropertyType("TestPropertyEditor", ValueStorageType.Nvarchar, "test") + { + Id = 3, + CreateDate = DateTime.Now, + DataTypeId = 5, + PropertyEditorAlias = "propTest", + Description = "testing", + Key = Guid.NewGuid(), + Mandatory = true, + Name = "Test", + PropertyGroupId = new Lazy(() => 11), + SortOrder = 9, + UpdateDate = DateTime.Now, + ValidationRegExp = "xxxx", + ValueStorageType = ValueStorageType.Nvarchar + }}); + + var property = new Property(123, ptCollection[0]) + { + CreateDate = DateTime.Now, + Id = 4, + Key = Guid.NewGuid(), + UpdateDate = DateTime.Now + }; + + property.SetValue("hello"); + property.PublishValues(); + + var clone = (Property)property.DeepClone(); + + Assert.AreNotSame(clone, property); + Assert.AreNotSame(clone.Values, property.Values); + Assert.AreNotSame(property.PropertyType, clone.PropertyType); + for (int i = 0; i < property.Values.Count; i++) + { + Assert.AreNotSame(property.Values.ElementAt(i), clone.Values.ElementAt(i)); + } + + + //This double verifies by reflection + var allProps = clone.GetType().GetProperties(); + foreach (var propertyInfo in allProps) + { + Assert.AreEqual(propertyInfo.GetValue(clone, null), propertyInfo.GetValue(property, null)); + } + } + } +} diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index 90f9303423..3c359cdde8 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -142,6 +142,7 @@ +