4
\$\begingroup\$

this is my first time learning sqlite and I'm still wondering how to do it. I ended up with this sort of library code:

database_path = "userfiles\databases"

def connect(database):
    """Open a connection and a cursor to the database file provided."""
    conexion = sqlite3.connect(database_path + database + ".db")
    cursor = conexion.cursor()
    return {"conexion":conexion, "cursor":cursor}

def disconnect(conexion, cursor, savechanges=False):
    """Disconnect from database, closing the cursor first.
    and saving the cursor data with a commit."""
    cursor.close()
    if savechanges:
        conexion.commit()
    conexion.close()

def build_sqlquery(**sqlquery):
    """Create the query that we will send to the sqlite file."""
    if "select" in sqlquery:
        query_built = "SELECT " + str(sqlquery["select"])
        if "fromtable" in sqlquery:
            query_built = query_built + " FROM " + str(sqlquery["fromtable"])
    elif "insert_into" in sqlquery:
        query_built = "INSERT INTO " + str(sqlquery["insert_into"])
        if "values" in sqlquery:
            query_built = query_built + " VALUES " + str(sqlquery["values"])
    elif "delete" in sqlquery:
        query_built = "DELETE FROM " + str(sqlquery["delete"])
    elif "update" in sqlquery:
        query_built = "UPDATE " + str(sqlquery["update"])
        if "set_values" in sqlquery:
            query_built = query_built + " SET " + str(sqlquery["set_values"])
    if "where" in sqlquery:
        query_built = query_built + " WHERE " + str(sqlquery["where"])
    if "limit" in sqlquery:
        query_built = query_built + " LIMIT " + str(sqlquery["limit"])
    return query_built

def findifexists(cursor, pydata=False, **sqlquery):
    """Query the params into a working sqlite query and
    execute the result, including the pydata if found."""
    query_built = build_sqlquery(sqlquery)
    if not pydata:
        cursor.execute(query_built)
    else:
        cursor.execute(query_built, pydata)
    datos_de_registro = cursor.fetchone() is not None
    return datos_de_registro

And then, once that library is created, I executed my queries like this:

conn = connect("food")
exists = findifexists(conn["cursor"], (value1, value2, value3, condition1), 
             select="(?,?,?)",
             fromtable="mytable", 
             where="Something = ?")
disconnect(conn["conexion"], conn["cursor"])
if exists:
    print("Yes, it exists")
else:
    print("Nope")

Now my concern is that it feels like re-inventing the wheel, where I could just throw an SQLite query directly at the caller without building it up in the build_sqlquerymethod.

What do you think about it? What cons or pros does this have? Also: If I want to execute multiple queries, It's okay to keep a conection and a cursor open to execute them one after another before closing them? Or should I connect, save something, disconnect, and then connect again and repeat the procces?

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

Code improvements

Going from top to bottom, here are some improvements I would suggest.


If database_path is a constant, it should be named instead DATABASE_PATH, or some other all-caps name, to make it evident that it shouldn't be changed. You may consider storing this outside of the code in some config file, so if it ever changes you only need to change the config file and not the code.


Since you are using Python 3, consider using type hints. This will make your code easier to read, as well as allow static analysis of your code to make sure the correct types are used. For example, here would be your connect function with type hints. Note the added import from typing, which has common types like List, Dict, and Tuple.

from typing import Dict

def connect(database: str) -> Dict:
    """Open a connection and a cursor to the database file provided."""
    conexion = sqlite3.connect(database_path + database + ".db")
    cursor = conexion.cursor()
    return {"conexion":conexion, "cursor":cursor}

Instead of making up a word like conexion, consider just adding an underscore at the end when you are worried about using a reserved keyword, i.e.:

connection_ = sqlite3.connect(database_path + database + ".db")

I would recommend using the string .format() method instead of +'ing strings together. For example:

connection_ = sqlite3.connect("{0}{1}.db".format(database_path, database))

It makes the intention more clear, and makes sure everything is the right type to avoid type errors. You could also split the string building by itself for easier reading:

connection_string = "{0}{1}.db".format(database_path, database)
connection_ = sqlite3.connect(connection_string)

Design improvements

Having worked with SQL databases quite a bit, I personally think it is a bad idea to dynamically build SQL query strings as you are doing. The main reasons are as follows:

  1. They are error-prone
  2. They have potential security risks
  3. They usually have poor performance

To briefly address each:

  1. Imagine that you (or another programmer) makes a typing error, say typing "Usres" instead of "Users", the SQL DB will return an error stating that the "Usres" object doesn't exist. Then you need to build in error handling in your functions to account for misspelled or otherwise non-existent database objects.

  2. Imagine you had some kind of input form where a user types in some information, for example, their user name; then, you put that into your SQL query string and execute it on the DB. It's all well and good, as long as the user types something that could be valid. But there is a risk for what has become jokingly known as a Bobby Tables problem, or SQL injection.

Take this for example: query_built = query_built + " WHERE " + str(sqlquery["where"])

Say I entered this as user name: Phrancis'; DROP TABLE Users;--

Your executable query string would then look something like this:

"SELECT Id FROM Users WHERE Name = 'Phrancis'; DROP TABLE Users;--'"

Notice that trailing single quote at the very end after --, which is now commented out and ignored, and the single quote I put in the user name is used instead.

And if SQL executed that, then, you better have a recent backup of your users table!

  1. For performance, queries that are executed "ad hoc" (or, basically, improvised) on the database usually have a poorer performance than queries that are stored. The SQL database usually has to recalculate execution plans for ad hoc queries each time, as opposed to re-using the execution plan from the last time(s) a similar query was executed. On busy production databases, this can make a significant impact on the database processing load.

Ultimately, for any production code, database queries should each have their own functions/methods, as in this example:

def check_user_exists(user_name: str) -> bool:
    query = "SELECT id FROM users WHERE name = ?"
    cursor.execute(query, user_name)
    # etc.
\$\endgroup\$
  • \$\begingroup\$ This actually explains what is wrong and why quite well. I really appreciate the insight. But if I understood you correctly, what prevents exploit on your last function check_user_exists if the user_name is Phrancis'; DROP TABLE Users;--? \$\endgroup\$ – Saelyth Aug 26 '17 at 9:59
  • 2
    \$\begingroup\$ Nevermind, I am reading bobby-tables.com/python and it helped me to understand how Sql injection works and how to avoid it. \$\endgroup\$ – Saelyth Aug 26 '17 at 10:12
  • \$\begingroup\$ Great! Happy to help =) \$\endgroup\$ – Phrancis Aug 26 '17 at 11:16
1
\$\begingroup\$

In connect(), it seems a little odd that database apparently needs to start with a slash (or backslash, according to OS). One might use os.path.join(), for example. The return value is a dict, but might easily be a 2-tuple.

The docstring for build_sqlquery() is not very specific. It should describe the keys we expect callers to use.

Using

query_built = "SELECT " + str(sqlquery["select"])

is natural enough, but one might instead use 'SELECT %s' % sqlquery['select'], or even simple string catenation.

Consider using sqlalchemy. If nothing else, it offers nice portability across databases, and can potentially smooth over some vendor-specific SQL differences.

If I want to execute multiple queries, It's okay to keep a conection and a cursor open to execute them one after another before closing them?

Yes, that is definitely an appropriate strategy. Keep a connection open as long as you can.

\$\endgroup\$
  • \$\begingroup\$ yeah that backslash at database was a typo. My real question is if it's even worth to create the build_sqlquery method or should I just run an sql query directly at the code as: SELECT something FROM this WHERE condition. I don't see the benefits of one over another. \$\endgroup\$ – Saelyth Aug 26 '17 at 4:54
  • \$\begingroup\$ You asked if build_sqlquery() is worth it. No, I don't see that it is. Sometimes a bunch of SELECTs will have items in common that can usefully be factored out, but I'm not seeing that in the code you presented. \$\endgroup\$ – J_H Aug 27 '17 at 6:24

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.