[feature] Add support to reversion to the REST API #894#994
[feature] Add support to reversion to the REST API #894#994dee077 wants to merge 5 commits intoopenwisp:masterfrom
Conversation
dee077
force-pushed
the
feature/894-rest-api-revisions
branch
from
8690016 to
3fdfcaf
Compare
|
coverage: 98.642%. first build — dee077:feature/894-rest-api-revisions into openwisp:master |
dee077
force-pushed
the
feature/894-rest-api-revisions
branch
from
3fdfcaf to
98ca619
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Good work! The code is clean and well-structured.
However, this PR seems to be missing its main goal: ensuring that every versioned model admin has a corresponding versioned REST API that logs revisions—tracking who changed what—each time a modification is made.
Please check my comments below for further details.
dee077
changed the title
nemesifier
changed the title
dee077
force-pushed
the
feature/894-rest-api-revisions
branch
from
84a68d6 to
df5c3fa
Compare
|
I’ve added REST API revision support using django-reversion. Here’s a summary What’s Added
New Endpoints:
How It Works:
Issues Noticed
QuestionGiven the unexpected behavior in signals and added complexity: |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Looks on the right track.
I will do a manual round of testing soon, in the meantime see my comments below.
We'll need to add this to pki, geo and connection.
| 'controller/<str:model_slug>/revision/<str:pk>/restore/', | ||
| api_views.revision_restore, | ||
| name='revision_restore', | ||
| ), |
There was a problem hiding this comment.
What if we shorten this to just model? I think it would be clear enough and more concise. It's a small thing but nonetheless over time these simplifications help to keep maintenance sane.
There was a problem hiding this comment.
I have another doubt on this, how are we enforcing that we are enabling this only on the models that support reversions in this app? Eg: Device, Template, Vpn, etc. Would this work for any model?
There was a problem hiding this comment.
Currently, empty revisions are being stored with only user and comment entries, which does not offer anything.
There was a problem hiding this comment.
Added a fix for this
For reference these are the models that are getting revisions
github-project-automation
Bot
moved this to To do (general)
in OpenWISP Contributor's Board
Updates
|
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Can you point to the lines of code that are generating the additional queries?
Have you found out what additional queries are being generated that before weren't?
If we can see the query it would be faster to verify whether all the queries are needed, the increase seems pretty significant.
dee077
force-pushed
the
feature/894-rest-api-revisions
branch
from
34dbfcf to
47e1c70
Compare
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
Can you point to the lines of code that are generating the additional queries?
Have you found out what additional queries are being generated that before weren't?
Here's a diff of the queries generated by the test_patch_deviceconnectoin_detail,
https://www.diffchecker.com/ToUeQeyZ/
From my analysis, it appears that django-rivision creates a version for all related objects (here - DeviceConnection, Config and Device). This is inline with VersionAdmin behaviour.
Updates
|
Updates
With this approach, only registered models will store revisions, while unregistered models can still include the mixin without any side effects. This allows in the future to safely add the mixin to new POST, PUT, or PATCH requests without needing to worry about whether the model is registered with reversion. |
dee077
force-pushed
the
feature/894-rest-api-revisions
branch
2 times, most recently
from
36b017e to
92b7954
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@dee077 can you please run openwisp-qa-format here again with the latest version of openwisp-utils? See openwisp/openwisp-utils#456.
dee077
force-pushed
the
feature/894-rest-api-revisions
branch
2 times, most recently
from
fd1faab to
3afb759
Compare
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
I did a round of manual testing with the Templates model. Changing the model fields from the API endpoints creates revisions as expected. Kudos!
Is it possible to list all revisions of a particular object? Similar to this view
https://demo.openwisp.io/admin/config/template/e450012e-a482-48ad-9b2b-f0faa93b9cac/history/
If not, it really hurts the functionality of the REST API endpoints.
This end point is missing from the issue description. Let's discuss it with @nemesifier first before proceeding on this front.
| def get_queryset(self): | ||
| model = self.kwargs.get("model").lower() | ||
| queryset = self.queryset.filter(content_type__model=model) | ||
| revision_id = self.request.query_params.get("revision_id") | ||
| if revision_id: | ||
| queryset = queryset.filter(revision_id=revision_id) | ||
| return self.queryset.filter(content_type__model=model) |
There was a problem hiding this comment.
The way this method is written breaks the convention of Django/DRF. It is also inconsistent with OpenWISP.
| def get_queryset(self): | |
| model = self.kwargs.get("model").lower() | |
| queryset = self.queryset.filter(content_type__model=model) | |
| revision_id = self.request.query_params.get("revision_id") | |
| if revision_id: | |
| queryset = queryset.filter(revision_id=revision_id) | |
| return self.queryset.filter(content_type__model=model) | |
| def get_queryset(self): | |
| queryset = super().get_queryset() | |
| model = self.kwargs.get("model", "").lower() | |
| queryset = queryset.filter(content_type__model=model) | |
| if revision_id := self.request.query_params.get("revision_id"): | |
| queryset = queryset.filter(revision_id=revision_id) | |
| return queryset |
There was a problem hiding this comment.
Right now, it is not filtering the queryset with the revision_id, since the return statement filters the queryset with just content_type__model.
And, there's no test which verifies the filtering. This is bad.
There was a problem hiding this comment.
Filtering using revision_id should be done with django_filters instead.
https://django-filter.readthedocs.io/en/stable/guide/rest_framework.html#quickstart
You can check out other API views for details.
| def get_queryset(self): | ||
| model = self.kwargs.get("model").lower() | ||
| return self.queryset.filter(content_type__model=model) |
There was a problem hiding this comment.
| def get_queryset(self): | |
| model = self.kwargs.get("model").lower() | |
| return self.queryset.filter(content_type__model=model) | |
| def get_queryset(self): | |
| model = self.kwargs.get("model").lower() | |
| return super().get_querset().filter(content_type__model=model) |
Maybe, we can create a Mixin class which filters the queryset like this.
| def get_queryset(self): | ||
| model = self.kwargs.get("model").lower() | ||
| return self.queryset.filter(content_type__model=model) |
| with transaction.atomic(): | ||
| with reversion.create_revision(): | ||
| for version in versions: | ||
| version.revert() | ||
| reversion.set_user(request.user) | ||
| reversion.set_comment( | ||
| f"Restored to previous revision: {self.kwargs.get('pk')}" | ||
| ) |
There was a problem hiding this comment.
Why don't we use Revision.revert() instead of reverting individual versions?
nemesifier
changed the title
dee077
force-pushed
the
feature/894-rest-api-revisions
branch
from
3afb759 to
27e70d9
Compare
|
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:
📝 WalkthroughWalkthroughAdds django-reversion support to the REST API. Introduces AutoRevisionMixin that wraps authenticated POST/PUT/PATCH requests for reversion-registered models to create revisions, set the revision user, and add a comment. Exposes revision data via ReversionSerializer and ReversionListFilter and adds three API endpoints: reversion list, detail, and restore. Applies AutoRevisionMixin across config, connection, geo, and pki API views and adds tests exercising listing, detail, and restore flows; many tests' query-count assertions were updated. Sequence Diagram(s)sequenceDiagram
participant Client as Client/API Request
participant View as API View (with AutoRevisionMixin)
participant Mixin as AutoRevisionMixin
participant Reversion as django-reversion
participant DB as Database
Client->>View: POST/PUT/PATCH (authenticated)
View->>Mixin: dispatch(request)
Note right of Mixin: check method & model registration
alt model registered & mutation
Mixin->>Reversion: create_revision(atomic=self.revision_atomic)
Reversion->>DB: begin revision transaction
Mixin->>View: call original dispatch()
View->>DB: perform create/update
Mixin->>Reversion: set revision user & comment
Reversion->>DB: save Version records
Reversion->>DB: commit transaction
View-->>Client: response
else not applicable
Mixin->>View: call original dispatch()
View-->>Client: response
end
sequenceDiagram
participant Client as Client
participant RevAPI as Reversion API (List/Detail/Restore)
participant Serializer as ReversionSerializer
participant Reversion as django-reversion
participant DB as Database
Client->>RevAPI: GET /controller/{model}/reversion/
RevAPI->>DB: query Version objects filtered by content_type and params
DB-->>RevAPI: Version queryset
RevAPI->>Serializer: serialize versions
Serializer-->>RevAPI: JSON payload
RevAPI-->>Client: response
Client->>RevAPI: POST /controller/{model}/reversion/{pk}/restore/
RevAPI->>DB: fetch Version(s) by revision_id / pk
DB-->>RevAPI: Version and Revision
RevAPI->>Reversion: revert revision inside transaction
Reversion->>DB: restore object state(s)
DB-->>RevAPI: commit
RevAPI->>Serializer: serialize restored Version records
RevAPI-->>Client: restored revision payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @openwisp_controller/config/api/urls.py:
- Around line 16-30: RevisionListView.get_queryset currently builds a filtered
queryset into a local variable but returns self.queryset (ignoring the filter);
update RevisionListView.get_queryset to return the filtered queryset variable
(the one applying revision_id from request.GET) instead of self.queryset. Also
standardize error behavior across endpoints by replacing the use of
get_list_or_404 in RevisionRestoreView with the same safe filter/empty-list
pattern used by the list and detail views (i.e., perform a .filter(...) on the
model revisions and return an empty list when the model is invalid/unregistered
instead of raising 404), making the endpoints consistent.
In @openwisp_controller/config/tests/test_api.py:
- Around line 1662-1667: The test is asserting two results after filtering by
revision_id which is incorrect; update the test in
openwisp_controller/config/tests/test_api.py to assert the actual number of
versions for that specific revision (e.g., assert len(response_json) == 1 or
better: compute expected_count =
ModelVersion.objects.filter(revision_id=revision_id).count() and assert
equality), and if failures persist also fix RevisionListView.get_queryset to
apply the 'revision_id' GET filter (ensure get_queryset checks
request.GET.get("revision_id") and filters the queryset by revision_id before
returning).
🧹 Nitpick comments (2)
openwisp_controller/pki/tests/test_api.py (1)
26-28: Consider addingContentType.objects.clear_cache()for test stability.The geo tests added
ContentType.objects.clear_cache()insetUpto prevent inconsistent query counts due to django-reversion's ContentType caching. This test class may benefit from the same approach to avoid potential flakiness.🔎 Suggested fix
+from django.contrib.contenttypes.models import ContentType + class TestPkiApi( AssertNumQueriesSubTestMixin, TestAdminMixin, TestPkiMixin, TestOrganizationMixin, AuthenticationMixin, TestCase, ): def setUp(self): super().setUp() self._login() + ContentType.objects.clear_cache()openwisp_controller/connection/tests/test_api.py (1)
497-497: AddContentType.objects.clear_cache()tosetUpmethod for test stability.The connection test's
setUpmethod should includeContentType.objects.clear_cache()to match the pattern established in geo tests (line 304 ofopenwisp_controller/geo/tests/test_api.py). This helps prevent cache-related query count inconsistencies across test runs.Also applies to: 542-542, 556-556
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
openwisp_controller/config/api/serializers.pyopenwisp_controller/config/api/urls.pyopenwisp_controller/config/api/views.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/mixins.pyopenwisp_controller/pki/api/views.pyopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/api/views.py
🧰 Additional context used
🧬 Code graph analysis (10)
openwisp_controller/mixins.py (1)
openwisp_controller/config/controller/views.py (1)
dispatch(71-72)
openwisp_controller/connection/tests/test_api.py (1)
openwisp_controller/config/tests/utils.py (1)
assertNumQueries(384-388)
openwisp_controller/connection/api/views.py (1)
openwisp_controller/mixins.py (1)
AutoRevisionMixin(43-62)
openwisp_controller/geo/tests/test_api.py (1)
openwisp_controller/config/tests/utils.py (1)
assertNumQueries(384-388)
openwisp_controller/pki/tests/test_api.py (1)
openwisp_controller/config/tests/utils.py (1)
assertNumQueries(384-388)
openwisp_controller/geo/api/views.py (1)
openwisp_controller/mixins.py (3)
AutoRevisionMixin(43-62)ProtectedAPIMixin(39-40)RelatedDeviceProtectedAPIMixin(30-36)
tests/openwisp2/sample_config/api/views.py (1)
openwisp_controller/config/api/views.py (3)
RevisionListView(306-318)RevisionRestoreView(332-357)VersionDetailView(321-329)
openwisp_controller/config/api/views.py (2)
openwisp_controller/mixins.py (1)
AutoRevisionMixin(43-62)openwisp_controller/config/api/serializers.py (1)
VersionSerializer(382-404)
openwisp_controller/config/tests/test_api.py (1)
openwisp_controller/config/api/views.py (3)
post(141-147)post(154-160)post(342-357)
openwisp_controller/pki/api/views.py (1)
openwisp_controller/mixins.py (2)
AutoRevisionMixin(43-62)ProtectedAPIMixin(39-40)
🪛 Ruff (0.14.10)
openwisp_controller/config/api/serializers.py
392-404: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/config/api/views.py
61-61: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
76-76: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
169-169: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
342-342: Unused method argument: args
(ARG002)
🔇 Additional comments (27)
openwisp_controller/geo/tests/test_api.py (2)
304-304: Good practice: Clearing ContentType cache for consistent query counts.This addresses the test flakiness caused by Django Reversion caching ContentType lookups, which was causing inconsistent SQL query counts across test runs. This is the correct approach to stabilize the
assertNumQueriesassertions.
499-499: Query count adjustments for revision tracking overhead.The increased query counts across these tests reflect the additional database operations introduced by
AutoRevisionMixinfor creating revisions on POST, PUT, and PATCH requests. These increases are expected and consistent with the revision-tracking feature.Also applies to: 530-530, 541-541, 571-571, 582-582, 608-608, 632-632, 727-727, 765-765, 832-832, 872-872, 895-895, 914-914, 931-931, 945-945
openwisp_controller/pki/tests/test_api.py (1)
54-54: Query count adjustments are consistent with revision tracking.The increased query counts for CA and certificate mutation operations (POST, PUT, PATCH, renew, revoke) correctly reflect the additional database operations from
AutoRevisionMixin.Also applies to: 64-64, 80-80, 100-100, 127-127, 140-140, 164-164, 175-175, 192-192, 208-208, 224-224, 256-256, 267-267, 292-292, 303-303
tests/openwisp2/sample_config/api/views.py (2)
108-117: LGTM!The new revision-related view classes follow the established pattern in this sample config module, providing extension points for customization while delegating to the base implementations.
134-136: LGTM!The view bindings are correctly exposed following the same pattern as other views in the module.
openwisp_controller/mixins.py (2)
43-62: Well-implemented revision tracking mixin.The implementation correctly:
- Limits revision creation to POST, PUT, PATCH (excluding DELETE per review feedback)
- Checks
reversion.is_registered(model)to avoid empty revisions for unregistered models- Uses
revision_atomic = Falseto prevent transaction nesting issues withpost_savehandlers- Sets user and comment metadata within the revision context
The comment format
"API request: {method} {path}"provides useful debugging context for support scenarios.
47-48: The defensive handling is already in place and works correctly.The code safely handles views that don't define a
querysetattribute (e.g.,DeviceConnenctionListCreateViewandDeviceConnectionDetailViewwhich rely onget_queryset()only). Whenquerysetis undefined, bothqsandmodelbecomeNone, and the condition on line 52 (and model) prevents the revision block from executing—this is safe and acceptable behavior. However, consider whether revision tracking should be added to these connection detail views by explicitly defining theirquerysetattributes, as other views in the codebase do.openwisp_controller/config/api/serializers.py (1)
382-404: VersionSerializer structure is well-designed.The serializer correctly exposes revision metadata through the
revisionrelation using thesourceparameter, which is cleaner than usingSerializerMethodFieldas suggested in past reviews.Note: The static analysis hint about
ClassVarfor thefieldslist is a false positive — Django REST Framework'sMeta.fieldsis intentionally a mutable class attribute following DRF conventions.openwisp_controller/config/tests/test_api.py (2)
1-1: LGTM!Import added correctly to support revision-wrapped device creation in tests.
1676-1683: LGTM!The restore test correctly verifies that the device is restored to its original name after calling the restore endpoint.
openwisp_controller/geo/api/views.py (4)
17-21: LGTM!Import correctly updated to include
AutoRevisionMixinfor revision tracking.
64-66: LGTM!
AutoRevisionMixincorrectly added toDeviceCoordinatesView. The mixin will safely skip revision creation if the model is not registered with reversion.
112-117: LGTM!
AutoRevisionMixincorrectly added toDeviceLocationView.
213-215: LGTM!
AutoRevisionMixinconsistently added to all location and floorplan views following the same pattern.Also applies to: 223-227, 232-234, 242-246
openwisp_controller/connection/api/views.py (4)
16-21: LGTM!Import correctly updated to include
AutoRevisionMixin.
65-66: LGTM!
AutoRevisionMixincorrectly added toCommandListCreateView.
85-95: LGTM!
AutoRevisionMixincorrectly added to credential views.
125-141: LGTM!
AutoRevisionMixincorrectly added to device connection views.openwisp_controller/config/api/views.py (6)
1-7: LGTM!Imports correctly added for reversion support, transaction handling, and Version model.
57-80: LGTM!
AutoRevisionMixincorrectly added to template and VPN views.
93-160: LGTM!
AutoRevisionMixincorrectly added to device-related views including list, detail, activate, and deactivate views.
163-189: LGTM!
AutoRevisionMixincorrectly added to device group views.
321-329: Consider adding organization-based permission checks.The
VersionDetailViewcurrently only filters by model type. Any authenticated user with API access can view version history of any object across all organizations. Depending on security requirements, you may want to add organization-based filtering similar to other protected views.
371-373: LGTM!View instances correctly defined for the new revision endpoints.
openwisp_controller/pki/api/views.py (3)
13-13: LGTM!Import correctly updated to include
AutoRevisionMixin.
33-56: LGTM!
AutoRevisionMixincorrectly added to CA views including list, detail, and renew operations.
69-84: LGTM!
AutoRevisionMixincorrectly added to certificate views. TheCertRevokeRenewBaseViewbase class ensures derived views (CertRevokeView,CertRenewView) also get revision tracking.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@dee077 there's conflicts with master
dee077
force-pushed
the
feature/894-rest-api-revisions
branch
from
27e70d9 to
8edd817
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openwisp_controller/connection/api/views.py`:
- Around line 136-145: The code in assert_parent_exists (called from initial)
references ValidationError and NotFound but they are not imported; add the
missing imports at the top of the module: import ValidationError from
django.core.exceptions and NotFound from rest_framework.exceptions so that the
exceptions used in assert_parent_exists and the NotFound raised for device_id
are defined.
In `@openwisp_controller/connection/tests/test_api.py`:
- Line 564: The POST deviceconnection query count rose due to repeated
ContentType and reversion work; update AutoRevisionMixin to cache ContentType
lookups (use ContentType.objects.get_for_model(model, for_concrete_model=False)
with a simple per-process or per-request dict on the mixin/class to avoid
repeated DB hits) and use that cached value when creating revisions, and also
ensure any queryset used by the device connection create path (the viewset or
DeviceConnection queryset) uses select_related for the FK fields involved in
revision creation so the revision logic doesn't trigger extra queries; remove
test reliance on ContentType.objects.clear_cache() once caching is in place.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
openwisp_controller/config/api/serializers.pyopenwisp_controller/config/api/urls.pyopenwisp_controller/config/api/views.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/mixins.pyopenwisp_controller/pki/api/views.pyopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/api/views.py
🚧 Files skipped from review as they are similar to previous changes (2)
- openwisp_controller/config/api/urls.py
- openwisp_controller/mixins.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/tests/test_api.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/pki/api/views.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/views.pyopenwisp_controller/connection/api/views.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tests/test_api.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/sample_config/api/views.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/pki/api/views.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/views.pyopenwisp_controller/connection/api/views.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/geo/tests/test_api.py
🧬 Code graph analysis (10)
openwisp_controller/connection/tests/test_api.py (1)
openwisp_controller/config/tests/utils.py (1)
assertNumQueries(390-394)
openwisp_controller/geo/tests/test_api.py (1)
openwisp_controller/config/tests/utils.py (1)
assertNumQueries(390-394)
tests/openwisp2/sample_config/api/views.py (1)
openwisp_controller/config/api/views.py (5)
RevisionListView(306-318)RevisionRestoreView(332-357)TemplateDetailView(65-69)TemplateListCreateView(57-62)VersionDetailView(321-329)
openwisp_controller/geo/api/views.py (1)
openwisp_controller/mixins.py (2)
AutoRevisionMixin(41-60)ProtectedAPIMixin(37-38)
openwisp_controller/config/tests/test_api.py (5)
openwisp_controller/geo/base/models.py (1)
device(60-61)openwisp_controller/config/tests/utils.py (1)
_create_device(31-44)openwisp_controller/config/base/base.py (1)
json(277-284)openwisp_controller/config/api/views.py (3)
post(141-147)post(154-160)post(342-357)openwisp_controller/config/models.py (1)
Device(15-22)
openwisp_controller/config/api/serializers.py (1)
openwisp_controller/serializers.py (1)
BaseSerializer(5-12)
openwisp_controller/pki/api/views.py (1)
openwisp_controller/mixins.py (2)
AutoRevisionMixin(41-60)ProtectedAPIMixin(37-38)
openwisp_controller/pki/tests/test_api.py (1)
openwisp_controller/config/tests/utils.py (1)
assertNumQueries(390-394)
openwisp_controller/config/api/views.py (2)
openwisp_controller/mixins.py (1)
AutoRevisionMixin(41-60)openwisp_controller/config/api/serializers.py (1)
VersionSerializer(382-404)
openwisp_controller/connection/api/views.py (1)
openwisp_controller/mixins.py (2)
AutoRevisionMixin(41-60)ProtectedAPIMixin(37-38)
🪛 Ruff (0.15.0)
openwisp_controller/config/api/serializers.py
[warning] 392-404: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_controller/config/api/views.py
[warning] 61-61: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 76-76: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 169-169: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 342-342: Unused method argument: args
(ARG002)
openwisp_controller/connection/api/views.py
[error] 143-143: Undefined name ValidationError
(F821)
[warning] 145-145: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[error] 145-145: Undefined name NotFound
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | 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.11 | django~=4.2.0
🔇 Additional comments (13)
openwisp_controller/pki/tests/test_api.py (2)
50-55: CA query‑count updates look consistent with revision overhead.
Thanks for aligning the expected query budgets for CA create/update/renew paths with the new behavior.Also applies to: 59-66, 70-83, 91-102, 122-129, 134-142, 160-166
171-177: Cert query‑count updates look consistent with revision overhead.
The new expectations for cert create/update/renew/revoke flows look in line with the added revision work.Also applies to: 180-196, 203-210, 214-226, 247-258, 263-269, 288-294, 299-305
openwisp_controller/geo/api/views.py (1)
18-23: AutoRevisionMixin wiring across geo views looks consistent.
No concerns with the added mixin inheritance.Also applies to: 98-101, 146-151, 299-301, 309-313, 318-320, 328-332
openwisp_controller/geo/tests/test_api.py (3)
401-405: ContentType cache reset in setUp is a good stabilization step.
This should keep the query‑count assertions deterministic across runs.
588-600: Location/FloorPlan query‑budget updates look aligned.
Thanks for updating the expected query counts to reflect the revision-aware flow.Also applies to: 620-634, 638-645, 668-675, 679-686, 696-712, 716-736
824-840: DeviceLocation query‑budget updates look aligned.
The revised expectations here are consistent with the rest of the revision-enabled tests.Also applies to: 852-871, 920-936, 961-978, 983-1001, 1006-1021, 1022-1038, 1039-1051
openwisp_controller/config/api/views.py (1)
57-190: AutoRevisionMixin integration across config views looks good.
No concerns with the updated inheritance.tests/openwisp2/sample_config/api/views.py (1)
31-36: Revision view pass‑throughs are wired correctly.
No issues with the sample config wrappers and view bindings.Also applies to: 108-117, 134-136
openwisp_controller/connection/api/views.py (1)
66-72: AutoRevisionMixin + parent‑existence guard integration looks good.
The added mixin wiring and existence checks are straightforward.Also applies to: 102-110, 136-145, 151-167
openwisp_controller/connection/tests/test_api.py (1)
609-609: Consistent +9 query overhead for PUT/PATCH operations.Same reversion overhead pattern as the POST test. No additional concerns beyond the comment above.
Also applies to: 623-623
openwisp_controller/pki/api/views.py (3)
33-33: AutoRevisionMixin consistently applied to all write-capable views.MRO ordering is correct (
ProtectedAPIMixin→AutoRevisionMixin→ DRF view), andCrlDownloadViewis appropriately excluded since it's read-only. The mixin's guard (reversion.is_registered(model)) ensures safe behavior even ifCa/Certmodels aren't registered with reversion.Also applies to: 39-39, 44-44, 69-69, 75-77, 82-82
13-13: Import looks good.Clean addition of
AutoRevisionMixinalongside existingProtectedAPIMixinimport.
44-55: No action needed — Ca and Cert are properly registered with django-reversion.Both
CaAdminandCertAdmininherit fromVersionAdmin, which automatically registers the models with django-reversion. TheAutoRevisionMixinwill successfully create revisions for renew/revoke operations on these models.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "failure_reason": "", | ||
| } | ||
| with self.assertNumQueries(12): | ||
| with self.assertNumQueries(23): |
There was a problem hiding this comment.
Query count nearly doubled (12 → 23) for POST deviceconnection.
The +11 query increase from reversion tracking is substantial. While the PR discussion acknowledges increased queries from ContentType lookups and reversion operations, this level of overhead on every write API call could impact performance under load.
Consider whether select_related or caching of ContentType lookups within AutoRevisionMixin could reduce this overhead. The PR comments mention ContentType.objects.clear_cache() was needed in tests, which suggests ContentType caching does help in production — worth verifying that it's effective at reducing repeated lookups in real requests.
🤖 Prompt for AI Agents
In `@openwisp_controller/connection/tests/test_api.py` at line 564, The POST
deviceconnection query count rose due to repeated ContentType and reversion
work; update AutoRevisionMixin to cache ContentType lookups (use
ContentType.objects.get_for_model(model, for_concrete_model=False) with a simple
per-process or per-request dict on the mixin/class to avoid repeated DB hits)
and use that cached value when creating revisions, and also ensure any queryset
used by the device connection create path (the viewset or DeviceConnection
queryset) uses select_related for the FK fields involved in revision creation so
the revision logic doesn't trigger extra queries; remove test reliance on
ContentType.objects.clear_cache() once caching is in place.
Implemented three endpoints: 1. List all revisions with optional filtering by model. 2. Inspect a specific revision by its ID. 3. Restore a revision using its ID. Fixes openwisp#894
dee077
force-pushed
the
feature/894-rest-api-revisions
branch
from
8edd817 to
6d602b5
Compare
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Changes Since Last Review
Files Reviewed (13 files)
Fix these issues in Kilo Cloud Reviewed by kimi-k2.5-0127 · 1,258,608 tokens |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_controller/pki/tests/test_api.py (1)
26-28:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
ContentType.objects.clear_cache()insetUp— same flakiness risk as geo tests.
TestGeoApi.setUpaddedContentType.objects.clear_cache()(seegeo/tests/test_api.pyline 407) specifically because reversion cachesContentTypelookups in memory across tests, causing non-deterministic query counts depending on test execution order.TestPkiApiusesAssertNumQueriesSubTestMixinwith tightly specified counts and is equally susceptible.💡 Proposed fix
+from django.contrib.contenttypes.models import ContentType ... class TestPkiApi(...): def setUp(self): super().setUp() self._login() + ContentType.objects.clear_cache()🤖 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 `@openwisp_controller/pki/tests/test_api.py` around lines 26 - 28, The TestPkiApi setUp currently calls super().setUp() and self._login() but misses clearing ContentType cache, which can cause flaky query-count assertions; update TestPkiApi.setUp to call ContentType.objects.clear_cache() (via the django ContentType manager) before executing any login or test setup logic so reversion's cached ContentType lookups won't affect AssertNumQueriesSubTestMixin assertions.openwisp_controller/connection/tests/test_api.py (1)
430-432:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
ContentType.objects.clear_cache()insetUp— same risk as geo/PKI tests.Reversion caches
ContentTypelookups in memory. If any earlier test in the suite has already warmed theDeviceConnectionContentType, theassertNumQueries(23)/assertNumQueries(22)assertions may see 1–2 fewer queries and fail non-deterministically. The pattern established inTestGeoApi.setUp(ContentType.objects.clear_cache()) should be applied here too.💡 Proposed fix
+from django.contrib.contenttypes.models import ContentType ... class TestConnectionApi(...): def setUp(self): super().setUp() self._login() + ContentType.objects.clear_cache()🤖 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 `@openwisp_controller/connection/tests/test_api.py` around lines 430 - 432, In the setUp method add a call to ContentType.objects.clear_cache() before self._login() to avoid stale ContentType lookups affecting query-count assertions; update the setUp function (the setUp method in the test class in openwisp_controller/connection/tests/test_api.py) to call ContentType.objects.clear_cache() first, then proceed with self._login(), mirroring the pattern used in TestGeoApi to ensure deterministic assertNumQueries checks for DeviceConnection content type.
♻️ Duplicate comments (2)
openwisp_controller/connection/api/views.py (1)
140-145:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
ValidationErrorandNotFoundare not imported —NameErrorat runtime.
assert_parent_exists()catchesValidationError(not imported) and raisesNotFound(not imported). When a non-existent or invalid-UUIDdevice_idis passed, theexceptclause evaluation raises aNameErrorinstead of a clean 404, resulting in a 500 response.This was flagged in a previous review and remains unresolved.
🔧 Proposed fix
+from django.core.exceptions import ValidationError from django.utils.translation import gettext_lazy as _+from rest_framework.exceptions import NotFound from rest_framework.generics import (🤖 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 `@openwisp_controller/connection/api/views.py` around lines 140 - 145, The assert_parent_exists method catches ValidationError and raises NotFound but neither exception is imported, causing a NameError at runtime; import ValidationError (from django.core.exceptions) and NotFound (from rest_framework.exceptions) and update the module imports so assert_parent_exists can reference ValidationError and NotFound without errors — locate the assert_parent_exists function and add the two imports at the top of the file to restore correct 404 behavior.openwisp_controller/config/api/views.py (1)
308-349:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftScope reversion queries to supported, authorized controller objects.
These endpoints currently filter
Versionrows only byContentType(model=...). That leaves two holes: the slug is not uniquely tied to one app, and the queryset is never intersected with the caller’s accessible devices/templates/vpns/device groups. A user who passes the generic permission gate can therefore enumerate, inspect, or restore revisions for every object of that content type. Resolvemodelthrough an explicit controller-model allowlist and then restrictVersion.object_idwith the same scoped queryset the corresponding resource API uses.🤖 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 `@openwisp_controller/config/api/views.py` around lines 308 - 349, BaseReversionView.get_queryset currently filters Versions only by ContentType(model=...) which allows model name collision and unauthorized access; change it to first validate the requested model against an explicit controller-model allowlist (resolve the model string to a safe Model class) and then build the same scoped queryset used by that resource's API (e.g., the devices/templates/vpns/device groups list view logic) to get only objects the caller is authorized to see; finally filter Version by content_type and by object_id__in=scoped_qs.values_list('pk', flat=True) so ReversionListView, ReversionDetailView and ReversionRestoreView operate only on allowed, scoped objects and retain ProtectedAPIMixin checks.
🤖 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 `@openwisp_controller/config/api/views.py`:
- Around line 336-345: The POST handler currently uses get_object_or_404 on qs
with revision_id=kwargs["pk"] which fails for grouped-change revisions and
returns versions from the old revision; change post to load the Revision model
instance directly by id (use kwargs["pk"]) instead of calling .get() on the
Version queryset, then confirm the loaded Revision contains at least one Version
that belongs to the allowed queryset from get_queryset() (to validate
permissions), call revision.revert() inside the with reversion.create_revision()
transaction, set user/comment as before, capture the new restore Revision object
created by reversion (the new revision created by revert) and use that new
Revision to filter versions (replace versions = qs.filter(revision=revision)
with versions = qs.filter(revision=new_revision)) so grouped-change revisions
and returned data are correct.
In `@openwisp_controller/connection/api/views.py`:
- Around line 156-162: The get_queryset override in
DeviceConnenctionListCreateView is redundant because
BaseDeviceConnection.get_queryset already applies
.filter(device_id=self.kwargs["device_id"]) and .order_by("-created"); remove
the DeviceConnenctionListCreateView.get_queryset method entirely (or replace its
body with a simple return super().get_queryset() if you prefer) so the class
relies on BaseDeviceConnection.get_queryset to avoid duplicate
filtering/ordering.
In `@openwisp_controller/mixins.py`:
- Around line 41-42: AutoRevisionMixin currently subclasses RevisionMixin
causing redundant create_revision contexts and unnecessary wrapping on all
requests; change AutoRevisionMixin to stop inheriting RevisionMixin and
implement its own dispatch logic that only opens a single create_revision
context when needed (e.g., for write methods and registered models), call
set_user/set_comment once inside that context, and ensure revision_atomic
remains available on the mixin (keep the revision_atomic attribute and any
helper methods used by dispatch); remove or avoid calling super().dispatch()
from RevisionMixin so you don't get the second nested create_revision.
- Around line 44-60: The dispatch() revision logic currently checks
request.user.is_authenticated on the raw Django HttpRequest (so
token-authenticated users are skipped); remove the upfront
request.user.is_authenticated check and instead: keep the method check
(POST/PUT/PATCH), model and reversion.is_registered(model) guards, wrap
super().dispatch(request, *args, **kwargs) inside the
reversion.create_revision(...) block, then after calling super() use
self.request.user (the DRF-authenticated user) when calling
reversion.set_user(...) and reversion.set_comment(...); reference the dispatch
method in this mixin (and RevisionMixin.dispatch if present) and ensure you only
call reversion.set_user when getattr(self, "request", None) yields a user that
.is_authenticated.
---
Outside diff comments:
In `@openwisp_controller/connection/tests/test_api.py`:
- Around line 430-432: In the setUp method add a call to
ContentType.objects.clear_cache() before self._login() to avoid stale
ContentType lookups affecting query-count assertions; update the setUp function
(the setUp method in the test class in
openwisp_controller/connection/tests/test_api.py) to call
ContentType.objects.clear_cache() first, then proceed with self._login(),
mirroring the pattern used in TestGeoApi to ensure deterministic
assertNumQueries checks for DeviceConnection content type.
In `@openwisp_controller/pki/tests/test_api.py`:
- Around line 26-28: The TestPkiApi setUp currently calls super().setUp() and
self._login() but misses clearing ContentType cache, which can cause flaky
query-count assertions; update TestPkiApi.setUp to call
ContentType.objects.clear_cache() (via the django ContentType manager) before
executing any login or test setup logic so reversion's cached ContentType
lookups won't affect AssertNumQueriesSubTestMixin assertions.
---
Duplicate comments:
In `@openwisp_controller/config/api/views.py`:
- Around line 308-349: BaseReversionView.get_queryset currently filters Versions
only by ContentType(model=...) which allows model name collision and
unauthorized access; change it to first validate the requested model against an
explicit controller-model allowlist (resolve the model string to a safe Model
class) and then build the same scoped queryset used by that resource's API
(e.g., the devices/templates/vpns/device groups list view logic) to get only
objects the caller is authorized to see; finally filter Version by content_type
and by object_id__in=scoped_qs.values_list('pk', flat=True) so
ReversionListView, ReversionDetailView and ReversionRestoreView operate only on
allowed, scoped objects and retain ProtectedAPIMixin checks.
In `@openwisp_controller/connection/api/views.py`:
- Around line 140-145: The assert_parent_exists method catches ValidationError
and raises NotFound but neither exception is imported, causing a NameError at
runtime; import ValidationError (from django.core.exceptions) and NotFound (from
rest_framework.exceptions) and update the module imports so assert_parent_exists
can reference ValidationError and NotFound without errors — locate the
assert_parent_exists function and add the two imports at the top of the file to
restore correct 404 behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1e07db45-2719-42e4-9c29-5cda009b8253
📒 Files selected for processing (13)
openwisp_controller/config/api/filters.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/api/urls.pyopenwisp_controller/config/api/views.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/mixins.pyopenwisp_controller/pki/api/views.pyopenwisp_controller/pki/tests/test_api.pytests/openwisp2/sample_config/api/views.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/pki/api/views.pyopenwisp_controller/config/api/serializers.pytests/openwisp2/sample_config/api/views.pyopenwisp_controller/config/api/filters.pyopenwisp_controller/mixins.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/urls.pyopenwisp_controller/config/api/views.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/pki/api/views.pyopenwisp_controller/config/api/serializers.pytests/openwisp2/sample_config/api/views.pyopenwisp_controller/config/api/filters.pyopenwisp_controller/mixins.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/urls.pyopenwisp_controller/config/api/views.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 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_controller/pki/api/views.pyopenwisp_controller/config/api/serializers.pytests/openwisp2/sample_config/api/views.pyopenwisp_controller/config/api/filters.pyopenwisp_controller/mixins.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/urls.pyopenwisp_controller/config/api/views.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/pki/api/views.pyopenwisp_controller/config/api/serializers.pytests/openwisp2/sample_config/api/views.pyopenwisp_controller/config/api/filters.pyopenwisp_controller/mixins.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/urls.pyopenwisp_controller/config/api/views.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/pki/api/views.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/api/filters.pyopenwisp_controller/mixins.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/urls.pyopenwisp_controller/config/api/views.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/pki/api/views.pyopenwisp_controller/config/api/serializers.pyopenwisp_controller/config/api/filters.pyopenwisp_controller/mixins.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/urls.pyopenwisp_controller/config/api/views.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/pki/api/views.pyopenwisp_controller/config/api/serializers.pytests/openwisp2/sample_config/api/views.pyopenwisp_controller/config/api/filters.pyopenwisp_controller/mixins.pyopenwisp_controller/geo/api/views.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/urls.pyopenwisp_controller/config/api/views.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.py
🔇 Additional comments (5)
openwisp_controller/config/api/filters.py (1)
148-189:ReversionListFilterlooks correct.Filter definitions, label assignment, and
Metaconfiguration are consistent with the rest of the file. No issues.openwisp_controller/geo/api/views.py (1)
113-358:AutoRevisionMixincorrectly placed in all affected geo API views.Addition of
AutoRevisionMixinis consistent acrossDeviceCoordinatesView,DeviceLocationView,FloorPlanListCreateView,FloorPlanDetailView,LocationListCreateView, andLocationDetailView. MRO is sensible in all cases.openwisp_controller/geo/tests/test_api.py (1)
404-407:ContentType.objects.clear_cache()insetUpis the correct fix for reversion-related query count flakiness.openwisp_controller/pki/api/views.py (1)
33-82:AutoRevisionMixinapplied correctly across all PKI write views;CrlDownloadView(read-only) correctly excluded.openwisp_controller/mixins.py (1)
57-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winf-string uses double quotes — project convention requires single quotes.
Line 57:
f"API request: {request.method} {request.get_full_path()}"should use single quotes. As per coding guidelines and past review feedback, single-quote strings are used consistently throughout this repository.💡 Proposed fix
- reversion.set_comment( - f"API request: {request.method} {request.get_full_path()}" - ) + reversion.set_comment( + f'API request: {request.method} {request.get_full_path()}' + )> Likely an incorrect or invalid review comment.
Question
Pending
|
Multiple Flake8 Failures and Test FailuresHello @dee077, There are multiple issues that need to be addressed:
Please fix these unused imports and undefined names manually.
Please address these issues and push a new commit. |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@dee077 why are the queries almost doubling?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@openwisp_controller/config/api/views.py`:
- Around line 308-312: The BaseReversionView currently uses
ProtectedAPIMixin/FilterByOrganizationManaged which filters by organization__in
but reversion.models.Version has no organization field; override get_queryset on
BaseReversionView (and any ReversionList/ReversionDetail/ReversionRestore views
that inherit it) to perform access control by resolving the ContentType and then
restricting Versions to only those whose (object_id, content_type) refer to
model instances the current user may access (for example by querying the target
model for pk__in=version_ids & applying the existing organization/user
permission filter used for that model), or alternatively implement a custom
organization_field traversal that maps Version -> content_object.organization
before filtering; ensure you use the Version model and
self.kwargs["model"]/content_type to build the filtered queryset rather than
delegating to FilterByOrganizationManaged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 392716f5-17c4-460c-aeb5-50a84085626c
📒 Files selected for processing (6)
openwisp_controller/config/api/views.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/mixins.pyopenwisp_controller/pki/tests/test_api.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/tests/test_api.pyopenwisp_controller/mixins.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/tests/test_api.pyopenwisp_controller/mixins.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 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_controller/connection/tests/test_api.pyopenwisp_controller/mixins.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/views.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/tests/test_api.pyopenwisp_controller/mixins.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/views.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/tests/test_api.pyopenwisp_controller/mixins.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/views.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tests/test_api.pyopenwisp_controller/mixins.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/views.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tests/test_api.pyopenwisp_controller/mixins.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/pki/tests/test_api.pyopenwisp_controller/config/api/views.py
🔇 Additional comments (6)
openwisp_controller/config/api/views.py (1)
336-349: Two unresolved issues from the previous review remain.1.
MultipleObjectsReturnedon line 338:
get_object_or_404(qs, revision_id=kwargs["pk"])calls.get()under the hood. If a single revision contains multipleVersionrows for the samecontent_type(e.g. a bulk operation, orfollow=["device"]tracking creating versions for several objects of the same type), this raisesMultipleObjectsReturned→ HTTP 500.2. Response returns stale revision on line 345:
qs.filter(revision=revision)filters on the original revision object (before revert). Afterrevision.revert()runs insidecreate_revision(), a brand-new restore revision is committed. The response therefore returns metadata pointing at the oldrevision_id, not the newly-created restore event, which is misleading for audit purposes.openwisp_controller/connection/tests/test_api.py (1)
564-564:DeviceConnectionwrite operations gained ~10 queries vs the ~3 seen elsewhere.The elevated overhead is attributable to
reversion.register(model=DeviceConnection, follow=["device"])— reversion also versions the relatedDeviceon eachDeviceConnectionsave, doubling the ContentType lookups and Version/Revision inserts. This was noted in the prior review; raising it here for tracking as it is still unresolved.Also applies to: 609-609, 623-623
openwisp_controller/mixins.py (1)
40-59:AutoRevisionMixinimplementation looks correct.Both previously-raised concerns are resolved:
- The
RevisionMixininheritance that caused double-wrapping and unnecessary context overhead is removed.self.request.user(the DRF-authenticated user) is used aftersuper().dispatch()returns, so Bearer/token-authenticated users are now correctly captured.openwisp_controller/connection/api/views.py (1)
66-66:AutoRevisionMixinwiring looks correct.The previously-raised issues are resolved: the missing
ValidationError/NotFoundimports are gone, and the redundantget_querysetoverride inDeviceConnenctionListCreateViewwas removed. The mixin silently no-ops forCredentialsandCommand(not registered with reversion), which is the intended behavior.Also applies to: 102-113, 141-149
openwisp_controller/geo/tests/test_api.py (1)
404-408:ContentType.objects.clear_cache()insetUpis a correct and necessary fix.Calling this before each test ensures the in-memory ContentType cache doesn't carry state from previous tests that would make
assertNumQueriescounts non-deterministic when reversion performs ContentType lookups.openwisp_controller/pki/tests/test_api.py (1)
54-303: Query count adjustments are consistent and expected.Every write operation (POST/PUT/PATCH) on registered
Ca/Certmodels gains exactly +3 queries (ContentType lookup,reversion_version,reversion_revisioninserts). GET and DELETE operations are unchanged, confirmingAutoRevisionMixinonly triggers on the correct HTTP methods.
It’s not due to nested revision contexts So when That triggers additional ContentType lookups, related object fetches, PK collection When DeviceLocation is updated, reversion also versions This triggers additional ContentType lookups, related object fetches, PK collection, serialization of each object, and finally one |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
openwisp_controller/config/tests/test_api.py (1)
1806-1811: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winStrengthen the
revision_idfilter assertion.The assertion only checks the count (
== 1); it does not verify that the returned version actually belongs torevision_id. IfReversionListFilterever silently no-ops (as flagged in earlier reviews), this assertion would still pass with the wrong row. Add a per-result check onrevision_id.♻️ Suggested tightening
self.assertEqual(response.status_code, 200) self.assertEqual(len(response_json["results"]), 1) + for version in response_json["results"]: + self.assertEqual(version["revision_id"], revision_id)🤖 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 `@openwisp_controller/config/tests/test_api.py` around lines 1806 - 1811, The test currently only asserts the count of results for the ReversionList filter; update the subTest (the block using reverse("config_api:reversion_list", args=[model_slug]) and response_json) to also verify that each returned item actually belongs to the requested revision_id by iterating over response_json["results"] and asserting each item's "revision_id" equals the local revision_id variable (or otherwise contains the expected value), so the test fails if the filter silently no-ops.
🤖 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 `@openwisp_controller/config/api/views.py`:
- Around line 344-366: The post() handler is vulnerable because calling
revision.revert() will restore all Versions in that Revision (including
out-of-scope org objects); change the logic in post to either reject Revisions
that contain any Version not present in the current queryset (qs) or iterate and
revert only the visible versions: obtain the set/list of Versions for the
Revision (Revision -> related Version objects), ensure every Version is in qs
(e.g., compare IDs) before calling any revert, and if any are out-of-scope raise
Http404/PermissionDenied; alternatively replace revision.revert() with a loop
over qs.filter(revision=revision) and call revert on each Version instance so
only in-scope objects are modified, keeping the transaction/reversion context,
set_user and set_comment behavior unchanged.
- Around line 350-361: Remove the outer transaction.atomic() wrapper around
reversion.create_revision() (create_revision already opens an atomic block).
After the create_revision context (after calling revision.revert(),
reversion.set_user(request.user), reversion.set_comment(comment)), do not rely
on get_object_or_404(Revision, user=request.user, comment=comment) — instead
deterministically fetch the revision created in the context by querying the
Revision model for the latest revision for the user, e.g. use
Revision.objects.filter(user=request.user).latest("date_created") (or latest by
PK/timestamp) and raise Http404 if none; keep the calls to revision.revert(),
reversion.set_user and reversion.set_comment unchanged.
- Around line 308-320: BaseReversionView.get_queryset currently looks up
ContentType by model only and assumes every model has an organization FK; fix it
by narrowing ContentType lookup to allowed app labels or accepting an app_label
kwarg (e.g., use app_label__in=["config","connection","geo","pki"] or use
self.kwargs.get("app_label") when present) to avoid MultipleObjectsReturned, and
harden the non-superuser branch by only handling model classes that expose an
organization relation (either check for hasattr(model_class, "organization") or
maintain a whitelist of allowed model names/classes before calling
model_class.objects.filter(organization__in=...); if the model is not allowed,
return an empty queryset or raise Http404/PermissionDenied.
- Around line 323-342: Add the authentication/permission mixin to the reversion
endpoints: update ReversionListView, ReversionDetailView, and
ReversionRestoreView to include ProtectedAPIMixin (or explicitly set appropriate
permission_classes/authentication_classes) so requests are authenticated before
BaseReversionView.get_queryset() uses self.request.user; ensure the change is
applied to the class bases for those three views (ReversionListView,
ReversionDetailView, ReversionRestoreView) so anonymous users cannot reach them.
In `@openwisp_controller/config/tests/test_api.py`:
- Around line 1820-1827: The restore sub-test is incorrectly reusing
response_json from the earlier filtered-list test; update the "Test reversion
restore view" subTest to obtain the correct revision_id explicitly (e.g., call
the unfiltered list endpoint to populate response_json or reuse the earlier
captured revision_id variable created near the first list test) before building
path via reverse("config_api:reversion_restore", args=[model_slug,
revision_id]); then post to that path and add an assertion on the restore
response body (not just status_code) and/or verify the returned JSON contains
expected keys so the test no longer depends on the filtered-list result
containing exactly one row.
- Around line 1782-1827: Add a sub-test to cover the non-superuser org-scoping
path: after the existing superuser checks, call self._login(... ) to
authenticate an org-administrator (not superuser), create two Device instances
in two different organizations, then call the config_api endpoints
(reversion_list, reversion_detail, reversion_restore) and assert that
BaseReversionView.get_queryset() behavior is enforced — only revisions/versions
belonging to the admin's managed organization are returned and attempts to
restore a revision for a device in the other org return 404; also fix the typo
"sperate" → "separate" in the test name/comment. Ensure you reference
model_slug="device", Device, and the view names
reversion_list/reversion_detail/reversion_restore when adding assertions.
---
Duplicate comments:
In `@openwisp_controller/config/tests/test_api.py`:
- Around line 1806-1811: The test currently only asserts the count of results
for the ReversionList filter; update the subTest (the block using
reverse("config_api:reversion_list", args=[model_slug]) and response_json) to
also verify that each returned item actually belongs to the requested
revision_id by iterating over response_json["results"] and asserting each item's
"revision_id" equals the local revision_id variable (or otherwise contains the
expected value), so the test fails if the filter silently no-ops.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea9fb2bb-c065-4181-b0e6-b34c666afda7
📒 Files selected for processing (2)
openwisp_controller/config/api/views.pyopenwisp_controller/config/tests/test_api.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | 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~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 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_controller/config/tests/test_api.pyopenwisp_controller/config/api/views.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/views.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/views.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/views.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_api.pyopenwisp_controller/config/api/views.py
| class BaseReversionView: | ||
| def get_queryset(self): | ||
| model = self.kwargs.get("model").lower() | ||
| content_type = get_object_or_404(ContentType, model=model) | ||
| qs = super().get_queryset().filter(content_type=content_type) | ||
| user = self.request.user | ||
| if user.is_superuser: | ||
| return qs | ||
| model_class = content_type.model_class() | ||
| allowed_object_ids = model_class.objects.filter( | ||
| organization__in=user.organizations_managed | ||
| ).values_list("pk", flat=True) | ||
| return qs.filter(object_id__in=allowed_object_ids) |
There was a problem hiding this comment.
BaseReversionView.get_queryset() has two correctness/security risks.
-
get_object_or_404(ContentType, model=model)filters only bymodelname withoutapp_label.ContentTypeis unique on(app_label, model)so two apps registering the same model name will triggerMultipleObjectsReturned(HTTP 500). Constrain the lookup to the apps you actually intend to expose (e.g.,app_label__in=["config", "connection", "geo", "pki"]) or accept anapp_labelin the URL. -
The non-superuser branch assumes every reversion-registered
model_classhas anorganizationFK. Models without that field will raiseFieldErroron access. Either restrictmodelto a known whitelist that all exposeorganization, or branch on the field's existence. Without a whitelist this is also a soft authorization bypass risk if a model with a different ownership concept ever gets routed through here.
🛡️ Suggested hardening
-class BaseReversionView:
- def get_queryset(self):
- model = self.kwargs.get("model").lower()
- content_type = get_object_or_404(ContentType, model=model)
- qs = super().get_queryset().filter(content_type=content_type)
- user = self.request.user
- if user.is_superuser:
- return qs
- model_class = content_type.model_class()
- allowed_object_ids = model_class.objects.filter(
- organization__in=user.organizations_managed
- ).values_list("pk", flat=True)
- return qs.filter(object_id__in=allowed_object_ids)
+ ALLOWED_APP_LABELS = ("config", "connection", "geo", "pki")
+
+ def get_queryset(self):
+ model = self.kwargs.get("model", "").lower()
+ content_type = get_object_or_404(
+ ContentType, app_label__in=self.ALLOWED_APP_LABELS, model=model
+ )
+ qs = super().get_queryset().filter(content_type=content_type)
+ user = self.request.user
+ if user.is_superuser:
+ return qs
+ model_class = content_type.model_class()
+ if model_class is None or not hasattr(model_class, "organization"):
+ return qs.none()
+ allowed_object_ids = model_class.objects.filter(
+ organization__in=user.organizations_managed
+ ).values_list("pk", flat=True)
+ return qs.filter(object_id__in=allowed_object_ids)🤖 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 `@openwisp_controller/config/api/views.py` around lines 308 - 320,
BaseReversionView.get_queryset currently looks up ContentType by model only and
assumes every model has an organization FK; fix it by narrowing ContentType
lookup to allowed app labels or accepting an app_label kwarg (e.g., use
app_label__in=["config","connection","geo","pki"] or use
self.kwargs.get("app_label") when present) to avoid MultipleObjectsReturned, and
harden the non-superuser branch by only handling model classes that expose an
organization relation (either check for hasattr(model_class, "organization") or
maintain a whitelist of allowed model names/classes before calling
model_class.objects.filter(organization__in=...); if the model is not allowed,
return an empty queryset or raise Http404/PermissionDenied.
| class ReversionListView(BaseReversionView, ListAPIView): | ||
| serializer_class = ReversionSerializer | ||
| queryset = Version.objects.select_related("revision").order_by( | ||
| "-revision__date_created" | ||
| ) | ||
| filter_backends = [DjangoFilterBackend] | ||
| filterset_class = ReversionListFilter | ||
| pagination_class = ListViewPagination | ||
|
|
||
|
|
||
| class ReversionDetailView(BaseReversionView, RetrieveAPIView): | ||
| serializer_class = ReversionSerializer | ||
| queryset = Version.objects.select_related("revision") | ||
|
|
||
|
|
||
| class ReversionRestoreView(BaseReversionView, GenericAPIView): | ||
| serializer_class = serializers.Serializer | ||
| queryset = Version.objects.select_related("revision").order_by( | ||
| "-revision__date_created" | ||
| ) |
There was a problem hiding this comment.
Reversion endpoints lack authentication/permission classes.
ReversionListView, ReversionDetailView, and ReversionRestoreView do not include ProtectedAPIMixin (or any permission_classes/authentication_classes). All other views in this module inherit ProtectedAPIMixin to enforce auth + DRF model permissions; without it these endpoints fall back to DRF defaults and may be reachable by unauthenticated users — and self.request.user would be AnonymousUser, breaking user.organizations_managed in BaseReversionView.get_queryset().
🔒 Proposed fix
-class ReversionListView(BaseReversionView, ListAPIView):
+class ReversionListView(ProtectedAPIMixin, BaseReversionView, ListAPIView):
serializer_class = ReversionSerializer
...
-class ReversionDetailView(BaseReversionView, RetrieveAPIView):
+class ReversionDetailView(ProtectedAPIMixin, BaseReversionView, RetrieveAPIView):
serializer_class = ReversionSerializer
...
-class ReversionRestoreView(BaseReversionView, GenericAPIView):
+class ReversionRestoreView(ProtectedAPIMixin, BaseReversionView, GenericAPIView):🤖 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 `@openwisp_controller/config/api/views.py` around lines 323 - 342, Add the
authentication/permission mixin to the reversion endpoints: update
ReversionListView, ReversionDetailView, and ReversionRestoreView to include
ProtectedAPIMixin (or explicitly set appropriate
permission_classes/authentication_classes) so requests are authenticated before
BaseReversionView.get_queryset() uses self.request.user; ensure the change is
applied to the class bases for those three views (ReversionListView,
ReversionDetailView, ReversionRestoreView) so anonymous users cannot reach them.
| def post(self, request, *args, **kwargs): | ||
| qs = self.get_queryset() | ||
| revision = get_object_or_404(Revision, pk=kwargs["pk"]) | ||
| if not qs.filter(revision=revision).exists(): | ||
| raise Http404 | ||
| comment = f"Restored to previous revision: {revision.id}" | ||
| with transaction.atomic(): | ||
| with reversion.create_revision(): | ||
| revision.revert() | ||
| reversion.set_user(request.user) | ||
| reversion.set_comment(comment) | ||
| restored_revision = get_object_or_404( | ||
| Revision.objects.order_by( | ||
| "-date_created" | ||
| ), | ||
| user=request.user, | ||
| comment=comment, | ||
| ) | ||
| versions = qs.filter(revision=restored_revision) | ||
| serializer = ReversionSerializer( | ||
| versions, many=True, context=self.get_serializer_context() | ||
| ) | ||
| return Response(serializer.data, status=status.HTTP_200_OK) |
There was a problem hiding this comment.
Cross-organization privilege escalation in revision.revert().
The qs.filter(revision=revision).exists() check only confirms that at least one Version in the revision is in the current user's scope; revision.revert() will then restore every Version in that revision, including those belonging to objects in organizations the user does not manage. A non-superuser whose revision contains both an in-scope and an out-of-scope object can mutate the out-of-scope object via this endpoint.
Either reject revisions that contain any out-of-scope versions, or revert only the versions visible in qs (for v in qs.filter(revision=revision): v.revert()).
🤖 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 `@openwisp_controller/config/api/views.py` around lines 344 - 366, The post()
handler is vulnerable because calling revision.revert() will restore all
Versions in that Revision (including out-of-scope org objects); change the logic
in post to either reject Revisions that contain any Version not present in the
current queryset (qs) or iterate and revert only the visible versions: obtain
the set/list of Versions for the Revision (Revision -> related Version objects),
ensure every Version is in qs (e.g., compare IDs) before calling any revert, and
if any are out-of-scope raise Http404/PermissionDenied; alternatively replace
revision.revert() with a loop over qs.filter(revision=revision) and call revert
on each Version instance so only in-scope objects are modified, keeping the
transaction/reversion context, set_user and set_comment behavior unchanged.
| with transaction.atomic(): | ||
| with reversion.create_revision(): | ||
| revision.revert() | ||
| reversion.set_user(request.user) | ||
| reversion.set_comment(comment) | ||
| restored_revision = get_object_or_404( | ||
| Revision.objects.order_by( | ||
| "-date_created" | ||
| ), | ||
| user=request.user, | ||
| comment=comment, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Fragile lookup of the newly-created restore revision and redundant transaction nesting.
transaction.atomic()wrappingreversion.create_revision()is redundant —create_revisionalready opens an atomic block by default (andAutoRevisionMixin.revision_atomic = Falseexists specifically to opt out of nesting). Drop the outertransaction.atomic().- Looking up the newly created
Revisionvia(user=request.user, comment=comment)is racy: two concurrent restores of the same source revision by the same user produce identical comments andget_object_or_404will raiseMultipleObjectsReturned. Capture the created revision deterministically instead, e.g. by queryingRevision.objects.filter(user=request.user).latest("date_created")immediately after thewithblock, or by switching the lookup to use the latest version inserted in the create_revision context.
♻️ Proposed refactor
- comment = f"Restored to previous revision: {revision.id}"
- with transaction.atomic():
- with reversion.create_revision():
- revision.revert()
- reversion.set_user(request.user)
- reversion.set_comment(comment)
- restored_revision = get_object_or_404(
- Revision.objects.order_by(
- "-date_created"
- ),
- user=request.user,
- comment=comment,
- )
+ comment = f"Restored to previous revision: {revision.id}"
+ with reversion.create_revision():
+ revision.revert()
+ reversion.set_user(request.user)
+ reversion.set_comment(comment)
+ restored_revision = (
+ Revision.objects.filter(user=request.user)
+ .order_by("-date_created")
+ .first()
+ )🤖 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 `@openwisp_controller/config/api/views.py` around lines 350 - 361, Remove the
outer transaction.atomic() wrapper around reversion.create_revision()
(create_revision already opens an atomic block). After the create_revision
context (after calling revision.revert(), reversion.set_user(request.user),
reversion.set_comment(comment)), do not rely on get_object_or_404(Revision,
user=request.user, comment=comment) — instead deterministically fetch the
revision created in the context by querying the Revision model for the latest
revision for the user, e.g. use
Revision.objects.filter(user=request.user).latest("date_created") (or latest by
PK/timestamp) and raise Http404 if none; keep the calls to revision.revert(),
reversion.set_user and reversion.set_comment unchanged.
| # Todo: create sperate test for each endpoint | ||
| def test_reversion_list_and_restore_api(self): | ||
| org = self._get_org() | ||
| model_slug = "device" | ||
| with reversion.create_revision(): | ||
| device = self._create_device( | ||
| organization=org, | ||
| name="test", | ||
| ) | ||
| path = reverse("config_api:device_detail", args=[device.pk]) | ||
| data = dict(name="change-test-device") | ||
| response = self.client.patch(path, data, content_type="application/json") | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual(response.data["name"], "change-test-device") | ||
|
|
||
| with self.subTest("Test reversion list"): | ||
| path = reverse("config_api:reversion_list", args=[model_slug]) | ||
| response = self.client.get(path) | ||
| response_json = response.json() | ||
| version_id = response_json["results"][1]["id"] | ||
| revision_id = response_json["results"][1]["revision_id"] | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual(len(response_json["results"]), 2) | ||
|
|
||
| with self.subTest("Test revision list filter by revision id"): | ||
| path = reverse("config_api:reversion_list", args=[model_slug]) | ||
| response = self.client.get(f"{path}?revision_id={revision_id}") | ||
| response_json = response.json() | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual(len(response_json["results"]), 1) | ||
|
|
||
| with self.subTest("Test reversion detail"): | ||
| path = reverse("config_api:reversion_detail", args=[model_slug, version_id]) | ||
| response = self.client.get(path) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual(response.json()["id"], version_id) | ||
| self.assertEqual(response.json()["object_id"], str(device.pk)) | ||
|
|
||
| with self.subTest("Test reversion restore view"): | ||
| revision_id = response_json["results"][0]["revision_id"] | ||
| path = reverse( | ||
| "config_api:reversion_restore", args=[model_slug, revision_id] | ||
| ) | ||
| response = self.client.post(path) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual(Device.objects.get(name="test").pk, device.pk) |
There was a problem hiding this comment.
Test only covers a superuser; non-superuser org-scoping path is unverified.
self._login() authenticates an admin/superuser, which short-circuits BaseReversionView.get_queryset() at the is_superuser branch. The new non-superuser branch (the organizations_managed filter using model_class.objects.filter(organization__in=...)) — which is the security-critical path — has no coverage. Please add a sub-test that:
- Logs in as an org-administrator (non-superuser).
- Creates devices in two different organizations.
- Asserts only devices/versions from the user's managed org are returned by
reversion_list/reversion_detail, and thatreversion_restoredenies access (404) to a revision whose target object lives in another organization.
Also: the typo "sperate" → "separate" on line 1782.
🤖 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 `@openwisp_controller/config/tests/test_api.py` around lines 1782 - 1827, Add a
sub-test to cover the non-superuser org-scoping path: after the existing
superuser checks, call self._login(... ) to authenticate an org-administrator
(not superuser), create two Device instances in two different organizations,
then call the config_api endpoints (reversion_list, reversion_detail,
reversion_restore) and assert that BaseReversionView.get_queryset() behavior is
enforced — only revisions/versions belonging to the admin's managed organization
are returned and attempts to restore a revision for a device in the other org
return 404; also fix the typo "sperate" → "separate" in the test name/comment.
Ensure you reference model_slug="device", Device, and the view names
reversion_list/reversion_detail/reversion_restore when adding assertions.
| with self.subTest("Test reversion restore view"): | ||
| revision_id = response_json["results"][0]["revision_id"] | ||
| path = reverse( | ||
| "config_api:reversion_restore", args=[model_slug, revision_id] | ||
| ) | ||
| response = self.client.post(path) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual(Device.objects.get(name="test").pk, device.pk) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Restore sub-test reuses response_json from the previous filter sub-test.
response_json here is the response of the filtered-list sub-test, not the unfiltered one — so results[0]["revision_id"] happens to equal the previously captured revision_id only by accident of the filter result containing exactly one row. Re-deriving the source-revision id explicitly (e.g., from the unfiltered list, or via revision_id already captured at L1802) makes the test robust to refactors. Also consider asserting the response body of the restore endpoint (status only is currently checked beyond the device name).
🤖 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 `@openwisp_controller/config/tests/test_api.py` around lines 1820 - 1827, The
restore sub-test is incorrectly reusing response_json from the earlier
filtered-list test; update the "Test reversion restore view" subTest to obtain
the correct revision_id explicitly (e.g., call the unfiltered list endpoint to
populate response_json or reuse the earlier captured revision_id variable
created near the first list test) before building path via
reverse("config_api:reversion_restore", args=[model_slug, revision_id]); then
post to that path and add an assertion on the restore response body (not just
status_code) and/or verify the returned JSON contains expected keys so the test
no longer depends on the filtered-list result containing exactly one row.
Black Formatting FailureHello @dee077, The CI pipeline failed due to a Fix: |
4 participants
Checklist
Reference to Existing Issue
Closes #894.
Description of Changes
Introduces three new REST API endpoints for managing revisions:
List all revisions with optional filtering by model:
GET /controller/reversion/?model=<model_name>Inspect a specific revision using its ID:
GET /controller/reversion/<revision_id>/Restore a revision based on its ID:
POST /controller/reversion/<revision_id>/restore/Let me know if any changes are needed in the response data whether anything should be included, excluded, or modified. Also, please share any suggestions for adding more filters or adjusting the response format for the restore endpoint