3
\$\begingroup\$

I needed to create a JSON file from a couple of MySQL tables to migrate some recipes from wordpress. This was my first time writing a script in python and would like to get some feedback on what I could have done better. I come from a Javascript background.

import mysql.connector
import json

cnx = mysql.connector.connect(
    database='somedb', user='root', password='root', host='localhost'
)

cnx1 = mysql.connector.connect(
    database='somedb', user='root', password='root', host='localhost'
)

cursor = cnx.cursor();
cursor1 = cnx1.cursor();

query = "SELECT a.recipe_id, a.recipe_item_type, b.meta_key, b.meta_value, b.recipe_item_id FROM wp_simmer_recipe_items a, wp_simmer_recipe_itemmeta b WHERE a.recipe_item_id = b.recipe_item_id GROUP BY a.recipe_item_id"
query1 = "SELECT * FROM wp_simmer_recipe_itemmeta WHERE recipe_item_id=(%s)"

cursor.execute(query)
rs = cursor.fetchall()

data = {}

for row in rs:
    if row[1] == 'instruction':
        cursor1.execute(query1, (row[4],))
        insD = cursor1.fetchall()
        for instruction in insD:
            if instruction[2] != 'is_heading':
                data.setdefault(row[0], {'instructions':[], 'ingredients': []})['instructions'].append(instruction[3]);
    else:
        cursor1.execute(query1, (row[4],))
        rd = cursor1.fetchall()
        ingredient = {}
        for itemMeta in rd:
            ingredient[itemMeta[2]] = itemMeta[3]
        data.setdefault(row[0], {'ingredients': [], 'instructions': []})['ingredients'].append(ingredient)


with open('data.json', 'w') as outfile:
    json.dump(data, outfile, sort_keys=True, indent=4)

cursor1.close()
cnx1.close()
cursor.close()
cnx.close()
\$\endgroup\$
0

2 Answers 2

3
\$\begingroup\$

Preface

For this review, we shall need to refer to the database schema for the WordPress Simmer plugin:

if ( $items_table_name != $wpdb->get_var( "SHOW TABLES LIKE '$items_table_name'" ) ) {
        // The recipe items table.
        $query .= "CREATE TABLE $items_table_name (
               recipe_item_id bigint(20) NOT NULL auto_increment,
               recipe_item_type varchar(200) NOT NULL DEFAULT '',
               recipe_id bigint(20) NOT NULL,
               recipe_item_order int(11) NOT NULL DEFAULT '0',
               PRIMARY KEY  (recipe_item_id),
               KEY recipe_id (recipe_id)
               ) $charset_collate;";
}

if ( $item_meta_table_name != $wpdb->get_var( "SHOW TABLES LIKE '$item_meta_table_name'" ) ) {

        // The recipe item meta table.
        $query .= "CREATE TABLE $item_meta_table_name (
               meta_id bigint(20) NOT NULL auto_increment,
               recipe_item_id bigint(20) NOT NULL,
               meta_key varchar(255) NULL,
               meta_value longtext NULL,
               PRIMARY KEY  (meta_id),
               KEY recipe_item_id (recipe_item_id),
               KEY meta_key (meta_key)
               ) $charset_collate;";
}

Querying

To start, let's format your first query for readability:

query = """
    SELECT a.recipe_id, a.recipe_item_type
         , b.meta_key, b.meta_value, b.recipe_item_id
        FROM wp_simmer_recipe_items a, wp_simmer_recipe_itemmeta b
        WHERE a.recipe_item_id = b.recipe_item_id
        GROUP BY a.recipe_item_id
"""

Your query has a non-sensical GROUP BY clause. The only way that this query could be legal is if the schema and query are formulated so as to guarantee that each recipe_item_id only appears at most once in the query result. While the wp_simmer_recipe_items table does have a PRIMARY KEY (recipe_item_id) constraint, there is no such uniqueness guarantee in the wp_simmer_recipe_itemmeta table. MySQL < 5.7.5 somehow executes the query despite the fact that it makes no sense; in MySQL ≥ 5.7.5, it should fail with an error, as it should in any sane SQL implementation:

ERROR 1055 (42000): Expression #3 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'somedb.b.meta_key' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by

Based on the query results, you build the ingredients and instructions lists by appending entries. However, you fail to ORDER BY recipe_item_order, so the ingredients and instructions may appear in any order. (Try baking the bread before mixing the dough!)

When joining tables, write the query using JOIN. It's more readable than implicitly joining tables using the WHERE clause.

The worst sin in this code is executing query1 for each recipe found by query. This is a bad idea for two reasons:

  • For good performance, you should never execute queries in a loop, especially when the number of secondary queries depends on the length of the results of the primary query. Each additional query necessitates a round trip to the server, and takes time to interpret and execute.

  • The information returned by query1 is entirely redundant — all of the information you need from it (namely, the meta_key and meta_value for each recipe_item_id) is already contained within the results of query!

Furthermore, you don't need to make two connections to the same database. All you need is two cursors, which can be created from just one connection.

Suggested solution

Execute two queries: one to get the instructions, one to get the ingredients. (Here, I've taken a shortcut and assumed that every recipe will have at least one instruction and at least one ingredient.)

Take advantage of itertools.groupby() to help construct the inner lists and dictionaries.

from itertools import groupby
import json
from operator import itemgetter
import mysql.connector

cnx = mysql.connector.connect(
    database='somedb', user='root', password='root', host='localhost'
)
cursor = cnx.cursor()
data = {}

cursor.execute("""
    SELECT item.recipe_id
         , meta.meta_value AS instruction_text
        FROM wp_simmer_recipe_items item
            INNER JOIN wp_simmer_recipe_itemmeta meta
                ON item.recipe_item_id = meta.recipe_item_id
        WHERE
            item.recipe_item_type = 'instruction'
            AND meta.meta_key <> 'is_heading'
        ORDER BY item.recipe_id, item.recipe_item_order
""")
for recipe_id, instructions in groupby(cursor.fetchall(), itemgetter(0)):
    data[recipe_id] = {
        'instructions': [row[1] for row in instructions],
        'ingredients': [],
    }

cursor.execute("""
    SELECT item.recipe_id
         , meta.recipe_item_id
         , meta.meta_key
         , meta.meta_value
        FROM wp_simmer_recipe_items item
            INNER JOIN wp_simmer_recipe_itemmeta meta
                ON item.recipe_item_id = meta.recipe_item_id
        WHERE
            item.recipe_item_type <> 'instruction'
        ORDER BY item.recipe_id, item.recipe_item_order, meta.meta_key
""")
for (recipe_id, item_id), item in groupby(cursor.fetchall(), itemgetter(0, 1)):
    data[recipe_id]['ingredients'].append({row[2]: row[3] for row in item})

cursor.close()
cnx.close()

with open('data.json', 'w') as outfile:
    json.dump(data, outfile, sort_keys=True, indent=4)

Further discussion

MySQL, since version 5.7.22, has JSON support. It would be nice to rewrite the SQL such that the entire JSON result could be returned from just a single query. Unfortunately, the JSON_ARRAYAGG() function does not let you specify the order of its results:

JSON_ARRAYAGG(col_or_expr)

Aggregates a result set as a single JSON array whose elements consist of the rows. The order of elements in this array is undefined.

That, to me, would be a deal-breaker for exporting a recipe.

\$\endgroup\$
2
  • \$\begingroup\$ It would be nice to also give clear names for row[2]: row[3] for row in item_attributes, instead of indexing row. \$\endgroup\$
    – Graipher
    Commented Aug 14, 2018 at 5:05
  • 1
    \$\begingroup\$ @Graipher You could use cnx.cursor(dictionary=True) for better readability (presumably with a negligible performance impact). \$\endgroup\$ Commented Aug 14, 2018 at 5:14
1
\$\begingroup\$

Object-relational mapping

Consider using an ORM framework, such like SQLAlchemy or peewee.
it will significally enhance code readability and reusability.

With peewee:

class WpSimmerRecipeItems(Model):

    class Meta:
        table_name = 'wp_simmer_recipe_items'

    recipe_id = AutoField()
    …


class WpSimmerRecipeItemMeta(Mode):

    class Meta:
        table_name = 'wp_simmer_recipe_itemmeta'

    …

The respective select query:

selection = WpSimmerRecipeItems.select(
    WpSimmerRecipeItems.recipe_id,
    WpSimmerRecipeItems.recipe_item_type,
    WpSimmerRecipeItemMeta.meta_key,
    WpSimmerRecipeItemMeta.meta_value,
    WpSimmerRecipeItemMeta.recipe_item_id).join(
        WpSimmerRecipeItemMeta,
        on=(WpSimmerRecipeItems.recipe_item_id == WpSimmerRecipeItemMeta.recipe_item_id).group_by(
            WpSimmerRecipeItems.recipe_item_id)

Unfortunately I cannot go into too much detail here, since I don't know your database's structure.

In the case of peewee this would also make the JSON conversion trivial using playhouse.shortcuts.model_to_dict:

json_list = [model_to_dict(record) for record in selection]
\$\endgroup\$
1
  • \$\begingroup\$ Perhaps a better ORM would be the WordPress Simmer plugin itself, in which case the JSON exporter should be written in PHP. \$\endgroup\$ Commented Aug 14, 2018 at 13:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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