[feature] Add support to reversion to the REST API #894 #994
Conversation
dee077
force-pushed
the
feature/894-rest-api-revisions
branch
from
8690016 to
3fdfcaf
Compare
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
WalkthroughAdds django-reversion integration for REST API operations. Introduces AutoRevisionMixin that wraps POST/PUT/PATCH requests in a reversion revision when the user is authenticated and the model is registered. Adds VersionSerializer exposing reversion Version fields. Adds new revision endpoints (list, detail, restore) and registers them. Applies AutoRevisionMixin to many existing API views across config, connection, geo, and pki modules. Updates tests to exercise revision listing, detail retrieval, and restore flows and adjusts numerous expected DB query counts. Sequence Diagram(s)sequenceDiagram
participant Client as Client/API Request
participant View as API View (with AutoRevisionMixin)
participant Mixin as AutoRevisionMixin.dispatch()
participant Reversion as django-reversion
participant DB as Database
Client->>View: POST/PUT/PATCH request (authenticated)
View->>Mixin: dispatch(request, ...)
Note over Mixin: Check HTTP method, authentication, model registration
alt Conditions met
Mixin->>Reversion: create_revision() (enter context)
Reversion->>DB: begin atomic revision
Mixin->>View: call original dispatch()
View->>DB: perform create/update
Mixin->>Reversion: set revision metadata (user, comment)
Reversion->>DB: save Version record(s)
Reversion->>DB: commit
Mixin-->>Client: response
else Conditions not met
Mixin->>View: call original dispatch()
View-->>Client: response
end
sequenceDiagram
participant Client as Client
participant RevAPI as RevisionListView / VersionDetailView / RevisionRestoreView
participant Serializer as VersionSerializer
participant Reversion as django-reversion
participant DB as Database
Client->>RevAPI: GET /controller/{model}/revision/
RevAPI->>DB: query Version objects (filter by content_type / revision_id)
DB-->>RevAPI: Version queryset
RevAPI->>Serializer: serialize versions
Serializer-->>RevAPI: JSON payload
RevAPI-->>Client: response
Client->>RevAPI: POST /controller/{model}/revision/{pk}/restore/
RevAPI->>DB: begin transaction
RevAPI->>DB: fetch Version by id
DB-->>RevAPI: Version
RevAPI->>Reversion: reversion.restore_object(version)
Reversion->>DB: apply restore (save object)
RevAPI->>DB: commit transaction
RevAPI-->>Client: restored object/state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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.
| source="revision.date_created", read_only=True | ||
| ) | ||
| comment = serializers.CharField(source="revision.comment", read_only=True) | ||
| content_type = serializers.CharField(source="revision.content_type", read_only=True) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n openwisp_controller/config/api/serializers.py | sed -n '380,400p'Repository: openwisp/openwisp-controller
Length of output: 871
🏁 Script executed:
# Check django-reversion models structure
python3 << 'EOF'
try:
from reversion.models import Version, Revision
# Get Version fields
version_fields = [f.name for f in Version._meta.get_fields()]
print("Version model fields:")
print(version_fields)
print()
# Get Revision fields
revision_fields = [f.name for f in Revision._meta.get_fields()]
print("Revision model fields:")
print(revision_fields)
print()
# Check Version model for content_type
if 'content_type' in version_fields:
ct_field = Version._meta.get_field('content_type')
print(f"Version.content_type field type: {type(ct_field).__name__}")
print(f"Version.content_type related_model: {ct_field.related_model}")
# Check if Revision has content_type
if 'content_type' in revision_fields:
print("Revision HAS content_type field")
else:
print("Revision DOES NOT have content_type field")
except Exception as e:
print(f"Error: {e}")
print("May not be able to import reversion models in this environment")
EOFRepository: openwisp/openwisp-controller
Length of output: 158
🏁 Script executed:
# Search for how content_type is used in similar serializers or elsewhere in the codebase
rg "content_type" openwisp_controller/config/api/serializers.py -B 2 -A 2Repository: openwisp/openwisp-controller
Length of output: 385
🏁 Script executed:
# Look for Revision model usage in the codebase
rg "revision\.content_type" openwisp_controller/ || echo "No revision.content_type found"Repository: openwisp/openwisp-controller
Length of output: 206
🌐 Web query:
django-reversion Revision model content_type field structure
💡 Result:
Short answer
- In django-reversion the Version model stores the model’s ContentType as a standard ForeignKey to Django’s ContentType model (i.e. Version.content_type → django.contrib.contenttypes.models.ContentType). The DB column is content_type_id (integer FK to django_content_type). The paired identifier for the instance is Version.object_id (stored as a string). [1][2]
References
- django-reversion Version model fields (content_type, object_id, serialized_data, etc.). [1]
- Django contenttypes framework (ContentType usage and content_type / object_id pattern). [2]
🌐 Web query:
django-reversion Version model fields documentation
💡 Result:
The Version model fields (from django-reversion API) are:
- id
- revision (ForeignKey to Revision)
- content_type (ContentType of the serialized model)
- object_id (string PK of the serialized instance)
- db (Django DB alias where saved)
- format (serialization format name)
- serialized_data (raw serialized data)
- object_repr (stored str snapshot)
- field_dict (dict of stored model fields)
Source: django-reversion API documentation. (django-reversion.readthedocs.io)
Citations:
🌐 Web query:
django-reversion Revision model fields documentation
💡 Result:
The Revision model (reversion.models.Revision) — main fields / attributes and key methods:
- id — primary key. [1]
- date_created — datetime when the revision was created. [1]
- user — the User (or AUTH_USER_MODEL) who made the revision (may be None). [1]
- comment / get_comment() — text comment attached to the revision (access via get_comment()). [1]
- revert(delete=False) — method to restore all serialized instances in the revision (optionally delete new related instances). [1]
- Related metadata: custom meta models can be attached via a model with a FK/OneToOne to Revision and reversion.add_meta(). [1][2]
Sources: Django‑reversion API docs. [1] reversion API (Revision section). [2] Low‑level API / metadata examples.
🏁 Script executed:
# Search for how revision fields are accessed in the codebase
rg "revision\." openwisp_controller/config/api/serializers.py -B 1 -A 1Repository: openwisp/openwisp-controller
Length of output: 467
Incorrect source for content_type field.
The content_type field is sourced from revision.content_type, but the Revision model does not have a content_type field. The Version model (which this serializer handles) has its own content_type foreign key. This will cause an AttributeError or return None when the serializer tries to access the field.
Proposed fix
Use one of these approaches:
- content_type = serializers.CharField(source="revision.content_type", read_only=True)
+ content_type = serializers.StringRelatedField(read_only=True)Or to expose the content type model name:
- content_type = serializers.CharField(source="revision.content_type", read_only=True)
+ content_type = serializers.CharField(source="content_type.model", read_only=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| content_type = serializers.CharField(source="revision.content_type", read_only=True) | |
| id = serializers.IntegerField(read_only=True) | |
| revision_id = serializers.IntegerField(source="revision.id", read_only=True) | |
| object_id = serializers.CharField(read_only=True) | |
| content_type = serializers.CharField(source="content_type.model", read_only=True) | |
| db = serializers.CharField(source="revision.db", read_only=True) | |
| format = serializers.CharField(source="revision.format", read_only=True) | |
| serialized_data = serializers.CharField(source="revision.serialized_data", read_only=True) | |
| object_repr = serializers.CharField(read_only=True) | |
| user_id = serializers.CharField(source="revision.user_id", read_only=True) | |
| date_created = serializers.DateTimeField(source="revision.date_created", read_only=True) | |
| comment = serializers.CharField(source="revision.comment", read_only=True) |
| path( | ||
| "controller/<str:model>/revision/", | ||
| api_views.revision_list, | ||
| name="revision_list", | ||
| ), | ||
| path( | ||
| "controller/<str:model>/revision/<str:pk>/", | ||
| api_views.version_detail, | ||
| name="version_detail", | ||
| ), | ||
| path( | ||
| "controller/<str:model>/revision/<str:pk>/restore/", | ||
| api_views.revision_restore, | ||
| name="revision_restore", | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check RevisionListView implementation and model parameter handling
rg -n "class RevisionListView" openwisp_controller/config/api/views.py -A30Repository: openwisp/openwisp-controller
Length of output: 1334
🏁 Script executed:
#!/bin/bash
# Get complete RevisionRestoreView implementation
rg -n "class RevisionRestoreView" openwisp_controller/config/api/views.py -A20Repository: openwisp/openwisp-controller
Length of output: 1000
Fix logic error in RevisionListView.get_queryset().
The revision endpoint URLs are correctly positioned to avoid path conflicts. However, there is a critical bug in RevisionListView.get_queryset() at line 318: the filtered queryset variable (which applies the revision_id query parameter filtering from lines 315-317) is discarded, and the method instead returns the unfiltered self.queryset. This causes the revision_id parameter to be ignored.
Additionally, error handling is inconsistent: invalid/unregistered models return an empty list in list and detail views, but RevisionRestoreView uses get_list_or_404() which raises a 404 error. Consider standardizing the response behavior across endpoints.
🤖 Prompt for AI Agents
In @openwisp_controller/config/api/urls.py around lines 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.
| class RevisionRestoreView(ProtectedAPIMixin, GenericAPIView): | ||
| serializer_class = serializers.Serializer | ||
| queryset = Version.objects.select_related("revision").order_by( | ||
| "-revision__date_created" | ||
| ) | ||
|
|
||
| def get_queryset(self): | ||
| model = self.kwargs.get("model").lower() | ||
| return self.queryset.filter(content_type__model=model) | ||
|
|
||
| def post(self, request, *args, **kwargs): | ||
| qs = self.get_queryset() | ||
| versions = get_list_or_404(qs, revision_id=kwargs["pk"]) | ||
| 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')}" | ||
| ) | ||
|
|
||
| serializer = VersionSerializer( | ||
| versions, many=True, context=self.get_serializer_context() | ||
| ) | ||
| return Response(serializer.data, status=status.HTTP_200_OK) |
There was a problem hiding this comment.
Consider adding organization-based authorization for restore operations.
The RevisionRestoreView allows restoring any revision without checking whether the user has permission to modify objects in the relevant organization(s). This could allow users to restore objects they don't have access to.
Additionally, as noted in past review comments, consider using Revision.revert() instead of iterating individual versions, which handles the revert atomically:
from reversion.models import Revision
revision = get_object_or_404(Revision, pk=kwargs["pk"])
revision.revert()🧰 Tools
🪛 Ruff (0.14.10)
342-342: Unused method argument: args
(ARG002)
| with self.subTest("Test revision list filter by revision id"): | ||
| path = reverse("config_api:revision_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), 2) |
There was a problem hiding this comment.
Filter test assertion appears incorrect.
The test filters by revision_id but still expects len(response_json) == 2. If filtering by a specific revision ID, the result should contain only versions belonging to that single revision (likely 1-2 versions depending on what was captured), not all versions across all revisions.
Additionally, this test may be verifying a bug in RevisionListView.get_queryset() (see related comment on views.py line 318) where the filter is not actually applied.
🔎 Suggested fix
with self.subTest("Test revision list filter by revision id"):
path = reverse("config_api:revision_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), 2)
+ # Filtering by revision_id should return only versions from that revision
+ self.assertGreaterEqual(len(response_json), 1)
+ for version in response_json:
+ self.assertEqual(version["revision_id"], revision_id)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with self.subTest("Test revision list filter by revision id"): | |
| path = reverse("config_api:revision_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), 2) | |
| with self.subTest("Test revision list filter by revision id"): | |
| path = reverse("config_api:revision_list", args=[model_slug]) | |
| response = self.client.get(f"{path}?revision_id={revision_id}") | |
| response_json = response.json() | |
| self.assertEqual(response.status_code, 200) | |
| # Filtering by revision_id should return only versions from that revision | |
| self.assertGreaterEqual(len(response_json), 1) | |
| for version in response_json: | |
| self.assertEqual(version["revision_id"], revision_id) |
🤖 Prompt for AI Agents
In @openwisp_controller/config/tests/test_api.py around lines 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).
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@dee077 there's conflicts with master
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
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.
| def initial(self, *args, **kwargs): | ||
| super().initial(*args, **kwargs) | ||
| self.assert_parent_exists() | ||
|
|
||
| def assert_parent_exists(self): | ||
| try: | ||
| assert self.get_parent_queryset().exists() | ||
| except (AssertionError, ValidationError): | ||
| device_id = self.kwargs["device_id"] | ||
| raise NotFound(detail=f'Device with ID "{device_id}" not found.') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "views.py" -path "*/connection/api/*" | head -5Repository: openwisp/openwisp-controller
Length of output: 117
🏁 Script executed:
cat -n openwisp_controller/connection/api/views.py | head -50Repository: openwisp/openwisp-controller
Length of output: 1866
🏁 Script executed:
cat -n openwisp_controller/connection/api/views.py | sed -n '130,150p'Repository: openwisp/openwisp-controller
Length of output: 891
🏁 Script executed:
grep -n "ValidationError\|NotFound" openwisp_controller/connection/api/views.py | head -20Repository: openwisp/openwisp-controller
Length of output: 207
🏁 Script executed:
cat -n openwisp_controller/connection/api/views.py | head -30Repository: openwisp/openwisp-controller
Length of output: 1082
Add missing imports for ValidationError and NotFound.
Lines 143 and 145 use these exceptions without importing them, causing NameError at runtime. Add:
from django.core.exceptions import ValidationErrorfrom rest_framework.exceptions import NotFound
Proposed fix
-from django.utils.translation import gettext_lazy as _
+from django.core.exceptions import ValidationError
+from django.utils.translation import gettext_lazy as _-from rest_framework.generics import (
+from rest_framework.exceptions import NotFound
+from rest_framework.generics import (🧰 Tools
🪛 Ruff (0.15.0)
[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)
🤖 Prompt for AI Agents
In `@openwisp_controller/connection/api/views.py` around lines 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.
| "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.
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