Skip to content

Operate on a single database from the DSN; drop --databases#8

Merged
winebarrel merged 1 commit into
mainfrom
single-database
May 2, 2026
Merged

Operate on a single database from the DSN; drop --databases#8
winebarrel merged 1 commit into
mainfrom
single-database

Conversation

@winebarrel

Copy link
Copy Markdown
Owner

Summary

myschema operates on exactly one database per invocation, so the CSV --databases / -n flag was needless ceremony. Fold the target database into the DSN and let the DSN parser be the single source of truth.

CLI:

  • MYSCHEMA_DSN is now required:"". Default removed — no sensible default for "which DB to manage". Example: root@tcp(127.0.0.1:3306)/mydb.
  • Options.Databases (and MYSCHEMA_DATABASES) removed.
  • New Client.Database() returns the parsed DSN's database, erroring when missing.

Catalog:

  • catalog.NewCatalog(conn, database string) — single DB.
  • catalog/tables.go uses WHERE TABLE_SCHEMA = ? directly. The dbPlaceholders helper is gone.

Tests:

  • testutil still connects via a bare DSN so SetupDB can DROP/CREATE the test DB.
  • test_helper_test.go's newClient appends myschema_test to the bare DSN.
  • test/scenario/helper.sh derives MYSCHEMA_DSN from MYSCHEMA_TEST_DSN + MYSCHEMA_TEST_DB; -n flag dropped everywhere.

ObjectCount.Databases []stringObjectCount.Database string. DBLabel() now always reads database <name>.

AGENTS.md updated: examples use MYSCHEMA_DSN and explain the single-database invariant.

Test plan

  • go build ./...
  • make test (parser + diff + catalog + YAML harness)
  • make test-scenario
  • golangci-lint clean
  • ./myschema --help — flag list shows DSN required, no -n
  • CI green on this PR

🤖 Generated with Claude Code

myschema operates on exactly one MySQL database per invocation, so the
CSV --databases / -n flag (and the MYSCHEMA_DATABASES env var) was
needless ceremony. Fold the target database into the DSN and let the
DSN parser be the single source of truth.

CLI:
- Options.DSN gains required:"" and `MYSCHEMA_DSN` env var; default
  removed (no sensible default exists for "which DB to manage").
- Options.Databases removed.
- New Client.Database() returns the parsed DSN's database, erroring
  when missing.
- All operations (apply, plan, dump, diffAll) call Client.Database()
  once and pass the string down.

Catalog:
- catalog.NewCatalog(conn, database string) — single DB.
- catalog/tables.go uses `WHERE TABLE_SCHEMA = ?` directly. The old
  dbPlaceholders helper had no other callers and is gone.

Test infrastructure:
- testutil keeps connecting via a bare DSN (no DB binding) so
  SetupDB can DROP/CREATE the test database.
- test_helper_test.go's newClient appends `myschema_test` to the bare
  DSN to satisfy the new "DSN must include a DB" rule.
- catalog_test.go uses the single-string NewCatalog signature.
- test/scenario/helper.sh derives MYSCHEMA_DSN from MYSCHEMA_TEST_DSN
  + MYSCHEMA_TEST_DB and drops the `-n` flag everywhere.

ObjectCount.Databases []string → ObjectCount.Database string;
DBLabel() now always reads "database <name>".

AGENTS.md updated: examples use MYSCHEMA_DSN and explain the
single-database invariant; the database-name remap entry is reframed
under "not implemented" rather than implying a multi-DB world.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@winebarrel winebarrel enabled auto-merge May 2, 2026 02:47
@winebarrel winebarrel merged commit c0570c5 into main May 2, 2026
2 checks passed
@winebarrel winebarrel deleted the single-database branch May 2, 2026 02:47
@codecov

codecov Bot commented May 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 59.37500% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.75%. Comparing base (802d61e) to head (4ab4b5c).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
client.go 33.33% 3 Missing and 3 partials ⚠️
apply.go 50.00% 1 Missing and 1 partial ⚠️
dump.go 60.00% 1 Missing and 1 partial ⚠️
plan.go 50.00% 1 Missing and 1 partial ⚠️
options.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main       #8      +/-   ##
==========================================
- Coverage   39.14%   38.75%   -0.39%     
==========================================
  Files          20       20              
  Lines        1313     1321       +8     
==========================================
- Hits          514      512       -2     
- Misses        698      702       +4     
- Partials      101      107       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

winebarrel added a commit that referenced this pull request May 2, 2026
Two inline comments, both valid.

parser/directive.go
  - classifyInlineLine: extend the unnamed-index guard to the two-word
    keyword forms (UNIQUE KEY, UNIQUE INDEX, FULLTEXT KEY/INDEX,
    SPATIAL KEY/INDEX). Without this, `UNIQUE KEY (col)` tokenizes
    with tokens[2]="(col)" and the directive mis-attached to a name
    of "(col)", later surfacing as "target index '(col)' not found".
    Mirrors the guard already in the plain KEY/INDEX branch.
    (Copilot #1.)
  - tokenize doc fixed: previous comment claimed it peeled off
    `","`, `"("`, `"("`, but the implementation only trims `,(`.
    Comment now matches reality. (Copilot #2.)

Tests
  - parser/directive_test.go: TestExtractInlineRenamesUnnamedTwoWordIndexIsUnsupported
    covers the new guard for `UNIQUE KEY (col)` and
    `FULLTEXT INDEX (col)`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel added a commit that referenced this pull request May 5, 2026
Round 1 review on PR #75 caught eight issues; this commit addresses
all of them.

1. (review #1) loadPreSQL treated any non-empty PreSQL string as
   "set", including whitespace-only values. Whitespace-only env
   var (`MYSCHEMA_PRE_SQL=`) would trigger the mutually-exclusive
   error against a legitimate --pre-sql-file, and skip the no-op
   short-circuit. Fix: TrimSpace both fields before deciding "set".

2. (review #2) loadPreSQL read --pre-sql-file with os.ReadFile,
   which doesn't support the repo's existing `-` for stdin
   convention used by parser.ReadSQLFile (the desired-SQL file
   args already accept it). Fix: route through parser.ReadSQLFile
   so `--pre-sql-file=-` works.

3. (review #3 + #4) runPreSQL was invoked AFTER connect(), so
   flag-validation errors (both flags set, missing file) could be
   masked by a downstream connection failure, and the client
   opened a DB connection it then threw away. Split into:
   - loadPreSQL: validate / read, no DB contact (called BEFORE connect)
   - execPreSQL: run on conn (called AFTER connect)
   apply.go and plan.go updated accordingly.

4. CLI-level mutual exclusion: tag both PreSQL and PreSQLFile with
   kong's `xor:"pre-sql"` so kong rejects the both-set case at
   parse time before our code sees it. The runtime check in
   loadPreSQL stays in place for programmatic API callers
   (Apply / Plan invoked from Go without going through kong).

5. (review #5 / #6 / #7 / #8) Test reshuffle. The original
   suite over-relied on success-only smoke tests that would have
   silently passed even if pre-SQL were skipped:
   - TestApply_PreSQLString and TestApply_PreSQLMultiStatement
     only checked NoError.
   - TestApply_PreSQLAppliesToSession claimed to be the
     "strongest behavioural pin" but probed nothing — the
     connection it would have probed is closed by Apply's defer.
   - TestPlan_PreSQLString same issue.
   Fix: load-bearing pins now feed INVALID pre-SQL and assert
   the apply / plan aborts with a wrapped "pre-sql" error
   containing the exact failing piece. The multi-statement
   split test now uses a payload with an invalid SECOND piece
   and asserts the error references that piece exactly (proves
   the split happened — without splitting, the driver would
   reject the whole concatenated string with a different error).

6. YAML migration: the new harness field `pre_sql` lets
   inline-payload tests live as testdata/apply/pre_sql_*.yml and
   testdata/plan/pre_sql_*.yml fixtures (3 + 1 = 4 new fixtures).
   File-related and programmatic-API tests stay in Go because
   the YAML harness has no `pre_sql_file` field (would need
   fixture-relative path handling, out of scope).

Coverage is unchanged at the package level; loadPreSQL stays at
100% and execPreSQL is exercised by all the real-execution paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant