added foreign key constraint support to SQLite #50
Conversation
src/cs50/sql.py
Outdated
| @@ -32,12 +33,23 @@ def __init__(self, url, **kwargs): | |||
| if not os.path.isfile(matches.group(1)): | |||
| raise RuntimeError("not a file: {}".format(matches.group(1))) | |||
|
|
|||
| # Create engine, raising exception if back end's module not installed | |||
| self.engine = sqlalchemy.create_engine(url, **kwargs) | |||
| foreign_keys_enabled = kwargs.pop("foreign_keys_enabled", False) | |||
There was a problem hiding this comment.
Where did foreign_keys_enabled come from, is that your choice of arg names? Or borrowed from SQLAlchemy? If former, should probably be just foreign_keys, for consistency with the PRAGMA directive, http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html#foreign-key-support?
src/cs50/sql.py
Outdated
| # Create engine, raising exception if back end's module not installed | ||
| self.engine = sqlalchemy.create_engine(url, **kwargs) | ||
|
|
||
| # Whether to enable foreign key constraints |
There was a problem hiding this comment.
For consistency, I would change this to an imperative: "Enable foreign key constraints". (The conditional aspect can be inferred from the if.)
src/cs50/sql.py
Outdated
| # Whether to enable foreign key constraints | ||
| if foreign_keys_enabled: | ||
| sqlalchemy.event.listen(self.engine, "connect", _on_connect) | ||
| else: |
There was a problem hiding this comment.
Does PEP require a blank line above comment? Not sure.
There was a problem hiding this comment.
I thought that was your convention? 😛
There was a problem hiding this comment.
Don't think I've had occasion to write such? I'd just use _connect then.
There was a problem hiding this comment.
That was referring to newline before comment.
There was a problem hiding this comment.
Oh i do put a blank line below the else:.
There was a problem hiding this comment.
Ehm, I actually thought I put a blank line and you were arguing about why it's there 😇 Added.
tests/sql.py
Outdated
| @@ -107,18 +107,26 @@ class SQLiteTests(SQLTests): | |||
| @classmethod | |||
| def setUpClass(self): | |||
| self.db = SQL("sqlite:///test.db") | |||
| self.db1 = SQL("sqlite:///test1.db", foreign_keys_enabled=True) | |||
src/cs50/sql.py
Outdated
|
|
||
|
|
||
| # http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html#foreign-key-support | ||
| def _on_connect(dbapi_connection, connection_record): |
There was a problem hiding this comment.
Is _on_connect conventional? Or should we just call it _connect so that it matches the event name?
|
|
||
| def setUp(self): | ||
| self.db.execute("CREATE TABLE cs50(id INTEGER PRIMARY KEY, val TEXT)") | ||
|
|
||
| def multi_inserts_enabled(self): | ||
| return False |
|
Good catch. Did a student's code reveal this omission? |
@dmalan sqlite ignores foreign key constraints by default and they have to be enabled by hooking into the
connectevent per http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html#foreign-key-support. This PR adds support for optionally respecting foreign key constraints. Users would just have to passforeign_keys_enabled=TruetoSQL's constructor.As an aside this PR also restores the original value for
logger.disabledafter temporarily disabling it per89657ea.
Any thoughts?
PS, thanks to Wellington Rampazo who reported issues with foreign keys when using the library!