Cloning doesn't generate new id's for embedded elements

[stevo-knievo]

Describe the bug

When I clone a document via the CMS UI. The document gets a new ID (and a new version ID) but not the embedded elements, which are included via a bag content part.

To Reproduce Steps to reproduce the behaviour:

1. Create content-type: Bar
2. On Add Parts To "Bar", add: Title
3. Create a second content-type: Foo
4. On Add Parts To "Foo", add: Bag, Title
5. Edit that content-type "Foo": Edit Bag and select the "Bar" content-type at "Contained Content Types"
6. Create a new content item from content-type: Foo, with a title and at least one Bar item
7. Using the graphQL query below to query the just created a content item from content-type: Foo
8. Clone the content just created a content item from content-type: Foo
9. Using the same query to query both items

Expected behaviour I was expecting that a clone content item has new ids for all embedded elements.

GraphQL query

query fooQuery {
  foo {
    contentType
    contentItemId
    contentItemVersionId
    displayText
    bag {
      contentItems {
        contentItemId
        contentType
        displayText
      }
    }
  }
}

Query result from the first created content item

{
  "data": {
    "foo": [
      {
        "contentType": "Foo",
        "contentItemId": "4afm7w6fcdddvv4jnh7bym6y5f",
        "contentItemVersionId": "4cezhk796c95avxmecs6yrcfm6",
        "displayText": "Test 1",
        "bag": {
          "contentItems": [
            {
              "contentItemId": "4v1zxrewd0k0cwqf2kxvx8mvma",
              "contentType": "Bar",
              "displayText": "Bar 1"
            }
          ]
        }
      }
    ]
  }
}

Query result with the clone content item

{
  "data": {
    "foo": [
      {
        "contentType": "Foo",
        "contentItemId": "47yyt8a4fsjv63w492tt1er5da",
        "contentItemVersionId": "4jn8r5wxypwte50m6g5qygtcnr",
        "displayText": "Test 1 Clone",
        "bag": {
          "contentItems": [
            {
              "contentItemId": "4v1zxrewd0k0cwqf2kxvx8mvma",
              "contentType": "Bar",
              "displayText": "Bar 1"
            }
          ]
        }
      },
      {
        "contentType": "Foo",
        "contentItemId": "4afm7w6fcdddvv4jnh7bym6y5f",
        "contentItemVersionId": "4cezhk796c95avxmecs6yrcfm6",
        "displayText": "Test 1",
        "bag": {
          "contentItems": [
            {
              "contentItemId": "4v1zxrewd0k0cwqf2kxvx8mvma", <-- same ID
              "contentType": "Bar",
              "displayText": "Bar 1"
            }
          ]
        }
      }
    ]
  }
}

[Skrypt]

It has a new id if you combine the main content item with the sub content items. Since the data of these content items are contained inside a main content item there is no issue. Though, could you explain why you would need them to be singular?

Use case?

[jtkech]

@Skrypt Yes but now contained items can be routed base on their ContainedContentItemId

[stevo-knievo]

Thanks @Skrypt, Thanks @jtkech for your replies.

I asked at GITTER if it is possible to override the clone method strategy to change that behaviour to generate new IDs for all of the embedded elements as well.

@deanmarcussen replied:

can you open an issue for this, it's something we should do, and will just be a matter of implementing a Clone Handler for the Bag/Flow/Taxonomy parts. It would cause potential issues with some parts of contained routing if we don't do new ids

My use case is outside of the Orchard ecosystem: I use Orchard as a headless CMS. I utilize the contentItemId as a unique identifier which worked until we start using the clone functionality.

[jtkech]

@stevo-knievo Cool then, thanks for opening this issue

[stevo-knievo]

no, no thank you guys for all your great work! I much appreciate it!

[Skrypt]

Ok so if we have routes then maybe it should be based on ContentItemId dash ContainedContentItemId. Or simply generate new id's if it's not too much work would work too. Both methods would be fine. Unless we don't want to end up with long url's.

[jtkech]

@stevo-knievo @Skrypt

Just wrote the following in the existing BagPartHandler, but didn't try it yet, will do it when i will have time, or if someone want to do a PR.

@deanmarcussen there is a cloning handler in AutoroutePartHandler that may execute first as there is no inter-dependency between OC.Flows and OC.Autoroute. So here maybe too late to update the ids, so we may have to move the autoroute code of CloningAsync() to a ClonedAsync().

public class BagPartHandler : ContentPartHandler<BagPart>
{
    private readonly IContentItemIdGenerator _idGenerator;

    public BagPartHandler(IContentItemIdGenerator idGenerator)
    {
        _idGenerator = idGenerator;
    }

    public override Task CloningAsync(CloneContentContext context, BagPart part)
    {
        var clonedPart = context.CloneContentItem.As<BagPart>();

        foreach (var contentItem in clonedPart.ContentItems)
        {
            contentItem.ContentItemId = _idGenerator.GenerateUniqueId(contentItem);
        }

        clonedPart.Apply();

        return Task.CompletedTask;
    }

[deanmarcussen]

@jtkech looks good.

I have to have a little think about Autoroute. Normally it wouldn't matter, looks like I already wrote the code to handle it. For the most part it will be ok, because the contained routes tend to be relative to the parent. There is an option for absolute routing, which I think has an issue anyway (see https://github.com/OrchardCMS/OrchardCore/pull/7907 ).

So moving it to CloningAsync sounds like the simple option.

We should do this for Flows as well, WidgetsListPart, and Taxonomy Terms (i.e Part).

As we are maintaining ContentItemIds now on all of these by design, so it makes sense to generate a new id for the items inside.

Hmm, but have a look at the BagPartDisplayDriver for some notes I made about nested bags.

The solution to that, might be to trigger the CloningAsync inside the for loop you have there.

Have to think about the implications of that, as it might then hit other handlers. 🤔

(there's another issue somewhere about cloning media attached fields as well)

So maybe not so simple after all.

[jtkech]

@deanmarcussen Okay, i will take a deeper look at it asap

[stevo-knievo]

Today I noticed if you Unpublish a content item and Publish it again it has the same version id. I was wondering if that is a bug and if that is related?

Thanks