Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improvements to tool test error handling. #19009

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

Improvements to tool test error handling. #19009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/galaxy/tool_util/verify/interactor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,12 @@ def publish_history(self, history_id: str) -> None:
def test_data_path(self, tool_id, filename, tool_version=None):
version_fragment = f"&tool_version={tool_version}" if tool_version else ""
response = self._get(f"tools/{tool_id}/test_data_path?filename={filename}{version_fragment}", admin=True)
result = response.json()
try:
result = response.json()
except Exception:
raise Exception(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you dump the status code and response.text ? I would prefer this over a guess

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only time we know this will happen is Galaxies without this patch - so there will be a huge dump of HTML. I think we keep the guess but also I'll dump that huge page I guess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, and even our expected error messages are json dumps. Maybe move the response.json to below the statsu code check on the next line ? Then I'm fine with not dumping in the text.

f"Failed to parse test_data_path from Galaxy. An admin key is required for this feature and it is probably not set or not a valid admin key. Status Code: {response.status_code}. Response: {response.text}."
)
if response.status_code in [200, 404]:
return result
raise Exception(result["err_msg"])
Expand Down
18 changes: 9 additions & 9 deletions lib/galaxy/webapps/galaxy/api/tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,8 @@ def build(self, trans: GalaxyWebTransaction, id, **kwd):
tool = self.service._get_tool(trans, id, tool_version=tool_version, user=trans.user)
return tool.to_json(trans, kwd.get("inputs", kwd), history=history)

@web.require_admin
@expose_api
@web.require_admin
def test_data_path(self, trans: GalaxyWebTransaction, id, **kwd):
"""
GET /api/tools/{tool_id}/test_data_path?tool_version={tool_version}
Expand Down Expand Up @@ -347,8 +347,8 @@ def test_data(self, trans: GalaxyWebTransaction, id, **kwd) -> List[ToolTestDesc
test_defs.extend([t.to_dict() for t in tool.tests])
return test_defs

@web.require_admin
@expose_api
@web.require_admin
def reload(self, trans: GalaxyWebTransaction, id, **kwd):
"""
GET /api/tools/{tool_id}/reload
Expand All @@ -360,8 +360,8 @@ def reload(self, trans: GalaxyWebTransaction, id, **kwd):
raise exceptions.MessageException(message)
return {"message": message}

@web.require_admin
@expose_api
@web.require_admin
def all_requirements(self, trans: GalaxyWebTransaction, **kwds):
"""
GET /api/tools/all_requirements
Expand All @@ -370,8 +370,8 @@ def all_requirements(self, trans: GalaxyWebTransaction, **kwds):

return trans.app.toolbox.all_requirements

@web.require_admin
@expose_api
@web.require_admin
def requirements(self, trans: GalaxyWebTransaction, id, **kwds):
"""
GET /api/tools/{tool_id}/requirements
Expand All @@ -381,8 +381,8 @@ def requirements(self, trans: GalaxyWebTransaction, id, **kwds):
tool = self.service._get_tool(trans, id, user=trans.user)
return tool.tool_requirements_status

@web.require_admin
@expose_api
@web.require_admin
def install_dependencies(self, trans: GalaxyWebTransaction, id, **kwds):
"""
POST /api/tools/{tool_id}/dependencies
Expand All @@ -409,8 +409,8 @@ def install_dependencies(self, trans: GalaxyWebTransaction, id, **kwds):
# _view.install_dependencies should return a dict with stdout, stderr and success status
return tool.tool_requirements_status

@web.require_admin
@expose_api
@web.require_admin
def uninstall_dependencies(self, trans: GalaxyWebTransaction, id, **kwds):
"""
DELETE /api/tools/{tool_id}/dependencies
Expand All @@ -431,8 +431,8 @@ def uninstall_dependencies(self, trans: GalaxyWebTransaction, id, **kwds):
# TODO: rework resolver install system to log and report what has been done.
return tool.tool_requirements_status

@web.require_admin
@expose_api
@web.require_admin
def build_dependency_cache(self, trans: GalaxyWebTransaction, id, **kwds):
"""
POST /api/tools/{tool_id}/build_dependency_cache
Expand All @@ -446,8 +446,8 @@ def build_dependency_cache(self, trans: GalaxyWebTransaction, id, **kwds):
# TODO: Should also have a more meaningful return.
return tool.tool_requirements_status

@web.require_admin
@expose_api
@web.require_admin
def diagnostics(self, trans: GalaxyWebTransaction, id, **kwd):
"""
GET /api/tools/{tool_id}/diagnostics
Expand Down Expand Up @@ -563,8 +563,8 @@ def raw_tool_source(self, trans: GalaxyWebTransaction, id, **kwds):
trans.response.headers["language"] = tool.tool_source.language
return tool.tool_source.to_string()

@web.require_admin
@expose_api
@web.require_admin
def error_stack(self, trans: GalaxyWebTransaction, **kwd):
"""
GET /api/tools/error_stack
Expand Down
7 changes: 6 additions & 1 deletion lib/galaxy_test/api/test_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@
put,
)

from galaxy.exceptions import error_codes
from galaxy.tool_util.verify.interactor import ValidToolTestDict
from galaxy.util import galaxy_root_path
from galaxy.util.unittest_utils import skip_if_github_down
from galaxy_test.base import rules_test_data
from galaxy_test.base.api_asserts import (
assert_error_code_is,
assert_has_keys,
assert_status_code_is,
)
Expand Down Expand Up @@ -388,7 +390,10 @@ def test_test_data_filepath_security(self):
@skip_without_tool("composite_output")
def test_test_data_admin_security(self):
test_data_response = self._get("tools/composite_output/test_data_path?filename=../CONTRIBUTORS.md")
assert test_data_response.status_code == 403, test_data_response.text
assert_status_code_is(test_data_response, 403)
error_json = test_data_response.json()
assert_has_keys(error_json, "err_msg", "err_code")
assert_error_code_is(error_json, error_codes.error_codes_by_name["ADMIN_REQUIRED"].code)

@skip_without_tool("dbkey_filter_multi_input")
def test_data_table_requirement_annotated(self):
Expand Down
5 changes: 4 additions & 1 deletion lib/galaxy_test/base/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ def _setup_user_get_key(self, email, password=None):
return user, self._post(f"users/{user['id']}/api_key", admin=True).json()

@contextmanager
def _different_user(self, email=OTHER_USER, anon=False):
def _different_user(self, email=OTHER_USER, anon=False, invalid_admin_key=False):
"""Use in test cases to switch get/post operations to act as new user

..code-block:: python
Expand All @@ -157,6 +157,7 @@ def _different_user(self, email=OTHER_USER, anon=False):

"""
original_api_key = self.user_api_key
original_admin_api_key = self.master_api_key
original_interactor_key = self.galaxy_interactor.api_key
original_cookies = self.galaxy_interactor.cookies
if anon:
Expand All @@ -168,10 +169,12 @@ def _different_user(self, email=OTHER_USER, anon=False):
try:
self.user_api_key = new_key
self.galaxy_interactor.api_key = new_key
self.galaxy_interactor.master_api_key = None if not invalid_admin_key else new_key
yield
finally:
self.user_api_key = original_api_key
self.galaxy_interactor.api_key = original_interactor_key
self.master_api_key = original_admin_api_key
self.galaxy_interactor.cookies = original_cookies

def _get(self, *args, **kwds):
Expand Down
12 changes: 12 additions & 0 deletions test/integration/test_galaxy_interactor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,18 @@ def test_local_test_data_download(self):
b"chr1\t147962192\t147962580"
)

def test_data_path_error_message(self):
with self._different_user(invalid_admin_key=True): # other user is not admin, attempt to use their key anyway
exc = None
try:
data_path = self.galaxy_interactor.test_data_path("cat1", "1.bed")
print(data_path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print(data_path)

assert not data_path
except Exception as e:
exc = e
assert exc is not None
assert "You must be an administrator" in str(exc)

def test_run_test_select_version(self):
self._run_tool_test(tool_id="multiple_versions", tool_version="0.1")

Expand Down
Loading