Skip to content

Commit

Permalink
Removes the remaining part of the EntityRepository that was loading i…
Browse files Browse the repository at this point in the history 
…n ALL properties for media which we don't want whatsoever which means some other code is cleaned up/removed
  • Loading branch information
@Shazwazza
Shazwazza committed Jun 26, 2019
1 parent cf53ba3 commit 23b8e1c
Show file tree
Hide file tree
Showing 11 changed files with 27 additions and 373 deletions.
39 changes: 1 addition & 38 deletions src/Umbraco.Core/Models/UmbracoEntity.cs
View file
Expand Up @@ -225,42 +225,5 @@ public override object DeepClone()
return clone;
}

/// <summary>
/// A struction that can be contained in the additional data of an UmbracoEntity representing
/// a user defined property
/// </summary>
public class EntityProperty : IDeepCloneable
{
public string PropertyEditorAlias { get; set; }
public object Value { get; set; }
public object DeepClone()
{
//Memberwise clone on Entity will work since it doesn't have any deep elements
// for any sub class this will work for standard properties as well that aren't complex object's themselves.
var clone = MemberwiseClone();
return clone;
}

protected bool Equals(EntityProperty other)
{
return PropertyEditorAlias.Equals(other.PropertyEditorAlias) && string.Equals(Value, other.Value);
}

public override bool Equals(object obj)
{
if (ReferenceEquals(null, obj)) return false;
if (ReferenceEquals(this, obj)) return true;
if (obj.GetType() != this.GetType()) return false;
return Equals((EntityProperty) obj);
}

public override int GetHashCode()
{
unchecked
{
return (PropertyEditorAlias.GetHashCode() * 397) ^ (Value != null ? Value.GetHashCode() : 0);
}
}
}
}
}
}
146 changes: 17 additions & 129 deletions src/Umbraco.Core/Persistence/Repositories/EntityRepository.cs
View file
Expand Up @@ -20,27 +20,22 @@ namespace Umbraco.Core.Persistence.Repositories
/// </remarks>
internal class EntityRepository : DisposableObjectSlim, IEntityRepository
{
private readonly IDatabaseUnitOfWork _work;

public EntityRepository(IDatabaseUnitOfWork work)
{
_work = work;
UnitOfWork = work;
}

/// <summary>
/// Returns the Unit of Work added to the repository
/// </summary>
protected internal IDatabaseUnitOfWork UnitOfWork
{
get { return _work; }
}
protected internal IDatabaseUnitOfWork UnitOfWork { get; }

/// <summary>
/// Internal for testing purposes
/// </summary>
internal Guid UnitKey
{
get { return (Guid)_work.Key; }
get { return (Guid)UnitOfWork.Key; }
}

#region Query Methods
Expand Down Expand Up @@ -69,75 +64,8 @@ internal Guid UnitKey

IEnumerable<IUmbracoEntity> result;

if (isMedia)
{
//Treat media differently for now, as an Entity it will be returned with ALL of it's properties in the AdditionalData bag!
var pagedResult = _work.Database.Page<dynamic>(pageIndex + 1, pageSize, pagedSql);

var ids = pagedResult.Items.Select(x => (int) x.id).InGroupsOf(2000);
var entities = pagedResult.Items.Select(factory.BuildEntityFromDynamic).Cast<IUmbracoEntity>().ToList();

//Now we need to merge in the property data since we need paging and we can't do this the way that the big media query was working before
foreach (var idGroup in ids)
{
var propSql = GetPropertySql(Constants.ObjectTypes.Media)
.Where("contentNodeId IN (@ids)", new { ids = idGroup });
propSql = (orderDirection == Direction.Descending) ? propSql.OrderByDescending("contentNodeId") : propSql.OrderBy("contentNodeId");

//This does NOT fetch all data into memory in a list, this will read
// over the records as a data reader, this is much better for performance and memory,
// but it means that during the reading of this data set, nothing else can be read
// from SQL server otherwise we'll get an exception.
var allPropertyData = _work.Database.Query<dynamic>(propSql);

//keep track of the current property data item being enumerated
var propertyDataSetEnumerator = allPropertyData.GetEnumerator();
var hasCurrent = false; // initially there is no enumerator.Current

try
{
//This must be sorted by node id (which is done by SQL) because this is how we are sorting the query to lookup property types above,
// which allows us to more efficiently iterate over the large data set of property values.
foreach (var entity in entities)
{
// assemble the dtos for this def
// use the available enumerator.Current if any else move to next
while (hasCurrent || propertyDataSetEnumerator.MoveNext())
{
if (propertyDataSetEnumerator.Current.contentNodeId == entity.Id)
{
hasCurrent = false; // enumerator.Current is not available

//the property data goes into the additional data
entity.AdditionalData[propertyDataSetEnumerator.Current.propertyTypeAlias] = new UmbracoEntity.EntityProperty
{
PropertyEditorAlias = propertyDataSetEnumerator.Current.propertyEditorAlias,
Value = StringExtensions.IsNullOrWhiteSpace(propertyDataSetEnumerator.Current.dataNtext)
? propertyDataSetEnumerator.Current.dataNvarchar
: StringExtensions.ConvertToJsonIfPossible(propertyDataSetEnumerator.Current.dataNtext)
};
}
else
{
hasCurrent = true; // enumerator.Current is available for another def
break; // no more propertyDataDto for this def
}
}
}
}
finally
{
propertyDataSetEnumerator.Dispose();
}
}

result = entities;
}
else
{
var pagedResult = _work.Database.Page<dynamic>(pageIndex + 1, pageSize, pagedSql);
result = pagedResult.Items.Select(factory.BuildEntityFromDynamic).Cast<IUmbracoEntity>().ToList();
}
var pagedResult = UnitOfWork.Database.Page<dynamic>(pageIndex + 1, pageSize, pagedSql);
result = pagedResult.Items.Select(factory.BuildEntityFromDynamic).Cast<IUmbracoEntity>().ToList();

//The total items from the PetaPoco page query will be wrong due to the Outer join used on parent, depending on the search this will
//return duplicate results when the COUNT is used in conjuction with it, so we need to get the total on our own.
Expand All @@ -159,15 +87,15 @@ internal Guid UnitKey
var translatorCount = new SqlTranslator<IUmbracoEntity>(sqlCountClause, query);
var countSql = translatorCount.Translate();

totalRecords = _work.Database.ExecuteScalar<int>(countSql);
totalRecords = UnitOfWork.Database.ExecuteScalar<int>(countSql);

return result;
}

public IUmbracoEntity GetByKey(Guid key)
{
var sql = GetBaseWhere(GetBase, false, false, key);
var nodeDto = _work.Database.FirstOrDefault<dynamic>(sql);
var nodeDto = UnitOfWork.Database.FirstOrDefault<dynamic>(sql);
if (nodeDto == null)
return null;

Expand All @@ -187,7 +115,7 @@ public IUmbracoEntity GetByKey(Guid key, Guid objectTypeId)
var factory = new UmbracoEntityFactory();

//query = read forward data reader, do not load everything into mem
var dtos = _work.Database.Query<dynamic>(sql);
var dtos = UnitOfWork.Database.Query<dynamic>(sql);
var collection = new EntityDefinitionCollection();
foreach (var dto in dtos)
{
Expand All @@ -202,7 +130,7 @@ public IUmbracoEntity GetByKey(Guid key, Guid objectTypeId)
public virtual IUmbracoEntity Get(int id)
{
var sql = GetBaseWhere(GetBase, false, false, id);
var nodeDto = _work.Database.FirstOrDefault<dynamic>(sql);
var nodeDto = UnitOfWork.Database.FirstOrDefault<dynamic>(sql);
if (nodeDto == null)
return null;

Expand All @@ -222,7 +150,7 @@ public virtual IUmbracoEntity Get(int id, Guid objectTypeId)
var factory = new UmbracoEntityFactory();

//query = read forward data reader, do not load everything into mem
var dtos = _work.Database.Query<dynamic>(sql);
var dtos = UnitOfWork.Database.Query<dynamic>(sql);
var collection = new EntityDefinitionCollection();
foreach (var dto in dtos)
{
Expand Down Expand Up @@ -256,7 +184,7 @@ private IEnumerable<IUmbracoEntity> PerformGetAll(Guid objectTypeId, Action<Sql>
var factory = new UmbracoEntityFactory();

//query = read forward data reader, do not load everything into mem
var dtos = _work.Database.Query<dynamic>(sql);
var dtos = UnitOfWork.Database.Query<dynamic>(sql);
var collection = new EntityDefinitionCollection();
foreach (var dto in dtos)
{
Expand All @@ -283,7 +211,7 @@ private IEnumerable<EntityPath> PerformGetAllPaths(Guid objectTypeId, Action<Sql
{
var sql = new Sql("SELECT id, path FROM umbracoNode WHERE umbracoNode.nodeObjectType=@type", new { type = objectTypeId });
if (filter != null) filter(sql);
return _work.Database.Fetch<EntityPath>(sql);
return UnitOfWork.Database.Fetch<EntityPath>(sql);
}

public virtual IEnumerable<IUmbracoEntity> GetByQuery(IQuery<IUmbracoEntity> query)
Expand All @@ -292,7 +220,7 @@ public virtual IEnumerable<IUmbracoEntity> GetByQuery(IQuery<IUmbracoEntity> que
var translator = new SqlTranslator<IUmbracoEntity>(sqlClause, query);
var sql = translator.Translate().Append(GetGroupBy(false, false));

var dtos = _work.Database.Fetch<dynamic>(sql);
var dtos = UnitOfWork.Database.Fetch<dynamic>(sql);

var factory = new UmbracoEntityFactory();
var list = dtos.Select(factory.BuildEntityFromDynamic).Cast<IUmbracoEntity>().ToList();
Expand All @@ -306,10 +234,6 @@ public virtual IEnumerable<IUmbracoEntity> GetByQuery(IQuery<IUmbracoEntity> que
/// <param name="query"></param>
/// <param name="objectTypeId"></param>
/// <returns></returns>
/// <remarks>
/// Note that this will also fetch all property data for media items, which can cause performance problems
/// when used without paging, in sites with large amounts of data in cmsPropertyData.
/// </remarks>
public virtual IEnumerable<IUmbracoEntity> GetByQuery(IQuery<IUmbracoEntity> query, Guid objectTypeId)
{
var isContent = objectTypeId == Constants.ObjectTypes.DocumentGuid || objectTypeId == Constants.ObjectTypes.DocumentBlueprintGuid;
Expand All @@ -323,35 +247,15 @@ public virtual IEnumerable<IUmbracoEntity> GetByQuery(IQuery<IUmbracoEntity> que
return GetByQueryInternal(entitySql, isContent, isMedia);
}

/// <summary>
/// Gets entities by query without fetching property data.
/// </summary>
/// <param name="query"></param>
/// <param name="objectTypeId"></param>
/// <returns></returns>
/// <remarks>
/// This is supposed to be internal and can be used when getting all entities without paging, without causing
/// performance issues.
/// </remarks>
internal IEnumerable<IUmbracoEntity> GetMediaByQueryWithoutPropertyData(IQuery<IUmbracoEntity> query)
{
var sqlClause = GetBaseWhere(GetBase, false, true, null, UmbracoObjectTypes.Media.GetGuid());

var translator = new SqlTranslator<IUmbracoEntity>(sqlClause, query);
var entitySql = translator.Translate();

return GetByQueryInternal(entitySql, false, true);
}

internal IEnumerable<IUmbracoEntity> GetByQueryInternal(Sql entitySql, bool isContent, bool isMedia)
private IEnumerable<IUmbracoEntity> GetByQueryInternal(Sql entitySql, bool isContent, bool isMedia)
{
var factory = new UmbracoEntityFactory();

//use dynamic so that we can get ALL properties from the SQL so we can chuck that data into our AdditionalData
var finalSql = entitySql.Append(GetGroupBy(isContent, isMedia));

//query = read forward data reader, do not load everything into mem
var dtos = _work.Database.Query<dynamic>(finalSql);
var dtos = UnitOfWork.Database.Query<dynamic>(finalSql);
var collection = new EntityDefinitionCollection();
foreach (var dto in dtos)
{
Expand Down Expand Up @@ -392,22 +296,6 @@ protected Sql GetFullSqlForEntityType(bool isContent, bool isMedia, Guid objectT
return entitySql.Append(GetGroupBy(isContent, true, false));
}

private Sql GetPropertySql(string nodeObjectType)
{
var sql = new Sql()
.Select("contentNodeId, versionId, dataNvarchar, dataNtext, propertyEditorAlias, alias as propertyTypeAlias")
.From<PropertyDataDto>()
.InnerJoin<NodeDto>()
.On<PropertyDataDto, NodeDto>(dto => dto.NodeId, dto => dto.NodeId)
.InnerJoin<PropertyTypeDto>()
.On<PropertyTypeDto, PropertyDataDto>(dto => dto.Id, dto => dto.PropertyTypeId)
.InnerJoin<DataTypeDto>()
.On<PropertyTypeDto, DataTypeDto>(dto => dto.DataTypeId, dto => dto.DataTypeId)
.Where("umbracoNode.nodeObjectType = @nodeObjectType", new { nodeObjectType = nodeObjectType });

return sql;
}

protected virtual Sql GetBase(bool isContent, bool isMedia, Action<Sql> customFilter)
{
return GetBase(isContent, isMedia, customFilter, false);
Expand Down Expand Up @@ -638,13 +526,13 @@ protected override void DisposeResources()
public bool Exists(Guid key)
{
var sql = new Sql().Select("COUNT(*)").From("umbracoNode").Where("uniqueID=@uniqueID", new {uniqueID = key});
return _work.Database.ExecuteScalar<int>(sql) > 0;
return UnitOfWork.Database.ExecuteScalar<int>(sql) > 0;
}

public bool Exists(int id)
{
var sql = new Sql().Select("COUNT(*)").From("umbracoNode").Where("id=@id", new { id = id });
return _work.Database.ExecuteScalar<int>(sql) > 0;
return UnitOfWork.Database.ExecuteScalar<int>(sql) > 0;
}

#region private classes
Expand Down
21 changes: 0 additions & 21 deletions src/Umbraco.Core/Services/EntityService.cs
View file
Expand Up @@ -234,27 +234,6 @@ public virtual IEnumerable<IUmbracoEntity> GetChildren(int parentId, UmbracoObje
}
}

/// <summary>
/// Gets a collection of children by the parent's Id and UmbracoObjectType without adding property data
/// </summary>
/// <param name="parentId">Id of the parent to retrieve children for</param>
/// <returns>An enumerable list of <see cref="IUmbracoEntity"/> objects</returns>
internal IEnumerable<IUmbracoEntity> GetMediaChildrenWithoutPropertyData(int parentId)
{
var objectTypeId = UmbracoObjectTypes.Media.GetGuid();
using (var uow = UowProvider.GetUnitOfWork(readOnly: true))
{
var repository = RepositoryFactory.CreateEntityRepository(uow);
var query = Query<IUmbracoEntity>.Builder.Where(x => x.ParentId == parentId);

// Not pretty having to cast the repository, but it is the only way to get to use an internal method that we
// do not want to make public on the interface. Unfortunately also prevents this from being unit tested.
// See this issue for details on why we need this:
// https://github.com/umbraco/Umbraco-CMS/issues/3457
return ((EntityRepository)repository).GetMediaByQueryWithoutPropertyData(query);
}
}

/// <summary>
/// Returns a paged collection of children
/// </summary>
Expand Down
27 changes: 3 additions & 24 deletions src/Umbraco.Tests/Models/UmbracoEntityTests.cs
View file
Expand Up @@ -177,18 +177,7 @@ public void Can_Deep_Clone()
};
item.AdditionalData.Add("test1", 3);
item.AdditionalData.Add("test2", "valuie");

item.AdditionalData.Add("test3", new UmbracoEntity.EntityProperty()
{
Value = "test",
PropertyEditorAlias = "TestPropertyEditor"
});
item.AdditionalData.Add("test4", new UmbracoEntity.EntityProperty()
{
Value = "test2",
PropertyEditorAlias = "TestPropertyEditor2"
});


var clone = (UmbracoEntity)item.DeepClone();

Assert.AreNotSame(clone, item);
Expand Down Expand Up @@ -250,20 +239,10 @@ public void Can_Serialize_Without_Error()
};
item.AdditionalData.Add("test1", 3);
item.AdditionalData.Add("test2", "valuie");
item.AdditionalData.Add("test3", new UmbracoEntity.EntityProperty()
{
Value = "test",
PropertyEditorAlias = "TestPropertyEditor"
});
item.AdditionalData.Add("test4", new UmbracoEntity.EntityProperty()
{
Value = "test2",
PropertyEditorAlias = "TestPropertyEditor2"
});


var result = ss.ToStream(item);
var json = result.ResultStream.ToJsonString();
Debug.Print(json);
}
}
}
}
3 changes: 0 additions & 3 deletions src/Umbraco.Web/Trees/ContentTreeController.cs
View file
Expand Up @@ -207,9 +207,6 @@ protected override bool HasPathAccess(string id, FormDataCollection queryStrings
return HasPathAccess(entity, queryStrings);
}

internal override IEnumerable<IUmbracoEntity> GetChildrenFromEntityService(int entityId)
=> Services.EntityService.GetChildren(entityId, UmbracoObjectType).ToList();

/// <summary>
/// Returns a collection of all menu items that can be on a content node
/// </summary>
Expand Down

0 comments on commit 23b8e1c

Please sign in to comment.