Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
Fix validation for ListSerializer#8979
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fix validation for ListSerializer#8979
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…ect list object instead of the entire list when validating ListSerializer
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
…code#8986)* Fix Django Docs url in reverse.mdDjango 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 <jorn.van.wier@thunderbyte.ai>Co-authored-by: Asif Saif Uddin <auvipy@gmail.com>
* Make set_value a static method for SerializersAs an alternative toencode#7671, let the method be overridden if needed. Asthe function is only used for serializers, it has a better place in theSerializer 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 SerializersAs an alternative toencode#7671, let the method be overridden if needed. Asthe function is only used for serializers, it has a better place in theSerializer 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
Co-authored-by: Sergei Shishov <sshishov@users.noreply.github.com>
Co-authored-by: Sergei Shishov <sshishov@users.noreply.github.com>
| instance=kwargs.get('instance', []) | ||
| data=kwargs.get('data', []) | ||
| ifinstanceanddata: | ||
| assertlen(data)==len(instance),'Data and instance should have same length' | ||
felipedielDec 26, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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:
classBankBulkUpdateSerializer(serializers.ListSerializer):"""Bank bulk update serializer."""defupdate(self,instance:models.QuerySet[Bank],validated_data:list[OrderedDict], )->list[Bank]:"""Bulk update banks."""bank_dict= {bank.pk:bankforbankininstance}banks_to_create:list[Bank]= []banks_to_update:list[Bank]= []banks_created:list[Bank]= []banks_updated:list[Bank]= []bank_pks_to_keep:list[int]= []forbank_datainvalidated_data:bank_id=bank_data.get("id")bank=bank_dict.get(bank_id)ifbank_idisnotNoneelseNoneifbankisnotNone:forattr,valueinbank_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)withdb_transaction.atomic():instance.exclude(pk__in=bank_pks_to_keep).delete()ifbanks_to_update:update_fields= ["name","compe","ispb","website"]Bank.objects.bulk_update(banks_to_update,update_fields)banks_updated=banks_to_updateifbanks_to_create:banks_created:list[Bank]=Bank.objects.bulk_create(banks_to_create )returnsorted(banks_created+banks_updated,key=lambdaa:a.name)
Instead of indexing, it's better to relate the elements by id:
self.child.instance=self.instance.filter(id=item['id']).first()ifself.instanceelseNoneself.child.initial_data=itemvalidated=self.child.run_validation(item)
We could add a "pk_field" to the serializer Meta to make it more flexible.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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 |
Description
This PRfixes#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_validon a serializer that has been initialized withmany=True, theinstancevariable within anyvalidate_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 ofListSerializerwith the intended serializer set as thechildof theListSerializer. This PR dynamically sets the correctinstanceon the child serializer when iterating over all provided objects in thedatalist.If both
dataandinstanceare passed to a serializer withmany=True, the serializer raises an assertion error if their lengths are different.Assumptions
It is assumed that the objects within
dataandinstancecorrespond to each other based on the index.Further Work
We could also add the ability to pass a
keyorfieldvalue that matches objects withindataandinstance. Moreover, the API would be clearer ifinstancewas actually renamed toinstancesbut that might cause backwards compatibility issues.