Operate on a single database from the DSN; drop --databases#8
Merged
Conversation
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>
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
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>
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.
Summary
myschema operates on exactly one database per invocation, so the CSV
--databases/-nflag was needless ceremony. Fold the target database into the DSN and let the DSN parser be the single source of truth.CLI:
MYSCHEMA_DSNis nowrequired:"". Default removed — no sensible default for "which DB to manage". Example:root@tcp(127.0.0.1:3306)/mydb.Options.Databases(andMYSCHEMA_DATABASES) removed.Client.Database()returns the parsed DSN's database, erroring when missing.Catalog:
catalog.NewCatalog(conn, database string)— single DB.catalog/tables.gousesWHERE TABLE_SCHEMA = ?directly. ThedbPlaceholdershelper is gone.Tests:
testutilstill connects via a bare DSN soSetupDBcan DROP/CREATE the test DB.test_helper_test.go'snewClientappendsmyschema_testto the bare DSN.test/scenario/helper.shderivesMYSCHEMA_DSNfromMYSCHEMA_TEST_DSN+MYSCHEMA_TEST_DB;-nflag dropped everywhere.ObjectCount.Databases []string→ObjectCount.Database string.DBLabel()now always readsdatabase <name>.AGENTS.md updated: examples use
MYSCHEMA_DSNand explain the single-database invariant.Test plan
go build ./...make test(parser + diff + catalog + YAML harness)make test-scenariogolangci-lintclean./myschema --help— flag list shows DSN required, no-n🤖 Generated with Claude Code