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 업그레이드로 그 테스트가 +깨지면 테스트를 느슨하게 만들 것이 아니라 예외 자체를 다시 판단해야 합니다. 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 ( 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") + } +}