[feature] Add support to reversion to the REST API #994

dee077 · 2025-03-20T20:17:11Z

Checklist

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

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

coveralls · 2025-03-21T08:42:40Z

coverage: 98.828% (+0.01%) from 98.816%
when pulling 3afb759 on dee077:feature/894-rest-api-revisions
into 85eee35 on openwisp:master.

nemesifier

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.

openwisp_controller/config/api/filters.py

openwisp_controller/config/api/urls.py

dee077 · 2025-04-12T14:37:28Z

I’ve added REST API revision support using django-reversion. Here’s a summary

What’s Added

AutoRevisionMixin uses RevisionMixin suggested by @pandafy which wraps API requests in reversion.create_revision().
Automatically sets the user and a comment for each revision.

New Endpoints:

controller/<str:model_slug>/revision/ – List revisions
controller/<str:model_slug>/revision/<str:pk>/ – Revision detail
controller/<str:model_slug>/revision/<str:pk>/restore/ – Restore a revision
These endpoints are placed above similar paths like controller/device/<str:pk>/ to avoid conflicts, as pk will never be revision.

How It Works:

Each change creates an entry in the reversion_revision table.
The corresponding object snapshot is stored in reversion_version with the same revision_id.

Issues Noticed

When using AutoRevisionMixin in the DeviceDetailView, the devicegroup_change_handler behaves unexpectedly, so the template doesn’t update on group change.
Fails the test: test_device_api_change_group.
Tried the mixin in other modules (connection, geo, pki), observed complexity increase (some entries show +10 additional SQL queries).

Question

Given the unexpected behavior in signals and added complexity:
Is this approach okay to proceed with, or alternative design for API revision support is needed?

nemesifier

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.

nemesifier · 2025-05-06T15:16:43Z

openwisp_controller/config/api/urls.py

+                'controller/<str:model_slug>/revision/<str:pk>/restore/',
+                api_views.revision_restore,
+                name='revision_restore',
+            ),


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.

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?

Currently, empty revisions are being stored with only user and comment entries, which does not offer anything.

Added a fix for this
For reference these are the models that are getting revisions

openwisp_controller/config/api/filters.py

openwisp_controller/config/api/serializers.py

openwisp_controller/mixins.py

openwisp_controller/config/api/serializers.py

dee077 · 2025-05-16T11:28:44Z

Updates

Fix single quotes
Added the AutoRevisionMixin across the module. This caused 2 big issues
The test was failing due to AutoRevisionMixin wrapping the view in a transaction.atomic() block, which delayed the post_save signal (registered via transaction.on_commit) until the outermost transaction committed. Since the outer transaction wasn’t triggered when PUT request generating the response,that returned outdated data while the database was updated afterward. Resolved by setting revision_atomic = False in the mixin to avoid unnecessary nesting.
Some GeoTestsApi were inconsistently counting queries due to Django Reversion caching the ContentType lookup (used for versioning). On the first run, the lookup incurred extra queries, on subsequent runs, caching reduced them, leading to assertion failures. Fixed by adding ContentType.objects.clear_cache() in the setUp method of the test case to ensure consistent query counts.

nemesifier

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.

pandafy

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.

openwisp_controller/config/api/serializers.py

openwisp_controller/config/api/filters.py

openwisp_controller/config/api/views.py

dee077 · 2025-05-21T17:55:07Z

Updates

Again added test with increased queries.
The queryset uses Version objects because there is no reverse relationship from Revision to Version, but each Version is linked to a Revision.
Rename RevisionSerializer to VersionSerializer
In VersionSerializer directly defined the fields instead of using SerializerMethodField
Added a filter by revision_id in RevisionListView to allow retrieving versions linked to the same revision, which helps group related changes made at the same time.
Added test for revision_id filter
Small qa fix for double quotes.

dee077 · 2025-05-21T22:02:30Z

Updates

Removed RevisionFilter class as no longer using model filter by params
Added additional check for registered model

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.

nemesifier

@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

…d serializer for more clarity

… to geo_api:device_location

pandafy

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.

pandafy · 2025-06-10T12:40:03Z

openwisp_controller/config/api/views.py

+    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)


The way this method is written breaks the convention of Django/DRF. It is also inconsistent with OpenWISP.

Suggested change

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

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.

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.

pandafy · 2025-06-10T12:47:35Z

openwisp_controller/config/api/views.py

+    def get_queryset(self):
+        model = self.kwargs.get("model").lower()
+        return self.queryset.filter(content_type__model=model)


Suggested change

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.

pandafy · 2025-06-10T12:51:33Z

openwisp_controller/config/api/views.py

+    def get_queryset(self):
+        model = self.kwargs.get("model").lower()
+        return self.queryset.filter(content_type__model=model)


pandafy · 2025-06-10T13:16:58Z

openwisp_controller/config/api/views.py

+        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')}"
+                )


Why don't we use Revision.revert() instead of reverting individual versions?

dee077 force-pushed the feature/894-rest-api-revisions branch from 8690016 to 3fdfcaf Compare March 21, 2025 08:34

dee077 force-pushed the feature/894-rest-api-revisions branch from 3fdfcaf to 98ca619 Compare March 21, 2025 09:03

nemesifier requested changes Mar 22, 2025

View reviewed changes

openwisp_controller/config/api/filters.py Outdated Show resolved Hide resolved

openwisp_controller/config/api/urls.py Outdated Show resolved Hide resolved

dee077 changed the title ~~[feature] Rivision for Rest Api changes~~ [feature] Revisions for Rest Api changes Mar 25, 2025

nemesifier changed the title ~~[feature] Revisions for Rest Api changes~~ [feature] Add support to reversion to the REST API Mar 27, 2025

nemesifier assigned dee077 Mar 27, 2025

nemesifier added the enhancement label Mar 27, 2025

dee077 force-pushed the feature/894-rest-api-revisions branch from 84a68d6 to df5c3fa Compare April 12, 2025 13:56

nemesifier requested changes May 6, 2025

View reviewed changes

pandafy reviewed May 6, 2025

View reviewed changes

openwisp_controller/mixins.py Outdated Show resolved Hide resolved

openwisp_controller/config/api/serializers.py Show resolved Hide resolved

pandafy added this to OpenWISP Contributor's Board May 6, 2025

github-project-automation bot moved this to To do (general) in OpenWISP Contributor's Board May 6, 2025

pandafy moved this from To do (general) to In progress in OpenWISP Contributor's Board May 6, 2025

nemesifier reviewed May 16, 2025

View reviewed changes

dee077 force-pushed the feature/894-rest-api-revisions branch from 34dbfcf to 47e1c70 Compare May 17, 2025 08:29

pandafy requested changes May 20, 2025

View reviewed changes

dee077 force-pushed the feature/894-rest-api-revisions branch 2 times, most recently from 36b017e to 92b7954 Compare May 22, 2025 08:26

nemesifier requested changes Jun 3, 2025

View reviewed changes

dee077 added 6 commits June 4, 2025 14:11

[feature] Rivision for Rest Api changes openwisp#894

80288b6

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

[fix] Fix param

675f162

[feature] Added AutoRevisionMixin

dd5c232

[tests] Show failing tests

73d5145

[fix] Add revision_id filter to RevisionListView and renamed views an…

4bb281f

…d serializer for more clarity

[fix] Additional check for registered model

0aa5c4d

[fix] Remove ReversionFilter class

7580cdc

dee077 force-pushed the feature/894-rest-api-revisions branch from 92b7954 to fd1faab Compare June 4, 2025 09:13

[fix] Resolve conflicts and increased queries based on recent changes…

3afb759

… to geo_api:device_location

dee077 force-pushed the feature/894-rest-api-revisions branch from fd1faab to 3afb759 Compare June 4, 2025 09:49

pandafy requested changes Jun 10, 2025

View reviewed changes

Uh oh!

[feature] Add support to reversion to the REST API #994

Are you sure you want to change the base?

[feature] Add support to reversion to the REST API #994

Uh oh!

Conversation

dee077 commented Mar 20, 2025

Checklist

Reference to Existing Issue

Description of Changes

Uh oh!

coveralls commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dee077 commented Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What’s Added

New Endpoints:

How It Works:

Issues Noticed

Question

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dee077 May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dee077 commented May 16, 2025

Updates

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

pandafy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dee077 commented May 21, 2025

Updates

Uh oh!

dee077 commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Updates

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

pandafy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 21, 2025 •

edited

Loading

dee077 commented Apr 12, 2025 •

edited

Loading

dee077 May 21, 2025 •

edited

Loading

dee077 commented May 21, 2025 •

edited

Loading