Fix real_patterns mutation targeting undeclared extension point

[whisper67265]

Close #101.

Summary by CodeRabbit

• Documentation

  • Updated URL registration docs to describe the current startup registration flow, idempotency, and URL-prefix handling.

• Refactor

  • Moved URL registration logic into a dedicated adapter to centralize compatibility checks and registration behavior.

• Tests

  • Expanded tests covering compatibility validation, idempotent registration, registration-skipping cases, and an integration route resolution check.

[whisper67265]

@coderabbitai, full review.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details
gh pr view 106 --repo cppalliance/cppa-weblate-plugin

echo "=== PR DIFF ==="
gh pr diff 106 --repo cppalliance/cppa-weblate-plugin

echo "=== ISSUE 101 ==="
gh issue view 101 --repo cppalliance/cppa-weblate-plugin

Length of output: 19024

---

---

@whisper67265 Here is the full review of PR #106.

---

Overall Assessment

The PR successfully addresses issue #101 — URL registration is now delegated to a dedicated adapter, the sentinel attribute on Weblate's module is gone, and fail-fast layout checks replace silent skipping. The structure and test coverage are a significant improvement. I found one critical bug and one test-reliability bug that should be addressed before merge.

---

🔴 Critical Bug — Version("unknown") raises InvalidVersion, not WeblateUrlLayoutError

File: src/boost_weblate/endpoint/weblate_urls_adapter.py, Line 47

if Version(version) < _MIN_WEBLATE_VERSION:

_weblate_version() returns "unknown" when the Weblate package is not installed (e.g. in a bare test environment or a non-standard deployment). packaging.version.Version("unknown") raises packaging.version.InvalidVersion — not WeblateUrlLayoutError — so the error escapes the intended exception type and breaks the fail-fast contract.

Suggested fix:

def _assert_weblate_url_layout(wl_urls: ModuleType) -> None:
    """Verify Weblate exposes the ``real_patterns`` list before mutation."""
    version = _weblate_version()
    if not hasattr(wl_urls, "real_patterns"):
        msg = (
            "weblate.urls.real_patterns is missing; "
            f"Weblate {version} URL layout is incompatible with cppa-weblate-plugin"
        )
        raise WeblateUrlLayoutError(msg)
    if not isinstance(wl_urls.real_patterns, list):
        msg = (
            "weblate.urls.real_patterns is not a list; "
            f"Weblate {version} URL layout is incompatible with cppa-weblate-plugin"
        )
        raise WeblateUrlLayoutError(msg)
    if version == "unknown":
        msg = (
            "Weblate package version could not be determined; "
            "cppa-weblate-plugin cannot verify minimum version compatibility"
        )
        raise WeblateUrlLayoutError(msg)
    try:
        parsed = Version(version)
    except Exception as exc:  # packaging.version.InvalidVersion
        msg = (
            f"Weblate version string {version!r} could not be parsed; "
            "cppa-weblate-plugin cannot verify minimum version compatibility"
        )
        raise WeblateUrlLayoutError(msg) from exc
    if parsed < _MIN_WEBLATE_VERSION:
        msg = (
            f"Weblate {version} is below the minimum supported version "
            f"{_MIN_WEBLATE_VERSION}; cppa-weblate-plugin requires Weblate "
            f"{_MIN_WEBLATE_VERSION} or newer"
        )
        raise WeblateUrlLayoutError(msg)

This case is also not covered by any test — test_weblate_version_unknown_when_package_missing only tests _weblate_version() in isolation; it never exercises _assert_weblate_url_layout with the "unknown" version. A test like the following should be added:

def test_assert_layout_raises_when_weblate_version_unknown(
    monkeypatch: pytest.MonkeyPatch,
) -> None:
    fake = types.ModuleType("weblate.urls")
    fake.real_patterns = []
    monkeypatch.setattr(
        "boost_weblate.endpoint.weblate_urls_adapter._weblate_version",
        lambda: "unknown",
    )
    with pytest.raises(WeblateUrlLayoutError, match="version"):
        _assert_weblate_url_layout(fake)

---

🟡 Test Reliability — Missing lru_cache clearing in test_apps.py

File: tests/endpoint/test_apps.py, Line 40

test_register_plugin_urls_skips_when_weblate_urls_missing calls apps.register_plugin_urls() which internally calls the real register_boost_endpoint_urls(). Because register_boost_endpoint_urls is decorated with @lru_cache, if any earlier test in the same process already populated that cache, the monkeypatched builtins.__import__ is never reached and the test becomes a no-op. The _clear_url_registration_cache autouse fixture lives only in test_weblate_urls_adapter.py.

Suggested fix — add a fixture to test_apps.py:

from boost_weblate.endpoint.weblate_urls_adapter import register_boost_endpoint_urls

`@pytest.fixture`(autouse=True)
def _clear_cache() -> None:
    register_boost_endpoint_urls.cache_clear()
    yield
    register_boost_endpoint_urls.cache_clear()

---

🟡 Undeclared packaging dependency

File: src/boost_weblate/endpoint/weblate_urls_adapter.py, Line 14

from packaging.version import Version

packaging is ubiquitous (bundled with pip/setuptools) but it should be an explicit entry in the project's dependency list (pyproject.toml or setup.cfg) to avoid resolution surprises in minimal environments.

---

🟡 lru_cache caches early-exit path

If weblate.urls is not importable at startup, register_boost_endpoint_urls returns early with None, which is cached. Any subsequent call (e.g. from a test that later makes the module importable) will return the cached None without re-running. This is probably fine for production startup semantics, but it is worth a docstring note:

`@lru_cache`(maxsize=1)
def register_boost_endpoint_urls() -> None:
    """...
    
    Note: if ``weblate.urls`` is not importable at call time, the no-op result
    is still cached; retrying after the module becomes importable requires an
    explicit ``register_boost_endpoint_urls.cache_clear()`` call.
    """

---

ℹ️ test_plugin_ping_resolves_after_registration is environment-sensitive

File: tests/endpoint/test_weblate_urls_adapter.py, Line 153

def test_plugin_ping_resolves_after_registration() -> None:
    from django.urls import resolve
    register_boost_endpoint_urls()
    match = resolve("/boost-endpoint/plugin-ping/")

django.urls.resolve uses the ROOT_URLCONF setting. This test will pass only if the test environment's URL configuration includes Weblate's urlpatterns (which in turn uses real_patterns). If that assumption breaks, the test fails with a misleading Resolver404 rather than an assertion error. Consider adding a # noqa: integration-test marker or a guard that skips when Weblate is not configured.

---

✅ What's well done

• Extracting registration into its own module with a clear single responsibility is a clean improvement.
• _route_already_registered as a safety net beyond lru_cache (handles post-cache-clear re-entry correctly).
• The WeblateUrlLayoutError(RuntimeError) hierarchy is appropriate — callers can catch specifically or broadly.
• Documentation is updated consistently across README.md and docs/boost-endpoint-api.md.
• Test coverage for layout errors, idempotency, and import failure is thorough.

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactors URL registration to a new adapter: BoostEndpointConfig.ready() delegates to register_boost_endpoint_urls(), which validates weblate.urls.real_patterns and Weblate version, appends an idempotent URLResolver, with tests and docs updated and packaging pinned.

Changes

URL Registration Adapter Refactoring

Layer / File(s)	Summary
Adapter module structure, validation, and registration src/boost_weblate/endpoint/weblate_urls_adapter.py	New module adds WeblateUrlLayoutError, verifies weblate.urls.real_patterns exists and is a list, resolves Weblate package version, enforces minimum 2026.5, constructs a boost-endpoint/ URLResolver, detects existing registrations, and provides register_boost_endpoint_urls() decorated with functools.lru_cache(maxsize=1) to append the resolver idempotently.
apps.py delegation and app test refactoring src/boost_weblate/endpoint/apps.py, tests/endpoint/test_apps.py	register_plugin_urls() now delegates to register_boost_endpoint_urls() in the adapter; tests import the apps module and monkeypatch apps.register_boost_endpoint_urls to assert delegation.
Comprehensive adapter test suite tests/endpoint/test_weblate_urls_adapter.py	Tests add an autouse fixture clearing the adapter cache, assert WeblateUrlLayoutError on missing/invalid layouts or unsupported versions (including "unknown"), verify _weblate_version() behavior, validate route helpers and append semantics, and confirm registration idempotency and URL-resolution integration for /boost-endpoint/plugin-ping/.
Documentation updates & dependency pin README.md, docs/boost-endpoint-api.md, pyproject.toml	Docs updated to reflect delegation to register_boost_endpoint_urls(), lru_cache idempotency, and real_patterns/URL_PREFIX ordering; packaging==26.2 added to project dependencies.

Sequence Diagram(s)

sequenceDiagram
  participant BoostEndpointConfig
  participant Adapter as register_boost_endpoint_urls
  participant WeblateUrls as weblate.urls
  participant Patterns as real_patterns

  BoostEndpointConfig->>Adapter: call register_boost_endpoint_urls()
  Adapter->>WeblateUrls: import weblate.urls
  Adapter->>Adapter: validate layout and version
  Adapter->>WeblateUrls: check for existing boost-endpoint/ resolver
  Adapter->>Patterns: append URLResolver("boost-endpoint/") if missing

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• cppalliance/cppa-weblate-plugin#48: Modifies URL registration wiring in src/boost_weblate/endpoint/apps.py—related changes to how boost-endpoint is mounted into Weblate URLs.

Suggested reviewers

• henry0816191
• wpak-ai

Poem

🐰 A tiny rabbit hops on the code way,
I cached the routes so they won't run astray,
I check patterns, versions, then gently append,
No duplicates now — on this I depend! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR implements all acceptance criteria from issue #101: delegates registration to register_boost_endpoint_urls(), performs fail-fast layout validation, raises WeblateUrlLayoutError appropriately, and provides tests. However, the critical bug in _assert_weblate_url_layout() calling packaging.version.Version('unknown') causes InvalidVersion exception instead of WeblateUrlLayoutError when Weblate is absent.	Fix the exception handling in _assert_weblate_url_layout() to catch InvalidVersion when version='unknown' and re-raise as WeblateUrlLayoutError. Add a test case exercising the 'unknown' version path to prevent regression.
Docstring Coverage	⚠️ Warning	Docstring coverage is 10.71% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main refactoring: moving URL registration from BoostEndpointConfig.ready() to a dedicated adapter to fix direct mutation of weblate.urls.real_patterns without a declared extension point.
Out of Scope Changes check	✅ Passed	All changes are directly related to issue #101 objectives: refactoring URL registration into adapter, validation logic, idempotency via lru_cache, tests, and documentation updates. The packaging==26.2 dependency addition supports the new version parsing logic.
Description check	✅ Passed	The PR description in pr_objectives clearly explains the refactoring, objectives, and critical bug identified in code review. It references issue #101 and documents the fail-fast validation approach.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

src/boost_weblate/endpoint/weblate_urls_adapter.py (1)

72-80: ⚡ Quick win

Document that early-exit None is cached when weblate.urls import fails.

The lru_cache decorator makes this function idempotent, but it also caches the early return at line 88 when weblate.urls cannot be imported. This means if the module becomes importable later (e.g., after dynamic installation), subsequent calls will not retry. Callers who need to retry must explicitly call register_boost_endpoint_urls.cache_clear().

📝 Suggested docstring addition

 `@lru_cache`(maxsize=1)
 def register_boost_endpoint_urls() -> None:
     """Append Boost endpoint routes to ``weblate.urls.real_patterns`` once.
 
     Weblate builds ``urlpatterns`` from module-level ``real_patterns``. Appending
     before the ``URL_PREFIX`` wrapper keeps routes under prefix configuration.
 
     Idempotent via ``lru_cache``; safe to call from ``AppConfig.ready()``.
+    
+    Note: If ``weblate.urls`` import fails on the first call, the function
+    caches the early-exit ``None`` and will not retry on subsequent calls.
+    Call ``register_boost_endpoint_urls.cache_clear()`` to force a retry
+    after the module becomes importable.
     """

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/boost_weblate/endpoint/weblate_urls_adapter.py` around lines 72 - 80, The
function register_boost_endpoint_urls is wrapped with `@lru_cache`(maxsize=1) so
an early return (when importing weblate.urls fails) will be cached and future
calls won't retry; update the docstring of register_boost_endpoint_urls to
explicitly state that a failed import results in a cached None and callers that
need to retry after the module becomes available must call
register_boost_endpoint_urls.cache_clear() before re-invoking the function, and
mention lru_cache(maxsize=1) so readers can locate the caching behavior.

tests/endpoint/test_weblate_urls_adapter.py (1)

151-157: ⚡ Quick win

Test environment sensitivity: Route resolution depends on Weblate configuration.

This test calls Django's resolve() on /boost-endpoint/plugin-ping/, which requires that Django's ROOT_URLCONF is properly configured with Weblate's URL patterns. In test environments where Weblate is not fully configured or when running isolated unit tests, this may raise Resolver404.

Consider marking this test as integration-sensitive or conditionally skipping it when Weblate is not configured.

🧪 Suggested approach: Mark test or add conditional skip

Option 1: Mark as integration test with pytest marker:

`@pytest.mark.integration`  # or `@pytest.mark.django_db` if using pytest-django
def test_plugin_ping_resolves_after_registration() -> None:
    ...

Option 2: Add conditional skip:

def test_plugin_ping_resolves_after_registration() -> None:
    from django.urls import resolve, Resolver404
    
    try:
        register_boost_endpoint_urls()
        match = resolve("/boost-endpoint/plugin-ping/")
    except Resolver404:
        pytest.skip("Weblate URL configuration not available in test environment")
    
    assert match.url_name == "plugin-ping"
    assert match.func.__name__ == "plugin_ping"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/endpoint/test_weblate_urls_adapter.py` around lines 151 - 157, The test
test_plugin_ping_resolves_after_registration is fragile because resolving
"/boost-endpoint/plugin-ping/" can raise Resolver404 when Weblate URL patterns
aren't configured; update the test to either mark it as an integration/django_db
test (e.g., add a pytest marker on test_plugin_ping_resolves_after_registration)
or add a conditional skip by importing Resolver404 and pytest, wrapping the
register_boost_endpoint_urls() + resolve(...) call in a try/except that catches
Resolver404 and calls pytest.skip with a short reason; keep the existing
assertions unchanged so they only run when route resolution succeeds.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/endpoint/test_apps.py`:
- Around line 14-29: Add an autouse pytest fixture to
tests/endpoint/test_apps.py that clears the LRU cache of the cached function
register_boost_endpoint_urls before each test so monkeypatched implementations
are exercised; specifically, create a fixture (e.g.,
clear_register_boost_endpoint_urls_cache) marked autouse that calls
register_boost_endpoint_urls.cache_clear() (or the equivalent cache-clear API)
so tests like test_register_plugin_urls_delegates_to_adapter and
test_register_plugin_urls_skips_when_weblate_urls_missing always run against the
fresh, un-cached function.

In `@tests/endpoint/test_weblate_urls_adapter.py`:
- Around line 75-82: Add a test ensuring _assert_weblate_url_layout raises
WeblateUrlLayoutError when _weblate_version() returns "unknown": monkeypatch
_weblate_version to return "unknown", create a fake module object with a
real_patterns attribute (e.g., empty list), and assert that calling
_assert_weblate_url_layout(fake) raises WeblateUrlLayoutError with a message
like "Cannot determine Weblate version"; this verifies the code path where
Version("unknown") is avoided and the explicit WeblateUrlLayoutError is raised.

---

Nitpick comments:
In `@src/boost_weblate/endpoint/weblate_urls_adapter.py`:
- Around line 72-80: The function register_boost_endpoint_urls is wrapped with
`@lru_cache`(maxsize=1) so an early return (when importing weblate.urls fails)
will be cached and future calls won't retry; update the docstring of
register_boost_endpoint_urls to explicitly state that a failed import results in
a cached None and callers that need to retry after the module becomes available
must call register_boost_endpoint_urls.cache_clear() before re-invoking the
function, and mention lru_cache(maxsize=1) so readers can locate the caching
behavior.

In `@tests/endpoint/test_weblate_urls_adapter.py`:
- Around line 151-157: The test test_plugin_ping_resolves_after_registration is
fragile because resolving "/boost-endpoint/plugin-ping/" can raise Resolver404
when Weblate URL patterns aren't configured; update the test to either mark it
as an integration/django_db test (e.g., add a pytest marker on
test_plugin_ping_resolves_after_registration) or add a conditional skip by
importing Resolver404 and pytest, wrapping the register_boost_endpoint_urls() +
resolve(...) call in a try/except that catches Resolver404 and calls pytest.skip
with a short reason; keep the existing assertions unchanged so they only run
when route resolution succeeds.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 87dd90cd-9925-4991-9d0a-5019cd7dfa0e

📥 Commits

Reviewing files that changed from the base of the PR and between b3a9c8c and caf8c44.

📒 Files selected for processing (6)

• README.md
• docs/boost-endpoint-api.md
• src/boost_weblate/endpoint/apps.py
• src/boost_weblate/endpoint/weblate_urls_adapter.py
• tests/endpoint/test_apps.py
• tests/endpoint/test_weblate_urls_adapter.py

[coderabbitai]

🧹 Nitpick comments (1)

tests/endpoint/test_weblate_urls_adapter.py (1)

50-73: 💤 Low value

Optional: Consider adding a test for the InvalidVersion exception path.

The adapter wraps Version(version) parsing in a try-except at lines 55-62 of weblate_urls_adapter.py to catch InvalidVersion and re-raise as WeblateUrlLayoutError. While the current tests cover "unknown" (lines 50-61) and too-low versions (lines 63-73), there's no test exercising the InvalidVersion exception path with a malformed version string (e.g., "not.a.version!").

This case is unlikely in practice since importlib.metadata.version("Weblate") returns valid strings or raises PackageNotFoundError, but a test would complete the coverage of the validation logic.

🧪 Optional test case

def test_assert_layout_raises_when_version_malformed(
    monkeypatch: pytest.MonkeyPatch,
) -> None:
    """Verify malformed version strings raise WeblateUrlLayoutError."""
    fake = types.ModuleType("weblate.urls")
    fake.real_patterns = []
    monkeypatch.setattr(
        "boost_weblate.endpoint.weblate_urls_adapter._weblate_version",
        lambda: "not.a.version!",
    )
    with pytest.raises(
        WeblateUrlLayoutError,
        match="could not be parsed",
    ):
        _assert_weblate_url_layout(fake)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/endpoint/test_weblate_urls_adapter.py` around lines 50 - 73, Add a test
that exercises the InvalidVersion parsing path by monkeypatching
boost_weblate.endpoint.weblate_urls_adapter._weblate_version to return a
malformed string (e.g., "not.a.version!") and calling _assert_weblate_url_layout
with a fake module whose real_patterns = []; assert it raises
WeblateUrlLayoutError and matches the "could not be parsed" message so the
Version(...) parsing exception path is covered (refer to
_assert_weblate_url_layout, _weblate_version, and WeblateUrlLayoutError).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/endpoint/test_weblate_urls_adapter.py`:
- Around line 50-73: Add a test that exercises the InvalidVersion parsing path
by monkeypatching boost_weblate.endpoint.weblate_urls_adapter._weblate_version
to return a malformed string (e.g., "not.a.version!") and calling
_assert_weblate_url_layout with a fake module whose real_patterns = []; assert
it raises WeblateUrlLayoutError and matches the "could not be parsed" message so
the Version(...) parsing exception path is covered (refer to
_assert_weblate_url_layout, _weblate_version, and WeblateUrlLayoutError).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d7800631-b3d7-4ba3-87eb-5ea78f05559e

📥 Commits

Reviewing files that changed from the base of the PR and between caf8c44 and cf80655.

⛔ Files ignored due to path filters (1)

• uv.lock is excluded by !**/*.lock

📒 Files selected for processing (4)

• pyproject.toml
• src/boost_weblate/endpoint/weblate_urls_adapter.py
• tests/endpoint/test_apps.py
• tests/endpoint/test_weblate_urls_adapter.py

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/endpoint/test_apps.py

[henry0816191]

Lgtm