From 981c0f83f9e03d1ead487c34865eb1cebb64aeac Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 10 Feb 2023 11:45:42 +0100 Subject: [PATCH 1/2] Drop SentryWSGIMiddleware It appears that we can't combine the WSGI middleware with the ASGI middleware in the same app. We're currently not sending events that occur in the WSGI portion of the app. With this change they're logged again, though we do lose the WSGI request context (which contains, browser, route, etc.) It's usually pretty clear from the traceback what went wrong though, so I think this is much better than what's happening now. --- lib/galaxy/webapps/base/api.py | 5 +++++ lib/galaxy/webapps/galaxy/buildapp.py | 6 ------ lib/galaxy/webapps/galaxy/fast_app.py | 8 +++----- lib/tool_shed/webapp/buildapp.py | 7 ------- lib/tool_shed/webapp/fast_app.py | 3 +++ 5 files changed, 11 insertions(+), 18 deletions(-) diff --git a/lib/galaxy/webapps/base/api.py b/lib/galaxy/webapps/base/api.py index 94be727ca1a..06aa0112860 100644 --- a/lib/galaxy/webapps/base/api.py +++ b/lib/galaxy/webapps/base/api.py @@ -171,6 +171,11 @@ def add_empty_response_middleware(app: FastAPI) -> None: app.add_middleware(SuppressNoResponseReturnedMiddleware) +def add_sentry_middleware(app: FastAPI) -> None: + from sentry_sdk.integrations.asgi import SentryAsgiMiddleware + app.add_middleware(SentryAsgiMiddleware) + + def add_exception_handler(app: FastAPI) -> None: @app.exception_handler(RequestValidationError) async def validate_exception_middleware(request: Request, exc: RequestValidationError) -> Response: diff --git a/lib/galaxy/webapps/galaxy/buildapp.py b/lib/galaxy/webapps/galaxy/buildapp.py index 90ab6cd8174..e882a14d6f0 100644 --- a/lib/galaxy/webapps/galaxy/buildapp.py +++ b/lib/galaxy/webapps/galaxy/buildapp.py @@ -1366,13 +1366,7 @@ def wrap_in_middleware(app, global_conf, application_stack, **local_conf): from paste import recursive app = wrap_if_allowed(app, stack, recursive.RecursiveMiddleware, args=(conf,)) - # If sentry logging is enabled, log here before propogating up to - # the error middleware - sentry_dsn = conf.get("sentry_dsn", None) - if sentry_dsn: - from sentry_sdk.integrations.wsgi import SentryWsgiMiddleware - app = wrap_if_allowed(app, stack, SentryWsgiMiddleware) # Various debug middleware that can only be turned on if the debug # flag is set, either because they are insecure or greatly hurt # performance diff --git a/lib/galaxy/webapps/galaxy/fast_app.py b/lib/galaxy/webapps/galaxy/fast_app.py index 6c30dc22842..882363d2fac 100644 --- a/lib/galaxy/webapps/galaxy/fast_app.py +++ b/lib/galaxy/webapps/galaxy/fast_app.py @@ -11,6 +11,7 @@ from galaxy.webapps.base.api import ( add_empty_response_middleware, add_exception_handler, add_request_id_middleware, + add_sentry_middleware, GalaxyFileResponse, include_all_package_routers, ) @@ -103,11 +104,6 @@ def add_galaxy_middleware(app: FastAPI, gx_app): GalaxyFileResponse.nginx_x_accel_redirect_base = gx_app.config.nginx_x_accel_redirect_base GalaxyFileResponse.apache_xsendfile = gx_app.config.apache_xsendfile - if gx_app.config.sentry_dsn: - from sentry_sdk.integrations.asgi import SentryAsgiMiddleware - - app.add_middleware(SentryAsgiMiddleware) - if gx_app.config.get("allowed_origin_hostnames", None): app.add_middleware( GalaxyCORSMiddleware, @@ -158,6 +154,8 @@ def initialize_fast_app(gx_wsgi_webapp, gx_app): gx_app.haltables.append(("WSGI Middleware threadpool", wsgi_handler.executor.shutdown)) app.mount("/", wsgi_handler) add_empty_response_middleware(app) + if gx_app.config.sentry_dsn: + add_sentry_middleware(app) if gx_app.config.galaxy_url_prefix != "/": parent_app = FastAPI() parent_app.mount(gx_app.config.galaxy_url_prefix, app=app) diff --git a/lib/tool_shed/webapp/buildapp.py b/lib/tool_shed/webapp/buildapp.py index 95b75431371..718c42fd7ec 100644 --- a/lib/tool_shed/webapp/buildapp.py +++ b/lib/tool_shed/webapp/buildapp.py @@ -263,14 +263,7 @@ def wrap_in_middleware(app, global_conf, application_stack, **local_conf): from paste.translogger import TransLogger app = wrap_if_allowed(app, stack, TransLogger) - # If sentry logging is enabled, log here before propogating up to - # the error middleware - # TODO sentry config is duplicated between tool_shed/galaxy, refactor this. - sentry_dsn = conf.get("sentry_dsn", None) - if sentry_dsn: - from sentry_sdk.integrations.wsgi import SentryWsgiMiddleware - app = wrap_if_allowed(app, stack, SentryWsgiMiddleware) # X-Forwarded-Host handling from galaxy.web.framework.middleware.xforwardedhost import XForwardedHostMiddleware diff --git a/lib/tool_shed/webapp/fast_app.py b/lib/tool_shed/webapp/fast_app.py index a3e4c2f254c..4123d8ee6cd 100644 --- a/lib/tool_shed/webapp/fast_app.py +++ b/lib/tool_shed/webapp/fast_app.py @@ -5,6 +5,7 @@ from galaxy.webapps.base.api import ( add_empty_response_middleware, add_exception_handler, add_request_id_middleware, + add_sentry_middleware, include_all_package_routers, ) @@ -22,6 +23,8 @@ def initialize_fast_app(gx_webapp, tool_shed_app): tool_shed_app.haltables.append(("WSGI Middleware threadpool", wsgi_handler.executor.shutdown)) app.mount("/", wsgi_handler) add_empty_response_middleware(app) + if tool_shed_app.config.sentry_dsn: + add_sentry_middleware(app=app) return app From 2699f8c741d75d4ab4cfd7ef5072153467c076ae Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 15 Feb 2023 10:59:08 +0100 Subject: [PATCH 2/2] Fix unbound local error in sort collection tool Fixes ``` UnboundLocalError: local variable 'sorted_elements' referenced before assignment File "galaxy/tools/__init__.py", line 1909, in handle_single_execution rval = self.execute( File "galaxy/tools/__init__.py", line 2005, in execute return self.tool_action.execute( File "galaxy/tools/actions/model_operations.py", line 83, in execute self._produce_outputs( File "galaxy/tools/actions/model_operations.py", line 108, in _produce_outputs tool.produce_outputs( File "galaxy/tools/__init__.py", line 3490, in produce_outputs for dce in sorted_elements: ``` in https://sentry.galaxyproject.org/share/issue/264aea1fd3994e5ba67ab80fcbd2c577/ --- lib/galaxy/tools/__init__.py | 4 +-- lib/galaxy_test/api/test_workflows.py | 37 +++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/tools/__init__.py b/lib/galaxy/tools/__init__.py index 5f90690a9fb..2f7c519e2e9 100644 --- a/lib/galaxy/tools/__init__.py +++ b/lib/galaxy/tools/__init__.py @@ -3408,7 +3408,7 @@ class SortTool(DatabaseOperationTool): sorttype = incoming["sort_type"]["sort_type"] new_elements = {} elements = hdca.collection.elements - presort_elements = [] + presort_elements = None if sorttype == "alpha": presort_elements = [(dce.element_identifier, dce) for dce in elements] elif sorttype == "numeric": @@ -3433,7 +3433,7 @@ class SortTool(DatabaseOperationTool): else: raise Exception(f"Unknown sort_type '{sorttype}'") - if presort_elements: + if presort_elements is not None: sorted_elements = [x[1] for x in sorted(presort_elements, key=lambda x: x[0])] for dce in sorted_elements: diff --git a/lib/galaxy_test/api/test_workflows.py b/lib/galaxy_test/api/test_workflows.py index 8573cadb24e..184470ac9e2 100644 --- a/lib/galaxy_test/api/test_workflows.py +++ b/lib/galaxy_test/api/test_workflows.py @@ -5155,6 +5155,43 @@ input: put_response = self._update_workflow(workflow_id, workflow_object) assert put_response.status_code == 200 + def test_empty_collection_sort(self): + self._run_workflow( + """class: GalaxyWorkflow +inputs: + input: collection + filter_file: data +steps: + filter_collection: + tool_id: __FILTER_FROM_FILE__ + in: + input: input + how|filter_source: filter_file + sort_collection_1: + tool_id: __SORTLIST__ + in: + input: filter_collection/output_filtered + sort_collection_2: + tool_id: __SORTLIST__ + in: + input: filter_collection/output_discarded + merge_collection: + tool_id: __MERGE_COLLECTION__ + in: + inputs_0|input: sort_collection_1/output + inputs_1|input: sort_collection_2/output +test_data: + input: + collection_type: list + elements: + - identifier: i1 + content: "0" + filter_file: i1 +""", + wait=True, + assert_ok=True, + ) + @skip_without_tool("random_lines1") def test_run_replace_params_over_default_delayed(self): with self.dataset_populator.test_history() as history_id: