Skip to content

Conversation

rakdutta
Copy link
Collaborator

[CHORE]: Improve Error Messages - Replace Raw Technical Errors with User-Friendly Messages

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

Updated admin_add_prompt and admin_edit_prompt functions to handle ValidationError and IntegrityError exceptions
Updated mcpgateway/services/prompt_service.py.
Updated the admin.js and admin.html file to enable redirection to the correct page.

Removed explicit check for existing prompt name; now relying on database-level UNIQUE constraint and IntegrityError handling.
Update the below test file to handle new error code for Add prompt and Edit Prompt function
1)test_admin_api.py
2)test_admin.py
3)test_main.py
4)test_prompt_service.py
5)tests/e2e/test_main_apis.py (test case fixed for add_server router)

Address the issue https://github.com/IBM/mcp-context-forge/issues/591([Bug] Edit Prompt Fails When Template Field Is Empty]
Removed Required from template field for id=edit-prompt-template from admin.html

rakdutta1 added 12 commits July 23, 2025 12:17
Signed-off-by: RAKHI DUTTA <[email protected]>
Signed-off-by: RAKHI DUTTA <[email protected]>
Signed-off-by: RAKHI DUTTA <[email protected]>
Signed-off-by: RAKHI DUTTA <[email protected]>
Signed-off-by: RAKHI DUTTA <[email protected]>
Signed-off-by: RAKHI DUTTA <[email protected]>
Signed-off-by: RAKHI DUTTA <[email protected]>
Signed-off-by: RAKHI DUTTA <[email protected]>
Signed-off-by: RAKHI DUTTA <[email protected]>
Signed-off-by: RAKHI DUTTA <[email protected]>
Signed-off-by: RAKHI DUTTA <[email protected]>
Signed-off-by: RAKHI DUTTA <[email protected]>
@rakdutta rakdutta changed the title Dev 363 prompt Improved Error for Add Prompt and edit Prompt for Issue #361 and Issue #591 Jul 29, 2025
@rakdutta rakdutta marked this pull request as ready for review July 29, 2025 05:25
Copy link
Member

@crivetimihai crivetimihai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passed

  • make doctest test smoketest pylint flake8 ruff lint-web

Summary

This PR addresses issue #340 by improving error handling and converting prompt endpoints from redirect responses to JSON responses. The changes align with the goal of providing better user feedback and consistent error formatting.

Key Changes Reviewed:

  1. Admin prompt endpoints now return JSON responses instead of redirects
  2. Error handling added with proper HTTP status codes (422 for validation, 409 for conflicts, 500 for server errors)
  3. Validation improvements with specific error messages for integrity constraints
  4. Frontend JavaScript updated to handle the new JSON response format
  5. Test cases updated to reflect the new response types

✅ Strengths:

  • Consistent error response format across endpoints
  • Proper HTTP status codes (409 for conflicts instead of generic 400/500)
  • Database integrity errors are properly caught and formatted
  • Frontend gracefully handles both success and error cases
  • Comprehensive test coverage updated

🟡 Minor Observations (non-blocking):

  1. Logging improvement: The generic logger.info(f"error,{ex}") in admin_add_server could use a more descriptive message
  2. Error message consistency: The PR adds database error formatting but some endpoints still have different error response structures
  3. Template validation: The required attribute was removed from the template textarea in the edit form - ensure this is intentional

🔍 Security Considerations:

  • ✅ No SQL or schema details exposed in error responses
  • ✅ Proper sanitization of user inputs maintained
  • ✅ Authentication checks remain in place

📊 Test Coverage:

  • ✅ Unit tests updated for new response formats
  • ✅ E2E tests modified to expect JSON responses
  • ✅ Error scenarios properly tested

Post-merge suggestions:

  1. Consider creating a follow-up PR to standardize error responses across ALL admin endpoints
  2. Add integration tests to verify the frontend properly handles all error scenarios
  3. Document the new JSON response format

Post-Merge Code Review Checklist

🐛 Fix Required

  • Bad error logging: admin.py:439 - logger.info(f"error,{ex}")logger.error(f"Error in admin_add_server: {ex}")
  • Template validation gap: templates/admin.html:2718 - Removed required attribute; check if PromptUpdate schema in schemas.py validates non-empty
  • Null check missing: static/admin.js:4293-4294 - formData.get("name") needs null check before validateInputName()

🔧 Technical Debt

  • Duplicate code: static/admin.js:4281-4382 - handlePromptFormSubmit and handleEditPromptFormSubmit share 80% logic
  • Inconsistent API: Compare admin.py:3164-3200 (prompts return JSON) vs other endpoints that still use RedirectResponse
  • Dead code: static/admin.js:4334 - Remove commented // location.reload();

📋 Follow-up PRs

  • Extend error handling: Apply pattern from admin.py:3190-3209 to Server (admin.py:436), Tool, and Resource endpoints
  • Complete ErrorFormatter: utils/error_formatter.py:244-306 - Add patterns for CHECK constraints, FK cascades
  • Standardize admin API: Convert all RedirectResponse in admin.py to JSONResponse pattern

⚠️ Potential Issues

  • Race condition: services/prompt_service.py:221 - Removed duplicate check, only catches IntegrityError at line 260
  • Test inconsistency: tests/e2e/test_main_apis.py:453 accepts [400,409] but test_admin_apis.py expects exactly 409
  • Frontend resilience: static/admin.js:4308-4314 - Add timeout to fetchWithTimeout() calls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants