[issue8572] httplib getheader() throws error instead of default

[Senthil Kumaran]

Senthil Kumaran  added the comment:

David, your suggested description was much better.
Modified the documentation in r83529 and r83530.

--
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[R. David Murray]

R. David Murray  added the comment:

Right, the join behavior is correct.  But the definition of a *default* (aka 
sentinel) value is that it is returned *unchanged*.  It is counter intuitive 
that if you pass a list of strings as a default, that what you get back is 
those strings joined by ','.  But that is the released behavior. so we have to 
live with it.

I think the doc patch is OK except that it should be 'iterable' rather than 
'iterator'.  However, it might make the whole method clearer if we say instead:

Return the value of the header *name*, or *default* if there is no header 
matching *name*.  If there is more than one header with the name *name*, return 
all of the values joined by ', '.  If 'default' is any iterable other than a 
single string, its elements are similarly returned joined by commas.

--
priority:  -> normal

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Senthil Kumaran]

Senthil Kumaran  added the comment:

Fixed in revision 83521 (py3k) and 83522 (release31-maint).
I made slight modifications to the patch to include non-iterable values,like 
int, for default too.

If you feel the documentation could be made better, please suggest the wordings 
for the sentence. I am leaving the bug open for this, if the current one is 
fine, we can close the bug.

David (RDM): I am not sure if we need a Deprecation Warning here.
If you actually see getheader() is supposed to return a string only as Headers 
are strings. I don't think someone should be consciously passing a list of 
items and excepting the list of items back.

', '.join of header values seem to be for situations in which get_all returns 
list of values, which is possible when there are different values for same 
header key.  This is a correct behavior IMO. The 2.x one is a mistake where it 
returned only the first value as a string.

--
priority: release blocker -> 
resolution:  -> fixed
stage: unit test needed -> committed/rejected

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[R. David Murray]

R. David Murray  added the comment:

I'm changing this to release blocker because I don't want to see the erroneous 
behavior make it into 3.2.

--
priority: high -> release blocker

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[R. David Murray]

R. David Murray  added the comment:

Joining the iterator contents if default is an iterator is an ugly backward 
compatibility hack.  The default should really be returned unchanged.  But 
given there is code in the field working around the current bug, I guess we 
have to retain it.  Of course, it will fail if the iterator contains 
non-strings...oh well.

Perhaps we could add a deprecation warning when an iterator is used as a 
default, and in 3.3 stop special casing iterators.

And I agree that only iterators should be special cased, since anything else 
would currently be failing, and should instead be returned unchanged.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Walter Woods]

Walter Woods  added the comment:

Sigh.  I meant ``or not`` instead of ``or``.  Clearly, not having an iterator 
would be a case to not iterate over the default :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Walter Woods]

Walter Woods  added the comment:

Hi David,

I like most of your patch (especially since it has unit tests), and if people 
like yourself are actually using the current functionality then that's fine, 
but one recommendation: why not change this line:

if not headers or isinstance(headers, str):

To also include the clause ``or getattr(headers, '__iter__', False)``.  That 
way, other default values (such as numbers) would work as expected rather than 
throw an error.  What do you think of that?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[David Stanek]

David Stanek  added the comment:

Adding a patch that includes a documentation change.

--
Added file: http://bugs.python.org/file18302/8572-with-docs.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[David Stanek]

David Stanek  added the comment:

I created some tests for the existing behavior and the expected behavior. One 
of my apps was passing in a tuple and the default. Since this already worked 
and there are probably others besides me expecting this behavior I changed the 
patch to handle this.

The new patch treats any non-string value as an iterable and performs a ', 
'.join() on it. This is basically the old behavior.

This patch is against the py3k branch.

--
nosy: +dstanek
Added file: http://bugs.python.org/file18299/8572.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Shashwat Anand]

Changes by Shashwat Anand :


--
nosy: +l0nwlf

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Éric Araujo]

Changes by Éric Araujo :


--
keywords: +easy

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Éric Araujo]

Changes by Éric Araujo :


--
keywords:  -easy
nosy: +merwok

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Walter Woods]

Walter Woods  added the comment:

I'll add unit tests later if no one else gets to it first; and they have the 
same functionality, but I could change the default back to None and use is None 
if that would be clearer (it would).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[R. David Murray]

R. David Murray  added the comment:

Given what we've learned, I think that Walter's first patch is the best fix.  
No one should be relying on the current actual behavior of the default 
argument, and the fix makes it work as documented.

As for backporting to 2.7, I don't think so.  2.7 is in feature freeze, and the 
API change would be a feature and not a bug fix.  (You could argue that, 
though, based on the RFC...so someone can appeal to Benjamin if desired.)

Walter, re the patch, why pass a default to get_all at all?  It is going to 
return None if there are no such headers.  Then you could test 'if headers is 
None' and the logic would be a little more pedantically safe.

Also, we need some unit tests for this.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Walter Woods]

Walter Woods  added the comment:

And if you really think that the unfortunate behavior of 

   >>> getheader('a', ['a', 'b'])
   'a, b'

Is desired, issue8572-unfortunate.diff retains that behavior.  I think that is 
counter-intuitive though, and it is doubtful that many would be relying on this 
functionality, given that the default value of None throws an exception at the 
moment.

--
Added file: http://bugs.python.org/file17223/issue8572-unfortunate.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Walter Woods]

Walter Woods  added the comment:

Relevant part from trunk/Lib/rfc822.py illustrating that returning default 
unchanged is the legacy/defined behavior:

return self.dict.get(name.lower(), default)

See attached patch for py3k.  This preserves backwards compatibility (aside 
from the comma-joined list) and uses the default argument appropriately 
(consistently with the default argument for dict.get, for instance).

On another note, should we be patching Python 2.7 as well, since RFC 2616 
states that the 3.X behavior of a comma-joined list is appropriate (as cited  
by Senthil)?

--
Added file: http://bugs.python.org/file17222/issue8572-alt.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Walter Woods]

Walter Woods  added the comment:

Senthil, you are correct, I gave a bad example.  However, try list()ing an 
integer :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Senthil Kumaran]

Senthil Kumaran  added the comment:

Walter, just to address one of your point:

+if failobj and not isinstance(failobj, list):
+if isinstance(failobj, str):
+failobj = [failobj]
+else:
+failobj = list(failobj)
 return failobj   

If the failobj is None, list(None) does not occur here and no exception will be 
raised. 

Yes, please attach a patch to this issue as you consider would be appropriate. 
We can see the differences. In py2.6, HTTPResponse.getheader did not invoke 
get_all method at all, it invokes a geheader method which is a simple dict 
lookup with default return.

--
assignee:  -> orsenthil

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Walter Woods]

Walter Woods  added the comment:

Sorry I'm just getting back to this . . . Senthil, doesn't list(None) throw an 
exception?  That was the whole problem with list()ing the default argument.

And I don't think the problem should be fixed in 
email.message.Message.get_all() . . . that function works exactly as it says it 
should.  Its behavior is consistent.  This issue should not change that.  And 
even WITH changing that function, the patch would still need to fix 
http.client.HTTPResponse.getheader().  

Just check python 2.6, and it looks like that function works correctly.  If a 
number is passed, it returns a number as the default.  We'd be preserving 
backwards compatibility, not destroying it, by returning the default parameter 
unchanged in 3.X when the specified header does not exist.

I'll try attaching a patch before too long.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Senthil Kumaran]

Senthil Kumaran  added the comment:

previous patch had a typo and a mistake. uploading the correct one.

--
Added file: http://bugs.python.org/file17166/issue8572.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Senthil Kumaran]

Changes by Senthil Kumaran :


Removed file: http://bugs.python.org/file17165/issue8572.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Senthil Kumaran]

Senthil Kumaran  added the comment:

It seems that 3.x behavior is correct. I am quoting a relevant section from rfc 
2616.

"""
It MUST be possible to combine the multiple header fields into one "field-name: 
field-value" pair, without changing the semantics of the message, by appending 
each subsequent field-value to the first, each
separated by a comma.
"""

- Should we change the 2.x behavior then? (This can break apps, which might be 
depending upon the existing behavior of 2.x code)

Also, this bug is on default value, it fair to assume that the default might 
given as None, or a string and rarely as int. I don't see anyone would have 
passed it (correctly for current code) as list.

Why don't we handle it at email.message.Message's get_all method?
get_all is supposed to return a list or None.

Attaching a patch (which is almost David's first suggestion).

--
keywords: +patch
Added file: http://bugs.python.org/file17165/issue8572.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Senthil Kumaran]

Senthil Kumaran  added the comment:

On Fri, Apr 30, 2010 at 01:56:01AM +, R. David Murray wrote:
> 
> I seem to have been missing some context here. 

I was referring to Walter's comment on default being an int, like
HTTPResponse.getheader('Fake-Content-Length',default=42). It was
surprising to me, but the he provided a sample snippet and pointed out
that default value can be None. So, yes, just checking of string value
might not give the correct results in all circumstances (esp
default=None case which is strong).

>In the 2.x code getheader returns only the value of the first header
>with the given name, in the 3.x code it returns the comma separated
>concatenation of the values of all headers with that name, and breaks
>the handling of default.
> 
> Any idea which handling of header values is actually the correct
> behavior vis-a-vis the http protocol?

I believe, 2.x version is proper. I was surprised at get_all() method
introduced too, instead of getheader (if its present in Message)

Headers is a simply a dict and getheader is similar to dict.get(key,
default). I shall look up the HTTP RFC and confirm what should be done
if multiple values exists for same header.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[R. David Murray]

R. David Murray  added the comment:

I seem to have been missing some context here.  I now understand that this is a 
regression relative to Python 2.x.  It seems to me that the translation from 
rfc822.Message to email.Message was done incorrectly.  In the 2.x code 
getheader returns only the value of the first header with the given name, in 
the 3.x code it returns the comma separated concatenation of the values of all 
headers with that name, and breaks the handling of default.

Any idea which handling of header values is actually the correct behavior 
vis-a-vis the http protocol?

Either way, the translation broke 'default', and that should be fixed, 
especially since we know there is 2.x code depending on the documented 
behavior, and the documented behavior hasn't changed in 3.x.  Since it is a 
regression, I'm inclined to think we can get away with fixing it without 
preserving backward compatibility with 3.1.2 (ie: it's a bug, and no one is 
likely to have written code to work around the bug, they'd report it instead as 
Walter did). But my opinion on that should perhaps be verified by appeal to 
python-dev, once we've agreed on what the correct behavior of the routine 
should be given multiple headers with the same name.

I'm adding hdiogenes and barry as nosy since they were the principle people 
involved in the translation, but they may not remember why this particular 
change was made.  As far as I can see the change is in the patch Barry applied 
but not in hdiogenes' original patch in the relevant issue (issue 2848), so 
there is probably some discussion missing from the issue.

--
nosy: +barry, hdiogenes
priority: normal -> high
versions: +Python 3.2

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Walter Woods]

Walter Woods  added the comment:

Little more info:

The actual snippet from couchdb-python is:

  if int(resp.getheader('content-length', sys.maxsize)) < CHUNK_SIZE:

Which should be valid python . . . calling int() on an integer should work, and 
calling int() on a string expected to be an integer should work.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Walter Woods]

Walter Woods  added the comment:

couchdb-python makes http requests, yes.  

Passing in a number might be mildly inappropriate, BUT I'd like to point out 
that the default might be used to find out if the header is not set (hence its 
default value of None).  

It should not assume that a string is the proper input; any sentinel value 
should work (especially as python is dynamic, and the default parameter is not 
actually used for the function's logic).

Which goes back to the original patch, unless a list or tuple is provided, in 
which case the list is comma joined for backwards compatibility as suggested by 
David.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[R. David Murray]

R. David Murray  added the comment:

@senthil: I'm not quite sure what your sentence referencing couchdb is getting 
at, but the headers that are being queried are in an email.Message object, and 
contain only string values unless some code is misusing the API and setting new 
non-string values after the initial header parse is has been done.

Regardless, though, my suggested change makes the code work with the documented 
API (which strongly implies that default is a string) while continuing to 
accept any previously working values that existing code may be passing in.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Senthil Kumaran]

Senthil Kumaran  added the comment:

And also, the code which is using getheader of HTTPResponse object would mostly 
(but incorrectly in py3k) be passing a string as default. The HTTP headers have 
been a dict with key,values as strings. Counchdb-python - does this make HTTP 
reqs? I am not sure if having int-values in the header values is proper.

So, David's last e.g seems just fine. Check if it is string and for the code's 
requirement, make it a list.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[R. David Murray]

R. David Murray  added the comment:

No, because my code is a backward compatibility hack.  Currently if someone is 
passing a default successfully they must be doing it by passing in a list or 
tuple consisting of one or more strings.  So your code would result in 
something like:

  >>> ', '.join([str(['abc'])])
  "['abc']"

However, you are correct that if default is some object other than a list, it 
is either going to work in the ', ' or it isn't, so there's no need to call 
list on it.  So my code should be simplified to:

if isinstance(default, string):
   default = [default]
return ', '.join(self.headers_get_all(name, default))

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Walter Woods]

Walter Woods  added the comment:

David: Your example tests specifically for a string, but what about just 
converting default to a string always if it's not a list?  Otherwise the string 
join is just going to fail anyway (suppose an integer is passed, which is the 
case with couchdb-python).  Wouldn't


if not isinstance(default, list):
default = [str(default)]

Be safer?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Senthil Kumaran]

Senthil Kumaran  added the comment:

Walter, have a look at http://www.python.org/dev/ document. It is really easy 
and attach it when its ready.
1) Create a patch against trunk (or py3k).
2) Patch against trunk/ or (py3k/ if its py3k only)
3) Lib/test/test_httplib.py might have similar tests, so build on top of  some 
existing test.
4) NEWS if applicable.

--
nosy: +orsenthil

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Walter Woods]

Walter Woods  added the comment:

Good point.  Yeah, I could do that within a week or two at the latest.  Just 
attach a .patch here when done?  And, what should the base be for the patch . . 
. the Lib/http directory?  Never submitted anything to python before.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[R. David Murray]

R. David Murray  added the comment:

For backward compatibility this would have to be something more like:

if not isinstance(default, list):
if isinstance(default, string):
   default = [default]
else:
   default = list(default)
return ', '.join(self.headers_get_all(name, default))

Care to work up a patch, including unit test (it looks like the unit tests live 
in test_httplib)?

--
components: +Library (Lib)
keywords: +easy
nosy: +r.david.murray
stage:  -> unit test needed
type: crash -> behavior

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8572] httplib getheader() throws error instead of default

[Walter Woods]

Changes by Walter Woods :


--
title: httplib getheader() does not work as documented -> httplib getheader() 
throws error instead of default

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com