4
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ yes list comprehension is certainly more performance intuitive. \$\endgroup\$ – Ciasto piekarz Jun 28 '18 at 9:54
2
\$\begingroup\$

In addition to Mathias excelent answer


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()
            ]
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Mathias Ettinger Jun 28 '18 at 19:50
  • \$\begingroup\$ 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: \$\endgroup\$ – Ciasto piekarz Jun 29 '18 at 2:47
  • 2
    \$\begingroup\$ @Ciastopiekarz You can then change with conn as cursor: to with closing(conn.cursor()) as cursor:. \$\endgroup\$ – Mathias Ettinger Jun 29 '18 at 10:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.