Repository: simphony/simphony-remote

Issue:
Support arbitrary database implementation via the remoteappmanager config file
We could potentially support any arbitrary database implementation compliant to `remoteappmanager.db.interfaces.ABCAccounting` by making the `remoteappmanager.application.Application.db` a configurable object, e.g. by initialising an Accounting object inside the config file.

Currently the application initialises `remoteappmanager.db.orm.AppAccounting` or `remoteappmanager.db.csv_db.CSVAccounting` internally depending on the database file extension (see [`remoteappmanager.application.Application._db_default`](https://github.com/simphony/simphony-remote/blob/master/remoteappmanager/application.py#L112)).  

Discussion:

Patch:
diff --git a/jupyterhub/remoteappmanager_config.py b/jupyterhub/remoteappmanager_config.py
index 20a7b7c..299b780 100644
--- a/jupyterhub/remoteappmanager_config.py
+++ b/jupyterhub/remoteappmanager_config.py
@@ -28,11 +28,23 @@ else:
     raise RuntimeError("Unknown platform {}".format(_platform))
 
 
-# Define the sqlalchemy url for the database.
-# Notes:
-# 1. this database is shared among all instances of the remoteappmanager.
-# 2. When running with system-user mode, the jupyterhub spawners spawn in
-#    the user's home directory; when running in virtual-user mode,
-#    the current directory is the directory where jupyterhub is started
-# 3. '~' would be evaluated as the spawned user's home directory
-db_url = "sqlite:///"+os.path.abspath("./remoteappmanager.db")
+# -----------------------------
+# Define the accounting class
+# -----------------------------
+# Notes on os.path:
+#  1. When running with system-user mode, both the current directory and '~'
+#  are the system user's home directory.
+#  2. When running in virtual-user mode, the current directory is the
+#  directory where jupyterhub is started, '~' would be evaluated according to
+#  the spawned process's owner's home directory (not the virtual user's
+#  home directory)
+
+# CSV database support
+# accounting_class = "remoteappmanager.db.csv_db.CSVAccounting"
+# accounting_kwargs = {
+#     "csv_file_path": os.path.abspath("./remoteappmanager.csv")}
+
+# sqlite database support
+# accounting_class = "remoteappmanager.db.orm.AppAccounting"
+# accounting_kwargs = {
+#     "url": "sqlite:///"+os.path.abspath('./remoteappmanager.db')}
diff --git a/remoteappmanager/application.py b/remoteappmanager/application.py
index b5131db..1dc4e59 100644
--- a/remoteappmanager/application.py
+++ b/remoteappmanager/application.py
@@ -1,10 +1,11 @@
+import importlib
 import os
+
 from traitlets import Instance, default
 from tornado import web
 import tornado.ioloop
 from jinja2 import Environment, FileSystemLoader
 
-from remoteappmanager.db import csv_db, orm
 from remoteappmanager.db.interfaces import ABCAccounting
 from remoteappmanager.handlers.api import HomeHandler
 from remoteappmanager.logging.logging_mixin import LoggingMixin
@@ -111,14 +112,11 @@ class Application(web.Application, LoggingMixin):
     @default("db")
     def _db_default(self):
         """Initializes the database connection."""
-        if self.file_config.db_url.endswith('.db'):
-            db = orm.AppAccounting(self.file_config.db_url)
-        elif self.file_config.db_url.endswith('.csv'):
-            db = csv_db.CSVAccounting(self.file_config.db_url)
-        else:
-            raise ValueError("Unsupported database format: {}".format(
-                self.file_config.db_url))
-        return db
+        class_path = self.file_config.accounting_class
+        module_path, _, cls_name = class_path.rpartition('.')
+        cls = getattr(importlib.import_module(module_path), cls_name)
+
+        return cls(**self.file_config.accounting_kwargs)
 
     @default("user")
     def _user_default(self):
diff --git a/remoteappmanager/file_config.py b/remoteappmanager/file_config.py
index 771ad44..d374ce4 100644
--- a/remoteappmanager/file_config.py
+++ b/remoteappmanager/file_config.py
@@ -1,5 +1,6 @@
 import tornado.options
-from traitlets import HasTraits, Int, Unicode, Bool
+from traitlets import HasTraits, Int, Unicode, Bool, Dict
+from traitlets.utils.sentinel import Sentinel
 
 from remoteappmanager import paths
 from remoteappmanager.traitlets import set_traits_from_dict
@@ -30,8 +31,13 @@ class FileConfig(HasTraits):
     docker_host = Unicode(default_value="",
                           help="The docker host to connect to")
 
-    db_url = Unicode(default_value="sqlite:///remoteappmanager.db",
-                     help="The url of the database, in sqlalchemy format.")
+    accounting_class = Unicode(
+        default_value="remoteappmanager.db.orm.AppAccounting",
+        help="The import path to a subclass of ABCAccounting.")
+
+    accounting_kwargs = Dict(
+        default_value={'url': 'sqlite:///remoteappmanager.db'},
+        help="The keyword arguments for initalising the Accounting instance")
 
     login_url = Unicode(default_value="/hub",
                         help=("The url to be redirected to if the user is not "
@@ -64,10 +70,19 @@ class FileConfig(HasTraits):
         file_line_parser = tornado.options.OptionParser()
 
         for traitlet_name, traitlet in self.traits().items():
+            # tornado.OptionParser defines an option with a Python type
+            # and performs type validation.
+            # traitlet.default_value may be a Sentinel value (e.g. Tuple,
+            # Dict, Instance), in which case we use the repr
+            default_value = traitlet.default_value
+
+            if type(default_value) is Sentinel:
+                default_value = eval(traitlet.default_value_repr())
+
             file_line_parser.define(
                 traitlet_name,
-                default=traitlet.default_value,
-                type=type(traitlet.default_value),
+                default=default_value,
+                type=type(default_value),
                 help=traitlet.help)
 
         # Let it raise the exception if the file is not there.


Test patch:
diff --git a/tests/fixtures/remoteappmanager_config.py b/tests/fixtures/remoteappmanager_config.py
index adaf874..6d6ddb0 100644
--- a/tests/fixtures/remoteappmanager_config.py
+++ b/tests/fixtures/remoteappmanager_config.py
@@ -1,9 +1,6 @@
-from tests import fixtures
-
 tls = True
 tls_verify = True
 tls_ca = '~/.docker/machine/machines/default/ca.pem'
 tls_cert = '~/.docker/machine/machines/default/cert.pem'
 tls_key = '~/.docker/machine/machines/default/key.pem'
 docker_host = "tcp://192.168.99.100:2376"
-db_url = "sqlite:///"+fixtures.get("remoteappmanager.db")
diff --git a/tests/handlers/test_home_handler.py b/tests/handlers/test_home_handler.py
index c532f9e..6df1653 100644
--- a/tests/handlers/test_home_handler.py
+++ b/tests/handlers/test_home_handler.py
@@ -48,7 +48,7 @@ class TestHomeHandler(TempMixin, utils.AsyncHTTPTestCase):
 
         command_line_config = utils.basic_command_line_config()
         file_config = utils.basic_file_config()
-        file_config.db_url = "sqlite:///"+sqlite_file_path
+        file_config.accounting_kwargs = {'url': "sqlite:///"+sqlite_file_path}
 
         app = Application(command_line_config, file_config)
         app.reverse_proxy = mock.Mock(spec=ReverseProxy)
diff --git a/tests/test_application.py b/tests/test_application.py
index c51080d..04b0e65 100644
--- a/tests/test_application.py
+++ b/tests/test_application.py
@@ -2,12 +2,19 @@ import os
 
 from tests.temp_mixin import TempMixin
 from tornado import testing
+from traitlets.traitlets import TraitError
 
 from remoteappmanager.application import Application
+
 from tests import utils
 from tests.db import test_csv_db
 
 
+class DummyAccounting:
+    def __init__(self, *args, **kwargs):
+        pass
+
+
 class TestApplication(TempMixin, testing.AsyncTestCase):
     def setUp(self):
         super().setUp()
@@ -16,14 +23,9 @@ class TestApplication(TempMixin, testing.AsyncTestCase):
 
         os.environ["PROXY_API_TOKEN"] = "dummy_token"
 
-        self.sqlite_file_path = os.path.join(self.tempdir, "sqlite.db")
-        utils.init_sqlite_db(self.sqlite_file_path)
-
-        self.command_line_config = utils.basic_command_line_config()
+        # File config with orm.AppAccounting
         self.file_config = utils.basic_file_config()
-        self.file_config.db_url = "sqlite:///"+self.sqlite_file_path
-
-        self.app = Application(self.command_line_config, self.file_config)
+        self.command_line_config = utils.basic_command_line_config()
 
     def tearDown(self):
         if self._old_proxy_api_token is not None:
@@ -33,8 +35,18 @@ class TestApplication(TempMixin, testing.AsyncTestCase):
 
         super().tearDown()
 
-    def test_initialization(self):
-        app = self.app
+    def test_initialization_with_sqlite_db(self):
+        # Initialise database
+        sqlite_file_path = os.path.join(self.tempdir, "sqlite.db")
+        utils.init_sqlite_db(sqlite_file_path)
+
+        self.file_config.accounting_class = (
+            "remoteappmanager.db.orm.AppAccounting")
+        self.file_config.accounting_kwargs = {
+            "url": "sqlite:///"+sqlite_file_path}
+
+        app = Application(self.command_line_config, self.file_config)
+
         self.assertIsNotNone(app.command_line_config)
         self.assertIsNotNone(app.file_config)
 
@@ -46,6 +58,21 @@ class TestApplication(TempMixin, testing.AsyncTestCase):
         self.assertEqual(app.user.name, "username")
         self.assertEqual(app.user.account, None)
 
+    def test_error_default_value_with_unimportable_accounting(self):
+        self.file_config.accounting_class = "not.importable.Class"
+        app = Application(self.command_line_config, self.file_config)
+
+        with self.assertRaises(ImportError):
+            app.db
+
+    def test_db_default_value_with_accounting_wrong_subclass(self):
+        self.file_config.accounting_class = (
+            "tests.test_application.DummyAccounting")
+        app = Application(self.command_line_config, self.file_config)
+
+        with self.assertRaises(TraitError):
+            app.db
+
 
 # FIXME: Some of these tests are the same and should be refactored
 # Not doing it now to prevent more merge conflict with PR #52
@@ -60,15 +87,6 @@ class TestApplicationWithCSV(TempMixin, testing.AsyncTestCase):
         self.command_line_config = utils.basic_command_line_config()
         self.file_config = utils.basic_file_config()
 
-        self.csv_file = os.path.join(self.tempdir, 'testing.csv')
-        self.file_config.db_url = self.csv_file
-
-        test_csv_db.write_csv_file(self.csv_file,
-                                   test_csv_db.GoodTable.headers,
-                                   test_csv_db.GoodTable.records)
-
-        self.app = Application(self.command_line_config, self.file_config)
-
     def tearDown(self):
         if self._old_proxy_api_token is not None:
             os.environ["PROXY_API_TOKEN"] = self._old_proxy_api_token
@@ -78,15 +96,21 @@ class TestApplicationWithCSV(TempMixin, testing.AsyncTestCase):
         super().tearDown()
 
     def test_initialization(self):
-        app = self.app
-        self.assertIsNotNone(app.command_line_config)
-        self.assertIsNotNone(app.file_config)
+        self.file_config.accounting_class = (
+            "remoteappmanager.db.csv_db.CSVAccounting")
 
-    def test_database_initialization(self):
-        app = self.app
+        csv_file = os.path.join(self.tempdir, 'testing.csv')
+        self.file_config.accounting_kwargs = {"csv_file_path": csv_file}
 
+        test_csv_db.write_csv_file(csv_file,
+                                   test_csv_db.GoodTable.headers,
+                                   test_csv_db.GoodTable.records)
+
+        app = Application(self.command_line_config, self.file_config)
+
+        self.assertIsNotNone(app.command_line_config)
+        self.assertIsNotNone(app.file_config)
         self.assertIsNotNone(app.db)
         self.assertIsNotNone(app.user)
-
         self.assertEqual(app.user.name, "username")
         self.assertIsInstance(app.user.account, test_csv_db.CSVUser)
diff --git a/tests/test_file_config.py b/tests/test_file_config.py
index 258d347..61e530f 100644
--- a/tests/test_file_config.py
+++ b/tests/test_file_config.py
@@ -1,18 +1,53 @@
+import os
 import unittest
 
 from remoteappmanager.file_config import FileConfig
 
-# The arguments we pass
-from tests import fixtures
+from tests.temp_mixin import TempMixin
 
 
-class TestFileConfig(unittest.TestCase):
+DOCKER_CONFIG = '''
+tls = True
+tls_verify = True
+tls_ca = '~/.docker/machine/machines/default/ca.pem'
+tls_cert = '~/.docker/machine/machines/default/cert.pem'
+tls_key = '~/.docker/machine/machines/default/key.pem'
+docker_host = "tcp://192.168.99.100:2376"
+'''
+
+GOOD_ACCOUNTING_CONFIG = '''
+accounting_class = "remoteappmanager.db.csv_db.CSVAccounting"
+accounting_kwargs = {"csv_file_path": "file_path.csv"}
+'''
+
+
+class TestFileConfig(TempMixin, unittest.TestCase):
+
     def setUp(self):
+        super().setUp()
+
         # We can run the application config only once. It uses a global
         # object that can't be reinitialized cleanly unless we access a private
         # member.
-        self.config_file = fixtures.get("remoteappmanager_config.py")
+        self.config_file = os.path.join(self.tempdir,
+                                        'config.py')
+
+    def test_initialization_with_default_accounting(self):
+        with open(self.config_file, 'w') as fhandle:
+            print(DOCKER_CONFIG, file=fhandle)
 
-    def test_initialization(self):
         config = FileConfig()
         config.parse_config(self.config_file)
+
+    def test_initialization_with_good_accounting(self):
+        with open(self.config_file, 'w') as fhandle:
+            print(DOCKER_CONFIG, file=fhandle)
+            print(GOOD_ACCOUNTING_CONFIG, file=fhandle)
+
+        config = FileConfig()
+        config.parse_config(self.config_file)
+
+        self.assertEqual(config.accounting_class,
+                         "remoteappmanager.db.csv_db.CSVAccounting")
+        self.assertDictEqual(config.accounting_kwargs,
+                             {"csv_file_path": "file_path.csv"})
diff --git a/tests/utils.py b/tests/utils.py
index 28217dc..201c7d7 100644
--- a/tests/utils.py
+++ b/tests/utils.py
@@ -278,10 +278,7 @@ def basic_command_line_config():
 
 
 def basic_file_config():
-    options = {
-        "db_url": "sqlite://"
-    }
-    return FileConfig(**options)
+    return FileConfig()
 
 
 @contextlib.contextmanager