HttpResponse headers interface

[Tom Carrick]

I don't find myself using HttpResponse very often, usually I'm using DRF's Responses. But today I needed to use one, and as I was writing tests, I ended up somewhat astonished, so with the principle of least astonishment in mind... I had anticipated that I could check the headers with `response.headers`, similar to how the new request.headers works, but apparently this is not the case. After reading the docs, I found out that I should just treat the HttpResponse object itself as if it were a dictionary of headers. This seems very strange to me, it's not what I expect, but maybe I'm in the minority.

I have no interest in deprecating the old API, but it would be nice if the headers were all accessible from a simple headers dict, and perhaps make this the source of truth, allowing access with any casing but preserving the original casing for output. It looks like what is currently HttpResponse._headers was once HttpRequest.headers, but this was 13 years ago, I don't think it'd be confusing to add the property back as something different.

[Adam Johnson]

I have also found this a little odd when writing tests. It would certainly make it easier to write both normal Django code and tests, and it's a small addition, so +1 from me.

On Wed, 17 Jun 2020 at 15:35, Tom Carrick <t...@carrick.eu> wrote:

I don't find myself using HttpResponse very often, usually I'm using DRF's Responses. But today I needed to use one, and as I was writing tests, I ended up somewhat astonished, so with the principle of least astonishment in mind... I had anticipated that I could check the headers with `response.headers`, similar to how the new request.headers works, but apparently this is not the case. After reading the docs, I found out that I should just treat the HttpResponse object itself as if it were a dictionary of headers. This seems very strange to me, it's not what I expect, but maybe I'm in the minority.

I have no interest in deprecating the old API, but it would be nice if the headers were all accessible from a simple headers dict, and perhaps make this the source of truth, allowing access with any casing but preserving the original casing for output. It looks like what is currently HttpResponse._headers was once HttpRequest.headers, but this was 13 years ago, I don't think it'd be confusing to add the property back as something different.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAHoz%3DMa9-m%2Bfqj0wqzQ7qW5Aiw3POHtNOp2NTBaHeP_ux5FhLg%40mail.gmail.com.

--

Adam

[Tom Carrick]

I've pushed a proof of concept here: https://github.com/django/django/pull/13186

I decided to do a bit more than I initially intended. It seems like the headers can be stored as a public interface and single source of truth now, rather than just adding an extra public property based on it, by using a slightly modified CaseInsensitiveMapping.

Perhaps I'm missing some reason - and if there is one, let me know - that this shouldn't be a public, documented interface, but I'm not sure what that is.

If this all seems good, I'll write up docs and tests (all current tests seem to be passing). I'm wondering though, if we should prefer this interface over the old one, as it's a bit more explicit in my view. I'd be happy to go through the docs and change the examples to using the headers attribute directly. I don't see a reason to deprecate it at all just now (though perhaps in _my_ ideal world that would happen at some point), but I'm not sure if it's worth keeping the current interface in the documentation at all?

One further small addition, I think it would be good to be able to pass headers into the HttpResponse object, so rather than doing:

    response = HttpResponse()

    response['foo'] = 'bar'

    return response

You could instead return HttpResponse(headers={'foo': 'bar'})

Perhaps that's better in a separate ticket / PR, but it seems like a minimal amount of effort to add it at the same time.

Cheers,

Tom

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMyDDM2HCJKbW%3D3dNkB7BDV4R6eSAGokg%3DPENFd%3D%2BhNwaTt9OQ%40mail.gmail.com.

[Adam Johnson]

I'm wondering though, if we should prefer this interface over the old one, as it's a bit more explicit in my view. I'd be happy to go through the docs and change the examples to using the headers attribute directly.

I think it's preferable. The old interface was so unclear.

It would also be good to update as many of Django's tests as possible, at least those that can be found with a regex, e.g. resp(onse)?\[['"] .

 

don't see a reason to deprecate it at all just now (though perhaps in _my_ ideal world that would happen at some point)

Yes, this will probably need to live 'forever' for backwards compatibility concerns. The same as for request.GET etc. as per https://groups.google.com/d/msg/django-developers/Kx8BfU-z4_E/gJBuGeZTBwAJ .

One further small addition, I think it would be good to be able to pass headers into the HttpResponse object

I'm also behind that. A quick survey shows that Flask, Pyramid, and Starlette all allow passing headers in response construction. I think a separate ticket is warranted, it would require updating yet more docs/tests, so it would be good to merge step one before embarking on step two.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAHoz%3DMa7QfRHgaT7iJTAqnpzVo6ZoHKcdLVp8dER%2BCus0G_wpw%40mail.gmail.com.

--

Adam

[Tobias Kunze]

Hi all,

first off: Thank you for your work, Tom, this will be one of the changes that
I will start using immediately and then wonder how I got by without.

>One further small addition, I think it would be good to be able to pass
>> headers into the HttpResponse object
>
>I'm also behind that. A quick survey shows that Flask, Pyramid, and
>Starlette all allow passing headers in response construction.

Absolutely this. I always found the response[header_name] approach odd.
It gave me the feeling that Django responses are very low-level and meddling
with them was a bit hacky and not intended.

Tobias

[Jon Dufresne]

> I don't see a reason to deprecate it at all just now (though perhaps in _my_ ideal world that would happen at some point), but I'm not sure if it's worth keeping the current interface in the documentation at all?

IMHO, we should eventually take the advice from the zen of Python "There should be one-- and preferably only one --obvious way to do it.". While we should not take this as dogma, I do think it is generally wise.

If there are increased concerns about existing projects, perhaps we could delay the initial deprecation or apply some kind of extended deprecation period that would allow projects more time to migrate. Removing the old interface from the docs is a great first step.

But ultimately, I think having two interfaces to solve the same tasks confuses new library users and makes project coding styles more difficult than necessary.

[Carlton Gibson]

I think we should keep the old interface.

The BC concerns are one point: it's every bit of Django code ever written. To keep __setitem__ &co is a small price to pay not to needlessly break that code.

Beyond that though, the proposed API is very nice, but the pendulum will swing back: having response objects conform to the Python Data Model, by looking like dicts, will again come to seem desirable. <tone of humour>In the meantime it'll be merely be handy that you can so use them</tone of humour> — The number of times, "I can just..." will pay off here (IMO) tells further against removing those special methods.

Kind Regards,

Carlton

[Carlton Gibson]

Just to be clear:

> I think we should keep the old interface.

I mean as well as adding the new .headers property. (So +1)

(Sorry if that was already clear.)

[Tom Carrick]

I think the PR has everything now and is ready for review: https://github.com/django/django/pull/13186

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/9a37d067-381f-46ab-bfc9-543d59a61c31o%40googlegroups.com.

[Javier Buzzi]

@Tom looks great, should we add depreciation notices to the response.__gettitem__/del that way there are no 2 right ways to do things? I would probably keep it around until 3.2... I personally like the whole respose.headers it's much more readable.

[Tom Carrick]

I guess there is still some debate on how to handle the old interface. I'll give my opinion, but I want to make it clear I don't mind that much what we do with it.

When I've seen people learning Django, they come across these magical strings you somehow add to the response, but aren't content, that does magical things like make the content download rather than show in the browser. To people who don't yet understand HTTP, this feels very... well, magical.

This new interface has the advantage of being extremely clear what's happening to people who are less familiar with HTTP than we are. It's setting a header, it's right there in the name. I also feel it's much more explicit.

I would prefer "one right way to do it", but I also don't see a compelling reason to deprecate the old interface. It's been there forever, a huge amount of code uses it, and I don't see a problem with leaving it there. The way it's implemented is just interacting with the new headers anyway, so it should be trivial to keep them in sync. Adam has suggested in the PR to note that it "may" be deprecated in the future. Even if we don't actually plan to do this, I like it. It's sneaky, we get people to hopefully migrate to the newer interface but don't break people's code, at least for a good long time.

To summarise, I'm:

+1 for the new interface.

+1 for keeping the old interface.

-0 for documenting the old interface as an alternative.

On Wed, 15 Jul 2020 at 11:43, Javier Buzzi <buzzi....@gmail.com> wrote:

@Tom looks great, should we add depreciation notices to the response.__gettitem__/del that way there are no 2 right ways to do things? I would probably keep it around until 3.2... I personally like the whole respose.headers it's much more readable.

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/86889efe-b876-46cc-8dc4-1559d6a6487do%40googlegroups.com.

[Adam Johnson]

I'm in favour of retaining and documenting the old interface, with some kind of sentence pointing at the new one. Carlton posted on the PR that he felt it was a bit paternalistic to recommend one over the other. I don't agree, I think it's important to offer some guidance, especially for new or returning developers. But I'd be happy with just a pointer from the documentation of the old method that ".headers" is new, since that will at least prompt users to consider.

Deprecating would be too much churn for little benefit to working code.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAHoz%3DMa7VDDSVj-5e1yF0RrCr0WKxujpN2-LwQc3NQNNT92V%3DQ%40mail.gmail.com.

--

Adam

[Javier Buzzi]

All this makes a lot of sense.

[Jon Dufresne]

I would prefer "one right way to do it", but I also don't see a compelling reason to deprecate the old interface.

For me, the reason to eventually deprecate the old interface is to help new developers learning a new codebase. When these developers have been taught to use response.headers but inevitably happen upon the old interface, there will be a period of head scratching, diving into docs, code, whatever to learn what this means only to come to the conclusion that it is an alias. To me, that is an experience that could be improved.

The other benefit I see in deprecating the old interface is that new developers don't need to learn or care how this fits in a particular organization's coding style. Perhaps the organization requires using only response.headers. Someone that uses the old interface out of habit will need to unlearn this habit and perhaps deal with the frustration that comes up during a review to fix it.

Consolidating to only a single interface would resolve both of these, IMO. I'm quite happy to see the deprecation occur over a longer time than normal, so I think the "may be deprecated in the future" is a good choice to start and then we can act only after we feel enough time has passed.

[David Smith]

The recent change to `url()` was a good example of this; even though it was in a DEP and the docs for a long time it still caused a lot of noise when the deprecation path was finally started.

The projects (ok, small sample) I've looked at are only now making this change. Folks will only change their code when they absolutely need to.

Could a warning in a of 'hey, did you know there is a new way to do this and btw the current way is going away' be more helpful. (I suspect not, but putting it out here).

[Carlton Gibson]

Some concerns were expressed privately to me privately in the week about the change here.

I was thinking about it, and re-reading the API Stability document https://docs.djangoproject.com/en/3.0/misc/api-stability/.

The more I look at it, the less convinced I am that the proposal here meets the "when we discover clearly superior ways to do things" criterion.

Yes, I think, if we were starting from scratch, we'd probably put headers in a mapping attached to the response, so the request.headers here proposed.

I think that probably is (definitely is?) easier for a beginner to grok. But looking at the diff — where all the changes are just from:

response['Content-Disposition'] = 'attachment; filename="somefilename.csv"'

to:

response.headers['Content-Disposition'] = 'attachment; filename="somefilename.csv"'

I can't see that this is clearly superior, to just having the response itself act as said mapping.

I learnt Python through Django. This exact case was my first expose to the Python Data Model. Oh, objects can act like dictionaries. Cool.

Didn't think about it again for half a dozen years. But it wasn't so difficult.

The s/response/response.headers/g (from one POV) just looks like more code, despite the initial "Yeah, sounds good."

Then I come to the “one way to do it. OK, I get that.

But I look at the prospect of forcing every project that ever set an HTTP header to have to update. I look again at the "clearly superior", and I have to conclude that the change isn't justfied. 

That's too bigger cost for too small a gain.

So, despite earlier enthusiasm, I have to lean towards a -1 here.

Like request.headers, I initially took this as a purely additive gain, but that's not compatible with the goals expressed in the API Stability policy.

So I think we should pass.

Not easy.

Django is in a good place: we're at the point where it's easy to update, and that's the expectation. The API Stability policy (and it's enforcement) is largely responsible for that.

It's important not to forget. We on this list see a little change as a nothing. Across the massive install base it's anything but.

David's mention of url() is  a good one — for each complaint we see here, and we saw a few, there will be a thousand we don't — people just do the work, but that's not something we should impose lightly.

The are a couple of good additions to take:

* Setting headers via an init kwarg would be cool.

* Maybe the CaseInsenstiveMapping is worth using too. (Not so sure what the benefits are here.)

Kind Regards,

Carlton

[Tom Carrick]

Hmm. I do think that Python's data model is a Good Thing. Where we might disagree is that I don't think this is an appropriate use of it.I'll try to illustrate with an example. Consider this code:

response = HttpResponse()

response['foo'] = 'bar'

Now, if I try to look at this code without context, putting my Django knowledge to the back of my head, intuitively, this doesn't look like I'm setting a header. To me it actually looks more like my response is JSON (increasingly common these days) and I'm setting the 'foo' key on it to 'bar'. Now, is this likely to happen and cause confusion? Probably not, since we tend to spell out our headers such as Content-Type rather than content-type, and custom headers tend to be prefixed with 'X-'. But I do think there is potential confusion, especially to beginners or perhaps people who know a little frontend development and expect any request to return JSON, and perhaps haven't had to interact with headers as their libraries take care of them.

However, the reason I bring this up isn't so much this potential for confusion, but more this: When I think about a HTTP request, 99% of the time, I'm thinking about the response content. Only rarely am I ever thinking about the headers. Usually interacting with headers is abstracted away in middleware or some other place. So why should setting a key on the response set something on the headers? It seems unintuitive to me.

Personally, I think that is enough to meet the bar of "clearly superior", but I'm aware I don't have the only opinion.

Another thing this is useful for is that it's actually possible (in a documented, public way) to see all the response headers. Of course the server could modify this, but it's still useful when debugging to see what Django is setting, and right now the only way I can think to do this is to look at `response._headers`, and I'd rather not be using undocumented APIs.

If the concern is primarily about the diff size, it's quite trivial (if we decide to keep the existing API as-is forever and just add the new interface on top) to drop the commits changing those and just add tests for the new interface. It is then purely additive in the sense that the old API works just the same and isn't going anywhere.

Regardless, I'm happy to go wherever the consensus is.

Cheers,

Tom

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/98cba197-d079-4f7d-84e5-18308509a99ao%40googlegroups.com.

[Carlton Gibson]

The concern is the backwards incompatibility. 

I do see the reasons for the new approach. I accept that it’s easier to grok if you don’t know Django already — “principal of least astonishment” I think you opened with. 

I just can’t balance those gains against forcing a change on the entire community. It just doesn’t balance. (And being negative, as soon as you do know Django the new proposal is more verbose, so not to be preferred.)

I had thought that it would be additive but the API stability policy is quite clear on the “one way of doing things”.

Thus, as much as I do like the idea, I don’t think we can do this. 

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAHoz%3DMaBbPUw7dcTeH2M-PmLgwUb4oaLsdQi%2Bz8hCVMpCgtbYQ%40mail.gmail.com.

[Nick Pope]

I would agree that `response.headers` is by far clearer especially to those not familiar with Django or coming from other frameworks. As Tom says, we only visually know that setting a key on a response object is for headers because of the key name itself, e.g. "Content-Type". I also think that having `response.headers` brings a symmetry with `request.headers` that makes it easier for developers to remember how this all works. I am all for being concise, but I don't hold to the argument that the something isn't to be preferred just because it is more verbose and you "know" Django. The addition of ".headers" is hardly excessively verbose.

I think there has to be a balance struck between making things more accessible for developers that are new to Django and those that are long-time developers who need to maintain their existing code. There is always going to be some contention between satisfying both camps. I understand that there are the concerns raised referencing the API stability policy that there should be "one way to do it", but it does come with various provisos, e.g. "(eventually)" and "very high priority" (which also does not equal "the only priority" -  it isn't absolute).

In addition, I can highlight places with multiple ways of doing things, e.g. `Field.choices` with the ability to use enumeration classes or not, `Meta.indexes` or the not-yet-deprecated and only lightly discouraged `index_together` and `unique_together`, etc. While I think that "one way to do it" is an admirable goal, I don't think that we need to be quite so militant at adhering to that principle. Maybe this has originated from the Zen of Python, but even that states "There should be one-- and preferably only one --obvious way to do it." I see that as meaning "it is preferable to have one obvious way to do it" but not discounting multiple ways if that cannot be achieved or is undesirable.

If we're going to be driven forcibly by these principles it comes down to a tussle between Python's "obvious" - undisputedly `response.headers` - and Django's "superior" - well, arguably, neither, unless we choose clarity (`response.headers`) or Python's data model as what makes something "superior". In that regard we have a contradiction if we were to choose Python's data model as the "superior" solution as Python - or at least the "Zen of Python" - commends what is "obvious".

I honestly thought the approach Carlton mentioned in https://github.com/django/django/pull/13186#discussion_r454956921 struck the correct balance and we could even, if desired, highlight in the release notes for the new `response.headers` that the old approach has not gone away such that developers do not need to rush to update their code. As highlighted by the examples above, it feels like there is precedent for that. Also if the data model approach is plumbed in to use `.headers` as in the proposed implementation, I don't see this as being a burden to maintain.

Please let's be pragmatic about this stuff and not be driven to adhering to "one way to do it" as an extremist ideology rather than a laudable preference.

[Carlton Gibson]

Hey Nick. 

On 16 Jul 2020, at 17:41, Nick Pope <nickpo...@gmail.com> wrote:

I honestly thought the approach Carlton mentioned in https://github.com/django/django/pull/13186#discussion_r454956921 struck the correct balance and we could even, if desired, highlight in the release notes for the new `response.headers` that the old approach has not gone away such that developers do not need to rush to update their code. As highlighted by the examples above, it feels like there is precedent for that. Also if the data model approach is plumbed in to use `.headers` as in the proposed implementation, I don't see this as being a burden to maintain.

Please let's be pragmatic about this stuff and not be driven to adhering to "one way to do it" as an extremist ideology rather than a laudable preference.

This was exactly my thought. Add the new .headers feature — I don’t think there’s anyone saying it isn’t nicer in isolation — but leave the existing interface alone. 

But the concern was expressed to me that deprecation and eventual removal of the original way of doing it (under any timescale, as we saw just recently with `url`) involves updating every line of Django that ever set response header. It’s THAT burden that’s at issue. It would be too much: at that point the new niceness of .headers doesn’t justify the expense.[*] 

If we can add .headers without the breaking change (under any timescale — I joked with Adam that Django is 15, so perhaps we could discuss it when it’s 30…) then I’m all for it. I agree the implementation of `__setitem__` &co doesn’t add a maintenance burden. 

Not breaking existing code is our biggest selling point. 

Kind Regards,

Carlton

[*] Hopefully that’s not extremist…—unless cost-benefit analysis is extremist these days, which it could be 😀

[Tom Carrick]

But the proposed patch doesn't break any existing code, does it? As far as I can tell, the old interface works the same as it always did. As long as we agree to not deprecate it, which I thought we had, then nothing is breaking. Changing code in other places was at Adam's suggestion, but it's unnecessary, it can be removed without consequence.

Perhaps I'm missing something?

Tom

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/ED88B9B4-D474-4562-BFDB-4362B7168E56%40gmail.com.

[Carlton Gibson]

Hey Tom, 

The movement of the discussion was (or at least seemed to be, prompting concern) to eventually deprecate and remove, even if on a longer time scale. It’s that that raised the red flags I think.

Folks have been stung before: a new feature is introduced with a “May be deprecated in future”, rhetorically implying “don’t worry, it won’t really be deprecated”, but then inevitably it is, and then there’s the breaking change. 

(That we didn’t remove the feature immediately doesn’t change the situation for the end user — I think James made a similar point in the discussion around url() about NOT doing extended deprecations: it just delays the pain, it we can’t accept it straight out, maybe the change wasn’t so harmless.)

Currently the patch says "Both interfaces will continue to be supported.” As long as that’s not changed to any version of "May be deprecated in future”[*] then super, nice, I like it. 

> As long as we agree to not deprecate it, which I thought we had… 

My email this afternoon was prompted by the concern that this wasn’t agreed, and the consequences of that. 

I hope all that make sense: I feel like I’m saying the same things again — perhaps I’m not being clear. 

I like the feature; as far as I am aware no-one has said they don’t; if we’re not going to break the existing interface then concerns are allayed I think. 

Kind Regards,

Carlton

[*] See previous points about, “let’s talk about it when Django is 30” 

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAHoz%3DMZP-CrA1DbKJa5Jp6tAFW-Mh3VRka5Cfy868dvcaQt8rw%40mail.gmail.com.

[Tom Carrick]

Ah, I think I just misread your earlier mail then.

Sorry about the confusion!

Tom

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/235A1B7D-59DB-4A67-977F-63DC04CB30FD%40gmail.com.