Merge pull request #4041 from ckan/4041-remove-datastore-legacy-mode

Remove datastore legacy mode

ckanext/datastore/backend/postgres.py

@@ -1550,16 +1550,12 @@ def _check_urls_and_permissions(self):
             self._log_or_raise(
                 'CKAN and DataStore database cannot be the same.')
 
-        # in legacy mode, the read and write url are the same (both write url)
-        # consequently the same url check and and write privilege check
-        # don't make sense
-        if not self.legacy_mode:
-            if self._same_read_and_write_url():
-                self._log_or_raise('The write and read-only database '
-                                   'connection urls are the same.')
+        if self._same_read_and_write_url():
+            self._log_or_raise('The write and read-only database '
+                               'connection urls are the same.')
 
-            if not self._read_connection_has_correct_privileges():
-                self._log_or_raise('The read-only user has write privileges.')
+        if not self._read_connection_has_correct_privileges():
+            self._log_or_raise('The read-only user has write privileges.')
 
     def _is_read_only_database(self):
         ''' Returns True if no connection has CREATE privileges on the public
@@ -1614,26 +1610,15 @@ def _read_connection_has_correct_privileges(self):
             write_connection.close()
         return True
 
-    def is_legacy_mode(self, config):
-        '''
-            Decides if the DataStore should run on legacy mode
-
-            Returns True if `ckan.datastore.read_url` is not set in the
-            provided config object or CKAN is running on Postgres < 9.x
-        '''
-
-        engine = self._get_write_engine()
-        connection = engine.connect()
-
-        return (not config.get('ckan.datastore.read_url') or
-                not _pg_version_is_at_least(connection, '9.0'))
-
     def configure(self, config):
         self.config = config
         # check for ckan.datastore.write_url and ckan.datastore.read_url
         if ('ckan.datastore.write_url' not in config):
             error_msg = 'ckan.datastore.write_url not found in config'
             raise DatastoreException(error_msg)
+        if ('ckan.datastore.read_url' not in config):
+            error_msg = 'ckan.datastore.read_url not found in config'
+            raise DatastoreException(error_msg)
 
         # Check whether users have disabled datastore_search_sql
         self.enable_sql_search = toolkit.asbool(
@@ -1649,15 +1634,7 @@ def configure(self, config):
 
         self.ckan_url = self.config['sqlalchemy.url']
         self.write_url = self.config['ckan.datastore.write_url']
-
-        self.legacy_mode = self.is_legacy_mode(config)
-
-        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']
+        self.read_url = self.config['ckan.datastore.read_url']
 
         self.read_engine = self._get_read_engine()
         if not model.engine_is_pg(self.read_engine):

ckanext/datastore/logic/action.py

@@ -146,27 +146,21 @@ def datastore_create(context, data_dict):
                 'alias': [u'"{0}" is not a valid alias name'.format(alias)]
             })
 
-    # create a private datastore resource, if necessary
-    model = _get_or_bust(context, 'model')
-    resource = model.Resource.get(data_dict['resource_id'])
-    legacy_mode = 'ckan.datastore.read_url' not in config
-    if not legacy_mode and resource.package.private:
-        data_dict['private'] = True
-
     try:
         result = backend.create(context, data_dict)
-
     except InvalidDataError as err:
         raise p.toolkit.ValidationError(unicode(err))
+
     # Set the datastore_active flag on the resource if necessary
-    if resource.extras.get('datastore_active') is not True:
+    model = _get_or_bust(context, 'model')
+    resobj = model.Resource.get(data_dict['resource_id'])
+    if resobj.extras.get('datastore_active') is not True:
         log.debug(
-            'Setting datastore_active=True on resource {0}'.format(resource.id)
+            'Setting datastore_active=True on resource {0}'.format(resobj.id)
         )
         set_datastore_active_flag(model, data_dict, True)
 
     result.pop('id', None)
-    result.pop('private', None)
     result.pop('connection_url', None)
     result.pop('records', None)
     return result
@@ -486,12 +480,11 @@ def datastore_search_sql(context, data_dict):
     Queries are only allowed if you have access to the all the CKAN resources
     in the query and send the appropriate authorization.
 
-    .. note:: This action is only available when using PostgreSQL 9.X and
-        using a read-only user on the database.
-        It is not available in :ref:`legacy mode<legacy-mode>`.
-        
+    .. note:: This action is not available when
+        :ref:`ckan.datastore.sqlsearch.enabled` is set to false
+
     .. note:: When source data columns (i.e. CSV) heading names are provdied
-        in all UPPERCASE you need to double quote them in the SQL select 
+        in all UPPERCASE you need to double quote them in the SQL select
         statement to avoid returning null results.
 
     :param sql: a single SQL select statement

ckanext/datastore/plugin.py

@@ -40,7 +40,6 @@ class DatastorePlugin(p.SingletonPlugin):
     p.implements(interfaces.IDatastore, inherit=True)
     p.implements(interfaces.IDatastoreBackend, inherit=True)
 
-    legacy_mode = False
     resource_show_action = None
 
     def __new__(cls, *args, **kwargs):
@@ -80,13 +79,6 @@ def configure(self, config):
         self.config = config
         self.backend.configure(config)
 
-        # Legacy mode means that we have no read url. Consequently sql search
-        # is not available and permissions do not have to be changed. In
-        # legacy mode, the datastore runs on PG prior to 9.0 (for
-        # example 8.4).
-        if hasattr(self.backend, 'is_legacy_mode'):
-            self.legacy_mode = self.backend.is_legacy_mode(self.config)
-
     # IActions
 
     def get_actions(self):
@@ -100,12 +92,11 @@ def get_actions(self):
             'datastore_function_delete': action.datastore_function_delete,
             'datastore_run_triggers': action.datastore_run_triggers,
         }
-        if not self.legacy_mode:
-            if getattr(self.backend, 'enable_sql_search', False):
-                # Only enable search_sql if the config does not disable it
-                actions.update({
-                    'datastore_search_sql': action.datastore_search_sql,
-                })
+        if getattr(self.backend, 'enable_sql_search', False):
+            # Only enable search_sql if the config/backend does not disable it
+            actions.update({
+                'datastore_search_sql': action.datastore_search_sql,
+            })
         return actions
 
     # IAuthFunctions

ckanext/datastore/tests/test_configure.py

@@ -24,19 +24,6 @@ def tearDown(self):
         ckan.plugins.unload('datastore')
         pyutilib.component.core.PluginGlobals.singleton_services()[plugin.DatastorePlugin] = self._original_plugin
 
-    def test_set_legacy_mode(self):
-        c = {
-            'sqlalchemy.url': 'bar',
-            'ckan.datastore.write_url': 'foo'
-        }
-        try:
-            self.p.configure(c)
-        except Exception:
-            pass
-        assert self.p.legacy_mode
-        assert self.p.write_url == 'foo'
-        assert self.p.read_url == 'foo'
-
     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'
@@ -68,56 +55,24 @@ def test_check_urls_and_permissions_requires_different_ckan_and_datastore_dbs(se
         self.p._same_ckan_and_datastore_db = lambda: True
         nose.tools.assert_raises(InvalidUrlsOrPermissionsException, self.p._check_urls_and_permissions)
 
-    def test_check_urls_and_permissions_requires_different_read_and_write_urls_when_not_in_legacy_mode(self):
+    def test_check_urls_and_permissions_requires_different_read_and_write_urls(self):
         self.setUp_plugin_for_check_urls_and_permissions_tests()
-        self.p.legacy_mode = False
 
         self.p._same_read_and_write_url = lambda: False
         self.p._check_urls_and_permissions()  # Should be OK
 
         self.p._same_read_and_write_url = lambda: True
         nose.tools.assert_raises(InvalidUrlsOrPermissionsException, self.p._check_urls_and_permissions)
 
-    def test_check_urls_and_permissions_doesnt_require_different_read_and_write_urls_when_in_legacy_mode(self):
+    def test_check_urls_and_permissions_requires_read_connection_with_correct_privileges(self):
         self.setUp_plugin_for_check_urls_and_permissions_tests()
-        self.p.legacy_mode = True
-
-        self.p._same_read_and_write_url = lambda: False
-        self.p._check_urls_and_permissions()  # Should be OK
-
-        self.p._same_read_and_write_url = lambda: True
-        self.p._check_urls_and_permissions()  # Should be OK
-
-    def test_check_urls_and_permissions_requires_read_connection_with_correct_privileges_when_not_in_legacy_mode(self):
-        self.setUp_plugin_for_check_urls_and_permissions_tests()
-        self.p.legacy_mode = False
 
         self.p._read_connection_has_correct_privileges = lambda: True
         self.p._check_urls_and_permissions()  # Should be OK
 
         self.p._read_connection_has_correct_privileges = lambda: False
         nose.tools.assert_raises(InvalidUrlsOrPermissionsException, self.p._check_urls_and_permissions)
 
-    def test_check_urls_and_permissions_doesnt_care_about_read_connection_privileges_when_in_legacy_mode(self):
-        self.setUp_plugin_for_check_urls_and_permissions_tests()
-        self.p.legacy_mode = True
-
-        self.p._read_connection_has_correct_privileges = lambda: True
-        self.p._check_urls_and_permissions()  # Should be OK
-
-        self.p._read_connection_has_correct_privileges = lambda: False
-        self.p._check_urls_and_permissions()  # Should be OK
-
-    def setUp_plugin_for_check_urls_and_permissions_tests(self):
-        def _raise_invalid_urls_or_permissions_exception(message):
-            raise InvalidUrlsOrPermissionsException(message)
-
-        self.p._same_ckan_and_datastore_db = lambda: False
-        self.p.legacy_mode = True
-        self.p._same_read_and_write_url = lambda: False
-        self.p._read_connection_has_correct_privileges = lambda: True
-        self.p._log_or_raise = _raise_invalid_urls_or_permissions_exception
-
 
 class InvalidUrlsOrPermissionsException(Exception):
     pass

ckanext/datastore/tests/test_disable.py

@@ -9,14 +9,6 @@
 
 class TestDisable(object):
 
-    @classmethod
-    def setup_class(cls):
-        with p.use_plugin('datastore') as the_plugin:
-            legacy = the_plugin.legacy_mode
-
-        if legacy:
-            raise nose.SkipTest("SQL tests are not supported in legacy mode")
-
     @t.raises(KeyError)
     def test_disable_sql_search(self):
         config['ckan.datastore.sqlsearch.enabled'] = False

ckanext/datastore/tests/test_helpers.py

@@ -68,9 +68,6 @@ class TestGetTables(object):
     @classmethod
     def setup_class(cls):
 
-        if not config.get('ckan.datastore.read_url'):
-            raise nose.SkipTest('Datastore runs on legacy mode, skipping...')
-
         engine = db.get_write_engine()
         cls.Session = orm.scoped_session(orm.sessionmaker(bind=engine))
 

ckanext/datastore/tests/test_info.py

@@ -28,10 +28,6 @@ def setup_class(cls):
         if not is_datastore_supported():
             raise nose.SkipTest("Datastore not supported")
         plugin = p.load('datastore')
-        if plugin.legacy_mode:
-            # make sure we undo adding the plugin
-            p.unload('datastore')
-            raise nose.SkipTest("Info is not supported in legacy mode")
 
     @classmethod
     def teardown_class(cls):

ckanext/datastore/tests/test_search.py

@@ -789,10 +789,6 @@ def setup_class(cls):
             raise nose.SkipTest("Datastore not supported")
         cls.app = helpers._get_test_app()
         plugin = p.load('datastore')
-        if plugin.legacy_mode:
-            # make sure we undo adding the plugin
-            p.unload('datastore')
-            raise nose.SkipTest("SQL tests are not supported in legacy mode")
         ctd.CreateTestData.create()
         cls.sysadmin_user = model.User.get('testsysadmin')
         cls.normal_user = model.User.get('annafan')

ckanext/datastore/tests/test_unit.py

@@ -44,48 +44,3 @@ def test_pg_version_check(self):
         connection = engine.connect()
         assert db._pg_version_is_at_least(connection, '8.0')
         assert not db._pg_version_is_at_least(connection, '20.0')
-
-
-class TestLegacyModeSetting():
-
-    @mock.patch('ckanext.datastore.backend.postgres._pg_version_is_at_least')
-    def test_legacy_mode_set_if_no_read_url_and_pg_9(self, pgv):
-        pgv.return_value = True
-
-        test_config = {
-            'ckan.datastore.write_url': config['ckan.datastore.write_url'],
-        }
-        assert postgres_backend.is_legacy_mode(test_config)
-
-    @mock.patch('ckanext.datastore.backend.postgres._pg_version_is_at_least')
-    def test_legacy_mode_set_if_no_read_url_and_pg_8(self, pgv):
-
-        pgv.return_value = False
-
-        test_config = {
-            'ckan.datastore.write_url': config['ckan.datastore.write_url'],
-        }
-        assert postgres_backend.is_legacy_mode(test_config)
-
-    @mock.patch('ckanext.datastore.backend.postgres._pg_version_is_at_least')
-    def test_legacy_mode_set_if_read_url_and_pg_8(self, pgv):
-
-        pgv.return_value = False
-
-        test_config = {
-            'ckan.datastore.write_url': config['ckan.datastore.write_url'],
-            'ckan.datastore.read_url': 'some_test_read_url',
-        }
-        assert postgres_backend.is_legacy_mode(test_config)
-
-    @mock.patch('ckanext.datastore.backend.postgres._pg_version_is_at_least')
-    def test_legacy_mode_not_set_if_read_url_and_pg_9(self, pgv):
-
-        pgv.return_value = True
-
-        test_config = {
-            'ckan.datastore.write_url': config['ckan.datastore.write_url'],
-            'ckan.datastore.read_url': 'some_test_read_url',
-        }
-
-        assert not postgres_backend.is_legacy_mode(test_config)

doc/maintaining/datastore.rst

@@ -125,8 +125,6 @@ Replace ``pass`` with the passwords you created for your |database_user| and
 Set permissions
 ---------------
 
-.. tip:: See :ref:`legacy-mode` if these steps continue to fail or seem too complicated for your set-up. However, keep in mind that the legacy mode is limited in its capabilities.
-
 Once the DataStore database and the users are created, the permissions on the DataStore and CKAN database have to be set. CKAN provides a paster command to help you correctly set these permissions.
 
 If you are able to use the ``psql`` command to connect to your database as a
@@ -194,29 +192,6 @@ You can now delete the DataStore table with::
 To find out more about the DataStore API, see `The DataStore API`_.
 
 
-.. _legacy-mode:
-
-Legacy mode: use the DataStore with old PostgreSQL versions
-===========================================================
-
-.. tip:: The legacy mode can also be used to simplify the set-up since it does not require you to set the permissions or create a separate user.
-
-The DataStore can be used with a PostgreSQL version prior to 9.0 in *legacy mode*. Due to the lack of some functionality, the :meth:`~ckanext.datastore.logic.action.datastore_search_sql` and consequently the :ref:`datastore_search_htsql` cannot be used. To enable the legacy mode, remove the declaration of the ``ckan.datastore.read_url``.
-
-The set-up for legacy mode is analogous to the normal set-up as described above with a few changes and consists of the following steps:
-
-1. Enable the plugin
-2. The legacy mode is enabled by **not** setting the ``ckan.datastore.read_url``
-#. Set-Up the database
-
-   a) Create a separate database
-   #) Create a write user on the DataStore database (optional since the CKAN user can be used)
-
-#. Test the set-up
-
-There is no need for a read-only user or special permissions. Therefore the legacy mode can be used for simple set-ups as well.
-
-
 ---------------------------------------------------
 DataPusher: Automatically Add Data to the DataStore
 ---------------------------------------------------