FakePool SQL string-matching: detect column-value binding errors

[henry0816191]

Summary:

tests/conftest.py

• _FakeCursor now receives a FakePool reference and logs every execute() call as (sql, params) in FakePool.call_log
• _FakeConn forwards the pool to _FakeCursor
• Added calls_matching(sql_fragment) and assert_param_at(sql_fragment, position, expected) helpers on FakePool

tests/test_storage.py

• Added TestFakePoolParamCapture with 8 tests covering:
  • user_watchlist INSERT param order (positions 0–2)
  • paper_cache key binding
  • discovered_urls url and last_modified binding
  • Regression test proving swapped params would fail via pytest.raises(AssertionError)

Test results

All 61 tests in tests/test_storage.py pass (53 existing + 8 new). No existing tests were modified. No production code changes.

Related Issue

• Close FakePool SQL string-matching: detect column-value binding errors #56

Summary by CodeRabbit

• Tests
  • Enhanced test infrastructure to capture and record SQL execute calls and their bound parameters.
  • Added helper assertions to query recorded calls and validate parameter values and positions.
  • New test coverage verifying parameter ordering and values for key write operations, including negative tests for out-of-range indexes and incorrect parameter values.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0c09eeaf-1667-4b28-8fd5-c847d4486b20

📥 Commits

Reviewing files that changed from the base of the PR and between ce1eafe and b2dd90b.

📒 Files selected for processing (1)

• tests/conftest.py

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/conftest.py

---

📝 Walkthrough

Walkthrough

This PR extends test DB stubs to record executed SQL and bound parameters to a shared FakePool.call_log, exposes calls_matching() and assert_param_at() helpers, and adds tests that assert positional parameter bindings for key storage writes.

Changes

SQL Parameter Capture and Binding Validation

Layer / File(s)	Summary
FakePool SQL parameter capture infrastructure tests/conftest.py	_FakeCursor accepts an optional pool reference and appends each (sql, params) invocation to pool.call_log on execute(). _FakeConn wires the pool through during cursor construction. FakePool gains call_log storage and helper methods: calls_matching() filters recorded calls by normalized SQL fragments, and assert_param_at() asserts the parameter value at a given position in the last matching call.
Parameter binding regression tests tests/test_storage.py	TestFakePoolParamCapture validates that storage writes to user_watchlist, paper_cache, and discovered_urls bind parameters in the correct positions. Tests use fake_pool.calls_matching() and fake_pool.assert_param_at() to verify SQL and parameter ordering. Includes negative tests for out-of-range and incorrect expected values.

sequenceDiagram
  participant Test as TestFakePoolParamCapture
  participant Cursor as _FakeCursor
  participant Pool as FakePool
  Test->>Cursor: execute(sql, params)
  Cursor->>Pool: append (sql, params) to call_log
  Test->>Pool: calls_matching(sql_fragment)
  Pool-->>Test: return matching (sql, params)
  Test->>Pool: assert_param_at(fragment, position, expected)
  Pool-->>Test: assertion result

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nibble logs in moonlit code,

SQL and params in tidy mode,
No swap or slip can hide from sight,
Tests now catch the binding right. 🥕✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description includes sections on code changes, test results, and a related issue reference. However, it is missing the 'Test plan' section with checkboxes from the required template.	Add the 'Test plan' section with checkboxes (./run check, pre-commit run) to match the repository template.
Docstring Coverage	⚠️ Warning	Docstring coverage is 17.65% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: extending FakePool to capture and assert SQL parameter bindings for detecting column-value binding errors.
Linked Issues check	✅ Passed	The PR fully addresses issue #56's acceptance criteria: FakePool captures bound parameters, provides assertion helpers like assert_param_at, includes regression tests proving the detection works, and all existing tests pass.
Out of Scope Changes check	✅ Passed	All changes are strictly test infrastructure improvements (conftest.py and test_storage.py). No production code was modified, and all changes directly support the PR's stated objective of detecting column-value binding errors.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/fakepool-param-assertions

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[henry0816191]

@coderabbitai full review again

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the PR diff
gh pr view 59 --repo cppalliance/paperscout --json title,body,files,state,headRefName

Length of output: 1772

---

🏁 Script executed:

#!/bin/bash
# Read the changed files
cat tests/conftest.py
echo "=== END OF conftest.py ==="
cat tests/test_storage.py
echo "=== END OF test_storage.py ==="

Length of output: 29873

---

---

@henry0816191 Here is the full review of PR #59.

---

Overview

The implementation is clean and purposeful. All acceptance criteria from issue #56 are met:

• ✅ FakePool captures bound parameter values alongside SQL text
• ✅ Assertion helpers verify parameter ordering
• ✅ Regression test confirms transposed params are caught
• ✅ Backward compatibility — all 53 existing tests unmodified and passing

---

tests/conftest.py

Issue 1 – assert_param_at raises IndexError instead of AssertionError on out-of-bounds position

When position >= len(params), Python raises IndexError at params[position], not AssertionError. pytest will still mark the test as failed, but the error message will say tuple index out of range rather than something meaningful. An explicit guard makes diagnostics much clearer:

def assert_param_at(self, sql_fragment: str, position: int, expected) -> None:
    """Assert that the last call matching sql_fragment has params[position] == expected."""
    matches = self.calls_matching(sql_fragment)
    assert matches, f"No SQL call matching {sql_fragment!r} in call_log"
    _, params = matches[-1]
    assert position < len(params), (
        f"params has only {len(params)} element(s); position {position} is out of range"
    )
    assert params[position] == expected, (
        f"params[{position}] expected {expected!r}, got {params[position]!r}"
    )

Nitpick – call_log type annotation vs. runtime type

call_log is annotated as list[tuple[str, tuple]], but execute(self, sql: str, params=()) accepts any sequence. If callers ever pass a list as params, the stored value won't match the annotation. Widening the annotation avoids a potential mypy/pyright complaint later:

call_log: list[tuple[str, tuple | list]] = []

Or, more idiomatically:

from collections.abc import Sequence
call_log: list[tuple[str, Sequence]] = []

---

tests/test_storage.py — TestFakePoolParamCapture

The eight tests are well-structured and focused. A few observations:

Suggestion – test_call_log_records_sql_and_params could be more specific

The test currently uses a bare truthiness check:

assert fake_pool.calls_matching("INSERT INTO user_watchlist")

It would be more meaningful to also assert that at least one entry has non-empty params, since the whole point is that params are captured:

calls = fake_pool.calls_matching("INSERT INTO user_watchlist")
assert calls
assert calls[-1][1]  # params tuple is non-empty

Minor – test_regression_swapped_user_id_and_entry_would_fail

The test is correct and clearly documents intent. The comment could make it even more explicit that this is testing the test infrastructure itself, not production code, to avoid confusion for future contributors:

def test_regression_swapped_user_id_and_entry_would_fail(self, fake_pool):
    """
    Prove assert_param_at catches binding transposition.
    This test exercises the FakePool test helper itself, not production code.
    """

---

Summary

Severity	File	Finding
Medium	conftest.py	assert_param_at raises IndexError (not AssertionError) for out-of-bounds position
Nitpick	conftest.py	call_log annotation (tuple) doesn't cover list params
Suggestion	test_storage.py	test_call_log_records_sql_and_params could assert params are non-empty
Suggestion	test_storage.py	Regression test docstring could clarify it tests the helper, not production code

The assert_param_at bounds check is the only item worth addressing before merge. Everything else is polish.

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@tests/conftest.py`:
- Around line 208-210: The current assertion in the helper that checks parameter
index uses "assert position < len(params)" which still permits negative indices;
update the check to enforce a strict positional index by changing it to assert 0
<= position < len(params) and adjust the assertion message to reflect the new
precondition (e.g., mention valid range or that position must be non-negative
and less than len(params)) so functions like the one validating parameter bounds
reject negative positions instead of treating them as Python negative indices.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3f28b15e-b3a4-418e-951e-4965041922da

📥 Commits

Reviewing files that changed from the base of the PR and between d75977f and ce1eafe.

📒 Files selected for processing (2)

• tests/conftest.py
• tests/test_storage.py

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/test_storage.py