django-rest-framework (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation11 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

cc @D3X, @tomchristie.

Hi Tom, see my comment here for context. It seems like setting request._post in addition to request._files might have been the correct thing to do. Thoughts?

@rpkilby One thing I'm concerned about is that this changes the contract on request._post.

Normally, Django would set it to an empty QueryDict if the Content-Type of the request was not "application/x-www-form-urlencoded" or "multipart/form-data".

With this PR, it gets set to parsed data even for other types, like "application/json". This could result in not-easy-to-track bugs if some middlewares depended on Django's behaviour in this case.

This shouldn't impact middlewares as they are called with Django's request. DRF's views does the wrapping so it's really used "inside" DRF whether core or the application code.

@xordoquy I wrote a short test to show how the interface on request.POST changes when using DRF and posting JSON: D3X@aa6a4c4

I don't know any app that would break because of it, but it is possible some would.

D3X and others added 2 commits

November 14, 2017 20:12

@D3X - thanks, good catch.

D3X approved these changes Nov 15, 2017

@rpkilby This looks fine to me — I'm inclined to just pull it in, but I'm not quite seeing how the commits match the conversation. (Did you rebase? What changed?)

I wrote a short test to show how the interface on request.POST changes...

OK, added, but what was this line in the first version?

     self._request._post = self.POST

Thanks!

@carltongibson - yeah, I amended the commits from earlier in the conversation. Previously was

 self._request._post = self._data
 self._request._files = self._files

@rpkilby OK, thanks. That makes perfect sense.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rpkilby deleted the django-request-body branch

November 15, 2017 19:59

I have no idea how this PR works.
_load_data_and_files call POST which may call _load_data_and_files.

@xordoquy - it's definitely weird, but it won't reenter. eg, if you call request.POST, it will trigger the call to _load_data_and_files(). At this point, _data/_files/_full_data are set, so the inner call to request.POST won't call _load_data_and_files() again.

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request

Nov 17, 2020

Modernize middleware tests
Added a failing test for encode#5582
Set data ref on underlying django request

Techcable added a commit to Techcable/vmprof-server that referenced this pull request

Apr 11, 2021