feat(store): add ConversationItemStore trait and implementations#645
feat(store): add ConversationItemStore trait and implementations#645leseb wants to merge 1 commit into
Conversation
|
PR too large: 1341 lines added (limit: 750, excludes Cargo files, tests, docs, examples, and benchmarks). Please split into smaller PRs. Add |
|
AI tool authorship detected:
Sorry, this project does not accept commits authored by tools as valid. |
leseb
force-pushed
the
leseb/issue-631-conversation-item-store
branch
from
0e7e609 to
506674e
Compare
Add a separate ConversationItemStore trait for conversation item CRUD operations (create, get, list, delete) with cursor-based pagination using composite (position, item_id) ordering. Implement for both SQLite and PostgreSQL backends with ON CONFLICT upsert semantics. Conversation deletion now cascade-deletes associated items within a transaction when the items table is configured. Closes praxis-proxy#631 Signed-off-by: Sébastien Han <seb@redhat.com>
leseb
force-pushed
the
leseb/issue-631-conversation-item-store
branch
from
506674e to
1bff360
Compare
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: Adds a ConversationItemStore trait with SQLite and PostgreSQL implementations for persisting OpenAI Responses API conversation items.
Overall: Clean trait design with good test coverage for the happy paths. The main concern is transactional safety in create_conversation_items and the upsert conflict key design.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 1 |
| Medium | 2 |
| limit: u32, | ||
| ascending: bool, | ||
| ) -> Result<Vec<ConversationItemRecord>, StoreError> { | ||
| let table = self |
There was a problem hiding this comment.
[Large] create_conversation_items inserts items one-by-one in a loop without wrapping the batch in a transaction. If the third of five inserts fails, the first two are already committed — leaving the store in a partial state with no way for the caller to know which items succeeded. Wrap the loop in conn.call(move |conn| { let tx = conn.transaction()?; ... tx.commit()?; }) so the batch is atomic.
| .await | ||
| .map_err(|e| StoreError::Database(e.to_string()))?; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Medium] The upsert conflict key is (item_id, tenant_id), but conversation_id is in the UPDATE SET clause. This means an item with the same item_id and tenant_id but a different conversation_id silently migrates to the new conversation on conflict, rather than being rejected. If cross-conversation item migration is not intended, add conversation_id to the conflict target or add a check constraint.
| remaining.is_empty(), | ||
| "items should be cascade-deleted with conversation" | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Medium] No test exercises the StoreError::Unavailable error path — i.e., calling ConversationItemStore methods on a store instance where items_table was not configured. Add a negative test that constructs a store without the items table and asserts that each method returns StoreError::Unavailable.
2 participants
Summary
ConversationItemStoretrait with 7 item CRUD methods: create (batch upsert), list (cursor-based pagination with composite(position, item_id)ordering), get, delete single item, get position, max position, and delete all items for a conversationON CONFLICTupsert semanticsdelete_conversationis called and items table is configuredDATABASE_URL) covering pagination, duplicate positions, tenant isolation, upsert, field round-tripping, nonexistent cursor handling, and cascade deleteTest plan
cargo test -p praxis-proxy-filter -- store— 215 passed, 24 ignoredcargo clippy -p praxis-proxy-filter -- -D warnings— cleancargo +nightly fmt— cleanCloses #631
🤖 Generated with Claude Code