Rename '_request' to 'http_request'

[rpkilby]

Per @sigmavirus24's suggestion in #5618, this adds the underlying HttpRequest object to the public interface of the DRF Request. Request._request has been deprecated in favor of Request.http_request.

This also fixes a bug introduced in #5590. POST/FILES should only be set on the Django request when the media type is one of the form types.

[carltongibson]

Hmmm. I thought you were just going to add a read-only accessor...

Bottom line: whilst it's not going anywhere, I'm not at all sure about making _request a fully public mutable property: whilst there are valid use-cases, accessing _request probably means you're holding it wrong... — updating _request seems, well, I'm not sure I'd do that at all...

Not sure.

(Full disclosure: I'm not as yet convinced we need even a read-only accessor — this isn't Java. _request is perfectly usable. It's not going anywhere. The name implies the appropriate Are you sure? when using it.)

[rpkilby]

Either works. The main question is if http_request should be added at all. From there, it's easy to fix up the PR.

[carltongibson]

@rpkilby 👍. Let's leave it open for now. As I say, Not sure.

[sigmavirus24]

So I don't think there's danger of someone replacing the http_request and if there was danger of that, then there's likely already danger of someone replacing _request. Either way, someone can modify attributes on it that could be as potentially problematic as replacing the request wholesale. I'm 👍 either way.

[tomchristie]

The main question is if http_request should be added at all.

I'd rather we didn't, personally.

Tho I do think we probably ought to implement __setattr__, and ensure that attributes are set on the _request instance, where appropriate.

[rpkilby]

I'd rather we didn't, personally.

Should we add a modified version of the docs note then?

[carltongibson]

@rpkilby Yes. How did we start this? From a user worry about using private API. Well, _request isn't going anywhere, so documenting it, with suitable caveats, seems reasonable.

[sigmavirus24]

I'd rather we didn't, personally.

@tomchristie can you provide a bit more reasoning around why you'd rather not?

[tomchristie]

@sigmavirus24 Because we shouldn’t have to. It’s an implementation detail that the underlying HttpRequest is proxied too. Instead we should fix the implementation, eg. Implement setattr

[sigmavirus24]

Instead we should fix the implementation, eg. Implement setattr

I think we have wildly different understandings of what this is attempting to solve. The official project position is that if we need to reuse a request we have to access a private attribute that is supposedly not going anywhere. To alleviate concerns over using a private attribute, a public attribute is being added and documented. How does __setattr__ fix this?

[rpkilby]

I think the bugfix for #5590 is confusing the conversation. That bugfix is only tangentially related, in that it was preventing a test intended for this PR from raising an expected Exception.

Let me separate out the bugfix from this PR, and circle back around once that's been updated separately.

[tomchristie]

The official project position is that if we need to reuse a request

Can you give an example of what you mean by “reuse a request” here. Understanding that would help me dig why you need to access _request in the first place?

[tomchristie]

I'm going to close this off. All in favour of figuring out any problems where we can't just treat the request API as we'd expect, but I'd like the underlying request to remain an implementation detail that users shouldn't really need to know about.

[rpkilby]

@sigmavirus24 - can you provide a more concrete example of the view delegation? I can describe it, but I haven't used this pattern and any example I think of would be trivial/contrived.

[sigmavirus24]

So at the present time, we're maintaining a code-base that we're restricted in terms of changes that can be made. The old code base had one view that returned child objects and used a different view to render. Given that this is a GET request with no body, we just passed DRF's Request object to the other view. With the change that adds explicit type-checking where there wasn't any (in a bug-fix release) we had to switch to passing request._request to make the delegation work. This is an older code-base that always seems to break with DRF releases. It seems like there were patterns that worked explicitly or implicitly that are constantly being broken. Given that context, it'd be great to have an explicitly stable API to rely on for this. It seems like the project isn't in favor of that so I'll just drop it.

[carltongibson]

...in a bug-fix release...

This is the issue with this idea that you can maintain perfect backwards compatibility: all fixes are breaking changes if you're relying on the broken behaviour.

Calling a view from another view has never been explicitly supported. As such "an explicitly stable API" isn't (or ever was) on the cards.

If you are going to do it you shouldn't just pass the DRF Request to the new view — it wants a Django HttpRequest. People passing the Request by mistake were experiencing issues which is what the bug fix in #5618 was addressing.

That this ever worked at all is just weakness of dynamically typed languages. Passing one type when another is needed and expecting it to work is Programming by Coincidence par excellence. Ideally we'd always have had the check here but it's impossible to predict in advance which of such possible checks will actually add value. Ultimately if you want static typing use a different language.

Instead (of passing the DRF request) you should access the underlying HttpRequest which has always lived and _request, and always will (and if it were would move would certainly go via the deprecation path). So it has always been the correct approach to use request._request here.
(Incidentally if you are doing this, you should almost certainly be cloning the _request instance instead of reusing it directly, but I bet ≈no-one is doing that.)

Alas you've had to add a ._request to the sites where you've been using this pattern. There's no way round that. Sorry for the inconvenience.

Whilst it sounds like you're not going to edit your codebase, for those wanting to re-use view logic, what should you do? Factor the "Get the data" part of your view function into a separate method that can be called independently of full HTTP method handler. (i.e. instead of re-running the whole request-response cycle in your subview only in order to grad response.data just re-use the logic needed to generate the data bit directly.)