send_file in flask 0.6 issues

[ThomasWaldmann]

See there:

http://hg.moinmo.in/moin/2.0-dev/rev/58eb8f978188

http://hg.moinmo.in/moin/2.0-dev/rev/25de9bb78a9f

Please apply upstream, having to have patched files is obviously a pain for packaging.

[mitsuhiko]

I see the intention but I will only pull this partially. I will add the ability to set a custom etag, the rest will not be pulled for consistency with stdlib and other things.

[mitsuhiko]

Actually after close reviewing I don't see a reason to pull this patch. If you want a custom etag, just don't set the add_etags flag and set it on the response returned:

rv = send_file('filename.txt')
rv.set_etag('whatever you want')
return rv

I don't see the reason why you should have a custom etag anyways when sending files from the filesystem. If you think the default algorithm is not good enough, that could be improved.

Regarding the .name attribute: that should be a valid filename, otherwise the whole send_file method is pointless. If you cannot guarantee that, don't set the value or set it to None.

[ThomasWaldmann]

the whole point of the patch is to remove the wrong assumption that all file-like things have to live on the filesystem (and be accessible via os.path.* functions) and that the .name attribute is always a filesystem filename (which obviously is not neccessarily the case if it is just a file-like object).

as your code does not just accept filesystem filenames, but also file-like objects, restricting it to filesystem files and failing badly for every other file-like object is unneccessary.

once you have seen this, the rest comes rather naturally.

and you can just admit it, filename_or_fp IS ugly. :)

[ThomasWaldmann]

I nothing else helps, you can also just read the python docs:

"""
name

If the file object was created using open(), the name of the file. Otherwise, some string that indicates the source of the file object, of the form "<...>". This is a read-only attribute and may not be present on all file-like objects.
"""

As you see, all file like objects (except file instances created with open) are not required to have a fs filename in there, but your code relies on this wrong assumption and fails badly if this is not the case.

In my case it tried to access cwd/SomeWikiPagename and tried to send it from the filesystem, but as it is no filesystem file, there is NOTHING at that place.

[mitsuhiko]

I will change the function to check if the name attribute points to a valid file. However the x senfile stuff only works for files from the filesystem so the function loses all of its usefulness for files from memory or non filesystem sources.

The filename or fp stuff is an existimg interface of many libraries and also already used in werkzeug and jinja. For consistency that will not be changed.

Do I see this correctly that if .name is Validated that solves mon's issues?

[ThomasWaldmann]

you can't validate it, this is simply impossible for the general case.

if i send a file-like object that happens to have "foo" as .name and (by chance) there happens to be a "foo" file in cwd, you can't just assume that my file-like object was made from that.

and I still see no reasons not to simply apply those patches, they are made so they are backwards compatible even. if you don't like the deprecation warning, then remove it and live on with that api.

[ThomasWaldmann]

the function doesn't "lose all its usefulness" if it is not a fs file, it has code to deal with sending non-fs files (and offering same api for files, no matter whether they live in fs or not is useful for ease-of-use also).

[mitsuhiko]

Then it's not a file-like object, is it?

[mitsuhiko]

I will probably just deprecate the support for file-like object's calling back to name. If a file like object is set x-sendfile is never used and the guessing happens from the given attachment_filename.

[mitsuhiko]

fda1467 deprecates this functionality

[ThomasWaldmann]

the stuff is still misbehaving for file-like objects with a name attr that is not a fs filename

[mitsuhiko]

I said it deprecates, not it changes. In Flask 1.0 the name attribute will no longer be used.