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)