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.

I have a bit of code to insert some values in a table. It seems a bit clunky - is there a neater way?

words is a list of dictionaries, and the entries contain other fields which I don't want to insert.

# add the list of words to the words table
time_stamp = datetime.datetime.now().isoformat()
sql = "INSERT INTO words (word, user_id, game_id, score, time_stamp) VALUES "
vals = []
for word in words:
    vals.append("('%s', %s, %s, %s, '%s')" %
                (word['str'], user_id, game_id, word['score'], time_stamp))
cur.execute(sql + ",".join(vals))
share|improve this question
    
If user_id and game_id came from the user/UI, would you assemble the sql string that way? –  Mat's Mug Jun 3 at 16:26

1 Answer 1

up vote 4 down vote accepted

There is a neater, more Pythonic way of doing this using the str.format function. However, it is not the way you should do it.

Currently, the way you are building your query, you are vulnerable to SQL injection. This is the point @Mat's Mug was getting at in his comment. What if a malicious user decides to give the following as a user ID:

user_id = "1,1,99,'today');DROP TABLES;INSERT INTO words (word, user_id, game_id, score, time_stamp) VALUES ('Goodbye', 1"

If this happens, a disaster beyond your imagination will occur*!

To prevent this, the cursor from the MySQLdb module (which I assume you are using) allows you to pass in a tuple or dict of the parameters to the execute function:

time_stamp = datetime.datetime.now().isoformat()
sql = "INSERT INTO words (word, user_id, game_id, score, time_stamp) VALUES {}"

# Create the query
sql = sql.format(','.join(["('%s', %s, %s, %s, '%s')" for _ in words]))

# Create the parameter tuple.
params = ()
for word in words:
    params += (word['str'], user_id, game_id, word['score'], time_stamp,)

# Sending a tuple is easiest in this situation
cur.execute(sql, params)

If you want to look at another database solution for Python, take a look at SQLAlchemy. Its a pretty powerful framework that makes working with databases much simpler.


*=Sorry for the Phantom of the Opera quote. I'm feeling oddly dramatic right now.

Update

To answer your question about removing the for-loop, the best you can hope for is to use a list comprehension. In many situations, list comprehensions are typically the Pythonic way of iterating over a collection. However, in your situation, this probably is not the case.

The comprehension for your current code would look like this:

','.join([str(tuple([word['str'], user_id, game_id, word['score'], time_stamp]))
          for word in words])

This is really only effective creating the end to your query. Unfortunately, in addition to being hard to read, it is still vulnerable to SQL injection.

In your case, I think a for-loop would be the cleanest way to implement what you want.

share|improve this answer
    
Sorry, I'm well aware that it's not secure - I removed the escaping code to highlight the issue which is: can I remove the for loop somehow? Is there a more idiomatic way to build the params [string|tuple]? –  Charlie Skilbeck Jun 4 at 10:20
    
I have added a section addressing your question. –  BeetDemGuise Jun 4 at 12:44
    
OK, thanks for your help! –  Charlie Skilbeck Jun 4 at 13:04

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.