Do not treat missing non-form data as empty dict#7199
Do not treat missing non-form data as empty dict#7199peterthomassen wants to merge 2 commits intoencode:mainfrom
Conversation
|
|
||
| Default: `'rest_framework.schemas.openapi.AutoSchema'` | ||
|
|
||
| #### DEFAULT_MISSING_DATA |
There was a problem hiding this comment.
Maybe let's call this something like EMPTY_REQUEST_DATA?
The DEFAULT_XYZ cases make sense because they're things that can be overridden, and are populating the default value. In this case it's not a default case that can be overridden, but rather the base case that it always used if there's an empty request.
There was a problem hiding this comment.
Happy to take feedback on alternate naming choices, too.
There was a problem hiding this comment.
I chose "missing" as opposed to "empty" because conventional usage of the word "empty" would suggest that a request body of "", [] or {} is also empty (len(request.data) == 0 after deserialization). I feared that if we call it "EMPTY_DATA", one may think that any "empty-like" payload would be coerced into this value. -- However, I don't feel strongly, as long as the documentation is clear.
Regarding whether it is a default: If by "override", you mean overriding by view attribute or similar, fair enough; one could also argue that the client can override it (by sending data), which would warrant the name DEFAULT_DATA or DEFAULT_REQUEST_DATA.
(Also, it's conceivable that there would a view attribute to override this setting at some point, although right now the request would not have the corresponding context.)
Having said all this, I think my favorite would be DEFAULT_REQUEST_DATA, as I think it is accurate and future-proof. But in the end, I don't have any stakes in the naming, and will be ok with whatever you prefer.
|
@tomchristie Do you need anything else from me to take the next step with this? |
This allows views to distinguish missing payload from empty payload. Related: encode#3647, encode#4566
c26eb31 to
8b52dc0
Compare
|
@tomchristie I rebased this PR on top the the current master and resolved conflicts. Is there anything else I can do to help with this PR? |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
I think this is still current. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
I think this is still current. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
I think this is still current. |
|
@auvipy Do you think this can be revived? |
|
will come back to this next sun/monday |
|
I'm not convinced that adding new API surface area to REST framework is a net positive. |
| empty_data = QueryDict('', encoding=self._request._encoding) | ||
| else: | ||
| empty_data = {} | ||
| empty_data = api_settings.DEFAULT_MISSING_DATA |
There was a problem hiding this comment.
instead of exposing a new API can we just handle the empty data as None value instead of dict?
There was a problem hiding this comment.
That was my initial proposal, and I have no objection to doing so. However, please see the discussion in #7195 (comment) (and the context that led up to this post).
There was a problem hiding this comment.
what I am suggesting you is based on this comment #7199 (comment) as Tom is reluctant to introduce new API surface to the framework. I saw those discussions as well and do agree with most of them.
There was a problem hiding this comment.
Yes, but Tom was even more reluctant to silently change the behavior ("too big of a behavior change" in this earlier comment). He seemed to be more ok with adding the setting, see #7195 (comment).
I have no stakes, and either way is fine for me. It's just think that this question needs clarification between Tom and you, and I can't answer it.
There was a problem hiding this comment.
should we try empty_data = {} or None? and get rid of a new settings?
There was a problem hiding this comment.
I don't understand how we could "try" empty_data = {} as that is the current situation. As for trying None, that breaks existing tests, which is a non-starter.
Tom has already given his input and agreed to the above code change, see his thumbs-up on #7195 (comment) and the next comment, #7195 (comment).
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the handling of missing non-form data so that it is not treated as an empty dict but instead uses a configurable DEFAULT_MISSING_DATA setting. Key changes include:
- Adding a serializer flag (allow_null) based on query parameters to alter behavior in the view.
- Updating tests to assert the correct response when no data is provided.
- Introducing the DEFAULT_MISSING_DATA setting in both the settings and request parsing layers, with corresponding documentation updates.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_testing.py | Updates to test cases for missing payload behavior and view configuration. |
| tests/test_request.py | Adjusted expectations for request.data to reflect missing data as None. |
| rest_framework/settings.py | Introduction of DEFAULT_MISSING_DATA with a default value of None. |
| rest_framework/request.py | Use of DEFAULT_MISSING_DATA for non-form missing payloads. |
| docs/api-guide/settings.md | Documentation update for DEFAULT_MISSING_DATA setting. |
| docs/api-guide/requests.md | Updated documentation to explain request.data behavior when no payload is sent. |
Comments suppressed due to low confidence (1)
tests/test_testing.py:213
- Consider adding explicit assertions in the test_missing_post_payload_coerced_dict_uses_default_boolean_value test case to verify that when DEFAULT_MISSING_DATA is overridden to an empty dict, the response data exactly equals {}.
@override_settings(REST_FRAMEWORK={'DEFAULT_MISSING_DATA': {}})
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@auvipy Any follow-up after the review? Thanks! |
| ) | ||
|
|
||
| def test_empty_post_uses_default_boolean_value(self): | ||
| def test_missing_post_payload_causes_400(self): |
There was a problem hiding this comment.
I prefer - def test_missing_post_payload_causes_four_hundred(self): here or equivalent
https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/400
This allows views to distinguish missing payload from empty payload.
Related: #3647, #4566
Description
continuation of #7195