sneak/vaultik
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Move schema_migrations table creation into 000.sql with INTEGER version column #58

Open
clawbot wants to merge 2 commits from feat/migration-bootstrap-000sql into main
pull from: feat/migration-bootstrap-000sql
merge into: sneak:main
Conversation 3 Commits 2 Files Changed 5 +322 -28
clawbot commented 2026-03-26 17:45:40 +01:00
Collaborator
Copy Link

Closes #57

Adopts the pixa migration pattern for schema management. Replaces the monolithic schema.sql embed with a numbered migration system.

Changes

New: schema/000.sql — Bootstrap migration

  • Creates schema_migrations table with INTEGER PRIMARY KEY version column
  • Self-contained: includes both CREATE TABLE IF NOT EXISTS and INSERT OR IGNORE for version 0
  • Go code does zero INSERTs for bootstrap — just reads and executes 000.sql

Renamed: schema.sql → schema/001.sql — Initial schema migration

  • Full Vaultik schema (files, chunks, blobs, snapshots, uploads, all indexes)
  • Updated header comment to identify it as migration 001

Removed: schema/008_uploads.sql

  • Redundant — the uploads table with its current schema was already in the main schema file
  • The 008 file had a stale/different schema (TIMESTAMP instead of INTEGER, missing snapshot_id FK)

Rewritten: database.go — Migration engine

  • //go:embed schema/*.sql replaces //go:embed schema.sql
  • bootstrapMigrationsTable(): checks if schema_migrations table exists, applies 000.sql if missing
  • applyMigrations(): iterates through numbered .sql files, checks schema_migrations for each version, applies and records pending ones
  • collectMigrations(): reads embedded schema dir, returns sorted filenames
  • ParseMigrationVersion(): extracts numeric version from filenames like 001.sql or 001_description.sql (exported for testing)
  • Old createSchema() removed entirely

Updated: database_test.go

  • Verifies schema_migrations table exists alongside other core tables

Verification

docker build . passes — formatting, linting, all tests green.

Closes https://git.eeqj.de/sneak/vaultik/issues/57 Adopts the [pixa migration pattern](https://git.eeqj.de/sneak/pixa/pulls/36) for schema management. Replaces the monolithic `schema.sql` embed with a numbered migration system. ## Changes ### New: `schema/000.sql` — Bootstrap migration - Creates `schema_migrations` table with `INTEGER PRIMARY KEY` version column - Self-contained: includes both `CREATE TABLE IF NOT EXISTS` and `INSERT OR IGNORE` for version 0 - Go code does zero INSERTs for bootstrap — just reads and executes 000.sql ### Renamed: `schema.sql` → `schema/001.sql` — Initial schema migration - Full Vaultik schema (files, chunks, blobs, snapshots, uploads, all indexes) - Updated header comment to identify it as migration 001 ### Removed: `schema/008_uploads.sql` - Redundant — the uploads table with its current schema was already in the main schema file - The 008 file had a stale/different schema (TIMESTAMP instead of INTEGER, missing snapshot_id FK) ### Rewritten: `database.go` — Migration engine - `//go:embed schema/*.sql` replaces `//go:embed schema.sql` - `bootstrapMigrationsTable()`: checks if `schema_migrations` table exists, applies 000.sql if missing - `applyMigrations()`: iterates through numbered .sql files, checks `schema_migrations` for each version, applies and records pending ones - `collectMigrations()`: reads embedded schema dir, returns sorted filenames - `ParseMigrationVersion()`: extracts numeric version from filenames like `001.sql` or `001_description.sql` (exported for testing) - Old `createSchema()` removed entirely ### Updated: `database_test.go` - Verifies `schema_migrations` table exists alongside other core tables ## Verification `docker build .` passes — formatting, linting, all tests green.
clawbot added 1 commit 2026-03-26 17:45:40 +01:00
Move schema_migrations table creation into 000.sql with INTEGER version column
All checks were successful
check / check (pull_request) Successful in 2m30s
Details
eb1d5dd561 
Replace the monolithic schema.sql embed with a numbered migration system
following the pixa pattern:

- Add schema/000.sql: bootstrap migration creating schema_migrations table
  with INTEGER PRIMARY KEY version column and applied_at timestamp
- Move full schema to schema/001.sql (renamed from schema.sql)
- Remove redundant schema/008_uploads.sql (uploads table already in main schema)
- Rewrite database.go migration logic:
  - Embed schema/*.sql directory instead of single schema.sql
  - bootstrapMigrationsTable() checks for table existence, applies 000.sql
  - applyMigrations() iterates numbered files, skips already-applied versions
  - ParseMigrationVersion() extracts version from filenames (e.g. 001.sql)
  - Go code does zero INSERTs for bootstrap — 000.sql is self-contained
- Update database_test.go to verify schema_migrations table exists
clawbot added the needs-review label 2026-03-26 17:46:14 +01:00
clawbot commented 2026-03-26 17:49:55 +01:00
Author
Collaborator
Copy Link

Review: FAIL — Missing test coverage for migration engine

What's good

  • 000.sql matches pixa reference exactly — INTEGER PRIMARY KEY, self-contained CREATE TABLE IF NOT EXISTS + INSERT OR IGNORE. Identical to pixa's 000.sql. ✅
  • Go code does zero INSERTs for bootstrap — bootstrapMigrationsTable() reads and executes 000.sql; no Go-level INSERT for version 0. ✅
  • 008_uploads.sql removal is safe — the uploads table already exists in 001.sql with a better schema (INTEGER timestamps instead of TIMESTAMP, snapshot_id FK with CASCADE). The deleted 008 was stale and redundant. ✅
  • Migration ordering and idempotency logic is correct — bootstrap runs 000.sql when table missing, loop skips already-applied versions, upgrade path works because 001.sql uses CREATE TABLE IF NOT EXISTS. ✅
  • docker build . passes — lint, format check, and all tests green. ✅

What must be fixed

Missing TestParseMigrationVersion: The function is exported (the PR description says "exported for testing") and has non-trivial parsing logic with 5+ distinct code paths (empty filename, empty version prefix, underscore splitting, non-numeric chars, valid parse). Yet there are zero tests for it. The reference implementation pixa's database_test.go includes a comprehensive table-driven TestParseMigrationVersion with 9 test cases. Since issue #57 says to follow the pixa pattern, the matching test coverage is required.

Missing migration idempotency test: Pixa includes TestApplyMigrations_Idempotent that verifies running the migration engine twice succeeds without errors or duplicate rows in schema_migrations. This is a critical safety property for a migration engine — proving that re-running on an already-migrated database is a no-op. Vaultik should have an equivalent test.

Missing bootstrap isolation test: Pixa includes TestBootstrapMigrationsTable_FreshDatabase that verifies the bootstrap step in isolation (creates schema_migrations table and records version 0). This test exercises the bootstrap independently from the full migration loop.

Summary

The migration engine code itself is correct and faithfully reproduces the pixa pattern. The issue is test coverage — the PR adopts pixa's code but not pixa's tests. At minimum, add TestParseMigrationVersion (table-driven) and TestApplyMigrations_Idempotent. The bootstrap isolation test would also be nice to have.

Note: applyMigrations is unexported in vaultik (vs exported ApplyMigrations in pixa), but since vaultik's tests are in package database (not package database_test), they can call unexported functions directly, so this is fine.

## Review: FAIL — Missing test coverage for migration engine ### What's good - **`000.sql` matches pixa reference exactly** — `INTEGER PRIMARY KEY`, self-contained `CREATE TABLE IF NOT EXISTS` + `INSERT OR IGNORE`. Identical to [pixa's 000.sql](https://git.eeqj.de/sneak/pixa/src/branch/main/internal/database/schema/000.sql). ✅ - **Go code does zero INSERTs for bootstrap** — `bootstrapMigrationsTable()` reads and executes 000.sql; no Go-level INSERT for version 0. ✅ - **008_uploads.sql removal is safe** — the uploads table already exists in [001.sql](https://git.eeqj.de/sneak/vaultik/src/branch/feat/migration-bootstrap-000sql/internal/database/schema/001.sql#L120-L132) with a *better* schema (INTEGER timestamps instead of TIMESTAMP, snapshot_id FK with CASCADE). The deleted 008 was stale and redundant. ✅ - **Migration ordering and idempotency logic is correct** — bootstrap runs 000.sql when table missing, loop skips already-applied versions, upgrade path works because 001.sql uses `CREATE TABLE IF NOT EXISTS`. ✅ - **`docker build .` passes** — lint, format check, and all tests green. ✅ ### What must be fixed **Missing `TestParseMigrationVersion`**: The function is exported (the PR description says "exported for testing") and has non-trivial parsing logic with 5+ distinct code paths (empty filename, empty version prefix, underscore splitting, non-numeric chars, valid parse). Yet there are zero tests for it. The reference implementation [pixa's `database_test.go`](https://git.eeqj.de/sneak/pixa/src/branch/main/internal/database/database_test.go) includes a comprehensive table-driven `TestParseMigrationVersion` with 9 test cases. Since [issue #57](https://git.eeqj.de/sneak/vaultik/issues/57) says to follow the pixa pattern, the matching test coverage is required. **Missing migration idempotency test**: Pixa includes `TestApplyMigrations_Idempotent` that verifies running the migration engine twice succeeds without errors or duplicate rows in `schema_migrations`. This is a critical safety property for a migration engine — proving that re-running on an already-migrated database is a no-op. Vaultik should have an equivalent test. **Missing bootstrap isolation test**: Pixa includes `TestBootstrapMigrationsTable_FreshDatabase` that verifies the bootstrap step in isolation (creates `schema_migrations` table and records version 0). This test exercises the bootstrap independently from the full migration loop. ### Summary The migration engine code itself is correct and faithfully reproduces the pixa pattern. The issue is test coverage — the PR adopts pixa's code but not pixa's tests. At minimum, add `TestParseMigrationVersion` (table-driven) and `TestApplyMigrations_Idempotent`. The bootstrap isolation test would also be nice to have. Note: `applyMigrations` is unexported in vaultik (vs exported `ApplyMigrations` in pixa), but since vaultik's tests are in `package database` (not `package database_test`), they can call unexported functions directly, so this is fine.
clawbot added needs-rework and removed needs-review labels 2026-03-26 17:50:31 +01:00
clawbot added 1 commit 2026-03-26 17:55:42 +01:00
test: add migration engine tests
All checks were successful
check / check (pull_request) Successful in 4m26s
Details
197f09066b 
Add three tests requested in review:
- TestParseMigrationVersion: table-driven test with 9 cases covering valid
  filenames, descriptions, invalid alpha/mixed, and empty string
- TestApplyMigrations_Idempotent: verifies running migrations twice is a
  no-op with no duplicate rows in schema_migrations
- TestBootstrapMigrationsTable_FreshDatabase: isolation test verifying
  bootstrap creates schema_migrations table with version 0 row
clawbot commented 2026-03-26 17:55:51 +01:00
Author
Collaborator
Copy Link

Rework complete — added missing migration engine tests

Added all three tests requested in review:

TestParseMigrationVersion

Table-driven test with 9 cases:

  • Valid: 000.sql, 001.sql, 099.sql
  • Valid with description: 001_initial_schema.sql, 123_big_migration.sql
  • Invalid alpha: abc.sql
  • Invalid mixed: 12a.sql
  • Invalid word: schema.sql
  • Empty string

TestApplyMigrations_Idempotent

Opens a fresh in-memory DB, runs applyMigrations twice, verifies no errors and schema_migrations row count is unchanged between runs.

TestBootstrapMigrationsTable_FreshDatabase

Isolation test: fresh DB → bootstrapMigrationsTable → verifies schema_migrations table exists and version 0 row is present.

docker build . passes — formatting, linting, all tests green.

## Rework complete — added missing migration engine tests Added all three tests requested in [review](https://git.eeqj.de/sneak/vaultik/pulls/58#issuecomment-1369): ### TestParseMigrationVersion Table-driven test with 9 cases: - Valid: `000.sql`, `001.sql`, `099.sql` - Valid with description: `001_initial_schema.sql`, `123_big_migration.sql` - Invalid alpha: `abc.sql` - Invalid mixed: `12a.sql` - Invalid word: `schema.sql` - Empty string ### TestApplyMigrations_Idempotent Opens a fresh in-memory DB, runs `applyMigrations` twice, verifies no errors and `schema_migrations` row count is unchanged between runs. ### TestBootstrapMigrationsTable_FreshDatabase Isolation test: fresh DB → `bootstrapMigrationsTable` → verifies `schema_migrations` table exists and version 0 row is present. **`docker build .` passes** — formatting, linting, all tests green.
clawbot added needs-review and removed needs-rework labels 2026-03-26 17:56:32 +01:00
clawbot reviewed 2026-03-26 17:57:52 +01:00
clawbot left a comment
Author
Collaborator
Copy Link

Review: PASS ✅

All three tests requested in the previous review are present and meaningful.

TestParseMigrationVersion

9-case table-driven test covering:

  • Valid cases: 000.sql (zero), 001.sql, 099.sql (boundary), 001_initial_schema.sql and 123_big_migration.sql (with description suffix)
  • Invalid cases: abc.sql (pure alpha), 12a.sql (mixed chars), schema.sql (word without numeric prefix), empty string
  • Assertions check both return value and error presence correctly. Each invalid case verifies err != nil; each valid case verifies exact version number.

TestApplyMigrations_Idempotent

  • Opens fresh in-memory SQLite DB
  • Runs applyMigrations twice on the same connection
  • Counts schema_migrations rows after each run and asserts equality
  • Correctly exercises the "already applied" skip path in the migration loop

TestBootstrapMigrationsTable_FreshDatabase

  • Verifies schema_migrations does not exist before bootstrap (queries sqlite_master)
  • Runs bootstrapMigrationsTable
  • Verifies table now exists (count=1 in sqlite_master)
  • Verifies version 0 row is present via SELECT version FROM schema_migrations WHERE version = 0

No cheating

  • Makefile: untouched ✅
  • .golangci.yml: untouched ✅
  • CI config: untouched ✅
  • Dockerfile: untouched ✅

Migration engine code

  • 000.sql matches pixa reference exactly: INTEGER PRIMARY KEY, CREATE TABLE IF NOT EXISTS, INSERT OR IGNORE INTO schema_migrations (version) VALUES (0) ✅
  • schema.sql → schema/001.sql rename preserves full content with only header comment updated ✅
  • 008_uploads.sql removal is safe — uploads table already exists in 001.sql with a better schema (INTEGER timestamps, snapshot_id FK with CASCADE) ✅
  • Go code does zero INSERTs for bootstrap — just reads and executes 000.sql ✅
  • ParseMigrationVersion properly validates numeric-only prefixes with character-level checking before strconv.Atoi ✅

docker build .

Passes — lint, format check, all tests green. ✅

## Review: PASS ✅ All three tests requested in the [previous review](https://git.eeqj.de/sneak/vaultik/pulls/58#issuecomment-1369) are present and meaningful. ### TestParseMigrationVersion 9-case table-driven test covering: - **Valid cases**: `000.sql` (zero), `001.sql`, `099.sql` (boundary), `001_initial_schema.sql` and `123_big_migration.sql` (with description suffix) - **Invalid cases**: `abc.sql` (pure alpha), `12a.sql` (mixed chars), `schema.sql` (word without numeric prefix), empty string - Assertions check both return value and error presence correctly. Each invalid case verifies `err != nil`; each valid case verifies exact version number. ### TestApplyMigrations_Idempotent - Opens fresh in-memory SQLite DB - Runs `applyMigrations` **twice** on the same connection - Counts `schema_migrations` rows after each run and asserts equality - Correctly exercises the "already applied" skip path in the migration loop ### TestBootstrapMigrationsTable_FreshDatabase - Verifies `schema_migrations` does **not** exist before bootstrap (queries `sqlite_master`) - Runs `bootstrapMigrationsTable` - Verifies table **now exists** (count=1 in `sqlite_master`) - Verifies version 0 row is present via `SELECT version FROM schema_migrations WHERE version = 0` ### No cheating - Makefile: untouched ✅ - `.golangci.yml`: untouched ✅ - CI config: untouched ✅ - Dockerfile: untouched ✅ ### Migration engine code - `000.sql` matches [pixa reference](https://git.eeqj.de/sneak/pixa/src/branch/main/internal/database/schema/000.sql) exactly: `INTEGER PRIMARY KEY`, `CREATE TABLE IF NOT EXISTS`, `INSERT OR IGNORE INTO schema_migrations (version) VALUES (0)` ✅ - `schema.sql` → `schema/001.sql` rename preserves full content with only header comment updated ✅ - `008_uploads.sql` removal is safe — uploads table already exists in [001.sql](https://git.eeqj.de/sneak/vaultik/src/branch/feat/migration-bootstrap-000sql/internal/database/schema/001.sql) with a better schema (INTEGER timestamps, snapshot_id FK with CASCADE) ✅ - Go code does zero INSERTs for bootstrap — just reads and executes 000.sql ✅ - `ParseMigrationVersion` properly validates numeric-only prefixes with character-level checking before `strconv.Atoi` ✅ ### `docker build .` Passes — lint, format check, all tests green. ✅
clawbot added merge-ready and removed needs-review labels 2026-03-26 17:58:18 +01:00
sneak was assigned by clawbot 2026-03-26 17:58:19 +01:00
All checks were successful
check / check (pull_request) Successful in 4m26s
Details
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feat/migration-bootstrap-000sql:feat/migration-bootstrap-000sql
git checkout feat/migration-bootstrap-000sql
Sign in to join this conversation.
Reviewers
No Reviewers
clawbot
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-review
needs-review
needs-review
needs-review
needs-rework
Code review found issues to fix
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/vaultik#58
Delete Branch "feat/migration-bootstrap-000sql"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?