Improved Error for edit gateway for Issue #363 and Bug fixing of issue #603

mcpgateway/admin.py

@@ -2389,7 +2389,7 @@ async def admin_edit_gateway(
     request: Request,
     db: Session = Depends(get_db),
     user: str = Depends(require_auth),
-) -> RedirectResponse:
+) -> JSONResponse:
     """Edit a gateway via the admin UI.
 
     Expects form fields:
@@ -2419,52 +2419,68 @@ async def admin_edit_gateway(
         >>> gateway_id = "gateway-to-edit"
         >>>
         >>> # Happy path: Edit gateway successfully
-        >>> form_data_success = FormData([("name", "Updated Gateway"), ("url", "http://updated.com"), ("is_inactive_checked", "false"), ("auth_type", "basic")]) # Added auth_type
+        >>> form_data_success = FormData([
+        ...  ("name", "Updated Gateway"),
+        ...  ("url", "http://updated.com"),
+        ...  ("is_inactive_checked", "false"),
+        ...  ("auth_type", "basic"),
+        ...  ("auth_username", "user"),
+        ...  ("auth_password", "pass")
+        ... ])
         >>> mock_request_success = MagicMock(spec=Request, scope={"root_path": ""})
         >>> mock_request_success.form = AsyncMock(return_value=form_data_success)
         >>> original_update_gateway = gateway_service.update_gateway
         >>> gateway_service.update_gateway = AsyncMock()
         >>>
         >>> async def test_admin_edit_gateway_success():
         ...     response = await admin_edit_gateway(gateway_id, mock_request_success, mock_db, mock_user)
-        ...     return isinstance(response, RedirectResponse) and response.status_code == 303 and "/admin#gateways" in response.headers["location"]
+        ...     return isinstance(response, JSONResponse) and response.status_code == 200 and json.loads(response.body)["success"] is True
         >>>
         >>> asyncio.run(test_admin_edit_gateway_success())
         True
         >>>
-        >>> # Edge case: Edit gateway with inactive checkbox checked
-        >>> form_data_inactive = FormData([("name", "Inactive Edit"), ("url", "http://inactive.com"), ("is_inactive_checked", "true"), ("auth_type", "basic")]) # Added auth_type
-        >>> mock_request_inactive = MagicMock(spec=Request, scope={"root_path": "/api"})
-        >>> mock_request_inactive.form = AsyncMock(return_value=form_data_inactive)
-        >>>
-        >>> async def test_admin_edit_gateway_inactive_checked():
-        ...     response = await admin_edit_gateway(gateway_id, mock_request_inactive, mock_db, mock_user)
-        ...     return isinstance(response, RedirectResponse) and response.status_code == 303 and "/api/admin/?include_inactive=true#gateways" in response.headers["location"]
-        >>>
-        >>> asyncio.run(test_admin_edit_gateway_inactive_checked())
-        True
-        >>>
+        # >>> # Edge case: Edit gateway with inactive checkbox checked
+        # >>> form_data_inactive = FormData([("name", "Inactive Edit"), ("url", "http://inactive.com"), ("is_inactive_checked", "true"), ("auth_type", "basic"), ("auth_username", "user"),
+        # ...     ("auth_password", "pass")]) # Added auth_type
+        # >>> mock_request_inactive = MagicMock(spec=Request, scope={"root_path": "/api"})
+        # >>> mock_request_inactive.form = AsyncMock(return_value=form_data_inactive)
+        # >>>
+        # >>> async def test_admin_edit_gateway_inactive_checked():
+        # ...     response = await admin_edit_gateway(gateway_id, mock_request_inactive, mock_db, mock_user)
+        # ...     return isinstance(response, RedirectResponse) and response.status_code == 303 and "/api/admin/?include_inactive=true#gateways" in response.headers["location"]
+        # >>>
+        # >>> asyncio.run(test_admin_edit_gateway_inactive_checked())
+        # True
+        # >>>
         >>> # Error path: Simulate an exception during update
-        >>> form_data_error = FormData([("name", "Error Gateway"), ("url", "http://error.com"), ("auth_type", "basic")]) # Added auth_type
+        >>> form_data_error = FormData([("name", "Error Gateway"), ("url", "http://error.com"), ("auth_type", "basic"),("auth_username", "user"),
+        ...     ("auth_password", "pass")]) # Added auth_type
         >>> mock_request_error = MagicMock(spec=Request, scope={"root_path": ""})
         >>> mock_request_error.form = AsyncMock(return_value=form_data_error)
         >>> gateway_service.update_gateway = AsyncMock(side_effect=Exception("Update failed"))
         >>>
         >>> async def test_admin_edit_gateway_exception():
         ...     response = await admin_edit_gateway(gateway_id, mock_request_error, mock_db, mock_user)
-        ...     return isinstance(response, RedirectResponse) and response.status_code == 303 and "/admin#gateways" in response.headers["location"]
+        ...     return (
+        ...         isinstance(response, JSONResponse)
+        ...         and response.status_code == 500
+        ...         and json.loads(response.body)["success"] is False
+        ...         and "Update failed" in json.loads(response.body)["message"]
+        ...     )
         >>>
         >>> asyncio.run(test_admin_edit_gateway_exception())
         True
         >>>
         >>> # Error path: Pydantic Validation Error (e.g., invalid URL format)
-        >>> form_data_validation_error = FormData([("name", "Bad URL Gateway"), ("url", "invalid-url"), ("auth_type", "basic")]) # Added auth_type
+        >>> form_data_validation_error = FormData([("name", "Bad URL Gateway"), ("url", "invalid-url"), ("auth_type", "basic"),("auth_username", "user"),
+        ...     ("auth_password", "pass")]) # Added auth_type
         >>> mock_request_validation_error = MagicMock(spec=Request, scope={"root_path": ""})
         >>> mock_request_validation_error.form = AsyncMock(return_value=form_data_validation_error)
         >>>
         >>> async def test_admin_edit_gateway_validation_error():
         ...     response = await admin_edit_gateway(gateway_id, mock_request_validation_error, mock_db, mock_user)
-        ...     return isinstance(response, RedirectResponse) and response.status_code == 303 and "/admin#gateways" in response.headers["location"]
+        ...     body = json.loads(response.body.decode())
+        ...     return isinstance(response, JSONResponse) and response.status_code in (422,400) and body["success"] is False
         >>>
         >>> asyncio.run(test_admin_edit_gateway_validation_error())
         True
@@ -2474,7 +2490,6 @@ async def admin_edit_gateway(
     """
     logger.debug(f"User {user} is editing gateway ID {gateway_id}")
     form = await request.form()
-    is_inactive_checked = form.get("is_inactive_checked", "false")
     try:
         gateway = GatewayUpdate(  # Pydantic validation happens here
             name=form.get("name"),
@@ -2489,18 +2504,22 @@ async def admin_edit_gateway(
             auth_header_value=form.get("auth_header_value", None),
         )
         await gateway_service.update_gateway(db, gateway_id, gateway)
-
-        root_path = request.scope.get("root_path", "")
-        if is_inactive_checked.lower() == "true":
-            return RedirectResponse(f"{root_path}/admin/?include_inactive=true#gateways", status_code=303)
-        return RedirectResponse(f"{root_path}/admin#gateways", status_code=303)
-    except Exception as e:  # Catch all exceptions including ValidationError for redirect
-        logger.error(f"Error editing gateway: {e}")
-
-        root_path = request.scope.get("root_path", "")
-        if is_inactive_checked.lower() == "true":
-            return RedirectResponse(f"{root_path}/admin/?include_inactive=true#gateways", status_code=303)
-        return RedirectResponse(f"{root_path}/admin#gateways", status_code=303)
+        return JSONResponse(
+            content={"message": "Gateway update successfully!", "success": True},
+            status_code=200,
+        )
+    except Exception as ex:
+        if isinstance(ex, GatewayConnectionError):
+            return JSONResponse(content={"message": str(ex), "success": False}, status_code=502)
+        if isinstance(ex, ValueError):
+            return JSONResponse(content={"message": str(ex), "success": False}, status_code=400)
+        if isinstance(ex, RuntimeError):
+            return JSONResponse(content={"message": str(ex), "success": False}, status_code=500)
+        if isinstance(ex, ValidationError):
+            return JSONResponse(content=ErrorFormatter.format_validation_error(ex), status_code=422)
+        if isinstance(ex, IntegrityError):
+            return JSONResponse(status_code=409, content=ErrorFormatter.format_database_error(ex))
+        return JSONResponse(content={"message": str(ex), "success": False}, status_code=500)
 
 
 @admin_router.post("/gateways/{gateway_id}/delete")

mcpgateway/main.py

@@ -1812,6 +1812,10 @@ async def register_gateway(
             return JSONResponse(content={"message": "Gateway name already exists"}, status_code=400)
         if isinstance(ex, RuntimeError):
             return JSONResponse(content={"message": "Error during execution"}, status_code=500)
+        if isinstance(ex, ValidationError):
+            return JSONResponse(content=ErrorFormatter.format_validation_error(ex), status_code=422)
+        if isinstance(ex, IntegrityError):
+            return JSONResponse(status_code=409, content=ErrorFormatter.format_database_error(ex))
         return JSONResponse(content={"message": "Unexpected error"}, status_code=500)
 
 

mcpgateway/services/gateway_service.py

@@ -530,6 +530,8 @@ async def update_gateway(self, db: Session, gateway_id: str, gateway_update: Gat
             GatewayNotFoundError: If gateway not found
             GatewayError: For other update errors
             GatewayNameConflictError: If gateway name conflict occurs
+            IntegrityError: If there is a database integrity error
+            ValidationError: If validation fails
         """
         try:
             # Find gateway
@@ -602,7 +604,6 @@ async def update_gateway(self, db: Session, gateway_id: str, gateway_update: Gat
                                         auth_value=gateway.auth_value,
                                     )
                                 )
-
                         gateway.capabilities = capabilities
                         gateway.tools = [tool for tool in gateway.tools if tool.original_name in new_tool_names]  # keep only still-valid rows
                         gateway.last_seen = datetime.now(timezone.utc)
@@ -621,8 +622,14 @@ async def update_gateway(self, db: Session, gateway_id: str, gateway_update: Gat
                 await self._notify_gateway_updated(gateway)
 
                 logger.info(f"Updated gateway: {gateway.name}")
-                return GatewayRead.model_validate(gateway).masked()
 
+                return GatewayRead.model_validate(gateway)
+        except GatewayNameConflictError as ge:
+            logger.error(f"GatewayNameConflictError in group: {ge}")
+            raise ge
+        except IntegrityError as ie:
+            logger.error(f"IntegrityErrors in group: {ie}")
+            raise ie
         except Exception as e:
             db.rollback()
             raise GatewayError(f"Failed to update gateway: {str(e)}")

mcpgateway/static/admin.js

@@ -4774,6 +4774,48 @@ async function handleEditToolFormSubmit(event) {
     }
 }
 
+async function handleEditGatewayFormSubmit(e) {
+    e.preventDefault();
+    const form = e.target;
+    const formData = new FormData(form);
+    try {
+        // Validate form inputs
+        const name = formData.get("name");
+        const url = formData.get("url");
+
+        const nameValidation = validateInputName(name, "gateway");
+        const urlValidation = validateUrl(url);
+
+        if (!nameValidation.valid) {
+            throw new Error(nameValidation.error);
+        }
+
+        if (!urlValidation.valid) {
+            throw new Error(urlValidation.error);
+        }
+
+        const isInactiveCheckedBool = isInactiveChecked("gateways");
+        formData.append("is_inactive_checked", isInactiveCheckedBool);
+        // Submit via fetch
+        const response = await fetch(form.action, {
+            method: "POST",
+            body: formData,
+        });
+
+        const result = await response.json();
+        if (!result.success) {
+            throw new Error(result.message || "An error occurred");
+        }
+        // Only redirect on success
+        const redirectUrl = isInactiveCheckedBool
+            ? `${window.ROOT_PATH}/admin?include_inactive=true#gateways`
+            : `${window.ROOT_PATH}/admin#gateways`;
+        window.location.href = redirectUrl;
+    } catch (error) {
+        console.error("Error:", error);
+        showErrorMessage(error.message);
+    }
+}
 // ===================================================================
 // ENHANCED FORM VALIDATION for All Forms
 // ===================================================================
@@ -5335,6 +5377,16 @@ function setupFormHandlers() {
             }
         });
     }
+
+    const editGatewayForm = safeGetElement("edit-gateway-form");
+    if (editGatewayForm) {
+        editGatewayForm.addEventListener("submit", handleEditGatewayFormSubmit);
+        editGatewayForm.addEventListener("click", () => {
+            if (getComputedStyle(editGatewayForm).display !== "none") {
+                refreshEditors();
+            }
+        });
+    }
 }
 
 function setupSchemaModeHandlers() {

tests/unit/mcpgateway/test_admin.py

@@ -893,9 +893,10 @@ async def test_admin_edit_gateway_url_validation(self, mock_update_gateway, mock
 
         # Should handle validation in GatewayUpdate
         result = await admin_edit_gateway("gateway-1", mock_request, mock_db, "test-user")
-
-        # Even with invalid URL, should return redirect (validation happens in service)
-        assert isinstance(result, RedirectResponse)
+        body = json.loads(result.body.decode())
+        assert isinstance(result, JSONResponse)
+        assert result.status_code in (400, 422)
+        assert body["success"] is False
 
     @patch.object(GatewayService, "toggle_gateway_status")
     async def test_admin_toggle_gateway_concurrent_calls(self, mock_toggle_status, mock_request, mock_db):