Skip to content

Commit 80f7bd6

Browse files
kevalmahajankevalmahajan
authored
Merge pull request #665 from rakdutta/dev_363_edit_ser_res
Improved Error for edit server and resource for Issue #363
2 parents 093b689 + 75eeb9b commit 80f7bd6

File tree

11 files changed

+1132
-98
lines changed

11 files changed

+1132
-98
lines changed

‎mcpgateway/admin.py

Lines changed: 140 additions & 75 deletions
Large diffs are not rendered by default.

‎mcpgateway/main.py

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -852,6 +852,12 @@ async def create_server(
852852
raise HTTPException(status_code=409, detail=str(e))
853853
except ServerError as e:
854854
raise HTTPException(status_code=400, detail=str(e))
855+
except ValidationError as e:
856+
logger.error(f"Validation error while creating server: {e}")
857+
raise HTTPException(status_code=422, detail=ErrorFormatter.format_validation_error(e))
858+
except IntegrityError as e:
859+
logger.error(f"Integrity error while creating server: {e}")
860+
raise HTTPException(status_code=409, detail=ErrorFormatter.format_database_error(e))
855861

856862

857863
@server_router.put("/{server_id}", response_model=ServerRead)
@@ -885,6 +891,12 @@ async def update_server(
885891
raise HTTPException(status_code=409, detail=str(e))
886892
except ServerError as e:
887893
raise HTTPException(status_code=400, detail=str(e))
894+
except ValidationError as e:
895+
logger.error(f"Validation error while updating server {server_id}: {e}")
896+
raise HTTPException(status_code=422, detail=ErrorFormatter.format_validation_error(e))
897+
except IntegrityError as e:
898+
logger.error(f"Integrity error while updating server {server_id}: {e}")
899+
raise HTTPException(status_code=409, detail=ErrorFormatter.format_database_error(e))
888900

889901

890902
@server_router.post("/{server_id}/toggle", response_model=ServerRead)
@@ -1407,7 +1419,7 @@ async def create_resource(
14071419
ResourceRead: The created resource.
14081420
14091421
Raises:
1410-
HTTPException: On conflict or validation errors.
1422+
HTTPException: On conflict or validation errors or IntegrityError.
14111423
"""
14121424
logger.debug(f"User {user} is creating a new resource")
14131425
try:
@@ -1419,7 +1431,11 @@ async def create_resource(
14191431
raise HTTPException(status_code=400, detail=str(e))
14201432
except ValidationError as e:
14211433
# Handle validation errors from Pydantic
1422-
return JSONResponse(content=ErrorFormatter.format_validation_error(e), status_code=422)
1434+
logger.error(f"Validation error while creating resource: {e}")
1435+
raise HTTPException(status_code=422, detail=ErrorFormatter.format_validation_error(e))
1436+
except IntegrityError as e:
1437+
logger.error(f"Integrity error while creating resource: {e}")
1438+
raise HTTPException(status_code=409, detail=ErrorFormatter.format_database_error(e))
14231439

14241440

14251441
@resource_router.get("/{uri:path}")
@@ -1477,6 +1493,12 @@ async def update_resource(
14771493
result = await resource_service.update_resource(db, uri, resource)
14781494
except ResourceNotFoundError as e:
14791495
raise HTTPException(status_code=404, detail=str(e))
1496+
except ValidationError as e:
1497+
logger.error(f"Validation error while updating resource {uri}: {e}")
1498+
raise HTTPException(status_code=422, detail=ErrorFormatter.format_validation_error(e))
1499+
except IntegrityError as e:
1500+
logger.error(f"Integrity error while updating resource {uri}: {e}")
1501+
raise HTTPException(status_code=409, detail=ErrorFormatter.format_database_error(e))
14801502
await invalidate_resource_cache(uri)
14811503
return result
14821504

‎mcpgateway/services/resource_service.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -498,6 +498,7 @@ async def update_resource(self, db: Session, uri: str, resource_update: Resource
498498
Raises:
499499
ResourceNotFoundError: If the resource is not found
500500
ResourceError: For other update errors
501+
IntegrityError: If a database integrity error occurs.
501502
Exception: For unexpected errors
502503
503504
Examples:
@@ -560,7 +561,10 @@ async def update_resource(self, db: Session, uri: str, resource_update: Resource
560561

561562
logger.info(f"Updated resource: {uri}")
562563
return self._convert_resource_to_read(resource)
563-
564+
except IntegrityError as ie:
565+
db.rollback()
566+
logger.error(f"IntegrityErrors in group: {ie}")
567+
raise ie
564568
except Exception as e:
565569
db.rollback()
566570
if isinstance(e, ResourceNotFoundError):

‎mcpgateway/services/server_service.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,7 @@ async def update_server(self, db: Session, server_id: str, server_update: Server
409409
ServerNotFoundError: If the server is not found.
410410
ServerNameConflictError: If a new name conflicts with an existing server.
411411
ServerError: For other update errors.
412+
IntegrityError: If a database integrity error occurs.
412413
413414
Examples:
414415
>>> from mcpgateway.services.server_service import ServerService
@@ -498,6 +499,14 @@ async def update_server(self, db: Session, server_id: str, server_update: Server
498499
}
499500
logger.debug(f"Server Data: {server_data}")
500501
return self._convert_server_to_read(server)
502+
except IntegrityError as ie:
503+
db.rollback()
504+
logger.error(f"IntegrityErrors in group: {ie}")
505+
raise ie
506+
except ServerNameConflictError as snce:
507+
db.rollback()
508+
logger.error(f"Server name conflict: {snce}")
509+
raise snce
501510
except Exception as e:
502511
db.rollback()
503512
raise ServerError(f"Failed to update server: {str(e)}")

‎mcpgateway/static/admin.js

Lines changed: 120 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4627,7 +4627,6 @@ async function handleServerFormSubmit(e) {
46274627

46284628
const result = await response.json();
46294629
if (!result.success) {
4630-
console.log(result.message);
46314630
throw new Error(result.message || "Failed to add server.");
46324631
} else {
46334632
// Success redirect
@@ -4778,7 +4777,6 @@ async function handleEditToolFormSubmit(event) {
47784777
showErrorMessage(error.message);
47794778
}
47804779
}
4781-
47824780
async function handleEditGatewayFormSubmit(e) {
47834781
e.preventDefault();
47844782
const form = e.target;
@@ -4821,6 +4819,111 @@ async function handleEditGatewayFormSubmit(e) {
48214819
showErrorMessage(error.message);
48224820
}
48234821
}
4822+
4823+
async function handleEditServerFormSubmit(e) {
4824+
e.preventDefault();
4825+
const form = e.target;
4826+
const formData = new FormData(form);
4827+
4828+
try {
4829+
// Validate inputs
4830+
const name = formData.get("name");
4831+
const nameValidation = validateInputName(name, "server");
4832+
if (!nameValidation.valid) {
4833+
throw new Error(nameValidation.error);
4834+
}
4835+
4836+
// Save CodeMirror editors' contents if present
4837+
if (window.promptToolHeadersEditor) {
4838+
window.promptToolHeadersEditor.save();
4839+
}
4840+
if (window.promptToolSchemaEditor) {
4841+
window.promptToolSchemaEditor.save();
4842+
}
4843+
4844+
const isInactiveCheckedBool = isInactiveChecked("servers");
4845+
formData.append("is_inactive_checked", isInactiveCheckedBool);
4846+
4847+
// Submit via fetch
4848+
const response = await fetch(form.action, {
4849+
method: "POST",
4850+
body: formData,
4851+
});
4852+
4853+
const result = await response.json();
4854+
if (!result.success) {
4855+
throw new Error(result.message || "An error occurred");
4856+
}
4857+
// Only redirect on success
4858+
else {
4859+
// Redirect to the appropriate page based on inactivity checkbox
4860+
const redirectUrl = isInactiveCheckedBool
4861+
? `${window.ROOT_PATH}/admin?include_inactive=true#catalog`
4862+
: `${window.ROOT_PATH}/admin#catalog`;
4863+
window.location.href = redirectUrl;
4864+
}
4865+
} catch (error) {
4866+
console.error("Error:", error);
4867+
showErrorMessage(error.message);
4868+
}
4869+
}
4870+
4871+
async function handleEditResFormSubmit(e) {
4872+
e.preventDefault();
4873+
const form = e.target;
4874+
const formData = new FormData(form);
4875+
4876+
try {
4877+
// Validate inputs
4878+
const name = formData.get("name");
4879+
const uri = formData.get("uri");
4880+
const nameValidation = validateInputName(name, "resource");
4881+
const uriValidation = validateInputName(uri, "resource URI");
4882+
4883+
if (!nameValidation.valid) {
4884+
showErrorMessage(nameValidation.error);
4885+
return;
4886+
}
4887+
4888+
if (!uriValidation.valid) {
4889+
showErrorMessage(uriValidation.error);
4890+
return;
4891+
}
4892+
4893+
// Save CodeMirror editors' contents if present
4894+
if (window.promptToolHeadersEditor) {
4895+
window.promptToolHeadersEditor.save();
4896+
}
4897+
if (window.promptToolSchemaEditor) {
4898+
window.promptToolSchemaEditor.save();
4899+
}
4900+
4901+
const isInactiveCheckedBool = isInactiveChecked("resources");
4902+
formData.append("is_inactive_checked", isInactiveCheckedBool);
4903+
// Submit via fetch
4904+
const response = await fetch(form.action, {
4905+
method: "POST",
4906+
body: formData,
4907+
});
4908+
4909+
const result = await response.json();
4910+
if (!result.success) {
4911+
throw new Error(result.message || "An error occurred");
4912+
}
4913+
// Only redirect on success
4914+
else {
4915+
// Redirect to the appropriate page based on inactivity checkbox
4916+
const redirectUrl = isInactiveCheckedBool
4917+
? `${window.ROOT_PATH}/admin?include_inactive=true#resources`
4918+
: `${window.ROOT_PATH}/admin#resources`;
4919+
window.location.href = redirectUrl;
4920+
}
4921+
} catch (error) {
4922+
console.error("Error:", error);
4923+
showErrorMessage(error.message);
4924+
}
4925+
}
4926+
48244927
// ===================================================================
48254928
// ENHANCED FORM VALIDATION for All Forms
48264929
// ===================================================================
@@ -5365,14 +5468,26 @@ function setupFormHandlers() {
53655468
serverForm.addEventListener("submit", handleServerFormSubmit);
53665469
}
53675470

5471+
const editServerForm = safeGetElement("edit-server-form");
5472+
if (editServerForm) {
5473+
editServerForm.addEventListener("submit", handleEditServerFormSubmit);
5474+
editServerForm.addEventListener("click", () => {
5475+
if (getComputedStyle(editServerForm).display !== "none") {
5476+
refreshEditors();
5477+
}
5478+
});
5479+
}
5480+
53685481
const editResourceForm = safeGetElement("edit-resource-form");
53695482
if (editResourceForm) {
5370-
editResourceForm.addEventListener("submit", () => {
5371-
if (window.editResourceContentEditor) {
5372-
window.editResourceContentEditor.save();
5483+
editResourceForm.addEventListener("submit", handleEditResFormSubmit);
5484+
editResourceForm.addEventListener("click", () => {
5485+
if (getComputedStyle(editResourceForm).display !== "none") {
5486+
refreshEditors();
53735487
}
53745488
});
53755489
}
5490+
53765491
const editToolForm = safeGetElement("edit-tool-form");
53775492
if (editToolForm) {
53785493
editToolForm.addEventListener("submit", handleEditToolFormSubmit);

‎mcpgateway/utils/verify_credentials.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -471,7 +471,6 @@ async def require_docs_basic_auth(auth_header: str) -> str:
471471
"""
472472
scheme, param = get_authorization_scheme_param(auth_header)
473473
if scheme.lower() == "basic" and param and settings.docs_allow_basic_auth:
474-
475474
try:
476475
data = b64decode(param).decode("ascii")
477476
username, separator, password = data.partition(":")

‎tests/e2e/test_admin_apis.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ async def test_admin_server_lifecycle(self, client: AsyncClient, mock_settings):
186186
"associatedPrompts": "",
187187
}
188188
response = await client.post(f"/admin/servers/{server_id}/edit", data=edit_data, headers=TEST_AUTH_HEADER, follow_redirects=False)
189-
assert response.status_code == 303
189+
assert response.status_code == 200
190190

191191
# Toggle server status
192192
response = await client.post(f"/admin/servers/{server_id}/toggle", data={"activate": "false"}, headers=TEST_AUTH_HEADER, follow_redirects=False)

‎tests/e2e/test_main_apis.py

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -444,9 +444,7 @@ async def test_server_name_conflict(self, client: AsyncClient, mock_auth):
444444
response = await client.post("/servers", json=server_data, headers=TEST_AUTH_HEADER)
445445
assert response.status_code == 409
446446
resp_json = response.json()
447-
if "detail" in resp_json:
448-
assert "already exists" in resp_json["detail"]
449-
elif "message" in resp_json:
447+
if "message" in resp_json:
450448
assert "already exists" in resp_json["message"]
451449
else:
452450
# Accept any error format as long as status is correct
@@ -637,10 +635,11 @@ async def test_resource_uri_conflict(self, client: AsyncClient, mock_auth):
637635
response = await client.post("/resources", json=resource_data, headers=TEST_AUTH_HEADER)
638636
assert response.status_code in [400, 409]
639637
resp_json = response.json()
640-
if "detail" in resp_json:
641-
assert "already exists" in resp_json["detail"]
642-
elif "message" in resp_json:
638+
if "message" in resp_json:
643639
assert "already exists" in resp_json["message"]
640+
else:
641+
# Accept any error format as long as status is correct
642+
assert response.status_code == 409
644643

645644
"""Test resource management endpoints."""
646645

@@ -779,9 +778,7 @@ async def test_resource_uri_conflict(self, client: AsyncClient, mock_auth):
779778
response = await client.post("/resources", json=resource_data, headers=TEST_AUTH_HEADER)
780779
assert response.status_code in [400, 409]
781780
resp_json = response.json()
782-
if "detail" in resp_json:
783-
assert "already exists" in resp_json["detail"]
784-
elif "message" in resp_json:
781+
if "message" in resp_json:
785782
assert "already exists" in resp_json["message"]
786783
else:
787784
# Accept any error format as long as status is correct

0 commit comments

Comments
 (0)