Skip to content

Conversation

@jfrench9
Copy link
Member

Summary

This PR significantly enhances the robustness and reliability of the RoboSystems API client by implementing comprehensive error handling improvements and safer URL parameter handling across all API endpoints.

Key Accomplishments

  • Enhanced Error Handling: Improved HTTP response error handling in the QueryClient to provide better error messages and more graceful failure modes
  • Safer URL Parameter Handling: Updated URL formatting across all API calls to use proper quoting mechanisms, preventing issues with special characters and improving security
  • Comprehensive Coverage: Applied improvements consistently across 80 API endpoint files covering all major service areas:
    • Agent operations (auto-select, batch processing, execution, metadata)
    • Backup and restore functionality
    • Billing and subscription management
    • Connection management and OAuth flows
    • Credits and storage monitoring
    • File operations
    • Graph health, info, and limits
    • Materialization processes
    • MCP tool integration
    • Operations management
    • Organization and member management
    • Query execution
    • Schema operations
    • Subgraph management
    • Subscription handling
    • Table operations
    • Usage analytics
    • User management
    • View operations

Breaking Changes

None. These changes are backward-compatible and only enhance the existing functionality without modifying public APIs.

Testing Notes

  • All existing API functionality should continue to work as expected
  • Error responses should now provide more informative messages
  • URLs with special characters in parameters should be handled more reliably
  • Test cases involving malformed parameters or edge cases should show improved behavior

Infrastructure Considerations

  • The QueryClient improvements may change the format of error messages, which could affect logging and monitoring
  • URL encoding changes ensure better compliance with HTTP standards and may resolve issues with certain parameter values
  • These changes improve the overall stability and security posture of API communications

🤖 Generated with Claude Code

Branch Info:

  • Source: feature/more-robust-gen
  • Target: main
  • Type: feature

Co-Authored-By: Claude noreply@anthropic.com

… parameter handling

## Summary
This commit refactors multiple API endpoint definitions across various modules to utilize the `quote` function from `urllib.parse`. This change ensures that URL parameters are properly encoded, enhancing the safety and reliability of API calls.

## Key Changes
- Updated URL formatting in API calls to replace f-strings with `.format()` method using `quote` for `graph_id`, `agent_type`, `backup_id`, `org_id`, `subscription_id`, `user_id`, and other parameters.
- Ensured consistent handling of special characters in URLs to prevent potential issues with API requests.

## Files Affected
- Multiple files across the `agent`, `backup`, `billing`, `connections`, `credits`, `files`, `graph_health`, `graph_info`, `graph_limits`, `org`, `org_members`, `org_usage`, `query`, `schema`, `subgraphs`, `subscriptions`, `tables`, `usage`, and `user` modules.

## Testing Notes
- Verify that all API calls function correctly with the updated URL formatting.
- Ensure that any existing tests cover the modified endpoints to validate the changes.

## Infrastructure Considerations
This change improves the robustness of API interactions by ensuring that all parameters are safely encoded, reducing the risk of errors during API requests.
## Summary
This commit enhances the error handling mechanism in the `QueryClient` class to better manage HTTP error responses (4xx/5xx). It provides clearer error messages by parsing the response content and extracting relevant details.

## Key Changes
- Added logic to handle error responses where the parsed response is `None`.
- Improved exception messages to include HTTP status codes and detailed error information from the response body.

## Testing Notes
- Verify that the error handling works correctly for various HTTP error scenarios.
- Ensure that the exceptions raised provide meaningful information for debugging.

## Infrastructure Considerations
This change improves the robustness of the `QueryClient` by ensuring that errors are reported with more context, aiding in troubleshooting and enhancing overall reliability.
@jfrench9
Copy link
Member Author

@claude please review this PR

@claude
Copy link

claude bot commented Jan 31, 2026

Claude finished @jfrench9's task in 1m 51s —— View job


PR Review Complete

Tasks

  • Read repository guidelines (CLAUDE.md)
  • Review QueryClient error handling changes
  • Review URL parameter handling pattern
  • Sample API endpoint changes
  • Analyze security implications
  • Provide comprehensive feedback

Summary

This PR implements two key improvements to the RoboSystems API client:

  1. Enhanced error handling in QueryClient for better HTTP error responses
  2. URL parameter encoding using urllib.parse.quote across all 80 API endpoints

Both changes are well-implemented and improve the robustness of the client. However, there are a few considerations worth discussing.


✅ Strengths

1. Comprehensive URL Encoding (robosystems_client/api/**/*)

  • Consistent implementation: All 79 API endpoint files properly use quote(str(param), safe="")
  • Security improvement: Using safe="" ensures all special characters are encoded, preventing URL injection issues
  • Proper encoding: Correctly encodes parameters like graph_id, file_id, agent_type, etc.

Example pattern (consistently applied):

from urllib.parse import quote

"url": "/v1/graphs/{graph_id}/agent/{agent_type}".format(
    graph_id=quote(str(graph_id), safe=""),
    agent_type=quote(str(agent_type), safe=""),
)

2. Improved Error Handling (robosystems_client/extensions/query_client.py:269-284)

  • Better error messages: Extracts meaningful error details from HTTP response bodies
  • Graceful degradation: Falls back to status code if parsing fails
  • Proper exception handling: Catches JSON decode errors appropriately

⚠️ Issues & Recommendations

1. Potential Double-Encoding Risk (Medium Priority)

The URL encoding is applied in the _get_kwargs() function, but it's important to verify that the HTTP client (httpx) doesn't perform additional URL encoding. Double-encoding could cause issues like:

  • /v1/graphs/my-graph/v1/graphs/my%2Dgraph/v1/graphs/my%252Dgraph (double-encoded)

Recommendation:

  • Add integration tests with special characters (spaces, slashes, Unicode) to verify correct encoding
  • Check httpx documentation to confirm it doesn't re-encode the URL path
  • Consider testing with parameters like: "test graph", "test/graph", "test@#$%", "测试"

Test suggestion:

def test_url_encoding_with_special_chars():
    # Test that parameters with special chars are encoded correctly
    graph_id = "my-graph/test"
    # Should encode to: /v1/graphs/my%2Dgraph%2Ftest

2. Inconsistent Error Handling Flow (Low Priority)

In query_client.py:256-287, there's a subtle issue with error handling flow:

except Exception as e:
    # ... handle exception (lines 256-267)

# This code is unreachable if exception is raised!
if hasattr(response, "status_code") and response.status_code >= 400:
    # ... (lines 269-284)

Issue: The HTTP error handling block (lines 269-284) appears after the try/except, which means:

  • If the execute_cypher_query() call succeeds, the error handling works
  • But the flow is confusing because it suggests error responses might not raise exceptions

Recommendation:

  • Clarify whether execute_cypher_query() raises exceptions for HTTP errors or returns error responses
  • If it returns responses, the current code is correct but could use a comment
  • If it raises exceptions, the lines 269-284 might be unreachable dead code

3. Missing Test Coverage (Medium Priority)

The changes don't include tests for:

  • URL encoding behavior with edge cases
  • HTTP error response parsing in QueryClient
  • Error message extraction from different response formats

Recommendation:
Add tests like:

# Test URL encoding
def test_special_chars_in_graph_id():
    """Verify special characters are properly encoded in URLs"""
    
# Test error handling
def test_query_client_parses_error_responses():
    """Verify error details are extracted from HTTP error responses"""

4. Hardcoded safe="" Parameter (Low Priority)

All URL encoding uses safe="", which encodes everything including / characters. This is correct for path parameters but worth noting:

quote(str(graph_id), safe="")

Why this matters:

  • safe="" means even forward slashes are encoded
  • This is correct for path parameters (like graph_id) since they shouldn't contain /
  • But it's worth documenting this choice

Recommendation:

  • Current implementation is correct - no changes needed
  • Consider adding a code comment explaining the safe="" choice

5. Error Message Format Change (Low Priority)

The new error handling changes error message format from:

# Before: Generic httpx errors
"Connection failed"

# After: Structured format  
"Query failed (404): Graph not found"

Impact:

  • Any code that parses error messages might break
  • Logging/monitoring dashboards might need updates
  • This is mentioned in the PR description ("Infrastructure Considerations")

Recommendation:

  • ✅ Already documented in PR description
  • Consider if error types should be custom exceptions instead of generic Exception

🔍 Security Analysis

URL Encoding (✅ Good)

  • Using quote(str(param), safe="") is secure and prevents URL injection
  • Encoding all special characters eliminates ambiguity
  • No obvious security vulnerabilities

Error Handling (✅ Good)

  • Error messages include response details but not sensitive data
  • Falls back gracefully on parsing failures
  • Doesn't expose internal stack traces in error messages

📋 Minor Observations

  1. Consistent import placement: All files correctly import quote from urllib.parse at the top
  2. Type conversion: Using str(param) before encoding handles non-string types safely
  3. Generated code: These appear to be auto-generated files (consistent patterns across 80 files)

✅ Conclusion

Overall Assessment: This PR successfully improves error handling and URL parameter safety across the entire API client. The implementation is consistent and follows best practices.

Recommendation: ✅ Approve with minor suggestions

Before merging, consider:

  1. Adding tests for URL encoding edge cases (special characters, Unicode, etc.)
  2. Adding tests for error response parsing
  3. Verifying no double-encoding occurs with httpx
  4. Clarifying the error handling flow in QueryClient (lines 269-287)

Risk Level: Low - Changes are backward-compatible and improve robustness


📝 Optional Improvements (Future Work)

If you want to make the error handling even more robust:

  1. Custom exception types instead of generic Exception:

    class QueryExecutionError(Exception):
        def __init__(self, status_code, detail):
            self.status_code = status_code
            self.detail = detail
            super().__init__(f"Query failed ({status_code}): {detail}")
  2. Structured error responses for easier parsing:

    @dataclass
    class ErrorDetail:
        status_code: int
        message: str
        detail: Optional[Dict[str, Any]] = None
  3. URL encoding helper to centralize the logic:

    def encode_path_param(value: Any) -> str:
        """Encode a path parameter for use in URLs"""
        return quote(str(value), safe="")

Great work on this PR! The improvements are solid and will make the client more robust.

@jfrench9 jfrench9 merged commit 2ccacd7 into main Jan 31, 2026
1 check passed
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.

2 participants