docs: say what the raw-SQL rule means instead of forbidding what the code does - #109
Merged
Conversation
…table
The raw-SQL rule is about to name schema introspection as exempt, and that is
the one exemption worth checking rather than trusting: GORM does ship
`Migrator().HasTable`, `HasColumn` and `ColumnTypes`, so "the migrator cannot do
this" needs evidence, not an assertion.
Three tests run all three calls against a real FTS5 table:
HasTable answers correctly, both present and absent
ColumnTypes fails with "invalid DDL" — it parses the stored CREATE
statement, and CREATE VIRTUAL TABLE is not a shape it parses
HasColumn matches patterns against that DDL text rather than asking the
schema, so it reports false for `content` on a table that has
a `content` column
The HasColumn case is checked both ways: PRAGMA table_info sees the column in
the same test, so the fixture cannot be what is wrong.
HasTable is the one call that works, and sqliteTableExists still keeps its raw
statement — HasTable returns a bool with no error while every caller propagates
one, and swallowing it would read a transient failure as "the table is absent"
in the legacy-upgrade path. The test says so where a reader will find it.
If a GORM upgrade makes one of these fail, that is the test working: the
exemption gets reconsidered instead of the test relaxed.
Refs #108
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…code does
CLAUDE.md, AGENTS.md and guide/development.md all said "use GORM queries only
(no raw SQL)" with no exception written down anywhere, while
internal/adapters/outbound/searchsql holds 25 raw statements in non-test code.
As written, the rule declared that package a wholesale violation and gave a
reader no way to tell a deliberate raw statement from one nobody got round to.
The rule now states the criterion — GORM's model layer wherever it has a form
for the statement — and names the five categories where it has none:
full-text operators and index maintenance (FTS5 MATCH and its rank column,
to_tsvector / to_tsquery / ts_rank / @@)
DDL on the FTS5 virtual tables, which AutoMigrate does not model
writes into those tables, which have no GORM model at all
schema introspection the migrator cannot do (PRAGMA table_info,
sqlite_master, information_schema, pg_indexes, pg_trigger)
connection pragmas
Exempt statements are confined to internal/adapters/outbound/searchsql and
internal/db; a raw statement anywhere else is a review stop. Inside an exempt
statement two constraints hold regardless: identifiers come from package
constants, never caller input, and every value is a bound parameter. No
exemption makes concatenating a value into the string correct.
Nothing is converted, and the reason is recorded rather than claimed. The
introspection exemption is the one that looks convertible, since GORM ships
HasTable, HasColumn and ColumnTypes — searchsql/migrator_limits_test.go runs all
three against a real FTS5 table and shows ColumnTypes failing with "invalid DDL"
and HasColumn reporting false for a column that exists. HasTable does work, but
it returns no error where every caller of sqliteTableExists propagates one.
guide/development.md carries the full section; the two rule files carry the
compressed form and point at it. guide/ko/development.md mirrors it.
Closes #108
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A reader who opens sqlite.go finds raw statements and no local sign that they are sanctioned. The package comment now names the four categories they fall into, points at guide/development.md §Raw SQL for the rule and at migrator_limits_test.go for the introspection evidence, and says plainly that anything GORM's model layer can express belongs there instead — here as much as anywhere else. Refs #108 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Closes #108. Picks option 1 — write the exception down — and records why nothing is converted.
The decision
The rule now states a criterion instead of a prohibition: use GORM's model layer wherever it has a form for the statement, and raw SQL only where it has none. Five categories have none:
MATCHand itsrankcolumn;to_tsvector/to_tsquery/ts_rank/@@.CREATE VIRTUAL TABLE … USING fts5, and theDROP/ALTER TABLE … RENAME TOpairs the legacy upgrade needs.AutoMigratedoes not model a virtual table.Table("search_fts")would name the table in a string either way, and the bulk insert is one statement on purpose.PRAGMA table_info,sqlite_master,information_schema.columns,pg_indexes,pg_trigger.PRAGMA journal_mode,PRAGMA busy_timeout.Confined to
internal/adapters/outbound/searchsqlandinternal/db; a raw statement anywhere else is a review stop. Inside an exempt statement, identifiers come from package constants and every value is a bound parameter — no exemption makes concatenating a value into the string correct.Why nothing is converted
The issue's option 1 ends with "then convert the statements that fall outside the exemption". After checking each one, there are none — and the category that looks most convertible is the one I checked hardest, because GORM does ship
Migrator().HasTable,HasColumnandColumnTypes.searchsql/migrator_limits_test.goruns all three against a real FTS5 table:HasTableis the one call that works, andsqliteTableExistsstill keeps its raw statement:HasTablereturns a bool with no error, while all four callers propagate one. Swallowing it would read a transient failure as "the table is absent" — in the legacy-upgrade path that is the difference between stopping and rebuilding.The FTS5 writes are the other case that looks convertible.
db.Table(sqliteFTSTable).Where("namespace = ?", ns).Delete(nil)names the table in a string and keeps the raw predicate, so it trades one string for two and changes no safety property. The bulk insert is a single hand-built statement so a rebuild does not pay a round trip per row.If a GORM upgrade makes one of those tests fail, that is the test working: the exemption gets reconsidered, not the test relaxed. The doc and the test both say so.
What changed
CLAUDE.md,AGENTS.md— the compressed rule, pointing at the guideguide/development.md— new### Raw SQLsection with the categories, the two in-statement constraints, and the migrator evidenceguide/ko/development.md— the same section in Koreaninternal/adapters/outbound/searchsql/backend.go— package comment naming the categories and pointing at bothinternal/adapters/outbound/searchsql/migrator_limits_test.go— the evidence, three testsThree commits: the evidence test, then the rule, then the in-code pointer.
Verification
CGO_ENABLED=1 go test -tags "fts5" ./... -count=1— all packages passgo vet -tags fts5 ./internal/adapters/outbound/searchsql/— cleangofmt -l internal/— only the two pre-existing parse fixtures undertestdata/binding_gap/go/🤖 Generated with Claude Code