fix(reader_base): bind TextQuery args correctly instead of passing dal.QueryArg itself - #177
Merged
Conversation
…l.QueryArg itself
dal.NewTextQuery accepts dal.QueryArg{Name, Value} values, but
getReaderBase's TextQuery branch passed each arg struct straight
through as a[i] = arg — database/sql has no idea how to convert a
dal.QueryArg into a bind value, so any TextQuery with a non-empty
Args() failed outright ("sql: converting argument $1 type: unsupported
type dal.QueryArg, a struct"). Named @param/:param/$param placeholders
in native SQL could never bind at all; pkg/secureread.RunNativeSQL
(datatug-cli) currently works around this by binding placeholders
itself before ever reaching this adapter.
Fix: a named arg (Name != "") becomes a sql.NamedArg via sql.Named, so
an @name-style placeholder binds by name; a positional arg (Name == "")
passes its Value straight through for ordinary ?/$N placeholders.
Reproduced first with three new tests against a real in-memory SQLite
database (modernc.org/sqlite, not a mock) — positional-only,
named-only, and mixed — confirmed all three fail with the exact error
above against the unfixed code, then pass after the one-line fix.
Checked whether the pushed layered-acl-query branch already touched
this: it only changes reader_base.go's dal.StructuredQuery branch
(dialect-aware compilation), not the dal.TextQuery arg-binding branch
this fixes — no overlap, nothing duplicated.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dd3aoE41JSShyUuUmW8sSi
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
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
dal.NewTextQueryacceptsdal.QueryArg{Name, Value}values, butgetReaderBase'sdal.TextQuerybranch passed each arg struct straight through asa[i] = arg—database/sqlhas no idea how to convert adal.QueryArginto a bind value, so anyTextQuerywith a non-emptyArgs()failed outright:sql: converting argument $1 type: unsupported type dal.QueryArg, a struct. Named@param/:param/$paramplaceholders in native SQL could never bind at all. Found viadatatug-cliPR #206's body (item 2) — itspkg/secureread.RunNativeSQLcurrently works around this by binding placeholders itself before ever reaching this adapter.Name != "") becomes asql.NamedArgviasql.Named, so an@name-style placeholder binds by name; a positional arg (Name == "") passes itsValuestraight through for ordinary?/$Nplaceholders.Checked for duplicate work first
Per instructions, checked whether the pushed
layered-acl-querybranch (codex layered-ACL work) already fixes this before doing anything:It only changes
reader_base.go'sdal.StructuredQuerybranch (adds dialect-awarecompileStructuredSQLfor SQLite), leaving thedal.TextQuerybranch — the one this bug lives in — completely untouched. No overlap, nothing duplicated; proceeded with the fix.Reproduction
Three new tests, run against a real in-memory SQLite database (
modernc.org/sqlite, not a mock, matching this repo's ownopenTestSQLiteDBhelper) — positional-only, named-only, and mixed positional+named in the same query:TestGetReaderBase_TextQuery_PositionalArgsTestGetReaderBase_TextQuery_NamedArgsTestGetReaderBase_TextQuery_MixedArgsConfirmed all three fail with the exact error above against the unfixed code (
go test -run TestGetReaderBase_TextQuery -vbefore the fix), then pass cleanly after the one-line fix.Validation
wb run -- go build ./...— clean.wb run -- go vet ./...— clean.wb run -- go test ./...— clean, full suite green, including the three new tests.wb run -- go test ./... -run TestConformance -v— the shareddalgotest.RunConformancesuite (17 subtests) stays fully green.gofmt -l .— clean, no output.go mod tidy -diffreports a large, pre-existinggo.sumcleanup (stale versions ofdal-go/dalgo,dal-go/record,modernc.org/sqlite, etc. left over from past bumps) — unrelated to this fix and out of scope for a narrow bug fix, not touched here.Do not merge — landing is owned by a separate session. (Note: dalgo repos auto-tag on
main— no hand-tagging done or needed here.)🤖 Generated with Claude Code
https://claude.ai/code/session_01Dd3aoE41JSShyUuUmW8sSi