sqlite3: raise ProgrammingError when named param receives sequence - #8363
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSQLite sequence parameter binding now checks parameter names and raises ChangesSQLite parameter binding
Estimated code review effort: 2 (Simple) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.3).cspell.jsonFile contains syntax errors that prevent linting: Line 1: Expected an array, an object, or a literal but instead found '// See: https://github.com/streetsidesoftware/cspell/tree/; Line 6: Expected an array, an object, or a literal but instead found '// " ... [truncated 1271 characters] ... ; Line 102: End of file expected; Line 103: End of file expected; Line 103: End of file expected; Line 104: End of file expected; Line 104: End of file expected; Line 105: End of file expected; Line 105: End of file expected; Line 106: End of file expected; Line 106: End of file expected; Line 107: End of file expected; Line 107: End of file expected; Line 109: End of file expected; Line 110: End of file expected; Line 112: End of file expected; Line 112: End of file expected; Line 112: End of file expected; Line 113: End of file expected; Line 114: End of file expected; Line 114: End of file expected; Line 114: End of file expected; Line 115: End of file expected; Line 117: End of file expected; Line 117: End of file expected; Line 117: End of file expected; Line 123: End of file expected Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/sqlite3 dependencies:
dependent tests: (2 tests)
[x] lib: cpython/Lib/ast.py dependencies:
dependent tests: (149 tests)
Legend:
|
There was a problem hiding this comment.
Pull request overview
This PR aligns RustPython’s sqlite3 parameter binding behavior with CPython by raising ProgrammingError when a statement contains named placeholders (e.g., :a, :b) but the caller supplies a positional sequence instead of a mapping.
Changes:
- Add a named-parameter check to
bind_parameters_sequence()usingsqlite3_bind_parameter_name()and raiseProgrammingErrorwhen a named placeholder is detected. - Unmark
test_execute_named_param_and_sequenceasexpectedFailurenow that RustPython enforces the same validation as CPython.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/stdlib/src/_sqlite3.rs |
Adds named-placeholder detection during sequence binding and raises ProgrammingError early for mixed named/positional usage with a sequence. |
Lib/test/test_sqlite3/test_dbapi.py |
Removes the RustPython expectedFailure marker for the named-param-with-sequence validation test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let name = unsafe { sqlite3_bind_parameter_name(self.st, i) }; | ||
| if !name.is_null() && unsafe { *name } != b'?' as i8 { | ||
| return Err(new_programming_error( | ||
| vm, | ||
| format!("Binding {i} is a named parameter, but you supplied a sequence instead of a dict."), | ||
| )); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/stdlib/src/_sqlite3.rs (1)
3206-3211: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the
c_charcomparison target-independent.
sqlite3_bind_parameter_name()returns*const c_char, whileb'?' as i8assumesc_charis signed. Rust defines Ccharas platform-dependent (i8oru8), so this can fail to compile on supported targets using unsignedchar. (doc.rust-lang.org)Suggested fix
- if !name.is_null() && unsafe { *name } != b'?' as i8 { + if !name.is_null() && unsafe { *name as u8 } != b'?' {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stdlib/src/_sqlite3.rs` around lines 3206 - 3211, Update the comparison in the parameter-name validation around sqlite3_bind_parameter_name to use the platform’s c_char type rather than casting b'?' to i8. Preserve the existing null check and error behavior while making the comparison compile for both signed- and unsigned-char targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/stdlib/src/_sqlite3.rs`:
- Around line 3206-3211: Update the comparison in the parameter-name validation
around sqlite3_bind_parameter_name to use the platform’s c_char type rather than
casting b'?' to i8. Preserve the existing null check and error behavior while
making the comparison compile for both signed- and unsigned-char targets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2315ca3c-c85e-4d0e-9356-6df47741f3a2
⛔ Files ignored due to path filters (1)
Lib/test/test_sqlite3/test_dbapi.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/stdlib/src/_sqlite3.rs
68b534d to
45c9cc1
Compare
45c9cc1 to
872ecd7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/stdlib/src/_sqlite3.rs:3207
- The comparison
unsafe { *name } != b'?' as i8assumesc_charis signed. On targets wherelibc::c_charisu8, this can fail to compile (or lead to incorrect casts). Cast the literal tolibc::c_char(or compare asu8) to keep this portable.
if !name.is_null() && unsafe { *name } != b'?' as libc::c_char {
872ecd7 to
1508cb1
Compare
| format!( | ||
| "Binding {i} ('{name_str}') is a named parameter, but you \ | ||
| supplied a sequence which requires nameless (qmark) placeholders." | ||
| ), |
1508cb1 to
de95ea8
Compare
de95ea8 to
15ee651
Compare
CPython raises ProgrammingError with "Binding N is a named parameter" when a query uses named placeholders (, , ) but the caller passes a sequence instead of a mapping. RustPython's bind_parameters_sequence() did not check whether each binding slot is a named parameter, so no error was raised. Fix: call sqlite3_bind_parameter_name() for each slot in bind_parameters_sequence(). If the first byte of the returned name is not '?' (i.e. it is a named placeholder), raise ProgrammingError before attempting to bind. Assisted-by: GitHub Copilot:claude-sonnet-4-6
15ee651 to
09715f1
Compare
CPython raises ProgrammingError with "Binding N is a named parameter" when a query uses named placeholders (, , ) but
the caller passes a sequence instead of a mapping.
RustPython's bind_parameters_sequence() did not check whether each binding slot is a named parameter, so no error was raised.
Fix: call sqlite3_bind_parameter_name() for each slot in bind_parameters_sequence(). If the first byte of the returned name is not '?' (i.e. it is a named placeholder), raise ProgrammingError before attempting to bind.
Assisted-by: GitHub Copilot:claude-sonnet-4-6
Summary
Summary by CodeRabbit
?placeholders.