Cache Refreshers: Fix change tracking for content types

[kjac]

Prerequisites

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

Description

These changes ensure that cache refreshers for content types can perform a proper distinction between "destructive" and "constructive" changes to content types. This is the prelude for performance optimizations that require being able to make this exact distinction.

What are "destructive" and "constructive" changes?

Destructive changes are changes that directly affect the perception of existing content based on the content type. They include:

• Changing the content type alias.
• Removing a property type or a composition.
• Changing the alias of a property type (this effectively corresponds to removing a property type).
• Changing variance, either at property or content type level.

Constructive changes count all other changes - for example:

• Adding a property type or a composition.
• Rearranging property types or groups.
• Changes to name, description, icon etc.
• Changes to settings like allowed children and version cleanup.

The destructive changes are represented as RefreshMain, and constructive changes as RefreshOther in the cache refresher notification payloads.

This used to work?

Yes. Well, almost.

Change detection at content type level works today.

Change detection at property level (here, here and here) works in V13. It was broken in V14.

Change detection for composition removal (here) has not worked in ages; at least, it does even not work in V13.

The changes in this PR

To re-enable change tracking at property level I have addeded explicit property type removal when updating content type properties. This is solely for change tracking at content type level, as the property types are effectively overwritten later in the update flow (here and here) if there are any changes. This is not an entirely optimal solution, but seeking alternative and more explicit solutions led to all kinds of issues and potential instability, which is definitively not optimal either. So at least for the time being, this is likely the best solution, even if it is a rather implicit fix.

The changes to ContentTypeCompositionBase is the implementation of change detection for composition removal.

Lastly, while working through the integration tests, I realized that one can inadvertently (attempt to) change the key of an existing property type. This is not a problem for the backoffice client, as the existing keys are re-applied at update time. But for anyone doing programmatic updates of content types (like the integration tests do), this becomes an issue - not least towards change tracking. These changes have been added to counter this.

Testing this PR

Attach a debugger with a breakpoint here, and verify that the cache refresher notifications use RefreshMain and RefreshOther for destructive and constructive content type changes, respecitively.

[AndyButland]

This tests out just fine - here are the results I got for various document type updates:

Change name - Other
Change alias - Main
Change description - Other
Change icon - Other
Change property name - Other
Change property description - Other
Change property alias - Other
Change property editor - Other
Add new property - Other
Delete property - Main

Which are all as expected.

The main concern - and confusion - when reviewing this was that the definition of "constructive" and "destructive" that defines RefreshOther and RefreshMain doesn't align with the code comments we have describing these values, i.e.:

/// <summary>
///     Content type changes impact only the Content type being saved
/// </summary>
RefreshMain = 2,

/// <summary>
///     Content type changes impacts the content type being saved and others used that are composed of it
/// </summary>
RefreshOther = 4, // changed, other change

To me that reads as "Main = affects only the content type being saved, Other = affects related ones as well". But that's not how you are defining it, nor, as far as I can see, how it's intended that we use this. So I think it's just an unclear documentation issue. Please could you review the documentation on ContentTypeChangeTypes and update to reflect reality and intention here? Would be nice if there was a non-breaking way to rename the values too, but I don't think that's possible.

Still raises a little nagging concern that if this naming is a historical artifact that no longer match the meaning, have we been correct in changing how they are used over the years, or missed anything along the way?

[kjac]

Still raises a little nagging concern that if this naming is a historical artifact that no longer match the meaning, have we been correct in changing how they are used over the years, or missed anything along the way?

I deliberately did not alter the calculation of content type change impact in ContentTypeServiceBase.ComposeContentTypeChanges(). I only corrected the flow, so the intended behavior is actually happens. The naming is quite off, though. I will update the comments 👍

I'm also going to follow up with a suggestion of making the ContentTypeChangeTypes a lot more granular moving forward. That's for another PR, though, but it would be nice to be able to identify all the changes explicitly (like PropertyRemoved, PropertyAdded, PropertyTypeChanged, CompositionRemoved etc. etc. etc.) - this would allow for making more informed decisions.

[AndyButland]

I'm also going to follow up with a suggestion of making the ContentTypeChangeTypes a lot more granular moving forward. That's for another PR, though, but it would be nice to be able to identify all the changes explicitly (like PropertyRemoved, PropertyAdded, PropertyTypeChanged, CompositionRemoved etc. etc. etc.) - this would allow for making more informed decisions.

Just for awareness - I did start that process in #21558 (I needed to know if variations had changed, so added that as a specific change type).

[kjac]

@AndyButland the comments have been updated 👍