Skip to content

Look-up redirect in content finder for multi-lingual sites using path and legacy route prefixed with the integer ID of the node with domains defined #18763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 7, 2025

Conversation

AndyButland
Copy link
Contributor

@AndyButland AndyButland commented Mar 21, 2025

Prerequisites

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

Resolves: #18714

Description

The linked issue raises an issue with redirects on websites where domains are defined in the content tree. I've traced it to a difference in what we store in the umbracoRedirectUrl.url column when a redirect rule is added.

For a URL of /en/test:

  • With 13 we store e.g. 1057/test - where the number is the integer ID of the node where the domain is defined.
  • With 15 we store /en/test

The difference seems to be in part about because the RedirectTracker component in 15 uses IPublishedUrlProvider.GetUrl(), whereas in 13 we used IPublishedCache.GetRouteById().

But there may be a bit more to it than that - even with 13, it seemed we could have a mix of URLs depending on configuration (see #18763).

Given we in theory could have a mix of redirect URLs in an upgraded site, I've fixed in this PR by amending the content finder, so we look up both formats.

To Test:

  • Create an Umbraco set up with two languages
  • Create a document type that varies by culture, is allowed at root and allows itself as a child.
  • Create a root page using the document type in one of the languages.
  • Add an /en domain to this page.
  • Create a sub page under this using the same language.
  • Rename the sub page.
  • Rename it again.

Look in umbracoRedirectUrl.url and you'll see two redirects, e.g. /en/test and en/test-2.

Update once of the redirects to the 13 format, using a query like:

update umbracoRedirectUrl
set url = '1057/test', urlHash = '3fbca6debddf63167af3d7357c1f654b5beebe42'
where url = 'en/test'

The number prefix should be the integer ID of the root page you created above. It shouldn't have a leading / and the path should be the segment of the sub page only.
The URL hash you can generate from e.g. https://emn178.github.io/online-tools/sha1.html

Then via Content > Redirect URL Management check you can navigate to both old URLs and end up on the current version of the page.

… and legacy route prefixed with the integer ID of the node with domains defined.
@nikolajlauridsen
Copy link
Contributor

nikolajlauridsen commented Mar 28, 2025

The code looks fine, but I had some issues when testing. It might be because I misunderstood the premise a bit here.

I tried to do this with the not default language, so instead of doing it for English, I did it for Danish, the second language I added, and it doesn't work in that scenario because it can't find the domain.

So the question is, is this only supposed to work for the default language or for any language? Might be working as intended but I'm not sure 😄

@AndyButland
Copy link
Contributor Author

Not quite sure if we are seeing the same thing, but I've pushed an update that fixes something I found.

To review what you reported I replayed the above testing steps, with one addition - also adding a /da domain to the root page. Then I tried renaming pages in both languages.

I do get the correct redirects then stored in the database and they work in the sense of taking you to the expected destination page when you navigate to the old URL.

In the backoffice though I couldn't see the original URL of the Danish page - it was rendered just as "#".

The reason looks to be that the URL we retrieve from the database is passed into the IUrlProvider, specifically NewDefaultUrlProvider, where it doesn't handle the path. It only handles the default domain - and I don't understand why that is. So with the latest update I've amended this to do so for all cultures.

That seems OK - but I am a little concerned I'm missing something here, as to why this behaviour was as it was before.

@nikolajlauridsen
Copy link
Contributor

Yeah sorry I wasn't being clear, a bit of Friday brain going on 😛

I think the difference is in that I only have the default culture published, this tracks with the Can_Resolve_Urls_For_Non_Default_Domain_Culture_Only failing 😓

So my setup is like this:

I have English and Danish languages, English is the default, and my content structure is like this:

  • Root (ID 1056) -> Published only in Danish has Danish domain /da no English domain
    • Child -> Published only in Danish

I've renamed the child twice, changing one of them like you mentioned:

image

It looks right in URL tracking:
image

But I get a 404 when I navigate to /1056/barn/

Looking a little into the code it seems to happen because the domain is null on the frequest in ContentFinderByRedirectUrl

image

I did a little bit of digging without going too much in depth, but it seems to happen because the culture in DomainUtilities.SelectDomain is null:

image

Which is why I wasn't sure it wasn't actually a bug, because arguably we can't really reasonably determine what the segment of the ID should be, when we don't know the culture, and there's no default, but I'm not 100% on this 😄

@AndyButland
Copy link
Contributor Author

Just spotted one small thing wrong in your testing - /1056/barn/ isn't valid. It should be 1056/barn/ (i.e. no leading slash for the integer when this other format is used). That might be all it is - as there's some parsing going on to extract this numeric prefix.

@nikolajlauridsen
Copy link
Contributor

You're right 🤦 sorry about that, that's my bad, it works fine after I did that 😄

Does seem like we still have a broken integration test though 🤔

@AndyButland
Copy link
Contributor Author

AndyButland commented Mar 28, 2025

Yes, I see that failure. It's because of the last update I've made in ea97d9a.

So perhaps that's not valid, but I'm a bit confused by it to be honest.

Seems we can have routes of /en/test and /da/test - but we only route the first as it's the default language, but not the second. I'm not following what the logic is of that. One to ponder next week perhaps.

Have reverted the previous update for now.

@AndyButland
Copy link
Contributor Author

I've re-addressed this:

I do get the correct redirects then stored in the database and they work in the sense of taking you to the expected destination page when you navigate to the old URL.

In the backoffice though I couldn't see the original URL of the Danish page - it was rendered just as "#".

By just amending it in the presentation service - so if we can't get a URL from the route using the whole URL providers setup, if we have one that is a path, we'll display it anyway.

So I think this now improves the situation with redirects without making any amends to the actual URL provider logic. I.e. redirects for non-default languages now work, and they are displayed in the backoffice.

@AndyButland
Copy link
Contributor Author

Just a comment to say we'll hold this for now. @kjac is digging into something that sounds related - with regard to routing non-default, variant content - so it makes sense to wait and see the outcome of that.

@AndyButland
Copy link
Contributor Author

This is back on the table now. The work @kjac was looking at in the end was limited to the delivery API.

@nikolajlauridsen
Copy link
Contributor

Bit unsure what state we ended in here Andy, but it looks good to me, so feel free to merge if you're happy 👍

@AndyButland
Copy link
Contributor Author

AndyButland commented Apr 4, 2025

I'll ask @kjac for a review too - hopefully he'll have time next week. I'm just a bit concerned this overlaps with something else he's been looking at regarding routing non-default languages, and if so, maybe I'm "band-aiding" around a deeper issue.

@AndyButland AndyButland requested a review from kjac April 4, 2025 12:10
Copy link
Contributor

@kjac kjac left a comment

Choose a reason for hiding this comment

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

This works as advertised, and the code looks good. Much in line with what was done for the Delivery API for something along the same lines (you even mention #18160 in the code comment 👍 ).

I was able to reproduce the original issue on the old test-DB from when I did #18160 so... it works 💯

Top marks for adding tests! 😍

@kjac kjac merged commit 4078e83 into v15/dev Apr 7, 2025
20 of 21 checks passed
@kjac kjac deleted the v15/bugfix/redirects-with-multi-language branch April 7, 2025 06:18
kjac added a commit that referenced this pull request Apr 9, 2025
* Only prevent the unpublish or delete of a related item when configured to do so if it is related as a child, not as a parent (#18886)

* Only prevent the unpubkish or delete of a related item when configured to do so if it is related as a child, not as a parent.

* Fixed incorect parameter names.

* Fixed failing integration tests.

* Use using variable instead to reduce nesting

* Applied suggestions from code review.

* Used simple using statement throughout RelationService for consistency.

* Applied XML header comments consistently.

---------

Co-authored-by: mole <[email protected]>

* Feature: highlight invariant doc with variant blocks is unsupported (#18806)

* mark variant blocks in invariant docs as invalid

* implement RTE Blocks

* Fix pagination for users restricted by start nodes (#18907)

* Fix pagination for users restricted by start nodes

* Default implementation to avoid breakage

* Review comments

* Fix failing test

* Add media start node tests

* Fix issue preventing blueprint derived values from being scaffolded (#18917)

* Fix issue preventing blueprint derived values from being scaffolded.

* fix manipulating frooen array

* compare with variantId as well

---------

Co-authored-by: Niels Lyngsø <[email protected]>

* ci: add Azure Static Web Apps workflow file
on-behalf-of: @Azure [email protected]

* ci: add Azure Static Web Apps workflow file
on-behalf-of: @Azure [email protected]

* ci: add Azure Static Web Apps workflow file
on-behalf-of: @Azure [email protected]

* Remove admin permission on user configuration, allowing users with user section access only to manaage users and groups. (#18848)

* Tiptap RTE: Style Menu extension kind (#18918)

* Adds 'styleMenu' Tiptap toolbar extension kind

* Adds icons for `<h4>` and `<p>` tags

* Adds commands to HTML Global Attributes extension

for setting the `class` and `id` attributes.

* Renamed "default-tiptap-toolbar-element.api.ts" file

The "element" part was confusing.

* Toolbar Menu: uses correct `item` value

* Cascading Menu: adds localization for the label

* Adds `label` attribute to UUI components

for accessibility.

* Toolbar Menu: uses correct `appearance` value

* Removed unrequired `api` from Style Select

* Destructs the `item.data` object

* Ensure has children reflects only items with folder children when folders only are queried. (#18790)

* Ensure has children reflects only items with folder children when folders only are queried.

* Added supression for change to integration test public code.

---------

Co-authored-by: Migaroez <[email protected]>

* Only apply validation on content update to variant cultures where the editor has permission for the culture (#18778)

* Only apply validation on content update to variant cultures where the editor has permission for the culture.

* Remove inadvertent comment updates.

* Fixed failing integration test.

* Adds ancestor ID details on document tree and collection responses (#18909)

* Populate ancestor keys on document tree response items.

* Populate ancestor keys on document collection response items.

* Update OpenApi.json

* Use array of objects rather than Ids for the ancestor collection.

* Update OpenApi.json.

* Move publish with descendants to a background task with polling (#18497)

* Use background queue for database cache rebuild and track rebuilding status.

* Updated OpenApi.json and client-side types.

* Updated client to poll for completion of database rebuild.

* Move IBackgroundTaskQueue to core and prepare publish branch to run as background task.

* Endpoints for retrieval of status and result from branch publish operations.

* Poll and retrieve result for publish with descendants.

* Handled issues from testing.

* Rework to single controller for status and result.

* Updated client side sdk.

* OpenApi post dev merge gen

---------

Co-authored-by: Migaroez <[email protected]>

* Clear roots before rebuilding navigation dictionary (#18766)

* Clear roots before rebuilding navigation dictionary.

* Added tests to verify fix.

* Correct test implementation.

* Convert integration tests with method overloads into test cases.

* Integration test compatibility supressions.

* Fixes save of empty, invariant block list on variant content. (#18932)

* remove unnecessary code (#18927)

* V15/bugfix/fix route issue from 18859 (#18931)

* unique check

* unique for workspace empty path

* more unique routes

* Bump vite from 6.2.3 to 6.2.4 in /src/Umbraco.Web.UI.Client

Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 6.2.3 to 6.2.4.
- [Release notes](https://github.com/vitejs/vite/releases)
- [Changelog](https://github.com/vitejs/vite/blob/v6.2.4/packages/vite/CHANGELOG.md)
- [Commits](https://github.com/vitejs/vite/commits/v6.2.4/packages/vite)

---
updated-dependencies:
- dependency-name: vite
  dependency-version: 6.2.4
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <[email protected]>

* removes autogenerated workflows

* make getHasUnpersistedChanges public (#18929)

* Added management API endpoint, service and repository for retrieval of references from the recycle bin (#18882)

* Added management API endpoint, service and repository for retrieval of references from the recycle bin.

* Update src/Umbraco.Cms.Api.Management/Controllers/Document/RecycleBin/ReferencedByDocumentRecycleBinController.cs

Co-authored-by: Copilot <[email protected]>

* Removed unused code.

---------

Co-authored-by: Copilot <[email protected]>

* Updated management API endpoint and model for data type references to align with that used for documents, media etc. (#18905)

* Updated management API endpoint and model for data type references to align with that used for documents, media etc.

* Refactoring.

* Update src/Umbraco.Core/Constants-ReferenceTypes.cs

Co-authored-by: Copilot <[email protected]>

* Fixed typos.

* Added id to tracked reference content type response.

* Updated OpenApi.json.

* Added missing updates.

* Renamed model and constants from code review feedback.

* Fix typo

* Fix multiple enumeration

---------

Co-authored-by: Copilot <[email protected]>
Co-authored-by: mole <[email protected]>

* Skip lock tests

* Look-up redirect in content finder for multi-lingual sites using path and legacy route prefixed with the integer ID of the node with domains defined (#18763)

* Look-up redirect in content finder for multi-lingual sites using path and legacy route prefixed with the integer ID of the node with domains defined.

* Added tests to verify functionality.

* Added reference to previous PR.

* Referenced second PR.

* Assemble URLs for all cultures, not just the default.

* Revert previous update.

* Display an original URL if we have one.

* Bump vite from 6.2.4 to 6.2.5 in /src/Umbraco.Web.UI.Client

Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 6.2.4 to 6.2.5.
- [Release notes](https://github.com/vitejs/vite/releases)
- [Changelog](https://github.com/vitejs/vite/blob/v6.2.5/packages/vite/CHANGELOG.md)
- [Commits](https://github.com/vitejs/vite/commits/v6.2.5/packages/vite)

---
updated-dependencies:
- dependency-name: vite
  dependency-version: 6.2.5
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <[email protected]>

* Add raw value validation to multiple text strings property editor (#18936)

* Add raw value validation to multiple text strings property editor

* Added additional assert on unit test and comment on validation logic.

* Don't remove items to obtain a valid value

---------

Co-authored-by: Andy Butland <[email protected]>

* Integration tests for content publishing with ancestor unpublished (#18941)

* Resolved warnings in test class.

* Refactor regions into partial classes.

* Aligned test names.

* Variable name refactoring.

* Added tests for unpublished paths.

* Adjust tests to verify current behaviour.

* Cleaned up project file.

* fix circular icon import (#18952)

* remove segment toggle for elements (#18949)

* Fix modal route registration circular import (#18953)

* fix modal route registration circular import

* Update modal-route-registration.controller.ts

* V15/fix/18595 (#18925)

* fix for #18595

* updates the en.ts

* Avoid unneeded Dictionary operations (#18890)

* Avoid some heap allocations

* Remove unneeded double seek

* Avoid allocating new empty arrays, reuse existing empty array

* Avoid allocating strings for parsing comma separated int values (#18199)

* Data type References UI: Workspace + Delete (#18914)

* Updated management API endpoint and model for data type references to align with that used for documents, media etc.

* Refactoring.

* Update src/Umbraco.Core/Constants-ReferenceTypes.cs

Co-authored-by: Copilot <[email protected]>

* Fixed typos.

* generate server models

* add extension slot

* register data type reference info app

* add reference data mappers

* Added id to tracked reference content type response.

* Updated OpenApi.json.

* Added missing updates.

* generate new models

* update models

* register ref item

* remove debugger

* render types

* register member type property type ref

* register media type property type ref

* Renamed model and constants from code review feedback.

* register reference workspace info app kind

* use kind for document references

* use kind for media references

* use kind for member references

* use deleteWithRelation kind when deleting data types

* fix manifest types

* fix types

* Update types.gen.ts

* update code to fit new server models

---------

Co-authored-by: Andy Butland <[email protected]>
Co-authored-by: Copilot <[email protected]>

* Feature: discard changes for block workspace (#18930)

* make getHasUnpersistedChanges public

* Discard changes impl for Block Workspace

* fix 18367 (#18956)

* Merge commit from fork

* Prevent path traveral vulnerability with upload of temporary files.

* Used BadRequest instead of NotFound for invalid file name response.

* V15 QA Fixing the failing media acceptance tests (#18881)

* Fixed the function name due to test helper changes

* Updated assertion steps due to UI changes

* Added more waits

* Bumped version

* Increase timeout

* Reverted

---------

Co-authored-by: Andreas Zerbst <[email protected]>

* V15 QA added clipboard test for not being able to copy to root when block is not allowed at root (#18937)

* Added clipboard test

* Bumped version

* Updated to use the name

* Run all tests on the pipeline

* Reverted command

* build: adjusts circular ref number to 4

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Andy Butland <[email protected]>
Co-authored-by: mole <[email protected]>
Co-authored-by: Niels Lyngsø <[email protected]>
Co-authored-by: Niels Lyngsø <[email protected]>
Co-authored-by: Jacob Overgaard <[email protected]>
Co-authored-by: Lee Kelleher <[email protected]>
Co-authored-by: Migaroez <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Nikolaj Geisle <[email protected]>
Co-authored-by: Mads Rasmussen <[email protected]>
Co-authored-by: Jacob Welander Jensen <[email protected]>
Co-authored-by: Henrik <[email protected]>
Co-authored-by: Sebastiaan Janssen <[email protected]>
Co-authored-by: Nhu Dinh <[email protected]>
Co-authored-by: Andreas Zerbst <[email protected]>
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