Skip to content

Migrations: Align type attribute casing in locallink migration for integer-based legacy links (closes #22597)#22599

Merged
AndyButland merged 2 commits into
mainfrom
v17/bugfix/22597-fix-casing-in-local-link-migration
Apr 27, 2026
Merged

Migrations: Align type attribute casing in locallink migration for integer-based legacy links (closes #22597)#22599
AndyButland merged 2 commits into
mainfrom
v17/bugfix/22597-fix-casing-in-local-link-migration

Conversation

@AndyButland

@AndyButland AndyButland commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Description

#22597 reports a casing discrepancy in the ConvertLocalLinks migration between the legacy GUID UDI and integer formats. The former emit "document"/"media" and the latter "Document"/"Media". The former is expected (comparing to Constants.UdiEntityType.*).

The fix switches the integer branch to use Constants.UdiEntityType.Document ("document") and .Media ("media") so the two branches produce matching lowercase output.

Testing

Automated

LocalLinkProcessorTests are updated to reflect the change (they were previously incorrectly passing as they were asserting the incorrect case).

Manual

To verify against a real database, I have used the following method. Bear in mind it will replay all migrations since 15. Doesn't do any harm, but don't run on anything important.

  • Find a candidate RTE/TinyMCE property data row (the migration processes current = 1 or published = 1 versions):
SELECT
      pd.id                AS PropertyDataId,
      pd.versionId,
      pd.propertyTypeId,
      pt.alias             AS PropertyAlias,
      n.id                 AS NodeId,
      n.text               AS NodeName,
      dt.propertyEditorAlias,
      cv.[current]         AS IsCurrent,
      dv.published         AS IsPublished,
      LEN(pd.textValue)    AS TextLen,
      LEFT(pd.textValue, 500) AS TextPreview
  FROM umbracoPropertyData pd
  INNER JOIN cmsPropertyType    pt ON pt.id = pd.propertyTypeId
  INNER JOIN umbracoDataType    dt ON dt.nodeId = pt.dataTypeId
  INNER JOIN umbracoContentVersion cv ON cv.id = pd.versionId
  INNER JOIN umbracoNode        n  ON n.id = cv.nodeId
  LEFT  JOIN umbracoDocumentVersion dv ON dv.id = cv.id
  WHERE dt.propertyEditorAlias IN ('Umbraco.RichText', 'Umbraco.TinyMCE')
    AND (cv.[current] = 1 OR dv.published = 1)
    AND pd.textValue IS NOT NULL
  ORDER BY pd.id;
  • Pick a real integer document ID for the legacy link to target (IIdKeyMap.GetKeyForId has to resolve it, so it must correspond to an actual node):
  SELECT TOP 5 id, [text] AS Name, [path]
  FROM umbracoNode
  WHERE nodeObjectType = 'C66BA18E-EAF3-4CFF-8A22-41B16D66A972'  -- Document
    AND trashed = 0
  ORDER BY id;
  • Capture the current textValue of the property data row before overwriting it, so you can restore it afterwards:
SELECT pd.id, pd.textValue
FROM umbracoPropertyData pd
WHERE pd.id = @propertyDataId;
  • Seed the legacy integer-form link into that row (keep the existing JSON shape, replace the markup):
DECLARE @propertyDataId int = 123;   -- from the first query above
DECLARE @docIntId int = 456;       -- from the second query above

UPDATE umbracoPropertyData
SET textValue = '{"markup":"<p>Legacy int link: <a href=\"/{localLink:' +
                CAST(@docIntId AS nvarchar(20)) +
                '}\" title=\"test\">click here</a></p>","blocks":null}'
WHERE id = @propertyDataId;
  • Rewind the migration state so ConvertLocalLinks runs again on the seeded row. The state ID is taken from UmbracoPlan.cs — it's the state produced by the migration before ConvertLocalLinks:
UPDATE umbracoKeyValue
SET [value] = '37875E80-5CDD-42FF-A21A-7D4E3E23E0ED'
WHERE [key] = 'Umbraco.Core.Upgrader.State+Umbraco.Core';
  • Start the site. Watch the log for Migration starting for all properties of type: Umbraco.RichText.

  • Inspect the stored value after the migration runs. Expect type="document" (lowercase). Before the fix it was type="Document". The href will contain the GUID for the target node (umbracoNode.uniqueId):

SELECT id, textValue
FROM umbracoPropertyData
WHERE id = @propertyDataId;
  • Restore the original value if you want:
UPDATE umbracoPropertyData
SET textValue = '...'
WHERE id = @propertyDataId;

Copilot AI review requested due to automatic review settings April 24, 2026 13:55
@claude

claude Bot commented Apr 24, 2026

Copy link
Copy Markdown

Claude finished @AndyButland's task in 2m 28s —— View job


PR Review

Target: origin/main · Based on commit: 55efbc15

Fixes a casing mismatch in the v15 local-link migration: integer-based legacy links were being tagged type="Document"/type="Media" (PascalCase, from enum .ToString()) while UDI-based links correctly produced lowercase via Uri.Host. The fix replaces the enum calls with Constants.UdiEntityType.Document/.Media on both processor classes, and corrects the four unit test assertions that had encoded the wrong (buggy) output.

No breaking changes — both processor classes are already [Obsolete("Scheduled for removal in Umbraco 18.")] and no public API surface is altered.


No Critical, Important, or Suggestion findings.

The fix is minimal and precisely targeted. Constants.UdiEntityType.Document ("document") and Constants.UdiEntityType.Media ("media") are the correct values — they match the UDI branch's tag.Udi.EntityType which is lowercase-normalised by the URI spec, and they match what Tiptap's type attribute expects. Updating LocalLinkProcessorForFaultyLinks.CreateIntBasedKeyType (which is private and currently uncalled) for consistency is a sensible defensive move, as acknowledged in the PR description.

Test coverage across the two integer-based code paths and the trailing-content variants is adequate.


Approved

This looks good to be merged as-is. The PR description's note about already-migrated sites needing a republish to pick up the corrected markup is accurate and worth keeping visible for whoever merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a migration bug in the v15 ConvertLocalLinks upgrade step where integer-based legacy {localLink:1234} conversions produced type="Document" / type="Media" instead of the lowercase values expected by Tiptap and the rest of the link pipeline.

Changes:

  • Update the integer-ID conversion branch to emit lowercase entity types using Constants.UdiEntityType.Document / .Media.
  • Apply the same casing fix in LocalLinkProcessorForFaultyLinks’s (currently unused) integer conversion helper for consistency.
  • Update unit tests to assert the corrected lowercase type attribute values.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/LocalLinks/LocalLinkProcessor.cs Fixes int-based local link type casing by using Constants.UdiEntityType.* to match the UDI-based branch output.
src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/LocalLinks/LocalLinkProcessorForFaultyLinks.cs Aligns the int-based helper’s entity type casing with Constants.UdiEntityType.* to avoid reintroducing the bug if used later.
tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/LocalLinkProcessorTests.cs Updates assertions/comments to validate lowercase type="document" / type="media" for integer-based conversions.

@claude claude Bot added the area/backend label Apr 24, 2026
@Zeegaan

Zeegaan commented Apr 27, 2026

Copy link
Copy Markdown
Member

@AndyButland What do we do about sites having already run this migration? Presumably they will have the wrong uppercase Document until it gets republished? 🤔

@Zeegaan Zeegaan self-requested a review April 27, 2026 00:13
@AndyButland

Copy link
Copy Markdown
Contributor Author

What do we do about sites having already run this migration?

It's a good point. Would be better to avoid another migration if we could. This step is rather a heavy one, particularly on larger sites.

Better probably is if we can gracefully handle Pascal named types - e.g. "Document" - and I think we can. I've pushed an update to do that.

For testing I've verified with unit tests, and manually via:

  • Using an RTE, create a local link to a content item.
  • Edit the source of the RTE and amend the type attribute value from document to `Document.
  • Load up the page with the RTE property on it.
  • Verify on main that this will now render an empty href in the link (view source to see that - in the browser the effect will be to have the link navigating to the current page).
  • Verify on this branch that it'll link to the expected page.

@Zeegaan Zeegaan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good & tests good 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants