From 8330310dc82b22798d2eb51347b945e1339ed333 Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Wed, 8 Nov 2023 18:56:58 +0000 Subject: [PATCH 01/10] feat(jobs): added job to delete datastore tables; - Added config option `clean_datastore_tables`. - Added job to delete datastore tables from unsupported formats. --- ckanext/xloader/config_declaration.yaml | 8 ++++ ckanext/xloader/plugin.py | 62 +++++++++++++++++++++++-- requirements.txt | 1 + 3 files changed, 67 insertions(+), 4 deletions(-) diff --git a/ckanext/xloader/config_declaration.yaml b/ckanext/xloader/config_declaration.yaml index b31f12e2..521f0385 100644 --- a/ckanext/xloader/config_declaration.yaml +++ b/ckanext/xloader/config_declaration.yaml @@ -112,5 +112,13 @@ groups: to True. type: bool required: false + - key: ckanext.xloader.clean_datastore_tables + default: False + example: True + description: | + Enqueue jobs to remove Datastore tables from Resources that have a format + that is not in ckanext.xloader.formats after a Resource is updated. + type: bool + required: false diff --git a/ckanext/xloader/plugin.py b/ckanext/xloader/plugin.py index dbde8ed5..8b229536 100644 --- a/ckanext/xloader/plugin.py +++ b/ckanext/xloader/plugin.py @@ -4,6 +4,10 @@ from ckan import plugins from ckan.plugins import toolkit +from ckan.model.domain_object import DomainObjectOperation +from ckan.model.resource import Resource + +import ckanapi from . import action, auth, helpers as xloader_helpers, utils from .loader import fulltext_function_exists, get_write_engine @@ -53,7 +57,7 @@ def is_it_an_xloader_format(cls, format_): class xloaderPlugin(plugins.SingletonPlugin): plugins.implements(plugins.IConfigurer) plugins.implements(plugins.IConfigurable) - plugins.implements(plugins.IResourceUrlChange) + plugins.implements(plugins.IDomainObjectModification) plugins.implements(plugins.IActions) plugins.implements(plugins.IAuthFunctions) plugins.implements(plugins.ITemplateHelpers) @@ -94,16 +98,29 @@ def configure(self, config_): ) ) - # IResourceUrlChange + # IDomainObjectModification + + def notify(self, entity, operation): + # type: (ckan.model.Package|ckan.model.Resource, DomainObjectOperation) -> None + """ + Runs before_commit to database for Packages and Resources. + We only want to check for changed Resources for this. + We want to check if values have changed, namely the url and the format. + See: ckan/model/modification.py.DomainObjectModificationExtension + """ + if operation != DomainObjectOperation.changed or not isinstance(entity, Resource): + return + + if _should_remove_unsupported_resource_from_datastore(entity): + toolkit.enqueue_job(fn=_remove_unsupported_resource_from_datastore, args=[entity.id]) - def notify(self, resource): context = { "ignore_auth": True, } resource_dict = toolkit.get_action("resource_show")( context, { - "id": resource.id, + "id": entity.id, }, ) self._submit_to_xloader(resource_dict) @@ -212,3 +229,40 @@ def get_helpers(self): "xloader_status": xloader_helpers.xloader_status, "xloader_status_description": xloader_helpers.xloader_status_description, } + + +def _should_remove_unsupported_resource_from_datastore(res_dict_or_obj): + if not toolkit.asbool(toolkit.config.get('ckanext.xloader.clean_datastore_tables', False)): + return False + if isinstance(res_dict_or_obj, Resource): + res_dict_or_obj = res_dict_or_obj.__dict__ + return ((not XLoaderFormats.is_it_an_xloader_format(res_dict_or_obj.get('format', u'')) + or res_dict_or_obj.get('url_changed', False)) + and (res_dict_or_obj.get('url_type') == 'upload' + or res_dict_or_obj.get('url_type') == '') + and (res_dict_or_obj.get('datastore_active', False) + or res_dict_or_obj.get('extras', {}).get('datastore_active', False))) + + +def _remove_unsupported_resource_from_datastore(resource_id): + """ + Callback to remove unsupported datastore tables. + Controlled by config value: ckanext.xloader.clean_datastore_tables. + Double check the resource format. Only supported Xloader formats should have datastore tables. + If the resource format is not supported, we should delete the datastore tables. + """ + lc = ckanapi.LocalCKAN() + try: + res = lc.action.resource_show(id=resource_id) + except toolkit.ObjectNotFound: + log.error('Resource %s does not exist.' % res['id']) + return + + if _should_remove_unsupported_resource_from_datastore(res): + log.info('Unsupported resource format "{}". Deleting datastore tables for resource {}' + .format(res.get(u'format', u'').lower(), res['id'])) + try: + lc.action.datastore_delete(resource_id=res['id'], force=True) + log.info('Datastore table dropped for resource %s' % res['id']) + except toolkit.ObjectNotFound: + log.error('Datastore table for resource %s does not exist' % res['id']) diff --git a/requirements.txt b/requirements.txt index 58540beb..8124a06f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -4,3 +4,4 @@ six>=1.12.0 tabulator==1.53.5 Unidecode==1.0.22 python-dateutil>=2.8.2 +ckanapi From beb3be79d84b4a1b0154d4e18099adc765082263 Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Thu, 9 Nov 2023 15:41:20 +0000 Subject: [PATCH 02/10] feat(dev): code improvements; - Used `get_action` from toolkit instead of ckanapi dependency. - Reworked some coe to make it more gooder. - Removed `url_changed` check. --- ckanext/xloader/plugin.py | 34 +++++++++++++++++----------------- requirements.txt | 1 - 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/ckanext/xloader/plugin.py b/ckanext/xloader/plugin.py index e0fe664c..f3ec0ad0 100644 --- a/ckanext/xloader/plugin.py +++ b/ckanext/xloader/plugin.py @@ -81,9 +81,6 @@ def notify(self, entity, operation): if operation != DomainObjectOperation.changed or not isinstance(entity, Resource): return - if _should_remove_unsupported_resource_from_datastore(entity): - toolkit.enqueue_job(fn=_remove_unsupported_resource_from_datastore, args=[entity.id]) - context = { "ignore_auth": True, } @@ -93,6 +90,10 @@ def notify(self, entity, operation): "id": entity.id, }, ) + + if _should_remove_unsupported_resource_from_datastore(resource_dict): + toolkit.enqueue_job(fn=_remove_unsupported_resource_from_datastore, args=[entity.id]) + self._submit_to_xloader(resource_dict) # IResourceController @@ -202,17 +203,14 @@ def get_helpers(self): } -def _should_remove_unsupported_resource_from_datastore(res_dict_or_obj): +def _should_remove_unsupported_resource_from_datastore(res_dict): if not toolkit.asbool(toolkit.config.get('ckanext.xloader.clean_datastore_tables', False)): return False - if isinstance(res_dict_or_obj, Resource): - res_dict_or_obj = res_dict_or_obj.__dict__ - return ((not XLoaderFormats.is_it_an_xloader_format(res_dict_or_obj.get('format', u'')) - or res_dict_or_obj.get('url_changed', False)) - and (res_dict_or_obj.get('url_type') == 'upload' - or res_dict_or_obj.get('url_type') == '') - and (res_dict_or_obj.get('datastore_active', False) - or res_dict_or_obj.get('extras', {}).get('datastore_active', False))) + return (not XLoaderFormats.is_it_an_xloader_format(res_dict.get('format', u'')) + and (res_dict.get('url_type') == 'upload' + or res_dict.get('url_type') == '') + and (res_dict.get('datastore_active', False) + or res_dict.get('extras', {}).get('datastore_active', False))) def _remove_unsupported_resource_from_datastore(resource_id): @@ -222,18 +220,20 @@ def _remove_unsupported_resource_from_datastore(resource_id): Double check the resource format. Only supported Xloader formats should have datastore tables. If the resource format is not supported, we should delete the datastore tables. """ - lc = ckanapi.LocalCKAN() + context = {"ignore_auth": True} try: - res = lc.action.resource_show(id=resource_id) + res = toolkit.get_action('resource_show')(context, {"id": resource_id}) except toolkit.ObjectNotFound: log.error('Resource %s does not exist.' % res['id']) return if _should_remove_unsupported_resource_from_datastore(res): - log.info('Unsupported resource format "{}". Deleting datastore tables for resource {}' - .format(res.get(u'format', u'').lower(), res['id'])) + log.info('Unsupported resource format "%s". Deleting datastore tables for resource %s' + % (res.get(u'format', u'').lower(), res['id'])) try: - lc.action.datastore_delete(resource_id=res['id'], force=True) + toolkit.get_action('datastore_delete')(context, { + "resource_id": res['id'], + "force": True}) log.info('Datastore table dropped for resource %s' % res['id']) except toolkit.ObjectNotFound: log.error('Datastore table for resource %s does not exist' % res['id']) diff --git a/requirements.txt b/requirements.txt index d5373526..b00db5d8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -5,4 +5,3 @@ tabulator==1.53.5 Unidecode==1.0.22 python-dateutil>=2.8.2 chardet==5.2.0 -ckanapi From 2e50f7cf4f40ecdd50d018b9ed162e6049e67d80 Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Thu, 9 Nov 2023 15:43:02 +0000 Subject: [PATCH 03/10] removal(import): removed ckanapi import; - Removed `ckanapi` import. --- ckanext/xloader/plugin.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/ckanext/xloader/plugin.py b/ckanext/xloader/plugin.py index f3ec0ad0..a4418e3d 100644 --- a/ckanext/xloader/plugin.py +++ b/ckanext/xloader/plugin.py @@ -7,8 +7,6 @@ from ckan.model.domain_object import DomainObjectOperation from ckan.model.resource import Resource -import ckanapi - from . import action, auth, helpers as xloader_helpers, utils from ckanext.xloader.utils import XLoaderFormats From 0a210cf742d55e0be4f53d4e12fb556ec237f665 Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Thu, 9 Nov 2023 20:07:14 +0000 Subject: [PATCH 04/10] fix(dev): falsy values for datastore and variable logging; - Better logging. - Added `asbool` for `datastore_active` checks. --- ckanext/xloader/plugin.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ckanext/xloader/plugin.py b/ckanext/xloader/plugin.py index a4418e3d..2c5e86d1 100644 --- a/ckanext/xloader/plugin.py +++ b/ckanext/xloader/plugin.py @@ -207,8 +207,8 @@ def _should_remove_unsupported_resource_from_datastore(res_dict): return (not XLoaderFormats.is_it_an_xloader_format(res_dict.get('format', u'')) and (res_dict.get('url_type') == 'upload' or res_dict.get('url_type') == '') - and (res_dict.get('datastore_active', False) - or res_dict.get('extras', {}).get('datastore_active', False))) + and (toolkit.asbool(res_dict.get('datastore_active', False)) + or toolkit.asbool(res_dict.get('extras', {}).get('datastore_active', False)))) def _remove_unsupported_resource_from_datastore(resource_id): @@ -222,16 +222,16 @@ def _remove_unsupported_resource_from_datastore(resource_id): try: res = toolkit.get_action('resource_show')(context, {"id": resource_id}) except toolkit.ObjectNotFound: - log.error('Resource %s does not exist.' % res['id']) + log.error('Resource %s does not exist.', res['id']) return if _should_remove_unsupported_resource_from_datastore(res): - log.info('Unsupported resource format "%s". Deleting datastore tables for resource %s' - % (res.get(u'format', u'').lower(), res['id'])) + log.info('Unsupported resource format "%s". Deleting datastore tables for resource %s', + res.get(u'format', u'').lower(), res['id']) try: toolkit.get_action('datastore_delete')(context, { "resource_id": res['id'], "force": True}) - log.info('Datastore table dropped for resource %s' % res['id']) + log.info('Datastore table dropped for resource %s', res['id']) except toolkit.ObjectNotFound: - log.error('Datastore table for resource %s does not exist' % res['id']) + log.error('Datastore table for resource %s does not exist', res['id']) From 83281322e44c856d55d254ddc865467f915e5101 Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Fri, 10 Nov 2023 16:20:36 +0000 Subject: [PATCH 05/10] fix(dev): falsy for url type; - Added falsy check for `url_type`. --- ckanext/xloader/plugin.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ckanext/xloader/plugin.py b/ckanext/xloader/plugin.py index 2c5e86d1..b68900a1 100644 --- a/ckanext/xloader/plugin.py +++ b/ckanext/xloader/plugin.py @@ -204,9 +204,14 @@ def get_helpers(self): def _should_remove_unsupported_resource_from_datastore(res_dict): if not toolkit.asbool(toolkit.config.get('ckanext.xloader.clean_datastore_tables', False)): return False + has_url_type = True + try: + has_url_type = toolkit.asbool(res_dict.get('url_type')) + except ValueError: + pass return (not XLoaderFormats.is_it_an_xloader_format(res_dict.get('format', u'')) and (res_dict.get('url_type') == 'upload' - or res_dict.get('url_type') == '') + or not has_url_type) and (toolkit.asbool(res_dict.get('datastore_active', False)) or toolkit.asbool(res_dict.get('extras', {}).get('datastore_active', False)))) From 0bc5f9b841dca006db2e50e194ce1c8741656d74 Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Wed, 15 Nov 2023 14:19:06 +0000 Subject: [PATCH 06/10] fix(dev): falsy fix, removed lower from logging; - Removed `lower` call in logging. - Fixed falsy check on `url_type`. --- ckanext/xloader/plugin.py | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/ckanext/xloader/plugin.py b/ckanext/xloader/plugin.py index b68900a1..0ef0ea50 100644 --- a/ckanext/xloader/plugin.py +++ b/ckanext/xloader/plugin.py @@ -204,14 +204,9 @@ def get_helpers(self): def _should_remove_unsupported_resource_from_datastore(res_dict): if not toolkit.asbool(toolkit.config.get('ckanext.xloader.clean_datastore_tables', False)): return False - has_url_type = True - try: - has_url_type = toolkit.asbool(res_dict.get('url_type')) - except ValueError: - pass return (not XLoaderFormats.is_it_an_xloader_format(res_dict.get('format', u'')) and (res_dict.get('url_type') == 'upload' - or not has_url_type) + or not res_dict.get('url_type')) and (toolkit.asbool(res_dict.get('datastore_active', False)) or toolkit.asbool(res_dict.get('extras', {}).get('datastore_active', False)))) @@ -227,12 +222,12 @@ def _remove_unsupported_resource_from_datastore(resource_id): try: res = toolkit.get_action('resource_show')(context, {"id": resource_id}) except toolkit.ObjectNotFound: - log.error('Resource %s does not exist.', res['id']) + log.error('Resource %s does not exist.', resource_id) return if _should_remove_unsupported_resource_from_datastore(res): log.info('Unsupported resource format "%s". Deleting datastore tables for resource %s', - res.get(u'format', u'').lower(), res['id']) + res.get(u'format', u''), res['id']) try: toolkit.get_action('datastore_delete')(context, { "resource_id": res['id'], From 7fabca4e0bd15bc4f3f8d571c44533b74757c70d Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Wed, 31 Jan 2024 16:47:37 +0000 Subject: [PATCH 07/10] fix(syntax): flake8; - Syntax fixes from flake8. --- ckanext/xloader/plugin.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/ckanext/xloader/plugin.py b/ckanext/xloader/plugin.py index 0ef0ea50..dcb38c73 100644 --- a/ckanext/xloader/plugin.py +++ b/ckanext/xloader/plugin.py @@ -4,8 +4,10 @@ from ckan import plugins from ckan.plugins import toolkit + from ckan.model.domain_object import DomainObjectOperation from ckan.model.resource import Resource +from ckan.model.package import Package from . import action, auth, helpers as xloader_helpers, utils from ckanext.xloader.utils import XLoaderFormats @@ -69,14 +71,15 @@ def configure(self, config_): # IDomainObjectModification def notify(self, entity, operation): - # type: (ckan.model.Package|ckan.model.Resource, DomainObjectOperation) -> None + # type: (Package|Resource, DomainObjectOperation) -> None """ Runs before_commit to database for Packages and Resources. We only want to check for changed Resources for this. We want to check if values have changed, namely the url and the format. See: ckan/model/modification.py.DomainObjectModificationExtension """ - if operation != DomainObjectOperation.changed or not isinstance(entity, Resource): + if operation != DomainObjectOperation.changed \ + or not isinstance(entity, Resource): return context = { @@ -206,9 +209,9 @@ def _should_remove_unsupported_resource_from_datastore(res_dict): return False return (not XLoaderFormats.is_it_an_xloader_format(res_dict.get('format', u'')) and (res_dict.get('url_type') == 'upload' - or not res_dict.get('url_type')) + or not res_dict.get('url_type')) and (toolkit.asbool(res_dict.get('datastore_active', False)) - or toolkit.asbool(res_dict.get('extras', {}).get('datastore_active', False)))) + or toolkit.asbool(res_dict.get('extras', {}).get('datastore_active', False)))) def _remove_unsupported_resource_from_datastore(resource_id): @@ -227,7 +230,7 @@ def _remove_unsupported_resource_from_datastore(resource_id): if _should_remove_unsupported_resource_from_datastore(res): log.info('Unsupported resource format "%s". Deleting datastore tables for resource %s', - res.get(u'format', u''), res['id']) + res.get(u'format', u''), res['id']) try: toolkit.get_action('datastore_delete')(context, { "resource_id": res['id'], From cef20a919b48853b7f1126a0c49fd6620109780a Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Wed, 31 Jan 2024 18:20:08 +0000 Subject: [PATCH 08/10] feat(tests): added new test; - Added new test for should remove unsupported resources from datastore. --- ckanext/xloader/plugin.py | 1 + ckanext/xloader/tests/test_plugin.py | 60 ++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/ckanext/xloader/plugin.py b/ckanext/xloader/plugin.py index ec234abb..e0ce027e 100644 --- a/ckanext/xloader/plugin.py +++ b/ckanext/xloader/plugin.py @@ -96,6 +96,7 @@ def notify(self, entity, operation): toolkit.enqueue_job(fn=_remove_unsupported_resource_from_datastore, args=[entity.id]) if not getattr(entity, 'url_changed', False): + # do not submit to xloader if the url has not changed. return self._submit_to_xloader(resource_dict) diff --git a/ckanext/xloader/tests/test_plugin.py b/ckanext/xloader/tests/test_plugin.py index 05b83b5b..8988e750 100644 --- a/ckanext/xloader/tests/test_plugin.py +++ b/ckanext/xloader/tests/test_plugin.py @@ -9,6 +9,21 @@ from six import text_type as str from ckan.tests import helpers, factories from ckan.logic import _actions +from ckanext.xloader.plugin import _should_remove_unsupported_resource_from_datastore + + +@pytest.fixture +def mock_toolkit_config(request): + with mock.patch('ckan.plugins.toolkit.config.get') as mock_get: + mock_get.return_value = request.param + yield mock_get + + +@pytest.fixture +def mock_xloader_formats(request): + with mock.patch('ckanext.xloader.utils.XLoaderFormats.is_it_an_xloader_format') as mock_is_xloader_format: + mock_is_xloader_format.return_value = request.param + yield mock_is_xloader_format @pytest.mark.usefixtures("clean_db", "with_plugins") @@ -58,6 +73,51 @@ def test_submit_when_url_changes(self, monkeypatch): assert func.called + @pytest.mark.parametrize("toolkit_config_value, xloader_formats_value, url_type, datastore_active, expected_result", + [(True, True, 'upload', True, True), # Test1 + (True, False, 'upload', True, False), # Test2 + (False, True, 'upload', True, False), # Test3 + (False, False, 'upload', True, False), # Test4 + (True, True, 'custom_type', True, False), # Test5 + (True, True, 'upload', False, False), # Test6 + (True, True, '', True, True), # Test7 + (True, True, None, True, True), # Test8 + ]) + def test_should_remove_unsupported_resource_from_datastore( + mock_toolkit_config, mock_xloader_formats, toolkit_config_value, + xloader_formats_value, url_type, datastore_active, expected_result): + + # Test1: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='upload', datastore_active=True, expected_result=True + # Should pass as it is an Xloader format and supported url type and datastore active. + # Test2: clean_datastore_tables=True, is_it_an_xloader_format=False, url_type='upload', datastore_active=True, expected_result=False + # Should fail as it is not a supported Xloader format. + # Test3: clean_datastore_tables=False, is_it_an_xloader_format=True, url_type='upload', datastore_active=True, expected_result=False + # Should fail as the config option is turned off. + # Test4: clean_datastore_tables=False, is_it_an_xloader_format=False, url_type='upload', datastore_active=True, expected_result=False + # Should fail as the config option is turned off and the Xloader format is not supported. + # Test5: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='custom_type', datastore_active=True, expected_result=False + # Should fail as the url_type is not supported. + # Test6: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='upload', datastore_active=False, expected_result=False + # Should fail as datastore is inactive. + # Test7: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='', datastore_active=True, expected_result=True + # Should pass as it is an Xloader format and supported url type and datastore active. + # Test8: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type=None, datastore_active=True, expected_result=True + # Should pass as it is an Xloader format and supported url type as falsy and datastore active. + + # Setup mock data + res_dict = { + 'format': 'some_format', + 'url_type': url_type, + 'datastore_active': True, + 'extras': {'datastore_active': True} + } + + # Call the function + result = _should_remove_unsupported_resource_from_datastore(res_dict) + + # Assert the result based on the logic paths covered + assert result == expected_result + def _pending_task(self, resource_id): return { "entity_id": resource_id, From d7236fe365fa9eb7397b96c3fdf38efb20b2473e Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Thu, 1 Feb 2024 15:45:12 +0000 Subject: [PATCH 09/10] fix(tests): finalized test method; - Finalized fixtures and params for new test method. --- ckanext/xloader/tests/test_plugin.py | 63 ++++++++++++++-------------- 1 file changed, 31 insertions(+), 32 deletions(-) diff --git a/ckanext/xloader/tests/test_plugin.py b/ckanext/xloader/tests/test_plugin.py index 8988e750..e979d2e2 100644 --- a/ckanext/xloader/tests/test_plugin.py +++ b/ckanext/xloader/tests/test_plugin.py @@ -9,14 +9,19 @@ from six import text_type as str from ckan.tests import helpers, factories from ckan.logic import _actions +from ckan.plugins import toolkit from ckanext.xloader.plugin import _should_remove_unsupported_resource_from_datastore @pytest.fixture -def mock_toolkit_config(request): - with mock.patch('ckan.plugins.toolkit.config.get') as mock_get: - mock_get.return_value = request.param - yield mock_get +def toolkit_config_value(request): + _original_config = toolkit.config.copy() + toolkit.config['ckanext.xloader.clean_datastore_tables'] = request.param + try: + yield + finally: + toolkit.config.clear() + toolkit.config.update(_original_config) @pytest.fixture @@ -73,50 +78,44 @@ def test_submit_when_url_changes(self, monkeypatch): assert func.called - @pytest.mark.parametrize("toolkit_config_value, xloader_formats_value, url_type, datastore_active, expected_result", - [(True, True, 'upload', True, True), # Test1 - (True, False, 'upload', True, False), # Test2 - (False, True, 'upload', True, False), # Test3 - (False, False, 'upload', True, False), # Test4 - (True, True, 'custom_type', True, False), # Test5 - (True, True, 'upload', False, False), # Test6 - (True, True, '', True, True), # Test7 - (True, True, None, True, True), # Test8 - ]) + @pytest.mark.usefixtures("toolkit_config_value", "mock_xloader_formats") + @pytest.mark.parametrize("toolkit_config_value, mock_xloader_formats, url_type, datastore_active, expected_result", + [(True, False, 'upload', True, True), # Test1 + (True, True, 'upload', True, False), # Test2 + (False, False, 'upload', True, False), # Test3 + (True, False, 'custom_type', True, False), # Test4 + (True, False, 'upload', False, False), # Test5 + (True, False, '', True, True), # Test6 + (True, False, None, True, True), # Test7 + ], indirect=["toolkit_config_value", "mock_xloader_formats"]) def test_should_remove_unsupported_resource_from_datastore( - mock_toolkit_config, mock_xloader_formats, toolkit_config_value, - xloader_formats_value, url_type, datastore_active, expected_result): + toolkit_config_value, mock_xloader_formats, url_type, datastore_active, expected_result): # Test1: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='upload', datastore_active=True, expected_result=True - # Should pass as it is an Xloader format and supported url type and datastore active. + # Should pass as it is not an XLoader format and supported url type and datastore active. # Test2: clean_datastore_tables=True, is_it_an_xloader_format=False, url_type='upload', datastore_active=True, expected_result=False - # Should fail as it is not a supported Xloader format. + # Should fail as it is a supported XLoader format. # Test3: clean_datastore_tables=False, is_it_an_xloader_format=True, url_type='upload', datastore_active=True, expected_result=False # Should fail as the config option is turned off. - # Test4: clean_datastore_tables=False, is_it_an_xloader_format=False, url_type='upload', datastore_active=True, expected_result=False - # Should fail as the config option is turned off and the Xloader format is not supported. - # Test5: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='custom_type', datastore_active=True, expected_result=False + # Test4: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='custom_type', datastore_active=True, expected_result=False # Should fail as the url_type is not supported. - # Test6: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='upload', datastore_active=False, expected_result=False + # Test5: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='upload', datastore_active=False, expected_result=False # Should fail as datastore is inactive. - # Test7: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='', datastore_active=True, expected_result=True - # Should pass as it is an Xloader format and supported url type and datastore active. - # Test8: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type=None, datastore_active=True, expected_result=True - # Should pass as it is an Xloader format and supported url type as falsy and datastore active. + # Test6: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='', datastore_active=True, expected_result=True + # Should pass as it is not an XLoader format and supported url type and datastore active. + # Test7: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type=None, datastore_active=True, expected_result=True + # Should pass as it is not an XLoader format and supported url type as falsy and datastore active. # Setup mock data res_dict = { 'format': 'some_format', 'url_type': url_type, - 'datastore_active': True, - 'extras': {'datastore_active': True} + 'datastore_active': datastore_active, + 'extras': {'datastore_active': datastore_active} } - # Call the function - result = _should_remove_unsupported_resource_from_datastore(res_dict) - # Assert the result based on the logic paths covered - assert result == expected_result + assert _should_remove_unsupported_resource_from_datastore(res_dict) == expected_result def _pending_task(self, resource_id): return { From 8b6cab9469cabc0eab71ec80c0c2b6f26f888eb3 Mon Sep 17 00:00:00 2001 From: antuarc Date: Fri, 2 Feb 2024 12:28:48 +1000 Subject: [PATCH 10/10] replace complex indirect fixtures with simple 'with' blocks - Also move the test case documentation alongside the test data so cases can be added or removed in just one place --- ckanext/xloader/tests/test_plugin.py | 68 +++++++++------------------- 1 file changed, 22 insertions(+), 46 deletions(-) diff --git a/ckanext/xloader/tests/test_plugin.py b/ckanext/xloader/tests/test_plugin.py index e979d2e2..8382e68b 100644 --- a/ckanext/xloader/tests/test_plugin.py +++ b/ckanext/xloader/tests/test_plugin.py @@ -7,30 +7,12 @@ except ImportError: import mock from six import text_type as str + from ckan.tests import helpers, factories from ckan.logic import _actions -from ckan.plugins import toolkit from ckanext.xloader.plugin import _should_remove_unsupported_resource_from_datastore -@pytest.fixture -def toolkit_config_value(request): - _original_config = toolkit.config.copy() - toolkit.config['ckanext.xloader.clean_datastore_tables'] = request.param - try: - yield - finally: - toolkit.config.clear() - toolkit.config.update(_original_config) - - -@pytest.fixture -def mock_xloader_formats(request): - with mock.patch('ckanext.xloader.utils.XLoaderFormats.is_it_an_xloader_format') as mock_is_xloader_format: - mock_is_xloader_format.return_value = request.param - yield mock_is_xloader_format - - @pytest.mark.usefixtures("clean_db", "with_plugins") @pytest.mark.ckan_config("ckan.plugins", "datastore xloader") class TestNotify(object): @@ -78,33 +60,24 @@ def test_submit_when_url_changes(self, monkeypatch): assert func.called - @pytest.mark.usefixtures("toolkit_config_value", "mock_xloader_formats") - @pytest.mark.parametrize("toolkit_config_value, mock_xloader_formats, url_type, datastore_active, expected_result", - [(True, False, 'upload', True, True), # Test1 - (True, True, 'upload', True, False), # Test2 - (False, False, 'upload', True, False), # Test3 - (True, False, 'custom_type', True, False), # Test4 - (True, False, 'upload', False, False), # Test5 - (True, False, '', True, True), # Test6 - (True, False, None, True, True), # Test7 - ], indirect=["toolkit_config_value", "mock_xloader_formats"]) + @pytest.mark.parametrize("toolkit_config_value, mock_xloader_formats, url_type, datastore_active, expected_result", [ + # Test1: Should pass as it is an upload with an active datastore entry but an unsupported format + (True, False, 'upload', True, True), + # Test2: Should fail as it is a supported XLoader format. + (True, True, 'upload', True, False), + # Test3: Should fail as the config option is turned off. + (False, False, 'upload', True, False), + # Test4: Should fail as the url_type is not supported. + (True, False, 'custom_type', True, False), + # Test5: Should fail as datastore is inactive. + (True, False, 'upload', False, False), + # Test6: Should pass as it is a recognised resource type with an active datastore entry but an unsupported format + (True, False, '', True, True), + # Test7: Should pass as it is a recognised resource type with an active datastore entry but an unsupported format + (True, False, None, True, True), + ]) def test_should_remove_unsupported_resource_from_datastore( - toolkit_config_value, mock_xloader_formats, url_type, datastore_active, expected_result): - - # Test1: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='upload', datastore_active=True, expected_result=True - # Should pass as it is not an XLoader format and supported url type and datastore active. - # Test2: clean_datastore_tables=True, is_it_an_xloader_format=False, url_type='upload', datastore_active=True, expected_result=False - # Should fail as it is a supported XLoader format. - # Test3: clean_datastore_tables=False, is_it_an_xloader_format=True, url_type='upload', datastore_active=True, expected_result=False - # Should fail as the config option is turned off. - # Test4: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='custom_type', datastore_active=True, expected_result=False - # Should fail as the url_type is not supported. - # Test5: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='upload', datastore_active=False, expected_result=False - # Should fail as datastore is inactive. - # Test6: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type='', datastore_active=True, expected_result=True - # Should pass as it is not an XLoader format and supported url type and datastore active. - # Test7: clean_datastore_tables=True, is_it_an_xloader_format=True, url_type=None, datastore_active=True, expected_result=True - # Should pass as it is not an XLoader format and supported url type as falsy and datastore active. + self, toolkit_config_value, mock_xloader_formats, url_type, datastore_active, expected_result): # Setup mock data res_dict = { @@ -115,7 +88,10 @@ def test_should_remove_unsupported_resource_from_datastore( } # Assert the result based on the logic paths covered - assert _should_remove_unsupported_resource_from_datastore(res_dict) == expected_result + with helpers.changed_config('ckanext.xloader.clean_datastore_tables', toolkit_config_value): + with mock.patch('ckanext.xloader.utils.XLoaderFormats.is_it_an_xloader_format') as mock_is_xloader_format: + mock_is_xloader_format.return_value = mock_xloader_formats + assert _should_remove_unsupported_resource_from_datastore(res_dict) == expected_result def _pending_task(self, resource_id): return {