<br><br><div class="gmail_quote">On Fri, Jul 31, 2009 at 14:16, Jacob Rus <span dir="ltr"><<a href="mailto:jacobolus@gmail.com">jacobolus@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Hi all,<br>
<br>
In an attempt to figure out some twisted.web code, I was reading<br>
through the Python Standard Library’s mimetypes module today, and<br>
was shocked at the poor quality of the code. I wonder how the<br>
mimetypes code made it into the standard library, and whether anyone<br>
has ever bothered to read it or update it: it is an embarrassment.<br>
Much of the code is redundant, portions fail to execute, control<br>
flow is routed through a horribly confusing mess of spaghetti, and<br>
most of the complexity has no clear benefit as far as I can tell. I<br>
probably should drop the subject and get back to work, but as a good<br>
citizen, it’s hard to just ignore this sort of thing.<br>
</blockquote><div><br></div><div>I have not looked at the code nor ever used it (that I can remember) so I can't directly address the quality. But I can say the code was added in 1997 which puts it as an addition in Python 1.4. That's why before Python took off mainstream and began to tighten up the quality control on the standard library.</div>
<div><br></div><div>I also would like to stay that I am not embarrassed by anything in Python. It's unfortunate if the mimetypes module's code is a mess, but I think putting at embarrassing is taking a little far and borderline insulting (which I don't think you meant to do).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><br>
mimetypes.py stores its types in a pair of dictionaries, one for<br>
"strict" use, and the other for "non-standard types". It creates the<br>
strict dictionary by default out of apache's mime.types file, and<br>
then overrides the entries it finds with a set of exceptions. Then<br>
it creates the non-standard dictionary, which is set to match if the<br>
strict parameter is set to False when guessing types. Just in this<br>
basic design, and in the list of types in the file, there are<br>
several problems:<br>
<br>
* Various apache mime types files are read, if found, but the<br>
ordering of the files is such that older versions of apache are<br>
sometimes read after newer ones, overriding updated mime types<br>
with out-of-date versions if multiple versions of apache are<br>
installed on the system.<br>
<br>
* The vast majority of types declared in mimetypes.py are<br>
duplicates of types already declared by Apache. In a few cases<br>
this is to change the apache default (make an exception, that<br>
is), but in most cases the mime type and extension are<br>
completely identical. This huge number of redundant types makes<br>
the file substantially harder to follow. No comments are<br>
provided to explain why various sets of exceptions are made to<br>
Apache's default mime types, and in several cases mimetypes.py<br>
seems to just be out of date as compared to recent versions of<br>
Apache, for instance not knowing about the 'text/troff' type<br>
which was registered in January 2006 in RFC 4263.<br>
<br>
* The 'non-standard' type dictionary is nearly useless, because<br>
all of the types it declares are already in apache's mime.types<br>
file, meaning that types are, as far as I can tell trying to<br>
follow ugly program flow, *never* drawn from the non-strict<br>
dictionary, except in the improbable situation where the<br>
mimetypes module is initialized with a custom set of<br>
apache-mime.types–like files, which does not include those<br>
'non-standard' types. I personally cannot see a use case for<br>
initializing the module with a custom set of mime types, but<br>
then leaving the very few types included as non-strict to the<br>
defaults: this seems like a fragile and pathological use case.<br>
Given this, I don’t see any benefit to dragging the 'strict'<br>
parameter along all the way through the code, and would advise<br>
getting rid of it altogether. Does anyone know of any code that<br>
uses the mimetypes module with strict set to False, where the<br>
non-strict code path ever *actually* is executed?<br>
<br>
But though these problems, which affect actual use of the code and<br>
are therefore probably most important, are significant, they really<br>
pale in comparison to the awful quality of implementation. I'll try<br>
to briefly outline my understanding of how code flows in<br>
mimetypes.py, and what the problems are. I haven't stepped through<br>
the code in a debugger, this is just from reading it, so I apologize<br>
in advance if I get something wrong. This is, however, some of the<br>
worst code I’ve seen in the standard library or anywhere else.<br>
<br>
* It defines __all__: I didn’t even realize __all__ could be used<br>
for single-file modules (w/o submodules), but it definitely<br>
shouldn’t be here.</blockquote><div><br></div><div>__all__ is used to control what a module exports when used in an import *, nothing more. Thus it's use in a module compared to a package is completely legitimate.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> This specific __all__ oddly does not include<br>
all of the documented variables and functions in the mimetypes<br>
class. It’s not clear why someone calling import * here wouldn’t<br>
want the bits not included.</blockquote><div><br></div><div>If something is documented by not listed in __all__ that is a bug.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
<br>
* It creates a _default_mime_types() function which declares a<br>
bunch of global variables, and then immediately calls<br>
_default_mime_types() below the definition. There is literally<br>
no difference in result between this and just putting those<br>
variables at the top level of the file, so I have no idea why<br>
this function exists, except to make the code more confusing.<br>
</blockquote><div><br></div><div>It could potentially be used for testing, but that's a guess.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><br>
* It allows command line usage: I don’t think this is necessary<br>
for a part of the standard library like this. There are better<br>
tools for finding mime types from the command line which ship<br>
with most operating systems.</blockquote><div><br></div><div>Yeah, various modules have command-line versions which are not truly necessary. This can probably stand to go.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
<br>
* Its API is pretty poorly designed. It offers 6 functions when<br>
about 3 are needed, and it takes a couple reads-through of the<br>
code to figure out exactly what any of them are supposed to do.<br>
<br>
* The operation is crazy: It defines a MimeTypes class which<br>
actually stores the type mappings, but this class is designed to<br>
be a singleton. The way that such a design is enforced is<br>
through the use of the module-global 'init' function, which<br>
makes an instance of the class, and then maps all of the<br>
functions in the module global namespace to instance methods.<br>
But confusingly, all such functions are also defined<br>
independently of the init function, with definitions such as:<br>
<br>
def guess_type(url, strict=True):<br>
if not inited:<br>
init()<br>
return guess_type(url, strict)<br>
<br>
I’d be amazed if anyone could guess what that code was trying to<br>
do. I did a double-take when I saw it.<br>
</blockquote><div><br></div><div>Probably came from someone who is very OO happy. Not everyone comes to Python ready to embrace its procedural or slightly functional facets.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
Of course, that return call is only ever reached the first time<br>
this function is called, if init() has not happened yet. This<br>
was all presumably done for lazy initialization, so that the<br>
type information would only be loaded when needed. Needless to<br>
say, there are more pythonic ways to accomplish such a goal.<br>
<br>
Oh, also, the other good one here is that it means that someone<br>
who writes `from mimetypes import guess_types` gets something<br>
different than someone who writes:<br>
`import mimetypes; guess_types = mimetypes.guess_types`. In the<br>
former case, this wrapper function is saved as guess_type, which<br>
each time just calls the (changed after init())<br>
mimetypes.guess_types function. This caused a performance<br>
nightmare before March of this year, when there was no check for<br>
`if not inited` before running init() (amazing!?).<br>
<br>
* Because the type datastore is set up to be a singleton, any time<br>
init() is called in one section of code, it resets any types<br>
which have been added manually: this means that if init() is<br>
called by different pieces of code in the same python program,<br>
they will interfere with each-others’ type databases, and break<br>
each-other. This is extremely fragile and, in my opinion, crazy.<br>
It is hard for me to imagine any use case that would benefit<br>
from this ability to clobber custom type mappings, and I very<br>
much doubt that any code calling the mimetypes module realizes<br>
that the contract of the API is so flimsy by definition. In<br>
practice, I would not advise consumers of this API to ever call<br>
init() manually, or to ever add custom mime type mappings,<br>
because they are setting themselves up for hard-to-track bugs<br>
down the line.<br>
<br>
* The 'inited' flag is a documented part of the interface, in the<br>
standard library documentation. I cannot imagine any reason to<br>
set this flag manually: setting it to false when it was true<br>
will have no effect, because the top-level functions have<br>
already been replaced by instance methods of the 'db' MimeTypes<br>
instance. Setting it to true when it was false will make the<br>
code just break outright.<br>
<br>
* In python 3, this has been changed a bit. There’s still an<br>
inited flag, and it still in the docs, but now awful code from<br>
above has been changed slightly, to:<br>
<br>
def guess_type(url, strict=True):<br>
if _db is None:<br>
init()<br>
return _db.guess_type(url, strict)<br>
<br>
Which is still embarrassingly confusing. On the upside, the<br>
inited flag now does literally nothing, but remains defined, and<br>
in the docs.<br>
<br>
* The 'types_map' and 'common_types' (for 'strict' and<br>
'common' types, respectively) dictionaries are also a documented<br>
part of the interface. When init() is called, a new MimeTypes<br>
instance makes a (different) types_map which is a tuple of two<br>
dictionaries, for 'strict' and 'common' types. Then this<br>
instance reads the apache mime.types files and adds the types to<br>
its pair of self.types_map dictionaries, and then after that<br>
looks at the global types_map and common_types dictionaries and<br>
adds *those* types to its self.types_map. Then at the end it<br>
replaces the global types_map with self.types_map[True] and<br>
replaces common_types with self.types_map[False]. Unfortunately,<br>
while changing these dictionaries will have an effect on the<br>
operation of the library, it will not update the types_map_inv<br>
mapping, so inverse lookups will not behave as the changer<br>
expects. If these dictionaries are going to remain documented,<br>
the documentation should be clear to describe them as read only<br>
to avoid very confusing bugs.<br>
<br>
* Speaking of these dictionaries, .copy() is called on those two<br>
and a few other inside MimeTypes.__init__(), which happens every<br>
time the global init() function is called, but then init() puts<br>
the copies back in the global namespace, meaning that the<br>
original is discarded. Basically the only reason for the .copy()<br>
is to make sure that the correct updates are applied to the<br>
apache mimetype defaults, but the code will gladly re-read all<br>
of the apache files even after its mapped types are already in<br>
these dictionaries, essentially making re-initializing a (very<br>
expensive) no-op. All we’re doing is a lot of unnecessary extra<br>
disk reads and memory allocations and deallocations. The only<br>
time this has any effect is when a non-singleton MimeTypes<br>
instance is created, as in the read_mime_types function.<br>
<br>
* And that read_mime_types function is a doozy. It tries to open a<br>
filename, spits back None if there’s an IOError (instead of<br>
raising the exception as it should), and then creates a new<br>
MimeTypes instance (remember, this is identical to the singleton<br>
MimeTypes instance because it starts itself from that one’s<br>
mappings), adds any new types it finds in the file with that<br>
name, and then returns the 'strict' types_map from it. I’m not<br>
sure whether any sane user of this API would expect it to return<br>
the existing type mappings *plus* the extra ones in the provided<br>
filename, but I really can’t imagine this function ever being<br>
particularly useful: it requires you are reading mime types in<br>
apache format, but not the apache mime type files you already<br>
looked at, and then the only way to find out what new mappings<br>
were defined is to take the difference of the default mappings<br>
with the result of the function.<br>
<br>
* The code itself, on a line-by-line basis, is unpythonic and<br>
unnecessarily verbose, confusing, and slow. The code should be<br>
rewritten to use python 2.3–2.6 features: even leaving its<br>
functionality identical it could be cut to about half the number<br>
of lines, and made clearer.<br>
<br>
In case the above doesn’t make this clear: this code is extremely<br>
confusing.</blockquote><div><br></div><div>Yeah, kind of picked up on that. =)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> Trying to read it has caused all the people around me to<br>
look up as I shout "what the fuck??!" at the screen every few<br>
minutes, as each new revelation gives another surprise. I’m not<br>
convinced that I completely understand what the code does, because<br>
it has been quite effectively obfuscated, but I understand enough to<br>
want to throw the whole thing out, and start essentially from<br>
scratch.<br>
<br>
So the question is, what should be done about this? I’d like to hear<br>
how people use the mimetypes module, and how they expect it to work,<br>
to figure out the sanest possible mostly-backwards-compatible<br>
replacement which could be dropped in (ideally this would just allow<br>
the use of default mimetypes and rip out the ability to alter the<br>
default datastore: or is there some easy way to change this away<br>
from a singleton without breaking code which calls these methods?),<br>
and then extend that replacement to support a somewhat saner model<br>
for anyone who actually wants to extend the set of mappings. My<br>
guess is that replacement code could actually fix subtle bugs in<br>
existing uses of this module, by people who had a sane expectation<br>
of how it was supposed to work.<br>
<br>
At the very least, the parts about figuring out exactly which<br>
exceptions to Apache’s set of default types are useful would be a<br>
good idea, and I’d maybe even recommend including an up-to-date copy<br>
of Apache’s mime.types file in the Python Standard Library, and then<br>
only overriding its definitions for future versions of Apache (and<br>
then overriding the combination of both of those with further<br>
exceptions deemed useful for python, with comments explaining why<br>
each exception), so that we’re not bothering to look up horribly<br>
out-of-date types in multiple locations from Apache 1, 1.2, 1.3,<br>
etc. I’d also recommend making the API for overriding definitions be<br>
the same as the code used to declare the default overrides, because<br>
as it is there are three ways do define types: a) in a mime.types<br>
formatted file, b) in a python dictionary that gets initialized with<br>
a confusing bit of code, and c) through the add_type function.<br>
<br>
Does anyone else have thoughts about this, or maybe some good (it<br>
had better be *really* good) explanations why this code is the way<br>
it is? I'd be happy to try to rewrite it, but I think I’d need a bit<br>
of help figuring out how to make the rewrite backwards-compatible.</blockquote><div><br></div><div>So the problem of changing fundamentally how the code works, even for a cleanup, is that it will break someone's code out there because they depended on the module's crazy way of doing things. Now if they are cheating and looking at things that are meant to be hidden you might be able to clean things up, but if the semantics are exposed to the user, then there is not much we can do w/o breaking someone's code.</div>
<div><br></div><div>Honestly, if the code is as bad as it seems -- including its API --, the best bet would be to come up with a new module for handling MIME types from scratch, put it up on the Cheeseshop/PyPI, and get the community behind it. If the community picks it up as the de-facto replacement for mimetypes and the code has settled we can then talk about adding it to the standard library and begin deprecating mimetypes.</div>
<div><br></div><div>And thanks for willing to volunteer to fix this.</div><div><br></div><div>-Brett</div></div>