Skip to content

[Fixes #14552] Fragile /api/v2/ router URL registration - #14559

Open
etj wants to merge 1 commit into
masterfrom
fix-v2-router-registration-order
Open

[Fixes #14552] Fragile /api/v2/ router URL registration#14559
etj wants to merge 1 commit into
masterfrom
fix-v2-router-registration-order

Conversation

@etj

@etj etj commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Quick list:

  • Router-registration ordering fix (the core bug)
  • Dead upload-API override removed
  • urlpatterns += cosmetic merges
  • ^api/v2/ registrations grouped
  • Three prefixes hoisted (metadata, facets, management_commands_http) out of their apps' own files

PR description:

Fixes the /api/v2/ router registration bug: router.urls is a cached property, and resource/harvesting used to inject their URLs from AppConfig.ready() -- which runs after geonode/urls.py's own code -- so anything registered only that way could be silently missing depending on app-loading order. They're hardcoded includes now, like every other app, and the canonical include(router.urls) is positioned after everything that can register onto it.

Also, while in the area:

  • Removes a dead upload-API override mechanism left over from before geonode-importer was vendored in.
  • Merges a few urlpatterns += splits with no functional reason to be separate, and groups the /api/v2/ registrations into one block.
  • Hoists the common metadata/facets/management prefixes out of those apps' own urls.py into the include() in geonode/urls.py, removing the repeated prefix in every one of their paths.

No behavior change beyond the fix itself -- every step verified by diffing the fully resolved URLconf before/after (with PYTHONHASHSEED pinned, since DREST's field-name route ordering is otherwise non-deterministic per process).

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

For all pull requests:

  • Confirm you have read the contribution guidelines
  • You have sent a Contribution Licence Agreement (CLA) as necessary (not required for small changes, e.g., fixing typos in the documentation)
  • Make sure the first PR targets the master branch, eventual backports will be managed later. This can be ignored if the PR is fixing an issue that only happens in a specific branch, but not in newer ones.

The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):

  • There is a ticket in https://github.com/GeoNode/geonode/issues describing the issue/improvement/feature (a notable exemption is, changes not visible to end-users)
  • The issue connected to the PR must have Labels and Milestone assigned
  • PR for bug fixes and small new features are presented as a single commit
  • PR title must be in the form "[Fixes #<issue_number>] Title of the PR"
  • New unit tests have been added covering the changes, unless there is an explanation on why the tests are not necessary/implemented

Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.

@etj etj self-assigned this Aug 25, 2026
Copilot AI lite review requested due to automatic review settings August 25, 2026 15:26
@cla-bot cla-bot Bot added the cla-signed CLA Bot: community license agreement signed label Aug 25, 2026
@etj
etj requested a review from mattiagiupponi August 25, 2026 15:30

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

This PR fixes a fragile /api/v2/ URL registration issue caused by router.urls being a cached property and some endpoints being registered/injected too late (via AppConfig.ready()), making routes intermittently missing depending on app-loading order.

Changes:

  • Reorders and groups /api/v2/ URL includes so all router.register() side effects occur before the single canonical include(router.urls).
  • Removes the dead upload URL override/injection mechanism and related empty URLconf.
  • Hoists common URL prefixes (metadata, facets, management commands) into geonode/urls.py and adds regression tests to guard against router completeness regressions.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
geonode/urls.py Groups /api/v2/ registrations and moves include(router.urls) to the end to avoid cached/partial router URL sets.
geonode/upload/apps.py Removes dead ready() hook that attempted to inject upload API URLs.
geonode/upload/api/urls.py Deletes unused/empty upload API URLconf.
geonode/resource/apps.py Removes URL injection from AppConfig.ready() (registration now handled via URL includes).
geonode/metadata/api/urls.py Stops reading router.urls inside the module; keeps only metadata extra endpoints and relies on the global router include.
geonode/management_commands_http/urls.py Drops internal management/ prefix to rely on the include prefix from geonode/urls.py.
geonode/harvesting/apps.py Removes URL injection from AppConfig.ready(); keeps signal wiring and scheduler setup.
geonode/facets/urls.py Updates facets URL patterns to rely on the include prefix from geonode/urls.py.
geonode/api/tests.py Adds regression tests ensuring the shared router remains complete and no longer depends on ready() URL injection.
Suppressed comments (1)

geonode/facets/urls.py:26

  • path() routes cannot start with a leading / in Django; this will raise ImproperlyConfigured when loading the URLconf. With the current include prefix (^api/v2/facets), the remaining path for the detail view begins with /, so this should be expressed as a re_path() regex instead.
    path("/<facet>", GetFacetView.as_view(), name="get_facet"),

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread geonode/facets/urls.py
@@ -20,7 +20,8 @@
from django.urls import path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed CLA Bot: community license agreement signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants