From c0db38f523fadd3f787b378c39865ff4c1e89c3c Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Wed, 23 Aug 2023 14:35:59 +0200 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=F0=9F=93=9D=20API-server=20f?= =?UTF-8?q?iles=20collection=20cleanup=20and=20doc=20(#4645)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/coding-conventions.md | 59 +++--- services/api-server/openapi.json | 2 +- .../api/routes/files.py | 181 ++++++++---------- .../api/routes/solvers_jobs.py | 12 +- .../models/schemas/files.py | 8 +- .../solver_job_models_converters.py | 2 +- .../{utils => services}/solver_job_outputs.py | 0 .../services/storage.py | 8 +- ..._services_solver_job_models_converters.py} | 3 +- ...py => test_services_solver_job_outputs.py} | 2 +- 10 files changed, 140 insertions(+), 137 deletions(-) rename services/api-server/src/simcore_service_api_server/{utils => services}/solver_job_models_converters.py (99%) rename services/api-server/src/simcore_service_api_server/{utils => services}/solver_job_outputs.py (100%) rename services/api-server/tests/unit/{test_utils_solver_job_models_converters.py => test_services_solver_job_models_converters.py} (99%) rename services/api-server/tests/unit/{test_utils_solver_job_outputs.py => test_services_solver_job_outputs.py} (93%) diff --git a/docs/coding-conventions.md b/docs/coding-conventions.md index e885d0052c5..19eb4f95008 100644 --- a/docs/coding-conventions.md +++ b/docs/coding-conventions.md @@ -1,15 +1,44 @@ -# Coding Conventions and Linters +# Coding Conventions, Linters and Definitions Some conventions on coding style and tools for the Javascript and Python in this repository. ---- -## Javascript -In general the `qooxdoo` naming convention/style is followed. The [Access](http://qooxdoo.org/docs/#/core/oo_feature_summary?id=access) paragraph is the most notable. It is recommended to read the entire document. +## Definitions + +What is a ... + +- **Controller-Service-Repository** design-pattern ? + - An introduction: https://tom-collings.medium.com/controller-service-repository-16e29a4684e5 + - Example of adopted convention: https://github.com/ITISFoundation/osparc-simcore/pull/4389 -Have a look at `ESLint`'s configuration files [.eslintrc.json](.eslintrc.json) and [.eslintignore](.eslintignore). +---- +## General Coding Conventions + + + +### CC1: Can I use ``TODO:``, ``FIXME:``? + +We should avoid merging PRs with ``TODO:`` and ``FIXME:`` into master. One of our bots detects those and flag them as code-smells. If we still want to keep this idea/fix noted in the code, those can be rewritten as ``NOTE:`` and should be extended with a link to a github issue with more details. For a context, see [discussion here](https://github.com/ITISFoundation/osparc-simcore/pull/3380#discussion_r979893502). + + +### CC2: No commented code + +Avoid commented code, but if you *really* want to keep it then add an explanatory `NOTE:` +```python +import os +# import bar +# x = "not very useful" + +# NOTE: I need to keep this becase ... +# import foo +# x = "ok" +``` + +### CC3 ... + ---- ## Python @@ -62,28 +91,14 @@ In short we use the following naming convention ( roughly [PEP8](https://peps.p - Recommended inside of a ``scripts`` folder ----- -## General - - - -### CC1: Can I use ``TODO:``, ``FIXME:``? - -We should avoid merging PRs with ``TODO:`` and ``FIXME:`` into master. One of our bots detects those and flag them as code-smells. If we still want to keep this idea/fix noted in the code, those can be rewritten as ``NOTE:`` and should be extended with a link to a github issue with more details. For a context, see [discussion here](https://github.com/ITISFoundation/osparc-simcore/pull/3380#discussion_r979893502). -### CC2: No commented code +---- +## Javascript -Avoid commented code, but if you *really* want to keep it then add an explanatory `NOTE:` -```python -import os -# import bar -# x = "not very useful" +In general the `qooxdoo` naming convention/style is followed. The [Access](http://qooxdoo.org/docs/#/core/oo_feature_summary?id=access) paragraph is the most notable. It is recommended to read the entire document. -# NOTE: I need to keep this becase ... -# import foo -# x = "ok" -``` +Have a look at `ESLint`'s configuration files [.eslintrc.json](.eslintrc.json) and [.eslintignore](.eslintignore). diff --git a/services/api-server/openapi.json b/services/api-server/openapi.json index cac581a5f8e..6f395187f44 100644 --- a/services/api-server/openapi.json +++ b/services/api-server/openapi.json @@ -1570,8 +1570,8 @@ "PUBLISHED", "NOT_STARTED", "PENDING", + "WAITING_FOR_RESOURCES", "STARTED", - "RETRY", "SUCCESS", "FAILED", "ABORTED" diff --git a/services/api-server/src/simcore_service_api_server/api/routes/files.py b/services/api-server/src/simcore_service_api_server/api/routes/files.py index d8ad954417b..70e45213265 100644 --- a/services/api-server/src/simcore_service_api_server/api/routes/files.py +++ b/services/api-server/src/simcore_service_api_server/api/routes/files.py @@ -54,7 +54,10 @@ } -@router.get("", response_model=list[File]) +@router.get( + "", + response_model=list[File], +) async def list_files( storage_client: Annotated[StorageApi, Depends(get_api_client(StorageApi))], user_id: Annotated[int, Depends(get_current_user_id)], @@ -74,7 +77,7 @@ async def list_files( file_meta: File = to_file_api_model(stored_file_meta) - except (ValidationError, ValueError, AttributeError) as err: + except (ValidationError, ValueError, AttributeError) as err: # noqa: PERF203 _logger.warning( "Skipping corrupted entry in storage '%s' (%s)" "TIP: check this entry in file_meta_data table.", @@ -112,7 +115,10 @@ def _get_spooled_file_size(file_io: IO) -> int: return file_size -@router.put("/content", response_model=File) +@router.put( + "/content", + response_model=File, +) @cancel_on_disconnect async def upload_file( request: Request, @@ -129,6 +135,9 @@ async def upload_file( assert request # nosec + if file.filename is None: + file.filename = "Undefined" + file_size = await asyncio.get_event_loop().run_in_executor( None, _get_spooled_file_size, file.file ) @@ -151,16 +160,20 @@ async def upload_file( store_id=SIMCORE_LOCATION, store_name=None, s3_object=file_meta.storage_file_id, - path_to_upload=UploadableFileObject(file.file, file.filename, file_size), + path_to_upload=UploadableFileObject( + file_object=file.file, + file_name=file.filename, + file_size=file_size, + ), io_log_redirect_cb=None, ) - assert isinstance(upload_result, UploadedFile) + assert isinstance(upload_result, UploadedFile) # nosec file_meta.checksum = upload_result.etag return file_meta -# MaG suggested a single function that can upload one or multiple files instead of having +# NOTE: MaG suggested a single function that can upload one or multiple files instead of having # two of them. Tried something like upload_file( files: Union[list[UploadFile], File] ) but it # produces an error in the generated openapi.json # @@ -183,16 +196,7 @@ async def get_upload_links( client_file: ClientFile, user_id: Annotated[PositiveInt, Depends(get_current_user_id)], ): - """Get upload links for uploading a file to storage - - Arguments: - request -- Request object - client_file -- ClientFile object - user_id -- User Id - - Returns: - FileUploadSchema - """ + """Get upload links for uploading a file to storage""" assert request # nosec file_meta: File = await File.create_from_client_file( client_file, @@ -208,95 +212,27 @@ async def get_upload_links( file_size=ByteSize(client_file.filesize), is_directory=False, ) - query = str(upload_links.links.complete_upload.query).removesuffix(":complete") - complete_url = request.url_for( + + query = f"{upload_links.links.complete_upload.query}".removesuffix(":complete") + url = request.url_for( "complete_multipart_upload", file_id=file_meta.id ).include_query_params(**dict(item.split("=") for item in query.split("&"))) - query = str(upload_links.links.abort_upload.query).removesuffix(":abort") - abort_url = request.url_for( + upload_links.links.complete_upload = parse_obj_as(AnyUrl, f"{url}") + + query = f"{upload_links.links.abort_upload.query}".removesuffix(":abort") + url = request.url_for( "abort_multipart_upload", file_id=file_meta.id ).include_query_params(**dict(item.split("=") for item in query.split("&"))) + upload_links.links.abort_upload = parse_obj_as(AnyUrl, f"{url}") - upload_links.links.complete_upload = parse_obj_as(AnyUrl, f"{complete_url}") - upload_links.links.abort_upload = parse_obj_as(AnyUrl, f"{abort_url}") return ClientFileUploadSchema(file_id=file_meta.id, upload_schema=upload_links) -@router.post( - "/{file_id}:complete", +@router.get( + "/{file_id}", response_model=File, - include_in_schema=API_SERVER_DEV_FEATURES_ENABLED, -) -@cancel_on_disconnect -async def complete_multipart_upload( - request: Request, - file_id: UUID, - client_file: Annotated[ClientFile, Body(...)], - uploaded_parts: Annotated[FileUploadCompletionBody, Body(...)], - storage_client: Annotated[StorageApi, Depends(get_api_client(StorageApi))], - user_id: Annotated[PositiveInt, Depends(get_current_user_id)], -): - """Complete multipart upload - - Arguments: - request: The Request object - file_id: The Storage id - file: The File object which is to be completed - uploaded_parts: The uploaded parts - completion_link: The completion link - user_id: The user id - - Returns: - The completed File object - """ - assert request # nosec - assert user_id # nosec - - file: File = File(id=file_id, filename=client_file.filename, checksum=None) - complete_link: URL = await storage_client.generate_complete_upload_link( - file, dict(request.query_params) - ) - - e_tag: ETag = await complete_file_upload( - uploaded_parts=uploaded_parts.parts, - upload_completion_link=parse_obj_as(AnyUrl, f"{complete_link}"), - ) - - file.checksum = e_tag - return file - - -@router.post( - "/{file_id}:abort", - include_in_schema=API_SERVER_DEV_FEATURES_ENABLED, + responses={**_COMMON_ERROR_RESPONSES}, ) -@cancel_on_disconnect -async def abort_multipart_upload( - request: Request, - file_id: UUID, - client_file: Annotated[ClientFile, Body(..., embed=True)], - storage_client: Annotated[StorageApi, Depends(get_api_client(StorageApi))], - user_id: Annotated[PositiveInt, Depends(get_current_user_id)], -): - """Abort a multipart upload - - Arguments: - request: The Request - file_id: The StorageFileID - upload_links: The FileUploadSchema - user_id: The user id - - """ - assert request # nosec - assert user_id # nosec - file: File = File(id=file_id, filename=client_file.filename, checksum=None) - abort_link: URL = await storage_client.generate_abort_upload_link( - file, query=dict(request.query_params) - ) - await abort_upload(abort_upload_link=parse_obj_as(AnyUrl, str(abort_link))) - - -@router.get("/{file_id}", response_model=File, responses={**_COMMON_ERROR_RESPONSES}) async def get_file( file_id: UUID, storage_client: Annotated[StorageApi, Depends(get_api_client(StorageApi))], @@ -343,6 +279,57 @@ async def delete_file( raise NotImplementedError(msg) +@router.post( + "/{file_id}:abort", + include_in_schema=API_SERVER_DEV_FEATURES_ENABLED, +) +async def abort_multipart_upload( + request: Request, + file_id: UUID, + client_file: Annotated[ClientFile, Body(..., embed=True)], + storage_client: Annotated[StorageApi, Depends(get_api_client(StorageApi))], + user_id: Annotated[PositiveInt, Depends(get_current_user_id)], +): + assert request # nosec + assert user_id # nosec + file: File = File(id=file_id, filename=client_file.filename, checksum=None) + abort_link: URL = await storage_client.create_abort_upload_link( + file, query=dict(request.query_params) + ) + await abort_upload(abort_upload_link=parse_obj_as(AnyUrl, str(abort_link))) + + +@router.post( + "/{file_id}:complete", + response_model=File, + include_in_schema=API_SERVER_DEV_FEATURES_ENABLED, +) +@cancel_on_disconnect +async def complete_multipart_upload( + request: Request, + file_id: UUID, + client_file: Annotated[ClientFile, Body(...)], + uploaded_parts: Annotated[FileUploadCompletionBody, Body(...)], + storage_client: Annotated[StorageApi, Depends(get_api_client(StorageApi))], + user_id: Annotated[PositiveInt, Depends(get_current_user_id)], +): + assert request # nosec + assert user_id # nosec + + file: File = File(id=file_id, filename=client_file.filename, checksum=None) + complete_link: URL = await storage_client.create_complete_upload_link( + file, dict(request.query_params) + ) + + e_tag: ETag = await complete_file_upload( + uploaded_parts=uploaded_parts.parts, + upload_completion_link=parse_obj_as(AnyUrl, f"{complete_link}"), + ) + + file.checksum = e_tag + return file + + @router.get( "/{file_id}/content", response_class=RedirectResponse, @@ -370,7 +357,9 @@ async def download_file( # download from S3 using pre-signed link presigned_download_link = await storage_client.get_download_link( - user_id, file_meta.id, file_meta.filename + user_id=user_id, + file_id=file_meta.id, + file_name=file_meta.filename, ) _logger.info("Downloading %s to %s ...", file_meta, presigned_download_link) diff --git a/services/api-server/src/simcore_service_api_server/api/routes/solvers_jobs.py b/services/api-server/src/simcore_service_api_server/api/routes/solvers_jobs.py index 6ff05c9b8c0..8be8d352b05 100644 --- a/services/api-server/src/simcore_service_api_server/api/routes/solvers_jobs.py +++ b/services/api-server/src/simcore_service_api_server/api/routes/solvers_jobs.py @@ -32,13 +32,13 @@ from ...models.schemas.solvers import Solver, SolverKeyId from ...services.catalog import CatalogApi from ...services.director_v2 import DirectorV2Api, DownloadLink, NodeName -from ...services.storage import StorageApi, to_file_api_model -from ...utils.solver_job_models_converters import ( +from ...services.solver_job_models_converters import ( create_job_from_project, create_jobstatus_from_task, create_new_project_for_job, ) -from ...utils.solver_job_outputs import ResultsTypes, get_solver_output_results +from ...services.solver_job_outputs import ResultsTypes, get_solver_output_results +from ...services.storage import StorageApi, to_file_api_model from ..dependencies.application import get_product_name, get_reverse_url_mapper from ..dependencies.authentication import get_current_user_id from ..dependencies.database import Engine, get_db_engine @@ -127,11 +127,7 @@ async def get_jobs_page( url_for: Annotated[Callable, Depends(get_reverse_url_mapper)], product_name: Annotated[str, Depends(get_product_name)], ): - """List of jobs on a specific released solver (includes pagination) - - - Breaking change in *version 0.5*: response model changed from list[Job] to pagination Page[Job]. - """ + """List of jobs on a specific released solver (includes pagination)""" # NOTE: Different entry to keep backwards compatibility with list_jobs. # Eventually use a header with agent version to switch to new interface diff --git a/services/api-server/src/simcore_service_api_server/models/schemas/files.py b/services/api-server/src/simcore_service_api_server/models/schemas/files.py index f0d2e1a9052..c057f4a230b 100644 --- a/services/api-server/src/simcore_service_api_server/models/schemas/files.py +++ b/services/api-server/src/simcore_service_api_server/models/schemas/files.py @@ -13,7 +13,7 @@ from ...utils.hash import create_md5_checksum -NAMESPACE_FILEID_KEY = UUID("aa154444-d22d-4290-bb15-df37dba87865") +_NAMESPACE_FILEID_KEY = UUID("aa154444-d22d-4290-bb15-df37dba87865") class FileName(ConstrainedStr): @@ -106,7 +106,7 @@ async def create_from_uploaded( return cls( id=cls.create_id(md5check or file_size, file.filename, created_at), - filename=file.filename, + filename=file.filename or "Undefined", content_type=file.content_type, checksum=md5check, ) @@ -131,7 +131,7 @@ async def create_from_quoted_storage_id(cls, quoted_storage_id: str) -> "File": @classmethod def create_id(cls, *keys) -> UUID: - return uuid3(NAMESPACE_FILEID_KEY, ":".join(map(str, keys))) + return uuid3(_NAMESPACE_FILEID_KEY, ":".join(map(str, keys))) @property def storage_file_id(self) -> StorageFileID: @@ -145,7 +145,7 @@ def quoted_storage_file_id(self) -> str: class ClientFileUploadSchema(BaseModel): - file_id: UUID = Field(..., description="The file id") + file_id: UUID = Field(..., description="The file resource id") upload_schema: FileUploadSchema = Field( ..., description="Schema for uploading file" ) diff --git a/services/api-server/src/simcore_service_api_server/utils/solver_job_models_converters.py b/services/api-server/src/simcore_service_api_server/services/solver_job_models_converters.py similarity index 99% rename from services/api-server/src/simcore_service_api_server/utils/solver_job_models_converters.py rename to services/api-server/src/simcore_service_api_server/services/solver_job_models_converters.py index 9fc2894996f..734c3ca95e8 100644 --- a/services/api-server/src/simcore_service_api_server/utils/solver_job_models_converters.py +++ b/services/api-server/src/simcore_service_api_server/services/solver_job_models_converters.py @@ -24,7 +24,7 @@ PercentageInt, ) from ..models.schemas.solvers import Solver, SolverKeyId -from ..services.director_v2 import ComputationTaskGet +from .director_v2 import ComputationTaskGet # UTILS ------ _BASE_UUID = uuid.UUID("231e13db-6bc6-4f64-ba56-2ee2c73b9f09") diff --git a/services/api-server/src/simcore_service_api_server/utils/solver_job_outputs.py b/services/api-server/src/simcore_service_api_server/services/solver_job_outputs.py similarity index 100% rename from services/api-server/src/simcore_service_api_server/utils/solver_job_outputs.py rename to services/api-server/src/simcore_service_api_server/services/solver_job_outputs.py diff --git a/services/api-server/src/simcore_service_api_server/services/storage.py b/services/api-server/src/simcore_service_api_server/services/storage.py index 6905a83d9c0..38ffae2641a 100644 --- a/services/api-server/src/simcore_service_api_server/services/storage.py +++ b/services/api-server/src/simcore_service_api_server/services/storage.py @@ -47,6 +47,9 @@ class StorageApi(BaseServiceClientApi): async def list_files(self, user_id: int) -> list[StorageFileMetaData]: """Lists metadata of all s3 objects name as api/* from a given user""" + + # search_files_starting_with + response = await self.client.post( "/simcore-s3/files/metadata:search", params={ @@ -97,6 +100,7 @@ async def get_upload_links( ) -> FileUploadSchema: object_path = urllib.parse.quote_plus(f"api/{file_id}/{file_name}") + # complete_upload_file response = await self.client.put( f"/locations/{self.SIMCORE_S3_ID}/files/{object_path}", params={"user_id": user_id, "file_size": 0}, @@ -106,7 +110,7 @@ async def get_upload_links( assert enveloped_data.data # nosec return enveloped_data.data - async def generate_complete_upload_link( + async def create_complete_upload_link( self, file: File, query: dict[str, str] | None = None ) -> URL: url = URL( @@ -116,7 +120,7 @@ async def generate_complete_upload_link( url = url.include_query_params(**query) return url - async def generate_abort_upload_link( + async def create_abort_upload_link( self, file: File, query: dict[str, str] | None = None ) -> URL: url = URL( diff --git a/services/api-server/tests/unit/test_utils_solver_job_models_converters.py b/services/api-server/tests/unit/test_services_solver_job_models_converters.py similarity index 99% rename from services/api-server/tests/unit/test_utils_solver_job_models_converters.py rename to services/api-server/tests/unit/test_services_solver_job_models_converters.py index 88a8d6e00b5..250e94f1106 100644 --- a/services/api-server/tests/unit/test_utils_solver_job_models_converters.py +++ b/services/api-server/tests/unit/test_services_solver_job_models_converters.py @@ -9,7 +9,7 @@ from simcore_service_api_server.models.schemas.files import File from simcore_service_api_server.models.schemas.jobs import ArgumentTypes, Job, JobInputs from simcore_service_api_server.models.schemas.solvers import Solver -from simcore_service_api_server.utils.solver_job_models_converters import ( +from simcore_service_api_server.services.solver_job_models_converters import ( create_job_from_project, create_job_inputs_from_node_inputs, create_jobstatus_from_task, @@ -108,7 +108,6 @@ def test_job_to_node_inputs_conversion(): def test_create_job_from_project(): - project = Project.parse_obj( { "uuid": "f925e30f-19de-42dc-acab-3ce93ea0a0a7", diff --git a/services/api-server/tests/unit/test_utils_solver_job_outputs.py b/services/api-server/tests/unit/test_services_solver_job_outputs.py similarity index 93% rename from services/api-server/tests/unit/test_utils_solver_job_outputs.py rename to services/api-server/tests/unit/test_services_solver_job_outputs.py index 620bf95d5c3..04765e420f7 100644 --- a/services/api-server/tests/unit/test_utils_solver_job_outputs.py +++ b/services/api-server/tests/unit/test_services_solver_job_outputs.py @@ -6,7 +6,7 @@ from typing import get_args, get_origin from simcore_service_api_server.models.schemas.jobs import ArgumentTypes, File -from simcore_service_api_server.utils.solver_job_outputs import ( +from simcore_service_api_server.services.solver_job_outputs import ( BaseFileLink, ResultsTypes, )