-
-
Notifications
You must be signed in to change notification settings - Fork 95
python: Better error checking for events from python #270
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
base: master
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR improves error handling and thread safety in the Python OSDP peripheral device implementation. The changes use context managers for lock management to ensure locks are released even when exceptions occur, and add validation for card read events to catch malformed event data early.
Key changes:
- Replaced manual
lock.acquire()/lock.release()calls with context manager pattern (with lock:) for exception safety - Added input validation to
submit_event()to verify event structure and required fields for card read events - Imported
EventandCardFormatconstants to support the new validation logic
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not isinstance(event, dict): | ||
| raise TypeError("event must be of type dict") | ||
|
|
||
| if not 'event' in event: |
Copilot
AI
Dec 22, 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.
The membership test should use the more idiomatic Python syntax 'event' not in event instead of not 'event' in event. While both are functionally equivalent, the former is the preferred style according to PEP 8.
| if not 'event' in event: | |
| if 'event' not in event: |
| expected_keys = ["reader_no", "format", "direction", "data"] | ||
| if event['format'] != CardFormat.ASCII: | ||
| expected_keys.append("length") | ||
| if not all(key in event for key in expected_keys): | ||
| raise TypeError("cardread event missing expected key") |
Copilot
AI
Dec 22, 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.
The code accesses event['format'] before validating that the 'format' key exists in the event dictionary. This will raise a KeyError if the 'format' key is missing. The validation should first check that all expected keys exist before attempting to access any of them, or should check for the 'format' key existence before accessing it on line 76.
| expected_keys = ["reader_no", "format", "direction", "data"] | |
| if event['format'] != CardFormat.ASCII: | |
| expected_keys.append("length") | |
| if not all(key in event for key in expected_keys): | |
| raise TypeError("cardread event missing expected key") | |
| # Base required keys for all CardRead events | |
| expected_keys = ["reader_no", "format", "direction", "data"] | |
| if not all(key in event for key in expected_keys): | |
| raise TypeError("cardread event missing expected key") | |
| # At this point, 'format' is guaranteed to exist | |
| if event["format"] != CardFormat.ASCII and "length" not in event: | |
| raise TypeError("cardread event missing expected key: 'length'") |
| if event['format'] != CardFormat.ASCII: | ||
| expected_keys.append("length") | ||
| if not all(key in event for key in expected_keys): | ||
| raise TypeError("cardread event missing expected key") |
Copilot
AI
Dec 22, 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.
The error message does not specify which key is missing from the cardread event, making it difficult for users to debug the issue. Consider providing a more specific error message that indicates which expected key is missing, such as including the list of expected keys and the keys that are actually present.
| raise TypeError("cardread event missing expected key") | |
| missing_keys = [key for key in expected_keys if key not in event] | |
| present_keys = list(event.keys()) | |
| raise TypeError( | |
| "cardread event missing expected key(s): " | |
| f"{missing_keys}; expected keys: {expected_keys}; present keys: {present_keys}" | |
| ) |
|
@rm5248 Please ignore the AI review, I'm just trying it out. |
Use a context manager when locking in python to ensure that if an exception is thrown, the lock will be released.
Add some basic error checking to the
submit_eventfunction to make sure that card read events have the correct data set.fixes: #267