Pulling log messages from a MySQL table

Question

I want to get this code reviewed for a function I wrote that pulls log messages from a MySQL table:

def get_logs_messages(request_id, destination=None):
 """
 get logs from deployment table
 :param int request_id: request id of selected deployment.
 :param str destination: destination site
 :return: list of messagesfor selected deployment based on request id.
 :rtype: list
 """
 conn = MySQLdb.connect(**dbconfig)
 query = "SELECT deployment_id " \
 "from deployment " \
 "WHERE request_id={request_id} " \
 " {dest}".format(
 request_id=request_id,
 dest="AND DESTINATION = '{loc}'".format(loc=destination) if destination else "")
 cursor = conn.cursor()
 cursor.execute(query)
 data = cursor.fetchall()
 cursor.close()
 msgs = []
 for item in data:
 query = "SELECT modified_dt , message from scripts_deployment_logs WHERE deployment_id = {}".format(item[0])
 cursor = conn.cursor()
 cursor.execute(query)
 result = cursor.fetchall()
 for dt, msg in result:
 msgs.append("{dt}: {msg}".format(dt=str(dt), msg=msg))
 cursor.close()
 conn.close()
 return msgs

I get a request ID from a web interface which is using a REST API to call this function, and this function retrieves the deployment id based on which it displays the logs. I am not very fluent with the MySQL part so I have used mysql.connector with simple SQL queries.

Answer 1

Instead of joining the data of two tables in Python, you should une an SQL join query to ask the database do the work for you.

You also should not inject parameters directly into the query and let the driver do it for you by means of parametrized queries.

Lastly, a list-comprehension is better suited to build your output than using append.

Proposed improvements:

def get_logs_messages(request_id, destination=None):
 parameters = (request_id,)
 query = (
 'SELECT logs.modified_dt, logs.message '
 'FROM scripts_deployment_logs AS logs '
 'LEFT JOIN deployment ON logs.deployment_id = deployment.deployment_id '
 'WHERE deployment.request_id = %s'
 )
 if destination is not None:
 query += ' AND deployment.destination = %s'
 parameters = (request_id, destination)
 conn = MySQLdb.connect(**dbconfig)
 cursor = conn.cursor()
 cursor.execute(query, parameters)
 messages = [
 '{}: {}'.format(dt, message)
 for dt, message in cursor.fetchall()
 ]
 cursor.close()
 return messages

• Ciasto piekarz: yes list comprehension is certainly more performance intuitive.

Answer 2

In addition to Mathias excelent answer

• You could use context managers, so the connection will close automagically, when you are done. See PEP#343
• Don't string.format SQL queries, since this may lead to SQLi vulnerabilities.

---

from contextlib import closing
def get_logs_messages(request_id, destination=None):
 ...
 with closing(MySQLdb.connect(**dbconfig)) as conn: 
 with conn as cursor:
 cursor.execute(query, parameters)
 return [
 '{}: {}'.format(dt, message)
 for dt, message in cursor.fetchall()
 ]

• 301_Moved_Permanently: Since this is not part of the DB API 2.0, I wasn't sure if MySQLdb implemented such behaviours. Seems like every major DB driver adopted the practice anyways. And now that you have context managers, you can directly return the list-comprehension.
• Ciasto piekarz: I am using mysql.connector so I am doing from mysql.connector import MySQLConnection. on using @Ludisposed suggestion I get AttributeError: __exit__ after with conn as cursor:
• 301_Moved_Permanently: @Ciastopiekarz You can then change with conn as cursor: to with closing(conn.cursor()) as cursor:.