From c4ee9abe68b2ea7012ff565277a2faf2cdd08c3a Mon Sep 17 00:00:00 2001 From: Claus Date: Sat, 27 Oct 2018 12:51:05 +0200 Subject: [PATCH 1/3] adds internal methods for getting media entities through the entity repository/service without fetching all property data. ensures the trees use this method to avoid fetching property data in the media tree. --- .../Repositories/EntityRepository.cs | 43 +++++++++++++++---- src/Umbraco.Core/Services/EntityService.cs | 33 +++++++++++--- .../Trees/ContentTreeControllerBase.cs | 9 ++-- 3 files changed, 67 insertions(+), 18 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs b/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs index 40d35d2fee..ca6fc36e23 100644 --- a/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs @@ -331,24 +331,49 @@ namespace Umbraco.Core.Persistence.Repositories return list; } + /// + /// Gets entities by query. + /// Note that this will also fetch all property data for media items, which can cause performance problems + /// when used without paging, in sites with large amounts of data in cmsPropertyData. + /// + /// + /// + /// public virtual IEnumerable GetByQuery(IQuery query, Guid objectTypeId) { + return GetByQueryInternal(query, objectTypeId, true); + } - bool isContent = objectTypeId == Constants.ObjectTypes.DocumentGuid || objectTypeId == Constants.ObjectTypes.DocumentBlueprintGuid; - bool isMedia = objectTypeId == Constants.ObjectTypes.MediaGuid; + /// + /// Gets entities by query without fetching property data. + /// This is supposed to be internal and can be used when getting all entities without paging, without causing + /// performance issues. + /// + /// + /// + /// + internal IEnumerable GetByQueryWithoutPropertyData(IQuery query, Guid objectTypeId) + { + return GetByQueryInternal(query, objectTypeId, false); + } + + internal IEnumerable GetByQueryInternal(IQuery query, Guid objectTypeId, bool includePropertyData) + { + var isContent = objectTypeId == Constants.ObjectTypes.DocumentGuid || objectTypeId == Constants.ObjectTypes.DocumentBlueprintGuid; + var isMedia = objectTypeId == Constants.ObjectTypes.MediaGuid; var sqlClause = GetBaseWhere(GetBase, isContent, isMedia, null, objectTypeId); - + var translator = new SqlTranslator(sqlClause, query); var entitySql = translator.Translate(); var factory = new UmbracoEntityFactory(); - if (isMedia) + if (isMedia && includePropertyData) { var wheres = query.GetWhereClauses().ToArray(); - var mediaSql = GetFullSqlForMedia(entitySql.Append(GetGroupBy(isContent, true, false)), sql => + var mediaSql = GetFullSqlForMedia(entitySql.Append(GetGroupBy(isContent, isMedia, false)), sql => { //adds the additional filters foreach (var whereClause in wheres) @@ -365,21 +390,21 @@ namespace Umbraco.Core.Persistence.Repositories else { //use dynamic so that we can get ALL properties from the SQL so we can chuck that data into our AdditionalData - var finalSql = entitySql.Append(GetGroupBy(isContent, false)); + var finalSql = entitySql.Append(GetGroupBy(isContent, isMedia)); //query = read forward data reader, do not load everything into mem var dtos = _work.Database.Query(finalSql); var collection = new EntityDefinitionCollection(); foreach (var dto in dtos) { - collection.AddOrUpdate(new EntityDefinition(factory, dto, isContent, false)); + collection.AddOrUpdate(new EntityDefinition(factory, dto, isContent, isMedia)); } return collection.Select(x => x.BuildFromDynamic()).ToList(); } } #endregion - + #region Sql Statements @@ -851,4 +876,4 @@ namespace Umbraco.Core.Persistence.Repositories } #endregion } -} \ No newline at end of file +} diff --git a/src/Umbraco.Core/Services/EntityService.cs b/src/Umbraco.Core/Services/EntityService.cs index b660927df3..ef316409a1 100644 --- a/src/Umbraco.Core/Services/EntityService.cs +++ b/src/Umbraco.Core/Services/EntityService.cs @@ -2,8 +2,6 @@ using System.Collections.Generic; using System.Linq; using System.Linq.Expressions; -using System.Text; -using Umbraco.Core.Cache; using Umbraco.Core.CodeAnnotations; using Umbraco.Core.Events; using Umbraco.Core.Logging; @@ -13,6 +11,7 @@ using Umbraco.Core.Models.Rdbms; using Umbraco.Core.Persistence; using Umbraco.Core.Persistence.DatabaseModelDefinitions; using Umbraco.Core.Persistence.Querying; +using Umbraco.Core.Persistence.Repositories; using Umbraco.Core.Persistence.UnitOfWork; namespace Umbraco.Core.Services @@ -285,15 +284,37 @@ namespace Umbraco.Core.Services /// UmbracoObjectType of the children to retrieve /// An enumerable list of objects public virtual IEnumerable GetChildren(int parentId, UmbracoObjectTypes umbracoObjectType) + { + return GetChildrenInternal(parentId, umbracoObjectType, true); + } + + /// + /// Gets a collection of children by the parent's Id and UmbracoObjectType without adding property data + /// + /// Id of the parent to retrieve children for + /// UmbracoObjectType of the children to retrieve + /// An enumerable list of objects + internal IEnumerable GetChildrenWithoutPropertyData(int parentId, UmbracoObjectTypes umbracoObjectType) + { + return GetChildrenInternal(parentId, umbracoObjectType, false); + } + + internal IEnumerable GetChildrenInternal(int parentId, UmbracoObjectTypes umbracoObjectType, bool includePropertyData) { var objectTypeId = umbracoObjectType.GetGuid(); - using (var uow = UowProvider.GetUnitOfWork(readOnly:true)) + using (var uow = UowProvider.GetUnitOfWork(readOnly: true)) { var repository = RepositoryFactory.CreateEntityRepository(uow); var query = Query.Builder.Where(x => x.ParentId == parentId); - var contents = repository.GetByQuery(query, objectTypeId); - return contents; + if (includePropertyData) + return repository.GetByQuery(query, objectTypeId); + + // Not pretty having to cast the repository, but it is the only way to get to use an internal method that we + // do not want to make public on the interface. Unfortunately also prevents this from being unit tested. + // See this issue for details on why we need this: + // https://github.com/umbraco/Umbraco-CMS/issues/3457 + return ((EntityRepository)repository).GetByQueryWithoutPropertyData(query, objectTypeId); } } @@ -781,4 +802,4 @@ namespace Umbraco.Core.Services return node.NodeId; } } -} \ No newline at end of file +} diff --git a/src/Umbraco.Web/Trees/ContentTreeControllerBase.cs b/src/Umbraco.Web/Trees/ContentTreeControllerBase.cs index 55a430326c..ec0f0bf5aa 100644 --- a/src/Umbraco.Web/Trees/ContentTreeControllerBase.cs +++ b/src/Umbraco.Web/Trees/ContentTreeControllerBase.cs @@ -1,7 +1,6 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; -using System.ComponentModel; using System.Linq; using System.Net; using System.Net.Http; @@ -11,12 +10,12 @@ using Umbraco.Core; using Umbraco.Core.Logging; using Umbraco.Core.Models; using Umbraco.Core.Models.EntityBase; -using Umbraco.Core.Persistence; using Umbraco.Web.Models.Trees; using Umbraco.Web.WebApi.Filters; using umbraco; using umbraco.BusinessLogic.Actions; using System.Globalization; +using Umbraco.Core.Services; namespace Umbraco.Web.Trees { @@ -203,7 +202,11 @@ namespace Umbraco.Web.Trees entityId = entity.Id; } - return Services.EntityService.GetChildren(entityId, UmbracoObjectType).ToList(); + // Not pretty having to cast the service, but it is the only way to get to use an internal method that we + // do not want to make public on the interface. Unfortunately also prevents this from being unit tested. + // See this issue for details on why we need this: + // https://github.com/umbraco/Umbraco-CMS/issues/3457 + return ((EntityService)Services.EntityService).GetChildrenWithoutPropertyData(entityId, UmbracoObjectType).ToList(); } /// From c0109e05318c0a8672f74b1feb41c7a4a47936c6 Mon Sep 17 00:00:00 2001 From: Claus Date: Sat, 27 Oct 2018 13:02:21 +0200 Subject: [PATCH 2/3] whitespace --- src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs b/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs index ca6fc36e23..ce8c27e474 100644 --- a/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs @@ -359,7 +359,7 @@ namespace Umbraco.Core.Persistence.Repositories internal IEnumerable GetByQueryInternal(IQuery query, Guid objectTypeId, bool includePropertyData) { - var isContent = objectTypeId == Constants.ObjectTypes.DocumentGuid || objectTypeId == Constants.ObjectTypes.DocumentBlueprintGuid; + var isContent = objectTypeId == Constants.ObjectTypes.DocumentGuid || objectTypeId == Constants.ObjectTypes.DocumentBlueprintGuid; var isMedia = objectTypeId == Constants.ObjectTypes.MediaGuid; var sqlClause = GetBaseWhere(GetBase, isContent, isMedia, null, objectTypeId); From 1429053681f4b6852e8a331fa37af50606fa0c16 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 1 Nov 2018 13:12:47 +1100 Subject: [PATCH 3/3] Slight refactor and cleanup --- .../Persistence/Factories/MediaFactory.cs | 5 +- .../Repositories/EntityRepository.cs | 81 +++++++++++-------- src/Umbraco.Core/Services/EntityService.cs | 25 +++--- .../Trees/ContentTreeController.cs | 3 + .../Trees/ContentTreeControllerBase.cs | 13 +-- src/Umbraco.Web/Trees/MediaTreeController.cs | 9 ++- 6 files changed, 80 insertions(+), 56 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Factories/MediaFactory.cs b/src/Umbraco.Core/Persistence/Factories/MediaFactory.cs index 693aef594a..080a358657 100644 --- a/src/Umbraco.Core/Persistence/Factories/MediaFactory.cs +++ b/src/Umbraco.Core/Persistence/Factories/MediaFactory.cs @@ -140,6 +140,9 @@ namespace Umbraco.Core.Persistence.Factories /// internal static bool TryMatch(string text, out string mediaPath) { + //TODO: In v8 we should allow exposing this via the property editor in a much nicer way so that the property editor + // can tell us directly what any URL is for a given property if it contains an asset + mediaPath = null; if (string.IsNullOrWhiteSpace(text)) @@ -155,4 +158,4 @@ namespace Umbraco.Core.Persistence.Factories return true; } } -} \ No newline at end of file +} diff --git a/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs b/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs index ce8c27e474..adfce25cce 100644 --- a/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs @@ -333,31 +333,15 @@ namespace Umbraco.Core.Persistence.Repositories /// /// Gets entities by query. + /// + /// + /// + /// + /// /// Note that this will also fetch all property data for media items, which can cause performance problems /// when used without paging, in sites with large amounts of data in cmsPropertyData. - /// - /// - /// - /// + /// public virtual IEnumerable GetByQuery(IQuery query, Guid objectTypeId) - { - return GetByQueryInternal(query, objectTypeId, true); - } - - /// - /// Gets entities by query without fetching property data. - /// This is supposed to be internal and can be used when getting all entities without paging, without causing - /// performance issues. - /// - /// - /// - /// - internal IEnumerable GetByQueryWithoutPropertyData(IQuery query, Guid objectTypeId) - { - return GetByQueryInternal(query, objectTypeId, false); - } - - internal IEnumerable GetByQueryInternal(IQuery query, Guid objectTypeId, bool includePropertyData) { var isContent = objectTypeId == Constants.ObjectTypes.DocumentGuid || objectTypeId == Constants.ObjectTypes.DocumentBlueprintGuid; var isMedia = objectTypeId == Constants.ObjectTypes.MediaGuid; @@ -369,11 +353,11 @@ namespace Umbraco.Core.Persistence.Repositories var factory = new UmbracoEntityFactory(); - if (isMedia && includePropertyData) + if (isMedia) { var wheres = query.GetWhereClauses().ToArray(); - var mediaSql = GetFullSqlForMedia(entitySql.Append(GetGroupBy(isContent, isMedia, false)), sql => + var mediaSql = GetFullSqlForMedia(entitySql.Append(GetGroupBy(isContent, true, false)), sql => { //adds the additional filters foreach (var whereClause in wheres) @@ -389,20 +373,47 @@ namespace Umbraco.Core.Persistence.Repositories } else { - //use dynamic so that we can get ALL properties from the SQL so we can chuck that data into our AdditionalData - var finalSql = entitySql.Append(GetGroupBy(isContent, isMedia)); - - //query = read forward data reader, do not load everything into mem - var dtos = _work.Database.Query(finalSql); - var collection = new EntityDefinitionCollection(); - foreach (var dto in dtos) - { - collection.AddOrUpdate(new EntityDefinition(factory, dto, isContent, isMedia)); - } - return collection.Select(x => x.BuildFromDynamic()).ToList(); + return GetByQueryInternal(entitySql, isContent, isMedia); } } + /// + /// Gets entities by query without fetching property data. + /// + /// + /// + /// + /// + /// This is supposed to be internal and can be used when getting all entities without paging, without causing + /// performance issues. + /// + internal IEnumerable GetMediaByQueryWithoutPropertyData(IQuery query) + { + var sqlClause = GetBaseWhere(GetBase, false, true, null, UmbracoObjectTypes.Media.GetGuid()); + + var translator = new SqlTranslator(sqlClause, query); + var entitySql = translator.Translate(); + + return GetByQueryInternal(entitySql, false, true); + } + + internal IEnumerable GetByQueryInternal(Sql entitySql, bool isContent, bool isMedia) + { + var factory = new UmbracoEntityFactory(); + + //use dynamic so that we can get ALL properties from the SQL so we can chuck that data into our AdditionalData + var finalSql = entitySql.Append(GetGroupBy(isContent, isMedia)); + + //query = read forward data reader, do not load everything into mem + var dtos = _work.Database.Query(finalSql); + var collection = new EntityDefinitionCollection(); + foreach (var dto in dtos) + { + collection.AddOrUpdate(new EntityDefinition(factory, dto, isContent, isMedia)); + } + return collection.Select(x => x.BuildFromDynamic()).ToList(); + } + #endregion diff --git a/src/Umbraco.Core/Services/EntityService.cs b/src/Umbraco.Core/Services/EntityService.cs index ef316409a1..1e44bec909 100644 --- a/src/Umbraco.Core/Services/EntityService.cs +++ b/src/Umbraco.Core/Services/EntityService.cs @@ -285,39 +285,36 @@ namespace Umbraco.Core.Services /// An enumerable list of objects public virtual IEnumerable GetChildren(int parentId, UmbracoObjectTypes umbracoObjectType) { - return GetChildrenInternal(parentId, umbracoObjectType, true); + var objectTypeId = umbracoObjectType.GetGuid(); + using (var uow = UowProvider.GetUnitOfWork(readOnly: true)) + { + var repository = RepositoryFactory.CreateEntityRepository(uow); + var query = Query.Builder.Where(x => x.ParentId == parentId); + return repository.GetByQuery(query, objectTypeId); + } } /// /// Gets a collection of children by the parent's Id and UmbracoObjectType without adding property data /// /// Id of the parent to retrieve children for - /// UmbracoObjectType of the children to retrieve /// An enumerable list of objects - internal IEnumerable GetChildrenWithoutPropertyData(int parentId, UmbracoObjectTypes umbracoObjectType) + internal IEnumerable GetMediaChildrenWithoutPropertyData(int parentId) { - return GetChildrenInternal(parentId, umbracoObjectType, false); - } - - internal IEnumerable GetChildrenInternal(int parentId, UmbracoObjectTypes umbracoObjectType, bool includePropertyData) - { - var objectTypeId = umbracoObjectType.GetGuid(); + var objectTypeId = UmbracoObjectTypes.Media.GetGuid(); using (var uow = UowProvider.GetUnitOfWork(readOnly: true)) { var repository = RepositoryFactory.CreateEntityRepository(uow); var query = Query.Builder.Where(x => x.ParentId == parentId); - if (includePropertyData) - return repository.GetByQuery(query, objectTypeId); - // Not pretty having to cast the repository, but it is the only way to get to use an internal method that we // do not want to make public on the interface. Unfortunately also prevents this from being unit tested. // See this issue for details on why we need this: // https://github.com/umbraco/Umbraco-CMS/issues/3457 - return ((EntityRepository)repository).GetByQueryWithoutPropertyData(query, objectTypeId); + return ((EntityRepository)repository).GetMediaByQueryWithoutPropertyData(query); } } - + /// /// Returns a paged collection of children /// diff --git a/src/Umbraco.Web/Trees/ContentTreeController.cs b/src/Umbraco.Web/Trees/ContentTreeController.cs index 6cb13aa847..c3fe7df483 100644 --- a/src/Umbraco.Web/Trees/ContentTreeController.cs +++ b/src/Umbraco.Web/Trees/ContentTreeController.cs @@ -207,6 +207,9 @@ namespace Umbraco.Web.Trees return HasPathAccess(entity, queryStrings); } + internal override IEnumerable GetChildrenFromEntityService(int entityId) + => Services.EntityService.GetChildren(entityId, UmbracoObjectType).ToList(); + /// /// Returns a collection of all menu items that can be on a content node /// diff --git a/src/Umbraco.Web/Trees/ContentTreeControllerBase.cs b/src/Umbraco.Web/Trees/ContentTreeControllerBase.cs index ec0f0bf5aa..006355eb35 100644 --- a/src/Umbraco.Web/Trees/ContentTreeControllerBase.cs +++ b/src/Umbraco.Web/Trees/ContentTreeControllerBase.cs @@ -202,13 +202,16 @@ namespace Umbraco.Web.Trees entityId = entity.Id; } - // Not pretty having to cast the service, but it is the only way to get to use an internal method that we - // do not want to make public on the interface. Unfortunately also prevents this from being unit tested. - // See this issue for details on why we need this: - // https://github.com/umbraco/Umbraco-CMS/issues/3457 - return ((EntityService)Services.EntityService).GetChildrenWithoutPropertyData(entityId, UmbracoObjectType).ToList(); + return GetChildrenFromEntityService(entityId); } + /// + /// Abstract method to fetch the entities from the entity service + /// + /// + /// + internal abstract IEnumerable GetChildrenFromEntityService(int entityId); + /// /// Returns true or false if the current user has access to the node based on the user's allowed start node (path) access /// diff --git a/src/Umbraco.Web/Trees/MediaTreeController.cs b/src/Umbraco.Web/Trees/MediaTreeController.cs index 932ced9616..708357d66d 100644 --- a/src/Umbraco.Web/Trees/MediaTreeController.cs +++ b/src/Umbraco.Web/Trees/MediaTreeController.cs @@ -173,5 +173,12 @@ namespace Umbraco.Web.Trees { return _treeSearcher.ExamineSearch(Umbraco, query, UmbracoEntityTypes.Media, pageSize, pageIndex, out totalFound, searchFrom); } + + internal override IEnumerable GetChildrenFromEntityService(int entityId) + // Not pretty having to cast the service, but it is the only way to get to use an internal method that we + // do not want to make public on the interface. Unfortunately also prevents this from being unit tested. + // See this issue for details on why we need this: + // https://github.com/umbraco/Umbraco-CMS/issues/3457 + => ((EntityService)Services.EntityService).GetMediaChildrenWithoutPropertyData(entityId).ToList(); } -} \ No newline at end of file +}