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
DM-13963: Enable foreign_key integrity checking on Butler SqlRegistry #28
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall is fine. Can I please have some reasoning in the commit message that switches the datastore tests to a dummy registry rather than the in-memory registry?
@@ -21,7 +21,11 @@ | |||
|
|||
import itertools | |||
|
|||
from sqlite3 import Connection as SQLite3Connection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance we could have the sqlite knowledge in a subclass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should. But I'm not sure we want to do that right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be anything other than copying 20 lines to another file and changing the registry name in the yaml file?
def _enableForeignKeysOnSqlite3(dbapiConnection, connectionRecord): | ||
if isinstance(dbapiConnection, SQLite3Connection): | ||
cursor = dbapiConnection.cursor() | ||
cursor.execute("PRAGMA foreign_keys=ON;") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need a try/finally
here to ensure that cursor.close()
is called? Or the internet gives the alternative form of:
from contextlib import closing
with closing(self.connection.cursor()) as cur:
f9239ca
to
294c917
Compare
This is required because Registry.addStorageInfo enforces its foreign-key relation with Registry.addDataset (which we don't want to have to do in the Datastore only tests).
294c917
to
a4a7337
Compare
No description provided.