From 67271cc9ac1e769b343edbb0605562ddb16da808 Mon Sep 17 00:00:00 2001 From: Manasa Venkatakrishnan <14958785+manasaV3@users.noreply.github.com> Date: Thu, 22 Feb 2024 13:58:47 -0800 Subject: [PATCH] Validate plugins removed from hub before marking them stale (#1301) * Updating processor to sanity check deletion * Adding Classifier Adapters * Adding back Zulip credentials * Fixing linting * Fixing linting for napari-hub-commons * Installing libffi7 * Updating ubuntu distros * Updating tests * Revert "Installing libffi7" This reverts commit d9e4039b4dd05c2e13a8b792fdac257ec4a9202e. * Revert "Updating ubuntu distros" This reverts commit b9b3320ae4445d2362f04b617166840572c6f75e. * Revert "Adding back Zulip credentials" This reverts commit 6597f4554c6e8aeaf0140d5710ed15f380233050. * Fix linting --- data-workflows/plugin/classifier_adapter.py | 28 +++++++++ data-workflows/plugin/processor.py | 25 +++++--- .../plugin/tests/test_classifier_adapter.py | 61 +++++++++++++++++++ data-workflows/plugin/tests/test_processor.py | 36 +++++++++-- .../src/nhcommons/tests/models/test_plugin.py | 2 +- .../src/nhcommons/utils/adapter_helpers.py | 8 ++- 6 files changed, 143 insertions(+), 17 deletions(-) create mode 100644 data-workflows/plugin/classifier_adapter.py create mode 100644 data-workflows/plugin/tests/test_classifier_adapter.py diff --git a/data-workflows/plugin/classifier_adapter.py b/data-workflows/plugin/classifier_adapter.py new file mode 100644 index 000000000..70baa8121 --- /dev/null +++ b/data-workflows/plugin/classifier_adapter.py @@ -0,0 +1,28 @@ +import logging +from functools import cache + +from nhcommons.utils.adapter_helpers import GithubClientHelper + + +REPOSITORY = "https://github.com/napari/npe2api" +FILE_NAME = "public/classifiers.json" +LOGGER = logging.getLogger(__name__) + + +@cache +def _get_recent_query_data() -> dict: + github_client_helper = GithubClientHelper(REPOSITORY) + data = github_client_helper.get_file(FILE_NAME, "json") + if not data: + raise RuntimeError(f"Unable to fetch {FILE_NAME} from {REPOSITORY}") + return data + + +def is_plugin_active(name: str, version: str) -> bool: + try: + recent_query_update = _get_recent_query_data() + active_versions = set(recent_query_update.get("active", {}).get(name, [])) + return version in active_versions + except RuntimeError: + LOGGER.warning(f"Returning {name} {version} is active due to RuntimeError") + return True diff --git a/data-workflows/plugin/processor.py b/data-workflows/plugin/processor.py index 697daa868..27d44fcef 100644 --- a/data-workflows/plugin/processor.py +++ b/data-workflows/plugin/processor.py @@ -2,6 +2,7 @@ from concurrent import futures from typing import Optional +from plugin.classifier_adapter import is_plugin_active from plugin.lambda_adapter import LambdaAdapter from nhcommons.models.plugin_utils import PluginMetadataType from nhcommons.utils import pypi_adapter @@ -39,15 +40,23 @@ def _is_new_plugin(plugin_version_pair): # update for removed plugins and existing older version of plugins for name, version in dynamo_latest_plugins.items(): pypi_plugin_version = pypi_latest_plugins.get(name) - if pypi_plugin_version != version: - logger.info(f"Updating old plugin={name} version={version}") - put_plugin_metadata( - plugin=name, - version=version, - plugin_metadata_type=PluginMetadataType.PYPI, + if pypi_plugin_version == version: + continue + if pypi_plugin_version is None and is_plugin_active(name, version): + logger.info( + f"Skipping marking plugin={name} version={version} stale as the " + f"plugin is still active in npe2api" ) - if pypi_plugin_version is None: - zulip.plugin_no_longer_on_hub(name) + continue + + logger.info(f"Updating old plugin={name} version={version}") + put_plugin_metadata( + plugin=name, + version=version, + plugin_metadata_type=PluginMetadataType.PYPI, + ) + if pypi_plugin_version is None: + zulip.plugin_no_longer_on_hub(name) def _update_for_new_plugin(name: str, version: str, old_version: Optional[str]) -> None: diff --git a/data-workflows/plugin/tests/test_classifier_adapter.py b/data-workflows/plugin/tests/test_classifier_adapter.py new file mode 100644 index 000000000..4c9193008 --- /dev/null +++ b/data-workflows/plugin/tests/test_classifier_adapter.py @@ -0,0 +1,61 @@ +from unittest.mock import Mock, call + +import pytest + +import nhcommons.utils.adapter_helpers +import plugin.classifier_adapter + +REPOSITORY = "https://github.com/napari/npe2api" +FILE_NAME = "public/classifiers.json" + + +class TestClassifierAdapter: + @pytest.fixture(autouse=True) + def clear_cache(self): + plugin.classifier_adapter._get_recent_query_data.cache_clear() + + @pytest.fixture(autouse=True) + def github_client_helper(self, monkeypatch): + self._github_client_helper = Mock() + self._github_client_helper.get_file.side_effect = ( + lambda _, __: self._classifier_json + ) + self._github_client_helper_call = Mock( + spec=nhcommons.utils.adapter_helpers.GithubClientHelper, + return_value=self._github_client_helper, + ) + monkeypatch.setattr( + plugin.classifier_adapter, + "GithubClientHelper", + self._github_client_helper_call, + ) + + def test_handle_valid_query_data(self): + self._classifier_json = {"active": {"foo": ["1.0.0", "1.0.1", "1.0.2"]}} + assert True == plugin.classifier_adapter.is_plugin_active("foo", "1.0.2") + assert False == plugin.classifier_adapter.is_plugin_active("foo", "0.0.9") + assert False == plugin.classifier_adapter.is_plugin_active("bar", "1.0.4") + self._github_client_helper_call.assert_called_once_with(REPOSITORY) + self._github_client_helper.get_file.assert_called_once_with(FILE_NAME, "json") + + def test_handle_invalid_query_data(self): + self._classifier_json = {"inactive": []} + assert False == plugin.classifier_adapter.is_plugin_active("foo", "1.0.2") + assert False == plugin.classifier_adapter.is_plugin_active("bar", "1.0.2") + self._github_client_helper_call.assert_called_once_with(REPOSITORY) + self._github_client_helper.get_file.assert_called_once_with(FILE_NAME, "json") + + @pytest.mark.parametrize("init_classifier_json", [None, {}]) + def test_is_plugin_live_does_not_cache_error(self, init_classifier_json): + # When the classifier json is not a dict with values, _get_recent_query_data() + # should throw a RuntimeError + self._classifier_json = init_classifier_json + assert True == plugin.classifier_adapter.is_plugin_active("foo", "1.0.2") + self._classifier_json = {"active": {"foo": ["1.0.0", "1.0.1", "1.0.2"]}} + assert True == plugin.classifier_adapter.is_plugin_active("foo", "1.0.2") + self._github_client_helper_call.assert_has_calls( + [call(REPOSITORY), call(REPOSITORY)] + ) + self._github_client_helper.get_file.assert_has_calls( + [call(FILE_NAME, "json"), call(FILE_NAME, "json")] + ) diff --git a/data-workflows/plugin/tests/test_processor.py b/data-workflows/plugin/tests/test_processor.py index c9ae5f0bb..b2f49ba93 100644 --- a/data-workflows/plugin/tests/test_processor.py +++ b/data-workflows/plugin/tests/test_processor.py @@ -3,6 +3,7 @@ import pytest import plugin.lambda_adapter +import plugin.classifier_adapter import plugin.metadata from nhcommons.models.plugin_utils import PluginMetadataType as PMType @@ -68,6 +69,15 @@ def mock_lambda_adapter(self, monkeypatch) -> Mock: monkeypatch.setattr(processor, "LambdaAdapter", mock) return mock + @pytest.fixture + def mock_classifier_adapter(self, monkeypatch) -> Mock: + mock = Mock( + side_effect=lambda _, __: self._is_plugin_live, + spec=plugin.classifier_adapter, + ) + monkeypatch.setattr(processor, "is_plugin_active", mock) + return mock + @pytest.fixture def mock_zulip(self, monkeypatch) -> Mock: mock = Mock(spec=zulip) @@ -83,6 +93,7 @@ def setup( mock_get_existing_types, mock_get_formatted_metadata, mock_lambda_adapter, + mock_classifier_adapter, mock_zulip, ) -> None: self._get_latest_plugins = mock_get_latest_plugins @@ -91,6 +102,7 @@ def setup( self._get_existing_types = mock_get_existing_types self._get_formatted_metadata = mock_get_formatted_metadata self._lambda_adapter = mock_lambda_adapter + self._classifier_adapter = mock_classifier_adapter self._zulip = mock_zulip @pytest.fixture @@ -103,6 +115,7 @@ def _verify_calls( get_formatted_metadata_called: bool = False, lambda_invoked: bool = False, put_pm_calls: list = None, + classifier_adapter_not_called: bool = True, ) -> None: verify_call(True, self._get_latest_plugins, empty_call_list) verify_call(True, self._get_all_plugins, empty_call_list) @@ -125,6 +138,8 @@ def _verify_calls( default_call_list == self._lambda_adapter.return_value.invoke.call_args_list ) + if classifier_adapter_not_called: + self._classifier_adapter.assert_not_called() return _verify_calls @@ -138,16 +153,27 @@ def test_all_latest_plugin_in_dynamo(self, verify_calls): verify_calls() self._zulip.assert_not_called() - def test_stale_plugin_in_dynamo(self, verify_calls): + @pytest.mark.parametrize("is_plugin_live", [False, True]) + def test_stale_plugin_in_dynamo( + self, + is_plugin_live, + verify_calls, + ): self._dynamo_latest_plugins = {PLUGIN: VERSION, "bar": "2.4.6"} self._pypi_latest_plugins = {"bar": "2.4.6"} - put_pm_calls = [_create_put_pm_call(PMType.PYPI)] + self._is_plugin_live = is_plugin_live processor.update_plugin() - verify_calls(put_pm_calls=put_pm_calls) - assert len(self._zulip.method_calls) == 1 - self._zulip.plugin_no_longer_on_hub.assert_called_once_with(PLUGIN) + if is_plugin_live: + assert len(self._zulip.method_calls) == 0 + put_pm_calls = [] + else: + assert len(self._zulip.method_calls) == 1 + self._zulip.plugin_no_longer_on_hub.assert_called_once_with(PLUGIN) + put_pm_calls = [_create_put_pm_call(PMType.PYPI)] + verify_calls(put_pm_calls=put_pm_calls, classifier_adapter_not_called=False) + self._classifier_adapter.assert_called_once_with(PLUGIN, VERSION) @pytest.mark.parametrize( "existing_types, put_pm_data, formatted_metadata", diff --git a/napari-hub-commons/src/nhcommons/tests/models/test_plugin.py b/napari-hub-commons/src/nhcommons/tests/models/test_plugin.py index 3190538a5..e556e6fd1 100644 --- a/napari-hub-commons/src/nhcommons/tests/models/test_plugin.py +++ b/napari-hub-commons/src/nhcommons/tests/models/test_plugin.py @@ -135,7 +135,7 @@ def seed_data(self, put_item): "2.2", release_date="2023-03-01", data=plugin_data("plugin-1", "2.2"), - is_latest=True + is_latest=True, ) put_item( "Plugin-2", diff --git a/napari-hub-commons/src/nhcommons/utils/adapter_helpers.py b/napari-hub-commons/src/nhcommons/utils/adapter_helpers.py index 94c4573cf..2c25ca8ae 100644 --- a/napari-hub-commons/src/nhcommons/utils/adapter_helpers.py +++ b/napari-hub-commons/src/nhcommons/utils/adapter_helpers.py @@ -1,5 +1,5 @@ import logging -from typing import Dict, List, Optional +from typing import Dict, List, Optional, Union import yaml from cffconvert import Citation @@ -47,7 +47,9 @@ def get_first_valid_file( return file return None - def get_file(self, file: str = "", file_format: str = "") -> Optional[str]: + def get_file( + self, file: str = "", file_format: str = "" + ) -> Optional[Union[str, dict]]: """ Get file from github. :param file: filename to get if specified @@ -61,7 +63,7 @@ def get_file(self, file: str = "", file_format: str = "") -> Optional[str]: response = get_request(api_url, auth=self._auth) return response.json() if file_format == "json" else response.text except (HTTPError, JSONDecodeError): - logger.error(f"Encountered error fetching {api_url}") + logger.error(f"Encountered error fetching {api_url}", exc_info=True) return None @classmethod