[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.
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
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
WalkthroughThis pull request implements automatic revision tracking for REST API mutations using django-reversion. A new Sequence Diagram(s)sequenceDiagram
participant Client as Client/API Request
participant View as API View<br/>(with AutoRevisionMixin)
participant ARM as AutoRevisionMixin<br/>dispatch()
participant Reversion as django-reversion<br/>revision context
participant DB as Database
Client->>View: PATCH/POST/PUT request<br/>(authenticated user)
View->>ARM: dispatch(request, ...)
rect rgb(200, 220, 255)
Note over ARM: Check conditions:<br/>- Method is POST/PUT/PATCH<br/>- User authenticated<br/>- Model is registered
end
alt Conditions met
ARM->>Reversion: create_revision()<br/>enter revision context
Reversion->>DB: Begin transaction<br/>(revision_atomic)
ARM->>View: Call original dispatch()
Note over View: Execute mutation<br/>(create/update logic)
View->>DB: Save changes
ARM->>Reversion: Set revision metadata<br/>(user, comment, method, path)
Reversion->>DB: Save Version record<br/>with revision info
Reversion->>DB: Commit transaction
Reversion-->>ARM: Revision created
ARM-->>Client: Response
else Conditions not met
ARM->>View: Call original dispatch()
Note over View: Execute without<br/>revision tracking
View-->>Client: Response
end
sequenceDiagram
participant Client as Client/API Request
participant RevListView as RevisionListView
participant Filter as Queryset Filter<br/>(by model, revision_id)
participant Serializer as VersionSerializer
participant DB as Database
Client->>RevListView: GET /controller/{model}/revision/
RevListView->>DB: Fetch Version records<br/>for model
DB-->>RevListView: Version objects
RevListView->>Filter: Filter by model<br/>content_type
Filter->>DB: Query filtered versions
DB-->>Filter: Filtered results
Filter->>Serializer: Serialize Version objects
Serializer->>Serializer: Map fields:<br/>id, revision_id, user_id,<br/>date_created, comment
Serializer-->>RevListView: Serialized data
RevListView-->>Client: JSON response
Client->>RevListView: POST /restore/
Note over RevListView: Extract version_id from request
RevListView->>DB: Begin atomic transaction
RevListView->>DB: Fetch Version by id
DB-->>RevListView: Version record
RevListView->>DB: Restore via reversion<br/>(deserialize & save)
RevListView->>DB: Commit transaction
RevListView-->>Client: Restored object
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
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
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