-
-
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 7 commits
f0375ca
ac82e50
07de4b8
c402a57
66b8012
90e1a24
0acf49a
1484520
ef7e976
c665595
b61b472
22caa96
de40cb5
5176e44
c9665dd
83e9965
21417c8
781cf7e
5bdf57e
f08921e
bb6b3bb
dcb4ad1
2fa19ed
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 |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ | |
| /env/ | ||
| MANIFEST | ||
| coverage.* | ||
|
|
||
| venv/ | ||
| !.github | ||
| !.gitignore | ||
| !.pre-commit-config.yaml | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -312,18 +312,6 @@ def __new__(cls, name, bases, attrs): | |
|
|
||
|
|
||
| def as_serializer_error(exc): | ||
| """ | ||
| Coerce validation exceptions into a standardized serialized error format. | ||
|
|
||
| This function normalizes both Django's `ValidationError` and REST | ||
| framework's `ValidationError` into a dictionary structure compatible | ||
| with serializer `.errors`, ensuring all values are represented as | ||
| lists of error details. | ||
|
|
||
| The returned structure conforms to the serializer error contract: | ||
| - Field-specific errors are returned as '{field-name: [errors]}' | ||
| - Non-field errors are returned under the 'NON_FIELD_ERRORS_KEY' | ||
| """ | ||
| assert isinstance(exc, (ValidationError, DjangoValidationError)) | ||
|
|
||
| if isinstance(exc, DjangoValidationError): | ||
|
|
@@ -664,7 +652,24 @@ 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 | ||
| try: | ||
| if ( | ||
| hasattr(self, '_instance_map') and | ||
| isinstance(data, Mapping) and | ||
| original_instance is self.instance | ||
| ): | ||
| data_pk = data.get('id') | ||
| if data_pk is None: | ||
| data_pk = data.get('pk') | ||
| self.child.instance = self._instance_map.get(str(data_pk)) if data_pk is not None else None | ||
|
|
||
| return self.child.run_validation(data) | ||
| finally: | ||
| self.child.instance = original_instance | ||
zainnadeem786 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def to_internal_value(self, data): | ||
| """ | ||
|
|
@@ -674,12 +679,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
+714
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,17 +711,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()} | ||
|
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. This line isn't covered by tests. Would be nice to come up with a test case covering it... Also, what happens if |
||
| elif isinstance(self.instance, (list, tuple, models.query.QuerySet)): | ||
| for obj in self.instance: | ||
| pk = getattr(obj, 'pk', getattr(obj, 'id', None)) | ||
|
||
| 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._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({}) | ||
|
|
||
| if any(errors): | ||
| raise ValidationError(errors) | ||
|
|
||
| return ret | ||
| finally: | ||
| if hasattr(self, '_instance_map'): | ||
| delattr(self, '_instance_map') | ||
|
|
||
| return ret | ||
zainnadeem786 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
|
|
@@ -749,6 +780,13 @@ 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.' | ||
| ) | ||
|
Comment on lines
+814
to
+819
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. That seems unrelated and lacks test coverage |
||
|
|
||
| # 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. " | ||
|
|
@@ -758,6 +796,14 @@ def save(self, **kwargs): | |
| "need to set extra attributes on the saved model instance. " | ||
| "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.
Show resolved
Hide resolved
|
||
| "If you need to access data before committing to the database then " | ||
| "inspect 'serializer.validated_data' instead. " | ||
| ) | ||
|
Comment on lines
+830
to
834
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. Same here |
||
| assert hasattr(self, '_validated_data'), ( | ||
| 'You must call `.is_valid()` before calling `.save()`.' | ||
| ) | ||
zainnadeem786 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| validated_data = [ | ||
| {**attrs, **kwargs} for attrs in self.validated_data | ||
|
|
@@ -827,29 +873,22 @@ def errors(self): | |
|
|
||
| def raise_errors_on_nested_writes(method_name, serializer, validated_data): | ||
| """ | ||
| Enforce explicit handling of writable nested and dotted-source fields. | ||
|
|
||
| This helper raises clear and actionable errors when a serializer attempts | ||
| to perform writable nested updates or creates using the default | ||
| `ModelSerializer` behavior. | ||
| Give explicit errors when users attempt to pass writable nested data. | ||
browniebroke marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| Writable nested relationships and dotted-source fields are intentionally | ||
| unsupported by default due to ambiguous persistence semantics. Developers | ||
| must either: | ||
| - Override the `.create()` / `.update()` methods explicitly, or | ||
| - Mark nested serializers as `read_only=True` | ||
| If we don't do this explicitly they'd get a less helpful error when | ||
| calling `.save()` on the serializer. | ||
|
|
||
| This check is invoked internally by default `ModelSerializer.create()` | ||
| and `ModelSerializer.update()` implementations. | ||
| We don't *automatically* support these sorts of nested writes because | ||
| there are too many ambiguities to define a default behavior. | ||
|
|
||
| Eg. Suppose we have a `UserSerializer` with a nested profile. How should | ||
| we handle the case of an update, where the `profile` relationship does | ||
| not exist? Any of the following might be valid: | ||
|
|
||
| * Raise an application error. | ||
| * Silently ignore the nested part of the update. | ||
| * Automatically create a profile instance. | ||
| """ | ||
|
|
||
| ModelClass = serializer.Meta.model | ||
| model_field_info = model_meta.get_field_info(ModelClass) | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.