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