diff --git a/src/Umbraco.Core/ContentVariationExtensions.cs b/src/Umbraco.Core/ContentVariationExtensions.cs index d18fb4b091..516192b905 100644 --- a/src/Umbraco.Core/ContentVariationExtensions.cs +++ b/src/Umbraco.Core/ContentVariationExtensions.cs @@ -115,7 +115,7 @@ namespace Umbraco.Core /// /// Determines whether a variation varies by culture and segment. /// - public static bool VariesByCultureAndSegment(this ContentVariation variation) => (variation & ContentVariation.CultureAndSegment) > 0; + public static bool VariesByCultureAndSegment(this ContentVariation variation) => (variation & ContentVariation.CultureAndSegment) == ContentVariation.CultureAndSegment; /// /// Validates that a combination of culture and segment is valid for the variation. diff --git a/src/Umbraco.Core/Models/ContentTypeBaseExtensions.cs b/src/Umbraco.Core/Models/ContentTypeBaseExtensions.cs index 0d2f817660..8af48bb881 100644 --- a/src/Umbraco.Core/Models/ContentTypeBaseExtensions.cs +++ b/src/Umbraco.Core/Models/ContentTypeBaseExtensions.cs @@ -63,20 +63,5 @@ namespace Umbraco.Core.Models aliases = a; return hasAnyPropertyVariationChanged; } - - /// - /// Returns the list of content types the composition is used in - /// - /// - /// - /// - internal static IEnumerable GetWhereCompositionIsUsedInContentTypes(this IContentTypeComposition source, - IContentTypeComposition[] allContentTypes) - { - var sourceId = source != null ? source.Id : 0; - - // find which content types are using this composition - return allContentTypes.Where(x => x.ContentTypeComposition.Any(y => y.Id == sourceId)).ToArray(); - } } } diff --git a/src/Umbraco.Core/Models/ContentTypeCompositionBase.cs b/src/Umbraco.Core/Models/ContentTypeCompositionBase.cs index 838a75b98b..2f455083c0 100644 --- a/src/Umbraco.Core/Models/ContentTypeCompositionBase.cs +++ b/src/Umbraco.Core/Models/ContentTypeCompositionBase.cs @@ -114,6 +114,27 @@ namespace Umbraco.Core.Models } } + /// + /// Gets the property types obtained via composition. + /// + /// + /// Gets them raw, ie with their original variation. + /// + [IgnoreDataMember] + internal IEnumerable RawComposedPropertyTypes => GetRawComposedPropertyTypes(); + + private IEnumerable GetRawComposedPropertyTypes(bool start = true) + { + var propertyTypes = ContentTypeComposition + .Cast() + .SelectMany(x => start ? x.GetRawComposedPropertyTypes(false) : x.CompositionPropertyTypes); + + if (!start) + propertyTypes = propertyTypes.Union(PropertyTypes); + + return propertyTypes; + } + /// /// Adds a content type to the composition. /// diff --git a/src/Umbraco.Core/Persistence/Repositories/IContentTypeRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/IContentTypeRepositoryBase.cs index 3bb1ac38ca..cc9b86c56b 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IContentTypeRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IContentTypeRepositoryBase.cs @@ -11,13 +11,6 @@ namespace Umbraco.Core.Persistence.Repositories TItem Get(string alias); IEnumerable> Move(TItem moving, EntityContainer container); - /// - /// Returns the content types that are direct compositions of the content type - /// - /// The content type id - /// - IEnumerable GetTypesDirectlyComposedOf(int id); - /// /// Derives a unique alias from an existing alias. /// diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepository.cs index aa61383f85..4bec3160a7 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepository.cs @@ -67,7 +67,6 @@ namespace Umbraco.Core.Persistence.Repositories.Implement return ContentTypeQueryMapper.GetContentTypes(Database, SqlSyntax, IsPublishing, this, _templateRepository); } - protected override IEnumerable PerformGetAll(params Guid[] ids) { // use the underlying GetAll which will force cache all content types diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs index adf02a52f3..8da92991a4 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs @@ -119,7 +119,6 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected void PersistNewBaseContentType(IContentTypeComposition entity) { - var dto = ContentTypeFactory.BuildContentTypeDto(entity); //Cannot add a duplicate content type type @@ -234,7 +233,6 @@ AND umbracoNode.nodeObjectType = @objectType", protected void PersistUpdatedBaseContentType(IContentTypeComposition entity) { - var dto = ContentTypeFactory.BuildContentTypeDto(entity); // ensure the alias is not used already @@ -314,7 +312,7 @@ AND umbracoNode.id <> @id", } } - // delete the allowed content type entries before re-inserting the collectino of allowed content types + // delete the allowed content type entries before re-inserting the collection of allowed content types Database.Delete("WHERE Id = @Id", new { entity.Id }); foreach (var allowedContentType in entity.AllowedContentTypes) { @@ -420,6 +418,7 @@ AND umbracoNode.id <> @id", // collect property types that have a dirty variation List propertyTypeVariationDirty = null; + // note: this only deals with *local* property types, we're dealing w/compositions later below foreach (var propertyType in entity.PropertyTypes) { if (contentTypeVariationChanging) @@ -458,6 +457,35 @@ AND umbracoNode.id <> @id", ? GetPropertyVariationChanges(propertyTypeVariationDirty) : null; + // deal with composition property types + // add changes for property types obtained via composition, which change due + // to this content type variations change + if (contentTypeVariationChanging) + { + // must use RawComposedPropertyTypes here: only those types that are obtained + // via composition, with their original variations (ie not filtered by this + // content type variations - we need this true value to make decisions. + + foreach (var propertyType in ((ContentTypeCompositionBase) entity).RawComposedPropertyTypes) + { + if (propertyType.VariesBySegment() || newContentTypeVariation.VariesBySegment()) + throw new NotSupportedException(); // TODO: support this + + if (propertyType.Variations == ContentVariation.Culture) + { + if (propertyTypeVariationChanges == null) + propertyTypeVariationChanges = new Dictionary(); + + // if content type moves to Culture, property type becomes Culture here again + // if content type moves to Nothing, property type becomes Nothing here + if (newContentTypeVariation == ContentVariation.Culture) + propertyTypeVariationChanges[propertyType.Id] = (ContentVariation.Nothing, ContentVariation.Culture); + else if (newContentTypeVariation == ContentVariation.Nothing) + propertyTypeVariationChanges[propertyType.Id] = (ContentVariation.Culture, ContentVariation.Nothing); + } + } + } + // insert or update properties // all of them, no-group and in-groups foreach (var propertyType in entity.PropertyTypes) @@ -484,9 +512,19 @@ AND umbracoNode.id <> @id", orphanPropertyTypeIds?.Remove(typeId); } + // must restrict property data changes to impacted content types - if changing a composing + // type, some composed types (those that do not vary) are not impacted and should be left + // unchanged + // + // getting 'all' from the cache policy is prone to race conditions - fast but dangerous + //var all = ((FullDataSetRepositoryCachePolicy)CachePolicy).GetAllCached(PerformGetAll); + var all = PerformGetAll(); + + var impacted = GetImpactedContentTypes(entity, all); + // if some property types have actually changed, move their variant data if (propertyTypeVariationChanges != null) - MovePropertyTypeVariantData(propertyTypeVariationChanges); + MovePropertyTypeVariantData(propertyTypeVariationChanges, impacted); // deal with orphan properties: those that were in a deleted tab, // and have not been re-mapped to another tab or to 'generic properties' @@ -495,6 +533,38 @@ AND umbracoNode.id <> @id", DeletePropertyType(entity.Id, id); } + private IEnumerable GetImpactedContentTypes(IContentTypeComposition contentType, IEnumerable all) + { + var impact = new List(); + var set = new List { contentType }; + + var tree = new Dictionary>(); + foreach (var x in all) + foreach (var y in x.ContentTypeComposition) + { + if (!tree.TryGetValue(y.Id, out var list)) + list = tree[y.Id] = new List(); + list.Add(x); + } + + var nset = new List(); + do + { + impact.AddRange(set); + + foreach (var x in set) + { + if (!tree.TryGetValue(x.Id, out var list)) continue; + nset.AddRange(list.Where(y => y.VariesByCulture())); + } + + set = nset; + nset = new List(); + } while (set.Count > 0); + + return impact; + } + // gets property types that have actually changed, and the corresponding changes // returns null if no property type has actually changed private Dictionary GetPropertyVariationChanges(IEnumerable propertyTypes) @@ -575,9 +645,10 @@ AND umbracoNode.id <> @id", /// /// Moves variant data for property type variation changes. /// - private void MovePropertyTypeVariantData(IDictionary propertyTypeChanges) + private void MovePropertyTypeVariantData(IDictionary propertyTypeChanges, IEnumerable impacted) { var defaultLanguageId = GetDefaultLanguageId(); + var impactedL = impacted.Select(x => x.Id).ToList(); //Group by the "To" variation so we can bulk update in the correct batches foreach(var grouping in propertyTypeChanges.GroupBy(x => x.Value.ToVariation)) @@ -588,10 +659,10 @@ AND umbracoNode.id <> @id", switch (toVariation) { case ContentVariation.Culture: - CopyPropertyData(null, defaultLanguageId, propertyTypeIds); + CopyPropertyData(null, defaultLanguageId, propertyTypeIds, impactedL); break; case ContentVariation.Nothing: - CopyPropertyData(defaultLanguageId, null, propertyTypeIds); + CopyPropertyData(defaultLanguageId, null, propertyTypeIds, impactedL); break; case ContentVariation.CultureAndSegment: case ContentVariation.Segment: @@ -689,12 +760,36 @@ AND umbracoNode.id <> @id", /// The source language (can be null ie invariant). /// The target language (can be null ie invariant) /// The property type identifiers. - private void CopyPropertyData(int? sourceLanguageId, int? targetLanguageId, IReadOnlyCollection propertyTypeIds) + /// The content type identifiers. + private void CopyPropertyData(int? sourceLanguageId, int? targetLanguageId, IReadOnlyCollection propertyTypeIds, IReadOnlyCollection contentTypeIds = null) { + // fixme - should we batch then? + var whereInArgsCount = propertyTypeIds.Count + (contentTypeIds?.Count ?? 0); + if (whereInArgsCount > 2000) + throw new NotSupportedException("Too many property/content types."); + //first clear out any existing property data that might already exists under the target language var sqlDelete = Sql() .Delete(); + // not ok for SqlCe (no JOIN in DELETE) + //if (contentTypeIds != null) + // sqlDelete + // .From() + // .InnerJoin().On((pdata, cversion) => pdata.VersionId == cversion.Id) + // .InnerJoin().On((cversion, c) => cversion.NodeId == c.NodeId); + + Sql inSql = null; + if (contentTypeIds != null) + { + inSql = Sql() + .Select(x => x.Id) + .From() + .InnerJoin().On((cversion, c) => cversion.NodeId == c.NodeId) + .WhereIn(x => x.ContentTypeId, contentTypeIds); + sqlDelete.WhereIn(x => x.VersionId, inSql); + } + // NPoco cannot turn the clause into IS NULL with a nullable parameter - deal with it if (targetLanguageId == null) sqlDelete.Where(x => x.LanguageId == null); @@ -704,6 +799,11 @@ AND umbracoNode.id <> @id", sqlDelete .WhereIn(x => x.PropertyTypeId, propertyTypeIds); + // see note above, not ok for SqlCe + //if (contentTypeIds != null) + // sqlDelete + // .WhereIn(x => x.ContentTypeId, contentTypeIds); + Database.Execute(sqlDelete); //now insert all property data into the target language that exists under the source language @@ -713,6 +813,11 @@ AND umbracoNode.id <> @id", .Append(", " + targetLanguageIdS) //default language ID .From(); + if (contentTypeIds != null) + sqlSelectData + .InnerJoin().On((pdata, cversion) => pdata.VersionId == cversion.Id) + .InnerJoin().On((cversion, c) => cversion.NodeId == c.NodeId); + // NPoco cannot turn the clause into IS NULL with a nullable parameter - deal with it if (sourceLanguageId == null) sqlSelectData.Where(x => x.LanguageId == null); @@ -722,6 +827,10 @@ AND umbracoNode.id <> @id", sqlSelectData .WhereIn(x => x.PropertyTypeId, propertyTypeIds); + if (contentTypeIds != null) + sqlSelectData + .WhereIn(x => x.ContentTypeId, contentTypeIds); + var sqlInsert = Sql($"INSERT INTO {PropertyDataDto.TableName} ({cols})").Append(sqlSelectData); Database.Execute(sqlInsert); @@ -731,7 +840,12 @@ AND umbracoNode.id <> @id", if (sourceLanguageId == null) { sqlDelete = Sql() - .Delete() + .Delete(); + + if (contentTypeIds != null) + sqlDelete.WhereIn(x => x.VersionId, inSql); + + sqlDelete .Where(x => x.LanguageId == null) .WhereIn(x => x.PropertyTypeId, propertyTypeIds); @@ -872,24 +986,6 @@ AND umbracoNode.id <> @id", } } - /// - public IEnumerable GetTypesDirectlyComposedOf(int id) - { - //fixme - this will probably be more efficient to simply load all content types and do the calculation, see GetWhereCompositionIsUsedInContentTypes - - var sql = Sql() - .SelectAll() - .From() - .InnerJoin() - .On(left => left.NodeId, right => right.ChildId) - .Where(x => x.NodeObjectType == NodeObjectTypeId) - .Where(x => x.ParentId == id); - var dtos = Database.Fetch(sql); - return dtos.Any() - ? GetMany(dtos.DistinctBy(x => x.NodeId).Select(x => x.NodeId).ToArray()) - : Enumerable.Empty(); - } - internal static class ContentTypeQueryMapper { public class AssociatedTemplate diff --git a/src/Umbraco.Core/Services/ContentTypeServiceExtensions.cs b/src/Umbraco.Core/Services/ContentTypeServiceExtensions.cs index 66b3982b49..06ba1ada79 100644 --- a/src/Umbraco.Core/Services/ContentTypeServiceExtensions.cs +++ b/src/Umbraco.Core/Services/ContentTypeServiceExtensions.cs @@ -30,12 +30,12 @@ namespace Umbraco.Core.Services string[] filterPropertyTypes = null) { filterContentTypes = filterContentTypes == null - ? new string[] { } - : filterContentTypes.Where(x => x.IsNullOrWhiteSpace() == false).ToArray(); + ? Array.Empty() + : filterContentTypes.Where(x => !x.IsNullOrWhiteSpace()).ToArray(); filterPropertyTypes = filterPropertyTypes == null - ? new string[] {} - : filterPropertyTypes.Where(x => x.IsNullOrWhiteSpace() == false).ToArray(); + ? Array.Empty() + : filterPropertyTypes.Where(x => !x.IsNullOrWhiteSpace()).ToArray(); //create the full list of property types to use as the filter //this is the combination of all property type aliases found in the content types passed in for the filter @@ -47,7 +47,7 @@ namespace Umbraco.Core.Services .Union(filterPropertyTypes) .ToArray(); - var sourceId = source != null ? source.Id : 0; + var sourceId = source?.Id ?? 0; // find out if any content type uses this content type var isUsing = allContentTypes.Where(x => x.ContentTypeComposition.Any(y => y.Id == sourceId)).ToArray(); @@ -161,6 +161,5 @@ namespace Umbraco.Core.Services return all; } - } } diff --git a/src/Umbraco.Core/Services/Implement/ContentTypeServiceBaseOfTRepositoryTItemTService.cs b/src/Umbraco.Core/Services/Implement/ContentTypeServiceBaseOfTRepositoryTItemTService.cs index a114f415cc..9a29176860 100644 --- a/src/Umbraco.Core/Services/Implement/ContentTypeServiceBaseOfTRepositoryTItemTService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentTypeServiceBaseOfTRepositoryTItemTService.cs @@ -344,37 +344,18 @@ namespace Umbraco.Core.Services.Implement } } + public IEnumerable GetComposedOf(int id, IEnumerable all) + { + return all.Where(x => x.ContentTypeComposition.Any(y => y.Id == id)); + + } + public IEnumerable GetComposedOf(int id) { - //fixme: this is essentially the same as ContentTypeServiceExtensions.GetWhereCompositionIsUsedInContentTypes which loads - // all content types to figure this out, this instead makes quite a few queries so should be replaced - - using (var scope = ScopeProvider.CreateScope(autoComplete: true)) - { - scope.ReadLock(ReadLockIds); - - // hash set handles duplicates - var composed = new HashSet(new DelegateEqualityComparer( - (x, y) => x.Id == y.Id, - x => x.Id.GetHashCode())); - - var ids = new Stack(); - ids.Push(id); - - while (ids.Count > 0) - { - var i = ids.Pop(); - var result = Repository.GetTypesDirectlyComposedOf(i).ToArray(); - - foreach (var c in result) - { - composed.Add(c); - ids.Push(c.Id); - } - } - - return composed.ToArray(); - } + // GetAll is cheap, repository has a full dataset cache policy + // fixme - still, because it uses the cache, race conditions! + var allContentTypes = GetAll(Array.Empty()); + return GetComposedOf(id, allContentTypes); } public int Count() diff --git a/src/Umbraco.Tests/Services/ContentTypeServiceTests.cs b/src/Umbraco.Tests/Services/ContentTypeServiceTests.cs index f25382d557..f186ae8e83 100644 --- a/src/Umbraco.Tests/Services/ContentTypeServiceTests.cs +++ b/src/Umbraco.Tests/Services/ContentTypeServiceTests.cs @@ -1,10 +1,8 @@ -using System.Runtime.Remoting; -using NUnit.Framework; +using NUnit.Framework; using System; using System.Collections.Generic; using System.Linq; using System.Threading; -using NPoco; using Umbraco.Core; using Umbraco.Core.Events; using Umbraco.Core.Exceptions; @@ -12,10 +10,9 @@ using Umbraco.Core.Models; using Umbraco.Core.Persistence.Dtos; using Umbraco.Core.Services; using Umbraco.Core.Services.Implement; -using Umbraco.Tests.TestHelpers; using Umbraco.Tests.TestHelpers.Entities; using Umbraco.Tests.Testing; -using Umbraco.Core.Components; +using Umbraco.Tests.Scoping; namespace Umbraco.Tests.Services { diff --git a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs index 22d86631ca..563e8dda1a 100644 --- a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs +++ b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs @@ -119,66 +119,74 @@ namespace Umbraco.Web.Editors /// Type of content Type, eg documentType or mediaType /// Id of composition content type /// - protected IEnumerable PerformGetWhereCompositionIsUsedInContentTypes(int contentTypeId, - UmbracoObjectTypes type) + protected IEnumerable PerformGetWhereCompositionIsUsedInContentTypes(int contentTypeId, UmbracoObjectTypes type) { - IContentTypeComposition source = null; + var id = 0; - //below is all ported from the old doc type editor and comes with the same weaknesses /insanity / magic + if (contentTypeId > 0) + { + IContentTypeComposition source; - IContentTypeComposition[] allContentTypes; + switch (type) + { + case UmbracoObjectTypes.DocumentType: + source = Services.ContentTypeService.Get(contentTypeId); + break; + + case UmbracoObjectTypes.MediaType: + source = Services.ContentTypeService.Get(contentTypeId); + break; + + case UmbracoObjectTypes.MemberType: + source = Services.MemberTypeService.Get(contentTypeId); + break; + + default: + throw new ArgumentOutOfRangeException(nameof(type)); + } + + if (source == null) + throw new HttpResponseException(Request.CreateResponse(HttpStatusCode.NotFound)); + + id = source.Id; + } + + IEnumerable composedOf; switch (type) { case UmbracoObjectTypes.DocumentType: - if (contentTypeId > 0) - { - source = Services.ContentTypeService.Get(contentTypeId); - if (source == null) throw new HttpResponseException(Request.CreateResponse(HttpStatusCode.NotFound)); - } - allContentTypes = Services.ContentTypeService.GetAll().Cast().ToArray(); + composedOf = Services.ContentTypeService.GetComposedOf(id); break; case UmbracoObjectTypes.MediaType: - if (contentTypeId > 0) - { - source = Services.ContentTypeService.Get(contentTypeId); - if (source == null) throw new HttpResponseException(Request.CreateResponse(HttpStatusCode.NotFound)); - } - allContentTypes = Services.ContentTypeService.GetAll().Cast().ToArray(); + composedOf = Services.MediaTypeService.GetComposedOf(id); break; case UmbracoObjectTypes.MemberType: - if (contentTypeId > 0) - { - source = Services.MemberTypeService.Get(contentTypeId); - if (source == null) throw new HttpResponseException(Request.CreateResponse(HttpStatusCode.NotFound)); - } - allContentTypes = Services.MemberTypeService.GetAll().Cast().ToArray(); + composedOf = Services.MemberTypeService.GetComposedOf(id); break; default: - throw new ArgumentOutOfRangeException("The entity type was not a content type"); + throw new ArgumentOutOfRangeException(nameof(type)); } - var contentTypesWhereCompositionIsUsed = source.GetWhereCompositionIsUsedInContentTypes(allContentTypes); - return contentTypesWhereCompositionIsUsed - .Select(x => Mapper.Map(x)) - .Select(x => - { - //translate the name - x.Name = TranslateItem(x.Name); + EntityBasic TranslateName(EntityBasic e) + { + e.Name = TranslateItem(e.Name); + return e; + } - return x; - }) + return composedOf + .Select(Mapper.Map) + .Select(TranslateName) .ToList(); } + protected string TranslateItem(string text) { if (text == null) - { return null; - } if (text.StartsWith("#") == false) return text;