From ada000437bb761a81011881745c28175a82be0dd Mon Sep 17 00:00:00 2001 From: Claus Date: Mon, 6 Jan 2020 15:45:34 +0100 Subject: [PATCH 1/3] reverting more changes for optimizations to ensure tests are not failing. --- .../Repositories/Implement/DocumentRepository.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs index 293045a50b..820896c004 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs @@ -241,7 +241,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement return MapDtosToContent(Database.Fetch(sql), true, // load bare minimum - false, false, false, false).Skip(skip).Take(take); + false, false, false, true).Skip(skip).Take(take); } public override IContent GetVersion(int versionId) @@ -988,7 +988,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement return MapDtosToContent(Database.Fetch(sql), // load the bare minimum - false, false, false, false, false); + false, false, false, true, true); } /// @@ -1006,7 +1006,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement return MapDtosToContent(Database.Fetch(sql), // load the bare minimum - false, false, false, false, false); + false, false, false, true, true); } #endregion From 9a192131d650f37209627fcd096356c9ac77dd9e Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Jan 2020 14:53:17 +1100 Subject: [PATCH 2/3] Ensures the DocumentRepository behavior for looking up data to populate IContent remains the same. --- .../Implement/DocumentRepository.cs | 53 ++++++++++--------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs index 820896c004..6fdb30dd22 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs @@ -74,9 +74,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (ids.Any()) sql.WhereIn(x => x.NodeId, ids); - return MapDtosToContent(Database.Fetch(sql), false, - // load everything - true, true, true, true); + return MapDtosToContent(Database.Fetch(sql)); } protected override IEnumerable PerformGetByQuery(IQuery query) @@ -88,9 +86,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement AddGetByQueryOrderBy(sql); - return MapDtosToContent(Database.Fetch(sql), false, - // load everything - true, true, true, true); + return MapDtosToContent(Database.Fetch(sql)); } private void AddGetByQueryOrderBy(Sql sql) @@ -229,9 +225,24 @@ namespace Umbraco.Core.Persistence.Repositories.Implement .OrderByDescending(x => x.Current) .AndByDescending(x => x.VersionDate); - return MapDtosToContent(Database.Fetch(sql), true, true, true, true, true); + return MapDtosToContent(Database.Fetch(sql), true); } + // TODO: This method needs to return a readonly version of IContent! The content returned + // from this method does not contain all of the data required to re-persist it and if that + // is attempted some odd things will occur. + // Either we create an IContentReadOnly (which ultimately we should for vNext so we can + // differentiate between methods that return entities that can be re-persisted or not), or + // in the meantime to not break API compatibility, we can add a property to IContentBase + // (or go further and have it on IUmbracoEntity): "IsReadOnly" and if that is true we throw + // an exception if that entity is passed to a Save method. + // Ideally we return "Slim" versions of content for all sorts of methods here and in ContentService. + // Perhaps another non-breaking alternative is to have new services like IContentServiceReadOnly + // which can return IContentReadOnly. + // We have the ability with `MapDtosToContent` to reduce the amount of data looked up for a + // content item. Ideally for paged data that populates list views, these would be ultra slim + // content items, there's no reason to populate those with really anything apart from property data, + // but until we do something like the above, we can't do that since it would be breaking and unclear. public override IEnumerable GetAllVersionsSlim(int nodeId, int skip, int take) { var sql = GetBaseQuery(QueryType.Many, false) @@ -241,7 +252,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement return MapDtosToContent(Database.Fetch(sql), true, // load bare minimum - false, false, false, true).Skip(skip).Take(take); + false, false, false, false).Skip(skip).Take(take); } public override IContent GetVersion(int versionId) @@ -837,9 +848,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement } return GetPage(query, pageIndex, pageSize, out totalRecords, - x => MapDtosToContent(x, false, - // load properties but nothing else - true, false, false, true), + x => MapDtosToContent(x), filterSql, ordering); } @@ -926,9 +935,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (ids.Length > 0) sql.WhereIn(x => x.UniqueId, ids); - return _outerRepo.MapDtosToContent(Database.Fetch(sql), false, - // load everything - true, true, true, true); + return _outerRepo.MapDtosToContent(Database.Fetch(sql)); } protected override IEnumerable PerformGetByQuery(IQuery query) @@ -986,9 +993,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement AddGetByQueryOrderBy(sql); - return MapDtosToContent(Database.Fetch(sql), - // load the bare minimum - false, false, false, true, true); + return MapDtosToContent(Database.Fetch(sql)); } /// @@ -1004,9 +1009,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement AddGetByQueryOrderBy(sql); - return MapDtosToContent(Database.Fetch(sql), - // load the bare minimum - false, false, false, true, true); + return MapDtosToContent(Database.Fetch(sql)); } #endregion @@ -1070,11 +1073,11 @@ namespace Umbraco.Core.Persistence.Repositories.Implement } private IEnumerable MapDtosToContent(List dtos, - bool withCache, - bool loadProperties, - bool loadTemplates, - bool loadSchedule, - bool loadVariants) + bool withCache = false, + bool loadProperties = true, + bool loadTemplates = true, + bool loadSchedule = true, + bool loadVariants = true) { var temps = new List>(); var contentTypes = new Dictionary(); From 5d7a1bacfed43d2c59c5aca9584bb6c17020c42d Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Jan 2020 15:34:53 +1100 Subject: [PATCH 3/3] Ensures the DocumentRepository behavior for looking up data to populate IContent remains the same. --- .../Persistence/Repositories/Implement/DocumentRepository.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs index 6fdb30dd22..afe1af7eb4 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs @@ -251,8 +251,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement .AndByDescending(x => x.VersionDate); return MapDtosToContent(Database.Fetch(sql), true, - // load bare minimum - false, false, false, false).Skip(skip).Take(take); + // load bare minimum, need variants though since this is used to rollback with variants + false, false, false, true).Skip(skip).Take(take); } public override IContent GetVersion(int versionId)