From 1134ea5be586b594f229593a8f99ffeb65e38383 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 Aug 2025 21:20:58 +0000 Subject: [PATCH 1/2] Initial plan From 5b5ac59de9cc30783a8c93930ab2ce11da88f4c6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 Aug 2025 21:37:18 +0000 Subject: [PATCH 2/2] Add comprehensive API endpoint specifications with response_model, status_code, responses, and examples Co-authored-by: nolan1999 <54246789+nolan1999@users.noreply.github.com> --- tests/unit/test_api_specification_static.py | 179 +++++++++++++++++++ tests/unit/test_openapi_spec.py | 188 ++++++++++++++++++++ workerfacing_api/endpoints/access.py | 4 + workerfacing_api/endpoints/files.py | 12 ++ workerfacing_api/endpoints/jobs.py | 31 +++- workerfacing_api/endpoints/jobs_post.py | 4 + workerfacing_api/main.py | 16 +- workerfacing_api/schemas/files.py | 16 +- workerfacing_api/schemas/queue_jobs.py | 47 ++--- workerfacing_api/schemas/responses.py | 13 ++ 10 files changed, 478 insertions(+), 32 deletions(-) create mode 100644 tests/unit/test_api_specification_static.py create mode 100644 tests/unit/test_openapi_spec.py create mode 100644 workerfacing_api/schemas/responses.py diff --git a/tests/unit/test_api_specification_static.py b/tests/unit/test_api_specification_static.py new file mode 100644 index 0000000..d7ca050 --- /dev/null +++ b/tests/unit/test_api_specification_static.py @@ -0,0 +1,179 @@ +""" +Static validation tests for API endpoint definitions to ensure they have proper FastAPI decorator parameters. +This tests the code structure without running the full application. +""" +import os +import ast +import sys +from typing import Dict, List, Set + +# Add the project root to the path +sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', '..')) + +def get_fastapi_decorators_from_file(file_path: str) -> List[Dict]: + """Extract FastAPI decorators and their parameters from a Python file.""" + with open(file_path, 'r') as f: + tree = ast.parse(f.read()) + + decorators = [] + + for node in ast.walk(tree): + if isinstance(node, ast.FunctionDef): + for decorator in node.decorator_list: + if isinstance(decorator, ast.Call): + # Check if it's a router method call (e.g., @router.get, @router.post, etc.) + if (isinstance(decorator.func, ast.Attribute) and + isinstance(decorator.func.value, ast.Name) and + decorator.func.value.id == 'router'): + + method = decorator.func.attr + path = None + params = {} + + # Get the path (first positional argument) + if decorator.args: + if isinstance(decorator.args[0], ast.Constant): + path = decorator.args[0].value + + # Get keyword arguments + for keyword in decorator.keywords: + if isinstance(keyword.value, ast.Constant): + params[keyword.arg] = keyword.value.value + elif isinstance(keyword.value, ast.Name): + params[keyword.arg] = keyword.value.id + elif isinstance(keyword.value, ast.List): + # Handle list values like tags=["Jobs"] + list_items = [] + for item in keyword.value.elts: + if isinstance(item, ast.Constant): + list_items.append(item.value) + params[keyword.arg] = list_items + elif isinstance(keyword.value, ast.Dict): + # Handle dict values like responses={} + params[keyword.arg] = "dict_value" + + decorators.append({ + 'method': method, + 'path': path, + 'function_name': node.name, + 'params': params + }) + + elif isinstance(decorator, ast.Attribute): + # Handle decorators like @workerfacing_app.get + if (isinstance(decorator.value, ast.Name) and + decorator.value.id == 'workerfacing_app'): + # This is likely from main.py root endpoint + decorators.append({ + 'method': decorator.attr, + 'path': '/', # Root path + 'function_name': node.name, + 'params': {} + }) + + return decorators + + +def test_endpoints_have_required_fields(): + """Test that all endpoint decorators have the required FastAPI fields.""" + endpoint_files = [ + 'workerfacing_api/endpoints/access.py', + 'workerfacing_api/endpoints/files.py', + 'workerfacing_api/endpoints/jobs.py', + 'workerfacing_api/endpoints/jobs_post.py', + 'workerfacing_api/main.py' + ] + + all_endpoints = [] + for file_path in endpoint_files: + if os.path.exists(file_path): + decorators = get_fastapi_decorators_from_file(file_path) + all_endpoints.extend(decorators) + + print(f"Found {len(all_endpoints)} endpoints to validate") + + # Required fields for all endpoints + required_fields = ['description'] + + # Fields that should be present (either explicitly or through defaults) + expected_fields = ['response_model', 'status_code', 'responses'] + + issues = [] + + for endpoint in all_endpoints: + method = endpoint['method'] + path = endpoint['path'] + params = endpoint['params'] + func_name = endpoint['function_name'] + + endpoint_id = f"{method.upper()} {path} ({func_name})" + + # Check required fields + for field in required_fields: + if field not in params: + issues.append(f"{endpoint_id} missing {field}") + + # Check for responses field + if 'responses' not in params: + issues.append(f"{endpoint_id} missing responses parameter") + + # Check status_code for non-GET methods or specific cases + if method in ['post', 'put', 'delete'] and 'status_code' not in params: + issues.append(f"{endpoint_id} missing explicit status_code") + + # Print all found endpoints for debugging + print("\nFound endpoints:") + for endpoint in all_endpoints: + print(f" {endpoint['method'].upper()} {endpoint['path']} - {endpoint['function_name']}") + print(f" Params: {list(endpoint['params'].keys())}") + + if issues: + print(f"\nFound {len(issues)} issues:") + for issue in issues: + print(f" - {issue}") + assert False, f"Found {len(issues)} endpoint specification issues" + else: + print(f"\nAll {len(all_endpoints)} endpoints have proper specifications!") + + +def test_pydantic_models_have_examples(): + """Test that Pydantic models have Field examples for OpenAPI documentation.""" + schema_files = [ + 'workerfacing_api/schemas/files.py', + 'workerfacing_api/schemas/queue_jobs.py', + 'workerfacing_api/schemas/responses.py' + ] + + models_with_examples = [] + models_without_examples = [] + + for file_path in schema_files: + if os.path.exists(file_path): + with open(file_path, 'r') as f: + content = f.read() + + # Check for Field with example parameter + if 'Field(' in content and 'example=' in content: + models_with_examples.append(file_path) + else: + # Check if there are any BaseModel classes that should have examples + tree = ast.parse(content) + has_models = any( + isinstance(node, ast.ClassDef) and + any(isinstance(base, ast.Name) and base.id == 'BaseModel' for base in node.bases) + for node in ast.walk(tree) + ) + if has_models and file_path not in models_with_examples: + models_without_examples.append(file_path) + + print(f"Schema files with examples: {models_with_examples}") + print(f"Schema files that may need examples: {models_without_examples}") + + # For this test, we'll check that at least some models have examples + assert len(models_with_examples) > 0, "No Pydantic models found with Field examples" + + +if __name__ == "__main__": + test_endpoints_have_required_fields() + test_pydantic_models_have_examples() + print("All static validation tests passed!") \ No newline at end of file diff --git a/tests/unit/test_openapi_spec.py b/tests/unit/test_openapi_spec.py new file mode 100644 index 0000000..eb0b10c --- /dev/null +++ b/tests/unit/test_openapi_spec.py @@ -0,0 +1,188 @@ +""" +Tests to validate that all API endpoints have proper OpenAPI specification including: +- response_model +- status_code +- responses +- description +- examples +""" +import json +import sys +import os + +# Add the project root to the path +sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..', '..')) + +import pytest +from unittest.mock import patch, MagicMock +from fastapi.testclient import TestClient + +# Mock all the dependencies that require external services +mock_modules = [ + 'workerfacing_api.dependencies', + 'workerfacing_api.settings', + 'workerfacing_api.core.filesystem', + 'workerfacing_api.core.queue', + 'workerfacing_api.crud', + 'fastapi_utils.tasks', + 'sqlalchemy', +] + +for module in mock_modules: + sys.modules[module] = MagicMock() + +# Mock repeat_every decorator +def mock_repeat_every(*args, **kwargs): + def decorator(func): + return func + return decorator + +sys.modules['fastapi_utils.tasks'].repeat_every = mock_repeat_every + +# Now import the app +from workerfacing_api.main import workerfacing_app + + +def test_openapi_schema_structure(): + """Test that the OpenAPI schema contains expected fields for all endpoints.""" + # Mock environment variables + with patch.dict(os.environ, { + 'COGNITO_USER_POOL_ID': 'test-pool', + 'COGNITO_CLIENT_ID': 'test-client', + 'COGNITO_REGION': 'us-east-1' + }): + with TestClient(workerfacing_app) as client: + response = client.get("/openapi.json") + assert response.status_code == 200 + + openapi_spec = response.json() + paths = openapi_spec["paths"] + + # Expected paths from the endpoints + expected_paths = [ + "/", + "/access_info", + "/files/{file_id}/download", + "/files/{file_id}/url", + "/jobs", + "/jobs/{job_id}/status", + "/jobs/{job_id}/files/upload", + "/jobs/{job_id}/files/url", + "/_jobs" + ] + + for path in expected_paths: + assert path in paths, f"Path {path} not found in OpenAPI spec" + + +def test_endpoints_have_descriptions(): + """Test that all endpoints have descriptions.""" + with patch.dict(os.environ, { + 'COGNITO_USER_POOL_ID': 'test-pool', + 'COGNITO_CLIENT_ID': 'test-client', + 'COGNITO_REGION': 'us-east-1' + }): + with TestClient(workerfacing_app) as client: + response = client.get("/openapi.json") + openapi_spec = response.json() + paths = openapi_spec["paths"] + + for path, methods in paths.items(): + for method, spec in methods.items(): + assert "description" in spec, f"Endpoint {method.upper()} {path} missing description" + assert spec["description"], f"Endpoint {method.upper()} {path} has empty description" + + +def test_endpoints_have_responses(): + """Test that all endpoints have responses defined.""" + with patch.dict(os.environ, { + 'COGNITO_USER_POOL_ID': 'test-pool', + 'COGNITO_CLIENT_ID': 'test-client', + 'COGNITO_REGION': 'us-east-1' + }): + with TestClient(workerfacing_app) as client: + response = client.get("/openapi.json") + openapi_spec = response.json() + paths = openapi_spec["paths"] + + for path, methods in paths.items(): + for method, spec in methods.items(): + assert "responses" in spec, f"Endpoint {method.upper()} {path} missing responses" + responses = spec["responses"] + assert len(responses) > 0, f"Endpoint {method.upper()} {path} has no response codes defined" + + # Check for success status codes + success_codes = [code for code in responses.keys() if code.startswith("2")] + assert len(success_codes) > 0, f"Endpoint {method.upper()} {path} has no success response codes" + + +def test_schemas_have_examples(): + """Test that key schemas have examples for OpenAPI documentation.""" + with patch.dict(os.environ, { + 'COGNITO_USER_POOL_ID': 'test-pool', + 'COGNITO_CLIENT_ID': 'test-client', + 'COGNITO_REGION': 'us-east-1' + }): + with TestClient(workerfacing_app) as client: + response = client.get("/openapi.json") + openapi_spec = response.json() + components = openapi_spec.get("components", {}) + schemas = components.get("schemas", {}) + + # Key schemas that should have examples + schema_examples_to_check = [ + "FileHTTPRequest", + "HardwareSpecs", + "MetaSpecs", + "AppSpecs", + "HandlerSpecs", + "PathsUploadSpecs", + "SubmittedJob", + "WelcomeMessage" + ] + + for schema_name in schema_examples_to_check: + if schema_name in schemas: + schema = schemas[schema_name] + properties = schema.get("properties", {}) + + # Check if any properties have examples + has_examples = any("example" in prop for prop in properties.values()) + assert has_examples, f"Schema {schema_name} should have examples in its properties" + + +def test_root_endpoint_specification(): + """Test that the root endpoint has proper API specification.""" + with patch.dict(os.environ, { + 'COGNITO_USER_POOL_ID': 'test-pool', + 'COGNITO_CLIENT_ID': 'test-client', + 'COGNITO_REGION': 'us-east-1' + }): + with TestClient(workerfacing_app) as client: + response = client.get("/openapi.json") + openapi_spec = response.json() + + root_spec = openapi_spec["paths"]["/"]["get"] + + # Check all required fields are present + assert "description" in root_spec + assert "responses" in root_spec + assert "200" in root_spec["responses"] + assert "tags" in root_spec + + # Test the actual endpoint response + root_response = client.get("/") + assert root_response.status_code == 200 + data = root_response.json() + assert "message" in data + assert data["message"] == "Welcome to the DECODE OpenCloud Worker-facing API" + + +if __name__ == "__main__": + # Can run directly for quick testing + test_openapi_schema_structure() + test_endpoints_have_descriptions() + test_endpoints_have_responses() + test_schemas_have_examples() + test_root_endpoint_specification() + print("All OpenAPI specification tests passed!") \ No newline at end of file diff --git a/workerfacing_api/endpoints/access.py b/workerfacing_api/endpoints/access.py index 2130ea3..8c31912 100644 --- a/workerfacing_api/endpoints/access.py +++ b/workerfacing_api/endpoints/access.py @@ -18,6 +18,10 @@ class AccessType(enum.Enum): @router.get( "/access_info", response_model=dict[AccessType, dict[str, str | None]], + status_code=200, + responses={ + 200: {"description": "Access information retrieved successfully"}, + }, description="Get information about where API users should authenticate.", ) def get_access_info() -> dict[AccessType, dict[str, str | None]]: diff --git a/workerfacing_api/endpoints/files.py b/workerfacing_api/endpoints/files.py index ef8df50..573c921 100644 --- a/workerfacing_api/endpoints/files.py +++ b/workerfacing_api/endpoints/files.py @@ -13,6 +13,12 @@ @router.get( "/files/{file_id:path}/download", response_class=FileResponse, + status_code=200, + responses={ + 200: {"description": "File downloaded successfully"}, + 404: {"description": "File not found"}, + 403: {"description": "Permission denied"}, + }, description="Download a file from the filesystem.", ) async def download_file( @@ -29,6 +35,12 @@ async def download_file( @router.get( "/files/{file_id:path}/url", response_model=FileHTTPRequest, + status_code=200, + responses={ + 200: {"description": "File download URL retrieved successfully"}, + 404: {"description": "File not found"}, + 403: {"description": "Permission denied"}, + }, description="Get request parameters to download a file from the filesystem.", ) async def get_download_presigned_url( diff --git a/workerfacing_api/endpoints/jobs.py b/workerfacing_api/endpoints/jobs.py index 06eea1f..096d669 100644 --- a/workerfacing_api/endpoints/jobs.py +++ b/workerfacing_api/endpoints/jobs.py @@ -25,6 +25,7 @@ JobSpecs, ) from workerfacing_api.schemas.rds_models import JobStates, QueuedJob +from workerfacing_api.schemas.responses import UploadSuccess router = APIRouter() @@ -32,6 +33,11 @@ @router.get( "/jobs", response_model=dict[int, JobSpecs], + status_code=200, + responses={ + 200: {"description": "Jobs retrieved successfully"}, + 500: {"description": "Internal server error"}, + }, tags=["Jobs"], description="Pull jobs from the queue", ) @@ -85,6 +91,11 @@ async def get_jobs( "/jobs/{job_id}/status", tags=["Jobs"], response_model=JobStates, + status_code=200, + responses={ + 200: {"description": "Job status retrieved successfully"}, + 404: {"description": "Job not found"}, + }, description="Get the status of a job", ) async def get_job_status( @@ -100,6 +111,10 @@ async def get_job_status( "/jobs/{job_id}/status", tags=["Jobs"], status_code=httpstatus.HTTP_204_NO_CONTENT, + responses={ + 204: {"description": "Job status updated successfully"}, + 404: {"description": "Job not found or not assigned to this worker"}, + }, description="Update the status of a job (or ping for keep-alive)", ) async def put_job_status( @@ -132,6 +147,12 @@ def _upload_path(job: QueuedJob, type: UploadType, path: str) -> str: @router.post( "/jobs/{job_id}/files/upload", status_code=httpstatus.HTTP_201_CREATED, + response_model=UploadSuccess, + responses={ + 201: {"description": "File uploaded successfully"}, + 404: {"description": "Job not found"}, + 403: {"description": "Permission denied"}, + }, tags=["Files"], description="Upload a file to the job's output, log or artifact directory", ) @@ -143,14 +164,15 @@ async def upload_file( file: UploadFile = File(...), filesystem: FileSystem = Depends(filesystem_dep), queue: RDSJobQueue = Depends(queue_dep), -) -> None: +) -> UploadSuccess: try: job = queue.get_job(job_id, hostname=request.state.current_user.username) except ValueError: raise HTTPException(status_code=httpstatus.HTTP_404_NOT_FOUND) path = _upload_path(job, type, base_path) try: - return filesystem.post_file(file, path) + filesystem.post_file(file, path) + return UploadSuccess() except PermissionError as e: raise HTTPException(status_code=httpstatus.HTTP_403_FORBIDDEN, detail=str(e)) @@ -159,6 +181,11 @@ async def upload_file( "/jobs/{job_id}/files/url", status_code=httpstatus.HTTP_201_CREATED, response_model=FileHTTPRequest, + responses={ + 201: {"description": "Presigned upload URL generated successfully"}, + 404: {"description": "Job not found"}, + 403: {"description": "Permission denied"}, + }, tags=["Files"], description="Get a presigned URL to upload a file to the job's output, log or artifact directory", ) diff --git a/workerfacing_api/endpoints/jobs_post.py b/workerfacing_api/endpoints/jobs_post.py index aa86a47..5a17960 100644 --- a/workerfacing_api/endpoints/jobs_post.py +++ b/workerfacing_api/endpoints/jobs_post.py @@ -11,6 +11,10 @@ "/_jobs", status_code=status.HTTP_201_CREATED, response_model=SubmittedJob, + responses={ + 201: {"description": "Job submitted successfully"}, + 400: {"description": "Invalid job data"}, + }, description="Submit a job to the queue (private internal endpoint).", ) async def post_job( diff --git a/workerfacing_api/main.py b/workerfacing_api/main.py index 9c851d1..0affaeb 100644 --- a/workerfacing_api/main.py +++ b/workerfacing_api/main.py @@ -6,6 +6,7 @@ from workerfacing_api import dependencies, settings, tags from workerfacing_api.endpoints import access, files, jobs, jobs_post +from workerfacing_api.schemas.responses import WelcomeMessage workerfacing_app = FastAPI(openapi_tags=tags.tags_metadata) @@ -45,6 +46,15 @@ async def find_failed_jobs() -> dict[str, int]: return {"n_retry": 0, "n_fail": 0} -@workerfacing_app.get("/") -async def root() -> dict[str, str]: - return {"message": "Welcome to the DECODE OpenCloud Worker-facing API"} +@workerfacing_app.get( + "/", + response_model=WelcomeMessage, + status_code=200, + responses={ + 200: {"description": "Welcome message", "model": WelcomeMessage}, + }, + description="Root endpoint that returns a welcome message for the DECODE OpenCloud Worker-facing API", + tags=["Root"], +) +async def root() -> WelcomeMessage: + return WelcomeMessage(message="Welcome to the DECODE OpenCloud Worker-facing API") diff --git a/workerfacing_api/schemas/files.py b/workerfacing_api/schemas/files.py index 130c0bd..fee8dc5 100644 --- a/workerfacing_api/schemas/files.py +++ b/workerfacing_api/schemas/files.py @@ -1,8 +1,14 @@ -from pydantic import BaseModel +from pydantic import BaseModel, Field class FileHTTPRequest(BaseModel): - method: str - url: str - headers: dict[str, str | dict[str, str]] = {} - data: dict[str, str] = {} + method: str = Field(..., example="POST") + url: str = Field(..., example="https://example.com/upload") + headers: dict[str, str | dict[str, str]] = Field( + default={}, + example={"Content-Type": "multipart/form-data"} + ) + data: dict[str, str] = Field( + default={}, + example={"key": "test-upload-key"} + ) diff --git a/workerfacing_api/schemas/queue_jobs.py b/workerfacing_api/schemas/queue_jobs.py index 3a421c7..5d1921a 100644 --- a/workerfacing_api/schemas/queue_jobs.py +++ b/workerfacing_api/schemas/queue_jobs.py @@ -1,7 +1,7 @@ import enum from typing import Literal -from pydantic import BaseModel +from pydantic import BaseModel, Field class EnvironmentTypes(enum.Enum): @@ -11,33 +11,36 @@ class EnvironmentTypes(enum.Enum): class HardwareSpecs(BaseModel): - cpu_cores: int | None = None - memory: int | None = None - gpu_model: str | None = None - gpu_archi: str | None = None - gpu_mem: int | None = None + cpu_cores: int | None = Field(default=None, example=2) + memory: int | None = Field(default=None, example=4096) + gpu_model: str | None = Field(default=None, example="RTX3080") + gpu_archi: str | None = Field(default=None, example="ampere") + gpu_mem: int | None = Field(default=None, example=8192) class MetaSpecs(BaseModel): - job_id: int - date_created: str # iso format + job_id: int = Field(..., example=12345) + date_created: str = Field(..., example="2024-01-15T10:30:00Z") # iso format class Config: extra = "allow" class AppSpecs(BaseModel): - cmd: list[str] | None = None - env: dict[str, str] | None = None + cmd: list[str] | None = Field(default=None, example=["python", "main.py", "--config", "config.json"]) + env: dict[str, str] | None = Field(default=None, example={"CUDA_VISIBLE_DEVICES": "0", "PYTHONPATH": "/app"}) class HandlerSpecs(BaseModel): - image_url: str - image_name: str | None = None - image_version: str | None = None - entrypoint: str | None = None - files_down: dict[str, str] | None = None - files_up: dict[Literal["output", "log", "artifact"], str] | None = None + image_url: str = Field(..., example="ghcr.io/decode/decode-ml:latest") + image_name: str | None = Field(default=None, example="decode-ml") + image_version: str | None = Field(default=None, example="v1.2.0") + entrypoint: str | None = Field(default=None, example="/app/entrypoint.sh") + files_down: dict[str, str] | None = Field(default=None, example={"data": "/input/data"}) + files_up: dict[Literal["output", "log", "artifact"], str] | None = Field( + default=None, + example={"output": "/results", "log": "/logs", "artifact": "/artifacts"} + ) class JobSpecs(BaseModel): @@ -48,16 +51,16 @@ class JobSpecs(BaseModel): class PathsUploadSpecs(BaseModel): - output: str - log: str - artifact: str + output: str = Field(..., example="s3://decode-bucket/jobs/12345/output") + log: str = Field(..., example="s3://decode-bucket/jobs/12345/log") + artifact: str = Field(..., example="s3://decode-bucket/jobs/12345/artifact") class SubmittedJob(BaseModel): job: JobSpecs - environment: EnvironmentTypes = EnvironmentTypes.any - group: str | None = None - priority: int = 5 + environment: EnvironmentTypes = Field(default=EnvironmentTypes.any, example=EnvironmentTypes.cloud) + group: str | None = Field(default=None, example="gpu-group") + priority: int = Field(default=5, example=7) paths_upload: PathsUploadSpecs diff --git a/workerfacing_api/schemas/responses.py b/workerfacing_api/schemas/responses.py new file mode 100644 index 0000000..d99174c --- /dev/null +++ b/workerfacing_api/schemas/responses.py @@ -0,0 +1,13 @@ +from pydantic import BaseModel, Field + + +class WelcomeMessage(BaseModel): + message: str = Field(..., example="Welcome to the DECODE OpenCloud Worker-facing API") + + +class HTTPErrorDetail(BaseModel): + detail: str = Field(..., example="Resource not found") + + +class UploadSuccess(BaseModel): + message: str = Field(default="File uploaded successfully", example="File uploaded successfully") \ No newline at end of file