[Bug] GPS Fabric Connector: Logic errors and missing error handling #1608
Description
🐛 Bug Description
I found several logic errors and missing error handling in the GPS Fabric connector that could cause issues in production.
📍 Location
File: src/actions/gps/connector/fabric.py
🔍 Issues Found
1. Critical: Incorrect coordinate validation logic (Line ~65)
Current code:
if latitude is None and longitude is None and yaw is None:Problem: Uses and instead of or, so it only validates when ALL coordinates are None, not when ANY is None.
Impact: Partial coordinates could be sent to the network, causing data integrity issues.
2. Missing return value consistency
The send_coordinates() method returns None on error but has no explicit return on success.
3. Missing error handling
- No HTTP status code validation
- No JSON parsing error handling
- No handling for network timeouts or connection errors
- No retry mechanism for transient failures
4. Missing response validation
JSON-RPC error field is not checked in the response.
💡 Proposed Solution
I'd like to submit a PR with the following improvements:
- ✅ Fix coordinate validation logic (
orinstead ofand) - ✅ Add consistent boolean return values
- ✅ Add comprehensive error handling (HTTP errors, JSON parsing, timeouts)
- ✅ Add retry mechanism using
tenacity - ✅ Make timeout and retry configurable
- ✅ Add proper logging for all error cases
- ✅ Check JSON-RPC error field in responses
- ✅ Refactor into smaller, testable methods
🔧 Additional Changes
- Add
request_timeoutandmax_retriestoGPSFabricConfig - Extract coordinate retrieval to
_get_coordinates()method - Extract HTTP request to
_send_request()method with retry decorator
📦 Dependencies
This will add a dependency on tenacity for retry logic. If this is not acceptable, I can implement a simple retry mechanism without external dependencies.
🙋 I'd like to work on this
I'm ready to submit a PR with these fixes. Please let me know if this approach looks good or if you'd like me to adjust anything.