refactor: add validation to storage, repo and session blueprints

Panaetius · Panaetius · commit ceef2c4875be · 2024-08-09T14:51:49.000+02:00
diff --git a/components/renku_data_services/repositories/blueprints.py b/components/renku_data_services/repositories/blueprints.py
@@ -3,14 +3,15 @@
 from dataclasses import dataclass
 from urllib.parse import unquote
 
-from sanic import HTTPResponse, Request, json
+from sanic import HTTPResponse, Request
 from sanic.response import JSONResponse
 
 import renku_data_services.base_models as base_models
 from renku_data_services import errors
 from renku_data_services.base_api.auth import authenticate
 from renku_data_services.base_api.blueprint import BlueprintFactoryResponse, CustomBlueprint
 from renku_data_services.base_api.etag import extract_if_none_match
+from renku_data_services.base_models.validation import validated_json
 from renku_data_services.repositories import apispec
 from renku_data_services.repositories.apispec_base import RepositoryParams
 from renku_data_services.repositories.db import GitRepositoriesRepository
@@ -53,10 +54,7 @@ async def get_internal_gitlab_user() -> base_models.APIUser:
                 if result.repository_metadata and result.repository_metadata.etag is not None
                 else None
             )
-            return json(
-                apispec.RepositoryProviderMatch.model_validate(result).model_dump(exclude_none=True, mode="json"),
-                headers=headers,
-            )
+            return validated_json(apispec.RepositoryProviderMatch, result, headers=headers)
 
         return "/repositories/<repository_url>", ["GET"], _get_one_repository
 
diff --git a/components/renku_data_services/session/blueprints.py b/components/renku_data_services/session/blueprints.py
@@ -3,7 +3,7 @@
 from dataclasses import dataclass
 from datetime import UTC, datetime
 
-from sanic import HTTPResponse, Request, json
+from sanic import HTTPResponse, Request
 from sanic.response import JSONResponse
 from sanic_ext import validate
 from ulid import ULID
@@ -12,6 +12,7 @@
 from renku_data_services import errors
 from renku_data_services.base_api.auth import authenticate, only_authenticated
 from renku_data_services.base_api.blueprint import BlueprintFactoryResponse, CustomBlueprint
+from renku_data_services.base_models.validation import validated_json
 from renku_data_services.session import apispec, models
 from renku_data_services.session.db import SessionRepository
 
@@ -28,9 +29,7 @@ def get_all(self) -> BlueprintFactoryResponse:
 
         async def _get_all(_: Request) -> JSONResponse:
             environments = await self.session_repo.get_environments()
-            return json(
-                [apispec.Environment.model_validate(e).model_dump(exclude_none=True, mode="json") for e in environments]
-            )
+            return validated_json(apispec.EnvironmentList, environments)
 
         return "/environments", ["GET"], _get_all
 
@@ -39,7 +38,7 @@ def get_one(self) -> BlueprintFactoryResponse:
 
         async def _get_one(_: Request, environment_id: ULID) -> JSONResponse:
             environment = await self.session_repo.get_environment(environment_id=environment_id)
-            return json(apispec.Environment.model_validate(environment).model_dump(exclude_none=True, mode="json"))
+            return validated_json(apispec.Environment, environment)
 
         return "/environments/<environment_id:ulid>", ["GET"], _get_one
 
@@ -59,7 +58,7 @@ async def _post(_: Request, user: base_models.APIUser, body: apispec.Environment
                 creation_date=datetime.now(UTC).replace(microsecond=0),
             )
             environment = await self.session_repo.insert_environment(user=user, new_environment=environment_model)
-            return json(apispec.Environment.model_validate(environment).model_dump(exclude_none=True, mode="json"), 201)
+            return validated_json(apispec.Environment, environment, 201)
 
         return "/environments", ["POST"], _post
 
@@ -75,7 +74,7 @@ async def _patch(
             environment = await self.session_repo.update_environment(
                 user=user, environment_id=environment_id, **body_dict
             )
-            return json(apispec.Environment.model_validate(environment).model_dump(exclude_none=True, mode="json"))
+            return validated_json(apispec.Environment, environment)
 
         return "/environments/<environment_id:ulid>", ["PATCH"], _patch
 
@@ -103,12 +102,7 @@ def get_all(self) -> BlueprintFactoryResponse:
         @authenticate(self.authenticator)
         async def _get_all(_: Request, user: base_models.APIUser) -> JSONResponse:
             launchers = await self.session_repo.get_launchers(user=user)
-            return json(
-                [
-                    apispec.SessionLauncher.model_validate(item).model_dump(exclude_none=True, mode="json")
-                    for item in launchers
-                ]
-            )
+            return validated_json(apispec.SessionLaunchersList, launchers)
 
         return "/session_launchers", ["GET"], _get_all
 
@@ -118,7 +112,7 @@ def get_one(self) -> BlueprintFactoryResponse:
         @authenticate(self.authenticator)
         async def _get_one(_: Request, user: base_models.APIUser, launcher_id: ULID) -> JSONResponse:
             launcher = await self.session_repo.get_launcher(user=user, launcher_id=launcher_id)
-            return json(apispec.SessionLauncher.model_validate(launcher).model_dump(exclude_none=True, mode="json"))
+            return validated_json(apispec.SessionLauncher, launcher)
 
         return "/session_launchers/<launcher_id:ulid>", ["GET"], _get_one
 
@@ -150,9 +144,7 @@ async def _post(_: Request, user: base_models.APIUser, body: apispec.SessionLaun
                 creation_date=datetime.now(UTC).replace(microsecond=0),
             )
             launcher = await self.session_repo.insert_launcher(user=user, new_launcher=launcher_model)
-            return json(
-                apispec.SessionLauncher.model_validate(launcher).model_dump(exclude_none=True, mode="json"), 201
-            )
+            return validated_json(apispec.SessionLauncher, launcher, 201)
 
         return "/session_launchers", ["POST"], _post
 
@@ -166,7 +158,7 @@ async def _patch(
         ) -> JSONResponse:
             body_dict = body.model_dump(exclude_none=True)
             launcher = await self.session_repo.update_launcher(user=user, launcher_id=launcher_id, **body_dict)
-            return json(apispec.SessionLauncher.model_validate(launcher).model_dump(exclude_none=True, mode="json"))
+            return validated_json(apispec.SessionLauncher, launcher)
 
         return "/session_launchers/<launcher_id:ulid>", ["PATCH"], _patch
 
@@ -186,11 +178,6 @@ def get_project_launchers(self) -> BlueprintFactoryResponse:
         @authenticate(self.authenticator)
         async def _get_launcher(_: Request, user: base_models.APIUser, project_id: ULID) -> JSONResponse:
             launchers = await self.session_repo.get_project_launchers(user=user, project_id=project_id)
-            return json(
-                [
-                    apispec.SessionLauncher.model_validate(item).model_dump(exclude_none=True, mode="json")
-                    for item in launchers
-                ]
-            )
+            return validated_json(apispec.SessionLaunchersList, launchers)
 
         return "/projects/<project_id:ulid>/session_launchers", ["GET"], _get_launcher
diff --git a/components/renku_data_services/session/models.py b/components/renku_data_services/session/models.py
@@ -41,7 +41,7 @@ class UnsavedEnvironment(BaseModel):
 class Environment(UnsavedEnvironment):  # type: ignore[misc]
     """Session environment model."""
 
-    id: str
+    id: ULID
 
 
 @dataclass(frozen=True, eq=True, kw_only=True)
diff --git a/components/renku_data_services/session/orm.py b/components/renku_data_services/session/orm.py
@@ -10,6 +10,7 @@
 from renku_data_services.crc.orm import ResourceClassORM
 from renku_data_services.project.orm import ProjectORM
 from renku_data_services.session import models
+from renku_data_services.utils.sqlalchemy import ULIDType
 
 metadata_obj = MetaData(schema="sessions")  # Has to match alembic ini section name
 
@@ -25,7 +26,7 @@ class EnvironmentORM(BaseORM):
 
     __tablename__ = "environments"
 
-    id: Mapped[str] = mapped_column("id", String(26), primary_key=True, default_factory=lambda: str(ULID()), init=False)
+    id: Mapped[ULID] = mapped_column("id", ULIDType, primary_key=True, default_factory=lambda: str(ULID()), init=False)
     """Id of this session environment object."""
 
     name: Mapped[str] = mapped_column("name", String(99))
diff --git a/components/renku_data_services/storage/api.spec.yaml b/components/renku_data_services/storage/api.spec.yaml
@@ -262,7 +262,7 @@ paths:
         content:
           application/json:
             schema:
-              $ref: "#/components/schemas/RCloneConfig"
+              $ref: "#/components/schemas/RCloneConfigValidate"
       responses:
         "204":
           description: The configuration is valid
@@ -346,6 +346,16 @@ components:
           nullable: true
         - type: boolean
         - type: object
+    RCloneConfigValidate: #this is the same as RCloneConfig but duplicated so a class gets generated
+      type: object
+      description: Dictionary of rclone key:value pairs (based on schema from '/storage_schema')
+      additionalProperties:
+        oneOf:
+        - type: integer
+        - type: string
+          nullable: true
+        - type: boolean
+        - type: object
     CloudStorageUrl:
       allOf:
         - $ref: "#/components/schemas/GitRequest"
diff --git a/components/renku_data_services/storage/apispec.py b/components/renku_data_services/storage/apispec.py
@@ -1,6 +1,6 @@
 # generated by datamodel-codegen:
 #   filename:  api.spec.yaml
-#   timestamp: 2024-08-06T05:55:29+00:00
+#   timestamp: 2024-08-09T12:39:58+00:00
 
 from __future__ import annotations
 
@@ -11,6 +11,12 @@
 from renku_data_services.storage.apispec_base import BaseAPISpec
 
 
+class RCloneConfigValidate(
+    RootModel[Optional[Dict[str, Union[int, Optional[str], bool, Dict[str, Any]]]]]
+):
+    root: Optional[Dict[str, Union[int, Optional[str], bool, Dict[str, Any]]]] = None
+
+
 class Example(BaseAPISpec):
     value: Optional[str] = Field(
         None, description="a potential value for the option (think enum)"
diff --git a/components/renku_data_services/storage/blueprints.py b/components/renku_data_services/storage/blueprints.py
@@ -3,7 +3,7 @@
 from dataclasses import dataclass
 from typing import Any
 
-from sanic import HTTPResponse, Request, empty, json
+from sanic import HTTPResponse, Request, empty
 from sanic.response import JSONResponse
 from sanic_ext import validate
 from ulid import ULID
@@ -13,6 +13,7 @@
 from renku_data_services.base_api.auth import authenticate
 from renku_data_services.base_api.blueprint import BlueprintFactoryResponse, CustomBlueprint
 from renku_data_services.base_api.misc import validate_query
+from renku_data_services.base_models.validation import validated_json
 from renku_data_services.storage import apispec, models
 from renku_data_services.storage.db import StorageRepository, StorageV2Repository
 from renku_data_services.storage.rclone import RCloneValidator
@@ -49,7 +50,9 @@ async def _get(
             storage: list[models.CloudStorage]
             storage = await self.storage_repo.get_storage(user=user, project_id=query.project_id)
 
-            return json([dump_storage_with_sensitive_fields(s, validator) for s in storage])
+            return validated_json(
+                apispec.StorageGetResponse, [dump_storage_with_sensitive_fields(s, validator) for s in storage]
+            )
 
         return "/storage", ["GET"], _get
 
@@ -65,7 +68,7 @@ async def _get_one(
         ) -> JSONResponse:
             storage = await self.storage_repo.get_storage_by_id(storage_id, user=user)
 
-            return json(dump_storage_with_sensitive_fields(storage, validator))
+            return validated_json(apispec.CloudStorageGet, dump_storage_with_sensitive_fields(storage, validator))
 
         return "/storage/<storage_id:ulid>", ["GET"], _get_one
 
@@ -96,7 +99,7 @@ async def _post(request: Request, user: base_models.APIUser, validator: RCloneVa
             validator.validate(storage.configuration.model_dump())
 
             res = await self.storage_repo.insert_storage(storage=storage, user=user)
-            return json(dump_storage_with_sensitive_fields(res, validator), 201)
+            return validated_json(apispec.CloudStorageGet, dump_storage_with_sensitive_fields(res, validator), 201)
 
         return "/storage", ["POST"], _post
 
@@ -131,7 +134,7 @@ async def _put(
             body_dict = new_storage.model_dump()
             del body_dict["storage_id"]
             res = await self.storage_repo.update_storage(storage_id=storage_id, user=user, **body_dict)
-            return json(dump_storage_with_sensitive_fields(res, validator))
+            return validated_json(apispec.CloudStorageGet, dump_storage_with_sensitive_fields(res, validator))
 
         return "/storage/<storage_id:ulid>", ["PUT"], _put
 
@@ -161,7 +164,7 @@ async def _patch(
             body_dict = body.model_dump(exclude_none=True)
 
             res = await self.storage_repo.update_storage(storage_id=storage_id, user=user, **body_dict)
-            return json(dump_storage_with_sensitive_fields(res, validator))
+            return validated_json(apispec.CloudStorageGet, dump_storage_with_sensitive_fields(res, validator))
 
         return "/storage/<storage_id:ulid>", ["PATCH"], _patch
 
@@ -195,9 +198,11 @@ async def _get(
             query: apispec.StorageV2Params,
         ) -> JSONResponse:
             storage: list[models.CloudStorage]
-            storage = await self.storage_v2_repo.get_storage(user=user, project_id=query.project_id)
+            storage = await self.storage_v2_repo.get_storage(user=user, project_id=ULID.from_str(query.project_id))
 
-            return json([dump_storage_with_sensitive_fields(s, validator) for s in storage])
+            return validated_json(
+                apispec.StoragesV2GetResponse, [dump_storage_with_sensitive_fields(s, validator) for s in storage]
+            )
 
         return "/storages_v2", ["GET"], _get
 
@@ -213,7 +218,7 @@ async def _get_one(
         ) -> JSONResponse:
             storage = await self.storage_v2_repo.get_storage_by_id(storage_id, user=user)
 
-            return json(dump_storage_with_sensitive_fields(storage, validator))
+            return validated_json(apispec.CloudStorageGet, dump_storage_with_sensitive_fields(storage, validator))
 
         return "/storages_v2/<storage_id:ulid>", ["GET"], _get_one
 
@@ -244,7 +249,7 @@ async def _post(request: Request, user: base_models.APIUser, validator: RCloneVa
             validator.validate(storage.configuration.model_dump())
 
             res = await self.storage_v2_repo.insert_storage(storage=storage, user=user)
-            return json(dump_storage_with_sensitive_fields(res, validator), 201)
+            return validated_json(apispec.CloudStorageGet, dump_storage_with_sensitive_fields(res, validator), 201)
 
         return "/storages_v2", ["POST"], _post
 
@@ -274,7 +279,7 @@ async def _patch(
             body_dict = body.model_dump(exclude_none=True)
 
             res = await self.storage_v2_repo.update_storage(storage_id=storage_id, user=user, **body_dict)
-            return json(dump_storage_with_sensitive_fields(res, validator))
+            return validated_json(apispec.CloudStorageGet, dump_storage_with_sensitive_fields(res, validator))
 
         return "/storages_v2/<storage_id:ulid>", ["PATCH"], _patch
 
@@ -297,29 +302,19 @@ def get(self) -> BlueprintFactoryResponse:
         """Get cloud storage for a repository."""
 
         async def _get(_: Request, validator: RCloneValidator) -> JSONResponse:
-            return json(validator.asdict())
+            return validated_json(apispec.RCloneSchema, validator.asdict())
 
         return "/storage_schema", ["GET"], _get
 
     def test_connection(self) -> BlueprintFactoryResponse:
         """Validate an RClone config."""
 
-        async def _test_connection(request: Request, validator: RCloneValidator) -> HTTPResponse:
-            if not request.json:
-                raise errors.ValidationError(message="The request body is empty. Please provide a valid JSON object.")
-            if not isinstance(request.json, dict):
-                raise errors.ValidationError(message="The request body is not a valid JSON object.")
-            if not request.json.get("configuration"):
-                raise errors.ValidationError(message="No 'configuration' sent.")
-            if not isinstance(request.json.get("configuration"), dict):
-                config_type = type(request.json.get("configuration"))
-                raise errors.ValidationError(
-                    message=f"The R clone configuration should be a dictionary, not {config_type.__name__}"
-                )
-            if not request.json.get("source_path"):
-                raise errors.ValidationError(message="'source_path' is required to test the connection.")
-            validator.validate(request.json["configuration"], keep_sensitive=True)
-            result = await validator.test_connection(request.json["configuration"], request.json["source_path"])
+        @validate(json=apispec.StorageSchemaTestConnectionPostRequest)
+        async def _test_connection(
+            request: Request, validator: RCloneValidator, body: apispec.StorageSchemaTestConnectionPostRequest
+        ) -> HTTPResponse:
+            validator.validate(body.configuration, keep_sensitive=True)
+            result = await validator.test_connection(body.configuration, body.source_path)
             if not result.success:
                 raise errors.ValidationError(message=result.error)
             return empty(204)
@@ -329,25 +324,23 @@ async def _test_connection(request: Request, validator: RCloneValidator) -> HTTP
     def validate(self) -> BlueprintFactoryResponse:
         """Validate an RClone config."""
 
-        async def _validate(request: Request, validator: RCloneValidator) -> HTTPResponse:
-            if not request.json:
-                raise errors.ValidationError(message="The request body is empty. Please provide a valid JSON object.")
-            if not isinstance(request.json, dict):
-                raise errors.ValidationError(message="The request body is not a valid JSON object.")
-            validator.validate(request.json, keep_sensitive=True)
+        @validate(json=apispec.RCloneConfigValidate)
+        async def _validate(
+            request: Request, validator: RCloneValidator, body: apispec.RCloneConfigValidate
+        ) -> HTTPResponse:
+            validator.validate(body.root, keep_sensitive=True)  # type: ignore[arg-type]
             return empty(204)
 
         return "/storage_schema/validate", ["POST"], _validate
 
     def obscure(self) -> BlueprintFactoryResponse:
         """Obscure values in config."""
 
-        async def _obscure(request: Request, validator: RCloneValidator) -> JSONResponse:
-            if not request.json:
-                raise errors.ValidationError(message="The request body is empty. Please provide a valid JSON object.")
-            if not isinstance(request.json, dict):
-                raise errors.ValidationError(message="The request body is not a valid JSON object.")
-            config = await validator.obscure_config(request.json)
-            return json(config)
+        @validate(json=apispec.StorageSchemaObscurePostRequest)
+        async def _obscure(
+            request: Request, validator: RCloneValidator, body: apispec.StorageSchemaObscurePostRequest
+        ) -> JSONResponse:
+            config = await validator.obscure_config(body.configuration)
+            return validated_json(apispec.RCloneConfigValidate, config)
 
         return "/storage_schema/obscure", ["POST"], _obscure
