Skip to content

[chores:fix] Fixed collection of metrics via hex UUID path#821

Merged
nemesifier merged 2 commits into
masterfrom
fix-regression-monitoring-data
Jun 23, 2026
Merged

[chores:fix] Fixed collection of metrics via hex UUID path#821
nemesifier merged 2 commits into
masterfrom
fix-regression-monitoring-data

Conversation

@nemesifier

Copy link
Copy Markdown
Member

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • N/A I have updated the documentation.

Reference to Existing Issue

Fixing regression introduced in #816.

Description of Changes

The recent change to url patterns introduced a regression. A similar fix to the same issue was done recently to OpenWISP Controller, the best solution is to reuse the same solution also in OpenWISP Monitoring.

The recent change to url patterns introduced a regression.
A similar fix to the same issue was done recently to
OpenWISP Controller, the best solution is to reuse the same
solution also in OpenWISP Monitoring.
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9fc666c6-22a5-46b3-ad3f-54ca32e3a84c

📥 Commits

Reviewing files that changed from the base of the PR and between b784329 and b86283b.

📒 Files selected for processing (1)
  • openwisp_monitoring/device/api/urls.py
📜 Recent review details
⏰ Context from checks skipped due to timeout. (13)
  • GitHub Check: Python==3.12 | django~=5.2.0
  • GitHub Check: Python==3.11 | django~=5.2.0
  • GitHub Check: Python==3.13 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=4.2.0
  • GitHub Check: Python==3.10 | django~=5.2.0
  • GitHub Check: Python==3.11 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=5.1.0
  • GitHub Check: Python==3.10 | django~=4.2.0
  • GitHub Check: Python==3.11 | django~=4.2.0
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Python==3.10 | django~=5.1.0
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (2)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}

📄 CodeRabbit inference engine (Custom checks)

Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable

Files:

  • openwisp_monitoring/device/api/urls.py
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready).
Avoid unnecessary blank lines inside function and method bodies.
Mark user-facing strings for translation with Django i18n helpers in Django code.
Preserve tenant isolation and object-level permissions for organizations, devices, checks, metrics, charts, alerts, and related data.
Be careful with queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, time series database backends, and dashboard/websocket updates.
Watch for cross-tenant data leaks, permission bypasses, unsafe query construction, excessive metric ingestion, insecure credentials, and secrets.
Preserve validation around metric payloads, device data, alert configuration, time series queries, URLs, and backend credentials.
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it.

Files:

  • openwisp_monitoring/device/api/urls.py
🧠 Learnings (3)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.

Applied to files:

  • openwisp_monitoring/device/api/urls.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.

Applied to files:

  • openwisp_monitoring/device/api/urls.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.

Applied to files:

  • openwisp_monitoring/device/api/urls.py
🔇 Additional comments (1)
openwisp_monitoring/device/api/urls.py (1)

19-20: 🗄️ Data Integrity & Integration

No changes needed. The uuid_any converter is correctly used for the device metric endpoint to support hex UUID format (without dashes), as required by the test_200_create_uuid_without_dashes test which posts device metrics using device.pk.hex. The converter is registered by openwisp-controller (listed as a dependency in requirements.txt). The selective use on only the device metric endpoint (line 20) is intentional—other device endpoints (lines 14, 25, etc.) remain unchanged because clients use standard UUID format, while monitoring devices send hex UUIDs. The comment on line 19 properly explains this non-obvious choice.


📝 Walkthrough

Walkthrough

The device metrics API URL pattern in openwisp_monitoring/device/api/urls.py changes the pk path converter from the built-in uuid to uuid_any, enabling the endpoint to match UUIDs formatted without dashes. A new test test_200_create_uuid_without_dashes is added in TestDeviceApi that creates an organization and device, then POSTs DeviceMonitoring data using the device UUID in hex form (no dashes), asserting an HTTP 200 response.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the type and nature of the fix: using hex UUID path support for metrics collection through the URL pattern change.
Description check ✅ Passed The description addresses the regression fix, references the related issue, and confirms completion of testing and documentation requirements as per the template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Bug Fixes ✅ Passed The PR fixes the root cause of the regression by changing the device metric API route's UUID path converter from uuid (strict format only) to uuid_any (accepts both formats). A proper regressio...

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-regression-monitoring-data

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kilo-code-bot

kilo-code-bot Bot commented Jun 23, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • openwisp_monitoring/device/api/urls.py
  • openwisp_monitoring/device/tests/test_api.py

Notes

The change correctly addresses the regression from #816 by switching the api_device_metric route's path converter from <uuid:pk> to <uuid_any:pk>, allowing device metric collection via hex (dashless) UUID paths. The uuid_any converter is provided by the openwisp-controller dependency (1.3 branch, per requirements.txt), consistent with the approach mentioned in the PR description. The latest commit adds a clarifying comment documenting that uuid_any is registered by openwisp-controller.

A focused regression test (test_200_create_uuid_without_dashes) is included: it posts data using device.pk.hex (UUID without dashes) and asserts a 200 response. This test would fail under the old <uuid:pk> converter (which only matches dashed UUIDs) and passes with the new <uuid_any:pk> converter, satisfying the regression-test requirement.

Previous Review Summary (commit b784329)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit b784329)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • openwisp_monitoring/device/api/urls.py
  • openwisp_monitoring/device/tests/test_api.py

Notes

The change correctly addresses the regression from #816 by switching the api_device_metric route's path converter from <uuid:pk> to <uuid_any:pk>, allowing device metric collection via hex (dashless) UUID paths. The uuid_any converter is provided by the openwisp-controller dependency (1.3 branch, per requirements.txt), consistent with the approach mentioned in the PR description.

A focused regression test (test_200_create_uuid_without_dashes) is included: it posts data using device.pk.hex (UUID without dashes) and asserts a 200 response. This test would fail under the old <uuid:pk> converter (which only matches dashed UUIDs) and passes with the new <uuid_any:pk> converter, satisfying the regression-test requirement.


Reviewed by glm-5.2-20260616 · Input: 10.5K · Output: 659 · Cached: 61.1K

@github-project-automation github-project-automation Bot moved this from In progress to Reviewer approved in OpenWISP Priorities for next releases Jun 23, 2026

@pandafy pandafy 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.

LGTM!

BTW, I didn't find any documentation for uuid_any in openwisp-controller.

@nemesifier

Copy link
Copy Markdown
Member Author

LGTM!

BTW, I didn't find any documentation for uuid_any in openwisp-controller.

It's not documented, we should not needed it outside of controller and monitoring, so it can remain for internal use only.

@nemesifier nemesifier merged commit dff5149 into master Jun 23, 2026
32 checks passed
@nemesifier nemesifier deleted the fix-regression-monitoring-data branch June 23, 2026 14:40
@github-project-automation github-project-automation Bot moved this from Reviewer approved to Done in OpenWISP Priorities for next releases Jun 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Development

Successfully merging this pull request may close these issues.

2 participants