From ef2208ccc6dae21633d642573dcead0e948dfb30 Mon Sep 17 00:00:00 2001 From: Ryan P Kilby Date: Thu, 25 Jan 2018 04:25:25 -0500 Subject: [PATCH 1/5] Rename 'wsgi' request test to more accurate 'http' --- tests/test_request.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_request.py b/tests/test_request.py index 8c680baa0..632f4a85e 100644 --- a/tests/test_request.py +++ b/tests/test_request.py @@ -271,23 +271,23 @@ class TestSecure(TestCase): assert request.scheme == 'https' -class TestWSGIRequestProxy(TestCase): - def test_attribute_access(self): - wsgi_request = factory.get('/') - request = Request(wsgi_request) +class TestHttpRequest(TestCase): + def test_attribute_access_proxy(self): + http_request = factory.get('/') + request = Request(http_request) inner_sentinel = object() - wsgi_request.inner_property = inner_sentinel + http_request.inner_property = inner_sentinel assert request.inner_property is inner_sentinel outer_sentinel = object() request.inner_property = outer_sentinel assert request.inner_property is outer_sentinel - def test_exception(self): + def test_exception_proxy(self): # ensure the exception message is not for the underlying WSGIRequest - wsgi_request = factory.get('/') - request = Request(wsgi_request) + http_request = factory.get('/') + request = Request(http_request) message = "'Request' object has no attribute 'inner_property'" with self.assertRaisesMessage(AttributeError, message): From f457f3d90657afaae2c2cbe8907b2f2f944087d7 Mon Sep 17 00:00:00 2001 From: Ryan P Kilby Date: Thu, 25 Jan 2018 04:29:47 -0500 Subject: [PATCH 2/5] Add 'Request.http_request', deprecate '._request' --- rest_framework/request.py | 19 ++++++++++++++++++- tests/test_request.py | 19 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/rest_framework/request.py b/rest_framework/request.py index 7e4daf274..230e7deaf 100644 --- a/rest_framework/request.py +++ b/rest_framework/request.py @@ -11,6 +11,7 @@ The wrapped request then offers a richer API, in particular : from __future__ import unicode_literals import sys +import warnings from contextlib import contextmanager from django.conf import settings @@ -159,7 +160,7 @@ class Request(object): .format(request.__class__.__module__, request.__class__.__name__) ) - self._request = request + self.http_request = request self.parsers = parsers or () self.authenticators = authenticators or () self.negotiator = negotiator or self._default_negotiator() @@ -181,6 +182,22 @@ class Request(object): forced_auth = ForcedAuthentication(force_user, force_token) self.authenticators = (forced_auth,) + @property + def _request(self): + warnings.warn( + "`_request` has been deprecated in favor of `http_request`, and will be removed in 3.10", + PendingDeprecationWarning, stacklevel=2 + ) + return self.http_request + + @_request.setter + def _request(self, value): + warnings.warn( + "`_request` has been deprecated in favor of `http_request`, and will be removed in 3.10", + PendingDeprecationWarning, stacklevel=2 + ) + self.http_request = value + def _default_negotiator(self): return api_settings.DEFAULT_CONTENT_NEGOTIATION_CLASS() diff --git a/tests/test_request.py b/tests/test_request.py index 632f4a85e..36ef6676c 100644 --- a/tests/test_request.py +++ b/tests/test_request.py @@ -292,3 +292,22 @@ class TestHttpRequest(TestCase): message = "'Request' object has no attribute 'inner_property'" with self.assertRaisesMessage(AttributeError, message): request.inner_property + + def test_request_deprecation(self): + with pytest.warns(PendingDeprecationWarning) as record: + Request(factory.get('/'))._request + + assert len(record) == 1 + assert str(record[0].message) == ( + "`_request` has been deprecated in favor of " + "`http_request`, and will be removed in 3.10" + ) + + with pytest.warns(PendingDeprecationWarning) as record: + Request(factory.get('/'))._request = None + + assert len(record) == 1 + assert str(record[0].message) == ( + "`_request` has been deprecated in favor of " + "`http_request`, and will be removed in 3.10" + ) From 9bb17e4df6463003fa783974d231fe0e39eb1bb1 Mon Sep 17 00:00:00 2001 From: Ryan P Kilby Date: Thu, 25 Jan 2018 04:32:06 -0500 Subject: [PATCH 3/5] Fix references to 'Request._request' --- rest_framework/authentication.py | 2 +- rest_framework/request.py | 34 ++++++++++++++++---------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/rest_framework/authentication.py b/rest_framework/authentication.py index 0749968bc..f3fff2793 100644 --- a/rest_framework/authentication.py +++ b/rest_framework/authentication.py @@ -121,7 +121,7 @@ class SessionAuthentication(BaseAuthentication): """ # Get the session-based user from the underlying HttpRequest object - user = getattr(request._request, 'user', None) + user = getattr(request.http_request, 'user', None) # Unauthenticated, CSRF validation not required if not user or not user.is_active: diff --git a/rest_framework/request.py b/rest_framework/request.py index 230e7deaf..199df7c34 100644 --- a/rest_framework/request.py +++ b/rest_framework/request.py @@ -98,7 +98,7 @@ def clone_request(request, method): Internal helper method to clone a request, replacing with a different HTTP method. Used for checking permissions against other methods. """ - ret = Request(request=request._request, + ret = Request(request=request.http_request, parsers=request.parsers, authenticators=request.authenticators, negotiator=request.negotiator, @@ -203,7 +203,7 @@ class Request(object): @property def content_type(self): - meta = self._request.META + meta = self.http_request.META return meta.get('CONTENT_TYPE', meta.get('HTTP_CONTENT_TYPE', '')) @property @@ -220,7 +220,7 @@ class Request(object): """ More semantically correct name for request.GET. """ - return self._request.GET + return self.http_request.GET @property def data(self): @@ -250,7 +250,7 @@ class Request(object): instance, ensuring that it is available to any middleware in the stack. """ self._user = value - self._request.user = value + self.http_request.user = value @property def auth(self): @@ -270,7 +270,7 @@ class Request(object): request, such as an authentication token. """ self._auth = value - self._request.auth = value + self.http_request.auth = value @property def successful_authenticator(self): @@ -297,14 +297,14 @@ class Request(object): # copy data & files refs to the underlying request so that closable # objects are handled appropriately. - self._request._post = self.POST - self._request._files = self.FILES + self.http_request._post = self.POST + self.http_request._files = self.FILES def _load_stream(self): """ Return the content body of the request, as a stream. """ - meta = self._request.META + meta = self.http_request.META try: content_length = int( meta.get('CONTENT_LENGTH', meta.get('HTTP_CONTENT_LENGTH', 0)) @@ -314,8 +314,8 @@ class Request(object): if content_length == 0: self._stream = None - elif not self._request._read_started: - self._stream = self._request + elif not self.http_request._read_started: + self._stream = self.http_request else: self._stream = six.BytesIO(self.body) @@ -339,18 +339,18 @@ class Request(object): try: stream = self.stream except RawPostDataException: - if not hasattr(self._request, '_post'): + if not hasattr(self.http_request, '_post'): raise # If request.POST has been accessed in middleware, and a method='POST' # request was made with 'multipart/form-data', then the request stream # will already have been exhausted. if self._supports_form_parsing(): - return (self._request.POST, self._request.FILES) + return (self.http_request.POST, self.http_request.FILES) stream = None if stream is None or media_type is None: if media_type and is_form_media_type(media_type): - empty_data = QueryDict('', encoding=self._request._encoding) + empty_data = QueryDict('', encoding=self.http_request._encoding) else: empty_data = {} empty_files = MultiValueDict() @@ -368,7 +368,7 @@ class Request(object): # re-raise. Ensures we don't simply repeat the error when # attempting to render the browsable renderer response, or when # logging the request or similar. - self._data = QueryDict('', encoding=self._request._encoding) + self._data = QueryDict('', encoding=self.http_request._encoding) self._files = MultiValueDict() self._full_data = self._data raise @@ -424,7 +424,7 @@ class Request(object): to proxy it to the underlying HttpRequest object. """ try: - return getattr(self._request, attr) + return getattr(self.http_request, attr) except AttributeError: return self.__getattribute__(attr) @@ -442,7 +442,7 @@ class Request(object): self._load_data_and_files() if is_form_media_type(self.content_type): return self._data - return QueryDict('', encoding=self._request._encoding) + return QueryDict('', encoding=self.http_request._encoding) @property def FILES(self): @@ -463,4 +463,4 @@ class Request(object): def force_plaintext_errors(self, value): # Hack to allow our exception handler to force choice of # plaintext or html error responses. - self._request.is_ajax = lambda: value + self.http_request.is_ajax = lambda: value From 335ba0e377fa1d8f0fbc3b145c24a07c57d56825 Mon Sep 17 00:00:00 2001 From: Ryan P Kilby Date: Thu, 25 Jan 2018 06:35:18 -0500 Subject: [PATCH 4/5] Add docs for 'Request.http_request` - Add docs for accessing the underlying 'HttpRequest' object, and warn users that doing so is considered to be advanced, non-standard usage. - Add tests about duplicate stream parsing assumptions, which warrant the above warning. These tests are currently failing due to a bug. --- docs/api-guide/requests.md | 3 +++ tests/test_request.py | 47 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/docs/api-guide/requests.md b/docs/api-guide/requests.md index 83a38d1c3..5127edee6 100644 --- a/docs/api-guide/requests.md +++ b/docs/api-guide/requests.md @@ -130,6 +130,9 @@ As REST framework's `Request` extends Django's `HttpRequest`, all the other stan Note that due to implementation reasons the `Request` class does not inherit from `HttpRequest` class, but instead extends the class using composition. +# Accessing the HttpRequest + +The underlying `HttpRequest` can be accessed through the `.http_request` attribute. While direct manipulation of the `HttpRequest` is discouraged, there are some advanced use cases that may require it. For example, one view may delegate request handling to a secondary view function. In this case, it is necessary to pass the original `HttpRequest` to the delegated view instead of the DRF `Request` object. Be aware that duplicate processing of the `HttpRequest` may have have unintended side effects. For example, if the request stream has already been consumed, it may not be accessible for a second read and will raise an exception. [cite]: https://groups.google.com/d/topic/django-developers/dxI4qVzrBY4/discussion [parsers documentation]: parsers.md diff --git a/tests/test_request.py b/tests/test_request.py index 36ef6676c..9ccd7781e 100644 --- a/tests/test_request.py +++ b/tests/test_request.py @@ -13,6 +13,7 @@ from django.contrib.auth.middleware import AuthenticationMiddleware from django.contrib.auth.models import User from django.contrib.sessions.middleware import SessionMiddleware from django.core.files.uploadedfile import SimpleUploadedFile +from django.http.request import RawPostDataException from django.test import TestCase, override_settings from django.utils import six @@ -137,6 +138,11 @@ class MockView(APIView): return Response(status=status.HTTP_500_INTERNAL_SERVER_ERROR) +class EchoView(APIView): + def post(self, request): + return Response(status=status.HTTP_200_OK, data=request.data) + + class FileUploadView(APIView): def post(self, request): filenames = [file.temporary_file_path() for file in request.FILES.values()] @@ -149,6 +155,7 @@ class FileUploadView(APIView): urlpatterns = [ url(r'^$', MockView.as_view()), + url(r'^echo/$', EchoView.as_view()), url(r'^upload/$', FileUploadView.as_view()) ] @@ -311,3 +318,43 @@ class TestHttpRequest(TestCase): "`_request` has been deprecated in favor of " "`http_request`, and will be removed in 3.10" ) + + @override_settings(ROOT_URLCONF='tests.test_request') + def test_duplicate_request_stream_parsing_exception(self): + """ + Check assumption that duplicate stream parsing will result in a + `RawPostDataException` being raised. + """ + response = APIClient().post('/echo/', data={'a': 'b'}, format='json') + request = response.renderer_context['request'] + + # ensure that request stream was consumed by json parser + assert request.content_type.startswith('application/json') + assert response.data == {'a': 'b'} + + # pass same HttpRequest to view, stream already consumed + with pytest.raises(RawPostDataException): + EchoView.as_view()(request.http_request) + + @override_settings(ROOT_URLCONF='tests.test_request') + def test_duplicate_request_form_data_access(self): + """ + Form data is copied to the underlying django request for middleware + and file closing reasons. Duplicate processing of a request with form + data is 'safe' in so far as accessing `request.POST` does not trigger + the duplicate stream parse exception. + """ + response = APIClient().post('/echo/', data={'a': 'b'}) + request = response.renderer_context['request'] + + # ensure that request stream was consumed by form parser + assert request.content_type.startswith('multipart/form-data') + assert response.data == {'a': ['b']} + + # pass same HttpRequest to view, form data set on underlying request + response = EchoView.as_view()(request.http_request) + request = response.renderer_context['request'] + + # ensure that request stream was consumed by form parser + assert request.content_type.startswith('multipart/form-data') + assert response.data == {'a': ['b']} From 6226f070f0c60c34ee1ccf435c26e5659d1ea054 Mon Sep 17 00:00:00 2001 From: Ryan P Kilby Date: Thu, 25 Jan 2018 06:40:31 -0500 Subject: [PATCH 5/5] Fix setting post/files on the underlying request --- rest_framework/request.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/rest_framework/request.py b/rest_framework/request.py index 199df7c34..2339712b7 100644 --- a/rest_framework/request.py +++ b/rest_framework/request.py @@ -295,10 +295,11 @@ class Request(object): else: self._full_data = self._data - # copy data & files refs to the underlying request so that closable - # objects are handled appropriately. - self.http_request._post = self.POST - self.http_request._files = self.FILES + # if a form media type, copy data & files refs to the underlying + # http request so that closable objects are handled appropriately. + if is_form_media_type(self.content_type): + self.http_request._post = self.POST + self.http_request._files = self.FILES def _load_stream(self): """