Extend RTE output in Delivery API for better support for multi-site URL resolution

[MiguelGuedelha]

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: #20845

Description

This makes the change for Umbraco v17, back-porting to a v16 (and v13) addition should be relatively straightforward if necessary after this merge, if accepted, as the change is non breaking.

Adds both the ID and type to the resolved link information in the Delivery API's RTE data (both JSON and Markup format)

This allows for much easier content/URL resolution in multi-site setups, both through the Delivery API (direct ID request) and also through custom built Link Resolution service/endpoints that might make use of the built in URL resolver tools, etc.

The distinction between media and document type simply allows users to, for example, only process document type links based on the discriminator field being present and == document

I've fixed up the relevant unit tests I could find (both ones that failed from the change and also others that I reckoned would be better to check for the changes moving forward)

Replicating the feature is relatively straightforward:

• Create a simple Document Type with an optional Rich Text editor based field inside it (allow as root) in a blank installation with the Delivery API enabled
• Create the root item and publish it, create a child item under it and publish it.
• Fill in the parent's node RTE field with a link pointing to the child and re-publish
• Run the solution twice (once with RTE outputting as JSON and once with RTE outputting as Markup)
• Verify the solution is adding the 2 data points (content ID and type on Markup mode, content ID only on JSON as the type was already coming through before)

It should work both with normal and legacy links

[github-actions]

Hi there @MiguelGuedelha, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[AndyButland]

Thanks for this contribution @MiguelGuedelha - and great job on ensuring the automated tests and providing clear justifications for why it's needed and how to verify. From my side, all looks good and I can confirm it does what is described.

I'm just going to run it by the team to see if there's anything I've overlooked in introducing these additional attributes in the output.

[kjac]

Hi @MiguelGuedelha

Thanks for this 🙌 really excellent work!

Two things come to mind:

1. The naming of data-content-type implies the alias of the content type, at least to me.
2. Perhaps the output should be aligned closer to that of the Multi URL Picker - the ApiLink? That is:
   • data-content-type becomes data-link-type, and uses the LinkType enum instead.
   • data-content-id becomes data-destination-id, with the same value as now.

Does it sound reasonable to you?

[MiguelGuedelha]

Hi @kjac

Yeah, no real oppositions to whatever name changes/preferences might be best for consistency sake.

Just mostly ported over the custom implementation of the parser that we've had implemented on a project before (damn you for ruining my plans of not needing to update our BFF api layer 😜)

I'll make the changes possibly today if I can towards the end of it and push them up.

[MiguelGuedelha]

Have made the amends now @kjac

Using nameof should easily allow the data contract to update automatically if LinkType ever evolves in the future for whatever reason without needing much reworks on the tests, etc

[AndyButland]

Maybe this is a little inconsistent @kjac - in that for the content type, when we emit markup we have data-link-type=\"Content\".

But with JSON output we have "type": "document". What do you think?

[MiguelGuedelha]

@kjac @AndyButland

Should we instead simply remove the lines where the following is executed in the markup parser:

link.Attributes["type"]?.Remove();

So that type is simply left in the markup (which should be document / media, possibly will be external if a manual link is added)?

And not add any data-link-type entries at all with the LinkType enum values?

[kjac]

We can't remove anything that's already there - that would be a breaking change.

I agree this is a little inconsistent. But the inconsistency is there already, between the URL picker output and URLs in the RTE output.

One option is to add link-type to the JSON output as well, but that might just confuse matters even worse - it'd be "link-type": "Content" for "type": "document".

Let's look at the output for JSON rendered RTE content links (with this PR applied) versus URL picker content links:

RTE content link

{
  "tag": "a",
  "attributes": {
    "target": "",
    "router-slot": "disabled",
    "title": "Root",
    "type": "document",
    "destination-id": "bea875f0-5bfd-48f6-87d9-6c4edc2050dd",
    "route": {
      "path": "/",
      "queryString": null,
      "startItem": {
        "id": "bea875f0-5bfd-48f6-87d9-6c4edc2050dd",
        "path": "root"
      }
    }
  },
  "elements": [
    {
      "text": "link",
      "tag": "#text"
    }
  ]
}

URL picker content link

{
  "url": null,
  "queryString": null,
  "title": "Root",
  "target": null,
  "destinationId": "bea875f0-5bfd-48f6-87d9-6c4edc2050dd",
  "destinationType": "page",
  "route": {
    "path": "/",
    "queryString": null,
    "startItem": {
      "id": "bea875f0-5bfd-48f6-87d9-6c4edc2050dd",
      "path": "root"
    }
  },
  "linkType": "Content"
}

They are actually quite similar - but with some key differences:

• destination-id vs. destinationId (property naming, easily fixed).
• No destinationType on RTE links (not so easily fixed, as providing it might incur a performance penalty).
• No type on URL picker links (easily fixed - if we want it).
• No linkType on RTE links (easily fixed - if we want it).

Now the real question is: Do we want to align these two closely? My gut feeling tells me that we should, but I have nothing concrete to pin it on.

[MiguelGuedelha]

Answering to the reply to my feedback

Sorry @kjac when I meant removing something that is there, it was removing a line of code, but removing that line of code would actually add data to the markup RTE rendering, not remove it.

The RTE markup comes with a pre-populated attributed called "type" with has the "document"/"media" designation. The parser removes it, but if it didn't it would simply leave it in the RTE, essentially leading to a "new" field in the RTE output (hence non breaking as it's non-destructive, but rather additive).

This would make the markup match the existing RTE JSON "type" output from the get go, instead of renaming it to "data-link-type" etc?

---

JSON output alignment between RTE and URL picker

While aligning with the URL picker would be great for a more uniform and predictable data signature, I'd assuming aligning both the RTE markup vs RTE JSON would be first priority?

• While calling it destinationId on JSON probably feels more natural, these are technically meant to be part of the HTML attributes of the element, so the kebab-case makes it more aligned with the RTE markup
• destination-type: is this meant to be the document type alias?
  • If so the RTE parser base already does a published content/media fetch so calling publishedContent.ContentType.Alias on the node we've fetched. It wouldn't be very expensive/require extra fetching work as far as I can tell, so this seems like as easy an addition as adding the ID I just added in this PR
• Agree with the bottom two, we could add them easily to more closely align (would the RTE markup follow suit though or stay as is?)