HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

MySQL class to add user/database

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
userdatabasemysqlclassadd

Problem

I'm creating tool to add new virtualhost on UNIX-box.
One of tasks is to add a new user and database to MariaDB (aka MySQL) server.
In fact - this my first 'real' attempt to use OOP.

Here is class, which I created for it:

```
class mysqlUserDb:
warnings.filterwarnings('error')
def __init__(self):
self.root = 'root'
self.host = 'localhost'
self.rootpw = 'p@ss'

try:
print '\nChecking MySQL connection...'
self.db = MySQLdb.connect(self.host, self.root, self.rootpw)
self.cursor = self.db.cursor()
self.cursor.execute('select version()')
print 'Connection OK, proceeding.'
except MySQLdb.Error as error:
print 'Error: %s ' %error + '\nStop.\n'
sys.exit()

def createdb(self, db):
print '\nCreating database...'
try:
self.cursor.execute('create database if not exists ' + db)
self.cursor.execute('show databases like ' + '\'' + db + '\'')
dbs = self.cursor.fetchone()
print 'Database created: %s' %dbs
except Warning as warn:
print 'Warning: %s ' %warn + '\nStop.\n'
sys.exit()

def grants(self, user, userpass, db):
print '\nGrant privilegies... '
try:
self.cursor.execute('grant all on ' + db + '.*' + 'to ' + '\'' + user + '\'' + "@'localhost'" + 'identified by ' + '\'' + userpass + '\'')
self.cursor.execute('select user, db from mysql.db where db=' + '\'' + db + '\'')
grs = self.cursor.fetchall()
print 'Access granted: %r' %grs
except Warning as warn:

Solution

(This is in addition to what @200_success already said about exit codes and error handling.)

Your main questions


I'm pretty sure, here is lot of stuff, which I missed or did incorrectly (or not the "Python way").

The convention is to use CamelCase for class names. So MysqlUserDb would be better for your class. See PEP8 for more details.

Don't mix string concatenation and formatting expressions like this:

print 'Error: %s ' % error + '\nStop.\n'


Stick to formatting expressions without concatenation:

print 'Error: %s\nStop.\n' % error


You do this a lot. Apply this technique everywhere.

Similarly, you're not using formatting expressions enough. For example in SQL queries like this one:

self.cursor.execute('show databases like ' + '\'' + db + '\'')


With all those quotations and escaping, it's easy to overlook a mistake. It becomes simpler if you use formatting expression, and if you change the outermost quotes to double quotes:

self.cursor.execute("show databases like '%s'" % db)


Your code appears to be for Python 2.x. To be a bit more ready for a possible future migration to Python 3.x, I recommend to start writing your print ... statements as print(...), for example:

print('Connection OK, proceeding.')



Also, I'm not sure - which way better:



  • create methods for each action (add database, add user) and just call this methods from script (currently done)



  • create one method, which will get needed action ('create database lalala') as argument and call this method several times in script




Your current approach is good: use a separate method for each separate action.

Unused fields

In __init__ you are setting these fields:

self.root = 'root'
self.host = 'localhost'
self.rootpw = 'p@ss'


But then you're not using them anywhere else within the class. So it seems these should not be fields, but perhaps global constants declared at the top of the class. I would also name them a bit differently:

DBROOTUSER = 'root'
DBROOTPASS = 'p@ss'
DBHOST = 'localhost'


Security

Are you sure you want to store the db's root account password in this script? Make sure the file has restricted permissions (do a chmod 0600 on it).

Needless to say, but better make sure the method parameters are cleaned before reaching your class to avoid SQL injection attacks. Or implement validation inside your methods.

Naming

Instead of these parameter names:

def createdb(self, db):
    # ...

def grants(self, user, userpass, db):
    # ...


I recommend this:

def createdb(self, dbname):
    # ...

def grants(self, dbuser, dbpass, dbname):
    # ...


This may be a matter of taste though.

Code Snippets

print 'Error: %s ' % error + '\nStop.\n'
print 'Error: %s\nStop.\n' % error
self.cursor.execute('show databases like ' + '\'' + db + '\'')
self.cursor.execute("show databases like '%s'" % db)
print('Connection OK, proceeding.')

Context

StackExchange Code Review Q#61798, answer score: 8

Revisions (0)

No revisions yet.