From 9dd271ecab314d42651c00bec472a21ca2daeb82 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Sun, 13 Sep 2026 20:34:04 +0800 Subject: [PATCH] =?UTF-8?q?fix=F0=9F=90=9B:=20claim=20the=20rows=20an=20ap?= =?UTF-8?q?plication=20wrote=20before=20seed=5Fcode=20existed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1786700008000 added sys_menu.seed_code and left it NULL on every row that was already there. That is right for the host's own hand-placed menus: there is nothing to derive one from. An application's rows are in that population too, and for those it is derivable - menu_name is what identified them before the column existed. The natural-key lookup missed them, so a reseed inserted a second copy beside each one, and the new unique index could not object, because NULL never collides on MySQL, PostgreSQL or SQLite and is filtered out of the index on SQL Server. Claimed when the application is seeded rather than by a backfill migration. The value is only derivable where the spec's own Code is in hand: menuName concatenates two pascalCase strings and does not reverse, so a migration looking at menu_name alone would be guessing. For the same reason more than one match is refused and named rather than picked from - attaching an application's menu to whichever row the database returned first is the failure this is meant to prevent, not a smaller version of it. The match is scoped to the application's own app_code, so a row belonging to another application, or to the host, is not claimed. An adopted row then goes through the ordinary repair, so it comes out carrying what the spec says rather than what it held from before. Three degradations turn the new assertions red: not adopting at all, picking a row when there is more than one, and dropping the app_code from the match. --- app/admin/service/seed.go | 60 ++++++++++++++++-- app/admin/service/seed_test.go | 111 +++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 4 deletions(-) diff --git a/app/admin/service/seed.go b/app/admin/service/seed.go index 37705693..bfb09d45 100644 --- a/app/admin/service/seed.go +++ b/app/admin/service/seed.go @@ -210,9 +210,23 @@ func seedMenuTree(tx *gorm.DB, appCode string, specs []seed.MenuSpec, apiRows ma // the soft-delete plugin scopes deleted_at = 0 automatically on // every query against models.SysMenu. var existing models.SysMenu + found := false err := tx.Where("app_code = ? AND seed_code = ?", appCode, s.Code).First(&existing).Error switch { case err == nil: + found = true + case errors.Is(err, gorm.ErrRecordNotFound): + // Nothing under the natural key. It may still be here from + // before seed_code existed, under the name that identified + // it then. + existing, found, err = adoptLegacyMenu(tx, appCode, s) + if err != nil { + return nil, fmt.Errorf("%q: %w", s.Code, err) + } + default: + return nil, fmt.Errorf("%q: checking for an existing row: %w", s.Code, err) + } + if found { 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) @@ -221,10 +235,6 @@ func seedMenuTree(tx *gorm.DB, appCode string, specs []seed.MenuSpec, apiRows ma ids = append(ids, row.MenuId) progressed = true continue - case errors.Is(err, gorm.ErrRecordNotFound): - // Not written yet; fall through to create it below. - default: - return nil, fmt.Errorf("%q: checking for an existing row: %w", s.Code, err) } row := menuRowFor(appCode, s, parentRow) @@ -567,3 +577,45 @@ func repairPaths(tx *gorm.DB, row *models.SysMenu, s seed.MenuSpec, parentRow mo row.Paths = want return nil } + +// adoptLegacyMenu claims a row this application wrote before sys_menu had a +// seed_code column, so a reseed repairs it instead of inserting a second copy +// beside it. +// +// 1786700008000 added the column and left it NULL on every row already there, +// which is right for the host's own hand-placed menus - there is nothing to +// derive one from. An application's rows are in that population too, and for +// those the value is derivable, because menu_name is what identified them +// before the column existed. Without this the natural-key lookup misses them, +// the seed inserts a duplicate, and the unique index cannot object: NULL +// never collides. +// +// Ambiguity is refused rather than guessed. menuName concatenates two +// pascalCase strings and pascalCase is not injective, so two specs can land +// on one name; picking one of several rows would attach an application's +// menu to whichever the database returned first. +func adoptLegacyMenu(tx *gorm.DB, appCode string, s seed.MenuSpec) (models.SysMenu, bool, error) { + name := menuName(appCode, s.Code) + var rows []models.SysMenu + if err := tx.Where("app_code = ? AND menu_name = ? AND seed_code IS NULL", appCode, name). + Find(&rows).Error; err != nil { + return models.SysMenu{}, false, fmt.Errorf("looking for a row written before seed_code existed: %w", err) + } + switch len(rows) { + case 0: + return models.SysMenu{}, false, nil + case 1: + default: + return models.SysMenu{}, false, fmt.Errorf( + "%d rows carry menu_name %q with no seed_code; which of them belongs to %q cannot be decided here, because menuName is not reversible - reconcile them by hand", + len(rows), name, s.Code) + } + + seedCode := s.Code + if err := tx.Model(&models.SysMenu{}).Where("menu_id = ?", rows[0].MenuId). + Update("seed_code", seedCode).Error; err != nil { + return models.SysMenu{}, false, fmt.Errorf("claiming the row written before seed_code existed: %w", err) + } + rows[0].SeedCode = &seedCode + return rows[0], true, nil +} diff --git a/app/admin/service/seed_test.go b/app/admin/service/seed_test.go index 9826689b..983850f3 100644 --- a/app/admin/service/seed_test.go +++ b/app/admin/service/seed_test.go @@ -3,6 +3,7 @@ package service import ( "context" "errors" + "fmt" "strconv" "strings" "sync" @@ -1140,3 +1141,113 @@ func orderMenuSpecs(title, component string) ([]seed.MenuSpec, []seed.ApiSpec) { } return menus, apis } + +// 1786700008000 added seed_code and left it NULL on every row already there. +// An application's rows are in that population, and the natural-key lookup +// misses them, so the seed used to insert a second copy beside each one - +// which the unique index cannot object to, because NULL never collides. +func TestSeedMenusAdoptsARowWrittenBeforeSeedCodeExisted(t *testing.T) { + db := newSeedTestDB(t) + useCompositeSeedCodeIndex(t, db) + seedAdminRole(t, db) + + // What an older SeedMenus left: app_code set, seed_code absent, and the + // name that identified it then. + legacy := models.SysMenu{ + MenuName: menuName("order", "dir"), AppCode: "order", Title: "the old title", + MenuType: contractmodels.Directory, Path: "/apps/order", Component: "Layout", Sort: 10, + } + if err := db.Create(&legacy).Error; err != nil { + t.Fatal(err) + } + + menus, apis := orderMenuSpecs("Orders", "apps/order/index") + if err := (adminSeeder{}).SeedMenus(db, "order", menus, apis); err != nil { + t.Fatalf("seed: %v", err) + } + + var rows []models.SysMenu + if err := db.Where("app_code = ? AND menu_name = ?", "order", menuName("order", "dir")). + Find(&rows).Error; err != nil { + t.Fatal(err) + } + if len(rows) != 1 { + t.Fatalf("%d rows carry that name; the row from before the column existed was not found", len(rows)) + } + if rows[0].MenuId != legacy.MenuId { + t.Errorf("menu_id = %d, want the row that was already there (%d)", rows[0].MenuId, legacy.MenuId) + } + if rows[0].SeedCode == nil || *rows[0].SeedCode != "dir" { + t.Errorf("seed_code = %v, want it claimed", rows[0].SeedCode) + } + // Adopted and then repaired, like any other existing row. + if rows[0].Title != "Order Example" { + t.Errorf("title = %q; the adopted row was not brought up to the spec", rows[0].Title) + } +} + +// menuName concatenates two pascalCase strings and pascalCase is not +// injective, so two specs can land on one name. Picking one of several rows +// would attach an application's menu to whichever the database returned +// first. +func TestSeedMenusRefusesAnAmbiguousAdoption(t *testing.T) { + db := newSeedTestDB(t) + useCompositeSeedCodeIndex(t, db) + seedAdminRole(t, db) + + for i := 0; i < 2; i++ { + row := models.SysMenu{ + MenuName: menuName("order", "dir"), AppCode: "order", Title: fmt.Sprintf("copy %d", i), + MenuType: contractmodels.Directory, + } + if err := db.Create(&row).Error; err != nil { + t.Fatal(err) + } + } + + menus, apis := orderMenuSpecs("Orders", "apps/order/index") + err := (adminSeeder{}).SeedMenus(db, "order", menus, apis) + if err == nil { + t.Fatal("an ambiguous adoption was accepted") + } + if !strings.Contains(err.Error(), "2 rows") || !strings.Contains(err.Error(), "by hand") { + t.Errorf("error = %q, it has to say how many and that it is not deciding", err) + } + // And it did not write a third. + var n int64 + db.Model(&models.SysMenu{}).Where("app_code = ? AND menu_name = ?", "order", menuName("order", "dir")).Count(&n) + if n != 2 { + t.Errorf("%d rows carry that name; the refusal still inserted", n) + } +} + +// A row belonging to another application, or to the host, carries a different +// app_code and is not this application's to claim. +func TestSeedMenusDoesNotAdoptAnotherApplicationsRow(t *testing.T) { + db := newSeedTestDB(t) + useCompositeSeedCodeIndex(t, db) + seedAdminRole(t, db) + + other := models.SysMenu{ + MenuName: menuName("order", "dir"), AppCode: "crm", Title: "crm's own", + MenuType: contractmodels.Directory, + } + if err := db.Create(&other).Error; err != nil { + t.Fatal(err) + } + + menus, apis := orderMenuSpecs("Orders", "apps/order/index") + if err := (adminSeeder{}).SeedMenus(db, "order", menus, apis); err != nil { + t.Fatalf("seed: %v", err) + } + + var after models.SysMenu + db.Where("menu_id = ?", other.MenuId).First(&after) + if after.SeedCode != nil || after.Title != "crm's own" { + t.Errorf("another application's row was claimed: %+v", after) + } + var mine models.SysMenu + if err := db.Where("app_code = ? AND seed_code = ?", "order", "dir").First(&mine).Error; err != nil { + t.Fatalf("this application's own row was not created: %v", err) + } +}