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

Ensure content_type is set when passing empty body to RequestFactory #5351

Merged
merged 2 commits into from Aug 31, 2017

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Aug 22, 2017

This resulted from and fixes #5349. In short, because of how DRF overrides RequestFactory, passing data=None to the request methods will result in the request object not having it's CONTENT_TYPE set.

The solution is to just simply set the header if present.


In more detail:

  • Django's RequestFactory methods will generally override data=None to an empty dict.
    • DRF's RequestFactory overrides this behavior, and does not set data to an empty dict.
  • Django's RequestFactory has special handling when encoding data for multipart/form-data requests, otherwise it simply force_bytes.
  • In general use w/ Django, these methods will provide a truthy data value
    • multipart/form-data encoding will include boundary lines.
    • force_bytes({}) => b'{}'
  • Because general usage has truthy data, this block is executed and sets the request's CONTENT_TYPE.
  • Since DRF's RequestFactory methods don't override data, _encode_data returns an empty string, thus not setting the request's CONTENT_TYPE.

@rpkilby rpkilby force-pushed the requestfactory-contenttype branch from 3f6aabb to c9b3fd6 Compare Aug 31, 2017
@rpkilby
Copy link
Member Author

rpkilby commented Aug 31, 2017

Alright, this has been updated with an actual fix for #5349.

@rpkilby rpkilby added this to the 3.6.5 Release milestone Aug 31, 2017
content_type='application/octet-stream', secure=False, **extra):
# Include the CONTENT_TYPE, regardless of whether or not data is empty.
if content_type is not None:
extra['CONTENT_TYPE'] = content_type
Copy link
Collaborator

@carltongibson carltongibson Aug 31, 2017

Choose a reason for hiding this comment

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

Should this be done after the call to super? (i.e. give Django a chance to do its thing first.)

Copy link
Collaborator

@carltongibson carltongibson left a comment

@rpkilby I'm happy to merge this.

I just had the one question about adding content_type to extra — this means it will always be applied.

Perhaps that's right—I can't see a problem with it—but I wonder if we shouldn't defer to Django setting it from the content_type parameter (rather than extras) and only set it after the super call if/when it comes back None? Happy to go with whatever you think there.

@rpkilby rpkilby force-pushed the requestfactory-contenttype branch from c9b3fd6 to 0ec915e Compare Aug 31, 2017
@rpkilby
Copy link
Member Author

rpkilby commented Aug 31, 2017

This is ultimately doing the same thing as the super call, except that it's not conditional on data being non-empty. The result is identical, except that we would always force the inclusion of the content_type.

Doing this after the call to super would be less dry.

@carltongibson
Copy link
Collaborator

carltongibson commented Aug 31, 2017

OK. That’s good enough for me. 👌🏼

@carltongibson carltongibson merged commit 27c382c into encode:master Aug 31, 2017
1 check passed
@rpkilby rpkilby deleted the requestfactory-contenttype branch Aug 31, 2017
@carltongibson carltongibson changed the title Unexpected result when passing empty body to RequestFactory Ensure content_type is set when passing empty body to RequestFactory Sep 28, 2017
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.

3 participants