Improved Error for Add tool and edit Tool

[rakdutta]

Refactored admin.py for both add and edit tool routers:

Updated admin_add_tool and admin_edit_tool functions to handle ValidationError and IntegrityError exceptions
Updated mcpgateway/services/tool_service.py:

• Removed explicit check for existing tool name; now relying on database-level UNIQUE constraint and IntegrityError handling.
  Update the below test file to handle new error code for Add tool and Edit Tool function
  1)test_admin_api.py
  2)test_admin.py
  3)test_main.py
  4)test_tool_service.py

[madhav165]

@rakdutta I tried this flow

Step 1: Add a REST tool
name: test
url: http://example.com
integration type: REST
request type: GET

Step 2: Add a second REST tool
name: test2
url: http://example.com
integration type: REST
request type: GET

Step 3: Modify second REST tool name to 'test'

Noticed 2 issues

1. It should show duplicate name error message, showing invalid input
2. Error is showing in a new page instead of the current one.

[rakdutta]

@madhav165
The "Edit Tool" functionality was working correctly when the integration_type was set to "MCP", but it was failing for "REST".
Upon investigation, the issue was traced to the validate_request_type function in schemas.py. The problem occurred because the data inside the validator did not contain the integration_type field, which caused it to default or behave as if the type was always "MCP".
To resolve this, the field order in the Pydantic model was updated to ensure integration_type is parsed before request_type. This allows it to be available in info.data during validation.
Raised issue #579

[madhav165]

@rakdutta Getting the correct error message, but still redirecting to a new page.

[rakdutta]

@madhav165
We need to update admin.js and admin.html for all routers. These changes will be handled in a separate PR.

[crivetimihai]

Could we check if all tests are still present please?

[rakdutta]

@madhav165
Updated the admin.js file to enable redirection to the correct page. Please test the changes on your end. I've also reverted test_tool_service.py to the main branch version. I will address the failed test cases separately based on the version.

[MohanLaksh]

Hi @rakdutta / @rakdutta1 ,

1. make test - FAILS
   ==========5 failed, 1074 passed, 5 skipped, 46 warnings in 48.58s ===============================

Can you please recheck?

Summary Below:
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
FAILED tests/unit/mcpgateway/services/test_tool_service.py::TestToolService::test_register_tool_with_gateway_id - assert 'Tool already exists with name' in "Failed to register tool: 'NoneType' object has no attribute 'slug'"
FAILED tests/unit/mcpgateway/services/test_tool_service.py::TestToolService::test_register_tool_name_conflict - assert 'Tool already exists with name' in 'Failed to register tool: "Deferred loader for attribute 'id' failed to populate correctly"'
FAILED tests/unit/mcpgateway/services/test_tool_service.py::TestToolService::test_register_inactive_tool_name_conflict - sqlalchemy.exc.IntegrityError: (sqlite3.IntegrityError) UNIQUE constraint failed: tools.name
FAILED tests/unit/mcpgateway/services/test_tool_service.py::TestToolService::test_register_tool_db_integrity_error - sqlalchemy.exc.IntegrityError: (builtins.str) orig
FAILED tests/unit/mcpgateway/services/test_tool_service.py::TestToolService::test_update_tool_name_conflict - assert 'Tool already exists with name' in "Failed to update tool: Class 'unittest.mock.MagicMock' is not mapped"
==========5 failed, 1074 passed, 5 skipped, 46 warnings in 48.58s ==============================

2. make autoflake isort black flake8

make black - suggests 2 files (/mcp-context-forge/mcpgateway/validators.py and /mcp-context-forge/mcpgateway/config.py) need reformatting...

3. make pylint - FAILS

---

Your code has been rated at 9.98/10 (previous run: 10.00/10, -0.02)

************* Module mcpgateway.schemas
mcpgateway/schemas.py:1574:0: C0115: Missing class docstring (missing-class-docstring)
mcpgateway/schemas.py:1692:4: C0116: Missing function or method docstring (missing-function-docstring)
************* Module mcpgateway.admin
mcpgateway/admin.py:297:0: R0911: Too many return statements (11/8) (too-many-return-statements)
mcpgateway/admin.py:2362:31: E1101: Instance of 'ValueError' has no 'errors' member (no-member)
mcpgateway/admin.py:2363:19: E1101: Instance of 'ValueError' has no 'errors' member (no-member)
mcpgateway/admin.py:2231:0: R0911: Too many return statements (9/8) (too-many-return-statements)
************* Module mcpgateway.handlers.sampling
mcpgateway/handlers/sampling.py:219:13: W0511: TODO: Sample from selected model (fixme)
mcpgateway/handlers/sampling.py:358:9: W0511: TODO: Implement context gathering based on type (fixme)

4. make smoketest
   ✅ Smoketest passed!

5. make doctest

---

============================short test summary info ==================
SKIPPED [8] ../../.venv/mcpgateway/lib/python3.11/site-packages/_pytest/doctest.py:458: all tests skipped by +SKIP option
372 passed, 8 skipped in 12.51s

[rakdutta]

@MohanLaksh
Please refer my previous comment. As per yesterday discussion I've reverted test_tool_service.py to the main branch version. Currently I am working on the failed test cases.

[rakdutta]

@madhav165 @MohanLaksh
Applied formatting changes using Make Black to the following files:

1. mcpgateway/config.py
2. mcpgateway/validators.py

Fixed failing test cases in test_tool_service.py

The following pylint errors have already been addressed in PR #577 by @TS0713:
mcpgateway/admin.py:2362:31: E1101: Instance of 'ValueError' has no 'errors' member (no-member)
mcpgateway/admin.py:2363:19: E1101: Instance of 'ValueError' has no 'errors' member (no-member)

[crivetimihai]

PR Review Summary

• make doctest test smoketest

✅ Overall Assessment: APPROVED with minor recommendations

This PR successfully refactors the tool registration and editing flow to use database-level constraint checking instead of application-level duplicate checks. The changes are well-implemented across the entire stack.

Key Changes Verified:

1. Database Constraint Approach ✓

   • Removes race conditions inherent in pre-check queries
   • Correctly uses HTTP 409 (Conflict) instead of 400 for duplicates
   • Properly handles IntegrityError exceptions

2. Frontend Updates ✓

   • JSON response handling implemented correctly
   • New handleEditToolFormSubmit function properly manages async responses
   • Redirect logic moved from backend to frontend

3. Test Coverage ✓

   • All tests updated to expect 409 status codes for conflicts
   • Mock IntegrityError scenarios properly implemented
   • Both unit and e2e tests align with new behavior

4. Code Quality ✓

   • No dead code or commented blocks remain
   • Improved error logging with correct method names
   • Enhanced docstrings with relevant examples

Recommendations Before Merge:

1. Verify Database Schema

   • Confirm UNIQUE constraints exist on (name, gateway_id) columns
   • Check migration files to ensure constraints are properly defined

2. User Experience Enhancement

   • Verify ErrorFormatter.format_database_error produces user-friendly messages
   • Consider parsing "UNIQUE constraint failed" errors to show "Tool name already exists"

3. Additional Test Coverage

   • Add tests for non-conflict IntegrityErrors
   • Test concurrent tool creation scenarios
   • Verify behavior with same name but different gateway_id

4. Frontend Validation

   • Ensure all UI paths expecting redirects are updated
   • Test error message display for various error scenarios

Technical Notes:

• The values.data.get change is correct for Pydantic v2
• The add-resource-loading vs add-gateway-loading fix is appropriate
• Error handling follows RESTful conventions properly

Pre-Merge Checklist

Database Schema

• Verify UNIQUE constraint exists on tools.name column in database schema - see tool_service.py
• Verify UNIQUE constraint exists on combination of (name, gateway_id) if tools are scoped by gateway
• Check migration files to ensure constraints are properly applied

Error Formatting

• Test that ErrorFormatter.format_database_error produces user-friendly messages for UNIQUE constraint violations
• Ensure error messages say "Tool name already exists" instead of raw SQL errors
• Verify error messages properly identify which field caused the conflict

Frontend

• Search for any remaining code expecting redirect responses from tool edit endpoint
• Remove any unreachable/dead code related to old redirect behavior
• Test error message display in UI for all error scenarios (409, 422, 500)

Test Coverage

• Add test for tools with same name but different gateway_id (if this should be allowed) - see test_admin_add_server_with_empty_associations
• Add test for IntegrityError that's NOT a name conflict (e.g., missing required field) - see test_admin_add_server_with_integrity_error
• Add test for concurrent tool creation to verify race condition handling
• Add test for malformed/incomplete POST data beyond just missing name - see test_admin_add_tool_with_invalid_json and test_admin_add_tool_with_missing_fields

Final Verification

• Manually test tool creation/editing flow in UI
• Verify error messages are clear and actionable for end users

[crivetimihai]

Quick Test

export MCPGATEWAY_BEARER_TOKEN=$(python3 -m mcpgateway.utils.create_jwt_token -u admin --secret my-test-key)

✅ 1. First, create a tool (this should succeed):

curl -X POST http://localhost:4444/admin/tools/ \
  -H "Authorization: Bearer $MCPGATEWAY_BEARER_TOKEN" \
  -H "Content-Type: application/x-www-form-urlencoded" \
  -d "name=test_tool&url=http://example.com&requestType=POST&integrationType=REST"

✅ 2. Try to create the SAME tool again (this should fail with 409):

curl -X POST http://localhost:4444/admin/tools/ \
  -H "Authorization: Bearer $MCPGATEWAY_BEARER_TOKEN" \
  -H "Content-Type: application/x-www-form-urlencoded" \
  -d "name=test_tool&url=http://different.com&requestType=GET&integrationType=REST"

Expected response:

{
  "message": "A tool with this name already exists",
  "success": false
}

✅ 3. Test editing a tool to conflict with another name:

First, create a second tool:

curl -X POST http://localhost:4444/admin/tools/ \
  -H "Authorization: Bearer $MCPGATEWAY_BEARER_TOKEN" \
  -H "Content-Type: application/x-www-form-urlencoded" \
  -d "name=another_tool&url=http://another.com&requestType=PUT&integrationType=REST"

Then get the tool id:

curl -H "Authorization: Bearer $MCPGATEWAY_BEARER_TOKEN"  http://localhost:4444/admin/tools | jq 

Then try to edit it to have the same name as the first tool (replace {tool_id} with actual ID):

curl -X POST http://localhost:4444/admin/tools/{tool_id}/edit/ \
  -H "Authorization: Bearer $MCPGATEWAY_BEARER_TOKEN" \
  -H "Content-Type: application/x-www-form-urlencoded" \
  -d "name=test_tool&url=http://another.com&requestType=DELETE&integrationType=REST"

[crivetimihai]

#!/bin/bash

# Check if token is set
if [ -z "$MCPGATEWAY_BEARER_TOKEN" ]; then
    echo "Error: MCPGATEWAY_BEARER_TOKEN not set"
    echo "Run: export MCPGATEWAY_BEARER_TOKEN='your-token-here'"
    exit 1
fi

BASE_URL="http://localhost:4444"
AUTH="Authorization: Bearer $MCPGATEWAY_BEARER_TOKEN"

# Colors for output
GREEN='\033[0;32m'
RED='\033[0;31m'
YELLOW='\033[1;33m'
NC='\033[0m' # No Color

# Function to make requests and handle responses properly
make_request() {
    local method="$1"
    local endpoint="$2"
    local data="$3"
    local expected_status="$4"
    local test_name="$5"

    echo -e "\n${YELLOW}=== $test_name ===${NC}"

    # Make request and capture response with status
    local response
    response=$(curl -s -w "\n__STATUS__%{http_code}" -X "$method" "$BASE_URL$endpoint" \
        -H "$AUTH" \
        -H "Content-Type: application/x-www-form-urlencoded" \
        -d "$data" 2>/dev/null)

    # Extract body and status
    local body=$(echo "$response" | sed -n '1,/__STATUS__/p' | sed '$d')
    local status=$(echo "$response" | grep -o '__STATUS__[0-9]*' | sed 's/__STATUS__//')

    # Display results
    echo "HTTP Status: $status"
    echo "Response:"
    echo "$body" | jq . 2>/dev/null || echo "$body"

    # Check if status matches expected
    if [ "$status" = "$expected_status" ]; then
        echo -e "${GREEN}✓ Status matches expected: $expected_status${NC}"
    else
        echo -e "${RED}✗ Expected status $expected_status but got $status${NC}"
    fi

    # Return status for further processing
    echo "$status"
}

# Function to extract ID from JSON response
extract_id() {
    local json="$1"
    echo "$json" | jq -r '.id // empty' 2>/dev/null
}

echo -e "${YELLOW}Starting MCP Gateway Tool Tests${NC}"
echo "Using endpoint: $BASE_URL"

# Test 1: Create a valid tool
echo -e "\n${YELLOW}Test 1: Create Valid Tool${NC}"
RESPONSE1=$(curl -s -X POST "$BASE_URL/admin/tools/" \
    -H "$AUTH" \
    -H "Content-Type: application/x-www-form-urlencoded" \
    -d "name=test_tool_1&url=http://example.com&requestType=POST&integrationType=REST")
echo "$RESPONSE1" | jq . 2>/dev/null || echo "$RESPONSE1"
TOOL1_ID=$(extract_id "$RESPONSE1")
echo "Created tool ID: $TOOL1_ID"

# Test 2: Try to create duplicate (should fail with 409)
make_request "POST" "/admin/tools/" \
    "name=test_tool_1&url=http://different.com&requestType=GET&integrationType=REST" \
    "409" \
    "Test 2: Duplicate Tool Name (Should Fail)"

# Test 3: Create second tool with different name
echo -e "\n${YELLOW}Test 3: Create Second Tool${NC}"
RESPONSE3=$(curl -s -X POST "$BASE_URL/admin/tools/" \
    -H "$AUTH" \
    -H "Content-Type: application/x-www-form-urlencoded" \
    -d "name=test_tool_2&url=http://another.com&requestType=PUT&integrationType=REST")
echo "$RESPONSE3" | jq . 2>/dev/null || echo "$RESPONSE3"
TOOL2_ID=$(extract_id "$RESPONSE3")
echo "Created tool ID: $TOOL2_ID"

# Test 4: Edit tool to conflict with existing name
if [ -n "$TOOL2_ID" ]; then
    make_request "POST" "/admin/tools/$TOOL2_ID/edit/" \
        "name=test_tool_1&url=http://edited.com&requestType=DELETE&integrationType=REST" \
        "409" \
        "Test 4: Edit Tool to Existing Name (Should Fail)"
fi

# Test 5: Successful edit with unique name
if [ -n "$TOOL2_ID" ]; then
    make_request "POST" "/admin/tools/$TOOL2_ID/edit/" \
        "name=test_tool_2_edited&url=http://edited.com&requestType=PATCH&integrationType=REST" \
        "200" \
        "Test 5: Edit Tool with Unique Name (Should Succeed)"
fi

# Test 6: Validation errors
make_request "POST" "/admin/tools/" \
    "name=missing_url&requestType=POST&integrationType=REST" \
    "422" \
    "Test 6: Missing URL (Validation Error)"

make_request "POST" "/admin/tools/" \
    "url=http://example.com&requestType=POST&integrationType=REST" \
    "422" \
    "Test 7: Missing Name (Validation Error)"

make_request "POST" "/admin/tools/" \
    "name=invalid_combo&url=http://example.com&requestType=SSE&integrationType=REST" \
    "422" \
    "Test 8: Invalid REST+SSE Combination"

# Test 9: Special characters
make_request "POST" "/admin/tools/" \
    "name=test_with_underscore&url=http://example.com&requestType=POST&integrationType=REST" \
    "200" \
    "Test 9: Name with Underscore"

make_request "POST" "/admin/tools/" \
    "name=test-with-hyphen&url=http://example.com&requestType=POST&integrationType=REST" \
    "200" \
    "Test 10: Name with Hyphen"

# Test 11: Concurrent duplicate attempts
echo -e "\n${YELLOW}Test 11: Concurrent Duplicate Attempts${NC}"
echo "Launching 5 parallel requests for the same tool name..."
for i in {1..5}; do
    (
        RESPONSE=$(curl -s -w "\n__STATUS__%{http_code}" -X POST "$BASE_URL/admin/tools/" \
            -H "$AUTH" \
            -H "Content-Type: application/x-www-form-urlencoded" \
            -d "name=concurrent_test&url=http://concurrent$i.com&requestType=POST&integrationType=REST" 2>/dev/null)
        STATUS=$(echo "$RESPONSE" | grep -o '__STATUS__[0-9]*' | sed 's/__STATUS__//')
        echo "Request $i: Status $STATUS"
    ) &
done
wait

echo -e "\n${YELLOW}Test Summary Complete${NC}"
echo "All tests have been executed. Check the results above."

Results

�[1;33mStarting MCP Gateway Tool Tests�[0m
Using endpoint: http://localhost:4444

�[1;33mTest 1: Create Valid Tool�[0m
{
  "message": "A tool with this name already exists",
  "success": false
}
Created tool ID: 

�[1;33m=== Test 2: Duplicate Tool Name (Should Fail) ===�[0m
HTTP Status: 409
Response:
{
  "message": "A tool with this name already exists",
  "success": false
}
�[0;32m✓ Status matches expected: 409�[0m
409

�[1;33mTest 3: Create Second Tool�[0m
{
  "message": "A tool with this name already exists",
  "success": false
}
Created tool ID: 

�[1;33m=== Test 6: Missing URL (Validation Error) ===�[0m
HTTP Status: 422
Response:
{
  "message": "Validation failed",
  "details": [
    {
      "field": "constrained-str",
      "message": "Invalid constrained-str"
    },
    {
      "field": "function-wrap[wrap_val()]",
      "message": "Invalid function-wrap[wrap_val()]"
    }
  ],
  "success": false
}
�[0;32m✓ Status matches expected: 422�[0m
422

�[1;33m=== Test 7: Missing Name (Validation Error) ===�[0m
HTTP Status: 422
Response:
{
  "message": "Validation failed",
  "details": [
    {
      "field": "name",
      "message": "Invalid name"
    }
  ],
  "success": false
}
�[0;32m✓ Status matches expected: 422�[0m
422

�[1;33m=== Test 8: Invalid REST+SSE Combination ===�[0m
HTTP Status: 422
Response:
{
  "message": "Validation failed",
  "details": [
    {
      "field": "request_type",
      "message": "Invalid request_type"
    }
  ],
  "success": false
}
�[0;32m✓ Status matches expected: 422�[0m
422

�[1;33m=== Test 9: Name with Underscore ===�[0m
HTTP Status: 409
Response:
{
  "message": "A tool with this name already exists",
  "success": false
}
�[0;31m✗ Expected status 200 but got 409�[0m
409

�[1;33m=== Test 10: Name with Hyphen ===�[0m
HTTP Status: 409
Response:
{
  "message": "A tool with this name already exists",
  "success": false
}
�[0;31m✗ Expected status 200 but got 409�[0m
409

�[1;33mTest 11: Concurrent Duplicate Attempts�[0m
Launching 5 parallel requests for the same tool name...
Request 1: Status 409
Request 4: Status 409
Request 2: Status 409
Request 3: Status 409
Request 5: Status 409

�[1;33mTest Summary Complete�[0m
All tests have been executed. Check the results above.

[crivetimihai]

Looks good to merge, this will need further testing downstream, but for now - merging the PR.