ref(rest-framework): Fixing Bugs in ListField. (#11830)

* writing more tests for this class.

* Fixed list behavior.

* Added small modifications that affected commitpatch.

src/sentry/api/endpoints/project_releases.py

@@ -34,7 +34,11 @@ class CommitPatchSetSerializer(serializers.Serializer):
 
 
 class CommitSerializerWithPatchSet(CommitSerializer):
-    patch_set = ListField(child=CommitPatchSetSerializer(), required=False, allow_null=False)
+    patch_set = ListField(
+        child=CommitPatchSetSerializer(
+            required=False),
+        required=False,
+        allow_null=True)
 
 
 class ReleaseSerializer(serializers.Serializer):
@@ -43,7 +47,11 @@ class ReleaseSerializer(serializers.Serializer):
     url = serializers.URLField(required=False)
     owner = UserField(required=False)
     dateReleased = serializers.DateTimeField(required=False)
-    commits = ListField(child=CommitSerializerWithPatchSet(), required=False, allow_null=False)
+    commits = ListField(
+        child=CommitSerializerWithPatchSet(
+            required=False),
+        required=False,
+        allow_null=True)
 
     def validate_version(self, attrs, source):
         value = attrs[source]

src/sentry/api/serializers/rest_framework/list.py

@@ -2,6 +2,7 @@ from __future__ import absolute_import
 
 import six
 
+from collections import defaultdict
 from rest_framework.serializers import WritableField, ValidationError
 
 
@@ -12,10 +13,13 @@ class ListField(WritableField):
         self.child = child
         self.allow_null = allow_null
         super(ListField, self).__init__(**kwargs)
+        self._child_errors = defaultdict(list)
 
     def initialize(self, parent, field_name):
         super(ListField, self).initialize(parent, field_name)
         if self.child:
+            self.child._errors = []
+            self._child_errors = defaultdict(list)
             self.child.initialize(parent, field_name)
 
     def to_native(self, value):
@@ -35,11 +39,15 @@ class ListField(WritableField):
         if self.child is None:
             return value
 
-        return [self.child.from_native(item) for item in value]
+        children = []
+        for item in value:
+            children.append(self.child.from_native(item))
+            self.add_child_errors()
+        return children
 
     def format_child_errors(self):
         errors = []
-        for k, v in six.iteritems(self.child.errors):
+        for k, v in six.iteritems(self._child_errors):
             errors.append('%s: %s' % (k, v[0]))
         return ', '.join(errors)
 
@@ -56,14 +64,22 @@ class ListField(WritableField):
         if self.child:
             # the `self.child.from_native` call might have already
             # validated/changed data so check for child errors first
-            if self.child._errors:
+            if self._child_errors:
                 raise ValidationError(self.format_child_errors())
             for item in value:
                 if item is None and not self.allow_null:
                     raise ValidationError('Incorrect type. Expected value, but got null')
                 self.child.validate(item)
+                self.add_child_errors()
 
     def run_validators(self, value):
         if self.child:
             for item in value:
                 self.child.run_validators(item)
+                self.add_child_errors()
+
+    def add_child_errors(self):
+        if not isinstance(self.child._errors, dict):
+            return
+        for k, v in six.iteritems(self.child._errors):
+            self._child_errors[k] += v

tests/sentry/api/endpoints/test_project_releases.py

@@ -688,5 +688,5 @@ class ProjectReleaseCreateCommitPatch(ReleaseCommitPatchTest):
         )
 
         assert response.status_code == 400
-        # TODO(lb): What happened to my ValidationError??? this is not as helpful
-        assert response.content == '{"commits": ["patch_set: Incorrect type. Expected value, but got null"]}'
+        assert response.data == {
+            'commits': [u'patch_set: type: Commit patch_set type Z is not supported.']}

tests/sentry/api/serializers/rest_framework/test_list.py

@@ -1,9 +1,9 @@
 from __future__ import absolute_import
-from sentry.api.serializers.rest_framework import ListField
+from sentry.api.serializers.rest_framework import ListField, ValidationError
 
 from rest_framework import serializers
 
-from sentry.testutils import TestCase
+from sentry.testutils import TestCase, APITestCase
 
 
 class ListFieldTest(TestCase):
@@ -23,6 +23,12 @@ class ListFieldTest(TestCase):
         serializer = DummySerializer(data={'list_field': [1, 2, 3]})
         self.assert_success(serializer, {'list_field': [1, 2, 3]})
 
+    def test_simple_invalid(self):
+        class DummySerializer(serializers.Serializer):
+            list_field = ListField(child=serializers.IntegerField())
+        serializer = DummySerializer(data={'list_field': [1, 'q', 3]})
+        self.assert_unsuccessful(serializer, {'list_field': ['Enter a whole number.']})
+
     def test_single_element_list(self):
         class DummySerializer(serializers.Serializer):
             list_field = ListField(child=serializers.IntegerField())
@@ -98,3 +104,89 @@ class ListFieldTest(TestCase):
             list_field = ListField(child=serializers.IntegerField(), required=False)
         serializer = DummySerializer(data={'list_field': [1, 2, 3]})
         self.assert_success(serializer, {'list_field': [1, 2, 3]})
+
+
+class ListFieldMultipleValuesTest(APITestCase):
+    def setUp(self):
+        super(ListFieldMultipleValuesTest, self).setUp()
+
+        class DummyChildSerializer(serializers.Serializer):
+            age = serializers.IntegerField()
+
+            def validate_age(self, attrs, source):
+                age = attrs[source]
+                if age > 5:
+                    raise ValidationError('age %d is not allowed' % age)
+                return attrs
+
+        class DummySerializer(serializers.Serializer):
+            list_field = ListField(
+                required=False,
+                child=DummyChildSerializer(required=False),
+                allow_null=True,
+            )
+            name = serializers.CharField()
+
+        self.serializer_class = DummySerializer
+
+    def test_simple(self):
+        serializer = self.serializer_class(data={
+            'name': 'John',
+            'list_field': [{'age': 200}]
+        })
+        assert not serializer.is_valid()
+
+    def test_incorrect_value_before_correct_value(self):
+        serializer = self.serializer_class(data={
+            'name': 'John',
+            'list_field': [
+                {'age': 200}, {'age': 3}
+            ]
+        })
+        assert not serializer.is_valid()
+        assert serializer._errors == {'list_field': [u'age: age 200 is not allowed']}
+
+    def test_correct_value_before_incorrect_value(self):
+        serializer = self.serializer_class(data={
+            'name': 'John',
+            'list_field': [
+                {'age': 3}, {'age': 200}
+            ]
+        })
+        assert not serializer.is_valid()
+        assert serializer._errors == {'list_field': [u'age: age 200 is not allowed']}
+
+    def test_mulitple_errors_in_child(self):
+        serializer = self.serializer_class(data={
+            'name': 'John',
+            'list_field': [
+                {'age': 3}, {'age': 200}, {'age': 5000}, {'age': 20}, {'age': 1}
+            ]
+        })
+        assert not serializer.is_valid()
+        assert serializer.fields['list_field']._child_errors == {
+            'age': ['age 200 is not allowed', 'age 5000 is not allowed', 'age 20 is not allowed']}
+        assert serializer._errors == {'list_field': ['age: age 200 is not allowed']}
+
+    def test_clears_child_errors_between_use_empty_list(self):
+        serializer = self.serializer_class(data={
+            'name': 'John',
+            'list_field': [{'age': 200}]
+        })
+        assert not serializer.is_valid()
+        serializer = self.serializer_class(data={
+            'name': 'John',
+            'list_field': []
+        })
+        assert serializer.is_valid()
+
+    def test_clears_child_errors_between_use_not_supplied(self):
+        serializer = self.serializer_class(data={
+            'name': 'John',
+            'list_field': [{'age': 200}]
+        })
+        assert not serializer.is_valid()
+        serializer = self.serializer_class(data={
+            'name': 'John',
+        })
+        assert serializer.is_valid()