Skip to content
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.

Sign up for GitHub

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

Fixed regression that tests using format still work #9615

Merged
merged 4 commits into from
Jan 10, 2025
Merged

Fixed regression that tests using format still work #9615

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ff6b519
Fixed regression that tests using format still work
sliverc Jan 7, 2025
c05926b
Fixed linting
sliverc Jan 7, 2025
1b5a84a
Used early return as before
sliverc Jan 8, 2025
77a8015
Move ret str check back to where it was
sliverc Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions rest_framework/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,8 @@ def _encode_data(self, data, format=None, content_type=None):
"""
Encode the data returning a two tuple of (bytes, content_type)
"""
if data is None:
return (b'', content_type)

assert format is None or content_type is None, (
'You may not set both `format` and `content_type`.'
Expand All @@ -161,9 +163,6 @@ def _encode_data(self, data, format=None, content_type=None):
except AttributeError:
pass

if data is None:
data = ''

# Content type specified explicitly, treat data as a raw bytestring
ret = force_bytes(data, settings.DEFAULT_CHARSET)

Expand All @@ -181,6 +180,7 @@ def _encode_data(self, data, format=None, content_type=None):

# Use format and render the data into a bytestring
renderer = self.renderer_classes[format]()
ret = renderer.render(data)

# Determine the content-type header from the renderer
content_type = renderer.media_type
Expand All @@ -189,14 +189,9 @@ def _encode_data(self, data, format=None, content_type=None):
content_type, renderer.charset
)

if data is None:
ret = ''
else:
ret = renderer.render(data)

# Coerce text to bytes if required.
if isinstance(ret, str):
ret = ret.encode(renderer.charset)
# Coerce text to bytes if required.
if isinstance(ret, str):
ret = ret.encode(renderer.charset)
Copy link
Member

@browniebroke browniebroke Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Noting that this bit used to be de-dented one level before #6511:

Suggested change
# Coerce text to bytes if required.
if isinstance(ret, str):
ret = ret.encode(renderer.charset)
# Coerce text to bytes if required.
if isinstance(ret, str):
ret = ret.encode(renderer.charset)

On the other hand, I don't see how ret.encode(renderer.charset) can work if renderer.charset is falsy (most likely None)... I'm guessing the if branch at line 187 is always True.

All in all, I'm not sure my above suggestion matters, but if someone can think of a test case where it does, I'm open to hear it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this as charset can be None and encode raises an exception when None is passed. As far as I understand it, when a renderer sets charset to None (e.g. JSONRenderer) it handles the encoding on its own and returns bytes. However, if the renderer faultily returns str and has charset set to None, an exception occurred before this change.

My fix indenting the str check when no charset is set, actually avoids the exception which, when thinking about it again, might make things worse as it is a misconfiguration. One could argue an assert might be better in this case. I guess though if so that should be better part of another PR and I assume it has not been an issue in the past, so let's keep it as it was.

I will remove the indent again then.


return ret, content_type

Expand Down
18 changes: 16 additions & 2 deletions tests/test_testing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@
from django.test import TestCase, override_settings
from django.urls import path

from rest_framework import fields, parsers, serializers
from rest_framework import fields, parsers, renderers, serializers, status
from rest_framework.authtoken.models import Token
from rest_framework.decorators import api_view, parser_classes
from rest_framework.decorators import (
api_view, parser_classes, renderer_classes
)
from rest_framework.response import Response
from rest_framework.test import (
APIClient, APIRequestFactory, URLPatternsTestCase, force_authenticate
Expand Down Expand Up @@ -56,6 +58,12 @@ def post_json_view(request):
return Response(request.data)


@api_view(['DELETE'])
@renderer_classes((renderers.JSONRenderer, ))
def delete_json_view(request):
return Response(status=status.HTTP_204_NO_CONTENT)


@api_view(['POST'])
def post_view(request):
serializer = BasicSerializer(data=request.data)
Expand All @@ -69,6 +77,7 @@ def post_view(request):
path('redirect-view/', redirect_view),
path('redirect-view/<int:code>/', redirect_307_308_view),
path('post-json-view/', post_json_view),
path('delete-json-view/', delete_json_view),
path('post-view/', post_view),
]

Expand Down Expand Up @@ -254,6 +263,11 @@ def test_post_encodes_data_based_on_json_content_type(self):
assert response.status_code == 200
assert response.data == data

def test_delete_based_on_format(self):
response = self.client.delete('/delete-json-view/', format='json')
assert response.status_code == status.HTTP_204_NO_CONTENT
assert response.data is None


class TestAPIRequestFactory(TestCase):
def test_csrf_exempt_by_default(self):
Expand Down
Loading