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
3 changes: 1 addition & 2 deletions src/Umbraco.Core/Models/Content.cs
Original file line number Diff line number Diff line change
Expand Up @@ -405,9 +405,8 @@ public override bool WasPropertyDirty(string propertyName)
public IContent DeepCloneWithResetIdentities()
{
var clone = (Content)DeepClone();
clone.Key = Guid.Empty;
clone.VersionId = clone.PublishedVersionId = 0;
clone.ResetIdentity();
clone.VersionId = clone.PublishedVersionId = 0;

foreach (IProperty property in clone.Properties)
{
Expand Down
4 changes: 2 additions & 2 deletions src/Umbraco.Core/Models/ContentTypeBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -470,8 +470,9 @@ public override void ResetDirtyProperties()
public IContentTypeBase DeepCloneWithResetIdentities(string alias)
{
var clone = (IContentTypeBase)DeepClone();
clone.ResetIdentity();
clone.Alias = alias;
clone.Key = Guid.Empty;

foreach (PropertyGroup propertyGroup in clone.PropertyGroups)
{
propertyGroup.ResetIdentity();
Expand All @@ -484,7 +485,6 @@ public IContentTypeBase DeepCloneWithResetIdentities(string alias)
propertyType.ResetDirtyProperties(false);
}

clone.ResetIdentity();
clone.ResetDirtyProperties(false);
return clone;
}
Expand Down
49 changes: 47 additions & 2 deletions src/Umbraco.Core/Models/Entities/EntityBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public abstract class EntityBase : BeingDirtyBase, IEntity
private bool _hasIdentity;
private int _id;
private Guid _key;
private bool _keyIsAssigned;
Comment thread
AndyButland marked this conversation as resolved.
private DateTime _createDate;
private DateTime _updateDate;

Expand All @@ -33,6 +34,12 @@ public int Id
}
}

/// <summary>
/// Gets a value indicating whether the Key can be changed after the entity has identity.
/// Override this to return <c>true</c> for entities where Key is derived from other properties (e.g., file path).
/// </summary>
protected virtual bool SupportsKeyChange => false;
Comment thread
AndyButland marked this conversation as resolved.

/// <inheritdoc />
[DataMember]
public Guid Key
Expand All @@ -47,7 +54,33 @@ public Guid Key

return _key;
}
set => SetPropertyValueAndDetectChanges(value, ref _key, nameof(Key));
set
{
// The Key (GUID) should be immutable once an entity is persisted to the database.
// We throw if ALL of these conditions are true:
// - !SupportsKeyChange: entity doesn't allow Key changes (file-based entities override this)
// - HasIdentity: entity has been persisted (Id != 0)
// - _keyIsAssigned: Key was previously set while entity had identity (i.e., loaded from DB or set after save)
// - value != Guid.Empty: not resetting the Key (allowed for cloning/identity reset)
// - value != _key: actually trying to change to a different value
if (!SupportsKeyChange && HasIdentity && _keyIsAssigned && value != Guid.Empty && value != _key)
{
throw new InvalidOperationException($"Cannot change the Key of an existing {GetType().Name}.");
}

SetPropertyValueAndDetectChanges(value, ref _key, nameof(Key));

// Track that Key has been assigned, but only when the entity already has identity.
// This distinction is important:
// - Before HasIdentity (new entity): Key can be set freely during setup, _keyIsAssigned stays false
// - After HasIdentity (persisted entity): first assignment sets _keyIsAssigned = true, blocking future changes
// Note: _keyIsAssigned can only be reset via ResetIdentity(), not by setting Key to Guid.Empty.
// This prevents bypassing the immutability check by setting Key = Guid.Empty then Key = newValue.
if (HasIdentity && value != Guid.Empty)
{
_keyIsAssigned = true;
}
}
}

/// <inheritdoc />
Expand All @@ -71,7 +104,10 @@ public DateTime UpdateDate
public DateTime? DeleteDate { get; set; } // no change tracking - not persisted

/// <inheritdoc />
[DataMember]
/// <remarks>
/// Not serialized as [DataMember] because it's derived from Id.
/// When Id is deserialized, the setter sets _hasIdentity correctly.
/// </remarks>
public virtual bool HasIdentity => _hasIdentity;

/// <summary>
Expand All @@ -81,9 +117,18 @@ public virtual void ResetIdentity()
{
_id = default;
_key = Guid.Empty;
_keyIsAssigned = false;
_hasIdentity = false;
}

/// <summary>
/// Called after deserialization to restore the _keyIsAssigned flag.
/// This ensures Key immutability is enforced regardless of property deserialization order.
/// </summary>
[OnDeserialized]
private void OnDeserialized(StreamingContext context) =>
_keyIsAssigned = HasIdentity && _key != Guid.Empty;

public virtual bool Equals(EntityBase? other) =>
other != null && (ReferenceEquals(this, other) || SameIdentityAs(other));

Expand Down
5 changes: 5 additions & 0 deletions src/Umbraco.Core/Models/File.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ public abstract class File : EntityBase, IFile
private string? _name;
private string _path;

/// <summary>
/// File-based entities derive their Key from the file path, so Key must be allowed to change when the path changes.
/// </summary>
protected override bool SupportsKeyChange => true;

protected File(string path, Func<File, string?>? getFileContent = null)
{
_path = SanitizePath(path);
Expand Down
1 change: 0 additions & 1 deletion src/Umbraco.Core/Models/IDataType.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ public interface IDataType : IUmbracoEntity, IRememberBeingDirty
IDataType DeepCloneWithResetIdentities()
{
var clone = (DataType)DeepClone();
clone.Key = Guid.Empty;
clone.ResetIdentity();
return clone;
}
Expand Down
15 changes: 10 additions & 5 deletions src/Umbraco.Infrastructure/Packaging/PackageDataInstallation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -944,13 +944,16 @@
{
throw new InvalidOperationException("Content type was null");
}
contentType.Key = key;
contentType.Name = infoElement!.Element("Name")!.Value;
if (infoElement.Element("Key") != null)

// Only set Key on new entities - existing entities already have their Key from the database
// and it should not be changed (Key is immutable once persisted).
if (contentType.HasIdentity is false)
{
contentType.Key = new Guid(infoElement.Element("Key")!.Value);
contentType.Key = key;
}

contentType.Name = infoElement!.Element("Name")!.Value;

contentType.Icon = infoElement.Element("Icon")?.Value;
contentType.Thumbnail = infoElement.Element("Thumbnail")?.Value;
contentType.Description = infoElement.Element("Description")?.Value;
Expand Down Expand Up @@ -1134,7 +1137,9 @@
contentType.AddPropertyGroup(alias, name);
PropertyGroup propertyGroup = contentType.PropertyGroups[alias];

if (Guid.TryParse(propertyGroupElement.Element("Key")?.Value, out Guid key))
// Only set Key on new property groups - existing ones already have their Key from the database
if (propertyGroup.HasIdentity is false &&
Guid.TryParse(propertyGroupElement.Element("Key")?.Value, out Guid key))

Check warning on line 1142 in src/Umbraco.Infrastructure/Packaging/PackageDataInstallation.cs

View check run for this annotation

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

❌ New issue: Complex Method

UpdateContentTypesPropertyGroups has a cyclomatic complexity of 9, 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.
{
propertyGroup.Key = key;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3891,4 +3891,23 @@ public class ContentNotificationHandler :

return (langEn, langDa, contentType);
}

[Test]
public void Cannot_Change_Key_Of_Persisted_Content()
{
// Arrange - get a persisted content item
var content = ContentService.GetById(Textpage.Id);
Assert.That(content, Is.Not.Null);

var originalKey = content!.Key;
var newKey = Guid.NewGuid();

// Act & Assert - attempting to change the Key should throw
var exception = Assert.Throws<InvalidOperationException>(() => content.Key = newKey);
Assert.That(exception!.Message, Does.Contain("Cannot change the Key"));
Assert.That(exception.Message, Does.Contain("Content"));

// Verify the Key was not changed
Assert.That(content.Key, Is.EqualTo(originalKey));
}
}
Loading
Loading