Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions src/Umbraco.Core/Models/ContentTypeBase.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Collections.Specialized;

Check notice on line 1 in src/Umbraco.Core/Models/ContentTypeBase.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (v13/dev)

✅ Getting better: Primitive Obsession

The ratio of primitive types in function arguments decreases from 66.67% to 62.96%, threshold = 30.0%. The functions in this file have too many primitive types (e.g. int, double, float) in their function argument lists. Using many primitive types lead to the code smell Primitive Obsession. Avoid adding more primitive arguments.

Check notice on line 1 in src/Umbraco.Core/Models/ContentTypeBase.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (v13/dev)

✅ No longer an issue: String Heavy Function Arguments

The ratio of strings in function arguments is no longer above the threshold
using System.Diagnostics;
using System.Runtime.Serialization;
using Umbraco.Cms.Core.Models.Entities;
Expand Down Expand Up @@ -132,6 +132,30 @@
}
}

/// <summary>
/// Detects if any persisted property types have been removed by comparing old and new collections,
/// and sets <see cref="HasPropertyTypeBeenRemoved"/> accordingly.
/// </summary>
private void DetectPropertyTypeRemovals(IEnumerable<IPropertyType> oldPropertyTypes, IEnumerable<IPropertyType> newPropertyTypes)
{
if (HasPropertyTypeBeenRemoved)
{
return;
}

var oldIds = new HashSet<int>(oldPropertyTypes.Select(pt => pt.Id).Where(id => id > 0));
if (oldIds.Count == 0)
{
return;
}

var newIds = new HashSet<int>(newPropertyTypes.Select(pt => pt.Id).Where(id => id > 0));
if (oldIds.Any(id => !newIds.Contains(id)))
{
HasPropertyTypeBeenRemoved = true;
}
}

/// <summary>
/// PropertyTypes that are not part of a PropertyGroup
/// </summary>
Expand Down Expand Up @@ -273,6 +297,7 @@
{
if (PropertyTypeCollection != null)
{
DetectPropertyTypeRemovals(PropertyTypeCollection, value);
PropertyTypeCollection.ClearCollectionChangedEvents();
}

Expand Down Expand Up @@ -443,21 +468,21 @@
/// Please note that resetting the dirty properties could potentially
/// obstruct the saving of a new or updated entity.
/// </remarks>
public override void ResetDirtyProperties()
public override void ResetDirtyProperties(bool rememberDirty)
{
base.ResetDirtyProperties();
base.ResetDirtyProperties(rememberDirty);

// loop through each property group to reset the property types
var propertiesReset = new List<int>();

foreach (PropertyGroup propertyGroup in PropertyGroups)
{
propertyGroup.ResetDirtyProperties();
propertyGroup.ResetDirtyProperties(rememberDirty);
if (propertyGroup.PropertyTypes is not null)
{
foreach (IPropertyType propertyType in propertyGroup.PropertyTypes)
{
propertyType.ResetDirtyProperties();
propertyType.ResetDirtyProperties(rememberDirty);
propertiesReset.Add(propertyType.Id);
}
}
Expand All @@ -467,7 +492,7 @@
// but don't re-reset ones we've already done.
foreach (IPropertyType propertyType in PropertyTypes.Where(x => propertiesReset.Contains(x.Id) == false))
{
propertyType.ResetDirtyProperties();
propertyType.ResetDirtyProperties(rememberDirty);
}
}

Expand Down
21 changes: 21 additions & 0 deletions src/Umbraco.Core/Models/ContentTypeCompositionBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ public abstract class ContentTypeCompositionBase : ContentTypeBase, IContentType
{
private List<IContentTypeComposition> _contentTypeComposition = new();
private List<int> _removedContentTypeKeyTracker = new();
private bool _hasCompositionBeenRemoved;

protected ContentTypeCompositionBase(IShortStringHelper shortStringHelper, int parentId)
: base(shortStringHelper, parentId)
Expand Down Expand Up @@ -104,6 +105,24 @@ IPropertyType AcquireProperty(IPropertyType propertyType)
}
}

/// <summary>
/// A boolean flag indicating if a composition has been removed from this instance.
/// </summary>
/// <remarks>
/// This is currently (specifically) used in order to know that we need to refresh the content cache which
/// needs to occur when a composition has been removed from a content type
/// </remarks>
[IgnoreDataMember]
internal bool HasCompositionTypeBeenRemoved
{
get => _hasCompositionBeenRemoved;
private set
{
_hasCompositionBeenRemoved = value;
OnPropertyChanged(nameof(HasCompositionTypeBeenRemoved));
}
}

/// <inheritdoc />
public IEnumerable<IPropertyType> GetOriginalComposedPropertyTypes() => GetRawComposedPropertyTypes();

Expand Down Expand Up @@ -179,6 +198,8 @@ public bool RemoveContentType(string alias)
_removedContentTypeKeyTracker.AddRange(compositionIdsToRemove);
}

HasCompositionTypeBeenRemoved = true;

OnPropertyChanged(nameof(ContentTypeComposition));

return _contentTypeComposition.Remove(contentTypeComposition);
Expand Down
39 changes: 39 additions & 0 deletions src/Umbraco.Core/Models/PropertyGroup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public class PropertyGroup : EntityBase, IEquatable<PropertyGroup>

private string _alias;
private string? _name;
private bool _hasPropertyTypeBeenRemoved;
private PropertyTypeCollection? _propertyTypes;
private int _sortOrder;

Expand Down Expand Up @@ -94,6 +95,20 @@ public int SortOrder
set => SetPropertyValueAndDetectChanges(value, ref _sortOrder, nameof(SortOrder));
}

/// <summary>
/// A boolean flag indicating if a property type has been removed from this group.
/// </summary>
[IgnoreDataMember]
internal bool HasPropertyTypeBeenRemoved
{
get => _hasPropertyTypeBeenRemoved;
private set
{
_hasPropertyTypeBeenRemoved = value;
OnPropertyChanged(nameof(HasPropertyTypeBeenRemoved));
}
}

/// <summary>
/// Gets or sets a collection of property types for the group.
/// </summary>
Expand All @@ -112,6 +127,7 @@ public PropertyTypeCollection? PropertyTypes
{
if (_propertyTypes != null)
{
DetectPropertyTypeRemovals(_propertyTypes, value);
_propertyTypes.ClearCollectionChangedEvents();
}

Expand Down Expand Up @@ -155,4 +171,27 @@ protected override void PerformDeepClone(object clone)

private void PropertyTypesChanged(object? sender, NotifyCollectionChangedEventArgs e) =>
OnPropertyChanged(nameof(PropertyTypes));

private void DetectPropertyTypeRemovals(PropertyTypeCollection oldPropertyTypes, PropertyTypeCollection? newPropertyTypes)
{
if (HasPropertyTypeBeenRemoved)
{
return;
}

var oldIds = new HashSet<int>(oldPropertyTypes.Select(pt => pt.Id).Where(id => id > 0));
if (oldIds.Count == 0)
{
return;
}

var newIds = newPropertyTypes != null
? new HashSet<int>(newPropertyTypes.Select(pt => pt.Id).Where(id => id > 0))
: new HashSet<int>();

if (oldIds.Any(id => !newIds.Contains(id)))
{
HasPropertyTypeBeenRemoved = true;
}
}
}
16 changes: 16 additions & 0 deletions src/Umbraco.Core/Services/Changes/ContentTypeChangeExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,20 @@ public static bool HasTypesAny(this ContentTypeChangeTypes change, ContentTypeCh

public static bool HasTypesNone(this ContentTypeChangeTypes change, ContentTypeChangeTypes types) =>
(change & types) == ContentTypeChangeTypes.None;

/// <summary>
/// Determines whether the change has structural change impact.
/// </summary>
/// <param name="change">The change to check.</param>
/// <returns><c>true</c> if the change has structural impact; otherwise, <c>false</c>.</returns>
public static bool IsStructuralChange(this ContentTypeChangeTypes change) =>
change.HasType(ContentTypeChangeTypes.RefreshMain);

/// <summary>
/// Determines whether the change has non-structural change impact.
/// </summary>
/// <param name="change">The change to check.</param>
/// <returns><c>true</c> if the change has non-structural impact; otherwise, <c>false</c>.</returns>
public static bool IsNonStructuralChange(this ContentTypeChangeTypes change) =>
change.HasType(ContentTypeChangeTypes.RefreshOther) && !change.HasType(ContentTypeChangeTypes.RefreshMain);
}
21 changes: 18 additions & 3 deletions src/Umbraco.Core/Services/Changes/ContentTypeChangeTypes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,29 @@ public enum ContentTypeChangeTypes : byte
Create = 1,

/// <summary>
/// Content type changes impact only the Content type being saved
/// Content type changes directly impact existing content of this content type.
/// </summary>
/// <remarks>
/// These changes are "destructive" of nature. They include:
/// - Changing the content type alias.
/// - Removing a property type or a composition.
/// - Changing the alias of a property type (this effectively corresponds to removing a property type).
/// - Changing variance, either at property or content type level.
/// </remarks>
RefreshMain = 2,

/// <summary>
/// Content type changes impacts the content type being saved and others used that are composed of it
/// Content type changes that do not directly impact existing content of this content type.
/// </summary>
RefreshOther = 4, // changed, other change
/// <remarks>
/// These changes are "constructive" of nature, and include all changes not included in
/// <see cref="RefreshMain"/> - for example:
/// - Adding a property type or a composition.
/// - Rearranging property types or groups.
/// - Changes to name, description, icon etc.
/// - Changes to other content type settings, i.e. allowed child types and version cleanup.
/// </remarks>
RefreshOther = 4,

/// <summary>
/// Content type was removed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,10 @@
});

// removed properties?
var hasAnyPropertyBeenRemoved = dirty.WasPropertyDirty("HasPropertyTypeBeenRemoved");
// check both the content type level flag (set by RemovePropertyType) and
// individual property group flags (set when PropertyTypes collection is replaced, e.g. by the mapper)
var hasAnyPropertyBeenRemoved = dirty.WasPropertyDirty("HasPropertyTypeBeenRemoved")
|| contentType.PropertyGroups.Any(g => g.WasPropertyDirty("HasPropertyTypeBeenRemoved"));

Check warning on line 232 in src/Umbraco.Core/Services/ContentTypeServiceBaseOfTRepositoryTItemTService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (v13/dev)

❌ Getting worse: Complex Method

ComposeContentTypeChanges increases in cyclomatic complexity from 13 to 14, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.

// removed compositions?
var hasAnyCompositionBeenRemoved = dirty.WasPropertyDirty("HasCompositionTypeBeenRemoved");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,49 +52,45 @@
throw new NotSupportedException();
}

var changedIds = new Dictionary<string, (List<int> removedIds, List<int> refreshedIds, List<int> otherIds)>();
var changedIds = new Dictionary<string, (List<int> removedIds, List<int> refreshedIds)>();

foreach (ContentTypeCacheRefresher.JsonPayload payload in (ContentTypeCacheRefresher.JsonPayload[])args
.MessageObject)
{
if (!changedIds.TryGetValue(
payload.ItemType,
out (List<int> removedIds, List<int> refreshedIds, List<int> otherIds) idLists))
out (List<int> removedIds, List<int> refreshedIds) idLists))
{
idLists = (removedIds: new List<int>(), refreshedIds: new List<int>(), otherIds: new List<int>());
idLists = (removedIds: new List<int>(), refreshedIds: new List<int>());
changedIds.Add(payload.ItemType, idLists);
}

if (payload.ChangeTypes.HasType(ContentTypeChangeTypes.Remove))
{
idLists.removedIds.Add(payload.Id);
}
else if (payload.ChangeTypes.HasType(ContentTypeChangeTypes.RefreshMain))
{
idLists.refreshedIds.Add(payload.Id);
}
else if (payload.ChangeTypes.HasType(ContentTypeChangeTypes.RefreshOther))
{
idLists.otherIds.Add(payload.Id);
}
}

foreach (KeyValuePair<string, (List<int> removedIds, List<int> refreshedIds, List<int> otherIds)> ci in
foreach (KeyValuePair<string, (List<int> removedIds, List<int> refreshedIds)> ci in
changedIds)
{
if (ci.Value.refreshedIds.Count > 0 || ci.Value.otherIds.Count > 0)
if (ci.Value.refreshedIds.Count > 0)
{
switch (ci.Key)
{
case var itemType when itemType == typeof(IContentType).Name:
RefreshContentOfContentTypes(ci.Value.refreshedIds.Concat(ci.Value.otherIds).Distinct()
RefreshContentOfContentTypes(ci.Value.refreshedIds.Distinct()
.ToArray());
break;
case var itemType when itemType == typeof(IMediaType).Name:
RefreshMediaOfMediaTypes(ci.Value.refreshedIds.Concat(ci.Value.otherIds).Distinct().ToArray());
RefreshMediaOfMediaTypes(ci.Value.refreshedIds.Distinct().ToArray());
break;
case var itemType when itemType == typeof(IMemberType).Name:
RefreshMemberOfMemberTypes(ci.Value.refreshedIds.Concat(ci.Value.otherIds).Distinct()
RefreshMemberOfMemberTypes(ci.Value.refreshedIds.Distinct()

Check notice on line 93 in src/Umbraco.Infrastructure/Search/IndexingNotificationHandler.ContentType.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (v13/dev)

✅ Getting better: Complex Method

Handle decreases in cyclomatic complexity from 15 to 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
.ToArray());
break;
}
Expand Down Expand Up @@ -154,6 +150,10 @@
const int pageSize = 500;
var page = 0;
var total = long.MaxValue;

// track which Ids have their paths are published
var publishChecked = new Dictionary<int, bool>();

while (page * pageSize < total)
{
IEnumerable<IContent> contentToRefresh = _contentService.GetPagedOfTypes(
Expand All @@ -165,20 +165,20 @@
// order by shallowest to deepest, this allows us to check it's published state without checking every item
Ordering.By("Path"));

// track which Ids have their paths are published
var publishChecked = new Dictionary<int, bool>();

foreach (IContent c in contentToRefresh)
{
var isPublished = false;
if (c.Published)
{
if (!publishChecked.TryGetValue(c.ParentId, out isPublished))
if (publishChecked.TryGetValue(c.ParentId, out isPublished) is false)
{
// nothing by parent id, so query the service and cache the result for the next child to check against
isPublished = _contentService.IsPathPublished(c);
publishChecked[c.Id] = isPublished;

// the parent *must* be published if the entire path is published
publishChecked[c.ParentId] = isPublished;
}

publishChecked[c.Id] = isPublished;
}

_umbracoIndexingHandler.ReIndexForContent(c, isPublished);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Umbraco.Cms.Core.Events;

Check notice on line 1 in src/Umbraco.PublishedCache.NuCache/PublishedSnapshotServiceEventHandler.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (v13/dev)

✅ Getting better: Code Duplication

reduced similar code in: Handle. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Notifications;
using Umbraco.Cms.Core.PublishedCache;
Expand Down Expand Up @@ -53,9 +53,7 @@

public void Handle(ContentTypeRefreshedNotification notification)
{
const ContentTypeChangeTypes types // only for those that have been refreshed
= ContentTypeChangeTypes.RefreshMain | ContentTypeChangeTypes.RefreshOther;
var contentTypeIds = notification.Changes.Where(x => x.ChangeTypes.HasTypesAny(types)).Select(x => x.Item.Id)
var contentTypeIds = notification.Changes.Where(x => x.ChangeTypes.IsStructuralChange()).Select(x => x.Item.Id)
.ToArray();
if (contentTypeIds.Any())
{
Expand Down
Loading
Loading