Repository navigation
feat(postgres_persistence): add single-row constraint with auto-migration - #137
Conversation
…tion. Context: current implementation may cause race condition, resulting in multiple duplicated rows being inserted into the persistence table. Changes in this commit: Enforce single-row persistence table using id column with CHECK constraint. Replace INSERT with UPSERT pattern. Add automatic migration from old schema with full backward compatibility. Includes comprehensive test coverage.
starry-shivam
left a comment
starry-shivam
left a comment
There was a problem hiding this comment.
These changes look really good overall and make database management much more robust and clean. I just had one suggestion, which I’ve described below.
However, while looking through this, I noticed an unrelated issue that’s been present from before your PR. In _dump_into_json(), the callback data is dumped with the key "callback_data", but during initialization, the code tries to load it from a non-existent key called "callback_data_json". This means the callback data never actually gets loaded... I don’t quite remember what kind of data callback_data is supposed to store as it’s been a while since I did bot development, but it seems this part of the Postgres persistence hasn’t been functioning correctly for a while. So, while you’re at it, maybe you could fix that as well?
Also, I just wanted to add that I really liked your idea of calling _session.rollback() instead of closing the session when there’s an error while updating the database.
…id column existence and whether it is a primary key and of the right data type. Also added check for valid row with id=1). Added further tests for migration detection and execution for more comprehensive coverage. Fixed callback_data key mismatch bug
f675347 to
86d2d9a
Compare
|
Hey. was the deletion of the test files intentional? if not, could you kindly revert that? Otherwise I'm happy to merge :) |
Ah, definitely not intentional. I think the local directory got synced to my OneDrive and the test files showed up as images in my photos library so I accidentally deleted them. Have restored them now in the latest commit |
|
Thanks very much for the contribution! |
3 participants
Purpose of branch: add single-row constraint with auto-migration for postgres_persistence implementation.
Context: current implementation may cause race condition, resulting in multiple duplicated rows being inserted into the persistence table.
Changes in this commit: