-
Notifications
You must be signed in to change notification settings - Fork 700
feat(api2): validate and freeze dataclasses
#7716
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: 7639-apiv2-minor-versioning
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 enhances type safety and validation in the API v2 implementation by making dataclasses frozen (immutable), adding comprehensive validation in __post_init__ methods, and updating tests to properly instantiate immutable Event objects instead of mutating existing ones.
- Adds validation for version strings (must be 64 hex digits), UUIDs, and event IDs
- Makes Target, EventData, Event, EventResult, and BatchRequest dataclasses frozen to prevent mutation
- Updates test cases to create new Event instances instead of modifying existing ones
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| securedrop/journalist_app/api2/types.py | Adds VERSION_LEN constant, implements __post_init__ validation for all dataclasses, makes dataclasses frozen, and enhances type annotations |
| securedrop/tests/test_journalist_api2.py | Updates tests to create new Event instances rather than mutating existing ones, and uses VERSION_LEN constant for test data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8f717d0 to
ae351ff
Compare
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6533bc5 to
b4f274e
Compare
88d5991 to
e888fef
Compare
Thanks to ChatGPT.[^1] [^1]: https://chatgpt.com/share/691687e3-87b8-800f-b463-378fa7b3c593
b4f274e to
3eb21db
Compare
api2): validate and freeze dataclasses
api2):Prefer: securedrop=xlimits response shape to minor version2.x#7703Closes #7682 with two out of its three goals:
__post_init__()validationfrozen/api/v2/data: handle batched events from clients #7672 (comment)) to explicitly mark untrusted values as tainted until sanitizedNewTypes, and it's just not worth it. Consider this experience more evidence in support of Evaluate and implement improvements to code analysis #6111 and Add annotations for data sources and sinks and further static analysis securedrop-client#385.Overall I much prefer the declarative schema-based approach that the app has taken with Zod. Since we've decided to avoid further such dependencies on the server side, however, this has to be done separately and imperatively like this.
Test plan
Checklist
This change accounts for: