fix: expand sqlc.slice placeholders at runtime for the sqlite drivers#211
Merged
Conversation
sqlc leaves a /*SLICE:name*/? marker in the SQL and delegates the expansion to the code generator, but the plugin only mapped the parameter type and copied the SQL through verbatim, so every query using sqlc.slice failed with "type 'list' is not supported" once the driver was handed the sequence. Generated functions now rewrite the constant per call, mirroring sqlc's own Go codegen: the placeholder becomes one "?" per element, or NULL for an empty sequence so IN (NULL) matches no rows, and the sequence is star-unpacked into the positional arguments in SQL text order. Overridden and converted slice parameters keep their element-wise conversion. The marker is built from the raw sqlc name, which can differ from the escaped Python parameter name. asyncpg is deliberately untouched: sqlc emits a bare IN ($1) there with no marker to expand, and the idiomatic form on PostgreSQL is ANY($1::type[]), which the plugin already supports. Closes #210
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis change carries ChangesSQLite
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant GeneratedSQLiteFunction
participant SQLiteConnection
Caller->>GeneratedSQLiteFunction: pass slice parameters
GeneratedSQLiteFunction->>GeneratedSQLiteFunction: replace slice marker and flatten arguments
GeneratedSQLiteFunction->>SQLiteConnection: execute expanded SQL
SQLiteConnection-->>GeneratedSQLiteFunction: return query result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The converters PR dropped both the query file and its TAGTYPE override from the dataclass/functions block while the generated module and its runtime tests stayed, so sqlc generate no longer regenerated it and sqlc diff no longer guarded it. Restore the wiring and the override; the regenerated output is byte-identical to the committed module.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/transform/type.go (1)
1-1: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject nullable
sqlc.sliceparams ininternal/transform/type.go.buildPyTypestill carriesIsNullablethrough for slice params, which lets generated SQLite code star-unpack or calllen()onNoneand crash at runtime. ForceIsNullable: falsewhenGetIsSqlcSlice()is true.🤖 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 `@internal/transform/type.go` at line 1, Update buildPyType to override IsNullable as false whenever the parameter’s GetIsSqlcSlice() is true, preventing nullable slice parameters from reaching generated SQLite code. Preserve the existing nullability behavior for non-slice parameters.
🤖 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 `@internal/driver/sqlite_base.go`:
- Around line 267-283: Update writeSliceExpansion to handle nullable slice
parameters before evaluating len(param.expr): either reject nullable slice
params during validation in sliceParams or guard the generated expansion so None
does not reach len(). Preserve normal expansion for non-null slice values and
ensure nullable parameters follow the intended NULL behavior.
---
Outside diff comments:
In `@internal/transform/type.go`:
- Line 1: Update buildPyType to override IsNullable as false whenever the
parameter’s GetIsSqlcSlice() is true, preventing nullable slice parameters from
reaching generated SQLite code. Preserve the existing nullability behavior for
non-slice parameters.
🪄 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.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3f1f710-af38-40da-975c-785f6ad274d6
⛔ Files ignored due to path filters (8)
test/driver_aiosqlite/dataclass/functions/models.pyis excluded by!test/driver_*/*/functions/**test/driver_aiosqlite/dataclass/functions/queries_slice.pyis excluded by!test/driver_*/*/functions/**test/driver_aiosqlite/sqlc-gen-better-python.wasmis excluded by!**/*.wasm,!**/*.wasmtest/driver_asyncpg/sqlc-gen-better-python.wasmis excluded by!**/*.wasm,!**/*.wasmtest/driver_sqlite3/attrs/classes/models.pyis excluded by!test/driver_*/*/classes/**test/driver_sqlite3/dataclass/functions/models.pyis excluded by!test/driver_*/*/functions/**test/driver_sqlite3/dataclass/functions/queries_slice.pyis excluded by!test/driver_*/*/functions/**test/driver_sqlite3/sqlc-gen-better-python.wasmis excluded by!**/*.wasm,!**/*.wasm
📒 Files selected for processing (19)
.changes/unreleased/Fixed-20260721-210000.yamlinternal/driver/common.gointernal/driver/common_test.gointernal/driver/sqlite_base.gointernal/driver/sqlite_test.gointernal/model/types.gointernal/transform/type.gointernal/transform/type_test.gosqlc.yamltest/conftest.pytest/driver_aiosqlite/dataclass/test_aiosqlite_dataclass_functions.pytest/driver_aiosqlite/queries_slice.sqltest/driver_aiosqlite/schema.sqltest/driver_aiosqlite/sqlc.yamltest/driver_asyncpg/sqlc.yamltest/driver_sqlite3/dataclass/test_sqlite3_dataclass_functions.pytest/driver_sqlite3/queries_slice.sqltest/driver_sqlite3/schema.sqltest/driver_sqlite3/sqlc.yaml
A sqlc.slice parameter against a nullable column arrives with notNull false, which would have annotated it Sequence[T] | None and crashed the generated len() call, failing pyright strict on the way. Force such parameters non-nullable in the type layer: the expansion consumes the sequence itself, "no values" is an empty sequence, and IN (NULL) never matches NULL rows anyway, matching the plain slice parameters of sqlc's own Go codegen. Cover it end to end with a nullable note column on the slice fixture table and a query slicing on it, for both sqlite drivers. Drop test_slice in the session cleanup instead of deleting from it, so existing local db files created before the column was added heal themselves.
count(*) always returns a row, so the generated None branch of the two-slice scalar query could never execute and capped patch coverage below 100%. Query the first matching name instead: same two-slice scalar shape, but an empty result is real, and the test asserts both the hit and the miss.
Two review findings on the slice expansion. A parameter named sql was silently overwritten: the expansion assigns the expanded query to a local named sql before any parameter is read, so the query text itself got bound as the parameter value, type-correct and invisible to pyright. Seed "sql" into the parameter dedup map for queries with a slice parameter, exactly like the implicit conn/self argument, so such a parameter becomes sql_2. Reusing one slice name twice in a query failed on every call: sqlc merges same-named sqlc.slice uses into one parameter but keeps a marker per use site, and the count-1 replace left the second marker as a live unbound placeholder. Replace all occurrences when a marker appears more than once and bind one starred copy of the sequence per occurrence, which makes such queries work; sqlc's own Go codegen still generates broken code for them. Covered by a fixture query matching on name or note from the same list, on both sqlite drivers. Also name the remaining sqlite3 slice tests so the delete depends on every reader, mirroring the aiosqlite file, and correct the cleanup comment now that test_slice exists in both driver schemas.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
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 `@internal/driver/common.go`:
- Around line 68-72: Update the argument-flattening logic around
sliceMarkerCount so reused slice arguments are expanded at each SQL marker
occurrence rather than emitted consecutively; preserve the original
placeholder-occurrence order when scalar placeholders appear between slice
markers. Add a regression case covering an interleaved scalar and reused slice,
verifying the flattened arguments align with the expanded SQL placeholders.
🪄 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.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21eb50d1-6e9f-4b81-812e-963695b4b0a4
⛔ Files ignored due to path filters (8)
test/driver_aiosqlite/dataclass/functions/models.pyis excluded by!test/driver_*/*/functions/**test/driver_aiosqlite/dataclass/functions/queries_slice.pyis excluded by!test/driver_*/*/functions/**test/driver_aiosqlite/sqlc-gen-better-python.wasmis excluded by!**/*.wasm,!**/*.wasmtest/driver_asyncpg/sqlc-gen-better-python.wasmis excluded by!**/*.wasm,!**/*.wasmtest/driver_sqlite3/attrs/classes/models.pyis excluded by!test/driver_*/*/classes/**test/driver_sqlite3/dataclass/functions/models.pyis excluded by!test/driver_*/*/functions/**test/driver_sqlite3/dataclass/functions/queries_slice.pyis excluded by!test/driver_*/*/functions/**test/driver_sqlite3/sqlc-gen-better-python.wasmis excluded by!**/*.wasm,!**/*.wasm
📒 Files selected for processing (19)
internal/driver/common.gointernal/driver/common_test.gointernal/driver/sqlite_base.gointernal/driver/sqlite_test.gointernal/transform/queries.gointernal/transform/queries_test.gointernal/transform/type.gointernal/transform/type_test.gosqlc.yamltest/conftest.pytest/driver_aiosqlite/dataclass/test_aiosqlite_dataclass_functions.pytest/driver_aiosqlite/queries_slice.sqltest/driver_aiosqlite/schema.sqltest/driver_aiosqlite/sqlc.yamltest/driver_asyncpg/sqlc.yamltest/driver_sqlite3/dataclass/test_sqlite3_dataclass_functions.pytest/driver_sqlite3/queries_slice.sqltest/driver_sqlite3/schema.sqltest/driver_sqlite3/sqlc.yaml
The consecutive starred copies broke as soon as another placeholder sat between the two use sites of a reused slice: sqlc orders the parameter array by first use, so the plain value was bound into the second IN list, silently returning wrong rows with a matching argument count. Order the flattened arguments by the SQL text instead. A small scanner walks the query for markers and plain placeholders, skipping string literals with doubled-quote escapes, quoted identifiers, and both comment styles, so a question mark inside a literal never counts. Plain expressions fill the "?" slots in parameter order and every marker occurrence gets its slice's starred copy; when the SQL cannot account for exactly the given arguments, emission falls back to the previous consecutive form. Covered by a fixture query with a plain parameter between the reuse sites, on both sqlite drivers.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
The placeholder expansion ships with this branch, so the feature support page now explains how sqlc.slice works on the sqlite drivers, that it cannot be used with prepared statements, and that PostgreSQL uses ANY instead.
Contributor
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #210: queries using
sqlc.slicefailed at runtime on the SQLite drivers because the/*SLICE:name*/?placeholder sqlc leaves in the SQL was never expanded.sqlc's contract puts that expansion on the code generator ("the
/*SLICE:ages*/placeholder is dynamically replaced on a per-query basis"), and its own Go codegen does it viastrings.Replacein the generated code. The plugin only mapped the parameter tocollections.abc.Sequence[...]and copied the SQL through verbatim, so binding the sequence raisedsqlite3.ProgrammingError: type 'list' is not supported.What generated code looks like now
?per element; an empty sequence becomesNULL, soIN (NULL)matches no rows instead of raising (same semantics as sqlc's Go codegen).(name, *ids, note)).*[conv(v) for v in ids]), and bundledquery_parameter_limitparams work (*params.ids).sqlc.slice('for')-> parameterfor_, marker/*SLICE:for*/?).Why asyncpg is untouched
sqlc emits a bare
IN ($1)on PostgreSQL - no marker, nothing to expand - and that SQL is broken at the sqlc level (verified against live postgres:DataError: invalid input for query argument $1). The macro exists "for drivers that do not support passing slices to the IN operator"; on PostgreSQL the idiomatic form is= ANY($1::type[]), which the plugin fully supports, converters included.Implementation
model.PyType.SqlcSliceNamecarries the raw slice name;transformpopulates it in both the override and normal branches.driver:sliceParamscollects slice parameters (plain and bundled),expandParamsFlattenSlicesstar-unpacks them, and the sqlite driver emits the per-call.replace(...)chain. For:manythe expansion sits after the decode hook so the output stays aruff formatno-op.Validation
:one,:exec,:execrows- all previously raising, all passing now.queries_slice.sqlfixture module for both SQLite drivers with runtime pytest coverage of every generated function and bothQueryResultsconsumption forms.make pipelinesclean, full test suite green (shuffled).noxgreen (1453 pytest tests), pyright strict + ruff on the regenerated fixtures, and all three*_checksessions confirm the committed fixtures are in sync.Summary by CodeRabbit
sqlc.sliceon SQLite so slice placeholders expand at runtime into the right SQL placeholder order.NULL, while overrides/conversions remain element-wise.= ANY(...::type[])syntax.sqlc.slicemacro documentation with SQLite runtime expansion details and corrected guidance.