Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#17323 closed Cleanup/optimization (fixed)

Rename request.raw_post_data to request.body

Reported by: carljm Owned by: adrian
Component: HTTP handling Version: 1.3
Severity: Normal Keywords:
Cc: dstufft Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

request.raw_post_data is a bad name. It has nothing to do with POST in particular, it's just the body of the HTTP request. This confuses users, and makes it look like Django doesn't understand how HTTP works. We should change the name to request.body and start a deprecation process.

Attachments (3)

rename-raw_post_data-to-body.diff (19.0 KB) - added by dstufft 4 years ago.
Rename raw_post_data to body
rename_raw_post_data_to_body_updated_docs.diff (20.7 KB) - added by dstufft 4 years ago.
Updated for Fix in Docs
rename-raw_post_data-to-body-updated2.diff (16.2 KB) - added by dstufft 4 years ago.
Removed Duplicate Tests + Added A Test to Verify raw_post_data == body

Download all attachments as: .zip

Change History (20)

Changed 4 years ago by dstufft

Rename raw_post_data to body

comment:1 Changed 4 years ago by dstufft

  • Owner changed from nobody to dstufft

Attached is a patch that fixes this ticket. I wasn't 100% sure on how the documentation/tests should be done.

For documentation I left the docs for raw_post_data alone (other then I added a deprecation in Django 1.4) and copy/pasted the raw_post_data and renamed it to body and added that it was added in Django 1.4.

For tests I copied all the raw_post_data tests and duplicated them, except testing against body. I left the original raw_post_data tests intact, but I catch PendingDeprecation and Deprecation Warnings in them so that those tests do not emit the warning.

Hopefully this is correct!

comment:2 Changed 4 years ago by dstufft

  • Cc dstufft added

comment:3 Changed 4 years ago by dstufft

  • Has patch set

comment:4 Changed 4 years ago by ptone

  • Patch needs improvement set

The documentation should reflect the new way immediately - docs don't go through a deprecation timeline they way the code does, it is only to give people a chance to fix code.

I would change the docs to say "changed in 1.4" and see if there is a way to note that the change was a name change.

A reference to this change is needed in both the release notes and the deprecation timeline:

https://docs.djangoproject.com/en/dev/internals/deprecation/

Changed 4 years ago by dstufft

Updated for Fix in Docs

comment:5 Changed 4 years ago by dstufft

  • Patch needs improvement unset

This has been updated. You can see it at:

https://github.com/django/django/pull/90

Diff is available at:

https://github.com/django/django/pull/90.diff

Or i've uploaded a copy of the diff to the ticket as well.

comment:6 Changed 4 years ago by carljm

  • Patch needs improvement set

warnings.catch_warnings() is Python 2.6+ only, and 1.4 is still supporting Python 2.5.

In any case, I don't think an entire duplication of the tests under the raw_post_data name is necessary. In fact, I don't think it's necessary to leave in tests for the backward-compat shim at all in a case like this.

comment:7 follow-up: Changed 4 years ago by ptone

Does this render #9054 invalid?

comment:8 in reply to: ↑ 7 Changed 4 years ago by carljm

Replying to ptone:

Does this render #9054 invalid?

No, this is purely a name-change, it has zero functionality impact, so it's not going to have any effect on any other bugs (unless those bugs are also about the mis-naming of request.raw_post_data in which case they are probably dupes).

comment:9 Changed 4 years ago by dstufft

I removed the duplicate tests.

Should I add in a test at all to test that raw_post_data still works? If so where should that live?

comment:10 Changed 4 years ago by dstufft

Alright I updated this again so that we test that HttpRequest.raw_post_data is equal to HttpRequest.body.

The Pull Request is updated and i'll attach another updated diff.

Changed 4 years ago by dstufft

Removed Duplicate Tests + Added A Test to Verify raw_post_data == body

comment:11 Changed 4 years ago by dstufft

  • Patch needs improvement unset

comment:12 follow-up: Changed 4 years ago by adrian

  • Owner changed from dstufft to adrian

Thanks, dstufft. I'm going to have a look at this now.

comment:13 in reply to: ↑ 12 Changed 4 years ago by carljm

Replying to adrian:

Thanks, dstufft. I'm going to have a look at this now.

Oh, cool, I was just about to do so myself. Made one comment at github (posting it here in case you're not watching the pull request for comments): https://github.com/dstufft/django/commit/f857631197f5d1b55033735436231bd307e231c7#L0R430

comment:14 Changed 4 years ago by adrian

  • Resolution set to fixed
  • Status changed from new to closed

In [17210]:

Fixed #17323 -- Renamed HttpRequest.raw_post_data to request.body. Thanks for the patch, dstufft

comment:15 Changed 4 years ago by adrian

Aha, Carl, sorry we got our wires crossed. It'll be much easier when we're on a single system. :) I'll let you commit the try/finally bit that you pointed out in that Github comment.

comment:16 Changed 4 years ago by dstufft

For what it's worth. I pushed a fix to the pull request right as I got the email saying this ticket got closed for the try/finally :|

comment:17 Changed 4 years ago by carljm

In [17211]:

Refs #17323 -- Updated a test to use try/finally to avoid state leakage. Thanks dstufft for the patch.

Note: See TracTickets for help on using tickets.
Back to Top