openwisp-controller icon indicating copy to clipboard operation
openwisp-controller copied to clipboard

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

Open dee077 opened this issue 9 months ago • 5 comments

Checklist

  • [x] I have read the OpenWISP Contributing Guidelines.
  • [x] I have manually tested the changes proposed in this pull request.
  • [x] 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

dee077 avatar Mar 20 '25 20:03 dee077

Coverage Status

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

coveralls avatar Mar 21 '25 08:03 coveralls

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?

dee077 avatar Apr 12 '25 14:04 dee077

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.

dee077 avatar May 16 '25 11:05 dee077

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 avatar May 21 '25 17:05 dee077

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.

dee077 avatar May 21 '25 22:05 dee077