Umbraco-CMS: NC keys: When copying page nested content does not update properly

Thanks @creativesuspects! Still think that we need to address this in the core, we should not expect the average developer or beginner to find this issue here and apply the fix. Why not apply the fix in core until the underlying problem is solved? @Shazwazza what do you say?

@nul800sebastiaan @rbottema Okay, so that seems to work. Also, I just realized I had previously been reloading Memory Cache before reloading the Database Cache, so that might have been an issue as well. Like I said, it was very late.

I have only tested it on a very small site with only a few content nodes, but I’m going to do some more testing on a larger site with multiple language variants to make sure it works.

So to sum it all up, here’s what I did:

1. I’ve confirmed that there were duplicate nested content keys in the database. These nested content values are showing the same output when loaded onto a page even though the content is different. But obviously that’s because Umbraco caches the content based on the keys which are duplicates.
2. I’ve created a simple API controller that replaces all the nested content keys in the umbracoPropertyData table using the logic from PR #8177, see code below. So you’d only have to execute this once: /umbraco/backoffice/api/ResetNestedContentKeys/GetItDone
3. Settings -> Published Status -> Rebuild Database Cache
4. Settings -> Published Status -> Rebuild Memory Cache
5. Reload affected page and you should see unique content for each nested content item

using System;
using System.Linq;
using System.Web.Mvc;
using Newtonsoft.Json.Linq;
using NPoco;
using Umbraco.Core;
using Umbraco.Core.Persistence;
using Umbraco.Core.Scoping;
using Umbraco.Web.WebApi;

namespace Application.Core.Controllers
{
    public class ResetNestedContentKeysController : UmbracoAuthorizedApiController
    {
        private readonly IScopeProvider _scopeProvider;

        public ResetNestedContentKeysController(IScopeProvider scopeProvider)
        {
            _scopeProvider = scopeProvider;
        }

        [HttpGet]
        public HttpStatusCodeResult GetItDone()
        {
            using (var scope = _scopeProvider.CreateScope(autoComplete: true))
            {
                var selectSql = scope.SqlContext.Sql(@"
                    SELECT id, textValue FROM umbracoPropertyData WHERE propertyTypeId IN (
                        SELECT id FROM cmsPropertyType WHERE dataTypeId IN (
                            SELECT nodeId FROM umbracoDataType WHERE propertyEditorAlias = 'Umbraco.NestedContent'
                        )
                    )
                ");
                var records = scope.Database.Fetch<dynamic>(selectSql);

                foreach (var record in records)
                {
                    var r = (PocoExpando)record;
                    var id = Convert.ToInt32(r.Values.First());
                    var text = r.Values.Last().ToString();

                    if (!string.IsNullOrEmpty(text))
                    {
                        var updatedText = CreateNestedContentKeys(text, false);
                        scope.Database.Execute("UPDATE umbracoPropertyData SET textValue = @0 WHERE id = @1", new object[] { updatedText, id });
                    }
                }
            }

            return new HttpStatusCodeResult(200);
        }

        private string CreateNestedContentKeys(string rawJson, bool onlyMissingKeys, Func<Guid> createGuid = null)
        {
            // used so we can test nicely
            if (createGuid == null)
                createGuid = () => Guid.NewGuid();

            if (string.IsNullOrWhiteSpace(rawJson) || !rawJson.DetectIsJson())
                return rawJson;

            // Parse JSON
            var complexEditorValue = JToken.Parse(rawJson);

            UpdateNestedContentKeysRecursively(complexEditorValue, onlyMissingKeys, createGuid);

            return complexEditorValue.ToString();
        }

        private void UpdateNestedContentKeysRecursively(JToken json, bool onlyMissingKeys, Func<Guid> createGuid)
        {
            // check if this is NC
            var isNestedContent = json.SelectTokens($"$..['ncContentTypeAlias']", false).Any();

            // select all values (flatten)
            var allProperties = json.SelectTokens("$..*").OfType<JValue>().Select(x => x.Parent as JProperty).WhereNotNull().ToList();
            foreach (var prop in allProperties)
            {
                if (prop.Name == "ncContentTypeAlias")
                {
                    // get it's sibling 'key' property
                    var ncKeyVal = prop.Parent["key"] as JValue;
                    // TODO: This bool seems odd, if the key is null, shouldn't we fill it in regardless of onlyMissingKeys?
                    if ((onlyMissingKeys && ncKeyVal == null) || (!onlyMissingKeys && ncKeyVal != null))
                    {
                        // create or replace
                        prop.Parent["key"] = createGuid().ToString();
                    }
                }
                else if (!isNestedContent || prop.Name != "key")
                {
                    // this is an arbitrary property that could contain a nested complex editor
                    var propVal = prop.Value?.ToString();
                    // check if this might contain a nested NC
                    if (!propVal.IsNullOrWhiteSpace() && propVal.DetectIsJson() && propVal.InvariantContains("ncContentTypeAlias"))
                    {
                        // recurse
                        var parsed = JToken.Parse(propVal);
                        UpdateNestedContentKeysRecursively(parsed, onlyMissingKeys, createGuid);
                        // set the value to the updated one
                        prop.Value = parsed.ToString();
                    }
                }
            }
        }
    }
}

Hi all, there’s a lot of information both here and in the original issue #5961. For now, I’ll keep the conversation here.

IMO the GUIDs for each element should be unique since that is what we would expect it to be. They should be both unique and not change per element, for example, if one day we can persist elements to the DB, they will have their own unique GUID that won’t change. Even though we are currently storing this data in JSON, it should be the same principal. I’m not a fan of having new random keys or non-consistent keys per request. Each of the stored keys should just be unique and consistent.

To solve this issue a custom event handler for ContentService.Copying will be needed to handle this type of situation which will re-generate new GUIDs per element.

I do understand there’s a complexity here. Currently nested content is the only editor that has unique keys per element that we’d need to take into account but the challenge is that nested content can be nested within other complex editors. Originally i was thinking of just being pragmatic and creating a ContentService.Copying handler within the nested content property editor and just re-generating the keys for each element. This would be quite easy to do but doesn’t take into account situations like if there is nested content inside the grid or some other nested editor.

It might be possible to detect nested content json within a json structure, and check if there’s a GUID stored for any key and re-generate that as a more generic solution. I think this is a bit ugly but could work for the interim. Else we would just need handlers for the grid and nested content to deal with detecting nested content items and handle the key regeneration. I don’t think this would be too difficult and would solve the issue for most cases. I understand it doesn’t solve the problem if NC is nested in 3rd party complex editors, but until we streamline how elements are stored and how complex editors are supposed to work (i.e. that is coming with the standardization of the block editor) we can’t magically just make this work 100% of the time but i think we can easily solve it for the majority. For the edge cases we can have some re-usable methods for devs to use to handle ContentService.Copying in case they support having NC nested inside their own complex editors.

What do you think?

@kimschurmann As of v8.6.2 this works now, with one caveat, something went wrong with the release and there’s a manual fix for that available at the moment: https://github.com/umbraco/Umbraco-CMS/issues/8223 - that will be fixed in 8.6.3.

I will close this issue now as a “duplicate” of #7133 which was marked as fixed and released in 8.6.2.

You can let me know! 😉

On second thought it’s probably best to just replace ALL records in case someone decides to do a rollback on a page, because that would also bring back those duplicate keys.

This is the SQL that gives you all of the results for all versions:

SELECT id, textValue FROM umbracoPropertyData WHERE propertyTypeId IN (
	SELECT id FROM cmsPropertyType WHERE dataTypeId IN (
		SELECT nodeId FROM umbracoDataType WHERE propertyEditorAlias = 'Umbraco.NestedContent'
	)
)

After rebuilding NuCache (from the dashboard in the developer section), you still need to read the new cache into memory, I could only make that happen with an app pool recycle.

Your mistake is that you’re using [current] = 1, which means the version that is being edited right now and is not published yet. I think you can just leave off that part of the query and update all of the keys, as far as I understand it doesn’t matter exactly what the key is as long as it’s unique, right?

@enkelmedia - TOTALLY agree

I would be happy to help out in some way here but I don’t really have a clear picture of how - anyone who has more insight in to the core that can provide some pointers?

I think that this issue is quite important and really confusing for editors to deal with.

You can use the workaround mentioned earlier until you or someone else implements a fix. I’m using the workaround in a couple of 8.6.x projects and it works great. As far as I can tell performance impact is limited.

I would be happy to help out in some way here but I don’t really have a clear picture of how - anyone who has more insight in to the core that can provide some pointers?

I think that this issue is quite important and really confusing for editors to deal with.

The client side copy is a different issue and yup I’ve chatted to Niels L about this. This is when copying property values client side. This is when copying a whole node server side. The perf won’t be too bad though, it’s just de/serialization and would only occur for property types in question.

On the server side, a different approach could be to introduce a “you’re being copied” event system on property level and let Nested Content and the like do their own thing. This would probably scale better for 3rd party editors. But it would probably also end up being a breaking change.

We could do something like that as part of an extension interface that a property editor could implement without breaks. Would be a simpler than handling the whole ContentService.Copying event and filtering by property editor. Like IPropertyValueCopying.

@ronaldbarendse - after some tests, I conclude you are totally right. The reason I was having the same issue is that the items had been duplicated too - no relation with the Nested Content or Composition structure.
So disregard my previous comments on this ticket. Thanks!