Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix validation for ListSerializer #8979

Merged
merged 20 commits into from
May 29, 2023
Merged

Fix validation for ListSerializer #8979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
a717d74
fix: Make the instance variable of child serializer point to the corr…
saadullahaleem May 10, 2023
4bbc689
fix formatting issues for list serializer validation fix
saadullahaleem May 10, 2023
516ba52
fix imports sorting for list serializer tests
saadullahaleem May 10, 2023
b9c8d19
remove django 2.2 from docs index (#8982)
deronnax May 14, 2023
e6655e3
Declared Django 4.2 support in README.md (#8985)
MehrazRumman May 15, 2023
01b99dd
Fix Links in Documentation to Django `reverse` and `reverse_lazy` (#8…
theomega May 17, 2023
8d7e250
fix URLPathVersioning reverse fallback (#7247)
jornvanwier May 18, 2023
b9b50bd
Make set_value a method within `Serializer` (#8001)
tienne-B May 24, 2023
869d46f
fix: Make the instance variable of child serializer point to the corr…
saadullahaleem May 10, 2023
f969143
Make set_value a method within `Serializer` (#8001)
tienne-B May 24, 2023
a1f03d5
fix: Make the instance variable of child serializer point to the corr…
saadullahaleem May 10, 2023
9ac6417
fix: Make the instance variable of child serializer point to the corr…
saadullahaleem May 10, 2023
76110bf
fix formatting issues for list serializer validation fix
saadullahaleem May 10, 2023
c1ee7e7
fix: Make the instance variable of child serializer point to the corr…
saadullahaleem May 10, 2023
74cb500
fix formatting issues for list serializer validation fix
saadullahaleem May 10, 2023
9577880
Merge branch 'master' into fix/list_serializer_validation
saadullahaleem May 27, 2023
7197a86
fix linting
saadullahaleem May 27, 2023
bb4102a
Update rest_framework/serializers.py
auvipy May 27, 2023
0dc6201
Update rest_framework/serializers.py
saadullahaleem May 27, 2023
d747449
fix: instance variable in list serializer, remove commented code
saadullahaleem May 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion rest_framework/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,12 @@ def __init__(self, *args, **kwargs):
self.min_length = kwargs.pop('min_length', None)
assert self.child is not None, '`child` is a required argument.'
assert not inspect.isclass(self.child), '`child` has not been instantiated.'

instance = kwargs.get('instance', [])
data = kwargs.get('data', [])
if instance and data:
assert len(data) == len(instance), 'Data and instance should have same length'

Comment on lines +612 to +617
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lengths might be different in some use cases. Let's say we want the list serializer to delete from queryset all elements that are not provided in the data during an update. The queryset will be bigger than the data in this case.

This update breaks serializers like:

class BankBulkUpdateSerializer(serializers.ListSerializer):
    """Bank bulk update serializer."""

    def update(
        self,
        instance: models.QuerySet[Bank],
        validated_data: list[OrderedDict],
    ) -> list[Bank]:
        """Bulk update banks."""
        bank_dict = {bank.pk: bank for bank in instance}
        banks_to_create: list[Bank] = []
        banks_to_update: list[Bank] = []
        banks_created: list[Bank] = []
        banks_updated: list[Bank] = []
        bank_pks_to_keep: list[int] = []

        for bank_data in validated_data:
            bank_id = bank_data.get("id")
            bank = bank_dict.get(bank_id) if bank_id is not None else None

            if bank is not None:
                for attr, value in bank_data.items():
                    setattr(bank, attr, value)
                banks_to_update.append(bank)
                bank_pks_to_keep.append(bank.pk)

            else:
                bank = Bank(**bank_data)
                banks_to_create.append(bank)

        with db_transaction.atomic():
            instance.exclude(pk__in=bank_pks_to_keep).delete()

            if banks_to_update:
                update_fields = ["name", "compe", "ispb", "website"]
                Bank.objects.bulk_update(banks_to_update, update_fields)
                banks_updated = banks_to_update

            if banks_to_create:
                banks_created: list[Bank] = Bank.objects.bulk_create(
                    banks_to_create
                )

        return sorted(banks_created + banks_updated, key=lambda a: a.name)

Instead of indexing, it's better to relate the elements by id:

self.child.instance = self.instance.filter(id=item['id']).first() if self.instance else None
self.child.initial_data = item
validated = self.child.run_validation(item)

We could add a "pk_field" to the serializer Meta to make it more flexible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you mind come with an improvement in another PR and ping me?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@auvipy sure! #9244

super().__init__(*args, **kwargs)
self.child.bind(field_name='', parent=self)

Expand Down Expand Up @@ -683,7 +689,13 @@ def to_internal_value(self, data):
ret = []
errors = []

for item in data:
for idx, item in enumerate(data):
if (
hasattr(self, 'instance')
and self.instance
and len(self.instance) > idx
):
self.child.instance = self.instance[idx]
try:
validated = self.child.run_validation(item)
except ValidationError as exc:
Expand Down
61 changes: 61 additions & 0 deletions tests/test_serializer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import pickle
import re
import sys
import unittest
from collections import ChainMap
from collections.abc import Mapping

Expand Down Expand Up @@ -783,3 +784,63 @@ def test_nested_key(self):
ret = {'a': 1}
self.s.set_value(ret, ['x', 'y'], 2)
assert ret == {'a': 1, 'x': {'y': 2}}


class MyClass(models.Model):
name = models.CharField(max_length=100)
value = models.CharField(max_length=100, blank=True)

app_label = "test"

@property
def is_valid(self):
return self.name == 'valid'


class MyClassSerializer(serializers.ModelSerializer):
class Meta:
model = MyClass
fields = ('id', 'name', 'value')

def validate_value(self, value):
if value and not self.instance.is_valid:
raise serializers.ValidationError(
'Status cannot be set for invalid instance')
return value


class TestMultipleObjectsValidation(unittest.TestCase):
def setUp(self):
self.objs = [
MyClass(name='valid'),
MyClass(name='invalid'),
MyClass(name='other'),
]

def test_multiple_objects_are_validated_separately(self):

serializer = MyClassSerializer(
data=[{'value': 'set', 'id': instance.id} for instance in
self.objs],
instance=self.objs,
many=True,
partial=True,
)

assert not serializer.is_valid()
assert serializer.errors == [
{},
{'value': ['Status cannot be set for invalid instance']},
{'value': ['Status cannot be set for invalid instance']}
]

def test_exception_raised_when_data_and_instance_length_different(self):

with self.assertRaises(AssertionError):
MyClassSerializer(
data=[{'value': 'set', 'id': instance.id} for instance in
self.objs],
instance=self.objs[:-1],
many=True,
partial=True,
)