diff --git a/.claude/skills/new-business-module/SKILL.md b/.claude/skills/new-business-module/SKILL.md index 9310a582..1cc0b66c 100644 --- a/.claude/skills/new-business-module/SKILL.md +++ b/.claude/skills/new-business-module/SKILL.md @@ -49,8 +49,18 @@ model、dto、router 三个文件,完整写法照抄 `app/demo/` 的结构。 ### 4. 写菜单、接口与权限种子数据 这一步最容易被漏掉——代码能编译、接口能测通,但界面上看不到菜单、点了按钮说 -没权限,往往就是漏了这一步。**完整参照 `cmd/migrate/migration/version/1786700001000_demo_menu.go`** -——那是可运行、幂等(用 `upsert`,重复跑不会报错)的真实例子,逐字照抄结构,只换 ID 和业务字段。 +没权限,往往就是漏了这一步。结构参照 `cmd/migrate/migration/version/1786700001000_demo_menu.go` +——它是可运行、幂等(用 `upsert`,重复跑不会报错)的真实例子。 + +:::danger +**但不要照抄它的 import。** 那个文件用的是 `cmd/migrate/migration/models`, +只因为它的版本号排在软删除转换(`1786700003000`)之前才是安全的。 + +**你新写的迁移版本号在转换之后,必须改用 `app/` 下的运行时模型** +(`app/admin/models.SysApi`、`SysMenu`),否则第一条 insert 就会 +`NOT NULL constraint failed: sys_api.deleted_at`。 +`TestPostConversionMigrationsAvoidFrozenSeedModels` 会拦住这个错误。 +::: 一个模块要在界面上可用,需要四类数据,缺一样都不行: diff --git a/AGENTS.md b/AGENTS.md index 309aa170..439c63fd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -193,6 +193,22 @@ go run -tags sqlite3 . server -c config/settings.sqlite.yml `git status` 看不到,PR 里也不会出现。两个目录的包名分别是 `version` 与 `version_local`(后者与目录名不一致,因为标识符不能含连字符)。 +### 写种子数据用哪个 models 包 + +`1786700003000` 之后新增的迁移,**种子数据要用 `app/` 下的运行时模型** +(如 `app/admin/models.SysApi`、`SysMenu`),**不要用 `cmd/migrate/migration/models`**。 + +后者的 `ModelTime` 声明的是可空的 `gorm.DeletedAt`,这对它之前的迁移是对的(那正是 +当时列的形状),转换之后就不再成立,两个方向都会出问题: + +- **写**:往 NOT NULL 列里塞 NULL,第一条 insert 就 `NOT NULL constraint failed` +- **读**:GORM 拼 `WHERE deleted_at IS NULL`,而活跃行存的是 `0`,静默查不到—— + 照抄 `demo_menu.go` 的授权段落会因此跳过授权,菜单建好、权限没授、迁移仍记为成功 + +干净库跑不出这个问题,今天所有用该包的迁移都排在转换之前。完整推导见 +`schema_coverage_test.go` 里 `TestPostConversionMigrationsAvoidFrozenSeedModels` +的注释,那个测试也守着这条边界。 + ## 提交规范 格式 `type+emoji: 描述`: diff --git a/cmd/migrate/migration/models/by.go b/cmd/migrate/migration/models/by.go index c9dd9061..6cb811d8 100644 --- a/cmd/migrate/migration/models/by.go +++ b/cmd/migrate/migration/models/by.go @@ -15,6 +15,14 @@ type Model struct { Id int `json:"id" gorm:"primaryKey;autoIncrement;comment:主键编码"` } +// ModelTime is frozen at the schema shape these tables had before +// 1786700003000 converted deleted_at to a NOT NULL millisecond marker. That is +// correct for the migrations ordered before the conversion, and wrong for any +// added after it: writes put NULL into a NOT NULL column, and reads are scoped +// "WHERE deleted_at IS NULL" and match nothing. +// +// Migrations after that version seed through the runtime models in app/. +// TestPostConversionMigrationsAvoidFrozenSeedModels enforces this. type ModelTime struct { CreatedAt time.Time `json:"createdAt" gorm:"comment:创建时间"` UpdatedAt time.Time `json:"updatedAt" gorm:"comment:最后更新时间"` diff --git a/cmd/migrate/migration/version/schema_coverage_test.go b/cmd/migrate/migration/version/schema_coverage_test.go index 7aa8f856..0fa37e2b 100644 --- a/cmd/migrate/migration/version/schema_coverage_test.go +++ b/cmd/migrate/migration/version/schema_coverage_test.go @@ -9,6 +9,8 @@ import ( "strconv" "strings" "testing" + + "go-admin/cmd/migrate/migration" ) // The repository carries two ModelTime types. The one in @@ -79,9 +81,13 @@ func runtimeSoftDeleteTables(t *testing.T) map[string]string { } func importsRuntimeModels(f *ast.File) bool { + return importsPackage(f, "go-admin/common/models") +} + +func importsPackage(f *ast.File, pkg string) bool { for _, imp := range f.Imports { p, err := strconv.Unquote(imp.Path.Value) - if err == nil && p == "go-admin/common/models" { + if err == nil && p == pkg { return true } } @@ -171,3 +177,89 @@ func repoRoot(t *testing.T) string { t.Fatal("go.mod not found above the test directory") return "" } + +// softDeleteConversion is the version at which sys_api, sys_menu and the rest +// stop storing deleted_at as a nullable timestamp and start storing the NOT +// NULL millisecond marker. +const softDeleteConversion = 1786700003000 + +// versionPrefixLen is the width migration.GetFilename slices off a filename. +const versionPrefixLen = 13 + +// Migrations ordered after the conversion must not seed rows through +// cmd/migrate/migration/models. +// +// That package's ModelTime still declares a nullable gorm.DeletedAt, which is +// correct for the migrations that predate the conversion - it is the shape the +// column had when they ran. Reusing it afterwards writes NULL into a NOT NULL +// column and the migration fails on its first insert: +// +// NOT NULL constraint failed: sys_api.deleted_at +// +// A fresh database never catches this, because every migration using that +// package today is ordered before the conversion and so runs while the column +// is still nullable. Only a migration added afterwards hits it, which in +// practice means the next person adding a business module - the reference +// they copy, 1786700001000_demo_menu.go, is itself one of the safe ones. +// +// Reads through that package are worse than writes, which is why the whole +// import is banned rather than just the inserts. gorm scopes a nullable +// DeletedAt as "WHERE deleted_at IS NULL", and after the conversion live rows +// hold 0, so the row is simply not there: +// +// frozen SysRole -> record not found +// runtime SysRole -> roleId=1 +// +// 1786700001000_demo_menu.go looks the admin role up that way and treats +// ErrRecordNotFound as "roles are not seeded yet, skip authorisation". A +// post-conversion copy that switched its inserts to the runtime models but +// kept this lookup would seed the menu, grant nothing, and still record the +// migration as applied - the menu appears, its buttons do nothing, and no +// error is reported anywhere. +func TestPostConversionMigrationsAvoidFrozenSeedModels(t *testing.T) { + const frozenModels = "go-admin/cmd/migrate/migration/models" + + dir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + + checked := 0 + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + // GetFilename is what every migration uses to derive its own version, + // so the two stay in step if the filename convention ever changes. + if len(name) < versionPrefixLen { + continue + } + version, err := strconv.ParseInt(migration.GetFilename(name), 10, 64) + if err != nil || version <= softDeleteConversion { + continue // not a versioned migration, or one that predates the change + } + checked++ + + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, filepath.Join(dir, name), nil, parser.ImportsOnly) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + if importsPackage(f, frozenModels) { + t.Errorf("%s is ordered after the soft-delete conversion but seeds through %s;\n"+ + " that package writes a nullable deleted_at and will fail with\n"+ + " \"NOT NULL constraint failed\" on its first insert.\n"+ + " Use the runtime models under app/ instead - they carry the marker.", + name, frozenModels) + } + } + + if checked == 0 { + t.Fatal("no post-conversion migrations found; the scan is broken, not the code") + } +}