Skip to content

sqlite3: allow Row construction with cursor having no description - #8364

Merged
youknowone merged 1 commit into
RustPython:mainfrom
ever0de:fix/sqlite-row-no-description
Jul 25, 2026
Merged

youknowone merged 1 commit into
RustPython:mainfrom
ever0de:fix/sqlite-row-no-description

Conversation

@ever0de

@ever0de ever0de commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

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

Summary

Summary by CodeRabbit

  • Bug Fixes
    • Improved error messages when accessing a missing row key by including the requested key.
    • Rows can now be created without cursor metadata; their description defaults to empty.

Copilot AI review requested due to automatic review settings July 25, 2026 09:12
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6beb434c-2af3-455b-9afc-80bf431912c5

📥 Commits

Reviewing files that changed from the base of the PR and between 979edcb and 5c10d9d.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_sqlite3/test_dbapi.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/stdlib/src/_sqlite3.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/stdlib/src/_sqlite3.rs

📝 Walkthrough

Walkthrough

Updates SQLite Row behavior for missing string-key lookups and cursors without descriptions.

Changes

SQLite Row behavior

Layer / File(s) Summary
Update Row lookup and construction behavior
crates/stdlib/src/_sqlite3.rs
Missing string keys now appear in IndexError messages, and missing cursor descriptions default to an empty tuple.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the main change: allowing sqlite3.Row construction when the cursor description is missing.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 == None as an empty description tuple during Row construction (instead of raising ValueError).
  • Include the missing string key in the raised IndexError message for Row.__getitem__.
  • Enable the upstream test by removing the RustPython-specific @unittest.expectedFailure marker.

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.

Copilot AI review requested due to automatic review settings July 25, 2026 09:18
@ever0de
ever0de force-pushed the fix/sqlite-row-no-description branch from b50b901 to 979edcb Compare July 25, 2026 09:18
@github-actions

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/sqlite3
[x] test: cpython/Lib/test/test_sqlite3 (TODO: 79)

dependencies:

  • sqlite3

dependent tests: (2 tests)

  • sqlite3: test_dbm_sqlite3 test_sqlite3

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@ever0de
ever0de marked this pull request as ready for review July 25, 2026 09:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
crates/stdlib/src/_sqlite3.rs (1)

2253-2275: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add regression coverage for both compatibility changes.

Test that missing string keys include the requested key, and that constructing a Row with description is None succeeds with row.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

📥 Commits

Reviewing files that changed from the base of the PR and between 28454cc and 979edcb.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_sqlite3/test_dbapi.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/stdlib/src/_sqlite3.rs

Comment thread crates/stdlib/src/_sqlite3.rs Outdated
}
}
Err(vm.new_index_error("No item with that key"))
Err(vm.new_index_error(format!("No item with key '{}'", name.expect_str())))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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 -n

Repository: 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}")
PY

Repository: 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 4

Repository: 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
Copilot AI review requested due to automatic review settings July 25, 2026 09:32
@ever0de
ever0de force-pushed the fix/sqlite-row-no-description branch from 979edcb to 5c10d9d Compare July 25, 2026 09:32

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

}
}
Err(vm.new_index_error("No item with that key"))
Err(vm.new_index_error(format!("No item with key '{}'", name.to_string_lossy())))

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@youknowone
youknowone merged commit d797f99 into RustPython:main Jul 25, 2026
27 checks passed
youknowone pushed a commit that referenced this pull request Sep 16, 2026
)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants