Adding Postgres support to database.py #419

Closed
wants to merge 1 commit into
from

2 participants

@JonChu

This commit will allow for a more robust database.py that covers both major open source sql implementations.

A Connection object has been put into database.py with a method create()
that takes in a constant denoting if you want a Postgres connection or a
MySql one. The Postgres library models that of the MySql library with
minor modifications where necessary due to differences in the databases.

The differences are documented in the code, but two to note are that
Postgres by default no longer supports returning lastrowid, so
execute/executemany now returns None or the result of the query (if
RETURNING is specified).

Postgres also does not have max idle times by default so this was
accounted for in code.

@jparise jparise and 1 other commented on an outdated diff Dec 13, 2011
tornado/database.py
import copy
import MySQLdb.constants
import MySQLdb.converters
import MySQLdb.cursors
+import psycopg2
@jparise
jparise added a line comment Dec 13, 2011

I think you should protect the MySQLdb and psycopg2 imports so that neither is required until the point that the user selects a particular database connection type.

@JonChu
JonChu added a line comment Dec 14, 2011

Yea, I agree. I'll go ahead and do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jparise jparise and 1 other commented on an outdated diff Dec 13, 2011
tornado/database.py
class Connection(object):
+ @staticmethod
+ def create(host, database, db_type, user=None, password=None,
@jparise
jparise added a line comment Dec 13, 2011

Instead of using enumeration-ish constants for db_type, what if you either passed in a string (containing the letters "mysql" or "postgresql") or the desired connection class object (MysqlConnection or PostgresConnection)?

@JonChu
JonChu added a line comment Dec 14, 2011

Passing in strings seems really ugly to me. There's a reason why enums exist, and when code gets a lot larger, strings become unmaintainable and generally exhibit bad code smell.

I like the idea of passing in the class object itself, but I'll need to ponder this a little more before I go ahead and do that.

@jparise
jparise added a line comment Dec 14, 2011

I suppose I don't really see the practical benefit of:

from database import MYSQL, Connection
c = Connection('localhost', 'database', MYSQL)

over:

from database import Connection
c = Connection('localhost', 'database', 'mysql')

I'm still typing the letters "mysql", and both cases will result in some sort of runtime failure if I mispel it. If that MYSQL symbol wrapped a particularly interesting value (other than just a faux enum) and was used in more than one place (other than the create() factory method), I'd consider it more useful.

My opinion here is rather Python-specific, by the way, because it doesn't actually have enumerations in the strict typing sense.

@JonChu
JonChu added a line comment Dec 14, 2011

I decided the right thing to do was remove the static method and factory class altogether and just allow the programmer to use duck typing. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JonChu JonChu Adding Postgres support to database.py
database.py has been modified to have a MySQLConnection and PostgresConnection
class, creating connections for each database respectively. The Postgres
library models that of the MySql library with minor modifications where
necessary due to differences in the databases.

The differences are documented in the code, but two to note are that
Postgres by default no longer supports returning lastrowid, so
execute/executemany now returns None or the result of the query (if
RETURNING is specified).

Postgres also does not have max idle times by default so this was
accounted for in code.
9e6f590
@bdarnell
tornadoweb member

I don't really want to add more one-offs to tornado.database, especially if they hardly share any code. I'd consider a refactoring to separate the mysqlisms from the common code so it takes less than 200 lines to add support for a new database, though. I think it would be ideal if there were a tornado.database.BaseConnection class (since the mysql-specific version has unfortunately claimed the name "Connection") which could take either a DB-API connection or a factory function for such a connection. The base class could provide as much functionality as it could from the base API, with database-specific subclasses to provide additional optimizations like MySQL's SSCursors.

@bdarnell
tornadoweb member

The tornado.database module has been removed and now lives at https://github.com/bdarnell/torndb

@bdarnell bdarnell closed this Mar 31, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment