feat: implement middleware management controllers - #199
Open
keshxvdayal wants to merge 11 commits into
Open
Conversation
Reviewer's GuideImplements a new middleware management controller layer backed by the FOCA MongoDB config, exposing CRUD and reordering endpoints plus basic pagination/sorting, and documents its implementation status. Sequence diagram for ReorderMiddlewares runtime flowsequenceDiagram
actor Client
participant Connexion
participant Controllers as ReorderMiddlewares
participant FlaskApp as current_app
participant FOCA as foca_db
participant Mongo as middlewares_collection
Client->>Connexion: PUT /middlewares/reorder
Connexion->>Controllers: ReorderMiddlewares(body)
Controllers->>FlaskApp: _collection()
FlaskApp->>FOCA: config.foca.db.dbs[taskStore]
FOCA->>Mongo: collections[middlewares]
Mongo-->>Controllers: collection_client
Controllers->>Mongo: find({}, {_id})
Mongo-->>Controllers: existing_ids
Controllers-->>Controllers: [validate ordered_ids]
loop for each id in ordered_ids
Controllers->>Mongo: update_one({_id: oid}, {$set: {order, updated_at}})
Controllers->>Mongo: find_one({_id: oid}, projection)
Mongo-->>Controllers: middleware_doc
Controllers-->>Controllers: _doc_to_response(middleware_doc)
end
Controllers-->>Connexion: {message, middlewares}
Connexion-->>Client: 200 OK + body
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 1 issue, and left some high level feedback:
- In
ListMiddlewares, consider validatingsort_byandsort_orderagainst a small allowlist of supported fields/directions so a bad query parameter doesn’t propagate as an unhandled database error. - In
ReorderMiddlewares, the implementation performs oneupdate_oneand onefind_oneper middleware; you could reduce this N+1 pattern by using a bulk update and then a single sorted query to return the updated middlewares in the new order. - The timestamp formatting logic (
datetime.now(...).replace(...).isoformat().replace(...)) is duplicated in several functions; extracting a small helper (e.g.,utc_now_iso()) would make the code easier to maintain and avoid inconsistencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ListMiddlewares`, consider validating `sort_by` and `sort_order` against a small allowlist of supported fields/directions so a bad query parameter doesn’t propagate as an unhandled database error.
- In `ReorderMiddlewares`, the implementation performs one `update_one` and one `find_one` per middleware; you could reduce this N+1 pattern by using a bulk update and then a single sorted query to return the updated middlewares in the new order.
- The timestamp formatting logic (`datetime.now(...).replace(...).isoformat().replace(...)`) is duplicated in several functions; extracting a small helper (e.g., `utc_now_iso()`) would make the code easier to maintain and avoid inconsistencies.
## Individual Comments
### Comment 1
<location path="pro_tes/api/middlewares/controllers.py" line_range="174-175" />
<code_context>
+
+ coll = _collection()
+ # Fetch existing ids
+ existing = list(coll.find({}, {"_id": True}))
+ existing_ids = {str(d["_id"]): d for d in existing}
+
+ if set(ordered_ids) != set(existing_ids.keys()):
</code_context>
<issue_to_address>
**suggestion:** The `existing_ids` mapping stores full docs but only keys are used; also the error message may not clearly surface invalid IDs.
Since only the keys are used later, `existing_ids` can just be a `set` of string IDs instead of a mapping to full documents. Also, when `ordered_ids` contains syntactically invalid IDs, the initial `set(ordered_ids) != set(existing_ids.keys())` check will fail with a generic mismatch message, and the more specific `Invalid id in ordered_ids` case is only reached later. If you care about clearer client feedback, consider validating that each `ordered_id` is a valid `ObjectId` string before the set comparison.
Suggested implementation:
```python
coll = _collection()
# Fetch existing ids
existing = list(coll.find({}, {"_id": True}))
existing_ids = {str(d["_id"]) for d in existing}
# Validate that all provided ids are syntactically valid ObjectId strings
for mid in ordered_ids:
try:
ObjectId(mid)
except Exception:
raise BadRequest(f"Invalid id in ordered_ids: {mid}")
if set(ordered_ids) != existing_ids:
raise BadRequest("`ordered_ids` must contain exactly all middleware ids")
```
```python
# Apply new order
updated_docs: List[Dict[str, Any]] = []
now = datetime.now(timezone.utc).replace(microsecond=0).isoformat().replace("+00:00", "Z")
for idx, mid in enumerate(ordered_ids):
# At this point, all ids are known to be valid ObjectId strings
oid = ObjectId(mid)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+174
to
+175
| existing = list(coll.find({}, {"_id": True})) | ||
| existing_ids = {str(d["_id"]): d for d in existing} |
There was a problem hiding this comment.
suggestion: The existing_ids mapping stores full docs but only keys are used; also the error message may not clearly surface invalid IDs.
Since only the keys are used later, existing_ids can just be a set of string IDs instead of a mapping to full documents. Also, when ordered_ids contains syntactically invalid IDs, the initial set(ordered_ids) != set(existing_ids.keys()) check will fail with a generic mismatch message, and the more specific Invalid id in ordered_ids case is only reached later. If you care about clearer client feedback, consider validating that each ordered_id is a valid ObjectId string before the set comparison.
Suggested implementation:
coll = _collection()
# Fetch existing ids
existing = list(coll.find({}, {"_id": True}))
existing_ids = {str(d["_id"]) for d in existing}
# Validate that all provided ids are syntactically valid ObjectId strings
for mid in ordered_ids:
try:
ObjectId(mid)
except Exception:
raise BadRequest(f"Invalid id in ordered_ids: {mid}")
if set(ordered_ids) != existing_ids:
raise BadRequest("`ordered_ids` must contain exactly all middleware ids") # Apply new order
updated_docs: List[Dict[str, Any]] = []
now = datetime.now(timezone.utc).replace(microsecond=0).isoformat().replace("+00:00", "Z")
for idx, mid in enumerate(ordered_ids):
# At this point, all ids are known to be valid ObjectId strings
oid = ObjectId(mid)There was a problem hiding this comment.
Pull request overview
Adds a new FOCA/Connexion controller module to expose the Middleware Management API (CRUD + reorder) backed by the taskStore.middlewares MongoDB collection, and updates docs to reflect the new API layer.
Changes:
- Implement
pro_tes.api.middlewares.controllerswith handlers for list/create/get/update/delete/reorder. - Add
pro_tes.api.middlewarespackage initializer. - Update
docs/middleware.mdto describe implementation status and how to validate.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| pro_tes/api/middlewares/controllers.py | New Mongo-backed controller functions implementing middleware management endpoints. |
| pro_tes/api/middlewares/init.py | Declares the middlewares API package exports. |
| docs/middleware.md | Documents controller implementation status and validation guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+94
to
+99
| # Insert | ||
| try: | ||
| res = coll.insert_one(doc) | ||
| except Exception as exc: | ||
| logger.exception("Failed to insert middleware") | ||
| raise |
Comment on lines
+136
to
+146
| update_fields["updated_at"] = datetime.now(timezone.utc).replace(microsecond=0).isoformat().replace("+00:00", "Z") | ||
| coll = _collection() | ||
| updated = coll.find_one_and_update( | ||
| {"_id": oid}, | ||
| {"$set": update_fields}, | ||
| projection={"_id": True, "name": True, "source": True, "order": True, "config": True, "created_at": True, "updated_at": True}, | ||
| return_document=True, | ||
| ) | ||
| if updated is None: | ||
| raise NotFound(f"Middleware with ID '{middleware_id}' not found") | ||
| return _doc_to_response(updated) |
Comment on lines
+35
to
+39
| def ListMiddlewares(page_size: int = 50, | ||
| page: int = 0, | ||
| sort_by: str = "order", | ||
| sort_order: str = "asc", | ||
| source: Optional[str] = None) -> Dict[str, Any]: |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Keshav Dayal <115068840+keshxvdayal@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Keshav Dayal <115068840+keshxvdayal@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Keshav Dayal <115068840+keshxvdayal@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Keshav Dayal <115068840+keshxvdayal@users.noreply.github.com>
keshxvdayal
changed the title
keshxvdayal
changed the title
Signed-off-by: Keshav Dayal <115068840+keshxvdayal@users.noreply.github.com>
Signed-off-by: Keshav Dayal <115068840+keshxvdayal@users.noreply.github.com>
uniqueg
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (7)
pro_tes/api/middlewares/controllers.py:90
- This PR introduces a new controller surface (CRUD + reorder) but does not add any unit tests for these endpoints, despite the PR description stating mongomock-based controller tests would be added. Adding tests (or updating the PR description) would help prevent regressions, especially with
validate_responses: trueenabled for this spec.
def ListMiddlewares(page_size: int = 50,
page: int = 0,
sort_by: str = "order",
sort_order: str = "asc",
source: Optional[str] = None) -> Dict[str, Any]:
"""List middlewares with pagination and sorting."""
docs/middleware.md:44
- The documentation notes that controller unit tests are “expected under tests/unitTest/pro_tes/api/middlewares/”, but no such tests are included in this PR. Either add the referenced tests or update this section to reflect the current state to avoid misleading readers.
## Implementation Status
The middleware management controller layer is now implemented in
[`pro_tes/api/middlewares/controllers.py`](../pro_tes/api/middlewares/controllers.py)
and wired through the OpenAPI configuration in
[`pro_tes/config.yaml`](../pro_tes/config.yaml).
Unit tests for the middleware management controllers are expected under
tests/unitTest/pro_tes/api/middlewares/ (e.g. test_controllers.py).
pro_tes/api/middlewares/controllers.py:253
middleware_idis converted toObjectIdforexclude_idbefore validating the ID format. Ifmiddleware_idis invalid, thisObjectId(...)call will raise outside thetry/exceptbelow, resulting in an unintended 500 instead of a 400.
if "name" in update_fields:
_validate_duplicates(
_collection(),
name=update_fields.get("name"),
class_path=None,
exclude_id=ObjectId(middleware_id) if middleware_id else None,
)
pro_tes/api/middlewares/controllers.py:101
sourcefiltering only matches documents wheresourceis an object (source.type). For fallback groups,sourceis an array per the OpenAPI schema, soGET /middlewares?source=...will silently exclude those entries. Consider adjusting the query to also match array sources (e.g.,$orwith$elemMatchonsource.type).
query: Dict[str, Any] = {}
if source:
query["source.type"] = source
pro_tes/api/middlewares/controllers.py:246
- The OpenAPI spec for
PATCH /middlewares/{middleware_id}defines a 409 Conflict for duplicate names, but_validate_duplicatesraisesBadRequest(400). Either adjust the controller to raise a 409-compatible exception (and ensure it’s mapped) or align the spec/status codes.
_validate_duplicates(
_collection(),
name=update_fields.get("name"),
class_path=None,
exclude_id=ObjectId(middleware_id) if middleware_id else None,
)
try:
pro_tes/api/middlewares/controllers.py:330
ReorderMiddlewaresappends_doc_to_response(doc)even iffind_onereturnsNone(which becomes{}), producing invalid response items when response validation is enabled. Consider assertingmatched_count == 1and/or raising if the document can’t be re-fetched, rather than returning empty objects.
coll.update_one(
{"_id": oid},
{"$set": {"order": idx, "updated_at": now}},
)
doc = coll.find_one(
{"_id": oid},
{
"_id": True,
"name": True,
"source": True,
"order": True,
"config": True,
"created_at": True,
"updated_at": True,
},
)
updated_docs.append(_doc_to_response(doc))
pro_tes/api/middlewares/controllers.py:65
_derive_nameis defined but unused in this module, which adds dead code and can confuse future maintenance. Either remove it or use it as the single name-derivation path (instead of duplicating derivation logic inAddMiddleware).
def _derive_name(body: Dict[str, Any]) -> Optional[str]:
if body.get("name"):
return body["name"]
source_val = body.get("source")
entry_point = None
if isinstance(source_val, dict):
entry_point = source_val.get("entry_point")
elif isinstance(source_val, list):
for item in source_val:
if isinstance(item, dict) and item.get("entry_point"):
entry_point = item["entry_point"]
break
if not entry_point:
return None
name = entry_point.split(".")[-1]
name = name.split(":")[-1]
return name
uniqueg
reviewed
Aug 18, 2026
uniqueg
left a comment
uniqueg
left a comment
Member
There was a problem hiding this comment.
Thanks @keshxvdayal!
A few comments:
Let's think about what we originally wanted to do here: We wanted to allow a (pro)TES client (e.g., the TES dashboard) to define the middlewares that are used for a specific TES job.
The middleware management doesn't really help us here, because we're creating a race condition:
- Request 1: Client sends one or more requests to set up the the desired middleware stack via the middleware controllers.
- Request 2: Client sends TES request, expecting the configured middlewares to be used.
But between calls (1) and (2), there may be other calls that change the middleware setup, and so it cannot be guaranteed that call (2) actually uses the desired middleware stack.
That being said, the middleware controllers may still be useful, as long as we add an extension to /tasks that allows setting the middleware stack directly. But then the question is: Wouldn't this make more sense within FOCA, so that all FOCA-based services benefit from middleware management? 🤔
But that brings up other problems: FOCA is hopelessly outdated. It still uses Connexion 2, which is in legacy maintenance mode, so it is not clear how much longer it will be supported. IMO, it doesn't make much sense to invest any considerable time into adding features into the current FOCA (or proTES, for that matter).
The issue is that migrating to Connexion 3 is a considerable undertaking. One of the changes is a migration from WSGI and Flask (Connexion 2) to ASGI and Starlette (Connexion 3). But Starlette comes with its own middleware engine (https://starlette.dev/middleware/), which uses standardized ASGI middlewares and is certainly better than whatever we would come up with.
I see three paths forward:
(1) We keep on working on the current proTES, but only focus on what we want to achieve, i.e., setting the middleware stack via the /tasks request (as a custom proTES extension of that endpoint).
(2) We invest the time into upgrading FOCA to Connexion 3 and make use of Starlette's middleware engine, update proTES accordingly, and then add what's necessary to make use of it for our purpose (including rewriting existing middlewares).
(3) We ditch FOCA altogether (most of the services that depend on it are not maintained anymore) and build proTES and all middlewares from scratch using a fast/modern web service (e.g., written in Golang).
Given that Starlette is reasonably fast, I would tend towards option (2), as it represents a reasonable middlegroumd wrt to speed, time commitment, maintainability etc.
But it's quite some work (though perhaps not all that much with the help of agentic AI)!
Let's discuss this on a call.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detailed Implementation Plan
Create Middleware Controller Module
Implement
pro_tes.api.middlewares.controllersto serve the middleware management API expected by FOCA/Connexion.Add Endpoint Handlers
Implement handlers for:
GET /middlewares- List configured middlewaresPOST /middlewares- Add a new middlewareGET /middlewares/{middleware_id}- Get middleware detailsPATCH /middlewares/{middleware_id}- Update middleware configurationDELETE /middlewares/{middleware_id}- Remove middlewarePUT /middlewares/reorder- Reorder the middleware stackConnect to FOCA Database Configuration
Use the existing FOCA database wiring in config.yaml so the controller layer reads and writes from the
taskStore.middlewarescollection.Implement API Behavior
Add the required runtime behavior for:
idfieldsAdd Unit Tests
Create controller tests using
mongomockto validate:Validate the Implementation
Run the middleware controller test suite to confirm the controller layer works as expected.
Document the Completion
Add a short status note to the middleware docs so the API documentation reflects that subtask 2 is implemented and verified.
Summary by Sourcery
Implement controller layer and routing for middleware management APIs backed by the taskStore.middlewares collection.
New Features:
Enhancements:
Documentation:
Tests: