fix(api): move JSONPath modifier to query parameter with improved error handling#3159
Merged
crivetimihai merged 11 commits intomainfrom Mar 18, 2026
Merged
fix(api): move JSONPath modifier to query parameter with improved error handling#3159crivetimihai merged 11 commits intomainfrom
crivetimihai merged 11 commits intomainfrom
Conversation
066bdf4 to
2e43d5c
Compare
dce8465 to
85dabff
Compare
TS0713
previously approved these changes
Mar 9, 2026
Collaborator
TS0713
left a comment
There was a problem hiding this comment.
Review: apijsonpath query parameter change
Tested all edge cases for the apijsonpath query parameter change. All tests pass as expected.
Tests executed
| # | Test | Result |
|---|---|---|
| 1 | No apijsonpath — full object returned |
✅ |
| 2 | {"jsonpath":"$[*].name"} — filters names |
✅ |
| 3 | {"jsonpath":"$[*].id"} — filters IDs |
✅ |
| 4 | Mapping {"tool_name":"name","tool_id":"id"} — renames fields |
✅ |
| 5 | {"jsonpath":null} — defaults gracefully |
✅ |
| 6 | {"jsonpath":""} — empty string defaults to $[*] |
✅ |
| 7 | Invalid JSON string — returns 400 with clear message |
✅ |
| 8 | Unknown fields {"unexpected_field":123} — returns 400 Extra inputs are not permitted |
✅ |
| 9 | Invalid JSONPath syntax $$[invalid — returns 400 |
✅ |
| 10 | JSONPath matches nothing — returns [] |
✅ |
| 11 | Body on GET ignored — full object returned, not filtered | ✅ regression confirmed fixed |
| 12 | Access to non-existent resource ID — returns 403, consistent with existing access control behaviour across all endpoints |
✅ working as designed |
Verdict: Approved ✅
Member
|
Hi @crivetimihai , we need conflict resolution here. |
ef94a23 to
78a2198
Compare
TS0713
previously approved these changes
Mar 16, 2026
Collaborator
TS0713
left a comment
There was a problem hiding this comment.
Tested the updated implementation for /tools and /tools/{tool_id} endpoints. All scenarios behave as expected.
Tests executed
| # | Test | Result |
|---|---|---|
| 1 | No apijsonpath — full object returned |
✅ |
| 2 | {"jsonpath":"$[*].name"} — filters tool names |
✅ |
| 3 | {"jsonpath":"$[*].id"} — filters tool IDs |
✅ |
| 4 | Mapping {"tool_name":"name","tool_id":"id"} — renames fields correctly |
✅ |
| 5 | {"jsonpath":null} — defaults gracefully |
✅ |
| 6 | {"jsonpath":""} — empty string defaults to $[*] |
✅ |
| 7 | Invalid JSON string — returns 400 Bad Request | ✅ |
| 8 | Unknown fields {"unexpected_field":123} — returns 400 |
✅ |
| 9 | Invalid JSONPath syntax — returns 400 | ✅ |
| 10 | JSONPath matches nothing — returns [] |
✅ |
| 11 | Body on GET ignored — full object returned (regression fixed) | ✅ |
| 12 | Access to non-existent resource ID — returns 403 | ✅ |
| 13 | Pagination works with JSONPath (include_pagination=true) |
✅ |
Pagination verification
Verified pagination works correctly when JSONPath is used.
curl -G "$BASE/tools" \
-H "Authorization: Bearer $MY_BEARER" \
--data-urlencode "include_pagination=true" \
--data-urlencode 'apijsonpath={"jsonpath":"$[*].name"}'Approved!
e859a84 to
c0ae6e2
Compare
…points - Move apijsonpath parameter from request body to query string for list_tools and get_tool endpoints - Add _parse_apijsonpath() helper to handle both JSON string and JsonPathModifier inputs - Return ORJSONResponse to bypass FastAPI response_model validation when using JSONPath modifiers - Add comprehensive error handling with proper HTTPException status codes - Add test coverage for Query parameter handling and error cases This change resolves issue #2848 by making GET requests RESTful-compliant (no request body). Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
…re in apijsonpath parsing Address code review feedback with three security and performance improvements: 1. Specific Exception Handling (Issue #1) - Catch specific exceptions (json.JSONDecodeError, ValueError, ValidationError) - Separate user errors (400) from system errors (500) - Log unexpected errors with full context for debugging 2. Sanitize Exception Messages (Issue #2) - Return generic error messages in production (non-DEBUG mode) - Prevent information disclosure about internal types and stack traces - Detailed messages only shown when LOG_LEVEL=DEBUG for troubleshooting 3. Optimize Debug Logging (Issue #3) - Check if debug logging is enabled before building log message - Avoid string formatting overhead in production - Improves performance in high-traffic scenarios Updated test assertions to verify generic error messages (security improvement). All tests pass (18 jsonpath tests verified). Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
Signed-off-by: Rakhi Dutta <rakhibiswas@yahoo.com>
- Move _parse_apijsonpath() call before the database query in list_tools to fail fast on invalid input without wasting a DB round-trip - Add tests for the ValidationError path (extra fields rejected by extra="forbid" on JsonPathModifier) - Run black/isort on test file Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
… e2e tests - Fix pagination+JSONPath response to use "nextCursor" (camelCase) matching the CursorPaginatedToolsResponse alias contract. The hand-built dict bypassed Pydantic aliasing via ORJSONResponse, emitting "next_cursor" which broke any client relying on the documented field name. Confirmed regression against live server at localhost:8080. - Sanitize user-supplied JSONPath expression in debug log via SecurityValidator.sanitize_log_message() to prevent CWE-117 log forging (newlines, ANSI escapes in crafted jsonpath values). - Add end-to-end HTTP tests (TestApijsonpathHTTP) exercising the apijsonpath query parameter through FastAPI TestClient, covering the full HTTP binding path that __wrapped__() unit tests skip. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
34f3b08 to
1ba2c5b
Compare
Member
Author
Review Complete — Ready to mergeRebased onto current Changes made during review (2 commits)
Verification
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
closes #2848
This PR enhances the tools API endpoints (
/toolsand/tools/{tool_id}) by refactoring JSONPath modifier support from request body to query parameters, improving error handling, and adding comprehensive logging for better debugging.Changes
API Parameter Changes
apijsonpathparameter from request body to optional query parameter in bothlist_toolsandget_toolendpointsapijsonpathas either a string (HTTP requests) or model instance (internal calls/tests)Response Handling
ORJSONResponsewhen JSONPath modifiers are applied, bypassing FastAPI's response model validationError Handling
Test Updates
apijsonpathJsonPathModifiermodel instancesORJSONResponseobjects when modifiers are appliedDocumentation
Impact
These changes improve API flexibility by allowing JSONPath modifiers in GET requests, enhance reliability through better error handling, and improve maintainability with comprehensive logging and updated tests.
Testing
All existing unit tests have been updated and pass successfully. The changes maintain backward compatibility for internal calls while adding new query parameter support for HTTP requests.