Report error when update specifies a read-only key

The fields would otherwise be ignored, which is confusing for the user.
This change required fixing a couple tests. There were tests that
actually checked that the fields are ignored, now they verify that the
request fails.

A bigger change was needed in repository clone view to make sure ids are
not fed into the serializer. This actually fixes an error when
previously the response of repo clone would include old identifiers.

JIRA: PDC-951

contrib/drf_introspection/serializers.py

@@ -11,6 +11,15 @@
 from django.core.exceptions import FieldError
 
 
+def _error_with_fields(message, fields):
+    """
+    Helper function to create an error message with a list of quoted, comma
+    separated field names. The `message` argument should be a format string
+    with a single `%s` placeholder.
+    """
+    return message % ', '.join('"%s"' % f for f in fields)
+
+
 class IntrospectableSerializerMixin(object):
     """
     Basic mixin for a serializer that supports introspection.
@@ -49,13 +58,15 @@ class StrictSerializerMixin(IntrospectableSerializerMixin):
     fields are allowed.
 
     Additionally, if the input to the serializer is not a dict, a
-    ``rest_framework.serializers.ValidationError`` will be raised.
+    ``rest_framework.serializers.ValidationError`` will be raised. Also, when a
+    read-only field is specified, a FieldError will be raised as well.
     """
     def to_internal_value(self, data):
         if not isinstance(data, dict):
             raise serializers.ValidationError('Invalid input: must be a dict.')
         extra_fields = set(data.keys()) - self.get_allowed_keys()
         self.maybe_raise_error(extra_fields)
+        self.check_read_only_fields(data.keys())
         return super(StrictSerializerMixin, self).to_internal_value(data)
 
     @staticmethod
@@ -70,4 +81,19 @@ def maybe_raise_error(extra_fields):
         serializer, but still wants to validate its input.
         """
         if extra_fields:
-            raise FieldError('Unknown fields: %s.' % ', '.join('"%s"' % f for f in extra_fields))
+            raise FieldError(_error_with_fields('Unknown fields: %s.', extra_fields))
+
+    def check_read_only_fields(self, keys):
+        """
+        Check that all fields are not read-only. If some are, a FieldError is
+        raised with an error message.
+        """
+        updated_read_only = [key for key in keys if self._is_read_only(key)]
+        if updated_read_only:
+            raise FieldError(_error_with_fields('Can not update read only fields: %s.',
+                                                updated_read_only))
+
+    def _is_read_only(self, field_name):
+        if field_name not in self.fields:
+            return False
+        return getattr(self.fields[field_name], 'read_only', False)

pdc/apps/component/tests.py

@@ -75,13 +75,7 @@ def test_create_global_component_with_labels(self):
         url = reverse('globalcomponent-list')
         data = {'name': 'TestCaseComponent', 'dist_git_path': 'python', 'labels': [{'name': 'label1', 'description': 'abc'}]}
         response = self.client.post(url, data, format='json')
-        self.assertEqual(response.status_code, status.HTTP_201_CREATED)
-        del response.data['id']
-        del response.data['dist_git_web_url']
-        data.update({'contacts': []})
-        data.update({'labels': []})
-        data.update({'upstream': None})
-        self.assertEqual(response.data, data)
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
 
     def test_create_global_component_with_upstream(self):
         url = reverse('globalcomponent-list')

pdc/apps/compose/tests.py

@@ -679,6 +679,7 @@ def test_create_duplicit(self):
 
     def test_create_correct(self):
         self.override_rpm["rpm_name"] = "bash-debuginfo"
+        del self.override_rpm["id"]
         response = self.client.post(reverse('overridesrpm-list'), self.override_rpm)
         self.assertEqual(response.status_code, status.HTTP_201_CREATED)
         self.assertEqual(models.OverrideRPM.objects.count(), 2)

pdc/apps/package/tests.py

@@ -234,7 +234,8 @@ def test_update_rpm_with_linked_compose_should_read_only(self):
         url = reverse('rpms-detail', args=[3])
         data = {'linked_composes': [u'compose-1']}
         response = self.client.patch(url, data, format='json')
-        self.assertEqual(response.data.get('linked_composes'), [])
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        self.assertNumChanges([])
 
     def test_bulk_update_patch(self):
         self.client.patch(reverse('rpms-list'),

pdc/apps/release/tests.py

@@ -225,6 +225,12 @@ def test_update_only_short(self):
         response = self.client.get(reverse('product-detail', args=['tcudorp']))
         self.assertEqual(response.status_code, status.HTTP_200_OK)
 
+    def test_patch_read_only_field(self):
+        response = self.client.patch(reverse('product-detail', args=['product']),
+                                     {'active': True}, format='json')
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        self.assertNumChanges([])
+
 
 class ProductVersionRESTTestCase(TestCaseWithChangeSetMixin, APITestCase):
     fixtures = [
@@ -317,6 +323,9 @@ def test_clone(self):
         response = self.client.get(reverse('productversion-detail', args=['product-1']))
         self.assertEqual(status.HTTP_200_OK, response.status_code)
         response.data['version'] = 2
+        del response.data['product_version_id']
+        del response.data['releases']
+        del response.data['active']
         response = self.client.post(reverse('productversion-list'), response.data)
         self.assertEqual(status.HTTP_201_CREATED, response.status_code)
         self.assertEqual(dict(response.data),

pdc/apps/repository/tests.py

@@ -77,6 +77,7 @@ def test_deserialize_missing_value(self):
             self.data[field] = old_val
 
     def test_deserialize_duplicit(self):
+        del self.fixture_data['id']
         serializer = serializers.RepoSerializer(data=self.fixture_data)
         self.assertFalse(serializer.is_valid())
         self.assertEqual(serializer.errors,
@@ -158,9 +159,11 @@ def test_update_without_product_id(self):
         """The repo has product_id, update tries to change name with product_id unspecified in request."""
         pid = self.existing.pop('product_id')
         self.existing['name'] = 'new_name'
+        id = self.existing.pop('id')
         response = self.client.put(reverse('repo-detail', args=[1]), self.existing, format='json')
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.existing['product_id'] = pid
+        self.existing['id'] = id
         self.assertDictEqual(dict(response.data), self.existing)
         self.assertNumChanges([1])
 
@@ -326,8 +329,7 @@ class RepositoryCloneTestCase(TestCaseWithChangeSetMixin, APITestCase):
     ]
 
     def setUp(self):
-        self.repo1 = {"id": 1,
-                      "shadow": False,
+        self.repo1 = {"shadow": False,
                       "release_id": "release-1.1",
                       "variant_uid": "Server",
                       "arch": "x86_64",
@@ -337,8 +339,7 @@ def setUp(self):
                       "content_category": "binary",
                       "name": "test_repo_1",
                       "product_id": 11}
-        self.repo2 = {"id": 2,
-                      "shadow": True,
+        self.repo2 = {"shadow": True,
                       "release_id": "release-1.1",
                       "variant_uid": "Client",
                       "arch": "x86_64",
@@ -382,6 +383,9 @@ def test_clone(self):
         args = {'release_id_from': 'release-1.0', 'release_id_to': 'release-1.1'}
         response = self.client.post(reverse('repoclone-list'), args, format='json')
         self.assertEqual(response.status_code, status.HTTP_200_OK)
+        # Drop ids, they are not easily predictable on PostgreSQL
+        for repo in response.data:
+            del repo['id']
         self.assertItemsEqual(response.data, [self.repo1, self.repo2])
         repos = models.Repo.objects.filter(variant_arch__variant__release__release_id='release-1.1')
         self.assertEqual(len(repos), 2)
@@ -394,6 +398,8 @@ def test_clone_with_explicit_includes(self):
                 'include_content_format': ['iso', 'rpm'],
                 'include_content_category': ['debug', 'binary']}
         response = self.client.post(reverse('repoclone-list'), args, format='json')
+        for repo in response.data:
+            del repo['id']
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertItemsEqual(response.data, [self.repo1, self.repo2])
         repos = models.Repo.objects.filter(variant_arch__variant__release__release_id='release-1.1')
@@ -413,6 +419,8 @@ def test_skip_on_include_service(self):
         args = {'release_id_from': 'release-1.0', 'release_id_to': 'release-1.1',
                 'include_service': ['pulp']}
         response = self.client.post(reverse('repoclone-list'), args, format='json')
+        for repo in response.data:
+            del repo['id']
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertItemsEqual(response.data, [self.repo2])
         self.assertNumChanges([1])
@@ -421,6 +429,8 @@ def test_skip_on_include_repo_family(self):
         args = {'release_id_from': 'release-1.0', 'release_id_to': 'release-1.1',
                 'include_repo_family': ['beta']}
         response = self.client.post(reverse('repoclone-list'), args, format='json')
+        for repo in response.data:
+            del repo['id']
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertItemsEqual(response.data, [self.repo2])
         self.assertNumChanges([1])
@@ -429,6 +439,8 @@ def test_skip_on_include_content_format(self):
         args = {'release_id_from': 'release-1.0', 'release_id_to': 'release-1.1',
                 'include_content_format': ['iso']}
         response = self.client.post(reverse('repoclone-list'), args, format='json')
+        for repo in response.data:
+            del repo['id']
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertItemsEqual(response.data, [self.repo2])
         self.assertNumChanges([1])
@@ -437,6 +449,8 @@ def test_skip_on_include_content_category(self):
         args = {'release_id_from': 'release-1.0', 'release_id_to': 'release-1.1',
                 'include_content_category': ['debug']}
         response = self.client.post(reverse('repoclone-list'), args, format='json')
+        for repo in response.data:
+            del repo['id']
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertItemsEqual(response.data, [self.repo2])
         self.assertNumChanges([1])
@@ -445,6 +459,8 @@ def test_skip_on_include_shadow(self):
         args = {'release_id_from': 'release-1.0', 'release_id_to': 'release-1.1',
                 'include_shadow': 'true'}
         response = self.client.post(reverse('repoclone-list'), args, format='json')
+        for repo in response.data:
+            del repo['id']
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertItemsEqual(response.data, [self.repo2])
         self.assertNumChanges([1])
@@ -453,6 +469,8 @@ def test_skip_on_include_product_id(self):
         args = {'release_id_from': 'release-1.0', 'release_id_to': 'release-1.1',
                 'include_product_id': 12}
         response = self.client.post(reverse('repoclone-list'), args, format='json')
+        for repo in response.data:
+            del repo['id']
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertItemsEqual(response.data, [self.repo2])
         self.assertNumChanges([1])

pdc/apps/repository/views.py

@@ -214,6 +214,8 @@ def create(self, request):
         serializer = serializers.RepoSerializer(repos_in_target_release, many=True)
         copy = serializer.data
         for repo in copy:
+            # The serializer will reject read-only fields, so we need to drop the id.
+            del repo['id']
             repo['release_id'] = target_release.release_id
         new_repos = serializers.RepoSerializer(data=copy, many=True)
         if not new_repos.is_valid():
@@ -225,7 +227,7 @@ def create(self, request):
             request.changeset.add('Repo', repo_obj.pk,
                                   'null', json.dumps(raw_repo))
 
-        return Response(status=status.HTTP_200_OK, data=copy)
+        return Response(status=status.HTTP_200_OK, data=new_repos.data)
 
 
 class RepoFamilyViewSet(StrictQueryParamMixin,