From af287c387ebd733243d889adbfb3647db88ae23e Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 16 Feb 2017 18:54:44 +1100 Subject: [PATCH] Adds test to show that the content repo can deal with corrupted data, this fix actually simplifies a bunch of logic too and should consume slightly less memory, have added some TODOs in the code and need to then write more tests and code to deal with this in the EntityRepository. --- .../Models/PropertyTypeCollection.cs | 1 + .../Persistence/Factories/MemberFactory.cs | 12 ++- .../Repositories/ContentRepository.cs | 60 +++++------- .../Repositories/MediaRepository.cs | 47 ++++----- .../Repositories/MemberRepository.cs | 53 ++++------ .../Repositories/VersionableRepositoryBase.cs | 96 +++++++++++++++++-- .../Repositories/ContentRepositoryTest.cs | 52 ++++++++++ 7 files changed, 211 insertions(+), 110 deletions(-) diff --git a/src/Umbraco.Core/Models/PropertyTypeCollection.cs b/src/Umbraco.Core/Models/PropertyTypeCollection.cs index 38abd0c57d..a06e6d737d 100644 --- a/src/Umbraco.Core/Models/PropertyTypeCollection.cs +++ b/src/Umbraco.Core/Models/PropertyTypeCollection.cs @@ -69,6 +69,7 @@ namespace Umbraco.Core.Models OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset)); } + //TODO: Instead of 'new' this should explicitly implement one of the collection interfaces members internal new void Add(PropertyType item) { using (new WriteLock(_addLocker)) diff --git a/src/Umbraco.Core/Persistence/Factories/MemberFactory.cs b/src/Umbraco.Core/Persistence/Factories/MemberFactory.cs index 2901f48539..7b28808429 100644 --- a/src/Umbraco.Core/Persistence/Factories/MemberFactory.cs +++ b/src/Umbraco.Core/Persistence/Factories/MemberFactory.cs @@ -28,17 +28,17 @@ namespace Umbraco.Core.Persistence.Factories #region Implementation of IEntityFactory - public IMember BuildEntity(MemberDto dto) + public static IMember BuildEntity(MemberDto dto, IMemberType contentType) { var member = new Member( dto.ContentVersionDto.ContentDto.NodeDto.Text, - dto.Email, dto.LoginName, dto.Password, _contentType); + dto.Email, dto.LoginName, dto.Password, contentType); try { member.DisableChangeTracking(); - member.Id = _id; + member.Id = dto.NodeId; member.Key = dto.ContentVersionDto.ContentDto.NodeDto.UniqueId; member.Path = dto.ContentVersionDto.ContentDto.NodeDto.Path; member.CreatorId = dto.ContentVersionDto.ContentDto.NodeDto.UserId.Value; @@ -62,6 +62,12 @@ namespace Umbraco.Core.Persistence.Factories } } + [Obsolete("Use the static BuildEntity instead so we don't have to allocate one of these objects everytime we want to map values")] + public IMember BuildEntity(MemberDto dto) + { + return BuildEntity(dto, _contentType); + } + public MemberDto BuildDto(IMember entity) { var dto = new MemberDto diff --git a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs index ff466ea84e..0e1228960e 100644 --- a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs @@ -64,7 +64,7 @@ namespace Umbraco.Core.Persistence.Repositories if (dto == null) return null; - var content = CreateContentFromDto(dto, dto.ContentVersionDto.VersionId, sql); + var content = CreateContentFromDto(dto, sql); return content; } @@ -287,7 +287,7 @@ namespace Umbraco.Core.Persistence.Repositories if (dto == null) return null; - var content = CreateContentFromDto(dto, versionId, sql); + var content = CreateContentFromDto(dto, sql); return content; } @@ -933,13 +933,14 @@ order by umbracoNode.{2}, umbracoNode.parentID, umbracoNode.sortOrder", parsedOriginalSql = parsedOriginalSql.Substring(0, parsedOriginalSql.LastIndexOf("ORDER BY ", StringComparison.Ordinal)); } + //order by update date DESC, if there is corrupted published flags we only want the latest! var publishedSql = new Sql(@"SELECT * FROM cmsDocument AS doc2 INNER JOIN (" + parsedOriginalSql + @") as docData ON doc2.nodeId = docData.nodeId WHERE doc2.published = 1 -ORDER BY doc2.nodeId +ORDER BY doc2.updateDate DESC ", sqlFull.Arguments); //go and get the published version data, we do a Query here and not a Fetch so we are @@ -955,8 +956,8 @@ ORDER BY doc2.nodeId } - var content = new IContent[dtos.Count]; - var defs = new List(); + var content = new List(); + var defs = new DocumentDefinitionCollection(); var templateIds = new List(); //track the looked up content types, even though the content types are cached @@ -964,9 +965,8 @@ ORDER BY doc2.nodeId // the overhead of deep cloning them on every item in this loop var contentTypes = new Dictionary(); - for (var i = 0; i < dtos.Count; i++) + foreach (var dto in dtos) { - var dto = dtos[i]; DocumentPublishedReadOnlyDto publishedDto; publishedDataCollection.TryGetValue(dto.NodeId, out publishedDto); @@ -975,9 +975,10 @@ ORDER BY doc2.nodeId { var cached = RuntimeCache.GetCacheItem(GetCacheIdKey(dto.NodeId)); //only use this cached version if the dto returned is also the publish version, they must match + //TODO: Shouldn't this also match on version!? if (cached != null && cached.Published && dto.Published) { - content[i] = cached; + content.Add(cached); continue; } } @@ -996,20 +997,15 @@ ORDER BY doc2.nodeId contentTypes[dto.ContentVersionDto.ContentDto.ContentTypeId] = contentType; } - content[i] = ContentFactory.BuildEntity(dto, contentType, publishedDto); + // track the definition and if it's successfully added or updated then processed + if (defs.AddOrUpdate(new DocumentDefinition(dto, contentType))) + { + // need template + if (dto.TemplateId.HasValue && dto.TemplateId.Value > 0) + templateIds.Add(dto.TemplateId.Value); - // need template - if (dto.TemplateId.HasValue && dto.TemplateId.Value > 0) - templateIds.Add(dto.TemplateId.Value); - - // need properties - defs.Add(new DocumentDefinition( - dto.NodeId, - dto.VersionId, - dto.ContentVersionDto.VersionDate, - dto.ContentVersionDto.ContentDto.NodeDto.CreateDate, - contentType - )); + content.Add(ContentFactory.BuildEntity(dto, contentType, publishedDto)); + } } // load all required templates in 1 query @@ -1019,26 +1015,21 @@ ORDER BY doc2.nodeId // load all properties for all documents from database in 1 query var propertyData = GetPropertyCollection(pagingSqlQuery, defs); - // assign - var dtoIndex = 0; - foreach (var def in defs) + // assign template and property data + foreach (var cc in content) { - // move to corresponding item (which has to exist) - while (dtos[dtoIndex].NodeId != def.Id) dtoIndex++; + var def = defs[cc.Id]; - // complete the item - var cc = content[dtoIndex]; - var dto = dtos[dtoIndex]; ITemplate template = null; - if (dto.TemplateId.HasValue) - templates.TryGetValue(dto.TemplateId.Value, out template); // else null + if (def.DocumentDto.TemplateId.HasValue) + templates.TryGetValue(def.DocumentDto.TemplateId.Value, out template); // else null cc.Template = template; cc.Properties = propertyData[cc.Id]; //on initial construction we don't want to have dirty properties tracked // http://issues.umbraco.org/issue/U4-1946 cc.ResetDirtyProperties(false); - } + } return content; } @@ -1047,10 +1038,9 @@ ORDER BY doc2.nodeId /// Private method to create a content object from a DocumentDto, which is used by Get and GetByVersion. /// /// - /// /// /// - private IContent CreateContentFromDto(DocumentDto dto, Guid versionId, Sql docSql) + private IContent CreateContentFromDto(DocumentDto dto, Sql docSql) { var contentType = _contentTypeRepository.Get(dto.ContentVersionDto.ContentDto.ContentTypeId); @@ -1062,7 +1052,7 @@ ORDER BY doc2.nodeId content.Template = _templateRepository.Get(dto.TemplateId.Value); } - var docDef = new DocumentDefinition(dto.NodeId, versionId, content.UpdateDate, content.CreateDate, contentType); + var docDef = new DocumentDefinition(dto, contentType); var properties = GetPropertyCollection(docSql, new[] { docDef }); diff --git a/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs b/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs index 53b04e1322..3f0d1d2486 100644 --- a/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs @@ -55,7 +55,7 @@ namespace Umbraco.Core.Persistence.Repositories if (dto == null) return null; - var content = CreateMediaFromDto(dto, dto.VersionId, sql); + var content = CreateMediaFromDto(dto, sql); return content; } @@ -161,25 +161,24 @@ namespace Umbraco.Core.Persistence.Repositories { // fetch returns a list so it's ok to iterate it in this method var dtos = Database.Fetch(sqlFull); - var content = new IMedia[dtos.Count]; - var defs = new List(); + var content = new List(); + var defs = new DocumentDefinitionCollection(); //track the looked up content types, even though the content types are cached // they still need to be deep cloned out of the cache and we don't want to add // the overhead of deep cloning them on every item in this loop var contentTypes = new Dictionary(); - for (var i = 0; i < dtos.Count; i++) + foreach (var dto in dtos) { - var dto = dtos[i]; - // if the cache contains the item, use it if (withCache) { var cached = RuntimeCache.GetCacheItem(GetCacheIdKey(dto.NodeId)); + //TODO: Shouldn't this also match on version!? if (cached != null) { - content[i] = cached; + content.Add(cached); continue; } } @@ -197,37 +196,26 @@ namespace Umbraco.Core.Persistence.Repositories contentType = _mediaTypeRepository.Get(dto.ContentDto.ContentTypeId); contentTypes[dto.ContentDto.ContentTypeId] = contentType; } - - content[i] = MediaFactory.BuildEntity(dto, contentType); - // need properties - defs.Add(new DocumentDefinition( - dto.NodeId, - dto.VersionId, - dto.VersionDate, - dto.ContentDto.NodeDto.CreateDate, - contentType - )); + // track the definition and if it's successfully added or updated then processed + if (defs.AddOrUpdate(new DocumentDefinition(dto, contentType))) + { + content.Add(MediaFactory.BuildEntity(dto, contentType)); + } } // load all properties for all documents from database in 1 query var propertyData = GetPropertyCollection(pagingSqlQuery, defs); - // assign - var dtoIndex = 0; - foreach (var def in defs) + // assign property data + foreach (var cc in content) { - // move to corresponding item (which has to exist) - while (dtos[dtoIndex].NodeId != def.Id) dtoIndex++; - - // complete the item - var cc = content[dtoIndex]; cc.Properties = propertyData[cc.Id]; //on initial construction we don't want to have dirty properties tracked // http://issues.umbraco.org/issue/U4-1946 cc.ResetDirtyProperties(false); - } + } return content; } @@ -243,7 +231,7 @@ namespace Umbraco.Core.Persistence.Repositories if (dto == null) return null; - var content = CreateMediaFromDto(dto, versionId, sql); + var content = CreateMediaFromDto(dto, sql); return content; } @@ -528,16 +516,15 @@ namespace Umbraco.Core.Persistence.Repositories /// Private method to create a media object from a ContentDto /// /// - /// /// /// - private IMedia CreateMediaFromDto(ContentVersionDto dto, Guid versionId, Sql docSql) + private IMedia CreateMediaFromDto(ContentVersionDto dto, Sql docSql) { var contentType = _mediaTypeRepository.Get(dto.ContentDto.ContentTypeId); var media = MediaFactory.BuildEntity(dto, contentType); - var docDef = new DocumentDefinition(dto.NodeId, versionId, media.UpdateDate, media.CreateDate, contentType); + var docDef = new DocumentDefinition(dto, contentType); var properties = GetPropertyCollection(new PagingSqlQuery(docSql), new[] { docDef }); diff --git a/src/Umbraco.Core/Persistence/Repositories/MemberRepository.cs b/src/Umbraco.Core/Persistence/Repositories/MemberRepository.cs index ae8cb08255..23b42108bc 100644 --- a/src/Umbraco.Core/Persistence/Repositories/MemberRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/MemberRepository.cs @@ -54,7 +54,7 @@ namespace Umbraco.Core.Persistence.Repositories if (dto == null) return null; - var content = CreateMemberFromDto(dto, dto.ContentVersionDto.VersionId, sql); + var content = CreateMemberFromDto(dto, sql); return content; @@ -448,16 +448,16 @@ namespace Umbraco.Core.Persistence.Repositories var memberType = _memberTypeRepository.Get(dto.ContentVersionDto.ContentDto.ContentTypeId); var factory = new MemberFactory(memberType, NodeObjectTypeId, dto.NodeId); - var media = factory.BuildEntity(dto); + var member = factory.BuildEntity(dto); - var properties = GetPropertyCollection(new PagingSqlQuery(sql), new[] { new DocumentDefinition(dto.NodeId, dto.ContentVersionDto.VersionId, media.UpdateDate, media.CreateDate, memberType) }); + var properties = GetPropertyCollection(new PagingSqlQuery(sql), new[] { new DocumentDefinition(dto.ContentVersionDto, memberType) }); - media.Properties = properties[dto.NodeId]; + member.Properties = properties[dto.NodeId]; //on initial construction we don't want to have dirty properties tracked // http://issues.umbraco.org/issue/U4-1946 - ((Entity)media).ResetDirtyProperties(false); - return media; + ((Entity)member).ResetDirtyProperties(false); + return member; } @@ -681,20 +681,19 @@ namespace Umbraco.Core.Persistence.Repositories // fetch returns a list so it's ok to iterate it in this method var dtos = Database.Fetch(sqlFull); - var content = new IMember[dtos.Count]; - var defs = new List(); + var content = new List(); + var defs = new DocumentDefinitionCollection(); - for (var i = 0; i < dtos.Count; i++) + foreach (var dto in dtos) { - var dto = dtos[i]; - // if the cache contains the item, use it if (withCache) { var cached = RuntimeCache.GetCacheItem(GetCacheIdKey(dto.NodeId)); + //TODO: Shouldn't this also match on version!? if (cached != null) { - content[i] = cached; + content.Add(cached); continue; } } @@ -702,36 +701,25 @@ namespace Umbraco.Core.Persistence.Repositories // else, need to fetch from the database // content type repository is full-cache so OK to get each one independently var contentType = _memberTypeRepository.Get(dto.ContentVersionDto.ContentDto.ContentTypeId); - var factory = new MemberFactory(contentType, NodeObjectTypeId, dto.NodeId); - content[i] = factory.BuildEntity(dto); // need properties - defs.Add(new DocumentDefinition( - dto.NodeId, - dto.ContentVersionDto.VersionId, - dto.ContentVersionDto.VersionDate, - dto.ContentVersionDto.ContentDto.NodeDto.CreateDate, - contentType - )); + if (defs.AddOrUpdate(new DocumentDefinition(dto.ContentVersionDto, contentType))) + { + content.Add(MemberFactory.BuildEntity(dto, contentType)); + } } // load all properties for all documents from database in 1 query var propertyData = GetPropertyCollection(pagingSqlQuery, defs); - // assign - var dtoIndex = 0; - foreach (var def in defs) + // assign property data + foreach (var cc in content) { - // move to corresponding item (which has to exist) - while (dtos[dtoIndex].NodeId != def.Id) dtoIndex++; - - // complete the item - var cc = content[dtoIndex]; cc.Properties = propertyData[cc.Id]; //on initial construction we don't want to have dirty properties tracked // http://issues.umbraco.org/issue/U4-1946 - ((Entity)cc).ResetDirtyProperties(false); + cc.ResetDirtyProperties(false); } return content; @@ -741,17 +729,16 @@ namespace Umbraco.Core.Persistence.Repositories /// Private method to create a member object from a MemberDto /// /// - /// /// /// - private IMember CreateMemberFromDto(MemberDto dto, Guid versionId, Sql docSql) + private IMember CreateMemberFromDto(MemberDto dto, Sql docSql) { var memberType = _memberTypeRepository.Get(dto.ContentVersionDto.ContentDto.ContentTypeId); var factory = new MemberFactory(memberType, NodeObjectTypeId, dto.ContentVersionDto.NodeId); var member = factory.BuildEntity(dto); - var docDef = new DocumentDefinition(dto.ContentVersionDto.NodeId, versionId, member.UpdateDate, member.CreateDate, memberType); + var docDef = new DocumentDefinition(dto.ContentVersionDto, memberType); var properties = GetPropertyCollection(docSql, new[] { docDef }); diff --git a/src/Umbraco.Core/Persistence/Repositories/VersionableRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/VersionableRepositoryBase.cs index df96112a30..c2c8d3e096 100644 --- a/src/Umbraco.Core/Persistence/Repositories/VersionableRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/VersionableRepositoryBase.cs @@ -773,24 +773,102 @@ ORDER BY contentNodeId, propertytypeid /// protected abstract Sql GetBaseQuery(BaseQueryType queryType); + internal class DocumentDefinitionCollection : KeyedCollection + { + protected override int GetKeyForItem(DocumentDefinition item) + { + return item.Id; + } + + /// + /// if this key already exists if it does then we need to check + /// if the existing item is 'older' than the new item and if that is the case we'll replace the older one + /// + /// + /// + public bool AddOrUpdate(DocumentDefinition item) + { + if (Dictionary == null) + { + base.Add(item); + return true; + } + + var key = GetKeyForItem(item); + DocumentDefinition found; + if (TryGetValue(key, out found)) + { + //it already exists and it's older so we need to replace it + if (item.VersionDate > found.VersionDate) + { + var currIndex = Items.IndexOf(found); + if (currIndex == -1) + throw new IndexOutOfRangeException("Could not find the item in the list: " + found.Version); + + //replace the current one with the newer one + SetItem(currIndex, item); + return true; + } + //could not add or update + return false; + } + + base.Add(item); + return true; + } + + public bool TryGetValue(int key, out DocumentDefinition val) + { + if (Dictionary == null) + { + val = null; + return false; + } + return Dictionary.TryGetValue(key, out val); + } + } + internal class DocumentDefinition { /// /// Initializes a new instance of the class. /// - public DocumentDefinition(int id, Guid version, DateTime versionDate, DateTime createDate, IContentTypeComposition composition) + public DocumentDefinition(DocumentDto dto, IContentTypeComposition composition) { - Id = id; - Version = version; - VersionDate = versionDate; - CreateDate = createDate; + DocumentDto = dto; + ContentVersionDto = dto.ContentVersionDto; Composition = composition; } - public int Id { get; set; } - public Guid Version { get; set; } - public DateTime VersionDate { get; set; } - public DateTime CreateDate { get; set; } + public DocumentDefinition(ContentVersionDto dto, IContentTypeComposition composition) + { + ContentVersionDto = dto; + Composition = composition; + } + + public DocumentDto DocumentDto { get; private set; } + public ContentVersionDto ContentVersionDto { get; private set; } + + public int Id + { + get { return ContentVersionDto.NodeId; } + } + + public Guid Version + { + get { return DocumentDto != null ? DocumentDto.VersionId : ContentVersionDto.VersionId; } + } + + public DateTime VersionDate + { + get { return ContentVersionDto.VersionDate; } + } + + public DateTime CreateDate + { + get { return ContentVersionDto.ContentDto.NodeDto.CreateDate; } + } + public IContentTypeComposition Composition { get; set; } } diff --git a/src/Umbraco.Tests/Persistence/Repositories/ContentRepositoryTest.cs b/src/Umbraco.Tests/Persistence/Repositories/ContentRepositoryTest.cs index a561af08a2..c6f9ed21ba 100644 --- a/src/Umbraco.Tests/Persistence/Repositories/ContentRepositoryTest.cs +++ b/src/Umbraco.Tests/Persistence/Repositories/ContentRepositoryTest.cs @@ -67,6 +67,58 @@ namespace Umbraco.Tests.Persistence.Repositories return repository; } + //TODO: We need to write this same test and fix the EntityRepository! + [Test] + public void Deal_With_Corrupt_Duplicate_Newest_Published_Flags() + { + // Arrange + var provider = new PetaPocoUnitOfWorkProvider(Logger); + var unitOfWork = provider.GetUnitOfWork(); + ContentTypeRepository contentTypeRepository; + IContent content1; + + using (var repository = CreateRepository(unitOfWork, out contentTypeRepository)) + { + var hasPropertiesContentType = MockedContentTypes.CreateSimpleContentType("umbTextpage1", "Textpage"); + content1 = MockedContent.CreateSimpleContent(hasPropertiesContentType); + + contentTypeRepository.AddOrUpdate(hasPropertiesContentType); + repository.AddOrUpdate(content1); + unitOfWork.Commit(); + } + + //Now manually corrupt the data + for (var index = 0; index < new[] {Guid.NewGuid(), Guid.NewGuid()}.Length; index++) + { + var version = new[] {Guid.NewGuid(), Guid.NewGuid()}[index]; + var versionDate = DateTime.Now.AddMinutes(index); + this.DatabaseContext.Database.Insert(new ContentVersionDto + { + NodeId = content1.Id, + VersionDate = versionDate, + VersionId = version + }); + this.DatabaseContext.Database.Insert(new DocumentDto + { + Newest = true, + NodeId = content1.Id, + Published = true, + Text = content1.Name, + VersionId = version, + WriterUserId = 0, + UpdateDate = versionDate, + TemplateId = content1.Template == null || content1.Template.Id <= 0 ? null : (int?) content1.Template.Id + }); + } + + // Assert + using (var repository = CreateRepository(unitOfWork, out contentTypeRepository)) + { + var content = repository.GetByQuery(new Query().Where(c => c.Id == content1.Id)); + Assert.AreEqual(1, content.Count()); + } + } + /// /// This tests the regression issue of U4-9438 ///