feat: add middleware management OpenAPI Spec #196
Conversation
enAPI spec with 7 endpoints and FOCA integration
Reviewer's GuideAdds a new OpenAPI 3.0 specification for runtime middleware management and wires it into FOCA/Connexion configuration and project docs, without yet implementing controller logic. Sequence diagram for adding a new middleware via the OpenAPI specsequenceDiagram
actor Admin
participant Client as API_client
participant Connexion as FOCA_Connexion
participant Controller as MiddlewaresController
participant DB as MongoDB_middlewares
Admin->>Client: Prepare MiddlewareCreate_payload
Client->>Connexion: POST /ga4gh/tes/v1/middlewares
Connexion->>Connexion: Validate_request_against_OpenAPI
Connexion->>Controller: addMiddleware(MiddlewareCreate)
Controller->>DB: Query existing_middlewares_for_order
DB-->>Controller: Existing_middlewares_with_orders
Controller->>Controller: Compute_final_order_and_shift_stack
Controller->>DB: Insert new_middleware_document
DB-->>Controller: Insert_result_with_id
Controller-->>Connexion: 201 MiddlewareCreateResponse
Connexion-->>Client: HTTP_201_with_JSON_body
Client-->>Admin: Show_created_middleware_id_and_order
Class diagram for middleware management OpenAPI schemasclassDiagram
class MiddlewareConfig {
string _id
string name
string class_path_string
string[] class_path_group
int order
string source
string github_url
object config
boolean enabled
string created_at
string updated_at
}
class MiddlewareCreate {
string name
string class_path_string
string[] class_path_group
int order
string github_url
object config
boolean enabled
}
class MiddlewareUpdate {
string name
int order
object config
boolean enabled
}
class MiddlewareList {
MiddlewareConfig[] middlewares
int total
int limit
int offset
}
class MiddlewareCreateResponse {
string _id
int order
string message
}
class MiddlewareOrder {
string[] ordered_ids
}
class ValidationRequest {
string class_path
string github_url
string code
}
class ValidationErrorItem {
int line
int column
string message
string severity
}
class ValidationWarningItem {
int line
string message
string severity
}
class ValidationResponse {
boolean valid
string message
ValidationErrorItem[] errors
ValidationWarningItem[] warnings
}
class ErrorResponse {
string error
string message
object details
string timestamp
}
MiddlewareList "1" --> "*" MiddlewareConfig : contains
MiddlewareCreateResponse --> MiddlewareConfig : _id_and_order_align_with
MiddlewareOrder "1" --> "*" MiddlewareConfig : orders
ValidationResponse "1" --> "*" ValidationErrorItem : has
ValidationResponse "1" --> "*" ValidationWarningItem : has
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
MiddlewareCreateResponseschema only exposes_id,order, andmessage, but the docs indocs/middleware.mdstate that it returns the full middleware object; consider either expanding the response schema (e.g., embedMiddlewareConfig) or updating the docs to reflect the actual payload to avoid confusion for API consumers. - In
ValidationRequest, theclass_pathfield is described as "Class path or code to validate" while a separatecodefield is also defined; it would be clearer to tighten the descriptions and possibly add explicit rules (e.g., mutual exclusivity or precedence) so clients know exactly how these fields should be used together.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `MiddlewareCreateResponse` schema only exposes `_id`, `order`, and `message`, but the docs in `docs/middleware.md` state that it returns the full middleware object; consider either expanding the response schema (e.g., embed `MiddlewareConfig`) or updating the docs to reflect the actual payload to avoid confusion for API consumers.
- In `ValidationRequest`, the `class_path` field is described as "Class path or code to validate" while a separate `code` field is also defined; it would be clearer to tighten the descriptions and possibly add explicit rules (e.g., mutual exclusivity or precedence) so clients know exactly how these fields should be used together.
## Individual Comments
### Comment 1
<location> `pro_tes/config.yaml:82` </location>
<code_context>
+ - api/middleware_management.yaml
+ add_operation_fields:
+ x-openapi-router-controller: pro_tes.api.middlewares.controllers
+ disable_auth: True
+ connexion:
+ strict_validation: True
</code_context>
<issue_to_address>
**🚨 issue (security):** Exposing middleware management without auth looks risky for production use.
Since this endpoint can change the middleware stack at runtime, keeping `disable_auth: True` lets anyone with network access reorder/enable/disable/add middlewares. Unless this is strictly limited to a trusted/debug environment, this should require authz (or be configurable per deployment) to avoid a straightforward privilege escalation path.
</issue_to_address>
### Comment 2
<location> `pro_tes/api/middleware_management.yaml:572-358` </location>
<code_context>
+ - "507f1f77bcf86cd799439012"
+ - "507f1f77bcf86cd799439013"
+
+ ValidationRequest:
+ type: object
+ description: Request body for validating middleware code
+ required:
+ - class_path
+ properties:
</code_context>
<issue_to_address>
**issue (bug_risk):** ValidationRequest requires class_path even when validating raw code, which conflicts with the description.
The schema contradicts the docs: `code` is documented as an alternative to `class_path`, but `class_path` is required. This blocks use cases where clients submit only raw code or only a `github_url`. Please adjust the schema (e.g., with `oneOf`/`anyOf` to require at least one of `class_path`, `github_url`, or `code`, or by making `class_path` optional and clarifying the validation rules) so the spec matches the intended behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive OpenAPI 3.0 specification for middleware management functionality in proTES, enabling dynamic management of middleware components at runtime. This is the first subtask in a multi-part implementation following a design-first approach, focusing solely on the API specification and documentation without implementing the actual controller logic.
Changes:
- Added complete OpenAPI 3.0 specification defining 7 REST endpoints for middleware CRUD operations, reordering, and validation
- Configured FOCA integration with new middleware management API routes and MongoDB collection
- Added comprehensive documentation describing the API design, endpoints, data models, and implementation plan
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
| pro_tes/api/middleware_management.yaml | New OpenAPI 3.0 specification defining middleware management API with 7 endpoints and 9 schemas |
| pro_tes/config.yaml | Added FOCA integration for middleware API, database collection configuration, and routing setup |
| docs/middleware.md | Comprehensive documentation covering API design, implementation details, security considerations, and future work |
Comments suppressed due to low confidence (1)
docs/middleware.md:210
- The changelog date "2026-01-24" is in the future. This should be corrected to use the actual date when this work was completed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| description: | | ||
| Retrieve all configured middlewares with their order, metadata, and status. | ||
| Results are sorted by execution order (ascending) by default. | ||
| operationId: listMiddlewares |
There was a problem hiding this comment.
The operationId naming convention is inconsistent with the existing TES API. The TES API uses PascalCase (GetServiceInfo, CreateTask, GetTask, CancelTask, ListTasks) while the middleware API uses camelCase (listMiddlewares, addMiddleware, getMiddleware, updateMiddleware, deleteMiddleware, reorderMiddlewares, validateMiddleware). For consistency with the existing GA4GH TES API in this codebase, consider using PascalCase for all operationIds in the middleware API.
| local class paths or fetched from GitHub repositories. If order is not specified, | ||
| the middleware is appended to the end of the stack. If order is specified, | ||
| existing middlewares at that position or higher are shifted up by one. | ||
| operationId: addMiddleware |
There was a problem hiding this comment.
The operationId naming convention is inconsistent with the existing TES API. Consider using PascalCase (AddMiddleware instead of addMiddleware) to match the pattern used in the existing API (e.g., CreateTask, GetTask, etc. in pro_tes/api/9e9c5aa.task_execution_service.openapi.yaml).
| get: | ||
| summary: Get middleware details | ||
| description: Retrieve detailed information about a specific middleware by ID | ||
| operationId: getMiddleware |
There was a problem hiding this comment.
The operationId naming convention is inconsistent with the existing TES API. Consider using PascalCase (GetMiddleware instead of getMiddleware) to match the pattern used in the existing API.
| description: | | ||
| Remove a middleware from the execution stack. By default performs soft delete | ||
| (sets enabled=false). Use force=true query parameter for hard deletion. | ||
| operationId: deleteMiddleware |
There was a problem hiding this comment.
The operationId naming convention is inconsistent with the existing TES API. Consider using PascalCase (DeleteMiddleware instead of deleteMiddleware) to match the pattern used in the existing API.
| ValidationRequest: | ||
| type: object | ||
| description: Request body for validating middleware code | ||
| required: | ||
| - class_path | ||
| properties: | ||
| class_path: | ||
| type: string | ||
| description: Class path or code to validate | ||
| example: "pro_tes.plugins.middlewares.task_distribution.distance.TaskDistributionDistance" | ||
| github_url: | ||
| type: string | ||
| description: GitHub URL for fetching middleware code | ||
| nullable: true | ||
| pattern: '^https://raw\.githubusercontent\.com/.+\.py$' | ||
| example: "https://raw.githubusercontent.com/user/repo/main/middleware.py" | ||
| code: | ||
| type: string | ||
| description: Raw Python code to validate (alternative to class_path) | ||
| nullable: true |
There was a problem hiding this comment.
The ValidationRequest schema marks 'class_path' as required but also provides 'code' as an "alternative to class_path" (line 590). This is contradictory - if 'code' is truly an alternative, then 'class_path' should not be required. Consider using oneOf to express that either 'class_path' or 'code' is required, but not both, or clarify the validation logic in the description.
| middlewares: | ||
| indexes: | ||
| - keys: | ||
| name: 1 |
There was a problem hiding this comment.
The database index is created on the 'name' field, but the OpenAPI spec indicates that both 'name' and 'class_path' should be unique (based on the 400 error description mentioning 'duplicate name/class_path'). Consider adding indexes on both 'name' and 'class_path' fields with unique constraints to enforce this at the database level, or clarify whether only 'name' needs to be unique.
| name: 1 | |
| name: 1 | |
| options: | |
| "unique": True | |
| - keys: | |
| class_path: 1 | |
| options: | |
| "unique": True |
| ## File Structure | ||
| ``` | ||
| pro_tes/ | ||
| ├── api/ | ||
| │ └── middleware_management.yaml (OpenAPI specification) | ||
| └── config.yaml (FOCA integration) | ||
|
|
||
| docs/ | ||
| └── middleware.md (This file) | ||
| ``` | ||
|
|
||
| ## Documentation Deliverables | ||
|
|
||
| **API Documentation**: Comprehensive guide with request/response examples for each endpoint. Includes curl commands, common use cases, and troubleshooting tips. | ||
|
|
||
| **Architecture Decision Record**: Documents twelve major design decisions with rationale, alternatives considered, and consequences. Serves as reference for future development. | ||
|
|
||
| **Postman Collection**: Ready-to-use collection with fourteen pre-configured requests. Includes environment variables, test scripts, and example data for all scenarios. | ||
|
|
||
| **Quick Reference**: Single-page reference with essential endpoints, parameters, and response codes. Designed for daily development use. | ||
|
|
||
| **Validation Script**: Bash script that validates OpenAPI syntax using multiple tools. Checks for common errors like undefined schema references and invalid endpoint definitions. | ||
|
|
||
| ## Testing Approach | ||
|
|
||
| This subtask focuses on specification validation rather than runtime testing since no executable code is implemented yet. Validation performed: | ||
|
|
||
| **YAML Syntax**: Verified file parses correctly as valid YAML without syntax errors. | ||
|
|
||
| **OpenAPI Compliance**: Confirmed specification follows OpenAPI 3.0 standards including required fields, valid schema definitions, and proper reference resolution. | ||
|
|
There was a problem hiding this comment.
The documentation references several files that are not included in this PR: 'docs/api/middleware_management.md', 'middleware_management.postman_collection.json', 'QUICK_REFERENCE.md', 'docs/architecture/middleware_api_design.md', and 'scripts/validate_openapi.sh'. Either these files should be included in this PR, or the documentation should clarify that these are planned deliverables for future PRs. The "Documentation Deliverables" section also describes these files in detail, which creates confusion about what is actually delivered in this subtask.
| ErrorResponse: | ||
| type: object | ||
| description: Standard error response | ||
| required: | ||
| - error | ||
| - message | ||
| properties: | ||
| error: | ||
| type: string | ||
| description: Error type/code | ||
| example: "MiddlewareNotFound" | ||
| message: | ||
| type: string | ||
| description: Human-readable error message | ||
| example: "Middleware with ID '507f1f77bcf86cd799439011' not found" | ||
| details: | ||
| type: object | ||
| description: Additional error details | ||
| nullable: true | ||
| additionalProperties: true | ||
| timestamp: | ||
| type: string | ||
| format: date-time | ||
| description: Error timestamp | ||
| example: "2026-01-24T10:30:00Z" |
There was a problem hiding this comment.
The ErrorResponse schema is inconsistent with the existing error handling convention. The FOCA exception configuration (pro_tes/config.yaml:120) requires error responses to have 'message' and 'code' fields, but the ErrorResponse schema defines 'error', 'message', 'details', and 'timestamp' fields without a 'code' field. For consistency with the existing error handling (see pro_tes/exceptions.py:52-121), the ErrorResponse schema should include a 'code' field containing the HTTP status code, and consider using 'code' instead of or in addition to 'error' for the error type.
pro_tes/config.yaml
Outdated
| - "pro_tes.plugins.middlewares.task_distribution.random.TaskDistributionRandom" | ||
|
|
There was a problem hiding this comment.
Trailing whitespace on empty line. Remove the trailing spaces for consistency with code style.
| - "pro_tes.plugins.middlewares.task_distribution.random.TaskDistributionRandom" | |
| - "pro_tes.plugins.middlewares.task_distribution.random.TaskDistributionRandom" |
docs/middleware.md
Outdated
| ```yaml | ||
| specs: | ||
| - path: | ||
| - api/middleware_management.yaml | ||
| add_operation_fields: | ||
| x-openapi-router-controller: pro_tes.api.middlewares.controllers | ||
| disable_auth: True |
There was a problem hiding this comment.
The FOCA configuration for the middleware management API is setting disable_auth: True, which exposes all middleware management endpoints without any authentication. In any non-isolated environment this allows an unauthenticated attacker to list, create, modify, or delete middlewares, potentially leading to arbitrary code execution or unauthorized changes to workflow behavior. Require authentication for these endpoints (e.g., by wiring them into your existing security scheme instead of using disable_auth) and, if needed, restrict unauthenticated access to development-only configs that are not deployed to production.
keshxvdayal
force-pushed
the
feature/middleware-api-spec
branch
from
b3118e8 to
5025c12
Compare
Detailed Implementation Plan
Create OpenAPI Specification Document
data/specs/middleware_management.yaml/ga4gh/tes/v1/middlewares)GET /middlewares- List all configured middlewaresPOST /middlewares- Add new middleware (local or from GitHub)GET /middlewares/{middleware_id}- Get middleware detailsPUT /middlewares/{middleware_id}- Update middleware configurationDELETE /middlewares/{middleware_id}- Remove middlewarePUT /middlewares/reorder- Reorder middleware stackPOST /middlewares/validate- Validate middleware code without addingMiddlewareConfig- Full middleware configuration objectMiddlewareCreate- Request body for creating middlewareMiddlewareUpdate- Request body for updating middlewareMiddlewareList- Array response with pagination supportMiddlewareOrder- Order configuration for reorderingValidationRequest- Code validation requestErrorResponse- Standard error formatIntegrate with FOCA Configuration
pro_tes/config.yamlto reference the new specWrite API Documentation
docs/api/middleware_management.mdValidation and Review
openapi-generator validateSummary by Sourcery
Add an OpenAPI specification and configuration wiring for a new runtime middleware management API in proTES.
New Features:
Enhancements:
Documentation: