Skip to content

Commit 709c79f

Browse files
rakduttarakdutta1
rakdutta
and
rakdutta1
authored
Improved Error for edit gateway for Issue IBM#361 and Bug fixing of issue IBM#603 (IBM#648)
* gateway edit Signed-off-by: RAKHI DUTTA <[email protected]> * edit gateway Signed-off-by: RAKHI DUTTA <[email protected]> * edit gateway Signed-off-by: RAKHI DUTTA <[email protected]> * edit gateway Signed-off-by: RAKHI DUTTA <[email protected]> * edit gateway Signed-off-by: RAKHI DUTTA <[email protected]> * edit gateway Signed-off-by: RAKHI DUTTA <[email protected]> * test case update Signed-off-by: RAKHI DUTTA <[email protected]> * edit gateway Signed-off-by: RAKHI DUTTA <[email protected]> --------- Signed-off-by: RAKHI DUTTA <[email protected]> Co-authored-by: RAKHI DUTTA <[email protected]>
1 parent 8abdc3b commit 709c79f

File tree

5 files changed

+120
-37
lines changed

5 files changed

+120
-37
lines changed

‎mcpgateway/admin.py‎

Lines changed: 51 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -2389,7 +2389,7 @@ async def admin_edit_gateway(
23892389
request: Request,
23902390
db: Session = Depends(get_db),
23912391
user: str = Depends(require_auth),
2392-
) -> RedirectResponse:
2392+
) -> JSONResponse:
23932393
"""Edit a gateway via the admin UI.
23942394
23952395
Expects form fields:
@@ -2419,52 +2419,68 @@ async def admin_edit_gateway(
24192419
>>> gateway_id = "gateway-to-edit"
24202420
>>>
24212421
>>> # Happy path: Edit gateway successfully
2422-
>>> form_data_success = FormData([("name", "Updated Gateway"), ("url", "http://updated.com"), ("is_inactive_checked", "false"), ("auth_type", "basic")]) # Added auth_type
2422+
>>> form_data_success = FormData([
2423+
... ("name", "Updated Gateway"),
2424+
... ("url", "http://updated.com"),
2425+
... ("is_inactive_checked", "false"),
2426+
... ("auth_type", "basic"),
2427+
... ("auth_username", "user"),
2428+
... ("auth_password", "pass")
2429+
... ])
24232430
>>> mock_request_success = MagicMock(spec=Request, scope={"root_path": ""})
24242431
>>> mock_request_success.form = AsyncMock(return_value=form_data_success)
24252432
>>> original_update_gateway = gateway_service.update_gateway
24262433
>>> gateway_service.update_gateway = AsyncMock()
24272434
>>>
24282435
>>> async def test_admin_edit_gateway_success():
24292436
... response = await admin_edit_gateway(gateway_id, mock_request_success, mock_db, mock_user)
2430-
... return isinstance(response, RedirectResponse) and response.status_code == 303 and "/admin#gateways" in response.headers["location"]
2437+
... return isinstance(response, JSONResponse) and response.status_code == 200 and json.loads(response.body)["success"] is True
24312438
>>>
24322439
>>> asyncio.run(test_admin_edit_gateway_success())
24332440
True
24342441
>>>
2435-
>>> # Edge case: Edit gateway with inactive checkbox checked
2436-
>>> form_data_inactive = FormData([("name", "Inactive Edit"), ("url", "http://inactive.com"), ("is_inactive_checked", "true"), ("auth_type", "basic")]) # Added auth_type
2437-
>>> mock_request_inactive = MagicMock(spec=Request, scope={"root_path": "/api"})
2438-
>>> mock_request_inactive.form = AsyncMock(return_value=form_data_inactive)
2439-
>>>
2440-
>>> async def test_admin_edit_gateway_inactive_checked():
2441-
... response = await admin_edit_gateway(gateway_id, mock_request_inactive, mock_db, mock_user)
2442-
... return isinstance(response, RedirectResponse) and response.status_code == 303 and "/api/admin/?include_inactive=true#gateways" in response.headers["location"]
2443-
>>>
2444-
>>> asyncio.run(test_admin_edit_gateway_inactive_checked())
2445-
True
2446-
>>>
2442+
# >>> # Edge case: Edit gateway with inactive checkbox checked
2443+
# >>> form_data_inactive = FormData([("name", "Inactive Edit"), ("url", "http://inactive.com"), ("is_inactive_checked", "true"), ("auth_type", "basic"), ("auth_username", "user"),
2444+
# ... ("auth_password", "pass")]) # Added auth_type
2445+
# >>> mock_request_inactive = MagicMock(spec=Request, scope={"root_path": "/api"})
2446+
# >>> mock_request_inactive.form = AsyncMock(return_value=form_data_inactive)
2447+
# >>>
2448+
# >>> async def test_admin_edit_gateway_inactive_checked():
2449+
# ... response = await admin_edit_gateway(gateway_id, mock_request_inactive, mock_db, mock_user)
2450+
# ... return isinstance(response, RedirectResponse) and response.status_code == 303 and "/api/admin/?include_inactive=true#gateways" in response.headers["location"]
2451+
# >>>
2452+
# >>> asyncio.run(test_admin_edit_gateway_inactive_checked())
2453+
# True
2454+
# >>>
24472455
>>> # Error path: Simulate an exception during update
2448-
>>> form_data_error = FormData([("name", "Error Gateway"), ("url", "http://error.com"), ("auth_type", "basic")]) # Added auth_type
2456+
>>> form_data_error = FormData([("name", "Error Gateway"), ("url", "http://error.com"), ("auth_type", "basic"),("auth_username", "user"),
2457+
... ("auth_password", "pass")]) # Added auth_type
24492458
>>> mock_request_error = MagicMock(spec=Request, scope={"root_path": ""})
24502459
>>> mock_request_error.form = AsyncMock(return_value=form_data_error)
24512460
>>> gateway_service.update_gateway = AsyncMock(side_effect=Exception("Update failed"))
24522461
>>>
24532462
>>> async def test_admin_edit_gateway_exception():
24542463
... response = await admin_edit_gateway(gateway_id, mock_request_error, mock_db, mock_user)
2455-
... return isinstance(response, RedirectResponse) and response.status_code == 303 and "/admin#gateways" in response.headers["location"]
2464+
... return (
2465+
... isinstance(response, JSONResponse)
2466+
... and response.status_code == 500
2467+
... and json.loads(response.body)["success"] is False
2468+
... and "Update failed" in json.loads(response.body)["message"]
2469+
... )
24562470
>>>
24572471
>>> asyncio.run(test_admin_edit_gateway_exception())
24582472
True
24592473
>>>
24602474
>>> # Error path: Pydantic Validation Error (e.g., invalid URL format)
2461-
>>> form_data_validation_error = FormData([("name", "Bad URL Gateway"), ("url", "invalid-url"), ("auth_type", "basic")]) # Added auth_type
2475+
>>> form_data_validation_error = FormData([("name", "Bad URL Gateway"), ("url", "invalid-url"), ("auth_type", "basic"),("auth_username", "user"),
2476+
... ("auth_password", "pass")]) # Added auth_type
24622477
>>> mock_request_validation_error = MagicMock(spec=Request, scope={"root_path": ""})
24632478
>>> mock_request_validation_error.form = AsyncMock(return_value=form_data_validation_error)
24642479
>>>
24652480
>>> async def test_admin_edit_gateway_validation_error():
24662481
... response = await admin_edit_gateway(gateway_id, mock_request_validation_error, mock_db, mock_user)
2467-
... return isinstance(response, RedirectResponse) and response.status_code == 303 and "/admin#gateways" in response.headers["location"]
2482+
... body = json.loads(response.body.decode())
2483+
... return isinstance(response, JSONResponse) and response.status_code in (422,400) and body["success"] is False
24682484
>>>
24692485
>>> asyncio.run(test_admin_edit_gateway_validation_error())
24702486
True
@@ -2474,7 +2490,6 @@ async def admin_edit_gateway(
24742490
"""
24752491
logger.debug(f"User {user} is editing gateway ID {gateway_id}")
24762492
form = await request.form()
2477-
is_inactive_checked = form.get("is_inactive_checked", "false")
24782493
try:
24792494
gateway = GatewayUpdate( # Pydantic validation happens here
24802495
name=form.get("name"),
@@ -2489,18 +2504,22 @@ async def admin_edit_gateway(
24892504
auth_header_value=form.get("auth_header_value", None),
24902505
)
24912506
await gateway_service.update_gateway(db, gateway_id, gateway)
2492-
2493-
root_path = request.scope.get("root_path", "")
2494-
if is_inactive_checked.lower() == "true":
2495-
return RedirectResponse(f"{root_path}/admin/?include_inactive=true#gateways", status_code=303)
2496-
return RedirectResponse(f"{root_path}/admin#gateways", status_code=303)
2497-
except Exception as e: # Catch all exceptions including ValidationError for redirect
2498-
logger.error(f"Error editing gateway: {e}")
2499-
2500-
root_path = request.scope.get("root_path", "")
2501-
if is_inactive_checked.lower() == "true":
2502-
return RedirectResponse(f"{root_path}/admin/?include_inactive=true#gateways", status_code=303)
2503-
return RedirectResponse(f"{root_path}/admin#gateways", status_code=303)
2507+
return JSONResponse(
2508+
content={"message": "Gateway update successfully!", "success": True},
2509+
status_code=200,
2510+
)
2511+
except Exception as ex:
2512+
if isinstance(ex, GatewayConnectionError):
2513+
return JSONResponse(content={"message": str(ex), "success": False}, status_code=502)
2514+
if isinstance(ex, ValueError):
2515+
return JSONResponse(content={"message": str(ex), "success": False}, status_code=400)
2516+
if isinstance(ex, RuntimeError):
2517+
return JSONResponse(content={"message": str(ex), "success": False}, status_code=500)
2518+
if isinstance(ex, ValidationError):
2519+
return JSONResponse(content=ErrorFormatter.format_validation_error(ex), status_code=422)
2520+
if isinstance(ex, IntegrityError):
2521+
return JSONResponse(status_code=409, content=ErrorFormatter.format_database_error(ex))
2522+
return JSONResponse(content={"message": str(ex), "success": False}, status_code=500)
25042523

25052524

25062525
@admin_router.post("/gateways/{gateway_id}/delete")

‎mcpgateway/main.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1812,6 +1812,10 @@ async def register_gateway(
18121812
return JSONResponse(content={"message": "Gateway name already exists"}, status_code=400)
18131813
if isinstance(ex, RuntimeError):
18141814
return JSONResponse(content={"message": "Error during execution"}, status_code=500)
1815+
if isinstance(ex, ValidationError):
1816+
return JSONResponse(content=ErrorFormatter.format_validation_error(ex), status_code=422)
1817+
if isinstance(ex, IntegrityError):
1818+
return JSONResponse(status_code=409, content=ErrorFormatter.format_database_error(ex))
18151819
return JSONResponse(content={"message": "Unexpected error"}, status_code=500)
18161820

18171821

‎mcpgateway/services/gateway_service.py‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -530,6 +530,8 @@ async def update_gateway(self, db: Session, gateway_id: str, gateway_update: Gat
530530
GatewayNotFoundError: If gateway not found
531531
GatewayError: For other update errors
532532
GatewayNameConflictError: If gateway name conflict occurs
533+
IntegrityError: If there is a database integrity error
534+
ValidationError: If validation fails
533535
"""
534536
try:
535537
# Find gateway
@@ -602,7 +604,6 @@ async def update_gateway(self, db: Session, gateway_id: str, gateway_update: Gat
602604
auth_value=gateway.auth_value,
603605
)
604606
)
605-
606607
gateway.capabilities = capabilities
607608
gateway.tools = [tool for tool in gateway.tools if tool.original_name in new_tool_names] # keep only still-valid rows
608609
gateway.last_seen = datetime.now(timezone.utc)
@@ -621,8 +622,14 @@ async def update_gateway(self, db: Session, gateway_id: str, gateway_update: Gat
621622
await self._notify_gateway_updated(gateway)
622623

623624
logger.info(f"Updated gateway: {gateway.name}")
624-
return GatewayRead.model_validate(gateway).masked()
625625

626+
return GatewayRead.model_validate(gateway)
627+
except GatewayNameConflictError as ge:
628+
logger.error(f"GatewayNameConflictError in group: {ge}")
629+
raise ge
630+
except IntegrityError as ie:
631+
logger.error(f"IntegrityErrors in group: {ie}")
632+
raise ie
626633
except Exception as e:
627634
db.rollback()
628635
raise GatewayError(f"Failed to update gateway: {str(e)}")

‎mcpgateway/static/admin.js‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4774,6 +4774,48 @@ async function handleEditToolFormSubmit(event) {
47744774
}
47754775
}
47764776

4777+
async function handleEditGatewayFormSubmit(e) {
4778+
e.preventDefault();
4779+
const form = e.target;
4780+
const formData = new FormData(form);
4781+
try {
4782+
// Validate form inputs
4783+
const name = formData.get("name");
4784+
const url = formData.get("url");
4785+
4786+
const nameValidation = validateInputName(name, "gateway");
4787+
const urlValidation = validateUrl(url);
4788+
4789+
if (!nameValidation.valid) {
4790+
throw new Error(nameValidation.error);
4791+
}
4792+
4793+
if (!urlValidation.valid) {
4794+
throw new Error(urlValidation.error);
4795+
}
4796+
4797+
const isInactiveCheckedBool = isInactiveChecked("gateways");
4798+
formData.append("is_inactive_checked", isInactiveCheckedBool);
4799+
// Submit via fetch
4800+
const response = await fetch(form.action, {
4801+
method: "POST",
4802+
body: formData,
4803+
});
4804+
4805+
const result = await response.json();
4806+
if (!result.success) {
4807+
throw new Error(result.message || "An error occurred");
4808+
}
4809+
// Only redirect on success
4810+
const redirectUrl = isInactiveCheckedBool
4811+
? `${window.ROOT_PATH}/admin?include_inactive=true#gateways`
4812+
: `${window.ROOT_PATH}/admin#gateways`;
4813+
window.location.href = redirectUrl;
4814+
} catch (error) {
4815+
console.error("Error:", error);
4816+
showErrorMessage(error.message);
4817+
}
4818+
}
47774819
// ===================================================================
47784820
// ENHANCED FORM VALIDATION for All Forms
47794821
// ===================================================================
@@ -5335,6 +5377,16 @@ function setupFormHandlers() {
53355377
}
53365378
});
53375379
}
5380+
5381+
const editGatewayForm = safeGetElement("edit-gateway-form");
5382+
if (editGatewayForm) {
5383+
editGatewayForm.addEventListener("submit", handleEditGatewayFormSubmit);
5384+
editGatewayForm.addEventListener("click", () => {
5385+
if (getComputedStyle(editGatewayForm).display !== "none") {
5386+
refreshEditors();
5387+
}
5388+
});
5389+
}
53385390
}
53395391

53405392
function setupSchemaModeHandlers() {

‎tests/unit/mcpgateway/test_admin.py‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -893,9 +893,10 @@ async def test_admin_edit_gateway_url_validation(self, mock_update_gateway, mock
893893

894894
# Should handle validation in GatewayUpdate
895895
result = await admin_edit_gateway("gateway-1", mock_request, mock_db, "test-user")
896-
897-
# Even with invalid URL, should return redirect (validation happens in service)
898-
assert isinstance(result, RedirectResponse)
896+
body = json.loads(result.body.decode())
897+
assert isinstance(result, JSONResponse)
898+
assert result.status_code in (400, 422)
899+
assert body["success"] is False
899900

900901
@patch.object(GatewayService, "toggle_gateway_status")
901902
async def test_admin_toggle_gateway_concurrent_calls(self, mock_toggle_status, mock_request, mock_db):

0 commit comments

Comments
 (0)