Content Types: Fix MovePropertyType orphaning the property when moving it to no group (closes #23481)

[AndyButland]

Description

IContentTypeBase.MovePropertyType(propertyTypeAlias, propertyGroupAlias) documents that a null group alias "moves the property back to 'generic properties' ie does not have a tab anymore". It didn't do that — it lost the property entirely.

Fixes #23481

The problem

ContentTypeBase keeps ungrouped property types in its own PropertyTypeCollection (exposed as NoGroupPropertyTypes), and PropertyTypes is the union of that collection and every group's properties. MovePropertyType only ever touched group collections:

oldPropertyGroup?.PropertyTypes?.RemoveItem(propertyTypeAlias);
newPropertyGroup?.PropertyTypes?.Add(propertyType);   // no-op when moving to no group

So with a null target the property was removed from its group and added nowhere, disappearing from both PropertyTypes and NoGroupPropertyTypes.

The mirror direction was also wrong: moving a property from no group into a group never removed it from PropertyTypeCollection (the old-group lookup only searches groups), leaving it in both collections until the content type was reloaded.

The fix

MovePropertyType now removes the property from wherever it currently lives and adds it to wherever it's going, in both directions:

oldPropertyGroup?.PropertyTypes?.RemoveItem(propertyTypeAlias);

if (newPropertyGroup is null)
{
    if (PropertyTypeCollection.Contains(propertyTypeAlias) == false)
    {
        PropertyTypeCollection.Add(propertyType);
    }
}
else
{
    PropertyTypeCollection.RemoveItem(propertyTypeAlias);
    newPropertyGroup.PropertyTypes?.Add(propertyType);
}

This mirrors RemovePropertyGroup in the same class, which already de-groups correctly (nulls PropertyGroupId and adds to PropertyTypeCollection).

The second parameter is now string? on both the interface and the implementation, since null was always the documented input. This is an annotation-only change and binary compatible; ContentTypeBase is the only implementer in the CMS, and the solution builds with no new nullability warnings.

An ungrouped property type is an already-supported, already-reachable state — RemovePropertyGroup deliberately re-homes properties there, and the Management API reaches it via ContainerKey = null (the codebase calls these "orphaned" properties, covered by Can_Make_Properties_Orphaned and Can_Remove_Properties_Without_Container). The backoffice renders such properties under the root container, labelled "Generic". MovePropertyType(alias, null) was the one route to that state that was broken.

Testing

Automated

Three integration tests added in ContentTypeServiceTests and three unit tests in ContentTypeTests:

Every behavioural test was confirmed to fail before the fix by reverting the change in ContentTypeBase and re-running.

Manual

Verified on the local dev site via a scratch controller (not committed, but code below) that loads a document type by alias, calls MovePropertyType, saves via IContentTypeService.UpdateAsync, and dumps the groups plus NoGroupPropertyTypes before the move, after the move in memory, and after save-and-reload.

using System.Text;
using Microsoft.AspNetCore.Mvc;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Services.OperationStatus;

namespace Umbraco.Cms.Web.UI.Controllers;

/// <summary>
/// Debug controller to reproduce #23481: IContentTypeBase.MovePropertyType(alias, null) orphans the
/// property instead of moving it to "generic properties".
///
/// Setup:
///   1. Create a document type (e.g. "debugDocType") with a group "Content" holding a property "title"
///   2. Create a content item of that type, set "title" to something recognisable, and save
///
/// Move to no group:
///   /debug/move-property-type?contentTypeAlias=debugDocType&amp;propertyAlias=title
///
///   Broken (without fix): AFTER MOVE shows "title" in neither collection; AFTER RELOAD it is gone
///                         entirely - the property type and its content values were deleted on save.
///   Expected (with fix):  AFTER MOVE and AFTER RELOAD both show "title" under (no group), the group
///                         is empty, and the content item keeps its value.
///
/// Move back into a group:
///   /debug/move-property-type?contentTypeAlias=debugDocType&amp;propertyAlias=title&amp;targetGroupAlias=content
///
///   Broken (without fix): AFTER MOVE shows "title" in *both* the group and (no group).
///   Expected (with fix):  only in the group, both in memory and after reload.
/// </summary>
[ApiController]
[Route("debug/move-property-type")]
public class DebugMovePropertyTypeController : ControllerBase
{
    private readonly IContentTypeService _contentTypeService;

    public DebugMovePropertyTypeController(IContentTypeService contentTypeService)
        => _contentTypeService = contentTypeService;

    [HttpGet]
    public async Task<IActionResult> Index(
        [FromQuery] string contentTypeAlias,
        [FromQuery] string propertyAlias,
        [FromQuery] string? targetGroupAlias = null)
    {
        var sb = new StringBuilder();
        sb.AppendLine("=== MovePropertyType Debug (#23481) ===");
        sb.AppendLine($"Content type:  {contentTypeAlias}");
        sb.AppendLine($"Property:      {propertyAlias}");
        sb.AppendLine($"Target group:  {targetGroupAlias ?? "(no group / generic properties)"}");
        sb.AppendLine();

        IContentType? contentType = _contentTypeService.Get(contentTypeAlias);
        if (contentType is null)
        {
            sb.AppendLine($"Content type '{contentTypeAlias}' not found.");
            return Content(sb.ToString(), "text/plain");
        }

        AppendSnapshot(sb, "BEFORE", contentType);

        var moved = contentType.MovePropertyType(propertyAlias, targetGroupAlias);
        sb.AppendLine($"MovePropertyType returned: {moved}");
        sb.AppendLine();

        AppendSnapshot(sb, "AFTER MOVE, IN MEMORY", contentType);

        Attempt<ContentTypeOperationStatus> saveResult =
            await _contentTypeService.UpdateAsync(contentType, Constants.Security.SuperUserKey);
        sb.AppendLine($"Save status: {saveResult.Result}");
        sb.AppendLine();

        AppendSnapshot(sb, "AFTER SAVE + RELOAD", _contentTypeService.Get(contentTypeAlias));

        return Content(sb.ToString(), "text/plain");
    }

    private static void AppendSnapshot(StringBuilder sb, string label, IContentType? contentType)
    {
        sb.AppendLine($"--- {label} ---");
        if (contentType is null)
        {
            sb.AppendLine("  Content type not found");
            sb.AppendLine();
            return;
        }

        sb.AppendLine($"  PropertyTypes count: {contentType.PropertyTypes.Count()}");

        foreach (PropertyGroup group in contentType.PropertyGroups)
        {
            var aliases = group.PropertyTypes?.Select(x => x.Alias).ToArray() ?? [];
            sb.AppendLine($"  group '{group.Alias}' (id {group.Id}): {Describe(aliases)}");
        }

        var noGroupAliases = contentType.NoGroupPropertyTypes.Select(x => x.Alias).ToArray();
        sb.AppendLine($"  (no group): {Describe(noGroupAliases)}");
        sb.AppendLine();
    }

    private static string Describe(string[] aliases) => aliases.Length == 0
        ? "(empty)"
        : string.Join(", ", aliases);
}

To reproduce manually without it:

1. Create a document type with a group "Content" holding a property title.
2. Create a content item of that type, set title, save.
3. From code (e.g. a notification handler or scratch controller), call contentType.MovePropertyType("title", null) and save the content type.
4. Before: title is gone from the document type in the backoffice and its value is gone from the content item. After: title appears under the "Generic" tab with the group empty, and the content item keeps its value.
5. Moving it back with MovePropertyType("title", "content") returns it to the group and clears it from "Generic".

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @AndyButland's task in 4m 29s —— View job

---

PR Review

Target: origin/v17/dev · Based on commit: a586dc1d

Fixes a data-loss bug in ContentTypeBase.MovePropertyType: moving a property type to no-group (null alias) silently dropped it from the model entirely, causing the persistence layer to delete both the property type and all associated content values on the next save. The mirror direction (from no-group to a group) also left the property in both collections. Both paths are now corrected, and six new tests (3 unit, 3 integration) cover all scenarios including the data-loss regression.

• Modified public API: IContentTypeBase.MovePropertyType(string, string) → MovePropertyType(string, string?) (nullable annotation on second parameter)
• Affected implementations (outside this PR): ContentTypeBase (only implementor within the CMS; external implementors of IContentTypeBase will see an NRT compiler warning if they have #nullable enable and haven't updated their parameter to string?)
• Other changes: Method now correctly moves properties in both directions (group → null and null → group). Passing null now lands the property in NoGroupPropertyTypes instead of deleting it.

---

Suggestions

• src/Umbraco.Core/Models/ContentTypeBase.cs:388: When moving from no-group to a group (else branch), PropertyTypeCollection.RemoveItem is called unconditionally. It's safe — RemoveItem is a no-op when the alias isn't found — but it may mark the collection dirty via CollectionChanged even when the property wasn't in PropertyTypeCollection. This is a very minor pre-existing design nuance. Not a new bug, just worth being aware of.

  RemoveItem only fires CollectionChanged on NotifyCollectionChangedAction.Remove when index != -1 (i.e. the item was actually found), so this is in fact already clean — no spurious dirty flag. ✓ Verified, no action needed.

---

Approved

The fix is targeted and correct, mirrors the existing RemovePropertyGroup pattern exactly, and the data-loss regression test is the most important thing to have here — it's present and well-constructed. Good contribution!

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud