U4-7596 - Allow Compositions to be Composed of Compositions (i.e., Pseudo-Inheritance)

Created by Nicholas Westby 22 Dec 2015, 02:58:33 Updated by Søren Kottal 10 Nov 2017, 19:27:59

See discussion here: https://our.umbraco.org/forum/core/general/73809-no-more-master-doctypes-in-74-beta-is-this-really-an-improvement

This is for Umbraco 7.4 beta. The short version is that the 7.4 beta removes document type inheritance. The claim is that compositions are much like document type inheritance. However, there are some problems with that claim:

  • You can't use more than one level of composition (i.e., you can't have your compositions composed of other compositions). That means that any time you create a new document type, you have to select several compositions rather than a single composition.
  • The inability to create new inherited document types breaks POCO mappers. For example, the content mapper my company built relies on document types supporting inheritance (the C# classes mirror that inheritance). In particular, the one my company built looks at document types (including parent doctypes) to figure out how to map properties in the types that a class implements. We'd have to mirror the existing properties on new compositions, which would have different names than the existing document types, which would not work (e.g., it would break our "Master.cshtml", as it expects particular classes that the mapper constructs).

By the way, it's a lot of code to look through, but here's my company's content mapper if you are curious: https://github.com/rhythmagency/rhythm.umbraco.extensions/tree/master/trunk/Rhythm.Extensions/Rhythm.Extensions/Mapping and https://github.com/rhythmagency/rhythm.umbraco.extensions/tree/master/trunk/Rhythm.Extensions/Rhythm.Extensions7/Mapping

Comments

Stephan 24 Dec 2015, 11:07:07

Technically speaking, nested compositions do work already already. When a content type is a child of another content type, it means (a) there's a "tree" relation between them and (b) the child is ''composed'' of the parent. And since you can have nested parent/child, nested compositions have to work.

So we ''do'' support a (direct, acyclic) graph of compositions.

However, they have been disabled ''in the content type editor'' (ie at UI level) because we were missing some checks, error reporting, etc that were needed to prevent users from creating a graph that would not be direct nor acyclic.

We now need to look into the UI & checks part to assess what amount of work would be required to enable nested compos in a safe way.


Andy Butland 30 Dec 2015, 09:16:25

I've been giving some thought to this rather tricky problem. May be covering old ground or missing something key but hopefully as a useful fresh pair of eyes on it, here's what I've come up with...

Example scenario: C is composed of A and B, D is composed of C and E composed of D

A, B --> C --> D --> E

The problem: when picking from the selection of document type compositions, we only want to allow the user to pick valid ones.

There look to be 3 classes of "invalid" selections that I can see:

Cyclical - e.g. from the scenario above, selecting E as a composite of A

Part Relations - e.g. selecting A as a composite of B when A and B are already used together as compositions of C

Duplicating - e.g. selecting A as a composite of D, given it's already implicitly there from D's composition of C

Composition information is stored in table cmsContentType2ContentType with fields parentContentTypeId, childContentTypeId. It would be possible I imagine to query this table via a number of self joins to determine composition relationships that span multiple levels and from that omit options or reject selections that are invalid based on one of the criteria above. But it doesn't look straightforward and there would likely be edge cases - even if you self joined the table 10 times, you could still have a cycle of 11 steps that it wouldn't detect.

So in this scenario - where querying the currently structured information is difficult to answer the question that we need to ask of the data - it can be useful to think about how the information is stored. And consider storing information, duplicated in a denormalised form on each update, that makes it easier to query. Similar to the path field in umbracoNode.

Suggestion then is to introduce a new field called CompositionPath on cmsContentType that would store a delimited list of the full set of compositions contained by a document type.

So for the scenario above we would have (though using node ids course rather than the illustrative letters of I've got here):

A - ""
B - ""
C = "A,B"
D = "C;A,B"
E = "D;C;A,B"

And on every save (create or update) of a document type, we would need to update these fields for all document types based on any composition changes that have been made:

*If removing a composition we need to remove it from all composition paths that contain the document type being edited, e.g. if we remove A from C we need to ''update all composition paths that contain C, and remove A'' *If adding a composition we need to add it to all composition paths that contain the document type being edited, e.g. if we add A to C we need to ''update all composition paths that contain C, and add A'' *If creating a new type we need to create the composition path based on all selected compositions, e.g. if we added type I, made up of a composition of E and H, where H in turn is composed of F and G, we'd have a composition path of H;F,G|E;D;C;A,B

With that in place we could then have client side checks for the 3 classes of invalid selections as follows:

#When editing a document type only allow selection of other types as compositions if their composite paths do not contain the document type currently being edited. This can be determined when the list of composite check boxes is rendered to filter out or disable invalid ones. #Don't allow selection of a composite type if it and the document type currently being edited are already together, at the same level, in a composition path of another node. This one can also be determined at render time. E.g. if editing A, can't pick B as "A" and "B" are found together at the same level in the composition path of C #The "duplicating" scenario can't be detected just at render time as whether a selection is valid or not depends on other selections the user makes for the document type being edited. E.g. when editing D, we can correctly add A only if C is removed - in other words, if C is unchecked, adding A becomes valid. So here we'd need to store the composition path for each composite type, perhaps in a data- attribute on the checkbox itself. On change of the checkbox: *If checked, disable and uncheck all checkboxes matching the composition path - e.g. checking C, which has "A,B" in the composition path, should disable and uncheck A and B *If unchecked, enable all checkboxes for types contained in the composition path

With the above in place I don't think it would be possible to pick invalid compositions from the UI. The same checks should be in place on the server-side though, just in case perhaps two developers are working on a shared database:

#If a selected composition type has a composition path containing the document type currently being edited - throw exception. #If a selected composition type and the document type currently being edited are both at the same level in the composition path of another document type - throw exception. #If a selected composition type is in the composition path of another selected type for the document type currently being edited - throw exception.


Lars-Erik Aabech 22 Nov 2016, 23:19:58

I'm unduping this and duping a post from slack and our for record's sake.

The thing nowadays is that you can't compose a type that has been composed. So any base type cannot be given new compositions the moment you've derived it.

What's stopping us from being able to do it from the UI is that HQ haven't discovered a friendly enough feedback when you create cyclic references yet.

I just skimmed this thread: https://our.umbraco.org/forum/core/general/73809-no-more-master-doctypes-in-74-beta-is-this-really-an-improvement), and I also found that this duplicated issue that isn't linked to any duped issue. The above suggestions from @andybutland on how to solve it is quite good.

In any case, both show that there's a real need to keep flexible inheritance, although we are able to make due with the current state.

I checked the errors from the .net compiler for reference. Here's the exceptions it'd generate: Circular base class dependency involving 'Class1' and 'Class2' Inherited interface 'Interface1' causes a cycle in the interface hierarchy of 'Interface2'

Dunno if maybe "Doctype B is already composed of Doctype A, so Doctype A cannot be composed of Doctype B" might be a clear enough message. Given we're in a browser, a link to wikipedia's article on cyclic references could be added as a bonus. ;)

I for one hope we're looking at a shorter discussion this time, and that this'll be fixed sometime before v8.


Søren Kottal 10 Nov 2017, 19:27:59

I was just hacking around, trying to add a compose a doctype of an already composed doctype. The ContentTypeService let me do it, as long as the property aliases didn't collide, so I could have my page doctype composed of a settings doctype which was composed of a meta doctype and some other doc type.

I then noticed, that if you call .CompositionIds() on your IContentType, you get a path like the Composition Path @abutland mentioned.

So maybe a solution for this issue is not that far away?


Priority: Normal

Type: Usability Problem

State: Open

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.4.0

Due in version:

Sprint:

Story Points:

Cycle: