From d0c8a1f46f0d4caf50fed7270eedf07cc6603a54 Mon Sep 17 00:00:00 2001 From: Kit Yan Choi Date: Wed, 13 Jul 2016 14:22:45 +0100 Subject: [PATCH 1/5] Add foreign key support for sqlite and thereby support cascade deletions --- remoteappmanager/cli/remoteappdb/__main__.py | 8 +------ remoteappmanager/db/orm.py | 24 +++++++++++++++----- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/remoteappmanager/cli/remoteappdb/__main__.py b/remoteappmanager/cli/remoteappdb/__main__.py index 0b78dad95..354e4327c 100644 --- a/remoteappmanager/cli/remoteappdb/__main__.py +++ b/remoteappmanager/cli/remoteappdb/__main__.py @@ -145,12 +145,8 @@ def remove(ctx, user): orm_user = session.query(orm.User).filter( orm.User.name == user).one() - # Unfortunately sqlite does not support cascading, so we need to - # perform the cleanup of the accounting manually. - session.query(orm.Accounting).filter( - orm.Accounting.user == orm_user).delete() - session.delete(orm_user) + except sqlalchemy.orm.exc.NoResultFound: print_error("Could not find user {}".format(user)) @@ -249,8 +245,6 @@ def remove(ctx, image): app = session.query(orm.Application).filter( orm.Application.image == image).one() - session.query(orm.Accounting).filter( - orm.Accounting.application_id == app.id).delete() session.delete(app) except sqlalchemy.orm.exc.NoResultFound: print_error("Could not find application for image {}".format(image)) diff --git a/remoteappmanager/db/orm.py b/remoteappmanager/db/orm.py index 1af26d1f0..d037fd857 100644 --- a/remoteappmanager/db/orm.py +++ b/remoteappmanager/db/orm.py @@ -1,8 +1,10 @@ import contextlib +import sqlite3 from sqlalchemy import ( Column, Integer, Boolean, Unicode, ForeignKey, create_engine, Enum, - literal) + literal, event) +from sqlalchemy.engine import Engine from sqlalchemy.exc import OperationalError from sqlalchemy.ext.declarative import declarative_base from sqlalchemy.orm import sessionmaker, relationship @@ -72,16 +74,17 @@ class Accounting(Base): __tablename__ = "accounting" user_id = Column(Integer, - ForeignKey("user.id"), + ForeignKey("user.id", ondelete="CASCADE"), primary_key=True) application_id = Column(Integer, - ForeignKey("application.id"), + ForeignKey("application.id", ondelete="CASCADE"), primary_key=True) - application_policy_id = Column(Integer, - ForeignKey("application_policy.id"), - primary_key=True) + application_policy_id = Column( + Integer, + ForeignKey("application_policy.id", ondelete="CASCADE"), + primary_key=True) user = relationship("User") @@ -90,6 +93,14 @@ class Accounting(Base): application_policy = relationship("ApplicationPolicy") +@event.listens_for(Engine, "connect") +def set_sqlite_pragma(dbapi_connection, connection_record): + if type(dbapi_connection) is sqlite3.Connection: + cursor = dbapi_connection.cursor() + cursor.execute("PRAGMA foreign_keys=ON") + cursor.close() + + class Database(LoggingMixin): def __init__(self, url, **kwargs): """Initialises a database connection to a given database url. @@ -107,6 +118,7 @@ def __init__(self, url, **kwargs): self.log.info("Creating session to db: {}".format(self.url)) self.engine = create_engine(self.url, **kwargs) + try: self.session_class = sessionmaker(bind=self.engine) except OperationalError: From fb79068a2494d7139090a9673cdb625f1b901155 Mon Sep 17 00:00:00 2001 From: Kit Yan Choi Date: Wed, 13 Jul 2016 14:23:26 +0100 Subject: [PATCH 2/5] added tests for cascade delete --- tests/cli/test_remoteappdb.py | 54 +++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/tests/cli/test_remoteappdb.py b/tests/cli/test_remoteappdb.py index d06bd7b17..d906e811c 100644 --- a/tests/cli/test_remoteappdb.py +++ b/tests/cli/test_remoteappdb.py @@ -87,3 +87,57 @@ def test_app_revoke(self): self.assertEqual(len(out.split('\n')), 2) self.assertIn("frobniz", out) self.assertIn("froble", out) + + def test_delete_user_cascade(self): + """ Test if deleting user cascade to deleting accounting rows + """ + # Given user is created with two accountings (application, policy) + self._remoteappdb("app create myapp") + out = self._remoteappdb("user create user") + self.assertEqual(out, "1\n") + + self._remoteappdb("app grant myapp user") + self._remoteappdb("app grant myapp user --allow-view") + + out = self._remoteappdb("user list --show-apps --no-decoration") + self.assertEqual(len(out.split('\n')), 3) + + # When the user is deleted, the accounting rows should be deleted + # So when you add the user back later, those accountings should + # not exist (This test relies on the fact that there is only one + # user and so has the same id as before) + self._remoteappdb("user remove user") + out = self._remoteappdb("user create user") + self.assertEqual(out, "1\n") + + out = self._remoteappdb("user list --show-apps --no-decoration") + self.assertEqual(len(out.split('\n')), 2) + self.assertNotIn("myapp", out) + + def test_delete_application_cascade(self): + """ Test if deleting application cascade to deleting accounting rows + """ + # Given user is created with two accountings (application, policy) + out = self._remoteappdb("app create myapp") + self.assertEqual(out, "1\n") + out = self._remoteappdb("user create user") + self.assertEqual(out, "1\n") + + self._remoteappdb("app grant myapp user") + self._remoteappdb("app grant myapp user --allow-view") + + out = self._remoteappdb("user list --show-apps --no-decoration") + self.assertEqual(len(out.split('\n')), 3) + + # When the application is deleted, the associated accounting rows + # should be deleted. So when you add the application back later, + # those accountings should not exist + # (This test relies on the fact that there is only one app and so + # the application row has the same id as before) + self._remoteappdb("app remove myapp") + out = self._remoteappdb("app create myapp") + self.assertEqual(out, "1\n") + + out = self._remoteappdb("user list --show-apps --no-decoration") + self.assertEqual(len(out.split('\n')), 2) + self.assertNotIn("myapp", out) From 7523c5288591a575cc9f568fa07c50f59e1cc04b Mon Sep 17 00:00:00 2001 From: Kit Yan Choi Date: Wed, 13 Jul 2016 15:21:58 +0100 Subject: [PATCH 3/5] Fix tests after merge --- tests/cli/test_remoteappdb.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/cli/test_remoteappdb.py b/tests/cli/test_remoteappdb.py index 4cb61b742..095452931 100644 --- a/tests/cli/test_remoteappdb.py +++ b/tests/cli/test_remoteappdb.py @@ -137,7 +137,7 @@ def test_delete_user_cascade(self): """ Test if deleting user cascade to deleting accounting rows """ # Given user is created with two accountings (application, policy) - self._remoteappdb("app create myapp") + self._remoteappdb("app create myapp --no-verify") out = self._remoteappdb("user create user") self.assertEqual(out, "1\n") @@ -163,7 +163,7 @@ def test_delete_application_cascade(self): """ Test if deleting application cascade to deleting accounting rows """ # Given user is created with two accountings (application, policy) - out = self._remoteappdb("app create myapp") + out = self._remoteappdb("app create myapp --no-verify") self.assertEqual(out, "1\n") out = self._remoteappdb("user create user") self.assertEqual(out, "1\n") @@ -180,7 +180,7 @@ def test_delete_application_cascade(self): # (This test relies on the fact that there is only one app and so # the application row has the same id as before) self._remoteappdb("app remove myapp") - out = self._remoteappdb("app create myapp") + out = self._remoteappdb("app create myapp --no-verify") self.assertEqual(out, "1\n") out = self._remoteappdb("user list --show-apps --no-decoration") From d87c14ec04926986c64607a2284fa005b04da1df Mon Sep 17 00:00:00 2001 From: Kit Yan Choi Date: Thu, 14 Jul 2016 10:35:19 +0100 Subject: [PATCH 4/5] in case sqlite3 is not compiled --- remoteappmanager/db/orm.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/remoteappmanager/db/orm.py b/remoteappmanager/db/orm.py index d037fd857..be6d86593 100644 --- a/remoteappmanager/db/orm.py +++ b/remoteappmanager/db/orm.py @@ -1,5 +1,4 @@ import contextlib -import sqlite3 from sqlalchemy import ( Column, Integer, Boolean, Unicode, ForeignKey, create_engine, Enum, @@ -95,10 +94,20 @@ class Accounting(Base): @event.listens_for(Engine, "connect") def set_sqlite_pragma(dbapi_connection, connection_record): - if type(dbapi_connection) is sqlite3.Connection: - cursor = dbapi_connection.cursor() - cursor.execute("PRAGMA foreign_keys=ON") - cursor.close() + """ Set pragma for sqlite3 when the engine connects + Currently it adds support for foreign keys. + Do nothing if sqlite3 is not available or if the database + is not using sqlite3. + """ + try: + # In case sqlite3 is not compiled? + import sqlite3 + except ImportError: + return + else: + if isinstance(dbapi_connection, sqlite3.Connection): + with contextlib.closing(dbapi_connection.cursor()) as cursor: + cursor.execute("PRAGMA foreign_keys=ON") class Database(LoggingMixin): From 5ad6c3328d82d96c7a5c025dce76ef3b99f156bc Mon Sep 17 00:00:00 2001 From: Kit Yan Choi Date: Thu, 14 Jul 2016 10:36:26 +0100 Subject: [PATCH 5/5] make the set_sqlite_pragma a private function --- remoteappmanager/db/orm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/remoteappmanager/db/orm.py b/remoteappmanager/db/orm.py index be6d86593..d38af76a5 100644 --- a/remoteappmanager/db/orm.py +++ b/remoteappmanager/db/orm.py @@ -93,7 +93,7 @@ class Accounting(Base): @event.listens_for(Engine, "connect") -def set_sqlite_pragma(dbapi_connection, connection_record): +def _set_sqlite_pragma(dbapi_connection, connection_record): """ Set pragma for sqlite3 when the engine connects Currently it adds support for foreign keys. Do nothing if sqlite3 is not available or if the database