feat(api): Describe the API from the code and serve the description - #207
feat(api): Describe the API from the code and serve the description#207nfebe wants to merge 5 commits into
Conversation
Anything talking to the agent had to learn what an endpoint accepts by reading the agent's source, so a client could only guess at field names and find out it was wrong from a 400. The CLI's generated commands take arbitrary key=value fields for exactly this reason. The agent now describes itself: routes, path and query parameters, the body each handler binds, the permission each is gated on, and the response where a handler returns a type rather than an inline map. It is read out of the code, so it cannot claim something the code does not do, and a test fails the build if it drifts from the routes. The description is served, so a client asks the instance it is connected to rather than assuming whatever was true when the client was built. Deployments, backups and certificates now answer with declared types, and the fields worth showing as a table are named on those types. A client can lay out results without being taught each endpoint by hand. The rest still answer with maps and remain undescribed until they are converted.
Code Review SummaryThis PR introduces an automated OpenAPI specification generator ( 🚀 Key Improvements
💡 Minor Suggestions
🚨 Critical Issues
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
A generated description can be current and still be unusable: a reference pointing at nothing, an operation without an identifier, a path a client cannot reach. Each of those parses as JSON and breaks whoever trusts it.
|
|
||
| // queryParams are the query keys a handler reads, which is what makes them checkable by a caller | ||
| // rather than something to be discovered by trial. | ||
| func queryParams(api *packages.Package, fn *ast.FuncDecl) []string { |
There was a problem hiding this comment.
The current query parameter extraction logic misses several common Gin methods like GetQuery. Including GetQuery would improve the completeness of the generated specification.
| func queryParams(api *packages.Package, fn *ast.FuncDecl) []string { | |
| func queryParams(api *packages.Package, fn *ast.FuncDecl) []string { | |
| seen := map[string]bool{} | |
| var names []string | |
| ast.Inspect(fn, func(n ast.Node) bool { | |
| call, ok := n.(*ast.CallExpr) | |
| if !ok || len(call.Args) == 0 { | |
| return true | |
| } | |
| sel, ok := call.Fun.(*ast.SelectorExpr) | |
| if !ok { | |
| return true | |
| } | |
| name := sel.Sel.Name | |
| if name != "Query" && name != "DefaultQuery" && name != "GetQuery" { | |
| return true | |
| } | |
| lit, ok := call.Args[0].(*ast.BasicLit) | |
| if !ok { | |
| return true | |
| } | |
| paramName, err := strconv.Unquote(lit.Value) | |
| if err != nil || seen[paramName] { | |
| return true | |
| } | |
| seen[paramName] = true | |
| names = append(names, paramName) | |
| return true | |
| }) | |
| sort.Strings(names) | |
| return names | |
| } |
A response type named after its resource tells a client nothing it did not already know, so each one had to be learned separately. A collection is a collection whatever it holds: there is now one shape for a list, one for a single thing, and one for a report of what happened, and the generated description says which an endpoint answers in. A client renders any collection the same way, and an endpoint converted tomorrow renders without a line of client code. The fields worth showing as columns are every scalar the row declares, in the order it declares them, rather than a list repeated on each type; a field that is never worth a column says so once. Collections keep answering under their old name alongside the new one, so nothing reading the current shape breaks while it moves.
87ded86 to
c6350d4
Compare
| // where every collection ends up. | ||
| func NewList[T any](items []T, legacy string) List[T] { |
There was a problem hiding this comment.
The current NewList implementation hardcodes Total to len(items). This is semantically incorrect for paginated endpoints where items is just one page of a larger collection. In such cases, the client needs the actual total count to handle pagination UI (e.g., 'Showing 1-10 of 100'). The List struct and NewList constructor should allow providing an explicit total.
| // where every collection ends up. | |
| func NewList[T any](items []T, legacy string) List[T] { | |
| func NewList[T any](items []T, legacy string) List[T] { | |
| return NewListWithTotal(items, len(items), legacy) | |
| } | |
| // NewListWithTotal creates a list response with an explicit total count for pagination. | |
| func NewListWithTotal[T any](items []T, total int, legacy string) List[T] { | |
| if items == nil { | |
| items = []T{} | |
| } | |
| return List[T]{Items: items, Total: total, legacy: legacy} | |
| } |
| "deployments": deployments, | ||
| "path": s.manager.BasePath(), | ||
| }) | ||
| c.JSON(http.StatusOK, NewList(deployments, "deployments")) |
There was a problem hiding this comment.
This change removes the path field (returned by s.manager.BasePath()) from the response. This is a breaking change for clients relying on knowing the agent's base deployment directory. Since the generic List shape cannot carry extra top-level fields, use an anonymous struct to preserve compatibility while still providing the standard list fields for the generator.
| c.JSON(http.StatusOK, NewList(deployments, "deployments")) | |
| c.JSON(http.StatusOK, struct { | |
| List[models.Deployment] | |
| Path string `json:"path"` | |
| }{ | |
| List: NewList(deployments, "deployments"), | |
| Path: s.manager.BasePath(), | |
| }) |
| if !ok { | ||
| return false | ||
| } | ||
| return named.Obj().Name() == "H" && strings.HasSuffix(named.Obj().Pkg().Path(), "gin") |
There was a problem hiding this comment.
Potential nil pointer dereference: named.Obj().Pkg() can be nil for types in the universe scope (like error). Always check for nil before calling .Path().
| return named.Obj().Name() == "H" && strings.HasSuffix(named.Obj().Pkg().Path(), "gin") | |
| pkg := named.Obj().Pkg() | |
| return named.Obj().Name() == "H" && pkg != nil && strings.HasSuffix(pkg.Path(), "gin") |
| return found | ||
| } | ||
|
|
||
| func isStatusOK(expr ast.Expr) bool { |
There was a problem hiding this comment.
The generator currently only recognizes types returned with http.StatusOK. Handlers using 201 Created or 202 Accepted (like createBackup) will have their response bodies omitted from the spec. Rename and expand this check to cover other success status codes.
| func isStatusOK(expr ast.Expr) bool { | |
| func isSuccessStatus(expr ast.Expr) bool { | |
| sel, ok := expr.(*ast.SelectorExpr) | |
| if !ok { | |
| return false | |
| } | |
| return strings.HasPrefix(sel.Sel.Name, "Status") && (strings.Contains(sel.Sel.Name, "OK") || strings.Contains(sel.Sel.Name, "Created") || strings.Contains(sel.Sel.Name, "Accepted")) | |
| } |
| "deployments": deployments, | ||
| "path": s.manager.BasePath(), | ||
| }) | ||
| c.JSON(http.StatusOK, NewList(deployments, "deployments")) |
There was a problem hiding this comment.
Standardizing the response with NewList has removed the path field (s.manager.BasePath()) that was previously returned by GET /api/deployments. If any CLI or UI clients rely on this field to locate the deployment directory on the host, this change will break them. Consider if this metadata should be moved to a separate field or if the List type needs to support custom metadata.
| c.JSON(http.StatusOK, NewList(deployments, "deployments")) | |
| c.JSON(http.StatusOK, struct { | |
| List[models.Deployment] | |
| Path string `json:"path"` | |
| }{ | |
| List: NewList(deployments, "deployments"), | |
| Path: s.manager.BasePath(), | |
| }) |
| return found | ||
| } | ||
|
|
||
| func isStatusOK(expr ast.Expr) bool { |
There was a problem hiding this comment.
The current check only captures http.StatusOK (200). Many write operations in this API use http.StatusAccepted (202) or http.StatusCreated (201) (e.g., createBackup). Restricting this to StatusOK prevents the generator from extracting and documenting the response body schemas for those endpoints. Expanding this to cover standard success codes would improve documentation coverage.
| func isStatusOK(expr ast.Expr) bool { | |
| func isSuccessStatus(api *packages.Package, expr ast.Expr) bool { | |
| sel, ok := expr.(*ast.SelectorExpr) | |
| if !ok { | |
| return false | |
| } | |
| x, ok := sel.X.(*ast.Ident) | |
| if !ok { | |
| return false | |
| } | |
| // Verify it is the net/http package | |
| if obj := api.TypesInfo.ObjectOf(x); obj == nil || obj.Pkg() == nil || obj.Pkg().Path() != "net/http" { | |
| return false | |
| } | |
| switch sel.Sel.Name { | |
| case "StatusOK", "StatusCreated", "StatusAccepted", "StatusNoContent": | |
| return true | |
| } | |
| return false | |
| } |
| } | ||
|
|
||
| // groupPrefix is what each router group prepends to the paths registered on it. | ||
| var groupPrefix = map[string]string{ |
There was a problem hiding this comment.
The groupPrefix map is a manual lookup that must be updated whenever a new Gin router group is introduced. This creates a risk of silent failures where new endpoints are omitted from the documentation. Since the tool already loads the API package syntax, it would be more robust to dynamically discover these groups and their associated prefixes by tracing Group() calls in the AST.
| var groupPrefix = map[string]string{ | |
| // Consider replacing this static map with dynamic discovery by scanning the AST for Group() calls | |
| // and identifying the variable names assigned to each prefix. |
| if m := permPattern.FindStringSubmatch(rest); m != nil { | ||
| r.Permission = m[1] | ||
| } | ||
| if m := handlerPattern.FindStringSubmatch(strings.TrimSpace(rest)); m != nil { |
There was a problem hiding this comment.
The current regex-based extraction of handlers will fail if the route registration line includes a trailing comment (e.g., api.GET("/path", s.handler) // comment), because the $ anchor in handlerPattern prevents matching anything after the closing parenthesis. Stripping comments before matching ensures handlers are correctly identified.
| if m := handlerPattern.FindStringSubmatch(strings.TrimSpace(rest)); m != nil { | |
| if i := strings.Index(line, "//"); i != -1 { line = line[:i] } | |
| if m := handlerPattern.FindStringSubmatch(strings.TrimSpace(rest)); m != nil { |
| if len(call.Args) != 1 { | ||
| return true | ||
| } | ||
| unary, ok := call.Args[0].(*ast.UnaryExpr) |
There was a problem hiding this comment.
Restricting the extraction to *ast.UnaryExpr (like &req) misses cases where a pointer variable is passed directly to the binding method (e.g., req := &MyRequest{}; c.ShouldBindJSON(req)). Evaluating the type of the argument expression directly is more robust as the schema generator already handles pointer resolution.
| unary, ok := call.Args[0].(*ast.UnaryExpr) | |
| if t := api.TypesInfo.TypeOf(call.Args[0]); t != nil { | |
| found = t | |
| } |
| return true | ||
| } | ||
| sel, ok := call.Fun.(*ast.SelectorExpr) | ||
| if !ok || sel.Sel.Name != "JSON" { |
There was a problem hiding this comment.
The generator currently only picks up standard JSON calls. Supporting other Gin response methods like AbortWithStatusJSON, PureJSON, or IndentedJSON will improve documentation completeness for endpoints that use these variations.
| if !ok || sel.Sel.Name != "JSON" { | |
| if !ok || (sel.Sel.Name != "JSON" && sel.Sel.Name != "AbortWithStatusJSON" && sel.Sel.Name != "PureJSON" && sel.Sel.Name != "IndentedJSON") { |
Two endpoints' permissions were being invented from the name of a constant rather than read from it, so the ones with an acronym came out mangled and no client could match them against a key. They are now read from the value the agent itself checks. Nine paths kept the router's wildcard notation, which OpenAPI has no notion of, so they named no path a client could build and left the segment carrying the filename undescribed.
| pkg *packages.Package | ||
| } | ||
|
|
||
| func indexHandlers(pkgs []*packages.Package) map[string]*handler { |
There was a problem hiding this comment.
Indexing handlers using only their method names in a flat map causes collisions when different structs (e.g., Server and ClusterManager) define methods with the same name (like List or Delete). Since readRoutes currently only extracts the method name, one documentation schema will overwrite the other in the spec. Handlers should be indexed by a fully qualified key (ReceiverType + MethodName).
| func indexHandlers(pkgs []*packages.Package) map[string]*handler { | |
| func indexHandlers(pkgs []*packages.Package) map[string]*handler { | |
| handlers := map[string]*handler{} | |
| for _, pkg := range pkgs { | |
| for _, file := range pkg.Syntax { | |
| for _, decl := range file.Decls { | |
| fn, ok := decl.(*ast.FuncDecl) | |
| if !ok || fn.Recv == nil || len(fn.Recv.List) == 0 { | |
| continue | |
| } | |
| t := pkg.TypesInfo.TypeOf(fn.Recv.List[0].Type) | |
| if t == nil { | |
| continue | |
| } | |
| // Index by ReceiverType.MethodName to avoid collisions | |
| key := strings.TrimPrefix(t.String(), "*") + "." + fn.Name.Name | |
| handlers[key] = &handler{decl: fn, pkg: pkg} | |
| } | |
| } | |
| } | |
| return handlers | |
| } |
1 participant
Anything talking to the agent had to read the agent's source to learn what an endpoint accepts. The
CLI's generated commands take arbitrary
key=valuefields for exactly that reason, and a typo comesback as a 400 naming nothing.
The agent now describes itself in OpenAPI, read out of the code by
tools/genspecrather thanannotated, so it cannot claim something the code does not do:
Served at
GET /api/openapi.json, so a client asks the instance it is connected to instead ofassuming whatever was true when the client was built.
What it carries:
gin.Hagainst 33 typedDeployments, backups and certificates are converted here and name their table columns on the type,
which is what lets a client lay out results without being taught each endpoint. The rest stay
undescribed until their handlers return types too.
Three tests guard it: the description is regenerated and compared, so it fails the build when it
drifts from the routes; every reference must resolve and every operation must be identified; and the
fields a handler requires must arrive as required.