Delete references using custom relation types

[bjarnef]

Prerequisites

• I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes #13364

Description

When implementing GetReferences() method in a property editor using UmbracoEntityReference this didn't work for custom relation types, where it tries to insert records which already exists and doesn't remove any previous saved relations after removing items from picker.

An example with a custom property editor using contentpicker property editor and myCustomRelation:

using Umbraco.Cms.Core;
using Umbraco.Cms.Core.IO;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Models.Editors;
using Umbraco.Cms.Core.PropertyEditors;
using Umbraco.Cms.Core.Serialization;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Strings;

namespace Umbraco.Cms.Web.UI.Editors;

[DataEditor(
    name: "Product Picker",
    alias: "productPicker",
    view: "contentpicker",
    Icon = "icon-tag",
    ValueType = ValueTypes.Text,
    Group = Umbraco.Cms.Core.Constants.PropertyEditors.Groups.Pickers)]
public class ProductPickerPropertyEditor : DataEditor
{
    private readonly IEditorConfigurationParser _editorConfigurationParser;
    private readonly IIOHelper _ioHelper;
    private readonly IJsonSerializer _jsonSerializer;

    public ProductPickerPropertyEditor(
        IDataValueEditorFactory dataValueEditorFactory,
        IIOHelper ioHelper,
        IJsonSerializer jsonSerializer,
        IEditorConfigurationParser editorConfigurationParser,
        EditorType type = EditorType.PropertyValue)
        : base(dataValueEditorFactory, type)
    {
        _ioHelper = ioHelper;
        _jsonSerializer = jsonSerializer;
        _editorConfigurationParser = editorConfigurationParser;
        SupportsReadOnly = true;
    }

    /// <inheritdoc />
    protected override IConfigurationEditor CreateConfigurationEditor() =>
        new MultiNodePickerConfigurationEditor(_ioHelper, _editorConfigurationParser);

    /// <inheritdoc />
    //protected override IDataValueEditor CreateValueEditor() =>
    //    DataValueEditorFactory.Create<MultipleValueEditor>(Attribute!);

    protected override IDataValueEditor CreateValueEditor() =>
        DataValueEditorFactory.Create<ProductPickerPropertyValueEditor>(Attribute!);

    public class ProductPickerPropertyValueEditor : DataValueEditor, IDataValueReferenceFactory, IDataValueReference
    {
        public ProductPickerPropertyValueEditor(
            ILocalizedTextService localizedTextService,
            IShortStringHelper shortStringHelper,
            IJsonSerializer jsonSerializer,
            IIOHelper ioHelper,
            DataEditorAttribute attribute)
            : base(localizedTextService, shortStringHelper, jsonSerializer, ioHelper, attribute)
        {

        }

        public IDataValueReference GetDataValueReference() => this;

        public bool IsForEditor(IDataEditor? dataEditor) => dataEditor?.Alias == "productPicker";

        public IEnumerable<UmbracoEntityReference> GetReferences(object? value)
        {
            var asString = value == null ? string.Empty : value is string str ? str : value.ToString();

            if (string.IsNullOrEmpty(asString))
            {
                yield break;
            }

            var udiPaths = asString!.Split(',');

            foreach (var udiPath in udiPaths)
            {
                if (UdiParser.TryParse(udiPath, out Udi? udi))
                {
                    yield return new UmbracoEntityReference(udi, "myCustomRelation");
                }
            }
        }
    }
}

chrome_Niz0VkXzZK.mp4

After including changes in this PR it both get tracked references of type umbDocument, umbMedia and myCustomRelation which are deleted before inserting new/updated relations.

chrome_uvrCl9GxvW.mp4

Maybe the AutomaticRelationTypes constant should be marked as obsolete as it isn't used now? Not sure if it makes sense to have this when developers can implement references of own relation types, so it most likely would need to handle more than these two when it is possible to extend.

Maybe we also need to adjust RelationRepository.DeleteByParent(parentId, relationTypeAliases) with another method to delete for specific child ids e.g. RelationRepository.DeleteByParent(parentId, childIds, relationTypeAliases)

Currently it delete all relations by parent id and afterwards save the new relations, but it also seems to trigger Deliting and Deleted notifications for references which wasn't removed (just deleted and re-created).

var udiToGuids = trackedRelations.Select(x => x.Udi as GuidUdi)
      .ToDictionary(x => (Udi)x!, x => x!.Guid);

// lookup in the DB all INT ids for the GUIDs and chuck into a dictionary
var keyToIds = Database.Fetch<NodeIdKey>(Sql()
    .Select<NodeDto>(x => x.NodeId, x => x.UniqueId)
    .From<NodeDto>()
    .WhereIn<NodeDto>(x => x.UniqueId, udiToGuids.Values))
    .ToDictionary(x => x.UniqueId, x => x.NodeId);

Maybe we could do something like this, so it only delete relations which not exists in trackedRelations collection and saved afterwards again:

var keyToIds = Database.Fetch<NodeIdKey>(Sql()
    .Select<NodeDto>(x => x.NodeId, x => x.UniqueId)
    .From<NodeDto>()
    .WhereIn<NodeDto>(x => x.UniqueId, udiToGuids.Values))
    .ToDictionary(x => x.UniqueId, x => x.NodeId);

var childsToDelete = trackedRelations.Where(x => keyToIds.Any(y => y.Key == x.Udi.AsGuid()));

RelationRepository.DeleteByParent(entity.Id, childsToDelete, relationTypeAliases);

[bjarnef]

I also think Umbraco need to support more than relations for just umbDocument and umbMedia. Although Umbraco currently doesn't support relations to members in e.g. MNTP and Member Picker (and there is no umbMember relation type), it should be possible in custom property editors to create the relations using a custom relation type, but currently as originally mentioned it will cause the duplicate references.

I have target the PR for v11, but it would be great to fix in v10 as well, but other enhancements like relations to members in core pickers could be handed in v11, v12 etc.

[nikolajlauridsen]

Hey @bjarnef just had a look at this and it looks good to me, and tests out good as well.

It makes sense that we should delete the references from the _dataValueReferenceFactories, since these are really automatic relations types 👍 So great work, and thanks as always 🎉

I took the liberty of obsoleting the old constant like you mentioned 😉

In regards to the other enhancements like the member relations and stuff, I think maybe it makes sense to make this a separate feature request, or maybe or discussion or something 😄

[bjarnef]

@nikolajlauridsen great, thanks 👍
However I also wonder if it actually should delete all relations first and re-create them afterwards since it seems to trigger the Deleting and Deleted relation notifications even though the items wasn't removed from an UI/UX point of view?

e.g. if I have item A, B and C and remove item B, it also notify about A and C in the Deleting and Deleted relation notifications.

[nikolajlauridsen]

That's a good question, imo it kind of makes sense, from a system point of view, since the relations are in fact deleted, and then created again, so imo the notifications are telling the truth in this regard.

Of course if this becomes problematic for people maybe we should consider fixing this aspect.

[bjarnef]

Yeah, not sure if it makes sense to notify about all entities, eg. 10 items in Deleting / Deleted notifications. I think it is the same with Saving / Saved notifications in this context, so it is difficult to tell which was changed, new, deleted.

And yes the member relations would be great to make a discussion about this feature. Currently there is also node id on page level. However with Block List and Block Grid editor mayve there's a need to store a reference to the block guid as well. The relations haven't changed much during the years, but I wonder if guid would be better than the int ids in future.

[nikolajlauridsen]

I just realised last night that I might have been a bit too fast to merge this, since there is an issue with this solution.

The problem is that we're relying on the tracked relations themselves to give us the relation type alias. This means that if you try to remove a relation entirely, it won't be deleted.

For instance, if you pick a node with the content picker, then save, and then later remove the picked node and hit save again.

The first time we'll have created a relation since it was added to the trackedRelations, however, the second time around it won't be deleted, since there will be no tracked relation to get the alias from.

Trying to think of how we can fix this, I think one solution would be to extend the IDataValueReference interface with a new method IEnumerable<string> GetReferenceTypeAliases() this way you can implement this to always return the aliases that we need to delete, regardless if anything is picked.

And I see your point with the deleting, however I don't think that the notifications themselves are the issue, the issue is that we're deleting everything to then create it again, however I also think that it has been this way for a long time, and it's really a separate issue to this in my opinion. But yes in general relations seems like they could use a bit of love 😄

[nikolajlauridsen]

And I've created a PR with the proposed changes #13470, will get someone from the core team to have a look at reviewing it 😄

[bjarnef]

@nikolajlauridsen okay, the second time when removing the item from the picker, doesn't the relation still exists, where it fetched the alias from the tracked references and afterwards delete the relation?

Yeah, the relations haven't changed much the last 10 years until the tracked references were implemented. Now with Umbraco Cloud, Deploy, Forms in database (similar to content nodes), etc. I think relations are more important than ever :) and it makes it much easier to developers to check if it is safe to delete different kind of nodes.

I think the next step could be tracking member references in pickers :)

[nikolajlauridsen]

Testing it out this was not the case, but I messed up the terminology a bit, there will be no UmbracoEntityReference returned from the IDataValueReference, since the property value is now empty, so for example this will return an empty enumerable in the contentpicker:

var asString = value == null ? string.Empty : value is string str ? str : value.ToString();

if (string.IsNullOrEmpty(asString))
{
    yield break;
}

So it will not be present in the trackedRelations list, which is where you get the alias from, so the alias will be missing 😄

[bjarnef]

@nikolajlauridsen I have upgraded our project to Umbraco v10.4.0, but I still get this error without this notification handler.

public class CourseCategorySavingNotificationHandler : INotificationHandler<ContentSavingNotification>
{
    private readonly IRelationService _relationService;

    public CourseCategorySavingNotificationHandler(
        IRelationService relationService)
    {
        _relationService = relationService;
    }

    public void Handle(ContentSavingNotification notification)
    {
        var relationType = _relationService.GetRelationTypeByAlias(Constants.RelationTypes.RelatedCourseAlias);
        if (relationType == null)
            return;

        foreach (var node in notification.SavedEntities)
        {
            if (node.ContentType.Alias.Equals(Course.ModelTypeAlias) || node.ContentType.Alias.Equals(Category.ModelTypeAlias))
            {
                // Umbraco currently doesn't remove relations using custom relation type, when using UmbracoEntityReference in property editor,
                // so we need to remove these before saved.

                var relations = _relationService.GetByParentId(node.Id, Constants.RelationTypes.RelatedCourseAlias);

                if (relations != null)
                {
                    foreach (var relation in relations)
                    {
                        _relationService.Delete(relation);
                    }
                }
            }
        }
    }
}

The property editor is pretty identical with MNTP - only difference is that it use a custom relation type.

[DataEditor(
    name: "Category Relation Picker",
    alias: Constants.PropertyEditors.Aliases.CategoryRelationPicker,
    view: "~/App_Plugins/CategoryRelationPicker/views/categoryrelationpicker.html",
    Icon = "icon-mindmap",
    ValueType = ValueTypes.Text,
    Group = Umbraco.Cms.Core.Constants.PropertyEditors.Groups.Pickers)]
public class CategoryRelationPickerPropertyEditor : DataEditor
{
    private readonly IEditorConfigurationParser _editorConfigurationParser;
    private readonly IIOHelper _ioHelper;
    private readonly IJsonSerializer _jsonSerializer;

    public CategoryRelationPickerPropertyEditor(
        IDataValueEditorFactory dataValueEditorFactory,
        IIOHelper ioHelper,
        IJsonSerializer jsonSerializer,
        IEditorConfigurationParser editorConfigurationParser,
        EditorType type = EditorType.PropertyValue)
        : base(dataValueEditorFactory, type)
    {
        _ioHelper = ioHelper;
        _jsonSerializer = jsonSerializer;
        _editorConfigurationParser = editorConfigurationParser;
        SupportsReadOnly = true;
    }

    /// <inheritdoc />
    protected override IConfigurationEditor CreateConfigurationEditor() =>
        new CategoryRelationPickerConfigurationEditor(_ioHelper, _jsonSerializer, _editorConfigurationParser);

    /// <inheritdoc />
    //protected override IDataValueEditor CreateValueEditor() =>
    //    DataValueEditorFactory.Create<MultipleValueEditor>(Attribute!);

    protected override IDataValueEditor CreateValueEditor() =>
        DataValueEditorFactory.Create<CategoryRelationPickerPropertyValueEditor>(Attribute!);

    public class CategoryRelationPickerPropertyValueEditor : DataValueEditor, IDataValueReferenceFactory, IDataValueReference
    {
        public CategoryRelationPickerPropertyValueEditor(
            ILocalizedTextService localizedTextService,
            IShortStringHelper shortStringHelper,
            IJsonSerializer jsonSerializer,
            IIOHelper ioHelper,
            DataEditorAttribute attribute)
            : base(localizedTextService, shortStringHelper, jsonSerializer, ioHelper, attribute)
        {

        }

        public IDataValueReference GetDataValueReference() => this;

        public bool IsForEditor(IDataEditor? dataEditor) => dataEditor?.Alias == Constants.PropertyEditors.Aliases.CategoryRelationPicker;

        public IEnumerable<UmbracoEntityReference> GetReferences(object? value)
        {
            var asString = value == null ? string.Empty : value is string str ? str : value.ToString();

            if (string.IsNullOrEmpty(asString))
            {
                yield break;
            }

            var udiPaths = asString!.Split(',');

            foreach (var udiPath in udiPaths)
            {
                if (UdiParser.TryParse(udiPath, out Udi? udi))
                {
                    yield return new UmbracoEntityReference(udi, Constants.RelationTypes.RelatedCourseAlias);
                }
            }
        }
    }

[nikolajlauridsen]

You have to implement GetAutomaticRelationTypesAliases as mentioned in #13470 😄

[bjarnef]

@nikolajlauridsen okay, I though it was picked up from registered property editors.. missed that part 😊

Works now 👍

Not sure if it can be done globally in case one have multiple pickers and want to use same relation type for each of these.