From 4973ee030de301d274cf79a8bdd33461e3e160ba Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Sun, 13 Sep 2026 20:33:47 +0800 Subject: [PATCH] =?UTF-8?q?fix=F0=9F=90=9B:=20bring=20an=20existing=20seed?= =?UTF-8?q?ed=20menu=20up=20to=20what=20the=20spec=20says?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit repairExistingMenu reconciled a row's paths and its api bindings and left every other column as an earlier run had written it. That cost three different things, and none of them announced itself. A menu whose parent was removed and seeded again kept parent_id pointing at the dead row while its paths named the new one. The tree is built from parent_id - SysMenu.GetPage walks down from ParentId == 0 - so the menu was gone from the sidebar, with the migration reporting success. A menu somebody added by hand under a seeded one kept the old prefix when its ancestor moved. It is in no spec, so nothing else would ever rewrite it; SysMenu.Update already does this cascade for the same column when a menu is moved through the UI. An application that renamed a menu, or moved its component, in a new version had the change ignored: the row was found by its natural key and returned untouched. The row a spec describes now has one definition, and both the insert and the repair use it - they cannot drift into disagreeing about what a spec decides. The repair writes every column on that list. Visible and IsFrame are deliberately not on it. They are seeding defaults the application never expressed, so an administrator who hid a seeded menu keeps it hidden; there is a test that hides one and reseeds. The cascade matches the row itself or a row strictly underneath it, rather than `paths LIKE old || '%'`, which also catches /0/1/20 when old is /0/1/2. An empty old path takes the single-row branch instead: there is no subtree under one, and the LIKE would have matched the whole table. Five degradations turn the new assertions red: not writing the spec columns at all, leaving ParentId off the list, putting Visible on it, not cascading, and cascading on the loose prefix. The last one did not, at first - the decoy rows were built against the path of the menu whose parent moved rather than the path that actually gets rewritten, so the prefix they collided with was never the one passed to the query. --- app/admin/service/seed.go | 133 ++++++++++++++++++++++------ app/admin/service/seed_test.go | 157 +++++++++++++++++++++++++++++++++ 2 files changed, 261 insertions(+), 29 deletions(-) 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 +}