From efbcce526154b32c97c21b5b7eb2a28fa9ba90b0 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 21 Aug 2018 10:41:50 +1000 Subject: [PATCH] Fixes SQL generation for paged results for both UserRepository and AuditRepository --- .../Repositories/AuditRepository.cs | 37 +++----- .../Repositories/UserRepository.cs | 93 +++++++++++-------- .../Repositories/VersionableRepositoryBase.cs | 5 +- .../Repositories/AuditRepositoryTest.cs | 90 +++++++++++++++--- .../Repositories/UserRepositoryTest.cs | 69 +++++++++++++- 5 files changed, 215 insertions(+), 79 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/AuditRepository.cs b/src/Umbraco.Core/Persistence/Repositories/AuditRepository.cs index f7c8b0d3cd..e602358cf6 100644 --- a/src/Umbraco.Core/Persistence/Repositories/AuditRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/AuditRepository.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Text.RegularExpressions; using Umbraco.Core.Logging; using Umbraco.Core.Models; using Umbraco.Core.Models.Membership; @@ -47,43 +48,32 @@ namespace Umbraco.Core.Persistence.Repositories var sql = GetBaseQuery(false); if (query == null) query = new Query(); + var queryHasWhereClause = query.GetWhereClauses().Any(); var translatorIds = new SqlTranslator(sql, query); var translatedQuery = translatorIds.Translate(); - var customFilterWheres = customFilter != null ? customFilter.GetWhereClauses().ToArray() : null; + var customFilterWheres = customFilter?.GetWhereClauses().ToArray(); var hasCustomFilter = customFilterWheres != null && customFilterWheres.Length > 0; if (hasCustomFilter) { var filterSql = new Sql(); - var first = true; - foreach (var filterClaus in customFilterWheres) + foreach (var filterClause in customFilterWheres) { - if (first == false) - { - filterSql.Append(" AND "); - } - filterSql.Append(string.Format("({0})", filterClaus.Item1), filterClaus.Item2); - first = false; + filterSql.Append($"AND ({filterClause.Item1})", filterClause.Item2); } - translatedQuery = GetFilteredSqlForPagedResults(translatedQuery, filterSql); + translatedQuery = GetFilteredSqlForPagedResults(translatedQuery, filterSql, queryHasWhereClause); } if (auditTypeFilter.Length > 0) { var filterSql = new Sql(); - var first = true; - foreach (var filterClaus in auditTypeFilter) + foreach (var filterClause in auditTypeFilter) { - if (first == false || hasCustomFilter) - { - filterSql.Append(" AND "); - } - filterSql.Append("(logHeader = @logHeader)", new {logHeader = filterClaus.ToString() }); - first = false; + filterSql.Append("AND (logHeader = @logHeader)", new { logHeader = filterClause.ToString() }); } - translatedQuery = GetFilteredSqlForPagedResults(translatedQuery, filterSql); + translatedQuery = GetFilteredSqlForPagedResults(translatedQuery, filterSql, queryHasWhereClause || hasCustomFilter); } if (orderDirection == Direction.Descending) @@ -99,7 +89,7 @@ namespace Umbraco.Core.Persistence.Repositories dto => new AuditItem(dto.Id, dto.Comment, Enum.ParseOrNull(dto.Header) ?? AuditType.Custom, dto.UserId)).ToArray(); //Mapping the DateStamp - for (int i = 0; i < pages.Length; i++) + for (var i = 0; i < pages.Length; i++) { pages[i].CreateDate = pagedResult.Items[i].Datestamp; } @@ -169,14 +159,17 @@ namespace Umbraco.Core.Persistence.Repositories } #endregion - private Sql GetFilteredSqlForPagedResults(Sql sql, Sql filterSql) + private Sql GetFilteredSqlForPagedResults(Sql sql, Sql filterSql, bool hasWhereClause) { Sql filteredSql; // Apply filter if (filterSql != null) { - var sqlFilter = " WHERE " + filterSql.SQL.TrimStart("AND "); + //ensure we don't append a WHERE if there is already one + var sqlFilter = hasWhereClause + ? filterSql.SQL + : " WHERE " + filterSql.SQL.TrimStart("AND "); //NOTE: this is certainly strange - NPoco handles this much better but we need to re-create the sql // instance a couple of times to get the parameter order correct, for some reason the first diff --git a/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs b/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs index 5701cd1008..82ad1fdcbf 100644 --- a/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs @@ -4,6 +4,7 @@ using System.ComponentModel; using System.Linq; using System.Linq.Expressions; using System.Text; +using System.Text.RegularExpressions; using System.Web.Security; using Newtonsoft.Json; using Umbraco.Core.Configuration; @@ -707,10 +708,10 @@ ORDER BY colName"; - private IEnumerable GetPagedResultsByQuery(IQuery query, long pageIndex, int pageSize, out long totalRecords, string orderBy, Direction orderDirection, - string[] includeUserGroups = null, - string[] excludeUserGroups = null, - UserState[] userState = null, + private IEnumerable GetPagedResultsByQuery(IQuery query, long pageIndex, int pageSize, out long totalRecords, string orderBy, Direction orderDirection, + string[] includeUserGroups = null, + string[] excludeUserGroups = null, + UserState[] userState = null, IQuery customFilter = null) { if (string.IsNullOrWhiteSpace(orderBy)) throw new ArgumentException("Value cannot be null or whitespace.", "orderBy"); @@ -718,23 +719,23 @@ ORDER BY colName"; Sql filterSql = null; var customFilterWheres = customFilter != null ? customFilter.GetWhereClauses().ToArray() : null; - var hasCustomFilter = customFilterWheres != null && customFilterWheres.Length > 0; + var hasCustomFilter = customFilterWheres != null && customFilterWheres.Length > 0; if (hasCustomFilter - || (includeUserGroups != null && includeUserGroups.Length > 0) || (excludeUserGroups != null && excludeUserGroups.Length > 0) - || (userState != null && userState.Length > 0 && userState.Contains(UserState.All) == false)) - filterSql = new Sql(); + || (includeUserGroups != null && includeUserGroups.Length > 0) || (excludeUserGroups != null && excludeUserGroups.Length > 0) + || (userState != null && userState.Length > 0 && userState.Contains(UserState.All) == false)) + filterSql = new Sql(); if (hasCustomFilter) { foreach (var filterClause in customFilterWheres) { - filterSql.Append(string.Format("AND ({0})", filterClause.Item1), filterClause.Item2); + filterSql.Append($"AND ({filterClause.Item1})", filterClause.Item2); } - } + } if (includeUserGroups != null && includeUserGroups.Length > 0) { - var subQuery = @"AND (umbracoUser.id IN (SELECT DISTINCT umbracoUser.id + const string subQuery = @"AND (umbracoUser.id IN (SELECT DISTINCT umbracoUser.id FROM umbracoUser INNER JOIN umbracoUser2UserGroup ON umbracoUser2UserGroup.userId = umbracoUser.id INNER JOIN umbracoUserGroup ON umbracoUserGroup.id = umbracoUser2UserGroup.userGroupId @@ -744,21 +745,21 @@ ORDER BY colName"; if (excludeUserGroups != null && excludeUserGroups.Length > 0) { - var subQuery = @"AND (umbracoUser.id NOT IN (SELECT DISTINCT umbracoUser.id + const string subQuery = @"AND (umbracoUser.id NOT IN (SELECT DISTINCT umbracoUser.id FROM umbracoUser INNER JOIN umbracoUser2UserGroup ON umbracoUser2UserGroup.userId = umbracoUser.id INNER JOIN umbracoUserGroup ON umbracoUserGroup.id = umbracoUser2UserGroup.userGroupId WHERE umbracoUserGroup.userGroupAlias IN (@userGroups)))"; filterSql.Append(subQuery, new { userGroups = excludeUserGroups }); - } + } if (userState != null && userState.Length > 0) - { + { //the "ALL" state doesn't require any filtering so we ignore that, if it exists in the list we don't do any filtering if (userState.Contains(UserState.All) == false) { var sb = new StringBuilder("("); - var appended = false; + var appended = false; if (userState.Contains(UserState.Active)) { @@ -788,67 +789,81 @@ ORDER BY colName"; filterSql.Append("AND " + sb); } - } - - // Get base query for returning IDs - var sqlBaseIds = GetBaseQuery("id"); - + } + + // Get base query for returning IDs + var sqlBaseIds = GetBaseQuery("id"); + if (query == null) query = new Query(); + var queryHasWhereClause = query.GetWhereClauses().Any(); var translatorIds = new SqlTranslator(sqlBaseIds, query); - var sqlQueryIds = translatorIds.Translate(); - - //get sorted and filtered sql + var sqlQueryIds = translatorIds.Translate(); + var sqlBaseFull = GetBaseQuery("umbracoUser.*, umbracoUserGroup.*, umbracoUserGroup2App.*, umbracoUserStartNode.*"); + var translatorFull = new SqlTranslator(sqlBaseFull, query); + + //get sorted and filtered sql var sqlNodeIdsWithSort = GetSortedSqlForPagedResults( - GetFilteredSqlForPagedResults(sqlQueryIds, filterSql), + GetFilteredSqlForPagedResults(sqlQueryIds, filterSql, queryHasWhereClause), orderDirection, orderBy); // Get page of results and total count var pagedResult = Database.Page(pageIndex + 1, pageSize, sqlNodeIdsWithSort); - totalRecords = Convert.ToInt32(pagedResult.TotalItems); - - //NOTE: We need to check the actual items returned, not the 'totalRecords', that is because if you request a page number - // that doesn't actually have any data on it, the totalRecords will still indicate there are records but there are none in - // the pageResult. + totalRecords = Convert.ToInt32(pagedResult.TotalItems); + + //NOTE: We need to check the actual items returned, not the 'totalRecords', that is because if you request a page number + // that doesn't actually have any data on it, the totalRecords will still indicate there are records but there are none in + // the pageResult. if (pagedResult.Items.Any()) { //Create the inner paged query that was used above to get the paged result, we'll use that as the inner sub query var args = sqlNodeIdsWithSort.Arguments; string sqlStringCount, sqlStringPage; Database.BuildPageQueries(pageIndex * pageSize, pageSize, sqlNodeIdsWithSort.SQL, ref args, out sqlStringCount, out sqlStringPage); + + var sqlQueryFull = translatorFull.Translate(); - var sqlQueryFull = GetBaseQuery("umbracoUser.*, umbracoUserGroup.*, umbracoUserGroup2App.*, umbracoUserStartNode.*"); - - var fullQueryWithPagedInnerJoin = sqlQueryFull - .Append("INNER JOIN (") - //join the paged query with the paged query arguments + //We need to make this FULL query an inner join on the paged ID query + var splitQuery = sqlQueryFull.SQL.Split(new[] { "WHERE " }, StringSplitOptions.None); + var fullQueryWithPagedInnerJoin = new Sql(splitQuery[0]) + .Append("INNER JOIN (") + //join the paged query with the paged query arguments .Append(sqlStringPage, args) .Append(") temp ") .Append("ON umbracoUser.id = temp.id"); AddGroupLeftJoin(fullQueryWithPagedInnerJoin); + if (splitQuery.Length > 1) + { + //add the original where clause back with the original arguments + fullQueryWithPagedInnerJoin.Where(splitQuery[1], sqlQueryIds.Arguments); + } + //get sorted and filtered sql var fullQuery = GetSortedSqlForPagedResults( - GetFilteredSqlForPagedResults(fullQueryWithPagedInnerJoin, filterSql), + GetFilteredSqlForPagedResults(fullQueryWithPagedInnerJoin, filterSql, queryHasWhereClause), orderDirection, orderBy); var users = ConvertFromDtos(Database.Fetch(new UserGroupRelator().Map, fullQuery)) .ToArray(); // important so we don't iterate twice, if we don't do this we can end up with null values in cache if we were caching. return users; - } + } return Enumerable.Empty(); } - private Sql GetFilteredSqlForPagedResults(Sql sql, Sql filterSql) + private Sql GetFilteredSqlForPagedResults(Sql sql, Sql filterSql, bool hasWhereClause) { Sql filteredSql; // Apply filter if (filterSql != null) { - var sqlFilter = " WHERE " + filterSql.SQL.TrimStart("AND "); + //ensure we don't append a WHERE if there is already one + var sqlFilter = hasWhereClause + ? filterSql.SQL + : " WHERE " + filterSql.SQL.TrimStart("AND "); //NOTE: this is certainly strange - NPoco handles this much better but we need to re-create the sql // instance a couple of times to get the parameter order correct, for some reason the first @@ -915,4 +930,4 @@ ORDER BY colName"; } } -} \ No newline at end of file +} diff --git a/src/Umbraco.Core/Persistence/Repositories/VersionableRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/VersionableRepositoryBase.cs index 883b05b2fd..aa7566a4c3 100644 --- a/src/Umbraco.Core/Persistence/Repositories/VersionableRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/VersionableRepositoryBase.cs @@ -290,6 +290,9 @@ namespace Umbraco.Core.Persistence.Repositories // Apply filter if (defaultFilter != null) { + //NOTE: It is assumed here that the `sql` already contains a WHERE clause, see UserRepository.GetFilteredSqlForPagedResults + // for an example of when it's not assumed there's already a WHERE clause + var filterResult = defaultFilter(); //NOTE: this is certainly strange - NPoco handles this much better but we need to re-create the sql @@ -993,4 +996,4 @@ ORDER BY contentNodeId, versionId, propertytypeid } } } -} \ No newline at end of file +} diff --git a/src/Umbraco.Tests/Persistence/Repositories/AuditRepositoryTest.cs b/src/Umbraco.Tests/Persistence/Repositories/AuditRepositoryTest.cs index 04677c208b..e6f8f44019 100644 --- a/src/Umbraco.Tests/Persistence/Repositories/AuditRepositoryTest.cs +++ b/src/Umbraco.Tests/Persistence/Repositories/AuditRepositoryTest.cs @@ -66,6 +66,46 @@ namespace Umbraco.Tests.Persistence.Repositories } } + [Test] + public void Get_Paged_Items_By_User_Id_With_Query_And_Filter() + { + var provider = new PetaPocoUnitOfWorkProvider(Logger); + var unitOfWork = provider.GetUnitOfWork(); + using (var repo = new AuditRepository(unitOfWork, CacheHelper, Logger, SqlSyntax)) + { + for (int i = 0; i < 100; i++) + { + repo.AddOrUpdate(new AuditItem(i, string.Format("Content {0} created", i), AuditType.New, 0)); + repo.AddOrUpdate(new AuditItem(i, string.Format("Content {0} published", i), AuditType.Publish, 0)); + } + unitOfWork.Commit(); + } + + using (var repo = new AuditRepository(unitOfWork, CacheHelper, Logger, SqlSyntax)) + { + var query = Query.Builder.Where(x => x.UserId == 0); + + try + { + DatabaseContext.Database.EnableSqlTrace = true; + DatabaseContext.Database.EnableSqlCount(); + + var page = repo.GetPagedResultsByQuery(query, 0, 10, out var total, Direction.Descending, + new[] { AuditType.Publish }, + Query.Builder.Where(x => x.UserId > -1)); + + Assert.AreEqual(10, page.Count()); + Assert.AreEqual(100, total); + } + finally + { + DatabaseContext.Database.EnableSqlTrace = false; + DatabaseContext.Database.DisableSqlCount(); + } + } + } + + [Test] public void Get_Paged_Items_With_AuditType_Filter() { @@ -83,14 +123,24 @@ namespace Umbraco.Tests.Persistence.Repositories using (var repo = new AuditRepository(unitOfWork, CacheHelper, Logger, SqlSyntax)) { - long total; - var page = repo.GetPagedResultsByQuery(Query.Builder, 0, 9, out total, Direction.Descending, - new[] {AuditType.Publish}, null) - .ToArray(); + try + { + DatabaseContext.Database.EnableSqlTrace = true; + DatabaseContext.Database.EnableSqlCount(); - Assert.AreEqual(9, page.Length); - Assert.IsTrue(page.All(x => x.AuditType == AuditType.Publish)); - Assert.AreEqual(100, total); + var page = repo.GetPagedResultsByQuery(Query.Builder, 0, 9, out var total, Direction.Descending, + new[] { AuditType.Publish }, null) + .ToArray(); + + Assert.AreEqual(9, page.Length); + Assert.IsTrue(page.All(x => x.AuditType == AuditType.Publish)); + Assert.AreEqual(100, total); + } + finally + { + DatabaseContext.Database.EnableSqlTrace = false; + DatabaseContext.Database.DisableSqlCount(); + } } } @@ -111,15 +161,25 @@ namespace Umbraco.Tests.Persistence.Repositories using (var repo = new AuditRepository(unitOfWork, CacheHelper, Logger, SqlSyntax)) { - long total; - var page = repo.GetPagedResultsByQuery(Query.Builder, 0, 8, out total, Direction.Descending, - null, Query.Builder.Where(item => item.Comment == "Content created")) - .ToArray(); + try + { + DatabaseContext.Database.EnableSqlTrace = true; + DatabaseContext.Database.EnableSqlCount(); - Assert.AreEqual(8, page.Length); - Assert.IsTrue(page.All(x => x.Comment == "Content created")); - Assert.AreEqual(100, total); + var page = repo.GetPagedResultsByQuery(Query.Builder, 0, 8, out var total, Direction.Descending, + null, Query.Builder.Where(item => item.Comment == "Content created")) + .ToArray(); + + Assert.AreEqual(8, page.Length); + Assert.IsTrue(page.All(x => x.Comment == "Content created")); + Assert.AreEqual(100, total); + } + finally + { + DatabaseContext.Database.EnableSqlTrace = false; + DatabaseContext.Database.DisableSqlCount(); + } } } } -} \ No newline at end of file +} diff --git a/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs b/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs index 334c4f98c2..3fa490e742 100644 --- a/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs +++ b/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs @@ -6,6 +6,7 @@ using Umbraco.Core.Configuration.UmbracoSettings; using Umbraco.Core.IO; using Umbraco.Core.Logging; using Umbraco.Core.Models.Membership; +using Umbraco.Core.Persistence.DatabaseModelDefinitions; using Umbraco.Core.Persistence.Querying; using Umbraco.Core.Persistence.Repositories; using Umbraco.Core.Persistence.UnitOfWork; @@ -392,7 +393,71 @@ namespace Umbraco.Tests.Persistence.Repositories var result = repository.Count(query); // Assert - Assert.That(result, Is.GreaterThanOrEqualTo(2)); + Assert.AreEqual(2, result); + } + } + + [Test] + public void Can_Get_Paged_Results_By_Query_And_Filter_And_Groups() + { + // Arrange + var provider = new PetaPocoUnitOfWorkProvider(Logger); + var unitOfWork = provider.GetUnitOfWork(); + using (var repository = CreateRepository(unitOfWork)) + { + var users = CreateAndCommitMultipleUsers(repository, unitOfWork); + var query = Query.Builder.Where(x => x.Username == "TestUser1" || x.Username == "TestUser2"); + + try + { + DatabaseContext.Database.EnableSqlTrace = true; + DatabaseContext.Database.EnableSqlCount(); + + // Act + var result = repository.GetPagedResultsByQuery(query, 0, 10, out var totalRecs, user => user.Id, Direction.Ascending, + excludeUserGroups: new[] { Constants.Security.TranslatorGroupAlias }, + filter: Query.Builder.Where(x => x.Id > -1)); + + // Assert + Assert.AreEqual(2, totalRecs); + } + finally + { + DatabaseContext.Database.EnableSqlTrace = false; + DatabaseContext.Database.DisableSqlCount(); + } + } + } + + [Test] + public void Can_Get_Paged_Results_With_Filter_And_Groups() + { + // Arrange + var provider = new PetaPocoUnitOfWorkProvider(Logger); + var unitOfWork = provider.GetUnitOfWork(); + using (var repository = CreateRepository(unitOfWork)) + { + var users = CreateAndCommitMultipleUsers(repository, unitOfWork); + + try + { + DatabaseContext.Database.EnableSqlTrace = true; + DatabaseContext.Database.EnableSqlCount(); + + // Act + var result = repository.GetPagedResultsByQuery(null, 0, 10, out var totalRecs, user => user.Id, Direction.Ascending, + includeUserGroups: new[] { Constants.Security.AdminGroupAlias, Constants.Security.SensitiveDataGroupAlias }, + excludeUserGroups: new[] { Constants.Security.TranslatorGroupAlias }, + filter: Query.Builder.Where(x => x.Id == 0)); + + // Assert + Assert.AreEqual(1, totalRecs); + } + finally + { + DatabaseContext.Database.EnableSqlTrace = false; + DatabaseContext.Database.DisableSqlCount(); + } } } @@ -439,4 +504,4 @@ namespace Umbraco.Tests.Persistence.Repositories return new IUser[] { user1, user2, user3 }; } } -} \ No newline at end of file +}