patternpythonMinor
MySQL class to add user/database
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:
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
Don't mix string concatenation and formatting expressions like this:
Stick to formatting expressions without concatenation:
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:
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:
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
Also, I'm not sure - which way better:
Your current approach is good: use a separate method for each separate action.
Unused fields
In
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:
Security
Are you sure you want to store the db's
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:
I recommend this:
This may be a matter of taste though.
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' % errorYou 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' % errorself.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.