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.

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sliverc
Copy link
Contributor

@sliverc sliverc commented Jan 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.

Error only occurred on tests which return no content and use
a renderer without charset (e.g. JSONRenderer)
rest_framework/test.py Outdated Show resolved Hide resolved
@browniebroke
Copy link
Member

browniebroke commented Jan 7, 2025

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 always Tuple[bytes, str] rather than Tuple[Union[bytes,str], str]

We used to have an early return which was returning a str instead of bytes 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.

@sliverc
Copy link
Contributor Author

sliverc commented Jan 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.

@sliverc
Copy link
Contributor Author

sliverc commented Jan 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.

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

@browniebroke browniebroke Jan 8, 2025

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.

Copy link
Member

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

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

That looks good from my perspective. I'll leave the final word to @auvipy who reviewed #6511 to confirm my assesment of the revert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants