From 90ba9a1a3a546bb356ffecbc0b9130a0ad27653a Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 22 Feb 2017 17:22:22 +1100 Subject: [PATCH] Adds new overload to IMediaService.GetPagedChildren to filter on media type ids which turns out to be much easier than filtering on media type aliases, however i did all of the work to make that happen including unit tests and then it turned out to not be required but we now have the code if necessary, i've left comments about it. I've backported some updates from 7.6 for the SqlIn stuff for the ExpressionVisitor which we still use in the media type id filtering. I updated the JS to query for the folders which all works now. --- .../Persistence/Mappers/MappingResolver.cs | 4 +- .../Querying/ExpressionVisitorBase.cs | 57 ++++++++++-------- .../Querying/ModelToSqlExpressionVisitor.cs | 40 +++++++++++-- ...tensions.cs => SqlExpressionExtensions.cs} | 9 ++- .../Repositories/ContentRepository.cs | 3 + .../Repositories/MediaRepository.cs | 3 + src/Umbraco.Core/Services/IMediaService.cs | 16 +++++ src/Umbraco.Core/Services/MediaService.cs | 25 ++++++++ src/Umbraco.Core/Umbraco.Core.csproj | 2 +- .../ModelToSqlExpressionHelperBenchmarks.cs | 16 +++-- .../Umbraco.Tests.Benchmarks.csproj | 6 ++ src/Umbraco.Tests.Benchmarks/packages.config | 2 + .../Persistence/Querying/ExpressionTests.cs | 39 +++++++++---- .../Repositories/MediaRepositoryTest.cs | 58 ++++++++++++++++++- .../Services/MediaServiceTests.cs | 28 +++++++++ .../src/common/resources/media.resource.js | 13 +++-- .../listview/listview.controller.js | 34 ++++++----- src/Umbraco.Web/Editors/MediaController.cs | 37 +++++++++--- 18 files changed, 313 insertions(+), 79 deletions(-) rename src/Umbraco.Core/Persistence/Querying/{SqlStringExtensions.cs => SqlExpressionExtensions.cs} (83%) diff --git a/src/Umbraco.Core/Persistence/Mappers/MappingResolver.cs b/src/Umbraco.Core/Persistence/Mappers/MappingResolver.cs index 6909c77744..a4dee60ee9 100644 --- a/src/Umbraco.Core/Persistence/Mappers/MappingResolver.cs +++ b/src/Umbraco.Core/Persistence/Mappers/MappingResolver.cs @@ -31,7 +31,7 @@ namespace Umbraco.Core.Persistence.Mappers /// /// /// - internal BaseMapper ResolveMapperByType(Type type) + public virtual BaseMapper ResolveMapperByType(Type type) { return _mapperCache.GetOrAdd(type, type1 => { @@ -67,7 +67,7 @@ namespace Umbraco.Core.Persistence.Mappers return Attempt.Succeed(mapper); } - internal string GetMapping(Type type, string propertyName) + public virtual string GetMapping(Type type, string propertyName) { var mapper = ResolveMapperByType(type); var result = mapper.Map(propertyName); diff --git a/src/Umbraco.Core/Persistence/Querying/ExpressionVisitorBase.cs b/src/Umbraco.Core/Persistence/Querying/ExpressionVisitorBase.cs index 678ceb1d8e..0dc421045d 100644 --- a/src/Umbraco.Core/Persistence/Querying/ExpressionVisitorBase.cs +++ b/src/Umbraco.Core/Persistence/Querying/ExpressionVisitorBase.cs @@ -581,6 +581,18 @@ namespace Umbraco.Core.Persistence.Querying case "InvariantContains": case "InvariantEquals": + //special case, if it is 'Contains' and the argumet that Contains is being called on is + //Enumerable and the methodArgs is the actual member access, then it's an SQL IN claus + if (m.Object == null + && m.Arguments[0].Type != typeof(string) + && m.Arguments.Count == 2 + && methodArgs.Length == 1 + && methodArgs[0].NodeType == ExpressionType.MemberAccess + && TypeHelper.IsTypeAssignableFrom(m.Arguments[0].Type)) + { + goto case "SqlIn"; + } + string compareValue; if (methodArgs[0].NodeType != ExpressionType.Constant) @@ -597,13 +609,6 @@ namespace Umbraco.Core.Persistence.Querying compareValue = methodArgs[0].ToString(); } - //special case, if it is 'Contains' and the member that Contains is being called on is not a string, then - // we should be doing an 'In' clause - but we currently do not support this - if (methodArgs[0].Type != typeof(string) && TypeHelper.IsTypeAssignableFrom(methodArgs[0].Type)) - { - throw new NotSupportedException("An array Contains method is not supported"); - } - //default column type var colType = TextColumnType.NVarchar; @@ -705,29 +710,33 @@ namespace Umbraco.Core.Persistence.Querying // } // return string.Format("{0}{1}", r, s); - //case "In": + case "SqlIn": - // var member = Expression.Convert(m.Arguments[0], typeof(object)); - // var lambda = Expression.Lambda>(member); - // var getter = lambda.Compile(); + if (m.Object == null && methodArgs.Length == 1 && methodArgs[0].NodeType == ExpressionType.MemberAccess) + { + var memberAccess = VisitMemberAccess((MemberExpression) methodArgs[0]); + + var member = Expression.Convert(m.Arguments[0], typeof(object)); + var lambda = Expression.Lambda>(member); + var getter = lambda.Compile(); - // var inArgs = (object[])getter(); + var inArgs = (IEnumerable)getter(); - // var sIn = new StringBuilder(); - // foreach (var e in inArgs) - // { - // SqlParameters.Add(e); + var sIn = new StringBuilder(); + foreach (var e in inArgs) + { + SqlParameters.Add(e); - // sIn.AppendFormat("{0}{1}", - // sIn.Length > 0 ? "," : "", - // string.Format("@{0}", SqlParameters.Count - 1)); + sIn.AppendFormat("{0}{1}", + sIn.Length > 0 ? "," : "", + string.Format("@{0}", SqlParameters.Count - 1)); + } - // //sIn.AppendFormat("{0}{1}", - // // sIn.Length > 0 ? "," : "", - // // GetQuotedValue(e, e.GetType())); - // } + return string.Format("{0} IN ({1})", memberAccess, sIn); + } + + throw new NotSupportedException("SqlIn must contain the member being accessed"); - // return string.Format("{0} {1} ({2})", r, m.Method.Name, sIn.ToString()); //case "Desc": // return string.Format("{0} DESC", r); //case "Alias": diff --git a/src/Umbraco.Core/Persistence/Querying/ModelToSqlExpressionVisitor.cs b/src/Umbraco.Core/Persistence/Querying/ModelToSqlExpressionVisitor.cs index 7f5e479af6..c4029516a3 100644 --- a/src/Umbraco.Core/Persistence/Querying/ModelToSqlExpressionVisitor.cs +++ b/src/Umbraco.Core/Persistence/Querying/ModelToSqlExpressionVisitor.cs @@ -1,5 +1,7 @@ using System; +using System.Linq; using System.Linq.Expressions; +using Umbraco.Core.Models.EntityBase; using Umbraco.Core.Persistence.Mappers; using Umbraco.Core.Persistence.SqlSyntax; @@ -12,17 +14,19 @@ namespace Umbraco.Core.Persistence.Querying /// This object is stateful and cannot be re-used to parse an expression. internal class ModelToSqlExpressionVisitor : ExpressionVisitorBase { + private readonly MappingResolver _mappingResolver; private readonly BaseMapper _mapper; - public ModelToSqlExpressionVisitor(ISqlSyntaxProvider sqlSyntax, BaseMapper mapper) + public ModelToSqlExpressionVisitor(ISqlSyntaxProvider sqlSyntax, MappingResolver mappingResolver) : base(sqlSyntax) { - _mapper = mapper; + _mapper = mappingResolver.ResolveMapperByType(typeof(T)); + _mappingResolver = mappingResolver; } [Obsolete("Use the overload the specifies a SqlSyntaxProvider")] public ModelToSqlExpressionVisitor() - : this(SqlSyntaxContext.SqlSyntaxProvider, MappingResolver.Current.ResolveMapperByType(typeof(T))) + : this(SqlSyntaxContext.SqlSyntaxProvider, MappingResolver.Current) { } protected override string VisitMemberAccess(MemberExpression m) @@ -36,7 +40,7 @@ namespace Umbraco.Core.Persistence.Querying { var field = _mapper.Map(m.Member.Name, true); if (field.IsNullOrWhiteSpace()) - throw new InvalidOperationException("The mapper returned an empty field for the member name: " + m.Member.Name); + throw new InvalidOperationException(string.Format("The mapper returned an empty field for the member name: {0} for type: {1}", m.Member.Name, m.Expression.Type)); return field; } //already compiled, return @@ -50,13 +54,39 @@ namespace Umbraco.Core.Persistence.Querying { var field = _mapper.Map(m.Member.Name, true); if (field.IsNullOrWhiteSpace()) - throw new InvalidOperationException("The mapper returned an empty field for the member name: " + m.Member.Name); + throw new InvalidOperationException(string.Format("The mapper returned an empty field for the member name: {0} for type: {1}", m.Member.Name, m.Expression.Type)); return field; } //already compiled, return return string.Empty; } + if (m.Expression != null && m.Expression.Type != typeof(T) && TypeHelper.IsTypeAssignableFrom(m.Expression.Type)) + { + //if this is the case, it means we have a sub expression / nested property access, such as: x.ContentType.Alias == "Test"; + //and since the sub type (x.ContentType) is not the same as x, we need to resolve a mapper for x.ContentType to get it's mapped SQL column + + //don't execute if compiled + if (Visited == false) + { + var subMapper = _mappingResolver.ResolveMapperByType(m.Expression.Type); + if (subMapper == null) + throw new NullReferenceException("No mapper found for type " + m.Expression.Type); + var field = subMapper.Map(m.Member.Name, true); + if (field.IsNullOrWhiteSpace()) + throw new InvalidOperationException(string.Format("The mapper returned an empty field for the member name: {0} for type: {1}", m.Member.Name, m.Expression.Type)); + return field; + } + //already compiled, return + return string.Empty; + } + + //TODO: When m.Expression.NodeType == ExpressionType.Constant and it's an expression like: content => aliases.Contains(content.ContentType.Alias); + // then an SQL parameter will be added for aliases as an array, however in SqlIn on the subclass it will manually add these SqlParameters anyways, + // however the query will still execute because the SQL that is written will only contain the correct indexes of SQL parameters, this would be ignored, + // I'm just unsure right now due to time constraints how to make it correct. It won't matter right now and has been working already with this bug but I've + // only just discovered what it is actually doing. + var member = Expression.Convert(m, typeof(object)); var lambda = Expression.Lambda>(member); var getter = lambda.Compile(); diff --git a/src/Umbraco.Core/Persistence/Querying/SqlStringExtensions.cs b/src/Umbraco.Core/Persistence/Querying/SqlExpressionExtensions.cs similarity index 83% rename from src/Umbraco.Core/Persistence/Querying/SqlStringExtensions.cs rename to src/Umbraco.Core/Persistence/Querying/SqlExpressionExtensions.cs index cbecc0a591..5a0452c3aa 100644 --- a/src/Umbraco.Core/Persistence/Querying/SqlStringExtensions.cs +++ b/src/Umbraco.Core/Persistence/Querying/SqlExpressionExtensions.cs @@ -1,4 +1,6 @@ using System; +using System.Collections.Generic; +using System.Linq; using System.Text.RegularExpressions; namespace Umbraco.Core.Persistence.Querying @@ -6,8 +8,13 @@ namespace Umbraco.Core.Persistence.Querying /// /// String extension methods used specifically to translate into SQL /// - internal static class SqlStringExtensions + internal static class SqlExpressionExtensions { + public static bool SqlIn(this IEnumerable collection, T item) + { + return collection.Contains(item); + } + public static bool SqlWildcard(this string str, string txt, TextColumnType columnType) { var wildcardmatch = new Regex("^" + Regex.Escape(txt). diff --git a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs index 8488bdb65a..83c6095775 100644 --- a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs @@ -134,6 +134,9 @@ namespace Umbraco.Core.Persistence.Repositories .On(SqlSyntax, left => left.NodeId, right => right.NodeId) .InnerJoin(SqlSyntax) .On(SqlSyntax, left => left.NodeId, right => right.NodeId); + //TODO: IF we want to enable querying on content type information this will need to be joined + //.InnerJoin(SqlSyntax) + //.On(SqlSyntax, left => left.ContentTypeId, right => right.NodeId, SqlSyntax); if (queryType == BaseQueryType.FullSingle) { diff --git a/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs b/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs index 53b04e1322..3279571518 100644 --- a/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs @@ -94,6 +94,9 @@ namespace Umbraco.Core.Persistence.Repositories .On(SqlSyntax, left => left.NodeId, right => right.NodeId) .InnerJoin(SqlSyntax) .On(SqlSyntax, left => left.NodeId, right => right.NodeId, SqlSyntax) + //TODO: IF we want to enable querying on content type information this will need to be joined + //.InnerJoin(SqlSyntax) + //.On(SqlSyntax, left => left.ContentTypeId, right => right.NodeId, SqlSyntax); .Where(x => x.NodeObjectType == NodeObjectTypeId, SqlSyntax); return sql; } diff --git a/src/Umbraco.Core/Services/IMediaService.cs b/src/Umbraco.Core/Services/IMediaService.cs index d4218764b7..ac0ff94d3c 100644 --- a/src/Umbraco.Core/Services/IMediaService.cs +++ b/src/Umbraco.Core/Services/IMediaService.cs @@ -166,6 +166,22 @@ namespace Umbraco.Core.Services IEnumerable GetPagedChildren(int id, long pageIndex, int pageSize, out long totalRecords, string orderBy, Direction orderDirection, bool orderBySystemField, string filter); + /// + /// Gets a collection of objects by Parent Id + /// + /// Id of the Parent to retrieve Children from + /// Page number + /// Page size + /// Total records query would return without paging + /// Field to order by + /// Direction to order by + /// Flag to indicate when ordering by system field + /// Search text filter + /// A list of content type Ids to filter the list by + /// An Enumerable list of objects + IEnumerable GetPagedChildren(int id, long pageIndex, int pageSize, out long totalRecords, + string orderBy, Direction orderDirection, bool orderBySystemField, string filter, int[] contentTypeFilter); + [Obsolete("Use the overload with 'long' parameter types instead")] [EditorBrowsable(EditorBrowsableState.Never)] IEnumerable GetPagedDescendants(int id, int pageIndex, int pageSize, out int totalRecords, diff --git a/src/Umbraco.Core/Services/MediaService.cs b/src/Umbraco.Core/Services/MediaService.cs index a67ee8285d..d236dccc5a 100644 --- a/src/Umbraco.Core/Services/MediaService.cs +++ b/src/Umbraco.Core/Services/MediaService.cs @@ -20,6 +20,7 @@ using Umbraco.Core.Persistence.Repositories; using Umbraco.Core.Persistence.SqlSyntax; using Umbraco.Core.Persistence.UnitOfWork; using Umbraco.Core.Publishing; +using ContentType = System.Net.Mime.ContentType; namespace Umbraco.Core.Services { @@ -435,6 +436,25 @@ namespace Umbraco.Core.Services /// An Enumerable list of objects public IEnumerable GetPagedChildren(int id, long pageIndex, int pageSize, out long totalChildren, string orderBy, Direction orderDirection, bool orderBySystemField, string filter) + { + return GetPagedChildren(id, pageIndex, pageSize, out totalChildren, orderBy, orderDirection, true, filter, null); + } + + /// + /// Gets a collection of objects by Parent Id + /// + /// Id of the Parent to retrieve Children from + /// Page number + /// Page size + /// Total records query would return without paging + /// Field to order by + /// Direction to order by + /// Flag to indicate when ordering by system field + /// Search text filter + /// A list of content type Ids to filter the list by + /// An Enumerable list of objects + public IEnumerable GetPagedChildren(int id, long pageIndex, int pageSize, out long totalChildren, + string orderBy, Direction orderDirection, bool orderBySystemField, string filter, int[] contentTypeFilter) { Mandate.ParameterCondition(pageIndex >= 0, "pageIndex"); Mandate.ParameterCondition(pageSize > 0, "pageSize"); @@ -443,6 +463,11 @@ namespace Umbraco.Core.Services var query = Query.Builder; query.Where(x => x.ParentId == id); + if (contentTypeFilter != null && contentTypeFilter.Length > 0) + { + query.Where(x => contentTypeFilter.Contains(x.ContentTypeId)); + } + var medias = repository.GetPagedResultsByQuery(query, pageIndex, pageSize, out totalChildren, orderBy, orderDirection, orderBySystemField, filter); return medias; diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 2e8fc0fd39..4aca6bdf02 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -464,6 +464,7 @@ + @@ -649,7 +650,6 @@ - diff --git a/src/Umbraco.Tests.Benchmarks/ModelToSqlExpressionHelperBenchmarks.cs b/src/Umbraco.Tests.Benchmarks/ModelToSqlExpressionHelperBenchmarks.cs index 03aa3206ee..5fa395d2f1 100644 --- a/src/Umbraco.Tests.Benchmarks/ModelToSqlExpressionHelperBenchmarks.cs +++ b/src/Umbraco.Tests.Benchmarks/ModelToSqlExpressionHelperBenchmarks.cs @@ -16,6 +16,7 @@ using BenchmarkDotNet.Loggers; using BenchmarkDotNet.Reports; using BenchmarkDotNet.Running; using BenchmarkDotNet.Validators; +using Moq; using Umbraco.Core; using Umbraco.Core.Logging; using Umbraco.Core.Models; @@ -43,15 +44,18 @@ namespace Umbraco.Tests.Benchmarks public ModelToSqlExpressionHelperBenchmarks() { - _contentMapper = new ContentMapper(_syntaxProvider); - _contentMapper.BuildMap(); + var contentMapper = new ContentMapper(_syntaxProvider); + contentMapper.BuildMap(); _cachedExpression = new CachedExpression(); + var mappingResolver = new Mock(); + mappingResolver.Setup(resolver => resolver.ResolveMapperByType(It.IsAny())).Returns(contentMapper); + _mappingResolver = mappingResolver.Object; } private readonly ISqlSyntaxProvider _syntaxProvider = new SqlCeSyntaxProvider(); - private readonly BaseMapper _contentMapper; private readonly CachedExpression _cachedExpression; - + private readonly MappingResolver _mappingResolver; + [Benchmark(Baseline = true)] public void WithNonCached() { @@ -62,7 +66,7 @@ namespace Umbraco.Tests.Benchmarks Expression> predicate = content => content.Path.StartsWith("-1") && content.Published && (content.ContentTypeId == a || content.ContentTypeId == b); - var modelToSqlExpressionHelper = new ModelToSqlExpressionVisitor(_syntaxProvider, _contentMapper); + var modelToSqlExpressionHelper = new ModelToSqlExpressionVisitor(_syntaxProvider, _mappingResolver); var result = modelToSqlExpressionHelper.Visit(predicate); } @@ -80,7 +84,7 @@ namespace Umbraco.Tests.Benchmarks Expression> predicate = content => content.Path.StartsWith("-1") && content.Published && (content.ContentTypeId == a || content.ContentTypeId == b); - var modelToSqlExpressionHelper = new ModelToSqlExpressionVisitor(_syntaxProvider, _contentMapper); + var modelToSqlExpressionHelper = new ModelToSqlExpressionVisitor(_syntaxProvider, _mappingResolver); //wrap it! _cachedExpression.Wrap(predicate); diff --git a/src/Umbraco.Tests.Benchmarks/Umbraco.Tests.Benchmarks.csproj b/src/Umbraco.Tests.Benchmarks/Umbraco.Tests.Benchmarks.csproj index 50328b2ecf..46f9dfde10 100644 --- a/src/Umbraco.Tests.Benchmarks/Umbraco.Tests.Benchmarks.csproj +++ b/src/Umbraco.Tests.Benchmarks/Umbraco.Tests.Benchmarks.csproj @@ -49,6 +49,9 @@ ..\packages\BenchmarkDotNet.Toolchains.Roslyn.0.9.9\lib\net45\BenchmarkDotNet.Toolchains.Roslyn.dll + + ..\packages\Castle.Core.4.0.0\lib\net45\Castle.Core.dll + ..\packages\Microsoft.CodeAnalysis.Common.1.3.2\lib\net45\Microsoft.CodeAnalysis.dll @@ -58,6 +61,9 @@ ..\packages\Microsoft.Diagnostics.Tracing.TraceEvent.1.0.41\lib\net40\Microsoft.Diagnostics.Tracing.TraceEvent.dll + + ..\packages\Moq.4.7.0\lib\net45\Moq.dll + ..\packages\System.Collections.Immutable.1.1.37\lib\dotnet\System.Collections.Immutable.dll diff --git a/src/Umbraco.Tests.Benchmarks/packages.config b/src/Umbraco.Tests.Benchmarks/packages.config index c4d2ba1df2..ec400d2ae8 100644 --- a/src/Umbraco.Tests.Benchmarks/packages.config +++ b/src/Umbraco.Tests.Benchmarks/packages.config @@ -4,11 +4,13 @@ + + diff --git a/src/Umbraco.Tests/Persistence/Querying/ExpressionTests.cs b/src/Umbraco.Tests/Persistence/Querying/ExpressionTests.cs index 65a660146e..b9869005f7 100644 --- a/src/Umbraco.Tests/Persistence/Querying/ExpressionTests.cs +++ b/src/Umbraco.Tests/Persistence/Querying/ExpressionTests.cs @@ -1,5 +1,6 @@ using System; using System.Diagnostics; +using System.Linq; using System.Linq.Expressions; using Moq; using NUnit.Framework; @@ -16,19 +17,35 @@ namespace Umbraco.Tests.Persistence.Querying [TestFixture] public class ExpressionTests : BaseUsingSqlCeSyntax { - // [Test] - // public void Can_Query_With_Content_Type_Alias() - // { - // //Arrange - // Expression> predicate = content => content.ContentType.Alias == "Test"; - // var modelToSqlExpressionHelper = new ModelToSqlExpressionVisitor(); - // var result = modelToSqlExpressionHelper.Visit(predicate); + [Test] + public void Can_Query_With_Content_Type_Alias() + { + //Arrange + Expression> predicate = content => content.ContentType.Alias == "Test"; + var modelToSqlExpressionHelper = new ModelToSqlExpressionVisitor(); + var result = modelToSqlExpressionHelper.Visit(predicate); - // Debug.Print("Model to Sql ExpressionHelper: \n" + result); + Debug.Print("Model to Sql ExpressionHelper: \n" + result); - // Assert.AreEqual("[cmsContentType].[alias] = @0", result); - // Assert.AreEqual("Test", modelToSqlExpressionHelper.GetSqlParameters()[0]); - // } + Assert.AreEqual("([cmsContentType].[alias] = @0)", result); + Assert.AreEqual("Test", modelToSqlExpressionHelper.GetSqlParameters()[0]); + } + + [Test] + public void Can_Query_With_Content_Type_Aliases() + { + //Arrange + var aliases = new[] {"Test1", "Test2"}; + Expression> predicate = content => aliases.Contains(content.ContentType.Alias); + var modelToSqlExpressionHelper = new ModelToSqlExpressionVisitor(); + var result = modelToSqlExpressionHelper.Visit(predicate); + + Debug.Print("Model to Sql ExpressionHelper: \n" + result); + + Assert.AreEqual("[cmsContentType].[alias] IN (@1,@2)", result); + Assert.AreEqual("Test1", modelToSqlExpressionHelper.GetSqlParameters()[1]); + Assert.AreEqual("Test2", modelToSqlExpressionHelper.GetSqlParameters()[2]); + } [Test] public void CachedExpression_Can_Verify_Path_StartsWith_Predicate_In_Same_Result() diff --git a/src/Umbraco.Tests/Persistence/Repositories/MediaRepositoryTest.cs b/src/Umbraco.Tests/Persistence/Repositories/MediaRepositoryTest.cs index 1566441d5a..70e72eca9b 100644 --- a/src/Umbraco.Tests/Persistence/Repositories/MediaRepositoryTest.cs +++ b/src/Umbraco.Tests/Persistence/Repositories/MediaRepositoryTest.cs @@ -4,6 +4,7 @@ using System.Linq; using System.Xml.Linq; using Moq; using NUnit.Framework; +using Umbraco.Core; using Umbraco.Core.Configuration.UmbracoSettings; using Umbraco.Core.Logging; using Umbraco.Core.Models; @@ -345,6 +346,61 @@ namespace Umbraco.Tests.Persistence.Repositories } } + [Test] + public void Can_Perform_GetByQuery_On_MediaRepository_With_ContentType_Id_Filter() + { + // Arrange + var folderMediaType = ServiceContext.ContentTypeService.GetMediaType(1031); + var provider = new PetaPocoUnitOfWorkProvider(Logger); + var unitOfWork = provider.GetUnitOfWork(); + MediaTypeRepository mediaTypeRepository; + using (var repository = CreateRepository(unitOfWork, out mediaTypeRepository)) + { + // Act + for (int i = 0; i < 10; i++) + { + var folder = MockedMedia.CreateMediaFolder(folderMediaType, -1); + repository.AddOrUpdate(folder); + } + unitOfWork.Commit(); + + var types = new[] { 1031 }; + var query = Query.Builder.Where(x => types.Contains(x.ContentTypeId)); + var result = repository.GetByQuery(query); + + // Assert + Assert.That(result.Count(), Is.GreaterThanOrEqualTo(11)); + } + } + + [Ignore("We could allow this to work but it requires an extra join on the query used which currently we don't absolutely need so leaving this out for now")] + [Test] + public void Can_Perform_GetByQuery_On_MediaRepository_With_ContentType_Alias_Filter() + { + // Arrange + var folderMediaType = ServiceContext.ContentTypeService.GetMediaType(1031); + var provider = new PetaPocoUnitOfWorkProvider(Logger); + var unitOfWork = provider.GetUnitOfWork(); + MediaTypeRepository mediaTypeRepository; + using (var repository = CreateRepository(unitOfWork, out mediaTypeRepository)) + { + // Act + for (int i = 0; i < 10; i++) + { + var folder = MockedMedia.CreateMediaFolder(folderMediaType, -1); + repository.AddOrUpdate(folder); + } + unitOfWork.Commit(); + + var types = new[] { "Folder" }; + var query = Query.Builder.Where(x => types.Contains(x.ContentType.Alias)); + var result = repository.GetByQuery(query); + + // Assert + Assert.That(result.Count(), Is.GreaterThanOrEqualTo(11)); + } + } + [Test] public void Can_Perform_GetPagedResultsByQuery_ForFirstPage_On_MediaRepository() { @@ -470,7 +526,7 @@ namespace Umbraco.Tests.Persistence.Repositories Assert.That(result.First().Name, Is.EqualTo("Test File")); } } - + [Test] public void Can_Perform_GetPagedResultsByQuery_WithFilterMatchingAll_On_MediaRepository() { diff --git a/src/Umbraco.Tests/Services/MediaServiceTests.cs b/src/Umbraco.Tests/Services/MediaServiceTests.cs index 8193df911c..82738a26f4 100644 --- a/src/Umbraco.Tests/Services/MediaServiceTests.cs +++ b/src/Umbraco.Tests/Services/MediaServiceTests.cs @@ -7,6 +7,7 @@ using Umbraco.Core; using Umbraco.Core.Models; using Umbraco.Core.Models.Rdbms; using Umbraco.Core.Persistence; +using Umbraco.Core.Persistence.DatabaseModelDefinitions; using Umbraco.Core.Persistence.Repositories; using Umbraco.Core.Persistence.UnitOfWork; using Umbraco.Tests.TestHelpers; @@ -30,6 +31,33 @@ namespace Umbraco.Tests.Services base.TearDown(); } + [Test] + public void Get_Paged_Children_With_Media_Type_Filter() + { + var mediaService = ServiceContext.MediaService; + var mediaType1 = MockedContentTypes.CreateImageMediaType("Image2"); + ServiceContext.ContentTypeService.Save(mediaType1); + var mediaType2 = MockedContentTypes.CreateImageMediaType("Image3"); + ServiceContext.ContentTypeService.Save(mediaType2); + + for (int i = 0; i < 10; i++) + { + var m1 = MockedMedia.CreateMediaImage(mediaType1, -1); + mediaService.Save(m1); + var m2 = MockedMedia.CreateMediaImage(mediaType2, -1); + mediaService.Save(m2); + } + + long total; + var result = ServiceContext.MediaService.GetPagedChildren(-1, 0, 11, out total, "SortOrder", Direction.Ascending, true, null, new[] {mediaType1.Id, mediaType2.Id}); + Assert.AreEqual(11, result.Count()); + Assert.AreEqual(20, total); + + result = ServiceContext.MediaService.GetPagedChildren(-1, 1, 11, out total, "SortOrder", Direction.Ascending, true, null, new[] { mediaType1.Id, mediaType2.Id }); + Assert.AreEqual(9, result.Count()); + Assert.AreEqual(20, total); + } + [Test] public void Can_Move_Media() { diff --git a/src/Umbraco.Web.UI.Client/src/common/resources/media.resource.js b/src/Umbraco.Web.UI.Client/src/common/resources/media.resource.js index c0aee7280d..c41b7e91c7 100644 --- a/src/Umbraco.Web.UI.Client/src/common/resources/media.resource.js +++ b/src/Umbraco.Web.UI.Client/src/common/resources/media.resource.js @@ -427,7 +427,9 @@ function mediaResource($q, $http, umbDataFormatter, umbRequestHelper) { * Retrieves all media children with types used as folders. * Uses the convention of looking for media items with mediaTypes ending in * *Folder so will match "Folder", "bannerFolder", "secureFolder" etc, - * + * + * NOTE: This will return a max of 500 folders, if more is required it needs to be paged + * * ##usage * 
           * mediaResource.getChildFolders(1234)
@@ -438,21 +440,22 @@ function mediaResource($q, $http, umbDataFormatter, umbRequestHelper) {
           *
           * @param {int} parentId Id of the media item to query for child folders    
           * @returns {Promise} resourcePromise object.
-          * @deprecated This method is no longer used and shouldn't be because it performs poorly when there are a lot of media items
+          *
           */
         getChildFolders: function (parentId) {
             if (!parentId) {
                 parentId = -1;
             }
 
+            //NOTE: This will return a max of 500 folders, if more is required it needs to be paged
             return umbRequestHelper.resourcePromise(
                   $http.get(
                         umbRequestHelper.getApiUrl(
                               "mediaApiBaseUrl",
                               "GetChildFolders",
-                              [
-                                    { id: parentId }
-                              ])),
+                            {
+                                id: parentId
+                            })),
                   'Failed to retrieve child folders for media item ' + parentId);
         },
 
diff --git a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/listview/listview.controller.js b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/listview/listview.controller.js
index 40ab5ce35f..f8887c6c44 100644
--- a/src/Umbraco.Web.UI.Client/src/views/propertyeditors/listview/listview.controller.js
+++ b/src/Umbraco.Web.UI.Client/src/views/propertyeditors/listview/listview.controller.js
@@ -1,4 +1,4 @@
-function listViewController($rootScope, $scope, $routeParams, $injector, $cookieStore, mediaTypeHelper, notificationsService, iconHelper, dialogService, editorState, localizationService, $location, appState, $timeout, $q, mediaResource, listViewHelper, userService, navigationService, treeService) {
+function listViewController($rootScope, $scope, $routeParams, $injector, $cookieStore, notificationsService, iconHelper, dialogService, editorState, localizationService, $location, appState, $timeout, $q, mediaResource, listViewHelper, userService, navigationService, treeService) {
 
    //this is a quick check to see if we're in create mode, if so just exit - we cannot show children for content
    // that isn't created yet, if we continue this will use the parent id in the route params which isn't what
@@ -53,7 +53,9 @@ function listViewController($rootScope, $scope, $routeParams, $injector, $cookie
    $scope.isNew = false;
    $scope.actionInProgress = false;
    $scope.selection = [];
-   $scope.folders = [];
+   $scope.folders = [];   
+   //tracks if we've already loaded the folders for the current node
+   var foldersLoaded = false;
    $scope.listViewResultSet = {
       totalPages: 0,
       items: []
@@ -261,25 +263,25 @@ function listViewController($rootScope, $scope, $routeParams, $injector, $cookie
          $scope.actionInProgress = false;
          $scope.listViewResultSet = data;
 
-          //reset
-         $scope.folders = [];
-
-          //update all values for display
+         //update all values for display
          if ($scope.listViewResultSet.items) {
             _.each($scope.listViewResultSet.items, function (e, index) {
-                setPropertyValues(e);
-
-                //special case, we need to check if any of these types are folder types 
-                //and add them to the folders collection
-                if ($scope.entityType === 'media') {
-                    if (mediaTypeHelper.isFolderType(e)) {
-                        $scope.folders.push(e);
-                    }
-                }
+               setPropertyValues(e);
             });
          }
 
-         $scope.viewLoaded = true;
+         if (!foldersLoaded && $scope.entityType === 'media') {
+            //The folders aren't loaded - we only need to do this once since we're never changing node ids
+            mediaResource.getChildFolders($scope.contentId)
+                    .then(function (folders) {
+                       $scope.folders = folders;
+                       $scope.viewLoaded = true;
+                       foldersLoaded = true;
+                    });
+
+         } else {
+            $scope.viewLoaded = true;
+         }
 
          //NOTE: This might occur if we are requesting a higher page number than what is actually available, for example
          // if you have more than one page and you delete all items on the last page. In this case, we need to reset to the last
diff --git a/src/Umbraco.Web/Editors/MediaController.cs b/src/Umbraco.Web/Editors/MediaController.cs
index d00a26333e..2f3fe5bd94 100644
--- a/src/Umbraco.Web/Editors/MediaController.cs
+++ b/src/Umbraco.Web/Editors/MediaController.cs
@@ -167,18 +167,41 @@ namespace Umbraco.Web.Editors
         [Obsolete("This is no longer used and shouldn't be because it performs poorly when there are a lot of media items")]
         [FilterAllowedOutgoingMedia(typeof(IEnumerable>))]
         public IEnumerable> GetChildFolders(int id = -1)
+        {
+            //we are only allowing a max of 500 to be returned here, if more is required it needs to be paged
+            var result = GetChildFolders(id, 1, 500);
+            return result.Items;
+        }
+
+        /// 
+        /// Returns a paged result of media items known to be of a "Folder" type
+        /// 
+        /// 
+        /// 
+        /// 
+        /// 
+        public PagedResult> GetChildFolders(int id, int pageNumber, int pageSize)
         {
             //Suggested convention for folder mediatypes - we can make this more or less complicated as long as we document it...
             //if you create a media type, which has an alias that ends with ...Folder then its a folder: ex: "secureFolder", "bannerFolder", "Folder"
             var folderTypes = Services.ContentTypeService
-                .GetAllContentTypeAliases(Constants.ObjectTypes.MediaTypeGuid)
-                .Where(x => x.EndsWith("Folder"));
-            
-            var children = (id < 0) 
-                ? Services.MediaService.GetRootMedia()
-                : Services.MediaService.GetChildren(id);
+                .GetAllMediaTypes()
+                .Where(x => x.Alias.EndsWith("Folder"))
+                .Select(x => x.Id)
+                .ToArray();
 
-            return children.Where(x => folderTypes.Contains(x.ContentType.Alias)).Select(Mapper.Map>);
+            if (folderTypes.Length == 0)
+            {
+                return new PagedResult>(0, pageNumber, pageSize);
+            }
+
+            long total;
+            var children = Services.MediaService.GetPagedChildren(id, pageNumber - 1, pageSize, out total, "Name", Direction.Ascending, true, null, folderTypes.ToArray());
+            
+            return new PagedResult>(total, pageNumber, pageSize)
+            {
+                Items = children.Select(Mapper.Map>)
+            };
         }
 
         ///