encode/django-rest-frameworkPublic

NotificationsYou must be signed in to change notification settings
Fork7k
Star29.3k

Fixed regression that tests using format still work#9615

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

Already on GitHub?Sign in to your account

Jump to bottom

Merged

auvipy merged 4 commits intoencode:masterfromsliverc:test_client_no_content_by_format

Jan 10, 2025

Merged

Fixed regression that tests using format still work#9615

auvipy merged 4 commits intoencode:masterfromsliverc:test_client_no_content_by_format

Jan 10, 2025

+19 −10

Conversation

Copy link

Contributor

sliverc commentedJan 7, 2025

Description

This is a fix for a regression introduced in PR#6511 which we discovered in our tests run on Django REST framework JSON:API against DRF master.

The error only occurred on tests which return no content (e.g. delete), use a renderer without charset (e.g. JSONRenderer) and using format to determine the renderer.

sliverc added2 commits

January 7, 2025 21:59

Fixed regression that tests using format still work

ff6b519

Error only occurred on tests which return no content and usea renderer without charset (e.g. JSONRenderer)

Fixed linting

c05926b

sliverc mentioned this pull request

Jan 7, 2025

Removed support for Python 3.8django-json-api/django-rest-framework-json-api#1266

Merged

5 tasks

browniebroke reviewed

Jan 7, 2025

View reviewed changes

rest_framework/test.py OutdatedShow resolvedHide resolved

Copy link

Member

browniebroke commentedJan 7, 2025•
edited
Loading

Looking at the PR that caused the regression, it seems to be related to trying to fix something else:

Ensure the result of_encode_data is alwaysTuple[bytes, str] rather thanTuple[Union[bytes,str], str]

We used to havean early return which was returning astr instead ofbytes as body, and the OP fixed it by moving it down, missing the edge case you're hitting. Basically, the code of the fix was:https://github.com/encode/django-rest-framework/pull/6511/files#diff-41414b310a542c20e4f2a1eac261e03c2344ac69ff2ee4607c22632e6b19ad30R159-R162

I'd rather we go closer to the original code, keeping this core change, and with the early return changed from('', content_type) to(b'', content_type). That would IMO avoid the chance of more regressions before we release.

Copy link

ContributorAuthor

sliverc commentedJan 8, 2025

@browniebroke Thanks for your feedback. I totally agree that we should have an early return as before, which avoids many checks on the way. I will adjust the PR later on today then.

Used early return as before

1b5a84a

Copy link

ContributorAuthor

sliverc commentedJan 8, 2025

@browniebroke Updated. It is now as close to what it was before#6511 but ensuring that the early out is also bytes. Let me know what you think.

browniebroke reviewed

Jan 8, 2025

View reviewed changes

rest_framework/test.py Outdated

Comment on lines 192 to 194

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

Copy link

Member

browniebrokeJan 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.
	ifisinstance(ret,str):
	ret=ret.encode(renderer.charset)
	# Coerce text to bytes if required.
	ifisinstance(ret,str):
	ret=ret.encode(renderer.charset)

On the other hand, I don't see howret.encode(renderer.charset) can work ifrenderer.charset is falsy (most likelyNone)... I'mguessing theif branch at line 187 is alwaysTrue.

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

ContributorAuthor

slivercJan 8, 2025

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.