cookielib LWPCookieJar and MozillaCookieJar exceptions

[jjlee]

BPO	1117398
Nosy	@loewis
Files	loaderror.patch loaderror_v2.patch loaderror_v3.patch loaderror_docs.patch loaderror_docs_v2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2006-01-03.02:14:55.000>
created_at = <Date 2005-02-06.17:39:57.000>
labels = ['library']
title = 'cookielib LWPCookieJar and MozillaCookieJar exceptions'
updated_at = <Date 2006-01-03.02:14:55.000>
user = 'https://bugs.python.org/jjlee'

bugs.python.org fields:

activity = <Date 2006-01-03.02:14:55.000>
actor = 'nnorwitz'
assignee = 'nnorwitz'
closed = True
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2005-02-06.17:39:57.000>
creator = 'jjlee'
dependencies = []
files = ['6477', '6478', '6479', '6480', '6481']
hgrepos = []
issue_num = 1117398
keywords = ['patch']
message_count = 20.0
messages = ['47690', '47691', '47692', '47693', '47694', '47695', '47696', '47697', '47698', '47699', '47700', '47701', '47702', '47703', '47704', '47705', '47706', '47707', '47708', '47709']
nosy_count = 4.0
nosy_names = ['loewis', 'nnorwitz', 'jjlee', 'jimjjewett']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue1117398'
versions = ['Python 2.4']

[jjlee]

cookielib.LWPCookieJar and .MozillaCookieJar are
documented to raise cookielib.LoadError on attempt to
load an invalid cookies file, but do not.

I think this should be backported to the 2.4
maintenance branch.
Reason: I suspect more people will be bitten by the bug
than will be bitten by the patch, since cookies files
will rarely be invalid, so people are likely to have
written except statements based on the docs in this
case, rather than based on the actual exception that
currently occurs.

[jimjjewett]

Logged In: YES
user_id=764593

I often look at the code in a second idle window rather
than starting a web browser.

Would it work to make LoadError a subclass of IOError, at
least for the backport? People who followed the docs will
get a bugfix, but people who followed the code would get
no breakage.

Should LoadError be a subclass of IOError even in the
main trunk?

[jjlee]

Logged In: YES
user_id=261020

Jim, I don't understand the first sentence in your comment.

Re a 2.4 backport that makes LoadError derive from IOError:
it makes me wince, but I can't think of an argument against it.

No, LoadError should not be a subclass of IOError in the
trunk, because the cases where LoadError is documented to be
raised do not involve failures of I/O, but rather invalid
data. See the docs for IOError. (FWIW, EnvironmentError
(IOError's base class) wouldn't be a suitable subclass
either: eg. what would we want with an .errno attribute?)

[jimjjewett]

Logged In: YES
user_id=764593

The first sentence was pointing out that many people
(including me) never see the pretty-for-printing
documentation, and even the html form isn't generally
convenient when I'm actually programming. So I rely on
the introspection tools. I see object names, signatures,
and docstrings. If I need more information, I look at the
code, which raises IOError.

While I don't yet have code catching this particular error, I
do write in a way that would break; it wouldn't have
occurred to me to put both types of error in an except
clause just in case it changed later. (Well, unless the
docstring or at least a comment in the code itself warned
me to.)

So I would definately prefer that it remain (at least a
subclass of) IOError for at least 2.4.x I would also
appreciate comments in the fixed 2.4 code if it is going to
change for 2.5.

[jjlee]

Logged In: YES
user_id=261020

I agree the backport (but not the trunk) should have
LoadError derive from IOError.

I'd be happy for comments to go in noting the change in all
places where LoadError is raised, if that's considered good
practice.

Any committers reading: should I submit an updated patch
with just those added comments? Should I submit a patch for
the backport, with just that change plus the added (IOError)
for backwards-compat.?

(Jim: Besides the point for the matter at hand, but I should
point out that the "pretty-for-printing" cookielib docs and
the docstrings in cookielib.py form almost disjoint sets.
Most of the documentation lives under Doc/lib, the stuff in
the module source is the more obscure detail. And of
course, you have just demonstrated to yourself how not
reading the docs leaves you with an incomplete understanding
of the intent, which can be useful! Not that I *ever* do
that, of course ;-)

[loewis]

Logged In: YES
user_id=21627

Is it true that LoadError is currently not used anywhere? If
so, I think it would be best to eliminate this exception,
both from the code and the documentation. To support code
that might refer to LoadError already, you could make it a
synonym to IOError.

[jjlee]

Logged In: YES
user_id=261020

LoadError is not currently not used anywhere.

Without LoadError, how would one distinguish (for the
purpose of error handling) an error due to, on the one hand,
a missing file, insufficient permissions, etc. (IOError)
from an error due, on the other hand, to an invalid cookies
file (LoadError)? This is already a problem with IOError,
of course, but I don't see why we should make it worse.

[loewis]

Logged In: YES
user_id=21627

Can you give an example of an invalid cookies file? How does
the library respond to it right now?

[jjlee]

Logged In: YES
user_id=261020

>>> open('bad_cookies.txt', 'w').write("blah")
>>> import cookielib
>>> mcj = cookielib.MozillaCookieJar(filename="bad_cookies.txt")
>>> mcj.load()
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "c:\Python24\lib\cookielib.py", line 1727, in load
    self._really_load(f, filename, ignore_discard,
ignore_expires)
  File "c:\Python24\lib\_MozillaCookieJar.py", line 53, in
_really_load
    raise IOError(
IOError: bad_cookies.txt does not look like a Netscape
format cookies file
>>>

[loewis]

Logged In: YES
user_id=21627

I see. In retrospect, it might have been best to reject the
cookielib for Python 2.4, and wait for it to get a stable
interface and implementation. Without the time machine, we
have to accept the consequences, though, so we cannot break
existing code.

Therefore, I now propose that LoadError becomes a subclass a
*permanent* subclass of IOError, thus preserving the
historical interface.

[jjlee]

Logged In: YES
user_id=261020

Re IOError: OK, I'll submit a revised patch.

Yes, I agree on your first para, with hindsight. I
attempted to avoid making users change interfaces twice
(once for a release of a Python 2.4 candidate, once for 2.4
itself). Stupid idea, especially since the changes (though
"minor") touched a lot of code. Even then, I wonder if it
would have received any significant testing in practice from
people other than me, since people might not have bothered
to switch from the backwards-compatible (<=2.4) version.

Thanks for your attention to these bugs.

[jjlee]

Logged In: YES
user_id=261020

Revised patch attached.

[jjlee]

Logged In: YES
user_id=261020

This comment is not relevant to the patch, just a comment on
my own comment:

Even then, I wonder if it would have received any
significant testing in practice from people other than me,
since people might not have bothered to switch from the
backwards-compatible (<=2.4) version.

Hmm, that's not true: four bugs might have been picked up if
I'd released a final version of the <=2.4
backwards-compatible ClientCookie 0.9 version (with the
interface changes from ClientCookie 0.4 to cookielib), then
released a ClientCookie 1.0 after Python 2.4 came out (there
was enough time without needing to wait for 2.5). That's
what I should have done, instead of trying to protect
ClientCookie users from two rounds of interface changes.
Live and learn, I suppose.

OTOH, no guilt on 1117339 or 1028908 (except regret that I
was suddenly unable to work on it for a period up to the 2.4
beta release, hence missing deadline to get the latter
applied before it was released).

[jjlee]

Logged In: YES
user_id=261020

Patch loaderror_v2.patch implements MvL's suggestion (see
followup 2005-03-04 14:52), and includes tests.

It still applies cleanly (except that 'patch' doesn't like
the ',v' on the end of the CVS patch filenames) and the
tests pass.

Can it be committed, and preferably backported to the 2.4
maintenance branch?

[jjlee]

Logged In: YES
user_id=261020

jjlee> It still applies cleanly (except that 'patch' doesn't
like the ',v' on the end of the CVS patch filenames)

Sorry, FWLIW, it *does* apply cleanly, I just had the wrong
-p argument to patch.

[jjlee]

Logged In: YES
user_id=261020

OK, another patch applied since March causes
loaderror_v2.patch to add a class statement which is already
present in the patched file.

loaderror_v3.patch fixes that.

[nnorwitz]

Logged In: YES
user_id=33168

Should there be doc changes for LoadError subclassing IOError?
Should the re-raise of IOError be changed to raise LoadError?

Committed revision 41798.
Committed revision 41799. (Misc/NEWS)
Committed revision 41800. (2.4)

[jjlee]

Logged In: YES
user_id=261020

Thanks Neil (and thanks for applying my other cookielib patch).

I've attached a doc patch. I don't like to repeat the
wording (as the patch does), but it seems necessary here --
do you agree?

The re-raise should not be changed, since CookieJar.load()
may legitimately raise exactly IOError in addition to
LoadError (and was always documented as such). For example,
the IOError should be re-raised when the cookies file does
not exist.

[jjlee]

Logged In: YES
user_id=261020

jjlee> and was always documented as such

To be precise, it was documented under .revert(). I attach
an improved doc patch (loaderror_docs_v2.patch, obsoleting
loaderror_docs.patch) that moves that into the .load()
method docs, and makes the .revert() docs refer to that.

[nnorwitz]

Logged In: YES
user_id=33168

I updated the docs from the v2 patch. It doesn't seem to
need backporting.

I didn't love the duplication, but it's not a big deal, so I
kept it. I modified the wording some to be a bit.

Committed revision 41887.