fix(sqlite): Raise ProgrammingError when operating on a closed cursor #6339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

youknowone merged 1 commit into RustPython:main from ever0de:fix-sqlite3-raise-programming-error-when-closed-cursor

Dec 7, 2025

Contributor

ever0de commented Dec 7, 2025 •

edited by coderabbitai bot

Loading

fixed #6336

Summary by CodeRabbit

Bug Fixes
- Improved SQLite cursor handling and script execution initialization to ensure more reliable state management and proper resource cleanup.

_{✏️ Tip: You can customize this high-level summary in your review settings.}


          fix(sqlite): Raise ProgrammingError when operating on a closed cursor

4b00a9f

Contributor

coderabbitai bot commented Dec 7, 2025 •

edited

Loading

Walkthrough

This change enhances SQLite cursor state management by introducing eager initialization in executescript and refactoring the Cursor::close method to properly enforce cursor closure state. The modifications ensure inner state is initialized before operations and handle statement cleanup consistently.

Changes

Cohort / File(s)	Summary
SQLite Cursor State Management `crates/stdlib/src/sqlite.rs`	Modified `Cursor::executescript` to eagerly initialize inner state via `zelf.clone().inner(vm)?`. Refactored `Cursor::close` to conditionally reset statement when inner is present and consistently mark cursor as closed, removing duplicate guard checking.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Guard handling logic: Verify the guard branch in Cursor::close properly enforces inner state presence and cleanly handles statement reset.
Initialization flow: Confirm eager inner(vm)? call in executescript aligns with the closed cursor ProgrammingError requirement from issue #6336.
State consistency: Ensure inner.closed is reliably set after statement operations and that no code paths bypass closure checks.

Possibly related PRs

Fix sqlite Cursor initialization check #6198: Modifies SQLite Cursor initialization and close semantics for consistent state management.
Raise UnicodeEncodeError for surrogates in sqlite.executescript #6024: Also adds an early pre-check in Cursor::executescript, complementing initialization validation.
Fix sqlite connection reinitialization #6288: Refactors SQLite connection/cursor internal state initialization and guarding for improved closure handling.

Suggested reviewers

youknowone
ShaharNaveh

Poem

🐰 A cursor once closed tried to dance,
But now we enforce closure's trance,
Guard checks are tidy, state stays true,
No more slipping through the rift anew!
✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Linked Issues check	❓ Inconclusive	The PR addresses issue #6336 by implementing guard checks for cursor closure and reworking Cursor::close behavior, but the summary shows only foundational changes without evidence of full implementation across all required methods.	Verify that ProgrammingError is properly raised in all target methods (execute, executemany, executescript, fetchone, fetchall, etc.) and confirm the ClosedCurTests::test_closed passes.

✅ 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 accurately summarizes the main change: raising ProgrammingError when operating on a closed cursor, matching the PR's primary objective.
Out of Scope Changes check	✅ Passed	The changes are narrowly focused on sqlite.rs with modifications to executescript and Cursor::close, directly supporting the stated objective of enforcing closed cursor checks.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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 and usage tips.}

coderabbitai bot reviewed

View reviewed changes

Contributor

coderabbitai bot left a comment

Actionable comments posted: 0

🧹 Nitpick comments (2)

crates/stdlib/src/sqlite.rs (2)
1768-1791: Executescript now correctly respects closed/unninitialized cursor state

Calling inner(vm)? up front makes Cursor::executescript fail with ProgrammingError when the cursor is closed or not initialized, consistent with execute/executemany and fetch methods.

As a small nit, the clone() is not needed here; this is equivalent and a bit cleaner:
// Just validate initialization / closed state
zelf.inner(vm)?;
1831-1848: Closed flag wiring in Cursor::close correctly enforces “closed cursor” errors

This implementation makes close() mark the inner state as closed and reset any active statement, so all later operations that route through inner(vm)? will now raise ProgrammingError as intended. The method stays idempotent and still distinguishes the “Base Cursor.init not called.” case.

If you want to go further (not required for fixing #6336), you could also clear inner.statement after resetting to drop the Statement eagerly and free its underlying SQLite resources sooner:
if let Some(stmt) = &inner.statement {
    stmt.lock().reset();
}
inner.statement = None;
inner.closed = true;
But as-is this change looks correct for the PR’s closed‑cursor behavior goals.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 590da47 and 4b00a9f.

⛔ 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/sqlite.rs (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/stdlib/src/sqlite.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Check the WASM package and demo
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Ensure compilation on various targets
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Check Rust code with clippy

youknowone approved these changes

View reviewed changes

Member

youknowone left a comment

👍

youknowone merged commit a14dd59 into RustPython:main

13 checks passed

coderabbitai bot mentioned this pull request

refactor(sqlite): add check_cursor_valid method for cursor state validation #6342

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet