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.

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

Fix validation for ListSerializer #8979

Merged
merged 20 commits into from
May 29, 2023

Conversation

saadullahaleem
Copy link
Contributor

Description

This PR fixes #8926. It makes the instance variable of child serializer point to the correct list object instead of the entire list when validating ListSerializer.

Problem

As mentioned in #8926, when calling is_valid on a serializer that has been initialized with many=True, the instance variable within any validate_ method points to the list of multiple model instances instead of the relevant single model instance.

Solution

When a serializer is initialized with many=True, the returned serializer is actually an instance of ListSerializer with the intended serializer set as the child of the ListSerializer. This PR dynamically sets the correct instance on the child serializer when iterating over all provided objects in the data list.

If both data and instance are passed to a serializer with many=True, the serializer raises an assertion error if their lengths are different.

Assumptions

It is assumed that the objects within data and instance correspond to each other based on the index.

Further Work

We could also add the ability to pass a key or field value that matches objects within data and instance. Moreover, the API would be clearer if instance was actually renamed to instances but that might cause backwards compatibility issues.

@auvipy auvipy self-requested a review May 11, 2023 03:40
@saadullahaleem saadullahaleem marked this pull request as ready for review May 13, 2023 10:00
@saadullahaleem saadullahaleem changed the title [WIP] Fix validation for ListSerializer Fix validation for ListSerializer May 13, 2023
deronnax and others added 16 commits May 27, 2023 14:21
…code#8986)

* Fix Django Docs url in reverse.md

Django URLs of the documentation of `reverse` and `reverse_lazy` were wrong.

* Update reverse.md
* fix URLPathVersioning reverse fallback

* add test for URLPathVersioning reverse fallback

* Update tests/test_versioning.py

---------

Co-authored-by: Jorn van Wier <[email protected]>
Co-authored-by: Asif Saif Uddin <[email protected]>
* Make set_value a static method for Serializers

As an alternative to encode#7671, let the method be overridden if needed. As
the function is only used for serializers, it has a better place in the
Serializer class.

* Set `set_value` as an object (non-static) method

* Add tests for set_value()

These tests follow the examples given in the method.
…ect list object instead of the entire list when validating ListSerializer
* Make set_value a static method for Serializers

As an alternative to encode#7671, let the method be overridden if needed. As
the function is only used for serializers, it has a better place in the
Serializer class.

* Set `set_value` as an object (non-static) method

* Add tests for set_value()

These tests follow the examples given in the method.
…ect list object instead of the entire list when validating ListSerializer
…ect list object instead of the entire list when validating ListSerializer
…ect list object instead of the entire list when validating ListSerializer
@auvipy auvipy added this to the 3.15 milestone May 28, 2023
@auvipy auvipy added the Bug label May 29, 2023
@auvipy auvipy merged commit e2a4559 into encode:master May 29, 2023
Comment on lines +612 to +617

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

Copy link

@felipediel felipediel Dec 26, 2023

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?

Choose a reason for hiding this comment

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

@auvipy sure! #9244

@auvipy
Copy link
Member

auvipy commented Jan 29, 2024

@saadullahaleem @sshishov we got a PR here #9244, suggesting a better design choice. although not complete, we might need to improve the current implementation instead and make sure it is flexible. otherwise might be we have to revert it for a while

@sevdog sevdog mentioned this pull request Mar 12, 2024
auvipy added a commit that referenced this pull request Mar 13, 2024
tomchristie pushed a commit that referenced this pull request Mar 13, 2024
@sshishov
Copy link
Contributor

@saadullahaleem @sshishov we got a PR here #9244, suggesting a better design choice. although not complete, we might need to improve the current implementation instead and make sure it is flexible. otherwise might be we have to revert it for a while

@auvipy I am okay with any route/solution. Our goal is to improve the functionality of the package and if there is better design then we should try to use/apply it.

The approach mentioned in #9244 PR used for bulk actions is valid, we are also using similar approach to create/update objects inside same PATCH request.

In the original issue it wa proposed to use key argument, but I do not like it as you have to "refetch" objects from database when actually you already have them here during serializer creation. We have to map somehow objects "instances" and data, maybe by providing extra keyword argument like key, I do not know...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid self.instance when validating the serializer using many=True
9 participants