feat: add components to workflow

.flake8

@@ -0,0 +1,8 @@
+[flake8]
+exclude = venv,__pycache__
+# For black compatibility
+max-line-length = 1
+ignore =
+    # For black compatibility
+    E203,
+    E501,

.gitignore

@@ -4,4 +4,5 @@
 .idea/
 *local*
 *.egg-info/
-venv/
+venv/
+microservice.db

README.md

@@ -13,6 +13,21 @@ run the tests with
 
 ```bash
 python -m unittest discover
+
+# Quality
+python -m black .
+python -m flake8 .
+python -m isort .
+```
+
+
+run the application
+
+```bash
+# Optional, delete the database
+rm -f microservice.db
+# Run the API
+python -m fastapi dev microservice/api.py
 ```
 
 ## Instructions
@@ -25,8 +40,8 @@ For now, `POST /workflow/` accepts a JSON with only one property `name: str`.
 
 ### TODO
 
-Your task is 
-- [ ] to extend the `/workflow/` endpoint to also accept an optional list of components in its request and store it in the database
+Your task is
+- [x] to extend the `/workflow/` endpoint to also accept an optional list of components in its request and store it in the database
   - a component is a dictionary containing two fields 
     - `type` (required) whose value must be one of `{"import", "shadow", "crop", "export"}`
     - and `settings` (optional), a dictionary mapping strings (name of the setting) to a value whose type must be one of `{int, float, str, bool}`
@@ -41,14 +56,14 @@ Your task is
 >  ]
 > }
 
-- [ ] the list of components must be validated according to the following rules:
-  - [ ] the list should not contain two components of the same `type` 
-  - [ ] if present, component of `type` `"import"` and `"export"` must be, respectively, first and last in the list
-  - [ ] all components should either all contain the `settings` field or none shall contain it. 
-- [ ] if the input validation fails, the endpoint should return an appropriate status code and a helpful message
-- [ ] provide tests to show 
-  - [ ] that the endpoint now supports the new JSON schema 
-  - [ ] and enforces the rules for the input validation
+- [x] the list of components must be validated according to the following rules:
+  - [x] the list should not contain two components of the same `type`
+  - [x] if present, component of `type` `"import"` and `"export"` must be, respectively, first and last in the list
+  - [x] all components should either all contain the `settings` field or none shall contain it.
+- [x] if the input validation fails, the endpoint should return an appropriate status code and a helpful message
+- [x] provide tests to show
+  - [x] that the endpoint now supports the new JSON schema
+  - [x] and enforces the rules for the input validation
 
 You can base your tests on `tests/test_create_workflow.py` but please, do not modify `test_should_create_workflow_with_name()` since this test still needs to be green once you are done!
 
@@ -65,3 +80,16 @@ Once you are finished, please
 Finally, please feel free to make this repo yours! You're allowed to change anything and everything!
 
 Good luck and have fun!
+
+# Asumptions / Decisions
+- Validation is done at pydantic level as all the validations rules are based on the input.
+- Settings storage: one naive approach would be to store each settings in a row in a dedicated Setting table with a foreign key to the related component. If we would go that way, it would require to have a `key`, `value` and `type` columns as we receive a dictionary mapping a name with an heterogeneous value (either a int, float, string or boolean value). The alternative was to store it as JSON as we can easily go back and forth between dict and JSON. So because of that, it's making more sense to have the settings into the `Component` table instead of inside a `Setting` table if stored as JSON (prevent an unnecessary join for a 0,1 relationship).
+Storing as JSON instead of rows in a `Setting` also help reducing the number of necessary joins to retrive the workflow, ease the reconstruction of the settings (instead of having to check the `type` and cast it back to the correct type)
+- Validation error handling: the response returned by default by FastAPI is complete but a little too complicated with a lot of information. I have set up a handler in order to simply the response
+
+
+# Improvements
+- Better separation of concern: do not call database directly in the controller by using Service Layer or Repository Pattern
+- Better error message for `settings` value validation: at the moment, if the setting value is neither an int, float, string or boolean, the validation will fail (as expected) but will trigger 4 errors messages (one for each authorised type). It would be clearer for the end user to have one single error to digest instead of 4
+- Store the type as the enum value (lowercased) instead of the Enum name
+- Improve the response returned on request validation failing

microservice/api.py

@@ -1,8 +1,16 @@
-from pydantic import BaseModel
-from fastapi import FastAPI, Depends
+from collections import defaultdict
+from enum import Enum
+from typing import Union
+
+from fastapi import Depends, FastAPI, Request, status
+from fastapi.encoders import jsonable_encoder
+from fastapi.exceptions import RequestValidationError
+from fastapi.responses import JSONResponse
+from pydantic import BaseModel, field_validator
 from sqlmodel import Session
+
 from microservice.db.engine import create_tables, get_session
-from microservice.db.models import Workflow
+from microservice.db.models import Component, Workflow
 
 app = FastAPI()
 
@@ -11,22 +19,118 @@
 def start_db():
     create_tables()
 
-# TODO: add list of components with `type` and `settings` fields to the request
+
+@app.exception_handler(RequestValidationError)
+async def validation_exception_handler(request: Request, exc: RequestValidationError):
+    reformatted_error_messages = defaultdict(list)
+    for validations_errors in exc.errors():
+        error_message = validations_errors.get("msg", "")
+        invalid_field_raw = validations_errors.get("loc", "")
+
+        invalid_field = (
+            invalid_field_raw[1:]
+            if invalid_field_raw[0] in ("body", "query", "path")
+            else invalid_field_raw
+        )
+        invalid_field_path = ".".join([str(x) for x in invalid_field])
+
+        reformatted_error_messages[invalid_field_path].append(error_message)
+
+    return JSONResponse(
+        status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
+        content=jsonable_encoder(
+            {"detail": "Invalid request", "errors": reformatted_error_messages}
+        ),
+    )
+
+
+class ComponentTypeEnum(str, Enum):
+    IMPORT = "import"
+    SHADOW = "shadow"
+    CROP = "crop"
+    EXPORT = "export"
+
+
+class ComponentSchema(BaseModel):
+    type: ComponentTypeEnum
+    settings: dict[str, Union[int, float, str, bool]] | None = None
 
 
 class WorkflowSchema(BaseModel):
     name: str
+    components: list[ComponentSchema] = []
 
+    @field_validator("components")
+    def validate_components_uniqueness(cls, components):
+        unique_components_types = set()
+        duplicated_components_types = set()
 
-@app.post("/workflow")
-def create_workflow(
-        request: WorkflowSchema,
-        session: Session = Depends(get_session)
+        for c in components:
+            if c.type in unique_components_types:
+                duplicated_components_types.add(c.type)
+            unique_components_types.add(c.type)
+
+        if duplicated_components_types:
+            duplicated_components_types_str = ", ".join(
+                [c.value for c in duplicated_components_types]
+            )
+            raise ValueError(f"Duplicated types: {duplicated_components_types_str}")
 
-):
-    # TODO: validate and store components
-    workflow_db = Workflow(name=request.name)
+        return components
+
+    @field_validator("components")
+    def validate_components_import_export_position(cls, components):
+        import_index = next(
+            (i for i, c in enumerate(components) if c.type == ComponentTypeEnum.IMPORT),
+            None,
+        )
+        export_index = next(
+            (i for i, c in enumerate(components) if c.type == ComponentTypeEnum.EXPORT),
+            None,
+        )
+
+        # neither import nor export components are present
+        if import_index is None and export_index is None:
+            return components
+
+        if export_index is not None and export_index != len(components) - 1:
+            if import_index is not None and import_index != 0:
+                raise ValueError(
+                    "'import' must be at the start and 'export' must be at the end of components"
+                )
+            raise ValueError("'export' must be at the end of components")
+
+        if import_index is not None and import_index != 0:
+            raise ValueError("'import' must be at the start of components")
+
+        return components
+
+    @field_validator("components")
+    def validate_components_settings(cls, components):
+        has_settings = [c.settings is not None for c in components]
+
+        if any(has_settings) and not all(has_settings):
+            raise ValueError(
+                "settings must either be present or missing for all components"
+            )
+
+        return components
+
+
+@app.post("/workflow")
+def create_workflow(request: WorkflowSchema, session: Session = Depends(get_session)):
+    components_db = [
+        Component(
+            type=c.type.value,
+            settings=c.settings,
+        )
+        for c in request.components
+    ]
+    workflow_db = Workflow(
+        name=request.name,
+        components=components_db,
+    )
     session.add(workflow_db)
     session.commit()
     session.refresh(workflow_db)
-    return workflow_db.id
+    return JSONResponse(content={"workflow_id": str(workflow_db.id)})

microservice/db/engine.py

@@ -1,7 +1,6 @@
-from sqlmodel import SQLModel, create_engine, Session
+from sqlmodel import Session, SQLModel, create_engine
 from sqlmodel.pool import StaticPool
 
-
 engine = None
 
 

microservice/db/models.py

@@ -1,5 +1,16 @@
-from uuid import uuid4, UUID
-from sqlmodel import Field, SQLModel
+from enum import Enum
+from typing import Dict, Optional
+from uuid import UUID, uuid4
+
+from sqlalchemy.types import JSON
+from sqlmodel import Column, Field, Relationship, SQLModel
+
+
+class ComponentTypeEnum(str, Enum):
+    IMPORT = "import"
+    SHADOW = "shadow"
+    CROP = "crop"
+    EXPORT = "export"
 
 
 class Workflow(SQLModel, table=True):
@@ -10,5 +21,17 @@ class Workflow(SQLModel, table=True):
         nullable=False,
     )
     name: str
+    components: list["Component"] = Relationship(back_populates="workflow")
 
-# TODO: add tables for `Component` and `Settings`
+
+class Component(SQLModel, table=True):
+    id: UUID = Field(
+        default_factory=uuid4,
+        primary_key=True,
+        index=True,
+        nullable=False,
+    )
+    type: ComponentTypeEnum = Field()
+    settings: Dict = Field(default_factory=None, sa_column=Column(JSON))
+    workflow_id: Optional[UUID] = Field(default=None, foreign_key="workflow.id")
+    workflow: Optional[Workflow] = Relationship(back_populates="components")

requirements.test.txt

@@ -1,2 +1,8 @@
+assertpy
+black
+fastapi[standard]
+flake8
 httpx
-assertpy
+ipdb
+isort
+parameterized