[#642] Refactored datastore config to make it easier to understand an…

…d easier to test

ckanext/datastore/plugin.py

@@ -48,39 +48,30 @@ def configure(self, config):
         self.write_url = self.config['ckan.datastore.write_url']
         if self.legacy_mode:
             self.read_url = self.write_url
+            log.warn("Legacy mode active. "
+                     "The sql search will not be available.")
         else:
             self.read_url = self.config['ckan.datastore.read_url']
 
-        if model.engine_is_pg():
-            if not self._is_read_only_database():
-                # Make sure that the right permissions are set
-                # so that no harmful queries can be made
-                if self.config.get('debug'):
-                    if self._same_read_and_write_url():
-                        raise DatastoreException("The write and read-only database "
-                                                 "connection url are the same.")
-                    if self._same_ckan_and_datastore_db():
-                        raise DatastoreException("CKAN and DataStore database "
-                                                 "cannot be the same.")
-                if self.legacy_mode:
-                    log.warn("Legacy mode active. "
-                             "The sql search will not be available.")
-                elif not self._read_connection_has_correct_privileges():
-                    if self.config.get('debug'):
-                        log.critical("We have write permissions "
-                                     "on the read-only database.")
-                    else:
-                        raise DatastoreException("We have write permissions "
-                                                 "on the read-only database.")
-                self._create_alias_table()
-            else:
-                log.warn("We detected that CKAN is running on a read "
-                         "only database. Permission checks and the creation "
-                         "of _table_metadata are skipped.")
-        else:
+        if not model.engine_is_pg():
             log.warn("We detected that you do not use a PostgreSQL "
-                     "database. The DataStore will NOT work and datastore "
+                     "database. The DataStore will NOT work and DataStore "
                      "tests will be skipped.")
+            return
+
+        if self._is_read_only_database():
+            log.warn("We detected that CKAN is running on a read "
+                     "only database. Permission checks and the creation "
+                     "of _table_metadata are skipped.")
+        else:
+            if self.config.get('debug'):
+                handler = log.critical
+            else:
+                def handler(message):
+                    raise DatastoreException(message)
+            self._check_urls_and_permissions(handler)
+
+            self._create_alias_table()
 
         ## Do light wrapping around action function to add datastore_active
         ## to resource dict.  Not using IAction extension as this prevents
@@ -115,6 +106,22 @@ def new_resource_show(context, data_dict):
             new_resource_show._datastore_wrapped = True
             logic._actions['resource_show'] = new_resource_show
 
+    def _check_urls_and_permissions(self, handler):
+        # Make sure that the right permissions are set
+        # so that no harmful queries can be made
+
+        if not self.legacy_mode and self._same_read_and_write_url():
+            handler("The write and read-only database "
+                    "connection urls are the same.")
+
+        if self._same_ckan_and_datastore_db():
+            handler("CKAN and DataStore database "
+                    "cannot be the same.")
+
+        if not self._read_connection_has_correct_privileges():
+            handler("We have write permissions "
+                    "on the read-only database.")
+
     def _is_read_only_database(self):
         '''
         Returns True if no connection has CREATE privileges on the public
@@ -133,18 +140,12 @@ def _same_ckan_and_datastore_db(self):
         '''
         Returns True if the CKAN and DataStore db are the same
         '''
-        if self._get_db_from_url(self.ckan_url) == self._get_db_from_url(self.read_url):
-            return True
-        return False
+        return self._get_db_from_url(self.ckan_url) == self._get_db_from_url(self.read_url)
 
     def _get_db_from_url(self, url):
         return url[url.rindex("@"):]
 
     def _same_read_and_write_url(self):
-        # in legacy mode, this test can be ignored
-        # because both URLs are set to the same url
-        if self.legacy_mode:
-            return False
         return self.write_url == self.read_url
 
     def _read_connection_has_correct_privileges(self):

ckanext/datastore/tests/test_configure.py

@@ -1,5 +1,7 @@
 import unittest
 
+import sqlalchemy
+
 import ckanext.datastore.plugin as plugin
 
 
@@ -24,16 +26,7 @@ def test_set_legacy_mode(self):
         assert self.p.write_url == 'foo'
         assert self.p.read_url == 'foo'
 
-    def test_check_separate_write_and_read_if_not_legacy(self):
-        self.p.legacy_mode = True
-        self.p.write_url = 'postgresql://u:pass@localhost/ds'
-        self.p.read_url = 'postgresql://u:pass@localhost/ds'
-        assert not self.p._same_read_and_write_url()
-
-        self.p.legacy_mode = False
-
-        assert not self.p.legacy_mode
-
+    def test_check_separate_write_and_read_url(self):
         self.p.write_url = 'postgresql://u:pass@localhost/ds'
         self.p.read_url = 'postgresql://u:pass@localhost/ds'
         assert self.p._same_read_and_write_url()
@@ -43,14 +36,51 @@ def test_check_separate_write_and_read_if_not_legacy(self):
         assert not self.p._same_read_and_write_url()
 
     def test_same_ckan_and_datastore_db(self):
-        self.p.write_url = 'postgresql://u:pass@localhost/ckan'
-        self.p.read_url = 'postgresql://u:pass@localhost/ckan'
+        self.p.read_url = 'postgresql://u2:pass@localhost/ckan'
         self.p.ckan_url = 'postgresql://u:pass@localhost/ckan'
 
         assert self.p._same_ckan_and_datastore_db()
 
-        self.p.write_url = 'postgresql://u:pass@localhost/dt'
         self.p.read_url = 'postgresql://u:pass@localhost/dt'
         self.p.ckan_url = 'postgresql://u:pass@localhost/ckan'
 
         assert not self.p._same_ckan_and_datastore_db()
+
+    def test_check_urls_and_permissions(self):
+        self.p.legacy_mode = False
+        self.p.ckan_url = 'postgresql://u:pass@localhost/ckan'
+        self.p.write_url = 'postgresql://u:pass@localhost/ds'
+        self.p.read_url = 'postgresql://u:pass@localhost/ds'
+
+        def handler(message):
+            assert 'urls are the same' in message, message
+        try:
+            self.p._check_urls_and_permissions(handler)
+        except sqlalchemy.exc.OperationalError:
+            pass
+        else:
+            assert False
+
+        self.p.ckan_url = 'postgresql://u:pass@localhost/ds'
+        self.p.legacy_mode = True
+
+        def handler2(message):
+            assert 'cannot be the same' in message, message
+        try:
+            self.p._check_urls_and_permissions(handler2)
+        except sqlalchemy.exc.OperationalError:
+            pass
+        else:
+            assert False
+
+        self.p.read_url = 'postgresql://u2:pass@localhost/ds'
+        self.p.legacy_mode = False
+
+        def handler3(message):
+            assert 'cannot be the same' in message, message
+        try:
+            self.p._check_urls_and_permissions(handler3)
+        except sqlalchemy.exc.OperationalError:
+            pass
+        else:
+            assert False