[change] Improved consistency of REST API

* [change] Renamed serializer to serializers in connection.api (consistency)
* [change] Centralize the base serializer logic of the REST API
* [fix] Fixed bug with new basic config and different org templates
* [chores] Increased consistency of connection API pk params
* [chores] PKI set URL param type to int
* [chores] Config API: use uuid path converter

Author: nemesifier

openwisp_controller/config/api/serializers.py

@@ -1,15 +1,13 @@
-from copy import deepcopy
-
 from django.core.exceptions import ValidationError
 from django.db import transaction
 from django.db.models import Q
 from django.utils.translation import gettext_lazy as _
 from rest_framework import serializers
 from swapper import load_model
 
-from openwisp_users.api.mixins import FilterSerializerByOrgManaged
 from openwisp_utils.api.serializers import ValidatedModelSerializer
 
+from ...serializers import BaseSerializer
 from .. import settings as app_settings
 
 Template = load_model('config', 'Template')
@@ -24,10 +22,6 @@ class BaseMeta:
     read_only_fields = ['created', 'modified']
 
 
-class BaseSerializer(FilterSerializerByOrgManaged, ValidatedModelSerializer):
-    pass
-
-
 class TemplateSerializer(BaseSerializer):
     config = serializers.JSONField(initial={}, required=False)
     tags = serializers.StringRelatedField(many=True, read_only=True)
@@ -110,7 +104,12 @@ def get_queryset(self):
         return queryset
 
 
-class BaseConfigSerializer(serializers.ModelSerializer):
+class BaseConfigSerializer(ValidatedModelSerializer):
+    # The device object is excluded from validation
+    # because this serializer focuses on validating
+    # config objects.
+    exclude_validation = ['device']
+
     class Meta:
         model = Config
         fields = ['status', 'error_reason', 'backend', 'templates', 'context', 'config']
@@ -119,8 +118,26 @@ class Meta:
             'error_reason': {'read_only': True},
         }
 
+    def validate(self, data):
+        """
+        The validation here is a bit tricky:
+
+        For existing devices, we have to perform the
+        model validation of the existing config object,
+        because if we simulate the validation on a new
+        config object pointing to an existing device,
+        the validation will fail because a config object
+        for this device already exists (due to one-to-one relationship).
+        """
+        device = self.context.get('device')
+        if not self.instance and device:
+            # Existing device with existing config
+            if device._has_config():
+                self.instance = device.config
+        return super().validate(data)
+
 
-class DeviceConfigMixin(object):
+class DeviceConfigSerializer(BaseSerializer):
     def _get_config_templates(self, config_data):
         return [template.pk for template in config_data.pop('templates', [])]
 
@@ -131,11 +148,29 @@ def _prepare_config(self, device, config_data):
         config.full_clean()
         return config
 
+    def _is_config_data_relevant(self, config_data):
+        """
+        Returns True if ``config_data`` does not equal
+        the default values and hence the config is useful.
+        """
+        return not (
+            config_data.get('backend') == app_settings.DEFAULT_BACKEND
+            and not config_data.get('templates')
+            and not config_data.get('context')
+            and not config_data.get('config')
+        )
+
     @transaction.atomic
     def _create_config(self, device, config_data):
         config_templates = self._get_config_templates(config_data)
         try:
             if not device._has_config():
+                # if the user hasn't set any useful config data, skip
+                if (
+                    not self._is_config_data_relevant(config_data)
+                    and not config_templates
+                ):
+                    return
                 config = Config(device=device, **config_data)
                 config.full_clean()
                 config.save()
@@ -151,15 +186,10 @@ def _create_config(self, device, config_data):
             raise serializers.ValidationError({'config': error.messages})
 
     def _update_config(self, device, config_data):
-        if (
-            config_data.get('backend') == app_settings.DEFAULT_BACKEND
-            and not config_data.get('templates')
-            and not config_data.get('context')
-            and not config_data.get('config')
-        ):
-            # Do not create Config object if config_data only
-            # contains the default value.
-            # See https://github.com/openwisp/openwisp-controller/issues/699
+        # Do not create Config object if config_data only
+        # contains the default values.
+        # See https://github.com/openwisp/openwisp-controller/issues/699
+        if not self._is_config_data_relevant(config_data):
             return
         if not device._has_config():
             return self._create_config(device, config_data)
@@ -190,9 +220,7 @@ class DeviceListConfigSerializer(BaseConfigSerializer):
     templates = FilterTemplatesByOrganization(many=True, write_only=True)
 
 
-class DeviceListSerializer(
-    DeviceConfigMixin, FilterSerializerByOrgManaged, serializers.ModelSerializer
-):
+class DeviceListSerializer(DeviceConfigSerializer):
     config = DeviceListConfigSerializer(required=False)
 
     class Meta(BaseMeta):
@@ -219,14 +247,13 @@ class Meta(BaseMeta):
             'management_ip': {'allow_blank': True},
         }
 
-    def validate(self, attrs):
-        device_data = deepcopy(attrs)
+    def validate(self, data):
         # Validation of "config" is performed after
         # device object is created in the "create" method.
-        device_data.pop('config', None)
-        device = self.instance or self.Meta.model(**device_data)
-        device.full_clean()
-        return attrs
+        config_data = data.pop('config', None)
+        data = super().validate(data)
+        data['config'] = config_data
+        return data
 
     def create(self, validated_data):
         config_data = validated_data.pop('config', None)
@@ -247,7 +274,7 @@ class DeviceDetailConfigSerializer(BaseConfigSerializer):
     templates = FilterTemplatesByOrganization(many=True)
 
 
-class DeviceDetailSerializer(DeviceConfigMixin, BaseSerializer):
+class DeviceDetailSerializer(DeviceConfigSerializer):
     config = DeviceDetailConfigSerializer(allow_null=True)
     is_deactivated = serializers.BooleanField(read_only=True)
 
@@ -280,7 +307,7 @@ def update(self, instance, validated_data):
         if config_data:
             self._update_config(instance, config_data)
 
-        elif hasattr(instance, 'config') and validated_data.get('organization'):
+        elif instance._has_config() and validated_data.get('organization'):
             if instance.organization != validated_data.get('organization'):
                 # config.device.organization is used for validating
                 # the organization of templates. It is also used for adding

openwisp_controller/config/api/urls.py

@@ -19,12 +19,12 @@ def get_api_urls(api_views):
                 name='template_list',
             ),
             path(
-                'controller/template/<str:pk>/',
+                'controller/template/<uuid:pk>/',
                 api_views.template_detail,
                 name='template_detail',
             ),
             path(
-                'controller/template/<str:pk>/configuration/',
+                'controller/template/<uuid:pk>/configuration/',
                 api_download_views.download_template_config,
                 name='download_template_config',
             ),
@@ -34,12 +34,12 @@ def get_api_urls(api_views):
                 name='vpn_list',
             ),
             path(
-                'controller/vpn/<str:pk>/',
+                'controller/vpn/<uuid:pk>/',
                 api_views.vpn_detail,
                 name='vpn_detail',
             ),
             path(
-                'controller/vpn/<str:pk>/configuration/',
+                'controller/vpn/<uuid:pk>/configuration/',
                 api_download_views.download_vpn_config,
                 name='download_vpn_config',
             ),
@@ -49,17 +49,17 @@ def get_api_urls(api_views):
                 name='device_list',
             ),
             path(
-                'controller/device/<str:pk>/',
+                'controller/device/<uuid:pk>/',
                 api_views.device_detail,
                 name='device_detail',
             ),
             path(
-                'controller/device/<str:pk>/activate/',
+                'controller/device/<uuid:pk>/activate/',
                 api_views.device_activate,
                 name='device_activate',
             ),
             path(
-                'controller/device/<str:pk>/deactivate/',
+                'controller/device/<uuid:pk>/deactivate/',
                 api_views.device_deactivate,
                 name='device_deactivate',
             ),
@@ -69,7 +69,7 @@ def get_api_urls(api_views):
                 name='devicegroup_list',
             ),
             path(
-                'controller/group/<str:pk>/',
+                'controller/group/<uuid:pk>/',
                 api_views.devicegroup_detail,
                 name='devicegroup_detail',
             ),
@@ -79,7 +79,7 @@ def get_api_urls(api_views):
                 name='devicegroup_x509_commonname',
             ),
             path(
-                'controller/device/<str:pk>/configuration/',
+                'controller/device/<uuid:pk>/configuration/',
                 api_download_views.download_device_config,
                 name='download_device_config',
             ),

openwisp_controller/config/api/views.py

@@ -111,6 +111,18 @@ def perform_destroy(self, instance):
         force_deletion = self.request.query_params.get('force', None) == 'true'
         instance.delete(check_deactivated=(not force_deletion))
 
+    def get_object(self):
+        """Set device property for serializer context."""
+        obj = super().get_object()
+        self.device = obj
+        return obj
+
+    def get_serializer_context(self):
+        """Add device to serializer context for validation purposes."""
+        context = super().get_serializer_context()
+        context['device'] = self.device
+        return context
+
 
 class DeviceActivateView(ProtectedAPIMixin, GenericAPIView):
     serializer_class = serializers.Serializer

openwisp_controller/config/base/config.py

@@ -469,13 +469,13 @@ def clean(self):
         * modifies status if key attributes of the configuration
           have changed (queries the database)
         """
-        super().clean()
         if not self.context:
             self.context = {}
         if not isinstance(self.context, dict):
             raise ValidationError(
                 {'context': _('the supplied value is not a JSON object')}
             )
+        super().clean()
 
     def save(self, *args, **kwargs):
         created = self._state.adding