sqlite3: allow Row construction with cursor having no description - #8364
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates SQLite ChangesSQLite Row behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR aligns RustPython’s _sqlite3 Row behavior with CPython by allowing sqlite3.Row(cursor, data) construction when cursor.description is None, and by improving the IndexError message for missing string keys.
Changes:
- Treat
Cursor.description == Noneas an empty description tuple duringRowconstruction (instead of raisingValueError). - Include the missing string key in the raised
IndexErrormessage forRow.__getitem__. - Enable the upstream test by removing the RustPython-specific
@unittest.expectedFailuremarker.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
Lib/test/test_sqlite3/test_dbapi.py |
Unmarks the Row “no description” test as expected-failure now that behavior matches CPython. |
crates/stdlib/src/_sqlite3.rs |
Updates Row constructor and string-key lookup error handling to match CPython expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b50b901 to
979edcb
Compare
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/sqlite3 dependencies:
dependent tests: (2 tests)
Legend:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/stdlib/src/_sqlite3.rs (1)
2253-2275: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both compatibility changes.
Test that missing string keys include the requested key, and that constructing a
Rowwithdescription is Nonesucceeds withrow.keys() == []. Include a surrogate-containing key once the error formatting is fixed.🤖 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 2253 - 2275, Add regression tests covering the `Row` key lookup and construction behavior: verify missing string keys include the requested key in the raised error, including a surrogate-containing key after formatting support is fixed, and verify constructing `Row` when `description` is `None` succeeds with `row.keys()` returning an empty list.
🤖 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.
Inline comments:
In `@crates/stdlib/src/_sqlite3.rs`:
- Line 2253: Update the missing-key error construction in the relevant
dictionary lookup path to replace name.expect_str() with the surrogate-safe
name.to_string_lossy() conversion, preserving the existing IndexError message
and behavior.
---
Nitpick comments:
In `@crates/stdlib/src/_sqlite3.rs`:
- Around line 2253-2275: Add regression tests covering the `Row` key lookup and
construction behavior: verify missing string keys include the requested key in
the raised error, including a surrogate-containing key after formatting support
is fixed, and verify constructing `Row` when `description` is `None` succeeds
with `row.keys()` returning an empty list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6715ed7d-d636-40c8-a7a7-454211b22c67
⛔ 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
| } | ||
| } | ||
| Err(vm.new_index_error("No item with that key")) | ||
| Err(vm.new_index_error(format!("No item with key '{}'", name.expect_str()))) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline crates/stdlib/src/_sqlite3.rs --match sqlite3 --view expanded || true
echo "== relevant sqlite3 lines =="
sed -n '2220,2290p' crates/stdlib/src/_sqlite3.rs | cat -n
echo "== PyStr expect_str usages and to_str definitions =="
rg -n "expect_str|fn to_str|struct PyStr" crates/vm/src/builtins crates/vm/src crates/stdlib/src -S
echo "== str implementation slice =="
sed -n '1,220p' crates/vm/src/builtins/str.rs | cat -nRepository: RustPython/RustPython
Length of output: 15559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== PyStr to_str / expect_str implementations =="
sed -n '500,560p' crates/vm/src/builtins/str.rs | cat -n
echo "== Cursor Inner description initialization/usages around expected area =="
sed -n '1800,2220p' crates/stdlib/src/_sqlite3.rs | cat -n
echo "== Rust code static check for Row::subscript error path panic possibility =="
python3 - <<'PY'
from pathlib import Path
p = Path("crates/stdlib/src/_sqlite3.rs")
text = p.read_text()
start = text.index("impl Row {")
end = text.index("impl Constructor for Row", start)
body = text[start:end]
print("subscript contains two name.expect_str() in comparison:", "name.expect_str().chars().flat_map" in body)
print("subscript contains name.expect_str() in error message:", "No item with key" in body and "name.expect_str()" in body)
# Extract matching closure/error text as data only without executing repository code.
lines = body.splitlines()
for i, line in enumerate(lines, 1):
if "subscript" in line or "No item with key" in line:
print(f"{i}: {line}")
PYRepository: RustPython/RustPython
Length of output: 20249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Row subscript exact implementation for context =="
python3 - <<'PY'
from pathlib import Path
import re, itertools
text = Path("crates/stdlib/src/_sqlite3.rs").read_text()
start = text.index("impl Row {", text.index("struct CursorInner"))
end = text.index("impl Constructor for Row", start)
body = text[start:end].splitlines()
for i, line in enumerate(body, 1):
if line.strip().startswith("fn subscript") or line.strip().startswith("Ok(Self::new_uninitialized"):
for j in range(i-1 if i>1 else 0, min(i+65, len(body))):
print(f"{j+1}: {body[j]}")
print("---")
PY
echo "== describe row_factory construction behavior for Row subscript reachability =="
rg -n "row_factory|new_row|Row::|fetchone|fetchmany|fetchall" crates/stdlib/src/_sqlite3.rs -C 4Repository: RustPython/RustPython
Length of output: 1771
Avoid expect_str() in this error path.
PyStr::expect_str() panics when the key contains surrogates, so a missing dictionary key can surface as a panic instead of IndexError. Use a surrogate-safe conversion for the error key, such as name.to_string_lossy().
🤖 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` at line 2253, Update the missing-key error
construction in the relevant dictionary lookup path to replace name.expect_str()
with the surrogate-safe name.to_string_lossy() conversion, preserving the
existing IndexError message and behavior.
Row(cursor, data) raised ValueError when cursor.description was None. CPython allows this case and returns an empty key list. - Use empty tuple when description is None instead of raising - Include the key name in the IndexError when a string key is not found Assisted-by: GitHub Copilot:claude-sonnet-4-6
979edcb to
5c10d9d
Compare
| } | ||
| } | ||
| Err(vm.new_index_error("No item with that key")) | ||
| Err(vm.new_index_error(format!("No item with key '{}'", name.to_string_lossy()))) |
) Row(cursor, data) raised ValueError when cursor.description was None. CPython allows this case and returns an empty key list. - Use empty tuple when description is None instead of raising - Include the key name in the IndexError when a string key is not found Assisted-by: GitHub Copilot:claude-sonnet-4-6
Row(cursor, data) raised ValueError when cursor.description was None. CPython allows this case and returns an empty key list.
Assisted-by: GitHub Copilot:claude-sonnet-4-6
Summary
Summary by CodeRabbit