-
Notifications
You must be signed in to change notification settings - Fork 701
feat(api2): support conversation truncation via source_conversation_truncated event
#7723
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
Conversation
09f3229 to
fabbe9d
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
This PR implements support for the SOURCE_CONVERSATION_TRUNCATED event type in API v2, which allows selective deletion of items in a source's collection based on interaction counts. The key changes enable partial conversation cleanup without requiring strict versioning.
- Adds
SOURCE_CONVERSATION_TRUNCATEDevent type with associated data class containing anupper_boundparameter - Introduces
MultiStatus(207) status code for operations where some deletions succeed and others fail - Implements
handle_source_conversation_truncatedhandler that deletes items with interaction_count ≤ upper_bound - Adds error handling for
delete_collectionfailures withInternalServerError(500) status code - Updates API documentation to reflect MultiStatus in the event processing state machine
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| securedrop/journalist_app/api2/types.py | Adds SOURCE_CONVERSATION_TRUNCATED event type, SourceConversationTruncatedData class, MultiStatus and InternalServerError status codes |
| securedrop/journalist_app/api2/events.py | Implements handle_source_conversation_truncated handler, adds error handling to handle_source_deleted, registers new event handler |
| API2.md | Updates state machine diagram to include MultiStatus status code in idempotency and success branches |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f8ca800 to
4373340
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 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4373340 to
8a15184
Compare
api2): support conversation truncation via source_convresation_truncated event
api2): support conversation truncation via source_convresation_truncated eventapi2): support conversation truncation via source_conversation_truncated event
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 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A "source_conversation_truncated" event involves deleting all the items in the source's collection with interaction counts less than or equal to the specified upper bound, assumed to be the last item known to the client. This achieves the same consistency as a "source_conversation_deleted" event without requiring its strict versioning.
…ng for consistency
Thanks to ChatGPT.[^1] [^1]: https://chatgpt.com/share/6920be5d-c878-800f-aac4-0aecaad8399f
8a15184 to
1a07cbd
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 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aa681cc to
6c280c0
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 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8fe93da to
115d03f
Compare
legoktm
left a comment
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.
Looks good, just one minor comment.
I assume that once the app is migrated we'll get rid of SOURCE_CONVERSATION_DELETED?
…t() is non-atomic
115d03f to
16b7336
Compare
Might as well. Truncation should be easier both to understand and implement, so I don't see a compelling reason to keep deletion around. |
legoktm
left a comment
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.
Thanks!
Closes #7707 by adding a
source_conversation_truncatedevent that deletes items in a source's collection up to and including the specified interaction count.While I was here, I also added more try/catch logic around exceptions from utility functions for parity with the v1 API endpoints.
Test plan
Does this give us everything we want for a "source conversation deleted"‒like event without requiring strict versioning?
Checklist
This change accounts for: