From c88fa832a482ae676fd1ca26753e3dfccd76ba35 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 23 Aug 2024 16:37:04 +0200 Subject: [PATCH] Don't serialize view of item in delete/purge request This is a breaking change for the API, it is motivated by https://sentry.galaxyproject.org/share/issue/3970e49a695f4600bcaeae55021555a9/: ``` Message Unexpected error Stack Trace Newest FileNotFoundError: [Errno 2] No such file or directory: '' File "galaxy/datatypes/interval.py", line 1326, in get_estimated_display_viewport with open(dataset.get_file_name()) as fh: ``` This is a purge request, which does return the entire detailed response, including available display applications. We already check that the dataset isn't purged, but ther celery task often executes between the check and the deletion. I don't think it makes sense to return any data at all here, and there are other endpoints where we simply return 202 or 204, like purging user file source instanes. --- client/src/api/schema/schema.ts | 75 +++++++------------ lib/galaxy/schema/schema.py | 22 ------ .../webapps/galaxy/api/history_contents.py | 28 ++----- .../galaxy/services/history_contents.py | 20 ++--- 4 files changed, 39 insertions(+), 106 deletions(-) diff --git a/client/src/api/schema/schema.ts b/client/src/api/schema/schema.ts index 6f1ed33cf314..318cfffaaf66 100644 --- a/client/src/api/schema/schema.ts +++ b/client/src/api/schema/schema.ts @@ -7964,30 +7964,6 @@ export interface components { */ stop_job: boolean; }; - /** - * DeleteHistoryContentResult - * @description Contains minimum information about the deletion state of a history item. - * - * Can also contain any other properties of the item. - */ - DeleteHistoryContentResult: { - /** - * Deleted - * @description True if the item was successfully deleted. - */ - deleted: boolean; - /** - * ID - * @description The encoded ID of the history item. - * @example 0123456789ABCDEF - */ - id: string; - /** - * Purged - * @description True if the item was successfully removed from disk. - */ - purged?: boolean | null; - }; /** DeleteHistoryPayload */ DeleteHistoryPayload: { /** @@ -18063,10 +18039,6 @@ export interface operations { * @description Whether to stop the creating job if all outputs of the job have been deleted. */ stop_job?: boolean | null; - /** @description View to be passed to the serializer */ - view?: string | null; - /** @description Comma-separated list of keys to be passed to the serializer */ - keys?: string | null; }; header?: { /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ @@ -18084,13 +18056,13 @@ export interface operations { }; }; responses: { - /** @description Request has been executed. */ + /** @description Successful Response */ 200: { headers: { [name: string]: unknown; }; content: { - "application/json": components["schemas"]["DeleteHistoryContentResult"]; + "application/json": unknown; }; }; /** @description Request accepted, processing will finish later. */ @@ -18098,9 +18070,14 @@ export interface operations { headers: { [name: string]: unknown; }; - content: { - "application/json": components["schemas"]["DeleteHistoryContentResult"]; + content?: never; + }; + /** @description Request has been executed. */ + 204: { + headers: { + [name: string]: unknown; }; + content?: never; }; /** @description Request Error */ "4XX": { @@ -23518,10 +23495,6 @@ export interface operations { * @description Whether to stop the creating job if all outputs of the job have been deleted. */ stop_job?: boolean | null; - /** @description View to be passed to the serializer */ - view?: string | null; - /** @description Comma-separated list of keys to be passed to the serializer */ - keys?: string | null; }; header?: { /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ @@ -23541,13 +23514,13 @@ export interface operations { }; }; responses: { - /** @description Request has been executed. */ + /** @description Successful Response */ 200: { headers: { [name: string]: unknown; }; content: { - "application/json": components["schemas"]["DeleteHistoryContentResult"]; + "application/json": unknown; }; }; /** @description Request accepted, processing will finish later. */ @@ -23555,9 +23528,14 @@ export interface operations { headers: { [name: string]: unknown; }; - content: { - "application/json": components["schemas"]["DeleteHistoryContentResult"]; + content?: never; + }; + /** @description Request has been executed. */ + 204: { + headers: { + [name: string]: unknown; }; + content?: never; }; /** @description Request Error */ "4XX": { @@ -23929,10 +23907,6 @@ export interface operations { * @description Whether to stop the creating job if all outputs of the job have been deleted. */ stop_job?: boolean | null; - /** @description View to be passed to the serializer */ - view?: string | null; - /** @description Comma-separated list of keys to be passed to the serializer */ - keys?: string | null; }; header?: { /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ @@ -23954,13 +23928,13 @@ export interface operations { }; }; responses: { - /** @description Request has been executed. */ + /** @description Successful Response */ 200: { headers: { [name: string]: unknown; }; content: { - "application/json": components["schemas"]["DeleteHistoryContentResult"]; + "application/json": unknown; }; }; /** @description Request accepted, processing will finish later. */ @@ -23968,9 +23942,14 @@ export interface operations { headers: { [name: string]: unknown; }; - content: { - "application/json": components["schemas"]["DeleteHistoryContentResult"]; + content?: never; + }; + /** @description Request has been executed. */ + 204: { + headers: { + [name: string]: unknown; }; + content?: never; }; /** @description Request Error */ "4XX": { diff --git a/lib/galaxy/schema/schema.py b/lib/galaxy/schema/schema.py index e782cdb82092..4be57809541e 100644 --- a/lib/galaxy/schema/schema.py +++ b/lib/galaxy/schema/schema.py @@ -3392,28 +3392,6 @@ class DeleteHistoryContentPayload(Model): ) -class DeleteHistoryContentResult(Model): - """Contains minimum information about the deletion state of a history item. - - Can also contain any other properties of the item.""" - - id: DecodedDatabaseIdField = Field( - ..., - title="ID", - description="The encoded ID of the history item.", - ) - deleted: bool = Field( - ..., - title="Deleted", - description="True if the item was successfully deleted.", - ) - purged: Optional[bool] = Field( - default=None, - title="Purged", - description="True if the item was successfully removed from disk.", - ) - - class HistoryContentsArchiveDryRunResult(RootModel): """ Contains a collection of filepath/filename entries that represent diff --git a/lib/galaxy/webapps/galaxy/api/history_contents.py b/lib/galaxy/webapps/galaxy/api/history_contents.py index 95641f244c85..44fdee5b9722 100644 --- a/lib/galaxy/webapps/galaxy/api/history_contents.py +++ b/lib/galaxy/webapps/galaxy/api/history_contents.py @@ -44,7 +44,6 @@ DatasetAssociationRoles, DatasetSourceType, DeleteHistoryContentPayload, - DeleteHistoryContentResult, HistoryContentBulkOperationPayload, HistoryContentBulkOperationResult, HistoryContentsArchiveDryRunResult, @@ -147,13 +146,11 @@ def ContentTypeQueryParam(default: Optional[HistoryContentType]): CONTENT_DELETE_RESPONSES = { - 200: { - "description": "Request has been executed.", - "model": DeleteHistoryContentResult, - }, 202: { "description": "Request accepted, processing will finish later.", - "model": DeleteHistoryContentResult, + }, + 204: { + "description": "Request has been executed.", }, } @@ -874,7 +871,6 @@ def delete_typed( id: HistoryItemIDPathParam, trans: ProvidesHistoryContext = DependsOnTrans, type: HistoryContentType = ContentTypePathParam, - serialization_params: SerializationParams = Depends(query_serialization_params), purge: Optional[bool] = PurgeQueryParam, recursive: Optional[bool] = RecursiveQueryParam, stop_job: Optional[bool] = StopJobQueryParam, @@ -890,7 +886,6 @@ def delete_typed( trans, id, type, - serialization_params, purge, recursive, stop_job, @@ -910,7 +905,6 @@ def delete_legacy( id: HistoryItemIDPathParam, trans: ProvidesHistoryContext = DependsOnTrans, type: HistoryContentType = ContentTypeQueryParam(default=HistoryContentType.dataset), - serialization_params: SerializationParams = Depends(query_serialization_params), purge: Optional[bool] = PurgeQueryParam, recursive: Optional[bool] = RecursiveQueryParam, stop_job: Optional[bool] = StopJobQueryParam, @@ -926,7 +920,6 @@ def delete_legacy( trans, id, type, - serialization_params, purge, recursive, stop_job, @@ -944,7 +937,6 @@ def delete_dataset( response: Response, dataset_id: HistoryItemIDPathParam, trans: ProvidesHistoryContext = DependsOnTrans, - serialization_params: SerializationParams = Depends(query_serialization_params), purge: Optional[bool] = PurgeQueryParam, recursive: Optional[bool] = RecursiveQueryParam, stop_job: Optional[bool] = StopJobQueryParam, @@ -960,7 +952,6 @@ def delete_dataset( trans, dataset_id, HistoryContentType.dataset, - serialization_params, purge, recursive, stop_job, @@ -973,29 +964,24 @@ def _delete( trans: ProvidesHistoryContext, id: DecodedDatabaseIdField, type: HistoryContentType, - serialization_params: SerializationParams, purge: Optional[bool], recursive: Optional[bool], stop_job: Optional[bool], payload: DeleteHistoryContentPayload, ): - # TODO: should we just use the default payload and deprecate the query params? if payload is None: payload = DeleteHistoryContentPayload() payload.purge = payload.purge or purge is True payload.recursive = payload.recursive or recursive is True payload.stop_job = payload.stop_job or stop_job is True - rval = self.service.delete( + if self.service.delete( trans, id=id, - serialization_params=serialization_params, contents_type=type, payload=payload, - ) - async_result = rval.pop("async_result", None) - if async_result: - response.status_code = status.HTTP_202_ACCEPTED - return rval + ): + return Response(status_code=status.HTTP_202_ACCEPTED) + return Response(status_code=status.HTTP_204_NO_CONTENT) @router.get( "/api/histories/{history_id}/contents/archive/{filename}.{format}", diff --git a/lib/galaxy/webapps/galaxy/services/history_contents.py b/lib/galaxy/webapps/galaxy/services/history_contents.py index 54ef988f990c..366f6bb65bd7 100644 --- a/lib/galaxy/webapps/galaxy/services/history_contents.py +++ b/lib/galaxy/webapps/galaxy/services/history_contents.py @@ -738,7 +738,6 @@ def delete( self, trans, id: DecodedDatabaseIdField, - serialization_params: SerializationParams, contents_type: HistoryContentType, payload: DeleteHistoryContentPayload, ): @@ -746,12 +745,11 @@ def delete( Delete the history content with the given ``id`` and specified type (defaults to dataset) """ if contents_type == HistoryContentType.dataset: - return self.__delete_dataset(trans, id, payload.purge, payload.stop_job, serialization_params) + return self.__delete_dataset(trans, id, payload.purge, payload.stop_job) elif contents_type == HistoryContentType.dataset_collection: - async_result = self.dataset_collection_manager.delete( + return self.dataset_collection_manager.delete( trans, "history", id, recursive=payload.recursive, purge=payload.purge ) - return {"id": self.encode_id(id), "deleted": True, "async_result": async_result is not None} else: raise exceptions.UnknownContentsType(f"Unknown contents type: {contents_type}") @@ -865,24 +863,16 @@ def build_archive_files_and_paths(content, *parents): archive.write(file_path, archive_path) return archive - def __delete_dataset( - self, trans, id: DecodedDatabaseIdField, purge: bool, stop_job: bool, serialization_params: SerializationParams - ): + def __delete_dataset(self, trans, id: DecodedDatabaseIdField, purge: bool, stop_job: bool): hda = self.hda_manager.get_owned(id, trans.user, current_history=trans.history) self.history_manager.error_unless_mutable(hda.history) self.hda_manager.error_if_uploading(hda) - async_result = None if purge: - async_result = self.hda_manager.purge(hda, user=trans.user) + return self.hda_manager.purge(hda, user=trans.user) else: self.hda_manager.delete(hda, stop_job=stop_job) - serialization_params.default_view = "detailed" - rval = self.hda_serializer.serialize_to_view( - hda, user=trans.user, trans=trans, encode_id=False, **serialization_params.model_dump() - ) - rval["async_result"] = async_result is not None - return rval + return None def __update_dataset_collection(self, trans, id: DecodedDatabaseIdField, payload: Dict[str, Any]): return self.dataset_collection_manager.update(trans, "history", id, payload)