From 9c68bc25a5312e36383026e096dd03e476969dbb Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Wed, 9 Sep 2026 12:14:12 +0800 Subject: [PATCH 1/7] =?UTF-8?q?refactor=E2=99=BB=EF=B8=8F:=20report=20a=20?= =?UTF-8?q?failed=20migration=20instead=20of=20ending=20the=20process?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit run() called log.Fatalf on the first migration that failed, which ended the process from inside the migration engine. Nothing above it could record what happened - an installer needs to write down which version an attempt stopped on - and no test could exercise a failing migration at all without taking the test binary with it, which is why the one test that covers a failed migration drove the registered function directly and left the scheduler uncovered. run(), Migrate() and MigrateApp() now return an error, and the exit moved to the command layer where the exit code is the command's business. Two of those errors say more than "it failed". A migration that fails comes back as a *VersionFailure naming the version, because an installer records that as a diagnostic snapshot - the authoritative answer to where a retry resumes is always recomputed from sys_migration, never read back, and asking the database what is still pending answers a different question that merely has the same answer most of the time. An app code nothing registered under is now an error rather than a log line, so an installer asking for one app by name cannot be told that installing an app that does not exist succeeded; the command layer still rejects a typo before any database work. exitOnError is what makes the command exit non-zero, and it covers more than it replaces. Every path out of migrateModel used to return without an exit code: an unreachable tenant database or a failed AutoMigrate printed a line and exited 0, so a caller that migrates before starting a server - the deploy workflow does exactly that - carried on onto a schema that had not been brought forward. A failing migration function was the only failure reported, and only as a side effect of the log.Fatalf this commit removes. Each of these was checked by degrading it and watching the named assertion go red: returning nil instead of the failure, naming the first version rather than the one that failed, accepting an unregistered app code, and not exiting. One gap is left open deliberately. Go allows a call whose only result is an error to stand as a statement, so `migration.Migrate.Migrate()` still compiles while dropping what it returns - `go build` passed while migrateModel was doing exactly that during this change. Both call sites now return the value, which the compiler does check, but nothing guards against the statement form coming back. A checksilent rule was considered and dropped: that tool parses without type information, so it could only match the method name, and a guard that fires on any type with a Migrate method is noise. --- cmd/migrate/exit_test.go | 45 +++++++++++ cmd/migrate/migration/init.go | 50 ++++++++++-- cmd/migrate/migration/init_test.go | 118 ++++++++++++++++++++++------- cmd/migrate/server.go | 42 ++++++++-- 4 files changed, 211 insertions(+), 44 deletions(-) create mode 100644 cmd/migrate/exit_test.go diff --git a/cmd/migrate/exit_test.go b/cmd/migrate/exit_test.go new file mode 100644 index 00000000..22680912 --- /dev/null +++ b/cmd/migrate/exit_test.go @@ -0,0 +1,45 @@ +package migrate + +import ( + "bytes" + "errors" + "strings" + "testing" +) + +// A deployment decides whether to start the new version on this command's +// exit code. Before this batch the only failure that produced one was a +// failing migration function, and it produced it by ending the process from +// inside the migration engine; moving that out would have taken the last +// reported failure with it. +func TestExitOnErrorEndsTheCommandNonZero(t *testing.T) { + var codes []int + osExit = func(c int) { codes = append(codes, c) } + t.Cleanup(func() { osExit = origExit }) + + var out bytes.Buffer + exitOnError(&out, errors.New("the tenant database is unreachable")) + + if len(codes) != 1 || codes[0] != 1 { + t.Errorf("exit codes = %v, want [1]", codes) + } + if !strings.Contains(out.String(), "the tenant database is unreachable") { + t.Errorf("the reason was not reported: %q", out.String()) + } +} + +func TestExitOnErrorLetsSuccessThrough(t *testing.T) { + var codes []int + osExit = func(c int) { codes = append(codes, c) } + t.Cleanup(func() { osExit = origExit }) + + var out bytes.Buffer + exitOnError(&out, nil) + + if len(codes) != 0 { + t.Errorf("a successful migration exited with %v", codes) + } + if out.Len() != 0 { + t.Errorf("a successful migration wrote %q", out.String()) + } +} diff --git a/cmd/migrate/migration/init.go b/cmd/migrate/migration/init.go index 309d41c2..8f5070ab 100644 --- a/cmd/migrate/migration/init.go +++ b/cmd/migrate/migration/init.go @@ -284,12 +284,33 @@ func (e *Migration) Status() ([]StatusEntry, error) { } // Migrate applies every registered migration that has not been applied yet, -// across all apps. Existing callers are unaffected. -func (e *Migration) Migrate() { e.run(allApps) } +// across all apps. +func (e *Migration) Migrate() error { return e.run(allApps) } // MigrateApp applies only the migrations registered under appCode. Pass // FrameworkAppCode for the framework's own migrations. -func (e *Migration) MigrateApp(appCode string) { e.run(AppFilter(appCode)) } +func (e *Migration) MigrateApp(appCode string) error { return e.run(AppFilter(appCode)) } + +// VersionFailure names the migration that failed. +// +// The caller that needs this is an installer recording which version an +// install got stuck on. That is a diagnostic snapshot and nothing more: the +// authoritative answer to "where does a retry resume" is always recomputed +// by subtracting sys_migration's applied rows from what is registered, never +// read back from anywhere it was stored. Which is exactly why this carries +// the version rather than leaving the caller to infer it - inferring it +// would produce "what is pending now", a different question that happens to +// have the same answer most of the time. +type VersionFailure struct { + Version string + Err error +} + +func (e *VersionFailure) Error() string { + return fmt.Sprintf("migration %s failed: %v", e.Version, e.Err) +} + +func (e *VersionFailure) Unwrap() error { return e.Err } // NormalizeAppCode applies the same rule ForApp does, so a code typed on the // command line matches one written in an init(). @@ -332,7 +353,15 @@ func (e *Migration) AppCodes() []string { return out } -func (e *Migration) run(appCode string) { +// run applies the pending migrations selected by appCode. +// +// It reports failure instead of ending the process. It used to call +// log.Fatalf, which took the whole process down at the first failing +// migration - so a caller had nowhere to record what happened, and a test +// could not exercise a failing migration at all without killing the test +// binary. The exit now lives at the command layer, where the exit code is +// the command's business (see initDB in cmd/migrate/server.go). +func (e *Migration) run(appCode string) error { all := e.mergedEntries() versions := make([]string, 0, len(all)) entries := make(map[string]versionEntry, len(all)) @@ -347,10 +376,14 @@ func (e *Migration) run(appCode string) { // A mistyped --app would otherwise select nothing and report "no // migrations to apply", which reads exactly like "already up to date". + // + // The command layer rejects an unregistered code before any database + // work (exitUnlessAppRegistered), so on that path this is unreachable. + // It is reachable from an installer, which asks for one app by name and + // must not be told that installing an app nothing registered succeeded. if appCode != allApps && len(versions) == 0 { - log.Printf("no migrations are registered for app %q; registered: %s", + return fmt.Errorf("no migrations are registered for app %q; registered: %s", DisplayAppCode(appCode), strings.Join(e.AppCodes(), ", ")) - return } var err error @@ -359,7 +392,7 @@ func (e *Migration) run(appCode string) { for _, v := range versions { err = e.db.Table("sys_migration").Where("version = ?", v).Count(&count).Error if err != nil { - log.Fatalln(err) + return fmt.Errorf("checking whether migration %s was applied: %w", v, err) } if count > 0 { // Already applied. This used to print the bare count, so a mature @@ -369,7 +402,7 @@ func (e *Migration) run(appCode string) { } log.Printf("applying migration %s", v) if err = entries[v].fn(e.db.Debug(), v); err != nil { - log.Fatalf("migration %s failed: %v", v, err) + return &VersionFailure{Version: v, Err: err} } applied++ } @@ -378,6 +411,7 @@ func (e *Migration) run(appCode string) { } else { log.Printf("applied %d migration(s)", applied) } + return nil } // allApps is the sentinel run() takes to mean "do not filter". It is distinct diff --git a/cmd/migrate/migration/init_test.go b/cmd/migrate/migration/init_test.go index 13d8c6a6..e1287f8d 100644 --- a/cmd/migrate/migration/init_test.go +++ b/cmd/migrate/migration/init_test.go @@ -1,9 +1,7 @@ package migration import ( - "bytes" - "log" - "os" + "errors" "strings" "testing" "time" @@ -81,7 +79,9 @@ func TestForAppRecordsItsAppCode(t *testing.T) { m.ForApp("x").SetVersion("1786800001000", func(db *gorm.DB, version, appCode string) error { return recordFor(db, version, appCode) }) - m.Migrate() + if err := m.Migrate(); err != nil { + t.Fatalf("m.Migrate(): %v", err) + } rows := rowsByVersion(t, db) row, ok := rows["x-1786800001000"] @@ -104,7 +104,9 @@ func TestSetVersionStillRecordsTheFrameworkAsEmpty(t *testing.T) { m.SetVersion("1786700009000", func(db *gorm.DB, version string) error { return db.Create(&common.Migration{Version: version}).Error }) - m.Migrate() + if err := m.Migrate(); err != nil { + t.Fatalf("m.Migrate(): %v", err) + } rows := rowsByVersion(t, db) row, ok := rows["1786700009000"] @@ -136,7 +138,9 @@ func TestMigrateAppRunsOnlyThatApp(t *testing.T) { return recordFor(db, version, appCode) }) - m.MigrateApp("x") + if err := m.MigrateApp("x"); err != nil { + t.Fatalf("m.MigrateApp(\"x\"): %v", err) + } if !ran["x"] { t.Error("x did not run") @@ -167,7 +171,9 @@ func TestMigrateAppCoreSelectsTheFramework(t *testing.T) { return recordFor(db, version, appCode) }) - m.MigrateApp(FrameworkAppCode) + if err := m.MigrateApp(FrameworkAppCode); err != nil { + t.Fatalf("m.MigrateApp(FrameworkAppCode): %v", err) + } if !ran["core"] { t.Error("framework migration did not run") @@ -198,7 +204,9 @@ func TestMigrateRunsEveryApp(t *testing.T) { return recordFor(db, version, appCode) }) - m.Migrate() + if err := m.Migrate(); err != nil { + t.Fatalf("m.Migrate(): %v", err) + } // Namespacing puts every framework migration - bare digits - ahead of every // app migration, and orders apps by code rather than by whose timestamp @@ -232,7 +240,9 @@ func TestNamespacingKeepsTwoAppsWithTheSameTimestampApart(t *testing.T) { return recordFor(db, version, appCode) }) } - m.Migrate() + if err := m.Migrate(); err != nil { + t.Fatalf("m.Migrate(): %v", err) + } if ran != 2 { t.Errorf("ran %d migrations, want 2", ran) @@ -357,11 +367,11 @@ func TestFailedMigrationLeavesNoRecord(t *testing.T) { }) }) - // run() calls log.Fatal on failure, which would take the test binary with - // it, so drive the registered function directly - the point here is the - // transaction boundary, not the scheduler. - entry := m.version["crm-1786800001000"] - if err := entry.fn(db, "crm-1786800001000"); err == nil { + // Driven through the scheduler, not by calling the registered function + // directly. That workaround was here because run() called log.Fatal and + // would have taken the test binary with it, which also meant nothing + // covered what the scheduler does with a failure. + if err := m.MigrateApp("crm"); err == nil { t.Fatal("migration reported success") } if rows := rowsByVersion(t, db); len(rows) != 0 { @@ -369,6 +379,49 @@ func TestFailedMigrationLeavesNoRecord(t *testing.T) { } } +// An installer records which version an attempt got stuck on. It gets that +// from the error rather than by asking the database what is still pending, +// which is a different question - see VersionFailure. +func TestRunReportsWhichVersionFailed(t *testing.T) { + db := newTestDB(t) + m := newMigration() + m.SetDb(db) + + // Two versions, and the first one succeeds: the failure has to name the + // one that actually failed, which a report that just names the app, or + // the first version it looked at, would get wrong. + m.ForApp("crm").SetVersion("1786800001000", func(db *gorm.DB, version, appCode string) error { + return recordFor(db, version, appCode) + }) + m.ForApp("crm").SetVersion("1786800002000", func(db *gorm.DB, version, appCode string) error { + return errTestMigrationFailed + }) + + err := m.MigrateApp("crm") + if err == nil { + t.Fatal("MigrateApp reported success") + } + var vf *VersionFailure + if !errors.As(err, &vf) { + t.Fatalf("error is %T, want *VersionFailure: %v", err, err) + } + if vf.Version != "crm-1786800002000" { + t.Errorf("failed version = %q, want crm-1786800002000", vf.Version) + } + if !errors.Is(err, errTestMigrationFailed) { + t.Errorf("the cause is not reachable through the wrapper: %v", err) + } + // The one that succeeded before it stays recorded: a retry must not run + // it again. + rows := rowsByVersion(t, db) + if _, ok := rows["crm-1786800001000"]; !ok { + t.Errorf("the migration that succeeded was not recorded: %v", rows) + } + if _, ok := rows["crm-1786800002000"]; ok { + t.Errorf("the migration that failed was recorded: %v", rows) + } +} + var errTestMigrationFailed = &testError{"boom"} type testError struct{ s string } @@ -389,17 +442,18 @@ func TestMigrateAppOnAnUnknownCodeSaysSo(t *testing.T) { return recordFor(db, version, appCode) }) - var buf bytes.Buffer - log.SetOutput(&buf) - t.Cleanup(func() { log.SetOutput(os.Stderr) }) - - m.MigrateApp("crmm") - - if !strings.Contains(buf.String(), `no migrations are registered for app "crmm"`) { - t.Errorf("output = %q", buf.String()) + // Reported as an error rather than a log line, so an installer asking + // for one app by name cannot be told that installing an app nothing + // registered succeeded. + err := m.MigrateApp("crmm") + if err == nil { + t.Fatal("a typo reported success") } - if !strings.Contains(buf.String(), "registered: core, crm") { - t.Errorf("the message must list what is registered; got %q", buf.String()) + if !strings.Contains(err.Error(), `no migrations are registered for app "crmm"`) { + t.Errorf("error = %q", err) + } + if !strings.Contains(err.Error(), "registered: core, crm") { + t.Errorf("the message must list what is registered; got %q", err) } if rows := rowsByVersion(t, db); len(rows) != 0 { t.Errorf("a typo ran %v", rows) @@ -425,7 +479,9 @@ func TestMergedEntriesRunsAContractRegisteredAppMigration(t *testing.T) { return recordFor(db, version, appCode) }) - m.Migrate() + if err := m.Migrate(); err != nil { + t.Fatalf("m.Migrate(): %v", err) + } if !ran { t.Fatal("contract-registered migration did not run") @@ -466,7 +522,9 @@ func TestMergedEntriesStatusIncludesContractRegisteredMigrations(t *testing.T) { t.Fatalf("pending contract entry = %+v (ok=%v)", e, ok) } - m.Migrate() + if err := m.Migrate(); err != nil { + t.Fatalf("m.Migrate(): %v", err) + } entries, err = m.Status() if err != nil { @@ -522,7 +580,9 @@ func TestMergedEntriesMigrateAppRunsOnlyThatContractApp(t *testing.T) { return recordFor(db, version, appCode) }) - m.MigrateApp("order") + if err := m.MigrateApp("order"); err != nil { + t.Fatalf("m.MigrateApp(\"order\"): %v", err) + } if !ran["order"] { t.Error("order did not run") @@ -552,7 +612,9 @@ func TestMergedEntriesHostRegistrationWinsOnKeyCollision(t *testing.T) { return recordFor(db, version, appCode) }) - m.Migrate() + if err := m.Migrate(); err != nil { + t.Fatalf("m.Migrate(): %v", err) + } if !hostRan { t.Error("host registration did not run") diff --git a/cmd/migrate/server.go b/cmd/migrate/server.go index 951fedee..d845b0c7 100644 --- a/cmd/migrate/server.go +++ b/cmd/migrate/server.go @@ -3,6 +3,7 @@ package migrate import ( "bytes" "fmt" + "io" "os" "strconv" "strings" @@ -162,11 +163,9 @@ func migrateModel() error { } migration.Migrate.SetDb(db.Debug()) if appCode != "" { - migration.Migrate.MigrateApp(appCode) - return nil + return migration.Migrate.MigrateApp(appCode) } - migration.Migrate.Migrate() - return nil + return migration.Migrate.Migrate() } func initDB() { @@ -197,13 +196,40 @@ func initDB() { //4. 数据库迁移 fmt.Println("数据库迁移开始") - if err := migrateModel(); err != nil { - fmt.Println(err) - return - } + exitOnError(os.Stderr, migrateModel()) fmt.Println(`数据库基础数据初始化成功`) } +// exitOnError ends the command non-zero when the migration did not go through. +// +// A caller that migrates before starting a server decides whether to go ahead +// on the exit code alone - the deploy workflow does exactly that. Every path +// out of migrateModel used to return without one: an unreachable tenant +// database or a failed AutoMigrate printed a line and exited 0, so a +// deployment carried on onto a schema that had not been brought forward. A +// failing migration function was the only one reported, and only because it +// ended the process from inside the migration engine - which is the call this +// batch moved out here, so without this the last reported failure would have +// stopped being reported too. +// +// Split from the exit itself, the way appRegistrationError is split from +// exitUnlessAppRegistered, so what it decides can be tested without a +// subprocess. osExit is a variable for the same reason. +func exitOnError(w io.Writer, err error) { + if err == nil { + return + } + fmt.Fprintln(w, err) + osExit(1) +} + +// osExit is a variable so a test can watch the decision without ending the +// test binary; origExit is what it is put back to. +var ( + osExit = os.Exit + origExit = os.Exit +) + func runStatus() { config.Setup( file.NewSource(file.WithPath(configYml)), From 35d213f339a15709d3df6ea0db697a6a94471564 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Wed, 9 Sep 2026 12:22:16 +0800 Subject: [PATCH 2/7] =?UTF-8?q?feat=E2=9C=A8:=20install=20one=20applicatio?= =?UTF-8?q?n=20from=20its=20manifest?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `migrate install ` brings one application up to the version its manifest declares: it runs that application's outstanding migrations and records what it did in sys_app. It goes under migrate rather than under the existing `app` command, which already means "generate the skeleton of a new application" - a directory that does not exist yet, not an application already compiled into this binary. Installing one is running its migrations, which is what this command is, so --domain, resolveDB and the guard that refuses a mistyped code instead of reporting a successful no-op are all already here. Three phases, each committing on its own, and they are not one transaction. An application's versions are separate migration files, and on MySQL a DDL statement commits the transaction around it - destroying an outer transaction and every savepoint taken from it. So this does not promise that a half-installed application cannot happen. It promises one is visible when it does: phase A writes "installing" before anything that can fail, phase B runs the migrations, phase C turns that into "installed" or into "failed" with the version it stopped on. What is left to apply comes from sys_migration, never from sys_app. sys_app is a derived view - a summary, and the answer to "which version does this app think it is at". If it were the authority, an operator who deleted sys_migration rows by hand would be told an application is installed while its schema is not, which is worse than not knowing. So "already installed, nothing to do" needs all three: nothing outstanding, recorded as installed, and the same version. A row stuck at "installing" - what it reads as after the process was killed partway - is not installed, and retrying is just running the command again. An upgrade is in place and keeps the first install's time; a downgrade is refused, and refused before phase A writes anything, so a refusal cannot cost the operator the row that told them what they had. An unparseable recorded version is refused the same way, while it is still readable. The report ends by saying the code is not running yet. That is not a pleasantry: Go links at build time and Vite resolves its import globs at build time, so installing an application writes its menus, its APIs and its permissions and cannot make one line of its code run - and the menus appearing is exactly what makes an operator believe otherwise. Ten degradations were applied one at a time to check the tests name the behaviour rather than the shape: deciding the no-op from sys_app alone, always writing installed_at, allowing the downgrade, keeping the previous attempt's diagnostics on a row that now says installed, treating "installing" as installed, truncating last_error by bytes so a Chinese message is cut mid-rune, not recording the failure at all, skipping code normalization, and writing phase A before either the downgrade or the version-parse refusal. Each went red on the assertion it was aimed at. An eleventh was discarded rather than counted: it failed in the first install's setup, not on the claim. --- app/admin/models/sys_app.go | 16 ++ cmd/migrate/install.go | 308 ++++++++++++++++++++++++ cmd/migrate/install_test.go | 468 ++++++++++++++++++++++++++++++++++++ cmd/migrate/server.go | 42 ++++ 4 files changed, 834 insertions(+) create mode 100644 cmd/migrate/install.go create mode 100644 cmd/migrate/install_test.go diff --git a/app/admin/models/sys_app.go b/app/admin/models/sys_app.go index 72008266..885d5215 100644 --- a/app/admin/models/sys_app.go +++ b/app/admin/models/sys_app.go @@ -6,6 +6,22 @@ import ( "go-admin/common/models" ) +// The values sys_app.status takes. +// +// Three states rather than a single "installed", because an install that +// stopped partway has to be an observable row rather than the absence of one: +// the versions an app installs are separate migration files, and on MySQL a +// DDL statement commits the transaction around it, so they cannot be wrapped +// in one. See docs-prd/008-应用清单与安装器/数据库变更.md §1.5. +// +// AppInstalling is also what a row reads as after the process was killed +// mid-install, which is why it is not treated as "installed" by anything. +const ( + AppInstalling = 1 + AppInstalled = 2 + AppFailed = 3 +) + // SysApp is the sys_app row model: one row per installed application (PRD // 008 F2). It deliberately does not embed models.ModelTime - see the design // doc (docs-prd/008-应用清单与安装器/数据库变更.md) §1.1 for why an diff --git a/cmd/migrate/install.go b/cmd/migrate/install.go new file mode 100644 index 00000000..d8aeb25d --- /dev/null +++ b/cmd/migrate/install.go @@ -0,0 +1,308 @@ +package migrate + +import ( + "errors" + "fmt" + "io" + "sort" + "strings" + "time" + + "github.com/go-admin-team/go-admin-core/v2/sdk/contract/app" + "gorm.io/gorm" + + adminmodels "go-admin/app/admin/models" + "go-admin/cmd/migrate/migration" +) + +// engine is the part of the migration engine the installer drives. +// +// An interface rather than *migration.Migration because the concrete type is +// a package-level singleton with no exported constructor, so a test that took +// it would be sharing one registry with every other test in the process. +type engine interface { + SetDb(*gorm.DB) + Status() ([]migration.StatusEntry, error) + MigrateApp(string) error +} + +// installReport is what an install did, for the command to print. +type installReport struct { + Code string + // Version is the manifest version this run recorded. + Version string + // Previous is the version sys_app held before this run, empty when this + // is the first install. + Previous string + // Applied lists the versions this run brought in, in the order they were + // applied. Empty on a no-op, and also empty on a run that only corrected + // sys_app - the difference is NoOp. + Applied []string + // NoOp says nothing was left to do: the app is recorded as installed, at + // this same version, with no migration outstanding. + NoOp bool +} + +// install brings one application up to the version its manifest declares. +// +// Three phases, each committing on its own. They are not one transaction and +// cannot be: an application's versions are separate migration files, and a +// DDL statement inside any of them commits the transaction around it on +// MySQL, which destroys an outer transaction and every savepoint taken from +// it (docs-prd/008-应用清单与安装器/数据库变更.md §1.5). So this does not +// promise that a half-installed application cannot happen. It promises that +// one is visible when it does: phase A writes "installing" before anything +// that can fail, and phase C turns that into "installed" or "failed". +// +// What is left to apply comes from sys_migration, never from sys_app. +// sys_app is a derived view - a summary for a human, and the answer to "which +// version does this app think it is at". If it were the authority, then an +// operator who deleted sys_migration rows by hand would be told an app is +// installed while its schema is not, which is worse than not knowing. +func install(db *gorm.DB, eng engine, m app.Manifest) (installReport, error) { + code := migration.NormalizeAppCode(m.Code) + rep := installReport{Code: code, Version: m.Version} + if code == "" { + return rep, errors.New("the manifest declares no app code") + } + if code == migration.FrameworkAppCode { + // Installing the framework is what `migrate` is, and the framework + // has no manifest and no sys_app row. Saying so beats writing a row + // that nothing else in this batch expects to exist. + return rep, fmt.Errorf("%q is the framework's own migrations, not an application; run `migrate` for those", code) + } + if !db.Migrator().HasTable(&adminmodels.SysApp{}) { + return rep, errors.New("sys_app does not exist; run `migrate` first to bring the framework's own tables up to date") + } + + eng.SetDb(db) + + row, found, err := loadApp(db, code) + if err != nil { + return rep, err + } + // sameVersion is only meaningful when found; it stays false otherwise. + // The comparison happens here, before phase A, so an unparseable + // recorded version is refused while it is still readable rather than + // after being overwritten. + sameVersion := false + if found { + rep.Previous = row.Version + cmp, err := app.Compare(m.Version, row.Version) + if err != nil { + return rep, fmt.Errorf("comparing %s against the recorded %s: %w", m.Version, row.Version, err) + } + if cmp < 0 { + return rep, fmt.Errorf("%s is recorded at %s; installing %s would be a downgrade, which is not supported", + code, row.Version, m.Version) + } + sameVersion = cmp == 0 + } + + pending, err := pendingFor(eng, code) + if err != nil { + return rep, err + } + + // Nothing outstanding, recorded as installed, at this same version. All + // three, and the first one comes from sys_migration: a row that says + // installed while a migration of its has never run is exactly the case + // sys_app must not be believed about. AppInstalling is not installed - + // it is what a row reads as after the process was killed partway. + if found && sameVersion && row.Status == adminmodels.AppInstalled && len(pending) == 0 { + rep.NoOp = true + return rep, nil + } + + // Phase A: the attempt is on disk before anything that can fail. + now := time.Now() + if err := beginInstall(db, &row, m, code, found, now); err != nil { + return rep, err + } + + // Phase B: no atomicity across these, by the nature of the thing. + runErr := eng.MigrateApp(code) + + // Phase C. + if runErr != nil { + failed := "" + var vf *migration.VersionFailure + if errors.As(runErr, &vf) { + failed = vf.Version + } + if err := markFailed(db, code, failed, runErr, time.Now()); err != nil { + return rep, errors.Join(runErr, fmt.Errorf("recording the failure on sys_app: %w", err)) + } + return rep, runErr + } + if err := markInstalled(db, code, row.InstalledAt, time.Now()); err != nil { + return rep, err + } + rep.Applied = pending + return rep, nil +} + +// loadApp reads the sys_app row for code. A missing row is not an error: it +// is what a first install looks like. +func loadApp(db *gorm.DB, code string) (adminmodels.SysApp, bool, error) { + var row adminmodels.SysApp + err := db.Where("app_code = ?", code).First(&row).Error + if err == nil { + return row, true, nil + } + if errors.Is(err, gorm.ErrRecordNotFound) { + return adminmodels.SysApp{}, false, nil + } + return adminmodels.SysApp{}, false, fmt.Errorf("reading sys_app for %q: %w", code, err) +} + +// pendingFor is the authoritative answer to "what is left to apply", and it +// is recomputed every time rather than stored: what is registered in this +// process, minus what sys_migration says has run. sys_app.failed_version is a +// snapshot of what this returned once and may be stale by now; nothing may +// read it to decide this. +func pendingFor(eng engine, code string) ([]string, error) { + entries, err := eng.Status() + if err != nil { + return nil, err + } + var out []string + for _, e := range entries { + if e.AppCode == code && e.Registered && !e.Applied { + out = append(out, e.Version) + } + } + sort.Strings(out) + return out, nil +} + +// beginInstall is phase A. It refreshes every descriptive column from the +// manifest, because those are the manifest's to say and the row is only a +// copy, and it clears the two diagnostic columns so a stale failure from a +// previous attempt cannot be read as this one's. +func beginInstall(db *gorm.DB, row *adminmodels.SysApp, m app.Manifest, code string, found bool, now time.Time) error { + row.AppCode = code + row.Name = m.Name + row.Version = m.Version + row.Description = m.Description + row.Author = m.Author + row.Requires = strings.Join(m.Requires, ",") + row.Pricing = m.Pricing + row.License = m.License + row.Status = adminmodels.AppInstalling + row.FailedVersion = "" + row.LastError = "" + row.UpdatedAt = now + if !found { + if err := db.Create(row).Error; err != nil { + return fmt.Errorf("recording the install attempt for %q: %w", code, err) + } + return nil + } + if err := db.Save(row).Error; err != nil { + return fmt.Errorf("recording the install attempt for %q: %w", code, err) + } + return nil +} + +// markInstalled is the success half of phase C. installed_at is set once and +// never moved: an upgrade keeps the time of the first install, which is what +// the column is for. +// +// Computed here rather than with COALESCE so the statement is the same on all +// four drivers this repository supports. +func markInstalled(db *gorm.DB, code string, installedAt *time.Time, now time.Time) error { + updates := map[string]any{ + "status": adminmodels.AppInstalled, + "updated_at": now, + } + if installedAt == nil { + updates["installed_at"] = now + } + err := db.Model(&adminmodels.SysApp{}).Where("app_code = ?", code).Updates(updates).Error + if err != nil { + return fmt.Errorf("recording %q as installed: %w", code, err) + } + return nil +} + +// markFailed is the other half. Both columns it writes are diagnostic text +// for whoever reads the row; no code may branch on either one. +func markFailed(db *gorm.DB, code, failedVersion string, cause error, now time.Time) error { + updates := map[string]any{ + "status": adminmodels.AppFailed, + "failed_version": truncate(failedVersion, 64), + "last_error": truncate(cause.Error(), 255), + "updated_at": now, + } + return db.Model(&adminmodels.SysApp{}).Where("app_code = ?", code).Updates(updates).Error +} + +// truncate cuts s to at most n runes, not bytes: these columns are declared in +// characters, and a message that is partly Chinese would otherwise be cut in +// the middle of one and stored as an invalid sequence. +func truncate(s string, n int) string { + r := []rune(s) + if len(r) <= n { + return s + } + return string(r[:n]) +} + +// reportInstall prints what happened, and says that the data is in place but +// the code is not. +// +// That last sentence is not a pleasantry. Go links its applications at build +// time and Vite resolves its import globs at build time, so installing an +// application writes its menus, its APIs and its permissions and cannot make +// one line of its code run. An operator who is not told that sees the menus +// appear and reasonably concludes the thing is live. +func reportInstall(w io.Writer, rep installReport) { + if rep.NoOp { + fmt.Fprintf(w, "%s %s is already installed; nothing to do\n", rep.Code, rep.Version) + return + } + switch { + case rep.Previous == "": + fmt.Fprintf(w, "installed %s %s\n", rep.Code, rep.Version) + case rep.Previous == rep.Version: + fmt.Fprintf(w, "brought %s %s the rest of the way\n", rep.Code, rep.Version) + default: + fmt.Fprintf(w, "upgraded %s from %s to %s\n", rep.Code, rep.Previous, rep.Version) + } + if len(rep.Applied) > 0 { + fmt.Fprintf(w, "applied %d migration(s): %s\n", len(rep.Applied), strings.Join(rep.Applied, ", ")) + } else { + fmt.Fprintln(w, "no migration was outstanding; only sys_app was brought up to date") + } + fmt.Fprintln(w, "the database is up to date, but the application's code is not running yet:") + fmt.Fprintln(w, "rebuild and restart the server before expecting its routes to answer.") +} + +// manifestFor finds the manifest an application registered for code. +// +// A code nothing registered is an error naming what is registered, for the +// same reason exitUnlessAppRegistered exists: the alternative is telling an +// operator who typed `install ordr` that there was nothing to do. +func manifestFor(code string) (app.Manifest, error) { + want := migration.NormalizeAppCode(code) + all := app.Snapshot() + if m, ok := all[want]; ok { + return m, nil + } + codes := make([]string, 0, len(all)) + for c := range all { + codes = append(codes, c) + } + sort.Strings(codes) + if len(codes) == 0 { + // Worth its own sentence: no application is compiled into this + // binary at all, which is a different thing from having typed the + // wrong one of several. + return app.Manifest{}, fmt.Errorf( + "no application registers a manifest in this binary, so %q cannot be installed; "+ + "an application has to be compiled in before it can be installed", want) + } + return app.Manifest{}, fmt.Errorf("no application registers the code %q; registered: %s", + want, strings.Join(codes, ", ")) +} diff --git a/cmd/migrate/install_test.go b/cmd/migrate/install_test.go new file mode 100644 index 00000000..af7c64d8 --- /dev/null +++ b/cmd/migrate/install_test.go @@ -0,0 +1,468 @@ +package migrate + +import ( + "errors" + "strings" + "testing" + + "github.com/glebarez/sqlite" + "github.com/go-admin-team/go-admin-core/v2/sdk/contract/app" + "gorm.io/gorm" + "gorm.io/gorm/logger" + + adminmodels "go-admin/app/admin/models" + "go-admin/cmd/migrate/migration" +) + +func newInstallDB(t *testing.T) *gorm.DB { + t.Helper() + db, err := gorm.Open(sqlite.Open("file:"+t.Name()+"?mode=memory&cache=shared"), &gorm.Config{ + Logger: logger.Default.LogMode(logger.Silent), + }) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + if err := db.AutoMigrate(&adminmodels.SysApp{}); err != nil { + t.Fatalf("automigrate sys_app: %v", err) + } + return db +} + +// fakeEngine stands in for the migration engine. The real one is a +// package-level singleton with no exported constructor, so a test taking it +// would share one registry with every other test in this process. +type fakeEngine struct { + entries []migration.StatusEntry + // failWith, when set, is what MigrateApp returns instead of applying. + failWith error + calls []string +} + +func (f *fakeEngine) SetDb(*gorm.DB) {} + +func (f *fakeEngine) Status() ([]migration.StatusEntry, error) { + out := make([]migration.StatusEntry, len(f.entries)) + copy(out, f.entries) + return out, nil +} + +func (f *fakeEngine) MigrateApp(code string) error { + f.calls = append(f.calls, code) + if f.failWith != nil { + return f.failWith + } + for i := range f.entries { + if f.entries[i].AppCode == code && f.entries[i].Registered { + f.entries[i].Applied = true + } + } + return nil +} + +func orderManifest(version string) app.Manifest { + return app.Manifest{ + Code: "order", + Name: "Orders", + Version: version, + Description: "order management", + Author: "go-admin", + Requires: []string{"crm"}, + Pricing: "free", + License: "MIT", + } +} + +func loadRow(t *testing.T, db *gorm.DB, code string) adminmodels.SysApp { + t.Helper() + var row adminmodels.SysApp + if err := db.Where("app_code = ?", code).First(&row).Error; err != nil { + t.Fatalf("sys_app has no row for %q: %v", code, err) + } + return row +} + +// A1: a first install records the app, at the version the manifest declares, +// with every descriptive column copied from it. +func TestInstallRecordsAFirstInstall(t *testing.T) { + db := newInstallDB(t) + eng := &fakeEngine{entries: []migration.StatusEntry{ + {Version: "order-1786800001000", AppCode: "order", Registered: true}, + {Version: "order-1786800002000", AppCode: "order", Registered: true}, + {Version: "crm-1786800001000", AppCode: "crm", Registered: true}, + }} + + rep, err := install(db, eng, orderManifest("1.0.0")) + if err != nil { + t.Fatalf("install: %v", err) + } + if rep.NoOp { + t.Error("a first install reported nothing to do") + } + if got, want := len(rep.Applied), 2; got != want { + t.Errorf("applied %v, want %d versions", rep.Applied, want) + } + // Only this app's migrations, not every pending one in the process. + if len(eng.calls) != 1 || eng.calls[0] != "order" { + t.Errorf("MigrateApp calls = %v", eng.calls) + } + + row := loadRow(t, db, "order") + if row.Status != adminmodels.AppInstalled { + t.Errorf("status = %d, want installed", row.Status) + } + if row.Version != "1.0.0" { + t.Errorf("version = %q", row.Version) + } + if row.InstalledAt == nil { + t.Error("installed_at was not set") + } + if row.Name != "Orders" || row.Author != "go-admin" || row.Description != "order management" { + t.Errorf("descriptive columns not copied from the manifest: %+v", row) + } + if row.Requires != "crm" { + t.Errorf("requires = %q, want the manifest's list as CSV", row.Requires) + } + if row.Pricing != "free" || row.License != "MIT" { + t.Errorf("the reserved fields were not carried through: %+v", row) + } +} + +// A2: installing the same version again is a no-op, and says so. +func TestInstallIsANoOpAtTheSameVersion(t *testing.T) { + db := newInstallDB(t) + eng := &fakeEngine{entries: []migration.StatusEntry{ + {Version: "order-1786800001000", AppCode: "order", Registered: true}, + }} + if _, err := install(db, eng, orderManifest("1.0.0")); err != nil { + t.Fatalf("first install: %v", err) + } + before := loadRow(t, db, "order") + + rep, err := install(db, eng, orderManifest("1.0.0")) + if err != nil { + t.Fatalf("second install: %v", err) + } + if !rep.NoOp { + t.Error("installing the same version again was not reported as a no-op") + } + if len(eng.calls) != 1 { + t.Errorf("the engine was driven again: %v", eng.calls) + } + after := loadRow(t, db, "order") + if !after.UpdatedAt.Equal(before.UpdatedAt) { + t.Error("a no-op rewrote the row") + } + var n int64 + db.Model(&adminmodels.SysApp{}).Count(&n) + if n != 1 { + t.Errorf("sys_app has %d rows, want 1", n) + } +} + +// A no-op is only a no-op when nothing is outstanding. A row that says +// installed while a migration of its has never run is the case sys_app must +// not be believed over sys_migration. +func TestInstallRunsWhenTheRowSaysInstalledButAMigrationIsPending(t *testing.T) { + db := newInstallDB(t) + eng := &fakeEngine{entries: []migration.StatusEntry{ + {Version: "order-1786800001000", AppCode: "order", Registered: true, Applied: true}, + }} + if _, err := install(db, eng, orderManifest("1.0.0")); err != nil { + t.Fatalf("first install: %v", err) + } + + // A second version of the same app appears - the app was rebuilt with + // one more migration file, without its version changing. + eng.entries = append(eng.entries, migration.StatusEntry{ + Version: "order-1786800002000", AppCode: "order", Registered: true, + }) + + rep, err := install(db, eng, orderManifest("1.0.0")) + if err != nil { + t.Fatalf("install: %v", err) + } + if rep.NoOp { + t.Fatal("an outstanding migration was reported as nothing to do") + } + if len(rep.Applied) != 1 || rep.Applied[0] != "order-1786800002000" { + t.Errorf("applied = %v", rep.Applied) + } +} + +// A9: an upgrade is in place. installed_at is the first install's, not this +// one's. +func TestInstallUpgradesInPlaceAndKeepsTheFirstInstallTime(t *testing.T) { + db := newInstallDB(t) + eng := &fakeEngine{entries: []migration.StatusEntry{ + {Version: "order-1786800001000", AppCode: "order", Registered: true}, + }} + if _, err := install(db, eng, orderManifest("1.0.0")); err != nil { + t.Fatalf("first install: %v", err) + } + first := loadRow(t, db, "order") + if first.InstalledAt == nil { + t.Fatal("installed_at was not set by the first install") + } + + eng.entries = append(eng.entries, migration.StatusEntry{ + Version: "order-1786800002000", AppCode: "order", Registered: true, + }) + rep, err := install(db, eng, orderManifest("2.0.0")) + if err != nil { + t.Fatalf("upgrade: %v", err) + } + if rep.Previous != "1.0.0" { + t.Errorf("previous = %q, want 1.0.0", rep.Previous) + } + + row := loadRow(t, db, "order") + if row.Version != "2.0.0" { + t.Errorf("version = %q, want 2.0.0", row.Version) + } + if row.Status != adminmodels.AppInstalled { + t.Errorf("status = %d, want installed", row.Status) + } + if !row.InstalledAt.Equal(*first.InstalledAt) { + t.Errorf("installed_at moved from %v to %v; an upgrade keeps the first install's time", + first.InstalledAt, row.InstalledAt) + } +} + +// A10: a downgrade is refused, and refused before anything is written. +func TestInstallRefusesADowngrade(t *testing.T) { + db := newInstallDB(t) + eng := &fakeEngine{entries: []migration.StatusEntry{ + {Version: "order-1786800001000", AppCode: "order", Registered: true}, + }} + if _, err := install(db, eng, orderManifest("2.0.0")); err != nil { + t.Fatalf("first install: %v", err) + } + before := loadRow(t, db, "order") + + _, err := install(db, eng, orderManifest("1.0.0")) + if err == nil { + t.Fatal("a downgrade was accepted") + } + if !strings.Contains(err.Error(), "downgrade") { + t.Errorf("error = %q, it has to say what it refused", err) + } + after := loadRow(t, db, "order") + if after.Version != before.Version || after.Status != before.Status { + t.Errorf("the refused downgrade still wrote to the row: %+v -> %+v", before, after) + } +} + +// A5: a failing migration leaves a row that says so, and says where. +func TestInstallRecordsAFailure(t *testing.T) { + db := newInstallDB(t) + boom := errors.New("the seed hit a duplicate") + eng := &fakeEngine{ + entries: []migration.StatusEntry{ + {Version: "order-1786800001000", AppCode: "order", Registered: true}, + }, + failWith: &migration.VersionFailure{Version: "order-1786800001000", Err: boom}, + } + + _, err := install(db, eng, orderManifest("1.0.0")) + if err == nil { + t.Fatal("a failed install reported success") + } + if !errors.Is(err, boom) { + t.Errorf("the cause is not reachable: %v", err) + } + + row := loadRow(t, db, "order") + if row.Status != adminmodels.AppFailed { + t.Errorf("status = %d, want failed", row.Status) + } + if row.FailedVersion != "order-1786800001000" { + t.Errorf("failed_version = %q", row.FailedVersion) + } + if !strings.Contains(row.LastError, "duplicate") { + t.Errorf("last_error = %q", row.LastError) + } + if row.InstalledAt != nil { + t.Error("installed_at was set by an install that failed") + } +} + +// A failed install is retried by running it again - not by any special +// command, and without the previous attempt's diagnostics surviving into a +// row that now says installed. +func TestInstallResumesAfterAFailure(t *testing.T) { + db := newInstallDB(t) + eng := &fakeEngine{ + entries: []migration.StatusEntry{ + {Version: "order-1786800001000", AppCode: "order", Registered: true}, + }, + failWith: &migration.VersionFailure{Version: "order-1786800001000", Err: errors.New("boom")}, + } + if _, err := install(db, eng, orderManifest("1.0.0")); err == nil { + t.Fatal("the first attempt did not fail") + } + + eng.failWith = nil + rep, err := install(db, eng, orderManifest("1.0.0")) + if err != nil { + t.Fatalf("retry: %v", err) + } + if rep.NoOp { + t.Error("a failed row was treated as installed") + } + + row := loadRow(t, db, "order") + if row.Status != adminmodels.AppInstalled { + t.Errorf("status = %d, want installed", row.Status) + } + if row.FailedVersion != "" || row.LastError != "" { + t.Errorf("the previous failure survived onto a row that now says installed: %q / %q", + row.FailedVersion, row.LastError) + } + if row.InstalledAt == nil { + t.Error("installed_at was not set by the attempt that succeeded") + } +} + +// A row stuck at installing - the process was killed partway - is not +// installed, and must not be mistaken for it. +func TestInstallRetriesARowStuckAtInstalling(t *testing.T) { + db := newInstallDB(t) + if err := db.Create(&adminmodels.SysApp{ + AppCode: "order", Name: "Orders", Version: "1.0.0", + Status: adminmodels.AppInstalling, + }).Error; err != nil { + t.Fatalf("seed: %v", err) + } + eng := &fakeEngine{entries: []migration.StatusEntry{ + {Version: "order-1786800001000", AppCode: "order", Registered: true, Applied: true}, + }} + + rep, err := install(db, eng, orderManifest("1.0.0")) + if err != nil { + t.Fatalf("install: %v", err) + } + if rep.NoOp { + t.Fatal("a row stuck at installing was reported as already installed") + } + if row := loadRow(t, db, "order"); row.Status != adminmodels.AppInstalled { + t.Errorf("status = %d, want installed", row.Status) + } +} + +func TestInstallRejectsTheFrameworkCode(t *testing.T) { + db := newInstallDB(t) + m := orderManifest("1.0.0") + m.Code = migration.FrameworkAppCode + _, err := install(db, &fakeEngine{}, m) + if err == nil { + t.Fatal("the framework was installed as an application") + } + if !strings.Contains(err.Error(), "migrate") { + t.Errorf("error = %q, it should point at the command that does this", err) + } +} + +func TestInstallRefusesAnUnparseableRecordedVersion(t *testing.T) { + db := newInstallDB(t) + if err := db.Create(&adminmodels.SysApp{ + AppCode: "order", Name: "Orders", Version: "v1.0", Status: adminmodels.AppInstalled, + }).Error; err != nil { + t.Fatalf("seed: %v", err) + } + _, err := install(db, &fakeEngine{}, orderManifest("1.0.0")) + if err == nil { + t.Fatal("an unparseable recorded version was compared anyway") + } + row := loadRow(t, db, "order") + if row.Status != adminmodels.AppInstalled || row.Version != "v1.0" { + t.Errorf("the row was overwritten before the comparison failed: %+v", row) + } +} + +// A7: the report has to say the code is not running yet. Menus appearing is +// exactly what makes an operator think it is. +func TestReportInstallSaysTheCodeIsNotRunningYet(t *testing.T) { + var out strings.Builder + reportInstall(&out, installReport{Code: "order", Version: "1.0.0", Applied: []string{"order-1786800001000"}}) + got := out.String() + if !strings.Contains(got, "rebuild") || !strings.Contains(got, "restart") { + t.Errorf("the report does not say the binary has to be rebuilt: %q", got) + } + if !strings.Contains(got, "order-1786800001000") { + t.Errorf("the report does not name what it applied: %q", got) + } +} + +func TestReportInstallOnANoOp(t *testing.T) { + var out strings.Builder + reportInstall(&out, installReport{Code: "order", Version: "1.0.0", NoOp: true}) + if !strings.Contains(out.String(), "already installed") { + t.Errorf("output = %q", out.String()) + } +} + +// last_error is a varchar(255) declared in characters. A message that is +// partly Chinese would be cut mid-rune by a byte-wise truncation and stored +// as an invalid sequence. +func TestTruncateCutsRunesNotBytes(t *testing.T) { + s := strings.Repeat("迁", 300) + got := truncate(s, 255) + if n := len([]rune(got)); n != 255 { + t.Errorf("kept %d runes, want 255", n) + } + if !strings.HasPrefix(s, got) { + t.Error("truncation did not cut at a rune boundary") + } + if short := truncate("ok", 255); short != "ok" { + t.Errorf("a short message was altered: %q", short) + } +} + +func TestInstallNeedsSysApp(t *testing.T) { + db, err := gorm.Open(sqlite.Open("file:"+t.Name()+"?mode=memory&cache=shared"), &gorm.Config{ + Logger: logger.Default.LogMode(logger.Silent), + }) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + _, err = install(db, &fakeEngine{}, orderManifest("1.0.0")) + if err == nil { + t.Fatal("install ran against a database with no sys_app") + } + if !strings.Contains(err.Error(), "migrate") { + t.Errorf("error = %q, it should say what to run first", err) + } +} + +// The code written to sys_app and handed to the engine is the normalized one. +// A manifest whose Code was typed with different case or stray spaces has to +// land on the same identity migration.ForApp and seed.SeedMenus already use, +// or the row and the migrations it stands for are filed under two names. +func TestInstallNormalizesTheAppCode(t *testing.T) { + db := newInstallDB(t) + eng := &fakeEngine{entries: []migration.StatusEntry{ + {Version: "order-1786800001000", AppCode: "order", Registered: true}, + }} + m := orderManifest("1.0.0") + m.Code = " Order " + + rep, err := install(db, eng, m) + if err != nil { + t.Fatalf("install: %v", err) + } + if rep.Code != "order" { + t.Errorf("reported code = %q, want order", rep.Code) + } + if len(eng.calls) != 1 || eng.calls[0] != "order" { + t.Errorf("the engine was asked for %v, want [order]", eng.calls) + } + // The row has to be findable by the normalized code, which is what every + // other table in this batch is keyed by. + row := loadRow(t, db, "order") + if row.AppCode != "order" { + t.Errorf("app_code = %q", row.AppCode) + } + if len(rep.Applied) != 1 { + t.Errorf("applied = %v; the normalized code has to match what Status reports", rep.Applied) + } +} diff --git a/cmd/migrate/server.go b/cmd/migrate/server.go index d845b0c7..337ecc5d 100644 --- a/cmd/migrate/server.go +++ b/cmd/migrate/server.go @@ -48,6 +48,22 @@ var ( runStatus() }, } + // Under migrate rather than under the existing `app` command, which + // already means "generate the skeleton of a new app" - a directory that + // does not exist yet, not an application already compiled into this + // binary. Installing an application is running its migrations, which is + // what this command is; --app, --domain and resolveDB are all already + // here, including the guard that refuses a mistyped code instead of + // reporting a successful no-op. + installCmd = &cobra.Command{ + Use: "install ", + Short: "Install one application: run its migrations and record it in sys_app", + Example: "go-admin migrate install order -c config/settings.yml", + Args: cobra.ExactArgs(1), + Run: func(cmd *cobra.Command, args []string) { + runInstall(args[0]) + }, + } ) // fixme 在您看不见代码的时候运行迁移,我觉得是不安全的,所以编译后最好不要去执行迁移 @@ -65,6 +81,7 @@ func init() { StartCmd.Flags().BoolVar(&dryRun, "dry-run", false, "list what would be applied, in order, and write nothing") StartCmd.AddCommand(statusCmd) + StartCmd.AddCommand(installCmd) } func run() { @@ -255,6 +272,31 @@ func runStatus() { ) } +func runInstall(code string) { + config.Setup( + file.NewSource(file.WithPath(configYml)), + func() { + database.Setup() + db, err := resolveDB() + if err != nil { + exitOnError(os.Stderr, err) + return + } + m, err := manifestFor(code) + if err != nil { + exitOnError(os.Stderr, err) + return + } + rep, err := install(db, migration.Migrate, m) + if err != nil { + exitOnError(os.Stderr, err) + return + } + reportInstall(os.Stdout, rep) + }, + ) +} + func genFile() error { t1, err := template.ParseFiles("template/migrate.template") if err != nil { From 412413c12f08b7364485a2271c448cb293f941e6 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Wed, 9 Sep 2026 12:27:38 +0800 Subject: [PATCH 3/7] =?UTF-8?q?feat=E2=9C=A8:=20record=20which=20casbin=20?= =?UTF-8?q?policies=20an=20install=20created?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sys_app_casbin_grant has existed since the registry tables were added and nothing ever wrote to it. An uninstaller reading it would have found it empty, deleted no policy at all, and reported every one of them as an unattributable leftover - which is what "report and skip" looks like when the ledger was simply never written, and is indistinguishable from it working. grantToAdminRole now writes an entry for each policy it creates. The entry carries the tuple casbin_rule is unique on rather than a foreign key into it, because casbin_rule is not this project's table: the gorm adapter's SavePolicy truncates it and writes it back from memory, and SysRole.Update replaces a role's policy rows wholesale. Both rebuild the same tuple from the same sys_menu/sys_api data, so a match on the tuple survives what a row id does not. Only policies this install actually created are recorded - the insert is conditional and its RowsAffected says which. A policy that was already there was granted by somebody else and is not this app's to take away. The two ways that can be wrong are not equally bad, which is what settles it. Under-recording leaves a policy behind and the uninstall says so, because a policy naming this app's own path with no ledger entry is exactly what it reports as an orphan. Over-recording deletes somebody's authorization, silently. Between a visible leftover and an invisible deletion, take the leftover. The ledger insert is itself conditional, for a case the obvious retry test does not reach: on a plain re-run the policy still exists, so the insert is skipped before the ledger is touched. It is reached when the policy row was removed while its entry stayed, and a plain insert would then abort the whole seed on the ledger's unique index. There is a test for that specific shape, and replacing the insert with a plain one turns it red - which the plain retry test does not. Ordering: the ledger table is created by a framework migration, and version strings sort bare digits ahead of any app-prefixed one, so it exists before any application's seed runs. Nothing in the framework's own migrations calls SeedMenus. --- app/admin/service/seed.go | 50 +++++++++++- app/admin/service/seed_test.go | 144 ++++++++++++++++++++++++++++++++- 2 files changed, 189 insertions(+), 5 deletions(-) diff --git a/app/admin/service/seed.go b/app/admin/service/seed.go index e856566e..0e73a59a 100644 --- a/app/admin/service/seed.go +++ b/app/admin/service/seed.go @@ -5,6 +5,7 @@ import ( "fmt" "strconv" "strings" + "time" "gorm.io/gorm" @@ -76,7 +77,7 @@ func (adminSeeder) SeedMenus(tx *gorm.DB, appCode string, menus []seed.MenuSpec, if len(menuIDs) == 0 && len(apiRows) == 0 { return nil } - return grantToAdminRole(tx, menuIDs, apiRows) + return grantToAdminRole(tx, appCode, menuIDs, apiRows) } // seedApis writes one sys_api row per ApiSpec and returns them keyed by @@ -422,7 +423,7 @@ func pascalCase(s string) string { // framework migration sorts before every app-prefixed one - means that // should not happen in practice, but failing this call over it would be // worse than a menu with no grant yet. -func grantToAdminRole(tx *gorm.DB, menuIDs []int, apiRows map[string]models.SysApi) error { +func grantToAdminRole(tx *gorm.DB, appCode string, menuIDs []int, apiRows map[string]models.SysApi) error { var role models.SysRole if err := tx.Where("role_key = ?", adminRoleKey).First(&role).Error; err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { @@ -441,12 +442,53 @@ func grantToAdminRole(tx *gorm.DB, menuIDs []int, apiRows map[string]models.SysA } for _, a := range apiRows { - if err := tx.Exec( + res := tx.Exec( "INSERT INTO casbin_rule (ptype, v0, v1, v2, v3, v4, v5) SELECT 'p', ?, ?, ?, '', '', '' WHERE NOT EXISTS (SELECT 1 FROM casbin_rule WHERE ptype='p' AND v0=? AND v1=? AND v2=?)", role.RoleKey, a.Path, a.Action, role.RoleKey, a.Path, a.Action, - ).Error; err != nil { + ) + if res.Error != nil { + return res.Error + } + if res.RowsAffected == 0 { + // The policy was already there, so this install did not create + // it and it is not this app's to take away. Leaving it out of + // the ledger is what makes an uninstall report it instead of + // deleting it. + // + // The two ways this can be wrong are not equally bad, which is + // what settles it. Under-recording leaves a policy behind and + // the uninstall says so, because a policy naming an app's own + // path with no ledger entry is exactly what it lists as an + // orphan. Over-recording deletes a grant somebody else made, + // silently. Between a visible leftover and an invisible + // deletion of somebody's authorization, take the leftover. + continue + } + if err := recordGrant(tx, appCode, role.RoleKey, a.Path, a.Action); err != nil { return err } } return nil } + +// recordGrant writes down that this application's install created one casbin +// policy, keyed by the same tuple casbin_rule is unique on. +// +// A ledger rather than a column on casbin_rule, because casbin_rule is not +// this project's table: the gorm adapter's SavePolicy truncates it and writes +// it back from memory, which would drop any column added here, and +// SysRole.Update replaces a role's policy rows wholesale. The tuple survives +// both, because both rebuild it from the same sys_menu/sys_api data. +// +// Written with the same INSERT ... WHERE NOT EXISTS shape as the policy above +// rather than a plain insert: the ledger's unique index covers the tuple +// alone, so a duplicate would abort the whole seed instead of being the +// no-op it should be. +func recordGrant(tx *gorm.DB, appCode, roleKey, path, action string) error { + return tx.Exec( + "INSERT INTO sys_app_casbin_grant (app_code, ptype, v0, v1, v2, v3, v4, v5, created_at) "+ + "SELECT ?, 'p', ?, ?, ?, '', '', '', ? WHERE NOT EXISTS "+ + "(SELECT 1 FROM sys_app_casbin_grant WHERE ptype='p' AND v0=? AND v1=? AND v2=? AND v3='' AND v4='' AND v5='')", + appCode, roleKey, path, action, time.Now(), roleKey, path, action, + ).Error +} diff --git a/app/admin/service/seed_test.go b/app/admin/service/seed_test.go index 9fc02557..de220b75 100644 --- a/app/admin/service/seed_test.go +++ b/app/admin/service/seed_test.go @@ -32,7 +32,14 @@ func newSeedTestDB(t *testing.T) *gorm.DB { if err != nil { t.Fatalf("open: %v", err) } - if err := db.AutoMigrate(&models.SysMenu{}, &models.SysApi{}, &models.SysRole{}); err != nil { + // sys_app_casbin_grant is where grantToAdminRole records which policies + // this install created, so an uninstall can tell them from the ones + // somebody granted by hand. In a real database it is created by + // 1786700007000, which is a framework migration and therefore runs ahead + // of every application's - version strings sort bare digits before any + // app-prefixed one. + if err := db.AutoMigrate(&models.SysMenu{}, &models.SysApi{}, &models.SysRole{}, + &models.SysAppCasbinGrant{}); err != nil { t.Fatalf("automigrate: %v", err) } if err := db.Exec(`CREATE TABLE casbin_rule ( @@ -841,3 +848,138 @@ func TestSeedMenusPreservesAHandAddedBinding(t *testing.T) { t.Errorf("the seed's own binding count = %d, want 1 - it must survive the retry too", n) } } + +// Every policy grantToAdminRole creates has to be written down, or an +// uninstall has no way to tell this app's grants from a hand-made one and +// leaves all of them behind. +func TestSeedMenusRecordsTheGrantsItCreated(t *testing.T) { + db := newSeedTestDB(t) + role := seedAdminRole(t, db) + + apis := []seed.ApiSpec{ + {Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET", Handle: "apis.Order.GetPage-fm"}, + {Code: "create", Title: "Create order", Path: "/api/v1/order", Method: "POST", Handle: "apis.Order.Insert-fm"}, + } + if err := (adminSeeder{}).SeedMenus(db, "order", nil, apis); err != nil { + t.Fatalf("SeedMenus: %v", err) + } + + for _, a := range apis { + var n int64 + db.Model(&models.SysAppCasbinGrant{}). + Where("app_code = ? AND ptype = 'p' AND v0 = ? AND v1 = ? AND v2 = ?", + "order", role.RoleKey, a.Path, a.Method). + Count(&n) + if n != 1 { + t.Errorf("ledger rows for %s %s = %d, want 1", a.Method, a.Path, n) + } + } +} + +// A policy that was already there was not created by this install, so it is +// not this app's to take away later. Recording it would mean an uninstall +// deletes a grant somebody else made, and deletes it silently - the opposite +// mistake leaves a policy behind, which the uninstall reports. +func TestSeedMenusDoesNotClaimAPolicyItDidNotCreate(t *testing.T) { + db := newSeedTestDB(t) + role := seedAdminRole(t, db) + + if err := db.Exec( + "INSERT INTO casbin_rule (ptype, v0, v1, v2, v3, v4, v5) VALUES ('p', ?, '/api/v1/order', 'GET', '', '', '')", + role.RoleKey, + ).Error; err != nil { + t.Fatalf("pre-existing policy: %v", err) + } + + apis := []seed.ApiSpec{ + {Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET", Handle: "apis.Order.GetPage-fm"}, + {Code: "create", Title: "Create order", Path: "/api/v1/order", Method: "POST", Handle: "apis.Order.Insert-fm"}, + } + if err := (adminSeeder{}).SeedMenus(db, "order", nil, apis); err != nil { + t.Fatalf("SeedMenus: %v", err) + } + + var claimed int64 + db.Model(&models.SysAppCasbinGrant{}). + Where("v1 = ? AND v2 = ?", "/api/v1/order", "GET").Count(&claimed) + if claimed != 0 { + t.Errorf("the ledger claimed a policy that was already there (%d rows)", claimed) + } + // The one it did create is still recorded: the skip is per policy, not + // for the whole call. + var created int64 + db.Model(&models.SysAppCasbinGrant{}). + Where("v1 = ? AND v2 = ?", "/api/v1/order", "POST").Count(&created) + if created != 1 { + t.Errorf("ledger rows for the policy it did create = %d, want 1", created) + } + // And the pre-existing policy itself is untouched. + var policies int64 + db.Table("casbin_rule").Where("v1 = ? AND v2 = ?", "/api/v1/order", "GET").Count(&policies) + if policies != 1 { + t.Errorf("casbin_rule rows = %d, want the one that was already there", policies) + } +} + +// A migration that failed partway is re-run whole. The ledger must come out +// of a second run the same as the first, not with a duplicate or an error +// from its own unique index. +func TestSeedMenusLedgerSurvivesARetry(t *testing.T) { + db := newSeedTestDB(t) + seedAdminRole(t, db) + + apis := []seed.ApiSpec{ + {Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET", Handle: "apis.Order.GetPage-fm"}, + } + for i := 0; i < 2; i++ { + if err := (adminSeeder{}).SeedMenus(db, "order", nil, apis); err != nil { + t.Fatalf("SeedMenus run %d: %v", i+1, err) + } + } + + var n int64 + db.Model(&models.SysAppCasbinGrant{}).Count(&n) + if n != 1 { + t.Errorf("ledger has %d rows after two runs, want 1", n) + } +} + +// The ledger's own guard against a duplicate, which the plain retry above +// never reaches: there the policy still exists, so the insert is skipped +// before the ledger is touched at all. This is the case that does reach it - +// the policy row was removed while its ledger entry stayed, so the seed +// creates the policy again and writes a ledger entry that is already there. +// A plain insert would abort the whole seed on the ledger's unique index. +func TestSeedMenusLedgerToleratesAnEntryWhosePolicyWasRemoved(t *testing.T) { + db := newSeedTestDB(t) + seedAdminRole(t, db) + + apis := []seed.ApiSpec{ + {Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET", Handle: "apis.Order.GetPage-fm"}, + } + if err := (adminSeeder{}).SeedMenus(db, "order", nil, apis); err != nil { + t.Fatalf("first run: %v", err) + } + if err := db.Exec("DELETE FROM casbin_rule WHERE v1 = ? AND v2 = ?", "/api/v1/order", "GET").Error; err != nil { + t.Fatalf("removing the policy: %v", err) + } + var ledger int64 + db.Model(&models.SysAppCasbinGrant{}).Count(&ledger) + if ledger != 1 { + t.Fatalf("the ledger entry is gone, so this test is not set up: %d rows", ledger) + } + + if err := (adminSeeder{}).SeedMenus(db, "order", nil, apis); err != nil { + t.Fatalf("second run: %v", err) + } + + db.Model(&models.SysAppCasbinGrant{}).Count(&ledger) + if ledger != 1 { + t.Errorf("ledger has %d rows, want 1", ledger) + } + var policies int64 + db.Table("casbin_rule").Where("v1 = ? AND v2 = ?", "/api/v1/order", "GET").Count(&policies) + if policies != 1 { + t.Errorf("the policy was not put back: %d rows", policies) + } +} From 74b0ee8776da505d00060fb29b0557d35758b1b4 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Wed, 9 Sep 2026 12:35:17 +0800 Subject: [PATCH 4/7] =?UTF-8?q?fix=F0=9F=90=9B:=20stop=20sys=5Fmenu=20decl?= =?UTF-8?q?aring=20an=20index=20stricter=20than=20the=20real=20one?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit uk_sys_menu_app_seed_code_del covers (app_code, seed_code, deleted_at) and is created by 1786700008000 with explicit SQL. The struct tag named the same index on SeedCode alone, and a named uniqueIndex tag collects only the fields carrying that name - so AutoMigrate on this model would build a unique index on seed_code by itself: stricter than the real one, and forbidding two applications from both having a "dir" node, which the composite key exists to allow. Worse than being stricter, it would win. The migration only creates its index when HasIndex says the name is free, so a schema built by AutoMigrate first keeps the wrong index and the migration steps over it without a word. The tag cannot express the real index: deleted_at comes from the ModelTime embed shared by every table, which no single model can add a tag to. So the tag goes and the migration is the only thing that creates it. No database is affected. The initial table migration AutoMigrates a frozen snapshot of this model that has neither app_code nor seed_code, and nothing else in the repository AutoMigrates the live one - which is why this stayed invisible until a test built the schema from the live model and seeded two applications, and got a unique-constraint failure on a seed code they are supposed to be able to share. --- app/admin/models/sys_menu.go | 13 +++++- app/admin/models/sys_menu_seed_code_test.go | 49 +++++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 app/admin/models/sys_menu_seed_code_test.go diff --git a/app/admin/models/sys_menu.go b/app/admin/models/sys_menu.go index 1fdddc57..51adc8ec 100644 --- a/app/admin/models/sys_menu.go +++ b/app/admin/models/sys_menu.go @@ -44,7 +44,18 @@ type SysMenu struct { // unique index below: the database never treats two NULLs as equal, so // only rows that do carry a real code participate in the uniqueness // check at all. - SeedCode *string `json:"seedCode" gorm:"size:64;uniqueIndex:uk_sys_menu_app_seed_code_del;comment:raw MenuSpec.Code, null for rows not written through SeedMenus"` + // uk_sys_menu_app_seed_code_del is created by the migration, not from + // this tag, and deliberately: it covers (app_code, seed_code, + // deleted_at), and this struct cannot say so. A named uniqueIndex tag + // puts every field carrying that name into one index, and deleted_at + // comes from the shared ModelTime embed, which no single model can add a + // tag to. Naming it here anyway declared a unique index on seed_code + // alone under the same name - stricter than the real one, forbidding two + // applications from both having a "dir" node - and AutoMigrate on this + // model would have created that one first, after which the migration's + // HasIndex guard finds the name taken and leaves the wrong index in + // place. + SeedCode *string `json:"seedCode" gorm:"size:64;comment:raw MenuSpec.Code, null for rows not written through SeedMenus"` models.ControlBy models.ModelTime } diff --git a/app/admin/models/sys_menu_seed_code_test.go b/app/admin/models/sys_menu_seed_code_test.go new file mode 100644 index 00000000..aa8d125e --- /dev/null +++ b/app/admin/models/sys_menu_seed_code_test.go @@ -0,0 +1,49 @@ +package models + +import ( + "testing" + + "github.com/glebarez/sqlite" + "gorm.io/gorm" + "gorm.io/gorm/logger" +) + +func ptr(s string) *string { return &s } + +// uk_sys_menu_app_seed_code_del covers (app_code, seed_code, deleted_at) and +// is created by 1786700008000, not from a struct tag. It cannot come from a +// tag: a named uniqueIndex collects every field carrying that name, and +// deleted_at lives in the shared ModelTime embed that no single model can tag. +// +// Naming it on SeedCode alone anyway produced a unique index on seed_code by +// itself under the same name - stricter than the real one - and AutoMigrate +// here would create that one, after which the migration's HasIndex guard +// finds the name taken and leaves the wrong index in place. Nothing in +// production AutoMigrates this model (the initial table migration uses a +// frozen snapshot that has neither column), which is why this never showed up +// as a broken database; it showed up the first time a test built the schema +// from the live model and seeded two applications. +func TestSysMenuDeclaresNoSeedCodeIndexOfItsOwn(t *testing.T) { + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{ + Logger: logger.Default.LogMode(logger.Silent), + }) + if err != nil { + t.Fatalf("open: %v", err) + } + if err := db.AutoMigrate(&SysMenu{}); err != nil { + t.Fatalf("automigrate: %v", err) + } + if db.Migrator().HasIndex(&SysMenu{}, "uk_sys_menu_app_seed_code_del") { + t.Error("AutoMigrate created uk_sys_menu_app_seed_code_del from a tag; " + + "the migration's HasIndex guard will now skip the composite index it should create") + } + + // Two applications, the same seed code. The real index allows it because + // app_code is part of the key; an index on seed_code alone does not. + for _, app := range []string{"order", "crm"} { + row := SysMenu{MenuName: app + "Dir", AppCode: app, SeedCode: ptr("dir")} + if err := db.Create(&row).Error; err != nil { + t.Fatalf("%s could not use the seed code \"dir\": %v", app, err) + } + } +} From aa539c061ff108af5e78cb7b88bb9dcf8a56af5f Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Wed, 9 Sep 2026 12:35:35 +0800 Subject: [PATCH 5/7] =?UTF-8?q?feat=E2=9C=A8:=20uninstall=20one=20applicat?= =?UTF-8?q?ion's=20menus,=20apis=20and=20grants?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `migrate uninstall ` removes what an application's install wrote and leaves the application's own tables alone. Removing an order module is not the same decision as destroying the orders, and nothing here can tell an operator who is done with it from one who will reinstall tomorrow. One transaction, and this one really is one: every statement is DML or a SELECT, so unlike an install there is no DDL to commit it out from under itself. Child rows go first, while the ids that identify them can still be read from their parents, and the api paths are read before the rows carrying them are deleted. The two join tables need no ledger and get none. menu_id is a surrogate key, so a sys_role_menu or sys_menu_api_rule row can only have come from a menu this application wrote - there is no "looks like it but is not". A column on sys_role_menu would have been worse than unnecessary: SysRole.Update deletes a role's rows and writes them back through GORM's many2many, which does not carry extra columns, so the column would be blanked the first time anybody edited a role, silently. There is a test that performs that edit and then uninstalls. casbin_rule is the opposite case, because its key is business text somebody else may have written for their own reasons. Policies are removed one at a time, by exact tuple, and only the ones the ledger says this install created. A tuple the ledger names that is no longer there is reported, not treated as a failure - the uninstall wanted it gone and it is gone. Then, with everything the ledger could speak for already dealt with, a read-only pass lists the policies still naming this application's paths: those are grants somebody made by hand, they are about to point at APIs that no longer exist, and they are not this command's to delete. The two lists stay separate because they mean different things - one is something of ours that had already gone, the other is somebody else's grant now pointing at nothing - and merged into one "could not remove" list neither would be actionable. sys_migration's rows for the application go too. Without that a reinstall finds every version applied, runs no migration, seeds nothing, and reports success. It is the easiest step to leave out, because a migration record does not look like the application's data. A sys_app row is not required. `migrate` with no subcommand applies every registered migration, an application's included, so an application can have all of its rows without ever having gone through the installer - and that is the case where nothing else can clean up after it. Eleven degradations were applied one at a time, each red on the assertion it was aimed at: skipping either join table, deleting sys_role_menu without its filter, skipping sys_migration, deleting sys_migration without its filter, matching policies by path instead of by ledger tuple, dropping the orphan pass, treating a missing policy as a failure, leaving the ledger behind, soft deleting sys_menu instead of removing it, and running the whole thing outside a transaction. --- cmd/migrate/server.go | 33 +++ cmd/migrate/uninstall.go | 290 +++++++++++++++++++++++++ cmd/migrate/uninstall_test.go | 393 ++++++++++++++++++++++++++++++++++ 3 files changed, 716 insertions(+) create mode 100644 cmd/migrate/uninstall.go create mode 100644 cmd/migrate/uninstall_test.go diff --git a/cmd/migrate/server.go b/cmd/migrate/server.go index 337ecc5d..93222f97 100644 --- a/cmd/migrate/server.go +++ b/cmd/migrate/server.go @@ -64,6 +64,15 @@ var ( runInstall(args[0]) }, } + uninstallCmd = &cobra.Command{ + Use: "uninstall ", + Short: "Remove one application's menus, apis and permission grants; its own tables are left alone", + Example: "go-admin migrate uninstall order -c config/settings.yml", + Args: cobra.ExactArgs(1), + Run: func(cmd *cobra.Command, args []string) { + runUninstall(args[0]) + }, + } ) // fixme 在您看不见代码的时候运行迁移,我觉得是不安全的,所以编译后最好不要去执行迁移 @@ -82,6 +91,7 @@ func init() { StartCmd.AddCommand(statusCmd) StartCmd.AddCommand(installCmd) + StartCmd.AddCommand(uninstallCmd) } func run() { @@ -297,6 +307,29 @@ func runInstall(code string) { ) } +func runUninstall(code string) { + config.Setup( + file.NewSource(file.WithPath(configYml)), + func() { + database.Setup() + db, err := resolveDB() + if err != nil { + exitOnError(os.Stderr, err) + return + } + // No manifest lookup. An application whose code has already been + // taken out of the binary registers nothing, and that is exactly + // when somebody needs to clear its rows out of the database. + rep, err := uninstall(db, code) + if err != nil { + exitOnError(os.Stderr, err) + return + } + reportUninstall(os.Stdout, rep) + }, + ) +} + func genFile() error { t1, err := template.ParseFiles("template/migrate.template") if err != nil { diff --git a/cmd/migrate/uninstall.go b/cmd/migrate/uninstall.go new file mode 100644 index 00000000..c61e5ab0 --- /dev/null +++ b/cmd/migrate/uninstall.go @@ -0,0 +1,290 @@ +package migrate + +import ( + "errors" + "fmt" + "io" + "strings" + + "gorm.io/gorm" + + adminmodels "go-admin/app/admin/models" + "go-admin/cmd/migrate/migration" + commonmodels "go-admin/common/models" +) + +// policyKey is one casbin_rule row identified the way casbin_rule is unique: +// by its tuple, not by its id. Ids do not survive SysRole.Update, which +// removes a role's policies and adds them back. +type policyKey struct { + Ptype string `gorm:"column:ptype"` + V0 string `gorm:"column:v0"` + V1 string `gorm:"column:v1"` + V2 string `gorm:"column:v2"` + V3 string `gorm:"column:v3"` + V4 string `gorm:"column:v4"` + V5 string `gorm:"column:v5"` +} + +func (p policyKey) String() string { + return fmt.Sprintf("%s %s %s %s", p.Ptype, p.V0, p.V1, p.V2) +} + +// uninstallReport is what an uninstall removed, and what it deliberately did +// not. +type uninstallReport struct { + Code string + // Found says whether sys_app had a row. An application whose migrations + // were applied by plain `migrate` rather than by `install` has its menus + // and its permissions without ever having had one. + Found bool + Version string + Menus int64 + Apis int64 + Bindings int64 + RoleMenus int64 + Policies int64 + Migrations int64 + // Skipped are ledger entries whose casbin_rule row was not there any + // more: something this install created and something else removed. + Skipped []policyKey + // Orphans are policies naming this application's paths that no ledger + // entry claims - somebody granted this app's API to another role by + // hand. Reported, never deleted. + Orphans []policyKey +} + +// uninstall removes one application's menus, APIs and permission grants. +// +// It does not touch the application's own tables. Removing an order module +// is not the same decision as destroying the orders, and nothing here can +// tell the operator apart from someone who will reinstall tomorrow. +// +// One transaction, and this one really is one: every statement below is DML +// or a SELECT, so unlike an install there is no DDL to commit it out from +// under itself. Child rows go first, while the ids that identify them can +// still be read from the parents. +// +// A sys_app row is not required. `migrate` with no subcommand applies every +// registered migration, an application's included, so an application can +// have all of its data without ever having gone through the installer. +func uninstall(db *gorm.DB, code string) (uninstallReport, error) { + code = migration.NormalizeAppCode(code) + rep := uninstallReport{Code: code} + if code == "" { + return rep, errors.New("no app code given") + } + if code == migration.FrameworkAppCode { + return rep, fmt.Errorf("%q is the framework's own migrations; there is no uninstall for those", code) + } + + err := db.Transaction(func(tx *gorm.DB) error { + row, found, err := loadApp(tx, code) + if err != nil { + return err + } + rep.Found = found + if found { + rep.Version = row.Version + } + + // 1 and 2. Read before deleting: sys_api's rows are about to go, and + // step 5b needs their paths. + // + // Unscoped throughout. A row this application wrote that somebody + // soft-deleted from the UI is still this application's row, and + // leaving it behind would leave its join rows pointing at it. + var menuIDs []int + if err := tx.Unscoped().Model(&adminmodels.SysMenu{}). + Where("app_code = ?", code).Pluck("menu_id", &menuIDs).Error; err != nil { + return fmt.Errorf("reading this app's menus: %w", err) + } + var apiIDs []int + if err := tx.Unscoped().Model(&adminmodels.SysApi{}). + Where("app_code = ?", code).Pluck("id", &apiIDs).Error; err != nil { + return fmt.Errorf("reading this app's apis: %w", err) + } + var apiKeys []policyKey + if err := tx.Unscoped().Model(&adminmodels.SysApi{}). + Where("app_code = ?", code). + Select("path as v1, action as v2").Scan(&apiKeys).Error; err != nil { + return fmt.Errorf("reading this app's api paths: %w", err) + } + + // 3. The many2many rows behind SysMenu.SysApi. Either side is enough + // to make a row this application's. + if len(menuIDs) > 0 || len(apiIDs) > 0 { + q := tx.Table("sys_menu_api_rule") + switch { + case len(menuIDs) > 0 && len(apiIDs) > 0: + q = q.Where("sys_menu_menu_id IN ? OR sys_api_id IN ?", menuIDs, apiIDs) + case len(menuIDs) > 0: + q = q.Where("sys_menu_menu_id IN ?", menuIDs) + default: + q = q.Where("sys_api_id IN ?", apiIDs) + } + res := q.Delete(nil) + if res.Error != nil { + return fmt.Errorf("removing menu/api bindings: %w", res.Error) + } + rep.Bindings = res.RowsAffected + } + + // 4. Role assignments. menu_id is a surrogate key, so a row here can + // only have come from a menu this application wrote - there is no + // "looks like it but is not". That is why this needs no ledger, and + // why a column on sys_role_menu would have been wrong: SysRole.Update + // deletes a role's rows and writes them back through GORM's + // many2many, which does not carry extra columns, so any such column + // would be silently blanked the first time somebody edits a role. + if len(menuIDs) > 0 { + res := tx.Table("sys_role_menu").Where("menu_id IN ?", menuIDs).Delete(nil) + if res.Error != nil { + return fmt.Errorf("removing role assignments: %w", res.Error) + } + rep.RoleMenus = res.RowsAffected + } + + // 5. Policies, by ledger, one at a time and by exact tuple. + var grants []adminmodels.SysAppCasbinGrant + if err := tx.Where("app_code = ?", code).Find(&grants).Error; err != nil { + return fmt.Errorf("reading the grant ledger: %w", err) + } + for _, g := range grants { + k := policyKey{Ptype: g.Ptype, V0: g.V0, V1: g.V1, V2: g.V2, V3: g.V3, V4: g.V4, V5: g.V5} + res := tx.Table("casbin_rule"). + Where("ptype = ? AND v0 = ? AND v1 = ? AND v2 = ? AND v3 = ? AND v4 = ? AND v5 = ?", + k.Ptype, k.V0, k.V1, k.V2, k.V3, k.V4, k.V5). + Delete(nil) + if res.Error != nil { + return fmt.Errorf("removing policy %s: %w", k, res.Error) + } + if res.RowsAffected == 0 { + // Something this install created is not there any more. Not + // an error: the uninstall's job was to remove it and it is + // gone. Reported because a policy this app created and did + // not remove means something else rewrote casbin_rule. + rep.Skipped = append(rep.Skipped, k) + continue + } + rep.Policies += res.RowsAffected + } + // The ledger's job ends here whether or not each row matched. Left + // behind it would only grow, and a reinstall writes its own entries. + if err := tx.Where("app_code = ?", code). + Delete(&adminmodels.SysAppCasbinGrant{}).Error; err != nil { + return fmt.Errorf("clearing the grant ledger: %w", err) + } + + // 5b. Read-only. By now every policy the ledger could speak for has + // been dealt with, so a policy still matching one of this app's paths + // is one the ledger never claimed - somebody granted this app's API + // to another role by hand. Business rule 3 says do not delete what + // is not ours; without this step nobody would ever learn it is + // there, pointing at an API that is about to stop existing. + orphans, err := findOrphanPolicies(tx, apiKeys) + if err != nil { + return err + } + rep.Orphans = orphans + + // 6 and 7. + res := tx.Unscoped().Where("app_code = ?", code).Delete(&adminmodels.SysApi{}) + if res.Error != nil { + return fmt.Errorf("removing this app's apis: %w", res.Error) + } + rep.Apis = res.RowsAffected + + res = tx.Unscoped().Where("app_code = ?", code).Delete(&adminmodels.SysMenu{}) + if res.Error != nil { + return fmt.Errorf("removing this app's menus: %w", res.Error) + } + rep.Menus = res.RowsAffected + + // 8. Without this a reinstall finds every version already applied, + // runs no migration, seeds nothing, and reports success. It is the + // easiest step to leave out, because a migration record does not + // look like the application's data. + res = tx.Where("app_code = ?", code).Delete(&commonmodels.Migration{}) + if res.Error != nil { + return fmt.Errorf("removing this app's migration records: %w", res.Error) + } + rep.Migrations = res.RowsAffected + + // 9. + if found { + if err := tx.Where("app_code = ?", code). + Delete(&adminmodels.SysApp{}).Error; err != nil { + return fmt.Errorf("removing the sys_app row: %w", err) + } + } + return nil + }) + if err != nil { + return uninstallReport{Code: code}, err + } + return rep, nil +} + +// findOrphanPolicies looks for policies naming any of this application's +// paths. +// +// Written as an OR chain rather than a row-value IN, which MySQL and modern +// SQLite accept and SQL Server does not; this repository supports all of +// them. Chunked because a driver's placeholder limit is reached long before +// an application runs out of endpoints. +func findOrphanPolicies(tx *gorm.DB, keys []policyKey) ([]policyKey, error) { + const chunk = 100 + var out []policyKey + for start := 0; start < len(keys); start += chunk { + end := start + chunk + if end > len(keys) { + end = len(keys) + } + clauses := make([]string, 0, end-start) + args := make([]any, 0, (end-start)*2) + for _, k := range keys[start:end] { + clauses = append(clauses, "(v1 = ? AND v2 = ?)") + args = append(args, k.V1, k.V2) + } + var found []policyKey + if err := tx.Table("casbin_rule"). + Where("ptype = ? AND ("+strings.Join(clauses, " OR ")+")", append([]any{"p"}, args...)...). + Scan(&found).Error; err != nil { + return nil, fmt.Errorf("looking for policies nothing claims: %w", err) + } + out = append(out, found...) + } + return out, nil +} + +// reportUninstall prints what went and what stayed. +// +// The two lists are separate because they mean different things: one is +// something this application created that had already gone, the other is +// somebody else's grant that is now pointing at nothing. Merged into one +// "could not remove" list, neither would be actionable. +func reportUninstall(w io.Writer, rep uninstallReport) { + if !rep.Found { + fmt.Fprintf(w, "%s had no sys_app row; removed what its migrations had written\n", rep.Code) + } else { + fmt.Fprintf(w, "uninstalled %s %s\n", rep.Code, rep.Version) + } + fmt.Fprintf(w, "removed: %d menu(s), %d api(s), %d binding(s), %d role assignment(s), %d policy(ies), %d migration record(s)\n", + rep.Menus, rep.Apis, rep.Bindings, rep.RoleMenus, rep.Policies, rep.Migrations) + fmt.Fprintln(w, "the application's own tables were not touched.") + + if len(rep.Skipped) > 0 { + fmt.Fprintf(w, "\n%d policy(ies) this install had created were already gone:\n", len(rep.Skipped)) + for _, k := range rep.Skipped { + fmt.Fprintf(w, " %s\n", k) + } + } + if len(rep.Orphans) > 0 { + fmt.Fprintf(w, "\n%d policy(ies) name this application's paths and were granted by somebody else, so they were left alone:\n", len(rep.Orphans)) + for _, k := range rep.Orphans { + fmt.Fprintf(w, " %s\n", k) + } + fmt.Fprintln(w, "they now point at APIs that no longer exist. Harmless to the running server, and yours to clear up.") + } +} diff --git a/cmd/migrate/uninstall_test.go b/cmd/migrate/uninstall_test.go new file mode 100644 index 00000000..127f63da --- /dev/null +++ b/cmd/migrate/uninstall_test.go @@ -0,0 +1,393 @@ +package migrate + +import ( + "strings" + "testing" + "time" + + "github.com/glebarez/sqlite" + "github.com/go-admin-team/go-admin-core/v2/sdk/contract/seed" + "gorm.io/gorm" + "gorm.io/gorm/logger" + + adminmodels "go-admin/app/admin/models" + _ "go-admin/app/admin/service" // registers the seeder SeedMenus dispatches to + "go-admin/cmd/migrate/migration" + commonmodels "go-admin/common/models" +) + +const adminRoleKey = "admin" + +// newUninstallDB builds every table an install writes to, plus one table +// standing in for the application's own data, which an uninstall must not +// touch. +func newUninstallDB(t *testing.T) *gorm.DB { + t.Helper() + db, err := gorm.Open(sqlite.Open("file:"+t.Name()+"?mode=memory&cache=shared"), &gorm.Config{ + Logger: logger.Default.LogMode(logger.Silent), + }) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + if err := db.AutoMigrate( + &adminmodels.SysMenu{}, &adminmodels.SysApi{}, &adminmodels.SysRole{}, + &adminmodels.SysApp{}, &adminmodels.SysAppCasbinGrant{}, &commonmodels.Migration{}, + ); err != nil { + t.Fatalf("automigrate: %v", err) + } + // casbin_rule has no GORM model in this repository; the columns are the + // ones grantToAdminRole's INSERT addresses. + if err := db.Exec(`CREATE TABLE casbin_rule ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + ptype TEXT, v0 TEXT, v1 TEXT, v2 TEXT, v3 TEXT, v4 TEXT, v5 TEXT + )`).Error; err != nil { + t.Fatalf("create casbin_rule: %v", err) + } + if err := db.Exec(`CREATE TABLE app_order (id INTEGER PRIMARY KEY, note TEXT)`).Error; err != nil { + t.Fatalf("create app_order: %v", err) + } + if err := db.Exec(`INSERT INTO app_order (id, note) VALUES (1, 'a real order')`).Error; err != nil { + t.Fatalf("seed app_order: %v", err) + } + if err := db.Create(&adminmodels.SysRole{RoleName: "Administrator", RoleKey: adminRoleKey}).Error; err != nil { + t.Fatalf("seed admin role: %v", err) + } + return db +} + +// specsFor builds one application's menus and apis. The paths carry the app +// code, because two applications do not share an endpoint - and if a fixture +// let them, the second one's policies would already exist and its ledger +// would legitimately come out empty, which would make it a useless control. +func specsFor(code string) ([]seed.MenuSpec, []seed.ApiSpec) { + menus := []seed.MenuSpec{ + {Code: "dir", Kind: "M", Title: code + " example", Path: "/apps/" + code, Component: "Layout", Sort: 10}, + {Code: "list", Parent: "dir", Kind: "C", Title: code, Path: "list", Component: "apps/" + code + "/index", Sort: 1, ApiCodes: []string{"list"}}, + } + apis := []seed.ApiSpec{ + {Code: "list", Title: code + " list", Path: "/api/v1/" + code, Method: "GET", Handle: "apis." + code + ".GetPage-fm"}, + {Code: "create", Title: "create " + code, Path: "/api/v1/" + code, Method: "POST", Handle: "apis." + code + ".Insert-fm"}, + } + return menus, apis +} + +// seedApp runs the real seeding path, so what the uninstaller has to undo is +// what an install actually writes rather than a hand-built approximation. +func seedApp(t *testing.T, db *gorm.DB, code string) { + t.Helper() + menus, apis := specsFor(code) + if err := db.Transaction(func(tx *gorm.DB) error { + return seed.SeedMenus(tx, code, menus, apis) + }); err != nil { + t.Fatalf("seeding %q: %v", code, err) + } + if err := db.Create(&commonmodels.Migration{ + Version: code + "-1786800001000", AppCode: code, ApplyTime: time.Now(), + }).Error; err != nil { + t.Fatalf("recording the migration for %q: %v", code, err) + } + if err := db.Create(&adminmodels.SysApp{ + AppCode: code, Name: code, Version: "1.0.0", Status: adminmodels.AppInstalled, + }).Error; err != nil { + t.Fatalf("recording sys_app for %q: %v", code, err) + } +} + +func count(t *testing.T, db *gorm.DB, table, where string, args ...any) int64 { + t.Helper() + var n int64 + q := db.Table(table) + if where != "" { + q = q.Where(where, args...) + } + if err := q.Count(&n).Error; err != nil { + t.Fatalf("counting %s: %v", table, err) + } + return n +} + +// A3: everything the install wrote goes, and the application's own table does +// not. +func TestUninstallRemovesWhatWasSeededAndNothingElse(t *testing.T) { + db := newUninstallDB(t) + seedApp(t, db, "order") + + if count(t, db, "sys_menu", "app_code = ?", "order") == 0 { + t.Fatal("nothing was seeded, so this test proves nothing") + } + + rep, err := uninstall(db, "order") + if err != nil { + t.Fatalf("uninstall: %v", err) + } + if !rep.Found { + t.Error("the sys_app row was not found") + } + + for _, c := range []struct { + table, where string + args []any + }{ + {"sys_menu", "app_code = ?", []any{"order"}}, + {"sys_api", "app_code = ?", []any{"order"}}, + {"sys_menu_api_rule", "", nil}, + {"sys_role_menu", "", nil}, + {"casbin_rule", "v1 = ?", []any{"/api/v1/order"}}, + {"sys_app_casbin_grant", "app_code = ?", []any{"order"}}, + {"sys_migration", "app_code = ?", []any{"order"}}, + {"sys_app", "app_code = ?", []any{"order"}}, + } { + if n := count(t, db, c.table, c.where, c.args...); n != 0 { + t.Errorf("%s still has %d row(s)", c.table, n) + } + } + if n := count(t, db, "app_order", "", nil); n != 1 { + t.Errorf("app_order has %d row(s); the application's own data is not the uninstaller's to remove", n) + } + if len(rep.Skipped) != 0 || len(rep.Orphans) != 0 { + t.Errorf("a clean uninstall reported skipped=%v orphans=%v", rep.Skipped, rep.Orphans) + } + if rep.Menus == 0 || rep.Apis == 0 || rep.Policies == 0 || rep.Migrations == 0 { + t.Errorf("the report says nothing was removed: %+v", rep) + } +} + +// Uninstalling one application must not reach into another's rows. Every +// delete here is filtered, and a missing filter is invisible on a database +// with only one application in it. +func TestUninstallLeavesAnotherApplicationAlone(t *testing.T) { + db := newUninstallDB(t) + seedApp(t, db, "order") + seedApp(t, db, "crm") + + before := map[string]int64{ + "sys_menu": count(t, db, "sys_menu", "app_code = ?", "crm"), + "sys_api": count(t, db, "sys_api", "app_code = ?", "crm"), + "sys_app_casbin_grant": count(t, db, "sys_app_casbin_grant", "app_code = ?", "crm"), + "sys_migration": count(t, db, "sys_migration", "app_code = ?", "crm"), + "sys_app": count(t, db, "sys_app", "app_code = ?", "crm"), + } + for k, v := range before { + if v == 0 { + t.Fatalf("crm has no rows in %s, so this test proves nothing", k) + } + } + crmBindings := count(t, db, "sys_menu_api_rule", "", nil) + crmRoleMenus := count(t, db, "sys_role_menu", "", nil) + + if _, err := uninstall(db, "order"); err != nil { + t.Fatalf("uninstall: %v", err) + } + + for k, v := range before { + if n := count(t, db, k, "app_code = ?", "crm"); n != v { + t.Errorf("%s for crm went from %d to %d", k, v, n) + } + } + // crm's own bindings and role rows are half of each total, and must be + // exactly what is left. + if n := count(t, db, "sys_menu_api_rule", "", nil); n != crmBindings/2 { + t.Errorf("sys_menu_api_rule = %d, want %d (crm's half)", n, crmBindings/2) + } + if n := count(t, db, "sys_role_menu", "", nil); n != crmRoleMenus/2 { + t.Errorf("sys_role_menu = %d, want %d (crm's half)", n, crmRoleMenus/2) + } + // crm's policies name a different path, so they are untouched. + if n := count(t, db, "casbin_rule", "v1 = ?", "/api/v1/order"); n != 0 { + t.Errorf("order's policies survived: %d", n) + } +} + +// A6b: somebody granted this application's API to another role by hand. That +// grant is not in the ledger, is not this uninstall's to remove, and would +// otherwise vanish from view entirely. +func TestUninstallReportsAGrantSomebodyElseMade(t *testing.T) { + db := newUninstallDB(t) + seedApp(t, db, "order") + + if err := db.Exec( + "INSERT INTO casbin_rule (ptype, v0, v1, v2, v3, v4, v5) VALUES ('p', 'ops', '/api/v1/order', 'GET', '', '', '')", + ).Error; err != nil { + t.Fatalf("hand-made grant: %v", err) + } + + rep, err := uninstall(db, "order") + if err != nil { + t.Fatalf("uninstall: %v", err) + } + if n := count(t, db, "casbin_rule", "v0 = ?", "ops"); n != 1 { + t.Errorf("somebody else's grant was deleted (%d rows left)", n) + } + if len(rep.Orphans) != 1 { + t.Fatalf("orphans = %v, want the one hand-made grant", rep.Orphans) + } + if rep.Orphans[0].V0 != "ops" { + t.Errorf("orphan = %+v", rep.Orphans[0]) + } + // The admin grants it did own are gone. + if n := count(t, db, "casbin_rule", "v0 = ?", adminRoleKey); n != 0 { + t.Errorf("%d of this app's own policies survived", n) + } + + var out strings.Builder + reportUninstall(&out, rep) + if !strings.Contains(out.String(), "ops") || !strings.Contains(out.String(), "left alone") { + t.Errorf("the report does not say what was left behind: %q", out.String()) + } +} + +// A6a: a policy this install created is not there any more. Not an error - +// the uninstall wanted it gone and it is - but reported, because something +// else rewrote casbin_rule. +func TestUninstallReportsALedgerEntryWhosePolicyIsGone(t *testing.T) { + db := newUninstallDB(t) + seedApp(t, db, "order") + + if err := db.Exec("DELETE FROM casbin_rule WHERE v2 = 'POST'").Error; err != nil { + t.Fatalf("removing a policy: %v", err) + } + + rep, err := uninstall(db, "order") + if err != nil { + t.Fatalf("a missing policy made the uninstall fail: %v", err) + } + if len(rep.Skipped) != 1 { + t.Fatalf("skipped = %v, want the one that had gone", rep.Skipped) + } + if rep.Skipped[0].V2 != "POST" { + t.Errorf("skipped = %+v", rep.Skipped[0]) + } + if n := count(t, db, "sys_app_casbin_grant", "", nil); n != 0 { + t.Errorf("the ledger kept %d row(s); its job ends with the uninstall", n) + } + // It still committed: a skip is a reported branch, not a failure. + if n := count(t, db, "sys_menu", "app_code = ?", "order"); n != 0 { + t.Errorf("the transaction rolled back over a skip: sys_menu has %d row(s)", n) + } +} + +// G5/A4: without this the reinstall finds every version applied, runs no +// migration, seeds nothing, and reports success. +func TestUninstallClearsThisAppsMigrationRecordsOnly(t *testing.T) { + db := newUninstallDB(t) + seedApp(t, db, "order") + if err := db.Create(&commonmodels.Migration{ + Version: "1786700001000", AppCode: "", ApplyTime: time.Now(), + }).Error; err != nil { + t.Fatalf("framework migration row: %v", err) + } + + if _, err := uninstall(db, "order"); err != nil { + t.Fatalf("uninstall: %v", err) + } + if n := count(t, db, "sys_migration", "app_code = ?", "order"); n != 0 { + t.Errorf("sys_migration still has %d row(s) for order; a reinstall would seed nothing", n) + } + if n := count(t, db, "sys_migration", "app_code = ?", ""); n != 1 { + t.Errorf("the framework's own migration record was removed (%d left)", n) + } +} + +// A11: sys_role_menu is found by menu id, not by a column on it. A column +// would have been blanked the first time somebody edited a role, because +// SysRole.Update deletes the role's rows and writes them back through GORM's +// many2many, which does not carry extra columns. This reproduces that edit. +func TestUninstallSurvivesARoleMenuRewrite(t *testing.T) { + db := newUninstallDB(t) + seedApp(t, db, "order") + + var roleID int + if err := db.Model(&adminmodels.SysRole{}).Where("role_key = ?", adminRoleKey). + Pluck("role_id", &roleID).Error; err != nil { + t.Fatalf("reading the admin role: %v", err) + } + var menuIDs []int + if err := db.Model(&adminmodels.SysMenu{}).Where("app_code = ?", "order"). + Pluck("menu_id", &menuIDs).Error; err != nil { + t.Fatalf("reading menus: %v", err) + } + if len(menuIDs) == 0 { + t.Fatal("no menus were seeded") + } + // What SysRole.Update does: drop every row for the role, then write them + // back with nothing but the two keys. + if err := db.Exec("DELETE FROM sys_role_menu WHERE role_id = ?", roleID).Error; err != nil { + t.Fatalf("clearing role menus: %v", err) + } + for _, id := range menuIDs { + if err := db.Exec("INSERT INTO sys_role_menu (role_id, menu_id) VALUES (?, ?)", roleID, id).Error; err != nil { + t.Fatalf("rewriting role menus: %v", err) + } + } + + rep, err := uninstall(db, "order") + if err != nil { + t.Fatalf("uninstall: %v", err) + } + if rep.RoleMenus != int64(len(menuIDs)) { + t.Errorf("removed %d role assignment(s), want %d", rep.RoleMenus, len(menuIDs)) + } + if n := count(t, db, "sys_role_menu", "", nil); n != 0 { + t.Errorf("sys_role_menu still has %d row(s) after a role edit", n) + } +} + +// `migrate` with no subcommand applies every registered migration, an +// application's included, so an application can have all of its rows and +// never have had a sys_app row. Refusing to clean that up would leave the +// only case where nothing else can. +func TestUninstallWorksWithoutASysAppRow(t *testing.T) { + db := newUninstallDB(t) + seedApp(t, db, "order") + if err := db.Where("app_code = ?", "order").Delete(&adminmodels.SysApp{}).Error; err != nil { + t.Fatalf("removing the sys_app row: %v", err) + } + + rep, err := uninstall(db, "order") + if err != nil { + t.Fatalf("uninstall: %v", err) + } + if rep.Found { + t.Error("the report claims a sys_app row that was not there") + } + if n := count(t, db, "sys_menu", "app_code = ?", "order"); n != 0 { + t.Errorf("sys_menu still has %d row(s)", n) + } + var out strings.Builder + reportUninstall(&out, rep) + if !strings.Contains(out.String(), "no sys_app row") { + t.Errorf("the report does not say the row was missing: %q", out.String()) + } +} + +// One transaction, and it really is one: nothing here runs DDL, so unlike an +// install there is nothing to commit it out from under itself. +func TestUninstallRollsBackAsAWhole(t *testing.T) { + db := newUninstallDB(t) + seedApp(t, db, "order") + menusBefore := count(t, db, "sys_menu", "app_code = ?", "order") + policiesBefore := count(t, db, "casbin_rule", "", nil) + + // Step 8's table is gone, so the uninstall fails after it has already + // deleted menus, apis, bindings and policies. + if err := db.Migrator().DropTable(&commonmodels.Migration{}); err != nil { + t.Fatalf("dropping sys_migration: %v", err) + } + + if _, err := uninstall(db, "order"); err == nil { + t.Fatal("the uninstall reported success with sys_migration missing") + } + if n := count(t, db, "sys_menu", "app_code = ?", "order"); n != menusBefore { + t.Errorf("sys_menu = %d, want %d: the failed uninstall did not roll back", n, menusBefore) + } + if n := count(t, db, "casbin_rule", "", nil); n != policiesBefore { + t.Errorf("casbin_rule = %d, want %d: the failed uninstall did not roll back", n, policiesBefore) + } +} + +func TestUninstallRefusesTheFrameworkCode(t *testing.T) { + db := newUninstallDB(t) + if _, err := uninstall(db, migration.FrameworkAppCode); err == nil { + t.Fatal("the framework was uninstalled") + } +} From 006756ea40224772e48cc1fc7e3ef35776cfc422 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Wed, 9 Sep 2026 12:56:33 +0800 Subject: [PATCH 6/7] =?UTF-8?q?test=E2=9C=85:=20cover=20the=20uninstall's?= =?UTF-8?q?=20chunk=20boundary=20and=20its=20empty=20id=20lists?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot could not review this branch - the account is over its review quota - so these are what a second pass over the uninstaller turned up. No defect: the three cases were uncovered rather than wrong. findOrphanPolicies batches its OR chain because a driver runs out of placeholders long before an application runs out of endpoints, and nothing exercised the boundary. 205 paths across three batches, the last one short, plus one policy no key names as a control. Taking one fewer per batch, advancing one too far, and stopping after the first batch each turn it red. An application with apis and no menus, and one with menus and no apis, are both normal - endpoints another service calls, or a section with no endpoints of its own - and each leaves one of the two id lists the uninstall reads empty. That last pair also corrected a comment. The guard in front of the join-table delete was described as being there because an empty IN list is a syntax error. It is in raw SQL, but GORM renders IN with an empty slice as a condition that matches nothing, and removing the guard leaves the new test green. It stays as a statement of intent, and now says so. --- cmd/migrate/uninstall.go | 6 ++ cmd/migrate/uninstall_test.go | 111 ++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+) diff --git a/cmd/migrate/uninstall.go b/cmd/migrate/uninstall.go index c61e5ab0..6d0a5710 100644 --- a/cmd/migrate/uninstall.go +++ b/cmd/migrate/uninstall.go @@ -113,6 +113,12 @@ func uninstall(db *gorm.DB, code string) (uninstallReport, error) { // 3. The many2many rows behind SysMenu.SysApi. Either side is enough // to make a row this application's. + // + // The guard is intent, not necessity: GORM renders IN with an empty + // slice as a condition matching nothing rather than the empty IN + // list raw SQL would reject, so removing it changes no behaviour + // today. It says out loud that an application with no menus, or no + // apis, is a normal thing to uninstall. if len(menuIDs) > 0 || len(apiIDs) > 0 { q := tx.Table("sys_menu_api_rule") switch { diff --git a/cmd/migrate/uninstall_test.go b/cmd/migrate/uninstall_test.go index 127f63da..2ae79df4 100644 --- a/cmd/migrate/uninstall_test.go +++ b/cmd/migrate/uninstall_test.go @@ -1,6 +1,7 @@ package migrate import ( + "strconv" "strings" "testing" "time" @@ -391,3 +392,113 @@ func TestUninstallRefusesTheFrameworkCode(t *testing.T) { t.Fatal("the framework was uninstalled") } } + +// findOrphanPolicies chunks its OR chain because a driver runs out of +// placeholders long before an application runs out of endpoints. The +// boundary is where an off-by-one hides: a chunk size that drops the last +// element of each batch, or one that never advances, both leave policies +// unreported and nothing says so. +func TestFindOrphanPoliciesCoversEveryPathAcrossChunks(t *testing.T) { + db := newUninstallDB(t) + + // Deliberately not a multiple of the chunk size, so the last batch is + // short, and large enough to need three of them. + const n = 205 + keys := make([]policyKey, 0, n) + for i := 0; i < n; i++ { + path := "/api/v1/thing" + strconv.Itoa(i) + keys = append(keys, policyKey{V1: path, V2: "GET"}) + if err := db.Exec( + "INSERT INTO casbin_rule (ptype, v0, v1, v2, v3, v4, v5) VALUES ('p', 'ops', ?, 'GET', '', '', '')", + path, + ).Error; err != nil { + t.Fatalf("seeding policy %d: %v", i, err) + } + } + // One policy that must not match: a path no key names. + if err := db.Exec( + "INSERT INTO casbin_rule (ptype, v0, v1, v2, v3, v4, v5) VALUES ('p', 'ops', '/api/v1/elsewhere', 'GET', '', '', '')", + ).Error; err != nil { + t.Fatalf("seeding the control policy: %v", err) + } + + found, err := findOrphanPolicies(db, keys) + if err != nil { + t.Fatalf("findOrphanPolicies: %v", err) + } + if len(found) != n { + t.Fatalf("found %d policies, want %d", len(found), n) + } + seen := make(map[string]bool, len(found)) + for _, f := range found { + seen[f.V1] = true + if f.V1 == "/api/v1/elsewhere" { + t.Error("a path no key names was reported") + } + } + for _, k := range keys { + if !seen[k.V1] { + t.Errorf("%s was not reported", k.V1) + } + } +} + +// An application may register apis with no menus at all - endpoints another +// service calls - so either of the id lists an uninstall reads can be empty. +// The guard in front of the join-table delete turns out not to be what makes +// this work: GORM renders IN with an empty slice as a condition that matches +// nothing, rather than the empty IN list that would be a syntax error in raw +// SQL, and removing the guard leaves this test green. It stays as an explicit +// statement of intent rather than a reliance on that rendering. +func TestUninstallWithApisButNoMenus(t *testing.T) { + db := newUninstallDB(t) + apis := []seed.ApiSpec{ + {Code: "hook", Title: "Inbound hook", Path: "/api/v1/hook", Method: "POST", Handle: "hook.Receive"}, + } + if err := db.Transaction(func(tx *gorm.DB) error { + return seed.SeedMenus(tx, "hooks", nil, apis) + }); err != nil { + t.Fatalf("seeding: %v", err) + } + + rep, err := uninstall(db, "hooks") + if err != nil { + t.Fatalf("uninstall: %v", err) + } + if rep.Apis != 1 { + t.Errorf("removed %d api(s), want 1", rep.Apis) + } + if rep.Policies != 1 { + t.Errorf("removed %d policy(ies), want 1", rep.Policies) + } + if n := count(t, db, "casbin_rule", "", nil); n != 0 { + t.Errorf("casbin_rule has %d row(s)", n) + } +} + +// The mirror case: menus and no apis at all. +func TestUninstallWithMenusButNoApis(t *testing.T) { + db := newUninstallDB(t) + menus := []seed.MenuSpec{ + {Code: "dir", Kind: "M", Title: "Reports", Path: "/apps/reports", Component: "Layout", Sort: 10}, + } + if err := db.Transaction(func(tx *gorm.DB) error { + return seed.SeedMenus(tx, "reports", menus, nil) + }); err != nil { + t.Fatalf("seeding: %v", err) + } + + rep, err := uninstall(db, "reports") + if err != nil { + t.Fatalf("uninstall: %v", err) + } + if rep.Menus != 1 { + t.Errorf("removed %d menu(s), want 1", rep.Menus) + } + if n := count(t, db, "sys_menu", "app_code = ?", "reports"); n != 0 { + t.Errorf("sys_menu has %d row(s)", n) + } + if len(rep.Orphans) != 0 { + t.Errorf("an application with no apis reported orphans: %v", rep.Orphans) + } +} From b228152308d5688ca3a22c46693420a271ea227c Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Wed, 9 Sep 2026 13:28:45 +0800 Subject: [PATCH 7/7] =?UTF-8?q?docs=F0=9F=93=9D:=20drop=20a=20reference=20?= =?UTF-8?q?to=20a=20document=20this=20repository=20does=20not=20carry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two comments added in this branch cite docs-prd/008-.../数据库变更.md by path. That directory is not tracked here, so the citation reads as a file the reader can open and cannot. The reasoning it pointed at is short enough to state in place. Three comments from the previous batch cite the same path and are left alone; they belong to a different change. --- app/admin/models/sys_app.go | 5 +++-- cmd/migrate/install.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/admin/models/sys_app.go b/app/admin/models/sys_app.go index 885d5215..4e1c5aec 100644 --- a/app/admin/models/sys_app.go +++ b/app/admin/models/sys_app.go @@ -11,8 +11,9 @@ import ( // Three states rather than a single "installed", because an install that // stopped partway has to be an observable row rather than the absence of one: // the versions an app installs are separate migration files, and on MySQL a -// DDL statement commits the transaction around it, so they cannot be wrapped -// in one. See docs-prd/008-应用清单与安装器/数据库变更.md §1.5. +// DDL statement commits the transaction around it - taking an outer +// transaction and every savepoint under it with it - so they cannot be +// wrapped in one. // // AppInstalling is also what a row reads as after the process was killed // mid-install, which is why it is not treated as "installed" by anything. diff --git a/cmd/migrate/install.go b/cmd/migrate/install.go index d8aeb25d..5eeadd00 100644 --- a/cmd/migrate/install.go +++ b/cmd/migrate/install.go @@ -49,7 +49,7 @@ type installReport struct { // cannot be: an application's versions are separate migration files, and a // DDL statement inside any of them commits the transaction around it on // MySQL, which destroys an outer transaction and every savepoint taken from -// it (docs-prd/008-应用清单与安装器/数据库变更.md §1.5). So this does not +// it. So this does not // promise that a half-installed application cannot happen. It promises that // one is visible when it does: phase A writes "installing" before anything // that can fail, and phase C turns that into "installed" or "failed".