From 1b9868b72bf737a2f0a9fd9893a5fdba79dd61c0 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Thu, 10 Sep 2026 21:30:29 +0800 Subject: [PATCH] =?UTF-8?q?feat=E2=9C=A8:=20refuse=20an=20install=20whose?= =?UTF-8?q?=20dependencies=20are=20not=20installed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An application's manifest can name others it needs. Until now the list was stored and never read. It is checked, not satisfied. Installing the dependencies too would make "install this application" mean "and everything it happens to name, and everything those name" - a blast radius the operator did not ask for and cannot see beforehand. What they get is the list and the order to do it in. A dependency whose own install failed, or never finished, is not a dependency that is there. The message says which, because the two send you to different places: one to install it, the other to look at why it did not take. The check runs before anything is written, so a refusal cannot cost the operator the row that told them what they had. Separately, a cycle anywhere in the registered manifests is refused, whether or not the application being installed is in it. A cycle between two others is still an authoring mistake, and the day somebody installs into it - with an error naming two applications they did not ask for - is the worse time to find out. The error is the cycle rather than the walk that reached it, and the walk's order is sorted, so the same set of manifests always reports the same one. Requires naming an application that is not registered is not a cycle; it is the database's answer to give, at the time it matters. Six degradations turn the new assertions red: accepting any dependency, accepting a row regardless of its status, returning no cycle, not trimming the reported path to the cycle itself, and running the check after the row has already been written - the last of which was rebuilt after the first attempt at it deleted the check rather than moving it, and so went red on the wrong assertion. --- cmd/migrate/install.go | 125 +++++++++++++++++++++++++ cmd/migrate/install_test.go | 176 ++++++++++++++++++++++++++++++++++-- cmd/migrate/server.go | 9 ++ 3 files changed, 304 insertions(+), 6 deletions(-) diff --git a/cmd/migrate/install.go b/cmd/migrate/install.go index 0ce8e2e6..87279fb0 100644 --- a/cmd/migrate/install.go +++ b/cmd/migrate/install.go @@ -99,6 +99,10 @@ func install(db *gorm.DB, eng engine, m app.Manifest) (installReport, error) { sameVersion = cmp == 0 } + if err := requiresInstalled(db, m); err != nil { + return rep, err + } + pending, err := pendingFor(eng, code) if err != nil { return rep, err @@ -156,6 +160,127 @@ func loadApp(db *gorm.DB, code string) (adminmodels.SysApp, bool, error) { return adminmodels.SysApp{}, false, fmt.Errorf("reading sys_app for %q: %w", code, err) } +// requiresInstalled refuses an install whose declared dependencies are not +// installed, and names the ones that are not. +// +// It does not install them. "Install this application" would otherwise mean +// "and everything it happens to name, and everything those name" - a blast +// radius the operator did not ask for and cannot see before it happens. What +// they get instead is a list and the order to do it in. +// +// An unfinished or failed dependency counts as missing, and says which it is: +// "not installed" sends someone to install it, "did not finish" sends them to +// look at why. +func requiresInstalled(db *gorm.DB, m app.Manifest) error { + if len(m.Requires) == 0 { + return nil + } + apps, err := loadApps(db) + if err != nil { + return err + } + var ( + why []string + what []string + ) + for _, req := range m.Requires { + want := migration.NormalizeAppCode(req) + if want == "" { + continue + } + row, ok := apps[want] + switch { + case !ok: + why, what = append(why, want+" (not installed)"), append(what, want) + case row.Status == adminmodels.AppFailed: + why, what = append(why, want+" (its install failed)"), append(what, want) + case row.Status != adminmodels.AppInstalled: + why, what = append(why, want+" (its install did not finish)"), append(what, want) + } + } + if len(why) > 0 { + return fmt.Errorf("%s requires %s; install %s first", + migration.NormalizeAppCode(m.Code), strings.Join(why, ", "), strings.Join(what, " and ")) + } + return nil +} + +// refuseOnDependencyCycle reports a cycle anywhere in the registered +// manifests, whether or not the application being installed is part of it. +// +// Over the whole set rather than one application's closure, because a cycle +// between two applications neither of which is the one being installed is +// still an authoring mistake, and finding it the day somebody happens to +// install into it - with an error naming two applications they did not ask +// for - is the worse time to find it. +// +// Requires naming an application that is not registered is not a cycle and +// not reported here; that is requiresInstalled's answer to give, against the +// database, at the time it matters. +func refuseOnDependencyCycle(manifests map[string]app.Manifest) error { + const ( + white = 0 // not visited + grey = 1 // on the current path + black = 2 // finished + ) + colour := make(map[string]int, len(manifests)) + + codes := make([]string, 0, len(manifests)) + for code := range manifests { + codes = append(codes, code) + } + // Sorted, so the same set of manifests always reports the same cycle + // rather than whichever one the map happened to hand over first. + sort.Strings(codes) + + var path []string + var walk func(code string) error + walk = func(code string) error { + switch colour[code] { + case grey: + // Trim the path to where this code first appears, so the error + // is the cycle and not the walk that reached it. + for i, c := range path { + if c == code { + return fmt.Errorf("the declared dependencies form a cycle: %s", + strings.Join(append(append([]string{}, path[i:]...), code), " -> ")) + } + } + return fmt.Errorf("the declared dependencies form a cycle at %s", code) + case black: + return nil + } + colour[code] = grey + path = append(path, code) + m := manifests[code] + reqs := make([]string, 0, len(m.Requires)) + for _, r := range m.Requires { + if n := migration.NormalizeAppCode(r); n != "" { + reqs = append(reqs, n) + } + } + sort.Strings(reqs) + for _, r := range reqs { + if _, registered := manifests[r]; !registered { + continue + } + if err := walk(r); err != nil { + return err + } + } + path = path[:len(path)-1] + colour[code] = black + return nil + } + + for _, code := range codes { + if err := walk(code); err != nil { + return err + } + } + return nil +} + // loadApps reads every sys_app row, keyed by app code. // // A database that has never had 1786700007000 applied has no such table, and diff --git a/cmd/migrate/install_test.go b/cmd/migrate/install_test.go index af7c64d8..12a6fd0f 100644 --- a/cmd/migrate/install_test.go +++ b/cmd/migrate/install_test.go @@ -66,9 +66,22 @@ func orderManifest(version string) app.Manifest { Version: version, Description: "order management", Author: "go-admin", - Requires: []string{"crm"}, - Pricing: "free", - License: "MIT", + // No dependency by default: these tests are about installing, and a + // declared requirement would make every one of them set up a second + // application first. requiresInstalled has its own tests below. + Requires: nil, + Pricing: "free", + License: "MIT", + } +} + +// installedApp writes the sys_app row a satisfied dependency looks like. +func installedApp(t *testing.T, db *gorm.DB, code string) { + t.Helper() + if err := db.Create(&adminmodels.SysApp{ + AppCode: code, Name: code, Version: "1.0.0", Status: adminmodels.AppInstalled, + }).Error; err != nil { + t.Fatalf("seeding %q as installed: %v", code, err) } } @@ -119,9 +132,6 @@ func TestInstallRecordsAFirstInstall(t *testing.T) { 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) } @@ -466,3 +476,157 @@ func TestInstallNormalizesTheAppCode(t *testing.T) { t.Errorf("applied = %v; the normalized code has to match what Status reports", rep.Applied) } } + +// The manifest's dependency list is stored as it was declared, in the CSV +// shape sys_app.requires carries. +func TestInstallStoresTheDeclaredRequires(t *testing.T) { + db := newInstallDB(t) + installedApp(t, db, "crm") + installedApp(t, db, "billing") + eng := &fakeEngine{entries: []migration.StatusEntry{ + {Version: "order-1786800001000", AppCode: "order", Registered: true}, + }} + m := orderManifest("1.0.0") + m.Requires = []string{"crm", "billing"} + + if _, err := install(db, eng, m); err != nil { + t.Fatalf("install: %v", err) + } + if row := loadRow(t, db, "order"); row.Requires != "crm,billing" { + t.Errorf("requires = %q, want the manifest's list as CSV", row.Requires) + } +} + +// An application is not installed for you because something else names it. +// "Install this" would otherwise mean "and everything it happens to name, and +// everything those name". +func TestInstallRefusesWhenADependencyIsNotInstalled(t *testing.T) { + db := newInstallDB(t) + eng := &fakeEngine{entries: []migration.StatusEntry{ + {Version: "order-1786800001000", AppCode: "order", Registered: true}, + }} + m := orderManifest("1.0.0") + m.Requires = []string{"crm"} + + _, err := install(db, eng, m) + if err == nil { + t.Fatal("an application with an uninstalled dependency was installed") + } + if !strings.Contains(err.Error(), "crm") || !strings.Contains(err.Error(), "not installed") { + t.Errorf("error = %q, it has to name what is missing and why", err) + } + if len(eng.calls) != 0 { + t.Errorf("the engine ran anyway: %v", eng.calls) + } + // Refused before phase A, so a refusal leaves nothing behind. + var n int64 + db.Model(&adminmodels.SysApp{}).Where("app_code = ?", "order").Count(&n) + if n != 0 { + t.Errorf("a refused install wrote %d sys_app row(s)", n) + } +} + +// A dependency whose own install failed or never finished is not a dependency +// that is there, and the two say which they are - one sends you to install it, +// the other to look at why. +func TestInstallRefusesWhenADependencyIsNotFinished(t *testing.T) { + for _, tc := range []struct { + name string + status int + want string + }{ + {"failed", adminmodels.AppFailed, "its install failed"}, + {"installing", adminmodels.AppInstalling, "did not finish"}, + } { + t.Run(tc.name, func(t *testing.T) { + db := newInstallDB(t) + if err := db.Create(&adminmodels.SysApp{ + AppCode: "crm", Name: "crm", Version: "1.0.0", Status: tc.status, + }).Error; err != nil { + t.Fatal(err) + } + m := orderManifest("1.0.0") + m.Requires = []string{"crm"} + _, err := install(db, &fakeEngine{}, m) + if err == nil { + t.Fatal("the dependency was accepted") + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error = %q, want it to say %q", err, tc.want) + } + }) + } +} + +func TestInstallAcceptsASatisfiedDependency(t *testing.T) { + db := newInstallDB(t) + installedApp(t, db, "crm") + eng := &fakeEngine{entries: []migration.StatusEntry{ + {Version: "order-1786800001000", AppCode: "order", Registered: true}, + }} + m := orderManifest("1.0.0") + m.Requires = []string{"crm"} + + if _, err := install(db, eng, m); err != nil { + t.Fatalf("install: %v", err) + } + if row := loadRow(t, db, "order"); row.Status != adminmodels.AppInstalled { + t.Errorf("status = %d, want installed", row.Status) + } +} + +func TestDependencyCycleIsRefused(t *testing.T) { + manifests := map[string]app.Manifest{ + "a": {Code: "a", Requires: []string{"b"}}, + "b": {Code: "b", Requires: []string{"c"}}, + "c": {Code: "c", Requires: []string{"a"}}, + } + err := refuseOnDependencyCycle(manifests) + if err == nil { + t.Fatal("a cycle was accepted") + } + // The error is the cycle, not the walk that reached it. + if !strings.Contains(err.Error(), "a -> b -> c -> a") { + t.Errorf("error = %q", err) + } +} + +// A cycle between two applications neither of which is being installed is +// still an authoring mistake, and the day somebody installs into it is the +// worse time to find out. +func TestDependencyCycleIsRefusedEvenAwayFromTheTarget(t *testing.T) { + manifests := map[string]app.Manifest{ + "order": {Code: "order"}, + "x": {Code: "x", Requires: []string{"y"}}, + "y": {Code: "y", Requires: []string{"x"}}, + } + if err := refuseOnDependencyCycle(manifests); err == nil { + t.Fatal("a cycle away from the target was accepted") + } +} + +func TestDependencyGraphWithoutACycle(t *testing.T) { + manifests := map[string]app.Manifest{ + "a": {Code: "a", Requires: []string{"b", "c"}}, + "b": {Code: "b", Requires: []string{"c"}}, + "c": {Code: "c"}, + // Naming something that is not registered is not a cycle. Whether it + // is installed is a question for the database, at install time. + "d": {Code: "d", Requires: []string{"nowhere"}}, + } + if err := refuseOnDependencyCycle(manifests); err != nil { + t.Errorf("a graph with no cycle was refused: %v", err) + } +} + +// An application that names itself. +func TestDependencyCycleOfOne(t *testing.T) { + manifests := map[string]app.Manifest{"a": {Code: "a", Requires: []string{"a"}}} + err := refuseOnDependencyCycle(manifests) + if err == nil { + t.Fatal("an application requiring itself was accepted") + } + if !strings.Contains(err.Error(), "a -> a") { + t.Errorf("error = %q", err) + } +} diff --git a/cmd/migrate/server.go b/cmd/migrate/server.go index afc2768a..88ad9a6a 100644 --- a/cmd/migrate/server.go +++ b/cmd/migrate/server.go @@ -15,6 +15,7 @@ import ( "gorm.io/gorm" "github.com/go-admin-team/go-admin-core/v2/config/source/file" + "github.com/go-admin-team/go-admin-core/v2/sdk/contract/app" "github.com/spf13/cobra" "github.com/go-admin-team/go-admin-core/v2/sdk/config" @@ -305,6 +306,14 @@ func runInstall(code string) { exitOnError(os.Stderr, err) return } + // Over every registered manifest, not just this one's closure: a + // cycle between two other applications is still an authoring + // mistake, and the day somebody installs into it is the worse + // time to find out. + if err := refuseOnDependencyCycle(app.Snapshot()); err != nil { + exitOnError(os.Stderr, err) + return + } rep, err := install(db, migration.Migrate, m) if err != nil { exitOnError(os.Stderr, err)