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

Add foreign key support for sqlite #94

Merged
merged 6 commits into from
Jul 14, 2016
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions remoteappmanager/cli/remoteappdb/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,12 +170,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))

Expand Down Expand Up @@ -281,8 +277,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))
Expand Down
24 changes: 18 additions & 6 deletions remoteappmanager/db/orm.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import contextlib
import sqlite3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep it encapsulated in the enabling method, so that it works regardless if we have/use sqlite or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sqlite3 is in the standard library...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but not necessarily compiled in. You can disable it (at least it was optional in py27, I don't know if it is in py34)


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
Expand Down Expand Up @@ -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")

Expand All @@ -90,6 +93,14 @@ class Accounting(Base):
application_policy = relationship("ApplicationPolicy")


@event.listens_for(Engine, "connect")
def set_sqlite_pragma(dbapi_connection, connection_record):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import sqlite3 here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call it _enable_foreign_keys()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the function name _set_sqlite_pragma so that sphinx would skip it. It is the place where future pragma can be added though so I kept the rest of the name

if type(dbapi_connection) is sqlite3.Connection:
cursor = dbapi_connection.cursor()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with contextlib.closing(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.
Expand All @@ -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:
Expand Down
54 changes: 54 additions & 0 deletions tests/cli/test_remoteappdb.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,3 +132,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 --no-verify")
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 --no-verify")
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 --no-verify")
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)