From 71bb7432cd7cef3ed6d93965ec37a5bb9d78fa1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tiger=20Wang=20=28=E7=8E=8B=E8=B1=AB=29?= Date: Thu, 7 Jul 2022 21:54:13 -0400 Subject: [PATCH] API feedback (#341) * wip * wip * wip * wip * wip * wip * wip * wip --- model/app.go | 9 ++++ model/category.go | 4 +- model/docker.go | 2 +- route/route.go | 80 ++++++++++++++++++++++++++++++++---- route/v1/app.go | 6 ++- route/v1/docker.go | 16 +++++--- route/v1/system.go | 2 +- route/v1/user.go | 28 ++++++++++--- service/app.go | 2 + service/model/o_container.go | 4 +- service/model/o_user.go | 4 +- service/rely.go | 2 + 12 files changed, 128 insertions(+), 31 deletions(-) diff --git a/model/app.go b/model/app.go index 387e437..d195132 100644 --- a/model/app.go +++ b/model/app.go @@ -13,6 +13,15 @@ type ServerAppListCollection struct { Version string `json:"version"` } +// @tiger - 对于用于出参的数据结构，静态信息（例如 title）和 +// 动态信息（例如 state、query_count）应该划分到不同的数据结构中 +// +// 这样的好处是 +// 1 - 多次获取动态信息时可以减少出参复杂度，因为静态信息只获取一次就好 +// 2 - 在未来的迭代中，可以降低维护成本（所有字段都展开放在一个层级维护成本略高） +// +// 另外，一些针对性字段，例如 Docker 相关的，可以用 map 来保存。 +// 这样在未来增加多态 App，例如 Snap，不需要维护多个结构，或者一个结构保存不必要的字段 type ServerAppList struct { Id uint `gorm:"column:id;primary_key" json:"id"` Title string `json:"title"` diff --git a/model/category.go b/model/category.go index 3ee71a0..ed5e848 100644 --- a/model/category.go +++ b/model/category.go @@ -18,7 +18,7 @@ type CategoryList struct { //CreatedAt time.Time `json:"created_at"` // //UpdatedAt time.Time `json:"updated_at"` - Font string `json:"font"` + Font string `json:"font"` // @tiger - 如果这个和前端有关，应该不属于后端的出参范围，而是前端去界定 Name string `json:"name"` - Count uint `json:"count"` + Count uint `json:"count"` // @tiger - count 属于动态信息，应该单独放在一个出参结构中（原因见另外一个关于 静态/动态 出参的注释） } diff --git a/model/docker.go b/model/docker.go index 59b006d..2fb03ba 100644 --- a/model/docker.go +++ b/model/docker.go @@ -14,5 +14,5 @@ type DockerStatsModel struct { Icon string `json:"icon"` Title string `json:"title"` Data interface{} `json:"data"` - Pre interface{} `json:"pre"` + Pre interface{} `json:"pre"` // @tiger - pre 不知道什么意思，可以提高一下描述性 } diff --git a/route/route.go b/route/route.go index eb96ce9..215fbb7 100644 --- a/route/route.go +++ b/route/route.go @@ -1,3 +1,5 @@ +// 这是一个用来反馈 API 设计的 PR，不要 merge + package route import ( @@ -23,6 +25,8 @@ func InitRouter() *gin.Engine { r.Use(middleware.WriteLog()) r.Use(gzip.Gzip(gzip.DefaultCompression)) gin.SetMode(config.ServerInfo.RunMode) + + // @tiger - 为了方便未来的模块化迭代，前端输出需要独立端口，不要和 API 端口公用。 r.StaticFS("/ui", http.FS(web.Static)) r.GET("/", WebUIHome) // r.StaticFS("/assets", http.Dir("./static/assets")) @@ -33,17 +37,33 @@ func InitRouter() *gin.Engine { r.POST("/v1/user/register/:key", v1.PostUserRegister) r.POST("/v1/user/login", v1.PostUserLogin) // + + // @tiger - 如果遵循 RESTful 规范，name 本身并不是资源，而是属性；资源是 user + // 所以正规的方法是 改成 /v1/users 然后返回所有的 user 对象，具体 name 由前端自行抽取 + // 不正规的方式是 改成 /v1/users/names，假定 name 也是资源 r.GET("/v1/user/all/name", v1.GetUserAllUserName) - r.GET("/v1/sys/init/check", v1.GetSystemInitCheck) - r.GET("/v1/guide/check", v1.GetGuideCheck) - r.GET("/v1/debug", v1.GetSystemConfigDebug) + // @tiger - 1）不要把同一个词汇按单词来分割。2）同领域的 API 应该放在同路径下。 + r.GET("/v1/sys/init/check", v1.GetSystemInitCheck) // 这里改成 /v1/sys/init_check + r.GET("/v1/guide/check", v1.GetGuideCheck) // 这里改成 /v1/sys/guide_check + r.GET("/v1/debug", v1.GetSystemConfigDebug) // 这里改成 /v1/sys/debug + // @tiger - 如果遵循 RESTful avatar 本身并不是资源，而是属性；资源是 user + // 所以正规的方法是 改成 /v1/user/:id 然后返回 user 对象，具体 avatar 由前端自行抽取 + // 不正规的方式是 改成 /v1/user/:id/avatar，假定 avatar 也是资源 r.GET("/v1/user/avatar/:id", v1.GetUserAvatar) + + // @tiger - 如果遵循 RESTful image 本身并不是资源，而是属性；资源是 user + // 所以正规的方法是 改成 /v1/user/:id 然后返回 user 对象，具体 image 由前端自行抽取 + // 不正规的方式是 改成 /v1/user/:id/image，假定 image 也是资源 r.GET("/v1/user/image", v1.GetUserImage) + // @tiger - 不要把同一个词汇按单词来分割，改成 /v1/sys/socket_port r.GET("/v1/sys/socket/port", v1.GetSystemSocketPort) + + // @tiger - （nice-to-have）开源项目应该删除所有注释代码，增加代码整洁性。或者增加注释说明 //r.POST("/v1/user/refresh/token", v1.PostUserRefreshToken) + v1Group := r.Group("/v1") v1Group.Use(jwt2.JWT()) @@ -51,49 +71,86 @@ func InitRouter() *gin.Engine { v1UserGroup := v1Group.Group("/user") v1UserGroup.Use() { + // @tiger - info 一词名没有指定性，容易产生困扰。改成 /current v1UserGroup.GET("/info", v1.GetUserInfo) + + // @tiger - RESTful 规范下所有对 user 的写操作，都应该 POST /v1/user/:id + // - 不需要每个更改的属性建一个 API v1UserGroup.PUT("/username", v1.PutUserName) v1UserGroup.PUT("/password", v1.PutUserPwd) - v1UserGroup.PUT("/nick", v1.PutUserNick) - v1UserGroup.PUT("/desc", v1.PutUserDesc) + v1UserGroup.PUT("/nick", v1.PutUserNick) // 改成 /nickname + v1UserGroup.PUT("/desc", v1.PutUserDesc) // 改成 /description + + // @tiger - RESTful 规范建议是 GET /v1/users/?username=xxxx + // 这是一个查询 API，返回一个 users 数组（即使 username 是唯一的） + // 之所以不用 /v1/user/:username 是因为和 /v1/user/:id 路由冲突 + // + // 当前这个设计的问题是：GET 不应该同时接收 request body。 + // GET 方法应该只接收 URL 参数 v1UserGroup.GET("/info", v1.GetUserInfoByUserName) + + // @tiger - 改成 /user/current/custom/... 和上面的 current 对应 + // 如果未来想获得其它用户的 custom 数据，可以用 /v1/user/:id/custom/... 来保持统一 v1UserGroup.GET("/custom/:key", v1.GetUserCustomConf) v1UserGroup.POST("/custom/:key", v1.PostUserCustomConf) v1UserGroup.DELETE("/custom/:key", v1.DeleteUserCustomConf) + + // @tiger - 下面这两个 API 从感知上很难区分。 + // 如果前者是负责上传，后者负责指定的话，那么 + // 前者应该用一个统一的和目的无关的用户文件上传 API，而不是针对 image file 的 v1UserGroup.POST("/upload/image/:key", v1.PostUserUploadImage) v1UserGroup.POST("/file/image/:key", v1.PostUserFileImage) v1UserGroup.DELETE("/image", v1.DeleteUserImage) + // @tiger - 应该用上面提到的统一的文件上传 API 先上传头像文件，然后 + // 用类似上面第二个 API 的方式指定头像文件。这样整体 API 体验更加统一。 v1UserGroup.PUT("/avatar", v1.PutUserAvatar) v1UserGroup.GET("/avatar", v1.GetUserAvatar) + + // @tiger - 删除用户直接用 DELETE /v1/user/:id，不需要在路径中用谓语 v1UserGroup.DELETE("/delete/:id", v1.DeleteUser) } v1AppGroup := v1Group.Group("/app") v1AppGroup.Use() { + // @tiger - 按照 RESTFul 规范，改成 GET /v1/apps?installed=true //获取我的已安装的列表 v1AppGroup.GET("/my/list", v1.MyAppList) - // + + // @tiger - 按照 RESTFul 规范，改成 GET /v1/apps/usage v1AppGroup.GET("/usage", v1.AppUsageList) + + // @tiger - 按照 RESTFul 规范，改成 GET /v1/app/:id //app详情 v1AppGroup.GET("/appinfo/:id", v1.AppInfo) + + // @tiger - 按照 RESTFul 规范，改成 GET /v1/apps?installed=false //获取未安装的列表 v1AppGroup.GET("/list", v1.AppList) + + // @tiger - 这个信息和应用无关，应该挪到 /v1/sys/port/avaiable //获取端口 v1AppGroup.GET("/port", v1.GetPort) + + // @tiger - RESTFul 路径中尽量不要有动词，同时这个信息和应用无关，应该挪到 /v1/sys/port/:port //检查端口 v1AppGroup.GET("/check/:port", v1.PortCheck) + // @tiger - 应用分类和应用不是一类资源，应该挪到 GET /v1/app_categories v1AppGroup.GET("/category", v1.CategoryList) + // @tiger - Docker Terminal 和应用不是一类资源，应该挪到 GET /v1/container/:id/terminal + // 另外这个返回的不是一个 HTTP 响应，应该返回一个 wss://... 的 URL给前端，由前端另行处理 v1AppGroup.GET("/terminal/:id", v1.DockerTerminal) + + // @tiger - 所有跟 Docker 有关的 API，应该挪到 /v1/container 下 //app容器详情 - v1AppGroup.GET("/info/:id", v1.ContainerInfo) + v1AppGroup.GET("/info/:id", v1.ContainerInfo) // 改成 GET /v1/container/:id //app容器日志 - v1AppGroup.GET("/logs/:id", v1.ContainerLog) + v1AppGroup.GET("/logs/:id", v1.ContainerLog) // 改成 GET /v1/container/:id/log //暂停或启动容器 - v1AppGroup.PUT("/state/:id", v1.ChangAppState) + v1AppGroup.PUT("/state/:id", v1.ChangAppState) // 改成 PUT /v1/container/:id/state //安装app v1AppGroup.POST("/install", v1.InstallApp) //卸载app @@ -104,8 +161,13 @@ func InitRouter() *gin.Engine { v1AppGroup.PUT("/update/:id/setting", v1.UpdateSetting) //获取可能新数据 v1AppGroup.GET("/update/:id/info", v1.ContainerUpdateInfo) + + // @tiger - rely -> dependency - 依赖是什么意思？ v1AppGroup.GET("/rely/:id/info", v1.ContainerRelyInfo) + + // @tiger - 按照 RESTFul 规范，改成 GET /v1/container/:id/config v1AppGroup.GET("/install/config", v1.GetDockerInstallConfig) + v1AppGroup.PUT("/update/:id", v1.PutAppUpdate) v1AppGroup.POST("/share", v1.ShareAppFile) } diff --git a/route/v1/app.go b/route/v1/app.go index 5ff3953..3f6c705 100644 --- a/route/v1/app.go +++ b/route/v1/app.go @@ -83,6 +83,7 @@ func GetPort(c *gin.Context) { p, _ = port2.GetAvailablePort(t) ok = !port2.IsPortAvailable(p, t) } + // @tiger 这里最好封装成 {'port': ...} 的形式，来体现出参的上下文 c.JSON(http.StatusOK, &model.Result{Success: common_err.SUCCESS, Message: common_err.GetMsg(common_err.SUCCESS), Data: p}) } @@ -117,8 +118,8 @@ func MyAppList(c *gin.Context) { position, _ := strconv.ParseBool(c.DefaultQuery("position", "true")) list, unTranslation := service.MyService.App().GetMyList(index, size, position) data := make(map[string]interface{}, 2) - data["list"] = list - data["local"] = unTranslation + data["list"] = list // @tiger - list 不清楚是什么意思，可以提高一下描述性 + data["local"] = unTranslation // @tiger - local 不清楚是什么意思，可以提高一下描述性 c.JSON(http.StatusOK, &model.Result{Success: common_err.SUCCESS, Message: common_err.GetMsg(common_err.SUCCESS), Data: data}) } @@ -131,6 +132,7 @@ func MyAppList(c *gin.Context) { // @Success 200 {string} string "ok" // @Router /app/usage [get] func AppUsageList(c *gin.Context) { + // @tiger - 关于出参的问题，见 GetHardwareUsageSteam（） - 另外 steam 是不是应该为 stream? list := service.MyService.App().GetHardwareUsage() c.JSON(http.StatusOK, &model.Result{Success: common_err.SUCCESS, Message: common_err.GetMsg(common_err.SUCCESS), Data: list}) } diff --git a/route/v1/docker.go b/route/v1/docker.go index 463aed8..88549d8 100644 --- a/route/v1/docker.go +++ b/route/v1/docker.go @@ -708,7 +708,7 @@ func UnInstallApp(c *gin.Context) { // @Router /app/state/{id} [put] func ChangAppState(c *gin.Context) { appId := c.Param("id") - state := c.DefaultPostForm("state", "stop") + state := c.DefaultPostForm("state", "stop") // @tiger - 应该用 JSON 形式 var err error if state == "stop" { err = service.MyService.Docker().DockerContainerStop(appId) @@ -727,6 +727,8 @@ func ChangAppState(c *gin.Context) { c.JSON(http.StatusOK, model.Result{Success: common_err.ERROR, Message: common_err.GetMsg(common_err.ERROR), Data: err.Error()}) return } + + // @tiger - 用 {'state': ...} 来体现出参上下文 c.JSON(http.StatusOK, model.Result{Success: common_err.SUCCESS, Message: common_err.GetMsg(common_err.SUCCESS), Data: info.State}) } @@ -1006,6 +1008,8 @@ func PutAppUpdate(c *gin.Context) { // @Router /app/info/{id} [get] func ContainerInfo(c *gin.Context) { appId := c.Param("id") + + // @tiger - 作为最佳实践，不应该直接把数据库的信息返回，来避免未来数据库结构上的迭代带来的新字段 appInfo := service.MyService.App().GetAppDBInfo(appId) containerInfo, _ := service.MyService.Docker().DockerContainerStats(appId) var cpuModel = "arm" @@ -1027,13 +1031,13 @@ func ContainerInfo(c *gin.Context) { Status string `json:"status"` StartedAt string `json:"started_at"` CPUShares int64 `json:"cpu_shares"` - Memory int64 `json:"memory"` - Restart string `json:"restart"` + Memory int64 `json:"memory"` // @tiger - 改成 total_memory，方便以后增加 free_memory 之类的字段 + Restart string `json:"restart"` // @tiger - 改成 restart_policy? }{Status: info.State.Status, StartedAt: info.State.StartedAt, CPUShares: info.HostConfig.CPUShares, Memory: info.HostConfig.Memory >> 20, Restart: info.HostConfig.RestartPolicy.Name} data := make(map[string]interface{}, 5) - data["app"] = appInfo - data["cpu"] = cpuModel - data["memory"] = service.MyService.System().GetMemInfo()["total"] + data["app"] = appInfo // @tiget - 最佳实践是，返回 appid，然后具体的 app 信息由前端另行获取 + data["cpu"] = cpuModel // @tiger - 改成 arch + data["memory"] = service.MyService.System().GetMemInfo()["total"] // @tiger - 改成 total_memory，方便以后增加 free_memory 之类的字段 data["container"] = json2.RawMessage(containerInfo) data["info"] = con c.JSON(http.StatusOK, model.Result{Success: common_err.SUCCESS, Message: common_err.GetMsg(common_err.SUCCESS), Data: data}) diff --git a/route/v1/system.go b/route/v1/system.go index 4b57278..0f43fa8 100644 --- a/route/v1/system.go +++ b/route/v1/system.go @@ -470,7 +470,7 @@ func GetSystemSocketPort(c *gin.Context) { model.Result{ Success: common_err.SUCCESS, Message: common_err.GetMsg(common_err.SUCCESS), - Data: config.ServerInfo.SocketPort, + Data: config.ServerInfo.SocketPort, // @tiger 这里最好封装成 {'port': ...} 的形式，来体现出参的上下文 }) } diff --git a/route/v1/user.go b/route/v1/user.go index bb46416..e664c38 100644 --- a/route/v1/user.go +++ b/route/v1/user.go @@ -31,8 +31,11 @@ import ( func PostUserRegister(c *gin.Context) { json := make(map[string]string) c.BindJSON(&json) + + // @tiger - user_name 改成 username username := json["user_name"] pwd := json["password"] + key := c.Param("key") if _, ok := service.UserRegisterHash[key]; !ok { c.JSON(http.StatusOK, @@ -86,6 +89,8 @@ func PostUserLogin(c *gin.Context) { c.BindJSON(&json) username := json["username"] + + // @tiger - 字段命名要一直，在注册的时候如果用 password，这里也要用 password pwd := json["pwd"] //check params is empty if len(username) == 0 || len(pwd) == 0 { @@ -114,6 +119,8 @@ func PostUserLogin(c *gin.Context) { data := make(map[string]interface{}, 2) user.Password = "" data["token"] = token + + // @tiger - 不建议直接透传数据库对象，而是适配到用于 API 输出的 model 对象 data["user"] = user c.JSON(http.StatusOK, @@ -175,6 +182,8 @@ func GetUserAvatar(c *gin.Context) { if user.Id > 0 { path = user.Avatar } + + // @tiger - RESTful 规范下不应该返回文件本身内容，而是返回文件的静态URL，由前端去解析 c.File(path) } @@ -311,13 +320,14 @@ func GetUserInfo(c *gin.Context) { user := service.MyService.User().GetUserInfoById(id) //***** + // @tiger - 应该和 PostUserLogin 中的 user 对象一致。而不是重构一系列字段。 var u = make(map[string]string, 5) - u["user_name"] = user.UserName - u["head"] = user.Avatar + u["user_name"] = user.UserName // 改成 username + u["head"] = user.Avatar // 应该和 /v1/user/avatar/:id 一致，改成 avatar u["email"] = user.Email u["description"] = user.NickName - u["nick_name"] = user.NickName - u["id"] = strconv.Itoa(user.Id) + u["nick_name"] = user.NickName // 改成 nickname + u["id"] = strconv.Itoa(user.Id) // (nice-to-have) 最佳实践是用随机字符来代表 ID。顺序数字有可预测性 //** @@ -337,6 +347,9 @@ func GetUserInfo(c *gin.Context) { // @Router /user/info [get] func GetUserInfoByUserName(c *gin.Context) { json := make(map[string]string) + + // @tiger 当前这个设计的问题是：GET 不应该同时接收 request body。 + // GET 方法应该只接收 URL 参数 c.BindJSON(&json) userName := json["user_name"] if len(userName) == 0 { @@ -359,7 +372,7 @@ func GetUserInfoByUserName(c *gin.Context) { } /** - * @description: get all user name + * @description: get all usernames * @method:GET * @router:/user/all/name */ @@ -399,6 +412,7 @@ func GetUserCustomConf(c *gin.Context) { return } filePath := config.AppInfo.UserDataPath + "/" + id + "/" + name + ".json" + data := file.ReadFullFile(filePath) if !gjson.ValidBytes(data) { c.JSON(http.StatusOK, model.Result{Success: common_err.SUCCESS, Message: common_err.GetMsg(common_err.SUCCESS), Data: string(data)}) @@ -453,7 +467,7 @@ func DeleteUserCustomConf(c *gin.Context) { return } filePath := config.AppInfo.UserDataPath + "/" + strconv.Itoa(user.Id) + "/" + name + ".json" - os.Remove(filePath) + os.Remove(filePath) // @tiger - 这里万一无法实际删除，后面仍然有可能返回成功 c.JSON(http.StatusOK, model.Result{Success: common_err.SUCCESS, Message: common_err.GetMsg(common_err.SUCCESS)}) } @@ -583,6 +597,8 @@ func GetUserImage(c *gin.Context) { defer fileTmp.Close() fileName := path.Base(filePath) + + // @tiger - RESTful 规范下不应该返回文件本身内容，而是返回文件的静态URL，由前端去解析 c.Header("Content-Disposition", "attachment; filename*=utf-8''"+url2.PathEscape(fileName)) c.File(filePath) } diff --git a/service/app.go b/service/app.go index 7e1e96a..8da1063 100644 --- a/service/app.go +++ b/service/app.go @@ -438,6 +438,8 @@ func (a *appStruct) GetHardwareUsageSteam() { dockerStats.Icon = v.Labels["icon"] dockerStats.Title = strings.ReplaceAll(v.Names[0], "/", "") + // @tiger - 不建议直接把依赖的数据结构封装返回。 + // 如果依赖的数据结构有变化，应该在这里适配或者保存，这样更加对客户端负责 temp.Store(v.ID, dockerStats) if i == 99 { stats.Body.Close() diff --git a/service/model/o_container.go b/service/model/o_container.go index 5d4de3a..4c2f6ac 100644 --- a/service/model/o_container.go +++ b/service/model/o_container.go @@ -67,13 +67,13 @@ type MyAppList struct { Index string `json:"index"` //Order string `json:"order"` Port string `json:"port"` - UpTime string `json:"up_time"` + UpTime string `json:"up_time"` // @tiger - 如果是安装时间，应该写 installed_at。 Slogan string `json:"slogan"` Type string `json:"type"` //Rely model.MapStrings `json:"rely"` //[{"mysql":"id"},{"mysql":"id"}] Image string `json:"image"` Volumes string `json:"volumes"` - NewVersion bool `json:"new_version"` + NewVersion bool `json:"new_version"` // @tiger - 无法从词面理解含义，感觉可以更加有描述性 Host string `json:"host"` Protocol string `json:"protocol"` } diff --git a/service/model/o_user.go b/service/model/o_user.go index d66895d..0e1bfd2 100644 --- a/service/model/o_user.go +++ b/service/model/o_user.go @@ -15,11 +15,11 @@ import "time" //Soon to be removed type UserDBModel struct { Id int `gorm:"column:id;primary_key" json:"id"` - UserName string `json:"user_name"` + UserName string `json:"user_name"` // @tiger - user_name 改 username Password string `json:"password,omitempty"` Role string `json:"role"` Email string `json:"email"` - NickName string `json:"nick_name"` + NickName string `json:"nick_name"` // @tiger - nick_name 改 nickname Avatar string `json:"avatar"` Description string `json:"description"` CreatedAt time.Time `gorm:"<-:create;autoCreateTime" json:"created_at,omitempty"` diff --git a/service/rely.go b/service/rely.go index 9c6bd3e..26037fe 100644 --- a/service/rely.go +++ b/service/rely.go @@ -35,6 +35,8 @@ func (r *relyService) Create(rely model2.RelyDBModel) { func (r *relyService) GetInfo(id string) model2.RelyDBModel { var m model2.RelyDBModel r.db.Where("custom_id = ?", id).First(&m) + + // @tiger - 作为出参不应该直接返回数据库内的格式（见类似问题的注释） return m }