diff --git a/app/admin/service/seed.go b/app/admin/service/seed.go index 0e73a59a..37705693 100644 --- a/app/admin/service/seed.go +++ b/app/admin/service/seed.go @@ -213,7 +213,7 @@ func seedMenuTree(tx *gorm.DB, appCode string, specs []seed.MenuSpec, apiRows ma err := tx.Where("app_code = ? AND seed_code = ?", appCode, s.Code).First(&existing).Error switch { case err == nil: - row, err := repairExistingMenu(tx, existing, s, parentRow, apiRows) + row, err := repairExistingMenu(tx, existing, appCode, s, parentRow, apiRows) if err != nil { return nil, fmt.Errorf("%q: repairing an existing row: %w", s.Code, err) } @@ -227,26 +227,7 @@ func seedMenuTree(tx *gorm.DB, appCode string, specs []seed.MenuSpec, apiRows ma return nil, fmt.Errorf("%q: checking for an existing row: %w", s.Code, err) } - seedCode := s.Code - row := models.SysMenu{ - MenuName: menuName(appCode, s.Code), - Title: s.Title, - Icon: s.Icon, - Path: s.Path, - MenuType: s.Kind, - Permission: s.Permission, - ParentId: parentRow.MenuId, - Component: s.Component, - Sort: s.Sort, - // Visible "0" is shown, not hidden - the same defaults - // 1786700001000_demo_menu.go seeds its own menu with. A - // freshly installed application's menu should not need an - // administrator to first find and unhide it. - Visible: "0", - IsFrame: "1", - AppCode: appCode, - SeedCode: &seedCode, - } + row := menuRowFor(appCode, s, parentRow) for _, code := range s.ApiCodes { api, ok := apiRows[code] if !ok { @@ -345,15 +326,27 @@ func expectedPaths(menuID int, parent string, parentRow models.SysMenu) string { // here just as much as it does there. Reconciling stale seed-driven // bindings, if it is ever wanted, belongs in the upgrade path with that // same ownership check - not silently inside every retry of every install. -func repairExistingMenu(tx *gorm.DB, existing models.SysMenu, s seed.MenuSpec, parentRow models.SysMenu, apiRows map[string]models.SysApi) (models.SysMenu, error) { - want := expectedPaths(existing.MenuId, s.Parent, parentRow) - if existing.Paths != want { - if err := tx.Model(&models.SysMenu{}).Where("menu_id = ?", existing.MenuId). - Update("paths", want).Error; err != nil { - return models.SysMenu{}, fmt.Errorf("repairing paths: %w", err) - } - existing.Paths = want +func repairExistingMenu(tx *gorm.DB, existing models.SysMenu, appCode string, s seed.MenuSpec, parentRow models.SysMenu, apiRows map[string]models.SysApi) (models.SysMenu, error) { + // Every column the spec decides, not just the two this used to touch. A + // menu whose parent was removed and reseeded kept parent_id pointing at + // the dead row while its paths named the new one, and the tree is built + // from parent_id - so the menu vanished from the sidebar with the + // migration reporting success. An application that renamed a menu or + // moved its component between versions had its change silently ignored + // for the same reason: nothing here wrote those columns. + want := menuRowFor(appCode, s, parentRow) + if err := tx.Model(&models.SysMenu{}).Where("menu_id = ?", existing.MenuId). + Select(specMenuFields).Updates(want).Error; err != nil { + return models.SysMenu{}, fmt.Errorf("bringing the row up to the spec: %w", err) } + want.MenuId = existing.MenuId + want.Paths = existing.Paths + want.Visible, want.IsFrame = existing.Visible, existing.IsFrame + + if err := repairPaths(tx, &want, s, parentRow); err != nil { + return models.SysMenu{}, err + } + existing = want for _, code := range s.ApiCodes { api, ok := apiRows[code] @@ -492,3 +485,85 @@ func recordGrant(tx *gorm.DB, appCode, roleKey, path, action string) error { appCode, roleKey, path, action, time.Now(), roleKey, path, action, ).Error } + +// specMenuFields are the sys_menu columns a MenuSpec decides, and the only +// ones a reseed rewrites on a row that is already there. +// +// Visible and IsFrame are not in the list. They are seeding defaults the +// application never expressed, so an administrator who hid a seeded menu +// keeps it hidden. app_code and seed_code are not either: they are the +// natural key the row was found by, and writing them back would be writing +// what was just matched. +var specMenuFields = []string{ + "MenuName", "Title", "Icon", "Path", "MenuType", + "Permission", "ParentId", "Component", "Sort", +} + +// menuRowFor is the row a MenuSpec describes. One definition, so the insert +// path and the repair path cannot drift into disagreeing about what a spec +// decides. +func menuRowFor(appCode string, s seed.MenuSpec, parentRow models.SysMenu) models.SysMenu { + seedCode := s.Code + return models.SysMenu{ + MenuName: menuName(appCode, s.Code), + Title: s.Title, + Icon: s.Icon, + Path: s.Path, + MenuType: s.Kind, + Permission: s.Permission, + ParentId: parentRow.MenuId, + Component: s.Component, + Sort: s.Sort, + // Visible "0" is shown, not hidden - the same defaults + // 1786700001000_demo_menu.go seeds its own menu with. A freshly + // installed application's menu should not need an administrator to + // first find and unhide it. Only written when the row is created; + // see specMenuFields. + Visible: "0", + IsFrame: "1", + AppCode: appCode, + SeedCode: &seedCode, + } +} + +// repairPaths writes row.Paths, and moves whatever is underneath it. +// +// The subtree matters because it is not all in this call's specs: a menu an +// administrator added under a seeded one keeps the old prefix, and nothing +// else in the codebase would ever rewrite it. SysMenu.Update does the same +// cascade for the same column when somebody moves a menu by hand. +// +// The predicate is the row itself or a row strictly under it, rather than +// `paths LIKE old || '%'`, which also matches /0/10 when old is /0/1. +func repairPaths(tx *gorm.DB, row *models.SysMenu, s seed.MenuSpec, parentRow models.SysMenu) error { + want := expectedPaths(row.MenuId, s.Parent, parentRow) + old := row.Paths + if old == want { + return nil + } + if old == "" { + // A row whose paths was never written - an interrupted create. It + // has no subtree to speak of, and `LIKE '/%'` would match the whole + // table. + if err := tx.Model(&models.SysMenu{}).Where("menu_id = ?", row.MenuId). + Update("paths", want).Error; err != nil { + return fmt.Errorf("writing paths: %w", err) + } + row.Paths = want + return nil + } + + var subtree []models.SysMenu + if err := tx.Where("paths = ? OR paths LIKE ?", old, old+"/%").Find(&subtree).Error; err != nil { + return fmt.Errorf("reading the subtree under %s: %w", old, err) + } + for _, d := range subtree { + moved := want + strings.TrimPrefix(d.Paths, old) + if err := tx.Model(&models.SysMenu{}).Where("menu_id = ?", d.MenuId). + Update("paths", moved).Error; err != nil { + return fmt.Errorf("moving %d from %s to %s: %w", d.MenuId, d.Paths, moved, err) + } + } + row.Paths = want + return nil +} diff --git a/app/admin/service/seed_test.go b/app/admin/service/seed_test.go index de220b75..9826689b 100644 --- a/app/admin/service/seed_test.go +++ b/app/admin/service/seed_test.go @@ -983,3 +983,160 @@ func TestSeedMenusLedgerToleratesAnEntryWhosePolicyWasRemoved(t *testing.T) { t.Errorf("the policy was not put back: %d rows", policies) } } + +// The tree is built from parent_id, not from paths. A menu whose parent was +// removed and written again kept parent_id on the dead row while its paths +// named the new one, so the menu was gone from the sidebar and the migration +// said it had succeeded. +func TestSeedMenusRepairsParentIdAfterTheParentWasRemoved(t *testing.T) { + db := newSeedTestDB(t) + useCompositeSeedCodeIndex(t, db) + seedAdminRole(t, db) + menus, apis := orderMenuSpecs("Orders", "apps/order/index") + + if err := (adminSeeder{}).SeedMenus(db, "order", menus, apis); err != nil { + t.Fatalf("first seed: %v", err) + } + var dir models.SysMenu + if err := db.Where("app_code = ? AND seed_code = ?", "order", "dir").First(&dir).Error; err != nil { + t.Fatal(err) + } + if err := db.Delete(&models.SysMenu{}, "menu_id = ?", dir.MenuId).Error; err != nil { + t.Fatalf("removing the parent: %v", err) + } + + if err := (adminSeeder{}).SeedMenus(db, "order", menus, apis); err != nil { + t.Fatalf("second seed: %v", err) + } + + var newDir, list models.SysMenu + if err := db.Where("app_code = ? AND seed_code = ?", "order", "dir").First(&newDir).Error; err != nil { + t.Fatal(err) + } + if err := db.Where("app_code = ? AND seed_code = ?", "order", "list").First(&list).Error; err != nil { + t.Fatal(err) + } + if newDir.MenuId == dir.MenuId { + t.Fatal("the removed parent was reused, so this test proves nothing") + } + if list.ParentId != newDir.MenuId { + t.Errorf("parent_id = %d, want the new parent %d; the menu hangs off a row that is gone", + list.ParentId, newDir.MenuId) + } + if want := newDir.Paths + "/" + strconv.Itoa(list.MenuId); list.Paths != want { + t.Errorf("paths = %q, want %q", list.Paths, want) + } +} + +// A menu somebody added under a seeded one is not in any spec, so nothing but +// this would ever rewrite its path when its ancestor moves. +func TestSeedMenusMovesTheSubtreeUnderARepairedMenu(t *testing.T) { + db := newSeedTestDB(t) + useCompositeSeedCodeIndex(t, db) + seedAdminRole(t, db) + menus, apis := orderMenuSpecs("Orders", "apps/order/index") + + if err := (adminSeeder{}).SeedMenus(db, "order", menus, apis); err != nil { + t.Fatalf("first seed: %v", err) + } + var dir, list models.SysMenu + db.Where("app_code = ? AND seed_code = ?", "order", "dir").First(&dir) + db.Where("app_code = ? AND seed_code = ?", "order", "list").First(&list) + + // By hand, under the seeded menu, the way an administrator would. + hand := models.SysMenu{MenuName: "HandMade", Title: "By hand", MenuType: contractmodels.Menu, + ParentId: list.MenuId} + if err := db.Create(&hand).Error; err != nil { + t.Fatal(err) + } + hand.Paths = list.Paths + "/" + strconv.Itoa(hand.MenuId) + db.Model(&models.SysMenu{}).Where("menu_id = ?", hand.MenuId).Update("paths", hand.Paths) + + // Rows whose paths start with the moving one's as a string and are not + // underneath it as a path. /0/1/2 is a string prefix of /0/1/20, and a + // LIKE on the bare prefix cannot tell the two apart - so these have to + // be built against the path that actually moves, which is the one this + // repair rewrites. + var decoys []models.SysMenu + for _, suffix := range []string{"0", "1", "9"} { + d := models.SysMenu{MenuName: "Decoy" + suffix, Title: "decoy", MenuType: contractmodels.Menu} + if err := db.Create(&d).Error; err != nil { + t.Fatal(err) + } + d.Paths = list.Paths + suffix + db.Model(&models.SysMenu{}).Where("menu_id = ?", d.MenuId).Update("paths", d.Paths) + decoys = append(decoys, d) + } + + if err := db.Delete(&models.SysMenu{}, "menu_id = ?", dir.MenuId).Error; err != nil { + t.Fatal(err) + } + if err := (adminSeeder{}).SeedMenus(db, "order", menus, apis); err != nil { + t.Fatalf("second seed: %v", err) + } + + var newList, movedHand models.SysMenu + db.Where("app_code = ? AND seed_code = ?", "order", "list").First(&newList) + db.Where("menu_id = ?", hand.MenuId).First(&movedHand) + if want := newList.Paths + "/" + strconv.Itoa(hand.MenuId); movedHand.Paths != want { + t.Errorf("the hand-made menu's paths = %q, want %q; it no longer names its ancestors", + movedHand.Paths, want) + } + for _, d := range decoys { + var after models.SysMenu + db.Where("menu_id = ?", d.MenuId).First(&after) + if after.Paths != d.Paths { + t.Errorf("decoy %d moved from %q to %q; a prefix match caught a row that is not underneath", + d.MenuId, d.Paths, after.Paths) + } + } +} + +// An application that renames a menu or moves its component in a new version +// had the change ignored: the row was found and returned untouched. +func TestSeedMenusRefreshesWhatTheSpecDecides(t *testing.T) { + db := newSeedTestDB(t) + useCompositeSeedCodeIndex(t, db) + seedAdminRole(t, db) + + menus, apis := orderMenuSpecs("Orders", "apps/order/index") + if err := (adminSeeder{}).SeedMenus(db, "order", menus, apis); err != nil { + t.Fatalf("first seed: %v", err) + } + // An administrator hides it. That is not something the spec expresses, + // so a reseed has no business turning it back on. + if err := db.Model(&models.SysMenu{}).Where("app_code = ? AND seed_code = ?", "order", "list"). + Update("visible", "1").Error; err != nil { + t.Fatal(err) + } + + menus2, apis2 := orderMenuSpecs("Sales orders", "apps/order/list/index") + if err := (adminSeeder{}).SeedMenus(db, "order", menus2, apis2); err != nil { + t.Fatalf("second seed: %v", err) + } + + var list models.SysMenu + db.Where("app_code = ? AND seed_code = ?", "order", "list").First(&list) + if list.Title != "Sales orders" { + t.Errorf("title = %q, want the new one", list.Title) + } + if list.Component != "apps/order/list/index" { + t.Errorf("component = %q, want the new one", list.Component) + } + if list.Visible != "1" { + t.Errorf("visible = %q; a reseed unhid a menu an administrator had hidden", list.Visible) + } +} + +// orderMenuSpecs is a two-level tree plus one api, parameterised on the two +// columns the upgrade test changes. +func orderMenuSpecs(title, component string) ([]seed.MenuSpec, []seed.ApiSpec) { + menus := []seed.MenuSpec{ + {Code: "dir", Kind: contractmodels.Directory, Title: "Order Example", Path: "/apps/order", Component: "Layout", Sort: 10}, + {Code: "list", Parent: "dir", Kind: contractmodels.Menu, Title: title, Path: "list", Component: component, Sort: 1, ApiCodes: []string{"list"}}, + } + apis := []seed.ApiSpec{ + {Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET", Handle: "apis.Order.GetPage-fm"}, + } + return menus, apis +}