Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Simple post and download web app in Bottle:

server.py

import sys    
import bottle
from bottle import request, response
import pymongo
import gridfs
from bson.objectid import ObjectId

import mimetypes

mimetypes.init()


# Mongo Connection

connection = pymongo.MongoClient("mongodb://localhost")
db = connection.pacman
test_col = db.test
fs = gridfs.GridFS(db)

# Static Routes
@bottle.get('/<filename:re:.*\.js>')
def javascripts(filename):
    return bottle.static_file(filename, root='static/js')


@bottle.get('/<filename:re:.*\.css>')
def stylesheets(filename):
    return bottle.static_file(filename, root='static/css')


@bottle.get('/<filename:re:.*\.(jpg|png|gif|ico)>')
def images(filename):
    return bottle.static_file(filename, root='static/img')


@bottle.get('/<filename:re:.*\.(eot|ttf|woff|svg)>')
def fonts(filename):
    return bottle.static_file(filename, root='static/fonts')


@bottle.route('/')
def home_page():
    result = db.fs.files.find({"filename": {"$exists": True}})

    return bottle.template('files_test/home', files=list(result))


@bottle.route('/add')
def add_page():
    return bottle.template('files_test/add')


@bottle.route('/upload', method='POST')
def do_upload():
    data = request.files.data
    if data:
        raw = data.file.read()  # This is dangerous for big files
        file_name = data.filename
        try:
            newfile_id = fs.put(raw, filename=file_name)
        except:
            return "error inserting new file"
    print(newfile_id)
    return bottle.redirect('/')


@bottle.route('/download')
def download():
    file_id = ObjectId(bottle.request.query.id)
    if file_id:
        try:
            file_to_download = fs.get(file_id)
        except:
            return "document id not found for id:" + file_id, sys.exc_info()[0]
        file_extension = str(file_to_download.name)[(str(file_to_download.name).index('.')):]

        response.headers['Content-Type'] = (mimetypes.types_map[file_extension])
        response.headers['Content-Disposition'] = 'attachment; filename=' + file_to_download.name
        response.content_length = file_to_download.length
        # print response
        return file_to_download

bottle.debug(True)
bottle.run(host='localhost', port=8080)

add.tpl

<!DOCTYPE html>

<!-- pass in files (dict)-->
<html>
<head>
    <title>Preformance and Capacity Management</title>
    <link rel="stylesheet" type="text/css" href="style.css">
</head>
<body>
<form action="/upload" method="post" enctype="multipart/form-data">
  <input type="file" name="data" /><br>
    <input type="submit"/>
</form>
</body>
</html>

home.tpl

<!DOCTYPE html>

<!-- pass in files (dict)-->
<html>
<head>
    <title>Preformance and Capacity Management</title>
    <link rel="stylesheet" type="text/css" href="style.css">
</head>
<body>

<br><br><br><br>
%for fileobj in files:
    Filename: {{fileobj['filename']}}<br>
    Size: {{fileobj['length']}}<br>
    Date Uploaded: {{fileobj['uploadDate']}}<br>
    md5: {{fileobj['md5']}}<br>
    <a href="download?id={{fileobj['_id']}}">&lt--Download File--&gt</a>


<br><br><br><br>
%end



</body>
</html>
share|improve this question
1  
It's on-topic now, but would be a better question if you edited it to include some of the information described in What makes a good question? –  ChrisW Feb 18 '14 at 22:17

1 Answer 1

up vote 2 down vote accepted

Global

You're hard coding the URL to your Mongo instance; it might be better to load it from a settings file. You're also not performing any authentication, though we'll assume that you have your firewalls properly set up, I guess. Some people prefer opening a new connection per request, up to you I guess. I would say that global variables suck, but if you don't think your program will expand beyond a single script, then it could be OK.

Static views

You could combine all of the static views fairly simply; have one view that captures everything matching <filename:re:.*\.(.+)> (or whatever the correct regexp would be), and then have a dictionary mapping from extensions to directories:

STATIC_DIRS = {
    "jpg": "static/images",
    "gif": "static/images",
    ... # etc
}

That said, I don't see the advantage of being able to do /foo.jpg over /static/images/foo.jpg. Also, you could always eliminate the entire set of views and just serve your static content straight from the Nginx/Apache instance that is reverse proxying your application.

Do Upload

I would strongly suggest you look into reading the file in chunks; as it is, your server would be trivial to DOS.

It might create nicer code if you reversed your if condition, so the code looked more like

@bottle.route('/upload', method='POST')
def do_upload():
    data = request.files.data
    if not data:
        return bottle.redirect("/")

    raw = data.file.read()  # This is dangerous for big files
    file_name = data.filename
    try:
        newfile_id = fs.put(raw, filename=file_name)
    except:
        return "error inserting new file"
    print(newfile_id)
    return bottle.redirect('/')

This is known as a guard statement, and they are very nice.

Avoid using try: ... except: ... without specifying the type of the exception you wish to catch. In this case, it's particularly bad as you are not even logging the error; someone could come along, shoot a bullet through the hard drive of the machine you were running this on, and your service wouldn't note that anything of interest had happened. I'd suggest that when a request fails, you return a non-200 status code; at the moment, your bullet ridden hard drive will not cause a single non-200 status code.

Download

If the file exists in Mongo, but not in the FS, currently you say document id not found for id. However, due to the except statement which doesn't specify exception types, your code also says that no matter what else goes wrong (recall the hard drive with a gun wound). I'd find out what class of errors come under the file not found category, and then treat them differently to others.

Currently, if the user looks up a file that doesn't exist, you return None, which I think bottle will complain about. It'd probably be better to add a guard clause along the lines of

if not file_id:
    # I don't know the bottle api for returning a particular status code, but something like this
    return bottle.abort(404)
... # otherwise serve the file

You don't handle the case that someone gives you a file you don't know the mimetype for, I'd recommend checking before you try and set the header.

General

It's best to include a guard at the bottom of the file so that you can import the file without starting the webserver. I'd modify the last few lines to read

if __name__ == '__main__':
    bottle.debug(True)
    bottle.run(host='localhost', port=8080)
share|improve this answer
    
Thank you for all the comments. I haven't been able to configure a nginx reverse proxy and get it to work. Also, any suggestions on where I can learn some of the things you suggested? As I said, I'm not a full time coder. –  Jeff Feb 19 '14 at 22:51
1  
Frankly setting up nginx/gunicorn is the part of python web development I hate the most. I usually refer to the gunicorn documentation (docs.gunicorn.org/en/latest/deploy.html), but I wouldn't worry too much in this case; if you're just developing an application, then I wouldn't worry about it. I'd focus on getting the application working correctly before worrying about stuff like performance :) –  BluePeppers Feb 19 '14 at 23:35
    
Thanks for the input! I decided to upload the project to Github and try to make your changes. Feel free to look in / contribute if you wanted. github.com/jdell64/IPACS –  Jeff Feb 20 '14 at 17:52
    
One more follow-up, you mentioned reading in chunks -- how do you do that? –  Jeff Apr 2 '14 at 20:18

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.