-
Notifications
You must be signed in to change notification settings - Fork 244
Improved Error for Add Prompt and edit Prompt for Issue #361 and Issue #591 #629
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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]>
Signed-off-by: RAKHI DUTTA <[email protected]>
crivetimihai
approved these changes
Jul 30, 2025
There was a problem hiding this 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:
- Admin prompt endpoints now return JSON responses instead of redirects
- Error handling added with proper HTTP status codes (422 for validation, 409 for conflicts, 500 for server errors)
- Validation improvements with specific error messages for integrity constraints
- Frontend JavaScript updated to handle the new JSON response format
- 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):
- Logging improvement: The generic
logger.info(f"error,{ex}")
inadmin_add_server
could use a more descriptive message - Error message consistency: The PR adds database error formatting but some endpoints still have different error response structures
- 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:
- Consider creating a follow-up PR to standardize error responses across ALL admin endpoints
- Add integration tests to verify the frontend properly handles all error scenarios
- 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
- Removedrequired
attribute; check ifPromptUpdate
schema inschemas.py
validates non-empty - Null check missing:
static/admin.js:4293-4294
-formData.get("name")
needs null check beforevalidateInputName()
🔧 Technical Debt
- Duplicate code:
static/admin.js:4281-4382
-handlePromptFormSubmit
andhandleEditPromptFormSubmit
share 80% logic - Inconsistent API: Compare
admin.py:3164-3200
(prompts return JSON) vs other endpoints that still useRedirectResponse
- 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
inadmin.py
toJSONResponse
pattern
⚠️ Potential Issues
- Race condition:
services/prompt_service.py:221
- Removed duplicate check, only catchesIntegrityError
at line 260 - Test inconsistency:
tests/e2e/test_main_apis.py:453
accepts [400,409] buttest_admin_apis.py
expects exactly 409 - Frontend resilience:
static/admin.js:4308-4314
- Add timeout tofetchWithTimeout()
calls
This was referenced Aug 1, 2025
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[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