Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions openwisp_controller/config/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from django.db.models import Q
from django.utils.translation import gettext_lazy as _
from rest_framework import serializers
from reversion.models import Version
from swapper import load_model

from openwisp_utils.api.serializers import ValidatedModelSerializer
Expand Down Expand Up @@ -376,3 +377,28 @@ def update(self, instance, validated_data):
instance = super().update(instance, validated_data)
self._save_m2m_templates(instance)
return instance


class VersionSerializer(BaseSerializer):
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)
content_type = serializers.CharField(source="revision.content_type", read_only=True)

class Meta:
model = Version
fields = [
"id",
"revision_id",
"object_id",
"content_type",
"db",
"format",
"serialized_data",
"object_repr",
"user_id",
"date_created",
"comment",
]
15 changes: 15 additions & 0 deletions openwisp_controller/config/api/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,21 @@ def get_api_urls(api_views):
"""
if getattr(settings, "OPENWISP_CONTROLLER_API", True):
return [
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",
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dee077 dee077 May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot from 2025-05-22 02-40-35

path(
"controller/template/",
api_views.template_list,
Expand Down
95 changes: 83 additions & 12 deletions openwisp_controller/config/api/views.py
Original file line number Diff line number Diff line change
@@ -1,22 +1,27 @@
import reversion
from cache_memoize import cache_memoize
from django.core.exceptions import ObjectDoesNotExist
from django.db import transaction
from django.db.models import F, Q
from django.http import Http404
from django.shortcuts import get_list_or_404
from django.urls.base import reverse
from django_filters.rest_framework import DjangoFilterBackend
from rest_framework import pagination, serializers, status
from rest_framework.generics import (
GenericAPIView,
ListAPIView,
ListCreateAPIView,
RetrieveAPIView,
RetrieveUpdateDestroyAPIView,
)
from rest_framework.response import Response
from reversion.models import Version
from swapper import load_model

from openwisp_users.api.permissions import DjangoModelPermissions

from ...mixins import ProtectedAPIMixin
from ...mixins import AutoRevisionMixin, ProtectedAPIMixin
from .filters import (
DeviceGroupListFilter,
DeviceListFilter,
Expand All @@ -29,6 +34,7 @@
DeviceGroupSerializer,
DeviceListSerializer,
TemplateSerializer,
VersionSerializer,
VpnSerializer,
)

Expand All @@ -48,28 +54,30 @@ class ListViewPagination(pagination.PageNumberPagination):
max_page_size = 100


class TemplateListCreateView(ProtectedAPIMixin, ListCreateAPIView):
class TemplateListCreateView(ProtectedAPIMixin, AutoRevisionMixin, ListCreateAPIView):
serializer_class = TemplateSerializer
queryset = Template.objects.prefetch_related("tags").order_by("-created")
pagination_class = ListViewPagination
filter_backends = [DjangoFilterBackend]
filterset_class = TemplateListFilter


class TemplateDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView):
class TemplateDetailView(
ProtectedAPIMixin, AutoRevisionMixin, RetrieveUpdateDestroyAPIView
):
serializer_class = TemplateSerializer
queryset = Template.objects.all()


class VpnListCreateView(ProtectedAPIMixin, ListCreateAPIView):
class VpnListCreateView(ProtectedAPIMixin, AutoRevisionMixin, ListCreateAPIView):
serializer_class = VpnSerializer
queryset = Vpn.objects.select_related("subnet").order_by("-created")
pagination_class = ListViewPagination
filter_backends = [DjangoFilterBackend]
filterset_class = VPNListFilter


class VpnDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView):
class VpnDetailView(ProtectedAPIMixin, AutoRevisionMixin, RetrieveUpdateDestroyAPIView):
serializer_class = VpnSerializer
queryset = Vpn.objects.all()

Expand All @@ -82,7 +90,7 @@ def has_object_permission(self, request, view, obj):
return perm and not obj.is_deactivated()


class DeviceListCreateView(ProtectedAPIMixin, ListCreateAPIView):
class DeviceListCreateView(ProtectedAPIMixin, AutoRevisionMixin, ListCreateAPIView):
"""
Templates: Templates flagged as required will be added automatically
to the `config` of a device and cannot be unassigned.
Expand All @@ -97,7 +105,9 @@ class DeviceListCreateView(ProtectedAPIMixin, ListCreateAPIView):
filterset_class = DeviceListFilter


class DeviceDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView):
class DeviceDetailView(
ProtectedAPIMixin, AutoRevisionMixin, RetrieveUpdateDestroyAPIView
):
"""
Templates: Templates flagged as _required_ will be added automatically
to the `config` of a device and cannot be unassigned.
Expand All @@ -124,7 +134,7 @@ def get_serializer_context(self):
return context


class DeviceActivateView(ProtectedAPIMixin, GenericAPIView):
class DeviceActivateView(ProtectedAPIMixin, AutoRevisionMixin, GenericAPIView):
serializer_class = serializers.Serializer
queryset = Device.objects.filter(_is_deactivated=True)

Expand All @@ -137,7 +147,7 @@ def post(self, request, *args, **kwargs):
return Response(serializer.data, status=status.HTTP_200_OK)


class DeviceDeactivateView(ProtectedAPIMixin, GenericAPIView):
class DeviceDeactivateView(ProtectedAPIMixin, AutoRevisionMixin, GenericAPIView):
serializer_class = serializers.Serializer
queryset = Device.objects.filter(_is_deactivated=False)

Expand All @@ -150,15 +160,19 @@ def post(self, request, *args, **kwargs):
return Response(serializer.data, status=status.HTTP_200_OK)


class DeviceGroupListCreateView(ProtectedAPIMixin, ListCreateAPIView):
class DeviceGroupListCreateView(
ProtectedAPIMixin, AutoRevisionMixin, ListCreateAPIView
):
serializer_class = DeviceGroupSerializer
queryset = DeviceGroup.objects.prefetch_related("templates").order_by("-created")
pagination_class = ListViewPagination
filter_backends = [DjangoFilterBackend]
filterset_class = DeviceGroupListFilter


class DeviceGroupDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView):
class DeviceGroupDetailView(
ProtectedAPIMixin, AutoRevisionMixin, RetrieveUpdateDestroyAPIView
):
serializer_class = DeviceGroupSerializer
queryset = DeviceGroup.objects.select_related("organization").order_by("-created")

Expand All @@ -172,7 +186,7 @@ def get_cached_devicegroup_args_rewrite(cls, org_slugs, common_name):
return url


class DeviceGroupCommonName(ProtectedAPIMixin, RetrieveAPIView):
class DeviceGroupCommonName(ProtectedAPIMixin, AutoRevisionMixin, RetrieveAPIView):
serializer_class = DeviceGroupSerializer
queryset = DeviceGroup.objects.select_related("organization").order_by("-created")
# Not setting lookup_field makes DRF raise error. but it is not used
Expand Down Expand Up @@ -289,6 +303,60 @@ def certificate_delete_invalidates_cache(cls, organization_id, common_name):
cls.get_device_group.invalidate(cls, org_slug, common_name)


class RevisionListView(ProtectedAPIMixin, ListAPIView):
serializer_class = VersionSerializer
queryset = Version.objects.select_related("revision").order_by(
"-revision__date_created"
)

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)
Comment on lines +312 to +318
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



class VersionDetailView(ProtectedAPIMixin, RetrieveAPIView):
serializer_class = VersionSerializer
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)
Comment on lines +327 to +329
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



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)
Comment on lines +338 to +340
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here!


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')}"
)
Comment on lines +345 to +352
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


serializer = VersionSerializer(
versions, many=True, context=self.get_serializer_context()
)
return Response(serializer.data, status=status.HTTP_200_OK)


template_list = TemplateListCreateView.as_view()
template_detail = TemplateDetailView.as_view()
vpn_list = VpnListCreateView.as_view()
Expand All @@ -300,3 +368,6 @@ def certificate_delete_invalidates_cache(cls, organization_id, common_name):
devicegroup_list = DeviceGroupListCreateView.as_view()
devicegroup_detail = DeviceGroupDetailView.as_view()
devicegroup_commonname = DeviceGroupCommonName.as_view()
revision_list = RevisionListView.as_view()
version_detail = VersionDetailView.as_view()
revision_restore = RevisionRestoreView.as_view()
47 changes: 47 additions & 0 deletions openwisp_controller/config/tests/test_api.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import reversion
from django.contrib.auth.models import Permission
from django.test import TestCase
from django.test.client import BOUNDARY, MULTIPART_CONTENT, encode_multipart
Expand Down Expand Up @@ -1634,3 +1635,49 @@ def test_device_patch_with_templates_of_same_org(self):
self.assertEqual(r.status_code, 200)
self.assertEqual(d1.config.templates.count(), 2)
self.assertEqual(r.data["config"]["templates"], [t1.id, t2.id])

def test_revision_list_and_restore_api(self):
org = self._get_org()
model_slug = "device"
with reversion.create_revision():
device = self._create_device(
organization=org,
name="test",
)
path = reverse("config_api:device_detail", args=[device.pk])
data = dict(name="change-test-device")
response = self.client.patch(path, data, content_type="application/json")
self.assertEqual(response.status_code, 200)
self.assertEqual(response.data["name"], "change-test-device")

with self.subTest("Test revision list"):
path = reverse("config_api:revision_list", args=[model_slug])
response = self.client.get(path)
response_json = response.json()
version_id = response_json[1]["id"]
revision_id = response_json[1]["revision_id"]
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)
self.assertEqual(len(response_json), 2)

with self.subTest("Test version detail"):
path = reverse("config_api:version_detail", args=[model_slug, version_id])
response = self.client.get(path)
self.assertEqual(response.status_code, 200)
self.assertEqual(response.json()["id"], version_id)
self.assertEqual(response.json()["object_id"], str(device.pk))

with self.subTest("Test revision restore view"):
revision_id = response_json[1]["revision_id"]
path = reverse(
"config_api:revision_restore", args=[model_slug, revision_id]
)
response = self.client.post(path)
self.assertEqual(response.status_code, 200)
self.assertEqual(Device.objects.get(name="test").pk, device.pk)
17 changes: 12 additions & 5 deletions openwisp_controller/connection/api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from openwisp_users.api.mixins import ProtectedAPIMixin as BaseProtectedAPIMixin

from ...mixins import (
AutoRevisionMixin,
ProtectedAPIMixin,
RelatedDeviceModelPermission,
RelatedDeviceProtectedAPIMixin,
Expand Down Expand Up @@ -61,7 +62,7 @@ def get_serializer_context(self):
return context


class CommandListCreateView(BaseCommandView, ListCreateAPIView):
class CommandListCreateView(BaseCommandView, AutoRevisionMixin, ListCreateAPIView):
pagination_class = ListViewPagination

def create(self, request, *args, **kwargs):
Expand All @@ -81,13 +82,15 @@ def get_object(self):
return obj


class CredentialListCreateView(ProtectedAPIMixin, ListCreateAPIView):
class CredentialListCreateView(ProtectedAPIMixin, AutoRevisionMixin, ListCreateAPIView):
queryset = Credentials.objects.order_by("-created")
serializer_class = CredentialSerializer
pagination_class = ListViewPagination


class CredentialDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView):
class CredentialDetailView(
ProtectedAPIMixin, AutoRevisionMixin, RetrieveUpdateDestroyAPIView
):
queryset = Credentials.objects.all()
serializer_class = CredentialSerializer

Expand Down Expand Up @@ -119,7 +122,9 @@ def get_parent_queryset(self):
return Device.objects.filter(pk=self.kwargs["device_id"])


class DeviceConnenctionListCreateView(BaseDeviceConnection, ListCreateAPIView):
class DeviceConnenctionListCreateView(
BaseDeviceConnection, AutoRevisionMixin, ListCreateAPIView
):
pagination_class = ListViewPagination

def get_queryset(self):
Expand All @@ -131,7 +136,9 @@ def get_queryset(self):
)


class DeviceConnectionDetailView(BaseDeviceConnection, RetrieveUpdateDestroyAPIView):
class DeviceConnectionDetailView(
BaseDeviceConnection, AutoRevisionMixin, RetrieveUpdateDestroyAPIView
):
def get_object(self):
queryset = self.filter_queryset(self.get_queryset())
filter_kwargs = {
Expand Down
Loading
Loading