From 84b72d77c1d748adf4c6f803225b1a81e583bd17 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 18 Jul 2024 16:05:25 +0200 Subject: [PATCH 1/2] Improve datasets permissions API typing --- lib/galaxy/schema/schema.py | 61 +++++++++++++++---- lib/galaxy/webapps/galaxy/api/common.py | 46 ++++++++++---- lib/galaxy/webapps/galaxy/api/datasets.py | 14 ++--- .../webapps/galaxy/api/history_contents.py | 14 ++--- 4 files changed, 91 insertions(+), 44 deletions(-) diff --git a/lib/galaxy/schema/schema.py b/lib/galaxy/schema/schema.py index 8b75e50f5a69..9cdf22507164 100644 --- a/lib/galaxy/schema/schema.py +++ b/lib/galaxy/schema/schema.py @@ -3229,30 +3229,65 @@ class DatasetAssociationRoles(Model): ) -class UpdateDatasetPermissionsPayload(Model): +class UpdateDatasetPermissionsPayloadBase(Model): action: Optional[DatasetPermissionAction] = Field( DatasetPermissionAction.set_permissions, title="Action", description="Indicates what action should be performed on the dataset.", ) - access_ids: Optional[RoleIdList] = Field( - [], - alias="access_ids[]", # Added for backward compatibility but it looks really ugly... + + +AccessIdsField = Annotated[ + Optional[RoleIdList], + Field( + default=None, title="Access IDs", description="A list of role encoded IDs defining roles that should have access permission on the dataset.", - ) - manage_ids: Optional[RoleIdList] = Field( - [], - alias="manage_ids[]", + ), +] + +ManageIdsField = Annotated[ + Optional[RoleIdList], + Field( + default=None, title="Manage IDs", description="A list of role encoded IDs defining roles that should have manage permission on the dataset.", - ) - modify_ids: Optional[RoleIdList] = Field( - [], - alias="modify_ids[]", + ), +] + +ModifyIdsField = Annotated[ + Optional[RoleIdList], + Field( + default=None, title="Modify IDs", description="A list of role encoded IDs defining roles that should have modify permission on the dataset.", - ) + ), +] + + +class UpdateDatasetPermissionsPayload(UpdateDatasetPermissionsPayloadBase): + access_ids: Annotated[Optional[RoleIdList], Field(default=None, alias="access_ids[]")] = None + manage_ids: Annotated[Optional[RoleIdList], Field(default=None, alias="manage_ids[]")] = None + modify_ids: Annotated[Optional[RoleIdList], Field(default=None, alias="modify_ids[]")] = None + + +class UpdateDatasetPermissionsPayloadAliasB(UpdateDatasetPermissionsPayloadBase): + access: AccessIdsField = None + manage: ManageIdsField = None + modify: ModifyIdsField = None + + +class UpdateDatasetPermissionsPayloadAliasC(UpdateDatasetPermissionsPayloadBase): + access_ids: AccessIdsField = None + manage_ids: ManageIdsField = None + modify_ids: ModifyIdsField = None + + +UpdateDatasetPermissionsPayloadAliases = Union[ + UpdateDatasetPermissionsPayload, + UpdateDatasetPermissionsPayloadAliasB, + UpdateDatasetPermissionsPayloadAliasC, +] @partial_model() diff --git a/lib/galaxy/webapps/galaxy/api/common.py b/lib/galaxy/webapps/galaxy/api/common.py index 4ba9cedbe53e..5ade80943bf8 100644 --- a/lib/galaxy/webapps/galaxy/api/common.py +++ b/lib/galaxy/webapps/galaxy/api/common.py @@ -2,13 +2,13 @@ from typing import ( Any, - Dict, List, Optional, Set, ) from fastapi import ( + Body, Path, Query, Request, @@ -21,7 +21,10 @@ ValueFilterQueryParams, ) from galaxy.schema.fields import DecodedDatabaseIdField -from galaxy.schema.schema import UpdateDatasetPermissionsPayload +from galaxy.schema.schema import ( + UpdateDatasetPermissionsPayload, + UpdateDatasetPermissionsPayloadAliases, +) from galaxy.util import listify HistoryIDPathParam = Annotated[ @@ -66,6 +69,20 @@ Path(..., title="Role ID", description="The ID of the role."), ] +UpdateDatasetPermissionsBody = Annotated[ + UpdateDatasetPermissionsPayloadAliases, + Body( + ..., + examples=[ + UpdateDatasetPermissionsPayload( + action="set_permissions", + access_ids=[], + manage_ids=[], + modify_ids=[], + ).model_dump() + ], + ), +] LibraryIdPathParam = Annotated[ DecodedDatabaseIdField, @@ -194,16 +211,23 @@ def get_filter_query_params( ) -def get_update_permission_payload(payload: Dict[str, Any]) -> UpdateDatasetPermissionsPayload: - """Converts the generic payload dictionary into a UpdateDatasetPermissionsPayload model with custom parsing. - This is an attempt on supporting multiple aliases for the permissions params.""" - # There are several allowed names for the same role list parameter, i.e.: `access`, `access_ids`, `access_ids[]` - # The `access_ids[]` name is not pydantic friendly, so this will be modelled as an alias but we can only set one alias +def normalize_permission_payload( + payload_aliases: UpdateDatasetPermissionsPayloadAliases, +) -> UpdateDatasetPermissionsPayload: + """Normalize the payload by choosing the first non-None value for each field. + + This is an attempt on supporting multiple aliases for the permissions params. + There are several allowed names for the same role list parameter, i.e.: `access`, `access_ids`, `access_ids[]` + """ # TODO: Maybe we should choose only one way/naming and deprecate the others? - payload["access_ids"] = payload.get("access_ids[]") or payload.get("access") - payload["manage_ids"] = payload.get("manage_ids[]") or payload.get("manage") - payload["modify_ids"] = payload.get("modify_ids[]") or payload.get("modify") - update_payload = UpdateDatasetPermissionsPayload(**payload) + payload = payload_aliases.model_dump() + normalized_payload = { + "action": payload.get("action"), + "access_ids": payload.get("access_ids") or payload.get("access_ids[]") or payload.get("access"), + "manage_ids": payload.get("manage_ids") or payload.get("manage_ids[]") or payload.get("manage"), + "modify_ids": payload.get("modify_ids") or payload.get("modify_ids[]") or payload.get("modify"), + } + update_payload = UpdateDatasetPermissionsPayload.model_construct(**normalized_payload) return update_payload diff --git a/lib/galaxy/webapps/galaxy/api/datasets.py b/lib/galaxy/webapps/galaxy/api/datasets.py index 53006b4ee8c1..fdc790d9fd96 100644 --- a/lib/galaxy/webapps/galaxy/api/datasets.py +++ b/lib/galaxy/webapps/galaxy/api/datasets.py @@ -9,9 +9,7 @@ StringIO, ) from typing import ( - Any, cast, - Dict, List, Optional, ) @@ -40,7 +38,6 @@ AsyncTaskResultSummary, DatasetAssociationRoles, DatasetSourceType, - UpdateDatasetPermissionsPayload, ) from galaxy.util.zipstream import ZipstreamWrapper from galaxy.webapps.base.api import GalaxyFileResponse @@ -52,10 +49,11 @@ from galaxy.webapps.galaxy.api.common import ( get_filter_query_params, get_query_parameters_from_request_excluding, - get_update_permission_payload, HistoryDatasetIDPathParam, HistoryIDPathParam, + normalize_permission_payload, query_serialization_params, + UpdateDatasetPermissionsBody, ) from galaxy.webapps.galaxy.services.datasets import ( ComputeDatasetHashPayload, @@ -235,15 +233,11 @@ def converted( def update_permissions( self, dataset_id: HistoryDatasetIDPathParam, + payload: UpdateDatasetPermissionsBody, trans=DependsOnTrans, - # Using a generic Dict here as an attempt on supporting multiple aliases for the permissions params. - payload: Dict[str, Any] = Body( - default=..., - examples=[UpdateDatasetPermissionsPayload()], - ), ) -> DatasetAssociationRoles: """Set permissions of the given history dataset to the given role ids.""" - update_payload = get_update_permission_payload(payload) + update_payload = normalize_permission_payload(payload) return self.service.update_permissions(trans, dataset_id, update_payload) @router.get( diff --git a/lib/galaxy/webapps/galaxy/api/history_contents.py b/lib/galaxy/webapps/galaxy/api/history_contents.py index 052ea3a2e465..39d7d64c8e7d 100644 --- a/lib/galaxy/webapps/galaxy/api/history_contents.py +++ b/lib/galaxy/webapps/galaxy/api/history_contents.py @@ -4,8 +4,6 @@ import logging from typing import ( - Any, - Dict, List, Optional, Union, @@ -51,7 +49,6 @@ MaterializeDatasetInstanceAPIRequest, MaterializeDatasetInstanceRequest, StoreExportPayload, - UpdateDatasetPermissionsPayload, UpdateHistoryContentsBatchPayload, UpdateHistoryContentsPayload, WriteStoreToPayload, @@ -64,12 +61,13 @@ ) from galaxy.webapps.galaxy.api.common import ( get_filter_query_params, - get_update_permission_payload, get_value_filter_query_params, HistoryHDCAIDPathParam, HistoryIDPathParam, HistoryItemIDPathParam, + normalize_permission_payload, query_serialization_params, + UpdateDatasetPermissionsBody, ) from galaxy.webapps.galaxy.services.history_contents import ( CreateHistoryContentFromStore, @@ -727,15 +725,11 @@ def update_permissions( self, history_id: HistoryIDPathParam, dataset_id: HistoryItemIDPathParam, + payload: UpdateDatasetPermissionsBody, trans: ProvidesHistoryContext = DependsOnTrans, - # Using a generic Dict here as an attempt on supporting multiple aliases for the permissions params. - payload: Dict[str, Any] = Body( - default=..., - examples=[UpdateDatasetPermissionsPayload().model_dump()], - ), ) -> DatasetAssociationRoles: """Set permissions of the given history dataset to the given role ids.""" - update_payload = get_update_permission_payload(payload) + update_payload = normalize_permission_payload(payload) return self.service.update_permissions(trans, dataset_id, update_payload) @router.put( From 63a2533787f24b19057a5df947dda44558a46cff Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 18 Jul 2024 16:26:56 +0200 Subject: [PATCH 2/2] Update client API schema --- client/src/api/schema/schema.ts | 78 ++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 2 deletions(-) diff --git a/client/src/api/schema/schema.ts b/client/src/api/schema/schema.ts index 7a8a545e86e3..c10e2ba089d6 100644 --- a/client/src/api/schema/schema.ts +++ b/client/src/api/schema/schema.ts @@ -3980,6 +3980,11 @@ export interface components { */ name: string; }; + /** + * DatasetPermissionAction + * @enum {string} + */ + DatasetPermissionAction: "set_permissions" | "make_private" | "remove_restrictions"; /** * DatasetPermissions * @description Role-based permissions for accessing and managing a dataset. @@ -12272,6 +12277,69 @@ export interface components { /** Creator */ creator?: unknown; }; + /** UpdateDatasetPermissionsPayload */ + UpdateDatasetPermissionsPayload: { + /** Access Ids[] */ + "access_ids[]"?: string[] | string | null; + /** + * Action + * @description Indicates what action should be performed on the dataset. + * @default set_permissions + */ + action?: components["schemas"]["DatasetPermissionAction"] | null; + /** Manage Ids[] */ + "manage_ids[]"?: string[] | string | null; + /** Modify Ids[] */ + "modify_ids[]"?: string[] | string | null; + }; + /** UpdateDatasetPermissionsPayloadAliasB */ + UpdateDatasetPermissionsPayloadAliasB: { + /** + * Access IDs + * @description A list of role encoded IDs defining roles that should have access permission on the dataset. + */ + access?: string[] | string | null; + /** + * Action + * @description Indicates what action should be performed on the dataset. + * @default set_permissions + */ + action?: components["schemas"]["DatasetPermissionAction"] | null; + /** + * Manage IDs + * @description A list of role encoded IDs defining roles that should have manage permission on the dataset. + */ + manage?: string[] | string | null; + /** + * Modify IDs + * @description A list of role encoded IDs defining roles that should have modify permission on the dataset. + */ + modify?: string[] | string | null; + }; + /** UpdateDatasetPermissionsPayloadAliasC */ + UpdateDatasetPermissionsPayloadAliasC: { + /** + * Access IDs + * @description A list of role encoded IDs defining roles that should have access permission on the dataset. + */ + access_ids?: string[] | string | null; + /** + * Action + * @description Indicates what action should be performed on the dataset. + * @default set_permissions + */ + action?: components["schemas"]["DatasetPermissionAction"] | null; + /** + * Manage IDs + * @description A list of role encoded IDs defining roles that should have manage permission on the dataset. + */ + manage_ids?: string[] | string | null; + /** + * Modify IDs + * @description A list of role encoded IDs defining roles that should have modify permission on the dataset. + */ + modify_ids?: string[] | string | null; + }; /** * UpdateHistoryContentsBatchPayload * @description Contains property values that will be updated for all the history `items` provided. @@ -14653,7 +14721,10 @@ export interface operations { }; requestBody: { content: { - "application/json": Record; + "application/json": + | components["schemas"]["UpdateDatasetPermissionsPayload"] + | components["schemas"]["UpdateDatasetPermissionsPayloadAliasB"] + | components["schemas"]["UpdateDatasetPermissionsPayloadAliasC"]; }; }; responses: { @@ -18165,7 +18236,10 @@ export interface operations { }; requestBody: { content: { - "application/json": Record; + "application/json": + | components["schemas"]["UpdateDatasetPermissionsPayload"] + | components["schemas"]["UpdateDatasetPermissionsPayloadAliasB"] + | components["schemas"]["UpdateDatasetPermissionsPayloadAliasC"]; }; }; responses: {