Fix DictField HTML input returning empty dict for missing fields#9891
Fix DictField HTML input returning empty dict for missing fields#9891veeceey wants to merge 11 commits intoencode:mainfrom
Conversation
…input (encode#6234) When using DictField with HTML form (multipart/form-data) input, parse_html_dict always returned an empty MultiValueDict when no matching keys were found. This made it impossible to distinguish between an unspecified field and an empty input, causing issues with required/default field handling. This aligns parse_html_dict with parse_html_list by adding a default parameter that is returned when no matching keys are found. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Hi maintainers, friendly ping on this PR. It's been open for about 10 days without any review activity. Would really appreciate any feedback or direction when you get a chance. Happy to make adjustments if needed. Thank you! |
rest_framework/fields.py
Outdated
| """ | ||
| if html.is_html_input(data): | ||
| data = html.parse_html_dict(data) | ||
| data = html.parse_html_dict(data, default=data) |
There was a problem hiding this comment.
Wouldn't a better default be an empty dict here?
| data = html.parse_html_dict(data, default=data) | |
| data = html.parse_html_dict(data, default={}) |
There was a problem hiding this comment.
I tried this but it actually breaks test_querydict_dict_input. When to_internal_value gets called, the data is already a MultiValueDict with parsed keys. parse_html_dict with no prefix looks for dot-prefixed keys which don't match, so default={} would lose the already-parsed data. Keeping default=data as a fallback preserves it correctly. Happy to discuss further though!
|
|
||
|
|
||
| def parse_html_dict(dictionary, prefix=''): | ||
| def parse_html_dict(dictionary, prefix='', default=None): |
There was a problem hiding this comment.
Looks consistent with what we do for parsing html list 👍🏻 :
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where DictField could not distinguish between a field that was not specified in HTML form data versus a field that was specified but had no values. This caused issues with required fields, default values, and partial updates when using multipart/form-data requests.
Changes:
- Added a
defaultparameter toparse_html_dict()to return a specified value when no matching keys are found (consistent withparse_html_list) - Updated
DictField.get_value()andSerializer.get_value()to passdefault=emptyso missing fields are correctly detected - Updated
DictField.to_internal_value()to passdefault=datato preserve inner MultiValueDict data when no nested keys exist - Added comprehensive test coverage for DictField HTML form input scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rest_framework/utils/html.py | Added default parameter to parse_html_dict() to handle missing fields correctly |
| rest_framework/fields.py | Updated DictField.get_value() and to_internal_value() to use the new default parameter |
| rest_framework/serializers.py | Updated Serializer.get_value() to use explicit default=empty parameter for consistency |
| tests/test_fields.py | Added 4 test cases covering various DictField HTML form input scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for the review! I'll address all three points:
Pushing the fixes now. |
- Add regression test for nested serializer with QueryDict to cover
the serializers.py get_value change (test_nested_serializer_not_required_with_querydict)
- Use `assert serializer.is_valid(), serializer.errors` for better
test output on failures
- Keep `default=data` in to_internal_value as `default={}` would
discard already-parsed MultiValueDict keys from get_value
|
Hey @browniebroke, pushed the updates:
|
Change default from data to {} in parse_html_dict call within
DictField.to_internal_value(), falling back to a dict conversion
of the MultiValueDict when no dot-separated keys are found. This
is cleaner than using the input data as its own default.
|
Hey @browniebroke, circling back on the default={} suggestion — I ended up finding a way to make it work after all. Pushed a commit that uses |
browniebroke
left a comment
There was a problem hiding this comment.
The scope of this PR is doing more than fixing the bug reported in #6234, it is actually improving support for nested objects in HTML forms which was requested a long time ago.
I'm not convinced this is worth doing in terms of risking a regression but will keep this open to revisit in a while
|
I was leaning more towards #9906 because the fix was more targeted, but after more careful review I can see some limits to that other simpler approach, which has some regression. Although this one seemingly has the same issue, how do one clears a value from a def test_partial_update_can_clear_html_dict_field(self):
class TestSerializer(serializers.Serializer):
field_name = serializers.DictField(required=False)
serializer = TestSerializer(data=QueryDict('field_name='), partial=True)
assert serializer.is_valid()
assert 'field_name' in serializer.validated_data |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def parse_html_dict(dictionary, prefix='', default=None): | ||
| """ |
There was a problem hiding this comment.
parse_html_dict now returns default when no matching keys are found, and since default defaults to None this changes the previous behavior of returning an empty MultiValueDict() for missing prefixes. If any external callers rely on the old behavior, they may now get None and fail unexpectedly; consider keeping backward-compatible behavior (e.g., use a sentinel to detect “no default provided” and return an empty MultiValueDict in that case) or explicitly documenting this as a public API change.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
Fixes #6234
parse_html_dictalways returned an emptyMultiValueDictwhen no matching keys were found in the HTML form input, making it impossible to distinguish between:This caused
DictFieldto treat missing fields as if they contained an empty dict, breakingrequired,default, and partial update logic formultipart/form-datarequests.Changes
rest_framework/utils/html.py: Added adefaultparameter toparse_html_dict(consistent with howparse_html_listalready works). Returnsdefaultwhen no matching keys are found in the input.rest_framework/fields.py: UpdatedDictField.get_valueto passdefault=emptyso that missing fields are correctly treated as not provided. Also updatedDictField.to_internal_valueto passdefault=dataso inner MultiValueDict data is preserved when no dot-separated sub-keys are found.rest_framework/serializers.py: UpdatedSerializer.get_valueto use the newdefault=parameter instead ofor emptyfor consistency.tests/test_fields.py: Added 4 new test cases for DictField HTML form input covering:Test plan
test_fields.py,test_serializer.py, andtest_parsers.pysuites pass (401 passed, 1 pre-existing failure unrelated to this change)multipart/form-datarequests that DictField partial updates work correctly