From 3b12e0b72a040967d22a268478ca018d754b13ae Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 7 Feb 2019 13:28:23 +1100 Subject: [PATCH] Updates ContentBase to track the original PropertyType's which means we don't ever need to look them up again, adds notes about this too since it seems it is probably unnecessary to begin with. --- src/Umbraco.Core/Models/ContentBase.cs | 34 ++++++++++++------- src/Umbraco.Tests/Models/ContentTests.cs | 6 ++-- .../Services/ContentServiceTests.cs | 4 +-- .../Testing/ContentBaseExtensions.cs | 21 +++++------- 4 files changed, 35 insertions(+), 30 deletions(-) diff --git a/src/Umbraco.Core/Models/ContentBase.cs b/src/Umbraco.Core/Models/ContentBase.cs index c6f5f39e23..03eb5a4c9e 100644 --- a/src/Umbraco.Core/Models/ContentBase.cs +++ b/src/Umbraco.Core/Models/ContentBase.cs @@ -23,6 +23,7 @@ namespace Umbraco.Core.Models private int _writerId; private PropertyCollection _properties; private ContentCultureInfosCollection _cultureInfos; + internal IReadOnlyList AllPropertyTypes { get; } #region Used for change tracking @@ -64,6 +65,11 @@ namespace Umbraco.Core.Models _contentTypeId = contentType.Id; _properties = properties ?? throw new ArgumentNullException(nameof(properties)); _properties.EnsurePropertyTypes(contentType.CompositionPropertyTypes); + + //track all property types on this content type, these can never change during the lifetime of this single instance + //there is no real extra memory overhead of doing this since these property types are already cached on this object via the + //properties already. + AllPropertyTypes = new List(contentType.CompositionPropertyTypes); } [IgnoreDataMember] @@ -294,22 +300,26 @@ namespace Umbraco.Core.Models /// public void SetValue(string propertyTypeAlias, object value, string culture = null, string segment = null) { - if (Properties.Contains(propertyTypeAlias)) + if (Properties.TryGetValue(propertyTypeAlias, out var property)) { - Properties[propertyTypeAlias].SetValue(value, culture, segment); - //bump the culture to be flagged for updating - this.TouchCulture(culture); - return; + property.SetValue(value, culture, segment); } + else + { + //fixme: Can this ever happen? According to the ctor in ContentBase (EnsurePropertyTypes), all properties will be created based on the content type's property types + // so how can a property not be resolved by the alias on the content.Properties but it can on the content type? + // This maybe can happen if a developer has removed a property with the api and is trying to then set the value of that property again... + // BUT, as it turns out the content.Properties.Remove(...) method is NEVER used, because why and how could it? you never remove a property from + // a content item directly. - var propertyTypes = Current.Services.ContentTypeBaseServices.GetContentTypeOf(this).CompositionPropertyTypes; - var propertyType = propertyTypes.FirstOrDefault(x => x.Alias.InvariantEquals(propertyTypeAlias)); - if (propertyType == null) - throw new InvalidOperationException($"No PropertyType exists with the supplied alias \"{propertyTypeAlias}\"."); + var propertyType = AllPropertyTypes.FirstOrDefault(x => x.Alias.InvariantEquals(propertyTypeAlias)); + if (propertyType == null) + throw new InvalidOperationException($"No PropertyType exists with the supplied alias \"{propertyTypeAlias}\"."); - var property = propertyType.CreateProperty(); - property.SetValue(value, culture, segment); - Properties.Add(property); + property = propertyType.CreateProperty(); + property.SetValue(value, culture, segment); + Properties.Add(property); + } //bump the culture to be flagged for updating this.TouchCulture(culture); diff --git a/src/Umbraco.Tests/Models/ContentTests.cs b/src/Umbraco.Tests/Models/ContentTests.cs index 7d19c34167..f211e570e9 100644 --- a/src/Umbraco.Tests/Models/ContentTests.cs +++ b/src/Umbraco.Tests/Models/ContentTests.cs @@ -30,7 +30,6 @@ namespace Umbraco.Tests.Models public class ContentTests : UmbracoTestBase { private IContentTypeService _contentTypeService; - private IContentTypeBaseServiceProvider _contentTypeServiceProvider; protected override void Compose() { @@ -57,8 +56,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))); - - _contentTypeServiceProvider = new ContentTypeBaseServiceProvider(_contentTypeService, Mock.Of(), Mock.Of()); + } [Test] @@ -544,7 +542,7 @@ namespace Umbraco.Tests.Models var content = MockedContent.CreateTextpageContent(contentType, "Textpage", -1); // Act - content.PropertyValues(_contentTypeServiceProvider, new { title = "This is the new title"}); + content.PropertyValues(new { title = "This is the new title"}); // Assert Assert.That(content.Properties.Any(), Is.True); diff --git a/src/Umbraco.Tests/Services/ContentServiceTests.cs b/src/Umbraco.Tests/Services/ContentServiceTests.cs index b5069e040b..78f0d739fb 100644 --- a/src/Umbraco.Tests/Services/ContentServiceTests.cs +++ b/src/Umbraco.Tests/Services/ContentServiceTests.cs @@ -1581,12 +1581,12 @@ namespace Umbraco.Tests.Services tags = "[\"Hello\",\"World\"]" }; var content1 = MockedContent.CreateBasicContent(contentType); - content1.PropertyValues(ServiceContext.ContentTypeBaseServices, obj); + content1.PropertyValues(obj); content1.ResetDirtyProperties(false); ServiceContext.ContentService.Save(content1, Constants.Security.SuperUserId); Assert.IsTrue(ServiceContext.ContentService.SaveAndPublish(content1, userId: 0).Success); var content2 = MockedContent.CreateBasicContent(contentType); - content2.PropertyValues(ServiceContext.ContentTypeBaseServices, obj); + content2.PropertyValues(obj); content2.ResetDirtyProperties(false); ServiceContext.ContentService.Save(content2, Constants.Security.SuperUserId); Assert.IsTrue(ServiceContext.ContentService.SaveAndPublish(content2, userId: 0).Success); diff --git a/src/Umbraco.Tests/Testing/ContentBaseExtensions.cs b/src/Umbraco.Tests/Testing/ContentBaseExtensions.cs index 883c7682f4..6d3d2c7683 100644 --- a/src/Umbraco.Tests/Testing/ContentBaseExtensions.cs +++ b/src/Umbraco.Tests/Testing/ContentBaseExtensions.cs @@ -9,16 +9,12 @@ namespace Umbraco.Tests.Testing { public static class ContentBaseExtensions { - public static void PropertyValues(this IContentBase content, object value, string culture = null, string segment = null) - { - content.PropertyValues(Current.Services.ContentTypeBaseServices, value, culture, segment); - } - + /// /// Set property values by alias with an anonymous object. /// /// Does not support variants. - public static void PropertyValues(this IContentBase content, IContentTypeBaseServiceProvider contentTypeServiceProvider, object value, string culture = null, string segment = null) + public static void PropertyValues(this IContentBase content, object value, string culture = null, string segment = null) { if (value == null) throw new Exception("No properties has been passed in"); @@ -35,12 +31,13 @@ namespace Umbraco.Tests.Testing else { - //TODO: Will this ever happen?? In theory we don't need to lookup the content type here since we can just check if the content contains properties with the correct name, - // however, i think this may be needed in the case where the content type contains property types that do not exist yet as properties on the - // content item? But can that happen? AFAIK it can't/shouldn't because of how we create content items in ContentBase we do _properties.EnsurePropertyTypes! - - var contentType = contentTypeServiceProvider.GetContentTypeOf(content); - var propertyType = contentType.CompositionPropertyTypes.FirstOrDefault(x => x.Alias == propertyInfo.Name); + //fixme: Can this ever happen? According to the ctor in ContentBase (EnsurePropertyTypes), all properties will be created based on the content type's property types + // so how can a property not be resolved by the alias on the content.Properties but it can on the content type? + // This maybe can happen if a developer has removed a property with the api and is trying to then set the value of that property again... + // BUT, as it turns out the content.Properties.Remove(...) method is NEVER used, because why and how could it? you never remove a property from + // a content item directly. + + var propertyType = ((ContentBase)content).AllPropertyTypes.FirstOrDefault(x => x.Alias == propertyInfo.Name); if (propertyType == null) throw new Exception($"The property alias {propertyInfo.Name} is not valid, because no PropertyType with this alias exists"); //Create new Property to add to collection