From 3f21193bc4f316affdaf68d6cb15a468c473c7d9 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Mon, 5 Aug 2024 11:06:14 -0400 Subject: [PATCH 1/3] Improve tool shed API error messages for repo path bug. --- lib/galaxy/exceptions/__init__.py | 5 +++++ lib/galaxy/exceptions/error_codes.json | 5 +++++ lib/tool_shed/managers/tools.py | 9 +++++++++ 3 files changed, 19 insertions(+) diff --git a/lib/galaxy/exceptions/__init__.py b/lib/galaxy/exceptions/__init__.py index 8deab77bc17b..82f66efdbe7d 100644 --- a/lib/galaxy/exceptions/__init__.py +++ b/lib/galaxy/exceptions/__init__.py @@ -250,6 +250,11 @@ class InconsistentDatabase(MessageException): err_code = error_codes_by_name["INCONSISTENT_DATABASE"] +class InconsistentApplicationState(MessageException): + status_code = 500 + err_code = error_codes_by_name["INCONSISTENT_APPLICATION_STATE"] + + class InternalServerError(MessageException): status_code = 500 err_code = error_codes_by_name["INTERNAL_SERVER_ERROR"] diff --git a/lib/galaxy/exceptions/error_codes.json b/lib/galaxy/exceptions/error_codes.json index 99464306acb6..e148b102b786 100644 --- a/lib/galaxy/exceptions/error_codes.json +++ b/lib/galaxy/exceptions/error_codes.json @@ -189,6 +189,11 @@ "code": 500006, "message": "Reference data required for program execution failed to load." }, + { + "name": "INCONSISTENT_APPLICATION_STATE", + "code": 500007, + "message": "Inconsistent application state (likely not dbms related) prevented fulfilling the request." + }, { "name": "NOT_IMPLEMENTED", "code": 501001, diff --git a/lib/tool_shed/managers/tools.py b/lib/tool_shed/managers/tools.py index b9dc6209e1b3..6a6872b77ec4 100644 --- a/lib/tool_shed/managers/tools.py +++ b/lib/tool_shed/managers/tools.py @@ -10,6 +10,7 @@ from galaxy import exceptions from galaxy.exceptions import ( + InconsistentApplicationState, InternalServerError, ObjectNotFound, RequestParameterInvalidException, @@ -149,8 +150,16 @@ def _shed_tool_source_for( if error_message: raise InternalServerError("Failed to materialize target repository revision") repo_files_dir = repository_metadata.repository.repo_path(trans.app) + if not repo_files_dir: + raise InconsistentApplicationState( + f"Failed to resolve repository path from hgweb_config_manager for [{trs_tool_id}], inconsistent repository state or application configuration" + ) repo_rel_tool_path = relpath(tool_config, repo_files_dir) path_to_tool = os.path.join(work_dir, repo_rel_tool_path) + if not os.path.exists(path_to_tool): + raise InconsistentApplicationState( + f"Target tool expected at [{path_to_tool}] and not found, inconsistent repository state or application configuration" + ) tool_source = get_tool_source(path_to_tool) return tool_source finally: From efe76b53e0aa363f790c4a05c46c51835ec2dfab Mon Sep 17 00:00:00 2001 From: John Chilton Date: Mon, 5 Aug 2024 11:15:22 -0400 Subject: [PATCH 2/3] Skip buggy hgweb_config_manager to find tool sources. --- lib/tool_shed/managers/tools.py | 2 +- lib/tool_shed/util/repository_util.py | 7 +------ lib/tool_shed/webapp/model/__init__.py | 13 +++++++++++++ 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/lib/tool_shed/managers/tools.py b/lib/tool_shed/managers/tools.py index 6a6872b77ec4..0768758fc9bb 100644 --- a/lib/tool_shed/managers/tools.py +++ b/lib/tool_shed/managers/tools.py @@ -149,7 +149,7 @@ def _shed_tool_source_for( cloned_ok, error_message = clone_repository(repository_clone_url, work_dir, str(ctx.rev())) if error_message: raise InternalServerError("Failed to materialize target repository revision") - repo_files_dir = repository_metadata.repository.repo_path(trans.app) + repo_files_dir = repository_metadata.repository.hg_repository_path(trans.app.config.file_path) if not repo_files_dir: raise InconsistentApplicationState( f"Failed to resolve repository path from hgweb_config_manager for [{trs_tool_id}], inconsistent repository state or application configuration" diff --git a/lib/tool_shed/util/repository_util.py b/lib/tool_shed/util/repository_util.py index 9c9545c2e7f1..32829078f661 100644 --- a/lib/tool_shed/util/repository_util.py +++ b/lib/tool_shed/util/repository_util.py @@ -231,12 +231,7 @@ def create_repository( session = sa_session() with transaction(session): session.commit() - dir = os.path.join(app.config.file_path, *util.directory_hash_id(repository.id)) - # Define repo name inside hashed directory. - final_repository_path = os.path.join(dir, "repo_%d" % repository.id) - # Create final repository directory. - if not os.path.exists(final_repository_path): - os.makedirs(final_repository_path) + final_repository_path = repository.ensure_hg_repository_path(app.config.file_path) os.rename(repository_path, final_repository_path) app.hgweb_config_manager.add_entry(lhs, final_repository_path) # Update the repository registry. diff --git a/lib/tool_shed/webapp/model/__init__.py b/lib/tool_shed/webapp/model/__init__.py index a31ab4861f4a..6edd97d4511a 100644 --- a/lib/tool_shed/webapp/model/__init__.py +++ b/lib/tool_shed/webapp/model/__init__.py @@ -524,6 +524,19 @@ def repo_path(self, app=None): os.path.join(hgweb_config_manager.hgweb_repo_prefix, self.user.username, self.name) ) + def hg_repository_path(self, repositories_directory: str) -> str: + if self.id is None: + raise Exception("Attempting to call hg_repository_path before id has been set on repository object") + dir = os.path.join(repositories_directory, *util.directory_hash_id(self.id)) + final_repository_path = os.path.join(dir, "repo_%d" % self.id) + return final_repository_path + + def ensure_hg_repository_path(self, repositories_directory: str) -> str: + final_repository_path = self.hg_repository_path(repositories_directory) + if not os.path.exists(final_repository_path): + os.makedirs(final_repository_path) + return final_repository_path + def revision(self): repo = self.hg_repo tip_ctx = repo[repo.changelog.tip()] From fd2425fb37987af98efb049adc136808cfa27647 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Mon, 5 Aug 2024 12:19:07 -0400 Subject: [PATCH 3/3] Do not initialize a full repository registry in ToolShed 2.0. It seems like it is only used by repository_grids - which I think are unused by the new tool shed. I don't recall writing any client side grid framework code and my PR description did claim it was mako free. --- lib/tool_shed/managers/categories.py | 4 +- lib/tool_shed/repository_registry.py | 104 +++++++++++++++++-- lib/tool_shed/structured_app.py | 4 +- lib/tool_shed/webapp/app.py | 5 +- lib/tool_shed/webapp/buildapp.py | 3 +- lib/tool_shed/webapp/config.py | 1 + lib/tool_shed/webapp/fast_app.py | 2 +- lib/tool_shed/webapp/model/__init__.py | 18 ++++ test/unit/tool_shed/_util.py | 10 ++ test/unit/tool_shed/test_graphql.py | 16 +-- test/unit/tool_shed/test_repository_utils.py | 23 ++++ 11 files changed, 159 insertions(+), 31 deletions(-) diff --git a/lib/tool_shed/managers/categories.py b/lib/tool_shed/managers/categories.py index 1d418f1aa8fa..13b09297fdbf 100644 --- a/lib/tool_shed/managers/categories.py +++ b/lib/tool_shed/managers/categories.py @@ -59,9 +59,7 @@ def index(self, trans: ProvidesUserContext, deleted: bool) -> List[Dict[str, Any def to_dict(self, category: Category) -> Dict[str, Any]: category_dict = category.to_dict(view="collection", value_mapper=get_value_mapper(self.app)) - category_dict["repositories"] = self.app.repository_registry.viewable_repositories_and_suites_by_category.get( - category.name, 0 - ) + category_dict["repositories"] = category.active_repository_count() category_dict["url"] = web.url_for( controller="categories", action="show", id=self.app.security.encode_id(category.id) ) diff --git a/lib/tool_shed/repository_registry.py b/lib/tool_shed/repository_registry.py index 521ae1a06c6f..b49ce0b5695a 100644 --- a/lib/tool_shed/repository_registry.py +++ b/lib/tool_shed/repository_registry.py @@ -6,6 +6,7 @@ or_, select, ) +from typing_extensions import Protocol import tool_shed.repository_types.util as rt_util from tool_shed.util import ( @@ -18,11 +19,77 @@ Repository, RepositoryMetadata, ) +from .structured_app import ToolShedApp log = logging.getLogger(__name__) -class Registry: +class RegistryInterface(Protocol): + + # only used in legacy controllers - can drop when drop old tool shed webapp + def add_category_entry(self, category): ... + + def add_entry(self, repository): + # used when creating a repository + # Used in legacy add_repository_registry_entry API endpoint - already deemed not worth pulling over to 2.0 + # used in legacy undelete_repository admin controller + # used in legacy deprecate controller + ... + + def edit_category_entry(self, old_name, new_name): + # used in legacy admin controller + ... + + def is_valid(self, repository) -> bool: + # probably not used outside this class but also not hurting anything + ... + + def remove_category_entry(self, category): ... + + def remove_entry(self, repository): ... + + +# stop gap to implement the same general interface as repository_registry but do nothing, +# I don't think this stuff is needed - outside potentially old test cases? +class NullRepositoryRegistry(RegistryInterface): + + def __init__(self, app: ToolShedApp): + self.app = app + + # all of these are only used by repository_grids - which I think the tool shed 2.0 + # does not use at all - but they are part of the "public interface" consumed by + # the full registry. + self.certified_level_one_viewable_repositories_and_suites_by_category: dict = {} + self.certified_level_one_viewable_suites_by_category: dict = {} + self.viewable_repositories_and_suites_by_category: dict = {} + self.viewable_suites_by_category: dict = {} + self.viewable_valid_repositories_and_suites_by_category: dict = {} + self.viewable_valid_suites_by_category: dict = {} + + def add_category_entry(self, category): + # only used in legacy controllers - can drop when drop old tool shed webapp + pass + + def add_entry(self, repository): + # used when creating a repository, and maybe more? + pass + + def edit_category_entry(self, old_name, new_name): + pass + + def is_valid(self, repository) -> bool: + if repository and not repository.deleted and not repository.deprecated and repository.downloadable_revisions: + return True + return False + + def remove_category_entry(self, category): + pass + + def remove_entry(self, repository): + pass + + +class Registry(RegistryInterface): def __init__(self, app): log.debug("Loading the repository registry...") self.app = app @@ -32,17 +99,30 @@ def __init__(self, app): self.certified_level_one_suite_tuples = [] # These category dictionaries contain entries where the key is the category and the value is the integer count # of viewable repositories within that category. + + # only used internally to class and by repository_grids self.certified_level_one_viewable_repositories_and_suites_by_category = {} + # only used internally to class and by repository_grids self.certified_level_one_viewable_suites_by_category = {} - self.certified_level_two_repository_and_suite_tuples = [] - self.certified_level_two_suite_tuples = [] - self.certified_level_two_viewable_repositories_and_suites_by_category = {} - self.certified_level_two_viewable_suites_by_category = {} + + # not even used in legacy shed code... + # self.certified_level_two_repository_and_suite_tuples = [] + # self.certified_level_two_suite_tuples = [] + # self.certified_level_two_viewable_repositories_and_suites_by_category = {} + # self.certified_level_two_viewable_suites_by_category = {} + + # only used internally to class self.repository_and_suite_tuples = [] + # only used internally to class self.suite_tuples = [] + + # only used internally to class and by repository_grids self.viewable_repositories_and_suites_by_category = {} + # only used internally to class and by repository_grids self.viewable_suites_by_category = {} + # only used internally to class and by repository_grids self.viewable_valid_repositories_and_suites_by_category = {} + # only used internally to class and by repository_grids self.viewable_valid_suites_by_category = {} self.load() @@ -152,6 +232,7 @@ def edit_category_entry(self, old_name, new_name): self.certified_level_one_viewable_suites_by_category[new_name] = 0 def get_certified_level_one_clause_list(self): + # only used internally to class clause_list = [] for repository in get_repositories(self.sa_session): certified_level_one_tuple = self.get_certified_level_one_tuple(repository) @@ -169,6 +250,7 @@ def get_certified_level_one_tuple(self, repository): """ Return True if the latest installable changeset_revision of the received repository is level one certified. """ + # only used internally to class if repository is None: return (None, False) if repository.deleted or repository.deprecated: @@ -190,6 +272,7 @@ def get_certified_level_one_tuple(self, repository): return (None, False) def is_level_one_certified(self, repository_metadata): + # only used internally to class if repository_metadata: repository = repository_metadata.repository if repository: @@ -206,12 +289,13 @@ def is_level_one_certified(self, repository_metadata): return tuple in self.certified_level_one_repository_and_suite_tuples return False - def is_valid(self, repository): + def is_valid(self, repository) -> bool: if repository and not repository.deleted and not repository.deprecated and repository.downloadable_revisions: return True return False def load_certified_level_one_repository_and_suite_tuple(self, repository): + # only used internally to class # The received repository has been determined to be level one certified. name = str(repository.name) owner = str(repository.user.username) @@ -226,6 +310,7 @@ def load_certified_level_one_repository_and_suite_tuple(self, repository): self.certified_level_one_repository_and_suite_tuples.append(certified_level_one_tuple) def load_repository_and_suite_tuple(self, repository): + # only used internally to class name = str(repository.name) owner = str(repository.user.username) for repository_metadata in repository.metadata_revisions: @@ -238,6 +323,7 @@ def load_repository_and_suite_tuple(self, repository): self.suite_tuples.append(tuple) def load_repository_and_suite_tuples(self): + # only used internally to class # Load self.certified_level_one_repository_and_suite_tuples and self.certified_level_one_suite_tuples. clauses = self.get_certified_level_one_clause_list() for repository in get_certified_repositories_with_user(self.sa_session, clauses, model.User): @@ -247,11 +333,12 @@ def load_repository_and_suite_tuples(self): self.load_repository_and_suite_tuple(repository) def load_viewable_repositories_and_suites_by_category(self): + # only used internally to class # Clear all dictionaries just in case they were previously loaded. self.certified_level_one_viewable_repositories_and_suites_by_category = {} self.certified_level_one_viewable_suites_by_category = {} - self.certified_level_two_viewable_repositories_and_suites_by_category = {} - self.certified_level_two_viewable_suites_by_category = {} + # self.certified_level_two_viewable_repositories_and_suites_by_category = {} + # self.certified_level_two_viewable_suites_by_category = {} self.viewable_repositories_and_suites_by_category = {} self.viewable_suites_by_category = {} self.viewable_valid_repositories_and_suites_by_category = {} @@ -363,6 +450,7 @@ def remove_entry(self, repository): @property def sa_session(self): + # only used internally to class return self.app.model.session def unload_certified_level_one_repository_and_suite_tuple(self, repository): diff --git a/lib/tool_shed/structured_app.py b/lib/tool_shed/structured_app.py index 8fd828f9f5e1..8a38e008fbbe 100644 --- a/lib/tool_shed/structured_app.py +++ b/lib/tool_shed/structured_app.py @@ -4,7 +4,7 @@ if TYPE_CHECKING: from tool_shed.managers.model_cache import ModelCache - from tool_shed.repository_registry import Registry as RepositoryRegistry + from tool_shed.repository_registry import RegistryInterface from tool_shed.repository_types.registry import Registry as RepositoryTypesRegistry from tool_shed.util.hgweb_config import HgWebConfigManager from tool_shed.webapp.model import mapping @@ -14,7 +14,7 @@ class ToolShedApp(BasicSharedApp): repository_types_registry: "RepositoryTypesRegistry" model: "mapping.ToolShedModelMapping" - repository_registry: "RepositoryRegistry" + repository_registry: "RegistryInterface" hgweb_config_manager: "HgWebConfigManager" security_agent: "CommunityRBACAgent" model_cache: "ModelCache" diff --git a/lib/tool_shed/webapp/app.py b/lib/tool_shed/webapp/app.py index e046301497ad..6778d9028463 100644 --- a/lib/tool_shed/webapp/app.py +++ b/lib/tool_shed/webapp/app.py @@ -108,7 +108,10 @@ def __init__(self, **kwd) -> None: self.hgweb_config_manager.hgweb_config_dir = self.config.hgweb_config_dir self.hgweb_config_manager.hgweb_repo_prefix = self.config.hgweb_repo_prefix # Initialize the repository registry. - self.repository_registry = tool_shed.repository_registry.Registry(self) + if config.SHED_API_VERSION != "v2": + self.repository_registry = tool_shed.repository_registry.Registry(self) + else: + self.repository_registry = tool_shed.repository_registry.NullRepositoryRegistry(self) # Configure Sentry client if configured self.configure_sentry_client() # used for cachebusting -- refactor this into a *SINGLE* UniverseApplication base. diff --git a/lib/tool_shed/webapp/buildapp.py b/lib/tool_shed/webapp/buildapp.py index 6988874a6b52..16b47bc450b3 100644 --- a/lib/tool_shed/webapp/buildapp.py +++ b/lib/tool_shed/webapp/buildapp.py @@ -26,8 +26,7 @@ GalaxyWebTransaction, ) from galaxy.webapps.util import wrap_if_allowed - -SHED_API_VERSION = os.environ.get("TOOL_SHED_API_VERSION", "v1") +from .config import SHED_API_VERSION log = logging.getLogger(__name__) diff --git a/lib/tool_shed/webapp/config.py b/lib/tool_shed/webapp/config.py index 9c0e3d44964a..817d85776cc2 100644 --- a/lib/tool_shed/webapp/config.py +++ b/lib/tool_shed/webapp/config.py @@ -27,6 +27,7 @@ log = logging.getLogger(__name__) TOOLSHED_APP_NAME = "tool_shed" +SHED_API_VERSION = os.environ.get("TOOL_SHED_API_VERSION", "v1") class ToolShedAppConfiguration(BaseAppConfiguration, CommonConfigurationMixin): diff --git a/lib/tool_shed/webapp/fast_app.py b/lib/tool_shed/webapp/fast_app.py index ba094c74ec2c..56f3c0e2e711 100644 --- a/lib/tool_shed/webapp/fast_app.py +++ b/lib/tool_shed/webapp/fast_app.py @@ -166,7 +166,7 @@ def initialize_fast_app(gx_webapp, tool_shed_app): app = get_fastapi_instance() add_exception_handler(app) add_request_id_middleware(app) - from .buildapp import SHED_API_VERSION + from .config import SHED_API_VERSION def mount_static(directory: Path): name = directory.name diff --git a/lib/tool_shed/webapp/model/__init__.py b/lib/tool_shed/webapp/model/__init__.py index 6edd97d4511a..8cd5f31d8a04 100644 --- a/lib/tool_shed/webapp/model/__init__.py +++ b/lib/tool_shed/webapp/model/__init__.py @@ -28,6 +28,7 @@ not_, String, Table, + text, TEXT, true, UniqueConstraint, @@ -35,6 +36,7 @@ from sqlalchemy.orm import ( Mapped, mapped_column, + object_session, registry, relationship, ) @@ -618,6 +620,22 @@ class Category(Base, Dictifiable): dict_collection_visible_keys = ["id", "name", "description", "deleted"] dict_element_visible_keys = ["id", "name", "description", "deleted"] + def active_repository_count(self, session=None) -> int: + statement = """ +SELECT count(*) as count +FROM repository r +INNER JOIN repository_category_association rca on rca.repository_id = r.id +WHERE + rca.category_id = :category_id + AND r.private = false + AND r.deleted = false + and r.deprecated = false +""" + if session is None: + session = object_session(self) + params = {"category_id": self.id} + return session.execute(text(statement), params).scalar() + def __init__(self, deleted=False, **kwd): super().__init__(**kwd) self.deleted = deleted diff --git a/test/unit/tool_shed/_util.py b/test/unit/tool_shed/_util.py index e20bb17c3c13..03f27ce98874 100644 --- a/test/unit/tool_shed/_util.py +++ b/test/unit/tool_shed/_util.py @@ -33,6 +33,7 @@ Category, mapping, Repository, + RepositoryCategoryAssociation, User, ) from tool_shed_client.schema import CreateCategoryRequest @@ -195,3 +196,12 @@ def create_category(provides_repositories: ProvidesRepositoriesContext, create: request = CreateCategoryRequest(**create) return CategoryManager(provides_repositories.app).create(provides_repositories, request) + + +def attach_category(provides_repositories: ProvidesRepositoriesContext, repository: Repository, category: Category): + assoc = RepositoryCategoryAssociation( + repository=repository, + category=category, + ) + provides_repositories.sa_session.add(assoc) + provides_repositories.sa_session.flush() diff --git a/test/unit/tool_shed/test_graphql.py b/test/unit/tool_shed/test_graphql.py index 111dbf6fccd8..88318c75b6b4 100644 --- a/test/unit/tool_shed/test_graphql.py +++ b/test/unit/tool_shed/test_graphql.py @@ -12,12 +12,9 @@ ProvidesUserContext, ) from tool_shed.webapp.graphql.schema import schema -from tool_shed.webapp.model import ( - Category, - Repository, - RepositoryCategoryAssociation, -) +from tool_shed.webapp.model import Repository from ._util import ( + attach_category, create_category, repository_fixture, upload_directories_to_repository, @@ -110,15 +107,6 @@ def test_simple_repositories(provides_repositories: ProvidesRepositoriesContext, assert new_repository.name in repository_names -def attach_category(provides_repositories: ProvidesRepositoriesContext, repository: Repository, category: Category): - assoc = RepositoryCategoryAssociation( - repository=repository, - category=category, - ) - provides_repositories.sa_session.add(assoc) - provides_repositories.sa_session.flush() - - def test_relay_repos_by_category(provides_repositories: ProvidesRepositoriesContext, new_repository: Repository): name = new_repository.name category = create_category(provides_repositories, {"name": "test_graphql_relay_categories_1"}) diff --git a/test/unit/tool_shed/test_repository_utils.py b/test/unit/tool_shed/test_repository_utils.py index d23388b8a897..c648a13104ba 100644 --- a/test/unit/tool_shed/test_repository_utils.py +++ b/test/unit/tool_shed/test_repository_utils.py @@ -5,6 +5,8 @@ User, ) from ._util import ( + attach_category, + create_category, repository_fixture, TEST_DATA_FILES, TestToolShedApp, @@ -83,6 +85,27 @@ def test_upload_dry_run_ok(provides_repositories: ProvidesRepositoriesContext, n assert old_tip == new_tip +def test_category_count(provides_repositories: ProvidesRepositoriesContext, new_repository: Repository): + category = create_category(provides_repositories, {"name": "test_category_count_1"}) + attach_category(provides_repositories, new_repository, category) + + category = new_repository.categories[0].category + assert category.active_repository_count() == 1 + + tar_resource = TEST_DATA_FILES.joinpath("column_maker/column_maker.tar") + upload_ok, *_ = upload_tar( + provides_repositories, + new_repository.user.username, + new_repository, + tar_resource, + commit_message="Commit Message", + ) + assert upload_ok + + category = new_repository.categories[0].category + assert category.active_repository_count() == 1 + + def test_upload_dry_run_failed(provides_repositories: ProvidesRepositoriesContext, new_repository: Repository): tar_resource = TEST_DATA_FILES.joinpath("safetar_with_symlink.tar") upload_ok, message, _, _, _, _ = upload_tar(