From e8fd3122961f0353ca38b1344f78a269d60cd90d Mon Sep 17 00:00:00 2001 From: Jonathan Darko Adoo Date: Sun, 19 Apr 2026 11:25:40 +0000 Subject: [PATCH 1/3] fix: use absolute URL for canonical link in redirect pages The redirect_html_fragment was building the canonical by concatenating html_baseurl with a relative URI from get_relative_uri(). This produces broken canonical URLs on mirrors that build without sphinx-multiversion, because the relative path contains '..' segments that resolve incorrectly against the base URL. Fix by computing the canonical URL as an absolute path directly from html_baseurl and the canonical document's path, which is always correct regardless of how the site is built. Fixes #6112 --- conf.py | 6 +- pytest.ini | 3 + test/test_redirect_canonical.py | 152 ++++++++++++++++++++++++++++++++ 3 files changed, 158 insertions(+), 3 deletions(-) create mode 100644 pytest.ini create mode 100644 test/test_redirect_canonical.py diff --git a/conf.py b/conf.py index d6fab26cd14..2e58ee44e30 100644 --- a/conf.py +++ b/conf.py @@ -215,10 +215,10 @@ def generate(cls, app): return redirect_html_fragment = """ - + """ redirections = { @@ -265,7 +265,7 @@ def generate(cls, app): 'skip_sitemap': 'redirect', 'title': os.path.basename(redirect_url), 'metatags': redirect_html_fragment.format( - base_url=app.config.html_baseurl, + canonical_abs_url=app.config.html_baseurl.rstrip('/') + '/' + canonical_url + app.builder.out_suffix, url=app.builder.get_relative_uri( redirect_url, canonical_url ) diff --git a/pytest.ini b/pytest.ini new file mode 100644 index 00000000000..9421d69cf23 --- /dev/null +++ b/pytest.ini @@ -0,0 +1,3 @@ +[pytest] +addopts = -p no:launch-testing-ros -p no:ament-lint -p no:ament-copyright -p no:ament-xmllint -p no:ament-pep257 -p no:ament-flake8 +pythonpath = . \ No newline at end of file diff --git a/test/test_redirect_canonical.py b/test/test_redirect_canonical.py new file mode 100644 index 00000000000..48cf177bf7f --- /dev/null +++ b/test/test_redirect_canonical.py @@ -0,0 +1,152 @@ +""" +Tests for the RedirectFrom directive's canonical URL generation. + +Regression test for: https://github.com/ros2/ros2_documentation/issues/6112 +The canonical in redirect pages must be an absolute URL that includes +the version prefix, regardless of whether sphinx-multiversion is used. +""" +import textwrap +from pathlib import Path +from unittest.mock import MagicMock + + +def make_mock_app(html_baseurl, out_suffix='.html'): + """Helper: build a minimal mock of the Sphinx app object.""" + app = MagicMock() + app.config.html_baseurl = html_baseurl + app.builder.out_suffix = out_suffix + + # get_relative_uri returns a simple relative path for test purposes + def fake_relative_uri(from_doc, to_doc): + # Simulate going up one level then into the canonical page + from_parts = from_doc.split('/') + to_parts = to_doc.split('/') + # Count differing prefix levels + common = 0 + for a, b in zip(from_parts[:-1], to_parts[:-1]): + if a == b: + common += 1 + else: + break + ups = len(from_parts) - 1 - common + rel = '../' * ups + '/'.join(to_parts[common:]) + out_suffix + return rel + + app.builder.get_relative_uri.side_effect = fake_relative_uri + return app + + +def extract_canonical_href(metatags: str) -> str: + """Pull the href value out of the canonical tag.""" + import re + match = re.search(r' + + +""" + metatags = redirect_html_fragment.format( + canonical_abs_url=app.config.html_baseurl.rstrip('/') + '/' + canonical_url + app.builder.out_suffix, + url=app.builder.get_relative_uri(redirect_url, canonical_url), + ) + return metatags + + +class TestCanonicalURL: + + def test_canonical_includes_version_with_multiversion(self): + """ + When sphinx-multiversion is active, html_baseurl is already updated + to include the distro (e.g. https://docs.ros.org/en/humble). + The canonical link must include the full versioned path. + """ + metatags = build_metatags( + html_baseurl='https://docs.ros.org/en/humble', + redirect_url='How-To-Guides/Old-Page', + canonical_url='How-To-Guides/Using-Custom-Rosdistro', + ) + href = extract_canonical_href(metatags) + assert href == 'https://docs.ros.org/en/humble/How-To-Guides/Using-Custom-Rosdistro.html', \ + f"Unexpected canonical href: {href}" + + def test_canonical_does_not_use_relative_url(self): + """ + The canonical href must be absolute — never a relative path like + '../Some-Page.html' which breaks on third-party mirrors. + """ + metatags = build_metatags( + html_baseurl='https://docs.ros.org/en/humble', + redirect_url='Old-Section/Old-Page', + canonical_url='How-To-Guides/Using-Custom-Rosdistro', + ) + href = extract_canonical_href(metatags) + assert href.startswith('https://'), \ + f"Canonical href must be absolute, got: {href}" + assert '..' not in href, \ + f"Canonical href must not contain relative segments, got: {href}" + + def test_canonical_without_multiversion_still_correct(self): + """ + Regression: mirrors that build without sphinx-multiversion leave + html_baseurl as 'https://docs.ros.org/en' (no version). + The canonical URL must still resolve correctly and not 404. + Even without the version prefix in html_baseurl, the path must + be absolute and not contain relative segments. + """ + metatags = build_metatags( + html_baseurl='https://docs.ros.org/en', + redirect_url='How-To-Guides/Old-Page', + canonical_url='How-To-Guides/Using-Custom-Rosdistro', + ) + href = extract_canonical_href(metatags) + assert href.startswith('https://'), \ + f"Canonical href must be absolute, got: {href}" + assert 'Using-Custom-Rosdistro.html' in href, \ + f"Canonical href must contain the page name, got: {href}" + assert '..' not in href, \ + f"Canonical href must not contain relative segments, got: {href}" + + def test_meta_refresh_uses_relative_url(self): + """ + The and JS redirect should keep using the relative URL + so they work correctly within the built site structure. + """ + import re + metatags = build_metatags( + html_baseurl='https://docs.ros.org/en/humble', + redirect_url='How-To-Guides/Old-Page', + canonical_url='How-To-Guides/Using-Custom-Rosdistro', + ) + meta_match = re.search(r'content="0; url=([^"]+)"', metatags) + assert meta_match, "No meta refresh found" + meta_url = meta_match.group(1) + # The meta refresh URL should be relative (not start with https://) + assert not meta_url.startswith('https://'), \ + f"Meta refresh should use relative URL, got: {meta_url}" \ No newline at end of file From b095064d79deec8e855d38c2a71757ea94334077 Mon Sep 17 00:00:00 2001 From: Jonathan Darko Adoo Date: Wed, 22 Apr 2026 22:21:08 +0000 Subject: [PATCH 2/3] fix: default html_baseurl to rolling for correct canonical URLs Plain Running Sphinx v8.2.3 loading translations [en]... done loading pickled environment... The configuration has changed (3 options: 'html_css_files', 'html_js_files', 'html_static_path') done building [mo]: targets for 0 po files that are out of date writing output... building [html]: targets for 0 source files that are out of date updating environment: 0 added, 0 changed, 0 removed reading sources... looking for now-outdated files... none found no targets are out of date. preparing documents... done copying assets... copying downloadable files... [ 10%] The-ROS2-Project/Marketing/documents/ros2-brochure-a4-web.pdf copying downloadable files... [ 20%] The-ROS2-Project/Marketing/documents/ros2-brochure-a4-print.pdf copying downloadable files... [ 30%] The-ROS2-Project/Marketing/documents/ros2-brochure-ltr-web.pdf copying downloadable files... [ 40%] The-ROS2-Project/Marketing/documents/ros2-brochure-ltr-print.pdf copying downloadable files... [ 50%] Tutorials/Advanced/Discovery-Server/scripts/generate_discovery_packages.bash copying downloadable files... [ 60%] Tutorials/Advanced/Discovery-Server/scripts/discovery_packets.py copying downloadable files... [ 70%] Tutorials/Advanced/Discovery-Server/scripts/no_intraprocess_configuration.xml copying downloadable files... [ 80%] Tutorials/Advanced/Simulators/Webots/Code/my_world.wbt copying downloadable files... [ 90%] Tutorials/Intermediate/URDF/documents/r2d2.urdf.xml copying downloadable files... [100%] Tutorials/Intermediate/URDF/documents/r2d2.rviz copying static files... Writing evaluated template result to /home/adoodevv/Documents/ros2_documentation/build/html/_static/documentation_options.js Writing evaluated template result to /home/adoodevv/Documents/ros2_documentation/build/html/_static/language_data.js Writing evaluated template result to /home/adoodevv/Documents/ros2_documentation/build/html/_static/basic.css Writing evaluated template result to /home/adoodevv/Documents/ros2_documentation/build/html/_static/js/versions.js Writing evaluated template result to /home/adoodevv/Documents/ros2_documentation/build/html/_static/copybutton.js copying static files: done copying extra files... copying extra files: done copying assets: done generating indices... genindex done writing additional pages... search done dumping search index in English (code: en)... done dumping object inventory... done sphinx-sitemap-ros: sitemap.xml was generated for URL https://docs.ros.org/en/rolling in /home/adoodevv/Documents/ros2_documentation/build/html/sitemap.xml build succeeded. The HTML pages are in build/html. builds were leaving html_baseurl as 'https://docs.ros.org/en' (no version), causing redirect pages to generate canonical links missing the distro prefix. - Default html_baseurl to 'https://docs.ros.org/en/rolling' so basic builds produce correct canonical URLs out of the box - Update smv_rewrite_configs to replace the full URL (not append) when sphinx-multiversion runs with a specific distro - Fix redirect_html_fragment to use an absolute canonical_abs_url instead of concatenating base_url with a relative path Fixes #6112 --- conf.py | 5 +++-- test/test_redirect_canonical.py | 12 ++++++------ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/conf.py b/conf.py index 2e58ee44e30..5bd92a52a86 100644 --- a/conf.py +++ b/conf.py @@ -187,7 +187,7 @@ # Output file base name for HTML help builder. htmlhelp_basename = 'ros2_docsdoc' -html_baseurl = 'https://docs.ros.org/en' +html_baseurl = 'https://docs.ros.org/en/rolling' # The sitemap_url_scheme is used by the sitemap generator to figure out how # to generate links. Essentially, the sitemap generator uses the following: @@ -306,8 +306,9 @@ def smv_rewrite_configs(app, config): # external defines are setup, and environment variables aren't passed through to # conf.py). Instead, hook into the 'config-inited' event which is late enough # to rewrite the various configuration items with the current version. + if app.config.smv_current_version != '': - app.config.html_baseurl = app.config.html_baseurl + '/' + app.config.smv_current_version + app.config.html_baseurl = 'https://docs.ros.org/en/' + app.config.smv_current_version app.config.project = 'ROS 2 Documentation: ' + app.config.smv_current_version.title() app.config.html_logo = 'source/Releases/' + app.config.smv_current_version + '-small.png' diff --git a/test/test_redirect_canonical.py b/test/test_redirect_canonical.py index 48cf177bf7f..9251445be55 100644 --- a/test/test_redirect_canonical.py +++ b/test/test_redirect_canonical.py @@ -114,20 +114,20 @@ def test_canonical_does_not_use_relative_url(self): def test_canonical_without_multiversion_still_correct(self): """ - Regression: mirrors that build without sphinx-multiversion leave - html_baseurl as 'https://docs.ros.org/en' (no version). - The canonical URL must still resolve correctly and not 404. - Even without the version prefix in html_baseurl, the path must - be absolute and not contain relative segments. + Regression: a plain `make html` build (no sphinx-multiversion) now + defaults html_baseurl to 'https://docs.ros.org/en/rolling'. + The canonical URL must include the version prefix and be absolute. """ metatags = build_metatags( - html_baseurl='https://docs.ros.org/en', + html_baseurl='https://docs.ros.org/en/rolling', redirect_url='How-To-Guides/Old-Page', canonical_url='How-To-Guides/Using-Custom-Rosdistro', ) href = extract_canonical_href(metatags) assert href.startswith('https://'), \ f"Canonical href must be absolute, got: {href}" + assert 'rolling' in href, \ + f"Canonical href must contain the version prefix, got: {href}" assert 'Using-Custom-Rosdistro.html' in href, \ f"Canonical href must contain the page name, got: {href}" assert '..' not in href, \ From d39ec41b9fd0908ce616e7dda83617c8893311ca Mon Sep 17 00:00:00 2001 From: Jonathan Darko Adoo Date: Sat, 25 Apr 2026 18:13:02 +0000 Subject: [PATCH 3/3] test: rewrite tests to exercise actual conf.py logic Previous tests reimplemented the redirect logic inside the test file, meaning they would pass even if conf.py was wrong. Tests now read and exercise the actual redirect_html_fragment template and format call from conf.py directly, so they genuinely fail before the fix and pass after. --- test/test_redirect_canonical.py | 202 +++++++++++++++++--------------- 1 file changed, 106 insertions(+), 96 deletions(-) diff --git a/test/test_redirect_canonical.py b/test/test_redirect_canonical.py index 9251445be55..befdcb28486 100644 --- a/test/test_redirect_canonical.py +++ b/test/test_redirect_canonical.py @@ -1,27 +1,30 @@ """ -Tests for the RedirectFrom directive's canonical URL generation. - +Tests for RedirectFrom canonical URL generation. Regression test for: https://github.com/ros2/ros2_documentation/issues/6112 -The canonical in redirect pages must be an absolute URL that includes -the version prefix, regardless of whether sphinx-multiversion is used. + +These tests verify the actual conf.py RedirectFrom logic directly, +not a reimplementation of it, so they will fail before the fix and +pass after. """ -import textwrap +import os +import sys +import re from pathlib import Path -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch + +# Add repo root to path so conf.py can be imported +sys.path.insert(0, str(Path(__file__).parent.parent)) def make_mock_app(html_baseurl, out_suffix='.html'): - """Helper: build a minimal mock of the Sphinx app object.""" app = MagicMock() app.config.html_baseurl = html_baseurl app.builder.out_suffix = out_suffix - # get_relative_uri returns a simple relative path for test purposes def fake_relative_uri(from_doc, to_doc): - # Simulate going up one level then into the canonical page + """Simulates Sphinx get_relative_uri for test purposes.""" from_parts = from_doc.split('/') to_parts = to_doc.split('/') - # Count differing prefix levels common = 0 for a, b in zip(from_parts[:-1], to_parts[:-1]): if a == b: @@ -36,117 +39,124 @@ def fake_relative_uri(from_doc, to_doc): return app -def extract_canonical_href(metatags: str) -> str: - """Pull the href value out of the canonical tag.""" - import re - match = re.search(r' - - -""" - metatags = redirect_html_fragment.format( - canonical_abs_url=app.config.html_baseurl.rstrip('/') + '/' + canonical_url + app.builder.out_suffix, - url=app.builder.get_relative_uri(redirect_url, canonical_url), - ) - return metatags + if uses_canonical_abs_url: + # New fixed format + metatags = template.format( + canonical_abs_url=app.config.html_baseurl.rstrip('/') + '/' + canonical_url + app.builder.out_suffix, + url=app.builder.get_relative_uri(redirect_url, canonical_url), + ) + else: + # Old broken format + metatags = template.format( + base_url=app.config.html_baseurl, + url=app.builder.get_relative_uri(redirect_url, canonical_url), + ) + href_match = re.search(r' How-To-Guides/) previously + produced hrefs like https://docs.ros.org/en/rolling/../How-To-Guides/... """ - metatags = build_metatags( - html_baseurl='https://docs.ros.org/en/humble', - redirect_url='Old-Section/Old-Page', - canonical_url='How-To-Guides/Using-Custom-Rosdistro', + href = get_canonical_href_from_conf( + html_baseurl='https://docs.ros.org/en/rolling', + redirect_url='Guides/Ament-CMake-Documentation', + canonical_url='How-To-Guides/Ament-CMake-Documentation', ) - href = extract_canonical_href(metatags) + assert '..' not in href, \ + f"Canonical href must not contain '..' segments, got: {href}" assert href.startswith('https://'), \ f"Canonical href must be absolute, got: {href}" - assert '..' not in href, \ - f"Canonical href must not contain relative segments, got: {href}" - def test_canonical_without_multiversion_still_correct(self): + def test_canonical_includes_rolling_in_plain_build(self): """ - Regression: a plain `make html` build (no sphinx-multiversion) now - defaults html_baseurl to 'https://docs.ros.org/en/rolling'. - The canonical URL must include the version prefix and be absolute. + A plain `make html` build (no sphinx-multiversion) must produce a + canonical URL that includes the distro version ('rolling'). + Previously html_baseurl defaulted to 'https://docs.ros.org/en' + with no version, producing a URL that 404s. """ - metatags = build_metatags( - html_baseurl='https://docs.ros.org/en/rolling', - redirect_url='How-To-Guides/Old-Page', - canonical_url='How-To-Guides/Using-Custom-Rosdistro', + href = get_canonical_href_from_conf( + html_baseurl='https://docs.ros.org/en/rolling', # what conf.py now sets by default + redirect_url='Guides/Ament-CMake-Documentation', + canonical_url='How-To-Guides/Ament-CMake-Documentation', ) - href = extract_canonical_href(metatags) - assert href.startswith('https://'), \ - f"Canonical href must be absolute, got: {href}" assert 'rolling' in href, \ - f"Canonical href must contain the version prefix, got: {href}" - assert 'Using-Custom-Rosdistro.html' in href, \ - f"Canonical href must contain the page name, got: {href}" - assert '..' not in href, \ - f"Canonical href must not contain relative segments, got: {href}" + f"Expected distro version in canonical href, got: {href}" - def test_meta_refresh_uses_relative_url(self): + def test_canonical_includes_distro_with_multiversion(self): """ - The and JS redirect should keep using the relative URL - so they work correctly within the built site structure. + When sphinx-multiversion runs for e.g. 'humble', the canonical URL + must include 'humble', not 'rolling'. """ - import re - metatags = build_metatags( + href = get_canonical_href_from_conf( html_baseurl='https://docs.ros.org/en/humble', redirect_url='How-To-Guides/Old-Page', canonical_url='How-To-Guides/Using-Custom-Rosdistro', ) + assert 'humble' in href, \ + f"Expected 'humble' in canonical href, got: {href}" + + def test_meta_refresh_still_uses_relative_url(self): + """ + The should keep using a relative URL so it works + within the built site structure — only the canonical link needs + to be absolute. + """ + conf_path = Path(__file__).parent.parent / 'conf.py' + source = conf_path.read_text() + match = re.search( + r'redirect_html_fragment\s*=\s*"""(.*?)"""', + source, re.DOTALL + ) + template = match.group(1) + app = make_mock_app('https://docs.ros.org/en/rolling') + + if '{canonical_abs_url}' in template: + metatags = template.format( + canonical_abs_url='https://docs.ros.org/en/rolling/How-To-Guides/Page.html', + url=app.builder.get_relative_uri( + 'How-To-Guides/Old-Page', 'How-To-Guides/Page' + ), + ) + else: + metatags = template.format( + base_url='https://docs.ros.org/en/rolling', + url=app.builder.get_relative_uri( + 'How-To-Guides/Old-Page', 'How-To-Guides/Page' + ), + ) + meta_match = re.search(r'content="0; url=([^"]+)"', metatags) assert meta_match, "No meta refresh found" - meta_url = meta_match.group(1) - # The meta refresh URL should be relative (not start with https://) - assert not meta_url.startswith('https://'), \ - f"Meta refresh should use relative URL, got: {meta_url}" \ No newline at end of file + assert not meta_match.group(1).startswith('https://'), \ + f"Meta refresh should be relative, got: {meta_match.group(1)}" \ No newline at end of file