feat: Implement middleware management controllers#199
Open
keshxvdayal wants to merge 9 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
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: