Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

All SQLite access converted to use context manager and prepared statements #64

Merged
merged 3 commits into from
Sep 18, 2016
Merged

Conversation

RodentOfUnusualSize
Copy link
Collaborator

This pull request resolves issue #37.

Three classes were changed, but all were changed substantially.

Basically, the classes previously all had a pair of functions: __openDB() and __closeDB(), and every function that used the database had to call first one then the other. __closeDB() was not only tasked with closing the database, it also committed any outstanding transactions.

There are two main problems with this.

  1. It is error-prone. It is far too easy to get lost in a long function and return without calling __closeDB(). (See, for example, Users.uidExists() in users.py.) It is also not exception-safe - if an exception were to be thrown after a successful database command but before __closeDB(), the effects of the command would basically be lost.
  2. It is very inefficient. There were several functions that called other functions that opened the database, did something, then closed the database... then the caller re-opened the database and did something else. (This problem still remains in the Seen class, but that's a whole other issue.)

Generally, all functions in all three classes that actually accessed databases were converted from this form:

def f(self):
    self.__openDB()

    # self.db is set in __openDB()
    self.db.execute(query)

    data = self.db.fetchone()
    if data is not None:
        # return data
    else:
        # return defaults

to this form:

def f(self):
    with contextlib.closing(sqlite3.connect(self.__dbname)) as connection:
        with connection as cursor:
            for data in cursor.execute(query):
                # return data
    #return defaults

All SQL statements were also pulled out of the code and placed separately at the top of the files. This will make it easier to figure out any changes that have to be made if the database structure changes.

This pull request also resolves several other minor issues.

A potential future improvement is a new module database, with a db_open() function. Then there would be no need for the contextlib machinery anywhere else, leaving you with this:

from .database import db_open

def f(self):
    with db_open(self.__dbname) as db:
        with db:
            # first transaction
            db.execute(query)
        # first transaction auto-commits
        with db:
            # second transaction
            db.execute(query)
        # second transaction auto-commits
    # database auto-closes

@dwhagar
Copy link
Owner

dwhagar commented Sep 18, 2016

I made a lot of changes to the seen.py file, can you take a look and see if the commit could be then resolved? Truth be told, I'm not sure HOW to resolve a commit conflict.

@RodentOfUnusualSize
Copy link
Collaborator Author

No problem!

@RodentOfUnusualSize RodentOfUnusualSize merged commit cfac7df into dwhagar:dev Sep 18, 2016
RodentOfUnusualSize added a commit that referenced this pull request Sep 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants