fix: replace serializers.Serializer with MonitoringDeviceDetailSerial…#744
fix: replace serializers.Serializer with MonitoringDeviceDetailSerial…#744shakurah wants to merge 11 commits intoopenwisp:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIn openwisp_monitoring/device/api/views.py the DeviceMetricView.serializer_class was changed from rest_framework.serializers.Serializer to MonitoringDeviceDetailSerializer. The view now obtains device metric data via self.serializer_class(self.instance).data rather than directly instantiating MonitoringDeviceDetailSerializer. The import of Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_monitoring/device/api/views.py`:
- Line 123: The get() method currently bypasses the view's serializer_class by
calling MonitoringDeviceDetailSerializer(self.instance).data directly, causing
double-serialization or schema/runtime mismatches; change get() to use the
view's standard serializer retrieval (call self.get_serializer(self.instance))
and use its .data so both drf-yasg/schema generation and runtime responses rely
on the same serializer; ensure MonitoringApiViewMixin.get() behavior is
compatible (i.e., if it already calls get_serializer(), remove the hardcoded
MonitoringDeviceDetailSerializer usage) and keep serializer_class =
MonitoringDeviceDetailSerializer as the single source of truth.
- Line 123: The view sets serializer_class = MonitoringDeviceDetailSerializer
but assigns a plain JSON schema dict to schema, which drf-yasg ignores and
causes incorrect POST/GET/CSV documentation; fix by creating a drf-yasg
AutoSchema subclass (e.g., MonitoringDeviceAutoSchema) that returns the correct
per-method requestBody and responses using the existing JSON schema dict from
openwisp_monitoring/device/schema.py and the actual merged GET response (device
detail + charts) and CSV response (charts only), then set schema =
MonitoringDeviceAutoSchema() on the view; alternatively, if
MonitoringDeviceDetailSerializer is not used at runtime (the view hardcodes
serialization), remove serializer_class to avoid misleading docs.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_monitoring/device/api/views.py
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 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/views.py
🧬 Code graph analysis (1)
openwisp_monitoring/device/api/views.py (1)
openwisp_monitoring/device/api/serializers.py (1)
MonitoringDeviceDetailSerializer(109-110)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_monitoring/device/api/views.py (1)
123-158: Previously raised concerns remain unaddressed.The
serializer_classassignment is the right intent for fixing drf-yasg schema generation. However, two pre-existing issues flagged in prior review rounds are still present in the current code and are directly relevant to this change:
Double-serialization / decoupled
serializer_class(line 154):get()still hardcodesMonitoringDeviceDetailSerializer(self.instance).dataindependently ofserializer_class. IfMonitoringApiViewMixin.get()callsself.get_serializer()internally, the serializer is invoked twice per request. At minimum, line 154 should useself.get_serializer(self.instance).dataso the runtime path and the schema-inferred path share a single source of truth.drf-yasg schema mismatch (lines 123, 125): The
schema = schemaassignment on line 125 is a plaindict(fromopenwisp_monitoring/device/schema.py), not a drf-yasgAutoSchemasubclass — drf-yasg silently ignores it and falls back entirely toserializer_class. This means:
- POST: Request body is documented as
MonitoringDeviceDetailSerializerfields, but the actual POST payload is raw device-metrics JSON (NetJSON format).- GET: Response is documented as
MonitoringDeviceDetailSerializeronly, but the actual response isMonitoringDeviceDetailSerializerdata merged with chart data (line 155–156).- CSV path (line 158): Returns only chart data, but is documented as
MonitoringDeviceDetailSerializer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/device/api/views.py` around lines 123 - 158, The view still double-serializes and misleads drf-yasg: replace the hardcoded MonitoringDeviceDetailSerializer(self.instance).data in get() with self.get_serializer(self.instance).data (so the runtime serializer is the same as serializer_class and used by any MonitoringApiViewMixin.get() path), and replace the plain dict assigned to schema (the schema variable) with a proper drf-yasg AutoSchema-compatible inspector or remove the dict so drf-yasg uses serializer_class correctly; ensure GET responses documented reflect the merged payload (MonitoringDeviceDetailSerializer + charts) and the CSV branch documents/returns only chart data by updating the view’s schema implementation to return the correct request/response schemas for get/post/csv.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_monitoring/device/api/views.py`:
- Around line 123-158: The view still double-serializes and misleads drf-yasg:
replace the hardcoded MonitoringDeviceDetailSerializer(self.instance).data in
get() with self.get_serializer(self.instance).data (so the runtime serializer is
the same as serializer_class and used by any MonitoringApiViewMixin.get() path),
and replace the plain dict assigned to schema (the schema variable) with a
proper drf-yasg AutoSchema-compatible inspector or remove the dict so drf-yasg
uses serializer_class correctly; ensure GET responses documented reflect the
merged payload (MonitoringDeviceDetailSerializer + charts) and the CSV branch
documents/returns only chart data by updating the view’s schema implementation
to return the correct request/response schemas for get/post/csv.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_monitoring/device/api/views.py
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 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/views.py
🧬 Code graph analysis (1)
openwisp_monitoring/device/api/views.py (1)
openwisp_monitoring/device/api/serializers.py (1)
MonitoringDeviceDetailSerializer(109-110)
🔇 Additional comments (1)
openwisp_monitoring/device/api/views.py (1)
16-16: LGTM — import cleanup is correct.Removing
serializersfrom the import is accurate sinceserializers.Serializeris no longer referenced anywhere in the file.
nemesifier
left a comment
There was a problem hiding this comment.
@shakurah thanks for this! I think you're right. There's a suggestion from coderabbit which sounds valid see my comment below and let me know what you think.
9f9a8f9 to
42409f2
Compare
|
try this - thee are some of the valid tags |
7eb141c to
068e985
Compare
4987185 to
d116a94
Compare
CI Build Failed: Script Not FoundHello @shakurah,
Fix: Ensure that the |
CI Failures: Black Formatting and Missing ScriptHello @shakurah,
|
d081547 to
cf916d7
Compare
CI Failures: Black Formatting and Missing Test ScriptHello @shakurah,
|
…izer in DeviceMetricView inside api views to display correct response schema in API documentation
…self.serializer_class
…penwisp#716 Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Fixes openwisp#716
Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Fixes openwisp#716
Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Fixes openwisp#716
…p#716 Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Related to openwisp#716
…p#716 Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Related to openwisp#716
…p#716 Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Related to openwisp#716
…p#716 Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Related to openwisp#716
…wisp#716 Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Related to openwisp#716
cf916d7 to
6e2b495
Compare
replace serializers.Serializer with MonitoringDeviceDetailSerializer in DeviceMetricView
Checklist
Reference to Existing Issue
Closes #716.
Description of Changes
The
DeviceMetricViewwas usingserializers.Serializeras itsserializer_classinstead of
MonitoringDeviceDetailSerializer. This caused drf-yasg to be unable toinfer the correct response schema, resulting in a generic example value being displayed
in the API documentation instead of the actual response fields.
Replacing it with
MonitoringDeviceDetailSerializerallows drf-yasg to correctlygenerate the schema for the endpoint, making the API documentation accurately reflect
the real response structure.
Screenshot