-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Fix #8926: ListSerializer supports instance access during validation for many=True #9879
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
base: main
Are you sure you want to change the base?
Changes from 22 commits
f0375ca
ac82e50
07de4b8
c402a57
66b8012
90e1a24
0acf49a
1484520
ef7e976
c665595
b61b472
22caa96
de40cb5
5176e44
c9665dd
83e9965
21417c8
781cf7e
5bdf57e
f08921e
bb6b3bb
dcb4ad1
2fa19ed
dd07e02
2a84879
7ce5b8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| /env/ | ||
| MANIFEST | ||
| coverage.* | ||
| venv/ | ||
| .coverage | ||
| .cache/ | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -664,7 +664,31 @@ def run_child_validation(self, data): | |
| self.child.initial_data = data | ||
| return super().run_child_validation(data) | ||
| """ | ||
| return self.child.run_validation(data) | ||
| if not hasattr(self.child, 'instance'): | ||
| return self.child.run_validation(data) | ||
|
|
||
| original_instance = self.child.instance | ||
| had_initial_data = hasattr(self.child, 'initial_data') | ||
| original_initial_data = getattr(self.child, 'initial_data', None) | ||
|
|
||
| try: | ||
| if ( | ||
| hasattr(self, '_list_serializer_instance_map') and | ||
| isinstance(data, Mapping) | ||
| ): | ||
| data_pk = data.get('id') or data.get('pk') | ||
| self.child.instance = (self._list_serializer_instance_map.get(str(data_pk)) | ||
| if data_pk is not None else None) | ||
|
|
||
| self.child.initial_data = data | ||
| return self.child.run_validation(data) | ||
| finally: | ||
| self.child.instance = original_instance | ||
|
zainnadeem786 marked this conversation as resolved.
|
||
| if had_initial_data: | ||
| self.child.initial_data = original_initial_data | ||
| else: | ||
| if hasattr(self.child, 'initial_data'): | ||
| del self.child.initial_data | ||
|
|
||
| def to_internal_value(self, data): | ||
| """ | ||
|
|
@@ -674,12 +698,16 @@ def to_internal_value(self, data): | |
| data = html.parse_html_list(data, default=[]) | ||
|
|
||
| if not isinstance(data, list): | ||
| message = self.error_messages['not_a_list'].format( | ||
| input_type=type(data).__name__ | ||
| ) | ||
| raise ValidationError({ | ||
| api_settings.NON_FIELD_ERRORS_KEY: [message] | ||
| }, code='not_a_list') | ||
| api_settings.NON_FIELD_ERRORS_KEY: [ | ||
| ErrorDetail( | ||
| self.error_messages['not_a_list'].format( | ||
| input_type=type(data).__name__ | ||
| ), | ||
| code='not_a_list' | ||
| ) | ||
| ] | ||
| }) | ||
|
Comment on lines
-677
to
+715
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why this was changed? It looks a bit out of scope but I'm probably missing something... |
||
|
|
||
| if not self.allow_empty and len(data) == 0: | ||
| message = self.error_messages['empty'] | ||
|
|
@@ -702,19 +730,39 @@ def to_internal_value(self, data): | |
| ret = [] | ||
| errors = [] | ||
|
|
||
| for item in data: | ||
| try: | ||
| validated = self.run_child_validation(item) | ||
| except ValidationError as exc: | ||
| errors.append(exc.detail) | ||
| else: | ||
| ret.append(validated) | ||
| errors.append({}) | ||
| # Build a primary key lookup for instance matching in many=True updates. | ||
| instance_map = {} | ||
| if self.instance is not None: | ||
| if isinstance(self.instance, Mapping): | ||
| instance_map = {str(k): v for k, v in self.instance.items()} | ||
|
zainnadeem786 marked this conversation as resolved.
|
||
| elif isinstance(self.instance, (list, tuple, models.query.QuerySet)): | ||
| for obj in self.instance: | ||
| pk = getattr(obj, 'pk', getattr(obj, 'id', None)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again here on testing |
||
| if pk is not None: | ||
| key = str(pk) | ||
| # If duplicate keys are present, keep the last value, | ||
| # matching standard mapping assignment behavior. | ||
| instance_map[key] = obj | ||
|
|
||
| self._list_serializer_instance_map = instance_map | ||
|
|
||
| if any(errors): | ||
| raise ValidationError(errors) | ||
| try: | ||
| for item in data: | ||
| try: | ||
| validated = self.run_child_validation(item) | ||
| except ValidationError as exc: | ||
| errors.append(exc.detail) | ||
| else: | ||
| ret.append(validated) | ||
| errors.append({}) | ||
|
|
||
| return ret | ||
| if any(errors): | ||
| raise ValidationError(errors) | ||
|
|
||
| return ret | ||
| finally: | ||
| if hasattr(self, '_list_serializer_instance_map'): | ||
| delattr(self, '_list_serializer_instance_map') | ||
|
|
||
| def to_representation(self, data): | ||
| """ | ||
|
|
@@ -749,16 +797,27 @@ def save(self, **kwargs): | |
| """ | ||
| Save and return a list of object instances. | ||
| """ | ||
| assert hasattr(self, '_errors'), ( | ||
| 'You must call `.is_valid()` before calling `.save()`.' | ||
| ) | ||
| assert not self.errors, ( | ||
| 'You cannot call `.save()` on a serializer with invalid data.' | ||
| ) | ||
|
zainnadeem786 marked this conversation as resolved.
|
||
|
|
||
| # Guard against incorrect use of `serializer.save(commit=False)` | ||
| assert 'commit' not in kwargs, ( | ||
| "'commit' is not a valid keyword argument to the 'save()' method. " | ||
| "If you need to access data before committing to the database then " | ||
| "inspect 'serializer.validated_data' instead. " | ||
| "You can also pass additional keyword arguments to 'save()' if you " | ||
| "need to set extra attributes on the saved model instance. " | ||
| "For example: 'serializer.save(owner=request.user)'.'" | ||
| "For example: 'serializer.save(owner=request.user)'." | ||
| ) | ||
| assert not hasattr(self, '_data'), ( | ||
| "You cannot call `.save()` after accessing `serializer.data`." | ||
|
zainnadeem786 marked this conversation as resolved.
zainnadeem786 marked this conversation as resolved.
|
||
| "If you need to access data before committing to the database then " | ||
| "inspect 'serializer.validated_data' instead. " | ||
| ) | ||
|
zainnadeem786 marked this conversation as resolved.
|
||
|
|
||
| validated_data = [ | ||
| {**attrs, **kwargs} for attrs in self.validated_data | ||
| ] | ||
|
|
||
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 test covers the
"id"case, but not the"pk"case. Also, what happens if the unique identifier field is called something else in the data, e.g.uuid?Can we make the unique identifier field configurable by the user? I can see how the framework should infer the most common field names, but realistically we won't be to handle all cases and users should have an escape hatch to tweak it