From 1fba545759bad1ef4e2c89f322d9c840a06cc593 Mon Sep 17 00:00:00 2001 From: tae2089 Date: Tue, 11 Aug 2026 16:23:26 +0900 Subject: [PATCH 1/3] test(searchsql): record what GORM's migrator does to an FTS5 virtual table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../searchsql/migrator_limits_test.go | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 internal/adapters/outbound/searchsql/migrator_limits_test.go diff --git a/internal/adapters/outbound/searchsql/migrator_limits_test.go b/internal/adapters/outbound/searchsql/migrator_limits_test.go new file mode 100644 index 0000000..ca3758b --- /dev/null +++ b/internal/adapters/outbound/searchsql/migrator_limits_test.go @@ -0,0 +1,100 @@ +//go:build fts5 + +package searchsql + +import ( + "testing" + + "gorm.io/driver/sqlite" + "gorm.io/gorm" + "gorm.io/gorm/logger" +) + +// This file is the evidence behind one line of guide/development.md §Raw SQL: +// that the schema introspection in this package cannot go through GORM's +// migrator. GORM does ship HasTable, HasColumn and ColumnTypes, so the exemption +// would be a bare assertion without a test that runs all three against a real +// FTS5 virtual table and records what they do. +// +// If a GORM upgrade makes one of these fail, the right response is to +// reconsider the exemption for that call, not to relax the test. + +// newVirtualTableProbeDB opens an in-memory database holding the FTS5 table this +// package creates in production, plus a second one shaped like the pre-namespace +// legacy table. +func newVirtualTableProbeDB(t *testing.T) *gorm.DB { + t.Helper() + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{Logger: logger.Discard}) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + if err := createSQLiteFTSTable(db, sqliteFTSTable, true); err != nil { + t.Fatalf("create %s: %v", sqliteFTSTable, err) + } + if err := db.Exec( + "CREATE VIRTUAL TABLE legacy_probe USING fts5(node_id UNINDEXED, content, language)", + ).Error; err != nil { + t.Fatalf("create legacy_probe: %v", err) + } + return db +} + +// TestMigratorHasTableSeesFTS5VirtualTables records that table presence is the +// one introspection GORM answers correctly here — and why sqliteTableExists +// keeps its raw statement anyway: HasTable returns a bool with no error, while +// every caller of sqliteTableExists propagates one. Swallowing that error would +// read a transient failure as "the table is absent", which in the legacy-upgrade +// path is the difference between stopping and rebuilding. +func TestMigratorHasTableSeesFTS5VirtualTables(t *testing.T) { + db := newVirtualTableProbeDB(t) + + if !db.Migrator().HasTable(sqliteFTSTable) { + t.Errorf("HasTable(%q) = false, want true: the virtual table was just created", sqliteFTSTable) + } + if db.Migrator().HasTable("no_such_table") { + t.Error("HasTable(\"no_such_table\") = true, want false") + } + + exists, err := sqliteTableExists(db, sqliteFTSTable) + if err != nil || !exists { + t.Errorf("sqliteTableExists(%q) = (%v, %v), want (true, nil)", sqliteFTSTable, exists, err) + } +} + +// TestMigratorColumnTypesRejectsFTS5VirtualTables records that ColumnTypes +// cannot describe a virtual table at all: it parses the stored DDL, and +// CREATE VIRTUAL TABLE is not a shape it can parse. +func TestMigratorColumnTypesRejectsFTS5VirtualTables(t *testing.T) { + db := newVirtualTableProbeDB(t) + + types, err := db.Migrator().ColumnTypes(sqliteFTSTable) + if err == nil { + t.Fatalf("ColumnTypes(%q) succeeded with %d columns, want an error", sqliteFTSTable, len(types)) + } + if got := err.Error(); got != "invalid DDL" { + t.Errorf("ColumnTypes(%q) error = %q, want %q", sqliteFTSTable, got, "invalid DDL") + } +} + +// TestMigratorHasColumnMisreadsFTS5VirtualTables is the reason PRAGMA table_info +// stays. HasColumn matches patterns against the stored DDL text rather than +// asking the schema, so whether it finds a column depends on how that column +// happens to be spelled in the CREATE statement. `content` is a real column on +// legacy_probe and HasColumn says it is not there. +func TestMigratorHasColumnMisreadsFTS5VirtualTables(t *testing.T) { + db := newVirtualTableProbeDB(t) + + present, err := sqliteColumnExists(db, "legacy_probe", "content") + if err != nil { + t.Fatalf("sqliteColumnExists: %v", err) + } + if !present { + t.Fatal("PRAGMA table_info does not see legacy_probe.content; the fixture is wrong, not GORM") + } + + if db.Migrator().HasColumn("legacy_probe", "content") { + t.Error("HasColumn now agrees with the schema on this table. GORM may have gained real " + + "virtual-table introspection: reconsider the exemption in guide/development.md §Raw SQL " + + "rather than relaxing this test") + } +} From b2ffac44fc65c8254a59a03d13ffdd09377022dc Mon Sep 17 00:00:00 2001 From: tae2089 Date: Tue, 11 Aug 2026 16:23:40 +0900 Subject: [PATCH 2/3] docs: say what the raw-SQL rule means instead of forbidding what the code does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- AGENTS.md | 7 +++++- CLAUDE.md | 2 +- guide/development.md | 47 ++++++++++++++++++++++++++++++++++++++++- guide/ko/development.md | 43 ++++++++++++++++++++++++++++++++++++- 4 files changed, 95 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f0dbd8c..a86f1e9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -74,7 +74,12 @@ See the `guide/` directory for detailed documentation: - TDD: Red -> Green -> Refactor - Tidy First: separate structural changes from behavioral changes -- Use GORM queries only (no raw SQL) +- Use GORM's model layer for queries. Raw SQL is allowed only where GORM has no + form for the statement — full-text operators, FTS5 virtual-table DDL and + writes, schema introspection its migrator cannot do, connection pragmas — and + only inside `internal/adapters/outbound/searchsql` and `internal/db`. + Identifiers come from package constants, values are always bound parameters. + Details and the evidence for each exemption: `guide/development.md` §Raw SQL - Tests: `CGO_ENABLED=1 go test -tags "fts5" ./... -count=1` - Integration test: `./scripts/integration-test.sh` (full Gitea + PostgreSQL + ccg Docker pipeline) diff --git a/CLAUDE.md b/CLAUDE.md index e0dadd4..f7c1baf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,7 +57,7 @@ Graceful shutdown: SIGINT/SIGTERM 시 진행 중인 clone/build에 context cance - TDD: Red → Green → Refactor - Tidy First: 구조적 변경과 행위 변경 분리 -- GORM 쿼리만 사용 (raw SQL 금지) +- 쿼리는 GORM 모델 계층으로 작성한다. Raw SQL은 GORM에 대응 형태가 없는 경우에만 허용한다 — 전문 검색 연산자, FTS5 가상 테이블 DDL·쓰기, migrator가 못 하는 스키마 introspection, 연결 pragma — 그리고 `internal/adapters/outbound/searchsql`와 `internal/db` 안에서만이다. 식별자는 패키지 상수에서 오고, 값은 항상 bound parameter다 (상세와 각 예외의 근거: `guide/development.md` §Raw SQL) - 코드 정렬: 종류별 그룹화가 아니라 "타입 + 그 타입의 생성자·메소드"를 붙여 두는 응집 관례를 따른다 (상세: `guide/development.md` §Declaration order) - 테스트: `CGO_ENABLED=1 go test -tags "fts5" ./... -count=1` - Integration test: `./scripts/integration-test.sh` (Gitea + PostgreSQL + ccg Docker 전체 파이프라인) diff --git a/guide/development.md b/guide/development.md index 10b82f8..ddb45f8 100644 --- a/guide/development.md +++ b/guide/development.md @@ -205,11 +205,56 @@ go test ./internal/adapters/inbound/cli -run TestProjectSkills -count=1 - TDD: Red → Green → Refactor - Tidy First: Separate structural changes from behavioral changes -- Use GORM queries only (no raw SQL) +- Use GORM's model layer for queries; raw SQL only where GORM has no form for + the statement (see [Raw SQL](#raw-sql)) - Logging: `slog` - CLI: `cobra` framework - Build flags: `CGO_ENABLED=1 -tags "fts5"` +### Raw SQL + +Write queries through GORM's model layer — `Model`, `Where`, `FindInBatches`, +`Migrator` — wherever GORM has a form for the statement. That is the default, and +outside the two packages named below a raw `Raw`/`Exec` is a review stop. + +Raw SQL is allowed only where GORM has no form at all. That is not a matter of +taste; each category below names something GORM's builder or migrator cannot +express: + +- **Full-text operators and index maintenance** — SQLite FTS5 `MATCH` and its + `rank` column; PostgreSQL `to_tsvector`, `to_tsquery`, `ts_rank`, `@@`. GORM + has no builder form for a match operator or a rank expression. +- **DDL on the FTS5 virtual tables** — `CREATE VIRTUAL TABLE … USING fts5`, + and the `DROP`/`ALTER TABLE … RENAME TO` pairs the legacy upgrade needs. + `AutoMigrate` does not model a virtual table. +- **Writes into the FTS5 virtual tables** — the namespace-scoped deletes and the + bulk inserts. These tables have no GORM model and are not in `AutoMigrate`; + routing them through `Table("search_fts")` would name the table in a string + either way, and the bulk insert is one statement on purpose so a rebuild does + not pay a round trip per row. +- **Schema introspection GORM's migrator cannot do** — `PRAGMA table_info`, + `sqlite_master`, `information_schema.columns`, `pg_indexes`, `pg_trigger`. +- **Connection pragmas** — `PRAGMA journal_mode`, `PRAGMA busy_timeout`. + +These live in `internal/adapters/outbound/searchsql` and `internal/db` only. + +Two constraints hold inside an exempt statement. Table and column names come +from package constants, never from caller input. Every value is a bound +parameter — `Exec("DELETE FROM "+sqliteFTSTable+" WHERE namespace = ?", ns)` is +correct; concatenating `ns` into the string is not, and no amount of exemption +makes it correct. + +The introspection exemption is the one worth checking rather than trusting, +because GORM does ship `Migrator().HasTable`, `HasColumn` and `ColumnTypes`. +`searchsql/migrator_limits_test.go` runs all three against a real FTS5 table and +records what happens: `HasTable` works, `ColumnTypes` fails with `invalid DDL`, +and `HasColumn` matches the DDL text rather than the schema, so it can report +`false` for a column that exists. `HasTable` works but returns no error, while +every caller of `sqliteTableExists` propagates one — swallowing it would turn a +transient failure into "the table is absent" in the upgrade path. If a GORM +upgrade makes that test fail, the exemption should be reconsidered, not the +test relaxed. + ### Declaration order within a file Follow the standard-library convention of **cohesion over kind-grouping**: keep a diff --git a/guide/ko/development.md b/guide/ko/development.md index 720f123..dc08c7b 100644 --- a/guide/ko/development.md +++ b/guide/ko/development.md @@ -207,7 +207,48 @@ go test ./internal/adapters/inbound/cli -run TestProjectSkills -count=1 - TDD: Red → Green → Refactor - Tidy First: 구조적 변경과 행동 변경의 분리 -- GORM 쿼리만 사용 (Raw SQL 사용 금지) +- 쿼리는 GORM 모델 계층으로 작성. Raw SQL은 GORM에 대응 형태가 없는 경우에만 허용 + ([Raw SQL](#raw-sql) 참고) - 로깅: `slog` - CLI: `cobra` 프레임워크 - 빌드 플래그: `CGO_ENABLED=1 -tags "fts5"` + +### Raw SQL + +GORM에 대응 형태가 있는 문장은 모두 GORM 모델 계층(`Model`, `Where`, +`FindInBatches`, `Migrator`)으로 작성합니다. 이것이 기본이고, 아래 두 패키지 +밖에서 나타나는 `Raw`/`Exec`는 리뷰에서 멈춰야 합니다. + +Raw SQL은 GORM에 대응 형태가 아예 없는 경우에만 허용합니다. 취향 문제가 아니라, +아래 각 항목은 GORM builder나 migrator가 표현할 수 없는 것을 가리킵니다. + +- **전문 검색 연산자와 인덱스 관리** — SQLite FTS5의 `MATCH`와 `rank` 컬럼, + PostgreSQL의 `to_tsvector`, `to_tsquery`, `ts_rank`, `@@`. match 연산자나 rank + 표현식에 해당하는 builder 형태가 GORM에 없습니다. +- **FTS5 가상 테이블 DDL** — `CREATE VIRTUAL TABLE … USING fts5`, 그리고 legacy + 업그레이드가 필요로 하는 `DROP` / `ALTER TABLE … RENAME TO` 쌍. `AutoMigrate`는 + 가상 테이블을 모델링하지 않습니다. +- **FTS5 가상 테이블 쓰기** — namespace 범위 delete와 bulk insert. 이 테이블들은 + GORM 모델이 없고 `AutoMigrate` 대상도 아니어서, `Table("search_fts")`를 거쳐도 + 테이블 이름은 결국 문자열로 남습니다. bulk insert가 한 문장인 것은 의도이며, + rebuild가 행마다 round trip을 내지 않게 합니다. +- **GORM migrator가 못 하는 스키마 introspection** — `PRAGMA table_info`, + `sqlite_master`, `information_schema.columns`, `pg_indexes`, `pg_trigger`. +- **연결 pragma** — `PRAGMA journal_mode`, `PRAGMA busy_timeout`. + +허용 범위는 `internal/adapters/outbound/searchsql`와 `internal/db` 두 패키지뿐입니다. + +허용된 문장 안에서도 두 제약이 유지됩니다. 테이블·컬럼 이름은 패키지 상수에서만 +오고, 호출자 입력에서 오지 않습니다. 값은 항상 bound parameter입니다 — +`Exec("DELETE FROM "+sqliteFTSTable+" WHERE namespace = ?", ns)`는 맞고, `ns`를 +문자열에 이어 붙이는 것은 틀립니다. 어떤 예외도 이것을 맞게 만들지 않습니다. + +introspection 예외는 믿지 말고 확인해야 하는 항목입니다. GORM에도 +`Migrator().HasTable`, `HasColumn`, `ColumnTypes`가 있기 때문입니다. +`searchsql/migrator_limits_test.go`가 실제 FTS5 테이블을 상대로 셋 다 실행하고 +결과를 기록합니다: `HasTable`은 동작하고, `ColumnTypes`는 `invalid DDL`로 +실패하며, `HasColumn`은 스키마가 아니라 DDL 텍스트를 매칭하므로 존재하는 컬럼에도 +`false`를 반환할 수 있습니다. `HasTable`은 동작하지만 error를 돌려주지 않고, +`sqliteTableExists`의 모든 호출자는 error를 전파합니다. 그것을 삼키면 업그레이드 +경로에서 일시적 실패가 "테이블 없음"으로 바뀝니다. GORM 업그레이드로 그 테스트가 +깨지면 테스트를 느슨하게 만들 것이 아니라 예외 자체를 다시 판단해야 합니다. From 0f960d083508fadf7f0e960c13d1cd9275938356 Mon Sep 17 00:00:00 2001 From: tae2089 Date: Tue, 11 Aug 2026 16:23:49 +0900 Subject: [PATCH 3/3] docs(searchsql): point the package comment at the rule its SQL follows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/adapters/outbound/searchsql/backend.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/adapters/outbound/searchsql/backend.go b/internal/adapters/outbound/searchsql/backend.go index 2f409d1..cbc4aa6 100644 --- a/internal/adapters/outbound/searchsql/backend.go +++ b/internal/adapters/outbound/searchsql/backend.go @@ -1,4 +1,11 @@ // @index Shared search backend interface and errors for SQLite FTS5 and PostgreSQL tsvector implementations. +// +// The raw SQL in this package is bounded and deliberate: full-text operators, +// DDL and writes against the FTS5 virtual tables, and the schema introspection +// GORM's migrator cannot do. guide/development.md §Raw SQL states the rule and +// what falls under it; migrator_limits_test.go holds the evidence for the +// introspection part. Anything GORM's model layer can express belongs there +// instead, here as much as anywhere else. package searchsql import (