Address code review feedback: fix SQL injection, QueueMetrics bug, reduce boilerplate#22
Merged
jason810496 merged 6 commits intofeature/add-transaction-classfrom Jan 2, 2026
Merged
Conversation
Co-authored-by: jason810496 <[email protected]>
Co-authored-by: jason810496 <[email protected]>
Co-authored-by: jason810496 <[email protected]>
Co-authored-by: jason810496 <[email protected]>
Co-authored-by: jason810496 <[email protected]>
Copilot
AI
changed the title
[WIP] Address feedback from code review on op module
Address code review feedback: fix SQL injection, QueueMetrics bug, reduce boilerplate
Jan 2, 2026
jason810496
added a commit
that referenced
this pull request
Jan 3, 2026
* Add transation friendly op staticmethod class * Refactor PGMQueue methods to utilize PGMQOperation for extension checks, queue creation, validation, and message handling - Replaced direct SQL execution with calls to PGMQOperation methods for checking pgmq and pg_partman extensions. - Consolidated queue creation, validation, and message sending methods to use PGMQOperation for both sync and async operations. - Updated method signatures to include optional session and commit parameters for better session management. - Removed redundant sync and async methods, streamlining the codebase and improving maintainability. * Add empty test_operation * Move _validate_partition_interval to operations * Address code review feedback: fix SQL injection, QueueMetrics bug, reduce boilerplate (#22) * Initial plan * Fix SQL injection vulnerability and QueueMetrics bug in operation.py Co-authored-by: jason810496 <[email protected]> * Refactor queue.py to reduce boilerplate with _execute_operation helper Co-authored-by: jason810496 <[email protected]> * Add comprehensive tests for PGMQOperation class Co-authored-by: jason810496 <[email protected]> * Fix json import and send_batch parameter handling Co-authored-by: jason810496 <[email protected]> * Fix test exception handling to be more specific Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Improve test coverage for operation module and fix SQL parameterization bugs (#23) * Initial plan * Add comprehensive test coverage for PGMQOperation methods - Added tests for delete_batch (sync and async) - Added tests for archive_batch (sync and async) - Added tests for purge (sync and async) - Added tests for read_with_poll (sync and async) - Added tests for drop_queue (sync and async) - Added tests for check_pg_partman_ext (sync and async) - Added tests for create_partitioned_queue (sync and async) - Added tests for time-based partitioned queues Note: Tests currently fail due to SQL parameterization bug in operation.py where mixed parameter styles (:param and %(param)s) cause syntax errors. This was introduced in commit 849a8f1. Co-authored-by: jason810496 <[email protected]> * Fix SQL parameterization bugs and improve partition interval validation - Fixed SQL injection fix by using CAST(:param AS type) instead of :param::type - Fixed delete_batch and archive_batch to return message IDs instead of boolean - Improved partition interval validation to accept numeric strings - Added validation to sync create_partitioned_queue for consistency - Fixed queue name length in partitioned queue tests - All new tests now pass successfully Co-authored-by: jason810496 <[email protected]> * Fix send_batch array formatting and validate_queue_name test - Fixed send_batch to properly escape JSON in PostgreSQL array literal format - Fixed test_validate_queue_name to handle both ProgrammingError and InternalError - All 103 operation tests now pass successfully Co-authored-by: jason810496 <[email protected]> * Address code review feedback - Move imports to top of file (time, InternalError) - Add documentation comment explaining array literal escaping approach - All tests still passing Co-authored-by: jason810496 <[email protected]> * Improve import ordering and documentation - Fix import grouping (stdlib imports together) - Enhance documentation for array literal escaping with security notes - All tests still passing Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Fix PGMQueue with _execute_operation for rest of the methods * Fix Jsonb format * Fix CI to properly report test failures and refactor test fixtures (#24) * Initial plan * Fix CI configuration to properly report test failures Co-authored-by: jason810496 <[email protected]> * Refactor test fixtures to remove lazy fixtures and use dynamic parametrization Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Fix JSONB parameter binding to prevent psycopg2 dict adaptation errors (#25) * Initial plan * Fix JSONB handling with bindparams for send and send_batch operations Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Fix partition interval validation for negative numeric strings (#26) * Initial plan * Add read_archive methods to PGMQOperation and PGMQueue Co-authored-by: jason810496 <[email protected]> * Fix partition interval validation for negative numeric strings Co-authored-by: jason810496 <[email protected]> * Apply ruff formatting fixes Co-authored-by: jason810496 <[email protected]> * Remove read_archive methods and tests per spec clarification Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Add pytest-asyncio * Fix ambiguous column references in delete and archive SQL statements (#27) * Initial plan * Fix ambiguous column references in delete_batch and archive_batch Co-authored-by: jason810496 <[email protected]> * Add aliases to delete and archive statements to avoid ambiguity Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Fix ambiguous error manually Fix type hint * Fix bindparams usage Fix typ hint * Try fix text to varchar wrong cast error, remove drop_archive test * Fix text type casting * Fix sqlalchemy.exc.ArgumentError * Fix pgmq.delete_bactch and pgmq.archive_batch * Fix missing pgmq_all_variants fixture import in test_queue.py (#28) * Initial plan * Fix pgmq_all_variants fixture import in test_queue.py Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> * Improve test coverage for `op` module based on review feedback (#29) * Initial plan * Add tests for _execute_operation with provided session and read_with_poll without vt Co-authored-by: jason810496 <[email protected]> * Add test for async _execute_operation path when session is None Co-authored-by: jason810496 <[email protected]> * Remove fragile line number references from test comments Co-authored-by: jason810496 <[email protected]> * Clarify comment in test_read_with_poll_without_vt Co-authored-by: jason810496 <[email protected]> * Address final code review feedback: improve test structure and add cleanup verification Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: Copilot <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Addresses three code review comments on PR #21:
1. Fixed SQL injection vulnerability in send operations
_get_send_statementand_get_send_batch_statementsend,send_async,send_batch,send_batch_asyncto pass parameters toexecute()Before:
After:
2. Fixed QueueMetrics initialization error
scrape_timeparameter from all metrics methods3. Reduced boilerplate in queue.py
_execute_operationhelper to eliminate sync/async session management duplicationcreate_queue,create_partitioned_queue,validate_queue_name,drop_queue,list_queues,send,send_batch4. Added comprehensive tests for PGMQOperation
tests/test_operation.pywith 18+ test cases covering sync and async static methodsStatus
Checklist
pre-commitwithruff)💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.