From 082d504b331e4d73c3dc5b41a76e2e59be8480f8 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Wed, 10 Apr 2024 12:03:32 +0200 Subject: [PATCH] Fix tracked reference queries for SqlServer (#16020) * Fixed issue with SqlServer and optimized queries to not do the actual paging if total count is 0 * Cleanup --------- Co-authored-by: Elitsa --- .../ITrackedReferencesRepository.cs | 6 +- .../Implement/TrackedReferencesRepository.cs | 155 +++++++++++++----- .../Persistence/UmbracoDatabaseExtensions.cs | 3 +- .../Services/TrackedReferencesServiceTests.cs | 91 ++++++++++ 4 files changed, 209 insertions(+), 46 deletions(-) create mode 100644 tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/TrackedReferencesServiceTests.cs diff --git a/src/Umbraco.Core/Persistence/Repositories/ITrackedReferencesRepository.cs b/src/Umbraco.Core/Persistence/Repositories/ITrackedReferencesRepository.cs index 9466a1fcd8..fda745c2f6 100644 --- a/src/Umbraco.Core/Persistence/Repositories/ITrackedReferencesRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/ITrackedReferencesRepository.cs @@ -68,8 +68,7 @@ public interface ITrackedReferencesRepository long skip, long take, bool filterMustBeIsDependency, - out long totalRecords) => - throw new NotImplementedException(); + out long totalRecords); [Obsolete("Use overload that takes key instead of id. This will be removed in Umbraco 15.")] IEnumerable GetPagedRelationsForItem( @@ -77,8 +76,7 @@ public interface ITrackedReferencesRepository long skip, long take, bool filterMustBeIsDependency, - out long totalRecords) => - throw new NotImplementedException(); + out long totalRecords); /// /// Gets a page of items used in any kind of relation from selected integer ids. diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TrackedReferencesRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TrackedReferencesRepository.cs index 0a6ec43e0d..62d3f3acd6 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TrackedReferencesRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TrackedReferencesRepository.cs @@ -92,7 +92,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement } var innerUnionSqlChild = _scopeAccessor.AmbientScope.Database.SqlContext.Sql().Select( - "[cn].uniqueId as key", "[pn].uniqueId as otherKey, [cr].childId as id", "[cr].parentId as otherId", "[rt].[alias]", "[rt].[name]", + "[cn].uniqueId as [key]", "[pn].uniqueId as otherKey, [cr].childId as id", "[cr].parentId as otherId", "[rt].[alias]", "[rt].[name]", "[rt].[isDependency]", "[rt].[dual]") .From("cr") .InnerJoin("rt") @@ -103,7 +103,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement .On((cr, pn) => cr.ParentId == pn.NodeId, "cr", "pn"); var innerUnionSqlDualParent = _scopeAccessor.AmbientScope.Database.SqlContext.Sql().Select( - "[pn].uniqueId as key", "[cn].uniqueId as otherKey, [dpr].parentId as id", "[dpr].childId as otherId", "[dprt].[alias]", "[dprt].[name]", + "[pn].uniqueId as [key]", "[cn].uniqueId as otherKey, [dpr].parentId as id", "[dpr].childId as otherId", "[dprt].[alias]", "[dprt].[name]", "[dprt].[isDependency]", "[dprt].[dual]") .From("dpr") .InnerJoin("dprt") @@ -115,7 +115,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement .On((dpr, pn) => dpr.ParentId == pn.NodeId, "dpr", "pn"); var innerUnionSql3 = _scopeAccessor.AmbientScope.Database.SqlContext.Sql().Select( - "[cn].uniqueId as key", "[pn].uniqueId as otherKey, [dcr].childId as id", "[dcr].parentId as otherId", "[dcrt].[alias]", "[dcrt].[name]", + "[cn].uniqueId as [key]", "[pn].uniqueId as otherKey, [dcr].childId as id", "[dcr].parentId as otherId", "[dcrt].[alias]", "[dcrt].[name]", "[dcrt].[isDependency]", "[dcrt].[dual]") .From("dcr") .InnerJoin("dcrt") @@ -313,19 +313,32 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement aliasRight: "d") .Where(x => x.Key == key, "x"); + if (filterMustBeIsDependency) { sql = sql?.Where(rt => rt.IsDependency, "x"); } - // Ordering is required for paging - sql = sql?.OrderBy(x => x.Alias, "x"); - - RelationItemDto[] pagedResult = - _scopeAccessor.AmbientScope?.Database.SkipTake(skip, take, sql).ToArray() ?? - Array.Empty(); + // find the count before ordering totalRecords = _scopeAccessor.AmbientScope?.Database.Count(sql!) ?? 0; + RelationItemDto[] pagedResult; + + //Only to all this, if there is items + if (totalRecords > 0) + { + // Ordering is required for paging + sql = sql?.OrderBy(x => x.Alias, "x"); + + pagedResult = + _scopeAccessor.AmbientScope?.Database.SkipTake(skip, take, sql).ToArray() ?? + Array.Empty(); + } + else + { + pagedResult = Array.Empty(); + } + return _umbracoMapper.MapEnumerable(pagedResult); } @@ -368,14 +381,26 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement sql = sql?.Where(rt => rt.IsDependency, "x"); } - // Ordering is required for paging - sql = sql?.OrderBy(x => x.Alias, "x"); - - RelationItemDto[] pagedResult = - _scopeAccessor.AmbientScope?.Database.SkipTake(skip, take, sql).ToArray() ?? - Array.Empty(); + // find the count before ordering totalRecords = _scopeAccessor.AmbientScope?.Database.Count(sql!) ?? 0; + RelationItemDto[] pagedResult; + + //Only to all this, if there is items + if (totalRecords > 0) + { + // Ordering is required for paging + sql = sql?.OrderBy(x => x.Alias, "x"); + + pagedResult = + _scopeAccessor.AmbientScope?.Database.SkipTake(skip, take, sql).ToArray() ?? + Array.Empty(); + } + else + { + pagedResult = Array.Empty(); + } + return _umbracoMapper.MapEnumerable(pagedResult); } @@ -420,15 +445,26 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement sql = sql?.Where(rt => rt.IsDependency, "x"); } - // Ordering is required for paging - sql = sql?.OrderBy(x => x.Alias, "x"); - - RelationItemDto[] pagedResult = - _scopeAccessor.AmbientScope?.Database.SkipTake(skip, take, sql).ToArray() ?? - Array.Empty(); - + // find the count before ordering totalRecords = _scopeAccessor.AmbientScope?.Database.Count(sql!) ?? 0; + RelationItemDto[] pagedResult; + + //Only to all this, if there is items + if (totalRecords > 0) + { + // Ordering is required for paging + sql = sql?.OrderBy(x => x.Alias, "x"); + + pagedResult = + _scopeAccessor.AmbientScope?.Database.SkipTake(skip, take, sql).ToArray() ?? + Array.Empty(); + } + else + { + pagedResult = Array.Empty(); + } + return _umbracoMapper.MapEnumerable(pagedResult); } @@ -487,14 +523,27 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement sql = sql?.Where(rt => rt.IsDependency, "x"); } - // Ordering is required for paging - sql = sql?.OrderBy(x => x.Alias, "x"); - - List? pagedResult = _scopeAccessor.AmbientScope?.Database.SkipTake(skip, take, sql); + // find the count before ordering totalRecords = _scopeAccessor.AmbientScope?.Database.Count(sql!) ?? 0; - return _umbracoMapper.MapEnumerable(pagedResult ?? - new List()); + RelationItemDto[] pagedResult; + + //Only to all this, if there is items + if (totalRecords > 0) + { + // Ordering is required for paging + sql = sql?.OrderBy(x => x.Alias, "x"); + + pagedResult = + _scopeAccessor.AmbientScope?.Database.SkipTake(skip, take, sql).ToArray() ?? + Array.Empty(); + } + else + { + pagedResult = Array.Empty(); + } + + return _umbracoMapper.MapEnumerable(pagedResult); } [Obsolete("Use overload that takes keys instead of ids. This will be removed in Umbraco 15.")] @@ -539,14 +588,26 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement sql = sql?.Where(rt => rt.IsDependency, "x"); } - // Ordering is required for paging - sql = sql?.OrderBy(x => x.Alias, "x"); - - RelationItemDto[] pagedResult = - _scopeAccessor.AmbientScope?.Database.SkipTake(skip, take, sql).ToArray() ?? - Array.Empty(); + // find the count before ordering totalRecords = _scopeAccessor.AmbientScope?.Database.Count(sql!) ?? 0; + RelationItemDto[] pagedResult; + + //Only to all this, if there is items + if (totalRecords > 0) + { + // Ordering is required for paging + sql = sql?.OrderBy(x => x.Alias, "x"); + + pagedResult = + _scopeAccessor.AmbientScope?.Database.SkipTake(skip, take, sql).ToArray() ?? + Array.Empty(); + } + else + { + pagedResult = Array.Empty(); + } + return _umbracoMapper.MapEnumerable(pagedResult); } @@ -603,15 +664,27 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement sql = sql?.Where(rt => rt.IsDependency, "x"); } - // Ordering is required for paging - sql = sql?.OrderBy(x => x.Alias, "x"); - - List? pagedResult = - _scopeAccessor.AmbientScope?.Database.SkipTake(skip, take, sql); + // find the count before ordering totalRecords = _scopeAccessor.AmbientScope?.Database.Count(sql!) ?? 0; - return _umbracoMapper.MapEnumerable(pagedResult ?? - new List()); + RelationItemDto[] pagedResult; + + //Only to all this, if there is items + if (totalRecords > 0) + { + // Ordering is required for paging + sql = sql?.OrderBy(x => x.Alias, "x"); + + pagedResult = + _scopeAccessor.AmbientScope?.Database.SkipTake(skip, take, sql).ToArray() ?? + Array.Empty(); + } + else + { + pagedResult = Array.Empty(); + } + + return _umbracoMapper.MapEnumerable(pagedResult); } private class UnionHelperDto diff --git a/src/Umbraco.Infrastructure/Persistence/UmbracoDatabaseExtensions.cs b/src/Umbraco.Infrastructure/Persistence/UmbracoDatabaseExtensions.cs index 55ccfd6192..767655e4b3 100644 --- a/src/Umbraco.Infrastructure/Persistence/UmbracoDatabaseExtensions.cs +++ b/src/Umbraco.Infrastructure/Persistence/UmbracoDatabaseExtensions.cs @@ -77,7 +77,8 @@ internal static class UmbracoDatabaseExtensions public static long Count(this IUmbracoDatabase database, Sql sql) { - var query = new Sql().Select("COUNT(*)").From().Append("(").Append(sql).Append(")"); + // We need to copy the sql into a new object, to avoid this method from changing the sql. + var query = new Sql().Select("COUNT(*)").From().Append("(").Append(new Sql(sql.SQL, sql.Arguments)).Append(") as count_query"); return database.ExecuteScalar(query); } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/TrackedReferencesServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/TrackedReferencesServiceTests.cs new file mode 100644 index 0000000000..27517e23dd --- /dev/null +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/TrackedReferencesServiceTests.cs @@ -0,0 +1,91 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. +using NUnit.Framework; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Tests.Common.Builders; +using Umbraco.Cms.Tests.Common.Builders.Extensions; +using Umbraco.Cms.Tests.Common.Testing; +using Umbraco.Cms.Tests.Integration.Testing; + +namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services; + +[TestFixture] +[UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)] +public class TrackedReferencesServiceTests : UmbracoIntegrationTest +{ + private IContentTypeService ContentTypeService => GetRequiredService(); + + private IContentService ContentService => GetRequiredService(); + + private Content Root1 { get; set; } + + private Content Root2 { get; set; } + + private IContentType ContentType { get; set; } + + [SetUp] + public void Setup() => CreateTestData(); + + protected virtual void CreateTestData() + { + ContentType = new ContentTypeBuilder() + .WithName("Page") + .AddPropertyType() + .WithAlias("ContentPicker") + .WithName("contentPicker") + .WithDataTypeId(1046) + .WithPropertyEditorAlias(Constants.PropertyEditors.Aliases.ContentPicker) + .Done() + .Build(); + + ContentTypeService.Save(ContentType); + + Root1 = new ContentBuilder() + .WithContentType(ContentType) + .WithName("Root 1") + .Build(); + + ContentService.Save(Root1); + ContentService.Publish(Root1, ["*"]); + + Root2 = new ContentBuilder() + .WithContentType(ContentType) + .WithName("Root 2") + .WithPropertyValues(new + { + contentPicker = Udi.Create(Constants.UdiEntityType.Document, Root1.Key) // contentPicker is the alias of the property type + }) + .Build(); + + ContentService.Save(Root2); + ContentService.Publish(Root2, ["*"]); + } + + [Test] + public async Task Get_Pages_That_Reference_This() + { + var sut = GetRequiredService(); + + var actual = await sut.GetPagedRelationsForItemAsync(Root1.Key, 0, 10, true); + + Assert.Multiple(() => + { + Assert.AreEqual(1, actual.Total); + var item = actual.Items.FirstOrDefault(); + Assert.AreEqual(Root2.ContentType.Alias, item?.ContentTypeAlias); + Assert.AreEqual(Root2.Key, item?.NodeKey); + }); + } + + [Test] + public async Task Does_not_return_references_if_item_is_not_referenced() + { + var sut = GetRequiredService(); + + var actual = await sut.GetPagedRelationsForItemAsync(Root2.Key, 0, 10, true); + + Assert.AreEqual(0, actual.Total); + } +}