Fix validation in AttributeSerializer (cvat-ai#6447)

The current implementation of `to_internal_value` doesn't call the base class method, which is supposed to carry out generic validation of serializer fields. This means that we essentially don't validate attribute fields beyond what is done incidentally by database contraints. Fix it by removing the overridden method entirely, and adding a special field class for the `values` field.
retailnext · Oct 25, 2023 · d3141f5 · d3141f5
1 parent dc882a2
commit d3141f5
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -32,6 +32,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fixed SAM plugin (403 code for workers in organizations) (<https://github.com/opencv/cvat/pull/6514>)
 - Using initial frame from query parameter to open specific frame in a job
  (<https://github.com/opencv/cvat/pull/6506>)
+- Server-side validation for attribute specifications
+  (<https://github.com/opencv/cvat/pull/6447>)
 
 ### Security
 - TDB

diff --git a/cvat/apps/dataset_manager/tests/test_formats.py b/cvat/apps/dataset_manager/tests/test_formats.py
@@ -259,7 +259,8 @@ def _generate_task(self, images, **overrides):
                             "name": "parked",
                             "mutable": True,
                             "input_type": "checkbox",
-                            "default_value": False
+                            "default_value": "false",
+                            "values": [],
                         },
                     ]
                 },
@@ -561,7 +562,8 @@ def _generate_task(self, images):
                             "name": "parked",
                             "mutable": True,
                             "input_type": "checkbox",
-                            "default_value": False
+                            "default_value": "false",
+                            "values": [],
                         },
                     ]
                 },
@@ -723,7 +725,8 @@ def _generate_task(self, images, annotation_format, **overrides):
                             "name": "parked",
                             "mutable": True,
                             "input_type": "checkbox",
-                            "default_value": False
+                            "default_value": "false",
+                            "values": [],
                         }
                     ]
                 },

diff --git a/cvat/apps/engine/serializers.py b/cvat/apps/engine/serializers.py
@@ -212,30 +212,22 @@ class Meta:
             'last_login': { 'allow_null': True }
         }
 
+class DelimitedStringListField(serializers.ListField):
+    def to_representation(self, value):
+        return super().to_representation(value.split('\n'))
+
+    def to_internal_value(self, data):
+        return '\n'.join(super().to_internal_value(data))
+
 class AttributeSerializer(serializers.ModelSerializer):
-    values = serializers.ListField(allow_empty=True,
+    values = DelimitedStringListField(allow_empty=True,
         child=serializers.CharField(allow_blank=True, max_length=200),
     )
 
     class Meta:
         model = models.AttributeSpec
         fields = ('id', 'name', 'mutable', 'input_type', 'default_value', 'values')
 
-    # pylint: disable=no-self-use
-    def to_internal_value(self, data):
-        attribute = data.copy()
-        attribute['values'] = '\n'.join(data.get('values', []))
-        return attribute
-
-    def to_representation(self, instance):
-        if instance:
-            rep = super().to_representation(instance)
-            rep['values'] = instance.values.split('\n')
-        else:
-            rep = instance
-
-        return rep
-
 class SublabelSerializer(serializers.ModelSerializer):
     id = serializers.IntegerField(required=False)
     attributes = AttributeSerializer(many=True, source='attributespec_set', default=[],

diff --git a/cvat/apps/engine/tests/test_rest_api.py b/cvat/apps/engine/tests/test_rest_api.py
@@ -1811,7 +1811,8 @@ def _create_project(project_data):
                             "name": "bool_attribute",
                             "mutable": True,
                             "input_type": AttributeType.CHECKBOX,
-                            "default_value": "true"
+                            "default_value": "true",
+                            "values": [],
                         }],
                     }, {
                         "name": "person",
@@ -2572,7 +2573,8 @@ def test_api_v2_tasks_admin(self):
                     "name": "my_attribute",
                     "mutable": True,
                     "input_type": AttributeType.CHECKBOX,
-                    "default_value": "true"
+                    "default_value": "true",
+                    "values": [],
                 }]
             }]
         }
@@ -2895,7 +2897,8 @@ def _create_task(task_data, media_data):
                         "name": "bool_attribute",
                         "mutable": True,
                         "input_type": AttributeType.CHECKBOX,
-                        "default_value": "true"
+                        "default_value": "true",
+                        "values": [],
                     }],
                     }, {
                         "name": "person",
@@ -2915,7 +2918,8 @@ def _create_task(task_data, media_data):
                         "name": "bool_attribute",
                         "mutable": True,
                         "input_type": AttributeType.CHECKBOX,
-                        "default_value": "true"
+                        "default_value": "true",
+                        "values": [],
                     }],
                     }, {
                         "name": "person",
@@ -4649,7 +4653,8 @@ def _create_task(self, owner, assignee, annotation_format=""):
                             "name": "parked",
                             "mutable": True,
                             "input_type": "checkbox",
-                            "default_value": "false"
+                            "default_value": "false",
+                            "values": [],
                         },
                     ]
                 },

diff --git a/cvat/apps/lambda_manager/tests/assets/tasks.json b/cvat/apps/lambda_manager/tests/assets/tasks.json
@@ -18,7 +18,8 @@
             "name": "parked",
             "mutable": true,
             "input_type": "checkbox",
-            "default_value": false
+            "default_value": "false",
+            "values": []
           }
         ]
       },
@@ -47,7 +48,8 @@
             "name": "parked",
             "mutable": true,
             "input_type": "checkbox",
-            "default_value": false
+            "default_value": "false",
+            "values": []
           }
         ]
       },